CNTRLPLANE-2847: render: Read FeatureGate CR from rendered manifests#1648
Conversation
WalkthroughThe render command accepts optional rendered-manifest paths and a payload version, resolves ChangesFeatureGate rendering
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RenderCommand
participant renderOpts
participant FeatureGateManifest
participant InstallConfig
participant TemplateData
RenderCommand->>renderOpts: pass manifest paths and payload version
renderOpts->>FeatureGateManifest: scan and parse FeatureGate resource
FeatureGateManifest-->>renderOpts: return enabled and disabled feature sets
renderOpts->>InstallConfig: parse fallback featureSet and featureGates
renderOpts->>TemplateData: provide resolved feature gate state
TemplateData-->>RenderCommand: create template and PKI data
Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@pkg/cmd/render/render.go`:
- Around line 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7e583b38-9d3d-4186-bf0c-2dc2791d0d0e
📒 Files selected for processing (2)
pkg/cmd/render/render.gopkg/cmd/render/render_test.go
| 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 |
There was a problem hiding this comment.
🩺 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.
| 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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
05a3ad1 to
74d2dc0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/cmd/render/render_test.go (1)
650-658: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueKeep the feature-gate fixture consistent.
defaultFeatureGates()already putsConfigurablePKIindisabled, so appending it toenabledleaves the generated manifest listing the gate in both sections. Move it fromdisabledtoenabledfor this test fixture.🤖 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_test.go` around lines 650 - 658, Update the test fixture setup around defaultFeatureGates() so FeatureGateConfigurablePKI is moved from the disabled slice to the enabled slice rather than appended to both; remove it from disabled before assigning enabledFeatureGates and disabledFeatureGates in testConfig.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@pkg/cmd/render/render.go`:
- Around line 922-935: The fallback feature-gate parser uses unchecked
assertions that can panic on malformed install-config data. In the parser
containing featureGatesRaw and enabled/disabled updates, validate that
featureGatesRaw is a []any and each entry is a string, returning contextual
errors for invalid types; propagate these errors through newTemplateData so
parsing fails cleanly.
- Around line 817-823: Update renderOpts.getFeatureGates to distinguish both
flags being absent from only one being provided: fall back to install-config
parsing only when renderedManifestFiles and payloadVersion are both empty; if
exactly one is set, return a configuration error instead of silently falling
back, and continue using getFeatureGatesFromManifests when both are present.
---
Nitpick comments:
In `@pkg/cmd/render/render_test.go`:
- Around line 650-658: Update the test fixture setup around
defaultFeatureGates() so FeatureGateConfigurablePKI is moved from the disabled
slice to the enabled slice rather than appended to both; remove it from disabled
before assigning enabledFeatureGates and disabledFeatureGates in testConfig.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b49c7794-19d5-4e54-bb09-cfc5fe96ab1d
📒 Files selected for processing (2)
pkg/cmd/render/render.gopkg/cmd/render/render_test.go
hasbro17
left a comment
hasbro17
left a comment
There was a problem hiding this comment.
See: https://github.com/openshift/cluster-etcd-operator/pull/1648/changes#r3561973119
TLDR: The fallback getFeatureGatesFromInstallConfig must resolve configv1.Default when featureSet is absent from install-config, otherwise the empty gate sets cause a panic at
featureGate.Enabled(ShortCertRotation) during bootstrap. See this job's
bootkube.log.
| enabled := sets.New[configv1.FeatureGateName]() | ||
| disabled := sets.New[configv1.FeatureGateName]() | ||
|
|
||
| if featureSetRaw, found := installConfig["featureSet"]; found { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
🤖
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.
There was a problem hiding this comment.
I know this goes away when openshift/installer#10679 lands but this has to come first I think.
d5bbe4f to
ad61905
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@pkg/cmd/render/render_test.go`:
- Around line 1057-1067: Extend the test cases around the existing
ConfigurablePKI feature-gate scenarios in the render test table with a Default
featureSet configuration that explicitly sets featureGates to
ConfigurablePKI=true and expects ConfigurablePKI enabled. This should exercise
the fallback parser’s explicit-enable branch without changing the existing
TechPreview or false-override cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 473854b1-b33d-4caa-9078-8020dc0593db
📒 Files selected for processing (2)
pkg/cmd/render/render.gopkg/cmd/render/render_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/cmd/render/render.go
| "featureGates override disables ConfigurablePKI even with TechPreview": { | ||
| installConfig: ` | ||
| apiVersion: v1 | ||
| metadata: | ||
| name: my-cluster | ||
| featureGates: [ShortCertRotation=true, UpgradeStatus=false] | ||
| featureSet: TechPreviewNoUpgrade | ||
| featureGates: [ConfigurablePKI=false] | ||
| `, | ||
| expectedEnabled: sets.New(features.FeatureShortCertRotation), | ||
| expectedDisabled: sets.New(features.FeatureGateUpgradeStatus), | ||
| }, | ||
| "unexpected data": { | ||
| installConfig: ` | ||
| apiVersion: v1 | ||
| metadata: | ||
| name: my-cluster | ||
| featureGates: [ShortCertRotation=true, UpgradeStatus=foobar] | ||
| `, | ||
| expectedEnabled: sets.New(features.FeatureShortCertRotation), | ||
| expectedDisabled: sets.New[configv1.FeatureGateName](), | ||
| expectConfigurablePKI: false, | ||
| }, | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Cover explicit fallback enablement.
Add a Default-featureSet case with featureGates: [ConfigurablePKI=true]. The suite covers FeatureSet enablement and the false override, but not the fallback parser’s explicit-enable branch; the Custom PKI integration test uses manifest-backed resolution instead. As per coding guidelines, “All changes must include unit test additions/changes.”
Suggested test case
+ "featureGates override enables ConfigurablePKI with default featureSet": {
+ installConfig: `
+apiVersion: v1
+metadata:
+ name: my-cluster
+featureGates: [ConfigurablePKI=true]
+`,
+ expectConfigurablePKI: true,
+ },📝 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.
| "featureGates override disables ConfigurablePKI even with TechPreview": { | |
| installConfig: ` | |
| apiVersion: v1 | |
| metadata: | |
| name: my-cluster | |
| featureGates: [ShortCertRotation=true, UpgradeStatus=false] | |
| featureSet: TechPreviewNoUpgrade | |
| featureGates: [ConfigurablePKI=false] | |
| `, | |
| expectedEnabled: sets.New(features.FeatureShortCertRotation), | |
| expectedDisabled: sets.New(features.FeatureGateUpgradeStatus), | |
| }, | |
| "unexpected data": { | |
| installConfig: ` | |
| apiVersion: v1 | |
| metadata: | |
| name: my-cluster | |
| featureGates: [ShortCertRotation=true, UpgradeStatus=foobar] | |
| `, | |
| expectedEnabled: sets.New(features.FeatureShortCertRotation), | |
| expectedDisabled: sets.New[configv1.FeatureGateName](), | |
| expectConfigurablePKI: false, | |
| }, | |
| } | |
| "featureGates override disables ConfigurablePKI even with TechPreview": { | |
| installConfig: ` | |
| apiVersion: v1 | |
| metadata: | |
| name: my-cluster | |
| featureSet: TechPreviewNoUpgrade | |
| featureGates: [ConfigurablePKI=false] | |
| `, | |
| expectConfigurablePKI: false, | |
| }, | |
| "featureGates override enables ConfigurablePKI with default featureSet": { | |
| installConfig: ` | |
| apiVersion: v1 | |
| metadata: | |
| name: my-cluster | |
| featureGates: [ConfigurablePKI=true] | |
| `, | |
| expectConfigurablePKI: true, | |
| }, | |
| } |
🤖 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_test.go` around lines 1057 - 1067, Extend the test
cases around the existing ConfigurablePKI feature-gate scenarios in the render
test table with a Default featureSet configuration that explicitly sets
featureGates to ConfigurablePKI=true and expects ConfigurablePKI enabled. This
should exercise the fallback parser’s explicit-enable branch without changing
the existing TechPreview or false-override cases.
Source: Coding guidelines
|
/retest |
1 similar comment
|
/retest |
|
/testwith openshift/cluster-etcd-operator/main/e2e-aws-ovn-pki-rsa-techpreview openshift/installer#10595 |
|
/retest-required |
| 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.") |
There was a problem hiding this comment.
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 👀
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 🙏
|
/retest |
|
/testwith openshift/cluster-etcd-operator/main/e2e-aws-ovn-pki-rsa-techpreview openshift/installer#10595 |
|
/test e2e-agnostic-ovn |
|
/test e2e-gcp-operator |
|
/test e2e-aws-ovn-single-node |
|
Alright so this works now looking at the etcd-signer that's correctly configured per the PKI config to be RSA 4096 as the test does it. --- Checking: etcd-signer (openshift-etcd/etcd-signer) ---
...
Subject: CN=openshift-etcd_etcd-signer@1784038543
Subject Public Key Info:
Public Key Algorithm: rsaEncryption
Public-Key: (4096 bit)There's a typo bug in my test. The verification script checks for secret --- Checking: etcd-metrics-signer (openshift-etcd/etcd-metrics-signer) ---
Could not find certificate from <stdin>
FAIL: Expected algorithm 'rsaEncryption', got ''
FAIL: Expected 'Public-Key: (4096 bit)', got 'not found'Will fix that separately. But this should be good to move forward. |
| enabled, disabled := defaultFeatureGates() | ||
| enabled = append(enabled, features.FeatureGateConfigurablePKI) |
There was a problem hiding this comment.
The test appends ConfigurablePKI to the enabled list but doesn't remove it from disabled (which defaultFeatureGates() populates from the Default feature set). The resulting FeatureGate manifest has
ConfigurablePKI in both sections.
Works only because Enabled() checks the enabled slice first.
Fix: filter ConfigurablePKI out of the disabled slice when adding it to enabled.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hasbro17 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
Verification AnalysisWhat changed
CI Results (9/9 passing)
Artifact-level evidence: Panic fix confirmedThe fallback path (
Both jobs logged Multi-PR payload jobThe Caveats
|
|
/verified by pull-ci-openshift-cluster-etcd-operator-main-e2e-agnostic-ovn,pull-ci-openshift-cluster-etcd-operator-main-e2e-aws-ovn-single-node,pull-ci-openshift-cluster-etcd-operator-main-e2e-gcp-operator |
openshift-ci-robot
added
the
verified
|
@sanchezl: This PR has been marked as verified by DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
/label acknowledge-critical-fixes-only |
openshift-ci
Bot
added
the
acknowledge-critical-fixes-only
sanchezl
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@sanchezl: This pull request references CNTRLPLANE-2847 which is a valid jira issue. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
/lgtm |
|
Scheduling required tests: |
|
@sanchezl: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
0185d88
into
openshift:main
4 participants
Summary
--rendered-manifest-filesand--payload-versionfeatureSetandfeatureGatesfrom the install-config when the new flags are not yet availablefeatureSetresolution gap that PR #1647 addressesBackground
The CEO render was the only operator that parsed feature gates directly from the raw install-config (
featureGates: [Key=Value]strings). Every other operator render (CKASO, CKCMO, MCO) reads the pre-resolvedFeatureGateCR from/assets/manifests/— theapi-renderstep inbootkube.shruns first and resolvesfeatureSet(e.g.TechPreviewNoUpgrade) into individual gates, writing the result into the CR status.Because the CEO was parsing the raw install-config, it missed
featureSetresolution entirely, causingConfigurablePKIto not be detected when usingfeatureSet: TechPreviewNoUpgradeinstead of explicitfeatureGatesentries.Approach
Primary path (when
--rendered-manifest-filesand--payload-versionare provided): Read the pre-resolved FeatureGate CR from rendered manifests, matching the pattern used by CKASO and CKCMO. This eliminates the entire class of feature-gate-resolution bugs.Fallback path (when the flags are not provided): Resolve
featureSetinto individual gates viafeatures.FeatureSets()from openshift/api, then layer explicitfeatureGatesoverrides on top. This fallback usesstrings.Cutfor safe parsing and handles thefeatureSet→featureGatesoverride correctly. It should be removed once the installer carries the coordinated change.Changes
pkg/cmd/render/render.go:--rendered-manifest-filesand--payload-versionflagsgetFeatureGates()dispatcher that tries the CR path, falls back to install-configgetFeatureGatesFromManifests()— reads FeatureGate CR, returns enabled/disabled setsgetFeatureGatesFromInstallConfig()— resolvesfeatureSet+featureGatesfrom install-configgetFeatureGatesStatus()functionpkg/cmd/render/render_test.go:writeFeatureGateManifest()test helper for the CR pathTest_getFeatureGatesFromInstallConfigtesting featureSet resolution, overridesTestTemplateDataWithCustomPKIto provide ConfigurablePKI via FeatureGate CRCoordinated change
openshift/installer#10679 passes
--rendered-manifest-filesand--payload-versionto the etcd-render invocation. This CEO PR can land independently — the fallback path handles the transition. Once the installer PR merges, a follow-up can remove the fallback.Test plan
go test ./pkg/cmd/render/— all tests pass (both primary and fallback paths)go vet ./pkg/cmd/render/— cleanSummary by CodeRabbit