Add "ignored paths" for the proxy #38

vbehar · 2019-10-04T09:15:03Z

Expose a new proxy config for a list of "ignored paths". Requests to such paths won't be counted by the proxy.

Our use-case is the /metrics path used to expose prometheus metrics, and which is keeping our pods up all the time.

This is configured by adding a pod's annotation osiris.deislabs.io/ignoredPaths, as a comma-separated string value:

apiVersion: apps/v1
kind: Deployment
spec:
  template:
    metadata:
      annotations:
        osiris.deislabs.io/ignoredPaths: "/first/path,/second-path"
    spec:
      containers:
        ...

msftclas · 2019-10-04T09:15:15Z

All CLA requirements met.

Expose a new proxy config for a list of "ignored paths". Requests to such paths won't be counted by the proxy. Our use-case is the `/metrics` path used to expose prometheus metrics, and which is keeping our pods up all the time.

codecov-io · 2019-10-04T09:22:44Z

Codecov Report

Merging #38 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master      #38   +/-   ##
=======================================
  Coverage   59.09%   59.09%           
=======================================
  Files          11       11           
  Lines         638      638           
=======================================
  Hits          377      377           
  Misses        232      232           
  Partials       29       29

Impacted Files	Coverage Δ
pkg/kubernetes/osiris.go	`100% <ø> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f4df375...985d824. Read the comment docs.

krancour · 2019-10-07T14:30:41Z

@vbehar thanks for this contribution (and your others as well)!

In this case, I am a little reluctant to feel that ignored paths should (always / only) be globally configured. This would make all proxies for all Osiris-enabled pods ignore the configured paths.

For your use case, if your use of Prometheus is global and consistent, this may make sense, but my sense is that cases where specific paths needs to be ignored for specific deployments' pods wouldn't be out of the ordinary either.

What would you think about leaving this globally configurable (as it is now), but adding the ability to amend it (not override it) with an annotation?

The reason I suggest amending rather than overriding is that this allows the operator to configure paths that should be globally ignored based on their knowledge of the environment. (i.e. It is the operator who is more likely to be aware of Prometheus being in use within the cluster than the developer.) And then it allows developers to add to the ignored paths based on their understanding of the application.

lmk what you think.

vbehar · 2019-10-07T14:41:12Z

hi, yes, it sounds good to me. I can add a new annotation on the deployment, such as osiris.deislabs.io/ignoredPaths.

krancour · 2019-10-07T14:53:53Z

Thanks @vbehar! Appreciate the contribution!

the deployment annotation `osiris.deislabs.io/ignoredPaths` can be used to append custom paths to the list of "ignored paths" - that won't be counted in the metrics. this allow to configure a platform-wide list of ignoredPaths in osiris configuration (helm chart for example), and to add more paths on a per-deployment basis. same as for the global configuration, the list of paths should be written as a comma-separated string.

krancour · 2019-10-15T16:35:30Z

example/hello-osiris.yaml

  annotations:
    osiris.deislabs.io/enabled: "true"
    osiris.deislabs.io/minReplicas: "1"
+    osiris.deislabs.io/ignoredPaths: "/first/path,/second-path"


Just checking... does k8s actually allow a comma here?

yes, and some people/tools even store JSON in annotations

I could have sworn there were some limitations on characters used in annotation values, but double-checking here myself, I only see limitations mentioned on the characters used in the keys. 👍

krancour · 2019-10-15T16:37:14Z

README.md


+### Features
+
+#### Counting requests at the proxy level


I'm not sure if this feature is sufficiently important to warrant its own section, especially given that relevant config options are already covered in the tables above.

Could we consider dropping this section?

ok, I removed it.

krancour · 2019-10-18T13:22:15Z

pkg/metrics/proxy/injector/deployment_patch.go

+				Value: strings.Join(ignoredPaths, ","),
+			})
+		}
+


I'm having a hard time understanding what this bit of patching accomplishes. I might be missing something, but it looks as if it's patching the deployment's ignored paths annotation with values retrieved from the deployment's ignored paths annotation. Isn't this like saying let x = x?

Nevermind. I see it now. You're copying the annotation from the deployment to the the pod spec template within the deployment.

in fact it's reading from the deployment's annotations, and writing to the pod's annotations. because later to inject the proxy, we will only have access to the pod's annotations, not the deployment's annotations

krancour · 2019-10-18T13:26:43Z

pkg/metrics/proxy/injector/pod_patch.go

+		ignoredPaths,
+		kubernetes.GetIgnoredPaths(pod.Annotations)...,
+	)
+


I'm perplexed about how this is meant to work. It looks like this is meant to find the values later used in setting the IGNORED_PATHS env var, but it's retrieving some of that from the pod's ignored paths annotation... and I can't see how that annotation ever got set.

Nevermind. I see how this gets set now.

krancour · 2019-10-18T13:28:45Z

Added a couple comments about specific pieces of code. I think what might be helpful here is if you can just drop a response to this comment with a general overview of the flow through this so I can understand it high-level.

vbehar · 2019-10-18T13:40:14Z

hi, so the flow is:

user write a deployment's annotation
when osiris sees a new or updated deployment, it will read the annotation, and copy it in the pod template's annotations, so that each pod has this annotation
when osiris sees a pod being created, it will inject the proxy, and read the pod's annotation to get the custom proxy config

I'm doing that for 2 reasons:

it's the pattern that was already in place in the code (copying annotation from the deployment to the pod template)
this allows to have all osiris config as deployment annotations - instead of requiring the users to write part of the config as deployment annotations, and part of the config as pod template annotations

krancour · 2019-10-18T13:46:33Z

it's the pattern that was already in place in the code (copying annotation from the deployment to the pod template)

Yes. I see that now. The steps here could be consolidated into just one. The admissions controller (injector) could take info from the annotation on the deployment and directly set the env var in the deployment's pod spec template. i.e. We could cut out the step of copying the annotation from the deployment down to the deployment's pod spec template.

All that said... there was a reason I used the pattern you are emulating. Except now I can't remember what it was. (D'oh! Comment fail!)

I totally get what you're doing now. Thanks for taking the time to explain. Give me a little bit to see if I can figure out why I did it the other way to begin with and whether it's necessary to do it the same way here.

this allows to have all osiris config as deployment annotations - instead of requiring the users to write part of the config as deployment annotations, and part of the config as pod template annotations

Yes. Totally, completely agree with this.

krancour · 2019-10-18T14:48:22Z

@vbehar ok... I remember the reason for that pattern now.

If you kubectl edit deployment <some osiris-enabled deployment> (which is probably something you shouldn't do in production, but certainly happens frequently often enough in all kinds of other environments-- especially development environments), the mutating web hook (the injector) doesn't get involved, so I didn't want to rely on patching the deployment as a method of doing any heavy lifting.

Pods were a different story. There's a very, very limited set of modifications that Kubernetes even permits to a running pod. kubectl edit pod <some osiris-enabled pod> was a less likely scenario and I felt comfortable assuming that more often than not, people might edit the deployment and if the changes to the deployment affected the pod spec template, k8s would kill off existing pods and replace them with brand new ones and the mutating web hook would get involved in patching the brand new pods. This made patching the pods a safer method for most of the heavy lifting because it was more likely to run exactly when you think it should.

Make sense?

So... there's a gotcha lurking in there still...

If you edit an Osiris-enabled deployment to remove the annotation that makes it Osiris-enabled, the mutating webhook doesn't get involved (as I mentioned) and therefore the annotation never gets stripped from the deployment's pod spec template. (In fact, this is why there is no code in there for removing the annotation. It's just a thing that can't really happen.) With no change to the pod spec template, existing pods never get replaced. i.e. Stripping that annotation from the deployment does not disabled Osiris on existing pods.

The same problem is applicable to your scenario as well. If you edit the ignored paths annotation on a deployment, the bit of code that copies that down to the annotations in the pod spec template never gets triggered. The pod spec remains unchanged. Existing pods remain. You never get new ones with the right annotation that will subsequently be patched to have the right env var.

Now... all this being said... everything is just fine if people simple never kubectl edit <deployment>. Everything I did and everything you're trying to add would work just fine.

I'm thinking there are two possible paths forward.

Make docs clearly state that changes affecting Osiris applied via kubectl edit <deployment> won't work as expected. People will overlook this.
We rethink the notion that users shouldn't have to put any annotations on the deployment's pod spec template themselves. That would feel inconvenient, perhaps, but would be a reliable fix. We wouldn't need to patch deployments at all. And any change a user makes to the deployment, no matter how they make it, will work exactly as expected. On the flip side, it makes Osiris a tiny bit harder to use.

I am leaning towards option 2 because I think reliability trumps ease of use and the inconvenience is minor. It's a breaking change, but Osiris hasn't even had a single release yet, nevermind a GA release.

Option 2 simplifies things a bit for your new code as well as my existing code.

Now... all that said... (sorry... I know this response got really long)...

I think you still have a problem lurking in here. You're combining global/Osiris-system-level configuration with deployment/pod-level configuration. If the former changes, there is no process here that rebuilds the full set of ignored paths properly on existing pods. We might just have to remove the global option, unfortunately.

vbehar · 2019-10-18T15:01:01Z

ok, we can live with no global config for ignored paths, and just set the annotation on every deployment. I'll rework this PR next week to remove all the global config part

krancour · 2019-10-18T15:14:46Z

I'll rework this PR next week to remove all the global config part

Great. Start there. In the meantime, I will think about how to address the unexpected behavior when kubectl edit deployment ... inevitably happens.

we'll only configure "ignored paths" on a per-deployment basis to avoid refreshing issues

instead of on the deployment. this makes the code simpler, and safer because we don't have to ensure every change of the deployment needs to be reflected on the pod

vbehar · 2019-10-21T07:38:38Z

@krancour ok I removed a lot of code on this one ;-) so no more global config, and the annotation should now be written directly on the pod.

krancour · 2019-10-22T13:16:44Z

@vbehar thanks for this. This is looking really good. I am going to see about straightening out how we handle the existing annotation that gets copied from the deployment to the pod spec template. This might need a little rebase after that, but it won't be too bad and I think we have this almost where we'd want it. Thanks again, not only for the contribution, but also for your patience.

vbehar · 2019-10-24T05:50:39Z

@krancour ok, I rebased it on master after #47

krancour · 2019-10-24T17:06:49Z

@vbehar thanks for all the hard work on this. I am very happy with where it ended up.

backport deislabs#38 Expose a new proxy config for a list of "ignored paths". Requests to such paths won't be counted by the proxy. Our use-case is the `/metrics` path used to expose prometheus metrics, and which is keeping our pods up all the time. This is configured by adding a pod's annotation `osiris.dm.gg/ignoredPaths`, as a comma-separated string value: ``` apiVersion: apps/v1 kind: Deployment spec: template: metadata: annotations: osiris.dm.gg/ignoredPaths: "/first/path,/second-path" spec: containers: ... ```

vbehar requested review from jeremyrickard, krancour and yaron2 as code owners October 4, 2019 09:15

Add "ignored paths" for the proxy

0eba7a0

Expose a new proxy config for a list of "ignored paths". Requests to such paths won't be counted by the proxy. Our use-case is the `/metrics` path used to expose prometheus metrics, and which is keeping our pods up all the time.

vbehar force-pushed the ignored-paths branch from dd23e55 to 0eba7a0 Compare October 4, 2019 09:21

Merge branch 'master' into ignored-paths

9a30818

vbehar mentioned this pull request Oct 8, 2019

Document global config and supported annotations #42

Merged

krancour and others added 4 commits October 11, 2019 08:27

Merge branch 'master' into ignored-paths

ebd5733

Merge branch 'master' into ignored-paths

bec8898

add documentation

dcc59f4

Merge branch 'master' into ignored-paths

d070b1b

krancour reviewed Oct 15, 2019

View reviewed changes

remove feature doc

d96e06c

krancour reviewed Oct 18, 2019

View reviewed changes

vbehar added 2 commits October 21, 2019 09:13

Merge branch 'master' into ignored-paths

e576032

remove "ignored paths" global config

561b19e

we'll only configure "ignored paths" on a per-deployment basis to avoid refreshing issues

vbehar added 2 commits October 21, 2019 09:30

the ignoredPaths annotation should now be set on the pod itself

8c6abff

instead of on the deployment. this makes the code simpler, and safer because we don't have to ensure every change of the deployment needs to be reflected on the pod

fix merge mistake

985d824

krancour mentioned this pull request Oct 22, 2019

don't patch deployments anymore #47

Merged

Merge branch 'master' into ignored-paths

06f5977

krancour self-assigned this Oct 24, 2019

krancour self-requested a review October 24, 2019 17:07

krancour approved these changes Oct 24, 2019

View reviewed changes

krancour merged commit 3f71d07 into deislabs:master Oct 24, 2019

vbehar mentioned this pull request Sep 14, 2020

Add "ignored paths" for the proxy dailymotion-oss/osiris#11

Merged

Add "ignored paths" for the proxy #38

Add "ignored paths" for the proxy #38

Uh oh!

Conversation

vbehar commented Oct 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msftclas commented Oct 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Oct 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

krancour commented Oct 7, 2019

Uh oh!

vbehar commented Oct 7, 2019

Uh oh!

krancour commented Oct 7, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krancour commented Oct 18, 2019

Uh oh!

vbehar commented Oct 18, 2019

Uh oh!

krancour commented Oct 18, 2019

Uh oh!

krancour commented Oct 18, 2019

Uh oh!

vbehar commented Oct 18, 2019

Uh oh!

krancour commented Oct 18, 2019

Uh oh!

vbehar commented Oct 21, 2019

Uh oh!

krancour commented Oct 22, 2019

Uh oh!

vbehar commented Oct 24, 2019

Uh oh!

krancour commented Oct 24, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vbehar commented Oct 4, 2019 •

edited

Loading

msftclas commented Oct 4, 2019 •

edited

Loading

codecov-io commented Oct 4, 2019 •

edited

Loading