From c4a139b3c272a411344485aa348bc1d863e36827 Mon Sep 17 00:00:00 2001 From: Wei Weng Date: Thu, 30 Oct 2025 16:06:22 +0000 Subject: [PATCH 01/10] add RP validating webhook Signed-off-by: Wei Weng --- ...ourceplacement.go => resourceplacement.go} | 41 ++++++-- ...ment_test.go => resourceplacement_test.go} | 80 +++++++++++++++ pkg/webhook/add_handler.go | 2 + ...a1_resourceplacement_validating_webhook.go | 97 +++++++++++++++++++ 4 files changed, 211 insertions(+), 9 deletions(-) rename pkg/utils/validator/{clusterresourceplacement.go => resourceplacement.go} (94%) rename pkg/utils/validator/{clusterresourceplacement_test.go => resourceplacement_test.go} (95%) create mode 100644 pkg/webhook/resourceplacement/v1beta1_resourceplacement_validating_webhook.go diff --git a/pkg/utils/validator/clusterresourceplacement.go b/pkg/utils/validator/resourceplacement.go similarity index 94% rename from pkg/utils/validator/clusterresourceplacement.go rename to pkg/utils/validator/resourceplacement.go index 29fa95e4f..84d84669c 100644 --- a/pkg/utils/validator/clusterresourceplacement.go +++ b/pkg/utils/validator/resourceplacement.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -// Package validator provides utils to validate cluster resource placement resource. +// Package validator provides utils to validate both cluster resource placement and resource placement resource. package validator import ( @@ -53,15 +53,15 @@ var ( resourceCapacityTypes = supportedResourceCapacityTypes() ) -// ValidateClusterResourcePlacement validates a ClusterResourcePlacement object. -func ValidateClusterResourcePlacement(clusterResourcePlacement *placementv1beta1.ClusterResourcePlacement) error { +// validatePlacement validates a placement object (either ClusterResourcePlacement or ResourcePlacement). +func validatePlacement(name string, resourceSelectors []placementv1beta1.ResourceSelectorTerm, policy *placementv1beta1.PlacementPolicy, strategy placementv1beta1.RolloutStrategy, isClusterScoped bool) error { allErr := make([]error, 0) - if len(clusterResourcePlacement.Name) > validation.DNS1035LabelMaxLength { + if len(name) > validation.DNS1035LabelMaxLength { allErr = append(allErr, fmt.Errorf("the name field cannot have length exceeding %d", validation.DNS1035LabelMaxLength)) } - for _, selector := range clusterResourcePlacement.Spec.ResourceSelectors { + for _, selector := range resourceSelectors { if selector.LabelSelector != nil { if len(selector.Name) != 0 { allErr = append(allErr, fmt.Errorf("the labelSelector and name fields are mutually exclusive in selector %+v", selector)) @@ -84,7 +84,8 @@ func ValidateClusterResourcePlacement(clusterResourcePlacement *placementv1beta1 Version: selector.Version, Kind: selector.Kind, } - if !ResourceInformer.IsClusterScopedResources(gvk) { + // Only check cluster scope for ClusterResourcePlacement + if isClusterScoped && !ResourceInformer.IsClusterScopedResources(gvk) { allErr = append(allErr, fmt.Errorf("the resource is not found in schema (please retry) or it is not a cluster scoped resource: %v", gvk)) } } else { @@ -94,19 +95,41 @@ func ValidateClusterResourcePlacement(clusterResourcePlacement *placementv1beta1 } } - if clusterResourcePlacement.Spec.Policy != nil { - if err := validatePlacementPolicy(clusterResourcePlacement.Spec.Policy); err != nil { + if policy != nil { + if err := validatePlacementPolicy(policy); err != nil { allErr = append(allErr, fmt.Errorf("the placement policy field is invalid: %w", err)) } } - if err := validateRolloutStrategy(clusterResourcePlacement.Spec.Strategy); err != nil { + if err := validateRolloutStrategy(strategy); err != nil { allErr = append(allErr, fmt.Errorf("the rollout Strategy field is invalid: %w", err)) } return apiErrors.NewAggregate(allErr) } +// ValidateClusterResourcePlacement validates a ClusterResourcePlacement object. +func ValidateClusterResourcePlacement(clusterResourcePlacement *placementv1beta1.ClusterResourcePlacement) error { + return validatePlacement( + clusterResourcePlacement.Name, + clusterResourcePlacement.Spec.ResourceSelectors, + clusterResourcePlacement.Spec.Policy, + clusterResourcePlacement.Spec.Strategy, + true, // isClusterScoped + ) +} + +// ValidateResourcePlacement validates a ResourcePlacement object. +func ValidateResourcePlacement(resourcePlacement *placementv1beta1.ResourcePlacement) error { + return validatePlacement( + resourcePlacement.Name, + resourcePlacement.Spec.ResourceSelectors, + resourcePlacement.Spec.Policy, + resourcePlacement.Spec.Strategy, + false, // isClusterScoped + ) +} + func IsPlacementPolicyTypeUpdated(oldPolicy, currentPolicy *placementv1beta1.PlacementPolicy) bool { if oldPolicy == nil && currentPolicy != nil { // if placement policy is left blank, by default PickAll is chosen. diff --git a/pkg/utils/validator/clusterresourceplacement_test.go b/pkg/utils/validator/resourceplacement_test.go similarity index 95% rename from pkg/utils/validator/clusterresourceplacement_test.go rename to pkg/utils/validator/resourceplacement_test.go index aa92c5b15..1003ded9c 100644 --- a/pkg/utils/validator/clusterresourceplacement_test.go +++ b/pkg/utils/validator/resourceplacement_test.go @@ -1719,3 +1719,83 @@ func TestIsTolerationsUpdatedOrDeleted(t *testing.T) { }) } } + +func TestValidateResourcePlacement(t *testing.T) { + tests := map[string]struct { + rp *placementv1beta1.ResourcePlacement + resourceInformer informer.Manager + wantErr bool + wantErrMsg string + }{ + "RP with invalid placement policy": { + rp: &placementv1beta1.ResourcePlacement{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-rp", + }, + Spec: placementv1beta1.PlacementSpec{ + ResourceSelectors: []placementv1beta1.ResourceSelectorTerm{ + { + Group: "apps", + Version: "v1", + Kind: "Deployment", + Name: "test-deployment", + }, + }, + Policy: &placementv1beta1.PlacementPolicy{ + PlacementType: placementv1beta1.PickFixedPlacementType, + ClusterNames: []string{}, // Empty cluster names for PickFixed type + }, + }, + }, + resourceInformer: &testinformer.FakeManager{ + APIResources: map[schema.GroupVersionKind]bool{utils.DeploymentGVK: true}, + IsClusterScopedResource: false, + }, + wantErr: true, + wantErrMsg: "cluster names cannot be empty for policy type PickFixed", + }, + "RP with invalid rollout strategy": { + rp: &placementv1beta1.ResourcePlacement{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-rp", + }, + Spec: placementv1beta1.PlacementSpec{ + ResourceSelectors: []placementv1beta1.ResourceSelectorTerm{ + { + Group: "apps", + Version: "v1", + Kind: "Deployment", + Name: "test-deployment", + }, + }, + Strategy: placementv1beta1.RolloutStrategy{ + Type: placementv1beta1.RollingUpdateRolloutStrategyType, + RollingUpdate: &placementv1beta1.RollingUpdateConfig{ + MaxUnavailable: &intstr.IntOrString{Type: intstr.Int, IntVal: -1}, // Negative value + }, + }, + }, + }, + resourceInformer: &testinformer.FakeManager{ + APIResources: map[schema.GroupVersionKind]bool{utils.DeploymentGVK: true}, + IsClusterScopedResource: false, + }, + wantErr: true, + wantErrMsg: "maxUnavailable must be greater than or equal to 0", + }, + } + + for testName, testCase := range tests { + t.Run(testName, func(t *testing.T) { + RestMapper = utils.TestMapper{} + ResourceInformer = testCase.resourceInformer + gotErr := ValidateResourcePlacement(testCase.rp) + if (gotErr != nil) != testCase.wantErr { + t.Errorf("ValidateResourcePlacement() error = %v, wantErr %v", gotErr, testCase.wantErr) + } + if testCase.wantErr && !strings.Contains(gotErr.Error(), testCase.wantErrMsg) { + t.Errorf("ValidateResourcePlacement() got %v, should contain want %s", gotErr, testCase.wantErrMsg) + } + }) + } +} diff --git a/pkg/webhook/add_handler.go b/pkg/webhook/add_handler.go index e25d92255..08380e6b8 100644 --- a/pkg/webhook/add_handler.go +++ b/pkg/webhook/add_handler.go @@ -10,6 +10,7 @@ import ( "github.com/kubefleet-dev/kubefleet/pkg/webhook/pod" "github.com/kubefleet-dev/kubefleet/pkg/webhook/replicaset" "github.com/kubefleet-dev/kubefleet/pkg/webhook/resourceoverride" + "github.com/kubefleet-dev/kubefleet/pkg/webhook/resourceplacement" ) func init() { @@ -18,6 +19,7 @@ func init() { // AddToManagerFuncs is a list of functions to register webhook validators and mutators to the webhook server AddToManagerFuncs = append(AddToManagerFuncs, clusterresourceplacement.AddMutating) AddToManagerFuncs = append(AddToManagerFuncs, clusterresourceplacement.Add) + AddToManagerFuncs = append(AddToManagerFuncs, resourceplacement.Add) AddToManagerFuncs = append(AddToManagerFuncs, pod.Add) AddToManagerFuncs = append(AddToManagerFuncs, replicaset.Add) AddToManagerFuncs = append(AddToManagerFuncs, membercluster.Add) diff --git a/pkg/webhook/resourceplacement/v1beta1_resourceplacement_validating_webhook.go b/pkg/webhook/resourceplacement/v1beta1_resourceplacement_validating_webhook.go new file mode 100644 index 000000000..ccac5a0f9 --- /dev/null +++ b/pkg/webhook/resourceplacement/v1beta1_resourceplacement_validating_webhook.go @@ -0,0 +1,97 @@ +/* +Copyright 2025 The KubeFleet Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +// Package resourceplacement implements the webhook for v1beta1 ResourcePlacement. +package resourceplacement + +import ( + "context" + "fmt" + "net/http" + + admissionv1 "k8s.io/api/admission/v1" + "k8s.io/apimachinery/pkg/types" + "k8s.io/klog/v2" + "sigs.k8s.io/controller-runtime/pkg/manager" + "sigs.k8s.io/controller-runtime/pkg/webhook" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + placementv1beta1 "github.com/kubefleet-dev/kubefleet/apis/placement/v1beta1" + "github.com/kubefleet-dev/kubefleet/pkg/utils" + "github.com/kubefleet-dev/kubefleet/pkg/utils/validator" +) + +const ( + allowUpdateOldInvalidRPFmt = "allow update on old invalid v1beta1 RP with DeletionTimestamp set" + denyUpdateOldInvalidRPFmt = "deny update on old invalid v1beta1 RP with DeletionTimestamp not set %s" + denyCreateUpdateInvalidRPFmt = "deny create/update v1beta1 RP has invalid fields %s" +) + +var ( + // ValidationPath is the webhook service path which admission requests are routed to for validating v1beta1 RP resources. + ValidationPath = fmt.Sprintf(utils.ValidationPathFmt, placementv1beta1.GroupVersion.Group, placementv1beta1.GroupVersion.Version, "clusterresourceplacement") +) + +type resourcePlacementValidator struct { + decoder webhook.AdmissionDecoder +} + +// Add registers the webhook for K8s bulit-in object types. +func Add(mgr manager.Manager) error { + hookServer := mgr.GetWebhookServer() + hookServer.Register(ValidationPath, &webhook.Admission{Handler: &resourcePlacementValidator{admission.NewDecoder(mgr.GetScheme())}}) + return nil +} + +// Handle resourcePlacementValidator handles create, update RP requests. +func (v *resourcePlacementValidator) Handle(_ context.Context, req admission.Request) admission.Response { + var rp placementv1beta1.ResourcePlacement + if req.Operation == admissionv1.Create || req.Operation == admissionv1.Update { + klog.V(2).InfoS("handling RP", "operation", req.Operation, "namespacedName", types.NamespacedName{Name: req.Name}) + if err := v.decoder.Decode(req, &rp); err != nil { + klog.ErrorS(err, "failed to decode v1beta1 RP object for create/update operation", "userName", req.UserInfo.Username, "groups", req.UserInfo.Groups) + return admission.Errored(http.StatusBadRequest, err) + } + if req.Operation == admissionv1.Update { + var oldRP placementv1beta1.ResourcePlacement + if err := v.decoder.DecodeRaw(req.OldObject, &oldRP); err != nil { + return admission.Errored(http.StatusBadRequest, err) + } + // this is a special case where we allow updates to old v1beta1 RP with invalid fields so that we can + // update the RP to remove finalizer then delete RP. + if err := validator.ValidateResourcePlacement(&oldRP); err != nil { + if rp.DeletionTimestamp != nil { + return admission.Allowed(allowUpdateOldInvalidRPFmt) + } + return admission.Denied(fmt.Sprintf(denyUpdateOldInvalidRPFmt, err)) + } + // handle update case where placement type should be immutable. + if validator.IsPlacementPolicyTypeUpdated(oldRP.Spec.Policy, rp.Spec.Policy) { + return admission.Denied("placement type is immutable") + } + // handle update case where existing tolerations were updated/deleted + if validator.IsTolerationsUpdatedOrDeleted(oldRP.Spec.Tolerations(), rp.Spec.Tolerations()) { + return admission.Denied("tolerations have been updated/deleted, only additions to tolerations are allowed") + } + } + if err := validator.ValidateResourcePlacement(&rp); err != nil { + klog.V(2).InfoS("v1beta1 cluster resource placement has invalid fields, request is denied", "operation", req.Operation, "namespacedName", types.NamespacedName{Name: rp.Name}) + return admission.Denied(fmt.Sprintf(denyCreateUpdateInvalidRPFmt, err)) + } + } + klog.V(2).InfoS("user is allowed to modify v1beta1 cluster resource placement", "operation", req.Operation, "user", req.UserInfo.Username, "group", req.UserInfo.Groups, "namespacedName", types.NamespacedName{Name: rp.Name}) + return admission.Allowed("any user is allowed to modify v1beta1 RP") +} From 1ec2a82fe0f70cbe22f16bb43621684221b6c839 Mon Sep 17 00:00:00 2001 From: Wei Weng Date: Thu, 30 Oct 2025 16:06:22 +0000 Subject: [PATCH 02/10] fix bad copy paste Signed-off-by: Wei Weng --- .../v1beta1_resourceplacement_validating_webhook.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/webhook/resourceplacement/v1beta1_resourceplacement_validating_webhook.go b/pkg/webhook/resourceplacement/v1beta1_resourceplacement_validating_webhook.go index ccac5a0f9..e2e4b484b 100644 --- a/pkg/webhook/resourceplacement/v1beta1_resourceplacement_validating_webhook.go +++ b/pkg/webhook/resourceplacement/v1beta1_resourceplacement_validating_webhook.go @@ -88,10 +88,10 @@ func (v *resourcePlacementValidator) Handle(_ context.Context, req admission.Req } } if err := validator.ValidateResourcePlacement(&rp); err != nil { - klog.V(2).InfoS("v1beta1 cluster resource placement has invalid fields, request is denied", "operation", req.Operation, "namespacedName", types.NamespacedName{Name: rp.Name}) + klog.V(2).InfoS("v1beta1 resource placement has invalid fields, request is denied", "operation", req.Operation, "namespacedName", types.NamespacedName{Name: rp.Name}) return admission.Denied(fmt.Sprintf(denyCreateUpdateInvalidRPFmt, err)) } } - klog.V(2).InfoS("user is allowed to modify v1beta1 cluster resource placement", "operation", req.Operation, "user", req.UserInfo.Username, "group", req.UserInfo.Groups, "namespacedName", types.NamespacedName{Name: rp.Name}) + klog.V(2).InfoS("user is allowed to modify v1beta1 resource placement", "operation", req.Operation, "user", req.UserInfo.Username, "group", req.UserInfo.Groups, "namespacedName", types.NamespacedName{Name: rp.Name}) return admission.Allowed("any user is allowed to modify v1beta1 RP") } From 24048e1b7b3f1fe05d42a8ff70f0a5bb36736cb2 Mon Sep 17 00:00:00 2001 From: Wei Weng Date: Thu, 30 Oct 2025 16:06:22 +0000 Subject: [PATCH 03/10] fix validation path Signed-off-by: Wei Weng --- .../v1beta1_resourceplacement_validating_webhook.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/webhook/resourceplacement/v1beta1_resourceplacement_validating_webhook.go b/pkg/webhook/resourceplacement/v1beta1_resourceplacement_validating_webhook.go index e2e4b484b..e50d53659 100644 --- a/pkg/webhook/resourceplacement/v1beta1_resourceplacement_validating_webhook.go +++ b/pkg/webhook/resourceplacement/v1beta1_resourceplacement_validating_webhook.go @@ -42,7 +42,7 @@ const ( var ( // ValidationPath is the webhook service path which admission requests are routed to for validating v1beta1 RP resources. - ValidationPath = fmt.Sprintf(utils.ValidationPathFmt, placementv1beta1.GroupVersion.Group, placementv1beta1.GroupVersion.Version, "clusterresourceplacement") + ValidationPath = fmt.Sprintf(utils.ValidationPathFmt, placementv1beta1.GroupVersion.Group, placementv1beta1.GroupVersion.Version, "resourceplacement") ) type resourcePlacementValidator struct { From 11275dd05e891c58727c456cf68a43661da65623 Mon Sep 17 00:00:00 2001 From: Wei Weng Date: Thu, 30 Oct 2025 16:06:22 +0000 Subject: [PATCH 04/10] add rp validating webhook tests Signed-off-by: Wei Weng --- ...sourceplacement_validating_webhook_test.go | 432 ++++++++++++++++++ 1 file changed, 432 insertions(+) create mode 100644 pkg/webhook/resourceplacement/v1beta1_resourceplacement_validating_webhook_test.go diff --git a/pkg/webhook/resourceplacement/v1beta1_resourceplacement_validating_webhook_test.go b/pkg/webhook/resourceplacement/v1beta1_resourceplacement_validating_webhook_test.go new file mode 100644 index 000000000..1237dd231 --- /dev/null +++ b/pkg/webhook/resourceplacement/v1beta1_resourceplacement_validating_webhook_test.go @@ -0,0 +1,432 @@ +package resourceplacement + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + "testing" + "time" + + admissionv1 "k8s.io/api/admission/v1" + authenticationv1 "k8s.io/api/authentication/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/intstr" + "k8s.io/utils/ptr" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + placementv1beta1 "github.com/kubefleet-dev/kubefleet/apis/placement/v1beta1" + "github.com/kubefleet-dev/kubefleet/pkg/utils" + "github.com/kubefleet-dev/kubefleet/pkg/utils/informer" + "github.com/kubefleet-dev/kubefleet/pkg/utils/validator" + testinformer "github.com/kubefleet-dev/kubefleet/test/utils/informer" + "github.com/stretchr/testify/assert" +) + +var ( + resourceSelector = placementv1beta1.ResourceSelectorTerm{ + Group: "rbac.authorization.k8s.io", + Version: "v1", + Kind: "ClusterRole", + Name: "test-cluster-role", + } + errString = "the rollout Strategy field is invalid: maxUnavailable must be greater than or equal to 0, got `-1`" +) + +func TestHandle(t *testing.T) { + invalidRPObject := &placementv1beta1.ClusterResourcePlacement{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-rp", + Finalizers: []string{placementv1beta1.PlacementCleanupFinalizer}, + }, + Spec: placementv1beta1.PlacementSpec{ + Policy: &placementv1beta1.PlacementPolicy{ + PlacementType: placementv1beta1.PickAllPlacementType, + }, + ResourceSelectors: []placementv1beta1.ResourceSelectorTerm{resourceSelector}, + Strategy: placementv1beta1.RolloutStrategy{ + Type: placementv1beta1.RollingUpdateRolloutStrategyType, + RollingUpdate: &placementv1beta1.RollingUpdateConfig{ + MaxUnavailable: &intstr.IntOrString{Type: intstr.Int, IntVal: -1}, + }, + }, + }, + } + + invalidRPObjectDeletingFinalizersRemoved := &placementv1beta1.ClusterResourcePlacement{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-rp", + Finalizers: []string{}, + DeletionTimestamp: ptr.To(metav1.NewTime(time.Now())), + }, + Spec: placementv1beta1.PlacementSpec{ + Policy: &placementv1beta1.PlacementPolicy{ + PlacementType: placementv1beta1.PickAllPlacementType, + }, + ResourceSelectors: []placementv1beta1.ResourceSelectorTerm{resourceSelector}, + Strategy: placementv1beta1.RolloutStrategy{ + Type: placementv1beta1.RollingUpdateRolloutStrategyType, + RollingUpdate: &placementv1beta1.RollingUpdateConfig{ + MaxUnavailable: &intstr.IntOrString{Type: intstr.Int, IntVal: -1}, + }, + }, + }, + } + + invalidRPObjectFinalizersRemoved := &placementv1beta1.ClusterResourcePlacement{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-rp", + Finalizers: []string{}, + }, + Spec: placementv1beta1.PlacementSpec{ + Policy: &placementv1beta1.PlacementPolicy{ + PlacementType: placementv1beta1.PickAllPlacementType, + }, + ResourceSelectors: []placementv1beta1.ResourceSelectorTerm{resourceSelector}, + Strategy: placementv1beta1.RolloutStrategy{ + Type: placementv1beta1.RollingUpdateRolloutStrategyType, + RollingUpdate: &placementv1beta1.RollingUpdateConfig{ + MaxUnavailable: &intstr.IntOrString{Type: intstr.Int, IntVal: -1}, + }, + }, + }, + } + + validRPObject := &placementv1beta1.ClusterResourcePlacement{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-rp", + Finalizers: []string{placementv1beta1.PlacementCleanupFinalizer}, + }, + Spec: placementv1beta1.PlacementSpec{ + Policy: &placementv1beta1.PlacementPolicy{ + PlacementType: placementv1beta1.PickAllPlacementType, + }, + ResourceSelectors: []placementv1beta1.ResourceSelectorTerm{resourceSelector}, + Strategy: placementv1beta1.RolloutStrategy{ + Type: placementv1beta1.RollingUpdateRolloutStrategyType, + }, + }, + } + + validRPObjectWithTolerations := &placementv1beta1.ClusterResourcePlacement{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-rp", + }, + Spec: placementv1beta1.PlacementSpec{ + ResourceSelectors: []placementv1beta1.ResourceSelectorTerm{resourceSelector}, + Policy: &placementv1beta1.PlacementPolicy{ + PlacementType: placementv1beta1.PickAllPlacementType, + Tolerations: []placementv1beta1.Toleration{ + { + Key: "key1", + Value: "value1", + }, + }, + }, + Strategy: placementv1beta1.RolloutStrategy{ + Type: placementv1beta1.RollingUpdateRolloutStrategyType, + }, + }, + } + + updatedPlacementTypeRPObject := &placementv1beta1.ClusterResourcePlacement{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-rp", + }, + Spec: placementv1beta1.PlacementSpec{ + Policy: &placementv1beta1.PlacementPolicy{ + PlacementType: placementv1beta1.PickNPlacementType, + NumberOfClusters: ptr.To(int32(2)), + }, + ResourceSelectors: []placementv1beta1.ResourceSelectorTerm{resourceSelector}, + Strategy: placementv1beta1.RolloutStrategy{ + Type: placementv1beta1.RollingUpdateRolloutStrategyType, + }, + }, + } + + validRPObjectBytes, err := json.Marshal(validRPObject) + assert.Nil(t, err) + invalidRPObjectBytes, err := json.Marshal(invalidRPObject) + assert.Nil(t, err) + invalidRPObjectDeletingFinalizersRemovedBytes, err := json.Marshal(invalidRPObjectDeletingFinalizersRemoved) + assert.Nil(t, err) + invalidRPObjectFinalizersRemovedBytes, err := json.Marshal(invalidRPObjectFinalizersRemoved) + assert.Nil(t, err) + updatedPlacementTypeRPObjectBytes, err := json.Marshal(updatedPlacementTypeRPObject) + assert.Nil(t, err) + validRPObjectWithTolerationsBytes, err := json.Marshal(validRPObjectWithTolerations) + assert.Nil(t, err) + + scheme := runtime.NewScheme() + err = placementv1beta1.AddToScheme(scheme) + assert.Nil(t, err) + decoder := admission.NewDecoder(scheme) + assert.Nil(t, err) + + testCases := map[string]struct { + req admission.Request + resourceValidator resourcePlacementValidator + resourceInformer informer.Manager + wantResponse admission.Response + }{ + "allow RP create": { + req: admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Name: "test-rp", + Object: runtime.RawExtension{ + Raw: validRPObjectBytes, + Object: validRPObject, + }, + UserInfo: authenticationv1.UserInfo{ + Username: "test-user", + Groups: []string{"system:masters"}, + }, + RequestKind: &utils.ClusterResourcePlacementMetaGVK, + Operation: admissionv1.Create, + }, + }, + resourceInformer: &testinformer.FakeManager{ + APIResources: map[schema.GroupVersionKind]bool{utils.ClusterRoleGVK: true}, + IsClusterScopedResource: true, + }, + resourceValidator: resourcePlacementValidator{ + decoder: decoder, + }, + wantResponse: admission.Allowed("any user is allowed to modify v1beta1 RP"), + }, + "deny RP create - invalid RP object": { + req: admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Name: "test-rp", + Object: runtime.RawExtension{ + Raw: invalidRPObjectBytes, + Object: invalidRPObject, + }, + UserInfo: authenticationv1.UserInfo{ + Username: "test-user", + Groups: []string{"system:masters"}, + }, + RequestKind: &utils.ClusterResourcePlacementMetaGVK, + Operation: admissionv1.Create, + }, + }, + resourceInformer: &testinformer.FakeManager{ + APIResources: map[schema.GroupVersionKind]bool{utils.ClusterRoleGVK: true}, + IsClusterScopedResource: true, + }, + resourceValidator: resourcePlacementValidator{ + decoder: decoder, + }, + wantResponse: admission.Denied(fmt.Sprintf(denyCreateUpdateInvalidRPFmt, errString)), + }, + "allow RP update - invalid old RP object, invalid new RP is deleting, finalizer removed": { + req: admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Name: "test-rp", + OldObject: runtime.RawExtension{ + Raw: invalidRPObjectBytes, + Object: invalidRPObject, + }, + Object: runtime.RawExtension{ + Raw: invalidRPObjectDeletingFinalizersRemovedBytes, + Object: invalidRPObjectDeletingFinalizersRemoved, + }, + UserInfo: authenticationv1.UserInfo{ + Username: "test-user", + Groups: []string{"system:masters"}, + }, + RequestKind: &utils.ClusterResourcePlacementMetaGVK, + Operation: admissionv1.Update, + }, + }, + resourceInformer: &testinformer.FakeManager{ + APIResources: map[schema.GroupVersionKind]bool{utils.ClusterRoleGVK: true}, + IsClusterScopedResource: true, + }, + resourceValidator: resourcePlacementValidator{ + decoder: decoder, + }, + wantResponse: admission.Allowed(allowUpdateOldInvalidRPFmt), + }, + "deny RP update - invalid old RP, invalid new RP is not deleting, finalizer removed": { + req: admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Name: "test-rp", + OldObject: runtime.RawExtension{ + Raw: invalidRPObjectBytes, + Object: invalidRPObject, + }, + Object: runtime.RawExtension{ + Raw: invalidRPObjectFinalizersRemovedBytes, + Object: invalidRPObjectFinalizersRemoved, + }, + UserInfo: authenticationv1.UserInfo{ + Username: "test-user", + Groups: []string{"system:masters"}, + }, + RequestKind: &utils.ClusterResourcePlacementMetaGVK, + Operation: admissionv1.Update, + }, + }, + resourceInformer: &testinformer.FakeManager{ + APIResources: map[schema.GroupVersionKind]bool{utils.ClusterRoleGVK: true}, + IsClusterScopedResource: true, + }, + resourceValidator: resourcePlacementValidator{ + decoder: decoder, + }, + wantResponse: admission.Denied(fmt.Sprintf(denyUpdateOldInvalidRPFmt, errString)), + }, + "deny RP update - valid old RP, invalid new RP, spec updated": { + req: admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Name: "test-rp", + OldObject: runtime.RawExtension{ + Raw: validRPObjectBytes, + Object: validRPObject, + }, + Object: runtime.RawExtension{ + Raw: invalidRPObjectBytes, + Object: invalidRPObject, + }, + UserInfo: authenticationv1.UserInfo{ + Username: "test-user", + Groups: []string{"system:masters"}, + }, + RequestKind: &utils.ClusterResourcePlacementMetaGVK, + Operation: admissionv1.Update, + }, + }, + resourceInformer: &testinformer.FakeManager{ + APIResources: map[schema.GroupVersionKind]bool{utils.ClusterRoleGVK: true}, + IsClusterScopedResource: true, + }, + resourceValidator: resourcePlacementValidator{ + decoder: decoder, + }, + wantResponse: admission.Denied(fmt.Sprintf(denyCreateUpdateInvalidRPFmt, errString)), + }, + "deny RP update - new RP immutable placement type": { + req: admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Name: "test-rp", + OldObject: runtime.RawExtension{ + Raw: validRPObjectBytes, + Object: validRPObject, + }, + Object: runtime.RawExtension{ + Raw: updatedPlacementTypeRPObjectBytes, + Object: updatedPlacementTypeRPObject, + }, + UserInfo: authenticationv1.UserInfo{ + Username: "test-user", + Groups: []string{"system:masters"}, + }, + RequestKind: &utils.ClusterResourcePlacementMetaGVK, + Operation: admissionv1.Update, + }, + }, + resourceInformer: &testinformer.FakeManager{ + APIResources: map[schema.GroupVersionKind]bool{utils.ClusterRoleGVK: true}, + IsClusterScopedResource: true, + }, + resourceValidator: resourcePlacementValidator{ + decoder: decoder, + }, + wantResponse: admission.Denied("placement type is immutable"), + }, + "deny RP update - new RP tolerations updated": { + req: admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Name: "test-rp", + OldObject: runtime.RawExtension{ + Raw: validRPObjectWithTolerationsBytes, + Object: validRPObjectWithTolerations, + }, + Object: runtime.RawExtension{ + Raw: validRPObjectBytes, + Object: validRPObject, + }, + UserInfo: authenticationv1.UserInfo{ + Username: "test-user", + Groups: []string{"system:masters"}, + }, + RequestKind: &utils.ClusterResourcePlacementMetaGVK, + Operation: admissionv1.Update, + }, + }, + resourceInformer: &testinformer.FakeManager{ + APIResources: map[schema.GroupVersionKind]bool{utils.ClusterRoleGVK: true}, + IsClusterScopedResource: true, + }, + resourceValidator: resourcePlacementValidator{ + decoder: decoder, + }, + wantResponse: admission.Denied("tolerations have been updated/deleted, only additions to tolerations are allowed"), + }, + "decode error for main object": { + req: admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Name: "test-rp", + Object: runtime.RawExtension{ + Raw: []byte(`{"invalid": "json"`), // Invalid JSON + }, + Operation: admissionv1.Create, + }, + }, + resourceInformer: &testinformer.FakeManager{ + APIResources: map[schema.GroupVersionKind]bool{utils.ClusterRoleGVK: true}, + IsClusterScopedResource: true, + }, + resourceValidator: resourcePlacementValidator{ + decoder: decoder, + }, + wantResponse: admission.Errored(http.StatusBadRequest, fmt.Errorf("couldn't get version/kind; json parse error: unexpected end of JSON input")), + }, + "decode error for old object during update": { + req: admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Name: "test-rp", + Object: runtime.RawExtension{ + Raw: func() []byte { + validRP := &placementv1beta1.ResourcePlacement{ + ObjectMeta: metav1.ObjectMeta{Name: "test-rp"}, + Spec: placementv1beta1.PlacementSpec{ + ResourceSelectors: []placementv1beta1.ResourceSelectorTerm{ + {Group: "apps", Version: "v1", Kind: "Deployment", Name: "test"}, + }, + }, + } + validRPBytes, _ := json.Marshal(validRP) + return validRPBytes + }(), + }, + OldObject: runtime.RawExtension{ + Raw: []byte(`{"invalid": "json"`), // Invalid JSON for old object + }, + Operation: admissionv1.Update, + }, + }, + resourceInformer: &testinformer.FakeManager{ + APIResources: map[schema.GroupVersionKind]bool{utils.ClusterRoleGVK: true}, + IsClusterScopedResource: true, + }, + resourceValidator: resourcePlacementValidator{ + decoder: decoder, + }, + wantResponse: admission.Errored(http.StatusBadRequest, fmt.Errorf("couldn't get version/kind; json parse error: unexpected end of JSON input")), + }, + } + + for testName, testCase := range testCases { + t.Run(testName, func(t *testing.T) { + validator.RestMapper = utils.TestMapper{} + validator.ResourceInformer = testCase.resourceInformer + gotResult := testCase.resourceValidator.Handle(context.Background(), testCase.req) + assert.Equal(t, testCase.wantResponse, gotResult, utils.TestCaseMsg, testName) + }) + } +} From 62b9adf86c9e432c3644952ed26b686f8dd6d2e8 Mon Sep 17 00:00:00 2001 From: Wei Weng Date: Thu, 30 Oct 2025 16:12:29 +0000 Subject: [PATCH 05/10] check RP should only deploy namespaced objects Signed-off-by: Wei Weng --- pkg/utils/validator/resourceplacement.go | 6 +- pkg/utils/validator/resourceplacement_test.go | 70 +++++++++++++++++++ 2 files changed, 75 insertions(+), 1 deletion(-) diff --git a/pkg/utils/validator/resourceplacement.go b/pkg/utils/validator/resourceplacement.go index 84d84669c..196ff84b6 100644 --- a/pkg/utils/validator/resourceplacement.go +++ b/pkg/utils/validator/resourceplacement.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -// Package validator provides utils to validate both cluster resource placement and resource placement resource. +// Package validator provides utils to validate all fleet custom resources. package validator import ( @@ -88,6 +88,10 @@ func validatePlacement(name string, resourceSelectors []placementv1beta1.Resourc if isClusterScoped && !ResourceInformer.IsClusterScopedResources(gvk) { allErr = append(allErr, fmt.Errorf("the resource is not found in schema (please retry) or it is not a cluster scoped resource: %v", gvk)) } + + if !isClusterScoped && ResourceInformer.IsClusterScopedResources(gvk) { + allErr = append(allErr, fmt.Errorf("the resource is not found in schema (please retry) or it is a cluster scoped resource: %v", gvk)) + } } else { err := fmt.Errorf("cannot perform resource scope check for now, please retry") klog.ErrorS(controller.NewUnexpectedBehaviorError(err), "resource informer is nil") diff --git a/pkg/utils/validator/resourceplacement_test.go b/pkg/utils/validator/resourceplacement_test.go index 1003ded9c..6afd0ae7f 100644 --- a/pkg/utils/validator/resourceplacement_test.go +++ b/pkg/utils/validator/resourceplacement_test.go @@ -167,6 +167,29 @@ func TestValidateClusterResourcePlacement(t *testing.T) { wantErr: true, wantErrMsg: "cannot perform resource scope check for now, please retry", }, + "CRP with namespaced resource should fail": { + crp: &placementv1beta1.ClusterResourcePlacement{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-crp", + }, + Spec: placementv1beta1.PlacementSpec{ + ResourceSelectors: []placementv1beta1.ResourceSelectorTerm{ + { + Group: "apps", + Version: "v1", + Kind: "Deployment", + Name: "test-deployment", + }, + }, + }, + }, + resourceInformer: &testinformer.FakeManager{ + APIResources: map[schema.GroupVersionKind]bool{utils.DeploymentGVK: true}, + IsClusterScopedResource: false, // Deployment is namespaced + }, + wantErr: true, + wantErrMsg: "resource is not found in schema (please retry) or it is not a cluster scoped resource", + }, } for testName, testCase := range tests { t.Run(testName, func(t *testing.T) { @@ -1783,6 +1806,53 @@ func TestValidateResourcePlacement(t *testing.T) { wantErr: true, wantErrMsg: "maxUnavailable must be greater than or equal to 0", }, + "RP with cluster scoped resource should fail": { + rp: &placementv1beta1.ResourcePlacement{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-rp", + Namespace: "test-namespace", + }, + Spec: placementv1beta1.PlacementSpec{ + ResourceSelectors: []placementv1beta1.ResourceSelectorTerm{ + { + Group: "rbac.authorization.k8s.io", + Version: "v1", + Kind: "ClusterRole", + Name: "test-cluster-role", + }, + }, + }, + }, + resourceInformer: &testinformer.FakeManager{ + APIResources: map[schema.GroupVersionKind]bool{utils.ClusterRoleGVK: true}, + IsClusterScopedResource: true, // ClusterRole is cluster-scoped + }, + wantErr: true, + wantErrMsg: "resource is not found in schema (please retry) or it is a cluster scoped resource", + }, + "RP with namespaced resource should succeed": { + rp: &placementv1beta1.ResourcePlacement{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-rp", + Namespace: "test-namespace", + }, + Spec: placementv1beta1.PlacementSpec{ + ResourceSelectors: []placementv1beta1.ResourceSelectorTerm{ + { + Group: "apps", + Version: "v1", + Kind: "Deployment", + Name: "test-deployment", + }, + }, + }, + }, + resourceInformer: &testinformer.FakeManager{ + APIResources: map[schema.GroupVersionKind]bool{utils.DeploymentGVK: true}, + IsClusterScopedResource: false, // Deployment is namespaced + }, + wantErr: false, + }, } for testName, testCase := range tests { From 86d7dfb568845f45172f9c24160349f0bbd397bd Mon Sep 17 00:00:00 2001 From: Wei Weng Date: Thu, 30 Oct 2025 17:26:35 +0000 Subject: [PATCH 06/10] refactor wip Signed-off-by: Wei Weng --- .../v1beta1/clusterresourceplacement_types.go | 20 +++++ pkg/utils/validator/resourceplacement.go | 71 ++++++++++++++++ ...terresourceplacement_validating_webhook.go | 74 ++++++---------- ...sourceplacement_validating_webhook_test.go | 20 ++--- ...a1_resourceplacement_validating_webhook.go | 85 +++++++++---------- ...sourceplacement_validating_webhook_test.go | 62 +++++++------- 6 files changed, 199 insertions(+), 133 deletions(-) diff --git a/apis/placement/v1beta1/clusterresourceplacement_types.go b/apis/placement/v1beta1/clusterresourceplacement_types.go index f33254773..5aa2862ec 100644 --- a/apis/placement/v1beta1/clusterresourceplacement_types.go +++ b/apis/placement/v1beta1/clusterresourceplacement_types.go @@ -1595,6 +1595,16 @@ func (m *ClusterResourcePlacement) SetPlacementStatus(status PlacementStatus) { status.DeepCopyInto(&m.Status) } +// GetName returns the name of the ClusterResourcePlacement. +func (m *ClusterResourcePlacement) GetName() string { + return m.ObjectMeta.Name +} + +// GetDeletionTimestamp returns the deletion timestamp of the ClusterResourcePlacement. +func (m *ClusterResourcePlacement) GetDeletionTimestamp() *metav1.Time { + return m.ObjectMeta.DeletionTimestamp +} + const ( // ResourcePlacementCleanupFinalizer is a finalizer added by the RP controller to all RPs, to make sure // that the RP controller can react to RP deletions if necessary. @@ -1685,6 +1695,16 @@ func (rpl *ResourcePlacementList) GetPlacementObjs() []PlacementObj { return objs } +// GetName returns the name of the ResourcePlacement. +func (m *ResourcePlacement) GetName() string { + return m.ObjectMeta.Name +} + +// GetDeletionTimestamp returns the deletion timestamp of the ResourcePlacement. +func (m *ResourcePlacement) GetDeletionTimestamp() *metav1.Time { + return m.ObjectMeta.DeletionTimestamp +} + // +genclient // +genclient:Namespaced // +kubebuilder:object:root=true diff --git a/pkg/utils/validator/resourceplacement.go b/pkg/utils/validator/resourceplacement.go index 196ff84b6..bba2cfad0 100644 --- a/pkg/utils/validator/resourceplacement.go +++ b/pkg/utils/validator/resourceplacement.go @@ -18,20 +18,26 @@ limitations under the License. package validator import ( + "context" "errors" "fmt" + "net/http" "sort" "strings" + admissionv1 "k8s.io/api/admission/v1" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" apiErrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/apimachinery/pkg/util/validation" "k8s.io/klog/v2" + "sigs.k8s.io/controller-runtime/pkg/webhook" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" placementv1beta1 "github.com/kubefleet-dev/kubefleet/apis/placement/v1beta1" "github.com/kubefleet-dev/kubefleet/pkg/propertyprovider" @@ -48,6 +54,12 @@ var ( invalidTolerationValueErrFmt = "invalid toleration value %+v: %s" uniqueTolerationErrFmt = "toleration %+v already exists, tolerations must be unique" + // Webhook validation message format strings + AllowUpdateOldInvalidFmt = "allow update on old invalid v1beta1 %s with DeletionTimestamp set" + DenyUpdateOldInvalidFmt = "deny update on old invalid v1beta1 %s with DeletionTimestamp not set %s" + DenyCreateUpdateInvalidFmt = "deny create/update v1beta1 %s has invalid fields %s" + AllowModifyFmt = "any user is allowed to modify v1beta1 %s" + // Below is the map of supported capacity types. supportedResourceCapacityTypesMap = map[string]bool{propertyprovider.AllocatableCapacityName: true, propertyprovider.AvailableCapacityName: true, propertyprovider.TotalCapacityName: true} resourceCapacityTypes = supportedResourceCapacityTypes() @@ -536,3 +548,62 @@ func supportedResourceCapacityTypes() []string { sort.Strings(capacityTypes) return capacityTypes } + +// HandlePlacementValidation provides consolidated webhook validation logic for placement objects. +// This function accepts higher-order functions for type-specific operations. +func HandlePlacementValidation( + ctx context.Context, + req admission.Request, + decoder webhook.AdmissionDecoder, + resourceType string, + decodeFunc func(admission.Request, webhook.AdmissionDecoder) (interface{}, error), + decodeOldFunc func(admission.Request, webhook.AdmissionDecoder) (interface{}, error), + validateFunc func(interface{}) error, + getNameFunc func(interface{}) string, + getDeletionTimestampFunc func(interface{}) *metav1.Time, + getSpecFunc func(interface{}) *placementv1beta1.PlacementSpec, + getTolerationsFunc func(interface{}) []placementv1beta1.Toleration, +) admission.Response { + if req.Operation == admissionv1.Create || req.Operation == admissionv1.Update { + klog.V(2).InfoS("handling placement", "resourceType", resourceType, "operation", req.Operation, "namespacedName", types.NamespacedName{Name: req.Name, Namespace: req.Namespace}) + + placement, err := decodeFunc(req, decoder) + if err != nil { + klog.ErrorS(err, "failed to decode v1beta1 placement object for create/update operation", "resourceType", resourceType, "userName", req.UserInfo.Username, "groups", req.UserInfo.Groups) + return admission.Errored(http.StatusBadRequest, err) + } + + if req.Operation == admissionv1.Update { + oldPlacement, err := decodeOldFunc(req, decoder) + if err != nil { + return admission.Errored(http.StatusBadRequest, err) + } + + // Special case: allow updates to old placement objects with invalid fields so that we can + // update the placement to remove finalizer then delete it. + if err := validateFunc(oldPlacement); err != nil { + if getDeletionTimestampFunc(placement) != nil { + return admission.Allowed(fmt.Sprintf(AllowUpdateOldInvalidFmt, resourceType)) + } + return admission.Denied(fmt.Sprintf(DenyUpdateOldInvalidFmt, resourceType, err)) + } + + // Handle update case where placement type should be immutable. + if IsPlacementPolicyTypeUpdated(getSpecFunc(oldPlacement).Policy, getSpecFunc(placement).Policy) { + return admission.Denied("placement type is immutable") + } + + // Handle update case where existing tolerations were updated/deleted + if IsTolerationsUpdatedOrDeleted(getTolerationsFunc(oldPlacement), getTolerationsFunc(placement)) { + return admission.Denied("tolerations have been updated/deleted, only additions to tolerations are allowed") + } + } + + if err := validateFunc(placement); err != nil { + klog.V(2).InfoS("v1beta1 placement has invalid fields, request is denied", "resourceType", resourceType, "operation", req.Operation, "namespacedName", types.NamespacedName{Name: getNameFunc(placement), Namespace: req.Namespace}) + return admission.Denied(fmt.Sprintf(DenyCreateUpdateInvalidFmt, resourceType, err)) + } + } + + return admission.Allowed(fmt.Sprintf(AllowModifyFmt, resourceType)) +} diff --git a/pkg/webhook/clusterresourceplacement/v1beta1_clusterresourceplacement_validating_webhook.go b/pkg/webhook/clusterresourceplacement/v1beta1_clusterresourceplacement_validating_webhook.go index 7bc326103..26cdc426c 100644 --- a/pkg/webhook/clusterresourceplacement/v1beta1_clusterresourceplacement_validating_webhook.go +++ b/pkg/webhook/clusterresourceplacement/v1beta1_clusterresourceplacement_validating_webhook.go @@ -20,11 +20,8 @@ package clusterresourceplacement import ( "context" "fmt" - "net/http" - admissionv1 "k8s.io/api/admission/v1" - "k8s.io/apimachinery/pkg/types" - "k8s.io/klog/v2" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/manager" "sigs.k8s.io/controller-runtime/pkg/webhook" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" @@ -34,12 +31,6 @@ import ( "github.com/kubefleet-dev/kubefleet/pkg/utils/validator" ) -const ( - allowUpdateOldInvalidCRPFmt = "allow update on old invalid v1beta1 CRP with DeletionTimestamp set" - denyUpdateOldInvalidCRPFmt = "deny update on old invalid v1beta1 CRP with DeletionTimestamp not set %s" - denyCreateUpdateInvalidCRPFmt = "deny create/update v1beta1 CRP has invalid fields %s" -) - var ( // ValidationPath is the webhook service path which admission requests are routed to for validating v1beta1 CRP resources. ValidationPath = fmt.Sprintf(utils.ValidationPathFmt, placementv1beta1.GroupVersion.Group, placementv1beta1.GroupVersion.Version, "clusterresourceplacement") @@ -57,41 +48,32 @@ func Add(mgr manager.Manager) error { } // Handle clusterResourcePlacementValidator handles create, update CRP requests. -func (v *clusterResourcePlacementValidator) Handle(_ context.Context, req admission.Request) admission.Response { - var crp placementv1beta1.ClusterResourcePlacement - if req.Operation == admissionv1.Create || req.Operation == admissionv1.Update { - klog.V(2).InfoS("handling CRP", "operation", req.Operation, "namespacedName", types.NamespacedName{Name: req.Name}) - if err := v.decoder.Decode(req, &crp); err != nil { - klog.ErrorS(err, "failed to decode v1beta1 CRP object for create/update operation", "userName", req.UserInfo.Username, "groups", req.UserInfo.Groups) - return admission.Errored(http.StatusBadRequest, err) - } - if req.Operation == admissionv1.Update { +func (v *clusterResourcePlacementValidator) Handle(ctx context.Context, req admission.Request) admission.Response { + return validator.HandlePlacementValidation(ctx, req, v.decoder, + "CRP", + func(req admission.Request, decoder webhook.AdmissionDecoder) (interface{}, error) { + var crp placementv1beta1.ClusterResourcePlacement + err := decoder.Decode(req, &crp) + return &crp, err + }, + func(req admission.Request, decoder webhook.AdmissionDecoder) (interface{}, error) { var oldCRP placementv1beta1.ClusterResourcePlacement - if err := v.decoder.DecodeRaw(req.OldObject, &oldCRP); err != nil { - return admission.Errored(http.StatusBadRequest, err) - } - // this is a special case where we allow updates to old v1beta1 CRP with invalid fields so that we can - // update the CRP to remove finalizer then delete CRP. - if err := validator.ValidateClusterResourcePlacement(&oldCRP); err != nil { - if crp.DeletionTimestamp != nil { - return admission.Allowed(allowUpdateOldInvalidCRPFmt) - } - return admission.Denied(fmt.Sprintf(denyUpdateOldInvalidCRPFmt, err)) - } - // handle update case where placement type should be immutable. - if validator.IsPlacementPolicyTypeUpdated(oldCRP.Spec.Policy, crp.Spec.Policy) { - return admission.Denied("placement type is immutable") - } - // handle update case where existing tolerations were updated/deleted - if validator.IsTolerationsUpdatedOrDeleted(oldCRP.Spec.Tolerations(), crp.Spec.Tolerations()) { - return admission.Denied("tolerations have been updated/deleted, only additions to tolerations are allowed") - } - } - if err := validator.ValidateClusterResourcePlacement(&crp); err != nil { - klog.V(2).InfoS("v1beta1 cluster resource placement has invalid fields, request is denied", "operation", req.Operation, "namespacedName", types.NamespacedName{Name: crp.Name}) - return admission.Denied(fmt.Sprintf(denyCreateUpdateInvalidCRPFmt, err)) - } - } - klog.V(2).InfoS("user is allowed to modify v1beta1 cluster resource placement", "operation", req.Operation, "user", req.UserInfo.Username, "group", req.UserInfo.Groups, "namespacedName", types.NamespacedName{Name: crp.Name}) - return admission.Allowed("any user is allowed to modify v1beta1 CRP") + err := decoder.DecodeRaw(req.OldObject, &oldCRP) + return &oldCRP, err + }, + func(placement interface{}) error { + return validator.ValidateClusterResourcePlacement(placement.(*placementv1beta1.ClusterResourcePlacement)) + }, + func(placement interface{}) string { + return placement.(*placementv1beta1.ClusterResourcePlacement).Name + }, + func(placement interface{}) *metav1.Time { + return placement.(*placementv1beta1.ClusterResourcePlacement).DeletionTimestamp + }, + func(placement interface{}) *placementv1beta1.PlacementSpec { + return &placement.(*placementv1beta1.ClusterResourcePlacement).Spec + }, + func(placement interface{}) []placementv1beta1.Toleration { + return placement.(*placementv1beta1.ClusterResourcePlacement).Spec.Tolerations() + }) } diff --git a/pkg/webhook/clusterresourceplacement/v1beta1_clusterresourceplacement_validating_webhook_test.go b/pkg/webhook/clusterresourceplacement/v1beta1_clusterresourceplacement_validating_webhook_test.go index 30909093d..a41ea297a 100644 --- a/pkg/webhook/clusterresourceplacement/v1beta1_clusterresourceplacement_validating_webhook_test.go +++ b/pkg/webhook/clusterresourceplacement/v1beta1_clusterresourceplacement_validating_webhook_test.go @@ -279,7 +279,7 @@ func TestHandle(t *testing.T) { resourceValidator: clusterResourcePlacementValidator{ decoder: decoder, }, - wantResponse: admission.Allowed("any user is allowed to modify v1beta1 CRP"), + wantResponse: admission.Allowed(fmt.Sprintf(validator.AllowModifyFmt, "CRP")), }, "deny CRP create - invalid CRP object": { req: admission.Request{ @@ -304,7 +304,7 @@ func TestHandle(t *testing.T) { resourceValidator: clusterResourcePlacementValidator{ decoder: decoder, }, - wantResponse: admission.Denied(fmt.Sprintf(denyCreateUpdateInvalidCRPFmt, errString)), + wantResponse: admission.Denied(fmt.Sprintf(validator.DenyCreateUpdateInvalidFmt, "CRP", errString)), }, "allow CRP update - valid update": { req: admission.Request{ @@ -333,7 +333,7 @@ func TestHandle(t *testing.T) { resourceValidator: clusterResourcePlacementValidator{ decoder: decoder, }, - wantResponse: admission.Allowed("any user is allowed to modify v1beta1 CRP"), + wantResponse: admission.Allowed(fmt.Sprintf(validator.AllowModifyFmt, "CRP")), }, "allow CRP update - invalid old CRP object, invalid new CRP is deleting, finalizer removed": { req: admission.Request{ @@ -362,7 +362,7 @@ func TestHandle(t *testing.T) { resourceValidator: clusterResourcePlacementValidator{ decoder: decoder, }, - wantResponse: admission.Allowed(allowUpdateOldInvalidCRPFmt), + wantResponse: admission.Allowed(fmt.Sprintf(validator.AllowUpdateOldInvalidFmt, "CRP")), }, "allow CRP update - invalid old CRP object, valid new CRP is deleting, finalizer removed, spec updated": { req: admission.Request{ @@ -391,7 +391,7 @@ func TestHandle(t *testing.T) { resourceValidator: clusterResourcePlacementValidator{ decoder: decoder, }, - wantResponse: admission.Allowed(allowUpdateOldInvalidCRPFmt), + wantResponse: admission.Allowed(fmt.Sprintf(validator.AllowUpdateOldInvalidFmt, "CRP")), }, "deny CRP update - invalid old CRP, invalid new CRP is not deleting, finalizer removed": { req: admission.Request{ @@ -420,7 +420,7 @@ func TestHandle(t *testing.T) { resourceValidator: clusterResourcePlacementValidator{ decoder: decoder, }, - wantResponse: admission.Denied(fmt.Sprintf(denyUpdateOldInvalidCRPFmt, errString)), + wantResponse: admission.Denied(fmt.Sprintf(validator.DenyUpdateOldInvalidFmt, "CRP", errString)), }, "allow CRP update - invalid old CRP, invalid new CRP is deleting, finalizer not removed": { req: admission.Request{ @@ -449,7 +449,7 @@ func TestHandle(t *testing.T) { resourceValidator: clusterResourcePlacementValidator{ decoder: decoder, }, - wantResponse: admission.Allowed(allowUpdateOldInvalidCRPFmt), + wantResponse: admission.Allowed(fmt.Sprintf(validator.AllowUpdateOldInvalidFmt, "CRP")), }, "deny CRP update - invalid old CRP, invalid new CRP, label is updated": { req: admission.Request{ @@ -478,7 +478,7 @@ func TestHandle(t *testing.T) { resourceValidator: clusterResourcePlacementValidator{ decoder: decoder, }, - wantResponse: admission.Denied(fmt.Sprintf(denyUpdateOldInvalidCRPFmt, errString)), + wantResponse: admission.Denied(fmt.Sprintf(validator.DenyUpdateOldInvalidFmt, "CRP", errString)), }, "deny CRP update - invalid old CRP, valid new CRP, spec updated": { req: admission.Request{ @@ -507,7 +507,7 @@ func TestHandle(t *testing.T) { resourceValidator: clusterResourcePlacementValidator{ decoder: decoder, }, - wantResponse: admission.Denied(fmt.Sprintf(denyUpdateOldInvalidCRPFmt, errString)), + wantResponse: admission.Denied(fmt.Sprintf(validator.DenyUpdateOldInvalidFmt, "CRP", errString)), }, "deny CRP update - valid old CRP, invalid new CRP, spec updated": { req: admission.Request{ @@ -536,7 +536,7 @@ func TestHandle(t *testing.T) { resourceValidator: clusterResourcePlacementValidator{ decoder: decoder, }, - wantResponse: admission.Denied(fmt.Sprintf(denyCreateUpdateInvalidCRPFmt, errString)), + wantResponse: admission.Denied(fmt.Sprintf(validator.DenyCreateUpdateInvalidFmt, "CRP", errString)), }, "deny CRP update - new CRP immutable placement type": { req: admission.Request{ diff --git a/pkg/webhook/resourceplacement/v1beta1_resourceplacement_validating_webhook.go b/pkg/webhook/resourceplacement/v1beta1_resourceplacement_validating_webhook.go index e50d53659..1a8373530 100644 --- a/pkg/webhook/resourceplacement/v1beta1_resourceplacement_validating_webhook.go +++ b/pkg/webhook/resourceplacement/v1beta1_resourceplacement_validating_webhook.go @@ -20,11 +20,8 @@ package resourceplacement import ( "context" "fmt" - "net/http" - admissionv1 "k8s.io/api/admission/v1" - "k8s.io/apimachinery/pkg/types" - "k8s.io/klog/v2" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/manager" "sigs.k8s.io/controller-runtime/pkg/webhook" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" @@ -34,12 +31,6 @@ import ( "github.com/kubefleet-dev/kubefleet/pkg/utils/validator" ) -const ( - allowUpdateOldInvalidRPFmt = "allow update on old invalid v1beta1 RP with DeletionTimestamp set" - denyUpdateOldInvalidRPFmt = "deny update on old invalid v1beta1 RP with DeletionTimestamp not set %s" - denyCreateUpdateInvalidRPFmt = "deny create/update v1beta1 RP has invalid fields %s" -) - var ( // ValidationPath is the webhook service path which admission requests are routed to for validating v1beta1 RP resources. ValidationPath = fmt.Sprintf(utils.ValidationPathFmt, placementv1beta1.GroupVersion.Group, placementv1beta1.GroupVersion.Version, "resourceplacement") @@ -57,41 +48,43 @@ func Add(mgr manager.Manager) error { } // Handle resourcePlacementValidator handles create, update RP requests. -func (v *resourcePlacementValidator) Handle(_ context.Context, req admission.Request) admission.Response { - var rp placementv1beta1.ResourcePlacement - if req.Operation == admissionv1.Create || req.Operation == admissionv1.Update { - klog.V(2).InfoS("handling RP", "operation", req.Operation, "namespacedName", types.NamespacedName{Name: req.Name}) - if err := v.decoder.Decode(req, &rp); err != nil { - klog.ErrorS(err, "failed to decode v1beta1 RP object for create/update operation", "userName", req.UserInfo.Username, "groups", req.UserInfo.Groups) - return admission.Errored(http.StatusBadRequest, err) - } - if req.Operation == admissionv1.Update { +func (v *resourcePlacementValidator) Handle(ctx context.Context, req admission.Request) admission.Response { + return validator.HandlePlacementValidation( + ctx, + req, + v.decoder, + "RP", + // decodeFunc + func(req admission.Request, decoder webhook.AdmissionDecoder) (interface{}, error) { + var rp placementv1beta1.ResourcePlacement + err := decoder.Decode(req, &rp) + return &rp, err + }, + // decodeOldFunc + func(req admission.Request, decoder webhook.AdmissionDecoder) (interface{}, error) { var oldRP placementv1beta1.ResourcePlacement - if err := v.decoder.DecodeRaw(req.OldObject, &oldRP); err != nil { - return admission.Errored(http.StatusBadRequest, err) - } - // this is a special case where we allow updates to old v1beta1 RP with invalid fields so that we can - // update the RP to remove finalizer then delete RP. - if err := validator.ValidateResourcePlacement(&oldRP); err != nil { - if rp.DeletionTimestamp != nil { - return admission.Allowed(allowUpdateOldInvalidRPFmt) - } - return admission.Denied(fmt.Sprintf(denyUpdateOldInvalidRPFmt, err)) - } - // handle update case where placement type should be immutable. - if validator.IsPlacementPolicyTypeUpdated(oldRP.Spec.Policy, rp.Spec.Policy) { - return admission.Denied("placement type is immutable") - } - // handle update case where existing tolerations were updated/deleted - if validator.IsTolerationsUpdatedOrDeleted(oldRP.Spec.Tolerations(), rp.Spec.Tolerations()) { - return admission.Denied("tolerations have been updated/deleted, only additions to tolerations are allowed") - } - } - if err := validator.ValidateResourcePlacement(&rp); err != nil { - klog.V(2).InfoS("v1beta1 resource placement has invalid fields, request is denied", "operation", req.Operation, "namespacedName", types.NamespacedName{Name: rp.Name}) - return admission.Denied(fmt.Sprintf(denyCreateUpdateInvalidRPFmt, err)) - } - } - klog.V(2).InfoS("user is allowed to modify v1beta1 resource placement", "operation", req.Operation, "user", req.UserInfo.Username, "group", req.UserInfo.Groups, "namespacedName", types.NamespacedName{Name: rp.Name}) - return admission.Allowed("any user is allowed to modify v1beta1 RP") + err := decoder.DecodeRaw(req.OldObject, &oldRP) + return &oldRP, err + }, + // validateFunc + func(obj interface{}) error { + return validator.ValidateResourcePlacement(obj.(*placementv1beta1.ResourcePlacement)) + }, + // getNameFunc + func(obj interface{}) string { + return obj.(*placementv1beta1.ResourcePlacement).Name + }, + // getDeletionTimestampFunc + func(obj interface{}) *metav1.Time { + return obj.(*placementv1beta1.ResourcePlacement).DeletionTimestamp + }, + // getSpecFunc + func(obj interface{}) *placementv1beta1.PlacementSpec { + return &obj.(*placementv1beta1.ResourcePlacement).Spec + }, + // getTolerationsFunc + func(obj interface{}) []placementv1beta1.Toleration { + return obj.(*placementv1beta1.ResourcePlacement).Spec.Tolerations() + }, + ) } diff --git a/pkg/webhook/resourceplacement/v1beta1_resourceplacement_validating_webhook_test.go b/pkg/webhook/resourceplacement/v1beta1_resourceplacement_validating_webhook_test.go index 1237dd231..b06519b65 100644 --- a/pkg/webhook/resourceplacement/v1beta1_resourceplacement_validating_webhook_test.go +++ b/pkg/webhook/resourceplacement/v1beta1_resourceplacement_validating_webhook_test.go @@ -27,16 +27,16 @@ import ( var ( resourceSelector = placementv1beta1.ResourceSelectorTerm{ - Group: "rbac.authorization.k8s.io", + Group: "", Version: "v1", - Kind: "ClusterRole", + Kind: "ConfigMap", Name: "test-cluster-role", } errString = "the rollout Strategy field is invalid: maxUnavailable must be greater than or equal to 0, got `-1`" ) func TestHandle(t *testing.T) { - invalidRPObject := &placementv1beta1.ClusterResourcePlacement{ + invalidRPObject := &placementv1beta1.ResourcePlacement{ ObjectMeta: metav1.ObjectMeta{ Name: "test-rp", Finalizers: []string{placementv1beta1.PlacementCleanupFinalizer}, @@ -55,7 +55,7 @@ func TestHandle(t *testing.T) { }, } - invalidRPObjectDeletingFinalizersRemoved := &placementv1beta1.ClusterResourcePlacement{ + invalidRPObjectDeletingFinalizersRemoved := &placementv1beta1.ResourcePlacement{ ObjectMeta: metav1.ObjectMeta{ Name: "test-rp", Finalizers: []string{}, @@ -75,7 +75,7 @@ func TestHandle(t *testing.T) { }, } - invalidRPObjectFinalizersRemoved := &placementv1beta1.ClusterResourcePlacement{ + invalidRPObjectFinalizersRemoved := &placementv1beta1.ResourcePlacement{ ObjectMeta: metav1.ObjectMeta{ Name: "test-rp", Finalizers: []string{}, @@ -94,7 +94,7 @@ func TestHandle(t *testing.T) { }, } - validRPObject := &placementv1beta1.ClusterResourcePlacement{ + validRPObject := &placementv1beta1.ResourcePlacement{ ObjectMeta: metav1.ObjectMeta{ Name: "test-rp", Finalizers: []string{placementv1beta1.PlacementCleanupFinalizer}, @@ -110,7 +110,7 @@ func TestHandle(t *testing.T) { }, } - validRPObjectWithTolerations := &placementv1beta1.ClusterResourcePlacement{ + validRPObjectWithTolerations := &placementv1beta1.ResourcePlacement{ ObjectMeta: metav1.ObjectMeta{ Name: "test-rp", }, @@ -131,7 +131,7 @@ func TestHandle(t *testing.T) { }, } - updatedPlacementTypeRPObject := &placementv1beta1.ClusterResourcePlacement{ + updatedPlacementTypeRPObject := &placementv1beta1.ResourcePlacement{ ObjectMeta: metav1.ObjectMeta{ Name: "test-rp", }, @@ -189,13 +189,13 @@ func TestHandle(t *testing.T) { }, }, resourceInformer: &testinformer.FakeManager{ - APIResources: map[schema.GroupVersionKind]bool{utils.ClusterRoleGVK: true}, - IsClusterScopedResource: true, + APIResources: map[schema.GroupVersionKind]bool{utils.ConfigMapGVK: true}, + IsClusterScopedResource: false, }, resourceValidator: resourcePlacementValidator{ decoder: decoder, }, - wantResponse: admission.Allowed("any user is allowed to modify v1beta1 RP"), + wantResponse: admission.Allowed(fmt.Sprintf(validator.AllowModifyFmt, "RP")), }, "deny RP create - invalid RP object": { req: admission.Request{ @@ -214,13 +214,13 @@ func TestHandle(t *testing.T) { }, }, resourceInformer: &testinformer.FakeManager{ - APIResources: map[schema.GroupVersionKind]bool{utils.ClusterRoleGVK: true}, - IsClusterScopedResource: true, + APIResources: map[schema.GroupVersionKind]bool{utils.ConfigMapGVK: true}, + IsClusterScopedResource: false, }, resourceValidator: resourcePlacementValidator{ decoder: decoder, }, - wantResponse: admission.Denied(fmt.Sprintf(denyCreateUpdateInvalidRPFmt, errString)), + wantResponse: admission.Denied(fmt.Sprintf(validator.DenyCreateUpdateInvalidFmt, "RP", errString)), }, "allow RP update - invalid old RP object, invalid new RP is deleting, finalizer removed": { req: admission.Request{ @@ -243,13 +243,13 @@ func TestHandle(t *testing.T) { }, }, resourceInformer: &testinformer.FakeManager{ - APIResources: map[schema.GroupVersionKind]bool{utils.ClusterRoleGVK: true}, - IsClusterScopedResource: true, + APIResources: map[schema.GroupVersionKind]bool{utils.ConfigMapGVK: true}, + IsClusterScopedResource: false, }, resourceValidator: resourcePlacementValidator{ decoder: decoder, }, - wantResponse: admission.Allowed(allowUpdateOldInvalidRPFmt), + wantResponse: admission.Allowed(fmt.Sprintf(validator.AllowUpdateOldInvalidFmt, "RP")), }, "deny RP update - invalid old RP, invalid new RP is not deleting, finalizer removed": { req: admission.Request{ @@ -272,13 +272,13 @@ func TestHandle(t *testing.T) { }, }, resourceInformer: &testinformer.FakeManager{ - APIResources: map[schema.GroupVersionKind]bool{utils.ClusterRoleGVK: true}, - IsClusterScopedResource: true, + APIResources: map[schema.GroupVersionKind]bool{utils.ConfigMapGVK: true}, + IsClusterScopedResource: false, }, resourceValidator: resourcePlacementValidator{ decoder: decoder, }, - wantResponse: admission.Denied(fmt.Sprintf(denyUpdateOldInvalidRPFmt, errString)), + wantResponse: admission.Denied(fmt.Sprintf(validator.DenyUpdateOldInvalidFmt, "RP", errString)), }, "deny RP update - valid old RP, invalid new RP, spec updated": { req: admission.Request{ @@ -301,13 +301,13 @@ func TestHandle(t *testing.T) { }, }, resourceInformer: &testinformer.FakeManager{ - APIResources: map[schema.GroupVersionKind]bool{utils.ClusterRoleGVK: true}, - IsClusterScopedResource: true, + APIResources: map[schema.GroupVersionKind]bool{utils.ConfigMapGVK: true}, + IsClusterScopedResource: false, }, resourceValidator: resourcePlacementValidator{ decoder: decoder, }, - wantResponse: admission.Denied(fmt.Sprintf(denyCreateUpdateInvalidRPFmt, errString)), + wantResponse: admission.Denied(fmt.Sprintf(validator.DenyCreateUpdateInvalidFmt, "RP", errString)), }, "deny RP update - new RP immutable placement type": { req: admission.Request{ @@ -330,8 +330,8 @@ func TestHandle(t *testing.T) { }, }, resourceInformer: &testinformer.FakeManager{ - APIResources: map[schema.GroupVersionKind]bool{utils.ClusterRoleGVK: true}, - IsClusterScopedResource: true, + APIResources: map[schema.GroupVersionKind]bool{utils.ConfigMapGVK: true}, + IsClusterScopedResource: false, }, resourceValidator: resourcePlacementValidator{ decoder: decoder, @@ -359,8 +359,8 @@ func TestHandle(t *testing.T) { }, }, resourceInformer: &testinformer.FakeManager{ - APIResources: map[schema.GroupVersionKind]bool{utils.ClusterRoleGVK: true}, - IsClusterScopedResource: true, + APIResources: map[schema.GroupVersionKind]bool{utils.ConfigMapGVK: true}, + IsClusterScopedResource: false, }, resourceValidator: resourcePlacementValidator{ decoder: decoder, @@ -378,8 +378,8 @@ func TestHandle(t *testing.T) { }, }, resourceInformer: &testinformer.FakeManager{ - APIResources: map[schema.GroupVersionKind]bool{utils.ClusterRoleGVK: true}, - IsClusterScopedResource: true, + APIResources: map[schema.GroupVersionKind]bool{utils.ConfigMapGVK: true}, + IsClusterScopedResource: false, }, resourceValidator: resourcePlacementValidator{ decoder: decoder, @@ -411,8 +411,8 @@ func TestHandle(t *testing.T) { }, }, resourceInformer: &testinformer.FakeManager{ - APIResources: map[schema.GroupVersionKind]bool{utils.ClusterRoleGVK: true}, - IsClusterScopedResource: true, + APIResources: map[schema.GroupVersionKind]bool{utils.ConfigMapGVK: true}, + IsClusterScopedResource: false, }, resourceValidator: resourcePlacementValidator{ decoder: decoder, From 215ec06829872bc6fe1844a0515ccaed17f2ae2f Mon Sep 17 00:00:00 2001 From: Wei Weng Date: Thu, 30 Oct 2025 17:57:08 +0000 Subject: [PATCH 07/10] simplify common validation func Signed-off-by: Wei Weng --- pkg/utils/validator/resourceplacement.go | 24 ++++++++++--------- ...terresourceplacement_validating_webhook.go | 24 ++++++------------- ...a1_resourceplacement_validating_webhook.go | 23 +++--------------- ...sourceplacement_validating_webhook_test.go | 24 +++++++++---------- 4 files changed, 35 insertions(+), 60 deletions(-) diff --git a/pkg/utils/validator/resourceplacement.go b/pkg/utils/validator/resourceplacement.go index bba2cfad0..c6bb586bc 100644 --- a/pkg/utils/validator/resourceplacement.go +++ b/pkg/utils/validator/resourceplacement.go @@ -549,6 +549,12 @@ func supportedResourceCapacityTypes() []string { return capacityTypes } +type ResourcePlacementObject interface { + GetPlacementSpec() *placementv1beta1.PlacementSpec + GetName() string + GetDeletionTimestamp() *metav1.Time +} + // HandlePlacementValidation provides consolidated webhook validation logic for placement objects. // This function accepts higher-order functions for type-specific operations. func HandlePlacementValidation( @@ -556,13 +562,9 @@ func HandlePlacementValidation( req admission.Request, decoder webhook.AdmissionDecoder, resourceType string, - decodeFunc func(admission.Request, webhook.AdmissionDecoder) (interface{}, error), - decodeOldFunc func(admission.Request, webhook.AdmissionDecoder) (interface{}, error), - validateFunc func(interface{}) error, - getNameFunc func(interface{}) string, - getDeletionTimestampFunc func(interface{}) *metav1.Time, - getSpecFunc func(interface{}) *placementv1beta1.PlacementSpec, - getTolerationsFunc func(interface{}) []placementv1beta1.Toleration, + decodeFunc func(admission.Request, webhook.AdmissionDecoder) (ResourcePlacementObject, error), + decodeOldFunc func(admission.Request, webhook.AdmissionDecoder) (ResourcePlacementObject, error), + validateFunc func(ResourcePlacementObject) error, ) admission.Response { if req.Operation == admissionv1.Create || req.Operation == admissionv1.Update { klog.V(2).InfoS("handling placement", "resourceType", resourceType, "operation", req.Operation, "namespacedName", types.NamespacedName{Name: req.Name, Namespace: req.Namespace}) @@ -582,25 +584,25 @@ func HandlePlacementValidation( // Special case: allow updates to old placement objects with invalid fields so that we can // update the placement to remove finalizer then delete it. if err := validateFunc(oldPlacement); err != nil { - if getDeletionTimestampFunc(placement) != nil { + if placement.GetDeletionTimestamp() != nil { return admission.Allowed(fmt.Sprintf(AllowUpdateOldInvalidFmt, resourceType)) } return admission.Denied(fmt.Sprintf(DenyUpdateOldInvalidFmt, resourceType, err)) } // Handle update case where placement type should be immutable. - if IsPlacementPolicyTypeUpdated(getSpecFunc(oldPlacement).Policy, getSpecFunc(placement).Policy) { + if IsPlacementPolicyTypeUpdated(oldPlacement.GetPlacementSpec().Policy, placement.GetPlacementSpec().Policy) { return admission.Denied("placement type is immutable") } // Handle update case where existing tolerations were updated/deleted - if IsTolerationsUpdatedOrDeleted(getTolerationsFunc(oldPlacement), getTolerationsFunc(placement)) { + if IsTolerationsUpdatedOrDeleted(oldPlacement.GetPlacementSpec().Tolerations(), placement.GetPlacementSpec().Tolerations()) { return admission.Denied("tolerations have been updated/deleted, only additions to tolerations are allowed") } } if err := validateFunc(placement); err != nil { - klog.V(2).InfoS("v1beta1 placement has invalid fields, request is denied", "resourceType", resourceType, "operation", req.Operation, "namespacedName", types.NamespacedName{Name: getNameFunc(placement), Namespace: req.Namespace}) + klog.V(2).InfoS("v1beta1 placement has invalid fields, request is denied", "resourceType", resourceType, "operation", req.Operation, "namespacedName", types.NamespacedName{Name: placement.GetName(), Namespace: req.Namespace}) return admission.Denied(fmt.Sprintf(DenyCreateUpdateInvalidFmt, resourceType, err)) } } diff --git a/pkg/webhook/clusterresourceplacement/v1beta1_clusterresourceplacement_validating_webhook.go b/pkg/webhook/clusterresourceplacement/v1beta1_clusterresourceplacement_validating_webhook.go index 26cdc426c..930a2828a 100644 --- a/pkg/webhook/clusterresourceplacement/v1beta1_clusterresourceplacement_validating_webhook.go +++ b/pkg/webhook/clusterresourceplacement/v1beta1_clusterresourceplacement_validating_webhook.go @@ -21,7 +21,6 @@ import ( "context" "fmt" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/manager" "sigs.k8s.io/controller-runtime/pkg/webhook" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" @@ -51,29 +50,20 @@ func Add(mgr manager.Manager) error { func (v *clusterResourcePlacementValidator) Handle(ctx context.Context, req admission.Request) admission.Response { return validator.HandlePlacementValidation(ctx, req, v.decoder, "CRP", - func(req admission.Request, decoder webhook.AdmissionDecoder) (interface{}, error) { + // decodeFunc + func(req admission.Request, decoder webhook.AdmissionDecoder) (validator.ResourcePlacementObject, error) { var crp placementv1beta1.ClusterResourcePlacement err := decoder.Decode(req, &crp) return &crp, err }, - func(req admission.Request, decoder webhook.AdmissionDecoder) (interface{}, error) { + // decodeOldFunc + func(req admission.Request, decoder webhook.AdmissionDecoder) (validator.ResourcePlacementObject, error) { var oldCRP placementv1beta1.ClusterResourcePlacement err := decoder.DecodeRaw(req.OldObject, &oldCRP) return &oldCRP, err }, - func(placement interface{}) error { - return validator.ValidateClusterResourcePlacement(placement.(*placementv1beta1.ClusterResourcePlacement)) - }, - func(placement interface{}) string { - return placement.(*placementv1beta1.ClusterResourcePlacement).Name - }, - func(placement interface{}) *metav1.Time { - return placement.(*placementv1beta1.ClusterResourcePlacement).DeletionTimestamp - }, - func(placement interface{}) *placementv1beta1.PlacementSpec { - return &placement.(*placementv1beta1.ClusterResourcePlacement).Spec - }, - func(placement interface{}) []placementv1beta1.Toleration { - return placement.(*placementv1beta1.ClusterResourcePlacement).Spec.Tolerations() + // validateFunc + func(obj validator.ResourcePlacementObject) error { + return validator.ValidateClusterResourcePlacement(obj.(*placementv1beta1.ClusterResourcePlacement)) }) } diff --git a/pkg/webhook/resourceplacement/v1beta1_resourceplacement_validating_webhook.go b/pkg/webhook/resourceplacement/v1beta1_resourceplacement_validating_webhook.go index 1a8373530..c936b9265 100644 --- a/pkg/webhook/resourceplacement/v1beta1_resourceplacement_validating_webhook.go +++ b/pkg/webhook/resourceplacement/v1beta1_resourceplacement_validating_webhook.go @@ -21,7 +21,6 @@ import ( "context" "fmt" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/manager" "sigs.k8s.io/controller-runtime/pkg/webhook" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" @@ -55,36 +54,20 @@ func (v *resourcePlacementValidator) Handle(ctx context.Context, req admission.R v.decoder, "RP", // decodeFunc - func(req admission.Request, decoder webhook.AdmissionDecoder) (interface{}, error) { + func(req admission.Request, decoder webhook.AdmissionDecoder) (validator.ResourcePlacementObject, error) { var rp placementv1beta1.ResourcePlacement err := decoder.Decode(req, &rp) return &rp, err }, // decodeOldFunc - func(req admission.Request, decoder webhook.AdmissionDecoder) (interface{}, error) { + func(req admission.Request, decoder webhook.AdmissionDecoder) (validator.ResourcePlacementObject, error) { var oldRP placementv1beta1.ResourcePlacement err := decoder.DecodeRaw(req.OldObject, &oldRP) return &oldRP, err }, // validateFunc - func(obj interface{}) error { + func(obj validator.ResourcePlacementObject) error { return validator.ValidateResourcePlacement(obj.(*placementv1beta1.ResourcePlacement)) }, - // getNameFunc - func(obj interface{}) string { - return obj.(*placementv1beta1.ResourcePlacement).Name - }, - // getDeletionTimestampFunc - func(obj interface{}) *metav1.Time { - return obj.(*placementv1beta1.ResourcePlacement).DeletionTimestamp - }, - // getSpecFunc - func(obj interface{}) *placementv1beta1.PlacementSpec { - return &obj.(*placementv1beta1.ResourcePlacement).Spec - }, - // getTolerationsFunc - func(obj interface{}) []placementv1beta1.Toleration { - return obj.(*placementv1beta1.ResourcePlacement).Spec.Tolerations() - }, ) } diff --git a/pkg/webhook/resourceplacement/v1beta1_resourceplacement_validating_webhook_test.go b/pkg/webhook/resourceplacement/v1beta1_resourceplacement_validating_webhook_test.go index b06519b65..2a0a7a591 100644 --- a/pkg/webhook/resourceplacement/v1beta1_resourceplacement_validating_webhook_test.go +++ b/pkg/webhook/resourceplacement/v1beta1_resourceplacement_validating_webhook_test.go @@ -27,10 +27,10 @@ import ( var ( resourceSelector = placementv1beta1.ResourceSelectorTerm{ - Group: "", + Group: "apps", Version: "v1", - Kind: "ConfigMap", - Name: "test-cluster-role", + Kind: "Deployment", + Name: "test-deployment", } errString = "the rollout Strategy field is invalid: maxUnavailable must be greater than or equal to 0, got `-1`" ) @@ -189,7 +189,7 @@ func TestHandle(t *testing.T) { }, }, resourceInformer: &testinformer.FakeManager{ - APIResources: map[schema.GroupVersionKind]bool{utils.ConfigMapGVK: true}, + APIResources: map[schema.GroupVersionKind]bool{utils.DeploymentGVK: true}, IsClusterScopedResource: false, }, resourceValidator: resourcePlacementValidator{ @@ -214,7 +214,7 @@ func TestHandle(t *testing.T) { }, }, resourceInformer: &testinformer.FakeManager{ - APIResources: map[schema.GroupVersionKind]bool{utils.ConfigMapGVK: true}, + APIResources: map[schema.GroupVersionKind]bool{utils.DeploymentGVK: true}, IsClusterScopedResource: false, }, resourceValidator: resourcePlacementValidator{ @@ -243,7 +243,7 @@ func TestHandle(t *testing.T) { }, }, resourceInformer: &testinformer.FakeManager{ - APIResources: map[schema.GroupVersionKind]bool{utils.ConfigMapGVK: true}, + APIResources: map[schema.GroupVersionKind]bool{utils.DeploymentGVK: true}, IsClusterScopedResource: false, }, resourceValidator: resourcePlacementValidator{ @@ -272,7 +272,7 @@ func TestHandle(t *testing.T) { }, }, resourceInformer: &testinformer.FakeManager{ - APIResources: map[schema.GroupVersionKind]bool{utils.ConfigMapGVK: true}, + APIResources: map[schema.GroupVersionKind]bool{utils.DeploymentGVK: true}, IsClusterScopedResource: false, }, resourceValidator: resourcePlacementValidator{ @@ -301,7 +301,7 @@ func TestHandle(t *testing.T) { }, }, resourceInformer: &testinformer.FakeManager{ - APIResources: map[schema.GroupVersionKind]bool{utils.ConfigMapGVK: true}, + APIResources: map[schema.GroupVersionKind]bool{utils.DeploymentGVK: true}, IsClusterScopedResource: false, }, resourceValidator: resourcePlacementValidator{ @@ -330,7 +330,7 @@ func TestHandle(t *testing.T) { }, }, resourceInformer: &testinformer.FakeManager{ - APIResources: map[schema.GroupVersionKind]bool{utils.ConfigMapGVK: true}, + APIResources: map[schema.GroupVersionKind]bool{utils.DeploymentGVK: true}, IsClusterScopedResource: false, }, resourceValidator: resourcePlacementValidator{ @@ -359,7 +359,7 @@ func TestHandle(t *testing.T) { }, }, resourceInformer: &testinformer.FakeManager{ - APIResources: map[schema.GroupVersionKind]bool{utils.ConfigMapGVK: true}, + APIResources: map[schema.GroupVersionKind]bool{utils.DeploymentGVK: true}, IsClusterScopedResource: false, }, resourceValidator: resourcePlacementValidator{ @@ -378,7 +378,7 @@ func TestHandle(t *testing.T) { }, }, resourceInformer: &testinformer.FakeManager{ - APIResources: map[schema.GroupVersionKind]bool{utils.ConfigMapGVK: true}, + APIResources: map[schema.GroupVersionKind]bool{utils.DeploymentGVK: true}, IsClusterScopedResource: false, }, resourceValidator: resourcePlacementValidator{ @@ -411,7 +411,7 @@ func TestHandle(t *testing.T) { }, }, resourceInformer: &testinformer.FakeManager{ - APIResources: map[schema.GroupVersionKind]bool{utils.ConfigMapGVK: true}, + APIResources: map[schema.GroupVersionKind]bool{utils.DeploymentGVK: true}, IsClusterScopedResource: false, }, resourceValidator: resourcePlacementValidator{ From 4221755507f06c0ac4e7a72986a15748660d779f Mon Sep 17 00:00:00 2001 From: Wei Weng Date: Thu, 30 Oct 2025 18:02:51 +0000 Subject: [PATCH 08/10] fix comment Signed-off-by: Wei Weng --- pkg/utils/validator/resourceplacement.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/utils/validator/resourceplacement.go b/pkg/utils/validator/resourceplacement.go index c6bb586bc..50ba995a9 100644 --- a/pkg/utils/validator/resourceplacement.go +++ b/pkg/utils/validator/resourceplacement.go @@ -101,6 +101,7 @@ func validatePlacement(name string, resourceSelectors []placementv1beta1.Resourc allErr = append(allErr, fmt.Errorf("the resource is not found in schema (please retry) or it is not a cluster scoped resource: %v", gvk)) } + // Only check namespace scope for ResourcePlacement if !isClusterScoped && ResourceInformer.IsClusterScopedResources(gvk) { allErr = append(allErr, fmt.Errorf("the resource is not found in schema (please retry) or it is a cluster scoped resource: %v", gvk)) } From cdf53770f0b54b7fe9fa49f2df0caca03343b53e Mon Sep 17 00:00:00 2001 From: Wei Weng Date: Thu, 30 Oct 2025 20:54:46 +0000 Subject: [PATCH 09/10] use existing common interface Signed-off-by: Wei Weng --- .../v1beta1/clusterresourceplacement_types.go | 20 ------------------- pkg/utils/validator/resourceplacement.go | 12 +++-------- ...terresourceplacement_validating_webhook.go | 6 +++--- ...a1_resourceplacement_validating_webhook.go | 6 +++--- 4 files changed, 9 insertions(+), 35 deletions(-) diff --git a/apis/placement/v1beta1/clusterresourceplacement_types.go b/apis/placement/v1beta1/clusterresourceplacement_types.go index 5aa2862ec..f33254773 100644 --- a/apis/placement/v1beta1/clusterresourceplacement_types.go +++ b/apis/placement/v1beta1/clusterresourceplacement_types.go @@ -1595,16 +1595,6 @@ func (m *ClusterResourcePlacement) SetPlacementStatus(status PlacementStatus) { status.DeepCopyInto(&m.Status) } -// GetName returns the name of the ClusterResourcePlacement. -func (m *ClusterResourcePlacement) GetName() string { - return m.ObjectMeta.Name -} - -// GetDeletionTimestamp returns the deletion timestamp of the ClusterResourcePlacement. -func (m *ClusterResourcePlacement) GetDeletionTimestamp() *metav1.Time { - return m.ObjectMeta.DeletionTimestamp -} - const ( // ResourcePlacementCleanupFinalizer is a finalizer added by the RP controller to all RPs, to make sure // that the RP controller can react to RP deletions if necessary. @@ -1695,16 +1685,6 @@ func (rpl *ResourcePlacementList) GetPlacementObjs() []PlacementObj { return objs } -// GetName returns the name of the ResourcePlacement. -func (m *ResourcePlacement) GetName() string { - return m.ObjectMeta.Name -} - -// GetDeletionTimestamp returns the deletion timestamp of the ResourcePlacement. -func (m *ResourcePlacement) GetDeletionTimestamp() *metav1.Time { - return m.ObjectMeta.DeletionTimestamp -} - // +genclient // +genclient:Namespaced // +kubebuilder:object:root=true diff --git a/pkg/utils/validator/resourceplacement.go b/pkg/utils/validator/resourceplacement.go index 50ba995a9..86da654b4 100644 --- a/pkg/utils/validator/resourceplacement.go +++ b/pkg/utils/validator/resourceplacement.go @@ -550,12 +550,6 @@ func supportedResourceCapacityTypes() []string { return capacityTypes } -type ResourcePlacementObject interface { - GetPlacementSpec() *placementv1beta1.PlacementSpec - GetName() string - GetDeletionTimestamp() *metav1.Time -} - // HandlePlacementValidation provides consolidated webhook validation logic for placement objects. // This function accepts higher-order functions for type-specific operations. func HandlePlacementValidation( @@ -563,9 +557,9 @@ func HandlePlacementValidation( req admission.Request, decoder webhook.AdmissionDecoder, resourceType string, - decodeFunc func(admission.Request, webhook.AdmissionDecoder) (ResourcePlacementObject, error), - decodeOldFunc func(admission.Request, webhook.AdmissionDecoder) (ResourcePlacementObject, error), - validateFunc func(ResourcePlacementObject) error, + decodeFunc func(admission.Request, webhook.AdmissionDecoder) (placementv1beta1.PlacementObj, error), + decodeOldFunc func(admission.Request, webhook.AdmissionDecoder) (placementv1beta1.PlacementObj, error), + validateFunc func(placementv1beta1.PlacementObj) error, ) admission.Response { if req.Operation == admissionv1.Create || req.Operation == admissionv1.Update { klog.V(2).InfoS("handling placement", "resourceType", resourceType, "operation", req.Operation, "namespacedName", types.NamespacedName{Name: req.Name, Namespace: req.Namespace}) diff --git a/pkg/webhook/clusterresourceplacement/v1beta1_clusterresourceplacement_validating_webhook.go b/pkg/webhook/clusterresourceplacement/v1beta1_clusterresourceplacement_validating_webhook.go index 930a2828a..f5815cfa5 100644 --- a/pkg/webhook/clusterresourceplacement/v1beta1_clusterresourceplacement_validating_webhook.go +++ b/pkg/webhook/clusterresourceplacement/v1beta1_clusterresourceplacement_validating_webhook.go @@ -51,19 +51,19 @@ func (v *clusterResourcePlacementValidator) Handle(ctx context.Context, req admi return validator.HandlePlacementValidation(ctx, req, v.decoder, "CRP", // decodeFunc - func(req admission.Request, decoder webhook.AdmissionDecoder) (validator.ResourcePlacementObject, error) { + func(req admission.Request, decoder webhook.AdmissionDecoder) (placementv1beta1.PlacementObj, error) { var crp placementv1beta1.ClusterResourcePlacement err := decoder.Decode(req, &crp) return &crp, err }, // decodeOldFunc - func(req admission.Request, decoder webhook.AdmissionDecoder) (validator.ResourcePlacementObject, error) { + func(req admission.Request, decoder webhook.AdmissionDecoder) (placementv1beta1.PlacementObj, error) { var oldCRP placementv1beta1.ClusterResourcePlacement err := decoder.DecodeRaw(req.OldObject, &oldCRP) return &oldCRP, err }, // validateFunc - func(obj validator.ResourcePlacementObject) error { + func(obj placementv1beta1.PlacementObj) error { return validator.ValidateClusterResourcePlacement(obj.(*placementv1beta1.ClusterResourcePlacement)) }) } diff --git a/pkg/webhook/resourceplacement/v1beta1_resourceplacement_validating_webhook.go b/pkg/webhook/resourceplacement/v1beta1_resourceplacement_validating_webhook.go index c936b9265..2389c0495 100644 --- a/pkg/webhook/resourceplacement/v1beta1_resourceplacement_validating_webhook.go +++ b/pkg/webhook/resourceplacement/v1beta1_resourceplacement_validating_webhook.go @@ -54,19 +54,19 @@ func (v *resourcePlacementValidator) Handle(ctx context.Context, req admission.R v.decoder, "RP", // decodeFunc - func(req admission.Request, decoder webhook.AdmissionDecoder) (validator.ResourcePlacementObject, error) { + func(req admission.Request, decoder webhook.AdmissionDecoder) (placementv1beta1.PlacementObj, error) { var rp placementv1beta1.ResourcePlacement err := decoder.Decode(req, &rp) return &rp, err }, // decodeOldFunc - func(req admission.Request, decoder webhook.AdmissionDecoder) (validator.ResourcePlacementObject, error) { + func(req admission.Request, decoder webhook.AdmissionDecoder) (placementv1beta1.PlacementObj, error) { var oldRP placementv1beta1.ResourcePlacement err := decoder.DecodeRaw(req.OldObject, &oldRP) return &oldRP, err }, // validateFunc - func(obj validator.ResourcePlacementObject) error { + func(obj placementv1beta1.PlacementObj) error { return validator.ValidateResourcePlacement(obj.(*placementv1beta1.ResourcePlacement)) }, ) From dbf3c69db73a412d0caa175fa94aef453f69b592 Mon Sep 17 00:00:00 2001 From: Wei Weng Date: Fri, 31 Oct 2025 01:24:43 +0000 Subject: [PATCH 10/10] rename file Signed-off-by: Wei Weng --- pkg/utils/validator/{resourceplacement.go => placement.go} | 0 .../validator/{resourceplacement_test.go => placement_test.go} | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename pkg/utils/validator/{resourceplacement.go => placement.go} (100%) rename pkg/utils/validator/{resourceplacement_test.go => placement_test.go} (100%) diff --git a/pkg/utils/validator/resourceplacement.go b/pkg/utils/validator/placement.go similarity index 100% rename from pkg/utils/validator/resourceplacement.go rename to pkg/utils/validator/placement.go diff --git a/pkg/utils/validator/resourceplacement_test.go b/pkg/utils/validator/placement_test.go similarity index 100% rename from pkg/utils/validator/resourceplacement_test.go rename to pkg/utils/validator/placement_test.go