Skip to content

Commit 5051cce

Browse files
openshift-merge-bot[bot]openshift-merge-bot[bot]
authored
Merge pull request #5458 from pablintino/osimagestream-followup-1
MCO-1961: OSImageStream follow up
2 parents 8e6beb0 + 019e91e commit 5051cce

File tree

10 files changed

+220
-144
lines changed

10 files changed

+220
-144
lines changed

‎pkg/controller/osimagestream/helpers.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,12 @@ package osimagestream
22

33
import (
44
"fmt"
5-
"strings"
65

76
v1 "github.com/openshift/api/machineconfiguration/v1"
87
"github.com/openshift/api/machineconfiguration/v1alpha1"
98
"github.com/openshift/machine-config-operator/pkg/controller/common"
109
"github.com/openshift/machine-config-operator/pkg/helpers"
10+
k8serrors "k8s.io/apimachinery/pkg/api/errors"
1111
)
1212

1313
// GetStreamSetsNames extracts the names from a slice of OSImageStreamSets.
@@ -35,7 +35,7 @@ func GetOSImageStreamSetByName(osImageStream *v1alpha1.OSImageStream, name strin
3535
}
3636
}
3737

38-
return nil, fmt.Errorf("requested OSImageStream %s does not exist. Existing: %s", name, strings.Join(GetStreamSetsNames(osImageStream.Status.AvailableStreams), ","))
38+
return nil, k8serrors.NewNotFound(v1alpha1.GroupVersion.WithResource("osimagestreams").GroupResource(), name)
3939
}
4040

4141
// TryGetOSImageStreamSetByName retrieves an OSImageStreamSet by name, returning nil if not found.

‎pkg/controller/osimagestream/helpers_test.go‎

Lines changed: 64 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
"github.com/openshift/machine-config-operator/pkg/controller/common"
1010
"github.com/stretchr/testify/assert"
1111
"github.com/stretchr/testify/require"
12+
apierrors "k8s.io/apimachinery/pkg/api/errors"
1213
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1314
)
1415

@@ -49,8 +50,8 @@ func TestGetStreamSetsNames(t *testing.T) {
4950
}
5051
}
5152

