Skip to content

render: Remove install-config feature gate fallback#1649

Open
sanchezl wants to merge 1 commit into
openshift:mainfrom
sanchezl:render-featuregate-cr-cleanup
Open

render: Remove install-config feature gate fallback#1649
sanchezl wants to merge 1 commit into
openshift:mainfrom
sanchezl:render-featuregate-cr-cleanup

Conversation

@sanchezl

@sanchezl sanchezl commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Summary

Remove the install-config feature gate parsing fallback now that openshift/installer#10679 passes --rendered-manifest-files and --payload-version to the etcd-render invocation.

This makes both flags required and removes getFeatureGatesFromInstallConfig() and the getFeatureGates() dispatcher, leaving only the standard FeatureGate CR path via getFeatureGatesFromManifests().

Depends on

  • #1648 — render: Read FeatureGate CR from rendered manifests (merged)
  • openshift/installer#10679 — bootstrap: Pass rendered manifests and payload version to etcd render

This PR should merge after the installer dependency has landed.

Test plan

  • go test ./pkg/cmd/render/ — all tests pass
  • go vet ./pkg/cmd/render/ — clean

Summary by CodeRabbit

  • New Features
    • Added command-line options to supply rendered manifest locations and the payload version.
    • Feature gate enablement is now derived from FeatureGate manifests included in rendered outputs.
  • Bug Fixes
    • Rendering now reads FeatureGate resources directly (from either files or directories) and validates that feature-gate data can be determined.
    • Rendering fails early when required feature-gate information is missing or cannot be derived.
  • Tests
    • Updated renderer/template tests to generate and consume payload-versioned FeatureGate manifests.

@coderabbitai

coderabbitai Bot commented Jul 10, 2026

Copy link
Copy Markdown

Walkthrough

Rendering now requires rendered manifest paths and a payload version, reads FeatureGate resources from manifests, and derives feature sets through a payload-versioned accessor. Tests generate corresponding manifests, including custom PKI coverage.

Changes

Manifest-based FeatureGate rendering

Layer / File(s) Summary
Manifest FeatureGate resolution
pkg/cmd/render/render.go
Validation requires rendered manifests and a payload version; template data derives enabled and disabled features from the first discovered FeatureGate resource.
Manifest-backed render tests
pkg/cmd/render/render_test.go
Tests generate FeatureGate manifests, pass the new render options, cover custom PKI, and remove install-config fallback coverage.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant newTemplateData
  participant getFeatureGatesFromManifests
  participant readFeatureGate
  participant renderedManifests
  newTemplateData->>getFeatureGatesFromManifests: request feature sets
  getFeatureGatesFromManifests->>readFeatureGate: scan rendered manifest paths
  readFeatureGate->>renderedManifests: read FeatureGate YAML or JSON
  renderedManifests-->>readFeatureGate: return FeatureGate resource
  readFeatureGate-->>getFeatureGatesFromManifests: return FeatureGate object
  getFeatureGatesFromManifests-->>newTemplateData: return enabled and disabled feature sets
Loading

Possibly related PRs

Suggested reviewers: flavianmissi, rh-roman

🚥 Pre-merge checks | ✅ 14 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (14 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names ✅ Passed No Ginkgo-style tests are present in the changed render package, and existing t.Run names are static table keys, not generated values.
Test Structure And Quality ✅ Passed PASS: render_test.go is standard Go unit tests, uses t.TempDir for cleanup, has no Ginkgo/Eventually/cluster interactions, and assertions are mostly contextual.
Microshift Test Compatibility ✅ Passed No Ginkgo e2e tests were added; render_test.go only contains testing.T unit tests, with no MicroShift-unsupported OpenShift APIs or guards involved.
Single Node Openshift (Sno) Test Compatibility ✅ Passed No new Ginkgo e2e tests were added; pkg/cmd/render changes are plain Go unit tests with no Describe/It or node-topology assumptions.
Topology-Aware Scheduling Compatibility ✅ Passed Only feature-gate parsing and test fixtures changed; no pod affinity, node selectors, topology spread, replica, or PDB scheduling logic was introduced.
Ote Binary Stdout Contract ✅ Passed No process-level stdout writes were added; errors go to injected errOut and klog defaults to stderr, with no main/init/TestMain setup changes.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed Touched tests are stdlib unit tests only; no Ginkgo e2e, public internet, or new IPv4-only URL assumptions were added.
No-Weak-Crypto ✅ Passed No weak crypto, custom crypto, or secret/token comparisons were added; changes only touch feature-gate handling and ECDSA P521 test certs.
Container-Privileges ✅ Passed Only render code/tests changed; no privileged/root/hostPID/hostNetwork/hostIPC/allowPrivilegeEscalation settings appear in the diff.
No-Sensitive-Data-In-Logs ✅ Passed PASS: the diff adds no new logging and removes a warning log; touched files only add feature-gate manifest parsing/errors, not secrets/PII logs.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: removing install-config feature gate fallback in render.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@rh-roman

Copy link
Copy Markdown
Contributor

/lgtm

@openshift-ci openshift-ci Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 15, 2026
@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Jul 15, 2026
@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Scheduling required tests:
/test e2e-agnostic-ovn
/test e2e-agnostic-ovn-upgrade
/test e2e-aws-ovn-serial-1of2
/test e2e-aws-ovn-serial-2of2
/test e2e-aws-ovn-single-node
/test e2e-gcp-operator
/test e2e-gcp-operator-disruptive
/test e2e-metal-ipi-ovn-ipv6
/test e2e-operator

Now that the installer passes --rendered-manifest-files and
--payload-version to the etcd-render invocation, remove the
install-config feature gate parsing fallback and make both flags
required.

This removes getFeatureGatesFromInstallConfig and the getFeatureGates
dispatcher, leaving only the standard FeatureGate CR path.
@sanchezl
sanchezl force-pushed the render-featuregate-cr-cleanup branch from ef85c95 to 042318e Compare July 15, 2026 12:47
@openshift-ci openshift-ci Bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 15, 2026
@openshift-ci

openshift-ci Bot commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

New changes are detected. LGTM label has been removed.

@openshift-ci openshift-ci Bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 15, 2026
@openshift-ci

openshift-ci Bot commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from rh-roman. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci

openshift-ci Bot commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

@sanchezl: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-gcp-operator-disruptive ef85c95 link true /test e2e-gcp-operator-disruptive
ci/prow/e2e-gcp-operator ef85c95 link true /test e2e-gcp-operator
ci/prow/e2e-aws-ovn-serial-2of2 ef85c95 link true /test e2e-aws-ovn-serial-2of2
ci/prow/e2e-operator ef85c95 link true /test e2e-operator
ci/prow/e2e-aws-ovn-single-node ef85c95 link true /test e2e-aws-ovn-single-node
ci/prow/e2e-aws-ovn-serial-1of2 ef85c95 link true /test e2e-aws-ovn-serial-1of2
ci/prow/e2e-agnostic-ovn ef85c95 link true /test e2e-agnostic-ovn
ci/prow/e2e-metal-ipi-ovn-ipv6 ef85c95 link true /test e2e-metal-ipi-ovn-ipv6
ci/prow/configmap-scale 042318e link false /test configmap-scale

Full PR test history. Your PR dashboard.

Details

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

@sanchezl

Copy link
Copy Markdown
Contributor Author

/hold for openshift/installer#10679

@openshift-ci openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 15, 2026
@hasbro17

Copy link
Copy Markdown
Contributor

Reminder for myself to check e2e-aws-ovn-pki-rsa-techpreview to make sure this still passes without the fallback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants