-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support Auto-Inject Config Overrides Via Annotations #2471
Conversation
5193e8a
to
777f2ce
Compare
Looking through the test cases, I'm having a bit of an existential crisis. The annotations when you're just overriding a single thing seems okay. Seeing the wall of annotations for overrides gets me worried after experiencing the crazy set of annotations most ingress controllers try to support. So, is there a better way to do this that has some more structure? As this ends up on the injection side, the only options I can come up with are:
I went looking at kustomize again, but it doesn't look like the right solution to this either. Any other ideas? I'm not sure there are many other possibilities. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is all working great for me locally, and thanks for all the test cases. i left one nit and one comment around proxyConfigOverrides
, let me know what you think.
6fc2128
to
7aa9c61
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all looks great to me 🌟
I was just wondering about the usage of strconv.FormatUint
vs simply strconv.Itoa
in getOverrideOrDefault
@grampelberg About that wall of annotations, I believe in the majority of cases most won't be included. I can only think of people wanting to override |
It's just a personal nit. Underneath the hood, |
04b228b
to
c469ae2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what's up, when using a slightly modified emojivoto with overrides, I'm setting unexpected non-TLS traffic:
bin/linkerd install --proxy-auto-inject --tls optional | kubectl apply -f -
cat ~/emojivoto_overrides.yml | kubectl apply -f -
$ bin/linkerd -n emojivoto stat deploy
NAME MESHED SUCCESS RPS LATENCY_P50 LATENCY_P95 LATENCY_P99 TLS
emoji 1/1 100.00% 1.9rps 2ms 105ms 181ms 0%
vote-bot 1/1 - - - - - -
voting 1/1 82.76% 1.0rps 1ms 4ms 4ms 0%
web 1/1 92.92% 1.9rps 8ms 30ms 48ms 100%
on master
$ bin/linkerd -n emojivoto stat deploy
NAME MESHED SUCCESS RPS LATENCY_P50 LATENCY_P95 LATENCY_P99 TLS
emoji 1/1 100.00% 0.9rps 1ms 1ms 2ms 100%
vote-bot 1/1 - - - - - -
voting 1/1 88.00% 0.4rps 1ms 2ms 2ms 100%
web 1/1 90.74% 0.9rps 7ms 17ms 19ms 100%
@ihcsim i think the TLS issue i was seeing could be related to |
@siggy Yeah, same observation here. If I replaced |
@ihcsim yup, emoji-svc and voting-svc both use 8080 ;)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good! 🚢 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Glad to see this implemented! I had a few small suggestions, which we can talk about in more detail f2f... momentarily :)
Signed-off-by: Ivan Sim <[email protected]>
Signed-off-by: Ivan Sim <[email protected]>
Signed-off-by: Ivan Sim <[email protected]>
Without this check, some proxy-injector tests will fail due to zero-length ignore ports. Signed-off-by: Ivan Sim <[email protected]>
Signed-off-by: Ivan Sim <[email protected]>
Signed-off-by: Ivan Sim <[email protected]>
Signed-off-by: Ivan Sim <[email protected]>
…sn't triggered Signed-off-by: Ivan Sim <[email protected]>
c469ae2
to
a9da879
Compare
Signed-off-by: Ivan Sim <[email protected]>
Signed-off-by: Ivan Sim <[email protected]>
a9da879
to
1dbf22f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm modulo kevin's comments and one question from me. 👍 🚢
pkg/inject/inject.go
Outdated
conf.injectObjectMeta(patch) | ||
} else { | ||
if err := conf.overrideProxyConfigs(patch); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just want to confirm this behavior for my own understanding. if shouldInject == false
, but has a linkerd-proxy
in the pod, overrideProxyConfigs
will override the existing proxy settings in the pod.
does this mean if i specify some proxy config options during manual injection, those settings will be overridden by auto-inject?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just want to confirm this behavior for my own understanding. if shouldInject == false, but has a linkerd-proxy in the pod, overrideProxyConfigs will override the existing proxy settings in the pod.
That's correct. The current behaviour is that it will always perform an override (even if the old vs. new values are the same), either by using values found in the config annotations, or the defaults from the config map.
does this mean if i specify some proxy config options during manual injection, those settings will be overridden by auto-inject?
Those config options should be preserved. I still need to confirm with @klingerf on how to preserve them in light of his comment in 2447, if we decided not to store these config options as annotations.
Signed-off-by: Ivan Sim <[email protected]>
f67bb6d
to
ab38261
Compare
Signed-off-by: Ivan Sim <[email protected]>
b6f5316
to
b1567f5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks for making all of those updates. All of the new helpers in inject.go really help to clean that file up a lot.
Since this change also applies to configs that we inject via the CLI, can you add an additional CLI inject fixture that includes the annotations that you're adding, and validate that it's injected as expected? Another option would be to modify one of our existing text fixtures. For example:
diff --git a/cli/cmd/testdata/inject_emojivoto_deployment.golden.yml b/cli/cmd/testdata/inject_emojivoto_deployment.golden.yml
index 94e878b6..f86a261a 100644
--- a/cli/cmd/testdata/inject_emojivoto_deployment.golden.yml
+++ b/cli/cmd/testdata/inject_emojivoto_deployment.golden.yml
@@ -13,6 +13,7 @@ spec:
template:
metadata:
annotations:
+ config.linkerd.io/log-level: info
linkerd.io/created-by: linkerd/cli dev-undefined
linkerd.io/identity-mode: disabled
linkerd.io/proxy-version: testinjectversion
@@ -40,7 +41,7 @@ spec:
resources: {}
- env:
- name: LINKERD2_PROXY_LOG
- value: warn,linkerd2_proxy=info
+ value: info
- name: LINKERD2_PROXY_CONTROL_URL
value: tcp://linkerd-destination.linkerd.svc.cluster.local:8086
- name: LINKERD2_PROXY_CONTROL_LISTENER
diff --git a/cli/cmd/testdata/inject_emojivoto_deployment.input.yml b/cli/cmd/testdata/inject_emojivoto_deployment.input.yml
index d6b25a17..bc7e4e00 100644
--- a/cli/cmd/testdata/inject_emojivoto_deployment.input.yml
+++ b/cli/cmd/testdata/inject_emojivoto_deployment.input.yml
@@ -12,6 +12,8 @@ spec:
strategy: {}
template:
metadata:
+ annotations:
+ config.linkerd.io/log-level: info
creationTimestamp: null
labels:
app: web-svc
diff --git a/cli/cmd/testdata/inject_emojivoto_deployment_no_init_container.golden.yml b/cli/cmd/testdata/inject_emojivoto_deployment_no_init_container.golden.yml
index f4d7aeb1..d95d836e 100644
--- a/cli/cmd/testdata/inject_emojivoto_deployment_no_init_container.golden.yml
+++ b/cli/cmd/testdata/inject_emojivoto_deployment_no_init_container.golden.yml
@@ -13,6 +13,7 @@ spec:
template:
metadata:
annotations:
+ config.linkerd.io/log-level: info
linkerd.io/created-by: linkerd/cli dev-undefined
linkerd.io/identity-mode: disabled
linkerd.io/proxy-version: testinjectversion
@@ -40,7 +41,7 @@ spec:
resources: {}
- env:
- name: LINKERD2_PROXY_LOG
- value: warn,linkerd2_proxy=info
+ value: info
- name: LINKERD2_PROXY_CONTROL_URL
value: tcp://linkerd-destination.linkerd.svc.cluster.local:8086
- name: LINKERD2_PROXY_CONTROL_LISTENER
diff --git a/cli/cmd/testdata/inject_emojivoto_deployment_tls.golden.yml b/cli/cmd/testdata/inject_emojivoto_deployment_tls.golden.yml
index d94ef143..5ce03b77 100644
--- a/cli/cmd/testdata/inject_emojivoto_deployment_tls.golden.yml
+++ b/cli/cmd/testdata/inject_emojivoto_deployment_tls.golden.yml
@@ -13,6 +13,7 @@ spec:
template:
metadata:
annotations:
+ config.linkerd.io/log-level: info
linkerd.io/created-by: linkerd/cli dev-undefined
linkerd.io/identity-mode: optional
linkerd.io/proxy-version: testinjectversion
@@ -40,7 +41,7 @@ spec:
resources: {}
- env:
- name: LINKERD2_PROXY_LOG
- value: warn,linkerd2_proxy=info
+ value: info
- name: LINKERD2_PROXY_CONTROL_URL
value: tcp://linkerd-destination.linkerd.svc.cluster.local:8086
- name: LINKERD2_PROXY_CONTROL_LISTENER
But I think it's probably cleaner to just include a new fixture that includes all annotations.
Signed-off-by: Ivan Sim <[email protected]>
Signed-off-by: Ivan Sim <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⭐️ Awesome! Thanks for making all of those updates.
Signed-off-by: Ivan Sim <[email protected]>
This PR introduces a
useOverridesOrDefaults()
getOverrideOrDefault()
internal method to thepkg/inject
library, to perform proxy configuration overrides. It looks up the runtime value of a particular proxy configuration in the following places, and returns immediately when a match is found:linkerd-config
config map (containing defaults created during installation)The set of configurations that can be overridden is determined a predefined list of annotations defined in pkg/k8s/labels.go. Configuration overrides support for the CLI inject is tracked under #2447.
Test cases can be found in this gist.
Assumptions
useOverridesOrDefault()
method assumes that theconfig.ResourceConfig.nsAnnotations
andconfig.ResourceConfig.podMeta
fields always have the correct namespace annotations and pod template annotations.Limitations
Deployment
workload, per autoinject: Inject Pods rather than Deployments #2342.WIP
Perform configuration overrides when injected workload is updated (i.e. although the proxy injector won't inject the proxy into the YAML manifest, it still needs to check for config annotations.)Fixes #2287.
Signed-off-by: Ivan Sim [email protected]