Repository: zegl/kube-score (go)

Issue:
Feature request: Detect duplicate environment variables defined
Duplicate definitions of environment variables result in unexpected behavior, which we would like to discover.
This issue is reported in the Kubernetes repo but has not yet been fixed.
https://github.com/kubernetes/kubernetes/issues/58477

## Propose behavior

```yaml
# manifest.yaml
apiVersion: apps/v1
kind: Deployment
metadata:
  name: envvars-deployment
  labels:
    app: envvars
  annotations:
    kube-score/ignore: pod-networkpolicy,container-resources,container-image-pull-policy,container-security-context-privileged,container-security-context-user-group-id,container-security-context-readonlyrootfilesystem,container-ephemeral-storage-request-and-limit
spec:
  replicas: 1
  selector:
    matchLabels:
      app: envvars
  template:
    metadata:
      labels:
        app: envvars
    spec:
      containers:
      - name: envvars
        image: busybox:1.34
        command:
        - /bin/sh
        - -c
        - date; env; tail -f /dev/null
        env:
        - name: SAMPLE_ENV_VAR1
          value: sample1
        - name: SAMPLE_ENV_VAR2
          value: sample2
        - name: SAMPLE_ENV_VAR2 # WANT TO DETECT THIS
          value: sample2-duplicated
```

```
$ kube-score score manifest.yaml
apps/v1/Deployment envvars-deployment                                         💥
    [CRITICAL] Pod EnvironmentVariable
        · The pod has duplicated environment variables
            bla bla bla ...
```

Pull request:
Implement feature request: To detect duplicate defined environment variables
score/container: To detect duplicate defined environment variables.
    
Fixes #448

### behaviour

```
$ ./kube-score score ~/test.yaml 
apps/v1/Deployment sample-deployment                                          💥
    [CRITICAL] Environment Variable Key Duplication
        · sampleA -> Environment Variable Key Duplication
            Container environment variable key 'SAMPLE_ENV_VAR' duplicated
        · sampleB -> Environment Variable Key Duplication
            Container environment variable key 'SAMPLE_ENV_VAR1' duplicated
        · sampleB -> Environment Variable Key Duplication
            Container environment variable key 'SAMPLE_ENV_VAR2' duplicated
```

```yaml
apiVersion: apps/v1
kind: Deployment
metadata:
  name: sample-deployment
  labels:
    app: sample
  annotations:
    kube-score/ignore: pod-networkpolicy,container-resources,container-image-pull-policy,container-security-context-privileged,container-security-context-user-group-id,container-security-context-readonlyrootfilesystem,container-ephemeral-storage-request-and-limit,container-image-tag
spec:
  replicas: 1
  selector:
    matchLabels:
      app: sample
  template:
    metadata:
      labels:
        app: sample
    spec:
      containers:
      - name: sampleA
        image: sample:latest
        ports:
        - containerPort: 80
        env:
        - name: SAMPLE_ENV_VAR
          value: sample1
        - name: SAMPLE_ENV_VAR
          value: sample1
      - name: sampleB
        image: sample:latest
        ports:
        - containerPort: 80
        env:
        - name: SAMPLE_ENV_VAR1
          value: sample1
        - name: SAMPLE_ENV_VAR1
          value: sample1
        - name: SAMPLE_ENV_VAR2
          value: sample2
        - name: SAMPLE_ENV_VAR2
          value: sample2
```

Patch:
diff --git a/README_CHECKS.md b/README_CHECKS.md
index adfab29..696d23c 100644
--- a/README_CHECKS.md
+++ b/README_CHECKS.md
@@ -33,3 +33,4 @@
 | statefulset-pod-selector-labels-match-template-metadata-labels | StatefulSet | Ensure the StatefulSet selector labels match the template metadata labels. | default |
 | label-values | all | Validates label values | default |
 | horizontalpodautoscaler-has-target | HorizontalPodAutoscaler | Makes sure that the HPA targets a valid object | default |
