Skip to content
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

Define proxy version override annotation #2593

Merged
merged 3 commits into from
Apr 2, 2019

Conversation

ihcsim
Copy link
Contributor

@ihcsim ihcsim commented Mar 29, 2019

This PR adds a new config.linkerd.io/linkerd-version annotation to capture overridden proxy version, using linkerd inject --linkerd-version.

Assumption: The config.linkerd.io/linkerd-version and linkerd.io/proxy-version use the same overridden value.

$ bin/go-run cli inject isim-tests/nginx/nginx.yaml --linkerd-version=isim-test  > inject-override.yaml
$ bin/go-run cli inject isim-tests/nginx/nginx.yaml  > inject.yaml
$ diff -u inject.yaml inject-override.yaml
--- inject.yaml 2019-03-29 10:25:43.379796632 -0700
+++ inject-override.yaml        2019-03-29 10:25:33.787871270 -0700
@@ -11,9 +11,10 @@
   template:
     metadata:
       annotations:
+        config.linkerd.io/linkerd-version: isim-test
         linkerd.io/created-by: linkerd/cli dev-45aa10dc-isim
         linkerd.io/identity-mode: default
-        linkerd.io/proxy-version: git-45aa10dc
+        linkerd.io/proxy-version: isim-test
       creationTimestamp: null
       labels:
         app: nginx
@@ -85,7 +86,7 @@
           value: linkerd-identity.$(_l5d_ns).serviceaccount.identity.$(_l5d_ns).$(_l5d_trustdomain)
         - name: LINKERD2_PROXY_DESTINATION_SVC_NAME
           value: linkerd-controller.$(_l5d_ns).serviceaccount.identity.$(_l5d_ns).$(_l5d_trustdomain)
-        image: gcr.io/linkerd-io/proxy:git-45aa10dc
+        image: gcr.io/linkerd-io/proxy:isim-test
         imagePullPolicy: IfNotPresent
         livenessProbe:
           httpGet:
@@ -120,7 +121,7 @@
         - "2102"
         - --inbound-ports-to-ignore
         - 4190,4191
-        image: gcr.io/linkerd-io/proxy-init:git-45aa10dc
+        image: gcr.io/linkerd-io/proxy-init:isim-test
         imagePullPolicy: IfNotPresent
         name: linkerd-init
         resources: {}

Fixes #2551 #2497

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

@ihcsim ihcsim requested review from olix0r and alpeb March 29, 2019 17:39
@ihcsim ihcsim self-assigned this Mar 29, 2019
@ihcsim ihcsim added the priority/P1 Planned for Release label Mar 29, 2019
@l5d-bot
Copy link
Collaborator

l5d-bot commented Mar 29, 2019

Integration test results for 0fe5ea6: success 🎉
Log output: https://gist.github.com/f436d609658a3c8d63015759b4df6f9a

@ihcsim ihcsim requested a review from siggy March 29, 2019 17:52
Copy link
Member

@siggy siggy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code lgtm, and works as expected when I set config.linkerd.io/linkerd-version on the pod template:

template:
  metadata:
    annotations:
      linkerd.io/proxy-version: git-0fe5ea67
      config.linkerd.io/linkerd-version: edge-19.3.3

I'd like to understand a bit better your comment:

Assumption: The config.linkerd.io/linkerd-version and linkerd.io/proxy-version use the same overridden value.

...and related, what's the interaction between linkerd.io/proxy-version and config.linkerd.io/linkerd-version? Does the latter always override the former?

@ihcsim
Copy link
Contributor Author

ihcsim commented Mar 30, 2019

@siggy I assume in your test, you added the annotation to an existing workload? If I use linkerd inject --linkerd-version=isim-tests, both the linkerd.io/proxy-version and config.linkerd.io/linkerd-version annotations will have the same value:

  template:
    metadata:
      annotations:
        config.linkerd.io/linkerd-version: isim-test
        linkerd.io/proxy-version: isim-test

I am not 100% sure if that's the desired behaviour. I guess it really boils down to the purpose of these annotations. I am inclined to make linkerd.io/proxy-version always picks up the default value, and config.linkerd.io/linkerd-version the overridden value. WDYT?

Does the latter always override the former?

Yes, the function that is used to build the image name uses config.linkerd.io/linkerd-version, if it's available. Otherwise, it falls back to linkerd.io/proxy-version.

@alpeb
Copy link
Member

