From 6a43026ecf0e2593000e211293db759a3852a60f Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Wed, 18 Dec 2024 09:39:44 -0800 Subject: [PATCH] fix: use CRP defaulter to account for nil CRP policy in eviction controller (#987) --- .../controller.go | 7 ++- .../controller_test.go | 54 +++++++++++-------- 2 files changed, 37 insertions(+), 24 deletions(-) diff --git a/pkg/controllers/clusterresourceplacementeviction/controller.go b/pkg/controllers/clusterresourceplacementeviction/controller.go index 0229f8f69..da1d83c37 100644 --- a/pkg/controllers/clusterresourceplacementeviction/controller.go +++ b/pkg/controllers/clusterresourceplacementeviction/controller.go @@ -25,6 +25,7 @@ import ( bindingutils "go.goms.io/fleet/pkg/utils/binding" "go.goms.io/fleet/pkg/utils/condition" "go.goms.io/fleet/pkg/utils/controller" + "go.goms.io/fleet/pkg/utils/defaulter" ) const ( @@ -102,6 +103,10 @@ func (r *Reconciler) validateEviction(ctx context.Context, eviction *placementv1 } return nil, controller.NewAPIServerError(true, err) } + + // set default values for CRP. + defaulter.SetDefaultsClusterResourcePlacement(&crp) + if crp.DeletionTimestamp != nil { klog.V(2).InfoS(evictionInvalidDeletingCRPMessage, "clusterResourcePlacementEviction", eviction.Name, "clusterResourcePlacement", eviction.Spec.PlacementName) markEvictionInvalid(eviction, evictionInvalidDeletingCRPMessage) @@ -277,7 +282,7 @@ func isEvictionAllowed(bindings []placementv1beta1.ClusterResourceBinding, crp p desiredBindings = int(*crp.Spec.Policy.NumberOfClusters) case placementv1beta1.PickFixedPlacementType: desiredBindings = len(crp.Spec.Policy.ClusterNames) - case placementv1beta1.PickAllPlacementType: + default: // we don't know the desired bindings for PickAll. } diff --git a/pkg/controllers/clusterresourceplacementeviction/controller_test.go b/pkg/controllers/clusterresourceplacementeviction/controller_test.go index 4a69de9c4..6506a921c 100644 --- a/pkg/controllers/clusterresourceplacementeviction/controller_test.go +++ b/pkg/controllers/clusterresourceplacementeviction/controller_test.go @@ -16,11 +16,13 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/intstr" + "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" placementv1alpha1 "go.goms.io/fleet/apis/placement/v1alpha1" placementv1beta1 "go.goms.io/fleet/apis/placement/v1beta1" + "go.goms.io/fleet/pkg/utils/defaulter" ) const ( @@ -31,6 +33,13 @@ const ( testEvictionName = "test-eviction" ) +var ( + validationResultCmpOptions = []cmp.Option{ + cmp.AllowUnexported(evictionValidationResult{}), + cmpopts.IgnoreFields(metav1.ObjectMeta{}, "ResourceVersion"), + } +) + func TestValidateEviction(t *testing.T) { testCRP := &placementv1beta1.ClusterResourcePlacement{ ObjectMeta: metav1.ObjectMeta{ @@ -40,6 +49,18 @@ func TestValidateEviction(t *testing.T) { Policy: &placementv1beta1.PlacementPolicy{ PlacementType: placementv1beta1.PickAllPlacementType, }, + Strategy: placementv1beta1.RolloutStrategy{ + Type: placementv1beta1.RollingUpdateRolloutStrategyType, + RollingUpdate: &placementv1beta1.RollingUpdateConfig{ + MaxUnavailable: ptr.To(intstr.FromString(defaulter.DefaultMaxUnavailableValue)), + MaxSurge: ptr.To(intstr.FromString(defaulter.DefaultMaxSurgeValue)), + UnavailablePeriodSeconds: ptr.To(defaulter.DefaultUnavailablePeriodSeconds), + }, + ApplyStrategy: &placementv1beta1.ApplyStrategy{ + Type: placementv1beta1.ApplyStrategyTypeClientSideApply, + }, + }, + RevisionHistoryLimit: ptr.To(int32(defaulter.DefaultRevisionHistoryLimitValue)), }, } testBinding1 := placementv1beta1.ClusterResourceBinding{ @@ -153,9 +174,14 @@ func TestValidateEviction(t *testing.T) { wantErr: nil, }, { - name: "valid eviction", + name: "CRP with empty spec - valid eviction", eviction: buildTestEviction(testEvictionName, testCRPName, testClusterName), - crp: testCRP, + crp: &placementv1beta1.ClusterResourcePlacement{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-crp", + }, + Spec: placementv1beta1.ClusterResourcePlacementSpec{}, + }, bindings: []placementv1beta1.ClusterResourceBinding{testBinding2}, wantValidationResult: &evictionValidationResult{ isValid: true, @@ -185,7 +211,7 @@ func TestValidateEviction(t *testing.T) { Client: fakeClient, } gotValidationResult, gotErr := r.validateEviction(ctx, tc.eviction) - if diff := cmp.Diff(tc.wantValidationResult, gotValidationResult, cmp.AllowUnexported(evictionValidationResult{}), cmpopts.IgnoreFields(placementv1beta1.ClusterResourceBinding{}, "ResourceVersion")); diff != "" { + if diff := cmp.Diff(tc.wantValidationResult, gotValidationResult, validationResultCmpOptions...); diff != "" { t.Errorf("validateEviction() validation result mismatch (-want, +got):\n%s", diff) } gotInvalidCondition := tc.eviction.GetCondition(string(placementv1alpha1.PlacementEvictionConditionTypeValid)) @@ -503,16 +529,7 @@ func TestExecuteEviction(t *testing.T) { name: "PickAll CRP, Misconfigured PDB MaxUnavailable specified - eviction not executed", validationResult: &evictionValidationResult{ crb: availableBinding, - crp: &placementv1beta1.ClusterResourcePlacement{ - ObjectMeta: metav1.ObjectMeta{ - Name: testCRPName, - }, - Spec: placementv1beta1.ClusterResourcePlacementSpec{ - Policy: &placementv1beta1.PlacementPolicy{ - PlacementType: placementv1beta1.PickAllPlacementType, - }, - }, - }, + crp: ptr.To(buildTestPickAllCRP(testCRPName)), }, eviction: buildTestEviction(testEvictionName, testCRPName, testClusterName), pdb: &placementv1alpha1.ClusterResourcePlacementDisruptionBudget{ @@ -539,16 +556,7 @@ func TestExecuteEviction(t *testing.T) { name: "PickAll CRP, Misconfigured PDB MinAvailable specified as percentage - eviction not executed", validationResult: &evictionValidationResult{ crb: availableBinding, - crp: &placementv1beta1.ClusterResourcePlacement{ - ObjectMeta: metav1.ObjectMeta{ - Name: testCRPName, - }, - Spec: placementv1beta1.ClusterResourcePlacementSpec{ - Policy: &placementv1beta1.PlacementPolicy{ - PlacementType: placementv1beta1.PickAllPlacementType, - }, - }, - }, + crp: ptr.To(buildTestPickAllCRP(testCRPName)), }, eviction: buildTestEviction(testEvictionName, testCRPName, testClusterName), pdb: &placementv1alpha1.ClusterResourcePlacementDisruptionBudget{