Skip to content

CNTRLPLANE-2847: render: Read FeatureGate CR from rendered manifests #1648

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

Open
sanchezl wants to merge 1 commit into
base: main
Choose a base branch
from
+239 −75
Open

CNTRLPLANE-2847: render: Read FeatureGate CR from rendered manifests #1648

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
160 changes: 138 additions & 22 deletions pkg/cmd/render/render.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
configv1alpha1 "github.com/openshift/api/config/v1alpha1"
features "github.com/openshift/api/features"
"github.com/openshift/library-go/pkg/assets"
"github.com/openshift/library-go/pkg/operator/configobserver/featuregates"
"github.com/openshift/library-go/pkg/pki"
"github.com/spf13/cobra"
"github.com/spf13/pflag"
Expand All @@ -47,6 +48,9 @@ type renderOpts struct {
clusterConfigMapFile string
infraConfigFile string

renderedManifestFiles []string
payloadVersion string

delayedHABootstrapScalingStrategyMarker string
bootstrapIPLocator BootstrapIPLocator
}
Expand Down Expand Up @@ -86,6 +90,8 @@ func (r *renderOpts) AddFlags(fs *pflag.FlagSet) {
fs.StringVar(&r.networkConfigFile, "network-config-file", "", "File containing the network.config.openshift.io manifest.")
fs.StringVar(&r.clusterConfigMapFile, "cluster-configmap-file", "", "File containing the cluster-config-v1 configmap.")
fs.StringVar(&r.infraConfigFile, "infra-config-file", "", "File containing infrastructure.config.openshift.io manifest.")
fs.StringArrayVar(&r.renderedManifestFiles, "rendered-manifest-files", nil, "Files or directories containing pre-rendered manifests, used to determine FeatureGate status.")
fs.StringVar(&r.payloadVersion, "payload-version", "", "Version that will eventually be placed into ClusterOperator.status. This normally comes from the CVO set via env var: OPERATOR_IMAGE_VERSION.")
Comment on lines +93 to +94

@tthvo tthvo Jul 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These flags rendered-manifest-files and payload-version are already defined as generic render flags in openshift/library-go@pkg/operator/render/options/generic.go#L57-L59, right? I'm referencing kube-controller-render, which also accepts the 2 flags.

We may already have existing feature-gate funcs to help us here too, for example 👀

@sanchezl sanchezl Jul 14, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, CKCMO and CKASO embed genericrenderoptions.GenericOptions which provides rendered-manifest-files, payload-version, and FeatureGates() out of the box.

The etcd render command does not use library-go's generic render options infrastructure at all, it has its own renderOpts struct with manually-defined flags. Refactoring it to embed GenericOptions would be a much larger change (the entire render pipeline would need to be restructured to match the pattern CKCMO uses with ManifestOptions + GenericOptions).

This PR takes the narrower approach doing the minimum needed to unblock the configurable PKI work. A full refactor to align with the generic render framework is worth doing but can be a followup effort.

@tthvo tthvo Jul 14, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The etcd render command does not use library-go's generic render options infrastructure at all

Oh whoops, my bad, I didn't realize that at all! Thanks for the details 🙏


// TODO(marun) Discover scaling strategy with less hack
fs.StringVar(&r.delayedHABootstrapScalingStrategyMarker, "delayed-ha-bootstrap-scaling-marker-file", "/assets/assisted-install-bootstrap", "Marker file that, if present, enables the delayed HA bootstrap scaling strategy")
Expand Down Expand Up @@ -279,7 +285,10 @@ func newTemplateData(opts *renderOpts) (*TemplateData, error) {
base64.StdEncoding.WithPadding(base64.NoPadding).EncodeToString([]byte(templateData.BootstrapIP)), templateData.BootstrapIP)
}

enabledFeatureGates, disabledFeatureGates := getFeatureGatesStatus(installConfig)
enabledFeatureGates, disabledFeatureGates, err := opts.getFeatureGates(installConfig)
if err != nil {
return nil, fmt.Errorf("failed to get feature gates: %w", err)
}

