INSIGHTS-156 - Fix typo and improve failure message (#1055)

* fix typo

* fix failure message

* fix changelog

* fix missingPodDisruptionBudget validation

* Update failure.empty-labels.yaml

* Update failure.no-metadata.yaml

* INSIGHTS-159 - use go templating instead of custom function validation (#1056)

* use go templating instead of custom function validation

* fix changelog

docs/changelog.md

@@ -6,13 +6,69 @@ meta:
 
 ---
 
-## Unreleased
-* Change `metadataAndNameMismatched` to `metadataAndInstanceMismatched`
+## 9.1.1
+* Fix `hpaMinAvailability` failure message
+* Fix `missingPodDisruptionBudget` typo
+* Rewrite `hpaMaxAvailability` check to use go-template
 
-## 8.1.1
+## 9.1.0
+* Add HPA `minAvailable` and HPA `maxAvailable` checks 
+* Fix typo for PDB `minAvailable`
+
+## 9.0.1
+* Fix comments handling in `addOrReplaceValue` function
+
+## 9.0.0
+* Expose issue fixer and mutations in the library
+* Remove `packr` in favor of `go:embed`
+
+## 8.5.6
+* Fix trying to list cluster-level resources
+
+## 8.5.5
+* Fix missing PDB check
+
+## 8.5.4
+* Fix conditional expressions should be at very top of `additionalSchemaStrings`
+* Update alpine to 3.19
+
+## 8.5.3
+* Add quiet flag to polaris audit CLI command to suppress 'upload to Insights' prompt
+
+## 8.5.2
+* Switch to `controller-utils` package to get workloads
+
+## 8.5.1
+* Update `topologySpreadConstraint` check
+
+## 8.5.0
+* Add helm-skip-tests flag
+* Update CLI docs
+* Handle multiple helm-values files
+
+## 8.4.0
+* Change kubernetes.io/ label from name to instance
+
+## 8.3.0
+* Add option to filter audit results by severity level
+* Add insights prompt
+
+## 8.2.4
+* Fix nil pointer issue with webhook
+
+## 8.2.3
 * Add category for `metadataAndNameMismatched`.
 * Fix category for `priorityClassNotSet`.
 
+## 8.2.2
+* Fix webhook server cert dir argument
+
+## 8.2.1
+* Fix on Insights integration
+
+## 8.2.0
+* Minor fixes for NSA checks
+
 ## 8.1.0
 * Add `insights-host` global flag to configure Fairwinds Insights host (defaults to `https://insights.fairwinds.com`).
 * Add new `auth` sub-commands be able to authenticate on Polaris using Fairwinds Insights credentials

docs/checks/reliability.md

@@ -19,6 +19,8 @@ key | default | description
 `missingPodDisruptionBudget` | `warning` | Fails when PDB is missing.
 `metadataAndInstanceMismatched` | `warning` | Fails when label `app.kubernetes.io/instance` and `metadata.name` mismatch
 `topologySpreadConstraint` | `warning` | Fails when there is no topology spread constraint on the pod
+`hpaMaxAvailability` | `warning` | Fails when `maxAvailable` lesser or equal than `minAvailable` (if defined) for a HorizontalPodAutoscaler
+`hpaMinAvailability` | `warning` | Fails when `minAvailable` (if defined) lesser or equal to one for a HorizontalPodAutoscaler
 
 ## Background
 

pkg/config/checks.go

@@ -67,6 +67,8 @@ var (
 		"clusterrolebindingClusterAdmin",
 		"rolebindingClusterAdminClusterRole",
 		"rolebindingClusterAdminRole",
+		"hpaMaxAvailability",
+		"hpaMinAvailability",
 	}
 
 	// BuiltInChecks contains the checks that come pre-installed w/ Polaris

pkg/config/checks/hpaMaxAvailability.yaml

@@ -0,0 +1,35 @@
+successMessage: HPA has a valid max and min replica configuration
+failureMessage: HPA maxReplicas and minReplicas should be different 
+category: Reliability
+target: autoscaling/HorizontalPodAutoscaler
+schemaString: |
+  "$schema": http://json-schema.org/draft-07/schema#
+  type: object
+  properties:
+    spec:
+      type: object
+      properties:
+        minReplicas:
+          type: integer
+          minimum: 1
+        maxReplicas:
+          type: integer
+          minimum: 1
+      required:
+      - maxReplicas
+      {{- if .spec.minReplicas }}
+      if:
+        properties:
+          minReplicas:
+            type: integer
+          maxReplicas:
+            type: integer
+      then:
+        properties:
+          maxReplicas:
+            exclusiveMinimum: {{ .spec.minReplicas }}
+      else:
+        properties:
+          maxReplicas:
+            minimum: 1
+      {{- end }}

pkg/config/checks/hpaMinAvailability.yaml

@@ -0,0 +1,14 @@
+successMessage: HPA has a valid min replica configuration
+failureMessage: HPA minReplicas should be 2 or more
+category: Reliability
+target: autoscaling/HorizontalPodAutoscaler
+schema:
+  "$schema": http://json-schema.org/draft-07/schema#
+  type: object
+  properties:
+    spec:
+      type: object
+      properties:
+        minReplicas:
+          type: integer
+          minimum: 2

pkg/config/checks/missingPodDisruptionBudget.yaml

@@ -4,9 +4,9 @@ category: Reliability
 target: Controller
 controllers:
   include:
-  - Deployment
+    - Deployment
 schema:
-  '$schema': http://json-schema.org/draft-07/schema
+  "$schema": http://json-schema.org/draft-07/schema#
   type: object
   properties:
     spec:
@@ -14,13 +14,21 @@ schema:
       properties:
         template:
           type: object
-          properites:
+          properties:
             metadata:
               type: object
               properties:
                 labels:
                   type: object
                   minProperties: 1
+              required:
+                - labels
+          required:
+            - metadata
+      required:
+        - template
+  required:
+    - spec
 additionalSchemaStrings:
   policy/PodDisruptionBudget: |
     type: object

pkg/config/default.yaml

@@ -10,6 +10,8 @@ checks:
   pdbDisruptionsIsZero: warning
   missingPodDisruptionBudget: warning
   topologySpreadConstraint: warning
+  hpaMaxAvailability: warning
+  hpaMinAvailability: warning
 
   # efficiency
   cpuRequestsMissing: warning

pkg/config/examples/config-full.yaml

@@ -10,6 +10,8 @@ checks:
   pdbDisruptionsIsZero: warning
   missingPodDisruptionBudget: warning
   metadataAndInstanceMismatched: warning
+  hpaMaxAvailability: warning
+  hpaMinAvailability: warning
 
   # efficiency
   cpuRequestsMissing: warning

pkg/kube/resources.go

@@ -94,12 +94,16 @@ func (rkm resourceKindMap) GetNumberOfControllers() int {
 	return total
 }
 
+var kindRewrites = map[string]string{
+	"Ingress":                 "networking.k8s.io/Ingress",
+	"PodDisruptionBudget":     "policy/PodDisruptionBudget",
+	"HorizontalPodAutoscaler": "autoscaling/HorizontalPodAutoscaler",
+}
+
 // This is here for backward compatibility reasons
 func maybeTransformKindIntoGroupKind(k string) string {
-	if k == "Ingress" {
-		return "networking.k8s.io/Ingress"
-	} else if k == "PodDisruptionBudget" {
-		return "policy/PodDisruptionBudget"
+	if val, ok := kindRewrites[k]; ok {
+		return val
 	}
 	return k
 }

pkg/mutation/mutate.go

@@ -182,6 +182,9 @@ func addOrReplaceValue(node *yaml.Node, splits []string, value *yaml.Node) error
 	}
 	for _, node := range nodes {
 		if node.Kind == yaml.ScalarNode {
+			if value.LineComment == "" {
+				value.LineComment = node.LineComment // keep the original comment if override is not provided
+			}
 			// Overwrite an existing scalar value with a new value (whatever kind).
 			*node = *value
 		} else if node.Kind == yaml.MappingNode && value.Kind == yaml.MappingNode {

pkg/mutation/mutate_test.go

@@ -107,7 +107,8 @@ obj:
 			},
 			Path: "/obj/foo",
 		},
-		mutated: `obj:
+		mutated: `
+obj:
   foo:
     bar:
       - c
@@ -115,23 +116,17 @@ obj:
   baz: quux
 `,
 	}, {
-		original: `
-foo: bar
-`,
+		original: `foo: bar`,
 		patch: config.Mutation{
 			Op:      "replace",
 			Value:   "baz",
 			Path:    "/foo",
 			Comment: "# We set this to baz",
 		},
-		mutated: `
-foo: baz # We set this to baz
-`,
+		mutated: `foo: baz # We set this to baz`,
 		message: "Expected a comment to appear",
 	}, {
-		original: `
-foo: bar
-`,
+		original: `foo: bar`,
 		patch: config.Mutation{
 			Op: "add",
 			Value: map[string]interface{}{
@@ -167,6 +162,25 @@ extra:
   baz: quux
 `,
 		message: "Expected a comment to appear next to an object",
+	}, {
+		original: `foo: bar # we should keep this comment`,
+		patch: config.Mutation{
+			Op:    "replace",
+			Value: "baz",
+			Path:  "/foo",
+		},
+		mutated: `foo: baz # we should keep this comment`,
+		message: "Expected a comment to be kept",
+	}, {
+		original: `foo: bar # we should override this comment`,
+		patch: config.Mutation{
+			Op:      "replace",
+			Value:   "baz",
+			Path:    "/foo",
+			Comment: "override",
+		},
+		mutated: `foo: baz # override`,
+		message: "Expected a comment to overridden",
 	},
 }
 

pkg/validator/schema.go

@@ -367,8 +367,10 @@ func applySchemaCheck(conf *config.Configuration, checkID string, test schemaTes
 			prefix += "/containers/" + strconv.Itoa(containerIndex)
 		}
 		passes, issues, err = check.CheckContainer(test.Container)
-	} else {
+	} else if check.Validator.SchemaURI != "" {
 		passes, issues, err = check.CheckObject(test.Resource.Resource.Object)
+	} else {
+		passes, issues, err = true, []jsonschema.ValError{}, nil
 	}
 	if err != nil {
 		return nil, err

test/checks/hpaMaxAvailability/failure.yaml

@@ -0,0 +1,30 @@
+apiVersion: autoscaling/v2
+kind: HorizontalPodAutoscaler
+metadata:
+  name: php-apache
+spec:
+  scaleTargetRef:
+    apiVersion: apps/v1
+    kind: Deployment
+    name: php-apache
+  minReplicas: 5
+  maxReplicas: 5
+  metrics:
+  - type: Resource
+    resource:
+      name: cpu
+      target:
+        type: Utilization
+        averageUtilization: 50
+status:
+  observedGeneration: 1
+  lastScaleTime: "2023-06-26T15:04:05Z"
+  currentReplicas: 1
+  desiredReplicas: 1
+  currentMetrics:
+  - type: Resource
+    resource:
+      name: cpu
+      current:
+        averageUtilization: 0
+        averageValue: 0

test/checks/hpaMaxAvailability/success.minAvailabilityAbsent.yaml

@@ -0,0 +1,29 @@
+apiVersion: autoscaling/v2
+kind: HorizontalPodAutoscaler
+metadata:
+  name: php-apache
+spec:
+  scaleTargetRef:
+    apiVersion: apps/v1
+    kind: Deployment
+    name: php-apache
+  maxReplicas: 10
+  metrics:
+  - type: Resource
+    resource:
+      name: cpu
+      target:
+        type: Utilization
+        averageUtilization: 50
+status:
+  observedGeneration: 1
+  lastScaleTime: "2023-06-26T15:04:05Z"
+  currentReplicas: 1
+  desiredReplicas: 1
+  currentMetrics:
+  - type: Resource
+    resource:
+      name: cpu
+      current:
+        averageUtilization: 0
+        averageValue: 0

test/checks/hpaMaxAvailability/success.yaml

@@ -0,0 +1,30 @@
+apiVersion: autoscaling/v2
+kind: HorizontalPodAutoscaler
+metadata:
+  name: php-apache
+spec:
+  scaleTargetRef:
+    apiVersion: apps/v1
+    kind: Deployment
+    name: php-apache
+  minReplicas: 5
+  maxReplicas: 10
+  metrics:
+  - type: Resource
+    resource:
+      name: cpu
+      target:
+        type: Utilization
+        averageUtilization: 50
+status:
+  observedGeneration: 1
+  lastScaleTime: "2023-06-26T15:04:05Z"
+  currentReplicas: 1
+  desiredReplicas: 1
+  currentMetrics:
+  - type: Resource
+    resource:
+      name: cpu
+      current:
+        averageUtilization: 0
+        averageValue: 0

test/checks/hpaMinAvailability/failure.yaml

@@ -0,0 +1,30 @@
+apiVersion: autoscaling/v2
+kind: HorizontalPodAutoscaler
+metadata:
+  name: php-apache
+spec:
+  scaleTargetRef:
+    apiVersion: apps/v1
+    kind: Deployment
+    name: php-apache
+  minReplicas: 1
+  maxReplicas: 10
+  metrics:
+  - type: Resource
+    resource:
+      name: cpu
+      target:
+        type: Utilization
+        averageUtilization: 50
+status:
+  observedGeneration: 1
+  lastScaleTime: "2023-06-26T15:04:05Z"
+  currentReplicas: 1
+  desiredReplicas: 1
+  currentMetrics:
+  - type: Resource
+    resource:
+      name: cpu
+      current:
+        averageUtilization: 0
+        averageValue: 0

test/checks/hpaMinAvailability/success.minReplicaAbsent.yaml

@@ -0,0 +1,29 @@
+apiVersion: autoscaling/v2
+kind: HorizontalPodAutoscaler
+metadata:
+  name: php-apache
+spec:
+  scaleTargetRef:
+    apiVersion: apps/v1
+    kind: Deployment
+    name: php-apache
+  maxReplicas: 10
+  metrics:
+  - type: Resource
+    resource:
+      name: cpu
+      target:
+        type: Utilization
+        averageUtilization: 50
+status:
+  observedGeneration: 1
+  lastScaleTime: "2023-06-26T15:04:05Z"
+  currentReplicas: 1
+  desiredReplicas: 1
+  currentMetrics:
+  - type: Resource
+    resource:
+      name: cpu
+      current:
+        averageUtilization: 0
+        averageValue: 0

test/checks/hpaMinAvailability/success.yaml

@@ -0,0 +1,30 @@
+apiVersion: autoscaling/v2
+kind: HorizontalPodAutoscaler
+metadata:
+  name: php-apache
+spec:
+  scaleTargetRef:
+    apiVersion: apps/v1
+    kind: Deployment
+    name: php-apache
+  minReplicas: 2
+  maxReplicas: 10
+  metrics:
+  - type: Resource
+    resource:
+      name: cpu
+      target:
+        type: Utilization
+        averageUtilization: 50
+status:
+  observedGeneration: 1
+  lastScaleTime: "2023-06-26T15:04:05Z"
+  currentReplicas: 1
+  desiredReplicas: 1
+  currentMetrics:
+  - type: Resource
+    resource:
+      name: cpu
+      current:
+        averageUtilization: 0
+        averageValue: 0

test/checks/missingPodDisruptionBudget/failure.empty-labels.yaml

@@ -0,0 +1,22 @@
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: zookeeper
+spec:
+  template:
+    metadata:
+      labels: {} # empty labels
+    spec:
+      containers:
+        - name: zookeeper
+          image: zookeeper
+---
+apiVersion: policy/v1
+kind: PodDisruptionBudget
+metadata:
+  name: zookeeper-pdb
+spec:
+  minAvailable: 2
+  selector:
+    matchLabels:
+      app.kubernetes.io/name: zookeeper

test/checks/missingPodDisruptionBudget/failure.no-labels.yaml

@@ -0,0 +1,21 @@
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: zookeeper
+spec:
+  template:
+    metadata: {} # missing labels
+    spec:
+      containers:
+        - name: zookeeper
+          image: zookeeper
+---
+apiVersion: policy/v1
+kind: PodDisruptionBudget
+metadata:
+  name: zookeeper-pdb
+spec:
+  minAvailable: 2
+  selector:
+    matchLabels:
+      app.kubernetes.io/name: zookeeper

test/checks/missingPodDisruptionBudget/failure.no-metadata.yaml

@@ -0,0 +1,20 @@
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: zookeeper
+spec:
+  template: # missing metadata
+    spec:
+      containers:
+        - name: zookeeper
+          image: zookeeper
+---
+apiVersion: policy/v1
+kind: PodDisruptionBudget
+metadata:
+  name: zookeeper-pdb
+spec:
+  minAvailable: 2
+  selector:
+    matchLabels:
+      app.kubernetes.io/name: zookeeper

test/checks/pdbDisruptionsIsZero/success.both.yaml

@@ -5,5 +5,5 @@ metadata:
   labels:
     env: test
 spec:
-  minAvaiable: 5
+  minAvailable: 5
   maxUnavailable: 10%

test/checks/pdbDisruptionsIsZero/success.min.yaml

@@ -5,4 +5,4 @@ metadata:
   labels:
     env: test
 spec:
-  minAvaiable: 5
+  minAvailable: 5

test/fixtures.go

@@ -18,6 +18,7 @@ import (
 	"encoding/json"
 
 	appsv1 "k8s.io/api/apps/v1"
+	autoscalingV2 "k8s.io/api/autoscaling/v2"
 	batchv1 "k8s.io/api/batch/v1"
 	corev1 "k8s.io/api/core/v1"
 	networkingv1 "k8s.io/api/networking/v1"
@@ -227,6 +228,12 @@ func SetupTestAPI(objects ...runtime.Object) (kubernetes.Interface, dynamic.Inte
 				{Name: "poddisruptionbudgets", Namespaced: true, Kind: "PodDisruptionBudget", Version: "v1"},
 			},
 		},
+		{
+			GroupVersion: autoscalingV2.SchemeGroupVersion.String(),
+			APIResources: []metav1.APIResource{
+				{Name: "horizontalpodautoscalers", Namespaced: true, Kind: "HorizontalPodAutoscaler", Version: "v2"},
+			},
+		},
 		{
 			GroupVersion: "core/v1",
 			APIResources: []metav1.APIResource{

test/schema_test.go

@@ -44,7 +44,7 @@ var mutatedYamlContentMap = map[string]string{}
 var mutationTestCasesMap = map[string][]testCase{}
 
 func init() {
-	checkToTest := os.Getenv("POLARIS_CHECK_TEST")
+	checkToTest := os.Getenv("POLARIS_CHECK_TEST") // if set, only run tests for this check
 	_, baseDir, _, _ := runtime.Caller(0)
 	baseDir = filepath.Dir(baseDir) + "/checks"
 	dirs, err := os.ReadDir(baseDir)
@@ -56,6 +56,9 @@ func init() {
 		if checkToTest != "" && checkToTest != check {
 			continue
 		}
+		if strings.HasPrefix(check, "_") {
+			continue // skip directories starting with _
+		}
 		checkDir := baseDir + "/" + check
 		cases, err := os.ReadDir(checkDir)
 		if err != nil {
@@ -119,7 +122,7 @@ func TestChecks(t *testing.T) {
 	for _, tc := range testCases {
 		results, err := validator.ApplyAllSchemaChecksToResourceProvider(&tc.config, tc.resources)
 		if err != nil {
-			panic(err)
+			t.Fatalf("Error running checks: %v", err)
 		}
 		auditData := validator.AuditData{Results: results}
 		summary := auditData.GetSummary()