Add inject support for namespace configs (Fix #3255)

[mayankshah1607]

Fixes #3255

This PR adds support for linkerd inject to annotate namespaces as follows :
kubectl get ns/default -o yaml | linkerd inject -
Anything added to the said namespace gets linkerd support.

• Add method IsNamespace() to *ResourceConfig to check if a given
  workload if of Kind namespace

• Add method InjectNamespace() to *ResourceConfig: Unmarshals yaml
  to ResourceConfig object, appends given annotations and marshals back
  to yaml. Also supports overrideAnnotations.

• Add field allowNsInject to resourceTransformerInject to allow Ns
  injections only through inject command

• Update method ParseMetaAndYAML() to support namespace kind configs

• Add relevant unit tests (including overridden annotations)

Signed-off-by: Mayank Shah mayankshah1614@gmail.com

[ihcsim]

@mayankshah1607 Good work! Thanks for incorporating the review comments. It's cool to see that linkerd inject can now annotate namespaces to perform auto inject, as well as override proxy configurations.

⚡  k get ns default -oyaml | bin/linkerd inject --proxy-image=gcr.io/linkerd-io/proxy --proxy-version=stable-2.5.0 -
apiVersion: v1
kind: Namespace
metadata:
  annotations:
    config.linkerd.io/proxy-image: gcr.io/linkerd-io/proxy
    config.linkerd.io/proxy-version: stable-2.5.0
    kubectl.kubernetes.io/last-applied-configuration: |
      {"apiVersion":"v1","kind":"Namespace","metadata":{"annotations":{"config.linkerd.io/proxy-image":"gcr.io/linkerd-io/proxy","config.linkerd.io/proxy-version":"stable-2.5.0"},"name":"default"}}
    linkerd.io/inject: enabled
  creationTimestamp: "2019-10-21T18:39:31Z"
  name: default
  resourceVersion: "15588"
  selfLink: /api/v1/namespaces/default
  uid: 6454771e-50b0-4387-8996-772ba0d82de7
spec:
  finalizers:
  - kubernetes
status:
  phase: Active
---

namespace "default" injected

But you shouldn't need to change any of the cli/cmd/testdata/install_* test fixtures. We usually just run go test ./cli/... --update to sync up all existing test fixtures, when the CLI code is changed. Would you mind reverting them, to keep your PR small? Thanks!

[mayankshah1607]

@ihcsim I reverted the changes and let the cli/cmd/testdata/install_* files remain unaffected. However, it seems like some of the tests related to the install_* files are failing.

[ihcsim]

@mayankshah1607 Can you try running go test ./cli/... --update? That should auto-update the test fixtures for you. But make sure you diff them to see the changes are expected, before commiting them.

[mayankshah1607]

@ihcsim Done! I ran go test ./cli/... --update to update the test fixtures. I hope everything is working as expected now :)

[ihcsim]

@mayankshah1607 I was actually surprised by the updates to the install_* test fixtures. It turns out that because the install code calls the inject code, the code change introduced a bug where the linkerd namespace is now annotated with linkerd.io/inject: enabled; the correct annotation is linkerd.io/inject: disabled.

I think the simplest way to solve this is to add a new field to the inject transformer to allow a namespace to be annotated during inject, but not during install. See below diff:

diff --git a/cli/cmd/inject.go b/cli/cmd/inject.go
index 0a2af4a2..7b4100c8 100644
--- a/cli/cmd/inject.go
+++ b/cli/cmd/inject.go
@@ -31,10 +31,11 @@ const (
 )
 
 type resourceTransformerInject struct {
-	injectProxy         bool
-	configs             *cfg.All
-	overrideAnnotations map[string]string
-	enableDebugSidecar  bool
+	allowNamespaceInject bool
+	injectProxy          bool
+	configs              *cfg.All
+	overrideAnnotations  map[string]string
+	enableDebugSidecar   bool
 }
 
 func runInjectCmd(inputs []io.Reader, errWriter, outWriter io.Writer, transformer *resourceTransformerInject) int {
@@ -82,10 +83,11 @@ sub-folders, or coming from stdin.`,
 			options.overrideConfigs(configs, overrideAnnotations)
 
 			transformer := &resourceTransformerInject{
-				injectProxy:         manualOption,
-				configs:             configs,
-				overrideAnnotations: overrideAnnotations,
-				enableDebugSidecar:  enableDebugSidecar,
+				allowNamespaceInject: true,
+				injectProxy:          manualOption,
+				configs:              configs,
+				overrideAnnotations:  overrideAnnotations,
+				enableDebugSidecar:   enableDebugSidecar,
 			}
 			exitCode := uninjectAndInject(in, stderr, stdout, transformer)
 			os.Exit(exitCode)
@@ -148,7 +150,7 @@ func (rt resourceTransformerInject) transform(bytes []byte) ([]byte, []inject.Re
 	}
 	reports := []inject.Report{*report}
 
-	if conf.IsNamespace() {
+	if rt.allowNamespaceInject && conf.IsNamespace() {
 		b, err := conf.InjectNamespace(rt.overrideAnnotations)
 		return b, reports, err
 	}

With this change, I'd expect there are no changes to the install_* test fixtures.

[mayankshah1607]

@ihcsim Thank you for pointing that out. It did feel little weird as to why install_* golden files were catching the annotations. I have made all the required changes as you mentioned. I had to update inject_test to let the namespace annotations come in through tests as well.

The install_* golden files still went through a lot of updates but I don't think any of them are incorrectly annotated anymore! :)

[alpeb]

Thanks @mayankshah1607 , looking good 👍

I think you should also couple this change with an update to linkerd uninject, which should now be removing the annotation from the namespace as well.

Also, could you rename files like cli/cmd/testdata/inject_emojivoto_namespace.good.golden.stderr to cli/cmd/testdata/inject_emojivoto_namespace_good.golden.report to follow the same pattern other existing test fixtures have?

[mayankshah1607]

Sure @alpeb !
Would you want me to open another follow up PR with the uninject changes? As this PR and its related issue (#3255) only talks about the inject side of thigs, so to maybe keep things little more organized

Will make the remaining changes right away!

[alpeb]

@mayankshah1607 Yes, a separate PR for uninject makes sense 👍

[ihcsim]

@mayankshah1607 Thanks again for working on this issue 👍. All the integration tests passed on my machine:

=== PASS: all tests passed

I'll create a separate issue for updating the uninject command.

Also, please fill up this form when you next get the chance, so that we can send you some awesome Linkerd swags!

[ihcsim]

@alpeb Please approve and merge this PR when you next get the chance. Thanks!

[mayankshah1607]

Thank you @ihcsim !

@alpeb I have made the changes mentioned above. I shall open a PR for uninject (#3648 ) once this PR is.merged due to a small dependency between both the PRs.
It would be awesome if you could approve and merge this PR soon!