Forbid duplicate keys in strict mode (#121)

* Forbid duplicate keys in strict mode Prevents specifying duplicate keys using UnmarshallStrict: https://pkg.go.dev/gopkg.in/yaml.v2#UnmarshalStrict * Add acceptance tests for duplicated properties
yannh · Jul 15, 2022 · 9a6fff1 · 9a6fff1
1 parent 7bf1e01
commit 9a6fff1
Show file tree

Hide file tree

Showing 8 changed files with 109 additions and 8 deletions.
diff --git a/Readme.md b/Readme.md
@@ -98,7 +98,7 @@ Usage: ./bin/kubeconform [OPTION]... [FILE OR FOLDER]...
   -skip string
         comma-separated list of kinds to ignore
   -strict
-        disallow additional properties not in schema
+        disallow additional properties not in schema or duplicated keys
   -summary
         print a summary at the end (ignored for junit output)
   -v	show version information

diff --git a/acceptance.bats b/acceptance.bats
@@ -138,6 +138,16 @@ resetCacheFolder() {
   [ "$status" -eq 1 ]
 }
 
+@test "Fail when parsing a config with duplicate properties and strict set" {
+  run bin/kubeconform -strict -kubernetes-version 1.16.0 fixtures/duplicate_property.yaml
+  [ "$status" -eq 1 ]
+}
+
+@test "Pass when parsing a config with duplicate properties and strict NOT set" {
+  run bin/kubeconform -kubernetes-version 1.16.0 fixtures/duplicate_property.yaml
+  [ "$status" -eq 0 ]
+}
+
 @test "Pass when using a valid, preset -schema-location" {
   run bin/kubeconform -schema-location default fixtures/valid.yaml
   [ "$status" -eq 0 ]

diff --git a/fixtures/duplicate_property.yaml b/fixtures/duplicate_property.yaml
@@ -0,0 +1,18 @@
+---
+apiVersion: apps/v1
+kind: DaemonSet
+metadata:
+  name: nginx-ds
+spec:
+  replicas: 2
+  selector:
+    matchLabels:
+      k8s-app: nginx-ds
+  template:
+    spec:
+      containers:
+      - image: envoy
+        name: envoy
+      containers:
+      - image: nginx
+        name: nginx
diff --git a/pkg/config/config.go b/pkg/config/config.go
@@ -72,7 +72,7 @@ func FromFlags(progName string, args []string) (Config, string, error) {
 	flags.Var(&ignoreFilenamePatterns, "ignore-filename-pattern", "regular expression specifying paths to ignore (can be specified multiple times)")
 	flags.BoolVar(&c.Summary, "summary", false, "print a summary at the end (ignored for junit output)")
 	flags.IntVar(&c.NumberOfWorkers, "n", 4, "number of goroutines to run concurrently")
-	flags.BoolVar(&c.Strict, "strict", false, "disallow additional properties not in schema")
+	flags.BoolVar(&c.Strict, "strict", false, "disallow additional properties not in schema or duplicated keys")
 	flags.StringVar(&c.OutputFormat, "output", "text", "output format - json, junit, tap, text")
 	flags.BoolVar(&c.Verbose, "verbose", false, "print results for all resources (ignored for tap and junit output)")
 	flags.BoolVar(&c.SkipTLS, "insecure-skip-tls-verify", false, "disable verification of the server's SSL certificate. This will make your HTTPS connections insecure")

diff --git a/pkg/validator/validator.go b/pkg/validator/validator.go
@@ -112,7 +112,12 @@ func (val *v) ValidateResource(res resource.Resource) Result {
 	}
 
 	var r map[string]interface{}
-	if err := yaml.Unmarshal(res.Bytes, &r); err != nil {
+	unmarshaller := yaml.Unmarshal
+	if val.opts.Strict {
+		unmarshaller = yaml.UnmarshalStrict
+	}
+
+	if err := unmarshaller(res.Bytes, &r); err != nil {
 		return Result{Resource: res, Status: Error, Err: fmt.Errorf("error unmarshalling resource: %s", err)}
 	}
 

diff --git a/pkg/validator/validator_test.go b/pkg/validator/validator_test.go
@@ -27,6 +27,7 @@ func TestValidate(t *testing.T) {
 		rawResource, schemaRegistry1 []byte
 		schemaRegistry2              []byte
 		ignoreMissingSchema          bool
+		strict                       bool
 		expect                       Status
 	}{
 		{
@@ -60,6 +61,7 @@ lastName: bar
 }`),
 			nil,
 			false,
+			false,
 			Valid,
 		},
 		{
@@ -93,6 +95,7 @@ lastName: bar
 }`),
 			nil,
 			false,
+			false,
 			Invalid,
 		},
 		{
@@ -125,8 +128,61 @@ firstName: foo
 }`),
 			nil,
 			false,
+			false,
 			Invalid,
 		},
+		{
+			"key \"firstName\" already set in map",
+			[]byte(`
+kind: name
+apiVersion: v1
+firstName: foo
+firstName: bar
+`),
+			[]byte(`{
+  "title": "Example Schema",
+  "type": "object",
+  "properties": {
+    "kind": {
+      "type": "string"
+    },
+    "firstName": {
+      "type": "string"
+    }
+  },
+  "required": ["firstName"]
+}`),
+			nil,
+			false,
+			true,
+			Error,
+		},
+		{
+			"key firstname already set in map in non-strict mode",
+			[]byte(`
+kind: name
+apiVersion: v1
+firstName: foo
+firstName: bar
+`),
+			[]byte(`{
+  "title": "Example Schema",
+  "type": "object",
+  "properties": {
+    "kind": {
+      "type": "string"
+    },
+    "firstName": {
+      "type": "string"
+    }
+  },
+  "required": ["firstName"]
+}`),
+			nil,
+			false,
+			false,
+			Valid,
+		},
 		{
 			"resource has invalid yaml",
 			[]byte(`
@@ -161,6 +217,7 @@ lastName: bar
 }`),
 			nil,
 			false,
+			false,
 			Error,
 		},
 		{
@@ -196,6 +253,7 @@ lastName: bar
   },
   "required": ["firstName", "lastName"]
 }`),
+			false,
 			false,
 			Valid,
 		},
@@ -232,6 +290,7 @@ lastName: bar
  },
  "required": ["firstName", "lastName"]
 }`),
+			false,
 			false,
 			Valid,
 		},
@@ -246,6 +305,7 @@ lastName: bar
 			nil,
 			nil,
 			true,
+			false,
 			Skipped,
 		},
 		{
@@ -259,6 +319,7 @@ lastName: bar
 			nil,
 			nil,
 			false,
+			false,
 			Error,
 		},
 		{
@@ -272,6 +333,7 @@ lastName: bar
 			[]byte(`<html>error page</html>`),
 			[]byte(`<html>error page</html>`),
 			true,
+			false,
 			Skipped,
 		},
 		{
@@ -285,6 +347,7 @@ lastName: bar
 			[]byte(`<html>error page</html>`),
 			[]byte(`<html>error page</html>`),
 			false,
+			false,
 			Error,
 		},
 	} {
@@ -293,6 +356,7 @@ lastName: bar
 				SkipKinds:            map[string]struct{}{},
 				RejectKinds:          map[string]struct{}{},
 				IgnoreMissingSchemas: testCase.ignoreMissingSchema,
+				Strict:               testCase.strict,
 			},
 			schemaCache:    nil,
 			schemaDownload: downloadSchema,
@@ -306,7 +370,11 @@ lastName: bar
 			},
 		}
 		if got := val.ValidateResource(resource.Resource{Bytes: testCase.rawResource}); got.Status != testCase.expect {
-			t.Errorf("%d - expected %d, got %d: %s", i, testCase.expect, got.Status, got.Err.Error())
+			if got.Err != nil {
+				t.Errorf("%d - expected %d, got %d: %s", i, testCase.expect, got.Status, got.Err.Error())
+			} else {
+				t.Errorf("%d - expected %d, got %d", i, testCase.expect, got.Status)
+			}
 		}
 	}
 }
diff --git a/site/content/docs/usage.md b/site/content/docs/usage.md
@@ -34,7 +34,7 @@ Usage: ./bin/kubeconform [OPTION]... [FILE OR FOLDER]...
   -skip string
         comma-separated list of kinds to ignore
   -strict
-        disallow additional properties not in schema
+        disallow additional properties not in schema or duplicated keys
   -summary
         print a summary at the end (ignored for junit output)
   -v	show version information
@@ -83,4 +83,4 @@ fixtures/crd_schema.yaml - CustomResourceDefinition trainingjobs.sagemaker.aws.a
 fixtures/invalid.yaml - ReplicationController bob is invalid: Invalid type. Expected: [integer,null], given: string
 [...]
 Summary: 65 resources found in 34 files - Valid: 55, Invalid: 2, Errors: 8 Skipped: 0
-{{< /prism >}}
+{{< /prism >}}
diff --git a/site/public/docs/usage/index.html b/site/public/docs/usage/index.html
@@ -59,7 +59,7 @@ <h1>Usage</h1>
   -skip string
         comma-separated list of kinds to ignore
   -strict
-        disallow additional properties not in schema
+        disallow additional properties not in schema or duplicated keys
   -summary
         print a summary at the end (ignored for junit output)
   -v	show version information
@@ -117,4 +117,4 @@ <h3 id=validating-a-folder-increasing-the-number-of-parallel-workers>Validating
 </div>
 <script defer src=/js/prism.js></script>
 </body>
-</html>
+</html>