Skip to content

✨ 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

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ require (
github.com/onsi/gomega v1.38.0
github.com/spf13/cobra v1.9.1
github.com/spf13/pflag v1.0.7
github.com/stretchr/testify v1.10.0
Copy link
Contributor

Choose a reason for hiding this comment

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

New dependency?

Copy link
Member

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.

golang.org/x/tools v0.36.0
golang.org/x/tools/go/packages/packagestest v0.1.1-deprecated
gopkg.in/yaml.v2 v2.4.0
Expand Down Expand Up @@ -55,6 +56,7 @@ require (
github.com/modern-go/reflect2 v1.0.3-0.20250322232337-35a7c28c31ee // indirect
github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect
github.com/nxadm/tail v1.4.8 // indirect
github.com/pmezard/go-difflib v1.0.0 // indirect
github.com/prometheus/client_golang v1.22.0 // indirect
github.com/prometheus/client_model v0.6.1 // indirect
github.com/prometheus/common v0.62.0 // indirect
Expand Down
155 changes: 155 additions & 0 deletions pkg/crd/featuregate_integration_test.go
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
}
57 changes: 57 additions & 0 deletions pkg/crd/gen.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Author

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

The 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"

Expand Down Expand Up @@ -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.
//
Expand Down Expand Up @@ -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,
Expand All @@ -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)
Expand Down
36 changes: 36 additions & 0 deletions pkg/crd/markers/featuregate.go
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
}
7 changes: 7 additions & 0 deletions pkg/crd/markers/validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,13 @@
func init() {
AllDefinitions = append(AllDefinitions, ValidationMarkers...)

// Add FeatureGate markers
featureGateMarkers := []*definitionWithHelp{
must(markers.MakeDefinition("kubebuilder:feature-gate", markers.DescribesField, FeatureGate(""))).WithHelp(FeatureGate("").Help()),

Check failure on line 137 in pkg/crd/markers/validation.go

View workflow job for this annotation

GitHub Actions / lint

FeatureGate("").Help undefined (type FeatureGate has no field or method Help)
must(markers.MakeDefinition("kubebuilder:feature-gate", markers.DescribesType, FeatureGate(""))).WithHelp(FeatureGate("").Help()),

Check failure on line 138 in pkg/crd/markers/validation.go

View workflow job for this annotation

GitHub Actions / lint

FeatureGate("").Help undefined (type FeatureGate has no field or method Help) (typecheck)
}
AllDefinitions = append(AllDefinitions, featureGateMarkers...)

for _, def := range ValidationMarkers {
typDef := def.clone()
typDef.Target = markers.DescribesType
Expand Down
5 changes: 4 additions & 1 deletion pkg/crd/parser.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,9 @@ type Parser struct {

// GenerateEmbeddedObjectMeta specifies if any embedded ObjectMeta should be generated
GenerateEmbeddedObjectMeta bool

// FeatureGates specifies which feature gates are enabled for conditional field inclusion
FeatureGates FeatureGateMap
}

func (p *Parser) init() {
Expand Down Expand Up @@ -172,7 +175,7 @@ func (p *Parser) NeedSchemaFor(typ TypeIdent) {
// avoid tripping recursive schemata, like ManagedFields, by adding an empty WIP schema
p.Schemata[typ] = apiext.JSONSchemaProps{}

schemaCtx := newSchemaContext(typ.Package, p, p.AllowDangerousTypes, p.IgnoreUnexportedFields)
schemaCtx := newSchemaContext(typ.Package, p, p.AllowDangerousTypes, p.IgnoreUnexportedFields, p.FeatureGates)
ctxForInfo := schemaCtx.ForInfo(info)

pkgMarkers, err := markers.PackageMarkers(p.Collector, typ.Package)
Expand Down
16 changes: 15 additions & 1 deletion pkg/crd/schema.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}
}

Expand All @@ -95,6 +97,7 @@ func (c *schemaContext) ForInfo(info *markers.TypeInfo) *schemaContext {
schemaRequester: c.schemaRequester,
allowDangerousTypes: c.allowDangerousTypes,
ignoreUnexportedFields: c.ignoreUnexportedFields,
featureGates: c.featureGates,
}
}

Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The 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:

  • enable this field if feature A OR feature B are enabled
  • enable this field if feature A AND feature B are enabled

Do we want to support those here?

Copy link
Author

Choose a reason for hiding this comment

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

yes this behaviour is supported by the current implementation.

Copy link
Contributor

Choose a reason for hiding this comment

The 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)
Expand Down
2 changes: 1 addition & 1 deletion pkg/crd/schema_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ func transform(t *testing.T, expr string) *apiext.JSONSchemaProps {
pkg.NeedTypesInfo()
failIfErrors(t, pkg.Errors)

schemaContext := newSchemaContext(pkg, nil, true, false).ForInfo(&markers.TypeInfo{})
schemaContext := newSchemaContext(pkg, nil, true, false, FeatureGateMap{}).ForInfo(&markers.TypeInfo{})
// yick: grab the only type definition
definedType := pkg.Syntax[0].Decls[0].(*ast.GenDecl).Specs[0].(*ast.TypeSpec).Type
result := typeToSchema(schemaContext, definedType)
Expand Down
Loading
Loading