fix: protect reconcile labels on managed jobs and deployments

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")
 }

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{

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)

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")
 }

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
+}