Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: check for CT generateVap intent before generating vapbinding #3479

Merged
merged 15 commits into from
Aug 9, 2024
Merged

chore: check for CT generateVap intent before generating vapbinding #3479

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
adef05d
checking for possibility of VAP existing in cluster before generating…
JaydipGabani Aug 8, 2024
62ac785
fixing unit tests
JaydipGabani Aug 8, 2024
d4148f5
fixing tests
JaydipGabani Aug 8, 2024
6868702
adding rego in test
JaydipGabani Aug 8, 2024
b0f26cb
adding rego engine in tests
JaydipGabani Aug 8, 2024
70478ee
adding test for rego only engine
JaydipGabani Aug 8, 2024
af0988f
updating warning when template will not generate VAP
JaydipGabani Aug 8, 2024
34eb883
updating logs and test names
JaydipGabani Aug 8, 2024
ebb02b5
updating logs and test names
JaydipGabani Aug 8, 2024
2327de8
fixing test name
JaydipGabani Aug 8, 2024
589312a
fixing test name
JaydipGabani Aug 8, 2024
59cebbf
adding warning in CT when VAP API is not available
JaydipGabani Aug 9, 2024
a1ab00b
adding warning for CT when VAP API is not enabled and generation is on
JaydipGabani Aug 9, 2024
24dd83d
logging errors and reporting the same on CT and C, when generation in…
JaydipGabani Aug 9, 2024
d064c85
Merge branch 'master' into generatevap-check
sozercan Aug 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions pkg/controller/constraint/constraint_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,8 +323,12 @@ func (r *ReconcileConstraint) Reconcile(ctx context.Context, request reconcile.R
}
if generateVAPB {
if !isAPIEnabled {
r.log.V(1).Info("Warning: VAP API is not enabled, cannot create VAPBinding")
r.log.V(1).Info("Warning: ValidatingAdmissionPolicy API is not enabled, cannot create ValidatingAdmissionPolicyBinding")
generateVAPB = false
status.Status.Errors = append(status.Status.Errors, constraintstatusv1beta1.Error{Message: "Warning: ValidatingAdmissionPolicy API is not enabled, cannot create ValidatingAdmissionPolicyBinding"})
if err2 := r.writer.Update(ctx, status); err2 != nil {
log.Error(err2, "could not update constraint status error when VAP API is not enabled")
}
} else {
unversionedCT := &templates.ConstraintTemplate{}
if err := r.scheme.Convert(ct, unversionedCT, nil); err != nil {
Expand All @@ -345,6 +349,10 @@ func (r *ReconcileConstraint) Reconcile(ctx context.Context, request reconcile.R
if !hasVAP {
r.log.V(1).Info("Warning: Conditions are not satisfied to generate ValidatingAdmissionPolicy and ValidatingAdmissionPolicyBinding")
generateVAPB = false
status.Status.Errors = append(status.Status.Errors, constraintstatusv1beta1.Error{Message: "Warning: Conditions are not satisfied to generate ValidatingAdmissionPolicy and ValidatingAdmissionPolicyBinding"})
if err2 := r.writer.Update(ctx, status); err2 != nil {
log.Error(err2, "could not update constraint status error when conditions are not satisfied to generate VAP")
}
}
}
}
Expand Down Expand Up @@ -704,7 +712,7 @@ func IsVapAPIEnabled() (bool, *schema.GroupVersion) {
}
}

log.Error(err, "error checking VAP api availability", "IsVapAPIEnabled", "false")
log.Error(err, "error checking VAP API availability", "IsVapAPIEnabled", "false")
VapAPIEnabled = new(bool)
*VapAPIEnabled = false
return false, nil
Expand Down
6 changes: 5 additions & 1 deletion pkg/controller/constrainttemplate/constrainttemplate_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -484,7 +484,11 @@ func (r *ReconcileConstraintTemplate) handleUpdate(
logger.Info("isVAPapiEnabled", "isVAPapiEnabled", isVAPapiEnabled)
logger.Info("groupVersion", "groupVersion", groupVersion)
if generateVap && (!isVAPapiEnabled || groupVersion == nil) {
logger.V(1).Info("Warning: VAP API is not enabled, VAP will not be generated for constraint template", "name", ct.GetName())
logger.V(1).Info("Warning: ValidatingAdmissionPolicy API is not enabled, ValidatingAdmissionPolicy resource cannot be generated for ConstraintTemplate", "name", ct.GetName())
createErr := &v1beta1.CreateCRDError{Code: ErrCreateCode, Message: "ValidatingAdmissionPolicy API is not enabled, ValidatingAdmissionPolicy resource cannot be generated for ConstraintTemplate"}
status.Status.Errors = append(status.Status.Errors, createErr)
err := r.reportErrorOnCTStatus(ctx, ErrCreateCode, "Warning: ValidatingAdmissionPolicy resource cannot be generated for ConstraintTemplate", status, errors.New("ValidatingAdmissionPolicy API is not enabled"))
return reconcile.Result{}, err
Copy link
Member

@ritazh ritazh Aug 9, 2024 •
edited
Loading

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

Copy link
Contributor

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

}
// generating vap resources
if generateVap && isVAPapiEnabled && groupVersion != nil {
Expand Down
Loading