Repository: FairwindsOps/polaris
Pull request #726 (2022-03-17 17:02:04)

Change `target: Pod` to `target: PodSpec`
**breaking change**

## Checklist
* [x] I have signed the CLA
* [x] I have updated/added any relevant documentation

## Description
### What's the goal of this PR?
We currently use the `target` field a few ways:
* You can specify a `Kind` (e.g. `Deployment`, `Ingress`, `PodDisruptionBudget`), and the schema will apply to the top-level of that resource
* You can specify `Controller`, and the check will match anything that creates Pods (e.g. `Deployments`, `StatefulSets`, and even CRDs like `Prometheus` or `Rollout`). The schema will apply to the top-level resource.
* You can specify `Pod`, and the check will match the same way as `Controller`, but the schema will apply to the `podSpec` defined inside the controller (e.g. `spec.template.spec` inside a `Deployment`)
* You can specify `Container`, and the check will match the same way as `Controller`, but the schema will apply to the `containers` within the relevant podSpec (e.g. `spec.template.spec.containers[*]` inside a `Deployment`

The problem here is that there's no way to target `Pod` as a Kind (case 1) since it gets a special handler (case 3). `Pod` is also not entirely accurate, since the check applies to the pod _spec_, not the top-level pod object.

### What changes did you make?
This PR replaces the special target `Pod` with `PodSpec`. Anyone with custom checks that have `target: Pod` or `schemaTarget: Pod` will need to update.

### What alternative solution should we consider, if any?
We should consider also adding a `PodTemplate` target



fairwinds-insights[bot]: ## Fairwinds Insights CI Report
> [View the Full Report](https://insights.fairwinds.com/gh/FairwindsOps/polaris/rb%2Fpod-spec-target)

:white_check_mark: No new Action Items detected!

Patch:
diff --git a/checks/hostIPCSet.yaml b/checks/hostIPCSet.yaml
index 7a3607e1..88845c89 100644
--- a/checks/hostIPCSet.yaml
+++ b/checks/hostIPCSet.yaml
@@ -1,7 +1,7 @@
 successMessage: Host IPC is not configured
 failureMessage: Host IPC should not be configured
 category: Security
-target: Pod
+target: PodSpec
 schema:
   '$schema': http://json-schema.org/draft-07/schema
   type: object
diff --git a/checks/hostNetworkSet.yaml b/checks/hostNetworkSet.yaml
index 29965475..53728aa1 100644
--- a/checks/hostNetworkSet.yaml
+++ b/checks/hostNetworkSet.yaml
@@ -1,7 +1,7 @@
 successMessage: Host network is not configured
 failureMessage: Host network should not be configured
 category: Security
-target: Pod
+target: PodSpec
 schema:
   '$schema': http://json-schema.org/draft-07/schema
   type: object
diff --git a/checks/hostPIDSet.yaml b/checks/hostPIDSet.yaml
index 48336f37..fe1f5c7b 100644
--- a/checks/hostPIDSet.yaml
+++ b/checks/hostPIDSet.yaml
@@ -1,7 +1,7 @@
 successMessage: Host PID is not configured
 failureMessage: Host PID should not be configured
 category: Security
-target: Pod
+target: PodSpec
 schema:
   '$schema': http://json-schema.org/draft-07/schema
   type: object
diff --git a/checks/notReadOnlyRootFilesystem.yaml b/checks/notReadOnlyRootFilesystem.yaml
index 54152900..d614089d 100644
--- a/checks/notReadOnlyRootFilesystem.yaml
+++ b/checks/notReadOnlyRootFilesystem.yaml
@@ -2,7 +2,7 @@ successMessage: Filesystem is read only
 failureMessage: Filesystem should be read only
 category: Security
 target: Container
-schemaTarget: Pod
+schemaTarget: PodSpec
 schema:
   '$schema': http://json-schema.org/draft-07/schema
   definitions:
diff --git a/checks/priorityClassNotSet.yaml b/checks/priorityClassNotSet.yaml
index 49690027..dc9ee056 100644
--- a/checks/priorityClassNotSet.yaml
+++ b/checks/priorityClassNotSet.yaml
@@ -1,7 +1,7 @@
 successMessage: Priority class has been set
 failureMessage: Priority class should be set
 category: Security
-target: Pod
+target: PodSpec
 schema:
   '$schema': http://json-schema.org/draft-07/schema
   type: object
diff --git a/checks/privilegeEscalationAllowed.yaml b/checks/privilegeEscalationAllowed.yaml
index 8d2933d9..396f93c8 100644
--- a/checks/privilegeEscalationAllowed.yaml
+++ b/checks/privilegeEscalationAllowed.yaml
@@ -2,7 +2,7 @@ successMessage: Privilege escalation not allowed
 failureMessage: Privilege escalation should not be allowed
 category: Security
 target: Container
-schemaTarget: Pod
+schemaTarget: PodSpec
 schema:
   '$schema': http://json-schema.org/draft-07/schema
   definitions:
diff --git a/checks/runAsPrivileged.yaml b/checks/runAsPrivileged.yaml
index d641c752..fc4e59e6 100644
--- a/checks/runAsPrivileged.yaml
+++ b/checks/runAsPrivileged.yaml
@@ -2,7 +2,7 @@ successMessage: Not running as privileged
 failureMessage: Should not be running as privileged
 category: Security
 target: Container
-schemaTarget: Pod
+schemaTarget: PodSpec
 schema:
   '$schema': http://json-schema.org/draft-07/schema
   definitions:
diff --git a/checks/runAsRootAllowed.yaml b/checks/runAsRootAllowed.yaml
index a56fe02c..7039820f 100644
--- a/checks/runAsRootAllowed.yaml
+++ b/checks/runAsRootAllowed.yaml
@@ -2,7 +2,7 @@ successMessage: Is not allowed to run as root
 failureMessage: Should not be allowed to run as root
 category: Security
 target: Container
-schemaTarget: Pod
+schemaTarget: PodSpec
 schema:
   '$schema': http://json-schema.org/draft-07/schema
   definitions:
diff --git a/docs/customization/custom-checks.md b/docs/customization/custom-checks.md
index eac66d85..45b3742a 100644
--- a/docs/customization/custom-checks.md
+++ b/docs/customization/custom-checks.md
@@ -43,8 +43,8 @@ check ID. Note that you'll also have to set its severity in the `checks` section
 * `category` - one of `Security`, `Efficiency`, or `Reliability`
 * `target` - specifies the type of resource to check. This can be:
   * a group and kind, e.g. `apps/Deployment` or `networking.k8s.io/Ingress`
-  * `Controller`, to check _any_ resource that contains a pod spec (e.g. Deployments, CronJobs, StatefulSets), as well as naked Pods
-  * `Pod`, same as `Controller`, but the schema applies to the Pod spec rather than the top-level controller
+  * `Controller`, to check _any_ resource that creates Pods (e.g. Deployments, CronJobs, StatefulSets), as well as naked Pods
+  * `PodSpec`, same as `Controller`, but the schema applies to the Pod spec rather than the top-level controller
   * `Container` same as `Controller`, but the schema applies to all Container specs rather than the top-level controller
 * `controllers` - if `target` is `Controller`, `Pod` or `Container`, you can use this to change which types of controllers are checked
 * `controllers.include` - _only_ check these controllers
diff --git a/pkg/config/schema.go b/pkg/config/schema.go
index b80887d5..c9623dfb 100644
--- a/pkg/config/schema.go
+++ b/pkg/config/schema.go
@@ -39,15 +39,15 @@ const (
 	TargetController TargetKind = "Controller"
 	// TargetContainer points to the container spec
 	TargetContainer TargetKind = "Container"
-	// TargetPod points to the pod spec
-	TargetPod TargetKind = "Pod"
+	// TargetPodSpec points to the pod spec
+	TargetPodSpec TargetKind = "PodSpec"
 )
 
 // HandledTargets is a list of target names that are explicitly handled
 var HandledTargets = []TargetKind{
 	TargetController,
 	TargetContainer,
-	TargetPod,
+	TargetPodSpec,
 }
 
 // MutationComment is the comments added to a mutated file
@@ -253,8 +253,8 @@ func (check SchemaCheck) TemplateForResource(res interface{}) (*SchemaCheck, err
 	return &newCheck, err
 }
 
-// CheckPod checks a pod spec against the schema
-func (check SchemaCheck) CheckPod(pod *corev1.PodSpec) (bool, []jsonschema.ValError, error) {
+// CheckPodSpec checks a pod spec against the schema
+func (check SchemaCheck) CheckPodSpec(pod *corev1.PodSpec) (bool, []jsonschema.ValError, error) {
 	return check.CheckObject(pod)
 }
 
diff --git a/pkg/validator/schema.go b/pkg/validator/schema.go
index 1056101b..258819ac 100644
--- a/pkg/validator/schema.go
+++ b/pkg/validator/schema.go
@@ -15,6 +15,7 @@
 package validator
 
 import (
+	"errors"
 	"fmt"
 	"sort"
 	"strconv"
@@ -165,6 +166,17 @@ func applyControllerSchemaChecks(conf *config.Configuration, resourceProvider *k
 	}
 	finalResult.Results = resultSet
 
+	nonControllerResults, err := applyTopLevelSchemaChecks(conf, resourceProvider, resource, false)
+	if err != nil {
+		return finalResult, err
+	}
+	for key, val := range nonControllerResults {
+		if _, ok := finalResult.Results[key]; ok {
+			return finalResult, errors.New("Duplicate finding for check " + key)
+		}
+		finalResult.Results[key] = val
+	}
+
 	podRS, err := applyPodSchemaChecks(conf, resourceProvider, resource)
 	if err != nil {
 		return finalResult, err
@@ -214,7 +226,7 @@ func applyTopLevelSchemaChecks(conf *config.Configuration, resources *kube.Resou
 
 func applyPodSchemaChecks(conf *config.Configuration, resources *kube.ResourceProvider, controller kube.GenericResource) (ResultSet, error) {
 	test := schemaTestCase{
-		Target:           config.TargetPod,
+		Target:           config.TargetPodSpec,
 		ResourceProvider: resources,
 		Resource:         controller,
 	}
@@ -258,7 +270,7 @@ func applySchemaCheck(conf *config.Configuration, checkID string, test schemaTes
 	var issues []jsonschema.ValError
 	var prefix string
 	if check.SchemaTarget != "" {
-		if check.SchemaTarget == config.TargetPod && check.Target == config.TargetContainer {
+		if check.SchemaTarget == config.TargetPodSpec && check.Target == config.TargetContainer {
 			podCopy := *test.Resource.PodSpec
 			podCopy.InitContainers = []corev1.Container{}
 			podCopy.Containers = []corev1.Container{*test.Container}
@@ -269,12 +281,12 @@ func applySchemaCheck(conf *config.Configuration, checkID string, test schemaTes
 			if prefix != "" {
 				prefix += "/containers/" + strconv.Itoa(containerIndex)
 			}
-			passes, issues, err = check.CheckPod(&podCopy)
+			passes, issues, err = check.CheckPodSpec(&podCopy)
 		} else {
 			return nil, fmt.Errorf("Unknown combination of target (%s) and schema target (%s)", check.Target, check.SchemaTarget)
 		}
-	} else if check.Target == config.TargetPod {
-		passes, issues, err = check.CheckPod(test.Resource.PodSpec)
+	} else if check.Target == config.TargetPodSpec {
+		passes, issues, err = check.CheckPodSpec(test.Resource.PodSpec)
 		prefix = getJSONSchemaPrefix(test.Resource.Kind)
 	} else if check.Target == config.TargetContainer {
 		containerIndex := funk.IndexOf(test.Resource.PodSpec.Containers, func(value corev1.Container) bool {


Test patch:
diff --git a/test/checks/nakedPod/check.yaml b/test/checks/nakedPod/check.yaml
new file mode 100644
index 00000000..066889d6
--- /dev/null
+++ b/test/checks/nakedPod/check.yaml
@@ -0,0 +1,17 @@
+successMessage: example FP success
+failureMessage: example FP fail
+target: Pod
+schema:
+  '$schema': http://json-schema.org/draft-07/schema
+  type: object
+  properties:
+    metadata:
+      type: object
+      required: ["labels"]
+      properties:
+        labels:
+          type: object
+          required: ["app.kubernetes.io/name"]
+          properties:
+            app.kubernetes.io/name:
+              const: "{{ .metadata.name }}"
diff --git a/test/checks/nakedPod/failure.yaml b/test/checks/nakedPod/failure.yaml
new file mode 100644
index 00000000..b89334be
--- /dev/null
+++ b/test/checks/nakedPod/failure.yaml
@@ -0,0 +1,13 @@
+apiVersion: v1
+kind: Pod
+metadata:
+  name: nginx
+  labels:
+    app.kubernetes.io/name: nginxIDONTMATCH
+spec:
+  containers:
+  - name: nginx
+    image: nginx
+    resources:
+      limits:
+        memory: "512Mi"
diff --git a/test/checks/nakedPod/success.yaml b/test/checks/nakedPod/success.yaml
new file mode 100644
index 00000000..63ed1f65
--- /dev/null
+++ b/test/checks/nakedPod/success.yaml
@@ -0,0 +1,13 @@
+apiVersion: v1
+kind: Pod
+metadata:
+  name: nginx
+  labels:
+    app.kubernetes.io/name: nginx
+spec:
+  containers:
+  - name: nginx
+    image: nginx
+    resources:
+      limits:
+        memory: "512Mi"
diff --git a/test/checks/oneController/check.yaml b/test/checks/oneController/check.yaml
new file mode 100644
index 00000000..6f964ce3
--- /dev/null
+++ b/test/checks/oneController/check.yaml
@@ -0,0 +1,17 @@
+successMessage: example FP success
+failureMessage: example FP fail
+target: StatefulSet
+schema:
+  '$schema': http://json-schema.org/draft-07/schema
+  type: object
+  properties:
+    metadata:
+      type: object
+      required: ["labels"]
+      properties:
+        labels:
+          type: object
+          required: ["app.kubernetes.io/name"]
+          properties:
+            app.kubernetes.io/name:
+              const: "{{ .metadata.name }}"
diff --git a/test/checks/oneController/failure.yaml b/test/checks/oneController/failure.yaml
new file mode 100644
index 00000000..e5399f07
--- /dev/null
+++ b/test/checks/oneController/failure.yaml
@@ -0,0 +1,13 @@
+apiVersion: v1
+kind: StatefulSet
+metadata:
+  name: nginx
+  labels:
+    app.kubernetes.io/name: nginxIDONTMATCH
+spec:
+  containers:
+  - name: nginx
+    image: nginx
+    resources:
+      limits:
+        memory: "512Mi"
diff --git a/test/checks/oneController/success.yaml b/test/checks/oneController/success.yaml
new file mode 100644
index 00000000..693db747
--- /dev/null
+++ b/test/checks/oneController/success.yaml
@@ -0,0 +1,13 @@
+apiVersion: v1
+kind: StatefulSet
+metadata:
+  name: nginx
+  labels:
+    app.kubernetes.io/name: nginx
+spec:
+  containers:
+  - name: nginx
+    image: nginx
+    resources:
+      limits:
+        memory: "512Mi"
diff --git a/test/schema_test.go b/test/schema_test.go
index 2aca7cae..647ab317 100644
--- a/test/schema_test.go
+++ b/test/schema_test.go
@@ -37,6 +37,7 @@ type testCase struct {
 	filename  string
 	resources *kube.ResourceProvider
 	failure   bool
+	config    config.Configuration
 }
 
 var mutatedYamlContentMap = map[string]string{}
@@ -56,7 +57,25 @@ func init() {
 		if err != nil {
 			panic(err)
 		}
+		configString := "checks:\n  " + check + ": danger"
+		checkPath := checkDir + "/check.yaml"
+		customCheckContent, err := ioutil.ReadFile(checkPath)
+		if err == nil {
+			lines := strings.Split(string(customCheckContent), "\n")
+			for idx := range lines {
+				lines[idx] = "    " + lines[idx]
+			}
+			configString += "\ncustomChecks:\n  " + check + ":\n"
+			configString += strings.Join(lines, "\n")
+		}
+		c, err := config.Parse([]byte(configString))
+		if err != nil {
+			panic(err)
+		}
 		for _, tc := range cases {
+			if tc.Name() == "check.yaml" {
+				continue
+			}
 			resources, err := kube.CreateResourceProviderFromPath(checkDir + "/" + tc.Name())
 			if err != nil {
 				panic(err)
@@ -75,6 +94,7 @@ func init() {
 					check:     check,
 					resources: resources,
 					failure:   strings.Contains(tc.Name(), "failure"),
+					config:    c,
 				}
 				testCases = append(testCases, testcase)
 
@@ -93,11 +113,7 @@ func init() {
 
 func TestChecks(t *testing.T) {
 	for _, tc := range testCases {
-		c, err := config.Parse([]byte("checks:\n  " + tc.check + ": danger"))
-		if err != nil {
-			panic(err)
-		}
-		results, err := validator.ApplyAllSchemaChecksToResourceProvider(&c, tc.resources)
+		results, err := validator.ApplyAllSchemaChecksToResourceProvider(&tc.config, tc.resources)
 		if err != nil {
 			panic(err)
 		}