52-
func TestGetOSImageStreamSetByName(t *testing.T) {
53-
osImageStream := &v1alpha1.OSImageStream{
53+
func getStubOSImageStream() *v1alpha1.OSImageStream {
54+
return &v1alpha1.OSImageStream{
5455
Status: v1alpha1.OSImageStreamStatus{
5556
DefaultStream: "rhel-9",
5657
AvailableStreams: []v1alpha1.OSImageStreamSet{
@@ -59,53 +60,67 @@ func TestGetOSImageStreamSetByName(t *testing.T) {
5960
},
6061
},
6162
}
63+
}
6264

65+
func TestGetOSImageStreamSetByName(t *testing.T) {
6366
tests := []struct {
64-
name string
65-
osImageStream *v1alpha1.OSImageStream
66-
streamName string
67-
expected *v1alpha1.OSImageStreamSet
68-
errorContains string
67+
name string
68+
osImageStreamFactory func() *v1alpha1.OSImageStream
69+
streamName string
70+
expected *v1alpha1.OSImageStreamSet
71+
errorContains string
72+
errorCheckFn func(*testing.T, error)
6973
}{
7074
{
71-
name: "find existing stream",
72-
osImageStream: osImageStream,
73-
streamName: "rhel-9",
74-
expected: &v1alpha1.OSImageStreamSet{Name: "rhel-9", OSImage: "image1", OSExtensionsImage: "ext1"},
75+
name: "find existing stream",
76+
osImageStreamFactory: getStubOSImageStream,
77+
streamName: "rhel-9",
78+
expected: &v1alpha1.OSImageStreamSet{Name: "rhel-9", OSImage: "image1", OSExtensionsImage: "ext1"},
7579
},
7680
{
77-
name: "find another existing stream",
78-
osImageStream: osImageStream,
79-
streamName: "rhel-10",
80-
expected: &v1alpha1.OSImageStreamSet{Name: "rhel-10", OSImage: "image2", OSExtensionsImage: "ext2"},
81+
name: "find another existing stream",
82+
osImageStreamFactory: getStubOSImageStream,
83+
streamName: "rhel-10",
84+
expected: &v1alpha1.OSImageStreamSet{Name: "rhel-10", OSImage: "image2", OSExtensionsImage: "ext2"},
8185
},
8286
{
83-
name: "empty name returns default stream",
84-
osImageStream: osImageStream,
85-
streamName: "",
86-
expected: &v1alpha1.OSImageStreamSet{Name: "rhel-9", OSImage: "image1", OSExtensionsImage: "ext1"},
87+
name: "empty name returns default stream",
88+
osImageStreamFactory: getStubOSImageStream,
89+
streamName: "",
90+
expected: &v1alpha1.OSImageStreamSet{Name: "rhel-9", OSImage: "image1", OSExtensionsImage: "ext1"},
8791
},
8892
{
89-
name: "non-existent stream",
90-
osImageStream: osImageStream,
91-
streamName: "non-existent",
92-
errorContains: "does not exist",
93+
name: "non-existent stream",
94+
osImageStreamFactory: getStubOSImageStream,
95+
streamName: "non-existent",
96+
errorContains: "not found",
97+
errorCheckFn: func(t *testing.T, err error) {
98+
assert.True(t, apierrors.IsNotFound(err))
99+
},
93100
},
94101
{
95-
name: "nil osImageStream",
96-
osImageStream: nil,
97-
streamName: "rhel-9",
98-
errorContains: "cannot be nil",
102+
name: "nil osImageStream",
103+
osImageStreamFactory: nil,
104+
streamName: "rhel-9",
105+
errorContains: "cannot be nil",
99106
},
100107
}
101108

102109
for _, tt := range tests {
103110
t.Run(tt.name, func(t *testing.T) {
104-
result, err := GetOSImageStreamSetByName(tt.osImageStream, tt.streamName)
111+
var osImageStream *v1alpha1.OSImageStream
112+
if tt.osImageStreamFactory != nil {
113+
osImageStream = tt.osImageStreamFactory()
114+
}
115+
116+
result, err := GetOSImageStreamSetByName(osImageStream, tt.streamName)
105117
if tt.errorContains != "" {
106118
require.Error(t, err)
107119
assert.Contains(t, err.Error(), tt.errorContains)
108120
assert.Nil(t, result)
121+
if tt.errorCheckFn != nil {
122+
tt.errorCheckFn(t, err)
123+
}
109124
} else {
110125
require.NoError(t, err)
111126
assert.Equal(t, tt.expected, result)
@@ -115,45 +130,40 @@ func TestGetOSImageStreamSetByName(t *testing.T) {
115130
}
116131

117132
func TestTryGetOSImageStreamSetByName(t *testing.T) {
118-
osImageStream := &v1alpha1.OSImageStream{
119-
Status: v1alpha1.OSImageStreamStatus{
120-
DefaultStream: "rhel-9",
121-
AvailableStreams: []v1alpha1.OSImageStreamSet{
122-
{Name: "rhel-9", OSImage: "image1", OSExtensionsImage: "ext1"},
123-
{Name: "rhel-10", OSImage: "image2", OSExtensionsImage: "ext2"},
124-
},
125-
},
126-
}
127133

128134
tests := []struct {
129-
name string
130-
osImageStream *v1alpha1.OSImageStream
131-
streamName string
132-
expected *v1alpha1.OSImageStreamSet
135+
name string
136+
osImageStreamFactory func() *v1alpha1.OSImageStream
137+
streamName string
138+
expected *v1alpha1.OSImageStreamSet
133139
}{
134140
{
135-
name: "find existing stream",
136-
osImageStream: osImageStream,
137-
streamName: "rhel-9",
138-
expected: &v1alpha1.OSImageStreamSet{Name: "rhel-9", OSImage: "image1", OSExtensionsImage: "ext1"},
141+
name: "find existing stream",
142+
osImageStreamFactory: getStubOSImageStream,
143+
streamName: "rhel-9",
144+
expected: &v1alpha1.OSImageStreamSet{Name: "rhel-9", OSImage: "image1", OSExtensionsImage: "ext1"},
139145
},
140146
{
141-
name: "non-existent stream returns nil",
142-
osImageStream: osImageStream,
143-
streamName: "non-existent",
144-
expected: nil,
147+
name: "non-existent stream returns nil",
148+
osImageStreamFactory: getStubOSImageStream,
149+
streamName: "non-existent",
150+
expected: nil,
145151
},
146152
{
147-
name: "nil osImageStream returns nil",
148-
osImageStream: nil,
149-
streamName: "rhel-9",
150-
expected: nil,
153+
name: "nil osImageStream returns nil",
154+
osImageStreamFactory: nil,
155+
streamName: "rhel-9",
156+
expected: nil,
151157
},
152158
}
153159

154160
for _, tt := range tests {
155161
t.Run(tt.name, func(t *testing.T) {
156-
result := TryGetOSImageStreamSetByName(tt.osImageStream, tt.streamName)
162+
var osImageStream *v1alpha1.OSImageStream
163+
if tt.osImageStreamFactory != nil {
164+
osImageStream = tt.osImageStreamFactory()
165+
}
166+
result := TryGetOSImageStreamSetByName(osImageStream, tt.streamName)
157167
assert.Equal(t, tt.expected, result)
158168
})
159169
}

‎pkg/controller/osimagestream/image_data.go‎

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -96,11 +96,11 @@ func findLabelValue(m map[string]string, keys ...string) string {
9696
// Combines pairs of OS and extensions images that share the same stream name into OSImageStreamSet objects.
9797
// If multiple images are found for the same stream and type, the last one wins.
9898
func GroupOSContainerImageMetadataToStream(imagesMetadata []*ImageData) []*v1alpha1.OSImageStreamSet {
99-
streamMaps := make(map[string]*v1alpha1.OSImageStreamSet)
99+
streamMap := make(map[string]*v1alpha1.OSImageStreamSet)
100100
for _, imageMetadata := range imagesMetadata {
101-
streamURLSet, exists := streamMaps[imageMetadata.Stream]
101+
streamURLSet, exists := streamMap[imageMetadata.Stream]
102102
if !exists {
103-
streamMaps[imageMetadata.Stream] = NewOSImageStreamURLSetFromImageMetadata(imageMetadata)
103+
streamMap[imageMetadata.Stream] = NewOSImageStreamURLSetFromImageMetadata(imageMetadata)
104104
continue
105105
}
106106

@@ -120,7 +120,20 @@ func GroupOSContainerImageMetadataToStream(imagesMetadata []*ImageData) []*v1alp
120120
streamURLSet.OSExtensionsImage = imageName
121121
}
122122
}
123-
return slices.Collect(maps.Values(streamMaps))
123+
124+
// Remove mapped OSImageStreamSet with only one URL. A valid OSImageStreamSet must
125+
// point to both images.
126+
// How can a stream end here?
127+
// - An ImageStream with only one of the images tagged
128+
// - A proper built ImageStream but one of the images is lacking the stream labels
129+
// - A URL set with only one image having the labels
130+
for _, stream := range streamMap {
131+
if stream.OSExtensionsImage == "" || stream.OSImage == "" {
132+
delete(streamMap, stream.Name)
133+
}
134+
}
135+
136+
return slices.Collect(maps.Values(streamMap))
124137
}
125138

126139
// NewOSImageStreamURLSetFromImageMetadata creates an OSImageStreamSet from image metadata.

‎pkg/controller/osimagestream/image_data_test.go‎

Lines changed: 55 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -127,82 +127,72 @@ func TestGroupOSContainerImageMetadataToStream_MultipleStreams(t *testing.T) {
127127
assert.Equal(t, v1alpha1.ImageDigestFormat("quay.io/openshift/ext-10@sha256:ddd444"), rhel10.OSExtensionsImage)
128128
}
129129

130-
func TestGroupOSContainerImageMetadataToStream_OSImageOnly(t *testing.T) {
131-
imagesMetadata := []*ImageData{
130+
func TestGroupOSContainerImageMetadataToStream_PartialURLs(t *testing.T) {
131+
// This test ensures that GroupOSContainerImageMetadataToStream only returns
132+
// OSImageStreamSet that have both URLs
133+
134+
tests := []struct {
135+
name string
136+
imageData []*ImageData
137+
}{
132138
{
133-
Image: "quay.io/openshift/os@sha256:abc123",
134-
Type: ImageTypeOS,
135-
Stream: "rhel-9",
139+
name: "OS only",
140+
imageData: []*ImageData{
141+
{
142+
Image: "quay.io/openshift/os@sha256:abc123",
143+
Type: ImageTypeOS,
144+
Stream: "rhel-9",
145+
},
146+
},
136147
},
137-
}
138-
139-
result := GroupOSContainerImageMetadataToStream(imagesMetadata)
140-
141-
require.Len(t, result, 1)
142-
assert.Equal(t, "rhel-9", result[0].Name)
143-
assert.Equal(t, v1alpha1.ImageDigestFormat("quay.io/openshift/os@sha256:abc123"), result[0].OSImage)
144-
assert.Empty(t, result[0].OSExtensionsImage)
145-
}
146-
147-
func TestGroupOSContainerImageMetadataToStream_ExtensionsImageOnly(t *testing.T) {
148-
imagesMetadata := []*ImageData{
149148
{
150-
Image: "quay.io/openshift/ext@sha256:def456",
151-
Type: ImageTypeExtensions,
152-
Stream: "rhel-9",
149+
name: "Extensions only",
150+
imageData: []*ImageData{
151+
{
152+
Image: "quay.io/openshift/ext@sha256:def456",
153+
Type: ImageTypeExtensions,
154+
Stream: "rhel-9",
155+
},
156+
},
153157
},
154-
}
155-
156-
result := GroupOSContainerImageMetadataToStream(imagesMetadata)
157-
158-
require.Len(t, result, 1)
159-
assert.Equal(t, "rhel-9", result[0].Name)
160-
assert.Empty(t, result[0].OSImage)
161-
assert.Equal(t, v1alpha1.ImageDigestFormat("quay.io/openshift/ext@sha256:def456"), result[0].OSExtensionsImage)
162-
}
163-
164-
func TestGroupOSContainerImageMetadataToStream_DuplicateOSImage(t *testing.T) {
165-
imagesMetadata := []*ImageData{
166158
{
167-
Image: "quay.io/openshift/os-old@sha256:111",
168-
Type: ImageTypeOS,
169-
Stream: "rhel-9",
159+
name: "OS Duplicated",
160+
imageData: []*ImageData{
161+
{
162+
Image: "quay.io/openshift/os-old@sha256:111",
163+
Type: ImageTypeOS,
164+
Stream: "rhel-9",
165+
},
166+
{
167+
Image: "quay.io/openshift/os-new@sha256:222",
168+
Type: ImageTypeOS,
169+
Stream: "rhel-9",
170+
},
171+
},
170172
},
171173
{
172-
Image: "quay.io/openshift/os-new@sha256:222",
173-
Type: ImageTypeOS,
174-
Stream: "rhel-9",
174+
name: "Extensions Duplicated",
175+
imageData: []*ImageData{
176+
{
177+
Image: "quay.io/openshift/ext-old@sha256:333",
178+
Type: ImageTypeExtensions,
179+
Stream: "rhel-9",
180+
},
181+
{
182+
Image: "quay.io/openshift/ext-new@sha256:444",
183+
Type: ImageTypeExtensions,
184+
Stream: "rhel-9",
185+
},
186+
},
175187
},
176188
}
177189

178-
result := GroupOSContainerImageMetadataToStream(imagesMetadata)
179-
180-
require.Len(t, result, 1)
181-
assert.Equal(t, "rhel-9", result[0].Name)
182-
// Last one wins
183-
assert.Equal(t, v1alpha1.ImageDigestFormat("quay.io/openshift/os-new@sha256:222"), result[0].OSImage)
184-
}
185-
186-
func TestGroupOSContainerImageMetadataToStream_DuplicateExtensionsImage(t *testing.T) {
187-
imagesMetadata := []*ImageData{
188-
{
189-
Image: "quay.io/openshift/ext-old@sha256:333",
190-
Type: ImageTypeExtensions,
191-
Stream: "rhel-9",
192-
},
193-
{
194-
Image: "quay.io/openshift/ext-new@sha256:444",
195-
Type: ImageTypeExtensions,
196-
Stream: "rhel-9",
197-
},
190+
for _, tt := range tests {
191+
t.Run(tt.name, func(t *testing.T) {
192+
result := GroupOSContainerImageMetadataToStream(tt.imageData)
193+
require.Empty(t, result, "result should be empty")
194+
})
198195
}
199-
200-
result := GroupOSContainerImageMetadataToStream(imagesMetadata)
201-
202-
require.Len(t, result, 1)
203-
assert.Equal(t, "rhel-9", result[0].Name)
204-
// Last one wins
205-
assert.Equal(t, v1alpha1.ImageDigestFormat("quay.io/openshift/ext-new@sha256:444"), result[0].OSExtensionsImage)
206196
}
207197

208198
func TestGroupOSContainerImageMetadataToStream_EmptyInput(t *testing.T) {

0 commit comments

Comments
 (0)