Skip to content

Reapply "MCO-2120: Rework OS Image Stream label classification" #5767

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
cheesesashimi wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Reapply "MCO-2120: Rework OS Image Stream label classification" #5767

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 58 additions & 11 deletions pkg/osimagestream/image_data.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package osimagestream
import (
"maps"
"slices"
"strings"

"github.com/openshift/api/machineconfiguration/v1alpha1"
"k8s.io/klog/v2"
Expand All @@ -11,8 +12,15 @@ import (
const (
// coreOSLabelStreamClass is the container image label that identifies the OS stream name.
coreOSLabelStreamClass = "io.openshift.os.streamclass"
// coreOSLabelDiscriminator is the container image label that distinguishes OS images from Extensions images.
coreOSLabelDiscriminator = "ostree.linux"

// coreOSLabelExtension is the container image label that identifies an extensions image.
coreOSLabelExtension = "io.openshift.os.extensions"
coreOSLabelExtensionValue = "true"

// coreOSLabelBootc is the container image label present on bootc-based OS images.
coreOSLabelBootc = "containers.bootc"
coreOSLabelBootcValueBool = "true"
coreOSLabelBootcValueInteger = "1"
)

// ImageType indicates whether a container image is an OS image or an extensions image.
Expand Down Expand Up @@ -40,12 +48,16 @@ type ImageData struct {
Stream string // OS stream name
}

// ImageLabelMatcher is a function that checks whether a set of container image
// labels matches a specific criterion.
type ImageLabelMatcher func(labels map[string]string) bool

// ImageStreamExtractorImpl implements ImageDataExtractor using container image labels
// to identify and classify OS and extensions images.
type ImageStreamExtractorImpl struct {
imagesInspector ImagesInspector
streamLabels []string
osImageDiscriminator string
streamLabels []string
osImageMatchers []ImageLabelMatcher
extensionsImageMatchers []ImageLabelMatcher
}

// NewImageStreamExtractor creates a new ImageDataExtractor configured to recognize
Expand All @@ -54,14 +66,19 @@ func NewImageStreamExtractor() ImageDataExtractor {
// The type is thought to allow future extra label addition for
// i.e. Allow a customer to bring their own images with their own labels (defining a selector)
return &ImageStreamExtractorImpl{
streamLabels: []string{coreOSLabelStreamClass},
osImageDiscriminator: coreOSLabelDiscriminator,
streamLabels: []string{coreOSLabelStreamClass},
osImageMatchers: []ImageLabelMatcher{
labelEquals(coreOSLabelBootc, coreOSLabelBootcValueBool, coreOSLabelBootcValueInteger),
},
extensionsImageMatchers: []ImageLabelMatcher{
labelEquals(coreOSLabelExtension, coreOSLabelExtensionValue),
},
}
}

// GetImageData analyzes container image labels to extract OS stream metadata.
// Returns nil if the image does not have the required stream label.
// Distinguishes between OS and extensions images based on the presence of the ostree discriminator label.
// Returns nil if the image does not have the required stream label or if it
// cannot be classified as either an OS or extensions image.
func (e *ImageStreamExtractorImpl) GetImageData(image string, labels map[string]string) *ImageData {
imageData := &ImageData{
Image: image,
Expand All @@ -72,10 +89,14 @@ func (e *ImageStreamExtractorImpl) GetImageData(image string, labels map[string]
return nil
}

if findLabelValue(labels, e.osImageDiscriminator) != "" {
// Determine the type of image. OS or extensions.
switch {
case matchesAny(labels, e.osImageMatchers):
imageData.Type = ImageTypeOS
} else {
case matchesAny(labels, e.extensionsImageMatchers):
imageData.Type = ImageTypeExtensions
default:
return nil
Comment on lines +93 to +99
Copy link

@coderabbitai coderabbitai bot Mar 16, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Check extensions before OS matchers.

If an extensions image also carries containers.bootc, Line 94 classifies it as OS before Line 96 sees the extensions marker. GroupOSContainerImageMetadataToStream() then ends up with two OS images and drops the stream as incomplete. Please also add a regression case with both labels present.

Possible fix 
 	switch {
-	case matchesAny(labels, e.osImageMatchers):
-		imageData.Type = ImageTypeOS
 	case matchesAny(labels, e.extensionsImageMatchers):
 		imageData.Type = ImageTypeExtensions
+	case matchesAny(labels, e.osImageMatchers):
+		imageData.Type = ImageTypeOS
 	default:
 		return nil
 	}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
switch {
case matchesAny(labels, e.osImageMatchers):
imageData.Type = ImageTypeOS
} else {
case matchesAny(labels, e.extensionsImageMatchers):
imageData.Type = ImageTypeExtensions
default:
return nil
switch {
case matchesAny(labels, e.extensionsImageMatchers):
imageData.Type = ImageTypeExtensions
case matchesAny(labels, e.osImageMatchers):
imageData.Type = ImageTypeOS
default:
return nil
}
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@pkg/osimagestream/image_data.go` around lines 93 - 99, The switch currently
checks matchesAny(labels, e.osImageMatchers) before matchesAny(labels,
e.extensionsImageMatchers), which misclassifies images that have both OS and
extensions labels; update the ordering to check matchesAny(labels,
e.extensionsImageMatchers) first so images with extension markers are classified
as ImageTypeExtensions, and ensure ImageTypeOS remains fallback. Modify the
logic in the same block referencing e.osImageMatchers,
e.extensionsImageMatchers, ImageTypeOS, and ImageTypeExtensions to reflect this
ordering change, and add a regression test exercising
GroupOSContainerImageMetadataToStream() with a single image that carries both
the extensions label and an OS label to assert it becomes an extensions image
(and does not produce duplicate OS images).

Copy link
Contributor

@pablintino pablintino Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then ends up with two OS images and drops the stream as incomplete

And that's exactly what we want for a malformed image set. This is fine.

Copy link

@coderabbitai coderabbitai bot Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pablintino, understood — an image carrying both containers.bootc and io.openshift.os.extensions is considered malformed, and letting it be classified as OS (resulting in two OS images and the stream being silently dropped by GroupOSContainerImageMetadataToStream) is the intentional handling. I'll retract my concern.

(^･o･^)ﾉ"

📝 Learnings were identified and not saved (knowledge base disabled). Enable

}

return imageData
Expand Down Expand Up @@ -150,3 +171,29 @@ func NewOSImageStreamURLSetFromImageMetadata(imageMetadata *ImageData) *v1alpha1
}
return urlSet
}

// labelEquals returns a matcher that checks whether a label has the expected value (case-insensitive).
func labelEquals(key string, values ...string) ImageLabelMatcher {
return func(labels map[string]string) bool {
v, ok := labels[key]
if !ok {
return false
}
for _, value := range values {
if strings.EqualFold(v, value) {
return true
}
}
return false
}
}

// matchesAny returns true if any of the matchers match the given labels.
func matchesAny(labels map[string]string, matchers []ImageLabelMatcher) bool {
for _, m := range matchers {
if m(labels) {
return true
}
}
return false
}
94 changes: 70 additions & 24 deletions pkg/osimagestream/image_data_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,35 +9,81 @@ import (
"github.com/stretchr/testify/require"
)

func TestImageStreamExtractorImpl_GetImageData_OSImage(t *testing.T) {
extractor := NewImageStreamExtractor()

labels := map[string]string{
"io.openshift.os.streamclass": "rhel-9",
"ostree.linux": "present",
}

result := extractor.GetImageData("quay.io/openshift/os@sha256:abc123", labels)

require.NotNil(t, result)
assert.Equal(t, "quay.io/openshift/os@sha256:abc123", result.Image)
assert.EqualValues(t, ImageTypeOS, result.Type)
assert.Equal(t, "rhel-9", result.Stream)
}
const (
testCoreOSLabelStreamClass = "io.openshift.os.streamclass"
testCoreOSLabelExtension = "io.openshift.os.extensions"
testCoreOSLabelExtensionValue = "true"
testCoreOSLabelBootc = "containers.bootc"
testCoreOSLabelBootcValue1 = "1"
testCoreOSLabelBootcValueTrue = "true"
)

func TestImageStreamExtractorImpl_GetImageData_ExtensionsImage(t *testing.T) {
func TestImageStreamExtractorImpl_GetImageData_ImageType(t *testing.T) {
extractor := NewImageStreamExtractor()

labels := map[string]string{
"io.openshift.os.streamclass": "rhel-9",
tests := []struct {
label string
labelValue string
imageType ImageType
shouldMatch bool
}{
{
label: testCoreOSLabelBootc,
labelValue: testCoreOSLabelBootcValueTrue,
shouldMatch: true,
imageType: ImageTypeOS,
},
{
label: testCoreOSLabelBootc,
labelValue: testCoreOSLabelBootcValue1,
shouldMatch: true,
imageType: ImageTypeOS,
},
{
label: testCoreOSLabelBootc,
labelValue: "2",
shouldMatch: false,
},
{
label: testCoreOSLabelBootc,
labelValue: "nope",
shouldMatch: false,
},
{
label: testCoreOSLabelExtension,
labelValue: testCoreOSLabelExtensionValue,
shouldMatch: true,
imageType: ImageTypeExtensions,
},
{
label: testCoreOSLabelExtension,
shouldMatch: false,
},
{
label: "do.not.match",
shouldMatch: false,
},
}

result := extractor.GetImageData("quay.io/openshift/ext@sha256:def456", labels)

require.NotNil(t, result)
assert.Equal(t, "quay.io/openshift/ext@sha256:def456", result.Image)
assert.EqualValues(t, ImageTypeExtensions, result.Type)
assert.Equal(t, "rhel-9", result.Stream)
for _, tt := range tests {
t.Run(tt.label, func(t *testing.T) {

result := extractor.GetImageData(
"quay.io/openshift/os@sha256:abc123",
map[string]string{
testCoreOSLabelStreamClass: "rhel-9",
tt.label: tt.labelValue,
})
if tt.shouldMatch {
require.NotNil(t, result)
assert.Equal(t, "quay.io/openshift/os@sha256:abc123", result.Image)
assert.EqualValues(t, tt.imageType, result.Type)
assert.Equal(t, "rhel-9", result.Stream)
} else {
assert.Nil(t, result)
}
})
}
}

func TestImageStreamExtractorImpl_GetImageData_MissingStreamLabel(t *testing.T) {
Expand Down
40 changes: 20 additions & 20 deletions pkg/osimagestream/imagestream_source_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,6 @@ const (
testRHELCoreosTagName = "rhel-coreos-9.4"
testRHELCoreosExtTagName = "rhel-coreos-extensions-9.4"

labelStreamClass = "io.openshift.os.streamclass"
labelOSTreeLinux = "ostree.linux"

labelValuePresent = "present"

streamNameRHELCoreos = "rhel-coreos"
)

Expand Down Expand Up @@ -87,16 +82,17 @@ func TestImageStreamStreamSource_FetchStreams(t *testing.T) {
Image: testRHELCoreosImage,
InspectInfo: &types.ImageInspectInfo{
Labels: map[string]string{
labelStreamClass: streamNameRHELCoreos,
labelOSTreeLinux: labelValuePresent,
testCoreOSLabelStreamClass: streamNameRHELCoreos,
testCoreOSLabelBootc: testCoreOSLabelBootcValue1,
},
},
},
{
Image: testRHELCoreosExtensionsImage,
InspectInfo: &types.ImageInspectInfo{
Labels: map[string]string{
labelStreamClass: streamNameRHELCoreos,
testCoreOSLabelStreamClass: streamNameRHELCoreos,
testCoreOSLabelExtension: testCoreOSLabelExtensionValue,
},
},
},
Expand Down Expand Up @@ -151,16 +147,17 @@ func TestImageStreamStreamSource_FetchStreams(t *testing.T) {
Image: "quay.io/openshift/custom-os:latest",
InspectInfo: &types.ImageInspectInfo{
Labels: map[string]string{
labelStreamClass: "custom-stream",
testCoreOSLabelStreamClass: "custom-stream",
testCoreOSLabelBootc: testCoreOSLabelBootcValue1,
},
},
},
{
Image: "quay.io/openshift/custom-os-extensions:latest",
InspectInfo: &types.ImageInspectInfo{
Labels: map[string]string{
labelStreamClass: "custom-stream",
labelOSTreeLinux: labelValuePresent,
testCoreOSLabelStreamClass: "custom-stream",
testCoreOSLabelExtension: testCoreOSLabelExtensionValue,
},
},
},
Expand Down Expand Up @@ -205,16 +202,17 @@ func TestImageStreamStreamSource_FetchStreams(t *testing.T) {
Image: testRHELCoreosImage,
InspectInfo: &types.ImageInspectInfo{
Labels: map[string]string{
labelStreamClass: streamNameRHELCoreos,
testCoreOSLabelStreamClass: streamNameRHELCoreos,
testCoreOSLabelBootc: testCoreOSLabelBootcValue1,
},
},
},
{
Image: testRHELCoreosExtensionsImage,
InspectInfo: &types.ImageInspectInfo{
Labels: map[string]string{
labelStreamClass: streamNameRHELCoreos,
labelOSTreeLinux: labelValuePresent,
testCoreOSLabelStreamClass: streamNameRHELCoreos,
testCoreOSLabelExtension: testCoreOSLabelExtensionValue,
},
},
},
Expand Down Expand Up @@ -266,33 +264,35 @@ func TestImageStreamStreamSource_FetchStreams(t *testing.T) {
Image: testRHELCoreosImage,
InspectInfo: &types.ImageInspectInfo{
Labels: map[string]string{
labelStreamClass: streamNameRHELCoreos,
testCoreOSLabelStreamClass: streamNameRHELCoreos,
testCoreOSLabelBootc: testCoreOSLabelBootcValue1,
},
},
},
{
Image: testRHELCoreosExtensionsImage,
InspectInfo: &types.ImageInspectInfo{
Labels: map[string]string{
labelStreamClass: streamNameRHELCoreos,
labelOSTreeLinux: labelValuePresent,
testCoreOSLabelStreamClass: streamNameRHELCoreos,
testCoreOSLabelExtension: testCoreOSLabelExtensionValue,
},
},
},
{
Image: "quay.io/openshift/stream-coreos:10.0",
InspectInfo: &types.ImageInspectInfo{
Labels: map[string]string{
labelStreamClass: "stream-coreos",
testCoreOSLabelStreamClass: "stream-coreos",
testCoreOSLabelBootc: testCoreOSLabelBootcValue1,
},
},
},
{
Image: "quay.io/openshift/stream-coreos-extensions:10.0",
InspectInfo: &types.ImageInspectInfo{
Labels: map[string]string{
labelStreamClass: "stream-coreos",
labelOSTreeLinux: labelValuePresent,
testCoreOSLabelStreamClass: "stream-coreos",
testCoreOSLabelExtension: testCoreOSLabelExtensionValue,
},
},
},
Expand Down
Loading