From 49c35a78f1fe0efa8fe3eed133972e577e03ea6f Mon Sep 17 00:00:00 2001 From: Pablo Rodriguez Nava Date: Thu, 26 Feb 2026 09:29:49 +0100 Subject: [PATCH] MCO-2117: Allow default OSImageStream overrides Previously the default stream was determined by matching the OS version in the stream name, without the possibility of telling the MCO which default stream to use. This change replaces that logic, allowing users to input the defaultStream they want to use and falling back to the builtin if necessary. The builtin default stream determination logic has been improved to avoid hard-coding versions and it now picks the OSImageStream that points to the images of the default OS tags in the payload ImageStream. Signed-off-by: Pablo Rodriguez Nava --- ...etion-guard-validatingadmissionpolicy.yaml | 19 + ...uard-validatingadmissionpolicybinding.yaml | 7 + pkg/controller/bootstrap/bootstrap.go | 41 +- .../internalreleaseimage_controller.go | 6 +- pkg/controller/render/render_controller.go | 25 + pkg/operator/operator.go | 2 + pkg/operator/osimagestream_ocp.go | 95 +- pkg/operator/sync.go | 12 +- pkg/osimagestream/clusterversion.go | 41 +- pkg/osimagestream/clusterversion_test.go | 192 +++- pkg/osimagestream/helpers.go | 8 + pkg/osimagestream/helpers_test.go | 39 + pkg/osimagestream/imagestream_provider.go | 22 +- .../imagestream_provider_test.go | 63 ++ pkg/osimagestream/mocks_test.go | 3 + pkg/osimagestream/osimagestream.go | 172 ++-- pkg/osimagestream/osimagestream_test.go | 970 +++++++----------- pkg/osimagestream/streams.go | 45 + pkg/osimagestream/streams_test.go | 97 ++ test/e2e-2of2/osimagestream_test.go | 6 +- 20 files changed, 1112 insertions(+), 753 deletions(-) create mode 100644 manifests/machineconfigcontroller/osimagestream-deletion-guard-validatingadmissionpolicy.yaml create mode 100644 manifests/machineconfigcontroller/osimagestream-deletion-guard-validatingadmissionpolicybinding.yaml create mode 100644 pkg/osimagestream/streams.go create mode 100644 pkg/osimagestream/streams_test.go 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)