alpeb commented Mar 31, 2019

I think it can be a bit confusing for users to have both annotations. Let me know what you think of the following, making room for upgrade policy handling that we've alluded at some point in the past:

Leaving linkerd.io/proxy-version to track the version the injected proxy is using like it currently is, and instead of adding config.linkerd.io/linkerd-version adding something like config.linkerd.io/upgrade-policy whose values could be:

  • config.linkerd.io/upgrade-policy: default (or annotation missing):
    Current behavior of the auto-injector or CLI injectpick the latest version according to the global config.
  • config.linkerd.io/upgrade-policy: fixed and linkerd.io/proxy-version set to some value:
    The auto-injector will always use the value in linker.io/proxy-version. Same for CLI inject. If none of the annotations is set but we issue a CLI inject --linkerd-version=xxx then the annotations config.linkerd.io/upgrade-policy: fixed and linkerd.io/proxy-version: xxx will get set.
  • config.linkerd.io/upgrade-policy: auto (to-do for later):
    When the control panel is upgraded, look for all the workloads with this annotation and upgrade them.

@ihcsim
Copy link
Contributor Author

ihcsim commented Apr 1, 2019

@alpeb I like the idea of upgrade policy. But I think it might be a topic that merits further design and discussion with others like @olix0r and @grampelberg.

Fundamentally, for this PR, I think we just wanna decide on when a user override the proxy version using the --linkerd-version flag, do we want to persist that info.? Based on #2551 and #2497, I think we do.

@ihcsim
Copy link
Contributor Author

ihcsim commented Apr 1, 2019

Based on #2551 and #2497, I think we do.

To clarify, with or without update policy, if we don't encode the overridden version in the annotation, the next time the proxy injector intercepts the YAML (during an update), it will override the version with the default (if they aren't the same).

@siggy
Copy link
Member

siggy commented Apr 1, 2019

@ihcsim Thanks for the additional detail. To clarify my set up, I started with:

kind: Deployment
spec:
  template:
    metadata:
      annotations:
        config.linkerd.io/linkerd-version: edge-19.3.3

... and then after auto-injection I observed:

kind: Pod
metadata:
  annotations:
    config.linkerd.io/linkerd-version: edge-19.3.3
    linkerd.io/created-by: linkerd/proxy-injector git-0fe5ea67
    linkerd.io/proxy-version: git-0fe5ea67

Is this expected?

@ihcsim
Copy link
Contributor Author

ihcsim commented Apr 1, 2019

@siggy Yes, that's the expected behaviour for auto-inject; i.e. config.linkerd.io/linkerd-version tracks the user-provided value, while linkerd.io/proxy-version reads the default from the linkerd-config config map. In light of that, I think it makes sense to make both CLI inject and auto-inject consistent.

In short, the reason why the linkerd inject is slightly different is that because it has to account for CLI flag. My next commit to this branch will fix this.

Ivan Sim added 2 commits April 1, 2019 15:24
This ensures consistent usages of the config.linkerd.io/linkerd-version and
linkerd.io/proxy-version annotations. The former will only be used to track
overridden version, while the latter shows the cluster's current default
version.

Signed-off-by: Ivan Sim <[email protected]>
@ihcsim ihcsim force-pushed the isim/image-version-annotation branch from 0fe5ea6 to 3fd90b9 Compare April 1, 2019 22:25
@l5d-bot
Copy link
Collaborator

l5d-bot commented Apr 1, 2019

Integration test results for 3fd90b9: success 🎉
Log output: https://gist.github.com/804a6598f223962da5bcd0fd85423b06

@siggy
Copy link
Member

siggy commented Apr 1, 2019

@ihcsim I've confirmed with the latest commits that manual and auto-injection both result in the same combination of annotations, specifically:

Manual

$ cat ~/emojivoto-enabled.yml | bin/linkerd inject - --linkerd-version edge-19.3.3

kind: Deployment
spec:
  template:
    metadata:
      annotations:
        config.linkerd.io/linkerd-version: edge-19.3.3
        linkerd.io/created-by: linkerd/cli git-3fd90b90
        linkerd.io/proxy-version: git-3fd90b90

Auto

$ kubectl -n emojivoto-enabled get po/emoji-5f85b5d47-zdvkt -oyaml
kind: Pod
metadata:
  annotations:
    config.linkerd.io/linkerd-version: edge-19.3.3
    linkerd.io/created-by: linkerd/proxy-injector git-3fd90b90
    linkerd.io/proxy-version: git-3fd90b90

