CNTRLPLANE-3959: wire apiserver config through ignition server - #9136
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
area/control-plane-operator
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9136 +/- ##
==========================================
+ Coverage 44.94% 45.00% +0.05%
==========================================
Files 778 778
Lines 97427 97466 +39
==========================================
+ Hits 43790 43863 +73
+ Misses 50616 50565 -51
- Partials 3021 3038 +17
... and 1 file with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
4987511 to
1d0fa97
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change reconciles API server settings from HostedControlPlane configuration into MCS parameters and serializes them as Sequence Diagram(s)sequenceDiagram
participant HostedControlPlane
participant NewMCSParams
participant ReconcileMachineConfigServerConfig
participant MCSConfigMap
participant LocalIgnitionProvider
participant MCCDirectory
HostedControlPlane->>NewMCSParams: provide Spec.Configuration
NewMCSParams->>NewMCSParams: reconcile APIServer configuration
NewMCSParams->>ReconcileMachineConfigServerConfig: provide APIServer
ReconcileMachineConfigServerConfig->>MCSConfigMap: write cluster-apiserver-config.yaml
MCSConfigMap->>LocalIgnitionProvider: provide configuration files
LocalIgnitionProvider->>MCCDirectory: copy cluster-apiserver-config.yaml and image-config.yaml
Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
area/hypershift-operator
![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 `@hypershift-operator/controllers/nodepool/config.go`:
- Around line 405-406: Update the APIServer reconciliation flow around
ReconcileAPIServerConfiguration to capture its returned error, wrap and
propagate it before proceeding to encode or hash the configuration, matching the
established error-handling pattern in NewMCSParams.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 2ee9f8d1-4f49-4bae-b3bf-2571b566d276
📒 Files selected for processing (4)
control-plane-operator/controllers/hostedcontrolplane/mcs/params.gocontrol-plane-operator/controllers/hostedcontrolplane/mcs/reconcile.gohypershift-operator/controllers/nodepool/config.goignition-server/controllers/local_ignitionprovider.go
26da224 to
a219be9
Compare
ricardomaraschini
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@ricardomaraschini: This pull request references CNTRLPLANE-3959 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. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@control-plane-operator/controllers/hostedcontrolplane/mcs/reconcile.go`:
- Around line 40-43: Update the error handling after serialize(p.APIServer) in
the reconciliation flow to wrap the returned error with context identifying that
APIServer serialization failed, while preserving the existing early return
behavior.
In `@hypershift-operator/controllers/nodepool/config_test.go`:
- Line 128: Add a populated APIServer configuration case to the hash-contract
tests around the existing empty APIServer fixture, using a TLS security profile
or audit profile. Assert that the serialized global configuration and computed
hash differ from the empty configuration, covering the NodePool rollout
dependency and preventing regressions.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: b2cf2fe1-b78a-4c37-a51c-1a63b952f114
📒 Files selected for processing (8)
control-plane-operator/controllers/hostedcontrolplane/mcs/params.gocontrol-plane-operator/controllers/hostedcontrolplane/mcs/reconcile.gocontrol-plane-operator/controllers/hostedcontrolplane/mcs/reconcile_test.gohypershift-operator/controllers/nodepool/config.gohypershift-operator/controllers/nodepool/config_test.gohypershift-operator/controllers/nodepool/secret_janitor_test.goignition-server/controllers/local_ignitionprovider.goignition-server/controllers/local_ignitionprovider_test.go
a219be9 to
6dafaea
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ignition-server/controllers/local_ignitionprovider_test.go (1)
768-771: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert copied file contents.
Existence checks pass for empty or stale destination files; compare each destination’s bytes with its source content.
Proposed test improvement
- expectFiles []string + expectFiles map[string]string ... - expectFiles: []string{"cluster-apiserver-config.yaml", "image-config.yaml"}, + expectFiles: map[string]string{ + "cluster-apiserver-config.yaml": "apiserver-config", + "image-config.yaml": "image-config", + }, ... - for _, f := range tt.expectFiles { - _, err := os.Stat(filepath.Join(mccDir, f)) + for f, expectedContent := range tt.expectFiles { + actualContent, err := os.ReadFile(filepath.Join(mccDir, f)) g.Expect(err).NotTo(HaveOccurred(), "expected file %s to exist in mccDir", f) + g.Expect(string(actualContent)).To(Equal(expectedContent)) }As per coding guidelines, “Unit test any code changes and additions.”
🤖 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 `@ignition-server/controllers/local_ignitionprovider_test.go` around lines 768 - 771, Enhance the file verification loop in the relevant test by reading each copied destination file and its corresponding source file, then asserting their byte contents are equal in addition to checking existence. Reuse the existing test fixtures and path symbols so the assertion detects empty or stale copies.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@ignition-server/controllers/local_ignitionprovider_test.go`:
- Around line 768-771: Enhance the file verification loop in the relevant test
by reading each copied destination file and its corresponding source file, then
asserting their byte contents are equal in addition to checking existence. Reuse
the existing test fixtures and path symbols so the assertion detects empty or
stale copies.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 3630976d-da93-4426-b065-e6367cc8351e
📒 Files selected for processing (8)
control-plane-operator/controllers/hostedcontrolplane/mcs/params.gocontrol-plane-operator/controllers/hostedcontrolplane/mcs/reconcile.gocontrol-plane-operator/controllers/hostedcontrolplane/mcs/reconcile_test.gohypershift-operator/controllers/nodepool/config.gohypershift-operator/controllers/nodepool/config_test.gohypershift-operator/controllers/nodepool/secret_janitor_test.goignition-server/controllers/local_ignitionprovider.goignition-server/controllers/local_ignitionprovider_test.go
6dafaea to
08f32ad
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 `@hypershift-operator/controllers/nodepool/config.go`:
- Around line 405-415: Update the global-config hashing flow around
APIServerConfiguration and ReconcileAPIServerConfiguration to append and encode
the API server segment only when hcluster.Spec.Configuration and its APIServer
field are explicitly set. Preserve MCS’s default file generation independently,
and leave the existing reconciliation and encoding behavior unchanged for
configured API server settings.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 54db4e18-0b7c-46b0-bb8d-86dde4d4ecb2
📒 Files selected for processing (8)
control-plane-operator/controllers/hostedcontrolplane/mcs/params.gocontrol-plane-operator/controllers/hostedcontrolplane/mcs/reconcile.gocontrol-plane-operator/controllers/hostedcontrolplane/mcs/reconcile_test.gohypershift-operator/controllers/nodepool/config.gohypershift-operator/controllers/nodepool/config_test.gohypershift-operator/controllers/nodepool/secret_janitor_test.goignition-server/controllers/local_ignitionprovider.goignition-server/controllers/local_ignitionprovider_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- hypershift-operator/controllers/nodepool/secret_janitor_test.go
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
767f936 to
f19c6a3
Compare
ignition server needs to know what is the hosted cluster apiserver config in order to properly set kubelet and cri-o tls settings. we were not wiring this information.
f19c6a3 to
b12b4ca
Compare
|
/verified by @kaleemsiddiqu |
openshift-ci-robot
added
the
verified
|
@kaleemsiddiqu: 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. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: csrwng, ricardomaraschini 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
|
/verified by @csrwng |
|
@csrwng: 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. |
|
/lgtm |
|
Scheduling tests matching the |
|
/retest |
1 similar comment
|
/retest |
|
/override ci/prow/e2e-aws-upgrade-hypershift-operator |
|
@csrwng: Overrode contexts on behalf of csrwng: ci/prow/e2e-aws-upgrade-hypershift-operator 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 kubernetes-sigs/prow repository. |
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/override-sticky ci/prow/e2e-aws-upgrade-hypershift-operator |
|
@csrwng: Overrode contexts on behalf of csrwng: ci/prow/e2e-aws-upgrade-hypershift-operator These overrides will persist across retests on the current HEAD SHA. Pushing a new commit will clear them. Use 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 kubernetes-sigs/prow repository. |
|
@ricardomaraschini: 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. |
6 participants
What this PR does / why we need it:
Ignition server needs to know what is the
HostedClusterAPI Server configuration in order to properly set kubelet and cri-o TLS settings (as per PQC compliance requirement). This PR wires the missing config through themachine-config-serverConfigMap.Important
For versions >= 4.23.0: We are adding the
APIServerconfig to theglobalConfigString. This will, in turn, trigger a rollout across all NodePools on upgrade even if noAPIServerconfig is set (the resulting hash will be different).Checklist:
Summary by CodeRabbit
Summary
New Features
Bug Fixes
Tests