chore: check for CT generateVap intent before generating vapbinding #3479
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -476,28 +476,24 @@ func (r *ReconcileConstraintTemplate) handleUpdate( | |
| logger.Error(err, "error adding template to watch registry") | ||
| return reconcile.Result{}, err | ||
| } | ||
| isVapAPIEnabled := false | ||
| var groupVersion *schema.GroupVersion | ||
| if generateVap { | ||
| isVapAPIEnabled, groupVersion = constraint.IsVapAPIEnabled() | ||
| } | ||
| logger.Info("isVapAPIEnabled", "isVapAPIEnabled", isVapAPIEnabled) | ||
| logger.Info("groupVersion", "groupVersion", groupVersion) | ||
| if generateVap && (!isVapAPIEnabled || groupVersion == nil) { | ||
| logger.Error(constraint.ErrValidatingAdmissionPolicyAPIDisabled, "ValidatingAdmissionPolicy resource cannot be generated for ConstraintTemplate", "name", ct.GetName()) | ||
| err := r.reportErrorOnCTStatus(ctx, ErrCreateCode, "ValidatingAdmissionPolicy resource cannot be generated for ConstraintTemplate", status, constraint.ErrValidatingAdmissionPolicyAPIDisabled) | ||
| return reconcile.Result{}, err | ||
| } | ||
| // generating vap resources | ||
| if generateVap && isVapAPIEnabled && groupVersion != nil { | ||
| currentVap, err := vapForVersion(groupVersion) | ||
| if err != nil { | ||
| logger.Error(err, "error getting vap object with respective groupVersion") | ||
|
There was a problem hiding this comment. when you omit this, you are overriding existing status errors. please restore. There was a problem hiding this comment. we are not preserving statues errors here anyways - https://github.com/open-policy-agent/gatekeeper/blob/master/pkg/controller/constrainttemplate/constrainttemplate_controller.go#L404, this code I am removing isn't doing anything. There was a problem hiding this comment. Here is the code that we have wihtout appending to There was a problem hiding this comment. I see. ok then this has no impact on existing behavior. |
||
| err := r.reportErrorOnCTStatus(ctx, ErrCreateCode, "Could not get VAP with runtime group version", status, err) | ||
| return reconcile.Result{}, err | ||
| } | ||
| vapName := fmt.Sprintf("gatekeeper-%s", unversionedCT.GetName()) | ||
|
|
@@ -512,17 +508,13 @@ func (r *ReconcileConstraintTemplate) handleUpdate( | |
| transformedVap, err := transform.TemplateToPolicyDefinition(unversionedCT) | ||
| if err != nil { | ||
| logger.Error(err, "transform to vap error", "vapName", vapName) | ||
| err := r.reportErrorOnCTStatus(ctx, ErrCreateCode, "Could not transform to vap object", status, err) | ||
| return reconcile.Result{}, err | ||
| } | ||
|
|
||
| newVap, err := getRunTimeVAP(groupVersion, transformedVap, currentVap) | ||
| if err != nil { | ||
| logger.Error(err, "getRunTimeVAP error", "vapName", vapName) | ||
| err := r.reportErrorOnCTStatus(ctx, ErrCreateCode, "Could not get runtime vap object", status, err) | ||
| return reconcile.Result{}, err | ||
| } | ||
|
|
@@ -535,8 +527,6 @@ func (r *ReconcileConstraintTemplate) handleUpdate( | |
| logger.Info("creating vap", "vapName", vapName) | ||
| if err := r.Create(ctx, newVap); err != nil { | ||
| logger.Info("creating vap error", "vapName", vapName, "error", err) | ||
| err := r.reportErrorOnCTStatus(ctx, ErrCreateCode, "Could not create vap object", status, err) | ||
| return reconcile.Result{}, err | ||
| } | ||
|
|
@@ -547,21 +537,17 @@ func (r *ReconcileConstraintTemplate) handleUpdate( | |
| } else if !reflect.DeepEqual(currentVap, newVap) { | ||
| logger.Info("updating vap") | ||
| if err := r.Update(ctx, newVap); err != nil { | ||
| err := r.reportErrorOnCTStatus(ctx, ErrUpdateCode, "Could not update vap object", status, err) | ||
| return reconcile.Result{}, err | ||
| } | ||
| } | ||
| } | ||
| // do not generate vap resources | ||
| // remove if exists | ||
| if !generateVap && isVapAPIEnabled && groupVersion != nil { | ||
| currentVap, err := vapForVersion(groupVersion) | ||
| if err != nil { | ||
| logger.Error(err, "error getting vap object with respective groupVersion") | ||
| err := r.reportErrorOnCTStatus(ctx, ErrCreateCode, "Could not get VAP with correct group version", status, err) | ||
| return reconcile.Result{}, err | ||
| } | ||
|
|
@@ -576,8 +562,6 @@ func (r *ReconcileConstraintTemplate) handleUpdate( | |
| if currentVap != nil { | ||
| logger.Info("deleting vap") | ||
| if err := r.Delete(ctx, currentVap); err != nil { | ||
| err := r.reportErrorOnCTStatus(ctx, ErrUpdateCode, "Could not delete vap object", status, err) | ||
| return reconcile.Result{}, err | ||
| } | ||
|
|
@@ -714,8 +698,6 @@ func (r *ReconcileConstraintTemplate) triggerConstraintEvents(ctx context.Contex | |
| cstrObjs, err := r.listObjects(ctx, gvk) | ||
| if err != nil { | ||
| logger.Error(err, "get all constraints listObjects") | ||
| err := r.reportErrorOnCTStatus(ctx, ErrUpdateCode, "Could not list all constraint objects", status, err) | ||
| return err | ||
| } | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should be consistent everywhere in terms of logs and status reporting for these. if api is not enabled and other conditions for generateVap are not met, should they be considered errors or warnings? constraint and CT status reporting currently only has errors. @maxsmythe @sozercan thoughts? depending on this decision, please make all logs and status reporting consistent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's an error -- the user has signaled intent to trigger the generation pipeline but the pipeline is not generating.
If users don't care about the generation pipeline, they can interpret the severity of the error however they'd like.
If users would like to remove the error, they can change the expressed intent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sgtm. then lets log error and report error as part of CT and Constraint status.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sgtm, I am updating the PR.