Repository: FairwindsOps/polaris
Pull request #1053 (2024-06-26 19:20:18)

INSIGHTS-90 - implement HPA minAvailable and HPA maxAvailable checks
This PR fixes internal INSIGHTS-90

## Checklist
* [X] I have signed the CLA
* [X] I have updated/added any relevant documentation

## Description
### What's the goal of this PR?
- implement support for HPA `minAvailable` check
- implement support for HPA `maxAvailable` check

### What changes did you make?
- I had to implement a custom validator function for validations that are not supported by JSON validation
  - i.e.: in HPA `maxAvailable` check, we need to check fix `maxAvailable` is greater than `minAvailable` and in JSON validation you cannot self-reference fields

### What alternative solution should we consider, if any?



fairwinds-insights[bot]: ## Fairwinds Insights Scan Results
[View the full report at insights.fairwinds.com](https://insights.fairwinds.com/gh/FairwindsOps/polaris/hpa_min_max_availability%2Fvv%2FINSIGHTS-90).

:white_check_mark: No new Action Items detected!

Patch:
diff --git a/docs/changelog.md b/docs/changelog.md
index 11a8107e..c630a746 100644
--- a/docs/changelog.md
+++ b/docs/changelog.md
@@ -8,6 +8,7 @@ meta:
 
 ## Unreleased
 * Change `metadataAndNameMismatched` to `metadataAndInstanceMismatched`
+* Add `hpaMaxAvailability` and `hpaMinAvailability` check
 
 ## 8.1.1
 * Add category for `metadataAndNameMismatched`.
diff --git a/docs/checks/reliability.md b/docs/checks/reliability.md
index 1eb2d014..b551e1f6 100644
--- a/docs/checks/reliability.md
+++ b/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
 
diff --git a/pkg/config/checks.go b/pkg/config/checks.go
index b6d58779..7d1cb334 100644
--- a/pkg/config/checks.go
+++ b/pkg/config/checks.go
@@ -67,6 +67,8 @@ var (
 		"clusterrolebindingClusterAdmin",
 		"rolebindingClusterAdminClusterRole",
 		"rolebindingClusterAdminRole",
+		"hpaMaxAvailability",
+		"hpaMinAvailability",
 	}
 
 	// BuiltInChecks contains the checks that come pre-installed w/ Polaris
diff --git a/pkg/config/checks/hpaMaxAvailability.yaml b/pkg/config/checks/hpaMaxAvailability.yaml
new file mode 100644
index 00000000..1d0ff72f
--- /dev/null
+++ b/pkg/config/checks/hpaMaxAvailability.yaml
@@ -0,0 +1,4 @@
+successMessage: HPA has a valid max and min replica configuration
+failureMessage: HPA maxReplicas and minReplicas should be different 
+category: Reliability
+target: autoscaling/HorizontalPodAutoscaler
\ No newline at end of file
diff --git a/pkg/config/checks/hpaMinAvailability.yaml b/pkg/config/checks/hpaMinAvailability.yaml
new file mode 100644
index 00000000..5bc00104
--- /dev/null
+++ b/pkg/config/checks/hpaMinAvailability.yaml
@@ -0,0 +1,14 @@
+successMessage: HPA has a valid min replica configuration
+failureMessage: HPA maxReplicas should be greater than minReplicas
+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
diff --git a/pkg/config/default.yaml b/pkg/config/default.yaml
index 5c82272b..8796fac9 100644
--- a/pkg/config/default.yaml
+++ b/pkg/config/default.yaml
@@ -10,6 +10,8 @@ checks:
   pdbDisruptionsIsZero: warning
   missingPodDisruptionBudget: warning
   topologySpreadConstraint: warning
+  hpaMaxAvailability: warning
+  hpaMinAvailability: warning
 
   # efficiency
   cpuRequestsMissing: warning
diff --git a/pkg/config/examples/config-full.yaml b/pkg/config/examples/config-full.yaml
index 128f9b64..2f959864 100644
--- a/pkg/config/examples/config-full.yaml
+++ b/pkg/config/examples/config-full.yaml
@@ -10,6 +10,8 @@ checks:
   pdbDisruptionsIsZero: warning
   missingPodDisruptionBudget: warning
   metadataAndInstanceMismatched: warning
+  hpaMaxAvailability: warning
+  hpaMinAvailability: warning
 
   # efficiency
   cpuRequestsMissing: warning
diff --git a/pkg/kube/resources.go b/pkg/kube/resources.go
index 74e5aa7f..3ae7de08 100644
--- a/pkg/kube/resources.go
+++ b/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
 }
diff --git a/pkg/validator/schema.go b/pkg/validator/schema.go
index 510af7bd..f1be883a 100644
--- a/pkg/validator/schema.go
+++ b/pkg/validator/schema.go
@@ -367,8 +367,12 @@ 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 if customValidators[checkID] != nil {
+		passes, issues, err = customValidators[checkID](test.Resource.Resource.Object)
+	} else {
+		passes, issues, err = true, []jsonschema.ValError{}, nil
 	}
 	if err != nil {
 		return nil, err
diff --git a/pkg/validator/validations.go b/pkg/validator/validations.go
new file mode 100644
index 00000000..d2489eab
--- /dev/null
+++ b/pkg/validator/validations.go
@@ -0,0 +1,46 @@
+package validator
+
+import (
+	"encoding/json"
+	"fmt"
+
+	"github.com/qri-io/jsonschema"
+)
+
+type customValidator func(data interface{}) (bool, []jsonschema.ValError, error)
+
+// customValidators is a map of validation functions that can be used in schema checks
+// sometimes we need to validate things that aren't covered by the JSON validation schema
+var customValidators = map[string]customValidator{
+	"hpaMaxAvailability": validateHPAMaxAvailability,
+}
+
+type HorizontalPodAutoscalerView struct {
+	Spec struct {
+		MinReplicas *int `json:"minReplicas"`
+		MaxReplicas int  `json:"maxReplicas"`
+	} `json:"spec"`
+}
+
+func validateHPAMaxAvailability(data any) (bool, []jsonschema.ValError, error) {
+	jsonString, err := json.Marshal(data)
+	if err != nil {
+		return false, nil, err
+	}
+
+	hpa := HorizontalPodAutoscalerView{}
+	err = json.Unmarshal(jsonString, &hpa)
+	if err != nil {
+		return false, nil, err
+	}
+
+	if hpa.Spec.MinReplicas == nil {
+		return true, []jsonschema.ValError{}, nil
+	}
+
+	if hpa.Spec.MaxReplicas != *hpa.Spec.MinReplicas {
+		return true, []jsonschema.ValError{}, nil
+	}
+
+	return false, []jsonschema.ValError{{PropertyPath: "spec.maxReplicas", Message: fmt.Sprintf("maxReplicas (%d) and minReplicas (%d) should be different", hpa.Spec.MaxReplicas, *hpa.Spec.MinReplicas)}}, nil
+}


Test patch:
diff --git a/test/checks/hpaMaxAvailability/failure.yaml b/test/checks/hpaMaxAvailability/failure.yaml
new file mode 100644
index 00000000..88e35ddc
--- /dev/null
+++ b/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
\ No newline at end of file
diff --git a/test/checks/hpaMaxAvailability/success.minAvailabilityAbsent.yaml b/test/checks/hpaMaxAvailability/success.minAvailabilityAbsent.yaml
new file mode 100644
index 00000000..91f87a39
--- /dev/null
+++ b/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
\ No newline at end of file
diff --git a/test/checks/hpaMaxAvailability/success.yaml b/test/checks/hpaMaxAvailability/success.yaml
new file mode 100644
index 00000000..66d580cd
--- /dev/null
+++ b/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
\ No newline at end of file
diff --git a/test/checks/hpaMinAvailability/failure.yaml b/test/checks/hpaMinAvailability/failure.yaml
new file mode 100644
index 00000000..6cfbdeb2
--- /dev/null
+++ b/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
\ No newline at end of file
diff --git a/test/checks/hpaMinAvailability/success.minReplicaAbsent.yaml b/test/checks/hpaMinAvailability/success.minReplicaAbsent.yaml
new file mode 100644
index 00000000..91f87a39
--- /dev/null
+++ b/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
\ No newline at end of file
diff --git a/test/checks/hpaMinAvailability/success.yaml b/test/checks/hpaMinAvailability/success.yaml
new file mode 100644
index 00000000..318698d6
--- /dev/null
+++ b/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
\ No newline at end of file
diff --git a/test/fixtures.go b/test/fixtures.go
index aca948ed..ca76ca59 100644
--- a/test/fixtures.go
+++ b/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{
diff --git a/test/schema_test.go b/test/schema_test.go
index 90ec2ec3..ca09d6d9 100644
--- a/test/schema_test.go
+++ b/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()