Workaround for HPA dry-run metrics duplication

ssa/manager_diff.go

@@ -87,20 +87,25 @@ func (m *ResourceManager) hasDrifted(existingObject, dryRunObject *unstructured.
  return hasObjectDrifted(dryRunObject, existingObject)
 }
 
-// hasObjectDrifted removes the metadata and status fields from both objects
-// then performs a semantic equality check of the remaining fields
-func hasObjectDrifted(dryRunObject, existingObject *unstructured.Unstructured) bool {
- dryRunObj := dryRunObject.DeepCopy()
- unstructured.RemoveNestedField(dryRunObj.Object, "metadata")
- unstructured.RemoveNestedField(dryRunObj.Object, "status")
-
- existingObj := existingObject.DeepCopy()
- unstructured.RemoveNestedField(existingObj.Object, "metadata")
- unstructured.RemoveNestedField(existingObj.Object, "status")
+// hasObjectDrifted performs a semantic equality check of the given objects' spec
+func hasObjectDrifted(existingObject, dryRunObject *unstructured.Unstructured) bool {
+ existingObj := prepareObjectForDiff(existingObject)
+ dryRunObj := prepareObjectForDiff(dryRunObject)
 
  return !apiequality.Semantic.DeepEqual(dryRunObj.Object, existingObj.Object)
 }
 
+// prepareObjectForDiff removes the metadata and status fields from the given object
+func prepareObjectForDiff(object *unstructured.Unstructured) *unstructured.Unstructured {
+ deepCopy := object.DeepCopy()
+ unstructured.RemoveNestedField(deepCopy.Object, "metadata")
+ unstructured.RemoveNestedField(deepCopy.Object, "status")
+ if err := fixHorizontalPodAutoscaler(deepCopy); err != nil {
+ return object
+ }
+ return deepCopy
+}
+
 // validationError formats the given error and hides sensitive data
 // if the error was caused by an invalid Kubernetes secrets.
 func (m *ResourceManager) validationError(object *unstructured.Unstructured, err error) error {

ssa/manager_diff_test.go

@@ -202,3 +202,93 @@ func TestDiff_Removals(t *testing.T) {
  })
 
 }
+
+func TestDiffHPA(t *testing.T) {
+ timeout := 10 * time.Second
+ ctx, cancel := context.WithTimeout(context.Background(), timeout)
+ defer cancel()
+
+ id := generateName("diff")
+ objects, err := readManifest("testdata/test6.yaml", id)
+ if err != nil {
+ t.Fatal(err)
+ }
+
+ hpaName, hpa := getFirstObject(objects, "HorizontalPodAutoscaler", id)
+ var metrics []interface{}
+
+ if _, err = manager.ApplyAllStaged(ctx, objects, DefaultApplyOptions()); err != nil {
+ t.Fatal(err)
+ }
+
+ t.Run("generates empty diff for unchanged object", func(t *testing.T) {
+ changeSetEntry, _, _, err := manager.Diff(ctx, hpa)
+ if err != nil {
+ t.Fatal(err)
+ }
+
+ if diff := cmp.Diff(hpaName, changeSetEntry.Subject); diff != "" {
+ t.Errorf("Mismatch from expected value (-want +got):\n%s", diff)
+ }
+
+ if diff := cmp.Diff(string(UnchangedAction), changeSetEntry.Action); diff != "" {
+ t.Errorf("Mismatch from expected value (-want +got):\n%s", diff)
+ }
+ })
+
+ t.Run("generates diff for removed metric", func(t *testing.T) {
+ metrics, _, err = unstructured.NestedSlice(hpa.Object, "spec", "metrics")
+ if err != nil {
+ t.Fatal(err)
+ }
+
+ err = unstructured.SetNestedSlice(hpa.Object, metrics[:1], "spec", "metrics")
+ if err != nil {
+ t.Fatal(err)
+ }
+
+ changeSetEntry, _, _, err := manager.Diff(ctx, hpa)
+ if err != nil {
+ t.Fatal(err)
+ }
+
+ if diff := cmp.Diff(string(ConfiguredAction), changeSetEntry.Action); diff != "" {
+ t.Errorf("Mismatch from expected value (-want +got):\n%s", diff)
+ }
+
+ if _, err = manager.ApplyAllStaged(ctx, objects, DefaultApplyOptions()); err != nil {
+ t.Fatal(err)
+ }
+ })
+
+ t.Run("generates empty diff for unchanged metric", func(t *testing.T) {
+ changeSetEntry, _, _, err := manager.Diff(ctx, hpa)
+ if err != nil {
+ t.Fatal(err)
+ }
+
+ if diff := cmp.Diff(string(UnchangedAction), changeSetEntry.Action); diff != "" {
+ t.Errorf("Mismatch from expected value (-want +got):\n%s", diff)
+ }
+ })
+
+ t.Run("generates diff for added metric", func(t *testing.T) {
+ err = unstructured.SetNestedSlice(hpa.Object, metrics, "spec", "metrics")
+ if err != nil {
+ t.Fatal(err)
+ }
+
+ changeSetEntry, _, _, err := manager.Diff(ctx, hpa)
+ if err != nil {
+ t.Fatal(err)
+ }
+
+ if diff := cmp.Diff(string(ConfiguredAction), changeSetEntry.Action); diff != "" {
+ t.Errorf("Mismatch from expected value (-want +got):\n%s", diff)
+ }
+
+ if _, err = manager.ApplyAllStaged(ctx, objects, DefaultApplyOptions()); err != nil {
+ t.Fatal(err)
+ }
+ })
+}

