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

JaydipGabani · web-flow · commit f32d56e525fe · 2025-07-01T12:40:19.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"
@@ -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,7 +501,6 @@ 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)
@@ -547,7 +547,10 @@ 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)
+		require.NoError(t, flag.CommandLine.Parse([]string{"--default-create-vap-binding-for-constraints", "false"}))
+		t.Cleanup(func() {
+			require.NoError(t, flag.CommandLine.Parse([]string{"--default-create-vap-binding-for-constraints", "true"}))
+		})
 		constraintTemplate := makeReconcileConstraintTemplateForVap(suffix, ptr.To[bool](false))
 		cstr := newDenyAllCstr(suffix)
 		t.Cleanup(testutils.DeleteObjectAndConfirm(ctx, t, c, expectedCRD(suffix)))
@@ -583,7 +586,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,9 +621,7 @@ 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)
+		constraintTemplate := makeReconcileConstraintTemplateForVap(suffix, ptr.To(false))
 		cstr := newDenyAllCstr(suffix)
 		t.Cleanup(testutils.DeleteObjectAndConfirm(ctx, t, c, expectedCRD(suffix)))
 		testutils.CreateThenCleanup(ctx, t, c, constraintTemplate)
@@ -636,7 +636,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 +645,10 @@ 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)
+		require.NoError(t, flag.CommandLine.Parse([]string{"--default-create-vap-for-templates", "false"}))
+		t.Cleanup(func() {
+			require.NoError(t, flag.CommandLine.Parse([]string{"--default-create-vap-for-templates", "true"}))
+		})
 		constraintTemplate := makeReconcileConstraintTemplate(suffix)
 		cstr := newDenyAllCstr(suffix)
 		t.Cleanup(testutils.DeleteObjectAndConfirm(ctx, t, c, expectedCRD(suffix)))
@@ -760,7 +762,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)))
@@ -841,7 +842,6 @@ 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)
@@ -899,7 +899,6 @@ 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
 		constraintTemplate := makeReconcileConstraintTemplateForVap(suffix, ptr.To[bool](true))
 		cstr := newDenyAllCstr(suffix)
@@ -1612,7 +1611,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:
