ensure vap resources in a native way

Author: yjst2012

pkg/operator/controllers/guardrails/guardrails_vap.go

@@ -118,7 +118,9 @@ func (r *Reconciler) deployVAP(ctx context.Context, instance *arov1alpha1.Cluste
 	return nil
 }
 
-// ensureVAPPolicy creates the ValidatingAdmissionPolicy if it does not exist.
+// ensureVAPPolicy creates or updates the ValidatingAdmissionPolicy.
+// The dynamic helper's Ensure method detects native Kubernetes resources
+// and uses server-side apply, so all policy fields are correctly reconciled.
 func (r *Reconciler) ensureVAPPolicy(ctx context.Context, filename string) error {
 	data, err := fs.ReadFile(vapPolicies, filepath.Join(vapPolicyPath, filename))
 	if err != nil {
@@ -131,8 +133,10 @@ func (r *Reconciler) ensureVAPPolicy(ctx context.Context, filename string) error
 	return r.dh.Ensure(ctx, uns)
 }
 
-// ensureVAPBinding deletes an existing binding and recreates it so the
-// validationActions field always reflects the current enforcement setting.
+// ensureVAPBinding creates or updates the ValidatingAdmissionPolicyBinding.
+// The dynamic helper's Ensure method detects native Kubernetes resources
+// and uses server-side apply, so validationActions changes are applied
+// atomically without needing a delete-then-create workaround.
 func (r *Reconciler) ensureVAPBinding(ctx context.Context, policyName, validationAction string) error {
 	bindingFile := policyName + "-binding.yaml"
 	tmpl, err := template.ParseFS(vapBindings, filepath.Join(vapBindingPath, bindingFile))
@@ -155,17 +159,6 @@ func (r *Reconciler) ensureVAPBinding(ctx context.Context, policyName, validatio
 		return err
 	}
 
-	bindingName := uns.GetName()
-	gk := uns.GroupVersionKind().GroupKind().String()
-	ver := uns.GroupVersionKind().Version
-
-	// Delete then recreate to pick up enforcement changes; ensureUnstructuredObj
-	// in the dynamic helper only checks Gatekeeper enforcementAction, not VAP
-	// validationActions, so an in-place update would silently no-op.
-	if err := r.dh.EnsureDeletedGVR(ctx, gk, "", bindingName, ver); err != nil {
-		r.log.Warnf("failed to delete existing VAP binding %s for recreation: %s", bindingName, err.Error())
-	}
-
 	return r.dh.Ensure(ctx, uns)
 }
 

pkg/util/dynamichelper/dynamichelper.go

@@ -5,19 +5,24 @@ package dynamichelper
 
 import (
 	"context"
+	"encoding/json"
+	"fmt"
 	"strings"
 
 	"github.com/sirupsen/logrus"
 
 	kerrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	kruntime "k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/dynamic"
 	"k8s.io/client-go/kubernetes/scheme"
 	"k8s.io/client-go/rest"
 	"k8s.io/client-go/util/retry"
+	"k8s.io/utils/pointer"
 
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
@@ -97,9 +102,19 @@ func (dh *dynamicHelper) EnsureDeletedGVR(ctx context.Context, groupKind, namesp
 func (dh *dynamicHelper) Ensure(ctx context.Context, objs ...kruntime.Object) error {
 	for _, o := range objs {
 		if un, ok := o.(*unstructured.Unstructured); ok {
-			err := dh.ensureUnstructuredObj(ctx, un)
-			if err != nil {
-				return err
+			// ValidatingAdmissionPolicy and ValidatingAdmissionPolicyBinding
+			// are handled via server-side apply so that all fields are
+			// correctly reconciled. The Gatekeeper-specific path only
+			// compares enforcementAction and would silently skip updates
+			// to other resource types.
+			if isAdmissionRegistrationResource(un) {
+				if err := dh.ensureByServerSideApply(ctx, un); err != nil {
+					return err
+				}
+			} else {
+				if err := dh.ensureGatekeeperConstraint(ctx, un); err != nil {
+					return err
+				}
 			}
 			continue
 		}
@@ -160,6 +175,40 @@ func (dh *dynamicHelper) mergeWithLogic(name, groupKind string, old, new kruntim
 	return clienthelper.Merge(old.(client.Object), new.(client.Object))
 }
 
+// isAdmissionRegistrationResource returns true for ValidatingAdmissionPolicy
+// and ValidatingAdmissionPolicyBinding resources that should be managed via
+// server-side apply rather than the Gatekeeper-specific path.
+func isAdmissionRegistrationResource(uns *unstructured.Unstructured) bool {
+	return uns.GroupVersionKind().Group == "admissionregistration.k8s.io"
+}
+
+// ensureByServerSideApply creates or updates a single unstructured object
+// using server-side apply. This correctly reconciles all fields, unlike
+// ensureUnstructuredObj which only compares Gatekeeper's enforcementAction.
+func (dh *dynamicHelper) ensureByServerSideApply(ctx context.Context, uns *unstructured.Unstructured) error {
+	gvr, err := dh.Resolve(uns.GroupVersionKind().GroupKind().String(), uns.GroupVersionKind().Version)
+	if err != nil {
+		return err
+	}
+
+	data, err := json.Marshal(uns)
+	if err != nil {
+		return fmt.Errorf("marshalling %s/%s: %w", uns.GroupVersionKind().GroupKind(), uns.GetName(), err)
+	}
+
+	dh.log.Infof("Apply %s", keyFunc(uns.GroupVersionKind().GroupKind(), uns.GetNamespace(), uns.GetName()))
+	_, err = dh.dynamicClient.Resource(*gvr).
+		Namespace(uns.GetNamespace()).
+		Patch(ctx, uns.GetName(), types.ApplyPatchType, data, metav1.PatchOptions{
+			FieldManager: "aro-operator",
+			Force:        pointer.Bool(true),
+		})
+	if err != nil {
+		return fmt.Errorf("server-side apply %s/%s: %w", uns.GroupVersionKind().GroupKind(), uns.GetName(), err)
+	}
+	return nil
+}
+
 func makeURLSegments(gvr *schema.GroupVersionResource, namespace, name string) (url []string) {
 	if gvr.Group == "" {
 		url = append(url, "api")

pkg/util/dynamichelper/guardrails.go

@@ -23,9 +23,10 @@ import (
 	_ "github.com/Azure/ARO-RP/pkg/util/scheme"
 )
 
-// the UnstructuredObj related stuff is specifically for the Guardrails
-// to handle the gatekeeper Constraint as it does not have a scheme that can be imported
-func (dh *dynamicHelper) ensureUnstructuredObj(ctx context.Context, uns *unstructured.Unstructured) error {
+// ensureGatekeeperConstraint handles Gatekeeper Constraint resources which
+// do not have a scheme that can be imported. It only compares the
+// spec.enforcementAction field to decide whether an update is needed.
+func (dh *dynamicHelper) ensureGatekeeperConstraint(ctx context.Context, uns *unstructured.Unstructured) error {
 	gvr, err := dh.Resolve(uns.GroupVersionKind().GroupKind().String(), uns.GroupVersionKind().Version)
 	if err != nil {
 		return err