Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
32 changes: 30 additions & 2 deletions hypershift-operator/controllers/nodepool/aws.go
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,14 @@ func resolveAWSAMI(hostedCluster *hyperv1.HostedCluster, nodePool *hyperv1.NodeP
return ami, nil
}
// Default behavior for Linux/RHCOS AMIs
ami, err := defaultNodePoolAMI(region, arch, releaseImage)
if releaseImage == nil {
return "", fmt.Errorf("release image is nil")
}
streamMeta, err := releaseImage.StreamForName("")
if err != nil {
return "", fmt.Errorf("couldn't resolve stream metadata: %w", err)
}
ami, err := defaultNodePoolAMI(region, arch, streamMeta)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
if err != nil {
return "", fmt.Errorf("couldn't discover an AMI for release image: %w", err)
}
Expand Down Expand Up @@ -361,7 +368,28 @@ func (r *NodePoolReconciler) setAWSConditions(_ context.Context, nodePool *hyper
})
} else {
// Default behavior for Linux/RHCOS AMIs
ami, err := defaultNodePoolAMI(hcluster.Spec.Platform.AWS.Region, nodePool.Spec.Arch, releaseImage)
if releaseImage == nil {
SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{
Type: hyperv1.NodePoolValidPlatformImageType,
Status: corev1.ConditionFalse,
Reason: hyperv1.NodePoolValidationFailedReason,
Message: fmt.Sprintf("Release image metadata is nil for release image %q", nodePool.Spec.Release.Image),
ObservedGeneration: nodePool.Generation,
})
return fmt.Errorf("release image is nil")
}
streamMeta, err := releaseImage.StreamForName("")
if err != nil {
SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{
Type: hyperv1.NodePoolValidPlatformImageType,
Status: corev1.ConditionFalse,
Reason: hyperv1.NodePoolValidationFailedReason,
Message: fmt.Sprintf("Couldn't resolve stream metadata for release image %q: %s", nodePool.Spec.Release.Image, err.Error()),
ObservedGeneration: nodePool.Generation,
})
return fmt.Errorf("couldn't resolve stream metadata: %w", err)
}
ami, err := defaultNodePoolAMI(hcluster.Spec.Platform.AWS.Region, nodePool.Spec.Arch, streamMeta)
if err != nil {
SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{
Type: hyperv1.NodePoolValidPlatformImageType,
Expand Down
15 changes: 10 additions & 5 deletions hypershift-operator/controllers/nodepool/azure.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
capiazure "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1"

"github.com/blang/semver"
"github.com/coreos/stream-metadata-go/stream"
)

// dummySSHKey is a base64 encoded dummy SSH public key.
Expand Down Expand Up @@ -81,7 +82,11 @@ func defaultAzureNodePoolImage(nodePool *hyperv1.NodePool, releaseImage *release
}

// Extract marketplace metadata from release payload
azureMarketplace, err := getAzureMarketplaceMetadata(releaseImage, streamArch)
streamMeta, err := releaseImage.StreamForName("")
if err != nil {
return fmt.Errorf("couldn't resolve stream metadata: %w", err)
}
azureMarketplace, err := getAzureMarketplaceMetadata(streamMeta, streamArch)
if err != nil {
return fmt.Errorf("failed to get Azure Marketplace metadata: %w", err)
}
Expand Down Expand Up @@ -119,13 +124,13 @@ func defaultAzureNodePoolImage(nodePool *hyperv1.NodePool, releaseImage *release
return nil
}

// getAzureMarketplaceMetadata extracts Azure Marketplace metadata from the release payload
func getAzureMarketplaceMetadata(releaseImage *releaseinfo.ReleaseImage, arch string) (*azureMarketplaceMetadata, error) {
if releaseImage.StreamMetadata == nil {
// getAzureMarketplaceMetadata extracts Azure Marketplace metadata from stream metadata.
func getAzureMarketplaceMetadata(streamMeta *stream.Stream, arch string) (*azureMarketplaceMetadata, error) {
if streamMeta == nil {
return nil, nil // No stream metadata available
}

archData, foundArch := releaseImage.StreamMetadata.Architectures[arch]
archData, foundArch := streamMeta.Architectures[arch]
if !foundArch {
return nil, fmt.Errorf("architecture %s not found in stream metadata", arch)
}
Expand Down
6 changes: 5 additions & 1 deletion hypershift-operator/controllers/nodepool/gcp.go
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,11 @@ func resolveGCPImage(nodePool *hyperv1.NodePool, releaseImage *releaseinfo.Relea
}

// Resolve image from release metadata
image, err := defaultNodePoolGCPImage(nodePool.Spec.Arch, releaseImage)
streamMeta, err := releaseImage.StreamForName("")
if err != nil {
return "", fmt.Errorf("couldn't resolve stream metadata: %w", err)
}
image, err := DefaultNodePoolGCPImage(nodePool.Spec.Arch, streamMeta)
Comment on lines +171 to +175

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Guard releaseImage before StreamForName to avoid panic.

Line 171 calls a method on releaseImage without a nil check. If release metadata fetch fails upstream and nil is propagated, reconciliation panics instead of returning a controlled error.

Suggested fix
 func resolveGCPImage(nodePool *hyperv1.NodePool, releaseImage *releaseinfo.ReleaseImage) (string, error) {
 	gcpPlatform := nodePool.Spec.Platform.GCP
 
 	// If user specified a custom image, use it
 	if gcpPlatform.Image != "" {
 		return gcpPlatform.Image, nil
 	}
 
 	// Resolve image from release metadata
+	if releaseImage == nil {
+		return "", fmt.Errorf("release image is nil")
+	}
 	streamMeta, err := releaseImage.StreamForName("")
 	if err != nil {
 		return "", fmt.Errorf("couldn't resolve stream metadata: %w", err)
 	}
🤖 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 `@hypershift-operator/controllers/nodepool/gcp.go` around lines 171 - 175, The
code calls releaseImage.StreamForName("") without guarding releaseImage for nil;
update the reconciliation logic to check if releaseImage is nil before calling
StreamForName and return a clear error (or requeue) if it is nil. Specifically,
in the block around releaseImage and the subsequent call to StreamForName and
DefaultNodePoolGCPImage, add a nil-check on releaseImage (and optionally on
streamMeta after StreamForName) and return an informative fmt.Errorf referencing
releaseImage so the caller can handle the failure instead of panicking.

if err != nil {
return "", fmt.Errorf("couldn't discover a GCP image for release image: %w", err)
}
Expand Down
103 changes: 48 additions & 55 deletions hypershift-operator/controllers/nodepool/gcp_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -535,63 +535,55 @@ func TestDefaultNodePoolGCPImage(t *testing.T) {
testCases := []struct {
name string
arch string
releaseImage *releaseinfo.ReleaseImage
streamMeta *stream.Stream
expectedImage string
expectedErr bool
expectedErrMsg string
}{
{
name: "When stream metadata has project and name for amd64, it should construct image path",
arch: hyperv1.ArchitectureAMD64,
releaseImage: &releaseinfo.ReleaseImage{
StreamMetadata: &stream.Stream{
Architectures: map[string]stream.Arch{
"x86_64": {
Images: stream.Images{
Gcp: &stream.GcpImage{
Project: "rhcos-cloud",
Name: "rhcos-9-6-20251023-0-gcp-x86-64",
},
streamMeta: &stream.Stream{
Architectures: map[string]stream.Arch{
"x86_64": {
Images: stream.Images{
Gcp: &stream.GcpImage{
Project: "rhcos-cloud",
Name: "rhcos-9-6-20251023-0-gcp-x86-64",
},
},
},
},
},
expectedImage: "projects/rhcos-cloud/global/images/rhcos-9-6-20251023-0-gcp-x86-64",
expectedErr: false,
},
{
name: "When stream metadata has project and name for arm64, it should construct image path",
arch: hyperv1.ArchitectureARM64,
releaseImage: &releaseinfo.ReleaseImage{
StreamMetadata: &stream.Stream{
Architectures: map[string]stream.Arch{
"aarch64": {
Images: stream.Images{
Gcp: &stream.GcpImage{
Project: "rhcos-cloud",
Name: "rhcos-9-6-20251023-0-gcp-aarch64",
},
streamMeta: &stream.Stream{
Architectures: map[string]stream.Arch{
"aarch64": {
Images: stream.Images{
Gcp: &stream.GcpImage{
Project: "rhcos-cloud",
Name: "rhcos-9-6-20251023-0-gcp-aarch64",
},
},
},
},
},
expectedImage: "projects/rhcos-cloud/global/images/rhcos-9-6-20251023-0-gcp-aarch64",
expectedErr: false,
},
{
name: "When architecture is not found in release metadata, it should return error",
name: "When architecture is not found in stream metadata, it should return error",
arch: "unsupported-arch",
releaseImage: &releaseinfo.ReleaseImage{
StreamMetadata: &stream.Stream{
Architectures: map[string]stream.Arch{
"x86_64": {
Images: stream.Images{
Gcp: &stream.GcpImage{
Project: "rhcos-cloud",
Name: "rhcos-9-6-20251023-0-gcp-x86-64",
},
streamMeta: &stream.Stream{
Architectures: map[string]stream.Arch{
"x86_64": {
Images: stream.Images{
Gcp: &stream.GcpImage{
Project: "rhcos-cloud",
Name: "rhcos-9-6-20251023-0-gcp-x86-64",
},
},
},
Expand All @@ -603,13 +595,11 @@ func TestDefaultNodePoolGCPImage(t *testing.T) {
{
name: "When GCP project and name are empty, it should return error",
arch: hyperv1.ArchitectureAMD64,
releaseImage: &releaseinfo.ReleaseImage{
StreamMetadata: &stream.Stream{
Architectures: map[string]stream.Arch{
"x86_64": {
Images: stream.Images{
Gcp: &stream.GcpImage{},
},
streamMeta: &stream.Stream{
Architectures: map[string]stream.Arch{
"x86_64": {
Images: stream.Images{
Gcp: &stream.GcpImage{},
},
},
},
Expand All @@ -620,14 +610,12 @@ func TestDefaultNodePoolGCPImage(t *testing.T) {
{
name: "When GCP project is empty but name is set, it should return error",
arch: hyperv1.ArchitectureAMD64,
releaseImage: &releaseinfo.ReleaseImage{
StreamMetadata: &stream.Stream{
Architectures: map[string]stream.Arch{
"x86_64": {
Images: stream.Images{
Gcp: &stream.GcpImage{
Name: "rhcos-x86-64",
},
streamMeta: &stream.Stream{
Architectures: map[string]stream.Arch{
"x86_64": {
Images: stream.Images{
Gcp: &stream.GcpImage{
Name: "rhcos-x86-64",
},
},
},
Expand All @@ -639,14 +627,12 @@ func TestDefaultNodePoolGCPImage(t *testing.T) {
{
name: "When GCP name is empty but project is set, it should return error",
arch: hyperv1.ArchitectureAMD64,
releaseImage: &releaseinfo.ReleaseImage{
StreamMetadata: &stream.Stream{
Architectures: map[string]stream.Arch{
"x86_64": {
Images: stream.Images{
Gcp: &stream.GcpImage{
Project: "rhcos-cloud",
},
streamMeta: &stream.Stream{
Architectures: map[string]stream.Arch{
"x86_64": {
Images: stream.Images{
Gcp: &stream.GcpImage{
Project: "rhcos-cloud",
},
},
},
Expand All @@ -655,13 +641,20 @@ func TestDefaultNodePoolGCPImage(t *testing.T) {
expectedErr: true,
expectedErrMsg: "release image metadata has no GCP image for architecture \"amd64\"",
},
{
name: "When stream metadata is nil, it should return error",
arch: hyperv1.ArchitectureAMD64,
streamMeta: nil,
expectedErr: true,
expectedErrMsg: "stream metadata is nil",
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
g := NewWithT(t)

image, err := defaultNodePoolGCPImage(tc.arch, tc.releaseImage)
image, err := DefaultNodePoolGCPImage(tc.arch, tc.streamMeta)

if tc.expectedErr {
g.Expect(err).To(HaveOccurred())
Expand Down
13 changes: 9 additions & 4 deletions hypershift-operator/controllers/nodepool/kubevirt/kubevirt.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (

capikubevirt "sigs.k8s.io/cluster-api-provider-kubevirt/api/v1alpha1"

"github.com/coreos/stream-metadata-go/stream"
jsonpatch "github.com/evanphx/json-patch/v5"
kubevirtv1 "kubevirt.io/api/core/v1"
"kubevirt.io/containerized-data-importer-api/pkg/apis/core/v1beta1"
Expand All @@ -36,15 +37,15 @@ var LocalStorageVolumes = []string{
"hotplug-disks",
}

func defaultImage(nodePoolArch string, releaseImage *releaseinfo.ReleaseImage) (string, string, error) {
func defaultImage(nodePoolArch string, streamMeta *stream.Stream) (string, string, error) {
var archName string
switch nodePoolArch {
case hyperv1.ArchitectureS390X:
archName = hyperv1.ArchitectureS390X
default:
archName = hyperv1.ArchAliases[hyperv1.ArchitectureAMD64]
}
arch, foundArch := releaseImage.StreamMetadata.Architectures[archName]
arch, foundArch := streamMeta.Architectures[archName]

if !foundArch {
return "", "", fmt.Errorf("couldn't find OS metadata for architecture %q", archName)
Expand Down Expand Up @@ -90,9 +91,13 @@ func GetImage(nodePool *hyperv1.NodePool, releaseImage *releaseinfo.ReleaseImage
return newBootImage(imageName, isHTTP), nil
}

imageName, imageHash, err := defaultImage(nodePool.Spec.Arch, releaseImage)
streamMeta, err := releaseImage.StreamForName("")
if err != nil {
return nil, fmt.Errorf("couldn't resolve stream metadata: %w", err)
}
imageName, imageHash, err := defaultImage(nodePool.Spec.Arch, streamMeta)
if err != nil && allowUnsupportedRHCOSVariants(nodePool) {
imageName, imageHash, err = openstack.OpenstackDefaultImage(releaseImage)
imageName, imageHash, err = openstack.OpenstackDefaultImage(streamMeta)
if err != nil {
return nil, err
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1580,7 +1580,7 @@ func TestDefaultImage(t *testing.T) {
if testRI == nil {
testRI = ri
}
img, digest, err := defaultImage(tt.arch, testRI)
img, digest, err := defaultImage(tt.arch, testRI.StreamMetadata)
if tt.expectedError {
if err == nil {
t.Fatalf("expected error but got nil")
Expand Down
22 changes: 10 additions & 12 deletions hypershift-operator/controllers/nodepool/nodepool_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ import (
"sigs.k8s.io/controller-runtime/pkg/reconcile"

"github.com/blang/semver"
"github.com/coreos/stream-metadata-go/stream"
"github.com/pkg/errors"
)

Expand Down Expand Up @@ -736,11 +737,11 @@ func isAutoscalingEnabled(nodePool *hyperv1.NodePool) bool {
return nodePool.Spec.AutoScaling != nil
}

func defaultNodePoolAMI(region string, specifiedArch string, releaseImage *releaseinfo.ReleaseImage) (string, error) {
if releaseImage.StreamMetadata == nil {
return "", fmt.Errorf("release image stream metadata is nil")
func defaultNodePoolAMI(region string, specifiedArch string, streamMeta *stream.Stream) (string, error) {
if streamMeta == nil {
return "", fmt.Errorf("stream metadata is nil")
}
arch, foundArch := releaseImage.StreamMetadata.Architectures[hyperv1.ArchAliases[specifiedArch]]
arch, foundArch := streamMeta.Architectures[hyperv1.ArchAliases[specifiedArch]]
if !foundArch {
return "", fmt.Errorf("couldn't find OS metadata for architecture %q", specifiedArch)
}
Expand All @@ -758,16 +759,13 @@ func defaultNodePoolAMI(region string, specifiedArch string, releaseImage *relea
return regionData.Image, nil
}

// defaultNodePoolGCPImage returns the default GCP image for a given architecture from release metadata.
func defaultNodePoolGCPImage(specifiedArch string, releaseImage *releaseinfo.ReleaseImage) (string, error) {
if releaseImage == nil {
return "", fmt.Errorf("release image is nil, cannot determine GCP image")
}
if releaseImage.StreamMetadata == nil {
return "", fmt.Errorf("release image stream metadata is nil, cannot determine GCP image for architecture %q", specifiedArch)
// DefaultNodePoolGCPImage returns the default GCP image for a given architecture from stream metadata.
func DefaultNodePoolGCPImage(specifiedArch string, streamMeta *stream.Stream) (string, error) {
if streamMeta == nil {
return "", fmt.Errorf("stream metadata is nil, cannot determine GCP image for architecture %q", specifiedArch)
}

arch, foundArch := releaseImage.StreamMetadata.Architectures[hyperv1.ArchAliases[specifiedArch]]
arch, foundArch := streamMeta.Architectures[hyperv1.ArchAliases[specifiedArch]]
if !foundArch {
return "", fmt.Errorf("couldn't find OS metadata for architecture %q", specifiedArch)
}
Expand Down
Loading