Remove dependency of linkerd-config for control plane components #4915

Pothulapati · 2020-08-26T09:32:21Z

Part of #4914

This PR removes the dependency of mounting linkerd-config into control
plane components by making all that information passed through CLI
flags.`

Signed-off-by: Tarun Pothulapati [email protected]

This PR removes the dependency of mounting `linkerd-config` into control plane components by making all that information passed through CLI flags.` Signed-off-by: Tarun Pothulapati <[email protected]>

adleong

Looks good! I have a few minor comments.

controller/cmd/destination/main.go

adleong · 2020-08-26T21:14:37Z

controller/cmd/identity/main.go

@@ -42,6 +40,13 @@ func Main(args []string) {
 	addr := cmd.String("addr", ":8080", "address to serve on")
 	adminAddr := cmd.String("admin-addr", ":9990", "address of HTTP admin server")
 	kubeConfigPath := cmd.String("kubeconfig", "", "path to kube config")
+	controllerNS := cmd.String("controller-namespace", "", "namespace of the linkerd control plane")


I think this is just specifies which namespace to emit kubernetes events to, right? We could just call the flag --namespace. or maybe even get the current namespace from the downward api.

Hmm, I went with --controller-namespace as every other component also has it, and helps with consistency?

controller/cmd/identity/main.go

controller/cmd/destination/main.go

controller/cmd/identity/main.go

adleong · 2020-08-26T21:42:23Z

It looks like the golden files need to be updated.

Additionally there are some unit test failures related to the way that clock skew allowance is rendered during upgrade. It looks like this has revealed an existing bug where when the clock skew allowance is read from the configmap during upgrade, it is rendered as a protobuf duration instead of as a go duration:

IssuanceLifetime:    idctx.GetIssuanceLifetime().String(),

I think we need to first convert it to a go duration in order to render it correctly.

Protobuf's Duration is different from goland Duration, and hence this conversion has to be done manually during retrieval. This worked previously because we were not using that value directly, and were storing the same value in protobuf format once again (i.e in the configMap) but now with these duration values being used as flags outside the configmap, this was discovered. Signed-off-by: Tarun Pothulapati <[email protected]>

Signed-off-by: Tarun Pothulapati <[email protected]>

Pothulapati · 2020-08-27T08:26:00Z

cli/cmd/upgrade.go

@@ -587,15 +588,25 @@ func (options *upgradeOptions) fetchIdentityValues(k kubernetes.Interface, idctx
 		}
 	}

+	clockSkewDuration, err := ptypes.Duration(idctx.GetClockSkewAllowance())


This was causing the upgrade tests to fail, as protobuf's duration is different from Duration in golang, we have to manually convert that using this func, if we are using the value directly. I think this wasn't a problem previously because we were again converting it back into protobuf and using it only there.

Signed-off-by: Tarun Pothulapati <[email protected]>

adleong

I notice that the proxy-injector is not updated here. Make sure you update the description of this PR to indicate what the plan is there.

adleong · 2020-08-27T18:21:23Z

cli/cmd/upgrade_test.go