A more meta-question: What's the use case for having both linkerd.io/proxy-version and config.linkerd.io/linkerd-version? If config.linkerd.io/linkerd-version is present, we should use that. If config.linkerd.io/linkerd-version is not present, shouldn't we just use whatever is in the linkerd-config configMap at global.version?

@ihcsim
Copy link
Contributor Author

ihcsim commented Apr 2, 2019

What's the use case for having both linkerd.io/proxy-version and config.linkerd.io/linkerd-version?

Having both can feel a bit redundant. Our thought is that one is descriptive, while the other is an implicit explicit override. If we decided that this should be part of the bigger update policy story, I'm fine with holding off.

@olix0r Further thoughts?

If config.linkerd.io/linkerd-version is present, we should use that. If config.linkerd.io/linkerd-version is not present, shouldn't we just use whatever is in the linkerd-config configMap at global.version?

That's how it works in this PR, as seen here.

@olix0r
Copy link
Member

olix0r commented Apr 2, 2019

The config.linkerd.io/proxy-version annotation should always be considered an explicit override that is never changed to the default. linkerd.io/proxy-version is always set by the injector with the value used.

This is important when re-injecting a workload: Should we update to a newer proxy version or not? We need these separate namespaces to be semantically different.

@olix0r
Copy link
Member

olix0r commented Apr 2, 2019

If config.linkerd.io/linkerd-version is present, we should use that. If config.linkerd.io/linkerd-version is not present, shouldn't we just use whatever is in the linkerd-config configMap at global.version?

Correct. And the other annotation always indicates the version that was actually used.

Copy link
Member

@olix0r olix0r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@ihcsim
Copy link
Contributor Author

ihcsim commented Apr 2, 2019

And the other annotation always indicates the version that was actually used.

Wanna make sure I understand what you meant by the version that was actually used. This is referring to whatever version provided during install/upgrade and is saved in the config.Global.Version field, right? And this value shouldn't change often.

@olix0r
Copy link
Member

olix0r commented Apr 2, 2019

@ihcsim ah, i see the discrepancy: we have a few possible "linkerd versions". Up until now, (I think) linkerd.io/linkerd-version has referred to the proxy's tag. Should we split this into two distinct annotations, i.e. linkerd.io/proxy-version & linkerd.io/inject-version?

The intent here being that we need to be able to reconstitute how a specific pod was injected..

@ihcsim
Copy link
Contributor Author

ihcsim commented Apr 2, 2019

Per slack discussion, the version of the injector (CLI, webhook etc.) is currently encoded in the linkerd.io/created-by annotation. For clarity, we'll like to separate that into a separate annotation. Also, the CLI --linkerd-version flag should be renamed to --proxy-version, which configures only the proxy; everything else, including proxy-init, gets the controller's version. We will create a separate issue for these tasks. See #2614.

@l5d-bot
Copy link
Collaborator

l5d-bot commented Apr 2, 2019

Integration test results for 6f49072: success 🎉
Log output: https://gist.github.com/11f4c1350a2e97b13eaebc5db091abe2

@ihcsim ihcsim merged commit 92f15e7 into master Apr 2, 2019
@ihcsim ihcsim deleted the isim/image-version-annotation branch April 2, 2019 21:27
KatherineMelnyk pushed a commit to KatherineMelnyk/linkerd2 that referenced this pull request Apr 5, 2019
* Define proxy version override annotation
* Don't override global linkerd version during inject

This ensures consistent usages of the config.linkerd.io/linkerd-version and
linkerd.io/proxy-version annotations. The former will only be used to track
overridden version, while the latter shows the cluster's current default
version.

* Rename proxy version config override annotation

Signed-off-by: Ivan Sim <[email protected]>
Signed-off-by: [email protected] <[email protected]>
KatherineMelnyk pushed a commit to KatherineMelnyk/linkerd2 that referenced this pull request Apr 5, 2019
* Define proxy version override annotation
* Don't override global linkerd version during inject

This ensures consistent usages of the config.linkerd.io/linkerd-version and
linkerd.io/proxy-version annotations. The former will only be used to track
overridden version, while the latter shows the cluster's current default
version.

* Rename proxy version config override annotation

Signed-off-by: Ivan Sim <[email protected]>
Signed-off-by: [email protected] <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/inject priority/P1 Planned for Release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants