Split the `linkerd-version` CLI flag into `control-plane-version` and `proxy-version` #2702

ihcsim · 2019-04-16T03:46:44Z

This PR replaces the CLI linkerd-version flag with a new flag named proxy-version. When provided during install, upgrade or inject, this flag sets the proxy image tag to the provided value. The original linkerd-version flag is marked deprecated. If provided, it has no effects on the YAML output.

All the control plane components' and proxy init's image tags are now pinned to version.Version. My thought is to not provide any CLI flags to override them, because:

The override can be done via kubectl set image
The only way to pass this value to the proxy init container spec is either via annotation, the config proto, or the ResourceConfig struct, none of which feel clean, just to support a dev-only flag.

The CNI linkerd-version flag isn't changed; it refers to the CNI image tag.

How I tested it:

# install the control plane with an older proxy version
bin/linkerd install --proxy-auto-inject --proxy-version=edge-19.4.3 | kubectl apply -f -

# expect proxy version to be persisted as a global property in the linkerd-config config map
kubectl -n linkerd describe configmap linkerd-config

# check the control plane components YAML; expect proxy to be edge-19.4.3
kubectl -n linkerd get deploy -o yaml | less

# try with emojivoto; expect proxy to be edge-19.4.3
curl -sL https://run.linkerd.io/emojivoto.yml | bin/linkerd inject | less

# expect proxy to be edge-19.4.4
curl -sL https://run.linkerd.io/emojivoto.yml | bin/linkerd inject --proxy-version=edge-19.4.4 - | less

# upgrade the control plane, but use proxy version edge-19.4.4
bin/linkerd upgrade --proxy-version=edge-19.4.4 | kubectl apply -f -

# expect proxy version edge-19.4.4 to be persisted as a global property
kubectl -n linkerd describe configmap linkerd-config

# upgrade control plane and proxy to HEAD
bin/linkerd upgrade | kubectl apply -f -

# expect latest dev proxy version to be persisted as a global property
k -n linkerd describe configmap linkerd-config

Fixes #2614

Signed-off-by: Ivan Sim ivan@buoyant.io

l5d-bot · 2019-04-16T04:00:32Z

Integration test results for 7964a77: fail 😕
Log output: https://gist.github.com/ae976441a0610c9a394742d893e9d026

alpeb

Changes look good and tested fine for me 👍

I was thinking it might be nice to change Global.version to Global.proxy_version, but it seems changes in the config are delicate as discussed in #2671, so that could be something for the future after multi-stage install (upgrade?) lands...

l5d-bot · 2019-04-16T18:39:52Z

Integration test results for 453c546: success 🎉
Log output: https://gist.github.com/2fd9074a5210c8e1f9b6bf59517ceed5

siggy

looks good! a couple comments around deprecating --linkerd-version.

also i think you'll need to update the travis script as well:

linkerd2/.travis.yml

Line 115 in 80634d6

    
           - target/cli/linux/linkerd install --linkerd-version=$LINKERD_TAG |tee linkerd.yml

cli/cmd/root.go

siggy · 2019-04-17T18:14:41Z

cli/cmd/root.go

@@ -285,8 +290,10 @@ func (options *proxyConfigOptions) flagSet(e pflag.ErrorHandling) *pflag.FlagSet
 	// Deprecated flags
 	flags.StringVar(&options.proxyMemoryRequest, "proxy-memory", options.proxyMemoryRequest, "Amount of Memory that the proxy sidecar requests")
 	flags.StringVar(&options.proxyCPURequest, "proxy-cpu", options.proxyCPURequest, "Amount of CPU units that the proxy sidecar requests")
+	flags.StringVarP(&options.linkerdVersion, "linkerd-version", "", options.linkerdVersion, "Tag to be used for Linkerd images")


related to previous comment, remove?

I think it's valuable to leave this, especially for development. but i don't think it should be visible by default

i don't think it's used anymore?

$ bin/go-run cli install --linkerd-version edge-1.2.3 | grep edge-1.2.3 Flag --linkerd-version has been deprecated, use --proxy-version instead

Ah, I think that's wrong then. We still need a way to override the linkerd version. If we want to call it control-plane-version or something, that's fine with me.

Yeah, I did rename it to control-plane-version prior to submitting this PR, then back out of it, because of the changes I had to make to get it into the proxy-init image tag.

