Merge pull request #1876 from jkyros/OCPBUGS12210-jan-30

OCPBUGS-12210: Prevent partially filled HPA behaviors from crashing kube-controller-manager

pkg/controller/podautoscaler/horizontal.go

@@ -48,6 +48,7 @@ import (
 	"k8s.io/client-go/tools/record"
 	"k8s.io/client-go/util/workqueue"
 	"k8s.io/klog/v2"
+	autoscalingapiv2 "k8s.io/kubernetes/pkg/apis/autoscaling/v2"
 	"k8s.io/kubernetes/pkg/controller"
 	metricsclient "k8s.io/kubernetes/pkg/controller/podautoscaler/metrics"
 	"k8s.io/kubernetes/pkg/controller/util/selectors"
@@ -679,6 +680,11 @@ func (a *HorizontalController) reconcileAutoscaler(ctx context.Context, hpaShare
 	hpa := hpaShared.DeepCopy()
 	hpaStatusOriginal := hpa.Status.DeepCopy()
 
+	// Make sure we don't have any nil behaviors before we touch them. They can come in with old v1 objects,
+	// and it's easier here than having to nil check/default later for every calculation
+	// See: https://issues.redhat.com/browse/OCPBUGS-12210
+	a.ensureScalingBehaviorsNotPartiallyPopulated(hpa)
+
 	reference := fmt.Sprintf("%s/%s/%s", hpa.Spec.ScaleTargetRef.Kind, hpa.Namespace, hpa.Spec.ScaleTargetRef.Name)
 
 	targetGV, err := schema.ParseGroupVersion(hpa.Spec.ScaleTargetRef.APIVersion)
@@ -1342,3 +1348,13 @@ func min(a, b int32) int32 {
 	}
 	return b
 }
+
+// ensureScalingBehaviorsNotPartiallyPopulated uses the defualters to ensure that scaling behaviors cannot
+// be partially populated, because if they are, the calculation code will potentially dereference the
+// nil pointers and panic.
+func (a *HorizontalController) ensureScalingBehaviorsNotPartiallyPopulated(hpa *autoscalingv2.HorizontalPodAutoscaler) {
+	if hpa.Spec.Behavior != nil {
+		hpa.Spec.Behavior.ScaleUp = autoscalingapiv2.GenerateHPAScaleUpRules(hpa.Spec.Behavior.ScaleUp)
+		hpa.Spec.Behavior.ScaleDown = autoscalingapiv2.GenerateHPAScaleDownRules(hpa.Spec.Behavior.ScaleDown)
+	}
+}

pkg/controller/podautoscaler/horizontal_test.go

@@ -229,8 +229,11 @@ func (tc *testCase) prepareTestClient(t *testing.T) (*fake.Clientset, *metricsfa
 				LastScaleTime:   tc.lastScaleTime,
 			},
 		}
+		// jkyros: we were cheating our way through our tests by defaulting these here, we would
+		// have caught the "partially filled HPA behaviors" crashes a lot sooner otherwise
+
 		// Initialize default values
-		autoscalingapiv2.SetDefaults_HorizontalPodAutoscalerBehavior(&hpa)
+		// autoscalingapiv2.SetDefaults_HorizontalPodAutoscalerBehavior(&hpa)
 
 		obj := &autoscalingv2.HorizontalPodAutoscalerList{
 			Items: []autoscalingv2.HorizontalPodAutoscaler{hpa},
@@ -4559,3 +4562,44 @@ func TestMultipleHPAs(t *testing.T) {
 
 	assert.Equal(t, hpaCount, len(processedHPA), "Expected to process all HPAs")
 }
+
+// TestPartialBehaviors makes sure that the controller is filling in any
+// partial behaviors that are nil so we don't panic
+// See: https://issues.redhat.com/browse/OCPBUGS-12210
+func TestPartialBehaviors(t *testing.T) {
+
+	t.Run("ensure no panic if scaleDown rules are missing", func(t *testing.T) {
+		tc := testCase{
+			minReplicas:             2,
+			maxReplicas:             6,
+			specReplicas:            3,
+			statusReplicas:          3,
+			expectedDesiredReplicas: 5,
+			CPUTarget:               30,
+			verifyCPUCurrent:        true,
+			reportedLevels:          []uint64{300, 500, 700},
+			reportedCPURequests:     []resource.Quantity{resource.MustParse("1.0"), resource.MustParse("1.0"), resource.MustParse("1.0")},
+			useMetricsAPI:           true,
+			scaleUpRules:            autoscalingapiv2.GenerateHPAScaleUpRules(nil),
+		}
+		tc.runTest(t)
+	})
+
+	t.Run("ensure no panic if scaleUp rules are missing", func(t *testing.T) {
+		tc := testCase{
+			minReplicas:             2,
+			maxReplicas:             6,
+			specReplicas:            5,
+			statusReplicas:          5,
+			expectedDesiredReplicas: 3,
+			CPUTarget:               50,
+			verifyCPUCurrent:        true,
+			reportedLevels:          []uint64{100, 300, 500, 250, 250},
+			reportedCPURequests:     []resource.Quantity{resource.MustParse("1.0"), resource.MustParse("1.0"), resource.MustParse("1.0"), resource.MustParse("1.0"), resource.MustParse("1.0")},
+			useMetricsAPI:           true,
+			recommendations:         []timestampedRecommendation{},
+			scaleDownRules:          autoscalingapiv2.GenerateHPAScaleDownRules(nil),
+		}
+		tc.runTest(t)
+	})
+}