Skip to content

OCPBUGS-78424: [release-4.20] Use resource group when generating default Azure image #1475

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
patrickdillon wants to merge 3 commits into
base: release-4.20
Choose a base branch
from
+41 −8
Open

OCPBUGS-78424: [release-4.20] Use resource group when generating default Azure image #1475

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cc0b7c5
webhooks: add test for azure image with custom resource group
patrickdillon Mar 13, 2026
46458dc
OCPBUGS-78424: use resource group when generating default Azure image
patrickdillon Mar 13, 2026
f203cde
defensively handle empty Azure resource group for default image
patrickdillon Mar 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions pkg/webhooks/machine_webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ var (
defaultAzureNetworkResourceGroup = func(clusterID string) string {
return fmt.Sprintf("%s-rg", clusterID)
}
defaultAzureImageResourceID = func(clusterID string) string {
defaultAzureImageResourceID = func(clusterID, rg string) string {
// image gallery names cannot have dashes
galleryName := strings.Replace(clusterID, "-", "_", -1)
imageName := clusterID
Expand All @@ -70,7 +70,7 @@ var (
// before that change will have a -gen2 image.
imageName = fmt.Sprintf("%s-gen2", clusterID)
}
return fmt.Sprintf("/resourceGroups/%s/providers/Microsoft.Compute/galleries/gallery_%s/images/%s/versions/%s", clusterID+"-rg", galleryName, imageName, azureRHCOSVersion)
return fmt.Sprintf("/resourceGroups/%s/providers/Microsoft.Compute/galleries/gallery_%s/images/%s/versions/%s", rg, galleryName, imageName, azureRHCOSVersion)
}
defaultAzureManagedIdentiy = func(clusterID string) string {
return fmt.Sprintf("%s-identity", clusterID)
Expand Down Expand Up @@ -920,7 +920,11 @@ func defaultAzure(m *machinev1beta1.Machine, config *admissionConfig) (bool, []s
}

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)
Comment on lines 922 to +927
Copy link

@coderabbitai coderabbitai bot Mar 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

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.

}

if providerSpec.UserDataSecret == nil {
Expand Down
37 changes: 33 additions & 4 deletions pkg/webhooks/machine_webhook_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"errors"
"fmt"
"reflect"
"strings"
"testing"

"github.com/openshift/api/features"
Expand Down Expand Up @@ -1494,7 +1495,7 @@ func TestMachineUpdate(t *testing.T) {
Subnet: defaultAzureSubnet(azureClusterID),
NetworkResourceGroup: defaultAzureNetworkResourceGroup(azureClusterID),
Image: machinev1beta1.Image{
ResourceID: defaultAzureImageResourceID(azureClusterID),
ResourceID: defaultAzureImageResourceID(azureClusterID, defaultAzureResourceGroup(azureClusterID)),
},
ManagedIdentity: defaultAzureManagedIdentiy(azureClusterID),
ResourceGroup: defaultAzureResourceGroup(azureClusterID),
Expand Down Expand Up @@ -3399,6 +3400,7 @@ func TestDefaultAzureProviderSpec(t *testing.T) {
testCase string
providerSpec *machinev1beta1.AzureMachineProviderSpec
modifyDefault func(*machinev1beta1.AzureMachineProviderSpec)
platformStatus *osconfigv1.PlatformStatus
expectedError string
expectedOk bool
expectedWarnings []string
Expand Down Expand Up @@ -3448,6 +3450,29 @@ func TestDefaultAzureProviderSpec(t *testing.T) {
expectedError: "",
expectedWarnings: itWarnings,
},
{
testCase: "it generates azure image ResourceID with custom resource group",
providerSpec: &machinev1beta1.AzureMachineProviderSpec{
Image: machinev1beta1.Image{},
},
platformStatus: &osconfigv1.PlatformStatus{
Type: osconfigv1.AzurePlatformType,
Azure: &osconfigv1.AzurePlatformStatus{
ResourceGroupName: "test-rg",
},
},
modifyDefault: func(p *machinev1beta1.AzureMachineProviderSpec) {
// Generate expected image ID with custom resource group from infra config
galleryName := strings.Replace(clusterID, "-", "_", -1)
imageName := clusterID
p.Image = machinev1beta1.Image{
ResourceID: fmt.Sprintf("/resourceGroups/%s/providers/Microsoft.Compute/galleries/gallery_%s/images/%s/versions/%s", "test-rg", galleryName, imageName, azureRHCOSVersion),
}
},
expectedOk: true,
expectedError: "",
expectedWarnings: itWarnings,
},
{
testCase: "does not overwrite the network resource group if it already exists",
providerSpec: &machinev1beta1.AzureMachineProviderSpec{
Expand Down Expand Up @@ -3494,16 +3519,20 @@ func TestDefaultAzureProviderSpec(t *testing.T) {
},
}

platformStatus := &osconfigv1.PlatformStatus{Type: osconfigv1.AzurePlatformType}
h := createMachineDefaulter(platformStatus, clusterID)
defaultPlatformStatus := &osconfigv1.PlatformStatus{Type: osconfigv1.AzurePlatformType}
for _, tc := range testCases {
t.Run(tc.testCase, func(t *testing.T) {
platformStatus := defaultPlatformStatus
if tc.platformStatus != nil {
platformStatus = tc.platformStatus
}
h := createMachineDefaulter(platformStatus, clusterID)
defaultProviderSpec := &machinev1beta1.AzureMachineProviderSpec{
VMSize: defaultInstanceType,
Vnet: defaultAzureVnet(clusterID),
Subnet: defaultAzureSubnet(clusterID),
Image: machinev1beta1.Image{
ResourceID: defaultAzureImageResourceID(clusterID),
ResourceID: defaultAzureImageResourceID(clusterID, defaultAzureResourceGroup(clusterID)),
},
UserDataSecret: &corev1.SecretReference{
Name: defaultUserDataSecret,
Expand Down
2 changes: 1 addition & 1 deletion pkg/webhooks/machineset_webhook_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -603,7 +603,7 @@ func TestMachineSetUpdate(t *testing.T) {
Subnet: defaultAzureSubnet(azureClusterID),
NetworkResourceGroup: defaultAzureNetworkResourceGroup(azureClusterID),
Image: machinev1beta1.Image{
ResourceID: defaultAzureImageResourceID(azureClusterID),
ResourceID: defaultAzureImageResourceID(azureClusterID, defaultAzureResourceGroup(azureClusterID)),
},
ManagedIdentity: defaultAzureManagedIdentiy(azureClusterID),
ResourceGroup: defaultAzureResourceGroup(azureClusterID),
Expand Down