Update inject to throw an error while injecting non-compliant pods

[mayankshah1607]

Fixes #4214

This PR updates the way inject behaves while attempting to inject non-compliant pods, i.e, pods that have issues related to host network, UDP port, sidecars and automountServiceAccountToken

Now instead of displaying warning messages and writing an uninjected YAML to stdout, inject throws an error. For example :

$ cat cli/cmd/testdata/inject_emojivoto_deployment_hostNetwork_true.input.yml
apiVersion: apps/v1
kind: Deployment
metadata:
  name: web
  namespace: emojivoto
spec:
  replicas: 1
  selector:
    matchLabels:
      app: web-svc
  template:
    metadata:
      labels:
        app: web-svc
    spec:
      containers:
      - env:
        - name: WEB_PORT
          value: "80"
        - name: EMOJISVC_HOST
          value: emoji-svc.emojivoto:8080
        - name: VOTINGSVC_HOST
          value: voting-svc.emojivoto:8080
        - name: INDEX_BUNDLE
          value: dist/index_bundle.js
        image: buoyantio/emojivoto-web:v3
        name: web-svc
        ports:
        - containerPort: 9100
          hostPort: 9100
          name: http
      hostNetwork: true
---
$ cat cli/cmd/testdata/inject_emojivoto_deployment_hostNetwork_true.input.yml | bin/go-run cli inject -

Error transforming resources: failed to inject deployment/web - pods use host networking

[mayankshah1607]

Similarly, when the input has multiple non-compliant pods or a mix of compliant and non-compliant, the errors are accumulated and displayed as a single message and nothing is injected

$ echo bad.yaml | bin/go-run cli inject -

Error transforming resources: failed to inject deployment/web - pods have a 3rd party proxy or initContainer already injected, 
failed to inject deployment/web - pods use host networking

Find bad.yaml here

[mayankshah1607]

Ping @kleimkuhler @alpeb

[alpeb]

Looking good 👍

[alpeb]

Can you also please fix the referred issue number in the description?

[mayankshah1607]

@alpeb Thanks for the tip on wrapped errors - learnt something new! 😄
I've updated the code according to your comments. Had to force push as I ran into some problems while rebasing. But you might want to take a look at this commit in particular to see the new changes w.r.t to the comments on error accumulation mechanism. PTAL!
Thanks 😄

[alpeb]

@mayankshah1607 Thanks for having humored me with the error wrapping suggestion. After having become more comfortable with that concept I realize now it's really meant for adding context to errors and not really for accumulating errors. So I take back that suggestion.
I still think however that processYAML() and throwInjectErrors() should return an array of accumulated errors ([]error). LMKWYT.

[alpeb]

I think you should also reintroduce the logic to properly concatenate the error messages. Currently doing linkerd inject for example on a resources with both hostNetwork and automountServiceAccountToken yields something like this:

Error transforming resources: [failed to inject deployment/emoji - [automountServiceAccountToken set to "false" hostNetwork is enabled]]

[alpeb]

Almost there!

[alpeb]

Given this function is all about inject.Report, I think it's better to move it into report.go and make it a method of that struct. That will also avoid having to repeat these consts.

[mayankshah1607]

@alpeb Done! You can find that refactor in this commit. (Had to rebase against master because some of the CI integration tests didn't seem to run)

[alpeb]

Thanks @mayankshah1607 !

[mayankshah1607]

Pinging @zaharidichev for one more approval! 😄

[ihcsim]

LGTM. Just one minor feedback on the stdout output, and one question on the stderr output.

Also, it looks like we aren't handling the case when the automountServiceAccountToken property is defined in the service account YAML. E.g.,

kind: ServiceAccount
apiVersion: v1
metadata:
  name: emoji
  namespace: emojivoto
automountServiceAccountToken: false

@alpeb @grampelberg do we want an issue to track that?

[ihcsim]

For my own understanding, output writes to stderr, right? But I can't seem to get the CLI to output this error. What am I missing?

[mayankshah1607]

Ah, I think this might be redundant. The CLI will error out before this part of the code is executed.

[ihcsim]

Let's remove it then. Thanks.

[mayankshah1607]

Done 👍

[alpeb]

Also, it looks like we aren't handling the case when the automountServiceAccountToken property is defined in the service account YAML. E.g.,

Ah I didn't think of that... I guess the best we can do is to stop ignoring ServiceAccount manifests during injection and issue a warning whenever we encounter automountServiceAccountToken: false. Not outright failing because the SA wouldn't necessary belong to a workload being injected.

[mayankshah1607]

@alpeb @ihcsim Just to confirm my understanding - we should only error out automountServiceAccountToken: false when the object Kind is not a ServiceAccount, correct?

[ihcsim]

@mayankshah1607 That's correct. I created a separate issue (#4651) to deal with ServiceAccount kind.