Support Auto-Inject Config Overrides Via Annotations by ihcsim · Pull Request #2471 · linkerd/linkerd2

ihcsim · 2019-03-07T21:58:29Z

This PR introduces a ~~useOverridesOrDefaults()~~ getOverrideOrDefault() internal method to the pkg/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:

pod template annotations
owner namespace annotations
the 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

The useOverridesOrDefault() method assumes that the config.ResourceConfig.nsAnnotations and config.ResourceConfig.podMeta fields always have the correct namespace annotations and pod template annotations.

Limitations

Only works with 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]

grampelberg · 2019-03-08T16:35:14Z

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:

New CRD that uses selectors to target the overrides. This will be particularly painful to version between releases, introduces another CRD that isn't super generic/useful elsewhere.
Something with a ConfigMap. Feels as bad or worse than the annotations, unstructured and separate.

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.

siggy

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.

pkg/inject/inject.go

pkg/inject/inject_test.go

alpeb

This all looks great to me 🌟
I was just wondering about the usage of strconv.FormatUint vs simply strconv.Itoa in getOverrideOrDefault

pkg/inject/inject_test.go

alpeb · 2019-03-11T16:57:40Z

@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 ignore-inbound-ports and log-level for debugging purposes.

ihcsim · 2019-03-11T17:23:08Z

I was just wondering about the usage of strconv.FormatUint vs simply strconv.Itoa in getOverrideOrDefault

It's just a personal nit. Underneath the hood, strconv.Itoa uses FormatInt(int64(i), 10). Since the ports are declared as uint, I prefer not to have to convert an uint into an int, in order to use strconv.Itoa.

siggy

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%

siggy · 2019-03-11T18:58:32Z

@ihcsim i think the TLS issue i was seeing could be related to proxy.linkerd.io/ignore-outbound-ports: "8079,8080", will keep testing.

ihcsim · 2019-03-11T19:06:07Z

@siggy Yeah, same observation here. If I replaced 8079:8080 with something random like 9999, TLS works again. Is there anything special about 8080?

siggy · 2019-03-11T19:46:12Z

@ihcsim yup, emoji-svc and voting-svc both use 8080 ;)

$ kubectl -n emojivoto get svc
NAME         TYPE           CLUSTER-IP       EXTERNAL-IP   PORT(S)        AGE
emoji-svc    ClusterIP      None             <none>        8080/TCP       42m
voting-svc   ClusterIP      None             <none>        8080/TCP       42m
web-svc      LoadBalancer   10.108.179.230   localhost     80:30909/TCP   42m

siggy

looks good! 🚢 👍

klingerf

Glad to see this implemented! I had a few small suggestions, which we can talk about in more detail f2f... momentarily :)

pkg/inject/inject.go

pkg/k8s/labels.go

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]>

…sn't triggered Signed-off-by: Ivan Sim <[email protected]>

Signed-off-by: Ivan Sim <[email protected]>

siggy

lgtm modulo kevin's comments and one question from me. 👍 🚢

siggy · 2019-03-12T22:14:01Z

pkg/inject/inject.go

+			}
 			conf.injectObjectMeta(patch)