var pkiProfileProvider pki.PKIProfileProvider
if enabledFeatureGates.Has(features.FeatureGateConfigurablePKI) {
Expand Down Expand Up @@ -794,29 +803,136 @@ func getBootstrapScalingStrategy(installConfig map[string]any, delayedHAMarkerFi
return strategy, nil
}

// getFeatureGatesStatus returns the enabled and disabled feature gates.
func getFeatureGatesStatus(installConfig map[string]any) (sets.Set[configv1.FeatureGateName], sets.Set[configv1.FeatureGateName]) {
enabled, disabled := sets.Set[configv1.FeatureGateName]{}, sets.Set[configv1.FeatureGateName]{}
// getFeatureGates returns the enabled and disabled feature gate sets.
//
// Primary path: when --rendered-manifest-files and --payload-version are
// provided, reads the pre-resolved FeatureGate CR from rendered manifests.
// This is the standard operator render pattern used by CKASO and CKCMO.
//
// Fallback path: when rendered manifests are not available, resolves
// feature gates from the install-config's featureSet and featureGates
// fields directly. This fallback exists for transition until the
// installer passes the new flags to the etcd-render invocation, and
// should be removed once openshift/installer carries the change.
func (r *renderOpts) getFeatureGates(installConfig map[string]any) (sets.Set[configv1.FeatureGateName], sets.Set[configv1.FeatureGateName], error) {
if len(r.renderedManifestFiles) > 0 && len(r.payloadVersion) > 0 {
return r.getFeatureGatesFromManifests()
}
klog.Warningf("--rendered-manifest-files or --payload-version not provided, falling back to install-config feature gate parsing")
enabled, disabled := getFeatureGatesFromInstallConfig(installConfig)
return enabled, disabled, nil
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}

// On bootstrap we might not be able to fetch a list of all feature gates
// Hardcode a list of necessary for bootstrap here
necessaryFeatureGates := []configv1.FeatureGateName{"ShortCertRotation"}
disabled.Insert(necessaryFeatureGates...)
// getFeatureGatesFromManifests reads the FeatureGate CR from pre-rendered
// manifests and returns enabled/disabled sets.
func (r *renderOpts) getFeatureGatesFromManifests() (sets.Set[configv1.FeatureGateName], sets.Set[configv1.FeatureGateName], error) {
fg, err := r.readFeatureGate()
if err != nil {
return nil, nil, err
}
accessor, err := featuregates.NewHardcodedFeatureGateAccessFromFeatureGate(fg, r.payloadVersion)
if err != nil {
return nil, nil, err
}
featureGate, err := accessor.CurrentFeatureGates()
if err != nil {
return nil, nil, err
}
enabled := sets.New[configv1.FeatureGateName]()
disabled := sets.New[configv1.FeatureGateName]()
for _, name := range featureGate.KnownFeatures() {
if featureGate.Enabled(name) {
enabled.Insert(name)
} else {
disabled.Insert(name)
}
}
return enabled, disabled, nil
}

featureGates, found := installConfig["featureGates"]
if !found {
return enabled, disabled
}

for _, featureGate := range featureGates.([]any) {
key := strings.Split(featureGate.(string), "=")[0]
value := strings.Split(featureGate.(string), "=")[1]
if value == "true" {
enabled = enabled.Insert(configv1.FeatureGateName(key))
disabled = disabled.Delete(configv1.FeatureGateName(key))
} else if value == "false" {
enabled = enabled.Delete(configv1.FeatureGateName(key))
disabled = disabled.Insert(configv1.FeatureGateName(key))
// readFeatureGate scans rendered manifest files/directories for a FeatureGate CR.
func (r *renderOpts) readFeatureGate() (*configv1.FeatureGate, error) {
for _, manifestPath := range r.renderedManifestFiles {
info, err := os.Stat(manifestPath)
if err != nil {
return nil, fmt.Errorf("failed to stat %s: %w", manifestPath, err)
}

var files []string
if info.IsDir() {
entries, err := os.ReadDir(manifestPath)
if err != nil {
return nil, fmt.Errorf("failed to read directory %s: %w", manifestPath, err)
}
for _, entry := range entries {
if entry.IsDir() {
continue
}
files = append(files, filepath.Join(manifestPath, entry.Name()))
}
} else {
files = []string{manifestPath}
}

for _, file := range files {
data, err := os.ReadFile(file)
if err != nil {
continue
}
jsonBytes, err := yaml.YAMLToJSON(data)
if err != nil {
continue
}
fg := &configv1.FeatureGate{}
if err := json.Unmarshal(jsonBytes, fg); err != nil {
continue
Comment on lines +877 to +888

@coderabbitai coderabbitai Bot Jul 10, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Preserve manifest read and decoding errors.

These continue branches hide the actual failing file and can misleadingly report that no FeatureGate exists. Return contextual errors instead. As per path instructions, Go code must never ignore error returns.

Proposed fix 
 data, err := os.ReadFile(file)
 if err != nil {
-	continue
+	return nil, fmt.Errorf("failed to read manifest %s: %w", file, err)
 }
 jsonBytes, err := yaml.YAMLToJSON(data)
 if err != nil {
-	continue
+	return nil, fmt.Errorf("failed to convert manifest %s to JSON: %w", file, err)
 }
 fg := &configv1.FeatureGate{}
 if err := json.Unmarshal(jsonBytes, fg); err != nil {
-	continue
+	return nil, fmt.Errorf("failed to decode manifest %s: %w", file, err)
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
for _, file := range files {
data, err := os.ReadFile(file)
if err != nil {
continue
}
jsonBytes, err := yaml.YAMLToJSON(data)
if err != nil {
continue
}
fg := &configv1.FeatureGate{}
if err := json.Unmarshal(jsonBytes, fg); err != nil {
continue
for _, file := range files {
data, err := os.ReadFile(file)
if err != nil {
return nil, fmt.Errorf("failed to read manifest %s: %w", file, err)
}
jsonBytes, err := yaml.YAMLToJSON(data)
if err != nil {
return nil, fmt.Errorf("failed to convert manifest %s to JSON: %w", file, err)
}
fg := &configv1.FeatureGate{}
if err := json.Unmarshal(jsonBytes, fg); err != nil {
return nil, fmt.Errorf("failed to decode manifest %s: %w", file, err)
}
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/cmd/render/render.go` around lines 863 - 874, In the FeatureGate manifest
loop, replace each silent continue for os.ReadFile, yaml.YAMLToJSON, and
json.Unmarshal failures with contextual error returns that identify the affected
file and wrap the original error; update the enclosing function as needed to
propagate these errors instead of reporting that no FeatureGate exists. Use the
loop and FeatureGate decoding logic around os.ReadFile, YAMLToJSON, and
json.Unmarshal as the implementation reference.

Source: Path instructions

@sanchezl sanchezl Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The continue is intentional — this function scans a directory of mixed manifests (e.g. Deployment, ConfigMap, FeatureGate). Files that are not valid YAML or do not unmarshal into a FeatureGate are expected and must be skipped. Returning an error here would break on the first non-FeatureGate manifest.

@coderabbitai coderabbitai Bot Jul 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@sanchezl That makes sense — since readFeatureGate scans a directory that can contain arbitrary manifest kinds (Deployment, ConfigMap, FeatureGate, etc.), skipping files that aren't valid YAML or don't unmarshal into a FeatureGate is the correct behavior, not an ignored error. Thanks for the clarification, withdrawing this suggestion.

✏️ Learnings added 
Learnt from: sanchezl
Repo: openshift/cluster-etcd-operator PR: 1648
File: pkg/cmd/render/render.go:877-888
Timestamp: 2026-07-13T16:41:46.375Z
Learning: In `pkg/cmd/render/render.go` (cluster-etcd-operator), the `readFeatureGate` function on `renderOpts` scans directories of pre-rendered manifests that contain mixed resource kinds (Deployment, ConfigMap, FeatureGate, etc.). The `continue` statements on `os.ReadFile`, `yaml.YAMLToJSON`, and `json.Unmarshal` failures inside this loop are intentional: non-FeatureGate or non-YAML files are expected and must be skipped rather than treated as errors.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

}
if fg.APIVersion == "config.openshift.io/v1" && fg.Kind == "FeatureGate" {
return fg, nil
}
}
}
return nil, fmt.Errorf("no FeatureGate manifest found in %v", r.renderedManifestFiles)
}

// getFeatureGatesFromInstallConfig resolves feature gates from the
// install-config featureSet and featureGates fields. This is the fallback
// for when rendered manifests are not available.
// TODO: Remove this fallback once the installer passes --rendered-manifest-files
// and --payload-version to the etcd-render invocation.
func getFeatureGatesFromInstallConfig(installConfig map[string]any) (sets.Set[configv1.FeatureGateName], sets.Set[configv1.FeatureGateName]) {
enabled := sets.New[configv1.FeatureGateName]()
disabled := sets.New[configv1.FeatureGateName]()

featureSet := configv1.Default
if featureSetRaw, found := installConfig["featureSet"]; found {

@hasbro17 hasbro17 Jul 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this fallback path is still causing issues on non-techpreview installs. I'm seeing repeated panics in the bootkube logs for etcd-render.

Jul 10 03:42:15 ip-10-0-97-204 bootkube.sh[5696]: W0710 03:42:15.634081       1 render.go:821] --rendered-manifest-files or --payload-version not provided, falling back to install-config feature gate parsing
Jul 10 03:42:15 ip-10-0-97-204 bootkube.sh[5696]: panic: feature "ShortCertRotation" is not registered in FeatureGates []
...
Jul 10 03:42:15 ip-10-0-97-204 systemd[1]: bootkube.service: Main process exited, code=exited, status=2/INVALIDARGUMENT
Jul 10 03:42:15 ip-10-0-97-204 systemd[1]: bootkube.service: Failed with result 'exit-code'.
Jul 10 03:42:15 ip-10-0-97-204 systemd[1]: bootkube.service: Consumed 20.897s CPU time, 1.3G memory peak.
Jul 10 03:42:20 ip-10-0-97-204 systemd[1]: bootkube.service: Scheduled restart job, restart counter is at 1.

Will share the review bot analysis below for more details.

@hasbro17 hasbro17 Jul 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🤖
The fallback path in getFeatureGatesFromInstallConfig panics on Default installs where featureSet is absent from the install-config (the standard case — configv1.Default is "" and omitempty causes the
installer to omit it entirely).

When neither featureSet nor featureGates is present, the function returns empty enabled/disabled sets. These flow through createBootstrapCertSecrets → createCertSecrets (certs.go:58) →
NewEtcdCertSignerController (etcdcertsignercontroller.go:159) → NewHardcodedFeatureGateAccess([], []). The controller then calls featureGates.Enabled(features.FeatureShortCertRotation), and since
ShortCertRotation is in neither set, featuregate.go:39 panics:

panic(fmt.Errorf("feature %q is not registered in FeatureGates %v", key, f.KnownFeatures()))

The old getFeatureGatesStatus prevented this with disabled.Insert(necessaryFeatureGates...) where necessaryFeatureGates = []configv1.FeatureGateName{"ShortCertRotation"}. The new code drops that safety
net without a complete replacement for the absent-featureSet case.

CI evidence: The bootkube.log from this job's artifacts
shows the etcd-render container crashing twice with the panic stack trace:

Jul 10 03:42:15 bootkube.sh[5696]: etcdcertsignercontroller.go:159 +0x528
Jul 10 03:42:15 bootkube.sh[5696]: certs.go:58 +0xb49
Jul 10 03:42:15 bootkube.sh[5696]: render.go:301 +0xbdb
...
Jul 10 03:42:15 systemd[1]: bootkube.service: Main process exited, code=exited, status=2/INVALIDARGUMENT
Jul 10 03:42:15 systemd[1]: bootkube.service: Failed with result 'exit-code'.
Jul 10 03:42:20 systemd[1]: bootkube.service: Scheduled restart job, restart counter is at 1.

Then the same panic repeats at 03:42:26. The job eventually passes because bootkube.service keeps restarting and on a later attempt the api-render step has already produced the FeatureGate CR manifest, so
the primary getFeatureGatesFromManifests path succeeds. But the fallback panics every time it's hit, adding fragility and latency to bootstrap.

The install-config for this job (install-config.yaml) confirms no featureSet field is present.

Suggested fix: Always resolve the featureSet, defaulting to configv1.Default when absent:

featureSet := configv1.Default
if featureSetRaw, found := installConfig["featureSet"]; found {
    if featureSetStr, ok := featureSetRaw.(string); ok {
        featureSet = configv1.FeatureSet(featureSetStr)
    }
}
if resolved := features.FeatureSets(0, features.SelfManaged, featureSet); resolved != nil {
    // populate enabled/disabled from resolved
}

This populates all known gates (including ShortCertRotation in disabled) for every featureSet, making the old hardcode genuinely unnecessary.

@hasbro17 hasbro17 Jul 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I know this goes away when openshift/installer#10679 lands but this has to come first I think.

if featureSetStr, ok := featureSetRaw.(string); ok {
featureSet = configv1.FeatureSet(featureSetStr)
}
}
if resolved := features.FeatureSets(0, features.SelfManaged, featureSet); resolved != nil {
for _, fg := range resolved.Enabled {
enabled.Insert(fg.FeatureGateAttributes.Name)
}
for _, fg := range resolved.Disabled {
disabled.Insert(fg.FeatureGateAttributes.Name)
}
}

if featureGatesRaw, found := installConfig["featureGates"]; found {
for _, entry := range featureGatesRaw.([]any) {
key, value, found := strings.Cut(entry.(string), "=")
if !found {
continue
}
name := configv1.FeatureGateName(key)
if value == "true" {
enabled.Insert(name)
disabled.Delete(name)
} else if value == "false" {
enabled.Delete(name)
disabled.Insert(name)
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
}

Expand Down
Loading