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.

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: moving k8s-native-validation feature to beta #3476

Merged
merged 9 commits into from
Aug 9, 2024

Conversation

JaydipGabani
Copy link
Contributor

@JaydipGabani JaydipGabani commented Aug 7, 2024

What this PR does / why we need it:

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:

@JaydipGabani JaydipGabani requested a review from a team as a code owner August 7, 2024 20:57
@JaydipGabani JaydipGabani changed the title chore: moving experimental-enable-k8s-native-validation to beta and turning … chore: moving experimental-enable-k8s-native-validation to beta Aug 7, 2024
### Option 2: Install with Gatekeeper deployment
Edit the applicable deployments (`controller-manager` and `audit`), and update the following commandline flags:
- Set `--experimental-enable-k8s-native-validation=true`

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are defaulting to true I removed this doc section. However, we should add experimental-enable-k8s-native-validation is beta and is turned on by default somewhere in the docs.

@ritazh @maxsmythe @sozercan any suggestion on place on the doc for this^^?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should mention the flag is beta, default to true, set to false to opt-out

@codecov-commenter
Copy link

codecov-commenter commented Aug 7, 2024

Codecov Report

Attention: Patch coverage is 33.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 48.07%. Comparing base (3350319) to head (a533a7a).
Report is 119 commits behind head on master.

Files Patch % Lines
cmd/gator/verify/verify.go 0.00% 2 Missing ⚠️

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

HEAD has 1 upload less than BASE
Flag BASE (3350319) HEAD (a533a7a)
unittests 2 1
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3476      +/-   ##
==========================================
- Coverage   54.49%   48.07%   -6.42%     
==========================================
  Files         134      219      +85     
  Lines       12329    15165    +2836     
==========================================
+ Hits         6719     7291     +572     
- Misses       5116     7059    +1943     
- Partials      494      815     +321     
Flag Coverage Δ
unittests 48.07% <33.33%> (-6.42%) ⬇️

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.

