Don't override relatedImages in config CSV.

[kaovilai]

Bug Report

Looks like #5000 has reappeared after related image discovery features were added. This has caused us to change how we specify relatedImages in our operator openshift/oadp-operator#694

Seems to be sideeffect from related image discovery feature. Not sure which was the first PR for that.

Related PRs: #5567

What did you do?

operatorsdk generate bundle.

What did you expect to see?

relatedImages in config CSV preserved

What did you see instead? Under which circumstances?

relatedImages in config CSV removed from generated bundle

Environment

Operator type:

/language go

$ operator-sdk version

operator-sdk version: "v1.20.0", commit: "deb3531ae20a5805b7ee30b71f13792b80bd49b1", kubernetes version: "v1.23", go version: "go1.18.1", GOOS: "darwin", GOARCH: "amd64"
$ go version (if language is Go)

go version go1.18.1 darwin/amd64
$ kubectl version

Possible Solution

Don't override but append to existing relatedImages.

Additional context

[kaovilai]

/kind bug

[jackson-chris]

Same issue in v1.19.1. Can confirm this issue does NOT happen in v1.9.0.

[kaovilai]

Thanks @blindenvy for the investigation. Do you know if 1.9 is the latest that worked? or just what you tried.

Looks like my fix at #5765 maybe put on pause until they discuss use case further. If you could add a comment or two there that would be awesome.

[ryantking]

So I confirmed that the related image discovery feature does overwrite the base CSV related images. This was an unintended breaking change that I did not notice when implementing that feature so was not documented correctly. The current official way to define related Images is with environment variables that start with RELATED_IMAGE_ in the manager.yaml file. These will be discovered and added to the CSV spec then the application can access the image reference by reading RELATED_IMAGE_MYIMAGE from the environment. If possible, anyone experiencing this issue should move their related image definitions to the manager.yaml file or any other container environment that needs access to related images. More details are in the latest documentation.

Since this is a breaking change, we will be doing further investigation on a resolution. As I said in the PR comments, we will at the very least get a documentation change up with migration instructions. Ideally this should be unbroken, but figuring out how to do it correctly without breaking the de-dup logic will require further investigation/discussion, which we can use this issue for.

The root issue is that the de-dup logic handles the following kinds of duplicates:

1. Same name and image: Just collapsed into one entry. This could happen if you have the same related image in two containers. Both containers could need the environment variable, but you only need one entry in the related images.
2. Same name and different image: Prefix the image name with its source container/deployment. This happens if two containers use the same name to represent a different image. This one specifically breaks with the fix proposed in Don't override relatedImages in base CSV. #5763 #5765 because related images defined in the baseCSV lose the container/deployment name.
3. Different name and same image: We only want each image to be listed once so if the same image is referenced by multiple names we cannot determine what name to use so we notify the user with a warning message and use an empty name.

The path of least resistence to me seems to be writing entirely separate logic for bringing over related images from the base CSV and only adding them to the final CSV if the images don't exist. So that way if you have no related images defined in manager.yaml it will copy them all over, if you have only related images defined in manager.yaml it will continue to work, and if you have a mix, it will output the union of both sets.

The only outstanding question I have is what to do with conflicts across base CSV and manager environment. For example, if we have a base CSV with:

relatedImages:
- name: myImage
  image: some-image:latest

and a manager.yaml with:

env:
- name: RELATED_IMAGE_NOTMYIMAGE
  value: some-image:latest

What name would we want for some-image:latest, maintain myImage from the baseCSV, overwrite it with notmyimage from manager.yaml, or allow duplicate images in this one specific case?

Similar issue if the image overlaps but the name is different. Do we empty string out the name? Do we also allow duplicates here?

My gut says we can view definitions in the base CSV as "overrides" so they always take precedent over what we discover. So for each related image in the base CSV:

1. If a related image with the same name/image was discovered, do nothing. Its already there.
2. If a related image with the same name was discovered, prefix the discovered name with its container name, leave the base CSV definition as is and add it to the final CSV.
3. If a related image with the same image was discovered, just add it in. Maybe warn the user an image is used twice.
4. If there's no collision, just append it.

What do you think?

CC: @joelanford

[kaovilai]

I don't mind either. I err on the preference of preferring notmyimage from RELATED_IMAGE_ overriding the baseCSV since you're probably in the process of migrating and are aware that ENV RELAED_IMAGE_ affects your final relatedImage.

Even if you don't know about RELATED_IMAGE_ in ENV, it would be hard to miss scrolling through CSV.

Otherwise leave baseCSV relatedImage as is if not specified in ENV.

[ryantking]

I think there's a good case for both. It really comes down to if we want to say definitions in the base CSV are overrides or a deprecated way to define relatd images that we still support. If its the former, the base CSV definitions should take precedence and vise versa for the latter.

[kaovilai]

overrides or a deprecated

I think sticking with overrides will be best. Which overrides which is up for discussion.

Deprecation should be introduced slowly.