MCO-1961: OSImageStream follow up

pkg/controller/osimagestream/helpers.go

@@ -2,12 +2,12 @@ package osimagestream
 
 import (
 	"fmt"
-	"strings"
 
 	v1 "github.com/openshift/api/machineconfiguration/v1"
 	"github.com/openshift/api/machineconfiguration/v1alpha1"
 	"github.com/openshift/machine-config-operator/pkg/controller/common"
 	"github.com/openshift/machine-config-operator/pkg/helpers"
+	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 )
 
 // GetStreamSetsNames extracts the names from a slice of OSImageStreamSets.
@@ -35,7 +35,7 @@ func GetOSImageStreamSetByName(osImageStream *v1alpha1.OSImageStream, name strin
 		}
 	}
 
-	return nil, fmt.Errorf("requested OSImageStream %s does not exist. Existing: %s", name, strings.Join(GetStreamSetsNames(osImageStream.Status.AvailableStreams), ","))
+	return nil, k8serrors.NewNotFound(v1alpha1.GroupVersion.WithResource("osimagestreams").GroupResource(), name)
 }
 
 // TryGetOSImageStreamSetByName retrieves an OSImageStreamSet by name, returning nil if not found.

pkg/controller/osimagestream/helpers_test.go

@@ -9,6 +9,7 @@ import (
 	"github.com/openshift/machine-config-operator/pkg/controller/common"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
@@ -49,8 +50,8 @@ func TestGetStreamSetsNames(t *testing.T) {
 	}
 }
 
