OCPBUGS-78424: [release-4.20] Use resource group when generating default Azure image#1475
OCPBUGS-78424: [release-4.20] Use resource group when generating default Azure image#1475patrickdillon wants to merge 3 commits intoopenshift:release-4.20from
Conversation
Test that if a custom resource group is set, the default image uses that in the resource id.
Prior to this, the default always assumed the resource group was in the form infraID-rg, but that is not the case for users installing into existing resource groups. Instead, read it off the infrastructure config.
The resource group is required on the infrastructure resource, so it should be safe to always read it. To be completely defensive we can assume the default resource group in the impossible case that resource group is not specified.
openshift-ci-robot
added
jira/severity-critical
|
@patrickdillon: This pull request references Jira Issue OCPBUGS-78424, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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 pull request modifies Azure image resource ID construction to accept an explicit resource group parameter. The Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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.11.3)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment Tip You can disable sequence diagrams in the walkthrough.Disable the |
|
[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 |
|
@patrickdillon: This pull request references Jira Issue OCPBUGS-78424, which is invalid:
Comment 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. |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![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 (1)
pkg/webhooks/machine_webhook_test.go (1)
9-9: Good targeted regression; add the empty-name fallback next to it.Explicitly building the expected
ResourceIDhere is a solid way to avoid a tautological test. I’d add one more case forPlatformStatus.Azure != nilwithResourceGroupName == "", since the defensive fallback path is distinct from the existing default case where Azure status is absent.🧪 Suggested follow-up case
+ { + testCase: "it falls back to the default resource group when platform status resource group is empty", + providerSpec: &machinev1beta1.AzureMachineProviderSpec{ + Image: machinev1beta1.Image{}, + }, + platformStatus: &osconfigv1.PlatformStatus{ + Type: osconfigv1.AzurePlatformType, + Azure: &osconfigv1.AzurePlatformStatus{ + ResourceGroupName: "", + }, + }, + modifyDefault: func(p *machinev1beta1.AzureMachineProviderSpec) { + p.Image = machinev1beta1.Image{ + ResourceID: defaultAzureImageResourceID(clusterID, defaultAzureResourceGroup(clusterID)), + } + }, + expectedOk: true, + expectedError: "", + expectedWarnings: itWarnings, + },Also applies to: 3453-3475
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/webhooks/machine_webhook_test.go` at line 9, Add a targeted test case in pkg/webhooks/machine_webhook_test.go that covers PlatformStatus.Azure != nil with PlatformStatus.Azure.ResourceGroupName == "" to exercise the defensive empty-name fallback path; build the expected ResourceID explicitly (as done in the other cases) and assert equality against the function under test (the same helper that produces ResourceID in the file), ensuring the test is not tautological by hardcoding the expected ResourceID value for the empty-name fallback scenario.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/webhooks/machine_webhook.go`:
- Around line 922-927: The admissionConfig used in createMachineDefaulter is
missing platformStatus which causes a nil dereference when code in
getMachineDefaulterOperation accesses config.platformStatus.Azure; update
createMachineDefaulter to set admissionConfig.platformStatus =
infra.Status.PlatformStatus (same way createMachineValidator does) so
platformStatus is populated before calling getMachineDefaulterOperation and the
Azure branch (providerSpec.Image.ResourceID defaulting) no longer panics.
---
Nitpick comments:
In `@pkg/webhooks/machine_webhook_test.go`:
- Line 9: Add a targeted test case in pkg/webhooks/machine_webhook_test.go that
covers PlatformStatus.Azure != nil with PlatformStatus.Azure.ResourceGroupName
== "" to exercise the defensive empty-name fallback path; build the expected
ResourceID explicitly (as done in the other cases) and assert equality against
the function under test (the same helper that produces ResourceID in the file),
ensuring the test is not tautological by hardcoding the expected ResourceID
value for the empty-name fallback scenario.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 81b79d9e-aa4a-48b3-9186-a671751f0b75
📒 Files selected for processing (3)
pkg/webhooks/machine_webhook.gopkg/webhooks/machine_webhook_test.gopkg/webhooks/machineset_webhook_test.go
| if providerSpec.Image == (machinev1beta1.Image{}) { | ||
| providerSpec.Image.ResourceID = defaultAzureImageResourceID(config.clusterID) | ||
| rg := defaultAzureResourceGroup(config.clusterID) | ||
| if config.platformStatus.Azure != nil && config.platformStatus.Azure.ResourceGroupName != "" { | ||
| rg = config.platformStatus.Azure.ResourceGroupName | ||
| } | ||
| providerSpec.Image.ResourceID = defaultAzureImageResourceID(config.clusterID, rg) |
There was a problem hiding this comment.
Nil pointer dereference: config.platformStatus is not set in createMachineDefaulter.
At line 924, the code accesses config.platformStatus.Azure, but platformStatus is never set in the admissionConfig within createMachineDefaulter (lines 442-448). The platformStatus parameter is only passed to getMachineDefaulterOperation, not stored in the config.
This will cause a panic when the defaulting webhook runs for Azure machines.
Compare with createMachineValidator (lines 394-401), which correctly sets platformStatus: infra.Status.PlatformStatus in the admissionConfig.
🐛 Proposed fix: Set platformStatus in createMachineDefaulter
func createMachineDefaulter(platformStatus *osconfigv1.PlatformStatus, clusterID string) *machineDefaulterHandler {
return &machineDefaulterHandler{
admissionHandler: &admissionHandler{
- admissionConfig: &admissionConfig{clusterID: clusterID},
+ admissionConfig: &admissionConfig{clusterID: clusterID, platformStatus: platformStatus},
webhookOperations: getMachineDefaulterOperation(platformStatus),
},
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/webhooks/machine_webhook.go` around lines 922 - 927, The admissionConfig
used in createMachineDefaulter is missing platformStatus which causes a nil
dereference when code in getMachineDefaulterOperation accesses
config.platformStatus.Azure; update createMachineDefaulter to set
admissionConfig.platformStatus = infra.Status.PlatformStatus (same way
createMachineValidator does) so platformStatus is populated before calling
getMachineDefaulterOperation and the Azure branch (providerSpec.Image.ResourceID
defaulting) no longer panics.
|
@patrickdillon: The following test 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. |
2 participants
Prior to this, the default always assumed the resource group was in the form infraID-rg, but that is not the case for users
installing into existing resource groups. Instead, read it off the infrastructure config.
Now that Azure has moved to marketplace images, hive, vendoring the installer, cannot safely generate an image for all versions, so it is passing an empty value to MAO, depending on its defaults, which uncovered this bug.
#1441 changed the default for 4.21 to marketplace images, so this only affects 4.20 and earlier.
Summary by CodeRabbit