Repository: zegl/kube-score (go)

Issue:
Check "deployment-replicas" should consider minReplicas in HPA configuration
**Which version of kube-score are you using?**

kube-score version: v1.17.0

**What did you do?**

I validated the replicas of a deployment that is targeted by a service and an HPA.
However, the `minReplicas` of the HPA is set to `1`. This is, according to [kubernetes.io docs](https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/horizontal-pod-autoscaler-v2/#HorizontalPodAutoscalerSpec),  also the default value.

**What did you expect to see?**

The `deployment-replicas` check ([score/deployment/deployment.go#L54](https://github.com/zegl/kube-score/blob/a457bb9f5467451f6b2811a5a3feeec0b6e42b7e/score/deployment/deployment.go#L54) ) should have warned me about a single replica. In my opinion, when having an HPA, a replica of 1 isn't considered reliable either.

**What did you see instead?**

The `deployment-replicas` check ([score/deployment/deployment.go#L54](https://github.com/zegl/kube-score/blob/a457bb9f5467451f6b2811a5a3feeec0b6e42b7e/score/deployment/deployment.go#L54) ) didn't warn about single replica. Looking at the source code, it explicitly mentions marking deployments targeted by an HPA as good. However, what's missing is the part to check the HPA's configuration and the configured `minReplicas` to _really_ ensure having at least two replicas.

Happy to hear some more thoughts on this!


Pull request description:
score/hpa: added a check for hpa min replicas > 1
- Added a check for min replicas > 1 for HorizontalPodAutoscalers
- Clarified the comment if a deployment replica check was skipped

Fixes #581


```
RELNOTE: score/hpa: added a check for hpa min replicas > 1
```


Patch:
diff --git a/domain/kube-score.go b/domain/kube-score.go
index 6e54d1b..ffe6b0c 100644
--- a/domain/kube-score.go
+++ b/domain/kube-score.go
@@ -2,9 +2,9 @@ package domain
 
 import (
 	"io"
+	autoscalingv1 "k8s.io/api/autoscaling/v1"
 
 	appsv1 "k8s.io/api/apps/v1"
-	autoscalingv1 "k8s.io/api/autoscaling/v1"
 	corev1 "k8s.io/api/core/v1"
 	networkingv1 "k8s.io/api/networking/v1"
 	policyv1 "k8s.io/api/policy/v1"
@@ -49,6 +49,7 @@ type FileLocationer interface {
 type HpaTargeter interface {
 	GetTypeMeta() metav1.TypeMeta
 	GetObjectMeta() metav1.ObjectMeta
+	MinReplicas() *int32
 	HpaTarget() autoscalingv1.CrossVersionObjectReference
 	FileLocationer
 }
diff --git a/go.mod b/go.mod
index 0b08fa9..a5903dc 100644
--- a/go.mod
+++ b/go.mod
@@ -7,7 +7,7 @@ require (
 	github.com/mattn/go-isatty v0.0.20
 	github.com/spf13/pflag v1.0.5
 	github.com/stretchr/testify v1.9.0
-	golang.org/x/term v0.17.0
+	golang.org/x/term v0.18.0
 	gopkg.in/yaml.v3 v3.0.1
 	k8s.io/api v0.29.2
 	k8s.io/apimachinery v0.29.2
@@ -25,7 +25,7 @@ require (
 	github.com/modern-go/reflect2 v1.0.2 // indirect
 	github.com/pmezard/go-difflib v1.0.0 // indirect
 	golang.org/x/net v0.19.0 // indirect
-	golang.org/x/sys v0.17.0 // indirect
+	golang.org/x/sys v0.18.0 // indirect
 	golang.org/x/text v0.14.0 // indirect
 	gopkg.in/inf.v0 v0.9.1 // indirect
 	gopkg.in/yaml.v2 v2.4.0 // indirect
diff --git a/go.sum b/go.sum
index bac8f7b..0287368 100644
--- a/go.sum
+++ b/go.sum
@@ -64,10 +64,10 @@ golang.org/x/sys v0.0.0-20190412213103-97732733099d/go.mod h1:h1NjWce9XRLGQEsW7w
 golang.org/x/sys v0.0.0-20200930185726-fdedc70b468f/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
 golang.org/x/sys v0.0.0-20220811171246-fbc7d0a398ab/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.6.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
-golang.org/x/sys v0.17.0 h1:25cE3gD+tdBA7lp7QfhuV+rJiE9YXTcS3VG1SqssI/Y=
-golang.org/x/sys v0.17.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
-golang.org/x/term v0.17.0 h1:mkTF7LCd6WGJNL3K1Ad7kwxNfYAW6a8a8QqtMblp/4U=
-golang.org/x/term v0.17.0/go.mod h1:lLRBjIVuehSbZlaOtGMbcMncT+aqLLLmKrsjNrUguwk=
+golang.org/x/sys v0.18.0 h1:DBdB3niSjOA/O0blCZBqDefyWNYveAYMNF1Wum0DYQ4=
+golang.org/x/sys v0.18.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
+golang.org/x/term v0.18.0 h1:FcHjZXDMxI8mM3nwhX9HlKop4C0YQvCVCdwYl2wOtE8=
+golang.org/x/term v0.18.0/go.mod h1:ILwASektA3OnRv7amZ1xhE/KTR+u50pbXfZ03+6Nx58=
 golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
 golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ=
 golang.org/x/text v0.14.0 h1:ScX5w1eTa3QqT8oi6+ziP7dTV1S2+ALU0bI+0zXKWiQ=
diff --git a/parser/internal/hpa.go b/parser/internal/hpa.go
index bd619d1..3110881 100644
--- a/parser/internal/hpa.go
+++ b/parser/internal/hpa.go
@@ -26,6 +26,9 @@ func (d HPAv1) GetTypeMeta() metav1.TypeMeta {
 func (d HPAv1) GetObjectMeta() metav1.ObjectMeta {
 	return d.ObjectMeta
 }
+func (d HPAv1) MinReplicas() *int32 {
+	return d.Spec.MinReplicas
+}
 
 func (d HPAv1) HpaTarget() autoscalingv1.CrossVersionObjectReference {
 	return d.Spec.ScaleTargetRef
@@ -48,6 +51,10 @@ func (d HPAv2beta1) GetObjectMeta() metav1.ObjectMeta {
 	return d.ObjectMeta
 }
 
+func (d HPAv2beta1) MinReplicas() *int32 {
+	return d.Spec.MinReplicas
+}
+
 func (d HPAv2beta1) HpaTarget() autoscalingv1.CrossVersionObjectReference {
 	return autoscalingv1.CrossVersionObjectReference(d.Spec.ScaleTargetRef)
 }
@@ -69,6 +76,10 @@ func (d HPAv2beta2) GetObjectMeta() metav1.ObjectMeta {
 	return d.ObjectMeta
 }
 
+func (d HPAv2beta2) MinReplicas() *int32 {
+	return d.Spec.MinReplicas
+}
+
 func (d HPAv2beta2) HpaTarget() autoscalingv1.CrossVersionObjectReference {
 	return autoscalingv1.CrossVersionObjectReference(d.Spec.ScaleTargetRef)
 }
@@ -90,6 +101,10 @@ func (d HPAv2) GetObjectMeta() metav1.ObjectMeta {
 	return d.ObjectMeta
 }
 
+func (d HPAv2) MinReplicas() *int32 {
+	return d.Spec.MinReplicas
+}
+
 func (d HPAv2) HpaTarget() autoscalingv1.CrossVersionObjectReference {
 	return autoscalingv1.CrossVersionObjectReference(d.Spec.ScaleTargetRef)
 }
diff --git a/score/deployment/deployment.go b/score/deployment/deployment.go
index 8b202bd..a1f2f9d 100644
--- a/score/deployment/deployment.go
+++ b/score/deployment/deployment.go
@@ -91,9 +91,12 @@ func deploymentReplicas(svcs []ks.Service, hpas []ks.HpaTargeter) func(deploymen
 			}
 		}
 
-		if !referencedByService || hasHPA {
+		if !referencedByService {
 			score.Skipped = true
-			score.AddComment("", "Skipped as the Deployment is not targeted by service or is controlled by a HorizontalPodAutoscaler", "")
+			score.AddComment("", "Skipped as the Deployment is not targeted by service", "")
+		} else if hasHPA {
+			score.Skipped = true
+			score.AddComment("", "Skipped as the Deployment is controlled by a HorizontalPodAutoscaler", "")
 		} else {
 			if ptr.Deref(deployment.Spec.Replicas, 1) >= 2 {
 				score.Grade = scorecard.GradeAllOK
diff --git a/score/hpa/hpa.go b/score/hpa/hpa.go
index 7414fbf..63e5637 100644
--- a/score/hpa/hpa.go
+++ b/score/hpa/hpa.go
@@ -4,10 +4,12 @@ import (
 	"github.com/zegl/kube-score/domain"
 	"github.com/zegl/kube-score/score/checks"
 	"github.com/zegl/kube-score/scorecard"
+	"k8s.io/utils/ptr"
 )
 
 func Register(allChecks *checks.Checks, allTargetableObjs []domain.BothMeta) {
 	allChecks.RegisterHorizontalPodAutoscalerCheck("HorizontalPodAutoscaler has target", `Makes sure that the HPA targets a valid object`, hpaHasTarget(allTargetableObjs))
+	allChecks.RegisterHorizontalPodAutoscalerCheck("HorizontalPodAutoscaler Replicas", `Makes sure that the HPA has multiple replicas`, hpaHasMultipleReplicas())
 }
 
 func hpaHasTarget(allTargetableObjs []domain.BothMeta) func(hpa domain.HpaTargeter) (scorecard.TestScore, error) {
@@ -33,3 +35,15 @@ func hpaHasTarget(allTargetableObjs []domain.BothMeta) func(hpa domain.HpaTarget
 		return
 	}
 }
+
+func hpaHasMultipleReplicas() func(hpa domain.HpaTargeter) (scorecard.TestScore, error) {
+	return func(hpa domain.HpaTargeter) (score scorecard.TestScore, err error) {
+		if ptr.Deref(hpa.MinReplicas(), 1) >= 2 {
+			score.Grade = scorecard.GradeAllOK
+		} else {
+			score.Grade = scorecard.GradeWarning
+			score.AddComment("", "HPA few replicas", "HorizontalPodAutoscalers are recommended to have at least 2 replicas to prevent unwanted downtime.")
+		}
+		return
+	}
+}


Test patch:
diff --git a/score/apps/apps_test.go b/score/apps/apps_test.go
index 1994ea0..76561d6 100644
--- a/score/apps/apps_test.go
+++ b/score/apps/apps_test.go
@@ -355,6 +355,10 @@ func (d hpav1) GetObjectMeta() metav1.ObjectMeta {
 	return d.ObjectMeta
 }
 
+func (d hpav1) MinReplicas() *int32 {
+	return d.Spec.MinReplicas
+}
+
 func (d hpav1) HpaTarget() autoscalingv1.CrossVersionObjectReference {
 	return d.Spec.ScaleTargetRef
 }
diff --git a/score/deployment_test.go b/score/deployment_test.go
index 38373cd..fa5ccf9 100644
--- a/score/deployment_test.go
+++ b/score/deployment_test.go
@@ -39,10 +39,14 @@ func TestServiceTargetsDeploymentReplicasOk(t *testing.T) {
 
 func TestServiceNotTargetsDeploymentReplicasNotRelevant(t *testing.T) {
 	t.Parallel()
-	skipped := wasSkipped(t, config.Configuration{
+	assert.True(t, wasSkipped(t, config.Configuration{
+		AllFiles: []ks.NamedReader{testFile("service-not-target-deployment.yaml")},
+	}, "Deployment Replicas"))
+
+	summaries := getSummaries(t, config.Configuration{
 		AllFiles: []ks.NamedReader{testFile("service-not-target-deployment.yaml")},
 	}, "Deployment Replicas")
-	assert.True(t, skipped)
+	assert.Contains(t, summaries, "Skipped as the Deployment is not targeted by service")
 }
 
 func TestServiceTargetsDeploymentReplicasNok(t *testing.T) {
@@ -52,8 +56,12 @@ func TestServiceTargetsDeploymentReplicasNok(t *testing.T) {
 
 func TestHPATargetsDeployment(t *testing.T) {
 	t.Parallel()
-	skipped := wasSkipped(t, config.Configuration{
+	assert.True(t, wasSkipped(t, config.Configuration{
+		AllFiles: []ks.NamedReader{testFile("hpa-target-deployment.yaml")},
+	}, "Deployment Replicas"))
+
+	summaries := getSummaries(t, config.Configuration{
 		AllFiles: []ks.NamedReader{testFile("hpa-target-deployment.yaml")},
 	}, "Deployment Replicas")
-	assert.True(t, skipped)
+	assert.Contains(t, summaries, "Skipped as the Deployment is controlled by a HorizontalPodAutoscaler")
 }
diff --git a/score/hpa/hpa_test.go b/score/hpa/hpa_test.go
index 8b4be19..55ab3c7 100644
--- a/score/hpa/hpa_test.go
+++ b/score/hpa/hpa_test.go
@@ -177,6 +177,10 @@ func (d hpav1) GetObjectMeta() metav1.ObjectMeta {
 	return d.ObjectMeta
 }
 
+func (d hpav1) MinReplicas() *int32 {
+	return d.Spec.MinReplicas
+}
+
 func (d hpav1) HpaTarget() v1.CrossVersionObjectReference {
 	return d.Spec.ScaleTargetRef
 }
diff --git a/score/hpa_test.go b/score/hpa_test.go
index 6b0307a..30d47f7 100644
--- a/score/hpa_test.go
+++ b/score/hpa_test.go
@@ -20,3 +20,13 @@ func TestHorizontalPodAutoscalerHasNoTarget(t *testing.T) {
 	t.Parallel()
 	testExpectedScore(t, "hpa-has-no-target.yaml", "HorizontalPodAutoscaler has target", scorecard.GradeCritical)
 }
+
+func TestHorizontalPodAutoscalerMinReplicasOk(t *testing.T) {
+	t.Parallel()
+	testExpectedScore(t, "hpa-min-replicas-ok.yaml", "HorizontalPodAutoscaler Replicas", scorecard.GradeAllOK)
+}
+
+func TestHorizontalPodAutoscalerMinReplicasNok(t *testing.T) {
+	t.Parallel()
+	testExpectedScore(t, "hpa-min-replicas-nok.yaml", "HorizontalPodAutoscaler Replicas", scorecard.GradeWarning)
+}
diff --git a/score/score_test.go b/score/score_test.go
index 4bcfdd6..1086095 100644
--- a/score/score_test.go
+++ b/score/score_test.go
@@ -54,6 +54,25 @@ func wasSkipped(t *testing.T, config config.Configuration, testcase string) bool
 	return false
 }
 
+func getSummaries(t *testing.T, config config.Configuration, testcase string) []string {
+	sc, err := testScore(config)
+	assert.NoError(t, err)
+	var summaries []string
+	for _, objectScore := range sc {
+		for _, s := range objectScore.Checks {
+			if s.Check.Name == testcase {
+				for _, c := range s.Comments {
+					summaries = append(summaries, c.Summary)
+				}
+				return summaries
+			}
+		}
+	}
+
+	assert.Fail(t, "test was not run")
+	return summaries
+}
+
 func testScore(config config.Configuration) (scorecard.Scorecard, error) {
 	p, err := parser.New()
 	if err != nil {
diff --git a/score/testdata/hpa-min-replicas-nok.yaml b/score/testdata/hpa-min-replicas-nok.yaml
new file mode 100644
index 0000000..9a7c03c
--- /dev/null
+++ b/score/testdata/hpa-min-replicas-nok.yaml
@@ -0,0 +1,31 @@
+apiVersion: autoscaling/v2
+kind: HorizontalPodAutoscaler
+metadata:
+  name: php-apache
+  namespace: default
+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
+---
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: php-apache
+  namespace: default
+spec:
+  template:
+    spec:
+      containers:
+        - name: foo
+          image: foo:latest
\ No newline at end of file
diff --git a/score/testdata/hpa-min-replicas-ok.yaml b/score/testdata/hpa-min-replicas-ok.yaml
new file mode 100644
index 0000000..b40d178
--- /dev/null
+++ b/score/testdata/hpa-min-replicas-ok.yaml
@@ -0,0 +1,31 @@
+apiVersion: autoscaling/v2
+kind: HorizontalPodAutoscaler
+metadata:
+  name: php-apache
+  namespace: default
+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
+---
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: php-apache
+  namespace: default
+spec:
+  template:
+    spec:
+      containers:
+        - name: foo
+          image: foo:latest
\ No newline at end of file
diff --git a/score/testdata/hpa-target-deployment.yaml b/score/testdata/hpa-target-deployment.yaml
index 8536269..d2893a2 100644
--- a/score/testdata/hpa-target-deployment.yaml
+++ b/score/testdata/hpa-target-deployment.yaml
@@ -26,7 +26,23 @@ metadata:
 spec:
   replicas: 1
   template:
+    metadata:
+      labels:
+        app: my-app
     spec:
       containers:
         - name: foo
-          image: foo:latest
\ No newline at end of file
+          image: foo:latest
+---
+kind: Service
+apiVersion: v1
+metadata:
+  name: my-service
+  namespace: default
+spec:
+  selector:
+    app: my-app
+  ports:
+    - protocol: TCP
+      port: 80
+      targetPort: 8080
\ No newline at end of file