From 74241ce7d97aa1d0043e39d50e5aebcbe838f070 Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Fri, 9 Oct 2026 11:44:33 -0700 Subject: [PATCH 1/3] add Job mutating/validating webhook E2Es Signed-off-by: Arvind Thirumurugan --- test/e2e/fleet_guard_rail_test.go | 248 ++++++++++++++++++++++++++++++ 1 file changed, 248 insertions(+) diff --git a/test/e2e/fleet_guard_rail_test.go b/test/e2e/fleet_guard_rail_test.go index 67210736e..68e2af718 100644 --- a/test/e2e/fleet_guard_rail_test.go +++ b/test/e2e/fleet_guard_rail_test.go @@ -19,12 +19,14 @@ import ( "encoding/json" "errors" "fmt" + "strings" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" admissionv1 "k8s.io/api/admission/v1" appsv1 "k8s.io/api/apps/v1" authenticationv1 "k8s.io/api/authentication/v1" + batchv1 "k8s.io/api/batch/v1" corev1 "k8s.io/api/core/v1" policyv1 "k8s.io/api/policy/v1" rbacv1 "k8s.io/api/rbac/v1" @@ -1610,6 +1612,252 @@ var _ = Describe("fleet deployment webhook tests for validating and mutating dep }) }) +var _ = Describe("fleet Job webhook tests", Label("job-webhook"), Serial, Ordered, func() { + const testNamespace = "default" + + newJob := func(name, namespace string, jobLabels, podTemplateLabels map[string]string, suspend bool) *batchv1.Job { + podLabels := map[string]string{"app": name} + for key, value := range podTemplateLabels { + podLabels[key] = value + } + + return &batchv1.Job{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + Labels: jobLabels, + }, + Spec: batchv1.JobSpec{ + BackoffLimit: ptr.To(int32(0)), + Suspend: ptr.To(suspend), + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: podLabels, + }, + Spec: corev1.PodSpec{ + RestartPolicy: corev1.RestartPolicyNever, + Containers: []corev1.Container{ + { + Name: "test", + Image: "alpine:3", + Command: []string{"sh", "-c", "echo completed"}, + }, + }, + }, + }, + }, + } + } + + expectReconcileLabelDenial := func(err error) { + var statusErr *k8sErrors.StatusError + Expect(errors.As(err, &statusErr)).To(BeTrue(), "request error %T should be a StatusError", err) + Expect(statusErr.ErrStatus.Message).To(ContainSubstring(utils.ReconcileLabelKey)) + } + + Context("validating webhook", func() { + It("denies a regular user creating a Job labeled on Job metadata", func() { + job := newJob( + fmt.Sprintf("job-val-meta-%d", GinkgoParallelProcess()), + testNamespace, + map[string]string{utils.ReconcileLabelKey: utils.ReconcileLabelValue}, + nil, + true, + ) + expectReconcileLabelDenial(notMasterUser.Create(ctx, job)) + }) + + It("denies a regular user creating a Job labeled on pod-template metadata", func() { + job := newJob( + fmt.Sprintf("job-val-template-%d", GinkgoParallelProcess()), + testNamespace, + nil, + map[string]string{utils.ReconcileLabelKey: utils.ReconcileLabelValue}, + true, + ) + expectReconcileLabelDenial(notMasterUser.Create(ctx, job)) + }) + + It("denies a regular user updating a Job to add the reconcile label", func() { + job := newJob(fmt.Sprintf("job-val-update-%d", GinkgoParallelProcess()), testNamespace, nil, nil, true) + Expect(notMasterUser.Create(ctx, job)).To(Succeed()) + DeferCleanup(func() { + _ = hubClient.Delete(ctx, job) + }) + + Eventually(func() error { + var currentJob batchv1.Job + if err := hubClient.Get(ctx, types.NamespacedName{Name: job.Name, Namespace: job.Namespace}, ¤tJob); err != nil { + return err + } + if currentJob.Labels == nil { + currentJob.Labels = map[string]string{} + } + currentJob.Labels[utils.ReconcileLabelKey] = utils.ReconcileLabelValue + + err := notMasterUser.Update(ctx, ¤tJob) + if k8sErrors.IsConflict(err) { + return err + } + var statusErr *k8sErrors.StatusError + if !errors.As(err, &statusErr) { + return fmt.Errorf("update Job error %T, want StatusError", err) + } + if !strings.Contains(statusErr.ErrStatus.Message, utils.ReconcileLabelKey) { + return fmt.Errorf("update Job error %q does not mention %q", statusErr.ErrStatus.Message, utils.ReconcileLabelKey) + } + return nil + }, eventuallyDuration, eventuallyInterval).Should(Succeed()) + }) + + It("allows aksService to create a labeled Job", func() { + job := newJob( + fmt.Sprintf("job-val-aks-%d", GinkgoParallelProcess()), + testNamespace, + map[string]string{utils.ReconcileLabelKey: utils.ReconcileLabelValue}, + map[string]string{utils.ReconcileLabelKey: utils.ReconcileLabelValue}, + true, + ) + DeferCleanup(func() { + _ = hubClient.Delete(ctx, job) + }) + Expect(sysMastersClient.Create(ctx, job)).To(Succeed()) + }) + + It("allows a regular user to create an unlabeled Job", func() { + job := newJob(fmt.Sprintf("job-val-unlabeled-%d", GinkgoParallelProcess()), testNamespace, nil, nil, true) + DeferCleanup(func() { + _ = hubClient.Delete(ctx, job) + }) + Expect(notMasterUser.Create(ctx, job)).To(Succeed()) + }) + }) + + Context("mutating webhook", func() { + It("injects both reconcile labels when aksService creates a Job", func() { + job := newJob(fmt.Sprintf("job-mut-aks-%d", GinkgoParallelProcess()), testNamespace, nil, nil, true) + DeferCleanup(func() { + _ = hubClient.Delete(ctx, job) + }) + Expect(sysMastersClient.Create(ctx, job)).To(Succeed()) + + Eventually(func(g Gomega) { + var currentJob batchv1.Job + g.Expect(hubClient.Get(ctx, types.NamespacedName{Name: job.Name, Namespace: job.Namespace}, ¤tJob)).To(Succeed()) + g.Expect(currentJob.Labels).To(HaveKeyWithValue(utils.ReconcileLabelKey, utils.ReconcileLabelValue)) + g.Expect(currentJob.Spec.Template.Labels).To(HaveKeyWithValue(utils.ReconcileLabelKey, utils.ReconcileLabelValue)) + }, eventuallyDuration, eventuallyInterval).Should(Succeed()) + }) + + It("preserves both reconcile labels when aksService updates a Job", func() { + job := newJob(fmt.Sprintf("job-mut-update-%d", GinkgoParallelProcess()), testNamespace, nil, nil, true) + Expect(sysMastersClient.Create(ctx, job)).To(Succeed()) + DeferCleanup(func() { + _ = hubClient.Delete(ctx, job) + }) + + Eventually(func() error { + var currentJob batchv1.Job + if err := hubClient.Get(ctx, types.NamespacedName{Name: job.Name, Namespace: job.Namespace}, ¤tJob); err != nil { + return err + } + if currentJob.Annotations == nil { + currentJob.Annotations = map[string]string{} + } + currentJob.Annotations["fleet.azure.com/e2e-update"] = "true" + return sysMastersClient.Update(ctx, ¤tJob) + }, eventuallyDuration, eventuallyInterval).Should(Succeed()) + + Eventually(func(g Gomega) { + var currentJob batchv1.Job + g.Expect(hubClient.Get(ctx, types.NamespacedName{Name: job.Name, Namespace: job.Namespace}, ¤tJob)).To(Succeed()) + g.Expect(currentJob.Labels).To(HaveKeyWithValue(utils.ReconcileLabelKey, utils.ReconcileLabelValue)) + g.Expect(currentJob.Spec.Template.Labels).To(HaveKeyWithValue(utils.ReconcileLabelKey, utils.ReconcileLabelValue)) + }, eventuallyDuration, eventuallyInterval).Should(Succeed()) + }) + + It("does not inject reconcile labels when a regular user creates a Job", func() { + job := newJob(fmt.Sprintf("job-mut-user-%d", GinkgoParallelProcess()), testNamespace, nil, nil, true) + DeferCleanup(func() { + _ = hubClient.Delete(ctx, job) + }) + Expect(notMasterUser.Create(ctx, job)).To(Succeed()) + + Eventually(func(g Gomega) { + var currentJob batchv1.Job + g.Expect(hubClient.Get(ctx, types.NamespacedName{Name: job.Name, Namespace: job.Namespace}, ¤tJob)).To(Succeed()) + g.Expect(currentJob.Labels).NotTo(HaveKey(utils.ReconcileLabelKey)) + g.Expect(currentJob.Spec.Template.Labels).NotTo(HaveKey(utils.ReconcileLabelKey)) + }, eventuallyDuration, eventuallyInterval).Should(Succeed()) + }) + + It("does not inject reconcile labels in a reserved namespace", func() { + job := newJob(fmt.Sprintf("job-mut-reserved-%d", GinkgoParallelProcess()), "kube-system", nil, nil, true) + DeferCleanup(func() { + _ = hubClient.Delete(ctx, job) + }) + Expect(sysMastersClient.Create(ctx, job)).To(Succeed()) + + Eventually(func(g Gomega) { + var currentJob batchv1.Job + g.Expect(hubClient.Get(ctx, types.NamespacedName{Name: job.Name, Namespace: job.Namespace}, ¤tJob)).To(Succeed()) + g.Expect(currentJob.Labels).NotTo(HaveKey(utils.ReconcileLabelKey)) + g.Expect(currentJob.Spec.Template.Labels).NotTo(HaveKey(utils.ReconcileLabelKey)) + }, eventuallyDuration, eventuallyInterval).Should(Succeed()) + }) + }) + + Context("Job controller Pod creation", func() { + It("blocks Pod creation for an unlabeled regular-user Job", func() { + job := newJob(fmt.Sprintf("job-e2e-user-%d", GinkgoParallelProcess()), testNamespace, nil, nil, false) + DeferCleanup(func() { + _ = hubClient.Delete(ctx, job) + }) + Expect(notMasterUser.Create(ctx, job)).To(Succeed()) + + Consistently(func(g Gomega) { + var podList corev1.PodList + g.Expect(hubClient.List(ctx, &podList, client.InNamespace(testNamespace), client.MatchingLabels{"app": job.Name})).To(Succeed()) + g.Expect(podList.Items).To(BeEmpty()) + }, consistentlyDuration, consistentlyInterval).Should(Succeed()) + }) + + It("allows an aksService Job to create a labeled Pod and complete", func() { + job := newJob(fmt.Sprintf("job-e2e-aks-%d", GinkgoParallelProcess()), testNamespace, nil, nil, false) + DeferCleanup(func() { + _ = hubClient.Delete(ctx, job) + }) + Expect(sysMastersClient.Create(ctx, job)).To(Succeed()) + + Eventually(func(g Gomega) { + var currentJob batchv1.Job + g.Expect(hubClient.Get(ctx, types.NamespacedName{Name: job.Name, Namespace: job.Namespace}, ¤tJob)).To(Succeed()) + g.Expect(currentJob.Labels).To(HaveKeyWithValue(utils.ReconcileLabelKey, utils.ReconcileLabelValue)) + g.Expect(currentJob.Spec.Template.Labels).To(HaveKeyWithValue(utils.ReconcileLabelKey, utils.ReconcileLabelValue)) + }, eventuallyDuration, eventuallyInterval).Should(Succeed()) + + Eventually(func(g Gomega) { + var podList corev1.PodList + g.Expect(hubClient.List(ctx, &podList, client.InNamespace(testNamespace), client.MatchingLabels{"app": job.Name})).To(Succeed()) + g.Expect(podList.Items).NotTo(BeEmpty()) + for i := range podList.Items { + g.Expect(podList.Items[i].Labels).To(HaveKeyWithValue(utils.ReconcileLabelKey, utils.ReconcileLabelValue)) + } + }, workloadEventuallyDuration, eventuallyInterval).Should(Succeed()) + + Eventually(func(g Gomega) { + var currentJob batchv1.Job + g.Expect(hubClient.Get(ctx, types.NamespacedName{Name: job.Name, Namespace: job.Namespace}, ¤tJob)).To(Succeed()) + g.Expect(currentJob.Status.Succeeded).To(Equal(int32(1))) + g.Expect(currentJob.Status.Conditions).To(ContainElement(And( + HaveField("Type", batchv1.JobComplete), + HaveField("Status", corev1.ConditionTrue), + ))) + }, workloadEventuallyDuration, eventuallyInterval).Should(Succeed()) + }) + }) +}) + var _ = Describe("fleet guard rail webhook tests for service accounts in restricted namespaces", Ordered, func() { svcAccountGVK := metav1.GroupVersionKind{Group: corev1.SchemeGroupVersion.Group, Version: corev1.SchemeGroupVersion.Version, Kind: "ServiceAccount"} tokenReqGVK := metav1.GroupVersionKind{Group: "authentication.k8s.io", Version: "v1", Kind: "TokenRequest"} From c15cfa8e84f97329c658765ff7eb790f09fa50b3 Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Fri, 9 Oct 2026 12:41:29 -0700 Subject: [PATCH 2/3] fix E2E Signed-off-by: Arvind Thirumurugan --- test/e2e/fleet_guard_rail_test.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/test/e2e/fleet_guard_rail_test.go b/test/e2e/fleet_guard_rail_test.go index 68e2af718..736ce972d 100644 --- a/test/e2e/fleet_guard_rail_test.go +++ b/test/e2e/fleet_guard_rail_test.go @@ -1749,13 +1749,20 @@ var _ = Describe("fleet Job webhook tests", Label("job-webhook"), Serial, Ordere }, eventuallyDuration, eventuallyInterval).Should(Succeed()) }) - It("preserves both reconcile labels when aksService updates a Job", func() { + It("injects both reconcile labels when aksService updates an existing unlabeled Job", func() { job := newJob(fmt.Sprintf("job-mut-update-%d", GinkgoParallelProcess()), testNamespace, nil, nil, true) - Expect(sysMastersClient.Create(ctx, job)).To(Succeed()) + Expect(notMasterUser.Create(ctx, job)).To(Succeed()) DeferCleanup(func() { _ = hubClient.Delete(ctx, job) }) + Eventually(func(g Gomega) { + var currentJob batchv1.Job + g.Expect(hubClient.Get(ctx, types.NamespacedName{Name: job.Name, Namespace: job.Namespace}, ¤tJob)).To(Succeed()) + g.Expect(currentJob.Labels).NotTo(HaveKey(utils.ReconcileLabelKey)) + g.Expect(currentJob.Spec.Template.Labels).NotTo(HaveKey(utils.ReconcileLabelKey)) + }, eventuallyDuration, eventuallyInterval).Should(Succeed()) + Eventually(func() error { var currentJob batchv1.Job if err := hubClient.Get(ctx, types.NamespacedName{Name: job.Name, Namespace: job.Namespace}, ¤tJob); err != nil { From e2f5ed98f0ef86bcba7ef0a3820746eb3c048c83 Mon Sep 17 00:00:00 2001 From: Arvind Thirumurugan Date: Fri, 9 Oct 2026 13:28:29 -0700 Subject: [PATCH 3/3] ensure non aksService user cannot remove fleet reconcile label on jobs/deployments Signed-off-by: Arvind Thirumurugan --- pkg/webhook/deployment/validating_webhook.go | 31 ++++--- .../deployment/validating_webhook_test.go | 14 +++ pkg/webhook/job/mutating_webhook.go | 2 + pkg/webhook/job/validating_webhook.go | 29 ++++-- pkg/webhook/job/validating_webhook_test.go | 19 +++- test/e2e/fleet_guard_rail_test.go | 92 +++++++++++++++++++ 6 files changed, 165 insertions(+), 22 deletions(-) diff --git a/pkg/webhook/deployment/validating_webhook.go b/pkg/webhook/deployment/validating_webhook.go index 24f07c4aa..4cb65d130 100644 --- a/pkg/webhook/deployment/validating_webhook.go +++ b/pkg/webhook/deployment/validating_webhook.go @@ -38,7 +38,7 @@ import ( ) const ( - deniedReconcileLabelFmt = "the %s label is reserved for aksService and cannot be set by user %q" + deniedReconcileLabelFmt = "resources with the %s label are reserved for aksService and cannot be modified by user %q" ) // ValidationPath is the webhook service path for validating Deployment resources. @@ -77,19 +77,28 @@ func (v *deploymentValidator) Handle(_ context.Context, req admission.Request) a 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 utils.IsAKSService(req.UserInfo) { + if hasLabelOnDeploy || hasLabelOnPodTemplate { + if !utils.IsAKSService(req.UserInfo) { + klog.V(2).InfoS("denied non-aksService user from modifying deployment with reconcile label", + "user", req.UserInfo.Username, "groups", req.UserInfo.Groups, "namespacedName", namespacedName) + return admission.Denied(fmt.Sprintf(deniedReconcileLabelFmt, utils.ReconcileLabelKey, req.UserInfo.Username)) + } 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)) + if req.Operation == admissionv1.Update && !utils.IsAKSService(req.UserInfo) { + var oldDeploy appsv1.Deployment + if err := v.decoder.DecodeRaw(req.OldObject, &oldDeploy); err != nil { + return admission.Errored(http.StatusBadRequest, err) + } + if utils.HasReconcileLabel(oldDeploy.Labels) || utils.HasReconcileLabel(oldDeploy.Spec.Template.Labels) { + klog.V(2).InfoS("denied non-aksService user from removing reconcile label", + "user", req.UserInfo.Username, "groups", req.UserInfo.Groups, "namespacedName", namespacedName) + return admission.Denied(fmt.Sprintf(deniedReconcileLabelFmt, utils.ReconcileLabelKey, req.UserInfo.Username)) + } + } + + return admission.Allowed("deployment does not have the reconcile label, no validation needed") } diff --git a/pkg/webhook/deployment/validating_webhook_test.go b/pkg/webhook/deployment/validating_webhook_test.go index eb05a32f5..d368a6b31 100644 --- a/pkg/webhook/deployment/validating_webhook_test.go +++ b/pkg/webhook/deployment/validating_webhook_test.go @@ -143,11 +143,25 @@ func TestValidatingHandle(t *testing.T) { Namespace: "default", Operation: admissionv1.Update, Object: runtime.RawExtension{Raw: deployNoReconcileLabelBytes, Object: deployNoReconcileLabel}, + OldObject: runtime.RawExtension{Raw: deployNoReconcileLabelBytes, Object: deployNoReconcileLabel}, UserInfo: regularUser, }, }, wantAllowed: true, }, + "deny regular user from removing reconcile labels from both locations": { + req: admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Name: "test-deploy", + Namespace: "default", + Operation: admissionv1.Update, + Object: runtime.RawExtension{Raw: deployNoReconcileLabelBytes, Object: deployNoReconcileLabel}, + OldObject: runtime.RawExtension{Raw: deployWithReconcileLabelOnBothBytes, Object: deployWithReconcileLabelOnBoth}, + UserInfo: regularUser, + }, + }, + wantAllowed: false, + }, "deny regular user from setting reconcile label in kube-system namespace": { req: admission.Request{ AdmissionRequest: admissionv1.AdmissionRequest{ diff --git a/pkg/webhook/job/mutating_webhook.go b/pkg/webhook/job/mutating_webhook.go index 08b81777b..5edbed540 100644 --- a/pkg/webhook/job/mutating_webhook.go +++ b/pkg/webhook/job/mutating_webhook.go @@ -72,6 +72,8 @@ func (m *jobMutator) Handle(_ context.Context, req admission.Request) admission. if job.Spec.Template.Labels == nil { job.Spec.Template.Labels = map[string]string{} } + // Kubernetes permits pod-template metadata updates for suspended Jobs that + // have never started, but rejects them as immutable after a Job has started. job.Spec.Template.Labels[utils.ReconcileLabelKey] = utils.ReconcileLabelValue marshaled, err := json.Marshal(job) diff --git a/pkg/webhook/job/validating_webhook.go b/pkg/webhook/job/validating_webhook.go index 79e4bd3ad..c0c271639 100644 --- a/pkg/webhook/job/validating_webhook.go +++ b/pkg/webhook/job/validating_webhook.go @@ -33,7 +33,7 @@ import ( "go.goms.io/fleet/pkg/utils" ) -const deniedReconcileLabelFmt = "the %s label is reserved for aksService and cannot be set by user %q" +const deniedReconcileLabelFmt = "resources with the %s label are reserved for aksService and cannot be modified by user %q" // ValidationPath is the webhook service path for validating Job resources. var ValidationPath = fmt.Sprintf(utils.ValidationPathFmt, batchv1.SchemeGroupVersion.Group, batchv1.SchemeGroupVersion.Version, "job") @@ -67,17 +67,28 @@ func (v *jobValidator) Handle(_ context.Context, req admission.Request) admissio hasLabelOnJob := utils.HasReconcileLabel(job.Labels) hasLabelOnPodTemplate := utils.HasReconcileLabel(job.Spec.Template.Labels) - if !hasLabelOnJob && !hasLabelOnPodTemplate { - return admission.Allowed("job does not have the reconcile label, no validation needed") - } - - if utils.IsAKSService(req.UserInfo) { + if hasLabelOnJob || hasLabelOnPodTemplate { + if !utils.IsAKSService(req.UserInfo) { + klog.V(2).InfoS("denied non-aksService user from modifying job with reconcile label", + "user", req.UserInfo.Username, "groups", req.UserInfo.Groups, "namespacedName", namespacedName) + return admission.Denied(fmt.Sprintf(deniedReconcileLabelFmt, utils.ReconcileLabelKey, req.UserInfo.Username)) + } 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)) + if req.Operation == admissionv1.Update && !utils.IsAKSService(req.UserInfo) { + var oldJob batchv1.Job + if err := v.decoder.DecodeRaw(req.OldObject, &oldJob); err != nil { + return admission.Errored(http.StatusBadRequest, err) + } + if utils.HasReconcileLabel(oldJob.Labels) || utils.HasReconcileLabel(oldJob.Spec.Template.Labels) { + klog.V(2).InfoS("denied non-aksService user from removing reconcile label", + "user", req.UserInfo.Username, "groups", req.UserInfo.Groups, "namespacedName", namespacedName) + return admission.Denied(fmt.Sprintf(deniedReconcileLabelFmt, utils.ReconcileLabelKey, req.UserInfo.Username)) + } + } + + return admission.Allowed("job does not have the reconcile label, no validation needed") } diff --git a/pkg/webhook/job/validating_webhook_test.go b/pkg/webhook/job/validating_webhook_test.go index 87183833c..c91a6d132 100644 --- a/pkg/webhook/job/validating_webhook_test.go +++ b/pkg/webhook/job/validating_webhook_test.go @@ -106,10 +106,14 @@ func TestValidatingHandle(t *testing.T) { req: newAdmissionRequest("test-job", "default", admissionv1.Create, unlabeledJob, regularUser), wantAllowed: true, }, - "allow update that removes labels from both locations": { - req: newAdmissionRequest("test-job", "default", admissionv1.Update, unlabeledJob, regularUser), + "allow regular user update when old and new jobs are unlabeled": { + req: newUpdateAdmissionRequest("test-job", "default", unlabeledJob, unlabeledJob, regularUser), wantAllowed: true, }, + "deny regular user update that removes labels from both locations": { + req: newUpdateAdmissionRequest("test-job", "default", unlabeledJob, bothLabeled, regularUser), + wantAllowed: false, + }, "allow aksService user to create with label on job metadata": { req: newAdmissionRequest("test-job", "default", admissionv1.Create, jobMetadataLabeled, aksServiceUser), wantAllowed: true, @@ -175,6 +179,11 @@ func TestValidatingHandle(t *testing.T) { wantAllowed: false, wantErrCode: http.StatusBadRequest, }, + "error on malformed old object": { + req: newUpdateAdmissionRequest("test-job", "default", unlabeledJob, []byte("not valid json"), regularUser), + wantAllowed: false, + wantErrCode: http.StatusBadRequest, + }, } for testName, tc := range testCases { @@ -198,3 +207,9 @@ func TestValidatingHandle(t *testing.T) { }) } } + +func newUpdateAdmissionRequest(name, namespace string, raw, oldRaw []byte, userInfo authenticationv1.UserInfo) admission.Request { + req := newAdmissionRequest(name, namespace, admissionv1.Update, raw, userInfo) + req.OldObject = runtime.RawExtension{Raw: oldRaw} + return req +} diff --git a/test/e2e/fleet_guard_rail_test.go b/test/e2e/fleet_guard_rail_test.go index 736ce972d..e68bee7f5 100644 --- a/test/e2e/fleet_guard_rail_test.go +++ b/test/e2e/fleet_guard_rail_test.go @@ -1407,6 +1407,52 @@ var _ = Describe("fleet deployment webhook tests for validating and mutating dep return nil }, eventuallyDuration, eventuallyInterval).Should(Succeed()) }) + + It("should deny regular user from updating or removing labels from a managed deployment", func() { + deploy := newDeployment(fmt.Sprintf("test-deploy-val-managed-%d", GinkgoParallelProcess()), testNS, nil, nil) + Expect(sysMastersClient.Create(ctx, deploy)).Should(Succeed()) + DeferCleanup(func() { + _ = hubClient.Delete(ctx, deploy) + }) + + Eventually(func() error { + var currentDeploy appsv1.Deployment + if err := hubClient.Get(ctx, types.NamespacedName{Name: deploy.Name, Namespace: deploy.Namespace}, ¤tDeploy); err != nil { + return err + } + if currentDeploy.Annotations == nil { + currentDeploy.Annotations = map[string]string{} + } + currentDeploy.Annotations["fleet.azure.com/e2e-update"] = "true" + err := notMasterUser.Update(ctx, ¤tDeploy) + if k8sErrors.IsConflict(err) { + return err + } + var statusErr *k8sErrors.StatusError + if !errors.As(err, &statusErr) || !strings.Contains(statusErr.ErrStatus.Message, utils.ReconcileLabelKey) { + return fmt.Errorf("unrelated deployment update error = %v, want denial mentioning %q", err, utils.ReconcileLabelKey) + } + return nil + }, eventuallyDuration, eventuallyInterval).Should(Succeed()) + + Eventually(func() error { + var currentDeploy appsv1.Deployment + if err := hubClient.Get(ctx, types.NamespacedName{Name: deploy.Name, Namespace: deploy.Namespace}, ¤tDeploy); err != nil { + return err + } + delete(currentDeploy.Labels, utils.ReconcileLabelKey) + delete(currentDeploy.Spec.Template.Labels, utils.ReconcileLabelKey) + err := notMasterUser.Update(ctx, ¤tDeploy) + if k8sErrors.IsConflict(err) { + return err + } + var statusErr *k8sErrors.StatusError + if !errors.As(err, &statusErr) || !strings.Contains(statusErr.ErrStatus.Message, utils.ReconcileLabelKey) { + return fmt.Errorf("deployment label removal error = %v, want denial mentioning %q", err, utils.ReconcileLabelKey) + } + return nil + }, eventuallyDuration, eventuallyInterval).Should(Succeed()) + }) }) Context("validating webhook - allow aksService user to set reconcile label", func() { @@ -1710,6 +1756,52 @@ var _ = Describe("fleet Job webhook tests", Label("job-webhook"), Serial, Ordere }, eventuallyDuration, eventuallyInterval).Should(Succeed()) }) + It("denies a regular user updating or removing labels from a managed Job", func() { + job := newJob(fmt.Sprintf("job-val-managed-%d", GinkgoParallelProcess()), testNamespace, nil, nil, true) + Expect(sysMastersClient.Create(ctx, job)).To(Succeed()) + DeferCleanup(func() { + _ = hubClient.Delete(ctx, job) + }) + + Eventually(func() error { + var currentJob batchv1.Job + if err := hubClient.Get(ctx, types.NamespacedName{Name: job.Name, Namespace: job.Namespace}, ¤tJob); err != nil { + return err + } + if currentJob.Annotations == nil { + currentJob.Annotations = map[string]string{} + } + currentJob.Annotations["fleet.azure.com/e2e-update"] = "true" + err := notMasterUser.Update(ctx, ¤tJob) + if k8sErrors.IsConflict(err) { + return err + } + var statusErr *k8sErrors.StatusError + if !errors.As(err, &statusErr) || !strings.Contains(statusErr.ErrStatus.Message, utils.ReconcileLabelKey) { + return fmt.Errorf("unrelated Job update error = %v, want denial mentioning %q", err, utils.ReconcileLabelKey) + } + return nil + }, eventuallyDuration, eventuallyInterval).Should(Succeed()) + + Eventually(func() error { + var currentJob batchv1.Job + if err := hubClient.Get(ctx, types.NamespacedName{Name: job.Name, Namespace: job.Namespace}, ¤tJob); err != nil { + return err + } + delete(currentJob.Labels, utils.ReconcileLabelKey) + delete(currentJob.Spec.Template.Labels, utils.ReconcileLabelKey) + err := notMasterUser.Update(ctx, ¤tJob) + if k8sErrors.IsConflict(err) { + return err + } + var statusErr *k8sErrors.StatusError + if !errors.As(err, &statusErr) || !strings.Contains(statusErr.ErrStatus.Message, utils.ReconcileLabelKey) { + return fmt.Errorf("Job label removal error = %v, want denial mentioning %q", err, utils.ReconcileLabelKey) + } + return nil + }, eventuallyDuration, eventuallyInterval).Should(Succeed()) + }) + It("allows aksService to create a labeled Job", func() { job := newJob( fmt.Sprintf("job-val-aks-%d", GinkgoParallelProcess()),