-func TestGetOSImageStreamSetByName(t *testing.T) {
-	osImageStream := &v1alpha1.OSImageStream{
+func getStubOSImageStream() *v1alpha1.OSImageStream {
+	return &v1alpha1.OSImageStream{
 		Status: v1alpha1.OSImageStreamStatus{
 			DefaultStream: "rhel-9",
 			AvailableStreams: []v1alpha1.OSImageStreamSet{
@@ -59,53 +60,67 @@ func TestGetOSImageStreamSetByName(t *testing.T) {
 			},
 		},
 	}
+}
 
+func TestGetOSImageStreamSetByName(t *testing.T) {
 	tests := []struct {
-		name          string
-		osImageStream *v1alpha1.OSImageStream
-		streamName    string
-		expected      *v1alpha1.OSImageStreamSet
-		errorContains string
+		name                 string
+		osImageStreamFactory func() *v1alpha1.OSImageStream
+		streamName           string
+		expected             *v1alpha1.OSImageStreamSet
+		errorContains        string
+		errorCheckFn         func(*testing.T, error)
 	}{
 		{
-			name:          "find existing stream",
-			osImageStream: osImageStream,
-			streamName:    "rhel-9",
-			expected:      &v1alpha1.OSImageStreamSet{Name: "rhel-9", OSImage: "image1", OSExtensionsImage: "ext1"},
+			name:                 "find existing stream",
+			osImageStreamFactory: getStubOSImageStream,
+			streamName:           "rhel-9",
+			expected:             &v1alpha1.OSImageStreamSet{Name: "rhel-9", OSImage: "image1", OSExtensionsImage: "ext1"},
 		},
 		{
-			name:          "find another existing stream",
-			osImageStream: osImageStream,
-			streamName:    "rhel-10",
-			expected:      &v1alpha1.OSImageStreamSet{Name: "rhel-10", OSImage: "image2", OSExtensionsImage: "ext2"},
+			name:                 "find another existing stream",
+			osImageStreamFactory: getStubOSImageStream,
+			streamName:           "rhel-10",
+			expected:             &v1alpha1.OSImageStreamSet{Name: "rhel-10", OSImage: "image2", OSExtensionsImage: "ext2"},
 		},
 		{
-			name:          "empty name returns default stream",
-			osImageStream: osImageStream,
-			streamName:    "",
-			expected:      &v1alpha1.OSImageStreamSet{Name: "rhel-9", OSImage: "image1", OSExtensionsImage: "ext1"},
+			name:                 "empty name returns default stream",
+			osImageStreamFactory: getStubOSImageStream,
+			streamName:           "",
+			expected:             &v1alpha1.OSImageStreamSet{Name: "rhel-9", OSImage: "image1", OSExtensionsImage: "ext1"},
 		},
 		{
-			name:          "non-existent stream",
-			osImageStream: osImageStream,
-			streamName:    "non-existent",
-			errorContains: "does not exist",
+			name:                 "non-existent stream",
+			osImageStreamFactory: getStubOSImageStream,
+			streamName:           "non-existent",
+			errorContains:        "not found",
+			errorCheckFn: func(t *testing.T, err error) {
+				assert.True(t, apierrors.IsNotFound(err))
+			},
 		},
 		{
-			name:          "nil osImageStream",
-			osImageStream: nil,
-			streamName:    "rhel-9",
-			errorContains: "cannot be nil",
+			name:                 "nil osImageStream",
+			osImageStreamFactory: nil,
+			streamName:           "rhel-9",
+			errorContains:        "cannot be nil",
 		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			result, err := GetOSImageStreamSetByName(tt.osImageStream, tt.streamName)
+			var osImageStream *v1alpha1.OSImageStream
+			if tt.osImageStreamFactory != nil {
+				osImageStream = tt.osImageStreamFactory()
+			}
+
+			result, err := GetOSImageStreamSetByName(osImageStream, tt.streamName)
 			if tt.errorContains != "" {
 				require.Error(t, err)
 				assert.Contains(t, err.Error(), tt.errorContains)
 				assert.Nil(t, result)
+				if tt.errorCheckFn != nil {
+					tt.errorCheckFn(t, err)
+				}
 			} else {
 				require.NoError(t, err)
 				assert.Equal(t, tt.expected, result)
@@ -115,45 +130,40 @@ func TestGetOSImageStreamSetByName(t *testing.T) {
 }
 
 func TestTryGetOSImageStreamSetByName(t *testing.T) {
-	osImageStream := &v1alpha1.OSImageStream{
-		Status: v1alpha1.OSImageStreamStatus{
-			DefaultStream: "rhel-9",
-			AvailableStreams: []v1alpha1.OSImageStreamSet{
-				{Name: "rhel-9", OSImage: "image1", OSExtensionsImage: "ext1"},
-				{Name: "rhel-10", OSImage: "image2", OSExtensionsImage: "ext2"},
-			},
-		},
-	}
 
 	tests := []struct {
-		name          string
-		osImageStream *v1alpha1.OSImageStream
-		streamName    string
-		expected      *v1alpha1.OSImageStreamSet
+		name                 string
+		osImageStreamFactory func() *v1alpha1.OSImageStream
+		streamName           string
+		expected             *v1alpha1.OSImageStreamSet
 	}{
 		{
-			name:          "find existing stream",
-			osImageStream: osImageStream,
-			streamName:    "rhel-9",
-			expected:      &v1alpha1.OSImageStreamSet{Name: "rhel-9", OSImage: "image1", OSExtensionsImage: "ext1"},
+			name:                 "find existing stream",
+			osImageStreamFactory: getStubOSImageStream,
+			streamName:           "rhel-9",
+			expected:             &v1alpha1.OSImageStreamSet{Name: "rhel-9", OSImage: "image1", OSExtensionsImage: "ext1"},
 		},
 		{
-			name:          "non-existent stream returns nil",
-			osImageStream: osImageStream,
-			streamName:    "non-existent",
-			expected:      nil,
+			name:                 "non-existent stream returns nil",
+			osImageStreamFactory: getStubOSImageStream,
+			streamName:           "non-existent",
+			expected:             nil,
 		},
 		{
-			name:          "nil osImageStream returns nil",
-			osImageStream: nil,
-			streamName:    "rhel-9",
-			expected:      nil,
+			name:                 "nil osImageStream returns nil",
+			osImageStreamFactory: nil,
+			streamName:           "rhel-9",
+			expected:             nil,
 		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			result := TryGetOSImageStreamSetByName(tt.osImageStream, tt.streamName)
+			var osImageStream *v1alpha1.OSImageStream
+			if tt.osImageStreamFactory != nil {
+				osImageStream = tt.osImageStreamFactory()
+			}
+			result := TryGetOSImageStreamSetByName(osImageStream, tt.streamName)
 			assert.Equal(t, tt.expected, result)
 		})
 	}

pkg/controller/osimagestream/image_data.go

@@ -96,11 +96,11 @@ func findLabelValue(m map[string]string, keys ...string) string {
 // Combines pairs of OS and extensions images that share the same stream name into OSImageStreamSet objects.
 // If multiple images are found for the same stream and type, the last one wins.
 func GroupOSContainerImageMetadataToStream(imagesMetadata []*ImageData) []*v1alpha1.OSImageStreamSet {
-	streamMaps := make(map[string]*v1alpha1.OSImageStreamSet)
+	streamMap := make(map[string]*v1alpha1.OSImageStreamSet)
 	for _, imageMetadata := range imagesMetadata {
-		streamURLSet, exists := streamMaps[imageMetadata.Stream]
+		streamURLSet, exists := streamMap[imageMetadata.Stream]
 		if !exists {
-			streamMaps[imageMetadata.Stream] = NewOSImageStreamURLSetFromImageMetadata(imageMetadata)
+			streamMap[imageMetadata.Stream] = NewOSImageStreamURLSetFromImageMetadata(imageMetadata)
 			continue
 		}
 
@@ -120,7 +120,20 @@ func GroupOSContainerImageMetadataToStream(imagesMetadata []*ImageData) []*v1alp
 			streamURLSet.OSExtensionsImage = imageName
 		}
 	}
-	return slices.Collect(maps.Values(streamMaps))
+
+	// Remove mapped OSImageStreamSet with only one URL. A valid OSImageStreamSet must
+	// point to both images.
+	// How can a stream end here?
+	//  - An ImageStream with only one of the images tagged
+	//  - A proper built ImageStream but one of the images is lacking the stream labels
+	//  - A URL set with only one image having the labels
+	for _, stream := range streamMap {
+		if stream.OSExtensionsImage == "" || stream.OSImage == "" {
+			delete(streamMap, stream.Name)
+		}
+	}
+
+	return slices.Collect(maps.Values(streamMap))
 }
 
 // NewOSImageStreamURLSetFromImageMetadata creates an OSImageStreamSet from image metadata.

pkg/controller/osimagestream/image_data_test.go

@@ -127,82 +127,72 @@ func TestGroupOSContainerImageMetadataToStream_MultipleStreams(t *testing.T) {
 	assert.Equal(t, v1alpha1.ImageDigestFormat("quay.io/openshift/ext-10@sha256:ddd444"), rhel10.OSExtensionsImage)
 }
 
-func TestGroupOSContainerImageMetadataToStream_OSImageOnly(t *testing.T) {
-	imagesMetadata := []*ImageData{
+func TestGroupOSContainerImageMetadataToStream_PartialURLs(t *testing.T) {
+	// This test ensures that GroupOSContainerImageMetadataToStream only returns
+	// OSImageStreamSet that have both URLs
+
+	tests := []struct {
+		name      string
+		imageData []*ImageData
+	}{
 		{
-			Image:  "quay.io/openshift/os@sha256:abc123",
-			Type:   ImageTypeOS,
-			Stream: "rhel-9",
+			name: "OS only",
+			imageData: []*ImageData{
+				{
+					Image:  "quay.io/openshift/os@sha256:abc123",
+					Type:   ImageTypeOS,
+					Stream: "rhel-9",
+				},
+			},
 		},
-	}
-
-	result := GroupOSContainerImageMetadataToStream(imagesMetadata)
-
-	require.Len(t, result, 1)
-	assert.Equal(t, "rhel-9", result[0].Name)
-	assert.Equal(t, v1alpha1.ImageDigestFormat("quay.io/openshift/os@sha256:abc123"), result[0].OSImage)
-	assert.Empty(t, result[0].OSExtensionsImage)
-}
-
-func TestGroupOSContainerImageMetadataToStream_ExtensionsImageOnly(t *testing.T) {
-	imagesMetadata := []*ImageData{
 		{
-			Image:  "quay.io/openshift/ext@sha256:def456",
-			Type:   ImageTypeExtensions,
-			Stream: "rhel-9",
+			name: "Extensions only",
+			imageData: []*ImageData{
+				{
+					Image:  "quay.io/openshift/ext@sha256:def456",
+					Type:   ImageTypeExtensions,
+					Stream: "rhel-9",
+				},
+			},
 		},
-	}
-
-	result := GroupOSContainerImageMetadataToStream(imagesMetadata)
-
-	require.Len(t, result, 1)
-	assert.Equal(t, "rhel-9", result[0].Name)
-	assert.Empty(t, result[0].OSImage)
-	assert.Equal(t, v1alpha1.ImageDigestFormat("quay.io/openshift/ext@sha256:def456"), result[0].OSExtensionsImage)
-}
-
-func TestGroupOSContainerImageMetadataToStream_DuplicateOSImage(t *testing.T) {
-	imagesMetadata := []*ImageData{
 		{
-			Image:  "quay.io/openshift/os-old@sha256:111",
-			Type:   ImageTypeOS,
-			Stream: "rhel-9",
+			name: "OS Duplicated",
+			imageData: []*ImageData{
+				{
+					Image:  "quay.io/openshift/os-old@sha256:111",
+					Type:   ImageTypeOS,
+					Stream: "rhel-9",
+				},
+				{
+					Image:  "quay.io/openshift/os-new@sha256:222",
+					Type:   ImageTypeOS,
+					Stream: "rhel-9",
+				},
+			},
 		},
 		{
-			Image:  "quay.io/openshift/os-new@sha256:222",
-			Type:   ImageTypeOS,
-			Stream: "rhel-9",
+			name: "Extensions Duplicated",
+			imageData: []*ImageData{
+				{
+					Image:  "quay.io/openshift/ext-old@sha256:333",
+					Type:   ImageTypeExtensions,
+					Stream: "rhel-9",
+				},
+				{
+					Image:  "quay.io/openshift/ext-new@sha256:444",
+					Type:   ImageTypeExtensions,
+					Stream: "rhel-9",
+				},
+			},
 		},
 	}
 
-	result := GroupOSContainerImageMetadataToStream(imagesMetadata)
-
-	require.Len(t, result, 1)
-	assert.Equal(t, "rhel-9", result[0].Name)
-	// Last one wins
-	assert.Equal(t, v1alpha1.ImageDigestFormat("quay.io/openshift/os-new@sha256:222"), result[0].OSImage)
-}
-
-func TestGroupOSContainerImageMetadataToStream_DuplicateExtensionsImage(t *testing.T) {
-	imagesMetadata := []*ImageData{
-		{
-			Image:  "quay.io/openshift/ext-old@sha256:333",
-			Type:   ImageTypeExtensions,
-			Stream: "rhel-9",
-		},
-		{
-			Image:  "quay.io/openshift/ext-new@sha256:444",
-			Type:   ImageTypeExtensions,
-			Stream: "rhel-9",
-		},
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result := GroupOSContainerImageMetadataToStream(tt.imageData)
+			require.Empty(t, result, "result should be empty")
+		})
 	}
-
-	result := GroupOSContainerImageMetadataToStream(imagesMetadata)
-
-	require.Len(t, result, 1)
-	assert.Equal(t, "rhel-9", result[0].Name)
-	// Last one wins
-	assert.Equal(t, v1alpha1.ImageDigestFormat("quay.io/openshift/ext-new@sha256:444"), result[0].OSExtensionsImage)
 }
 
 func TestGroupOSContainerImageMetadataToStream_EmptyInput(t *testing.T) {