+		} else {
+			if err := conf.overrideProxyConfigs(patch); err != nil {


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?

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]>

klingerf

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.

pkg/inject/inject.go

pkg/k8s/labels.go

pkg/inject/inject.go

Signed-off-by: Ivan Sim <[email protected]>

klingerf

⭐️ Awesome! Thanks for making all of those updates.

cli/cmd/inject_test.go

Signed-off-by: Ivan Sim <[email protected]>

ihcsim added priority/P0 Release Blocker area/inject labels Mar 7, 2019

ihcsim self-assigned this Mar 7, 2019

ihcsim requested review from alpeb and klingerf March 7, 2019 21:58

ihcsim force-pushed the isim/inject-annotations branch 2 times, most recently from 5193e8a to 777f2ce Compare March 8, 2019 04:12

ihcsim requested a review from siggy March 8, 2019 18:01

siggy reviewed Mar 8, 2019

View reviewed changes

pkg/inject/inject.go Outdated Show resolved Hide resolved

pkg/inject/inject.go Outdated Show resolved Hide resolved

pkg/inject/inject_test.go Outdated Show resolved Hide resolved

ihcsim force-pushed the isim/inject-annotations branch from 6fc2128 to 7aa9c61 Compare March 10, 2019 20:47

alpeb reviewed Mar 11, 2019

View reviewed changes

pkg/inject/inject_test.go Outdated Show resolved Hide resolved

ihcsim force-pushed the isim/inject-annotations branch from 04b228b to c469ae2 Compare March 11, 2019 17:46

siggy reviewed Mar 11, 2019

View reviewed changes

siggy approved these changes Mar 11, 2019

View reviewed changes

klingerf reviewed Mar 11, 2019

View reviewed changes

pkg/inject/inject.go Outdated Show resolved Hide resolved

pkg/inject/inject.go Outdated Show resolved Hide resolved

pkg/k8s/labels.go Outdated Show resolved Hide resolved

Ivan Sim added 9 commits March 11, 2019 16:10

New constants for the config annotations

fce948b

Signed-off-by: Ivan Sim <[email protected]>

Functions to perform the config overrides

c3a5bf2

Signed-off-by: Ivan Sim <[email protected]>

Replace the 'overrides' map's interface{} type with string

c5d1b5b

Signed-off-by: Ivan Sim <[email protected]>

Check 'ignore inbound' and 'ignore outbound' ports slice length

0c9d82a

Without this check, some proxy-injector tests will fail due to zero-length ignore ports. Signed-off-by: Ivan Sim <[email protected]>

Add a useDefault param to indicate whether to use config map defaults

ef0e2da

Signed-off-by: Ivan Sim <[email protected]>

Unit tests for image pull policy annotations

3c25757

Signed-off-by: Ivan Sim <[email protected]>

Linter fixes

9db470c

Signed-off-by: Ivan Sim <[email protected]>

Remove unused test file

9028bac

Signed-off-by: Ivan Sim <[email protected]>

Fix return values

55dd690

Signed-off-by: Ivan Sim <[email protected]>

Ivan Sim added 3 commits March 11, 2019 16:10

Siggy's feedback

7938dd0

Signed-off-by: Ivan Sim <[email protected]>

Feedback from Alejandro

7523f1a

Signed-off-by: Ivan Sim <[email protected]>

Ensure proxy injector honors config overrides even when auto-inject i…

c2b09a9

…sn't triggered Signed-off-by: Ivan Sim <[email protected]>

ihcsim force-pushed the isim/inject-annotations branch from c469ae2 to a9da879 Compare March 12, 2019 03:59

Ivan Sim added 2 commits March 11, 2019 21:00

Change config annotations prefix to config.linkerd.io

6ca4427

Signed-off-by: Ivan Sim <[email protected]>

Remove namespace-level config annotations

1dbf22f

Signed-off-by: Ivan Sim <[email protected]>

ihcsim force-pushed the isim/inject-annotations branch from a9da879 to 1dbf22f Compare March 12, 2019 04:01

siggy approved these changes Mar 12, 2019

View reviewed changes

Kevin's feedback

ab38261

Signed-off-by: Ivan Sim <[email protected]>

ihcsim force-pushed the isim/inject-annotations branch from f67bb6d to ab38261 Compare March 13, 2019 05:23

Update log messages

b1567f5

Signed-off-by: Ivan Sim <[email protected]>

ihcsim force-pushed the isim/inject-annotations branch from b6f5316 to b1567f5 Compare March 13, 2019 17:01

klingerf reviewed Mar 13, 2019

View reviewed changes

Ivan Sim added 2 commits March 13, 2019 14:56

Momentarily remove support of 'overrides on updates'

9d0e9ed

Signed-off-by: Ivan Sim <[email protected]>

Kevin's feedback

90c82db

Signed-off-by: Ivan Sim <[email protected]>

klingerf mentioned this pull request Mar 13, 2019

Update CHANGES.md for the edge-19.3.2 release #2495

Merged

klingerf approved these changes Mar 14, 2019

View reviewed changes

cli/cmd/inject_test.go Show resolved Hide resolved

New uninject unit test for config overrides

b1bef99

Signed-off-by: Ivan Sim <[email protected]>

This was referenced Mar 14, 2019

Support Proxy Image Version Override #2497

Closed

Support Configs Override On Meshed Workloads #2498

Closed

ihcsim merged commit 468ad11 into master Mar 14, 2019

ihcsim deleted the isim/inject-annotations branch March 14, 2019 15:42

ihcsim mentioned this pull request Mar 14, 2019

Control skip port flag via custom annotation in resource #1997

Closed

klingerf mentioned this pull request Mar 14, 2019

Document inject override annotations linkerd/website#222

Closed

This was referenced Mar 29, 2019

Support Automatic Proxy Uninjection #2338

Closed

Consolidate Differences In Runtime YAML of Auto-Inject And Manual-Injected Workloads #2576

Closed

Conversation

ihcsim commented Mar 7, 2019 • edited by alpeb Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Assumptions

Limitations

WIP

Uh oh!

grampelberg commented Mar 8, 2019

Uh oh!

siggy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alpeb left a comment • edited by ihcsim Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alpeb commented Mar 11, 2019

Uh oh!

ihcsim commented Mar 11, 2019

Uh oh!

siggy left a comment

Choose a reason for hiding this comment

on master

Uh oh!

siggy commented Mar 11, 2019

Uh oh!

ihcsim commented Mar 11, 2019

Uh oh!

siggy commented Mar 11, 2019

Uh oh!

siggy left a comment

Choose a reason for hiding this comment

Uh oh!

klingerf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

siggy left a comment

Choose a reason for hiding this comment

Uh oh!

siggy Mar 12, 2019

Choose a reason for hiding this comment

Uh oh!

ihcsim Mar 13, 2019

Choose a reason for hiding this comment

Uh oh!

klingerf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

klingerf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ihcsim commented Mar 7, 2019 •

edited by alpeb

Loading

alpeb left a comment •

edited by ihcsim

Loading