OLS-1829: Detect change in specified BYOK images by syedriko · Pull Request #1171 · openshift/lightspeed-operator

syedriko · 2025-12-04T05:13:10Z

Description

Implemented change detection for BYOK images.

Type of change

Related Tickets & Documents

Related Issue #
Closes # https://issues.redhat.com/browse/OLS-1829

Checklist before requesting a review

I have performed a self-review of my code.
PR has passed all pre-merge test jobs.
If it is a core feature, I have added thorough tests.

Testing

Please provide detailed steps to perform tests related to this code change.
How were the fix/results from this change verified? Please provide relevant screenshots or results.

openshift-ci-robot · 2025-12-04T05:13:14Z

syedriko · 2025-12-06T17:34:56Z

/retest

syedriko · 2025-12-07T02:28:39Z

/retest

syedriko · 2025-12-07T04:32:19Z

/retest

syedriko · 2025-12-09T02:36:50Z

/test bundle-e2e-4-20

blublinsky · 2026-02-18T12:52:28Z

internal/controller/appserver/rag.go

+		isName := utils.ImageStreamNameFor(rag.Image)
+		initContainerName := fmt.Sprintf("rag-%d", idx)
+		triggers = append(triggers, fmt.Sprintf(`{"from":{"kind":"ImageStreamTag","name":"%s:latest"},"fieldPath":"spec.template.spec.initContainers[?(@.name==\"%s\")].image"}`, isName, initContainerName))
+	}


Problem: Line 56 builds JSON as a string using fmt.Sprintf(). This is fragile and error-prone.

The better option is
// Add struct definitions at top of file
type ImageTrigger struct {
From ImageTriggerFrom json:"from"
FieldPath string json:"fieldPath"
}

type ImageTriggerFrom struct {
Kind string json:"kind"
Name string json:"name"
}

// Updated function
func generateImageStreamTriggers(cr *olsv1alpha1.OLSConfig) (string, error) {
var triggers []ImageTrigger
for idx, rag := range cr.Spec.OLSConfig.RAG {
isName := utils.ImageStreamNameFor(rag.Image)
initContainerName := fmt.Sprintf("rag-%d", idx)
triggers = append(triggers, ImageTrigger{
From: ImageTriggerFrom{
Kind: "ImageStreamTag",
Name: fmt.Sprintf("%s:latest", isName),
},
FieldPath: fmt.Sprintf(spec.template.spec.initContainers[?(@.name=="%s")].image, initContainerName),
})
}
data, err := json.Marshal(triggers)
if err != nil {
return "", fmt.Errorf("marshal image triggers: %w", err)
}
return string(data), nil
}

blublinsky · 2026-02-18T12:55:31Z

internal/controller/appserver/deployment.go

 			deployment.Spec.Template.Spec.ImagePullSecrets = cr.Spec.OLSConfig.ImagePullSecrets
 		}
+		deployment.Annotations[utils.OLSAppServerImageStreamTriggerAnnotation] = generateImageStreamTriggers(cr)
 	}


this requires error handling

triggers, err := generateImageStreamTriggers(cr)
if err != nil {
return nil, fmt.Errorf("generate image stream triggers: %w", err)
}
deployment.Annotations[utils.OLSAppServerImageStreamTriggerAnnotation] = triggers

blublinsky · 2026-02-18T13:45:27Z

internal/controller/utils/utils.go


-func DeploymentSpecEqual(a, b *appsv1.DeploymentSpec) bool {
+func DeploymentSpecEqual(a, b *appsv1.DeploymentSpec, compareInitContainers bool) bool {
 	if !apiequality.Semantic.DeepEqual(a.Template.Spec.NodeSelector, b.Template.Spec.NodeSelector) || // check node selector


This looks like a breaking change in the API
Can you make sure it does not break anything else

Every call but one to DeploymentSpecEqual() is a DeploymentSpecEqual(..., true), preserving the previous behavior:

$ ack DeploymentSpecEqual internal/controller/appserver/deployment.go 510: changed := !utils.DeploymentSpecEqual(&existingDeployment.Spec, &desiredDeployment.Spec, false) internal/controller/console/reconciler.go 210: if utils.DeploymentSpecEqual(&foundDeployment.Spec, &deployment.Spec, true) { internal/controller/lcore/deployment.go 999: changed := !utils.DeploymentSpecEqual(&existingDeployment.Spec, &desiredDeployment.Spec, true) internal/controller/postgres/deployment.go 261: changed := !utils.DeploymentSpecEqual(&existingDeployment.Spec, &desiredDeployment.Spec, true) internal/controller/utils/utils.go 245:func DeploymentSpecEqual(a, b *appsv1.DeploymentSpec, compareInitContainers bool) bool { internal/controller/utils/utils_comparison_test.go 205: Describe("DeploymentSpecEqual", func() { 234: Expect(DeploymentSpecEqual(deployment1, deployment2, true)).To(BeTrue()) 241: Expect(DeploymentSpecEqual(deployment1, deployment2, true)).To(BeFalse()) 247: Expect(DeploymentSpecEqual(deployment1, deployment2, true)).To(BeFalse()) 253: Expect(DeploymentSpecEqual(deployment1, deployment2, true)).To(BeFalse()) 259: Expect(DeploymentSpecEqual(deployment1, deployment2, true)).To(BeFalse()) 265: Expect(DeploymentSpecEqual(deployment1, deployment2, true)).To(BeFalse()) 271: Expect(DeploymentSpecEqual(deployment1, deployment2, true)).To(BeFalse()) internal/controller/utils/utils_deployment_test.go 59: Describe("DeploymentSpecEqual - Replicas", func() { 65: equal := DeploymentSpecEqual(&deployment.Spec, &desiredDeployment.Spec, true) 73: equal := DeploymentSpecEqual(&deployment.Spec, &desiredDeployment.Spec, true) 83: equal := DeploymentSpecEqual(&deployment.Spec, &desiredDeployment.Spec, true) 89: Describe("DeploymentSpecEqual - Tolerations", func() { 101: equal := DeploymentSpecEqual(&deployment.Spec, &desiredDeployment.Spec, true) 113: equal := DeploymentSpecEqual(&deployment.Spec, &desiredDeployment.Spec, true) 123: equal := DeploymentSpecEqual(&deployment.Spec, &desiredDeployment.Spec, true) 129: Describe("DeploymentSpecEqual - NodeSelector", func() { 137: equal := DeploymentSpecEqual(&deployment.Spec, &desiredDeployment.Spec, true) 147: equal := DeploymentSpecEqual(&deployment.Spec, &desiredDeployment.Spec, true) 157: equal := DeploymentSpecEqual(&deployment.Spec, &desiredDeployment.Spec, true) 163: Describe("DeploymentSpecEqual - Volumes", func() { 177: equal := DeploymentSpecEqual(&deployment.Spec, &desiredDeployment.Spec, true) 185: equal := DeploymentSpecEqual(&deployment.Spec, &desiredDeployment.Spec, true) 201: equal := DeploymentSpecEqual(&deployment.Spec, &desiredDeployment.Spec, true) 208: Describe("DeploymentSpecEqual - VolumeMounts", func() { 215: equal := DeploymentSpecEqual(&deployment.Spec, &desiredDeployment.Spec, true) 223: equal := DeploymentSpecEqual(&deployment.Spec, &desiredDeployment.Spec, true) 232: equal := DeploymentSpecEqual(&deployment.Spec, &desiredDeployment.Spec, true) 276: Describe("DeploymentSpecEqual - Resources", func() { 286: equal := DeploymentSpecEqual(&deployment.Spec, &desiredDeployment.Spec, true) 294: equal := DeploymentSpecEqual(&deployment.Spec, &desiredDeployment.Spec, true) $

The only exception is for the OLS Deployment comparison at
https://github.com/syedriko/lightspeed-operator/blob/62151b231b17983837c9859a741dd278f8c65695/internal/controller/appserver/deployment.go#L506, where the init containers are left for the image stream to deal with.

blublinsky · 2026-02-18T13:47:55Z

internal/controller/appserver/reconciler.go

+	for _, rag := range cr.Spec.OLSConfig.RAG {
+		ragImages = append(ragImages, rag.Image)
+	}
+


Do we need images validation here?

Images will get validated when CRIO will try to pull them down to the node. I don't think we can meaningfully validate images without trying to pull them. This is similar to a function that takes a file name as a string - should it validate it or let the filesystem operation fail? I would argue for letting the filesystem operation fail and tell us what's wrong with the file name or else we end up writing imperfect validation code that's going to have false positives and false negatives, depending on the OS/file system type.

blublinsky · 2026-02-18T13:52:05Z

internal/controller/utils/utils.go

+	sum := sha1.Sum([]byte(image)) //nolint:gosec
+	sfx := hex.EncodeToString(sum[:])[:6]
+	return fmt.Sprintf("%s-%s", slug, sfx)
+}


Function performs multiple string replacements and regex operations without explaining or documenting the Kubernetes name constraints it's trying to satisfy.

At least we need comments what we are doing here and why
We also need unit tests for this function

blublinsky · 2026-02-18T13:56:01Z

internal/controller/appserver/reconciler.go

+				if err := r.Update(ctx, updated); err != nil {
+					return fmt.Errorf("update ImageStream %q: %w", name, err)
+				}
+			}


This code seems suspicious

Reworked to seem less suspicious :)

blublinsky · 2026-02-18T13:58:53Z

internal/controller/appserver/rag_test.go

+				err = testReconcilerInstance.Get(ctx, types.NamespacedName{Name: utils.ImageStreamNameFor(rag.Image), Namespace: testReconcilerInstance.GetNamespace()}, &is)
+				Expect(err).NotTo(HaveOccurred())
+			}
+		})


Missing Test Coverage:
No tests for generateImageStreamTriggers() - the JSON generation function
No tests for ImageStreamNameFor() - the name generation function
No tests for edge cases:
Multiple RAG entries with same image (deduplication)
Invalid image references
ImageStream update scenarios
Orphaned ImageStream cleanup
Empty RAG list

[ x] No tests for generateImageStreamTriggers() - the JSON generation function

[ x] No tests for ImageStreamNameFor() - the name generation function

[ x] Multiple RAG entries with same image (deduplication)
? Invalid image references // did we decide ^^^ to leave validation of container image references to CRIO?

[ x] ImageStream update scenarios // this is done in the e2e test

[ x] Orphaned ImageStream cleanup

[ x] Empty RAG list

blublinsky · 2026-02-18T14:00:29Z

test/e2e/byok_test.go

+		Expect(appServerDeployment.Spec.Template.Spec.InitContainers[0].Image).To(
+			Equal(internalRegistyHostName + "/" + utils.OLSNamespaceDefault + "/" + imageName + "@" + digest),
+		)
+	})


Add documentation on this?

This Expect checks that the RAG image after the update is what we expect it to be:

image-registry.openshift-image-registry.svc:5000/openshift-lightspeed/assisted-installer-guide@sha256:...

, where sha256 is that of the "2025-2" image.
In a real deployment, the InitContainers[i].Image (i=0..n) fields are going to have sha256 digests of the most recent updates to the floating tags. That is, if OLSConfig.spec.old.rag.image[i] has quay.io/openshift-lightspeed-test/assisted-installer-guide:latest, InitContainers[i].Image is going to be quay.io/openshift-lightspeed-test/assisted-installer-guide@sha256:... that latest most recently pointed at.

What aspect are you suggesting to document? https://github.com/openshift/openshift-docs/pull/104421/changes is the documentation of the auto-update feature. To the user as they're interacting with OLSConfig, the fact that the actual OLS deployment is running on top of a particular SHA should be an implementation detail. They specify my-byok-image:latest and it simply stays 'latest' as the latest tag moves.

All integration tests have documentation on what we are trying to do. This code has close to nothing

You seem to be talking about comments

syedriko · 2026-02-19T04:22:32Z

@blublinsky I believe I've addressed all your comments, PTAL. I'll squash the commits before merging when it gets to that.

syedriko · 2026-02-23T21:49:33Z

/retest

syedriko · 2026-02-23T23:30:44Z

/test bundle-e2e-4-20

openshift-ci · 2026-02-24T01:12:53Z

@syedriko: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/lint	`b72b920`	link	true	`/test lint`

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

blublinsky · 2026-02-25T15:22:22Z

/lgtm

syedriko · 2026-02-25T15:27:09Z

/test unit

blublinsky · 2026-02-26T14:11:17Z

/lgtm

syedriko · 2026-02-26T14:45:15Z

/approve

blublinsky · 2026-02-26T15:35:36Z

/approve

openshift-ci · 2026-02-26T15:37:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: blublinsky, syedriko

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [blublinsky]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Dec 4, 2025

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 4, 2025

openshift-ci bot requested review from bparees and xrajesh December 4, 2025 05:13

syedriko force-pushed the syedriko-ols-1829 branch from 3db7729 to 1c15fa8 Compare December 4, 2025 19:43

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 4, 2025

syedriko force-pushed the syedriko-ols-1829 branch 11 times, most recently from 09a7fb1 to ba9750b Compare December 6, 2025 17:29

syedriko force-pushed the syedriko-ols-1829 branch 3 times, most recently from 1ced66f to dae62eb Compare December 7, 2025 02:23

syedriko force-pushed the syedriko-ols-1829 branch from dae62eb to 99dad6c Compare December 7, 2025 04:04

syedriko force-pushed the syedriko-ols-1829 branch 4 times, most recently from 26719f6 to 7040380 Compare December 11, 2025 01:53

syedriko force-pushed the syedriko-ols-1829 branch from 55f171f to c32aa64 Compare February 12, 2026 17:26

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 12, 2026

syedriko force-pushed the syedriko-ols-1829 branch 3 times, most recently from c01d74e to 62151b2 Compare February 16, 2026 19:54

xrajesh requested a review from blublinsky February 17, 2026 18:41

blublinsky reviewed Feb 18, 2026

View reviewed changes

syedriko force-pushed the syedriko-ols-1829 branch from 62151b2 to b72b920 Compare February 19, 2026 04:21

syedriko force-pushed the syedriko-ols-1829 branch 2 times, most recently from 259f33f to f5330ff Compare February 23, 2026 15:09

syedriko force-pushed the syedriko-ols-1829 branch from f5330ff to c2d49da Compare February 25, 2026 14:51

openshift-ci bot assigned blublinsky Feb 25, 2026

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 25, 2026

syedriko added 2 commits February 26, 2026 09:08

OLS-1829: Detect change in specified BYOK images

8f46b9e

OLS-1829: review fixes

0f3de2b

syedriko force-pushed the syedriko-ols-1829 branch from c2d49da to 0f3de2b Compare February 26, 2026 14:10

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 26, 2026

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 26, 2026

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 26, 2026

openshift-merge-bot bot merged commit c4d5319 into openshift:main Feb 26, 2026
16 checks passed

Conversation

syedriko commented Dec 4, 2025

Description

Type of change

Related Tickets & Documents

Checklist before requesting a review

Testing

Uh oh!

openshift-ci-robot commented Dec 4, 2025 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

Related Tickets & Documents

Checklist before requesting a review

Testing

Uh oh!

syedriko commented Dec 6, 2025

Uh oh!

syedriko commented Dec 7, 2025

Uh oh!

syedriko commented Dec 7, 2025

Uh oh!

syedriko commented Dec 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

syedriko Feb 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

syedriko commented Feb 19, 2026

Uh oh!

syedriko commented Feb 23, 2026

Uh oh!

syedriko commented Feb 23, 2026

Uh oh!

openshift-ci bot commented Feb 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blublinsky commented Feb 25, 2026

Uh oh!

syedriko commented Feb 25, 2026

Uh oh!

blublinsky commented Feb 26, 2026

Uh oh!

syedriko commented Feb 26, 2026

Uh oh!

blublinsky commented Feb 26, 2026

openshift-ci-robot commented Dec 4, 2025 •

edited by openshift-ci bot

Loading

syedriko Feb 18, 2026 •

edited

Loading

openshift-ci bot commented Feb 24, 2026 •

edited

Loading