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

Merged
merged 9 commits into from
Aug 9, 2024
+21 −33
Merged

chore: moving k8s-native-validation feature to beta #3476

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
3c0e5fd
moving experimental-enable-k8s-native-validation to beta and turning …
JaydipGabani Aug 7, 2024
b62e900
updating makefile to remove enableK8sNativeValidation as it is turned…
JaydipGabani Aug 7, 2024
9597954
adding note for moving CEL validation to beta in docs
JaydipGabani Aug 7, 2024
ed5ea6c
addressing doc updates
JaydipGabani Aug 8, 2024
6f0cb45
fixing rebase
JaydipGabani Aug 8, 2024
7bf8f8d
fixing rebase
JaydipGabani Aug 8, 2024
7868066
updating docs
JaydipGabani Aug 8, 2024
a533a7a
renaming experimental-enable-k8s-native-validation -> enable-k8s-nati…
JaydipGabani Aug 9, 2024
77a685b
Merge branch 'master' into cel-validation-beta
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
4 changes: 0 additions & 4 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,6 @@ MANAGER_IMAGE_PATCH := "apiVersion: apps/v1\
\n - --mutation-annotations\
\n - --default-create-vap-for-templates=${GENERATE_VAP}\
\n - --default-create-vap-binding-for-constraints=${GENERATE_VAPBINDING}\
\n - --experimental-enable-k8s-native-validation\
\n - --log-level=${LOG_LEVEL}\
\n---\
\napiVersion: apps/v1\
Expand All @@ -99,7 +98,6 @@ MANAGER_IMAGE_PATCH := "apiVersion: apps/v1\
\n - --logtostderr\
\n - --default-create-vap-for-templates=${GENERATE_VAP}\
\n - --default-create-vap-binding-for-constraints=${GENERATE_VAPBINDING}\
\n - --experimental-enable-k8s-native-validation\
\n - --log-level=${LOG_LEVEL}\
\n"

Expand Down Expand Up @@ -243,7 +241,6 @@ else
--set disabledBuiltins={http.send} \
--set logMutations=true \
--set logLevel=${LOG_LEVEL} \
--set enableK8sNativeValidation=true \
--set defaultCreateVAPForTemplates=${GENERATE_VAP} \
--set defaultCreateVAPBindingForConstraints=${GENERATE_VAPBINDING} \
--set mutationAnnotations=true;
Expand Down Expand Up @@ -285,7 +282,6 @@ e2e-helm-upgrade:
--set disabledBuiltins={http.send} \
--set logMutations=true \
--set logLevel=${LOG_LEVEL} \
--set enableK8sNativeValidation=true \
--set defaultCreateVAPForTemplates=${GENERATE_VAP} \
--set defaultCreateVAPBindingForConstraints=${GENERATE_VAPBINDING} \
--set mutationAnnotations=true;\
Expand Down
2 changes: 1 addition & 1 deletion cmd/build/helmify/static/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) | `true` |
| defaultCreateVAPForTemplates | Create VAP resource for template containing VAP-style CEL source. Allowed values are false: do not create Validating Admission Policy unless generateVAP: true is set on constraint template explicitly, true: create Validating Admission Policy unless generateVAP: false is set on constraint template explicitly. (alpha feature) | `false` |
| defaultCreateVAPBindingForConstraints | Create VAPBinding resource for constraint of the template containing VAP-style CEL source. Allowed values are false: do not create Validating Admission Policy Binding, true: create Validating Admission Policy Binding. (alpha feature) | `false` |
| auditEventsInvolvedNamespace | Emit audit events for each violation in the involved objects namespace, the default (false) generates events in the namespace Gatekeeper is installed in. Audit events from cluster-scoped resources will continue to generate events in the namespace that Gatekeeper is installed in | `false` |
Expand Down
2 changes: 1 addition & 1 deletion cmd/build/helmify/static/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ admissionEventsInvolvedNamespace: false
auditEventsInvolvedNamespace: false
resourceQuota: true
externaldataProviderResponseCacheTTL: 3m
enableK8sNativeValidation: false
enableK8sNativeValidation: true
defaultCreateVAPForTemplates: false
defaultCreateVAPBindingForConstraints: false
image:
Expand Down
2 changes: 1 addition & 1 deletion cmd/gator/test/test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ func init() {
Cmd.Flags().StringVarP(&flagOutput, flagNameOutput, "o", "", fmt.Sprintf("Output format. One of: %s|%s.", stringJSON, stringYAML))
Cmd.Flags().BoolVarP(&flagIncludeTrace, "trace", "t", false, "include a trace for the underlying Constraint Framework evaluation.")
Cmd.Flags().BoolVarP(&flagGatherStats, "stats", "", false, "include performance stats returned from the Constraint Framework.")
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, "Beta: enable the validating admission policy driver")
Cmd.Flags().StringArrayVarP(&flagImages, flagNameImage, "i", []string{}, "a URL to an OCI image containing policies. Can be specified multiple times.")
Cmd.Flags().StringVarP(&flagTempDir, flagNameTempDir, "d", "", fmt.Sprintf("Specifies the temporary directory to download and unpack images to, if using the --%s flag. Optional.", flagNameImage))
}
Expand Down
4 changes: 2 additions & 2 deletions cmd/gator/verify/verify.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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.

