Handle incompatible automount token setting in service account YAML (#4651)

[han-so1omon]

Handle incompatible automount token setting in service account YAML (#4651)

Following #4145 the inject command will return an error with pods YAML that has the automountServiceAccountToken property set to false. The same validation should be applied to service accounts YAML. E.g., linkerd inject should return an error with the following YAML:

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

Now instead of skipping service account YAML, inject throws an error when automountServiceAccountToken is false. Using the YAML above:

$ cat cli/cmd/testdata/inject_emojivoto_serviceAccount_automountServiceAccountToken_false.input.yml
Error transforming resources: failed to inject
serviceaccount/emoji: automountServiceAccountToken set to "false"

Fixes #4651

[Pothulapati]

Hey @han-so1omon, Thanks for working on this! The inject test seems to fail because we were trying to de-reference a nil value which can be fixed by having a conditional check as in

diff --git a/pkg/inject/inject.go b/pkg/inject/inject.go
index 3eb29056..b17373c8 100644
--- a/pkg/inject/inject.go
+++ b/pkg/inject/inject.go
@@ -455,7 +455,9 @@ func (conf *ResourceConfig) parse(bytes []byte) error {

                conf.workload.obj = v
                conf.workload.Meta = &v.ObjectMeta
-               conf.workload.saAutomountServiceAccountToken = *v.AutomountServiceAccountToken
+               if v.AutomountServiceAccountToken != nil {
+                       conf.workload.saAutomountServiceAccountToken = *v.AutomountServiceAccountToken
+               }

but doing the above means that we would be setting the default value as false which was causing other problems. So, Rather than doing that we can store the pointer itself so that we can check for nil value directly as in https://github.com/linkerd/linkerd2/blob/main/pkg/inject/report.go#L93

diff --git a/pkg/inject/inject.go b/pkg/inject/inject.go
index 3eb29056..219bb6a6 100644
--- a/pkg/inject/inject.go
+++ b/pkg/inject/inject.go
@@ -111,7 +111,7 @@ type ResourceConfig struct {
                // saAutomountServiceAccountToken is a boolean used to capture a
                // ServiceAccount configuration. It is a top-level configuration
                // variable and cannot otherwise be captured by Meta or by pod/spec
-               saAutomountServiceAccountToken bool
+               saAutomountServiceAccountToken *bool

                ownerRef *metav1.OwnerReference
        }
@@ -455,7 +455,7 @@ func (conf *ResourceConfig) parse(bytes []byte) error {

                conf.workload.obj = v
                conf.workload.Meta = &v.ObjectMeta
-               conf.workload.saAutomountServiceAccountToken = *v.AutomountServiceAccountToken
+               conf.workload.saAutomountServiceAccountToken = v.AutomountServiceAccountToken

        case *corev1.Pod:
                if err := yaml.Unmarshal(bytes, v); err != nil {
diff --git a/pkg/inject/report.go b/pkg/inject/report.go
index df069b9c..41045e26 100644
--- a/pkg/inject/report.go
+++ b/pkg/inject/report.go
@@ -95,7 +95,9 @@ func newReport(conf *ResourceConfig) *Report {
                }
        } else if report.Kind == k8s.ServiceAccount {
                report.UnsupportedResource = true
-               report.AutomountServiceAccountToken = conf.workload.saAutomountServiceAccountToken
+               if conf.workload.saAutomountServiceAccountToken != nil {
+                       report.AutomountServiceAccountToken = *conf.workload.saAutomountServiceAccountToken
+               }
        } else if report.Kind != k8s.Namespace {
                report.UnsupportedResource = true
        }

[han-so1omon]

@Pothulapati good point. I'll implement that today.

[han-so1omon]

@ihcsim @mayankshah1607 #4651 review this please

[cpretzer]

@han-so1omon the changes look good, thanks for submitting this PR.

Would you mind amending your commit to include a signature for the DCO? The contribution details for the DCO are here and you can read the statement here

[mayankshah1607]

Following @alpeb 's comment - #4346 (comment) , do we want to issue a warning in this case instead of outright failing it?

[han-so1omon]

Following @alpeb 's comment - #4346 (comment) , do we want to issue a warning in this case instead of outright failing it?

@mayankshah1607 @alpeb

Do you think it is the responsibility of linkerd to validate unrelated service accounts for this commit? Since I do not see a simple way to perform a full validation of service accounts given the current linkerd inject method, I see two options:

1. Claim responsibility for validation of all service accounts through linkerd inject
2. Claim responsibility for validation of only linkerd workload-owned service accounts through linkerd inject.

For automountServiceAccountToken: false, if we choose 1), then we can issue is a warning. If we choose 2), then we can issue a failure.

With 2), I think that it is more clear that it is the responsibility of the program owner to correctly configure service accounts. If they will validate a service account with linkerd inject, then that account will only pass if is valid and usable within linkerd. Else, they can choose to apply a service account to k8s with another tool. With 1), linkerd inject may pass an invalid configuration.

[adleong]

Hey @han-so1omon, I haven't looked the code in your PR but I did have some thoughts on the underlying issue that will probably affect how we want to proceed here. Take a look at #4651 (comment) and let me know what you think.

[han-so1omon]

@adleong I've responded in the thread you linked. Maybe switch this to a warning for now as suggested by @mayankshah1607 and @alpeb ? In the meantime of a more useful fix, a warning could be helpful.