-
Notifications
You must be signed in to change notification settings - Fork 447
✨ WIP Add feature gate marker support for conditional CRD field inclusion #1259
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: main
Are you sure you want to change the base?
Changes from all commits
ea9c147
de2450b
a4024f9
15beb5e
cc41ffa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,155 @@ | ||
/* | ||
Copyright 2025. | ||
|
||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
|
||
http://www.apache.org/licenses/LICENSE-2.0 | ||
|
||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
*/ | ||
|
||
package crd_test | ||
|
||
import ( | ||
"bytes" | ||
"io" | ||
"os" | ||
"path/filepath" | ||
|
||
"github.com/google/go-cmp/cmp" | ||
. "github.com/onsi/ginkgo" | ||
. "github.com/onsi/gomega" | ||
"sigs.k8s.io/controller-tools/pkg/crd" | ||
crdmarkers "sigs.k8s.io/controller-tools/pkg/crd/markers" | ||
"sigs.k8s.io/controller-tools/pkg/genall" | ||
"sigs.k8s.io/controller-tools/pkg/loader" | ||
"sigs.k8s.io/controller-tools/pkg/markers" | ||
) | ||
|
||
var _ = Describe("CRD Feature Gate Generation", func() { | ||
var ( | ||
ctx *genall.GenerationContext | ||
out *featureGateOutputRule | ||
featureGateDir string | ||
originalWorkingDir string | ||
) | ||
|
||
BeforeEach(func() { | ||
var err error | ||
originalWorkingDir, err = os.Getwd() | ||
Expect(err).NotTo(HaveOccurred()) | ||
|
||
featureGateDir = filepath.Join(originalWorkingDir, "testdata", "featuregates") | ||
|
||
By("switching into featuregates testdata") | ||
err = os.Chdir(featureGateDir) | ||
Expect(err).NotTo(HaveOccurred()) | ||
|
||
By("loading the roots") | ||
pkgs, err := loader.LoadRoots(".") | ||
Expect(err).NotTo(HaveOccurred()) | ||
Expect(pkgs).To(HaveLen(1)) | ||
|
||
out = &featureGateOutputRule{buf: &bytes.Buffer{}} | ||
ctx = &genall.GenerationContext{ | ||
Collector: &markers.Collector{Registry: &markers.Registry{}}, | ||
Roots: pkgs, | ||
Checker: &loader.TypeChecker{}, | ||
OutputRule: out, | ||
} | ||
Expect(crdmarkers.Register(ctx.Collector.Registry)).To(Succeed()) | ||
}) | ||
|
||
AfterEach(func() { | ||
By("restoring original working directory") | ||
err := os.Chdir(originalWorkingDir) | ||
Expect(err).NotTo(HaveOccurred()) | ||
}) | ||
|
||
It("should not include feature-gated fields when no gates are enabled", func() { | ||
By("calling the generator") | ||
gen := &crd.Generator{ | ||
CRDVersions: []string{"v1"}, | ||
// No FeatureGates specified | ||
} | ||
Expect(gen.Generate(ctx)).NotTo(HaveOccurred()) | ||
|
||
By("loading the expected YAML") | ||
expectedFile, err := os.ReadFile("output_none/_featuregatetests.yaml") | ||
Expect(err).NotTo(HaveOccurred()) | ||
|
||
By("comparing the two") | ||
Expect(out.buf.String()).To(Equal(string(expectedFile)), cmp.Diff(out.buf.String(), string(expectedFile))) | ||
}) | ||
|
||
It("should include only alpha-gated fields when alpha gate is enabled", func() { | ||
By("calling the generator") | ||
gen := &crd.Generator{ | ||
CRDVersions: []string{"v1"}, | ||
FeatureGates: "alpha=true", | ||
} | ||
Expect(gen.Generate(ctx)).NotTo(HaveOccurred()) | ||
|
||
By("loading the expected YAML") | ||
expectedFile, err := os.ReadFile("output_alpha/_featuregatetests.yaml") | ||
Expect(err).NotTo(HaveOccurred()) | ||
|
||
By("comparing the two") | ||
Expect(out.buf.String()).To(Equal(string(expectedFile)), cmp.Diff(out.buf.String(), string(expectedFile))) | ||
}) | ||
|
||
It("should include only beta-gated fields when beta gate is enabled", func() { | ||
By("calling the generator") | ||
gen := &crd.Generator{ | ||
CRDVersions: []string{"v1"}, | ||
FeatureGates: "beta=true", | ||
} | ||
Expect(gen.Generate(ctx)).NotTo(HaveOccurred()) | ||
|
||
By("loading the expected YAML") | ||
expectedFile, err := os.ReadFile("output_beta/_featuregatetests.yaml") | ||
Expect(err).NotTo(HaveOccurred()) | ||
|
||
By("comparing the two") | ||
Expect(out.buf.String()).To(Equal(string(expectedFile)), cmp.Diff(out.buf.String(), string(expectedFile))) | ||
}) | ||
|
||
It("should include both feature-gated fields when both gates are enabled", func() { | ||
By("calling the generator") | ||
gen := &crd.Generator{ | ||
CRDVersions: []string{"v1"}, | ||
FeatureGates: "alpha=true,beta=true", | ||
} | ||
Expect(gen.Generate(ctx)).NotTo(HaveOccurred()) | ||
|
||
By("loading the expected YAML") | ||
expectedFile, err := os.ReadFile("output_both/_featuregatetests.yaml") | ||
Expect(err).NotTo(HaveOccurred()) | ||
|
||
By("comparing the two") | ||
Expect(out.buf.String()).To(Equal(string(expectedFile)), cmp.Diff(out.buf.String(), string(expectedFile))) | ||
}) | ||
}) | ||
|
||
// Helper types for testing | ||
type featureGateOutputRule struct { | ||
buf *bytes.Buffer | ||
} | ||
|
||
func (o *featureGateOutputRule) Open(_ *loader.Package, _ string) (io.WriteCloser, error) { | ||
return featureGateNopCloser{o.buf}, nil | ||
} | ||
|
||
type featureGateNopCloser struct { | ||
io.Writer | ||
} | ||
|
||
func (n featureGateNopCloser) Close() error { | ||
return nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,6 +32,47 @@ import ( | |
"sigs.k8s.io/controller-tools/pkg/version" | ||
) | ||
|
||
// FeatureGateMap represents a map of feature gate names to their enabled status. | ||
type FeatureGateMap map[string]bool | ||
|
||
// parseFeatureGates parses a feature gates string in the format "gate1=true,gate2=false" | ||
// and returns a FeatureGateMap. Supports comma-separated key=value pairs. | ||
// and returns a FeatureGateMap. | ||
func parseFeatureGates(featureGatesStr string) (FeatureGateMap, error) { | ||
gates := make(FeatureGateMap) | ||
if featureGatesStr == "" { | ||
return gates, nil | ||
} | ||
|
||
pairs := strings.Split(featureGatesStr, ",") | ||
for _, pair := range pairs { | ||
parts := strings.Split(strings.TrimSpace(pair), "=") | ||
if len(parts) != 2 { | ||
return nil, fmt.Errorf("invalid feature gate format: %s (expected format: gate1=true,gate2=false)", pair) | ||
} | ||
|
||
name := strings.TrimSpace(parts[0]) | ||
valueStr := strings.TrimSpace(parts[1]) | ||
|
||
switch valueStr { | ||
case "true": | ||
gates[name] = true | ||
case "false": | ||
gates[name] = false | ||
default: | ||
return nil, fmt.Errorf("invalid feature gate value for %s: %s (must be 'true' or 'false')", name, valueStr) | ||
} | ||
} | ||
|
||
return gates, nil | ||
} | ||
|
||
// isFeatureGateEnabled checks if a feature gate is enabled. | ||
func (fg FeatureGateMap) isEnabled(gateName string) bool { | ||
enabled, exists := fg[gateName] | ||
return exists && enabled | ||
} | ||
Comment on lines
+70
to
+74
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This means the absence of a feature gate in the list of provided gates automatically means its disabled, right? Do we want to be more explicit about this behaviour and make it an error if the generator finds a gate for which it doesn't have an opinion? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes you are right this is more like a silent disable, imho it's simple and predictable, yet it would allow typos in feature gate names to go unnoticed, I can switch that to explicitly error which will force decisions about every gate and would give user more clarity on possible issues, wdyt? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it would be safer to force a decision for each gate My concern is that someone might accidentally miss one gate, generate an updated set of CRDs, and then publish a breaking change because they didn't review properly and notice that a gated field got removed |
||
|
||
// The identifier for v1 CustomResourceDefinitions. | ||
const v1 = "v1" | ||
|
||
|
@@ -85,6 +126,16 @@ type Generator struct { | |
// Year specifies the year to substitute for " YEAR" in the header file. | ||
Year string `marker:",optional"` | ||
|
||
// FeatureGates specifies which feature gates are enabled for conditional field inclusion. | ||
// | ||
// Single gate format: "gatename=true" | ||
// Multiple gates format: "gate1=true,gate2=false" (must use quoted strings for comma-separated values) | ||
// | ||
// Examples: | ||
// controller-gen crd:featureGates="alpha=true" paths=./api/... | ||
// controller-gen 'crd:featureGates="alpha=true,beta=false"' paths=./api/... | ||
FeatureGates string `marker:",optional"` | ||
|
||
// DeprecatedV1beta1CompatibilityPreserveUnknownFields indicates whether | ||
// or not we should turn off field pruning for this resource. | ||
// | ||
|
@@ -124,6 +175,11 @@ func transformPreserveUnknownFields(value bool) func(map[string]interface{}) err | |
} | ||
|
||
func (g Generator) Generate(ctx *genall.GenerationContext) error { | ||
featureGates, err := parseFeatureGates(g.FeatureGates) | ||
if err != nil { | ||
return fmt.Errorf("invalid feature gates: %w", err) | ||
} | ||
|
||
parser := &Parser{ | ||
Collector: ctx.Collector, | ||
Checker: ctx.Checker, | ||
|
@@ -132,6 +188,7 @@ func (g Generator) Generate(ctx *genall.GenerationContext) error { | |
AllowDangerousTypes: g.AllowDangerousTypes != nil && *g.AllowDangerousTypes, | ||
// Indicates the parser on whether to register the ObjectMeta type or not | ||
GenerateEmbeddedObjectMeta: g.GenerateEmbeddedObjectMeta != nil && *g.GenerateEmbeddedObjectMeta, | ||
FeatureGates: featureGates, | ||
} | ||
|
||
AddKnownTypes(parser) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
/* | ||
Copyright 2025. | ||
|
||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
|
||
http://www.apache.org/licenses/LICENSE-2.0 | ||
|
||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
*/ | ||
|
||
package markers | ||
|
||
import ( | ||
apiext "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" | ||
) | ||
|
||
// +controllertools:marker:generateHelp:category="CRD feature gates" | ||
|
||
// FeatureGate marks a field to be conditionally included based on feature gate enablement. | ||
// Fields marked with +kubebuilder:feature-gate will only be included in generated CRDs | ||
// when the specified feature gate is enabled via the crd:featureGates parameter. | ||
type FeatureGate string | ||
|
||
// ApplyToSchema does nothing for feature gates - they are processed by the generator | ||
// to conditionally include/exclude fields. | ||
func (FeatureGate) ApplyToSchema(schema *apiext.JSONSchemaProps, field string) error { | ||
// Feature gates don't modify the schema directly. | ||
// They are processed by the generator to conditionally include/exclude fields. | ||
return nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -72,17 +72,19 @@ type schemaContext struct { | |
|
||
allowDangerousTypes bool | ||
ignoreUnexportedFields bool | ||
featureGates FeatureGateMap | ||
} | ||
|
||
// newSchemaContext constructs a new schemaContext for the given package and schema requester. | ||
// It must have type info added before use via ForInfo. | ||
func newSchemaContext(pkg *loader.Package, req schemaRequester, allowDangerousTypes, ignoreUnexportedFields bool) *schemaContext { | ||
func newSchemaContext(pkg *loader.Package, req schemaRequester, allowDangerousTypes, ignoreUnexportedFields bool, featureGates FeatureGateMap) *schemaContext { | ||
pkg.NeedTypesInfo() | ||
return &schemaContext{ | ||
pkg: pkg, | ||
schemaRequester: req, | ||
allowDangerousTypes: allowDangerousTypes, | ||
ignoreUnexportedFields: ignoreUnexportedFields, | ||
featureGates: featureGates, | ||
} | ||
} | ||
|
||
|
@@ -95,6 +97,7 @@ func (c *schemaContext) ForInfo(info *markers.TypeInfo) *schemaContext { | |
schemaRequester: c.schemaRequester, | ||
allowDangerousTypes: c.allowDangerousTypes, | ||
ignoreUnexportedFields: c.ignoreUnexportedFields, | ||
featureGates: c.featureGates, | ||
} | ||
} | ||
|
||
|
@@ -428,6 +431,17 @@ func structToSchema(ctx *schemaContext, structType *ast.StructType) *apiext.JSON | |
continue | ||
} | ||
|
||
// Check feature gate markers - skip field if feature gate is not enabled | ||
if featureGateMarker := field.Markers.Get("kubebuilder:feature-gate"); featureGateMarker != nil { | ||
if featureGate, ok := featureGateMarker.(crdmarkers.FeatureGate); ok { | ||
gateName := string(featureGate) | ||
if !ctx.featureGates.isEnabled(gateName) { | ||
// Skip this field as its feature gate is not enabled | ||
continue | ||
} | ||
} | ||
} | ||
Comment on lines
+435
to
+443
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have examples of APIs where they are gated on multiple feature gates, is that something we would be able to support? I have seen combinations in the past where both below cases are true:
Do we want to support those here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes this behaviour is supported by the current implementation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you provide an example? Of how I might do both the OR and AND scenarios? |
||
|
||
jsonTag, hasTag := field.Tag.Lookup("json") | ||
if !hasTag { | ||
// if the field doesn't have a JSON tag, it doesn't belong in output (and shouldn't exist in a serialized type) | ||
|
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.
New dependency?
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 do not use assert on the tests, give a look at the others.
You will see that the sintax is a little different but not hard to comply with.