Before I go ahead and include this change, can we not just use something like kubectl edit or kubectl set image to change the control plane version, for development purposes?

Actually, if we're gonna include the control plane version in the global config, then this won't be a problem. I can fix this in my next commit.

test/inject/inject_test.go

cli/cmd/testdata/inject_gettest_deployment.good.golden.yml

olix0r · 2019-04-17T22:02:06Z

cli/cmd/install.go

@@ -591,7 +592,7 @@ func (options *installOptions) globalConfig(identity *pb.IdentityContext) *pb.Gl
 		LinkerdNamespace:  controlPlaneNamespace,
 		AutoInjectContext: autoInjectContext,
 		CniEnabled:        options.noInitContainer,
-		Version:           options.linkerdVersion,
+		Version:           options.proxyVersion,


This isn't correct. the global version should be the installed control plane version. It sounds like we may need the ProxyConfig to get a Version field...

olix0r · 2019-04-17T22:02:25Z

cli/cmd/inject.go

-		configs.Global.Version = options.linkerdVersion
-		overrideAnnotations[k8s.ProxyVersionOverrideAnnotation] = options.linkerdVersion
+	if options.proxyVersion != "" {
+		configs.Global.Version = options.proxyVersion


again, I think this is conflating things...

Deprecate the linkerd-version flag. If provided, it has no effects on the YAML output. Signed-off-by: Ivan Sim <ivan@buoyant.io>

Signed-off-by: Ivan Sim <ivan@buoyant.io>

l5d-bot · 2019-04-18T22:45:09Z

Integration test results for 501ef0a: fail 😕
Log output: https://gist.github.com/7ec4e516008b1dc5958dbd14b82e0053

ihcsim · 2019-04-18T22:46:09Z

I split the --linkerd-version flag into --proxy-version and --control-plane-version.

--control-plane-version:

Available only for install and upgrade
Used as the tag for the controller, web, grafana and proxy-init images
Maps to the config.Global.Version proto field
During install, the protobuf field defaults to version.Version, unless overridden by the --control-plane-version field
This field isn't recorded in the config.Install config

--proxy-version:

Available for install, inject and upgrade.
Used as the tag for the proxy image
Maps to the config.Proxy.ProxyVersion proto field
During install, this protobuf field defaults toversion.Version, unless it's overridden by the --proxy-version field
This field isn't recorded in the config.Install config
It can be overridden at runtime via the config.linkerd.io/proxy-version annotation

LMK if I missed anything.

The 'linkerd-version' CLI flag is renamed to 'control-plane-version' Signed-off-by: Ivan Sim <ivan@buoyant.io>

l5d-bot · 2019-04-18T23:21:54Z

Integration test results for fbf36db: success 🎉
Log output: https://gist.github.com/9867eec63e23a9eec6527f36e16bbe76

cli/cmd/upgrade.go

Signed-off-by: Ivan Sim <ivan@buoyant.io>

l5d-bot · 2019-04-19T16:58:47Z

Integration test results for d2d08e4: success 🎉
Log output: https://gist.github.com/043edc572a60b3cbaabea856e773d0d6

siggy

this is looking good. most of my comments are around more explicit tests.

siggy · 2019-04-19T16:41:33Z

proto/config/config.proto

@@ -41,6 +41,8 @@ message Proxy {
  int64 proxy_uid = 10;
  LogLevel log_level = 11;
  bool disable_external_profiles = 12;
+
+  string proxy_version = 13;


maybe add a comment above Global.version calling out that it's for control-plane version, and also used as proxy-init version during inject.

pkg/inject/inject_test.go

cli/cmd/upgrade_test.go

cli/cmd/testdata/upgrade_default.golden

cli/cmd/inject_test.go

Signed-off-by: Ivan Sim <ivan@buoyant.io>

ihcsim · 2019-04-19T17:50:13Z

cli/cmd/upgrade_test.go

@@ -184,9 +188,9 @@ metadata:
  namespace: linkerd
 data:
  global: |
-    {"linkerdNamespace":"linkerd","cniEnabled":false,"version":"dev-undefined","identityContext":{"trustDomain":"cluster.local","trustAnchorsPem":"-----BEGIN CERTIFICATE-----\nMIIBYDCCAQegAwIBAgIBATAKBggqhkjOPQQDAjAYMRYwFAYDVQQDEw1jbHVzdGVy\nLmxvY2FsMB4XDTE5MDMwMzAxNTk1MloXDTI5MDIyODAyMDM1MlowGDEWMBQGA1UE\nAxMNY2x1c3Rlci5sb2NhbDBZMBMGByqGSM49AgEGCCqGSM49AwEHA0IABAChpAt0\nxtgO9qbVtEtDK80N6iCL2Htyf2kIv2m5QkJ1y0TFQi5hTVe3wtspJ8YpZF0pl364\n6TiYeXB8tOOhIACjQjBAMA4GA1UdDwEB/wQEAwIBBjAdBgNVHSUEFjAUBggrBgEF\nBQcDAQYIKwYBBQUHAwIwDwYDVR0TAQH/BAUwAwEB/zAKBggqhkjOPQQDAgNHADBE\nAiBQ/AAwF8kG8VOmRSUTPakSSa/N4mqK2HsZuhQXCmiZHwIgZEzI5DCkpU7w3SIv\nOLO4Zsk1XrGZHGsmyiEyvYF9lpY=\n-----END CERTIFICATE-----\n","issuanceLifetime":"86400s","clockSkewAllowance":"20s"},"autoInjectContext":null}
+    {"linkerdNamespace":"linkerd","cniEnabled":false,"version":"install-control-plane-version","identityContext":{"trustDomain":"cluster.local","trustAnchorsPem":"-----BEGIN CERTIFICATE-----\nMIIBYDCCAQegAwIBAgIBATAKBggqhkjOPQQDAjAYMRYwFAYDVQQDEw1jbHVzdGVy\nLmxvY2FsMB4XDTE5MDMwMzAxNTk1MloXDTI5MDIyODAyMDM1MlowGDEWMBQGA1UE\nAxMNY2x1c3Rlci5sb2NhbDBZMBMGByqGSM49AgEGCCqGSM49AwEHA0IABAChpAt0\nxtgO9qbVtEtDK80N6iCL2Htyf2kIv2m5QkJ1y0TFQi5hTVe3wtspJ8YpZF0pl364\n6TiYeXB8tOOhIACjQjBAMA4GA1UdDwEB/wQEAwIBBjAdBgNVHSUEFjAUBggrBgEF\nBQcDAQYIKwYBBQUHAwIwDwYDVR0TAQH/BAUwAwEB/zAKBggqhkjOPQQDAgNHADBE\nAiBQ/AAwF8kG8VOmRSUTPakSSa/N4mqK2HsZuhQXCmiZHwIgZEzI5DCkpU7w3SIv\nOLO4Zsk1XrGZHGsmyiEyvYF9lpY=\n-----END CERTIFICATE-----\n","issuanceLifetime":"86400s","clockSkewAllowance":"20s"},"autoInjectContext":null}


global.version and proxy.proxyVersion pick up the version test values used in testInstallOptions(), instead of just dev-undefined.

l5d-bot · 2019-04-19T18:15:45Z

Integration test results for 8d6092a: success 🎉
Log output: https://gist.github.com/b361f5fedb7d0a4494e9d1a3c435adde

siggy

lgtm 👍 🚢

ihcsim added the area/cli label Apr 16, 2019

ihcsim requested review from siggy, olix0r and alpeb April 16, 2019 03:46

ihcsim self-assigned this Apr 16, 2019

alpeb approved these changes Apr 16, 2019

View reviewed changes

siggy reviewed Apr 17, 2019

View reviewed changes

olix0r reviewed Apr 17, 2019

View reviewed changes

cli/cmd/testdata/inject_gettest_deployment.good.golden.yml Show resolved Hide resolved

olix0r reviewed Apr 17, 2019

View reviewed changes

Ivan Sim added 2 commits April 18, 2019 09:19

Replace the linkerd-version CLI flag with proxy-version

b9a381d

Deprecate the linkerd-version flag. If provided, it has no effects on the YAML output. Signed-off-by: Ivan Sim <ivan@buoyant.io>

Fix integration test

7e957f7

Signed-off-by: Ivan Sim <ivan@buoyant.io>

ihcsim changed the title ~~2.4 - Replace the linkerd-version CLI flag with proxy-version~~ Replace the linkerd-version CLI flag with proxy-version Apr 18, 2019

Add version field to proxy config

28cb85a

Signed-off-by: Ivan Sim <ivan@buoyant.io>

ihcsim force-pushed the isim/cli-proxy-version-flag branch from 453c546 to 6d157c6 Compare April 18, 2019 22:19

ihcsim changed the title ~~Replace the linkerd-version CLI flag with proxy-version~~ Split the linkerd-version CLI flag into control-plane-version and proxy-version Apr 18, 2019

ihcsim force-pushed the isim/cli-proxy-version-flag branch from 6d157c6 to 501ef0a Compare April 18, 2019 22:38

Add the control plane version to the global config

fbf36db

The 'linkerd-version' CLI flag is renamed to 'control-plane-version' Signed-off-by: Ivan Sim <ivan@buoyant.io>

ihcsim force-pushed the isim/cli-proxy-version-flag branch from 501ef0a to fbf36db Compare April 18, 2019 23:05

alpeb reviewed Apr 19, 2019

View reviewed changes

cli/cmd/upgrade.go Outdated Show resolved Hide resolved

Alejandro's feedback

d2d08e4

Signed-off-by: Ivan Sim <ivan@buoyant.io>

siggy approved these changes Apr 19, 2019

View reviewed changes

Ivan Sim added 4 commits April 19, 2019 10:10

Add comment to Global.Version proto field

83c0578

Signed-off-by: Ivan Sim <ivan@buoyant.io>

Unit test for init image version

1c42396

Signed-off-by: Ivan Sim <ivan@buoyant.io>

Use more specific control plane and proxy versions in unit tests

dfb492f

Signed-off-by: Ivan Sim <ivan@buoyant.io>

Fix broken unit test

8d6092a

Signed-off-by: Ivan Sim <ivan@buoyant.io>

ihcsim commented Apr 19, 2019

View reviewed changes

siggy approved these changes Apr 19, 2019

View reviewed changes

ihcsim merged commit 8d13084 into master Apr 19, 2019

ihcsim deleted the isim/cli-proxy-version-flag branch April 19, 2019 18:35

siggy mentioned this pull request Apr 19, 2019

proxy image version not populated in inject output #2727

Closed

dadjeibaah mentioned this pull request Apr 20, 2019

Add the ability to inject a debug sidecar #2726

Merged

adleong mentioned this pull request Apr 29, 2020

Hide development flags in release builds #4310

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split the `linkerd-version` CLI flag into `control-plane-version` and `proxy-version` #2702

Split the `linkerd-version` CLI flag into `control-plane-version` and `proxy-version` #2702

ihcsim commented Apr 16, 2019 •

edited

Loading

l5d-bot commented Apr 16, 2019

alpeb left a comment

l5d-bot commented Apr 16, 2019

siggy left a comment

siggy Apr 17, 2019

olix0r Apr 17, 2019

siggy Apr 17, 2019

olix0r Apr 17, 2019

ihcsim Apr 17, 2019

ihcsim Apr 17, 2019

olix0r Apr 17, 2019

olix0r Apr 17, 2019

l5d-bot commented Apr 18, 2019

ihcsim commented Apr 18, 2019

l5d-bot commented Apr 18, 2019

l5d-bot commented Apr 19, 2019

siggy left a comment

siggy Apr 19, 2019

ihcsim Apr 19, 2019

l5d-bot commented Apr 19, 2019

siggy left a comment

Split the linkerd-version CLI flag into control-plane-version and proxy-version #2702

Split the linkerd-version CLI flag into control-plane-version and proxy-version #2702

Conversation

ihcsim commented Apr 16, 2019 • edited Loading

l5d-bot commented Apr 16, 2019

alpeb left a comment

Choose a reason for hiding this comment

l5d-bot commented Apr 16, 2019

siggy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

l5d-bot commented Apr 18, 2019

ihcsim commented Apr 18, 2019

l5d-bot commented Apr 18, 2019

l5d-bot commented Apr 19, 2019

siggy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

l5d-bot commented Apr 19, 2019

siggy left a comment

Choose a reason for hiding this comment

Split the `linkerd-version` CLI flag into `control-plane-version` and `proxy-version` #2702

Split the `linkerd-version` CLI flag into `control-plane-version` and `proxy-version` #2702

ihcsim commented Apr 16, 2019 •

edited

Loading