CNTRLPLANE-2847: render: Resolve featureSet into individual feature gates during bootstrap#1647
CNTRLPLANE-2847: render: Resolve featureSet into individual feature gates during bootstrap#1647hasbro17 wants to merge 1 commit into
Conversation
…trap The render path's getFeatureGatesStatus only parsed explicit featureGates key=value entries from the install-config. It did not resolve featureSet (e.g. TechPreviewNoUpgrade) into the constituent gate list. When a cluster is installed with featureSet: TechPreviewNoUpgrade and no explicit featureGates entries, the ConfigurablePKI gate was never detected, causing bootstrap etcd-signer certs to be generated with legacy RSA-2048 instead of the configured algorithm. Use features.FeatureSets() from openshift/api to resolve the featureSet into enabled/disabled gates, then layer explicit featureGates overrides on top for CustomNoUpgrade support. Assisted-by: Claude Code (Opus 4.6)
openshift-ci-robot
added
the
jira/valid-reference
|
@hasbro17: This pull request references CNTRLPLANE-2847 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. 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. |
WalkthroughThe change extends feature-gate resolution in render.go to consider installConfig's featureSet field, resolving it through features.FeatureSets and merging results into enabled/disabled gate sets. featureGates parsing no longer early-returns when absent. Tests and fixtures cover TechPreviewNoUpgrade scenarios. ChangesFeatureSet feature gate support
Estimated code review effort: 2 (Simple) | ~15 minutes 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
/test e2e-aws-ovn-pki-default-techpreview |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/jira refresh |
|
@hasbro17: 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/cmd/render/render_test.go (1)
668-714: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate validation closure.
validateECDSAP521Signerhere is a verbatim copy of the one inTestTemplateDataWithCustomPKI(lines 622-657). Extract it into a single shared helper function to avoid drift between the two tests.♻️ Proposed refactor: extract shared validator
+func validateEtcdSignerECDSAP521(t *testing.T, td *TemplateData) { + if len(td.certificates) == 0 { + t.Fatal("no certificates generated") + } + + var signerCert []byte + for _, cert := range td.certificates { + if cert.Name == "etcd-signer" { + signerCert = cert.Data["tls.crt"] + break + } + } + + if signerCert == nil { + t.Fatal("etcd-signer certificate not found in generated certificates") + } + + block, _ := pem.Decode(signerCert) + if block == nil { + t.Fatal("failed to decode PEM block from etcd-signer certificate") + } + + x509Cert, err := x509.ParseCertificate(block.Bytes) + if err != nil { + t.Fatalf("failed to parse x509 certificate: %v", err) + } + + ecdsaKey, ok := x509Cert.PublicKey.(*ecdsa.PublicKey) + if !ok { + t.Fatalf("expected ECDSA public key, got %T", x509Cert.PublicKey) + } + + if ecdsaKey.Curve != elliptic.P521() { + t.Errorf("expected P521 curve, got %v", ecdsaKey.Curve.Params().Name) + } +} + func TestTemplateDataWithCustomPKIFeatureSet(t *testing.T) { - validateECDSAP521Signer := func(t *testing.T, td *TemplateData) { - ... - } - config := &testConfig{ clusterNetworkConfig: networkConfigIpv4, infraConfig: infraConfig, clusterConfigMap: clusterConfigMapWithCustomPKIAndFeatureSet, } - testTemplateData(t, config, validateECDSAP521Signer) + testTemplateData(t, config, validateEtcdSignerECDSAP521) }🤖 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 668 - 714, `validateECDSAP521Signer` is duplicated in `TestTemplateDataWithCustomPKIFeatureSet` and `TestTemplateDataWithCustomPKI`, so extract the validator into a shared test helper and reuse it from both tests. Move the certificate/key assertions into a single helper near `testTemplateData` or alongside the test functions, then call that helper from each test to keep the validation logic in one place and prevent drift.
🤖 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 821-833: The featureGates parsing in the render logic is indexing
the result of strings.Split without validation, which can panic on malformed
entries. Update the feature gate handling in render.go to parse each entry
safely in the same block that iterates featureGates, using a bounded split or
equivalent validation before reading key/value. If an entry does not contain a
valid key=value pair, report it as a configuration error instead of crashing,
and keep the enabled/disabled set updates in the existing render flow.
- Around line 806-807: The featureSet handling in render.go uses an unchecked
type assertion on installConfig["featureSet"], which can panic on malformed
external config. Update the featureSetRaw handling in the render path to use a
comma-ok string assertion (and skip or error out gracefully when the value is
not a string) so the bootstrap rendering flow does not crash. Use the existing
installConfig["featureSet"] lookup and configv1.FeatureSet assignment location
to apply the fix.
---
Nitpick comments:
In `@pkg/cmd/render/render_test.go`:
- Around line 668-714: `validateECDSAP521Signer` is duplicated in
`TestTemplateDataWithCustomPKIFeatureSet` and `TestTemplateDataWithCustomPKI`,
so extract the validator into a shared test helper and reuse it from both tests.
Move the certificate/key assertions into a single helper near `testTemplateData`
or alongside the test functions, then call that helper from each test to keep
the validation logic in one place and prevent drift.
🪄 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: 10ace7cb-2337-415c-b85f-fd41b932b21c
📒 Files selected for processing (2)
pkg/cmd/render/render.gopkg/cmd/render/render_test.go
| if featureSetRaw, found := installConfig["featureSet"]; found { | ||
| featureSet := configv1.FeatureSet(featureSetRaw.(string)) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unchecked type assertion on featureSetRaw.
featureSetRaw.(string) will panic if installConfig["featureSet"] is present but not a string (e.g., malformed YAML producing an int/bool/nil). This runs on the bootstrap render path, so a malformed install-config would crash rendering instead of degrading gracefully.
As per path instructions, Go code should "Never ignore error returns"; the same principle applies to unchecked type assertions on values sourced from external config here.
🛡️ Proposed fix using comma-ok assertion
if featureSetRaw, found := installConfig["featureSet"]; found {
- featureSet := configv1.FeatureSet(featureSetRaw.(string))
- if resolved := features.FeatureSets(0, features.SelfManaged, featureSet); resolved != nil {
+ featureSetStr, ok := featureSetRaw.(string)
+ if !ok {
+ return enabled, disabled
+ }
+ featureSet := configv1.FeatureSet(featureSetStr)
+ if resolved := features.FeatureSets(0, features.SelfManaged, featureSet); resolved != nil {📝 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.
| if featureSetRaw, found := installConfig["featureSet"]; found { | |
| featureSet := configv1.FeatureSet(featureSetRaw.(string)) | |
| if featureSetRaw, found := installConfig["featureSet"]; found { | |
| featureSetStr, ok := featureSetRaw.(string) | |
| if !ok { | |
| return enabled, disabled | |
| } | |
| featureSet := configv1.FeatureSet(featureSetStr) | |
| if resolved := features.FeatureSets(0, features.SelfManaged, featureSet); resolved != nil { |
🤖 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 806 - 807, The featureSet handling in
render.go uses an unchecked type assertion on installConfig["featureSet"], which
can panic on malformed external config. Update the featureSetRaw handling in the
render path to use a comma-ok string assertion (and skip or error out gracefully
when the value is not a string) so the bootstrap rendering flow does not crash.
Use the existing installConfig["featureSet"] lookup and configv1.FeatureSet
assignment location to apply the fix.
Source: Path instructions
| if featureGates, found := installConfig["featureGates"]; found { | ||
| 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)) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Unbounded index access on strings.Split result.
strings.Split(featureGate.(string), "=")[1] panics with an index-out-of-range if the entry has no = (e.g., a bare "FooBar" in the list). Since this is bootstrap-critical rendering, a single malformed entry crashes the whole render rather than being reported as a config error.
🛡️ Proposed fix using SplitN with a length check
for _, featureGate := range featureGates.([]any) {
- key := strings.Split(featureGate.(string), "=")[0]
- value := strings.Split(featureGate.(string), "=")[1]
+ parts := strings.SplitN(featureGate.(string), "=", 2)
+ if len(parts) != 2 {
+ continue
+ }
+ key, value := parts[0], parts[1]
if value == "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.
| if featureGates, found := installConfig["featureGates"]; found { | |
| 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)) | |
| } | |
| } | |
| } | |
| if featureGates, found := installConfig["featureGates"]; found { | |
| for _, featureGate := range featureGates.([]any) { | |
| parts := strings.SplitN(featureGate.(string), "=", 2) | |
| if len(parts) != 2 { | |
| continue | |
| } | |
| key, value := parts[0], parts[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)) | |
| } | |
| } | |
| } |
🤖 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 821 - 833, The featureGates parsing in
the render logic is indexing the result of strings.Split without validation,
which can panic on malformed entries. Update the feature gate handling in
render.go to parse each entry safely in the same block that iterates
featureGates, using a bounded split or equivalent validation before reading
key/value. If an entry does not contain a valid key=value pair, report it as a
configuration error instead of crashing, and keep the enabled/disabled set
updates in the existing render flow.
|
Waiting for |
|
|
||
| if featureGates, found := installConfig["featureGates"]; found { | ||
| for _, featureGate := range featureGates.([]any) { | ||
| key := strings.Split(featureGate.(string), "=")[0] |
There was a problem hiding this comment.
Could do key, value, found := strings.Cut(featureGate.(string), "=") and that would address CodeRabbit's complaint
|
Need a multi PR test to include the installer PKI support. |
|
/testwith e2e-aws-ovn-pki-rsa-techpreview openshift/installer#10595 |
|
@hasbro17, |
|
/testwith openshift/cluster-etcd-operator/main/e2e-aws-ovn-pki-rsa-techpreview openshift/installer#10595 |
|
@hasbro17: The following tests failed, say
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. |
There was a problem hiding this comment.
The CEO render should not parse the feature gates from the raw install-config. Looking at other operator render (CKASO, CKCMO, MCO) , they read the pre-resolved FeatureGate CR from /assets/manifests/ via --rendered-manifest-files and --payload-version. The api-render step in bootkube.sh already runs before etcd-render and produces the fully-resolved CR with featureSet expanded into individual gates, the CEO just never consumed it.
That architectural gap is why this bug exists in the first place: the other operators don't have this class of bug because they don't resolve feature gates themselves.
I've opened an alternative set of PRs that aligns CEO with the standard pattern, eliminating the entire class of feature-gate-resolution bugs rather than patching getFeatureGatesStatus for each new case:
-
#1648 — Adds
--rendered-manifest-filesand--payload-versionsupport to the render command. When provided, reads the FeatureGate CR from pre-rendered manifests (the same pattern CKASO/CKCMO use). Includes a fallback that resolvesfeatureSetfrom the install-config (covering this bug) so it can land independently without waiting for the installer change. -
openshift/installer#10679 — Passes the two new flags to the
etcd-renderinvocation inbootkube.sh, matching what's already done forkube-apiserver-render,kube-controller-manager-render, andconfig-render. -
#1649 — Cleanup: removes the install-config fallback and makes both flags required, once the installer PR has merged.
A few additional notes on this PR if it moves forward instead:
features.FeatureSets(0, ...)passes version0. Other callers (the installer, MCO tests) use the real major version (4). Today this is a no-op because no gate usesinVersion(), but if any gate adds a version constraint in the future, version0would silently exclude it.- The unchecked type assertion on
featureSetRaw.(string)(line 807) can panic on malformed install-config YAML — should use the comma-ok pattern. validateECDSAP521Signeris duplicated betweenTestTemplateDataWithCustomPKIandTestTemplateDataWithCustomPKIFeatureSet— worth extracting to a shared helper.
4 participants
Summary
getFeatureGatesStatusto resolvefeatureSet(e.g.,TechPreviewNoUpgrade) into individual feature gates during bootstrap rendering, so thatConfigurablePKIis correctly detected and bootstrapetcd-signer certs use the configured key algorithm
featureSetresolution,featureGatesoverride on top offeatureSet, and an end-to-end test that mirrors the real CI install-config structureBackground
PR #1593 added ConfigurablePKI support to the etcd-operator, threading a
PKIProfileProviderthrough both the bootstrap render path and the runtime controller. The installer counterpart(openshift/installer#10595) creates a PKI CR at install time with the desired key configuration (e.g., RSA-4096).
CI job
e2e-aws-ovn-pki-rsa-techpreviewon the installer PR showed the etcd-signer cert was still RSA-2048 instead of the configured RSA-4096. All 7 other signers (managed by kube-apiserver-operator,machine-config-operator, etc.) correctly used RSA-4096, confirming the PKI CR was created correctly and the feature gate was active.
Root cause
The install-config in CI has:
There are no explicit
featureGatesentries — the gates are implied byfeatureSet: TechPreviewNoUpgrade.getFeatureGatesStatus()inpkg/cmd/render/render.goonly parsed explicitfeatureGates: [Key=Value]strings. It did not resolvefeatureSetinto individual gates. So the enabled set was empty, theConfigurablePKIguard was never entered,pkiProfileProviderstayed nil, and the bootstrap certs were created with the legacy fallback (MakeSelfSignedCAConfigForDuration→ RSA-2048).At runtime, the operator correctly detected
ConfigurablePKIfrom the cluster's FeatureGate CR status, butneedNewSigningCertKeyPaironly checks time-based conditions (expiry, refresh) — it doesn't detectkey algorithm mismatches. So the already-valid RSA-2048 cert was never regenerated.
Fix
Use
features.FeatureSets()fromopenshift/apito resolve thefeatureSetfield into its constituent enabled/disabled gates, then layer explicitfeatureGatesoverrides on top (forCustomNoUpgradesupport). This ensures
ConfigurablePKIis detected during bootstrap rendering when installed withfeatureSet: TechPreviewNoUpgrade.Test plan
Test_getFeatureGatescases forfeatureSet: TechPreviewNoUpgradeandfeatureSet+featureGatesoverrideTestTemplateDataWithCustomPKIFeatureSetend-to-end test usingfeatureSet: TechPreviewNoUpgradewith PKI config (mirrors real CI install-config)TestTemplateDataWithCustomPKI(explicitfeatureGates) still passesmake verifypassese2e-aws-ovn-pki-rsa-techpreviewon installer PR with this change in the payloadSummary by CodeRabbit
New Features
Bug Fixes