fix: making sure VAPB is only deleted for constraints if it was enabled (#4034) (#4039)

JaydipGabani · web-flow · commit 9fc1738bbf1a · 2025-07-14T11:06:40.000-07:00
Signed-off-by: Jaydip Gabani &lt;gabanijaydip@gmail.com&gt;
diff --git a/pkg/controller/constraint/constraint_controller.go b/pkg/controller/constraint/constraint_controller.go
@@ -352,16 +352,43 @@ func (r *ReconcileConstraint) Reconcile(ctx context.Context, request reconcile.R
 		}
 		isAPIEnabled, groupVersion := transform.IsVapAPIEnabled(&log)
 		if isAPIEnabled {
-			currentVapBinding, err := vapBindingForVersion(*groupVersion)
+			shouldGenerateVAPB, _, err := shouldGenerateVAPB(*DefaultGenerateVAPB, enforcementAction, instance)
 			if err != nil {
 				return reconcile.Result{}, err
 			}
-			vapBindingName := getVAPBindingName(instance.GetName())
-			currentVapBinding.SetName(vapBindingName)
-			if err := r.writer.Delete(ctx, currentVapBinding); err != nil {
-				if !apierrors.IsNotFound(err) {
+			if shouldGenerateVAPB {
+				ct := &v1beta1.ConstraintTemplate{}
+				err = r.reader.Get(ctx, types.NamespacedName{Name: strings.ToLower(instance.GetKind())}, ct)
+				if err != nil {
+					if apierrors.IsNotFound(err) {
+						return reconcile.Result{}, nil
+					}
+					return reconcile.Result{}, err
+				}
+				unversionedCT := &templates.ConstraintTemplate{}
+				if err := r.scheme.Convert(ct, unversionedCT, nil); err != nil {
 					return reconcile.Result{}, err
 				}
+				hasVAP, err := ShouldGenerateVAP(unversionedCT)
+				if err != nil {
+					if errors.Is(err, celSchema.ErrCELEngineMissing) {
+						return reconcile.Result{}, nil
+					}
+					return reconcile.Result{}, err
+				}
+				if hasVAP {
+					vapBindingName := getVAPBindingName(instance.GetName())
+					currentVapBinding, err := vapBindingForVersion(*groupVersion)
+					if err != nil {
+						return reconcile.Result{}, err
+					}
+					currentVapBinding.SetName(vapBindingName)
+					if err := r.writer.Delete(ctx, currentVapBinding); err != nil {
+						if !apierrors.IsNotFound(err) {
+							return reconcile.Result{}, err
+						}
+					}
+				}
 			}
 		}
 	}
diff --git a/pkg/controller/constrainttemplate/constrainttemplate_controller_test.go b/pkg/controller/constrainttemplate/constrainttemplate_controller_test.go
@@ -18,6 +18,7 @@ package constrainttemplate
 import (
 	"encoding/json"
 	"errors"
+	"flag"
 	"fmt"
 	"strings"
 	"testing"
@@ -40,6 +41,7 @@ import (
 	"github.com/open-policy-agent/gatekeeper/v3/pkg/util"
 	testclient "github.com/open-policy-agent/gatekeeper/v3/test/clients"
 	"github.com/open-policy-agent/gatekeeper/v3/test/testutils"
+	"github.com/stretchr/testify/require"
 	"golang.org/x/net/context"
 	admissionv1 "k8s.io/api/admission/v1"
 	admissionregistrationv1 "k8s.io/api/admissionregistration/v1"
@@ -271,7 +273,7 @@ func TestReconcile(t *testing.T) {
 	testutils.StartManager(ctx, t, mgr)
 
 	transform.VapAPIEnabled = ptr.To[bool](true)
-	transform.GroupVersion = &admissionregistrationv1beta1.SchemeGroupVersion
+	transform.SetGroupVersionForVAP(&admissionregistrationv1beta1.SchemeGroupVersion)
 
 	t.Run("CRD Gets Created", func(t *testing.T) {
 		suffix := "CRDGetsCreated"
@@ -377,7 +379,6 @@ func TestReconcile(t *testing.T) {
 		suffix := "VapShouldNotBeCreatedForRegoOnlyTemplate"
 
 		logger.Info("Running test: Vap should not be created for rego only template")
-		constraint.DefaultGenerateVAP = ptr.To[bool](true)
 		constraintTemplate := makeReconcileConstraintTemplate(suffix)
 		t.Cleanup(testutils.DeleteObjectAndConfirm(ctx, t, c, expectedCRD(suffix)))
 		testutils.CreateThenCleanup(ctx, t, c, constraintTemplate)
@@ -500,11 +501,18 @@ func TestReconcile(t *testing.T) {
 	t.Run("Vap should be created without generateVAP field", func(t *testing.T) {
 		suffix := "VapShouldBeCreatedWithoutGenerateVAP"
 		logger.Info("Running test: Vap should be created without generateVAP field")
-		constraint.DefaultGenerateVAP = ptr.To[bool](true)
 		constraintTemplate := makeReconcileConstraintTemplateForVap(suffix, nil)
 		t.Cleanup(testutils.DeleteObjectAndConfirm(ctx, t, c, expectedCRD(suffix)))
 		testutils.CreateThenCleanup(ctx, t, c, constraintTemplate)
-
+		testFlagSet := flag.NewFlagSet("test", flag.ContinueOnError)
+		testFlag := testFlagSet.Bool("default-create-vap-for-templates", true, "test flag")
+		require.NoError(t, testFlagSet.Parse([]string{"--default-create-vap-for-templates", "true"}))
+		originalValue := *constraint.DefaultGenerateVAP
+		*constraint.DefaultGenerateVAP = *testFlag
+
+		t.Cleanup(func() {
+			*constraint.DefaultGenerateVAP = originalValue
+		})
 		err = retry.OnError(testutils.ConstantRetry, func(_ error) bool {
 			return true
 		}, func() error {
@@ -547,7 +555,6 @@ func TestReconcile(t *testing.T) {
 	t.Run("VapBinding should not be created", func(t *testing.T) {
 		suffix := "VapBindingShouldNotBeCreated"
 		logger.Info("Running test: VapBinding should not be created")
-		constraint.DefaultGenerateVAPB = ptr.To[bool](false)
 		constraintTemplate := makeReconcileConstraintTemplateForVap(suffix, ptr.To[bool](false))
 		cstr := newDenyAllCstr(suffix)
 		t.Cleanup(testutils.DeleteObjectAndConfirm(ctx, t, c, expectedCRD(suffix)))
@@ -583,7 +590,6 @@ func TestReconcile(t *testing.T) {
 	t.Run("VapBinding should not be created with missing CEL", func(t *testing.T) {
 		suffix := "VapBindingShouldNotBeCreatedMissingCEL"
 		logger.Info("Running test: VapBinding should not be created with missing CEL")
-		constraint.DefaultGenerateVAPB = ptr.To[bool](true)
 		constraintTemplate := makeReconcileConstraintTemplate(suffix)
 		cstr := newDenyAllCstr(suffix)
 		t.Cleanup(testutils.DeleteObjectAndConfirm(ctx, t, c, expectedCRD(suffix)))
@@ -619,10 +625,8 @@ func TestReconcile(t *testing.T) {
 	t.Run("Error should be present on constraint when VAP generation is off and VAPB generation is on for CEL templates", func(t *testing.T) {
 		suffix := "ErrorShouldBePresentOnConstraint"
 		logger.Info("Running test: Error should be present on constraint when VAP generation is off and VAPB generation is on")
-		constraint.DefaultGenerateVAP = ptr.To[bool](false)
-		constraint.DefaultGenerateVAPB = ptr.To[bool](true)
-		constraintTemplate := makeReconcileConstraintTemplateForVap(suffix, nil)
-		cstr := newDenyAllCstr(suffix)
+		constraintTemplate := makeReconcileConstraintTemplateForVap(suffix, ptr.To(false))
+		cstr := newDenyAllCstrWithScopedEA(suffix, util.VAPEnforcementPoint)
 		t.Cleanup(testutils.DeleteObjectAndConfirm(ctx, t, c, expectedCRD(suffix)))
 		testutils.CreateThenCleanup(ctx, t, c, constraintTemplate)
 
@@ -636,7 +640,7 @@ func TestReconcile(t *testing.T) {
 			t.Fatal(err)
 		}
 
-		err := isConstraintStatuErrorAsExpected(ctx, c, suffix, true, "Conditions are not satisfied")
+		err := isConstraintStatuErrorAsExpected(ctx, c, suffix, true, constraint.ErrVAPConditionsNotSatisfied.Error())
 		if err != nil {
 			t.Fatal(err)
 		}
@@ -645,8 +649,6 @@ func TestReconcile(t *testing.T) {
 	t.Run("Error should not be present on constraint when VAP generation if off and VAPB generation is on for templates without CEL", func(t *testing.T) {
 		suffix := "ErrorShouldNotBePresentOnConstraint"
 		logger.Info("Running test: Error should not be present on constraint when VAP generation is off and VAPB generation is on for templates wihout CEL")
-		constraint.DefaultGenerateVAP = ptr.To[bool](false)
-		constraint.DefaultGenerateVAPB = ptr.To[bool](true)
 		constraintTemplate := makeReconcileConstraintTemplate(suffix)
 		cstr := newDenyAllCstr(suffix)
 		t.Cleanup(testutils.DeleteObjectAndConfirm(ctx, t, c, expectedCRD(suffix)))
@@ -669,9 +671,8 @@ func TestReconcile(t *testing.T) {
 	t.Run("VapBinding should not be created without generateVap intent in CT", func(t *testing.T) {
 		suffix := "VapBindingShouldNotBeCreatedWithoutGenerateVapIntent"
 		logger.Info("Running test: VapBinding should not be created without generateVap intent in CT")
-		constraint.DefaultGenerateVAPB = ptr.To[bool](true)
 		constraintTemplate := makeReconcileConstraintTemplateForVap(suffix, ptr.To[bool](false))
-		cstr := newDenyAllCstr(suffix)
+		cstr := newDenyAllCstrWithScopedEA(suffix, util.VAPEnforcementPoint)
 		t.Cleanup(testutils.DeleteObjectAndConfirm(ctx, t, c, expectedCRD(suffix)))
 		testutils.CreateThenCleanup(ctx, t, c, constraintTemplate)
 
@@ -760,7 +761,6 @@ func TestReconcile(t *testing.T) {
 	t.Run("VapBinding should not be created without VAP enforcement Point", func(t *testing.T) {
 		suffix := "VapBShouldNotBeCreatedWithoutVAPEP"
 		logger.Info("Running test: VapBinding should not be created without VAP enforcement point")
-		constraint.DefaultGenerateVAPB = ptr.To[bool](true)
 		constraintTemplate := makeReconcileConstraintTemplateForVap(suffix, ptr.To[bool](true))
 		cstr := newDenyAllCstrWithScopedEA(suffix, util.AuditEnforcementPoint)
 		t.Cleanup(testutils.DeleteObjectAndConfirm(ctx, t, c, expectedCRD(suffix)))
@@ -797,7 +797,7 @@ func TestReconcile(t *testing.T) {
 		suffix := "VapShouldBeCreatedV1"
 
 		logger.Info("Running test: Vap should be created with v1")
-		transform.GroupVersion = &admissionregistrationv1.SchemeGroupVersion
+		transform.SetGroupVersionForVAP(&admissionregistrationv1.SchemeGroupVersion)
 		constraintTemplate := makeReconcileConstraintTemplateForVap(suffix, ptr.To[bool](true))
 		t.Cleanup(testutils.DeleteObjectAndConfirm(ctx, t, c, expectedCRD(suffix)))
 		testutils.CreateThenCleanup(ctx, t, c, constraintTemplate)
@@ -841,10 +841,9 @@ func TestReconcile(t *testing.T) {
 	t.Run("VapBinding should be created with v1 with some delay after constraint CRD is available", func(t *testing.T) {
 		suffix := "VapBindingShouldBeCreatedV1"
 		logger.Info("Running test: VapBinding should be created with v1 with some delay after constraint CRD is available")
-		constraint.DefaultGenerateVAPB = ptr.To[bool](true)
-		transform.GroupVersion = &admissionregistrationv1.SchemeGroupVersion
-		constraintTemplate := makeReconcileConstraintTemplateForVap(suffix, ptr.To[bool](true))
-		cstr := newDenyAllCstr(suffix)
+		transform.SetGroupVersionForVAP(&admissionregistrationv1.SchemeGroupVersion)
+		constraintTemplate := makeReconcileConstraintTemplateForVap(suffix, ptr.To(true))
+		cstr := newDenyAllCstrWithScopedEA(suffix, util.VAPEnforcementPoint)
 		t.Cleanup(testutils.DeleteObjectAndConfirm(ctx, t, c, expectedCRD(suffix)))
 		testutils.CreateThenCleanup(ctx, t, c, constraintTemplate)
 
@@ -899,8 +898,7 @@ func TestReconcile(t *testing.T) {
 	t.Run("VapBinding should be created with v1 without warnings in enforcementPointsStatus", func(t *testing.T) {
 		suffix := "VapBindingShouldBeCreatedV1EnforcementPointsStatus"
 		logger.Info("Running test: VapBinding should be created with v1 without warnings in enforcementPointsStatus")
-		constraint.DefaultGenerateVAPB = ptr.To[bool](true)
-		transform.GroupVersion = &admissionregistrationv1.SchemeGroupVersion
+		transform.SetGroupVersionForVAP(&admissionregistrationv1.SchemeGroupVersion)
 		constraintTemplate := makeReconcileConstraintTemplateForVap(suffix, ptr.To[bool](true))
 		cstr := newDenyAllCstr(suffix)
 		t.Cleanup(testutils.DeleteObjectAndConfirm(ctx, t, c, expectedCRD(suffix)))
@@ -1612,7 +1610,7 @@ func isConstraintStatuErrorAsExpected(ctx context.Context, c client.Client, suff
 
 		switch {
 		case wantErr && len(status.Errors) == 0:
-			return fmt.Errorf("expected error not found in status")
+			return fmt.Errorf("expected error not found in status, no errors present")
 		case !wantErr && len(status.Errors) > 0:
 			return fmt.Errorf("unexpected error found in status")
 		case wantErr:
diff --git a/pkg/drivers/k8scel/transform/vap_util.go b/pkg/drivers/k8scel/transform/vap_util.go
@@ -77,3 +77,9 @@ func IsVapAPIEnabled(log *logr.Logger) (bool, *schema.GroupVersion) {
 	*VapAPIEnabled = false
 	return false, nil
 }
+
+func SetGroupVersionForVAP(gv *schema.GroupVersion) {
+	vapMux.Lock()
+	defer vapMux.Unlock()
+	GroupVersion = gv
+}
