diff --git a/manifests/machineconfigcontroller/osimagestream-deletion-guard-validatingadmissionpolicy.yaml b/manifests/machineconfigcontroller/osimagestream-deletion-guard-validatingadmissionpolicy.yaml new file mode 100644 index 0000000000..6737a8dac4 --- /dev/null +++ b/manifests/machineconfigcontroller/osimagestream-deletion-guard-validatingadmissionpolicy.yaml @@ -0,0 +1,19 @@ +apiVersion: admissionregistration.k8s.io/v1 +kind: ValidatingAdmissionPolicy +metadata: + name: "osimagestream-prevent-deletion" +spec: + failurePolicy: Fail + matchConstraints: + matchPolicy: Equivalent + namespaceSelector: {} + objectSelector: {} + resourceRules: + - apiGroups: ["machineconfiguration.openshift.io"] + apiVersions: ["v1alpha1"] + operations: ["DELETE"] + resources: ["osimagestreams"] + scope: "*" + validations: + - expression: "false" + message: "Deletion of OSImageStream resources is not allowed" diff --git a/manifests/machineconfigcontroller/osimagestream-deletion-guard-validatingadmissionpolicybinding.yaml b/manifests/machineconfigcontroller/osimagestream-deletion-guard-validatingadmissionpolicybinding.yaml new file mode 100644 index 0000000000..a198a2295e --- /dev/null +++ b/manifests/machineconfigcontroller/osimagestream-deletion-guard-validatingadmissionpolicybinding.yaml @@ -0,0 +1,7 @@ +apiVersion: admissionregistration.k8s.io/v1 +kind: ValidatingAdmissionPolicyBinding +metadata: + name: "osimagestream-prevent-deletion-binding" +spec: + policyName: "osimagestream-prevent-deletion" + validationActions: [Deny] diff --git a/pkg/controller/bootstrap/bootstrap.go b/pkg/controller/bootstrap/bootstrap.go index 504ce97bb0..517bea6273 100644 --- a/pkg/controller/bootstrap/bootstrap.go +++ b/pkg/controller/bootstrap/bootstrap.go @@ -115,6 +115,7 @@ func (b *Bootstrap) Run(destDir string) error { imageStream *imagev1.ImageStream iri *mcfgv1alpha1.InternalReleaseImage iriTLSCert *corev1.Secret + osImageStream *mcfgv1alpha1.OSImageStream ) for _, info := range infos { if info.IsDir() { @@ -199,6 +200,9 @@ func (b *Bootstrap) Run(destDir string) error { if obj.GetName() == ctrlcommon.InternalReleaseImageTLSSecretName { iriTLSCert = obj } + case *mcfgv1alpha1.OSImageStream: + // If given, it's treated as user input with config such as the default stream + osImageStream = obj default: klog.Infof("skipping %q [%d] manifest because of unhandled %T", file.Name(), idx+1, obji) } @@ -224,10 +228,17 @@ func (b *Bootstrap) Run(destDir string) error { return fmt.Errorf("error filtering pools: %w", err) } - var osImageStream *mcfgv1alpha1.OSImageStream // Enable OSImageStreams if the FeatureGate is active and the deployment is not OKD if osimagestream.IsFeatureEnabled(fgHandler) { - osImageStream, err = b.fetchOSImageStream(imageStream, cconfig, icspRules, idmsRules, itmsRules, imgCfg, pullSecret) + osImageStream, err = b.fetchOSImageStream( + imageStream, + cconfig, + icspRules, + idmsRules, + itmsRules, + imgCfg, + pullSecret, + osImageStream) if err != nil { return err } @@ -240,6 +251,9 @@ func (b *Bootstrap) Run(destDir string) error { } cconfig.Spec.BaseOSContainerImage = string(defaultStreamSet.OSImage) cconfig.Spec.BaseOSExtensionsContainerImage = string(defaultStreamSet.OSExtensionsImage) + } else { + // Just ensuring the osImageStream is nil if the FG is disabled + osImageStream = nil } pullSecretBytes := pullSecret.Data[corev1.DockerConfigJsonKey] @@ -462,6 +476,7 @@ func (b *Bootstrap) fetchOSImageStream( itmsRules []*apicfgv1.ImageTagMirrorSet, imgCfg *apicfgv1.Image, pullSecret *corev1.Secret, + existingOSImageStream *mcfgv1alpha1.OSImageStream, ) (*mcfgv1alpha1.OSImageStream, error) { ctx, cancel := context.WithTimeout(context.Background(), time.Minute) @@ -479,15 +494,25 @@ func (b *Bootstrap) fetchOSImageStream( sysCtxBuilder.WithRegistriesConfig(registriesConfig) } - osImageStream, err := osimagestream.BuildOsImageStreamBootstrap(ctx, - sysCtxBuilder, - imageStream, - &osimagestream.OSImageTuple{ + sysCtx, err := sysCtxBuilder.Build() + if err != nil { + return nil, fmt.Errorf("could not prepare for OSImageStream inspection: %w", err) + } + defer func() { + if err := sysCtx.Cleanup(); err != nil { + klog.Warningf("Unable to clean resources after OSImageStream inspection: %s", err) + } + }() + + factory := osimagestream.NewDefaultStreamSourceFactory(&osimagestream.DefaultImagesInspectorFactory{}) + osImageStream, err := factory.Create(ctx, sysCtx.SysContext, osimagestream.CreateOptions{ + ExistingOSImageStream: existingOSImageStream, + ReleaseImageStream: imageStream, + CliImages: &osimagestream.OSImageTuple{ OSImage: cconfig.Spec.BaseOSContainerImage, OSExtensionsImage: cconfig.Spec.BaseOSExtensionsContainerImage, }, - osimagestream.NewDefaultStreamSourceFactory(nil, &osimagestream.DefaultImagesInspectorFactory{}), - ) + }) if err != nil { return nil, fmt.Errorf("error inspecting available OSImageStreams: %w", err) } diff --git a/pkg/controller/internalreleaseimage/internalreleaseimage_controller.go b/pkg/controller/internalreleaseimage/internalreleaseimage_controller.go index 23a53d671c..fe882d771b 100644 --- a/pkg/controller/internalreleaseimage/internalreleaseimage_controller.go +++ b/pkg/controller/internalreleaseimage/internalreleaseimage_controller.go @@ -389,7 +389,11 @@ func (ctrl *Controller) initializeInternalReleaseImageStatus(iri *mcfgv1alpha1.I klog.V(4).Infof("Initializing status for InternalReleaseImage %s", iri.Name) // Get the release payload image from ClusterVersion - releaseImage, err := osimagestream.GetReleasePayloadImage(ctrl.clusterVersionLister) + clusterVersion, err := osimagestream.GetClusterVersion(ctrl.clusterVersionLister) + if err != nil { + return fmt.Errorf("error getting ClusterVersion for InternalReleaseImage status initialization: %w", err) + } + releaseImage, err := osimagestream.GetReleasePayloadImage(clusterVersion) if err != nil { return fmt.Errorf("error getting Release Image from ClusterVersion for InternalReleaseImage status initialization: %w", err) } diff --git a/pkg/controller/render/render_controller.go b/pkg/controller/render/render_controller.go index 681e73402f..4da40af61c 100644 --- a/pkg/controller/render/render_controller.go +++ b/pkg/controller/render/render_controller.go @@ -150,6 +150,9 @@ func New( if osImageStreamInformer != nil && osimagestream.IsFeatureEnabled(ctrl.fgHandler) { ctrl.osImageStreamLister = osImageStreamInformer.Lister() ctrl.osImageStreamListerSynced = osImageStreamInformer.Informer().HasSynced + osImageStreamInformer.Informer().AddEventHandler( + cache.ResourceEventHandlerFuncs{UpdateFunc: ctrl.updateOSImageStream}, + ) } return ctrl } @@ -283,6 +286,28 @@ func (ctrl *Controller) updateMachineConfig(old, cur interface{}) { } } +func (ctrl *Controller) updateOSImageStream(old, cur interface{}) { + oldOSImageStream := old.(*v1alpha1.OSImageStream) + curOSImageStream := cur.(*v1alpha1.OSImageStream) + + // return if status hasn't changed + // The operator components only reacts to what the OSImageStream + // publish in its status subresource. + if reflect.DeepEqual(oldOSImageStream.Status, curOSImageStream.Status) { + return + } + klog.V(4).Info("OSImageStream updated") + + pList, err := ctrl.mcpLister.List(labels.Everything()) + if err != nil { + klog.Errorf("error listing MCPs for OSImageStream update: %v", err) + } + + for _, p := range pList { + ctrl.enqueueMachineConfigPool(p) + } +} + func (ctrl *Controller) deleteMachineConfig(obj interface{}) { mc, ok := obj.(*mcfgv1.MachineConfig) diff --git a/pkg/operator/operator.go b/pkg/operator/operator.go index d53291fa47..f9888b77e3 100644 --- a/pkg/operator/operator.go +++ b/pkg/operator/operator.go @@ -372,6 +372,8 @@ func New( if osImageStreamInformer != nil && osimagestream.IsFeatureEnabled(optr.fgHandler) { optr.osImageStreamLister = osImageStreamInformer.Lister() optr.osImageStreamListerSynced = osImageStreamInformer.Informer().HasSynced + // TODO: Move this AddEventHandler to the main loop when the FG is removed + osImageStreamInformer.Informer().AddEventHandler(optr.eventHandler()) } if iriInformer != nil { optr.iriLister = iriInformer.Lister() diff --git a/pkg/operator/osimagestream_ocp.go b/pkg/operator/osimagestream_ocp.go index 0da4a99835..f729b92345 100644 --- a/pkg/operator/osimagestream_ocp.go +++ b/pkg/operator/osimagestream_ocp.go @@ -32,27 +32,70 @@ func (optr *Operator) syncOSImageStream(_ *renderConfig, _ *configv1.ClusterOper // This sync runs once per version. Before performing the streams fetching // process, that takes time as it requires inspecting images, ensure this function // needs to build the stream. - existingOSImageStream, updateRequired, err := optr.isOSImageStreamBuildRequired() - if !updateRequired || err != nil { + existingOSImageStream, rebuildRequired, err := optr.isOSImageStreamBuildRequired() + if err != nil { return err } - // If the code reaches this point the OSImageStream CR is not - // present (new cluster) or it's out-dated (cluster update). - // Build the new OSImageStream and push it. - return optr.buildOSImageStream(existingOSImageStream) + if rebuildRequired { + // If the code reaches this point the OSImageStream status containing + // the available OS Image Streams is outdated or doesn't exist. + return optr.buildOSImageStream(existingOSImageStream) + } else if existingOSImageStream != nil { + // We didn't require a rebuild, but check if we need to update the default + return optr.updateOSImageStream(existingOSImageStream) + } + + return err +} + +func (optr *Operator) updateOSImageStream(existingOSImageStream *v1alpha1.OSImageStream) error { + requestedDefault := osimagestream.GetOSImageStreamSpecDefault(existingOSImageStream) + if requestedDefault == "" { + // Nothing to do. Empty requests are ignored + return nil + } + + currentDefault := existingOSImageStream.Status.DefaultStream + if currentDefault != requestedDefault { + if _, err := osimagestream.GetOSImageStreamSetByName(existingOSImageStream, requestedDefault); err != nil { + return fmt.Errorf("error syncing default OSImageStream with OSImageStream %s: %v", requestedDefault, err) + } + + // DeepCopy to avoid mutating the shared informer cache + osImageStream := existingOSImageStream.DeepCopy() + osImageStream.Status.DefaultStream = requestedDefault + if _, err := optr.client. + MachineconfigurationV1alpha1(). + OSImageStreams(). + UpdateStatus(context.TODO(), osImageStream, metav1.UpdateOptions{}); err != nil { + return fmt.Errorf("error updating the default OSImageStream status: %w", err) + } + klog.Infof("OSImageStream default has changed from %s to %s", currentDefault, requestedDefault) + } + return nil } func (optr *Operator) buildOSImageStream(existingOSImageStream *v1alpha1.OSImageStream) error { klog.Info("Starting building of the OSImageStream instance") // Get the release payload image from ClusterVersion - image, err := osimagestream.GetReleasePayloadImage(optr.clusterVersionLister) + clusterVersion, err := osimagestream.GetClusterVersion(optr.clusterVersionLister) + if err != nil { + return fmt.Errorf("error getting cluster version for OSImageStream inspection: %w", err) + } + image, err := osimagestream.GetReleasePayloadImage(clusterVersion) if err != nil { return fmt.Errorf("error getting the Release Image digest from the ClusterVersion for OSImageStream sync: %w", err) } + // The original cluster version is used as a fallback to infer the default stream + installVersion, err := osimagestream.GetInstallVersion(clusterVersion) + if err != nil { + klog.Warningf("Unable to get install version for OSImageStream build: %s", err) + } + // Get the cluster pull secret from well-known location clusterPullSecret, err := optr.kubeClient.CoreV1().Secrets("openshift-config").Get(context.TODO(), "pull-secret", metav1.GetOptions{}) if err != nil { @@ -76,8 +119,26 @@ func (optr *Operator) buildOSImageStream(existingOSImageStream *v1alpha1.OSImage return fmt.Errorf("could not build SysContext for OSImageStream build: %w", err) } - imageStreamFactory := osimagestream.NewDefaultStreamSourceFactory(optr.mcoCmLister, &osimagestream.DefaultImagesInspectorFactory{}) - osImageStream, err := osimagestream.BuildOsImageStreamRuntime(buildCtx, sysCtxBuilder, image, imageStreamFactory) + sysCtx, err := sysCtxBuilder.Build() + if err != nil { + return fmt.Errorf("could not prepare for OSImageStream inspection: %w", err) + } + defer func() { + if err := sysCtx.Cleanup(); err != nil { + klog.Warningf("Unable to clean resources after OSImageStream inspection: %s", err) + } + }() + + factory := osimagestream.NewDefaultStreamSourceFactory(&osimagestream.DefaultImagesInspectorFactory{}) + osImageStream, err := factory.Create( + buildCtx, + sysCtx.SysContext, + osimagestream.CreateOptions{ + ReleaseImage: image, + ConfigMapLister: optr.mcoCmLister, + ExistingOSImageStream: existingOSImageStream, + InstallVersion: installVersion, + }) if err != nil { return fmt.Errorf("error building the OSImageStream: %w", err) } @@ -96,8 +157,10 @@ func (optr *Operator) buildOSImageStream(existingOSImageStream *v1alpha1.OSImage oldVersion := existingOSImageStream.Annotations[ctrlcommon.ReleaseImageVersionAnnotationKey] klog.V(4).Infof("Updating OSImageStream (previous version: %s, new version: %s)", oldVersion, version.Hash) // Update metadata/spec first (mainly for annotations) - existingOSImageStream.ObjectMeta.Annotations = osImageStream.ObjectMeta.Annotations - updateOSImageStream, err = optr.client.MachineconfigurationV1alpha1().OSImageStreams().Update(context.TODO(), existingOSImageStream, metav1.UpdateOptions{}) + // DeepCopy to avoid mutating the shared informer cache + desired := existingOSImageStream.DeepCopy() + desired.ObjectMeta.Annotations = osImageStream.ObjectMeta.Annotations + updateOSImageStream, err = optr.client.MachineconfigurationV1alpha1().OSImageStreams().Update(context.TODO(), desired, metav1.UpdateOptions{}) if err != nil { return fmt.Errorf("error updating the OSImageStream: %w", err) } @@ -179,9 +242,9 @@ func (optr *Operator) isOSImageStreamBuildRequired() (*v1alpha1.OSImageStream, b } // Check if an update is needed - if !osImageStreamRequiresUpdate(existingOSImageStream) { + if !osImageStreamRequiresRebuild(existingOSImageStream) { klog.V(4).Info("OSImageStream is already up-to-date, skipping sync") - return nil, false, nil + return existingOSImageStream, false, nil } return existingOSImageStream, true, nil } @@ -235,9 +298,9 @@ func (optr *Operator) getExistingOSImageStream() (*v1alpha1.OSImageStream, error return osImageStream, nil } -// osImageStreamRequiresUpdate checks if the OSImageStream needs to be created or updated. -// Returns true if osImageStream is nil or if its version annotation doesn't match the current version. -func osImageStreamRequiresUpdate(osImageStream *v1alpha1.OSImageStream) bool { +// osImageStreamRequiresRebuild checks if the OSImageStream needs to be created or updated. +// Returns true if osImageStream is nil, if its version annotation doesn't match the current version. +func osImageStreamRequiresRebuild(osImageStream *v1alpha1.OSImageStream) bool { if osImageStream == nil { return true } diff --git a/pkg/operator/sync.go b/pkg/operator/sync.go index 5a2a23451a..12bd336a1f 100644 --- a/pkg/operator/sync.go +++ b/pkg/operator/sync.go @@ -106,6 +106,8 @@ const ( mccUpdateBootImagesValidatingAdmissionPolicyBindingPath = "manifests/machineconfigcontroller/update-bootimages-validatingadmissionpolicybinding.yaml" mccMachineConfigPoolOSImageStreamValidatingAdmissionPolicyPath = "manifests/machineconfigcontroller/machineconfigpool-osimagestream-reference-validatingadmissionpolicy.yaml" mccMachineConfigPoolOSImageStreamValidatingAdmissionPolicyBindingPath = "manifests/machineconfigcontroller/machineconfigpool-osimagestream-reference-validatingadmissionpolicybinding.yaml" + mccOSImageStreamDeletionGuardValidatingAdmissionPolicyPath = "manifests/machineconfigcontroller/osimagestream-deletion-guard-validatingadmissionpolicy.yaml" + mccOSImageStreamDeletionGuardValidatingAdmissionPolicyBindingPath = "manifests/machineconfigcontroller/osimagestream-deletion-guard-validatingadmissionpolicybinding.yaml" mccIRIDeletionGuardValidatingAdmissionPolicyPath = "manifests/machineconfigcontroller/internalreleaseimage-deletion-guard-validatingadmissionpolicy.yaml" mccIRIDeletionGuardValidatingAdmissionPolicyBindingPath = "manifests/machineconfigcontroller/internalreleaseimage-deletion-guard-validatingadmissionpolicybinding.yaml" mccUpdateBootImagesCPMSValidatingAdmissionPolicyPath = "manifests/machineconfigcontroller/update-bootimages-cpms-validatingadmissionpolicy.yaml" @@ -1207,8 +1209,14 @@ func (optr *Operator) syncMachineConfigController(config *renderConfig, _ *confi // Only deploy OS image stream validating admission policies when the feature is enabled and the cluster isn't OKD if osimagestream.IsFeatureEnabled(optr.fgHandler) { - paths.validatingAdmissionPolicies = append(paths.validatingAdmissionPolicies, mccMachineConfigPoolOSImageStreamValidatingAdmissionPolicyPath) - paths.validatingAdmissionPolicyBindings = append(paths.validatingAdmissionPolicyBindings, mccMachineConfigPoolOSImageStreamValidatingAdmissionPolicyBindingPath) + paths.validatingAdmissionPolicies = append(paths.validatingAdmissionPolicies, + mccMachineConfigPoolOSImageStreamValidatingAdmissionPolicyPath, + mccOSImageStreamDeletionGuardValidatingAdmissionPolicyPath, + ) + paths.validatingAdmissionPolicyBindings = append(paths.validatingAdmissionPolicyBindings, + mccMachineConfigPoolOSImageStreamValidatingAdmissionPolicyBindingPath, + mccOSImageStreamDeletionGuardValidatingAdmissionPolicyBindingPath, + ) } if optr.fgHandler.Enabled(features.FeatureGateManagedBootImagesCPMS) { paths.validatingAdmissionPolicies = append(paths.validatingAdmissionPolicies, mccUpdateBootImagesCPMSValidatingAdmissionPolicyPath) diff --git a/pkg/osimagestream/clusterversion.go b/pkg/osimagestream/clusterversion.go index 06915a5fc3..2886c87220 100644 --- a/pkg/osimagestream/clusterversion.go +++ b/pkg/osimagestream/clusterversion.go @@ -3,6 +3,10 @@ package osimagestream import ( "errors" "fmt" + "slices" + + configv1 "github.com/openshift/api/config/v1" + k8sversion "k8s.io/apimachinery/pkg/util/version" configlisters "github.com/openshift/client-go/config/listers/config/v1" ) @@ -10,15 +14,46 @@ import ( // clusterVersionSingletonName is the well-known name of the cluster-scoped ClusterVersion singleton resource. const clusterVersionSingletonName = "version" -// GetReleasePayloadImage retrieves the release payload image from the ClusterVersion resource. -func GetReleasePayloadImage(lister configlisters.ClusterVersionLister) (string, error) { +// GetClusterVersion retrieves the current ClusterVersion resource. +func GetClusterVersion(lister configlisters.ClusterVersionLister) (*configv1.ClusterVersion, error) { clusterVersion, err := lister.Get(clusterVersionSingletonName) if err != nil { - return "", fmt.Errorf("failed to get ClusterVersion: %w", err) + return nil, fmt.Errorf("failed to get ClusterVersion: %w", err) } + return clusterVersion, nil +} + +// GetReleasePayloadImage retrieves the release payload image from the given ClusterVersion resource. +func GetReleasePayloadImage(clusterVersion *configv1.ClusterVersion) (string, error) { if clusterVersion == nil || clusterVersion.Status.Desired.Image == "" { return "", errors.New("ClusterVersion desired image is not yet available") } // Got it, store the variable and exit return clusterVersion.Status.Desired.Image, nil } + +// GetInstallVersion returns the first known version from the given ClusterVersion history. +func GetInstallVersion(clusterVersion *configv1.ClusterVersion) (*k8sversion.Version, error) { + if clusterVersion == nil { + return nil, errors.New("ClusterVersion cannot be nil") + } + completed := make([]configv1.UpdateHistory, 0, len(clusterVersion.Status.History)) + for _, entry := range clusterVersion.Status.History { + if entry.CompletionTime != nil && entry.State == configv1.CompletedUpdate { + completed = append(completed, entry) + } + } + if len(completed) == 0 { + return nil, errors.New("ClusterVersion has no completed updates in history") + } + + slices.SortFunc(completed, func(a, b configv1.UpdateHistory) int { + return a.CompletionTime.Time.Compare(b.CompletionTime.Time) + }) + + v, err := k8sversion.ParseGeneric(completed[0].Version) + if err != nil { + return nil, fmt.Errorf("failed to parse install version %q: %w", completed[0].Version, err) + } + return v, nil +} diff --git a/pkg/osimagestream/clusterversion_test.go b/pkg/osimagestream/clusterversion_test.go index a8778565aa..70d1fd2356 100644 --- a/pkg/osimagestream/clusterversion_test.go +++ b/pkg/osimagestream/clusterversion_test.go @@ -3,6 +3,7 @@ package osimagestream import ( "testing" + "time" configv1 "github.com/openshift/api/config/v1" fakeconfigclientset "github.com/openshift/client-go/config/clientset/versioned/fake" @@ -12,7 +13,55 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) -func TestGetClusterVersionOperatorImage(t *testing.T) { +func TestGetClusterVersion(t *testing.T) { + tests := []struct { + name string + clusterVersion *configv1.ClusterVersion + errorContains string + }{ + { + name: "success - returns ClusterVersion", + clusterVersion: &configv1.ClusterVersion{ + ObjectMeta: metav1.ObjectMeta{Name: "version"}, + }, + }, + { + name: "error - ClusterVersion does not exist", + errorContains: "failed to get ClusterVersion", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var fakeClient *fakeconfigclientset.Clientset + if tt.clusterVersion != nil { + fakeClient = fakeconfigclientset.NewSimpleClientset(tt.clusterVersion) + } else { + fakeClient = fakeconfigclientset.NewSimpleClientset() + } + + informerFactory := configinformers.NewSharedInformerFactory(fakeClient, 0) + clusterVersionInformer := informerFactory.Config().V1().ClusterVersions() + + if tt.clusterVersion != nil { + err := clusterVersionInformer.Informer().GetIndexer().Add(tt.clusterVersion) + require.NoError(t, err) + } + + cv, err := GetClusterVersion(clusterVersionInformer.Lister()) + if tt.errorContains != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.errorContains) + return + } + + require.NoError(t, err) + assert.Equal(t, tt.clusterVersion, cv) + }) + } +} + +func TestGetReleasePayloadImage(t *testing.T) { tests := []struct { name string clusterVersion *configv1.ClusterVersion @@ -22,9 +71,7 @@ func TestGetClusterVersionOperatorImage(t *testing.T) { { name: "success - returns image from ClusterVersion", clusterVersion: &configv1.ClusterVersion{ - ObjectMeta: metav1.ObjectMeta{ - Name: "version", - }, + ObjectMeta: metav1.ObjectMeta{Name: "version"}, Status: configv1.ClusterVersionStatus{ Desired: configv1.Release{ Image: "quay.io/openshift-release-dev/ocp-release@sha256:abc123", @@ -34,19 +81,15 @@ func TestGetClusterVersionOperatorImage(t *testing.T) { expectedImage: "quay.io/openshift-release-dev/ocp-release@sha256:abc123", }, { - name: "error - ClusterVersion does not exist", - errorContains: "not found", + name: "error - nil ClusterVersion", + errorContains: "ClusterVersion desired image is not yet available", }, { name: "error - ClusterVersion has empty desired image", clusterVersion: &configv1.ClusterVersion{ - ObjectMeta: metav1.ObjectMeta{ - Name: "version", - }, + ObjectMeta: metav1.ObjectMeta{Name: "version"}, Status: configv1.ClusterVersionStatus{ - Desired: configv1.Release{ - Image: "", - }, + Desired: configv1.Release{Image: ""}, }, }, errorContains: "ClusterVersion desired image is not yet available", @@ -55,32 +98,127 @@ func TestGetClusterVersionOperatorImage(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - var fakeClient *fakeconfigclientset.Clientset - if tt.clusterVersion != nil { - fakeClient = fakeconfigclientset.NewSimpleClientset(tt.clusterVersion) - } else { - fakeClient = fakeconfigclientset.NewSimpleClientset() + image, err := GetReleasePayloadImage(tt.clusterVersion) + if tt.errorContains != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.errorContains) + assert.Empty(t, image) + return } - informerFactory := configinformers.NewSharedInformerFactory(fakeClient, 0) - clusterVersionInformer := informerFactory.Config().V1().ClusterVersions() + require.NoError(t, err) + assert.Equal(t, tt.expectedImage, image) + }) + } +} - if tt.clusterVersion != nil { - err := clusterVersionInformer.Informer().GetIndexer().Add(tt.clusterVersion) - require.NoError(t, err) - } +func TestGetInstallVersion(t *testing.T) { + t0 := metav1.NewTime(time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC)) + t1 := metav1.NewTime(time.Date(2024, 6, 1, 0, 0, 0, 0, time.UTC)) + t2 := metav1.NewTime(time.Date(2025, 1, 1, 0, 0, 0, 0, time.UTC)) - image, err := GetReleasePayloadImage(clusterVersionInformer.Lister()) + tests := []struct { + name string + clusterVersion *configv1.ClusterVersion + expectedMajor uint + expectedMinor uint + errorContains string + }{ + { + name: "single completed entry returns that version", + clusterVersion: &configv1.ClusterVersion{ + ObjectMeta: metav1.ObjectMeta{Name: "version"}, + Status: configv1.ClusterVersionStatus{ + History: []configv1.UpdateHistory{ + {State: configv1.CompletedUpdate, Version: "4.16.0", CompletionTime: &t0}, + }, + }, + }, + expectedMajor: 4, + expectedMinor: 16, + }, + { + name: "multiple entries returns the oldest completed", + clusterVersion: &configv1.ClusterVersion{ + ObjectMeta: metav1.ObjectMeta{Name: "version"}, + Status: configv1.ClusterVersionStatus{ + History: []configv1.UpdateHistory{ + {State: configv1.CompletedUpdate, Version: "5.0.0", CompletionTime: &t2}, + {State: configv1.CompletedUpdate, Version: "4.18.0", CompletionTime: &t1}, + {State: configv1.CompletedUpdate, Version: "4.16.0", CompletionTime: &t0}, + }, + }, + }, + expectedMajor: 4, + expectedMinor: 16, + }, + { + name: "unsorted history returns the oldest completed by completion time", + clusterVersion: &configv1.ClusterVersion{ + ObjectMeta: metav1.ObjectMeta{Name: "version"}, + Status: configv1.ClusterVersionStatus{ + History: []configv1.UpdateHistory{ + {State: configv1.CompletedUpdate, Version: "4.18.0", CompletionTime: &t1}, + {State: configv1.CompletedUpdate, Version: "5.0.0", CompletionTime: &t2}, + {State: configv1.CompletedUpdate, Version: "4.16.0", CompletionTime: &t0}, + }, + }, + }, + expectedMajor: 4, + expectedMinor: 16, + }, + { + name: "in-progress entries are ignored", + clusterVersion: &configv1.ClusterVersion{ + ObjectMeta: metav1.ObjectMeta{Name: "version"}, + Status: configv1.ClusterVersionStatus{ + History: []configv1.UpdateHistory{ + {State: configv1.PartialUpdate, Version: "5.0.0", CompletionTime: nil}, + {State: configv1.CompletedUpdate, Version: "4.18.0", CompletionTime: &t1}, + {State: configv1.CompletedUpdate, Version: "4.16.0", CompletionTime: &t0}, + }, + }, + }, + expectedMajor: 4, + expectedMinor: 16, + }, + { + name: "no completed entries returns error", + clusterVersion: &configv1.ClusterVersion{ + ObjectMeta: metav1.ObjectMeta{Name: "version"}, + Status: configv1.ClusterVersionStatus{ + History: []configv1.UpdateHistory{ + {State: configv1.PartialUpdate, Version: "4.16.0", CompletionTime: nil}, + }, + }, + }, + errorContains: "no completed updates", + }, + { + name: "empty history returns error", + clusterVersion: &configv1.ClusterVersion{ + ObjectMeta: metav1.ObjectMeta{Name: "version"}, + Status: configv1.ClusterVersionStatus{}, + }, + errorContains: "no completed updates", + }, + { + name: "nil ClusterVersion returns error", + errorContains: "ClusterVersion cannot be nil", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + v, err := GetInstallVersion(tt.clusterVersion) if tt.errorContains != "" { require.Error(t, err) assert.Contains(t, err.Error(), tt.errorContains) - assert.Empty(t, image) return } - require.NoError(t, err) - assert.Equal(t, tt.expectedImage, image) + assert.Equal(t, tt.expectedMajor, v.Major()) + assert.Equal(t, tt.expectedMinor, v.Minor()) }) } } diff --git a/pkg/osimagestream/helpers.go b/pkg/osimagestream/helpers.go index 107a462138..67cbdd0606 100644 --- a/pkg/osimagestream/helpers.go +++ b/pkg/osimagestream/helpers.go @@ -43,3 +43,11 @@ func GetOSImageStreamSetByName(osImageStream *v1alpha1.OSImageStream, name strin func IsFeatureEnabled(fgHandler common.FeatureGatesHandler) bool { return fgHandler.Enabled(features.FeatureGateOSStreams) && !version.IsSCOS() && !version.IsFCOS() } + +// GetOSImageStreamSpecDefault returns the user-requested default stream override, or empty string if not set. +func GetOSImageStreamSpecDefault(osImageStream *v1alpha1.OSImageStream) string { + if osImageStream != nil && osImageStream.Spec != nil { + return osImageStream.Spec.DefaultStream + } + return "" +} diff --git a/pkg/osimagestream/helpers_test.go b/pkg/osimagestream/helpers_test.go index 673500d664..a8d4d1b813 100644 --- a/pkg/osimagestream/helpers_test.go +++ b/pkg/osimagestream/helpers_test.go @@ -125,3 +125,42 @@ func TestGetOSImageStreamSetByName(t *testing.T) { }) } } + +func TestGetOSImageStreamSpecDefault(t *testing.T) { + tests := []struct { + name string + input *v1alpha1.OSImageStream + expected string + }{ + { + name: "nil OSImageStream", + input: nil, + expected: "", + }, + { + name: "nil Spec", + input: &v1alpha1.OSImageStream{}, + expected: "", + }, + { + name: "empty DefaultStream", + input: &v1alpha1.OSImageStream{ + Spec: &v1alpha1.OSImageStreamSpec{}, + }, + expected: "", + }, + { + name: "DefaultStream set", + input: &v1alpha1.OSImageStream{ + Spec: &v1alpha1.OSImageStreamSpec{DefaultStream: "rhel-10"}, + }, + expected: "rhel-10", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.expected, GetOSImageStreamSpecDefault(tt.input)) + }) + } +} diff --git a/pkg/osimagestream/imagestream_provider.go b/pkg/osimagestream/imagestream_provider.go index ba1e37fc02..08b620899b 100644 --- a/pkg/osimagestream/imagestream_provider.go +++ b/pkg/osimagestream/imagestream_provider.go @@ -3,6 +3,7 @@ package osimagestream import ( "context" "fmt" + "sync" imagev1 "github.com/openshift/api/image/v1" "github.com/openshift/client-go/image/clientset/versioned/scheme" @@ -37,6 +38,8 @@ func (i *ImageStreamProviderResource) ReadImageStream(_ context.Context) (*image type ImageStreamProviderNetwork struct { imagesInspector ImagesInspector imageName string + cacheLock sync.Mutex + imageStream *imagev1.ImageStream } // NewImageStreamProviderNetwork creates a new ImageStreamProviderNetwork that will fetch @@ -46,14 +49,27 @@ func NewImageStreamProviderNetwork(imagesInspector ImagesInspector, imageName st } // ReadImageStream fetches the ImageStream manifest from the container image and decodes it. -// Returns an error if the file cannot be fetched, is empty, contains invalid YAML, -// or does not contain an ImageStream resource. +// The result is cached on success; failures are retried on subsequent calls. func (i *ImageStreamProviderNetwork) ReadImageStream(ctx context.Context) (*imagev1.ImageStream, error) { + i.cacheLock.Lock() + defer i.cacheLock.Unlock() + if i.imageStream != nil { + return i.imageStream, nil + } + is, err := i.fetchImageStream(ctx) + if err != nil { + return nil, err + } + i.imageStream = is + return is, nil +} + +func (i *ImageStreamProviderNetwork) fetchImageStream(ctx context.Context) (*imagev1.ImageStream, error) { imageStreamBytes, err := i.imagesInspector.FetchImageFile(ctx, i.imageName, releaseImageStreamLocation) if err != nil { return nil, err } - if imageStreamBytes == nil || len(imageStreamBytes) == 0 { + if len(imageStreamBytes) == 0 { return nil, fmt.Errorf("no ImageStream found for %s", i.imageName) } diff --git a/pkg/osimagestream/imagestream_provider_test.go b/pkg/osimagestream/imagestream_provider_test.go index c5c7e56fd6..108410df54 100644 --- a/pkg/osimagestream/imagestream_provider_test.go +++ b/pkg/osimagestream/imagestream_provider_test.go @@ -20,6 +20,7 @@ const ( testReleaseName = "quay.io/openshift/release:4.15.0" ) + func TestImageStreamProviderResource_ReadImageStream(t *testing.T) { imageStream := &imagev1.ImageStream{ ObjectMeta: metav1.ObjectMeta{ @@ -138,3 +139,65 @@ func TestImageStreamProviderNetwork_ReadImageStream(t *testing.T) { }) } } + +func TestImageStreamProviderNetwork_CachesOnSuccess(t *testing.T) { + validImageStream := &imagev1.ImageStream{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "image.openshift.io/v1", + Kind: "ImageStream", + }, + ObjectMeta: metav1.ObjectMeta{Name: "test-stream"}, + } + validBytes, err := runtime.Encode(scheme.Codecs.LegacyCodec(imagev1.SchemeGroupVersion), validImageStream) + require.NoError(t, err) + + inspector := &mockImagesInspector{fetchData: validBytes} + provider := NewImageStreamProviderNetwork(inspector, testReleaseName) + ctx := context.Background() + + first, err := provider.ReadImageStream(ctx) + require.NoError(t, err) + assert.Equal(t, "test-stream", first.Name) + assert.Equal(t, 1, inspector.fetchCount) + + second, err := provider.ReadImageStream(ctx) + require.NoError(t, err) + assert.Same(t, first, second, "second call should return the cached pointer") + assert.Equal(t, 1, inspector.fetchCount, "should not fetch again after a successful call") +} + +func TestImageStreamProviderNetwork_RetriesOnFailure(t *testing.T) { + validImageStream := &imagev1.ImageStream{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "image.openshift.io/v1", + Kind: "ImageStream", + }, + ObjectMeta: metav1.ObjectMeta{Name: "test-stream"}, + } + validBytes, err := runtime.Encode(scheme.Codecs.LegacyCodec(imagev1.SchemeGroupVersion), validImageStream) + require.NoError(t, err) + + inspector := &mockImagesInspector{fetchErr: errors.New("network error")} + provider := NewImageStreamProviderNetwork(inspector, testReleaseName) + ctx := context.Background() + + // First call fails + _, err = provider.ReadImageStream(ctx) + require.Error(t, err) + assert.Equal(t, 1, inspector.fetchCount) + + // Fix the inspector so the next call succeeds + inspector.fetchErr = nil + inspector.fetchData = validBytes + + // Second call should retry and succeed + result, err := provider.ReadImageStream(ctx) + require.NoError(t, err) + assert.Equal(t, "test-stream", result.Name) + assert.Equal(t, 2, inspector.fetchCount, "should retry after a failed call") + + // Third call should be cached + _, err = provider.ReadImageStream(ctx) + require.NoError(t, err) + assert.Equal(t, 2, inspector.fetchCount, "should not fetch again after success") +} diff --git a/pkg/osimagestream/mocks_test.go b/pkg/osimagestream/mocks_test.go index b5c56866dd..c378c108e2 100644 --- a/pkg/osimagestream/mocks_test.go +++ b/pkg/osimagestream/mocks_test.go @@ -20,6 +20,8 @@ type mockImagesInspector struct { results []imageutils.BulkInspectResult fetchData []byte fetchErr error + // fetchCount tracks the number of FetchImageFile calls + fetchCount int } func (m *mockImagesInspector) Inspect(_ context.Context, images ...string) ([]imageutils.BulkInspectResult, error) { @@ -52,6 +54,7 @@ func (m *mockImagesInspector) Inspect(_ context.Context, images ...string) ([]im } func (m *mockImagesInspector) FetchImageFile(_ context.Context, image, path string) ([]byte, error) { + m.fetchCount++ if m.err != nil { return nil, m.err } diff --git a/pkg/osimagestream/osimagestream.go b/pkg/osimagestream/osimagestream.go index 71892ea865..3f55fdca2f 100644 --- a/pkg/osimagestream/osimagestream.go +++ b/pkg/osimagestream/osimagestream.go @@ -1,20 +1,20 @@ package osimagestream import ( + "cmp" "context" "errors" "fmt" "maps" "slices" - "strings" "github.com/containers/image/v5/types" imagev1 "github.com/openshift/api/image/v1" "github.com/openshift/api/machineconfiguration/v1alpha1" ctrlcommon "github.com/openshift/machine-config-operator/pkg/controller/common" - "github.com/openshift/machine-config-operator/pkg/imageutils" "github.com/openshift/machine-config-operator/pkg/version" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + k8sversion "k8s.io/apimachinery/pkg/util/version" corelisterv1 "k8s.io/client-go/listers/core/v1" "k8s.io/klog/v2" ) @@ -30,100 +30,83 @@ type StreamSource interface { // ImageStreamFactory creates OS image streams for different runtime contexts. type ImageStreamFactory interface { - // CreateRuntimeSources builds an OSImageStream for runtime operation. - CreateRuntimeSources(ctx context.Context, releaseImage string, sysCtx *types.SystemContext) (*v1alpha1.OSImageStream, error) - // CreateBootstrapSources builds an OSImageStream for bootstrap operation. - CreateBootstrapSources(ctx context.Context, imageStream *imagev1.ImageStream, cliImages *OSImageTuple, sysCtx *types.SystemContext) (*v1alpha1.OSImageStream, error) + // Create builds an OSImageStream from the configured sources and options. + Create(ctx context.Context, sysCtx *types.SystemContext, opts CreateOptions) (*v1alpha1.OSImageStream, error) +} + +// CreateOptions configures how an OSImageStream is built. +// One of ReleaseImage or ReleaseImageStream must be provided. +type CreateOptions struct { + // ReleaseImage is the release payload image to be pulled over the network. + // This field is ignored if ReleaseImageStream is given. + ReleaseImage string + // ReleaseImageStream is the payload ImageStream provided. + ReleaseImageStream *imagev1.ImageStream + // CliImages are OS images provided via CLI flags. + CliImages *OSImageTuple + // ConfigMapLister provides access to the osimageurl ConfigMap. + ConfigMapLister corelisterv1.ConfigMapLister + // ExistingOSImageStream is the current CR used to load user defined configuration in the spec. + ExistingOSImageStream *v1alpha1.OSImageStream + // InstallVersion is the OCP version the cluster was originally installed with. + // Optional: used as a fallback when the ImageStream name cannot be parsed as a version. + InstallVersion *k8sversion.Version } // DefaultStreamSourceFactory is the production implementation of ImageStreamFactory. type DefaultStreamSourceFactory struct { imagesExtractor ImageDataExtractor - cmLister corelisterv1.ConfigMapLister inspectorFactory ImagesInspectorFactory } // NewDefaultStreamSourceFactory creates a new DefaultStreamSourceFactory with the provided dependencies. -func NewDefaultStreamSourceFactory(cmLister corelisterv1.ConfigMapLister, inspectorFactory ImagesInspectorFactory) *DefaultStreamSourceFactory { - return &DefaultStreamSourceFactory{imagesExtractor: NewImageStreamExtractor(), cmLister: cmLister, inspectorFactory: inspectorFactory} -} - -// CreateRuntimeSources builds an OSImageStream for runtime operation. -func (f *DefaultStreamSourceFactory) CreateRuntimeSources(ctx context.Context, releaseImage string, sysCtx *types.SystemContext) (*v1alpha1.OSImageStream, error) { - imagesInspector := f.inspectorFactory.ForContext(sysCtx) - sources := []StreamSource{ - NewOSImagesURLStreamSource(NewConfigMapURLProviders(f.cmLister), f.imagesExtractor, imagesInspector), - NewImageStreamStreamSource(imagesInspector, NewImageStreamProviderNetwork(imagesInspector, releaseImage), f.imagesExtractor), - } - return BuildOSImageStreamFromSources(ctx, sources) +func NewDefaultStreamSourceFactory(inspectorFactory ImagesInspectorFactory) *DefaultStreamSourceFactory { + return &DefaultStreamSourceFactory{imagesExtractor: NewImageStreamExtractor(), inspectorFactory: inspectorFactory} } -// CreateBootstrapSources builds an OSImageStream for bootstrap operation. -func (f *DefaultStreamSourceFactory) CreateBootstrapSources(ctx context.Context, imageStream *imagev1.ImageStream, cliImages *OSImageTuple, sysCtx *types.SystemContext) (*v1alpha1.OSImageStream, error) { +// Create builds an OSImageStream from the configured sources and options. +func (f *DefaultStreamSourceFactory) Create(ctx context.Context, sysCtx *types.SystemContext, createOptions CreateOptions) (*v1alpha1.OSImageStream, error) { var sources []StreamSource imagesInspector := f.inspectorFactory.ForContext(sysCtx) - if cliImages != nil { - sources = append(sources, NewOSImagesURLStreamSource(NewStaticURLProvider(*cliImages), f.imagesExtractor, imagesInspector)) + if createOptions.CliImages != nil { + sources = append(sources, NewOSImagesURLStreamSource(NewStaticURLProvider(*createOptions.CliImages), f.imagesExtractor, imagesInspector)) } - if imageStream != nil { - sources = append(sources, NewImageStreamStreamSource(imagesInspector, NewImageStreamProviderResource(imageStream), f.imagesExtractor)) - } - return BuildOSImageStreamFromSources(ctx, sources) -} - -// BuildOsImageStreamBootstrap builds an OSImageStream for bootstrap using the provided factory. -func BuildOsImageStreamBootstrap( - ctx context.Context, - sysCtxBuilder *imageutils.SysContextBuilder, - imageStream *imagev1.ImageStream, - cliImages *OSImageTuple, - factory ImageStreamFactory, -) (*v1alpha1.OSImageStream, error) { - sysCtx, err := sysCtxBuilder.Build() - if err != nil { - return nil, fmt.Errorf("could not prepare for image inspection: %w", err) + if createOptions.ConfigMapLister != nil { + sources = append(sources, NewOSImagesURLStreamSource(NewConfigMapURLProviders(createOptions.ConfigMapLister), f.imagesExtractor, imagesInspector)) } - defer func() { - if err := sysCtx.Cleanup(); err != nil { - klog.Warningf("Unable to clean resources after OSImageStream inspection: %s", err) - } - }() - return factory.CreateBootstrapSources(ctx, imageStream, cliImages, sysCtx.SysContext) -} - -// BuildOsImageStreamRuntime builds an OSImageStream for runtime using the provided factory. -func BuildOsImageStreamRuntime( - ctx context.Context, - sysCtxBuilder *imageutils.SysContextBuilder, - releaseImage string, - factory ImageStreamFactory, -) (*v1alpha1.OSImageStream, error) { - sysCtx, err := sysCtxBuilder.Build() - if err != nil { - return nil, fmt.Errorf("could not prepare for image inspection: %w", err) + var imageStreamProvider ImageStreamProvider + //nolint:gocritic // I disagree that this would be more readable with a switch case @pablintino + if createOptions.ReleaseImageStream != nil { + imageStreamProvider = NewImageStreamProviderResource(createOptions.ReleaseImageStream) + } else if createOptions.ReleaseImage != "" { + imageStreamProvider = NewImageStreamProviderNetwork(imagesInspector, createOptions.ReleaseImage) + } else { + return nil, errors.New("one of ReleaseImageStream or ReleaseImage must be specified") } - defer func() { - if err := sysCtx.Cleanup(); err != nil { - klog.Warningf("Unable to clean resources after OSImageStream inspection: %s", err) - } - }() - return factory.CreateRuntimeSources(ctx, releaseImage, sysCtx.SysContext) -} + sources = append(sources, NewImageStreamStreamSource(imagesInspector, imageStreamProvider, f.imagesExtractor)) -// BuildOSImageStreamFromSources aggregates streams from multiple sources into a single OSImageStream. -func BuildOSImageStreamFromSources(ctx context.Context, sources []StreamSource) (*v1alpha1.OSImageStream, error) { streams := collect(ctx, sources) if len(streams) == 0 { return nil, ErrorNoOSImageStreamAvailable } - // TODO: This logic is temporal till we make an agreement on - // how to propagate the default stream value (ie, injected by - // the installer) - defaultStream, err := getDefaultStreamSet(streams) + defaultStream, err := getDefaultStreamSet(streams, &createOptions) if err != nil { return nil, fmt.Errorf("could not find default OSImageStream in the available streams: %w", err) } + + return newOSImageStream(createOptions.ExistingOSImageStream, streams, defaultStream), nil +} + +// newOSImageStream assembles the OSImageStream CR from the resolved streams, default, and existing spec. +func newOSImageStream(existing *v1alpha1.OSImageStream, streams []v1alpha1.OSImageStreamSet, defaultStream string) *v1alpha1.OSImageStream { + var spec *v1alpha1.OSImageStreamSpec + if existing != nil && existing.Spec != nil { + spec = existing.Spec.DeepCopy() + } else { + spec = &v1alpha1.OSImageStreamSpec{} + } + return &v1alpha1.OSImageStream{ ObjectMeta: metav1.ObjectMeta{ Name: ctrlcommon.ClusterInstanceNameOSImageStream, @@ -131,30 +114,42 @@ func BuildOSImageStreamFromSources(ctx context.Context, sources []StreamSource) ctrlcommon.ReleaseImageVersionAnnotationKey: version.Hash, }, }, - Spec: &v1alpha1.OSImageStreamSpec{}, + Spec: spec, Status: v1alpha1.OSImageStreamStatus{ DefaultStream: defaultStream, AvailableStreams: streams, }, - }, nil + } } -func getDefaultStreamSet(streams []v1alpha1.OSImageStreamSet) (string, error) { - // TODO This logic is temporal. For now, try to locate the RHEL 9 one in best effort - // Make a copy to avoid modifying the input slice +// getDefaultStreamSet returns the effective default stream: the user override if valid, otherwise the builtin. +func getDefaultStreamSet(streams []v1alpha1.OSImageStreamSet, createOptions *CreateOptions) (string, error) { streamNames := GetStreamSetsNames(streams) - // Sort by name length (shortest first) to prefer simpler names - slices.SortFunc(streamNames, func(a, b string) int { - return len(a) - len(b) - }) - - for _, stream := range streamNames { - if (strings.Contains(stream, "coreos-9") || strings.Contains(stream, "9-coreos") || strings.Contains(stream, "9")) && !strings.Contains(stream, "10") { - return stream, nil + existingOSImageStream := createOptions.ExistingOSImageStream + requestedDefaultStream := GetOSImageStreamSpecDefault(existingOSImageStream) + if requestedDefaultStream == "" && existingOSImageStream != nil && existingOSImageStream.Status.DefaultStream != "" { + // If the spec is empty but the CR is already populated pick whatever existed + requestedDefaultStream = existingOSImageStream.Status.DefaultStream + } + if requestedDefaultStream == "" { + // No user preference: if there's only one stream just use it, + // otherwise fall back to the builtin default. + if len(streams) == 1 { + return streams[0].Name, nil + } + builtinDefault, err := GetBuiltinDefaultStreamName(createOptions.InstallVersion) + if err != nil { + return "", fmt.Errorf("could not determine the builtin default stream: %w", err) } + requestedDefaultStream = builtinDefault } - return "", errors.New("could not find default stream in the list of OSImageStreams. No stream points to RHEL9") + + if slices.Contains(streamNames, requestedDefaultStream) { + return requestedDefaultStream, nil + } + + return "", fmt.Errorf("could not find the requested %s default stream in the list of OSImageStreams %s", requestedDefaultStream, streamNames) } func collect(ctx context.Context, sources []StreamSource) []v1alpha1.OSImageStreamSet { @@ -175,5 +170,10 @@ func collect(ctx context.Context, sources []StreamSource) []v1alpha1.OSImageStre result[stream.Name] = *stream } } - return slices.Collect(maps.Values(result)) + // Sort by name for consistent ordering across reconciliations + streams := slices.Collect(maps.Values(result)) + slices.SortFunc(streams, func(a, b v1alpha1.OSImageStreamSet) int { + return cmp.Compare(a.Name, b.Name) + }) + return streams } diff --git a/pkg/osimagestream/osimagestream_test.go b/pkg/osimagestream/osimagestream_test.go index f5c8cc3f17..4197371635 100644 --- a/pkg/osimagestream/osimagestream_test.go +++ b/pkg/osimagestream/osimagestream_test.go @@ -3,733 +3,491 @@ package osimagestream import ( "context" - "errors" + "fmt" "testing" "github.com/containers/image/v5/types" imagev1 "github.com/openshift/api/image/v1" "github.com/openshift/api/machineconfiguration/v1alpha1" - ctrlcommon "github.com/openshift/machine-config-operator/pkg/controller/common" "github.com/openshift/machine-config-operator/pkg/version" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + k8sversion "k8s.io/apimachinery/pkg/util/version" + corelisterv1 "k8s.io/client-go/listers/core/v1" + "k8s.io/client-go/informers" "k8s.io/client-go/kubernetes/fake" ) -// mockStreamSource is a test implementation of StreamSource -type mockStreamSource struct { - streams []*v1alpha1.OSImageStreamSet - err error +// testImageDef defines an OS image pair for test setup. +type testImageDef struct { + streamClass string + osImage string + extImage string } -func (m *mockStreamSource) FetchStreams(_ context.Context) ([]*v1alpha1.OSImageStreamSet, error) { - return m.streams, m.err +// newTestImageStream builds an ImageStream from a version name and image definitions. +// Each def produces the standard tagged pair (e.g. rhel-9-coreos, rhel-9-coreos-extensions). +func newTestImageStream(version string, defs ...testImageDef) *imagev1.ImageStream { + var tags []imagev1.TagReference + for _, d := range defs { + tags = append(tags, + imagev1.TagReference{ + Name: d.streamClass + "-coreos", + From: &corev1.ObjectReference{Kind: "DockerImage", Name: d.osImage}, + }, + imagev1.TagReference{ + Name: d.streamClass + "-coreos-extensions", + From: &corev1.ObjectReference{Kind: "DockerImage", Name: d.extImage}, + }, + ) + } + return &imagev1.ImageStream{ + ObjectMeta: metav1.ObjectMeta{Name: version}, + Spec: imagev1.ImageStreamSpec{Tags: tags}, + } } -func TestBuildOSImageStreamFromSources(t *testing.T) { - tests := []struct { - name string - sources []StreamSource - expectedDefault string - error error - errorContains string - validateStreams func(t *testing.T, streams []v1alpha1.OSImageStreamSet) - }{ - { - name: "success - builds OSImageStream with default", - sources: []StreamSource{ - &mockStreamSource{ - streams: []*v1alpha1.OSImageStreamSet{ - {Name: "rhel-9", OSImage: "image1", OSExtensionsImage: "ext1"}, - {Name: "rhel-10", OSImage: "image2", OSExtensionsImage: "ext2"}, - }, - }, - }, - expectedDefault: "rhel-9", - validateStreams: func(t *testing.T, streams []v1alpha1.OSImageStreamSet) { - assert.Len(t, streams, 2) - }, - }, - { - name: "error - no streams found", - sources: []StreamSource{ - &mockStreamSource{ - streams: []*v1alpha1.OSImageStreamSet{}, - }, - }, - error: ErrorNoOSImageStreamAvailable, - }, - { - name: "error - all sources fail", - sources: []StreamSource{ - &mockStreamSource{ - err: errors.New("fetch failed"), - }, - }, - error: ErrorNoOSImageStreamAvailable, - }, - { - name: "error - no default stream available", - sources: []StreamSource{ - &mockStreamSource{ - streams: []*v1alpha1.OSImageStreamSet{ - {Name: "rhel-8", OSImage: "image1", OSExtensionsImage: "ext1"}, - {Name: "rhel-10", OSImage: "image2", OSExtensionsImage: "ext2"}, - }, - }, - }, - errorContains: "could not find default OSImageStream", - }, - { - name: "selects shortest matching default", - sources: []StreamSource{ - &mockStreamSource{ - streams: []*v1alpha1.OSImageStreamSet{ - {Name: "rhel-9-extended", OSImage: "image1", OSExtensionsImage: "ext1"}, - {Name: "9", OSImage: "image2", OSExtensionsImage: "ext2"}, - {Name: "rhel-9-coreos", OSImage: "image3", OSExtensionsImage: "ext3"}, - }, - }, - }, - expectedDefault: "9", - }, - { - name: "multiple sources - streams merged", - sources: []StreamSource{ - &mockStreamSource{ - streams: []*v1alpha1.OSImageStreamSet{ - {Name: "rhel-9", OSImage: "image1", OSExtensionsImage: "ext1"}, - }, - }, - &mockStreamSource{ - streams: []*v1alpha1.OSImageStreamSet{ - {Name: "rhel-10", OSImage: "image2", OSExtensionsImage: "ext2"}, - }, - }, - }, - validateStreams: func(t *testing.T, streams []v1alpha1.OSImageStreamSet) { - assert.Len(t, streams, 2) - streamNames := make(map[string]bool) - for _, stream := range streams { - streamNames[stream.Name] = true - } - assert.True(t, streamNames["rhel-9"]) - assert.True(t, streamNames["rhel-10"]) - }, - }, - { - name: "duplicate streams - last one wins", - sources: []StreamSource{ - &mockStreamSource{ - streams: []*v1alpha1.OSImageStreamSet{ - {Name: "rhel-9", OSImage: "original-image", OSExtensionsImage: "original-ext"}, - }, - }, - &mockStreamSource{ - streams: []*v1alpha1.OSImageStreamSet{ - {Name: "rhel-9", OSImage: "overridden-image", OSExtensionsImage: "overridden-ext"}, - }, - }, - }, - validateStreams: func(t *testing.T, streams []v1alpha1.OSImageStreamSet) { - require.Len(t, streams, 1) - assert.Equal(t, "rhel-9", streams[0].Name) - assert.Equal(t, v1alpha1.ImageDigestFormat("overridden-image"), streams[0].OSImage) - assert.Equal(t, v1alpha1.ImageDigestFormat("overridden-ext"), streams[0].OSExtensionsImage) - }, - }, - { - name: "source with error - continues with other sources", - sources: []StreamSource{ - &mockStreamSource{ - err: errors.New("fetch failed"), - }, - &mockStreamSource{ - streams: []*v1alpha1.OSImageStreamSet{ - {Name: "rhel-9", OSImage: "image1", OSExtensionsImage: "ext1"}, - }, - }, - }, - expectedDefault: "rhel-9", - validateStreams: func(t *testing.T, streams []v1alpha1.OSImageStreamSet) { - assert.Len(t, streams, 1) - }, - }, +// newTestInspectData builds mock image inspection data from image definitions. +func newTestInspectData(defs ...testImageDef) map[string]*types.ImageInspectInfo { + data := make(map[string]*types.ImageInspectInfo) + for _, d := range defs { + data[d.osImage] = &types.ImageInspectInfo{ + Labels: map[string]string{ + "io.openshift.os.streamclass": d.streamClass, + "ostree.linux": "present", + }, + } + data[d.extImage] = &types.ImageInspectInfo{ + Labels: map[string]string{ + "io.openshift.os.streamclass": d.streamClass, + }, + } } + return data +} - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - ctx := context.Background() - result, err := BuildOSImageStreamFromSources(ctx, tt.sources) - - if tt.errorContains != "" { - require.Error(t, err) - assert.Contains(t, err.Error(), tt.errorContains) - return - } else if tt.error != nil { - require.Error(t, err) - require.ErrorIs(t, err, tt.error) - return - } - - require.NoError(t, err) - assert.NotNil(t, result) - assert.Equal(t, "cluster", result.Name) - assert.Equal(t, version.Hash, result.Annotations[ctrlcommon.ReleaseImageVersionAnnotationKey]) - if tt.expectedDefault != "" { - assert.Equal(t, tt.expectedDefault, result.Status.DefaultStream) - } - - if tt.validateStreams != nil { - tt.validateStreams(t, result.Status.AvailableStreams) - } - }) - } +// newTestFactory creates a factory with the given inspect data and optional file data for network release image fetching. +func newTestFactory(inspectData map[string]*types.ImageInspectInfo, fileData map[string][]byte) *DefaultStreamSourceFactory { + inspector := &mockImagesInspector{inspectData: inspectData, fileData: fileData} + return NewDefaultStreamSourceFactory(&mockImagesInspectorFactory{inspector: inspector}) } -func TestDefaultStreamSourceFactory_CreateRuntimeSources_BothSources(t *testing.T) { - configMap := &corev1.ConfigMap{ +// newTestConfigMapLister creates a ConfigMap lister with an osimageurl ConfigMap containing the given image refs. +func newTestConfigMapLister(t *testing.T, osImage, extImage string) corelisterv1.ConfigMapLister { + t.Helper() + cm := &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: "machine-config-osimageurl", Namespace: "openshift-machine-config-operator", }, Data: map[string]string{ - "baseOSContainerImage": "quay.io/openshift/os-cm@sha256:111", - "baseOSExtensionsContainerImage": "quay.io/openshift/ext-cm@sha256:222", + "baseOSContainerImage": osImage, + "baseOSExtensionsContainerImage": extImage, "osImageURL": "", "releaseVersion": "4.21.0", }, } + fakeClient := fake.NewSimpleClientset(cm) + factory := informers.NewSharedInformerFactory(fakeClient, 0) + cmInformer := factory.Core().V1().ConfigMaps() + require.NoError(t, cmInformer.Informer().GetIndexer().Add(cm)) + return cmInformer.Lister() +} - // Create fake client and informer - fakeClient := fake.NewSimpleClientset(configMap) - informerFactory := informers.NewSharedInformerFactory(fakeClient, 0) - cmInformer := informerFactory.Core().V1().ConfigMaps() - cmInformer.Informer().GetIndexer().Add(configMap) +// newTestEmptyConfigMapLister creates a ConfigMap lister with no ConfigMaps. +func newTestEmptyConfigMapLister() corelisterv1.ConfigMapLister { + fakeClient := fake.NewSimpleClientset() + factory := informers.NewSharedInformerFactory(fakeClient, 0) + return factory.Core().V1().ConfigMaps().Lister() +} - // Valid ImageStream manifest that will be fetched from the release image - imageStreamManifest := ` -apiVersion: image.openshift.io/v1 +// newTestImageStreamManifest generates a YAML ImageStream manifest for use as release image file data. +func newTestImageStreamManifest(version string, defs ...testImageDef) []byte { + manifest := fmt.Sprintf(`apiVersion: image.openshift.io/v1 kind: ImageStream metadata: - name: release-images + name: %s spec: - tags: - - name: rhel-9 + tags:`, version) + for _, d := range defs { + manifest += fmt.Sprintf(` + - name: %s-coreos from: kind: DockerImage - name: quay.io/openshift/os-net@sha256:333 - - name: rhel-9-coreos-extensions + name: %s + - name: %s-coreos-extensions from: kind: DockerImage - name: quay.io/openshift/ext-net@sha256:444 -` - - inspector := &mockImagesInspector{ - inspectData: map[string]*types.ImageInspectInfo{ - // ConfigMap source images - "quay.io/openshift/os-cm@sha256:111": { - Labels: map[string]string{ - "io.openshift.os.streamclass": "rhel-9", - "ostree.linux": "present", - }, - }, - "quay.io/openshift/ext-cm@sha256:222": { - Labels: map[string]string{ - "io.openshift.os.streamclass": "rhel-9", - }, - }, - // Network source images - "quay.io/openshift/os-net@sha256:333": { - Labels: map[string]string{ - "io.openshift.os.streamclass": "rhel-9", - "ostree.linux": "present", - }, - }, - "quay.io/openshift/ext-net@sha256:444": { - Labels: map[string]string{ - "io.openshift.os.streamclass": "rhel-9", - }, - }, - }, - fileData: map[string][]byte{ - "quay.io/openshift/release:4.16:/release-manifests/image-references": []byte(imageStreamManifest), - }, + name: %s`, d.streamClass, d.osImage, d.streamClass, d.extImage) } + return []byte(manifest) +} + +var ( + // testInstallVersion simulates a 4.x cluster for tests that need the builtin default to be rhel-9. + testInstallVersion = k8sversion.MustParseGeneric("4.22.0") - inspectorFactory := &mockImagesInspectorFactory{inspector: inspector} - factory := NewDefaultStreamSourceFactory(cmInformer.Lister(), inspectorFactory) + testStreamDefRHEL9 = testImageDef{ + streamClass: "rhel-9", + osImage: "quay.io/openshift/os-9@sha256:aaa111", + extImage: "quay.io/openshift/ext-9@sha256:bbb222", + } + testStreamDefRHEL10 = testImageDef{ + streamClass: "rhel-10", + osImage: "quay.io/openshift/os-10@sha256:ccc333", + extImage: "quay.io/openshift/ext-10@sha256:ddd444", + } + // Alternate image refs for ConfigMap sources (to distinguish from ImageStream sources). + testStreamDefRHEL9ConfigMap = testImageDef{ + streamClass: "rhel-9", + osImage: "quay.io/openshift/os-cm@sha256:111", + extImage: "quay.io/openshift/ext-cm@sha256:222", + } +) - ctx := context.Background() - sysCtx := &types.SystemContext{} +func TestDefaultStreamSourceFactory_Create_RuntimeBothSources(t *testing.T) { + cmLister := newTestConfigMapLister(t, testStreamDefRHEL9ConfigMap.osImage, testStreamDefRHEL9ConfigMap.extImage) - result, err := factory.CreateRuntimeSources(ctx, "quay.io/openshift/release:4.16", sysCtx) + // Network release image contains a different rhel-9 stream + rhel9Net := testImageDef{streamClass: "rhel-9", osImage: "quay.io/openshift/os-net@sha256:333", extImage: "quay.io/openshift/ext-net@sha256:444"} + manifest := newTestImageStreamManifest("4.22.0", rhel9Net) + + inspectData := newTestInspectData(testStreamDefRHEL9ConfigMap, rhel9Net) + factory := newTestFactory(inspectData, map[string][]byte{ + "quay.io/openshift/release:4.16:/release-manifests/image-references": manifest, + }) + + result, err := factory.Create(context.Background(), &types.SystemContext{}, CreateOptions{ + ReleaseImage: "quay.io/openshift/release:4.16", + ConfigMapLister: cmLister, + InstallVersion: testInstallVersion, + }) require.NoError(t, err) - assert.NotNil(t, result) assert.Equal(t, "cluster", result.Name) assert.Equal(t, "rhel-9", result.Status.DefaultStream) - assert.NotEmpty(t, result.Status.AvailableStreams) - // Should have stream from both sources (they have same name so should be merged) assert.Len(t, result.Status.AvailableStreams, 1) } -func TestDefaultStreamSourceFactory_CreateRuntimeSources_ConfigMapOnly(t *testing.T) { - configMap := &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: "machine-config-osimageurl", - Namespace: "openshift-machine-config-operator", - }, - Data: map[string]string{ - "baseOSContainerImage": "quay.io/openshift/os@sha256:abc123", - "baseOSExtensionsContainerImage": "quay.io/openshift/ext@sha256:def456", - "osImageURL": "", - "releaseVersion": "4.21.0", +func TestDefaultStreamSourceFactory_Create_RuntimeConfigMapOnly(t *testing.T) { + cmLister := newTestConfigMapLister(t, testStreamDefRHEL9.osImage, testStreamDefRHEL9.extImage) + + manifest := newTestImageStreamManifest("4.22.0", testStreamDefRHEL9) + inspectData := newTestInspectData(testStreamDefRHEL9) + factory := newTestFactory(inspectData, map[string][]byte{ + "quay.io/openshift/release:4.16:/release-manifests/image-references": manifest, + }) + + result, err := factory.Create(context.Background(), &types.SystemContext{}, CreateOptions{ + ReleaseImage: "quay.io/openshift/release:4.16", + ConfigMapLister: cmLister, + InstallVersion: testInstallVersion, + ExistingOSImageStream: &v1alpha1.OSImageStream{ + Spec: &v1alpha1.OSImageStreamSpec{DefaultStream: "rhel-9"}, }, - } - - // Create fake client and informer - fakeClient := fake.NewSimpleClientset(configMap) - informerFactory := informers.NewSharedInformerFactory(fakeClient, 0) - cmInformer := informerFactory.Core().V1().ConfigMaps() - cmInformer.Informer().GetIndexer().Add(configMap) - - inspector := &mockImagesInspector{ - inspectData: map[string]*types.ImageInspectInfo{ - "quay.io/openshift/os@sha256:abc123": { - Labels: map[string]string{ - "io.openshift.os.streamclass": "rhel-9", - "ostree.linux": "present", - }, - }, - "quay.io/openshift/ext@sha256:def456": { - Labels: map[string]string{ - "io.openshift.os.streamclass": "rhel-9", - }, - }, - }, - // Network source will fail (no fileData provided for release image) - } - - inspectorFactory := &mockImagesInspectorFactory{inspector: inspector} - factory := NewDefaultStreamSourceFactory(cmInformer.Lister(), inspectorFactory) - - ctx := context.Background() - sysCtx := &types.SystemContext{} - - result, err := factory.CreateRuntimeSources(ctx, "quay.io/openshift/release:4.16", sysCtx) + }) require.NoError(t, err) - assert.NotNil(t, result) assert.Equal(t, "cluster", result.Name) assert.Equal(t, "rhel-9", result.Status.DefaultStream) assert.NotEmpty(t, result.Status.AvailableStreams) } -func TestDefaultStreamSourceFactory_CreateRuntimeSources_BothSourcesFail(t *testing.T) { - // Create empty fake client with no ConfigMaps - fakeClient := fake.NewSimpleClientset() - informerFactory := informers.NewSharedInformerFactory(fakeClient, 0) - cmInformer := informerFactory.Core().V1().ConfigMaps() - - inspector := &mockImagesInspector{ - inspectData: map[string]*types.ImageInspectInfo{}, - } +func TestDefaultStreamSourceFactory_Create_RuntimeBothSourcesFail(t *testing.T) { + cmLister := newTestEmptyConfigMapLister() + factory := newTestFactory(map[string]*types.ImageInspectInfo{}, nil) - inspectorFactory := &mockImagesInspectorFactory{inspector: inspector} - factory := NewDefaultStreamSourceFactory(cmInformer.Lister(), inspectorFactory) - - ctx := context.Background() - sysCtx := &types.SystemContext{} - - result, err := factory.CreateRuntimeSources(ctx, "quay.io/openshift/release:4.16", sysCtx) + result, err := factory.Create(context.Background(), &types.SystemContext{}, CreateOptions{ + ReleaseImage: "quay.io/openshift/release:4.16", + ConfigMapLister: cmLister, + }) require.Error(t, err) assert.Nil(t, result) assert.ErrorIs(t, err, ErrorNoOSImageStreamAvailable) } -func TestDefaultStreamSourceFactory_CreateRuntimeSources_MultipleStreams(t *testing.T) { - configMap := &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: "machine-config-osimageurl", - Namespace: "openshift-machine-config-operator", - }, - Data: map[string]string{ - "baseOSContainerImage": "quay.io/openshift/os@sha256:abc123", - "baseOSExtensionsContainerImage": "quay.io/openshift/ext@sha256:def456", - "osImageURL": "", - "releaseVersion": "4.21.0", - }, - } +func TestDefaultStreamSourceFactory_Create_RuntimeMultipleStreams(t *testing.T) { + cmLister := newTestConfigMapLister(t, testStreamDefRHEL9.osImage, testStreamDefRHEL9.extImage) - // Create fake client and informer - fakeClient := fake.NewSimpleClientset(configMap) - informerFactory := informers.NewSharedInformerFactory(fakeClient, 0) - cmInformer := informerFactory.Core().V1().ConfigMaps() - cmInformer.Informer().GetIndexer().Add(configMap) + manifest := newTestImageStreamManifest("4.22.0", testStreamDefRHEL9, testStreamDefRHEL10) + inspectData := newTestInspectData(testStreamDefRHEL9, testStreamDefRHEL10) + factory := newTestFactory(inspectData, map[string][]byte{ + "quay.io/openshift/release:4.16:/release-manifests/image-references": manifest, + }) - // ImageStream manifest with multiple streams - imageStreamManifest := ` -apiVersion: image.openshift.io/v1 -kind: ImageStream -metadata: - name: release-images -spec: - tags: - - name: rhel-10-coreos - from: - kind: DockerImage - name: quay.io/openshift/os-10@sha256:aaa111 - - name: rhel-10-coreos-extensions - from: - kind: DockerImage - name: quay.io/openshift/ext-10@sha256:bbb222 -` - - inspector := &mockImagesInspector{ - inspectData: map[string]*types.ImageInspectInfo{ - // ConfigMap source images (rhel-9) - "quay.io/openshift/os@sha256:abc123": { - Labels: map[string]string{ - "io.openshift.os.streamclass": "rhel-9", - "ostree.linux": "present", - }, - }, - "quay.io/openshift/ext@sha256:def456": { - Labels: map[string]string{ - "io.openshift.os.streamclass": "rhel-9", - }, - }, - // Network source images (rhel-10) - "quay.io/openshift/os-10@sha256:aaa111": { - Labels: map[string]string{ - "io.openshift.os.streamclass": "rhel-10", - "ostree.linux": "present", - }, - }, - "quay.io/openshift/ext-10@sha256:bbb222": { - Labels: map[string]string{ - "io.openshift.os.streamclass": "rhel-10", - }, - }, - }, - fileData: map[string][]byte{ - "quay.io/openshift/release:4.16:/release-manifests/image-references": []byte(imageStreamManifest), - }, - } + result, err := factory.Create(context.Background(), &types.SystemContext{}, CreateOptions{ + ReleaseImage: "quay.io/openshift/release:4.16", + ConfigMapLister: cmLister, + InstallVersion: testInstallVersion, + }) - inspectorFactory := &mockImagesInspectorFactory{inspector: inspector} - factory := NewDefaultStreamSourceFactory(cmInformer.Lister(), inspectorFactory) + require.NoError(t, err) + assert.Equal(t, "cluster", result.Name) + assert.Equal(t, "rhel-9", result.Status.DefaultStream) + require.Len(t, result.Status.AvailableStreams, 2) + assert.ElementsMatch(t, []string{"rhel-9", "rhel-10"}, GetStreamSetsNames(result.Status.AvailableStreams)) +} - ctx := context.Background() - sysCtx := &types.SystemContext{} +func TestDefaultStreamSourceFactory_Create_BootstrapMultipleStreams(t *testing.T) { + imageStream := newTestImageStream("4.22.0", testStreamDefRHEL9, testStreamDefRHEL10) + // Also add the legacy rhel-coreos alias pointing to the rhel-9 image + imageStream.Spec.Tags = append(imageStream.Spec.Tags, imagev1.TagReference{ + Name: "rhel-coreos", + From: &corev1.ObjectReference{Kind: "DockerImage", Name: testStreamDefRHEL9.osImage}, + }) - result, err := factory.CreateRuntimeSources(ctx, "quay.io/openshift/release:4.16", sysCtx) + factory := newTestFactory(newTestInspectData(testStreamDefRHEL9, testStreamDefRHEL10), nil) + + result, err := factory.Create(context.Background(), &types.SystemContext{}, CreateOptions{ + ReleaseImageStream: imageStream, + InstallVersion: testInstallVersion, + }) require.NoError(t, err) - assert.NotNil(t, result) assert.Equal(t, "cluster", result.Name) - assert.Equal(t, "rhel-9", result.Status.DefaultStream) + assert.NotEmpty(t, result.Status.DefaultStream) require.Len(t, result.Status.AvailableStreams, 2) - - streamNames := []string{result.Status.AvailableStreams[0].Name, result.Status.AvailableStreams[1].Name} - assert.ElementsMatch(t, []string{"rhel-9", "rhel-10"}, streamNames) + assert.ElementsMatch(t, []string{"rhel-9", "rhel-10"}, GetStreamSetsNames(result.Status.AvailableStreams)) } -func TestDefaultStreamSourceFactory_CreateBootstrapSources_MultipleStreams(t *testing.T) { - // Create empty fake client - fakeClient := fake.NewSimpleClientset() - informerFactory := informers.NewSharedInformerFactory(fakeClient, 0) - cmInformer := informerFactory.Core().V1().ConfigMaps() - - imageStream := &imagev1.ImageStream{ - ObjectMeta: metav1.ObjectMeta{ - Name: "machine-os-images", - Namespace: "openshift-machine-config-operator", +func TestDefaultStreamSourceFactory_Create_UserDefaultOverride(t *testing.T) { + imageStream := newTestImageStream("4.22.0", testStreamDefRHEL9, testStreamDefRHEL10) + // Add legacy alias + imageStream.Spec.Tags = append(imageStream.Spec.Tags, imagev1.TagReference{ + Name: "rhel-coreos", + From: &corev1.ObjectReference{Kind: "DockerImage", Name: testStreamDefRHEL9.osImage}, + }) + factory := newTestFactory(newTestInspectData(testStreamDefRHEL9, testStreamDefRHEL10), nil) + + result, err := factory.Create(context.Background(), &types.SystemContext{}, CreateOptions{ + ReleaseImageStream: imageStream, + InstallVersion: testInstallVersion, + ExistingOSImageStream: &v1alpha1.OSImageStream{ + Spec: &v1alpha1.OSImageStreamSpec{DefaultStream: "rhel-10"}, }, - Spec: imagev1.ImageStreamSpec{ - Tags: []imagev1.TagReference{ - { - Name: "rhel-9-coreos", - From: &corev1.ObjectReference{ - Kind: "DockerImage", - Name: "quay.io/openshift/os-9@sha256:aaa111", - }, - }, - { - Name: "rhel-9-coreos-extensions", - From: &corev1.ObjectReference{ - Kind: "DockerImage", - Name: "quay.io/openshift/ext-9@sha256:bbb222", - }, - }, - { - Name: "rhel-10-coreos", - From: &corev1.ObjectReference{ - Kind: "DockerImage", - Name: "quay.io/openshift/os-10@sha256:ccc333", - }, - }, - { - Name: "rhel-10-coreos-extensions", - From: &corev1.ObjectReference{ - Kind: "DockerImage", - Name: "quay.io/openshift/ext-10@sha256:ddd444", - }, - }, - }, - }, - } + }) - inspector := &mockImagesInspector{ - inspectData: map[string]*types.ImageInspectInfo{ - "quay.io/openshift/os-9@sha256:aaa111": { - Labels: map[string]string{ - "io.openshift.os.streamclass": "rhel-9", - "ostree.linux": "present", - }, - }, - "quay.io/openshift/ext-9@sha256:bbb222": { - Labels: map[string]string{ - "io.openshift.os.streamclass": "rhel-9", - }, - }, - "quay.io/openshift/os-10@sha256:ccc333": { - Labels: map[string]string{ - "io.openshift.os.streamclass": "rhel-10", - "ostree.linux": "present", - }, - }, - "quay.io/openshift/ext-10@sha256:ddd444": { - Labels: map[string]string{ - "io.openshift.os.streamclass": "rhel-10", - }, - }, + require.NoError(t, err) + assert.Equal(t, "rhel-10", result.Status.DefaultStream, "status should reflect the user override") + assert.Equal(t, "rhel-10", result.Spec.DefaultStream, "spec should carry the user override") +} + +func TestDefaultStreamSourceFactory_Create_BootstrapCliImagesOnly(t *testing.T) { + imageStream := newTestImageStream("4.22.0", testStreamDefRHEL9) + // Add legacy alias + imageStream.Spec.Tags = append(imageStream.Spec.Tags, imagev1.TagReference{ + Name: "rhel-coreos", + From: &corev1.ObjectReference{Kind: "DockerImage", Name: testStreamDefRHEL9.osImage}, + }) + factory := newTestFactory(newTestInspectData(testStreamDefRHEL9), nil) + + result, err := factory.Create(context.Background(), &types.SystemContext{}, CreateOptions{ + CliImages: &OSImageTuple{OSImage: testStreamDefRHEL9.osImage, OSExtensionsImage: testStreamDefRHEL9.extImage}, + ReleaseImageStream: imageStream, + InstallVersion: testInstallVersion, + ExistingOSImageStream: &v1alpha1.OSImageStream{ + Spec: &v1alpha1.OSImageStreamSpec{DefaultStream: "rhel-9"}, }, + }) + + require.NoError(t, err) + assert.Equal(t, "cluster", result.Name) + assert.Equal(t, "rhel-9", result.Status.DefaultStream) + require.NotEmpty(t, result.Status.AvailableStreams) + assert.Equal(t, "rhel-9", result.Spec.DefaultStream) +} + +func TestDefaultStreamSourceFactory_Create_PreservesExistingSpec(t *testing.T) { + imageStream := newTestImageStream("4.22.0", testStreamDefRHEL9) + // Add legacy alias + imageStream.Spec.Tags = append(imageStream.Spec.Tags, imagev1.TagReference{ + Name: "rhel-coreos", + From: &corev1.ObjectReference{Kind: "DockerImage", Name: testStreamDefRHEL9.osImage}, + }) + factory := newTestFactory(newTestInspectData(testStreamDefRHEL9), nil) + + existing := &v1alpha1.OSImageStream{ + Spec: &v1alpha1.OSImageStreamSpec{DefaultStream: "rhel-9"}, } - inspectorFactory := &mockImagesInspectorFactory{inspector: inspector} - factory := NewDefaultStreamSourceFactory(cmInformer.Lister(), inspectorFactory) + result, err := factory.Create(context.Background(), &types.SystemContext{}, CreateOptions{ + ReleaseImageStream: imageStream, + InstallVersion: testInstallVersion, + ExistingOSImageStream: existing, + }) - ctx := context.Background() - sysCtx := &types.SystemContext{} + require.NoError(t, err) + assert.Equal(t, "rhel-9", result.Spec.DefaultStream) + assert.NotSame(t, existing.Spec, result.Spec) +} + +func TestDefaultStreamSourceFactory_Create_BootstrapImageStreamOnly(t *testing.T) { + imageStream := newTestImageStream("4.22.0", testStreamDefRHEL9) + // Add legacy alias + imageStream.Spec.Tags = append(imageStream.Spec.Tags, imagev1.TagReference{ + Name: "rhel-coreos", + From: &corev1.ObjectReference{Kind: "DockerImage", Name: testStreamDefRHEL9.osImage}, + }) + factory := newTestFactory(newTestInspectData(testStreamDefRHEL9), nil) - result, err := factory.CreateBootstrapSources(ctx, imageStream, nil, sysCtx) + result, err := factory.Create(context.Background(), &types.SystemContext{}, CreateOptions{ + ReleaseImageStream: imageStream, + InstallVersion: testInstallVersion, + }) require.NoError(t, err) - assert.NotNil(t, result) assert.Equal(t, "cluster", result.Name) assert.NotEmpty(t, result.Status.DefaultStream) - require.Len(t, result.Status.AvailableStreams, 2) - - streamNames := []string{result.Status.AvailableStreams[0].Name, result.Status.AvailableStreams[1].Name} - assert.Contains(t, streamNames, "rhel-9") - assert.Contains(t, streamNames, "rhel-10") } -func TestDefaultStreamSourceFactory_CreateBootstrapSources_CliImagesOnly(t *testing.T) { - // Create empty fake client (no ConfigMaps needed for this test) - fakeClient := fake.NewSimpleClientset() - informerFactory := informers.NewSharedInformerFactory(fakeClient, 0) - cmInformer := informerFactory.Core().V1().ConfigMaps() +func TestDefaultStreamSourceFactory_Create_BootstrapBothSources(t *testing.T) { + rhel9CLI := testImageDef{streamClass: "rhel-9", osImage: "quay.io/openshift/os-cli@sha256:111", extImage: "quay.io/openshift/ext-cli@sha256:222"} + rhel9Stream := testImageDef{streamClass: "rhel-9", osImage: "quay.io/openshift/os-stream@sha256:333", extImage: "quay.io/openshift/ext-stream@sha256:444"} - cliImages := &OSImageTuple{ - OSImage: "quay.io/openshift/os@sha256:abc123", - OSExtensionsImage: "quay.io/openshift/ext@sha256:def456", - } + imageStream := newTestImageStream("4.22.0", rhel9Stream) + // Add legacy alias + imageStream.Spec.Tags = append(imageStream.Spec.Tags, imagev1.TagReference{ + Name: "rhel-coreos", + From: &corev1.ObjectReference{Kind: "DockerImage", Name: rhel9Stream.osImage}, + }) + factory := newTestFactory(newTestInspectData(rhel9CLI, rhel9Stream), nil) - inspector := &mockImagesInspector{ - inspectData: map[string]*types.ImageInspectInfo{ - "quay.io/openshift/os@sha256:abc123": { - Labels: map[string]string{ - "io.openshift.os.streamclass": "rhel-9", - "ostree.linux": "present", - }, - }, - "quay.io/openshift/ext@sha256:def456": { - Labels: map[string]string{ - "io.openshift.os.streamclass": "rhel-9", - }, - }, - }, - } + result, err := factory.Create(context.Background(), &types.SystemContext{}, CreateOptions{ + ReleaseImageStream: imageStream, + CliImages: &OSImageTuple{OSImage: rhel9CLI.osImage, OSExtensionsImage: rhel9CLI.extImage}, + InstallVersion: testInstallVersion, + }) - inspectorFactory := &mockImagesInspectorFactory{inspector: inspector} - factory := NewDefaultStreamSourceFactory(cmInformer.Lister(), inspectorFactory) + require.NoError(t, err) + assert.Equal(t, "cluster", result.Name) + assert.NotEmpty(t, result.Status.DefaultStream) +} - ctx := context.Background() - sysCtx := &types.SystemContext{} +func TestDefaultStreamSourceFactory_Create_InstallVersionTakesPriority(t *testing.T) { + // ImageStream is version 5.0.0, but InstallVersion is 4.22.0 + // The builtin default should be "rhel-9" because InstallVersion takes priority + imageStream := newTestImageStream("5.0.0", testStreamDefRHEL9, testStreamDefRHEL10) + factory := newTestFactory(newTestInspectData(testStreamDefRHEL9, testStreamDefRHEL10), nil) - result, err := factory.CreateBootstrapSources(ctx, nil, cliImages, sysCtx) + result, err := factory.Create(context.Background(), &types.SystemContext{}, CreateOptions{ + ReleaseImageStream: imageStream, + InstallVersion: k8sversion.MustParseGeneric("4.22.0"), + }) require.NoError(t, err) - assert.NotNil(t, result) - assert.Equal(t, "cluster", result.Name) assert.Equal(t, "rhel-9", result.Status.DefaultStream) - require.NotEmpty(t, result.Status.AvailableStreams) } -func TestDefaultStreamSourceFactory_CreateBootstrapSources_ImageStreamOnly(t *testing.T) { - // Create empty fake client - fakeClient := fake.NewSimpleClientset() - informerFactory := informers.NewSharedInformerFactory(fakeClient, 0) - cmInformer := informerFactory.Core().V1().ConfigMaps() +func TestDefaultStreamSourceFactory_Create_DuplicateStreamsLastSourceWins(t *testing.T) { + rhel9Net := testImageDef{streamClass: "rhel-9", osImage: "quay.io/openshift/os-net@sha256:333", extImage: "quay.io/openshift/ext-net@sha256:444"} + cmLister := newTestConfigMapLister(t, testStreamDefRHEL9ConfigMap.osImage, testStreamDefRHEL9ConfigMap.extImage) - imageStream := &imagev1.ImageStream{ - ObjectMeta: metav1.ObjectMeta{ - Name: "machine-os-images", - Namespace: "openshift-machine-config-operator", - }, - Spec: imagev1.ImageStreamSpec{ - Tags: []imagev1.TagReference{ - { - Name: "rhel-9-coreos", - From: &corev1.ObjectReference{ - Kind: "DockerImage", - Name: "quay.io/openshift/os@sha256:abc123", - }, - }, - { - Name: "rhel-9-coreos-extensions", - From: &corev1.ObjectReference{ - Kind: "DockerImage", - Name: "quay.io/openshift/ext@sha256:def456", - }, - }, - }, - }, - } - - inspector := &mockImagesInspector{ - inspectData: map[string]*types.ImageInspectInfo{ - "quay.io/openshift/os@sha256:abc123": { - Labels: map[string]string{ - "io.openshift.os.streamclass": "rhel-9", - "ostree.linux": "present", - }, - }, - "quay.io/openshift/ext@sha256:def456": { - Labels: map[string]string{ - "io.openshift.os.streamclass": "rhel-9", - }, - }, - }, - } + manifest := newTestImageStreamManifest("4.22.0", rhel9Net) + inspectData := newTestInspectData(testStreamDefRHEL9ConfigMap, rhel9Net) + factory := newTestFactory(inspectData, map[string][]byte{ + "quay.io/openshift/release:4.16:/release-manifests/image-references": manifest, + }) - inspectorFactory := &mockImagesInspectorFactory{inspector: inspector} - factory := NewDefaultStreamSourceFactory(cmInformer.Lister(), inspectorFactory) + result, err := factory.Create(context.Background(), &types.SystemContext{}, CreateOptions{ + ReleaseImage: "quay.io/openshift/release:4.16", + ConfigMapLister: cmLister, + InstallVersion: testInstallVersion, + }) - ctx := context.Background() - sysCtx := &types.SystemContext{} + require.NoError(t, err) + require.Len(t, result.Status.AvailableStreams, 1) + // The ImageStream source is appended last, so its images should override the ConfigMap ones + assert.Equal(t, v1alpha1.ImageDigestFormat(rhel9Net.osImage), result.Status.AvailableStreams[0].OSImage) + assert.Equal(t, v1alpha1.ImageDigestFormat(rhel9Net.extImage), result.Status.AvailableStreams[0].OSExtensionsImage) +} - result, err := factory.CreateBootstrapSources(ctx, imageStream, nil, sysCtx) +func TestDefaultStreamSourceFactory_Create_PartialSourceFailure(t *testing.T) { + // ConfigMap source will fail (no ConfigMap in the lister), but ImageStream source succeeds + imageStream := newTestImageStream("4.22.0", testStreamDefRHEL9) + // Add legacy alias + imageStream.Spec.Tags = append(imageStream.Spec.Tags, imagev1.TagReference{ + Name: "rhel-coreos", + From: &corev1.ObjectReference{Kind: "DockerImage", Name: testStreamDefRHEL9.osImage}, + }) + factory := newTestFactory(newTestInspectData(testStreamDefRHEL9), nil) + + result, err := factory.Create(context.Background(), &types.SystemContext{}, CreateOptions{ + ReleaseImageStream: imageStream, + ConfigMapLister: newTestEmptyConfigMapLister(), + InstallVersion: testInstallVersion, + }) require.NoError(t, err) - assert.NotNil(t, result) - assert.Equal(t, "cluster", result.Name) - assert.NotEmpty(t, result.Status.DefaultStream) + require.Len(t, result.Status.AvailableStreams, 1) + assert.Equal(t, "rhel-9", result.Status.DefaultStream) } -func TestDefaultStreamSourceFactory_CreateBootstrapSources_BothSources(t *testing.T) { - // Create empty fake client - fakeClient := fake.NewSimpleClientset() - informerFactory := informers.NewSharedInformerFactory(fakeClient, 0) - cmInformer := informerFactory.Core().V1().ConfigMaps() +func TestDefaultStreamSourceFactory_Create_InvalidUserDefault(t *testing.T) { + imageStream := newTestImageStream("4.22.0", testStreamDefRHEL9) + // Add legacy alias + imageStream.Spec.Tags = append(imageStream.Spec.Tags, imagev1.TagReference{ + Name: "rhel-coreos", + From: &corev1.ObjectReference{Kind: "DockerImage", Name: testStreamDefRHEL9.osImage}, + }) + factory := newTestFactory(newTestInspectData(testStreamDefRHEL9), nil) + + _, err := factory.Create(context.Background(), &types.SystemContext{}, CreateOptions{ + ReleaseImageStream: imageStream, + InstallVersion: testInstallVersion, + ExistingOSImageStream: &v1alpha1.OSImageStream{ + Spec: &v1alpha1.OSImageStreamSpec{DefaultStream: "rhel-99"}, + }, + }) - cliImages := &OSImageTuple{ - OSImage: "quay.io/openshift/os-cli@sha256:111", - OSExtensionsImage: "quay.io/openshift/ext-cli@sha256:222", - } + require.Error(t, err) + assert.Contains(t, err.Error(), "could not find the requested rhel-99 default stream") +} - imageStream := &imagev1.ImageStream{ - ObjectMeta: metav1.ObjectMeta{ - Name: "machine-os-images", - Namespace: "openshift-machine-config-operator", +func TestDefaultStreamSourceFactory_Create_StatusDefaultPreserved(t *testing.T) { + imageStream := newTestImageStream("4.22.0", testStreamDefRHEL9, testStreamDefRHEL10) + factory := newTestFactory(newTestInspectData(testStreamDefRHEL9, testStreamDefRHEL10), nil) + + // Existing CR has rhel-10 as status default but no spec override. + // The builtin default for 4.x is rhel-9, but the existing status should be preserved. + result, err := factory.Create(context.Background(), &types.SystemContext{}, CreateOptions{ + ReleaseImageStream: imageStream, + InstallVersion: testInstallVersion, + ExistingOSImageStream: &v1alpha1.OSImageStream{ + Status: v1alpha1.OSImageStreamStatus{DefaultStream: "rhel-10"}, }, - Spec: imagev1.ImageStreamSpec{ - Tags: []imagev1.TagReference{ - { - Name: "rhel-9-coreos", - From: &corev1.ObjectReference{ - Kind: "DockerImage", - Name: "quay.io/openshift/os-stream@sha256:333", - }, - }, - { - Name: "rhel-9-coreos-extensions", - From: &corev1.ObjectReference{ - Kind: "DockerImage", - Name: "quay.io/openshift/ext-stream@sha256:444", - }, - }, - }, - }, - } + }) - inspector := &mockImagesInspector{ - inspectData: map[string]*types.ImageInspectInfo{ - "quay.io/openshift/os-cli@sha256:111": { - Labels: map[string]string{ - "io.openshift.os.streamclass": "rhel-9", - "ostree.linux": "present", - }, - }, - "quay.io/openshift/ext-cli@sha256:222": { - Labels: map[string]string{ - "io.openshift.os.streamclass": "rhel-9", - }, - }, - "quay.io/openshift/os-stream@sha256:333": { - Labels: map[string]string{ - "io.openshift.os.streamclass": "rhel-9", - "ostree.linux": "present", - }, - }, - "quay.io/openshift/ext-stream@sha256:444": { - Labels: map[string]string{ - "io.openshift.os.streamclass": "rhel-9", - }, - }, - }, - } + require.NoError(t, err) + assert.Equal(t, "rhel-10", result.Status.DefaultStream, "existing status default should be preserved when no spec override is set") +} - inspectorFactory := &mockImagesInspectorFactory{inspector: inspector} - factory := NewDefaultStreamSourceFactory(cmInformer.Lister(), inspectorFactory) +func TestDefaultStreamSourceFactory_Create_FallsBackToBuildVersion(t *testing.T) { + // No InstallVersion provided, the code should fall back to version.ReleaseVersion. + imageStream := newTestImageStream("4.22.0", testStreamDefRHEL9, testStreamDefRHEL10) + factory := newTestFactory(newTestInspectData(testStreamDefRHEL9, testStreamDefRHEL10), nil) - ctx := context.Background() - sysCtx := &types.SystemContext{} + originalReleaseVersion := version.ReleaseVersion + version.ReleaseVersion = "4.18.0" + defer func() { version.ReleaseVersion = originalReleaseVersion }() - result, err := factory.CreateBootstrapSources(ctx, imageStream, cliImages, sysCtx) + result, err := factory.Create(context.Background(), &types.SystemContext{}, CreateOptions{ + ReleaseImageStream: imageStream, + }) require.NoError(t, err) - assert.NotNil(t, result) - assert.Equal(t, "cluster", result.Name) - assert.NotEmpty(t, result.Status.DefaultStream) + assert.Equal(t, "rhel-9", result.Status.DefaultStream, "should fall back to build version 4.18.0 and pick rhel-9") } -func TestDefaultStreamSourceFactory_CreateBootstrapSources_NoSources(t *testing.T) { - // Create empty fake client - fakeClient := fake.NewSimpleClientset() - informerFactory := informers.NewSharedInformerFactory(fakeClient, 0) - cmInformer := informerFactory.Core().V1().ConfigMaps() +func TestDefaultStreamSourceFactory_Create_BootstrapNoSources(t *testing.T) { + factory := newTestFactory(map[string]*types.ImageInspectInfo{}, nil) - inspector := &mockImagesInspector{ - inspectData: map[string]*types.ImageInspectInfo{}, - } - - inspectorFactory := &mockImagesInspectorFactory{inspector: inspector} - factory := NewDefaultStreamSourceFactory(cmInformer.Lister(), inspectorFactory) - - ctx := context.Background() - sysCtx := &types.SystemContext{} - - result, err := factory.CreateBootstrapSources(ctx, nil, nil, sysCtx) + result, err := factory.Create(context.Background(), &types.SystemContext{}, CreateOptions{}) require.Error(t, err) assert.Nil(t, result) - assert.ErrorIs(t, err, ErrorNoOSImageStreamAvailable) + assert.Contains(t, err.Error(), "one of ReleaseImageStream or ReleaseImage must be specified") } diff --git a/pkg/osimagestream/streams.go b/pkg/osimagestream/streams.go new file mode 100644 index 0000000000..c981cf6069 --- /dev/null +++ b/pkg/osimagestream/streams.go @@ -0,0 +1,45 @@ +package osimagestream + +import ( + "fmt" + + "github.com/openshift/machine-config-operator/pkg/version" + k8sversion "k8s.io/apimachinery/pkg/util/version" +) + +const ( + // StreamNameRHEL9 is the stream name for RHEL 9 based CoreOS images. + StreamNameRHEL9 = "rhel-9" + // StreamNameRHEL10 is the stream name for RHEL 10 based CoreOS images. + StreamNameRHEL10 = "rhel-10" + // StreamNameCentOS10 is the stream name for SCOS 10 based CoreOS images. + StreamNameCentOS10 = "centos-10" +) + +// GetBuiltinDefaultStreamName returns the default stream name for the current build. +// For OKD/SCOS builds it always returns "centos-10" as OKD only ships a single stream. +// For OCP builds it returns the default stream based on the OCP version: +// OCP 4.x clusters default to "rhel-9", OCP 5+ default to "rhel-10". +// If installVersion is provided that value is used directly. Otherwise, the build's +// release version (version.ReleaseVersion) is used as fallback. +func GetBuiltinDefaultStreamName(installVersion *k8sversion.Version) (string, error) { + if version.IsSCOS() { + return StreamNameCentOS10, nil + } + + var releaseVersion *k8sversion.Version + if installVersion != nil { + releaseVersion = installVersion + } else { + var err error + releaseVersion, err = k8sversion.ParseGeneric(version.ReleaseVersion) + if err != nil { + return "", fmt.Errorf("unable to parse the build release version %q as a version: %w", version.ReleaseVersion, err) + } + } + + if releaseVersion.Major() == 4 { + return StreamNameRHEL9, nil + } + return StreamNameRHEL10, nil +} diff --git a/pkg/osimagestream/streams_test.go b/pkg/osimagestream/streams_test.go new file mode 100644 index 0000000000..ff7ecc4654 --- /dev/null +++ b/pkg/osimagestream/streams_test.go @@ -0,0 +1,97 @@ +package osimagestream + +import ( + "testing" + + "github.com/openshift/machine-config-operator/pkg/version" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + k8sversion "k8s.io/apimachinery/pkg/util/version" +) + +// TestGetBuiltinDefaultStreamName verifies that the builtin default stream name +// is resolved correctly for both OCP (RHEL) and OKD (SCOS) builds. +// This test mutates and restores package-level globals (version.SCOS, +// version.ReleaseVersion) and must NOT be run in parallel. +func TestGetBuiltinDefaultStreamName(t *testing.T) { + tests := []struct { + name string + scos bool + installVersion string + releaseVersion string + expected string + expectErr bool + }{ + { + name: "SCOS always returns centos-10", + scos: true, + installVersion: "5.0.0", + expected: StreamNameCentOS10, + }, + { + name: "SCOS returns centos-10 even for 4.x", + scos: true, + installVersion: "4.18.0", + expected: StreamNameCentOS10, + }, + { + name: "SCOS returns centos-10 with nil installVersion", + scos: true, + expected: StreamNameCentOS10, + }, + { + name: "OCP 4.x returns rhel-9", + installVersion: "4.18.0", + expected: StreamNameRHEL9, + }, + { + name: "OCP 5.x returns rhel-10", + installVersion: "5.0.0", + expected: StreamNameRHEL10, + }, + { + name: "OCP falls back to releaseVersion when installVersion is nil", + releaseVersion: "4.19.0", + expected: StreamNameRHEL9, + }, + { + name: "OCP falls back to releaseVersion 5.x", + releaseVersion: "5.1.0", + expected: StreamNameRHEL10, + }, + { + name: "OCP errors on unparseable releaseVersion", + releaseVersion: "not-a-version", + expectErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + origSCOS := version.SCOS + origRelease := version.ReleaseVersion + t.Cleanup(func() { + version.SCOS = origSCOS + version.ReleaseVersion = origRelease + }) + + version.SCOS = tt.scos + if tt.releaseVersion != "" { + version.ReleaseVersion = tt.releaseVersion + } + + var installVersion *k8sversion.Version + if tt.installVersion != "" { + installVersion = k8sversion.MustParseGeneric(tt.installVersion) + } + + result, err := GetBuiltinDefaultStreamName(installVersion) + if tt.expectErr { + require.Error(t, err) + return + } + require.NoError(t, err) + assert.Equal(t, tt.expected, result) + }) + } +} diff --git a/test/e2e-2of2/osimagestream_test.go b/test/e2e-2of2/osimagestream_test.go index 0b46538251..a34c1df8ad 100644 --- a/test/e2e-2of2/osimagestream_test.go +++ b/test/e2e-2of2/osimagestream_test.go @@ -35,7 +35,11 @@ func TestImageStreamProviderCVO(t *testing.T) { // Wait for the informer cache to sync before querying the lister ctrlctx.ConfigInformerFactory.WaitForCacheSync(ctx.Done()) - image, err := osimagestream.GetReleasePayloadImage(ctrlctx.ConfigInformerFactory.Config().V1().ClusterVersions().Lister()) + clusterVersion, err := osimagestream.GetClusterVersion(ctrlctx.ConfigInformerFactory.Config().V1().ClusterVersions().Lister()) + assert.NoError(t, err) + assert.NotNil(t, clusterVersion) + + image, err := osimagestream.GetReleasePayloadImage(clusterVersion) assert.NoError(t, err) assert.NotEmpty(t, image)