Phase 1 changes for MCS Firstboot Pivot Failure Reporting#6029
Phase 1 changes for MCS Firstboot Pivot Failure Reporting#6029CourtCourt521 wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: CourtCourt521 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 |
WalkthroughThe PR adds support for passing a Machine Config Server (MCS) URL through the system to enable status reporting. A new ChangesMCS URL Annotation Support and Server Contracts
Operator Infrastructure-to-URL Computation
CLI Flags and Manifest Configuration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
🧹 Nitpick comments (3)
cmd/machine-config-server/bootstrap.go (1)
34-34: ⚡ Quick winConsider validating the --mcs-url flag format.
Similar to the start command, the bootstrap command accepts any string value for
--mcs-urlwithout validating it's a well-formed URL. Adding basic URL validation when the flag is non-empty would improve robustness and prevent malformed URLs from being written to node annotations.✨ Suggested validation approach
func runBootstrapCmd(_ *cobra.Command, _ []string) { flag.Set("logtostderr", "true") flag.Parse() + if bootstrapOpts.mcsURL != "" { + if _, err := url.Parse(bootstrapOpts.mcsURL); err != nil { + klog.Exitf("--mcs-url is not a valid URL: %v", err) + } + } + // To help debugging, immediately log version🤖 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 `@cmd/machine-config-server/bootstrap.go` at line 34, The bootstrap command currently accepts any string for bootstrapOpts.mcsURL via bootstrapCmd.PersistentFlags().StringVar; add validation so if bootstrapOpts.mcsURL is non-empty you parse it (e.g., url.Parse) and ensure it has a valid scheme (http or https) and a non-empty host, returning an error from the command (RunE or a PreRun validation) when invalid; update the bootstrap command to perform this check before proceeding to avoid storing malformed URLs in node annotations.pkg/server/server_test.go (1)
188-195: ⚡ Quick winConsider adding explicit assertions for the mcsURL annotation.
The tests now pass a non-empty
mcsURLtogetNodeAnnotation, which should add theMachineConfigServerURLAnnotationKeyto the generated annotations. While the tests validate the overall ignition structure (which includes the annotations file), they don't explicitly assert that the new annotation is present with the expected value.Adding explicit assertions would improve test clarity and make it easier to catch regressions in this specific feature.
💡 Example assertion approach
After decoding the annotations JSON, add:
var annotations map[string]string err = json.Unmarshal([]byte(anno), &annotations) require.NoError(t, err) assert.Equal(t, "https://api-int.test.example.com:22623", annotations[daemonconsts.MachineConfigServerURLAnnotationKey], "MCS URL annotation should be present")Also applies to: 383-390
🤖 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/server/server_test.go` around lines 188 - 195, Add explicit assertions that the annotations returned by getNodeAnnotation include the MachineConfigServerURLAnnotationKey with the expected mcsURL value; after obtaining anno (used before appendFileToIgnition and referenced as mcIgnCfg), unmarshal anno into a map[string]string and assert that annotations[daemonconsts.MachineConfigServerURLAnnotationKey] equals "https://api-int.test.example.com:22623". Apply the same check for the other occurrence around lines 383-390 to ensure both test cases validate the presence and value of the MCS URL annotation.cmd/machine-config-server/start.go (1)
34-34: ⚡ Quick winConsider validating the --mcs-url flag format.
The flag accepts any string value without validating it's a well-formed URL. While the operator typically generates this URL from infrastructure status, manual CLI invocation could provide a malformed URL that would be written to node annotations and cause status reporting failures.
Consider adding basic URL validation when the flag is non-empty to improve robustness.
✨ Suggested validation approach
func runStartCmd(_ *cobra.Command, _ []string) { flag.Set("logtostderr", "true") flag.Parse() + if startOpts.mcsURL != "" { + if _, err := url.Parse(startOpts.mcsURL); err != nil { + klog.Exitf("--mcs-url is not a valid URL: %v", err) + } + } + // Initialize controller-runtime logger before any controller-runtime components are used🤖 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 `@cmd/machine-config-server/start.go` at line 34, Add basic URL validation for the --mcs-url flag value (startOpts.mcsURL) after it's parsed and before it's used in status reporting: if startOpts.mcsURL is non-empty, parse it (e.g., using net/url's Parse or ParseRequestURI) and ensure it has a valid scheme (http/https) and non-empty host; if validation fails return an error (or exit) with a clear message so the process doesn't write a malformed URL to node annotations. Place this check in the command initialization/run logic that executes after startCmd.PersistentFlags().StringVar is parsed so it rejects invalid values early.
🤖 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/operator/bootstrap.go`:
- Around line 150-157: The call to getIgnitionHost can panic because it indexes
APIServerInternalIPs[0] without verifying the slice length; update the
getIgnitionHost function to check that platform-specific APIServerInternalIPs
(or equivalent IP lists) are non-empty before indexing and return a clear error
if empty, and keep the existing error return path in the bootstrap caller (the
call site in bootstrap.go already checks err). Ensure the check covers all
platforms handled inside getIgnitionHost and that the returned error message
identifies the missing internal IP so the bootstrap render path can fail
gracefully instead of panicking.
---
Nitpick comments:
In `@cmd/machine-config-server/bootstrap.go`:
- Line 34: The bootstrap command currently accepts any string for
bootstrapOpts.mcsURL via bootstrapCmd.PersistentFlags().StringVar; add
validation so if bootstrapOpts.mcsURL is non-empty you parse it (e.g.,
url.Parse) and ensure it has a valid scheme (http or https) and a non-empty
host, returning an error from the command (RunE or a PreRun validation) when
invalid; update the bootstrap command to perform this check before proceeding to
avoid storing malformed URLs in node annotations.
In `@cmd/machine-config-server/start.go`:
- Line 34: Add basic URL validation for the --mcs-url flag value
(startOpts.mcsURL) after it's parsed and before it's used in status reporting:
if startOpts.mcsURL is non-empty, parse it (e.g., using net/url's Parse or
ParseRequestURI) and ensure it has a valid scheme (http/https) and non-empty
host; if validation fails return an error (or exit) with a clear message so the
process doesn't write a malformed URL to node annotations. Place this check in
the command initialization/run logic that executes after
startCmd.PersistentFlags().StringVar is parsed so it rejects invalid values
early.
In `@pkg/server/server_test.go`:
- Around line 188-195: Add explicit assertions that the annotations returned by
getNodeAnnotation include the MachineConfigServerURLAnnotationKey with the
expected mcsURL value; after obtaining anno (used before appendFileToIgnition
and referenced as mcIgnCfg), unmarshal anno into a map[string]string and assert
that annotations[daemonconsts.MachineConfigServerURLAnnotationKey] equals
"https://api-int.test.example.com:22623". Apply the same check for the other
occurrence around lines 383-390 to ensure both test cases validate the presence
and value of the MCS URL annotation.
🪄 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: f1f35220-fad5-4aec-94b8-29e7a568fbbc
📒 Files selected for processing (12)
cmd/machine-config-server/bootstrap.gocmd/machine-config-server/start.gomanifests/bootstrap-pod-v2.yamlmanifests/machineconfigserver/daemonset.yamlpkg/daemon/constants/constants.gopkg/operator/bootstrap.gopkg/operator/render.gopkg/operator/sync.gopkg/server/bootstrap_server.gopkg/server/cluster_server.gopkg/server/server.gopkg/server/server_test.go
| ignitionHost, err := getIgnitionHost(&dependencies.Infrastructure.Status) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| mcsURL := fmt.Sprintf("https://%s", ignitionHost) | ||
|
|
||
| config := getRenderConfig("", dependencies.KubeAPIServerServingCA, spec, | ||
| &imgs.RenderConfigImages, dependencies.Infrastructure, nil, nil, "2") | ||
| &imgs.RenderConfigImages, dependencies.Infrastructure, nil, nil, mcsURL, "2") |
There was a problem hiding this comment.
Guard getIgnitionHost against empty platform IP lists before bootstrap render.
Line 150 introduces a new bootstrap-time call path to getIgnitionHost, but that helper indexes APIServerInternalIPs[0] for some platforms without a length check. Empty lists will panic the render path.
Proposed hardening
--- a/pkg/operator/sync.go
+++ b/pkg/operator/sync.go
@@
if infraStatus.PlatformStatus != nil {
switch infraStatus.PlatformStatus.Type {
case configv1.BareMetalPlatformType:
- ignitionHost = net.JoinHostPort(infraStatus.PlatformStatus.BareMetal.APIServerInternalIPs[0], securePortStr)
+ if infraStatus.PlatformStatus.BareMetal != nil && len(infraStatus.PlatformStatus.BareMetal.APIServerInternalIPs) > 0 {
+ ignitionHost = net.JoinHostPort(infraStatus.PlatformStatus.BareMetal.APIServerInternalIPs[0], securePortStr)
+ }
case configv1.OpenStackPlatformType:
- ignitionHost = net.JoinHostPort(infraStatus.PlatformStatus.OpenStack.APIServerInternalIPs[0], securePortStr)
+ if infraStatus.PlatformStatus.OpenStack != nil && len(infraStatus.PlatformStatus.OpenStack.APIServerInternalIPs) > 0 {
+ ignitionHost = net.JoinHostPort(infraStatus.PlatformStatus.OpenStack.APIServerInternalIPs[0], securePortStr)
+ }
case configv1.OvirtPlatformType:
- ignitionHost = net.JoinHostPort(infraStatus.PlatformStatus.Ovirt.APIServerInternalIPs[0], securePortStr)
+ if infraStatus.PlatformStatus.Ovirt != nil && len(infraStatus.PlatformStatus.Ovirt.APIServerInternalIPs) > 0 {
+ ignitionHost = net.JoinHostPort(infraStatus.PlatformStatus.Ovirt.APIServerInternalIPs[0], securePortStr)
+ }🤖 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/operator/bootstrap.go` around lines 150 - 157, The call to
getIgnitionHost can panic because it indexes APIServerInternalIPs[0] without
verifying the slice length; update the getIgnitionHost function to check that
platform-specific APIServerInternalIPs (or equivalent IP lists) are non-empty
before indexing and return a clear error if empty, and keep the existing error
return path in the bootstrap caller (the call site in bootstrap.go already
checks err). Ensure the check covers all platforms handled inside
getIgnitionHost and that the returned error message identifies the missing
internal IP so the bootstrap render path can fail gracefully instead of
panicking.
1 participant
- What I did
Testing not an actual pull request
- How to verify it
Testing not an actual pull request
- Description for the changelog
Testing not an actual pull request
Testing not an actual pull request
Summary by CodeRabbit
New Features
--mcs-urlcommand-line flag to configure the Machine Config Server base URL for status reporting.Chores