enforce appwrapper components is immutable except to AW controller (#34)

Author: dgrove-oss

internal/controller/appwrapper_controller.go

@@ -384,15 +384,19 @@ func (r *AppWrapperReconciler) workloadStatus(ctx context.Context, aw *workloadv
 	return summary, nil
 }
 
+func replicas(ps workloadv1beta2.AppWrapperPodSet) int32 {
+	if ps.Replicas == nil {
+		return 1
+	} else {
+		return *ps.Replicas
+	}
+}
+
 func expectedPodCount(aw *workloadv1beta2.AppWrapper) int32 {
 	var expected int32
 	for _, c := range aw.Spec.Components {
 		for _, s := range c.PodSets {
-			if s.Replicas == nil {
-				expected++
-			} else {
-				expected += *s.Replicas
-			}
+			expected += replicas(s)
 		}
 	}
 	return expected

internal/controller/appwrapper_controller_test.go

@@ -52,7 +52,7 @@ var _ = Describe("AppWrapper Controller", func() {
 		By("Updating aw.Spec by invoking RunWithPodSetsInfo")
 		Expect((*AppWrapper)(aw).RunWithPodSetsInfo([]podset.PodSetInfo{markerPodSet, markerPodSet})).To(Succeed())
 		Expect(aw.Spec.Suspend).To(BeFalse())
-		Expect(k8sClient.Update(ctx, aw)).To(Succeed())
+		Expect(k8sControllerClient.Update(ctx, aw)).To(Succeed())
 
 		By("Reconciling: Suspended -> Resuming")
 		_, err = awReconciler.Reconcile(ctx, reconcile.Request{NamespacedName: awName})
@@ -111,7 +111,7 @@ var _ = Describe("AppWrapper Controller", func() {
 			Namespace: aw.Namespace,
 		}
 		awReconciler = &AppWrapperReconciler{
-			Client: k8sClient,
+			Client: k8sControllerClient,
 			Scheme: k8sClient.Scheme(),
 		}
 		kueuePodSets = (*AppWrapper)(aw).PodSets()
@@ -178,7 +178,7 @@ var _ = Describe("AppWrapper Controller", func() {
 		aw := getAppWrapper(awName)
 		(*AppWrapper)(aw).Suspend()
 		Expect((*AppWrapper)(aw).RestorePodSetsInfo(utilslices.Map(kueuePodSets, podset.FromPodSet))).To(BeTrue())
-		Expect(k8sClient.Update(ctx, aw)).To(Succeed())
+		Expect(k8sControllerClient.Update(ctx, aw)).To(Succeed())
 
 		By("Reconciling: Running -> Suspending")
 		_, err := awReconciler.Reconcile(ctx, reconcile.Request{NamespacedName: awName})

internal/controller/appwrapper_webhook.go

@@ -17,6 +17,7 @@ limitations under the License.
 package controller
 
 import (
+	"bytes"
 	"context"
 	"fmt"
 
@@ -51,8 +52,8 @@ var _ webhook.CustomDefaulter = &AppWrapperWebhook{}
 // Default ensures that Suspend is set appropriately when an AppWrapper is created
 func (w *AppWrapperWebhook) Default(ctx context.Context, obj runtime.Object) error {
 	aw := obj.(*workloadv1beta2.AppWrapper)
-	log.FromContext(ctx).Info("Applying defaults", "job", aw)
 	jobframework.ApplyDefaultForSuspend((*AppWrapper)(aw), w.Config.ManageJobsWithoutQueueName)
+	log.FromContext(ctx).Info("Applied defaults", "job", aw)
 	return nil
 }
 
@@ -64,7 +65,7 @@ var _ webhook.CustomValidator = &AppWrapperWebhook{}
 func (w *AppWrapperWebhook) ValidateCreate(ctx context.Context, obj runtime.Object) (admission.Warnings, error) {
 	aw := obj.(*workloadv1beta2.AppWrapper)
 
-	allErrors := w.validateAppWrapperInvariants(ctx, aw)
+	allErrors := w.validateAppWrapperCreate(ctx, aw)
 	if w.Config.ManageJobsWithoutQueueName || jobframework.QueueName((*AppWrapper)(aw)) != "" {
 		allErrors = append(allErrors, jobframework.ValidateCreateForQueueName((*AppWrapper)(aw))...)
 	}
@@ -83,7 +84,7 @@ func (w *AppWrapperWebhook) ValidateUpdate(ctx context.Context, oldObj, newObj r
 	oldAW := oldObj.(*workloadv1beta2.AppWrapper)
 	newAW := newObj.(*workloadv1beta2.AppWrapper)
 
-	allErrors := w.validateAppWrapperInvariants(ctx, newAW)
+	allErrors := w.validateAppWrapperUpdate(ctx, oldAW, newAW)
 	if w.Config.ManageJobsWithoutQueueName || jobframework.QueueName((*AppWrapper)(newAW)) != "" {
 		allErrors = append(allErrors, jobframework.ValidateUpdateForQueueName((*AppWrapper)(oldAW), (*AppWrapper)(newAW))...)
 		allErrors = append(allErrors, jobframework.ValidateUpdateForWorkloadPriorityClassName((*AppWrapper)(oldAW), (*AppWrapper)(newAW))...)
@@ -107,13 +108,13 @@ func (w *AppWrapperWebhook) ValidateDelete(context.Context, runtime.Object) (adm
 //+kubebuilder:rbac:groups=authorization.k8s.io,resources=subjectaccessreviews,verbs=create
 //+kubebuilder:rbac:groups=apiextensions.k8s.io,resources=customresourcedefinitions,verbs=list
 
-// validateAppWrapperInvariants checks these invariants:
+// validateAppWrapperCreate checks these invariants:
 //  1. AppWrappers must not contain other AppWrappers
 //  2. AppWrappers must only contain resources intended for their own namespace
 //  3. AppWrappers must not contain any resources that the user could not create directly
 //  4. Every PodSet must be well-formed: the Path must exist and must be parseable as a PodSpecTemplate
 //  5. AppWrappers must contain between 1 and 8 PodSets (Kueue invariant)
-func (w *AppWrapperWebhook) validateAppWrapperInvariants(ctx context.Context, aw *workloadv1beta2.AppWrapper) field.ErrorList {
+func (w *AppWrapperWebhook) validateAppWrapperCreate(ctx context.Context, aw *workloadv1beta2.AppWrapper) field.ErrorList {
 	allErrors := field.ErrorList{}
 	components := aw.Spec.Components
 	componentsPath := field.NewPath("spec").Child("components")
@@ -124,11 +125,6 @@ func (w *AppWrapperWebhook) validateAppWrapperInvariants(ctx context.Context, aw
 	}
 	userInfo := request.UserInfo
 
-	// To reduce overhead, skip invariant validation when the AppWrapper controller is the user performing the update
-	if w.Config.ServiceAccountName != "" && userInfo.Username == w.Config.ServiceAccountName {
-		return allErrors
-	}
-
 	for idx, component := range components {
 		compPath := componentsPath.Index(idx)
 		unstruct := &unstructured.Unstructured{}
@@ -205,6 +201,46 @@ func (w *AppWrapperWebhook) validateAppWrapperInvariants(ctx context.Context, aw
 	return allErrors
 }
 
+// validateAppWrapperUpdate enforces that AppWrapper.Spec.Components is deeply immutable
+func (w *AppWrapperWebhook) validateAppWrapperUpdate(ctx context.Context, old *workloadv1beta2.AppWrapper, new *workloadv1beta2.AppWrapper) field.ErrorList {
+	// The AppWrapper controller must be allowed to mutate Spec.Components
+	// to enable it to implement RunWithPodSetsInfo and RestorePodSetsInfo
+	if request, err := admission.RequestFromContext(ctx); err == nil {
+		if w.Config.ServiceAccountName != "" && request.UserInfo.Username == w.Config.ServiceAccountName {
+			return field.ErrorList{}
+		}
+	}
+
+	allErrors := field.ErrorList{}
+	msg := "attempt to change immutable field"
+	componentsPath := field.NewPath("spec").Child("components")
+	if len(old.Spec.Components) != len(new.Spec.Components) {
+		return field.ErrorList{field.Forbidden(componentsPath, msg)}
+	}
+	for idx := range new.Spec.Components {
+		compPath := componentsPath.Index(idx)
+		oldComponent := old.Spec.Components[idx]
+		newComponent := new.Spec.Components[idx]
+		if !bytes.Equal(oldComponent.Template.Raw, newComponent.Template.Raw) {
+			allErrors = append(allErrors, field.Forbidden(compPath.Child("template").Child("raw"), msg))
+		}
+		if len(oldComponent.PodSets) != len(newComponent.PodSets) {
+			allErrors = append(allErrors, field.Forbidden(compPath.Child("podsets"), msg))
+		} else {
+			for psIdx := range newComponent.PodSets {
+				if replicas(oldComponent.PodSets[psIdx]) != replicas(newComponent.PodSets[psIdx]) {
+					allErrors = append(allErrors, field.Forbidden(compPath.Child("podsets").Index(psIdx).Child("replicas"), msg))
+				}
+				if oldComponent.PodSets[psIdx].Path != newComponent.PodSets[psIdx].Path {
+					allErrors = append(allErrors, field.Forbidden(compPath.Child("podsets").Index(psIdx).Child("path"), msg))
+				}
+			}
+		}
+	}
+
+	return allErrors
+}
+
 func (w *AppWrapperWebhook) lookupResource(gvk *schema.GroupVersionKind) string {
 	if known, ok := w.kindToResourceCache[gvk.String()]; ok {
 		return known

internal/controller/suite_test.go

@@ -53,6 +53,7 @@ import (
 
 var cfg *rest.Config
 var k8sClient client.Client
+var k8sControllerClient client.Client
 var k8sLimitedClient client.Client
 var testEnv *envtest.Environment
 var ctx context.Context
@@ -110,6 +111,15 @@ var _ = BeforeSuite(func() {
 	Expect(err).NotTo(HaveOccurred())
 	Expect(k8sClient).NotTo(BeNil())
 
+	// configure a client that impersonates as the AppWrapper operator
+	ctrlUserName := "appwrapper-controller"
+	ctrlCfg := *cfg
+	ctrlCfg.Impersonate = rest.ImpersonationConfig{UserName: ctrlUserName, Groups: []string{"system:masters"}}
+	_, err = testEnv.AddUser(envtest.User{Name: ctrlUserName}, &ctrlCfg)
+	Expect(err).NotTo(HaveOccurred())
+	k8sControllerClient, err = client.New(&ctrlCfg, client.Options{Scheme: scheme})
+	Expect(err).NotTo(HaveOccurred())
+
 	// configure a restricted rbac user who can create AppWrappers and Pods but not Deployments
 	limitedUserName := "limited-user"
 	limitedCfg := *cfg
@@ -151,7 +161,7 @@ var _ = BeforeSuite(func() {
 	})
 	Expect(err).NotTo(HaveOccurred())
 
-	awConfig := AppWrapperConfig{ManageJobsWithoutQueueName: true}
+	awConfig := AppWrapperConfig{ManageJobsWithoutQueueName: true, ServiceAccountName: ctrlUserName}
 	err = (&AppWrapperWebhook{Config: &awConfig}).SetupWebhookWithManager(mgr)
 	Expect(err).NotTo(HaveOccurred())