diff --git a/hypershift-operator/controllers/nodepool/aws.go b/hypershift-operator/controllers/nodepool/aws.go index df5223ff2a9f..f3cde2061fde 100644 --- a/hypershift-operator/controllers/nodepool/aws.go +++ b/hypershift-operator/controllers/nodepool/aws.go @@ -134,7 +134,8 @@ func resolveAWSAMI(hostedCluster *hyperv1.HostedCluster, nodePool *hyperv1.NodeP return ami, nil } // Default behavior for Linux/RHCOS AMIs - ami, err := defaultNodePoolAMI(region, arch, releaseImage) + // TODO(CNTRLPLANE-3553): resolve streamName via GetRHELStream once osImageStream API field is available + ami, err := defaultNodePoolAMI(region, arch, "", releaseImage) if err != nil { return "", fmt.Errorf("couldn't discover an AMI for release image: %w", err) } @@ -361,7 +362,8 @@ 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) + // TODO(CNTRLPLANE-3553): resolve streamName via GetRHELStream once osImageStream API field is available + ami, err := defaultNodePoolAMI(hcluster.Spec.Platform.AWS.Region, nodePool.Spec.Arch, "", releaseImage) if err != nil { SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{ Type: hyperv1.NodePoolValidPlatformImageType, diff --git a/hypershift-operator/controllers/nodepool/aws_test.go b/hypershift-operator/controllers/nodepool/aws_test.go index 9bec3976807f..fc38aeaab01a 100644 --- a/hypershift-operator/controllers/nodepool/aws_test.go +++ b/hypershift-operator/controllers/nodepool/aws_test.go @@ -1083,6 +1083,138 @@ func TestIsSpotEnabled(t *testing.T) { } } +func TestSetAWSConditions(t *testing.T) { + t.Parallel() + + releaseImageWithStreams := &releaseinfo.ReleaseImage{ + ImageStream: &v1.ImageStream{ + ObjectMeta: metav1.ObjectMeta{Name: "4.17.0"}, + }, + StreamMetadata: &stream.Stream{ + Architectures: map[string]stream.Arch{ + "x86_64": { + Images: stream.Images{ + Aws: &stream.AwsImage{ + Regions: map[string]stream.SingleImage{ + "us-east-1": {Release: "4.17.0", Image: "ami-linux-us-east-1"}, + }, + }, + }, + }, + }, + }, + } + + testCases := []struct { + name string + nodePool *hyperv1.NodePool + hostedCluster *hyperv1.HostedCluster + releaseImage *releaseinfo.ReleaseImage + expectError bool + expectedCondType string + expectedCondValue corev1.ConditionStatus + }{ + { + name: "When Linux nodePool resolves AMI successfully it should set ValidPlatformImage to true", + nodePool: &hyperv1.NodePool{ + Spec: hyperv1.NodePoolSpec{ + Arch: hyperv1.ArchitectureAMD64, + Platform: hyperv1.NodePoolPlatform{Type: hyperv1.AWSPlatform, AWS: &hyperv1.AWSNodePoolPlatform{}}, + }, + }, + hostedCluster: &hyperv1.HostedCluster{ + Spec: hyperv1.HostedClusterSpec{ + Platform: hyperv1.PlatformSpec{AWS: &hyperv1.AWSPlatformSpec{Region: "us-east-1"}}, + }, + Status: hyperv1.HostedClusterStatus{ + Platform: &hyperv1.PlatformStatus{AWS: &hyperv1.AWSPlatformStatus{DefaultWorkerSecurityGroupID: "sg-123"}}, + }, + }, + releaseImage: releaseImageWithStreams, + expectedCondType: string(hyperv1.NodePoolValidPlatformImageType), + expectedCondValue: corev1.ConditionTrue, + }, + { + name: "When stream metadata is nil it should set ValidPlatformImage to false", + nodePool: &hyperv1.NodePool{ + Spec: hyperv1.NodePoolSpec{ + Arch: hyperv1.ArchitectureAMD64, + Platform: hyperv1.NodePoolPlatform{Type: hyperv1.AWSPlatform, AWS: &hyperv1.AWSNodePoolPlatform{}}, + Release: hyperv1.Release{Image: "quay.io/test:4.17"}, + }, + }, + hostedCluster: &hyperv1.HostedCluster{ + Spec: hyperv1.HostedClusterSpec{ + Platform: hyperv1.PlatformSpec{AWS: &hyperv1.AWSPlatformSpec{Region: "us-east-1"}}, + }, + }, + releaseImage: &releaseinfo.ReleaseImage{ + ImageStream: &v1.ImageStream{ObjectMeta: metav1.ObjectMeta{Name: "4.17.0"}}, + StreamMetadata: nil, + }, + expectError: true, + expectedCondType: string(hyperv1.NodePoolValidPlatformImageType), + expectedCondValue: corev1.ConditionFalse, + }, + { + name: "When region has no AMI it should set ValidPlatformImage to false", + nodePool: &hyperv1.NodePool{ + Spec: hyperv1.NodePoolSpec{ + Arch: hyperv1.ArchitectureAMD64, + Platform: hyperv1.NodePoolPlatform{Type: hyperv1.AWSPlatform, AWS: &hyperv1.AWSNodePoolPlatform{}}, + Release: hyperv1.Release{Image: "quay.io/test:4.17"}, + }, + }, + hostedCluster: &hyperv1.HostedCluster{ + Spec: hyperv1.HostedClusterSpec{ + Platform: hyperv1.PlatformSpec{AWS: &hyperv1.AWSPlatformSpec{Region: "ap-nowhere-1"}}, + }, + }, + releaseImage: releaseImageWithStreams, + expectError: true, + expectedCondType: string(hyperv1.NodePoolValidPlatformImageType), + expectedCondValue: corev1.ConditionFalse, + }, + { + name: "When HostedCluster has no AWS platform it should return error", + nodePool: &hyperv1.NodePool{ + Spec: hyperv1.NodePoolSpec{ + Arch: hyperv1.ArchitectureAMD64, + Platform: hyperv1.NodePoolPlatform{Type: hyperv1.AWSPlatform, AWS: &hyperv1.AWSNodePoolPlatform{}}, + }, + }, + hostedCluster: &hyperv1.HostedCluster{ + Spec: hyperv1.HostedClusterSpec{ + Platform: hyperv1.PlatformSpec{}, + }, + }, + releaseImage: releaseImageWithStreams, + expectError: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + r := &NodePoolReconciler{} + err := r.setAWSConditions(t.Context(), tc.nodePool, tc.hostedCluster, "", tc.releaseImage) + if tc.expectError { + g.Expect(err).To(HaveOccurred()) + } else { + g.Expect(err).ToNot(HaveOccurred()) + } + + if tc.expectedCondType != "" { + cond := FindStatusCondition(tc.nodePool.Status.Conditions, tc.expectedCondType) + g.Expect(cond).ToNot(BeNil()) + g.Expect(cond.Status).To(Equal(tc.expectedCondValue)) + } + }) + } +} + func TestResolveAWSAMI(t *testing.T) { releaseImageWithMetadata := &releaseinfo.ReleaseImage{ ImageStream: &v1.ImageStream{ @@ -1091,6 +1223,13 @@ func TestResolveAWSAMI(t *testing.T) { StreamMetadata: &stream.Stream{ Architectures: map[string]stream.Arch{ "x86_64": { + Images: stream.Images{ + Aws: &stream.AwsImage{ + Regions: map[string]stream.SingleImage{ + "us-east-1": {Release: "4.17.0", Image: "ami-linux-us-east-1"}, + }, + }, + }, RHELCoreOSExtensions: &rhcos.Extensions{ AwsWinLi: &rhcos.ReplicatedImage{ Regions: map[string]rhcos.SingleImage{ @@ -1161,6 +1300,22 @@ func TestResolveAWSAMI(t *testing.T) { releaseImage: releaseImageWithMetadata, expectError: true, }, + { + name: "When nodePool has default Linux type it should resolve AMI from stream metadata", + hostedCluster: &hyperv1.HostedCluster{ + Spec: hyperv1.HostedClusterSpec{ + Platform: hyperv1.PlatformSpec{AWS: &hyperv1.AWSPlatformSpec{Region: "us-east-1"}}, + }, + }, + nodePool: &hyperv1.NodePool{ + Spec: hyperv1.NodePoolSpec{ + Arch: hyperv1.ArchitectureAMD64, + Platform: hyperv1.NodePoolPlatform{AWS: &hyperv1.AWSNodePoolPlatform{}}, + }, + }, + releaseImage: releaseImageWithMetadata, + expectedAMI: "ami-linux-us-east-1", + }, { name: "When nodePool has no AMI and default Linux type with nil stream metadata, it should return error", hostedCluster: &hyperv1.HostedCluster{ diff --git a/hypershift-operator/controllers/nodepool/nodepool_controller.go b/hypershift-operator/controllers/nodepool/nodepool_controller.go index fbbd74145794..2a170c1137de 100644 --- a/hypershift-operator/controllers/nodepool/nodepool_controller.go +++ b/hypershift-operator/controllers/nodepool/nodepool_controller.go @@ -736,11 +736,18 @@ 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") +// defaultNodePoolAMI resolves the default AWS AMI for a NodePool from release image stream metadata. +// TODO(CNTRLPLANE-3553): once the osImageStream API field is available, callers should resolve +// streamName via GetRHELStream and pass it here instead of hardcoding "". +func defaultNodePoolAMI(region string, specifiedArch string, streamName string, releaseImage *releaseinfo.ReleaseImage) (string, error) { + if releaseImage == nil { + return "", fmt.Errorf("release image is nil") } - arch, foundArch := releaseImage.StreamMetadata.Architectures[hyperv1.ArchAliases[specifiedArch]] + streamMeta, err := releaseImage.StreamForName(streamName) + if err != nil { + return "", fmt.Errorf("couldn't resolve stream metadata: %w", err) + } + arch, foundArch := streamMeta.Architectures[hyperv1.ArchAliases[specifiedArch]] if !foundArch { return "", fmt.Errorf("couldn't find OS metadata for architecture %q", specifiedArch) } diff --git a/hypershift-operator/controllers/nodepool/nodepool_controller_test.go b/hypershift-operator/controllers/nodepool/nodepool_controller_test.go index f3ece180df0c..4c2e77bd6c37 100644 --- a/hypershift-operator/controllers/nodepool/nodepool_controller_test.go +++ b/hypershift-operator/controllers/nodepool/nodepool_controller_test.go @@ -18,6 +18,7 @@ import ( "github.com/openshift/hypershift/support/k8sutil" "github.com/openshift/hypershift/support/releaseinfo" fakereleaseprovider "github.com/openshift/hypershift/support/releaseinfo/fake" + "github.com/openshift/hypershift/support/releaseinfo/fixtures" "github.com/openshift/hypershift/support/thirdparty/library-go/pkg/image/dockerv1client" "github.com/openshift/hypershift/support/upsert" fakeimagemetadataprovider "github.com/openshift/hypershift/support/util/fakeimagemetadataprovider" @@ -540,80 +541,129 @@ func TestCreateValidGeneratedPayloadCondition(t *testing.T) { func TestDefaultNodePoolAMI(t *testing.T) { t.Parallel() + + basicReleaseImage := &releaseinfo.ReleaseImage{ + StreamMetadata: &stream.Stream{ + Architectures: map[string]stream.Arch{ + "x86_64": { + Images: stream.Images{ + Aws: &stream.AwsImage{ + Regions: map[string]stream.SingleImage{ + "us-east-1": {Release: "4.12.0", Image: "us-east-1-x86_64-image"}, + }, + }, + }, + }, + "aarch64": { + Images: stream.Images{ + Aws: &stream.AwsImage{ + Regions: map[string]stream.SingleImage{ + "us-east-1": {Release: "4.12.0", Image: "us-east-1-aarch64-image"}, + "us-west-1": {Release: "4.12.0", Image: ""}, + }, + }, + }, + }, + }, + }, + } + + defaultStream, osStreams, err := releaseinfo.DeserializeImageMetadata(fixtures.CoreOSBootImagesYAML_5_0) + if err != nil { + t.Fatalf("failed to parse multi-stream fixture: %v", err) + } + multiStreamReleaseImage := &releaseinfo.ReleaseImage{StreamMetadata: defaultStream, OSStreams: osStreams} + testCases := []struct { name string region string specifiedArch string + streamName string releaseImage *releaseinfo.ReleaseImage - image string - err error expectedImage string + expectedErr string }{ + // --- Happy paths --- { - name: "successfully pull amd64 AMI", + name: "When resolving amd64 AMI it should return the correct image", region: "us-east-1", specifiedArch: "amd64", + releaseImage: basicReleaseImage, expectedImage: "us-east-1-x86_64-image", }, { - name: "successfully pull arm64 AMI", + name: "When resolving arm64 AMI it should return the correct image", region: "us-east-1", specifiedArch: "arm64", + releaseImage: basicReleaseImage, expectedImage: "us-east-1-aarch64-image", }, { - name: "fail to pull amd64 AMI because region can't be found", - region: "us-east-2", + name: "When resolving rhel-9 stream it should return the rhel-9 AMI", + region: "us-east-1", specifiedArch: "amd64", - expectedImage: "", + streamName: "rhel-9", + releaseImage: multiStreamReleaseImage, + expectedImage: "ami-06a6b025350ff1e23", }, { - name: "fail to pull arm64 AMI because region can't be found", - region: "us-east-2", + name: "When resolving rhel-10 stream it should return the rhel-10 AMI", + region: "us-east-1", + specifiedArch: "amd64", + streamName: "rhel-10", + releaseImage: multiStreamReleaseImage, + expectedImage: "ami-04b3d999e39d62c5b", + }, + { + name: "When resolving rhel-10 arm64 stream it should return the rhel-10 arm64 AMI", + region: "us-east-1", specifiedArch: "arm64", - expectedImage: "", + streamName: "rhel-10", + releaseImage: multiStreamReleaseImage, + expectedImage: "ami-0d7237e6b04d9a9e1", }, { - name: "fail because architecture can't be found", - region: "us-east-2", - specifiedArch: "arm644", - expectedImage: "", + name: "When using default stream it should return the default AMI", + region: "us-east-1", + specifiedArch: "amd64", + releaseImage: multiStreamReleaseImage, + expectedImage: "ami-06a6b025350ff1e23", }, + // --- Sad paths --- { - name: "fail because architecture can't be found", + name: "When region is not found it should return error", region: "us-east-2", + specifiedArch: "amd64", + releaseImage: basicReleaseImage, + expectedErr: `couldn't find AWS image for region "us-east-2"`, + }, + { + name: "When architecture is not found it should return error", + region: "us-east-1", specifiedArch: "s390x", - expectedImage: "", + releaseImage: basicReleaseImage, + expectedErr: `couldn't find OS metadata for architecture "s390x"`, }, { - name: "fail because no image data is defined", + name: "When image data is empty for region it should return error", region: "us-west-1", specifiedArch: "arm64", - expectedImage: "", + releaseImage: basicReleaseImage, + expectedErr: `release image metadata has no image for region "us-west-1"`, }, { - name: "fail because Aws images is nil", + name: "When stream metadata is nil it should return error", region: "us-east-1", specifiedArch: "amd64", - releaseImage: &releaseinfo.ReleaseImage{ - StreamMetadata: &stream.Stream{ - Architectures: map[string]stream.Arch{ - "x86_64": { - Images: stream.Images{}, - }, - }, - }, - }, - expectedImage: "", + releaseImage: &releaseinfo.ReleaseImage{StreamMetadata: nil}, + expectedErr: "couldn't resolve stream metadata: no default stream metadata available", }, { - name: "fail because stream metadata is nil", + name: "When release image is nil it should return error", region: "us-east-1", specifiedArch: "amd64", - releaseImage: &releaseinfo.ReleaseImage{ - StreamMetadata: nil, - }, - expectedImage: "", + releaseImage: nil, + expectedErr: "release image is nil", }, } @@ -622,52 +672,14 @@ func TestDefaultNodePoolAMI(t *testing.T) { t.Parallel() g := NewWithT(t) - other := []client.Object{ - &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{Name: "pull-secret"}, - Data: map[string][]byte{ - corev1.DockerConfigJsonKey: nil, - }, - }, - } - - client := fake.NewClientBuilder().WithObjects(other...).Build() - releaseProvider := &fakereleaseprovider.FakeReleaseProvider{} - hc := &hyperv1.HostedCluster{ - Spec: hyperv1.HostedClusterSpec{ - PullSecret: corev1.LocalObjectReference{ - Name: "pull-secret", - }, - Release: hyperv1.Release{ - Image: "image-4.12.0", - }, - }, - } - - ctx := t.Context() - if tc.releaseImage == nil { - tc.releaseImage = fakereleaseprovider.GetReleaseImage(ctx, hc, client, releaseProvider) - } - - tc.image, tc.err = defaultNodePoolAMI(tc.region, tc.specifiedArch, tc.releaseImage) - if strings.Contains(tc.name, "successfully") { - g.Expect(tc.image).To(Equal(tc.expectedImage)) - g.Expect(tc.err).To(BeNil()) - } else if strings.Contains(tc.name, "fail to pull") { - g.Expect(tc.image).To(BeEmpty()) - g.Expect(tc.err.Error()).To(Equal("couldn't find AWS image for region \"" + tc.region + "\"")) - } else if strings.Contains(tc.name, "fail because architecture") { - g.Expect(tc.image).To(BeEmpty()) - g.Expect(tc.err.Error()).To(Equal("couldn't find OS metadata for architecture \"" + tc.specifiedArch + "\"")) - } else if strings.Contains(tc.name, "stream metadata is nil") { - g.Expect(tc.image).To(BeEmpty()) - g.Expect(tc.err.Error()).To(Equal("release image stream metadata is nil")) - } else if strings.Contains(tc.name, "Aws images is nil") { - g.Expect(tc.image).To(BeEmpty()) - g.Expect(tc.err.Error()).To(Equal("release image metadata has no AWS images")) + image, err := defaultNodePoolAMI(tc.region, tc.specifiedArch, tc.streamName, tc.releaseImage) + if tc.expectedErr != "" { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(Equal(tc.expectedErr)) + g.Expect(image).To(BeEmpty()) } else { - g.Expect(tc.image).To(BeEmpty()) - g.Expect(tc.err.Error()).To(Equal("release image metadata has no image for region \"" + tc.region + "\"")) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(image).To(Equal(tc.expectedImage)) } }) } diff --git a/hypershift-operator/controllers/nodepool/token.go b/hypershift-operator/controllers/nodepool/token.go index aaff447e9841..4a39caffba7d 100644 --- a/hypershift-operator/controllers/nodepool/token.go +++ b/hypershift-operator/controllers/nodepool/token.go @@ -404,13 +404,14 @@ func (t *Token) reconcileUserDataSecret(log logr.Logger, userDataSecret *corev1. } func setKarpenterAMILabels(log logr.Logger, userDataSecret *corev1.Secret, region string, releaseImage *releaseinfo.ReleaseImage, platform hyperv1.PlatformType) error { + // TODO(CNTRLPLANE-3553): resolve streamName via GetRHELStream once osImageStream API field is available supportedArchitectures, err := karpenterutil.SupportedArchitectures(platform) if err != nil { return fmt.Errorf("failed to get supported architectures: %w", err) } supported := 0 for _, arch := range supportedArchitectures { - ami, err := defaultNodePoolAMI(region, arch, releaseImage) + ami, err := defaultNodePoolAMI(region, arch, "", releaseImage) if err != nil { // skip unavailable architectures gracefully log.Error(err, "failed to get default NodePool AMI for architecture", "architecture", arch)