@@ -47,8 +47,8 @@ func init() {
`print extended test output`)
Cmd.Flags().BoolVarP(&includeTrace, "trace", "t", false,
`include a trace for the underlying constraint framework evaluation`)
Cmd.Flags().BoolVarP(&flagEnableK8sCel, "experimental-enable-k8s-native-validation", "", false,
`PROTOTYPE (not stable): enable the validating admission policy driver`)
Cmd.Flags().BoolVarP(&flagEnableK8sCel, "experimental-enable-k8s-native-validation", "", true,
Copy link
Member

@sozercan sozercan Aug 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can't take any action now, but we should not call flags experimental in the name again

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we not? "experimental" sends the message "this may break/go away, do not rely on it", I see no undesireable user impact here. If anything, this means users who were using an experiment are put on notice that its behavior is changing and they should double-check that wont be a problem for them.

Are there impacts other than backwards compatibility? Because, again, backwards compatibility is explicitly not a concern for experimental/alpha things.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, given this is no longer experimental, we should change the flag name.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing or removing flags will cause the pod to crash during in place upgrade. That's not a good experience even if the feature is in alpha. A feature flag can be used in the future even after the feature is stable to allow users to opt-out. Therefore, starting a flag with "experiment" is odd if it can be used in the future after the feature is stable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point is that the flag cannot be used in the future, and that is fine. That is why we used "experimental", we should feel free to change the flag name.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One option can be to leave the old flag but render it useless. This would avoid crashing. The flag should still go away eventually.

However, I would still like to understand how/why a Helm chart cannot handle a binary changing its flags.

Copy link
Member

@ritazh ritazh Aug 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concerns about UX due to breaking changes are specifically excluded for experimental/alpha features.

This is only true if we plan to never allow users to disable a new feature that is in development. If a new feature can be disabled AFTER it is stable, then it should NOT change or have "experimental" in the flag while it is alpha/beta. Do we not want users to ever disable this feature AFTER it is stable? If so, then we just need to deprecate the flag and return warnings for few releases before removing the flag completely. But if we do plan to allow users to disable a feature, then IMO it should NOT start with "experimental".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I refer you to the K8s API definitions for alpha/beta. They have a policy for flags that say alpha flags can be removed immediately:

https://kubernetes.io/docs/reference/using-api/deprecation-policy/#deprecating-a-flag-or-cli

I don't think it's reasonable to hold ourselves to a higher standard than K8s itself.

I also return to the meaning of "experimental"/"alpha" which is:

"The developers are still working on this feature. They make no claim to have any idea as to its final behavior or how it will be configured. Breaking changes are expected"

This isn't about whether we want to continue with a feature gate or not, it's that, at the time of implementation, we were not sure whether/how the feature should work or be gated and wanted to warn the user so that they knew breaking changes are expected.

Whether we want to include "experimental" in a flag name in the future or not... I don't really care much either way. So long as users are put on notice that the API/flag is unstable, the core goal is accomplished.

As to the core question of whether its okay to rename a flag that is in alpha, the answer simply has to be yes. For any reason. We could decide a different name more fully communicates the flag's significance, the underlying behavior the flag controls could change, we could simply not like the name.

Beta is when backwards compatibility becomes a concern, and that is why we want to be careful before graduating a feature to beta, and that is why the flag name needs to change now, because we will not be able to change it in the future -- because we will be held to the requirements of being backwards compatible.

Copy link
Member

@ritazh ritazh Aug 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree if we need to rename a flag before beta it makes sense. i just want to avoid having to rename when we go to beta because we started with experimental.

I think both of our concerns can be addressed with:

  • return a warning for experimental and deprecating flags
  • try not to name flags with experimental to avoid having to change it later

If everyone agrees, I suggest renaming this flag to enable-k8s-native-validation since we are moving to beta.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me.

@@ -40,6 +40,10 @@ In summary, these are potential options when running Gatekeeper:

Find out more about different [enforcement points](enforcement-points.md)

:::note
CEL validation is in beta and `--experimental-enable-k8s-native-validation` is turned on by default.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want to say something like "CEL policy validation through Gatekeeper and VAP generation is in ..."

Copy link
Member

@sozercan sozercan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor suggestion, otherwise LGTM. Thanks!

@@ -166,7 +166,7 @@ information._
| mutatingWebhookURL | Custom URL for Kubernetes API server to use to reach the mutating webhook pod. If not set, the default of connecting via the kubernetes service endpoint is used. | `null` |
| emitAdmissionEvents | Emit K8s events in configurable namespace for admission violations (alpha feature) | `false` |
| emitAuditEvents | Emit K8s events in configurable namespace for audit violations (alpha feature) | `false` |
| enableK8sNativeValidation | Enable the K8s Native Validating driver to allow constraint templates to use rules written in VAP-style CEL (alpha feature) | `false` |
| enableK8sNativeValidation | Enable the K8s Native Validating driver to allow constraint templates to use rules written in VAP-style CEL (beta feature) | `false` |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default should be true

…it on by default

Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>
… on by default

Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>
Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>
Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>
Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>
Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>
@JaydipGabani JaydipGabani requested a review from ritazh August 8, 2024 02:12
@@ -40,6 +40,10 @@ In summary, these are potential options when running Gatekeeper:

Find out more about different [enforcement points](enforcement-points.md)

:::note
CEL validation in constraint templates through Gatekeeper is in beta and `--experimental-enable-k8s-native-validation` is turned on by default. Set --experimental-enable-k8s-native-validation=false` to disable evaluating CEL in constraint templates.
Copy link
Member

@ritazh ritazh Aug 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/open-policy-agent/gatekeeper/pull/3476/files#diff-6f51397980ac58fff52f433210aa369e522c495bdd049d01ae538eaa3b9118b1R6-R8 should be updated too for 3.17 and this text can be added there instead of here.

Feature State: Gatekeeper version v3.17 (beta)
❗ This feature is beta, subject to change (feedback is welcome!). It is enabled by default. Set --experimental-enable-k8s-native-validation=false` to disable evaluating CEL in constraint templates.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ValidatingAdmissionPolicy is different than CEL validation in Gatekeeper, wouldn't updating in the section you mentioned be missleading? It might seem like we moved VAP/VAPB generation to beta. We probably need to distinguish between generation and CEL validation. How about the below?

VAP management through Gatekeeper:
Feature State: Gatekeeper version v3.16 (alpha)
❗ This feature is alpha, subject to change (feedback is welcome!). It is enabled by default.

CEL validation in Gatekeeper:
Feature State: Gatekeeper version v3.17 (beta)
❗ This feature is beta, subject to change (feedback is welcome!). It is enabled by default. Set --experimental-enable-k8s-native-validation=false` to disable evaluating CEL in constraint templates.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CEL validation in Gatekeeper:
Feature State: Gatekeeper version v3.17 (beta)
❗ This feature is beta, subject to change (feedback is welcome!). It is enabled by default. Set --experimental-enable-k8s-native-validation=false` to disable evaluating CEL in constraint templates.

VAP management through Gatekeeper:
Feature State: Gatekeeper version v3.16 (alpha)
❗ This feature is alpha, subject to change (feedback is welcome!). It is disabled by default unless explicitly enabled via feature flag and/or via constraint template.

@JaydipGabani JaydipGabani requested a review from ritazh August 8, 2024 18:52
Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>
Copy link
Member

@ritazh ritazh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@JaydipGabani JaydipGabani requested a review from maxsmythe August 9, 2024 00:43
@ritazh ritazh changed the title chore: moving experimental-enable-k8s-native-validation to beta chore: moving enable-k8s-native-validation to beta Aug 9, 2024
@ritazh ritazh changed the title chore: moving enable-k8s-native-validation to beta chore: moving k8s-native-validation feature to beta Aug 9, 2024
…ve-validation

Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>
Copy link
Contributor

@maxsmythe maxsmythe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@ritazh ritazh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sozercan sozercan merged commit c5ee94e into open-policy-agent:master Aug 9, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants