updating vap status on delete, fixing tests

Signed-off-by: Jaydip Gabani <[email protected]>
open-policy-agent · Nov 16, 2024 · 8ef8278 · 8ef8278
1 parent ae48233
commit 8ef8278
Show file tree

Hide file tree

Showing 3 changed files with 25 additions and 17 deletions.
diff --git a/pkg/controller/constraint/constraint_controller.go b/pkg/controller/constraint/constraint_controller.go
@@ -68,6 +68,7 @@ const (
 	ErrGenerateVAPBState               = "errror"
 	GeneratedVAPBState                 = "generated"
 	WaitVAPBState                      = "waiting"
+	DeletedVAPBState                   = "deleted"
 )
 
 var (
@@ -513,7 +514,6 @@ func (r *ReconcileConstraint) manageVAPB(ctx context.Context, enforcementAction
 		return noDelay, r.reportErrorOnConstraintStatus(ctx, status, err, "could not determine if ValidatingAdmissionPolicyBinding should be generated")
 	}
 	isAPIEnabled := false
-	couldGenerateVAPB := shouldGenerateVAPB
 	var groupVersion *schema.GroupVersion
 	if shouldGenerateVAPB {
 		isAPIEnabled, groupVersion = transform.IsVapAPIEnabled(&log)
@@ -522,7 +522,7 @@ func (r *ReconcileConstraint) manageVAPB(ctx context.Context, enforcementAction
 		if !isAPIEnabled {
 			log.Error(ErrValidatingAdmissionPolicyAPIDisabled, "Cannot generate ValidatingAdmissionPolicyBinding", "constraint", instance.GetName())
 			status.Status.Errors = append(status.Status.Errors, constraintstatusv1beta1.Error{Message: fmt.Sprintf("cannot generate ValidatingAdmissionPolicyBinding: %s", ErrValidatingAdmissionPolicyAPIDisabled)})
-			couldGenerateVAPB = false
+			shouldGenerateVAPB = false
 		} else {
 			unversionedCT := &templates.ConstraintTemplate{}
 			if err := r.scheme.Convert(ct, unversionedCT, nil); err != nil {
@@ -532,15 +532,15 @@ func (r *ReconcileConstraint) manageVAPB(ctx context.Context, enforcementAction
 			switch {
 			case errors.Is(err, celSchema.ErrCELEngineMissing):
 				status.Status.EnforcementPointsStatus[vapEnforcementPointStatusIndex].Message = err.Error()
-				couldGenerateVAPB = false
+				shouldGenerateVAPB = false
 			case err != nil:
 				log.Error(err, "could not determine if ConstraintTemplate is configured to generate ValidatingAdmissionPolicy", "constraint", instance.GetName(), "constraint_template", unversionedCT.GetName())
 				status.Status.Errors = append(status.Status.Errors, constraintstatusv1beta1.Error{Message: fmt.Sprintf("could not determine if ConstraintTemplate is configured to generate ValidatingAdmissionPolicy: %s", err)})
-				couldGenerateVAPB = false
+				shouldGenerateVAPB = false
 			case !hasVAP:
 				log.Error(ErrVAPConditionsNotSatisfied, "Cannot generate ValidatingAdmissionPolicyBinding", "constraint", instance.GetName(), "constraint_template", unversionedCT.GetName())
 				status.Status.Errors = append(status.Status.Errors, constraintstatusv1beta1.Error{Message: fmt.Sprintf("cannot generate ValidatingAdmissionPolicyBinding: %s", ErrVAPConditionsNotSatisfied)})
-				couldGenerateVAPB = false
+				shouldGenerateVAPB = false
 			default:
 				// reconcile for vapb generation if annotation is not set
 				if ct.Annotations == nil || ct.Annotations[BlockVAPBGenerationUntilAnnotation] == "" {
@@ -557,16 +557,15 @@ func (r *ReconcileConstraint) manageVAPB(ctx context.Context, enforcementAction
 					wait := time.Until(t)
 					status.Status.EnforcementPointsStatus[vapEnforcementPointStatusIndex].State = WaitVAPBState
 					status.Status.EnforcementPointsStatus[vapEnforcementPointStatusIndex].Message = fmt.Sprintf("waiting for %s before generating ValidatingAdmissionPolicyBinding to make sure api-server has cached constraint CRD", wait)
-					err := r.writer.Update(ctx, status)
-					return wait, err
+					return wait, r.writer.Update(ctx, status)
 				}
 			}
 		}
 	}
 
-	r.log.Info("constraint controller", "generateVAPB", couldGenerateVAPB)
+	r.log.Info("constraint controller", "generateVAPB", shouldGenerateVAPB)
 	// generate vapbinding resources
-	if couldGenerateVAPB && groupVersion != nil {
+	if shouldGenerateVAPB && groupVersion != nil {
 		currentVapBinding, err := vapBindingForVersion(*groupVersion)
 		if err != nil {
 			return noDelay, r.reportErrorOnConstraintStatus(ctx, status, err, "could not get ValidatingAdmissionPolicyBinding API version")
@@ -609,7 +608,7 @@ func (r *ReconcileConstraint) manageVAPB(ctx context.Context, enforcementAction
 	}
 	// do not generate vapbinding resources
 	// remove if exists
-	if !couldGenerateVAPB && groupVersion != nil {
+	if !shouldGenerateVAPB && groupVersion != nil {
 		currentVapBinding, err := vapBindingForVersion(*groupVersion)
 		if err != nil {
 			return noDelay, r.reportErrorOnConstraintStatus(ctx, status, err, "could not get ValidatingAdmissionPolicyBinding API version")
@@ -627,15 +626,11 @@ func (r *ReconcileConstraint) manageVAPB(ctx context.Context, enforcementAction
 			if err := r.writer.Delete(ctx, currentVapBinding); err != nil {
 				return noDelay, r.reportErrorOnConstraintStatus(ctx, status, err, fmt.Sprintf("could not delete ValidatingAdmissionPolicyBinding: %s", vapBindingName))
 			}
+			status.Status.EnforcementPointsStatus[vapEnforcementPointStatusIndex].State = DeletedVAPBState
+			status.Status.EnforcementPointsStatus[vapEnforcementPointStatusIndex].Message = ""
 		}
 	}
-	if shouldGenerateVAPB {
-		log.Info("updating constraint status with enforcement point status", "status", status.Status)
-		if err := r.writer.Update(ctx, status); err != nil {
-			return noDelay, err
-		}
-	}
-	return noDelay, nil
+	return noDelay, r.writer.Update(ctx, status)
 }
 
 func NewConstraintsCache() *ConstraintsCache {

diff --git a/pkg/controller/constrainttemplate/constants.go b/pkg/controller/constrainttemplate/constants.go
@@ -18,4 +18,6 @@ const (
 	ErrGenerateVAPState = "error"
 	// GeneratedVAPState indicates a VAP was generated.
 	GeneratedVAPState = "generated"
+	// DeletedVAPState indicates a VAP was deleted.
+	DeletedVAPState = "deleted"
 )
diff --git a/pkg/controller/constrainttemplate/constrainttemplate_controller.go b/pkg/controller/constrainttemplate/constrainttemplate_controller.go
@@ -445,6 +445,9 @@ func (r *ReconcileConstraintTemplate) handleUpdate(
 		logger.Error(err, "generateVap error")
 		if generateVap {
 			generateVap = false
+			if status.Status.VAPGenerationStatus == nil {
+				status.Status.VAPGenerationStatus = &statusv1beta1.VAPGenerationStatus{}
+			}
 			status.Status.VAPGenerationStatus.State = ErrGenerateVAPState
 			status.Status.VAPGenerationStatus.ObservedGeneration = ct.GetGeneration()
 			status.Status.VAPGenerationStatus.Warning = fmt.Sprintf("ValidatingAdmissionPolicy is not generated: %s", err.Error())
@@ -857,6 +860,9 @@ func (r *ReconcileConstraintTemplate) manageVAP(ctx context.Context, ct *v1beta1
 				return err
 			}
 		}
+		if status.Status.VAPGenerationStatus == nil {
+			status.Status.VAPGenerationStatus = &statusv1beta1.VAPGenerationStatus{}
+		}
 		status.Status.VAPGenerationStatus.State = GeneratedVAPState
 		status.Status.VAPGenerationStatus.ObservedGeneration = ct.GetGeneration()
 		status.Status.VAPGenerationStatus.Warning = ""
@@ -884,6 +890,11 @@ func (r *ReconcileConstraintTemplate) manageVAP(ctx context.Context, ct *v1beta1
 				err := r.reportErrorOnCTStatus(ctx, ErrUpdateCode, "Could not delete VAP object", status, err)
 				return err
 			}
+			if status.Status.VAPGenerationStatus != nil {
+				status.Status.VAPGenerationStatus.State = DeletedVAPState
+				status.Status.VAPGenerationStatus.ObservedGeneration = ct.GetGeneration()
+				status.Status.VAPGenerationStatus.Warning = ""
+			}
 			// after VAP is deleted, trigger update event for all constraints
 			if err := r.triggerConstraintEvents(ctx, ct, status); err != nil {
 				return err