-
Notifications
You must be signed in to change notification settings - Fork 759
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.
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
fix: adding flag to validate rego for templates #3026
Conversation
Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>
Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #3026 +/- ##
==========================================
- Coverage 52.49% 52.41% -0.08%
==========================================
Files 134 134
Lines 11937 11948 +11
==========================================
- Hits 6266 6263 -3
- Misses 5179 5192 +13
- Partials 492 493 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
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.
Thanks for doing this!
Should we also update the instructions in the VAP demo readme?
IMO it’s better to fail early when both flags are used especially because the new flag is enabled by default so the GK/cluster operator can make the decision whether or not they need this new flag enabled if they have enabled the vap flag in their environment already. Getting the error when a vap policy is used seems too late and requires extra work for the policy owner to work with the GK/cluster operator to update the flags. |
Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>
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.
LGTM, thanks for doing this!
Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>
website/versioned_docs/version-v3.13.x/validating-admission-policy.md
Outdated
Show resolved
Hide resolved
Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>
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.
LGTM
Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com> Co-authored-by: Sertaç Özercan <852750+sozercan@users.noreply.github.com> (cherry picked from commit 976ae01) Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>
Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com> Co-authored-by: Sertaç Özercan <852750+sozercan@users.noreply.github.com>
What this PR does / why we need it: Introducing flag to validate rego in constraint template.
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 #3008
Special notes for your reviewer:
I am not erroring out if
experimental-enable-k8s-native-validation
andvalidate-template-rego
both are enabled at the same time as we discussed. Instead, the behavior is ifvalidate-template-rego
is enabled then any templates with cel will be rejected with the error "invalid rego", but the templates that exist in the cluster already with cel will still work (feels like less breaking change with this approach). Additionally, updates on cel templates will throw an "invalid rego" error as well. I do not have any issue with erroring out, so let me know if this is not the desired approach and I will make changes accordingly to error out if both are enabled at the same time.