@@ -186,6 +189,11 @@ func TestUpgradeOverwriteIssuer(t *testing.T) {
 			if id == "ConfigMap/linkerd-config" {
 				continue
 			}
+
+			if id == "Deployment/linkerd-identity" {
+				// skip checking identity as `identity-trust-anchor-pem` is overridden, which is passed as a CLI flag


if we wanted to take this a step further, we could verify that that the path is {spec, containers, *, spec, args, *} or whatever and that the diff.b is issuerCerts.caCrt.

Great suggestion! Done 👍

This commit removes linkerd-config dependency in the proxy-injector and passes all the values through flags Signed-off-by: Tarun Pothulapati <[email protected]>

Signed-off-by: Tarun Pothulapati <[email protected]>

Pothulapati · 2020-08-31T14:25:44Z

Updated this PR to remove the linkerd-config dependency even for proxy-injector.

Signed-off-by: Tarun Pothulapati <[email protected]>

charts/partials/templates/_helpers.tpl

alpeb · 2020-09-03T17:31:34Z

Nit: can you use list instead of tuple? They're equivalent but only the former appears in the docs.

Signed-off-by: Tarun Pothulapati <[email protected]>

adleong · 2020-09-28T17:37:23Z

I think this will be simpler if we do #5008 first. Then we can use that in inject instead of loading the protobuf config.

Pothulapati · 2020-09-29T10:35:04Z

@adleong 👍 waiting for the new configMap PR's to merge first! :)

Signed-off-by: Tarun Pothulapati <[email protected]>

adleong · 2020-09-30T17:51:34Z

As we discussed, I think it would be helpful to narrow this PR to just focus on inject or to split the inject portions of this PR into a separate PR.

Signed-off-by: Tarun Pothulapati <[email protected]>

alpeb · 2020-10-01T16:28:31Z

cli/cmd/upgrade_test.go

@@ -202,6 +204,13 @@ func TestUpgradeOverwriteIssuer(t *testing.T) {
 				// Trust root has changed.
 				continue
 			}
+
+			if id == "Deployment/linkerd-identity" || id == "Deployment/linkerd-proxy-injector" {
+				if !pathMatch(diff.path, []string{"spec", "template", "spec", "containers", "*", "args", "*"}) || strings.TrimPrefix(diff.b.(string), "-identity-trust-anchors-pem=") != issuerCerts.ca {


If there's a diff outside the container args then fail

If there's a diff in some of the containers arg, check if that arg's value differs from issuerCerts.ca. What if the diff is not for that particular arg?

alpeb

This looks ok to me, I just had that test question above 👍

Signed-off-by: Tarun Pothulapati <[email protected]>

…omorrow. Signed-off-by: Tarun Pothulapati <[email protected]>

adleong

This looks pretty good.

I notice that we're using partials.FlagIfNotEmpty in most places to skip rendering the flag if it's empty. But, will these values ever be empty? I believe they all should have default values from the chart or are required values (such as the trust anchors PEM). I think it probably makes more sense to assume that all defaulting logic has already been applied by the time rendering happens and that all of these values have been populated. This way we're not spreading the defaulting logic between the values generation and the control plane component flags.

here, most args have some values by default. This was not the case with proxy-injector and hence we went down this route but now that its a separate thing (and probably uses a ConfigMap), these checks are removed. Signed-off-by: Tarun Pothulapati <[email protected]>

Signed-off-by: Tarun Pothulapati <[email protected]>

Pothulapati · 2020-10-02T15:49:18Z

@adleong That makes sense, and most fields used here are not empty. Also, The reason for adding that helper check function was for proxy-injector, as it had fields that could be nil but now it's not needed anymore as that will be a separate PR.

adleong · 2020-10-05T20:47:37Z

go.sum

@@ -259,6 +259,7 @@ github.com/golang/protobuf v1.4.0-rc.1.0.20200221234624-67d41d38c208/go.mod h1:x
 github.com/golang/protobuf v1.4.0-rc.2/go.mod h1:LlEzMj4AhA7rCAGe4KMBDvJI+AwstrUpVNzEA03Pprs=
 github.com/golang/protobuf v1.4.0-rc.4.0.20200313231945-b860323f09d0/go.mod h1:WU3c8KckQ9AFe+yFwt9sWVRKCVIyN9cPHBJSNnbL67w=
 github.com/golang/protobuf v1.4.0/go.mod h1:jodUvKwWbYaEsadDk5Fwe5c77LiNKVO9IDvqG2KuDX0=
+github.com/golang/protobuf v1.4.1 h1:ZFgWrT+bLgsYPirOnRfKLYJLvssAegOj/hgyMFdJZe0=


any reason why this needs to be added?

This is go.sum which is edited/used by only the go compiler, and this was done automatically. probably because some code paths do not depend on the protobuf pkg anymore. Also, Running go mod tidy on this branch removed some stuff.

adleong

I think the change to go.sum can be reverted. Once that is done, I think this is good to go.

Signed-off-by: Tarun Pothulapati <[email protected]>

Pothulapati requested a review from a team as a code owner August 26, 2020 09:32

Remove dependency of linkerd-config for control plane components

4a2acaa

This PR removes the dependency of mounting `linkerd-config` into control plane components by making all that information passed through CLI flags.` Signed-off-by: Tarun Pothulapati <[email protected]>

Pothulapati force-pushed the tarun/cp-no-configmap branch from 9811745 to 4a2acaa Compare August 26, 2020 10:44

adleong reviewed Aug 26, 2020

View reviewed changes

Pothulapati added 4 commits August 27, 2020 11:51

Merge branch 'main' into tarun/cp-no-configmap

08c70b2

update golden test files

a290140

Signed-off-by: Tarun Pothulapati <[email protected]>

fmt fix

2db3d7a

Signed-off-by: Tarun Pothulapati <[email protected]>

Pothulapati force-pushed the tarun/cp-no-configmap branch from db78e6b to 2db3d7a Compare August 27, 2020 08:16

Pothulapati commented Aug 27, 2020

View reviewed changes

Pothulapati added 3 commits August 27, 2020 15:33

fix identity template and field name

9e40900

Signed-off-by: Tarun Pothulapati <[email protected]>

update golden files

ae991bf

Signed-off-by: Tarun Pothulapati <[email protected]>

CLI check, get configMap directly instead of going through publicAPI

827f896

Signed-off-by: Tarun Pothulapati <[email protected]>

adleong reviewed Aug 27, 2020

View reviewed changes

Pothulapati added 11 commits August 28, 2020 20:30

Make proxy injector independent of linkerd-config

28ba177

This commit removes linkerd-config dependency in the proxy-injector and passes all the values through flags Signed-off-by: Tarun Pothulapati <[email protected]>

update go tests and golden files

88884e9

Signed-off-by: Tarun Pothulapati <[email protected]>

fmt fix

d19e045

Signed-off-by: Tarun Pothulapati <[email protected]>

update how webhooks read flags

6a8d8bb

Signed-off-by: Tarun Pothulapati <[email protected]>

update golden test files

025b7c3

Signed-off-by: Tarun Pothulapati <[email protected]>

fix how port ranges are handled

c5b82cd

Signed-off-by: Tarun Pothulapati <[email protected]>

update golden test files

5e61c46

Signed-off-by: Tarun Pothulapati <[email protected]>

provide default value for cniEnabled

e35faa8

Signed-off-by: Tarun Pothulapati <[email protected]>

add useful command flag usage descriptions

6d826a4

Signed-off-by: Tarun Pothulapati <[email protected]>

update go test files

67c36dc

Signed-off-by: Tarun Pothulapati <[email protected]>

nits around help, and error reporting

2eb0e17

Signed-off-by: Tarun Pothulapati <[email protected]>

Pothulapati added 3 commits August 31, 2020 20:13

add a more granular diff check

a7e3178

Signed-off-by: Tarun Pothulapati <[email protected]>

remove Config rpc from public API

89b7935

Signed-off-by: Tarun Pothulapati <[email protected]>

fmt fix

18a075d

Signed-off-by: Tarun Pothulapati <[email protected]>

Pothulapati force-pushed the tarun/cp-no-configmap branch from 6bc818e to 37b5ec6 Compare September 1, 2020 14:43

adleong approved these changes Sep 3, 2020

View reviewed changes

charts/partials/templates/_helpers.tpl Outdated Show resolved Hide resolved

Pothulapati added 4 commits September 4, 2020 18:01

nits around templating functions

25962be

Signed-off-by: Tarun Pothulapati <[email protected]>

Merge branch 'main' into tarun/cp-no-configmap

a36d443

Merge branch 'main' into tarun/cp-no-configmap

2377abd

update golden files as certs were updated

7cce4c3

Signed-off-by: Tarun Pothulapati <[email protected]>

Pothulapati mentioned this pull request Sep 28, 2020

Decode durations from protobuf into golang correctly #5015

Closed

Pothulapati added 3 commits September 30, 2020 14:55

Merge branch 'main' into tarun/cp-no-configmap

6733506

Signed-off-by: Tarun Pothulapati <[email protected]>

fix go tests

5c3ee90

Signed-off-by: Tarun Pothulapati <[email protected]>

trigger build

fcf5d8c

Signed-off-by: Tarun Pothulapati <[email protected]>

Pothulapati added 2 commits October 1, 2020 16:14

Merge branch 'main' into tarun/cp-no-configmap

a15a6b3

Signed-off-by: Tarun Pothulapati <[email protected]>

dont touch proxy injector yet...

e4567b9

Signed-off-by: Tarun Pothulapati <[email protected]>

alpeb reviewed Oct 1, 2020

View reviewed changes

alpeb approved these changes Oct 1, 2020

View reviewed changes

Pothulapati added 2 commits October 1, 2020 22:54

update diff test for PEM arg

2155f6f

Signed-off-by: Tarun Pothulapati <[email protected]>

very low chance of test skipoccurance, but in software that happens t…

e555b6f

…omorrow. Signed-off-by: Tarun Pothulapati <[email protected]>

adleong reviewed Oct 1, 2020

View reviewed changes

Pothulapati added 3 commits October 2, 2020 13:40

Merge branch 'main' into tarun/cp-no-configmap

09046be

Fix config fetch logic

22f6d00

Signed-off-by: Tarun Pothulapati <[email protected]>

adleong reviewed Oct 5, 2020

View reviewed changes

adleong approved these changes Oct 5, 2020

View reviewed changes

go mod tidy

80d658b

Signed-off-by: Tarun Pothulapati <[email protected]>

Pothulapati merged commit 5e774aa into main Oct 6, 2020

Pothulapati deleted the tarun/cp-no-configmap branch October 6, 2020 16:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove dependency of linkerd-config for control plane components #4915

Remove dependency of linkerd-config for control plane components #4915

Pothulapati commented Aug 26, 2020

adleong left a comment

adleong Aug 26, 2020

Pothulapati Aug 31, 2020

adleong commented Aug 26, 2020

Pothulapati Aug 27, 2020

adleong left a comment

adleong Aug 27, 2020

Pothulapati Aug 31, 2020

Pothulapati commented Aug 31, 2020

alpeb commented Sep 3, 2020

adleong commented Sep 28, 2020

Pothulapati commented Sep 29, 2020

adleong commented Sep 30, 2020

alpeb Oct 1, 2020

alpeb left a comment

adleong left a comment

Pothulapati commented Oct 2, 2020

adleong Oct 5, 2020

Pothulapati Oct 6, 2020

adleong left a comment

Remove dependency of linkerd-config for control plane components #4915

Remove dependency of linkerd-config for control plane components #4915

Conversation

Pothulapati commented Aug 26, 2020

adleong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adleong commented Aug 26, 2020

Choose a reason for hiding this comment

adleong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pothulapati commented Aug 31, 2020

alpeb commented Sep 3, 2020

adleong commented Sep 28, 2020

Pothulapati commented Sep 29, 2020

adleong commented Sep 30, 2020

Choose a reason for hiding this comment

alpeb left a comment

Choose a reason for hiding this comment

adleong left a comment

Choose a reason for hiding this comment

Pothulapati commented Oct 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adleong left a comment

Choose a reason for hiding this comment