`Beta: enable the validating admission policy driver`)
}

// Cmd is the gator verify subcommand.
Expand Down
2 changes: 1 addition & 1 deletion main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ var (
certServiceName = flag.String("cert-service-name", "gatekeeper-webhook-service", "The service name used to generate the TLS cert's hostname. Defaults to gatekeeper-webhook-service")
enableTLSHealthcheck = flag.Bool("enable-tls-healthcheck", false, "enable probing webhook API with certificate stored in certDir")
disabledBuiltins = util.NewFlagSet()
enableK8sCel = flag.Bool("experimental-enable-k8s-native-validation", false, "Alpha: enable the validating admission policy driver")
enableK8sCel = flag.Bool("experimental-enable-k8s-native-validation", true, "Beta: enable the validating admission policy driver")
externaldataProviderResponseCacheTTL = flag.Duration("external-data-provider-response-cache-ttl", 3*time.Minute, "TTL for the external data provider response cache. Specify the duration in 'h', 'm', or 's' for hours, minutes, or seconds respectively. Defaults to 3 minutes if unspecified. Setting the TTL to 0 disables the cache.")
)

Expand Down
2 changes: 1 addition & 1 deletion manifest_staging/charts/gatekeeper/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) | `true` |
| defaultCreateVAPForTemplates | Create VAP resource for template containing VAP-style CEL source. Allowed values are false: do not create Validating Admission Policy unless generateVAP: true is set on constraint template explicitly, true: create Validating Admission Policy unless generateVAP: false is set on constraint template explicitly. (alpha feature) | `false` |
| defaultCreateVAPBindingForConstraints | Create VAPBinding resource for constraint of the template containing VAP-style CEL source. Allowed values are false: do not create Validating Admission Policy Binding, true: create Validating Admission Policy Binding. (alpha feature) | `false` |
| auditEventsInvolvedNamespace | Emit audit events for each violation in the involved objects namespace, the default (false) generates events in the namespace Gatekeeper is installed in. Audit events from cluster-scoped resources will continue to generate events in the namespace that Gatekeeper is installed in | `false` |
Expand Down
2 changes: 1 addition & 1 deletion manifest_staging/charts/gatekeeper/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ admissionEventsInvolvedNamespace: false
auditEventsInvolvedNamespace: false
resourceQuota: true
externaldataProviderResponseCacheTTL: 3m
enableK8sNativeValidation: false
enableK8sNativeValidation: true
defaultCreateVAPForTemplates: false
defaultCreateVAPBindingForConstraints: false
image:
Expand Down
17 changes: 4 additions & 13 deletions website/docs/validating-admission-policy.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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.

:::

## Pre-requisites

- Requires minimum Gatekeeper v3.16.0
Expand All @@ -54,22 +58,9 @@ Find out more about different [enforcement points](enforcement-points.md)
runtimeConfig:
admissionregistration.k8s.io/v1beta1: true
```
- Set `--experimental-enable-k8s-native-validation` in Gatekeeper deployments, or `enableK8sNativeValidation=true` if using Helm.
ritazh marked this conversation as resolved.
Show resolved Hide resolved

## Get started

### Option 1: Install with Helm
Update the `enableK8sNativeValidation` parameter in values.yaml or set during deployment
- Enable the K8s Native Validating driver to allow users to create CEL-based rules in addition to the OPA driver and rego rules (alpha feature). Default is `false`
```shell
helm install gatekeeper/gatekeeper --name-template=gatekeeper --namespace gatekeeper-system --create-namespace \
--set enableK8sNativeValidation=true
```

### 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.

Added a note above in the same file, here - https://github.com/open-policy-agent/gatekeeper/pull/3476/files#diff-6f51397980ac58fff52f433210aa369e522c495bdd049d01ae538eaa3b9118b1R43

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

## Policy updates to add CEL
To see how it works, check out this [demo](https://github.com/open-policy-agent/gatekeeper/tree/master/demo/k8s-validating-admission-policy)

Expand Down
Loading