From e68f7d1cff4bf550c6fbadbc6a6509007b3c1e53 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Pedro=20Fernandes?= <72892120+jpedrobf@users.noreply.github.com> Date: Tue, 23 Jan 2024 09:33:34 -0300 Subject: [PATCH] fix: pdb-min-available when Replica number is controlled via HPA (#688) --- e2etests/bats-tests.sh | 4 ++ pkg/extract/hpa_spec.go | 16 ++++++ pkg/templates/pdbminavailable/template.go | 49 +++++++++++++++++-- .../pdbminavailable/template_test.go | 42 ++++++++++++++++ tests/checks/pdb-min-available.yaml | 41 ++++++++++++++++ 5 files changed, 147 insertions(+), 5 deletions(-) diff --git a/e2etests/bats-tests.sh b/e2etests/bats-tests.sh index 9a3662864..cbf54700d 100755 --- a/e2etests/bats-tests.sh +++ b/e2etests/bats-tests.sh @@ -638,8 +638,12 @@ get_value_from() { message1=$(get_value_from "${lines[0]}" '.Reports[0].Object.K8sObject.GroupVersionKind.Kind + ": " + .Reports[0].Diagnostic.Message') + message2=$(get_value_from "${lines[0]}" '.Reports[1].Object.K8sObject.GroupVersionKind.Kind + ": " + .Reports[1].Diagnostic.Message') + count=$(get_value_from "${lines[0]}" '.Reports | length') [[ "${message1}" == "PodDisruptionBudget: The current number of replicas for deployment foo is equal to or lower than the minimum number of replicas specified by its PDB." ]] + [[ "${message2}" == "PodDisruptionBudget: The current number of replicas for deployment foo2 is equal to or lower than the minimum number of replicas specified by its PDB." ]] + [[ "${count}" == "2" ]] } @test "privilege-escalation-container" { diff --git a/pkg/extract/hpa_spec.go b/pkg/extract/hpa_spec.go index 691cdd8c7..a94400d77 100644 --- a/pkg/extract/hpa_spec.go +++ b/pkg/extract/hpa_spec.go @@ -31,3 +31,19 @@ func checkReplicas(minReplicas *int32) (int32, bool) { // If numReplicas is a `nil` pointer, then it defaults to 1. return 1, true } + +// HPAScaleTargetRefName extracts Spec.ScaleTargetRef.Name +func HPAScaleTargetRefName(obj k8sutil.Object) (string, bool) { + switch hpa := obj.(type) { + case *autoscalingV2Beta1.HorizontalPodAutoscaler: + return hpa.Spec.ScaleTargetRef.Name, true + case *autoscalingV2Beta2.HorizontalPodAutoscaler: + return hpa.Spec.ScaleTargetRef.Name, true + case *autoscalingV2.HorizontalPodAutoscaler: + return hpa.Spec.ScaleTargetRef.Name, true + case *autoscalingV1.HorizontalPodAutoscaler: + return hpa.Spec.ScaleTargetRef.Name, true + default: + return "", false + } +} diff --git a/pkg/templates/pdbminavailable/template.go b/pkg/templates/pdbminavailable/template.go index 85caacba1..fac61d830 100644 --- a/pkg/templates/pdbminavailable/template.go +++ b/pkg/templates/pdbminavailable/template.go @@ -90,7 +90,10 @@ func minAvailableCheck(lintCtx lintcontext.LintContext, object lintcontext.Objec } } - // Evaluate Deploymet Likes in the lintContext to see if they have MinAvailable set too low + // Builds an HPA map for that namespace in case there is no replicas set on deployment + hpa := getHorizontalPodAutoscalers(lintCtx, pdb.Namespace) + + // Evaluate Deployment Likes in the lintContext to see if they have MinAvailable set too low deploymentLikes, err := getDeploymentLikeObjects(lintCtx, labelSelector, pdb.Namespace) if err != nil { return []diagnostic.Diagnostic{ @@ -103,6 +106,10 @@ func minAvailableCheck(lintCtx lintcontext.LintContext, object lintcontext.Objec for _, dl := range deploymentLikes { pdbMinAvailable := value replicas, _ := extract.Replicas(dl) + if int(replicas) == 1 { + // if replicas number not set on deployment, use HPA MinReplicas + replicas = transformReplicaIntoMinReplicas(dl, hpa, replicas) + } if isPercent { // Calulate the actual value of the MinAvailable with respect to the Replica count if a percentage is set pdbMinAvailable = int(math.Ceil(float64(replicas) * (float64(value) / float64(100)))) @@ -137,10 +144,6 @@ func getDeploymentLikeObjects(lintCtx lintcontext.LintContext, labelSelector lab if !exists { continue } - // If there are no Replicas set on the Deployment Like, it's not possible to compare to a PDB - if _, ok := extract.Replicas(obj.K8sObject); !ok { - continue - } objLabelSelector, err := metaV1.LabelSelectorAsSelector(selectors) if err != nil { @@ -177,3 +180,39 @@ func getIntOrPercentValueSafelyFromString(intOrStr string) (int, bool, error) { } return v, true, nil } + +// Function to get the list of HPA's provided +func getHorizontalPodAutoscalers(lintCtx lintcontext.LintContext, namespace string) map[string]k8sutil.Object { + + m := make(map[string]k8sutil.Object, len(lintCtx.Objects())) + + for _, obj := range lintCtx.Objects() { + // Ensure that only HPA objects are processed + if obj.GetK8sObjectName().GroupVersionKind.Kind != objectkinds.HorizontalPodAutoscaler { + continue + } + + // Ensure that only HPAs are in the same namespaces as the PDB + if obj.GetK8sObjectName().Namespace != namespace { + continue + } + // validate object with HPA versions using the HPAScaleTargetRefName extractor package function and add to map + hpaSpecScaleTargetRefName, ok := extract.HPAScaleTargetRefName(obj.K8sObject) + if !ok { + continue + } + m[hpaSpecScaleTargetRefName] = obj.K8sObject + } + + return m +} + +// Function to transform the replica count into the minReplicas count if the deployment has a HPA with a minReplicas set +func transformReplicaIntoMinReplicas(deployment k8sutil.Object, hpaMap map[string]k8sutil.Object, replicas int32) int32 { + hpa := hpaMap[deployment.GetName()] + hpaMinReplicas, ok := extract.HPAMinReplicas(hpa) + if !ok { + return replicas + } + return hpaMinReplicas +} diff --git a/pkg/templates/pdbminavailable/template_test.go b/pkg/templates/pdbminavailable/template_test.go index 2fc4d836e..d8469c0b1 100644 --- a/pkg/templates/pdbminavailable/template_test.go +++ b/pkg/templates/pdbminavailable/template_test.go @@ -9,6 +9,7 @@ import ( "golang.stackrox.io/kube-linter/pkg/lintcontext/mocks" "golang.stackrox.io/kube-linter/pkg/templates" appsV1 "k8s.io/api/apps/v1" + autoscalingV2 "k8s.io/api/autoscaling/v2" v1 "k8s.io/api/policy/v1" metaV1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/intstr" @@ -177,3 +178,44 @@ func (p *PDBTestSuite) TestPDBMinAvailableOneHundredPercent() { }, }) } + +// test that the check run with a deployment that has no replicas and a HPA that has a minReplicas +func (p *PDBTestSuite) TestPDBWithMinAvailableHPA() { + p.ctx.AddMockDeployment(p.T(), "test-deploy") + p.ctx.ModifyDeployment(p.T(), "test-deploy", func(deployment *appsV1.Deployment) { + deployment.Namespace = "test" + deployment.Spec.Replicas = nil + deployment.Spec.Selector = &metaV1.LabelSelector{} + deployment.Spec.Selector.MatchLabels = map[string]string{"foo": "bar"} + }) + p.ctx.AddMockHorizontalPodAutoscaler(p.T(), "test-hpa", "v2") + p.ctx.ModifyHorizontalPodAutoscalerV2(p.T(), "test-hpa", func(hpa *autoscalingV2.HorizontalPodAutoscaler) { + hpa.Namespace = "test" + hpa.Spec.ScaleTargetRef = autoscalingV2.CrossVersionObjectReference{ + Kind: "Deployment", + Name: "test-deploy", + APIVersion: "apps/v1", + } + hpa.Spec.MinReplicas = nil + }) + p.ctx.AddMockPodDisruptionBudget(p.T(), "test-pdb") + p.ctx.ModifyPodDisruptionBudget(p.T(), "test-pdb", func(pdb *v1.PodDisruptionBudget) { + pdb.Namespace = "test" + pdb.Spec.Selector = &metaV1.LabelSelector{} + pdb.Spec.Selector.MatchLabels = map[string]string{"foo": "bar"} + pdb.Spec.MinAvailable = &intstr.IntOrString{StrVal: "50%", Type: intstr.String} + }) + + p.Validate(p.ctx, []templates.TestCase{ + { + Param: params.Params{}, + Diagnostics: map[string][]diagnostic.Diagnostic{ + "test-pdb": { + {Message: "The current number of replicas for deployment test-deploy is equal to or lower than the minimum number of replicas specified by its PDB."}, + }, + }, + ExpectInstantiationError: false, + }, + }) + +} diff --git a/tests/checks/pdb-min-available.yaml b/tests/checks/pdb-min-available.yaml index 72a48e9b3..e2e490113 100644 --- a/tests/checks/pdb-min-available.yaml +++ b/tests/checks/pdb-min-available.yaml @@ -20,3 +20,44 @@ spec: selector: matchLabels: name: cloud-ingress-operator +--- +apiVersion: policy/v1 +kind: PodDisruptionBudget +metadata: + name: baz2 + namespace: bar2 +spec: + minAvailable: 1 + selector: + matchLabels: + name: cloud-ingress-operator2 +--- +apiVersion: autoscaling/v2beta1 +kind: HorizontalPodAutoscaler +metadata: + name: app2 + namespace: bar2 +spec: + minReplicas: 1 + maxReplicas: 100 + scaleTargetRef: + apiVersion: apps/v1 + kind: Deployment + name: foo2 + namespace: bar2 + targetCPUUtilizationPercentage: 85 +--- +apiVersion: apps/v1 +kind: Deployment +metadata: + name: foo2 + namespace: bar2 +spec: + replicas: 1 + selector: + matchLabels: + name: cloud-ingress-operator2 + template: + metadata: + labels: + name: cloud-ingress-operator2 \ No newline at end of file