ssa/testdata/test6.yaml

@@ -0,0 +1,51 @@
+---
+apiVersion: v1
+kind: Namespace
+metadata:
+ name: "%[1]s"
+---
+apiVersion: autoscaling/v2beta2
+kind: HorizontalPodAutoscaler
+metadata:
+ name: "%[1]s"
+ namespace: "%[1]s"
+spec:
+ behavior:
+ scaleDown:
+ policies:
+ - periodSeconds: 15
+ type: Percent
+ value: 100
+ selectPolicy: Max
+ stabilizationWindowSeconds: 60
+ scaleUp:
+ policies:
+ - periodSeconds: 15
+ type: Pods
+ value: 4
+ - periodSeconds: 15
+ type: Percent
+ value: 100
+ selectPolicy: Max
+ stabilizationWindowSeconds: 0
+ maxReplicas: 30
+ metrics:
+ - type: Pods
+ pods:
+ metric:
+ name: nginx_http_requests_total
+ target:
+ averageValue: "4"
+ type: AverageValue
+ - type: Pods
+ pods:
+ metric:
+ name: nginx_http_requests_total2
+ target:
+ averageValue: "4"
+ type: AverageValue
+ minReplicas: 2
+ scaleTargetRef:
+ apiVersion: apps/v1
+ kind: Deployment
+ name: frontend

ssa/utils.go

@@ -24,9 +24,10 @@ import (
  "strings"
 
  appsv1 "k8s.io/api/apps/v1"
- autoscalingv1 "k8s.io/api/autoscaling/v2beta1"
- autoscalingv2 "k8s.io/api/autoscaling/v2beta2"
+ hpav2beta1 "k8s.io/api/autoscaling/v2beta1"
+ hpav2beta2 "k8s.io/api/autoscaling/v2beta2"
  corev1 "k8s.io/api/core/v1"
+ apiequality "k8s.io/apimachinery/pkg/api/equality"
  "k8s.io/apimachinery/pkg/api/errors"
  "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
  "k8s.io/apimachinery/pkg/runtime"
@@ -350,7 +351,7 @@ func SetNativeKindsDefaults(objects []*unstructured.Unstructured) error {
  case "HorizontalPodAutoscaler":
  switch u.GetAPIVersion() {
  case "autoscaling/v2beta1":
- var d autoscalingv1.HorizontalPodAutoscaler
+ var d hpav2beta1.HorizontalPodAutoscaler
  err := runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, &d)
  if err != nil {
  return fmt.Errorf("%s validation error: %w", FmtUnstructured(u), err)
@@ -361,7 +362,7 @@ func SetNativeKindsDefaults(objects []*unstructured.Unstructured) error {
  }
  u.Object = out
  case "autoscaling/v2beta2":
- var d autoscalingv2.HorizontalPodAutoscaler
+ var d hpav2beta2.HorizontalPodAutoscaler
  err := runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, &d)
  if err != nil {
  return fmt.Errorf("%s validation error: %w", FmtUnstructured(u), err)
@@ -371,8 +372,45 @@ func SetNativeKindsDefaults(objects []*unstructured.Unstructured) error {
  return fmt.Errorf("%s validation error: %w", FmtUnstructured(u), err)
  }
  u.Object = out
+ }
+ }
+ }
+ return nil
+}
+
+// Fix bug in server-side dry-run apply that duplicates the first item in the metrics array
+// and inserts an empty metric as the last item in the array.
+func fixHorizontalPodAutoscaler(object *unstructured.Unstructured) error {
+ if object.GetKind() == "HorizontalPodAutoscaler" {
+ switch object.GetAPIVersion() {
+ case "autoscaling/v2beta2":
+ var d hpav2beta2.HorizontalPodAutoscaler
+ err := runtime.DefaultUnstructuredConverter.FromUnstructured(object.Object, &d)
+ if err != nil {
+ return fmt.Errorf("%s validation error: %w", FmtUnstructured(object), err)
+ }
 
+ var metrics []hpav2beta2.MetricSpec
+ for _, metric := range d.Spec.Metrics {
+ found := false
+ for _, existing := range metrics {
+ if apiequality.Semantic.DeepEqual(metric, existing) {
+ found = true
+ break
+ }
+ }
+ if !found && metric.Type != "" {
+ metrics = append(metrics, metric)
+ }
+ }
+
+ d.Spec.Metrics = metrics
+
+ out, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&d)
+ if err != nil {
+ return fmt.Errorf("%s validation error: %w", FmtUnstructured(object), err)
  }
+ object.Object = out
  }
  }
  return nil