From 909862abe0badd2a3d3a8ce33918a2d4380874e5 Mon Sep 17 00:00:00 2001 From: Wantong Jiang Date: Wed, 22 Apr 2026 22:41:48 +0000 Subject: [PATCH 1/2] feat: add mutating webhook to label managed deploys created by aksService --- pkg/utils/common.go | 13 + pkg/webhook/add_handler.go | 3 + .../deployment/deployment_mutating_webhook.go | 90 ++++ .../deployment_mutating_webhook_test.go | 393 ++++++++++++++++ .../deployment_validating_webhook.go | 109 +++++ .../deployment_validating_webhook_test.go | 436 ++++++++++++++++++ pkg/webhook/pod/pod_validating_webhook.go | 2 +- .../replicaset_validating_webhook.go | 2 +- pkg/webhook/webhook.go | 31 ++ pkg/webhook/webhook_test.go | 8 +- test/e2e/fleet_guard_rail_test.go | 200 ++++++++ 11 files changed, 1281 insertions(+), 6 deletions(-) create mode 100644 pkg/webhook/deployment/deployment_mutating_webhook.go create mode 100644 pkg/webhook/deployment/deployment_mutating_webhook_test.go create mode 100644 pkg/webhook/deployment/deployment_validating_webhook.go create mode 100644 pkg/webhook/deployment/deployment_validating_webhook_test.go diff --git a/pkg/utils/common.go b/pkg/utils/common.go index 8a3764e24..05a33e483 100644 --- a/pkg/utils/common.go +++ b/pkg/utils/common.go @@ -60,6 +60,13 @@ const ( MutatingPathFmt = "/mutate-%s-%s-%s" lessGroupsStringFormat = "groups: %v" moreGroupsStringFormat = "groups: [%s, %s, %s,......]" + + // ReconcileLabelKey is the label key added to resources that should be reconciled. + // The value indicates the reason the resource should be reconciled. + ReconcileLabelKey = "fleet.azure.com/reconcile" + // ReconcileLabelValue is the label value paired with ReconcileLabelKey, + // indicating the resource is managed and should be reconciled. + ReconcileLabelValue = "managed" ) const ( @@ -504,6 +511,12 @@ func IsReservedNamespace(namespace string) bool { return strings.HasPrefix(namespace, fleetPrefix) || strings.HasPrefix(namespace, kubePrefix) } +// HasReconcileLabel reports whether the given labels contain the fleet reconcile +// label key with a non-empty value. +func HasReconcileLabel(labels map[string]string) bool { + return labels[ReconcileLabelKey] != "" +} + // IsFleetMemberNamespace indicates if an argued namespace is a fleet member namespace. func IsFleetMemberNamespace(namespace string) bool { return strings.HasPrefix(namespace, fleetMemberNamespacePrefix) diff --git a/pkg/webhook/add_handler.go b/pkg/webhook/add_handler.go index 43eaf2331..dc1e81736 100644 --- a/pkg/webhook/add_handler.go +++ b/pkg/webhook/add_handler.go @@ -5,6 +5,7 @@ import ( "go.goms.io/fleet/pkg/webhook/clusterresourceplacement" "go.goms.io/fleet/pkg/webhook/clusterresourceplacementdisruptionbudget" "go.goms.io/fleet/pkg/webhook/clusterresourceplacementeviction" + "go.goms.io/fleet/pkg/webhook/deployment" "go.goms.io/fleet/pkg/webhook/fleetresourcehandler" "go.goms.io/fleet/pkg/webhook/membercluster" "go.goms.io/fleet/pkg/webhook/pdb" @@ -29,4 +30,6 @@ func init() { AddToManagerFuncs = append(AddToManagerFuncs, resourceoverride.Add) AddToManagerFuncs = append(AddToManagerFuncs, clusterresourceplacementeviction.Add) AddToManagerFuncs = append(AddToManagerFuncs, clusterresourceplacementdisruptionbudget.Add) + AddToManagerFuncs = append(AddToManagerFuncs, deployment.AddMutating) + AddToManagerFuncs = append(AddToManagerFuncs, deployment.Add) } diff --git a/pkg/webhook/deployment/deployment_mutating_webhook.go b/pkg/webhook/deployment/deployment_mutating_webhook.go new file mode 100644 index 000000000..f6e7765d0 --- /dev/null +++ b/pkg/webhook/deployment/deployment_mutating_webhook.go @@ -0,0 +1,90 @@ +/* +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 deployment + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + + appsv1 "k8s.io/api/apps/v1" + "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" + + "go.goms.io/fleet/pkg/utils" +) + +// MutatingPath is the webhook service path for mutating Deployment resources. +var MutatingPath = fmt.Sprintf(utils.MutatingPathFmt, appsv1.SchemeGroupVersion.Group, appsv1.SchemeGroupVersion.Version, "deployment") + +type deploymentMutator struct { + decoder webhook.AdmissionDecoder +} + +// AddMutating registers the mutating webhook for Deployments with the manager. +func AddMutating(mgr manager.Manager) error { + hookServer := mgr.GetWebhookServer() + hookServer.Register(MutatingPath, &webhook.Admission{Handler: &deploymentMutator{decoder: admission.NewDecoder(mgr.GetScheme())}}) + return nil +} + +// Handle injects the fleet.azure.com/reconcile=managed label onto the +// Deployment and its pod template when the request originated from the aksService user. +func (m *deploymentMutator) Handle(_ context.Context, req admission.Request) admission.Response { + klog.V(2).InfoS("handling deployment mutating webhook", + "operation", req.Operation, "namespace", req.Namespace, "name", req.Name, "user", req.UserInfo.Username) + + // Pass through requests targeting reserved system namespaces. + if utils.IsReservedNamespace(req.Namespace) { + return admission.Allowed(fmt.Sprintf("namespace %s is a reserved system namespace, no mutation needed", req.Namespace)) + } + + // Only mutate when the request was made by the aksService user with + // system:masters group membership. + if !isAKSService(req.UserInfo) { + return admission.Allowed("user is not aksService, no mutation needed") + } + + var deploy appsv1.Deployment + if err := m.decoder.Decode(req, &deploy); err != nil { + return admission.Errored(http.StatusBadRequest, err) + } + + // Add the reconcile label on the Deployment metadata. + if deploy.Labels == nil { + deploy.Labels = map[string]string{} + } + deploy.Labels[utils.ReconcileLabelKey] = utils.ReconcileLabelValue + + // Add the reconcile label on the pod template metadata. + if deploy.Spec.Template.Labels == nil { + deploy.Spec.Template.Labels = map[string]string{} + } + deploy.Spec.Template.Labels[utils.ReconcileLabelKey] = utils.ReconcileLabelValue + + marshaled, err := json.Marshal(deploy) + if err != nil { + return admission.Errored(http.StatusInternalServerError, err) + } + + klog.V(2).InfoS("mutated deployment with reconcile label", + "operation", req.Operation, "namespace", req.Namespace, "name", req.Name) + return admission.PatchResponseFromRaw(req.Object.Raw, marshaled) +} diff --git a/pkg/webhook/deployment/deployment_mutating_webhook_test.go b/pkg/webhook/deployment/deployment_mutating_webhook_test.go new file mode 100644 index 000000000..123d5c3b4 --- /dev/null +++ b/pkg/webhook/deployment/deployment_mutating_webhook_test.go @@ -0,0 +1,393 @@ +/* +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 deployment + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "net/http" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + "gomodules.xyz/jsonpatch/v2" + admissionv1 "k8s.io/api/admission/v1" + appsv1 "k8s.io/api/apps/v1" + authenticationv1 "k8s.io/api/authentication/v1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/utils/ptr" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + "go.goms.io/fleet/pkg/utils" +) + +const ( + kubeSystemNamespace = "kube-system" +) + +func TestMutatingPath(t *testing.T) { + want := "/mutate-apps-v1-deployment" + if MutatingPath != want { + t.Errorf("MutatingPath = %q, want %q", MutatingPath, want) + } +} + +func TestHandle(t *testing.T) { + scheme := runtime.NewScheme() + if err := appsv1.AddToScheme(scheme); err != nil { + t.Fatalf("appsv1.AddToScheme() = %v, want nil", err) + } + decoder := admission.NewDecoder(scheme) + + aksServiceUser := authenticationv1.UserInfo{ + Username: AKSServiceUserName, + Groups: []string{SystemMastersGroup}, + } + + aksServiceUserNoMasters := authenticationv1.UserInfo{ + Username: AKSServiceUserName, + Groups: []string{"system:authenticated"}, + } + + regularUser := authenticationv1.UserInfo{ + Username: "regular-user", + Groups: []string{SystemMastersGroup, "system:authenticated"}, + } + + // Deployment with no existing labels. + deployNoLabels := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-deploy", + Namespace: "default", + }, + Spec: appsv1.DeploymentSpec{ + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"app": "test"}, + }, + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{}, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + {Name: "test", Image: "nginx"}, + }, + }, + }, + }, + } + + // Deployment with existing labels on both deployment and pod template. + deployWithLabels := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-deploy-labels", + Namespace: "default", + Labels: map[string]string{"app": "myapp", "tier": "frontend"}, + }, + Spec: appsv1.DeploymentSpec{ + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"app": "myapp"}, + }, + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{"app": "myapp", "tier": "frontend"}, + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + {Name: "myapp", Image: "myapp:latest"}, + }, + }, + }, + }, + } + + // Deployment in kube-system namespace. + deployKubeSystem := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-deploy", + Namespace: kubeSystemNamespace, + }, + Spec: appsv1.DeploymentSpec{ + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"app": "test"}, + }, + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + {Name: "test", Image: "nginx"}, + }, + }, + }, + }, + } + + // Deployment in fleet-system namespace. + deployFleetSystem := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-deploy", + Namespace: utils.FleetSystemNamespace, + }, + Spec: appsv1.DeploymentSpec{ + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"app": "test"}, + }, + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + {Name: "test", Image: "nginx"}, + }, + }, + }, + }, + } + + // Deployment that already has the reconcile label. + deployAlreadyLabeled := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-deploy-already-labeled", + Namespace: "default", + Labels: map[string]string{utils.ReconcileLabelKey: utils.ReconcileLabelValue, "app": "myapp"}, + }, + Spec: appsv1.DeploymentSpec{ + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"app": "myapp"}, + }, + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{utils.ReconcileLabelKey: utils.ReconcileLabelValue, "app": "myapp"}, + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + {Name: "myapp", Image: "myapp:latest"}, + }, + }, + }, + }, + } + + deployNoLabelsBytes, err := json.Marshal(deployNoLabels) + if err != nil { + t.Fatalf("json.Marshal(deployNoLabels) = %v, want nil", err) + } + deployWithLabelsBytes, err := json.Marshal(deployWithLabels) + if err != nil { + t.Fatalf("json.Marshal(deployWithLabels) = %v, want nil", err) + } + deployKubeSystemBytes, err := json.Marshal(deployKubeSystem) + if err != nil { + t.Fatalf("json.Marshal(deployKubeSystem) = %v, want nil", err) + } + deployFleetSystemBytes, err := json.Marshal(deployFleetSystem) + if err != nil { + t.Fatalf("json.Marshal(deployFleetSystem) = %v, want nil", err) + } + deployAlreadyLabeledBytes, err := json.Marshal(deployAlreadyLabeled) + if err != nil { + t.Fatalf("json.Marshal(deployAlreadyLabeled) = %v, want nil", err) + } + + testCases := map[string]struct { + req admission.Request + wantResponse admission.Response + }{ + "allow and skip mutation for kube-system namespace": { + req: admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Name: "test-deploy", + Namespace: kubeSystemNamespace, + Operation: admissionv1.Create, + Object: runtime.RawExtension{ + Raw: deployKubeSystemBytes, + Object: deployKubeSystem, + }, + UserInfo: aksServiceUser, + }, + }, + wantResponse: admission.Allowed(fmt.Sprintf("namespace %s is a reserved system namespace, no mutation needed", kubeSystemNamespace)), + }, + "allow and skip mutation for fleet-system namespace": { + req: admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Name: "test-deploy", + Namespace: utils.FleetSystemNamespace, + Operation: admissionv1.Create, + Object: runtime.RawExtension{ + Raw: deployFleetSystemBytes, + Object: deployFleetSystem, + }, + UserInfo: aksServiceUser, + }, + }, + wantResponse: admission.Allowed(fmt.Sprintf("namespace %s is a reserved system namespace, no mutation needed", utils.FleetSystemNamespace)), + }, + "allow and skip mutation for non-aksService user": { + req: admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Name: "test-deploy", + Namespace: "default", + Operation: admissionv1.Create, + Object: runtime.RawExtension{ + Raw: deployNoLabelsBytes, + Object: deployNoLabels, + }, + UserInfo: regularUser, + }, + }, + wantResponse: admission.Allowed("user is not aksService, no mutation needed"), + }, + "allow and skip mutation for aksService user not in system:masters group": { + req: admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Name: "test-deploy", + Namespace: "default", + Operation: admissionv1.Create, + Object: runtime.RawExtension{ + Raw: deployNoLabelsBytes, + Object: deployNoLabels, + }, + UserInfo: aksServiceUserNoMasters, + }, + }, + wantResponse: admission.Allowed("user is not aksService, no mutation needed"), + }, + "mutate deployment with no labels by aksService user": { + req: admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Name: "test-deploy", + Namespace: "default", + Operation: admissionv1.Create, + Object: runtime.RawExtension{ + Raw: deployNoLabelsBytes, + Object: deployNoLabels, + }, + UserInfo: aksServiceUser, + }, + }, + wantResponse: admission.Response{ + AdmissionResponse: admissionv1.AdmissionResponse{ + Allowed: true, + PatchType: ptr.To(admissionv1.PatchTypeJSONPatch), + }, + Patches: []jsonpatch.JsonPatchOperation{ + { + Operation: "add", + Path: "/metadata/labels", + Value: map[string]any{ + utils.ReconcileLabelKey: utils.ReconcileLabelValue, + }, + }, + { + Operation: "add", + Path: "/spec/template/metadata/labels", + Value: map[string]any{ + utils.ReconcileLabelKey: utils.ReconcileLabelValue, + }, + }, + }, + }, + }, + "mutate deployment with existing labels by aksService user": { + req: admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Name: "test-deploy-labels", + Namespace: "default", + Operation: admissionv1.Create, + Object: runtime.RawExtension{ + Raw: deployWithLabelsBytes, + Object: deployWithLabels, + }, + UserInfo: aksServiceUser, + }, + }, + wantResponse: admission.Response{ + AdmissionResponse: admissionv1.AdmissionResponse{ + Allowed: true, + PatchType: ptr.To(admissionv1.PatchTypeJSONPatch), + }, + Patches: []jsonpatch.JsonPatchOperation{ + { + Operation: "add", + Path: "/metadata/labels/fleet.azure.com~1reconcile", + Value: utils.ReconcileLabelValue, + }, + { + Operation: "add", + Path: "/spec/template/metadata/labels/fleet.azure.com~1reconcile", + Value: utils.ReconcileLabelValue, + }, + }, + }, + }, + "no-op patch when reconcile label already present": { + req: admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Name: "test-deploy-already-labeled", + Namespace: "default", + Operation: admissionv1.Update, + Object: runtime.RawExtension{ + Raw: deployAlreadyLabeledBytes, + Object: deployAlreadyLabeled, + }, + UserInfo: aksServiceUser, + }, + }, + wantResponse: admission.Response{ + AdmissionResponse: admissionv1.AdmissionResponse{ + Allowed: true, + }, + Patches: []jsonpatch.JsonPatchOperation{}, + }, + }, + "error on malformed request object": { + req: admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Name: "test-deploy", + Namespace: "default", + Operation: admissionv1.Create, + Object: runtime.RawExtension{ + Raw: []byte("not valid json"), + }, + UserInfo: aksServiceUser, + }, + }, + // The exact error message from the decoder is implementation-defined; + // only the status code and allowed=false are compared. + wantResponse: admission.Errored(http.StatusBadRequest, errors.New("")), + }, + } + + for testName, tc := range testCases { + t.Run(testName, func(t *testing.T) { + m := &deploymentMutator{decoder: decoder} + gotResponse := m.Handle(context.Background(), tc.req) + cmpOptions := []cmp.Option{ + cmpopts.IgnoreFields(metav1.Status{}, "Message"), + cmpopts.SortSlices(func(a, b jsonpatch.JsonPatchOperation) bool { + if a.Path == b.Path { + return a.Operation < b.Operation + } + return a.Path < b.Path + }), + } + if diff := cmp.Diff(tc.wantResponse, gotResponse, cmpOptions...); diff != "" { + t.Errorf("Handle() mismatch (-want +got):\n%s", diff) + } + }) + } +} diff --git a/pkg/webhook/deployment/deployment_validating_webhook.go b/pkg/webhook/deployment/deployment_validating_webhook.go new file mode 100644 index 000000000..c597c383f --- /dev/null +++ b/pkg/webhook/deployment/deployment_validating_webhook.go @@ -0,0 +1,109 @@ +/* +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 deployment implements admission webhooks for Deployment resources. +// The mutating webhook injects the "fleet.azure.com/reconcile=managed" label on +// deployments created by the aksService user. The validating webhook prevents +// non-aksService users from setting this label, which would otherwise bypass +// pod and replicaset admission checks. +package deployment + +import ( + "context" + "fmt" + "net/http" + "slices" + + admissionv1 "k8s.io/api/admission/v1" + appsv1 "k8s.io/api/apps/v1" + authenticationv1 "k8s.io/api/authentication/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" + + "go.goms.io/fleet/pkg/utils" +) + +const ( + deniedReconcileLabelFmt = "the %s label is reserved for aksService and cannot be set by user %q" + + // AKSServiceUserName is the username whose deployment requests should be mutated. + AKSServiceUserName = "aksService" + + // SystemMastersGroup is the Kubernetes group representing cluster administrators. + SystemMastersGroup = "system:masters" +) + +// ValidationPath is the webhook service path for validating Deployment resources. +var ValidationPath = fmt.Sprintf(utils.ValidationPathFmt, appsv1.SchemeGroupVersion.Group, appsv1.SchemeGroupVersion.Version, "deployment") + +type deploymentValidator struct { + decoder webhook.AdmissionDecoder +} + +// Add registers the validating webhook for Deployments with the manager. +func Add(mgr manager.Manager) error { + hookServer := mgr.GetWebhookServer() + hookServer.Register(ValidationPath, &webhook.Admission{Handler: &deploymentValidator{decoder: admission.NewDecoder(mgr.GetScheme())}}) + return nil +} + +// Handle rejects deployments that carry the fleet reconcile label unless the +// request was made by the aksService user with system:masters group membership. +func (v *deploymentValidator) Handle(_ context.Context, req admission.Request) admission.Response { + namespacedName := types.NamespacedName{Name: req.Name, Namespace: req.Namespace} + klog.V(2).InfoS("handling deployment validating webhook", + "operation", req.Operation, "namespacedName", namespacedName, "user", req.UserInfo.Username) + + // Only validate CREATE and UPDATE operations. + if req.Operation != admissionv1.Create && req.Operation != admissionv1.Update { + return admission.Allowed("operation is not CREATE or UPDATE, no validation needed") + } + + var deploy appsv1.Deployment + if err := v.decoder.Decode(req, &deploy); err != nil { + return admission.Errored(http.StatusBadRequest, err) + } + + // Check whether the reconcile label is present on either the Deployment + // metadata or the pod template metadata. + hasLabelOnDeploy := utils.HasReconcileLabel(deploy.Labels) + hasLabelOnPodTemplate := utils.HasReconcileLabel(deploy.Spec.Template.Labels) + + if !hasLabelOnDeploy && !hasLabelOnPodTemplate { + return admission.Allowed("deployment does not have the reconcile label, no validation needed") + } + + // The reconcile label is present — only aksService with system:masters is + // allowed to set it. + if isAKSService(req.UserInfo) { + klog.V(2).InfoS("aksService user allowed to set reconcile label", + "namespacedName", namespacedName) + return admission.Allowed("aksService user is allowed to set the reconcile label") + } + + klog.V(2).InfoS("denied non-aksService user from setting reconcile label", + "user", req.UserInfo.Username, "groups", req.UserInfo.Groups, "namespacedName", namespacedName) + return admission.Denied(fmt.Sprintf(deniedReconcileLabelFmt, utils.ReconcileLabelKey, req.UserInfo.Username)) +} + +// isAKSService reports whether the user is the aksService user with +// system:masters group membership. +func isAKSService(userInfo authenticationv1.UserInfo) bool { + return userInfo.Username == AKSServiceUserName && slices.Contains(userInfo.Groups, SystemMastersGroup) +} diff --git a/pkg/webhook/deployment/deployment_validating_webhook_test.go b/pkg/webhook/deployment/deployment_validating_webhook_test.go new file mode 100644 index 000000000..289904947 --- /dev/null +++ b/pkg/webhook/deployment/deployment_validating_webhook_test.go @@ -0,0 +1,436 @@ +/* +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 deployment + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + admissionv1 "k8s.io/api/admission/v1" + appsv1 "k8s.io/api/apps/v1" + authenticationv1 "k8s.io/api/authentication/v1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + "go.goms.io/fleet/pkg/utils" +) + +func TestValidationPath(t *testing.T) { + want := "/validate-apps-v1-deployment" + if ValidationPath != want { + t.Errorf("ValidationPath = %q, want %q", ValidationPath, want) + } +} + +func TestValidatingHandle(t *testing.T) { + scheme := runtime.NewScheme() + if err := appsv1.AddToScheme(scheme); err != nil { + t.Fatalf("appsv1.AddToScheme() = %v, want nil", err) + } + decoder := admission.NewDecoder(scheme) + + aksServiceUser := authenticationv1.UserInfo{ + Username: AKSServiceUserName, + Groups: []string{SystemMastersGroup}, + } + + aksServiceUserNoMasters := authenticationv1.UserInfo{ + Username: AKSServiceUserName, + Groups: []string{"system:authenticated"}, + } + + regularUser := authenticationv1.UserInfo{ + Username: "regular-user", + Groups: []string{"system:authenticated"}, + } + + regularUserWithMasters := authenticationv1.UserInfo{ + Username: "regular-user", + Groups: []string{SystemMastersGroup, "system:authenticated"}, + } + + // Deployment without the reconcile label. + deployNoReconcileLabel := newTestDeployment("test-deploy", "default", nil, nil) + + // Deployment with reconcile label on deployment metadata only. + deployWithReconcileLabelOnDeploy := newTestDeployment("test-deploy", "default", + map[string]string{"app": "myapp", utils.ReconcileLabelKey: utils.ReconcileLabelValue}, + map[string]string{"app": "myapp"}, + ) + + // Deployment with reconcile label on pod template only. + deployWithReconcileLabelOnPodTemplate := newTestDeployment("test-deploy", "default", + map[string]string{"app": "myapp"}, + map[string]string{"app": "myapp", utils.ReconcileLabelKey: utils.ReconcileLabelValue}, + ) + + // Deployment with reconcile label on both. + deployWithReconcileLabelOnBoth := newTestDeployment("test-deploy", "default", + map[string]string{"app": "myapp", utils.ReconcileLabelKey: utils.ReconcileLabelValue}, + map[string]string{"app": "myapp", utils.ReconcileLabelKey: utils.ReconcileLabelValue}, + ) + + // Deployment in kube-system with reconcile label. + deployKubeSystem := newTestDeployment("test-deploy", kubeSystemNamespace, + map[string]string{utils.ReconcileLabelKey: utils.ReconcileLabelValue}, + nil, + ) + + // Deployment in fleet-system with reconcile label. + deployFleetSystem := newTestDeployment("test-deploy", utils.FleetSystemNamespace, + map[string]string{utils.ReconcileLabelKey: utils.ReconcileLabelValue}, + nil, + ) + + marshalOrFatal := func(t *testing.T, obj interface{}) []byte { + t.Helper() + b, err := json.Marshal(obj) + if err != nil { + t.Fatalf("json.Marshal() = %v, want nil", err) + } + return b + } + + deployNoReconcileLabelBytes := marshalOrFatal(t, deployNoReconcileLabel) + deployWithReconcileLabelOnDeployBytes := marshalOrFatal(t, deployWithReconcileLabelOnDeploy) + deployWithReconcileLabelOnPodTemplateBytes := marshalOrFatal(t, deployWithReconcileLabelOnPodTemplate) + deployWithReconcileLabelOnBothBytes := marshalOrFatal(t, deployWithReconcileLabelOnBoth) + deployKubeSystemBytes := marshalOrFatal(t, deployKubeSystem) + deployFleetSystemBytes := marshalOrFatal(t, deployFleetSystem) + + testCases := map[string]struct { + req admission.Request + wantAllowed bool + wantErrCode int32 + }{ + "allow deployment without reconcile label from regular user on CREATE": { + req: admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Name: "test-deploy", + Namespace: "default", + Operation: admissionv1.Create, + Object: runtime.RawExtension{Raw: deployNoReconcileLabelBytes, Object: deployNoReconcileLabel}, + UserInfo: regularUser, + }, + }, + wantAllowed: true, + }, + "allow deployment without reconcile label from regular user on UPDATE": { + req: admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Name: "test-deploy", + Namespace: "default", + Operation: admissionv1.Update, + Object: runtime.RawExtension{Raw: deployNoReconcileLabelBytes, Object: deployNoReconcileLabel}, + UserInfo: regularUser, + }, + }, + wantAllowed: true, + }, + "deny regular user from setting reconcile label in kube-system namespace": { + req: admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Name: "test-deploy", + Namespace: kubeSystemNamespace, + Operation: admissionv1.Create, + Object: runtime.RawExtension{Raw: deployKubeSystemBytes, Object: deployKubeSystem}, + UserInfo: regularUser, + }, + }, + wantAllowed: false, + }, + "deny regular user from setting reconcile label in fleet-system namespace": { + req: admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Name: "test-deploy", + Namespace: utils.FleetSystemNamespace, + Operation: admissionv1.Create, + Object: runtime.RawExtension{Raw: deployFleetSystemBytes, Object: deployFleetSystem}, + UserInfo: regularUser, + }, + }, + wantAllowed: false, + }, + "allow aksService user with reconcile label in kube-system namespace": { + req: admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Name: "test-deploy", + Namespace: kubeSystemNamespace, + Operation: admissionv1.Create, + Object: runtime.RawExtension{Raw: deployKubeSystemBytes, Object: deployKubeSystem}, + UserInfo: aksServiceUser, + }, + }, + wantAllowed: true, + }, + "allow DELETE operation even with reconcile label from regular user": { + req: admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Name: "test-deploy", + Namespace: "default", + Operation: admissionv1.Delete, + Object: runtime.RawExtension{Raw: deployWithReconcileLabelOnBothBytes, Object: deployWithReconcileLabelOnBoth}, + UserInfo: regularUser, + }, + }, + wantAllowed: true, + }, + "allow CONNECT operation even with reconcile label from regular user": { + req: admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Name: "test-deploy", + Namespace: "default", + Operation: admissionv1.Connect, + Object: runtime.RawExtension{Raw: deployWithReconcileLabelOnBothBytes, Object: deployWithReconcileLabelOnBoth}, + UserInfo: regularUser, + }, + }, + wantAllowed: true, + }, + "allow aksService user with system:masters to set reconcile label on deployment metadata": { + req: admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Name: "test-deploy", + Namespace: "default", + Operation: admissionv1.Create, + Object: runtime.RawExtension{Raw: deployWithReconcileLabelOnDeployBytes, Object: deployWithReconcileLabelOnDeploy}, + UserInfo: aksServiceUser, + }, + }, + wantAllowed: true, + }, + "allow aksService user with system:masters to set reconcile label on pod template": { + req: admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Name: "test-deploy", + Namespace: "default", + Operation: admissionv1.Create, + Object: runtime.RawExtension{Raw: deployWithReconcileLabelOnPodTemplateBytes, Object: deployWithReconcileLabelOnPodTemplate}, + UserInfo: aksServiceUser, + }, + }, + wantAllowed: true, + }, + "allow aksService user with system:masters to set reconcile label on both on UPDATE": { + req: admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Name: "test-deploy", + Namespace: "default", + Operation: admissionv1.Update, + Object: runtime.RawExtension{Raw: deployWithReconcileLabelOnBothBytes, Object: deployWithReconcileLabelOnBoth}, + UserInfo: aksServiceUser, + }, + }, + wantAllowed: true, + }, + "deny aksService user without system:masters group from setting reconcile label": { + req: admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Name: "test-deploy", + Namespace: "default", + Operation: admissionv1.Create, + Object: runtime.RawExtension{Raw: deployWithReconcileLabelOnDeployBytes, Object: deployWithReconcileLabelOnDeploy}, + UserInfo: aksServiceUserNoMasters, + }, + }, + wantAllowed: false, + }, + "deny regular user from setting reconcile label on deployment metadata": { + req: admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Name: "test-deploy", + Namespace: "default", + Operation: admissionv1.Create, + Object: runtime.RawExtension{Raw: deployWithReconcileLabelOnDeployBytes, Object: deployWithReconcileLabelOnDeploy}, + UserInfo: regularUser, + }, + }, + wantAllowed: false, + }, + "deny regular user from setting reconcile label on pod template only": { + req: admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Name: "test-deploy", + Namespace: "default", + Operation: admissionv1.Create, + Object: runtime.RawExtension{Raw: deployWithReconcileLabelOnPodTemplateBytes, Object: deployWithReconcileLabelOnPodTemplate}, + UserInfo: regularUser, + }, + }, + wantAllowed: false, + }, + "deny regular user from setting reconcile label on both deployment and pod template via UPDATE": { + req: admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Name: "test-deploy", + Namespace: "default", + Operation: admissionv1.Update, + Object: runtime.RawExtension{Raw: deployWithReconcileLabelOnBothBytes, Object: deployWithReconcileLabelOnBoth}, + UserInfo: regularUser, + }, + }, + wantAllowed: false, + }, + "deny regular user with system:masters but wrong username from setting reconcile label": { + req: admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Name: "test-deploy", + Namespace: "default", + Operation: admissionv1.Create, + Object: runtime.RawExtension{Raw: deployWithReconcileLabelOnDeployBytes, Object: deployWithReconcileLabelOnDeploy}, + UserInfo: regularUserWithMasters, + }, + }, + wantAllowed: false, + }, + "error on malformed request object": { + req: admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Name: "test-deploy", + Namespace: "default", + Operation: admissionv1.Create, + Object: runtime.RawExtension{ + Raw: []byte("not valid json"), + }, + UserInfo: aksServiceUser, + }, + }, + wantAllowed: false, + wantErrCode: http.StatusBadRequest, + }, + } + + for testName, tc := range testCases { + t.Run(testName, func(t *testing.T) { + v := &deploymentValidator{decoder: decoder} + gotResponse := v.Handle(context.Background(), tc.req) + + if gotResponse.Allowed != tc.wantAllowed { + t.Errorf("Handle() Allowed = %v, want %v, reason = %v", gotResponse.Allowed, tc.wantAllowed, gotResponse.Result) + } + + if tc.wantErrCode != 0 { + wantResponse := admission.Errored(tc.wantErrCode, errors.New("")) + cmpOptions := []cmp.Option{ + cmpopts.IgnoreFields(metav1.Status{}, "Message"), + } + if diff := cmp.Diff(wantResponse, gotResponse, cmpOptions...); diff != "" { + t.Errorf("Handle() error response mismatch (-want +got):\n%s", diff) + } + } + + // Denied responses should include a non-empty reason message. + if !tc.wantAllowed && tc.wantErrCode == 0 && gotResponse.Result != nil { + if gotResponse.Result.Message == "" { + t.Error("Handle() denied response should include a non-empty reason message") + } + } + }) + } +} + +func TestIsAKSService(t *testing.T) { + testCases := map[string]struct { + userInfo authenticationv1.UserInfo + want bool + }{ + "aksService with system:masters": { + userInfo: authenticationv1.UserInfo{ + Username: AKSServiceUserName, + Groups: []string{SystemMastersGroup}, + }, + want: true, + }, + "aksService with system:masters and other groups": { + userInfo: authenticationv1.UserInfo{ + Username: AKSServiceUserName, + Groups: []string{"system:authenticated", SystemMastersGroup, "custom-group"}, + }, + want: true, + }, + "aksService without system:masters": { + userInfo: authenticationv1.UserInfo{ + Username: AKSServiceUserName, + Groups: []string{"system:authenticated"}, + }, + want: false, + }, + "aksService with empty groups": { + userInfo: authenticationv1.UserInfo{ + Username: AKSServiceUserName, + Groups: []string{}, + }, + want: false, + }, + "regular user with system:masters": { + userInfo: authenticationv1.UserInfo{ + Username: "regular-user", + Groups: []string{SystemMastersGroup}, + }, + want: false, + }, + "empty username with system:masters": { + userInfo: authenticationv1.UserInfo{ + Username: "", + Groups: []string{SystemMastersGroup}, + }, + want: false, + }, + } + + for testName, tc := range testCases { + t.Run(testName, func(t *testing.T) { + got := isAKSService(tc.userInfo) + if got != tc.want { + t.Errorf("isAKSService(%+v) = %v, want %v", tc.userInfo, got, tc.want) + } + }) + } +} + +// newTestDeployment creates a Deployment for testing with the given labels. +func newTestDeployment(name, namespace string, deployLabels, podTemplateLabels map[string]string) *appsv1.Deployment { + return &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + Labels: deployLabels, + }, + Spec: appsv1.DeploymentSpec{ + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"app": "test"}, + }, + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: podTemplateLabels, + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + {Name: "test", Image: "nginx"}, + }, + }, + }, + }, + } +} diff --git a/pkg/webhook/pod/pod_validating_webhook.go b/pkg/webhook/pod/pod_validating_webhook.go index 132c92bf0..993cbf16d 100644 --- a/pkg/webhook/pod/pod_validating_webhook.go +++ b/pkg/webhook/pod/pod_validating_webhook.go @@ -64,7 +64,7 @@ func (v *podValidator) Handle(_ context.Context, req admission.Request) admissio if err != nil { return admission.Errored(http.StatusBadRequest, err) } - if !utils.IsReservedNamespace(pod.Namespace) { + if !utils.IsReservedNamespace(pod.Namespace) && !utils.HasReconcileLabel(pod.Labels) { klog.V(2).InfoS(deniedPodResource, "user", req.UserInfo.Username, "groups", req.UserInfo.Groups, "operation", req.Operation, "GVK", req.RequestKind, "subResource", req.SubResource, "namespacedName", namespacedName) return admission.Denied(fmt.Sprintf(podDeniedFormat, pod.Namespace, pod.Name)) } diff --git a/pkg/webhook/replicaset/replicaset_validating_webhook.go b/pkg/webhook/replicaset/replicaset_validating_webhook.go index b03683508..e5d862cb5 100644 --- a/pkg/webhook/replicaset/replicaset_validating_webhook.go +++ b/pkg/webhook/replicaset/replicaset_validating_webhook.go @@ -63,7 +63,7 @@ func (v *replicaSetValidator) Handle(_ context.Context, req admission.Request) a if err := v.decoder.Decode(req, rs); err != nil { return admission.Errored(http.StatusBadRequest, err) } - if !utils.IsReservedNamespace(rs.Namespace) { + if !utils.IsReservedNamespace(rs.Namespace) && !utils.HasReconcileLabel(rs.Labels) { klog.V(2).InfoS(deniedReplicaSetResource, "user", req.UserInfo.Username, "groups", req.UserInfo.Groups, "operation", req.Operation, "GVK", req.RequestKind, "subResource", req.SubResource, "namespacedName", namespacedName) return admission.Denied(fmt.Sprintf(replicaSetDeniedFormat, rs.Namespace, rs.Name)) } diff --git a/pkg/webhook/webhook.go b/pkg/webhook/webhook.go index 7b6457ad9..f2cf3c11c 100644 --- a/pkg/webhook/webhook.go +++ b/pkg/webhook/webhook.go @@ -59,6 +59,7 @@ import ( "go.goms.io/fleet/pkg/webhook/clusterresourceplacement" "go.goms.io/fleet/pkg/webhook/clusterresourceplacementdisruptionbudget" "go.goms.io/fleet/pkg/webhook/clusterresourceplacementeviction" + "go.goms.io/fleet/pkg/webhook/deployment" "go.goms.io/fleet/pkg/webhook/fleetresourcehandler" "go.goms.io/fleet/pkg/webhook/membercluster" "go.goms.io/fleet/pkg/webhook/pdb" @@ -413,6 +414,23 @@ func (w *Config) buildFleetMutatingWebhooks() []admv1.MutatingWebhook { }, TimeoutSeconds: longWebhookTimeout, }, + { + Name: "fleet.deployment.mutating", + ClientConfig: w.createClientConfig(deployment.MutatingPath), + FailurePolicy: &ignoreFailurePolicy, + SideEffects: &sideEffortsNone, + AdmissionReviewVersions: admissionReviewVersions, + Rules: []admv1.RuleWithOperations{ + { + Operations: []admv1.OperationType{ + admv1.Create, + admv1.Update, + }, + Rule: createRule([]string{appsv1.SchemeGroupVersion.Group}, []string{appsv1.SchemeGroupVersion.Version}, []string{deploymentResourceName}, &namespacedScope), + }, + }, + TimeoutSeconds: longWebhookTimeout, + }, } return webHooks } @@ -588,6 +606,19 @@ func (w *Config) buildFleetValidatingWebhooks() []admv1.ValidatingWebhook { }, ) + webHooks = append(webHooks, admv1.ValidatingWebhook{ + Name: "fleet.deployment.validating", + ClientConfig: w.createClientConfig(deployment.ValidationPath), + FailurePolicy: &failFailurePolicy, + SideEffects: &sideEffortsNone, + AdmissionReviewVersions: admissionReviewVersions, + Rules: []admv1.RuleWithOperations{{ + Operations: []admv1.OperationType{admv1.Create, admv1.Update}, + Rule: createRule([]string{appsv1.SchemeGroupVersion.Group}, []string{appsv1.SchemeGroupVersion.Version}, []string{deploymentResourceName}, &namespacedScope), + }}, + TimeoutSeconds: longWebhookTimeout, + }) + return webHooks } diff --git a/pkg/webhook/webhook_test.go b/pkg/webhook/webhook_test.go index 19be6ae32..4b397771d 100644 --- a/pkg/webhook/webhook_test.go +++ b/pkg/webhook/webhook_test.go @@ -33,7 +33,7 @@ func TestBuildFleetMutatingWebhooks(t *testing.T) { serviceURL: "test-url", clientConnectionType: &url, }, - wantLength: 1, + wantLength: 2, }, } @@ -60,7 +60,7 @@ func TestBuildFleetValidatingWebhooks(t *testing.T) { serviceURL: "test-url", clientConnectionType: &url, }, - wantLength: 9, + wantLength: 10, }, "enable workload": { config: Config{ @@ -70,7 +70,7 @@ func TestBuildFleetValidatingWebhooks(t *testing.T) { clientConnectionType: &url, enableWorkload: true, }, - wantLength: 7, + wantLength: 8, }, "enable PDBs": { config: Config{ @@ -80,7 +80,7 @@ func TestBuildFleetValidatingWebhooks(t *testing.T) { clientConnectionType: &url, enablePDBs: true, }, - wantLength: 8, + wantLength: 9, }, } diff --git a/test/e2e/fleet_guard_rail_test.go b/test/e2e/fleet_guard_rail_test.go index 7e8b32d77..85914245b 100644 --- a/test/e2e/fleet_guard_rail_test.go +++ b/test/e2e/fleet_guard_rail_test.go @@ -1304,3 +1304,203 @@ var _ = Describe("fleet guard rail restrict internal fleet resources from being Expect(checkIfStatusErrorWithMessage(notMasterUser.Create(ctx, &ise), fmt.Sprintf(validation.ResourceDeniedFormat, testUser, utils.GenerateGroupString(testGroups), admissionv1.Create, &iseGVK, "", types.NamespacedName{Name: ise.Name, Namespace: ise.Namespace}))).Should(Succeed()) }) }) + +var _ = Describe("fleet deployment webhook tests for validating and mutating deployments", Serial, Ordered, func() { + const ( + testNS = "default" + ) + + newDeployment := func(name, namespace string, deployLabels, podTemplateLabels map[string]string) *appsv1.Deployment { + mergeMaps := func(ms ...map[string]string) map[string]string { + result := make(map[string]string) + for _, m := range ms { + for k, v := range m { + result[k] = v + } + } + return result + } + return &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + Labels: deployLabels, + }, + Spec: appsv1.DeploymentSpec{ + Replicas: ptr.To(int32(1)), + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"app": name}, + }, + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: mergeMaps(map[string]string{"app": name}, podTemplateLabels), + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "test-container", + Image: "nginx:latest", + }, + }, + }, + }, + }, + } + } + + Context("validating webhook - deny regular user from setting reconcile label", func() { + It("should deny regular user from creating a deployment with reconcile label on deployment metadata", func() { + deploy := newDeployment("test-deploy-val-1", testNS, + map[string]string{utils.ReconcileLabelKey: utils.ReconcileLabelValue}, + nil, + ) + // Deployment creation should be denied; no cleanup needed. + err := notMasterUser.Create(ctx, deploy) + var statusErr *k8sErrors.StatusError + Expect(errors.As(err, &statusErr)).To(BeTrue(), fmt.Sprintf("Create deployment call produced error %s, want StatusError", fmt.Sprintf("%T", err))) + Expect(statusErr.ErrStatus.Message).Should(ContainSubstring(utils.ReconcileLabelKey)) + }) + + It("should deny regular user from creating a deployment with reconcile label on pod template", func() { + deploy := newDeployment("test-deploy-val-2", testNS, + nil, + map[string]string{utils.ReconcileLabelKey: utils.ReconcileLabelValue}, + ) + // Deployment creation should be denied; no cleanup needed. + err := notMasterUser.Create(ctx, deploy) + var statusErr *k8sErrors.StatusError + Expect(errors.As(err, &statusErr)).To(BeTrue(), fmt.Sprintf("Create deployment call produced error %s, want StatusError", fmt.Sprintf("%T", err))) + Expect(statusErr.ErrStatus.Message).Should(ContainSubstring(utils.ReconcileLabelKey)) + }) + + It("should deny regular user from updating a deployment to add reconcile label", func() { + // First create a deployment without reconcile label as admin. + deploy := newDeployment("test-deploy-val-3", testNS, nil, nil) + Expect(hubClient.Create(ctx, deploy)).Should(Succeed()) + DeferCleanup(func() { + _ = hubClient.Delete(ctx, deploy) + }) + + // Try to update as non-admin to add reconcile label. + Eventually(func(g Gomega) error { + var d appsv1.Deployment + err := hubClient.Get(ctx, types.NamespacedName{Name: deploy.Name, Namespace: deploy.Namespace}, &d) + if err != nil { + return err + } + if d.Labels == nil { + d.Labels = map[string]string{} + } + d.Labels[utils.ReconcileLabelKey] = utils.ReconcileLabelValue + err = notMasterUser.Update(ctx, &d) + if k8sErrors.IsConflict(err) { + return err + } + var statusErr *k8sErrors.StatusError + g.Expect(errors.As(err, &statusErr)).To(BeTrue(), fmt.Sprintf("Update deployment call produced error %s, want StatusError", fmt.Sprintf("%T", err))) + g.Expect(statusErr.ErrStatus.Message).Should(ContainSubstring(utils.ReconcileLabelKey)) + return nil + }, eventuallyDuration, eventuallyInterval).Should(Succeed()) + }) + }) + + Context("validating webhook - allow aksService user to set reconcile label", func() { + It("should allow aksService user to create a deployment with reconcile label", func() { + deploy := newDeployment("test-deploy-val-aks-1", testNS, + map[string]string{utils.ReconcileLabelKey: utils.ReconcileLabelValue}, + map[string]string{utils.ReconcileLabelKey: utils.ReconcileLabelValue}, + ) + DeferCleanup(func() { + _ = hubClient.Delete(ctx, deploy) + }) + Expect(sysMastersClient.Create(ctx, deploy)).Should(Succeed()) + }) + + It("should allow aksService user to create a deployment with reconcile label in kube-system", func() { + deploy := newDeployment("test-deploy-val-aks-2", "kube-system", + map[string]string{utils.ReconcileLabelKey: utils.ReconcileLabelValue}, + map[string]string{utils.ReconcileLabelKey: utils.ReconcileLabelValue}, + ) + DeferCleanup(func() { + _ = hubClient.Delete(ctx, deploy) + }) + Expect(sysMastersClient.Create(ctx, deploy)).Should(Succeed()) + }) + }) + + Context("validating webhook - allow deployment without reconcile label", func() { + It("should allow regular user to create a deployment without reconcile label", func() { + deploy := newDeployment("test-deploy-val-norc", testNS, nil, nil) + DeferCleanup(func() { + _ = hubClient.Delete(ctx, deploy) + }) + Expect(notMasterUser.Create(ctx, deploy)).Should(Succeed()) + }) + + It("should allow aksService user to create a deployment without reconcile label", func() { + deploy := newDeployment("test-deploy-val-norc", testNS, nil, nil) + DeferCleanup(func() { + _ = hubClient.Delete(ctx, deploy) + }) + Expect(sysMastersClient.Create(ctx, deploy)).Should(Succeed()) + }) + }) + + Context("mutating webhook - inject reconcile label for aksService user", func() { + It("should inject reconcile label when aksService creates a deployment", func() { + deploy := newDeployment("test-deploy-mut-1", testNS, nil, nil) + DeferCleanup(func() { + _ = hubClient.Delete(ctx, deploy) + }) + Expect(sysMastersClient.Create(ctx, deploy)).Should(Succeed()) + + // Verify the reconcile label was injected. The mutating webhook runs + // synchronously during admission, but Eventually is used defensively + // to handle brief etcd/cache propagation lag. + Eventually(func(g Gomega) { + var d appsv1.Deployment + g.Expect(hubClient.Get(ctx, types.NamespacedName{Name: deploy.Name, Namespace: deploy.Namespace}, &d)).Should(Succeed()) + g.Expect(d.Labels).Should(HaveKeyWithValue(utils.ReconcileLabelKey, utils.ReconcileLabelValue), + "deployment metadata should have reconcile label injected by mutating webhook") + g.Expect(d.Spec.Template.Labels).Should(HaveKeyWithValue(utils.ReconcileLabelKey, utils.ReconcileLabelValue), + "pod template should have reconcile label injected by mutating webhook") + }, eventuallyDuration, eventuallyInterval).Should(Succeed()) + }) + }) + + Context("mutating webhook - no injection for regular user", func() { + It("should not inject reconcile label when regular user creates a deployment", func() { + deploy := newDeployment("test-deploy-mut-2", testNS, nil, nil) + DeferCleanup(func() { + _ = hubClient.Delete(ctx, deploy) + }) + Expect(notMasterUser.Create(ctx, deploy)).Should(Succeed()) + + // Verify the reconcile label was NOT injected. + var d appsv1.Deployment + Expect(hubClient.Get(ctx, types.NamespacedName{Name: deploy.Name, Namespace: deploy.Namespace}, &d)).Should(Succeed()) + Expect(d.Labels).ShouldNot(HaveKey(utils.ReconcileLabelKey), + "deployment metadata should NOT have reconcile label for regular user") + Expect(d.Spec.Template.Labels).ShouldNot(HaveKey(utils.ReconcileLabelKey), + "pod template should NOT have reconcile label for regular user") + }) + }) + + Context("mutating webhook - no injection in reserved namespaces", func() { + It("should not inject reconcile label for deployment in kube-system even for aksService", func() { + deploy := newDeployment("test-deploy-mut-3", "kube-system", nil, nil) + DeferCleanup(func() { + _ = hubClient.Delete(ctx, deploy) + }) + Expect(sysMastersClient.Create(ctx, deploy)).Should(Succeed()) + + // Verify the reconcile label was NOT injected for reserved namespace. + var d appsv1.Deployment + Expect(hubClient.Get(ctx, types.NamespacedName{Name: deploy.Name, Namespace: deploy.Namespace}, &d)).Should(Succeed()) + Expect(d.Labels).ShouldNot(HaveKey(utils.ReconcileLabelKey), + "deployment in kube-system should NOT have reconcile label auto-injected") + Expect(d.Spec.Template.Labels).ShouldNot(HaveKey(utils.ReconcileLabelKey), + "pod template in kube-system should NOT have reconcile label auto-injected") + }) + }) +}) From 5f419c1285f76a124a419b169da7cc47c01c19f3 Mon Sep 17 00:00:00 2001 From: Wantong Jiang Date: Thu, 23 Apr 2026 22:48:03 +0000 Subject: [PATCH 2/2] fix comments --- pkg/utils/azure.go | 51 +++++++++++++++++++ pkg/utils/common.go | 13 ----- ...utating_webhook.go => mutating_webhook.go} | 2 +- ...bhook_test.go => mutating_webhook_test.go} | 8 +-- ...ating_webhook.go => validating_webhook.go} | 16 +----- ...ook_test.go => validating_webhook_test.go} | 26 +++++----- 6 files changed, 70 insertions(+), 46 deletions(-) create mode 100644 pkg/utils/azure.go rename pkg/webhook/deployment/{deployment_mutating_webhook.go => mutating_webhook.go} (98%) rename pkg/webhook/deployment/{deployment_mutating_webhook_test.go => mutating_webhook_test.go} (98%) rename pkg/webhook/deployment/{deployment_validating_webhook.go => validating_webhook.go} (86%) rename pkg/webhook/deployment/{deployment_validating_webhook_test.go => validating_webhook_test.go} (95%) diff --git a/pkg/utils/azure.go b/pkg/utils/azure.go new file mode 100644 index 000000000..3fed896c3 --- /dev/null +++ b/pkg/utils/azure.go @@ -0,0 +1,51 @@ +/* +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 utils + +import ( + "slices" + + authenticationv1 "k8s.io/api/authentication/v1" +) + +// This file contains constants and utility functions related to Azure-specific logic. +// We define these in a separate file to avoid potential merge conflicts from the KubeFleet repository when backporting changes. +const ( + // ReconcileLabelKey is the label key added to resources that should be reconciled. + // The value indicates the reason the resource should be reconciled. + ReconcileLabelKey = "fleet.azure.com/reconcile" + // ReconcileLabelValue is the label value paired with ReconcileLabelKey, + // indicating the resource is managed and should be reconciled. + ReconcileLabelValue = "managed" + + // AKSServiceUserName is the username whose deployment requests should be mutated. + AKSServiceUserName = "aksService" + // SystemMastersGroup is the Kubernetes group representing cluster administrators. + SystemMastersGroup = "system:masters" +) + +// IsAKSService reports whether the user is the aksService user with +// system:masters group membership. +func IsAKSService(userInfo authenticationv1.UserInfo) bool { + return userInfo.Username == AKSServiceUserName && slices.Contains(userInfo.Groups, SystemMastersGroup) +} + +// HasReconcileLabel reports whether the given labels contain the fleet reconcile +// label key with a non-empty value. +func HasReconcileLabel(labels map[string]string) bool { + return labels[ReconcileLabelKey] != "" +} diff --git a/pkg/utils/common.go b/pkg/utils/common.go index 05a33e483..8a3764e24 100644 --- a/pkg/utils/common.go +++ b/pkg/utils/common.go @@ -60,13 +60,6 @@ const ( MutatingPathFmt = "/mutate-%s-%s-%s" lessGroupsStringFormat = "groups: %v" moreGroupsStringFormat = "groups: [%s, %s, %s,......]" - - // ReconcileLabelKey is the label key added to resources that should be reconciled. - // The value indicates the reason the resource should be reconciled. - ReconcileLabelKey = "fleet.azure.com/reconcile" - // ReconcileLabelValue is the label value paired with ReconcileLabelKey, - // indicating the resource is managed and should be reconciled. - ReconcileLabelValue = "managed" ) const ( @@ -511,12 +504,6 @@ func IsReservedNamespace(namespace string) bool { return strings.HasPrefix(namespace, fleetPrefix) || strings.HasPrefix(namespace, kubePrefix) } -// HasReconcileLabel reports whether the given labels contain the fleet reconcile -// label key with a non-empty value. -func HasReconcileLabel(labels map[string]string) bool { - return labels[ReconcileLabelKey] != "" -} - // IsFleetMemberNamespace indicates if an argued namespace is a fleet member namespace. func IsFleetMemberNamespace(namespace string) bool { return strings.HasPrefix(namespace, fleetMemberNamespacePrefix) diff --git a/pkg/webhook/deployment/deployment_mutating_webhook.go b/pkg/webhook/deployment/mutating_webhook.go similarity index 98% rename from pkg/webhook/deployment/deployment_mutating_webhook.go rename to pkg/webhook/deployment/mutating_webhook.go index f6e7765d0..1fddb5c12 100644 --- a/pkg/webhook/deployment/deployment_mutating_webhook.go +++ b/pkg/webhook/deployment/mutating_webhook.go @@ -58,7 +58,7 @@ func (m *deploymentMutator) Handle(_ context.Context, req admission.Request) adm // Only mutate when the request was made by the aksService user with // system:masters group membership. - if !isAKSService(req.UserInfo) { + if !utils.IsAKSService(req.UserInfo) { return admission.Allowed("user is not aksService, no mutation needed") } diff --git a/pkg/webhook/deployment/deployment_mutating_webhook_test.go b/pkg/webhook/deployment/mutating_webhook_test.go similarity index 98% rename from pkg/webhook/deployment/deployment_mutating_webhook_test.go rename to pkg/webhook/deployment/mutating_webhook_test.go index 123d5c3b4..f1f30ba5c 100644 --- a/pkg/webhook/deployment/deployment_mutating_webhook_test.go +++ b/pkg/webhook/deployment/mutating_webhook_test.go @@ -58,18 +58,18 @@ func TestHandle(t *testing.T) { decoder := admission.NewDecoder(scheme) aksServiceUser := authenticationv1.UserInfo{ - Username: AKSServiceUserName, - Groups: []string{SystemMastersGroup}, + Username: utils.AKSServiceUserName, + Groups: []string{utils.SystemMastersGroup}, } aksServiceUserNoMasters := authenticationv1.UserInfo{ - Username: AKSServiceUserName, + Username: utils.AKSServiceUserName, Groups: []string{"system:authenticated"}, } regularUser := authenticationv1.UserInfo{ Username: "regular-user", - Groups: []string{SystemMastersGroup, "system:authenticated"}, + Groups: []string{utils.SystemMastersGroup, "system:authenticated"}, } // Deployment with no existing labels. diff --git a/pkg/webhook/deployment/deployment_validating_webhook.go b/pkg/webhook/deployment/validating_webhook.go similarity index 86% rename from pkg/webhook/deployment/deployment_validating_webhook.go rename to pkg/webhook/deployment/validating_webhook.go index c597c383f..24f07c4aa 100644 --- a/pkg/webhook/deployment/deployment_validating_webhook.go +++ b/pkg/webhook/deployment/validating_webhook.go @@ -25,11 +25,9 @@ import ( "context" "fmt" "net/http" - "slices" admissionv1 "k8s.io/api/admission/v1" appsv1 "k8s.io/api/apps/v1" - authenticationv1 "k8s.io/api/authentication/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/klog/v2" "sigs.k8s.io/controller-runtime/pkg/manager" @@ -41,12 +39,6 @@ import ( const ( deniedReconcileLabelFmt = "the %s label is reserved for aksService and cannot be set by user %q" - - // AKSServiceUserName is the username whose deployment requests should be mutated. - AKSServiceUserName = "aksService" - - // SystemMastersGroup is the Kubernetes group representing cluster administrators. - SystemMastersGroup = "system:masters" ) // ValidationPath is the webhook service path for validating Deployment resources. @@ -91,7 +83,7 @@ func (v *deploymentValidator) Handle(_ context.Context, req admission.Request) a // The reconcile label is present — only aksService with system:masters is // allowed to set it. - if isAKSService(req.UserInfo) { + if utils.IsAKSService(req.UserInfo) { klog.V(2).InfoS("aksService user allowed to set reconcile label", "namespacedName", namespacedName) return admission.Allowed("aksService user is allowed to set the reconcile label") @@ -101,9 +93,3 @@ func (v *deploymentValidator) Handle(_ context.Context, req admission.Request) a "user", req.UserInfo.Username, "groups", req.UserInfo.Groups, "namespacedName", namespacedName) return admission.Denied(fmt.Sprintf(deniedReconcileLabelFmt, utils.ReconcileLabelKey, req.UserInfo.Username)) } - -// isAKSService reports whether the user is the aksService user with -// system:masters group membership. -func isAKSService(userInfo authenticationv1.UserInfo) bool { - return userInfo.Username == AKSServiceUserName && slices.Contains(userInfo.Groups, SystemMastersGroup) -} diff --git a/pkg/webhook/deployment/deployment_validating_webhook_test.go b/pkg/webhook/deployment/validating_webhook_test.go similarity index 95% rename from pkg/webhook/deployment/deployment_validating_webhook_test.go rename to pkg/webhook/deployment/validating_webhook_test.go index 289904947..eb05a32f5 100644 --- a/pkg/webhook/deployment/deployment_validating_webhook_test.go +++ b/pkg/webhook/deployment/validating_webhook_test.go @@ -51,12 +51,12 @@ func TestValidatingHandle(t *testing.T) { decoder := admission.NewDecoder(scheme) aksServiceUser := authenticationv1.UserInfo{ - Username: AKSServiceUserName, - Groups: []string{SystemMastersGroup}, + Username: utils.AKSServiceUserName, + Groups: []string{utils.SystemMastersGroup}, } aksServiceUserNoMasters := authenticationv1.UserInfo{ - Username: AKSServiceUserName, + Username: utils.AKSServiceUserName, Groups: []string{"system:authenticated"}, } @@ -67,7 +67,7 @@ func TestValidatingHandle(t *testing.T) { regularUserWithMasters := authenticationv1.UserInfo{ Username: "regular-user", - Groups: []string{SystemMastersGroup, "system:authenticated"}, + Groups: []string{utils.SystemMastersGroup, "system:authenticated"}, } // Deployment without the reconcile label. @@ -357,28 +357,28 @@ func TestIsAKSService(t *testing.T) { }{ "aksService with system:masters": { userInfo: authenticationv1.UserInfo{ - Username: AKSServiceUserName, - Groups: []string{SystemMastersGroup}, + Username: utils.AKSServiceUserName, + Groups: []string{utils.SystemMastersGroup}, }, want: true, }, "aksService with system:masters and other groups": { userInfo: authenticationv1.UserInfo{ - Username: AKSServiceUserName, - Groups: []string{"system:authenticated", SystemMastersGroup, "custom-group"}, + Username: utils.AKSServiceUserName, + Groups: []string{"system:authenticated", utils.SystemMastersGroup, "custom-group"}, }, want: true, }, "aksService without system:masters": { userInfo: authenticationv1.UserInfo{ - Username: AKSServiceUserName, + Username: utils.AKSServiceUserName, Groups: []string{"system:authenticated"}, }, want: false, }, "aksService with empty groups": { userInfo: authenticationv1.UserInfo{ - Username: AKSServiceUserName, + Username: utils.AKSServiceUserName, Groups: []string{}, }, want: false, @@ -386,14 +386,14 @@ func TestIsAKSService(t *testing.T) { "regular user with system:masters": { userInfo: authenticationv1.UserInfo{ Username: "regular-user", - Groups: []string{SystemMastersGroup}, + Groups: []string{utils.SystemMastersGroup}, }, want: false, }, "empty username with system:masters": { userInfo: authenticationv1.UserInfo{ Username: "", - Groups: []string{SystemMastersGroup}, + Groups: []string{utils.SystemMastersGroup}, }, want: false, }, @@ -401,7 +401,7 @@ func TestIsAKSService(t *testing.T) { for testName, tc := range testCases { t.Run(testName, func(t *testing.T) { - got := isAKSService(tc.userInfo) + got := utils.IsAKSService(tc.userInfo) if got != tc.want { t.Errorf("isAKSService(%+v) = %v, want %v", tc.userInfo, got, tc.want) }