+| environment-variable-key-duplication | Pod | Makes sure that no duplicated environment variable keys. | default |
diff --git a/go.mod b/go.mod
index 1e6889e..059c8ed 100644
--- a/go.mod
+++ b/go.mod
@@ -3,6 +3,7 @@ module github.com/zegl/kube-score
 require (
 	github.com/eidolon/wordwrap v0.0.0-20161011182207-e0f54129b8bb
 	github.com/fatih/color v1.13.0
+	github.com/google/go-cmp v0.5.7
 	github.com/spf13/pflag v1.0.5
 	github.com/stretchr/testify v1.7.1
 	golang.org/x/crypto v0.0.0-20201216223049-8b5274cf687f
diff --git a/go.sum b/go.sum
index e14ba31..f437230 100644
--- a/go.sum
+++ b/go.sum
@@ -51,8 +51,9 @@ github.com/google/go-cmp v0.2.0/go.mod h1:oXzfMopK8JAjlY9xF4vHSVASa0yLyX7SntLO5a
 github.com/google/go-cmp v0.3.0/go.mod h1:8QqcDgzrUqlUb/G2PQTWiueGozuR1884gddMywk6iLU=
 github.com/google/go-cmp v0.3.1/go.mod h1:8QqcDgzrUqlUb/G2PQTWiueGozuR1884gddMywk6iLU=
 github.com/google/go-cmp v0.4.0/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE=
-github.com/google/go-cmp v0.5.5 h1:Khx7svrCpmxxtHBq5j2mp/xVjsi8hQMfNLvJFAlrGgU=
 github.com/google/go-cmp v0.5.5/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE=
+github.com/google/go-cmp v0.5.7 h1:81/ik6ipDQS2aGcBfIN5dHDB36BwrStyeAQquSYCV4o=
+github.com/google/go-cmp v0.5.7/go.mod h1:n+brtR0CgQNWTVd5ZUFpTBC8YFBDLK/h/bpaJ8/DtOE=
 github.com/google/gofuzz v1.0.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg=
 github.com/google/gofuzz v1.1.0 h1:Hsa8mG0dQ46ij8Sl2AYJDUv1oA9/d6Vk+3LG99Oe02g=
 github.com/google/gofuzz v1.1.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg=
diff --git a/score/container/container.go b/score/container/container.go
index 1bc0834..374b459 100644
--- a/score/container/container.go
+++ b/score/container/container.go
@@ -1,6 +1,7 @@
 package container
 
 import (
+	"fmt"
 	"strings"
 
 	"github.com/zegl/kube-score/config"
@@ -20,6 +21,7 @@ func Register(allChecks *checks.Checks, cnf config.Configuration) {
 	allChecks.RegisterPodCheck("Container Ephemeral Storage Request and Limit", "Makes sure all pods have ephemeral-storage requests and limits set", containerStorageEphemeralRequestAndLimit)
 	allChecks.RegisterOptionalPodCheck("Container Ephemeral Storage Request Equals Limit", "Make sure all pods have matching ephemeral-storage requests and limits", containerStorageEphemeralRequestEqualsLimit)
 	allChecks.RegisterOptionalPodCheck("Container Ports Check", "Container Ports Checks", containerPortsCheck)
+	allChecks.RegisterPodCheck("Environment Variable Key Duplication", "Makes sure that duplicated environment variable keys are not duplicated", environmentVariableKeyDuplication)
 }
 
 // containerResources makes sure that the container has resource requests and limits set
@@ -280,3 +282,27 @@ func containerPortsCheck(podTemplate corev1.PodTemplateSpec, typeMeta metav1.Typ
 
 	return
 }
+
+// environmentVariableKeyDuplication checks that no duplicated environment variable keys.
+func environmentVariableKeyDuplication(podTemplate corev1.PodTemplateSpec, _ metav1.TypeMeta) (score scorecard.TestScore) {
+	pod := podTemplate.Spec
+
+	allContainers := pod.InitContainers
+	allContainers = append(allContainers, pod.Containers...)
+
+	score.Grade = scorecard.GradeAllOK
+
+	for _, container := range allContainers {
+		envs := make(map[string]struct{})
+		for _, env := range container.Env {
+			if _, duplicated := envs[env.Name]; duplicated {
+				msg := fmt.Sprintf("Container environment variable key '%s' is duplicated", env.Name)
+				score.AddComment(container.Name, "Environment Variable Key Duplication", msg)
+				score.Grade = scorecard.GradeCritical
+				continue
+			}
+			envs[env.Name] = struct{}{}
+		}
+	}
+	return
+}


Test patch:
diff --git a/score/score_test.go b/score/score_test.go
index 8282b02..016bf37 100644
--- a/score/score_test.go
+++ b/score/score_test.go
@@ -4,6 +4,7 @@ import (
 	"os"
 	"testing"
 
+	"github.com/google/go-cmp/cmp"
 	"github.com/stretchr/testify/assert"
 	"github.com/zegl/kube-score/config"
 	ks "github.com/zegl/kube-score/domain"
@@ -373,3 +374,42 @@ func TestPodContainerPortsOK(t *testing.T) {
 		EnabledOptionalTests: structMap,
 	}, "Container Ports Check", scorecard.GradeAllOK)
 }
+
+func TestPodEnvOK(t *testing.T) {
+	t.Parallel()
+
+	structMap := make(map[string]struct{})
+	structMap["environment-variable-key-duplication"] = struct{}{}
+
+	testExpectedScoreWithConfig(t, config.Configuration{
+		AllFiles:             []ks.NamedReader{testFile("pod-env-ok.yaml")},
+		EnabledOptionalTests: structMap,
+	}, "Environment Variable Key Duplication", scorecard.GradeAllOK)
+}
+
+func TestPodEnvDuplicated(t *testing.T) {
+	t.Parallel()
+
+	structMap := make(map[string]struct{})
+	structMap["environment-variable-key-duplication"] = struct{}{}
+
+	actual := testExpectedScoreWithConfig(t, config.Configuration{
+		AllFiles:             []ks.NamedReader{testFile("pod-env-duplicated.yaml")},
+		EnabledOptionalTests: structMap,
+	}, "Environment Variable Key Duplication", scorecard.GradeCritical)
+
+	expected := []scorecard.TestScoreComment{
+		{
+			Path:        "foobar",
+			Summary:     "Environment Variable Key Duplication",
+			Description: "Container environment variable key 'bar' is duplicated",
+		},
+		{
+			Path:        "foobar",
+			Summary:     "Environment Variable Key Duplication",
+			Description: "Container environment variable key 'baz' is duplicated",
+		},
+	}
+	diff := cmp.Diff(expected, actual)
+	assert.Empty(t, diff)
+}
diff --git a/score/testdata/pod-env-duplicated.yaml b/score/testdata/pod-env-duplicated.yaml
new file mode 100644
index 0000000..681e60e
--- /dev/null
+++ b/score/testdata/pod-env-duplicated.yaml
@@ -0,0 +1,19 @@
+apiVersion: v1
+kind: Pod
+metadata:
+  name: pod-test-1
+spec:
+  containers:
+  - name: foobar
+    image: foo/bar:latest
+    env:
+    - name: foo
+      value: foo
+    - name: bar
+      value: bar
+    - name: bar
+      value: bar
+    - name: baz
+      value: baz
+    - name: baz
+      value: baz
diff --git a/score/testdata/pod-env-ok.yaml b/score/testdata/pod-env-ok.yaml
new file mode 100644
index 0000000..c6ec84e
--- /dev/null
+++ b/score/testdata/pod-env-ok.yaml
@@ -0,0 +1,13 @@
+apiVersion: v1
+kind: Pod
+metadata:
+  name: pod-test-1
+spec:
+  containers:
+  - name: foobar
+    image: foo/bar:latest
+    env:
+    - name: foo
+      value: foo
+    - name: bar
+      value: bar