diff --git a/api/hypershift/v1beta1/nodepool_types.go b/api/hypershift/v1beta1/nodepool_types.go index c7040ce8dc85..750360bffee5 100644 --- a/api/hypershift/v1beta1/nodepool_types.go +++ b/api/hypershift/v1beta1/nodepool_types.go @@ -101,6 +101,7 @@ type NodePool struct { } // NodePoolSpec is the desired behavior of a NodePool. +// +openshift:validation:FeatureGateAwareXValidation:featureGate=OSStreams,rule="!has(oldSelf.osImageStream) || has(self.osImageStream)",message="osImageStream cannot be removed once set; create a new NodePool instead" // +kubebuilder:validation:XValidation:rule="!has(oldSelf.arch) || has(self.arch)", message="Arch is required once set" // +kubebuilder:validation:XValidation:rule="self.arch != 'arm64' || has(self.platform.aws) || has(self.platform.azure) || has(self.platform.agent) || self.platform.type == 'GCP' || self.platform.type == 'None'", message="Setting Arch to arm64 is only supported for AWS, Azure, Agent, GCP and None" // +kubebuilder:validation:XValidation:rule="!has(self.replicas) || !has(self.autoScaling)", message="Both replicas or autoScaling should not be set" @@ -261,6 +262,13 @@ type NodePoolSpec struct { OSImageStream OSImageStreamReference `json:"osImageStream,omitzero"` } +const ( + // OSImageStreamRHEL9 is the OS image stream name for RHEL 9. + OSImageStreamRHEL9 = "rhel-9" + // OSImageStreamRHEL10 is the OS image stream name for RHEL 10. + OSImageStreamRHEL10 = "rhel-10" +) + // OSImageStreamReference references an OSImageStream by name. type OSImageStreamReference struct { // name is a required reference to an OSImageStream to be used for the pool. diff --git a/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/OSStreams.yaml b/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/OSStreams.yaml index 686feae62fab..9b837d2a6985 100644 --- a/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/OSStreams.yaml +++ b/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/OSStreams.yaml @@ -1528,6 +1528,9 @@ spec: - release type: object x-kubernetes-validations: + - message: osImageStream cannot be removed once set; create a new NodePool + instead + rule: '!has(oldSelf.osImageStream) || has(self.osImageStream)' - message: Arch is required once set rule: '!has(oldSelf.arch) || has(self.arch)' - message: Setting Arch to arm64 is only supported for AWS, Azure, Agent, diff --git a/cmd/install/assets/crds/hypershift-operator/tests/nodepools.hypershift.openshift.io/featuregated.nodepools.osimagestream.testsuite.yaml b/cmd/install/assets/crds/hypershift-operator/tests/nodepools.hypershift.openshift.io/featuregated.nodepools.osimagestream.testsuite.yaml index 5d15bd83cdaf..879d7508cee0 100644 --- a/cmd/install/assets/crds/hypershift-operator/tests/nodepools.hypershift.openshift.io/featuregated.nodepools.osimagestream.testsuite.yaml +++ b/cmd/install/assets/crds/hypershift-operator/tests/nodepools.hypershift.openshift.io/featuregated.nodepools.osimagestream.testsuite.yaml @@ -187,6 +187,55 @@ tests: osImageStream: name: "rhel-10" + - name: When removing osImageStream from an existing NodePool it should fail + initial: | + apiVersion: hypershift.openshift.io/v1beta1 + kind: NodePool + spec: + arch: amd64 + clusterName: some-cluster + management: + autoRepair: false + upgradeType: Replace + release: + image: quay.io/openshift-release-dev/ocp-release:4.17.0-rc.0-x86_64 + replicas: 0 + platform: + aws: + instanceProfile: a-profile + instanceType: m6a.2xlarge + rootVolume: + size: 120 + type: gp3 + subnet: + id: "subnet-any" + type: AWS + osImageStream: + name: "rhel-10" + updated: | + apiVersion: hypershift.openshift.io/v1beta1 + kind: NodePool + spec: + arch: amd64 + clusterName: some-cluster + management: + autoRepair: false + upgradeType: Replace + release: + image: quay.io/openshift-release-dev/ocp-release:4.17.0-rc.0-x86_64 + replicas: 0 + platform: + aws: + instanceProfile: a-profile + instanceType: m6a.2xlarge + rootVolume: + size: 120 + type: gp3 + subnet: + id: "subnet-any" + type: AWS + expectedError: "osImageStream cannot be removed once set; create a new NodePool instead" + - name: When adding osImageStream to an existing NodePool it should succeed initial: | apiVersion: hypershift.openshift.io/v1beta1 diff --git a/cmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/nodepools-CustomNoUpgrade.crd.yaml b/cmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/nodepools-CustomNoUpgrade.crd.yaml index 5b7938038b03..a46fb821ef5d 100644 --- a/cmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/nodepools-CustomNoUpgrade.crd.yaml +++ b/cmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/nodepools-CustomNoUpgrade.crd.yaml @@ -1993,6 +1993,9 @@ spec: - release type: object x-kubernetes-validations: + - message: osImageStream cannot be removed once set; create a new NodePool + instead + rule: '!has(oldSelf.osImageStream) || has(self.osImageStream)' - message: Arch is required once set rule: '!has(oldSelf.arch) || has(self.arch)' - message: Setting Arch to arm64 is only supported for AWS, Azure, Agent, diff --git a/cmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/nodepools-TechPreviewNoUpgrade.crd.yaml b/cmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/nodepools-TechPreviewNoUpgrade.crd.yaml index 4d15644f9fb5..a9419620c9db 100644 --- a/cmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/nodepools-TechPreviewNoUpgrade.crd.yaml +++ b/cmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/nodepools-TechPreviewNoUpgrade.crd.yaml @@ -1993,6 +1993,9 @@ spec: - release type: object x-kubernetes-validations: + - message: osImageStream cannot be removed once set; create a new NodePool + instead + rule: '!has(oldSelf.osImageStream) || has(self.osImageStream)' - message: Arch is required once set rule: '!has(oldSelf.arch) || has(self.arch)' - message: Setting Arch to arm64 is only supported for AWS, Azure, Agent, diff --git a/hypershift-operator/controllers/nodepool/aws.go b/hypershift-operator/controllers/nodepool/aws.go index df5223ff2a9f..807f831cf296 100644 --- a/hypershift-operator/controllers/nodepool/aws.go +++ b/hypershift-operator/controllers/nodepool/aws.go @@ -60,8 +60,8 @@ func isSpotEnabled(nodePool *hyperv1.NodePool) bool { return false } -func awsMachineTemplateSpec(infraName string, hostedCluster *hyperv1.HostedCluster, nodePool *hyperv1.NodePool, defaultSG bool, releaseImage *releaseinfo.ReleaseImage) (*capiaws.AWSMachineTemplateSpec, error) { - ami, err := resolveAWSAMI(hostedCluster, nodePool, releaseImage) +func awsMachineTemplateSpec(infraName string, hostedCluster *hyperv1.HostedCluster, nodePool *hyperv1.NodePool, defaultSG bool, releaseImage *releaseinfo.ReleaseImage, rhelStream string) (*capiaws.AWSMachineTemplateSpec, error) { + ami, err := resolveAWSAMI(hostedCluster, nodePool, releaseImage, rhelStream) if err != nil { return nil, err } @@ -118,7 +118,7 @@ func awsMachineTemplateSpec(infraName string, hostedCluster *hyperv1.HostedClust return awsMachineTemplateSpec, nil } -func resolveAWSAMI(hostedCluster *hyperv1.HostedCluster, nodePool *hyperv1.NodePool, releaseImage *releaseinfo.ReleaseImage) (string, error) { +func resolveAWSAMI(hostedCluster *hyperv1.HostedCluster, nodePool *hyperv1.NodePool, releaseImage *releaseinfo.ReleaseImage, rhelStream string) (string, error) { // TODO: Should the region be included in the NodePool platform information? region := hostedCluster.Spec.Platform.AWS.Region arch := nodePool.Spec.Arch @@ -134,7 +134,7 @@ func resolveAWSAMI(hostedCluster *hyperv1.HostedCluster, nodePool *hyperv1.NodeP return ami, nil } // Default behavior for Linux/RHCOS AMIs - ami, err := defaultNodePoolAMI(region, arch, releaseImage) + ami, err := defaultNodePoolAMI(region, arch, releaseImage, rhelStream) if err != nil { return "", fmt.Errorf("couldn't discover an AMI for release image: %w", err) } @@ -270,7 +270,7 @@ func awsAdditionalTags(nodePool *hyperv1.NodePool, hostedCluster *hyperv1.Hosted } func (c *CAPI) awsMachineTemplate(ctx context.Context, templateNameGenerator func(spec any) (string, error)) (*capiaws.AWSMachineTemplate, error) { - desiredSpec, err := awsMachineTemplateSpec(c.capiClusterName, c.hostedCluster, c.nodePool, c.cpoCapabilities.CreateDefaultAWSSecurityGroup, c.releaseImage) + desiredSpec, err := awsMachineTemplateSpec(c.capiClusterName, c.hostedCluster, c.nodePool, c.cpoCapabilities.CreateDefaultAWSSecurityGroup, c.releaseImage, c.rhelStream) if err != nil { return nil, fmt.Errorf("failed to generate AWSMachineTemplateSpec: %w", err) } @@ -361,7 +361,7 @@ 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) + ami, err := defaultNodePoolAMI(hcluster.Spec.Platform.AWS.Region, nodePool.Spec.Arch, releaseImage, nodePool.Spec.OSImageStream.Name) 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..4439fb65d088 100644 --- a/hypershift-operator/controllers/nodepool/aws_test.go +++ b/hypershift-operator/controllers/nodepool/aws_test.go @@ -298,6 +298,7 @@ func TestAWSMachineTemplateSpec(t *testing.T) { }, true, releaseImage, + "", ) if tc.checkError != nil { tc.checkError(t, err) @@ -1185,7 +1186,7 @@ func TestResolveAWSAMI(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { g := NewWithT(t) - ami, err := resolveAWSAMI(tc.hostedCluster, tc.nodePool, tc.releaseImage) + ami, err := resolveAWSAMI(tc.hostedCluster, tc.nodePool, tc.releaseImage, "") if tc.expectError { g.Expect(err).To(HaveOccurred()) } else { diff --git a/hypershift-operator/controllers/nodepool/capi.go b/hypershift-operator/controllers/nodepool/capi.go index 248c7fc468b0..6bc0483f6a00 100644 --- a/hypershift-operator/controllers/nodepool/capi.go +++ b/hypershift-operator/controllers/nodepool/capi.go @@ -614,6 +614,9 @@ func (c *CAPI) reconcileMachineDeploymentStatus(log logr.Logger, machineDeployme "previous", nodePool.Status.Version, "new", targetVersion) nodePool.Status.Version = targetVersion } + if stream, err := getRHELStream(nodePool, c.releaseImage); err == nil { + nodePool.Status.OSImageStream = hyperv1.OSImageStreamReference{Name: stream} + } if nodePool.Annotations == nil { nodePool.Annotations = make(map[string]string) @@ -1012,6 +1015,9 @@ func (c *CAPI) reconcileMachineSet(ctx context.Context, "previous", nodePool.Status.Version, "new", targetVersion) nodePool.Status.Version = targetVersion } + if stream, err := getRHELStream(nodePool, c.releaseImage); err == nil { + nodePool.Status.OSImageStream = hyperv1.OSImageStreamReference{Name: stream} + } if nodePool.Annotations == nil { nodePool.Annotations = make(map[string]string) diff --git a/hypershift-operator/controllers/nodepool/capi_test.go b/hypershift-operator/controllers/nodepool/capi_test.go index 1fe35dc09f60..47080feacb28 100644 --- a/hypershift-operator/controllers/nodepool/capi_test.go +++ b/hypershift-operator/controllers/nodepool/capi_test.go @@ -2641,6 +2641,7 @@ func TestReconcileMachineDeploymentStatus(t *testing.T) { nodePoolAnnotations map[string]string targetVersion string expectedVersion string + expectedOSImageStream string expectedReplicas int32 expectedConfigAnnotation bool expectedTemplateAnnotation bool @@ -2648,7 +2649,7 @@ func TestReconcileMachineDeploymentStatus(t *testing.T) { expectedReadyConditionSet bool }{ { - name: "When MachineDeployment is complete, it should update nodePool version and annotations", + name: "When MachineDeployment is complete, it should update nodePool version", machineDeployment: &capiv1.MachineDeployment{ ObjectMeta: metav1.ObjectMeta{Generation: 1}, Spec: capiv1.MachineDeploymentSpec{ @@ -2666,6 +2667,7 @@ func TestReconcileMachineDeploymentStatus(t *testing.T) { nodePoolAnnotations: map[string]string{}, targetVersion: "4.17.0", expectedVersion: "4.17.0", + expectedOSImageStream: "", expectedReplicas: 3, expectedConfigAnnotation: true, expectedTemplateAnnotation: true, @@ -2766,6 +2768,10 @@ func TestReconcileMachineDeploymentStatus(t *testing.T) { g.Expect(nodePool.Status.Replicas).To(Equal(tc.expectedReplicas)) g.Expect(nodePool.Status.Version).To(Equal(tc.expectedVersion)) + if tc.expectedOSImageStream != "" { + g.Expect(nodePool.Status.OSImageStream.Name).To(Equal(tc.expectedOSImageStream)) + } + if tc.expectedConfigAnnotation { g.Expect(nodePool.Annotations).To(HaveKey(nodePoolAnnotationCurrentConfig)) } diff --git a/hypershift-operator/controllers/nodepool/conditions.go b/hypershift-operator/controllers/nodepool/conditions.go index cff009e3590e..e47c6bea3484 100644 --- a/hypershift-operator/controllers/nodepool/conditions.go +++ b/hypershift-operator/controllers/nodepool/conditions.go @@ -385,6 +385,18 @@ func (r *NodePoolReconciler) validMachineConfigCondition(ctx context.Context, no }) return &ctrl.Result{}, fmt.Errorf("failed to generate config: %w", err) } + + if err := validateOSImageStream(nodePool); err != nil { + SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{ + Type: hyperv1.NodePoolValidMachineConfigConditionType, + Status: corev1.ConditionFalse, + Reason: hyperv1.NodePoolValidationFailedReason, + Message: err.Error(), + ObservedGeneration: nodePool.Generation, + }) + return &ctrl.Result{}, fmt.Errorf("failed to validate osImageStream: %w", err) + } + SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{ Type: hyperv1.NodePoolValidMachineConfigConditionType, Status: corev1.ConditionTrue, diff --git a/hypershift-operator/controllers/nodepool/config.go b/hypershift-operator/controllers/nodepool/config.go index 8771f45ea83c..76d22ac180af 100644 --- a/hypershift-operator/controllers/nodepool/config.go +++ b/hypershift-operator/controllers/nodepool/config.go @@ -60,6 +60,13 @@ type rolloutConfig struct { // TODO(alberto): consider let haproxyRawConfig be an implementation detail of ConfigGenerator. // For now, it's a required input to keep the haproxy business logic and files outside the scope of this initial refactor. haproxyRawConfig string + // rhelStream is the OS image stream name used for hash computation. + // It is set from spec.osImageStream.Name but normalized: if the explicit + // value matches the version-derived default it is kept empty so that + // setting the default stream does not change the hash. + // Only a non-default stream (e.g. "rhel-10" on a 4.x release) produces + // a non-empty value here and triggers a rollout. + rhelStream string } // NewConfigGenerator is the contract to create a new ConfigGenerator. @@ -77,6 +84,18 @@ func NewConfigGenerator(ctx context.Context, client client.Client, hostedCluster return nil, err } + // Normalize rhelStream for the config hash: when the user explicitly sets + // osImageStream to the version-derived default (e.g. "rhel-9" on a 4.x + // release), treat it as equivalent to "not set" so the hash doesn't change + // and no spurious rollout is triggered. + rhelStream := nodePool.Spec.OSImageStream.Name + if rhelStream != "" { + defaultStream, err := defaultRHELStream(releaseImage) + if err == nil && rhelStream == defaultStream { + rhelStream = "" + } + } + cg := &ConfigGenerator{ Client: client, hostedCluster: hostedCluster, @@ -87,6 +106,7 @@ func NewConfigGenerator(ctx context.Context, client client.Client, hostedCluster pullSecretName: hostedCluster.Spec.PullSecret.Name, globalConfig: globalConfig, haproxyRawConfig: haproxyRawConfig, + rhelStream: rhelStream, }, } @@ -118,7 +138,7 @@ func (cg *ConfigGenerator) CompressedAndEncoded() (*bytes.Buffer, error) { // TODO(alberto): hash the struct directly instead of the string representation field by field. // This is kept like this for now to contain the scope of the refactor and avoid backward compatibility issues. func (cg *ConfigGenerator) Hash() string { - return supportutil.HashSimple(cg.mcoRawConfig + cg.releaseImage.Version() + cg.pullSecretName + cg.additionalTrustBundleName + cg.globalConfig) + return supportutil.HashSimple(cg.mcoRawConfig + cg.releaseImage.Version() + cg.pullSecretName + cg.additionalTrustBundleName + cg.globalConfig + cg.rhelStream) } // HashWithOutVersion is like Hash but doesn't compute the release version. @@ -126,7 +146,7 @@ func (cg *ConfigGenerator) Hash() string { // TODO(alberto): This was left inconsistent in https://github.com/openshift/hypershift/pull/3795/files. It should also contain cg.globalConfig. // This is kept like this for now to contain the scope of the refactor and avoid backward compatibility issues. func (cg *ConfigGenerator) HashWithoutVersion() string { - return supportutil.HashSimple(cg.mcoRawConfig + cg.pullSecretName + cg.additionalTrustBundleName) + return supportutil.HashSimple(cg.mcoRawConfig + cg.pullSecretName + cg.additionalTrustBundleName + cg.rhelStream) } func (cg *ConfigGenerator) Version() string { diff --git a/hypershift-operator/controllers/nodepool/config_test.go b/hypershift-operator/controllers/nodepool/config_test.go index ef9096ab218c..b8d848f4884a 100644 --- a/hypershift-operator/controllers/nodepool/config_test.go +++ b/hypershift-operator/controllers/nodepool/config_test.go @@ -434,6 +434,7 @@ func TestHash(t *testing.T) { pullSecretName string additionalTrustBundleName string globalConfig string + rhelStream string expected string }{ { @@ -490,6 +491,16 @@ func TestHash(t *testing.T) { globalConfig: "different", expected: "e916ddfe", }, + { + name: "When rhelStream is a non-default stream, it should change the hash", + mcoRawConfig: baseCaseMCORawConfig, + releaseVersion: baseCaseReleaseVersion, + pullSecretName: baseCasePullSecretName, + additionalTrustBundleName: baseCaseAdditionalTrustBundleName, + globalConfig: baseCaseGlobalConfig, + rhelStream: "rhel-10", + expected: "2dbbd41b", + }, } for _, tc := range testCases { @@ -508,6 +519,7 @@ func TestHash(t *testing.T) { pullSecretName: tc.pullSecretName, additionalTrustBundleName: tc.additionalTrustBundleName, globalConfig: tc.globalConfig, + rhelStream: tc.rhelStream, releaseImage: releaseImage, }, } @@ -536,6 +548,7 @@ func TestHashWithoutVersion(t *testing.T) { pullSecretName string additionalTrustBundleName string globalConfig string + rhelStream string expected string }{ { @@ -594,6 +607,16 @@ func TestHashWithoutVersion(t *testing.T) { globalConfig: "different", expected: baseCaseHash, }, + { + name: "When rhelStream is a non-default stream, it should change the hash", + mcoRawConfig: baseCaseMCORawConfig, + releaseVersion: baseCaseReleaseVersion, + pullSecretName: baseCasePullSecretName, + additionalTrustBundleName: baseCaseAdditionalTrustBundleName, + globalConfig: baseCaseGlobalConfig, + rhelStream: "rhel-10", + expected: "671fe083", + }, } for _, tc := range testCases { @@ -612,6 +635,7 @@ func TestHashWithoutVersion(t *testing.T) { pullSecretName: tc.pullSecretName, additionalTrustBundleName: tc.additionalTrustBundleName, globalConfig: tc.globalConfig, + rhelStream: tc.rhelStream, releaseImage: releaseImage, }, } diff --git a/hypershift-operator/controllers/nodepool/gcp.go b/hypershift-operator/controllers/nodepool/gcp.go index 50a107b210b4..5768f2c07c87 100644 --- a/hypershift-operator/controllers/nodepool/gcp.go +++ b/hypershift-operator/controllers/nodepool/gcp.go @@ -37,6 +37,7 @@ func (c *CAPI) gcpMachineTemplate(_ context.Context, templateNameGenerator func( hc, nodePool, c.releaseImage, + c.rhelStream, ) if err != nil { return nil, fmt.Errorf("failed to generate GCP machine template spec: %w", err) @@ -81,12 +82,13 @@ func gcpMachineTemplateSpec( hostedCluster *hyperv1.HostedCluster, nodePool *hyperv1.NodePool, releaseImage *releaseinfo.ReleaseImage, + rhelStream string, ) (*capigcp.GCPMachineSpec, error) { gcpPlatform := nodePool.Spec.Platform.GCP hcGCPPlatform := hostedCluster.Spec.Platform.GCP // Resolve image - image, err := resolveGCPImage(nodePool, releaseImage) + image, err := resolveGCPImage(nodePool, releaseImage, rhelStream) if err != nil { return nil, fmt.Errorf("failed to resolve GCP image: %w", err) } @@ -159,7 +161,7 @@ func gcpMachineTemplateSpec( } // resolveGCPImage determines the correct image to use based on NodePool configuration and release info. -func resolveGCPImage(nodePool *hyperv1.NodePool, releaseImage *releaseinfo.ReleaseImage) (string, error) { +func resolveGCPImage(nodePool *hyperv1.NodePool, releaseImage *releaseinfo.ReleaseImage, rhelStream string) (string, error) { gcpPlatform := nodePool.Spec.Platform.GCP // If user specified a custom image, use it @@ -168,7 +170,7 @@ func resolveGCPImage(nodePool *hyperv1.NodePool, releaseImage *releaseinfo.Relea } // Resolve image from release metadata - image, err := defaultNodePoolGCPImage(nodePool.Spec.Arch, releaseImage) + image, err := defaultNodePoolGCPImage(nodePool.Spec.Arch, releaseImage, rhelStream) if err != nil { return "", fmt.Errorf("couldn't discover a GCP image for release image: %w", err) } diff --git a/hypershift-operator/controllers/nodepool/gcp_test.go b/hypershift-operator/controllers/nodepool/gcp_test.go index 14a3623acf82..5e5d50010e27 100644 --- a/hypershift-operator/controllers/nodepool/gcp_test.go +++ b/hypershift-operator/controllers/nodepool/gcp_test.go @@ -514,6 +514,7 @@ func TestGcpMachineTemplateSpec(t *testing.T) { tc.hc, tc.nodePool, releaseImage, + "", ) if tc.expectedErr { @@ -661,7 +662,7 @@ func TestDefaultNodePoolGCPImage(t *testing.T) { t.Run(tc.name, func(t *testing.T) { g := NewWithT(t) - image, err := defaultNodePoolGCPImage(tc.arch, tc.releaseImage) + image, err := defaultNodePoolGCPImage(tc.arch, tc.releaseImage, "") if tc.expectedErr { g.Expect(err).To(HaveOccurred()) diff --git a/hypershift-operator/controllers/nodepool/nodepool_controller.go b/hypershift-operator/controllers/nodepool/nodepool_controller.go index fbbd74145794..6899a264af84 100644 --- a/hypershift-operator/controllers/nodepool/nodepool_controller.go +++ b/hypershift-operator/controllers/nodepool/nodepool_controller.go @@ -403,6 +403,9 @@ func (r *NodePoolReconciler) reconcile(ctx context.Context, hcluster *hyperv1.Ho targetConfigHash := token.HashWithoutVersion() targetPayloadConfigHash := token.Hash() nodePool.Status.Version = releaseImage.Version() + if stream, err := getRHELStream(nodePool, releaseImage); err == nil { + nodePool.Status.OSImageStream = hyperv1.OSImageStreamReference{Name: stream} + } if nodePool.Annotations == nil { nodePool.Annotations = make(map[string]string) } @@ -736,7 +739,9 @@ func isAutoscalingEnabled(nodePool *hyperv1.NodePool) bool { return nodePool.Spec.AutoScaling != nil } -func defaultNodePoolAMI(region string, specifiedArch string, releaseImage *releaseinfo.ReleaseImage) (string, error) { +func defaultNodePoolAMI(region string, specifiedArch string, releaseImage *releaseinfo.ReleaseImage, rhelStream string) (string, error) { //nolint:unparam // rhelStream will be used when multi-stream metadata parsing lands (CNTRLPLANE-3553) + // TODO(CNTRLPLANE-3553): use rhelStream to select stream-specific metadata + // when multi-stream parsing is available. if releaseImage.StreamMetadata == nil { return "", fmt.Errorf("release image stream metadata is nil") } @@ -759,7 +764,9 @@ func defaultNodePoolAMI(region string, specifiedArch string, releaseImage *relea } // defaultNodePoolGCPImage returns the default GCP image for a given architecture from release metadata. -func defaultNodePoolGCPImage(specifiedArch string, releaseImage *releaseinfo.ReleaseImage) (string, error) { +func defaultNodePoolGCPImage(specifiedArch string, releaseImage *releaseinfo.ReleaseImage, rhelStream string) (string, error) { //nolint:unparam // rhelStream will be used when multi-stream metadata parsing lands (CNTRLPLANE-3553) + // TODO(CNTRLPLANE-3553): use rhelStream to select stream-specific metadata + // when multi-stream parsing is available. if releaseImage == nil { return "", fmt.Errorf("release image is nil, cannot determine GCP image") } diff --git a/hypershift-operator/controllers/nodepool/nodepool_controller_test.go b/hypershift-operator/controllers/nodepool/nodepool_controller_test.go index f3ece180df0c..2274882d4b12 100644 --- a/hypershift-operator/controllers/nodepool/nodepool_controller_test.go +++ b/hypershift-operator/controllers/nodepool/nodepool_controller_test.go @@ -649,7 +649,7 @@ func TestDefaultNodePoolAMI(t *testing.T) { tc.releaseImage = fakereleaseprovider.GetReleaseImage(ctx, hc, client, releaseProvider) } - tc.image, tc.err = defaultNodePoolAMI(tc.region, tc.specifiedArch, tc.releaseImage) + 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()) diff --git a/hypershift-operator/controllers/nodepool/osstream.go b/hypershift-operator/controllers/nodepool/osstream.go new file mode 100644 index 000000000000..d7ea28afe202 --- /dev/null +++ b/hypershift-operator/controllers/nodepool/osstream.go @@ -0,0 +1,60 @@ +package nodepool + +import ( + "fmt" + + hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1" + "github.com/openshift/hypershift/support/releaseinfo" + + "github.com/blang/semver" +) + +// defaultRHELStream derives the RHEL stream from the release image version: +// major >= 5 → "rhel-10", otherwise → "rhel-9". +func defaultRHELStream(releaseImage *releaseinfo.ReleaseImage) (string, error) { + version, err := semver.Parse(releaseImage.Version()) + if err != nil { + return "", fmt.Errorf("failed to parse release image version %q: %w", releaseImage.Version(), err) + } + + if version.Major >= 5 { + return hyperv1.OSImageStreamRHEL10, nil + } + return hyperv1.OSImageStreamRHEL9, nil +} + +// getRHELStream returns the effective RHEL stream for the NodePool. +// If spec.osImageStream.Name is set, it returns that directly. +// If unset and the release version is >= 5.0, it returns "rhel-10". +// If unset and the release version is < 5.0, it returns "" (legacy +// behavior: no OSImageStream CR is generated and the MCC uses +// BaseOSContainerImage from ControllerConfig as-is). +func getRHELStream(nodePool *hyperv1.NodePool, releaseImage *releaseinfo.ReleaseImage) (string, error) { + if nodePool.Spec.OSImageStream.Name != "" { + return nodePool.Spec.OSImageStream.Name, nil + } + + version, err := semver.Parse(releaseImage.Version()) + if err != nil { + return "", fmt.Errorf("failed to parse release image version %q: %w", releaseImage.Version(), err) + } + + if version.Major >= 5 { + return hyperv1.OSImageStreamRHEL10, nil + } + // For < 5.0, return empty string: legacy single-stream behavior. + return "", nil +} + +// validateOSImageStream checks that spec.osImageStream.Name, if set, is a +// known stream name. Returns an error describing the problem or nil. +func validateOSImageStream(nodePool *hyperv1.NodePool) error { + name := nodePool.Spec.OSImageStream.Name + if name == "" { + return nil + } + if name != hyperv1.OSImageStreamRHEL9 && name != hyperv1.OSImageStreamRHEL10 { + return fmt.Errorf("unsupported OS image stream %q; must be one of: %s, %s", name, hyperv1.OSImageStreamRHEL9, hyperv1.OSImageStreamRHEL10) + } + return nil +} diff --git a/hypershift-operator/controllers/nodepool/osstream_test.go b/hypershift-operator/controllers/nodepool/osstream_test.go new file mode 100644 index 000000000000..aa5352014d8d --- /dev/null +++ b/hypershift-operator/controllers/nodepool/osstream_test.go @@ -0,0 +1,184 @@ +package nodepool + +import ( + "testing" + + . "github.com/onsi/gomega" + + hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1" + "github.com/openshift/hypershift/support/releaseinfo" + + imageapi "github.com/openshift/api/image/v1" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func Test_getRHELStream(t *testing.T) { + testCases := []struct { + name string + nodePool *hyperv1.NodePool + releaseImage *releaseinfo.ReleaseImage + expectedStream string + expectErr bool + }{ + { + name: "When spec.osImageStream.Name is set, it should return that value", + nodePool: &hyperv1.NodePool{ + Spec: hyperv1.NodePoolSpec{ + OSImageStream: hyperv1.OSImageStreamReference{Name: "rhel-10"}, + }, + }, + releaseImage: &releaseinfo.ReleaseImage{}, + expectedStream: "rhel-10", + }, + { + name: "When spec.osImageStream.Name is empty and version is 4.x, it should return empty string", + nodePool: &hyperv1.NodePool{ + Spec: hyperv1.NodePoolSpec{}, + }, + releaseImage: &releaseinfo.ReleaseImage{ + ImageStream: &imageapi.ImageStream{ObjectMeta: metav1.ObjectMeta{Name: "4.18.0"}}, + }, + expectedStream: "", + }, + { + name: "When spec.osImageStream.Name is empty and version is 5.x, it should return rhel-10", + nodePool: &hyperv1.NodePool{ + Spec: hyperv1.NodePoolSpec{}, + }, + releaseImage: &releaseinfo.ReleaseImage{ + ImageStream: &imageapi.ImageStream{ObjectMeta: metav1.ObjectMeta{Name: "5.0.0"}}, + }, + expectedStream: "rhel-10", + }, + { + name: "When spec.osImageStream.Name is empty and version is 6.x, it should return rhel-10", + nodePool: &hyperv1.NodePool{ + Spec: hyperv1.NodePoolSpec{}, + }, + releaseImage: &releaseinfo.ReleaseImage{ + ImageStream: &imageapi.ImageStream{ObjectMeta: metav1.ObjectMeta{Name: "6.1.0"}}, + }, + expectedStream: "rhel-10", + }, + { + name: "When spec.osImageStream.Name is empty and version is unparsable, it should return an error", + nodePool: &hyperv1.NodePool{ + Spec: hyperv1.NodePoolSpec{}, + }, + releaseImage: &releaseinfo.ReleaseImage{ + ImageStream: &imageapi.ImageStream{ObjectMeta: metav1.ObjectMeta{Name: "not-a-version"}}, + }, + expectErr: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + g := NewWithT(t) + + stream, err := getRHELStream(tc.nodePool, tc.releaseImage) + if tc.expectErr { + g.Expect(err).To(HaveOccurred()) + return + } + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(stream).To(Equal(tc.expectedStream)) + }) + } +} + +func TestDefaultRHELStream(t *testing.T) { + testCases := []struct { + name string + releaseVersion string + expectedStream string + expectErr bool + }{ + { + name: "When version is 4.x, it should return rhel-9", + releaseVersion: "4.18.0", + expectedStream: "rhel-9", + }, + { + name: "When version is 5.x, it should return rhel-10", + releaseVersion: "5.0.0", + expectedStream: "rhel-10", + }, + { + name: "When version is unparsable, it should return an error", + releaseVersion: "not-a-version", + expectErr: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + g := NewWithT(t) + + releaseImage := &releaseinfo.ReleaseImage{ + ImageStream: &imageapi.ImageStream{ObjectMeta: metav1.ObjectMeta{Name: tc.releaseVersion}}, + } + stream, err := defaultRHELStream(releaseImage) + if tc.expectErr { + g.Expect(err).To(HaveOccurred()) + return + } + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(stream).To(Equal(tc.expectedStream)) + }) + } +} + +func TestValidateOSImageStream(t *testing.T) { + testCases := []struct { + name string + nodePool *hyperv1.NodePool + expectErr bool + }{ + { + name: "When osImageStream.Name is empty, it should succeed", + nodePool: &hyperv1.NodePool{ + Spec: hyperv1.NodePoolSpec{}, + }, + }, + { + name: "When osImageStream.Name is rhel-9, it should succeed", + nodePool: &hyperv1.NodePool{ + Spec: hyperv1.NodePoolSpec{ + OSImageStream: hyperv1.OSImageStreamReference{Name: "rhel-9"}, + }, + }, + }, + { + name: "When osImageStream.Name is rhel-10, it should succeed", + nodePool: &hyperv1.NodePool{ + Spec: hyperv1.NodePoolSpec{ + OSImageStream: hyperv1.OSImageStreamReference{Name: "rhel-10"}, + }, + }, + }, + { + name: "When osImageStream.Name is invalid, it should return an error", + nodePool: &hyperv1.NodePool{ + Spec: hyperv1.NodePoolSpec{ + OSImageStream: hyperv1.OSImageStreamReference{Name: "rhel-8"}, + }, + }, + expectErr: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + g := NewWithT(t) + + err := validateOSImageStream(tc.nodePool) + if tc.expectErr { + g.Expect(err).To(HaveOccurred()) + } else { + g.Expect(err).ToNot(HaveOccurred()) + } + }) + } +} diff --git a/hypershift-operator/controllers/nodepool/token.go b/hypershift-operator/controllers/nodepool/token.go index aaff447e9841..18d44365a839 100644 --- a/hypershift-operator/controllers/nodepool/token.go +++ b/hypershift-operator/controllers/nodepool/token.go @@ -45,6 +45,7 @@ const ( TokenSecretAnnotation = "hypershift.openshift.io/ignition-config" TokenSecretIgnitionReachedAnnotation = "hypershift.openshift.io/ignition-reached" TokenSecretNodePoolUpgradeType = "hypershift.openshift.io/node-pool-upgrade-type" + TokenSecretOSStreamKey = "os-stream" ) // Token knows how to create an UUUID token for a unique configGenerator Hash. @@ -354,6 +355,7 @@ func (t *Token) reconcileTokenSecret(tokenSecret *corev1.Secret) error { tokenSecret.Data[TokenSecretPullSecretHashKey] = t.pullSecretHash tokenSecret.Data[TokenSecretAdditionalTrustBundleKey] = t.additionalTrustBundleHash tokenSecret.Data[TokenSecretHCConfigurationHashKey] = t.globalConfigHash + tokenSecret.Data[TokenSecretOSStreamKey] = []byte(t.rhelStream) } // TODO (alberto): Only apply this on creation and change the hash generation to only use triggering upgrade fields. // We let this change to happen inplace now as the tokenSecret and the mcs config use the whole spec.Config for the comparing hash. @@ -381,7 +383,7 @@ func (t *Token) reconcileUserDataSecret(log logr.Logger, userDataSecret *corev1. if karpenterutil.IsKarpenterEnabled(t.hostedCluster.Spec.AutoNode) { npLabels := t.nodePool.GetLabels() if npLabels != nil && npLabels[karpenterutil.ManagedByKarpenterLabel] == "true" { - err := setKarpenterAMILabels(log, userDataSecret, t.hostedCluster.Spec.Platform.AWS.Region, t.releaseImage, t.hostedCluster.Spec.Platform.Type) + err := setKarpenterAMILabels(log, userDataSecret, t.hostedCluster.Spec.Platform.AWS.Region, t.releaseImage, t.hostedCluster.Spec.Platform.Type, t.rhelStream) if err != nil { return err } @@ -403,14 +405,14 @@ func (t *Token) reconcileUserDataSecret(log logr.Logger, userDataSecret *corev1. return nil } -func setKarpenterAMILabels(log logr.Logger, userDataSecret *corev1.Secret, region string, releaseImage *releaseinfo.ReleaseImage, platform hyperv1.PlatformType) error { +func setKarpenterAMILabels(log logr.Logger, userDataSecret *corev1.Secret, region string, releaseImage *releaseinfo.ReleaseImage, platform hyperv1.PlatformType, rhelStream string) error { 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, rhelStream) if err != nil { // skip unavailable architectures gracefully log.Error(err, "failed to get default NodePool AMI for architecture", "architecture", arch) diff --git a/hypershift-operator/controllers/nodepool/token_test.go b/hypershift-operator/controllers/nodepool/token_test.go index 5266d08331cb..6a5d9e8cc5d5 100644 --- a/hypershift-operator/controllers/nodepool/token_test.go +++ b/hypershift-operator/controllers/nodepool/token_test.go @@ -1181,7 +1181,7 @@ func TestSetKarpenterAMILabels(t *testing.T) { if ri == nil { ri = testutils.InitReleaseImageOrDie("test-release") } - err := setKarpenterAMILabels(log, tc.userDataSecret, tc.region, ri, tc.platform) + err := setKarpenterAMILabels(log, tc.userDataSecret, tc.region, ri, tc.platform, "") if tc.expectedError != "" { g.Expect(err).To(HaveOccurred()) g.Expect(err.Error()).To(Equal(tc.expectedError)) diff --git a/vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/nodepool_types.go b/vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/nodepool_types.go index c7040ce8dc85..750360bffee5 100644 --- a/vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/nodepool_types.go +++ b/vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/nodepool_types.go @@ -101,6 +101,7 @@ type NodePool struct { } // NodePoolSpec is the desired behavior of a NodePool. +// +openshift:validation:FeatureGateAwareXValidation:featureGate=OSStreams,rule="!has(oldSelf.osImageStream) || has(self.osImageStream)",message="osImageStream cannot be removed once set; create a new NodePool instead" // +kubebuilder:validation:XValidation:rule="!has(oldSelf.arch) || has(self.arch)", message="Arch is required once set" // +kubebuilder:validation:XValidation:rule="self.arch != 'arm64' || has(self.platform.aws) || has(self.platform.azure) || has(self.platform.agent) || self.platform.type == 'GCP' || self.platform.type == 'None'", message="Setting Arch to arm64 is only supported for AWS, Azure, Agent, GCP and None" // +kubebuilder:validation:XValidation:rule="!has(self.replicas) || !has(self.autoScaling)", message="Both replicas or autoScaling should not be set" @@ -261,6 +262,13 @@ type NodePoolSpec struct { OSImageStream OSImageStreamReference `json:"osImageStream,omitzero"` } +const ( + // OSImageStreamRHEL9 is the OS image stream name for RHEL 9. + OSImageStreamRHEL9 = "rhel-9" + // OSImageStreamRHEL10 is the OS image stream name for RHEL 10. + OSImageStreamRHEL10 = "rhel-10" +) + // OSImageStreamReference references an OSImageStream by name. type OSImageStreamReference struct { // name is a required reference to an OSImageStream to be used for the pool.