-
Notifications
You must be signed in to change notification settings - Fork 1.6k
✨ Add feature gates support for experimental API fields #4973
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
base: master
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: wazery The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Hi @wazery. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
f03050d
to
00e9604
Compare
/ok-to-test |
@@ -119,6 +119,8 @@ help: ## Display this help. | |||
.PHONY: manifests | |||
manifests: controller-gen ## Generate WebhookConfiguration, ClusterRole and CustomResourceDefinition objects. | |||
$(CONTROLLER_GEN) rbac:roleName=manager-role crd webhook paths="./..." output:crd:artifacts:config=config/crd/bases | |||
# TODO: When controller-tools supports feature gates (issue #1238), add: | |||
# $(CONTROLLER_GEN) --feature-gates=$(FEATURE_GATES) rbac:roleName=manager-role crd webhook paths="./..." output:crd:artifacts:config=config/crd/bases |
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 will need to think about how we test it out with the feature gates.
We probably need a e2e tests that run with manifests for alpha/beta, etc.
That would be called in the e2e tests, for example, to deploy the solution and would patch all experimental features. WDYT does it make sense?
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.
Pull Request Overview
This PR implements feature gates support for experimental API fields in Kubebuilder controllers. The implementation enables developers to gradually rollout experimental features while maintaining backward compatibility, similar to Kubernetes core feature gates.
- Adds
+feature-gate
marker support for API fields with automatic discovery - Implements feature gate parsing, validation, and runtime control via
--feature-gates
flag - Integrates feature gates into project scaffolding and generation workflow
Reviewed Changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
testdata/project-v4/api/v1/captain_types.go |
Adds example Bar field with experimental-bar feature gate marker |
test/e2e/v4/featuregates_test.go |
E2E tests for feature gate scaffolding and project building |
test/e2e/v4/featuregates_discovery_test.go |
Tests for feature gate discovery and parsing functionality |
pkg/plugins/golang/v4/scaffolds/internal/templates/makefile.go |
Adds TODO comment for future controller-gen feature gate support |
pkg/plugins/golang/v4/scaffolds/internal/templates/cmd/main.go |
Integrates feature gates flag and validation into main controller entry point |
pkg/plugins/golang/v4/scaffolds/internal/templates/cmd/featuregates.go |
Template for generating feature gates package with parsing and validation |
pkg/plugins/golang/v4/scaffolds/internal/templates/api/types.go |
Adds commented example of feature-gated field in generated API types |
pkg/plugins/golang/v4/scaffolds/init.go |
Includes feature gates scaffolding in initialization process |
pkg/plugins/golang/v4/scaffolds/api_test.go |
Unit tests for API scaffolder including feature gate discovery |
pkg/plugins/golang/v4/scaffolds/api.go |
Implements feature gate discovery and integration with scaffolding |
pkg/machinery/featuregate_test.go |
Unit tests for feature gate parsing and validation logic |
pkg/machinery/featuregate_marker_test.go |
Tests for feature gate marker parsing from Go source files |
pkg/machinery/featuregate_marker.go |
Parser for discovering +feature-gate markers in Go source code |
pkg/machinery/featuregate.go |
Core feature gate types and parsing functionality |
go.mod |
Adds testify dependency for improved testing |
docs/book/src/reference/markers/feature-gates.md |
Comprehensive documentation for feature gates usage |
docs/book/src/reference/markers.md |
Updates markers reference to include feature gates |
docs/book/src/SUMMARY.md |
Adds feature gates to documentation table of contents |
case "true", "1", "yes", "on": | ||
enabled = true | ||
case "false", "0", "no", "off": | ||
enabled = false | ||
default: | ||
return nil, fmt.Errorf("invalid feature gate value for %s: %s", name, value) |
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.
[nitpick] The parsing logic accepts 'yes', 'on', 'no', 'off' values but this is inconsistent with Kubernetes feature gate parsing which typically only accepts 'true' and 'false'. This could lead to confusion for users familiar with Kubernetes patterns.
case "true", "1", "yes", "on": | |
enabled = true | |
case "false", "0", "no", "off": | |
enabled = false | |
default: | |
return nil, fmt.Errorf("invalid feature gate value for %s: %s", name, value) | |
case "true": | |
enabled = true | |
case "false": | |
enabled = false | |
default: | |
return nil, fmt.Errorf("invalid feature gate value for %s: %s (must be 'true' or 'false')", name, value) |
Copilot uses AI. Check for mistakes.
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.
@camilamacedo86 what do you think about this, should we use only 'true' or 'false' as suggested?
265a76d
to
f906d39
Compare
/retest |
/retest |
/retest |
1 similar comment
/retest |
/retest |
/retest |
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
// Bar is an experimental field that requires the "experimental-bar" feature gate to be enabled | ||
// +feature-gate experimental-bar | ||
// +optional | ||
Bar *string `json:"bar,omitempty"` |
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.
All samples are auto-generated.
Whenever we make changes, we run make install and make generate, which calls this script to regenerate the samples:
https://github.com/kubernetes-sigs/kubebuilder/blob/master/test/testdata/generate.sh
Because of this, we can’t edit the samples directly. If the samples don’t match, the testdata CI check will fail.
For e2e tests, we create mock scenarios within the tests themselves. For example:
https://github.com/kubernetes-sigs/kubebuilder/blob/master/test/e2e/alphagenerate/generate_test.go#L36-L62
ctx.Group = "crew" | ||
ctx.Version = "v1" | ||
ctx.Kind = "Captain" | ||
ctx.Resources = "captains" |
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 cannot change the testsdata and try to use it here.
Here we need to init the project like:
kubebuilder/test/e2e/alphagenerate/generate_test.go
Lines 55 to 59 in 4155789
By("initializing a project") | |
err = kbc.Init( | |
"--plugins", "go/v4", | |
"--project-version", "3", | |
"--domain", kbc.Domain, |
Then, create the API like:
kubebuilder/test/e2e/alphagenerate/generate_test.go
Lines 104 to 114 in 4155789
By("creating API definition") | |
err = kbc.CreateAPI( | |
"--group", kbc.Group, | |
"--version", kbc.Version, | |
"--kind", kbc.Kind, | |
"--namespaced", | |
"--resource", | |
"--controller", | |
"--make=false", | |
) | |
Expect(err).NotTo(HaveOccurred(), "Failed to scaffold api with resource and controller") |
And to add the new spec we can use the utils : https://github.com/kubernetes-sigs/kubebuilder/blob/master/pkg/plugin/util/util.go
You will see that we have helpers to insert, replace and uncomment code.
It would be like we do for example here:
kubebuilder/test/e2e/utils/webhooks.go
Lines 44 to 75 in 4155789
// implement defaulting webhook logic | |
replace := fmt.Sprintf(`if %s.Spec.Count == 0 { | |
%s.Spec.Count = 5 | |
}`, lowerKind, lowerKind) | |
str, err = util.EnsureExistAndReplace( | |
str, | |
"// TODO(user): fill in your defaulting logic.", | |
replace, | |
) | |
if err != nil { | |
return fmt.Errorf("error replacing default logic in webhooks file %q: %w", filename, err) | |
} | |
// implement validation webhook logic | |
str, err = util.EnsureExistAndReplace( | |
str, | |
"// TODO(user): fill in your validation logic upon object creation.", | |
fmt.Sprintf(`if %s.Spec.Count < 0 { | |
return nil, errors.New(".spec.count must >= 0") | |
}`, lowerKind)) | |
if err != nil { | |
return fmt.Errorf("error replacing validation logic in webhooks file %q: %w", filename, err) | |
} | |
str, err = util.EnsureExistAndReplace( | |
str, | |
"// TODO(user): fill in your validation logic upon object update.", | |
fmt.Sprintf(`if %s.Spec.Count < 0 { | |
return nil, errors.New(".spec.count must >= 0") | |
}`, lowerKind)) | |
if err != nil { | |
return fmt.Errorf("error replacing validation logic in webhooks file %q: %w", filename, err) | |
} |
OR in the hack and many other palces like
kubebuilder/hack/docs/internal/getting-started/generate_getting_started.go
Lines 153 to 161 in 4155789
err = pluginutil.InsertCode( | |
filepath.Join(sp.ctx.Dir, pathFile), | |
"// +kubebuilder:rbac:groups=cache.example.com,resources=memcacheds/finalizers,verbs=update", | |
` | |
// +kubebuilder:rbac:groups=core,resources=events,verbs=create;patch | |
// +kubebuilder:rbac:groups=apps,resources=deployments,verbs=get;list;watch;create;update;patch;delete | |
// +kubebuilder:rbac:groups=core,resources=pods,verbs=get;list;watch`, | |
) | |
hackutils.CheckError("add markers", err) |
updatedContent := strings.Replace(string(typesContent), | ||
"Bar *string `json:\"bar,omitempty\"`", | ||
"Bar *string `json:\"bar,omitempty\"`\n"+newField, | ||
1) | ||
|
||
err = os.WriteFile(typesFile, []byte(updatedContent), 0644) | ||
Expect(err).NotTo(HaveOccurred()) |
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 have an utility for it: https://github.com/kubernetes-sigs/kubebuilder/blob/master/pkg/plugin/util/util.go#L235
@@ -0,0 +1,233 @@ | |||
/* |
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.
Note that these tests are not currently run in any CI.
Since they don’t interact with a cluster, they could be added to:
https://github.com/kubernetes-sigs/kubebuilder/blob/master/test/features.sh
However, if we want to actually validate that solutions with a feature gate are deployable—and not just test the scaffold like init
—then we need a real e2e test.
In that case, we should set it here:
https://github.com/kubernetes-sigs/kubebuilder/blob/master/test/e2e/setup.sh#L67-L68
so it runs using a kind cluster in Prow.
I’ll need more time to review this approach in detail, but here’s how I think it should work:
- Create the project.
- Create the controller.
- Add a feature gate that outputs something like "experimental feature gate" in the logs.
- Build and deploy, patching to enable the feature gate.
- Validate that the logs contain the expected output.
This would give us proper end-to-end coverage for feature-gate behavior.
@@ -119,6 +119,8 @@ help: ## Display this help. | |||
.PHONY: manifests | |||
manifests: controller-gen ## Generate WebhookConfiguration, ClusterRole and CustomResourceDefinition objects. | |||
$(CONTROLLER_GEN) rbac:roleName=manager-role crd webhook paths="./..." output:crd:artifacts:config=config/crd/bases | |||
# TODO: When controller-tools supports feature gates (issue #1238), add: |
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.
# TODO: When controller-tools supports feature gates (issue #1238), add: |
I do not think that we should change the default scaffolding, adding the TODO:
But we definilly would need to document
return nil | ||
} | ||
|
||
const featureGatesTemplate = `{{ .Boilerplate }} |
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.
One of the challenges here is the UX.
A feature gate is an advanced capability, and not all projects will require or use it.
We need to think about how to make it optional.
One idea is to use plugins. For example, if we run:
kubebuilder edit plugins=feature-gate
then we could add the feature gate logic into the scaffolds.
We also have markers that can inject code, which might be useful here. For reference:
https://book.kubebuilder.io/reference/markers/scaffold
|
||
const featureGatesTemplate = `{{ .Boilerplate }} | ||
|
||
package featuregates |
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.
How is your idea?
Could we have a UX where we minimise the utilities in the scaffolds?
Could we make it simple for common use cases?
✨ Add feature gates support for experimental API fields
Description
Implements initial feature gates functionality to enable/disable experimental features in Kubebuilder controllers. Adds
+feature-gate
marker support for API fields, automatic discovery during scaffolding, and--feature-gates
flag for runtime control.Motivation
Enables gradual rollout of experimental features while maintaining backward compatibility. Similar to Kubernetes core feature gates, this allows developers to safely introduce new API fields and functionality.
Changes
pkg/machinery/featuregate.go
)pkg/machinery/featuregate_marker.go
)--feature-gates
flag to controller main.goFuture Work
CRD schema modification requires controller-tools support (issue #1238). When implemented,
+kubebuilder:feature-gate
markers will automatically exclude disabled fields from CRD schemas.Fixes #849