chore: adding generateVAP field, removing annotations for vap

[JaydipGabani]

What this PR does / why we need it:
Removing use-vap annotation in favor of using generateVAP field on template to generate VAP and VAPB.

Which issue(s) this PR fixes (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged):
Fixes #

Special notes for your reviewer:

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 55.55556% with 12 lines in your changes missing coverage. Please review.

Project coverage is 48.02%. Comparing base (3350319) to head (e19c2de).
Report is 168 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/controller/constraint/constraint_controller.go	46.66%	8 Missing ⚠️
...onstrainttemplate/constrainttemplate_controller.go	66.66%	2 Missing and 2 partials ⚠️

❗ There is a different number of reports uploaded between BASE (3350319) and HEAD (e19c2de). Click for more details.

HEAD has 1 upload less than BASE

Flag	BASE (3350319)	HEAD (e19c2de)
unittests	2	1

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3398      +/-   ##
==========================================
- Coverage   54.49%   48.02%   -6.48%     
==========================================
  Files         134      219      +85     
  Lines       12329    14844    +2515     
==========================================
+ Hits         6719     7129     +410     
- Misses       5116     6907    +1791     
- Partials      494      808     +314     

Flag	Coverage Δ
unittests	48.02% <55.55%> (-6.48%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[sozercan]

a few minor comments, otherwise LGTM

[ritazh]

LGTM

[maxsmythe]

Nits other than the question about checking the template before generating a binding for a constraint.

[maxsmythe]

I thought we were going to get rid of the condition of only generating the binding if the constraint template had VAP CEL?

Link to relevant chat thread: https://openpolicyagent.slack.com/archives/GQK16NZEJ/p1719537610076419?thread_ts=1719532061.611639&cid=GQK16NZEJ

The thread mentions potentially adding a warning if we are creating a binding for a template that does not have a VAP implementation.

IIRC we are able to do this b/c users should be able to use scopedEnforcementActions if they need to modify behavior more granularly.

[JaydipGabani]

This condition was added so that we do not generate VAPB for constraints that have templates only with Rego code. Which makes sense, these will be lingering VAPB unless ommited through scopedEA. We can still add the warning saying this tamplate does not have VAPB created because of missing VAP implementation after we determine the shcema of the warning.

[maxsmythe]

I am skeptical. I suspect we are trying to be too clever here and that we will shoot ourselves in the foot. Particularly if users want to leverage fail-closed. I suppose the behavior could be customized further down the line with flags.

What happens when users specify "only use VAP" using scopedEA for a rego-only template? We continue to fail to create the binding and add a warning?

Looks like we are already triggering re-reconciles of all constraints when templates get updated, which is good:

gatekeeper/pkg/controller/constrainttemplate/constrainttemplate_controller.go

Lines 699 to 717 in 0f20484

	func (r *ReconcileConstraintTemplate) triggerConstraintEvents(ctx context.Context, ct *v1beta1.ConstraintTemplate, status *statusv1beta1.ConstraintTemplatePodStatus) error {
	gvk := makeGvk(ct.Spec.CRD.Spec.Names.Kind)
	logger.Info("list gvk objects", "gvk", gvk)
	cstrObjs, err := r.listObjects(ctx, gvk)
	if err != nil {
	logger.Error(err, "get all constraints listObjects")
	updateErr := &v1beta1.CreateCRDError{Code: ErrUpdateCode, Message: err.Error()}
	status.Status.Errors = append(status.Status.Errors, updateErr)
	err := r.reportErrorOnCTStatus(ctx, ErrUpdateCode, "Could not list all constraint objects", status, err)
	return err
	}
	logger.Info("list gvk objects", "cstrObjs", cstrObjs)
	for _, cstr := range cstrObjs {
	c := cstr
	logger.Info("triggering cstrEvent")
	r.cstrEvents <- event.GenericEvent{Object: &c}
	}
	return nil

If none of these concerns are blocking, SGTM

[JaydipGabani]

Particularly if users want to leverage fail-closed.

@maxsmythe what do you mean by this? can you elaborate?

What happens when users specify "only use VAP" using scopedEA for a rego-only template? We continue to fail to create the binding and add a warning?

I think yes 🤔. There wouldn't be any enforcement through in-tree VAP anyways in this situation given VAP won't be there even if we end up creating VAPB otherwise. We should follow up with VAP warning and error message schema for constraint status after this PR. In this situation GK would enforce rego with constraint.

I really do not have strong objection against creating VAPB for all constraints though.

@ritazh @sozercan do you have any thoughts on this?

[ritazh]

Particularly if users want to leverage fail-closed.

Are you referring to when user wants to use vap and vapb to enforce but the CT only has rego, GK should fail instead of warn and let GK webhook handle it? If so, then I agree if user specified scopedEA in the constraint to use VAP but CT doesnt have CEL, then we should report error in the constraint status. However, if there is no constraint level scopedEA provided, we should not generate vapb for all constraints that do not have CEL in the CT.

Creating VAPB when there is no corresponding VAP due to lack of CEL seems unnecessary. not sure what value there is by having a dangling VAPB without a corresponding VAP.

[maxsmythe]

this function should probably live next to the VAP driver once we move it from frameworks into G8r.

[maxsmythe]

LGTM

[nreisch]

Qq, is there a way to override on the constraint level still or only constraint template @JaydipGabani

[JaydipGabani]

--default-create-vapb-for-constraints if the flag for enabling generation of VAPB for constraint. However, to override that flag we are introducing VAP as an enforcement Point and a field in constraint called scopedEnforcementActions to override --default-create-vapb-for-constraints. Here is complete opt-in/opt-out of VAP and VAPB generation behavior - VAP-user-flow. Let me know if you have any more questions.

[JaydipGabani]

Looking into CI failures.