Skip to content
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.

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

Fix image component handling, honoring Installation spec #3491

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

Conversation

petrutlucian94
Copy link

components.GetReference currently ignores the image prefix and path specified in the "Installation" spec.

It makes a copy of the image component, appends the configured image path and prefix but then it uses the original component image when looping over the image set.

The fix is trivial, we just have to use the modified image string.

Description

For PR author

  • Tests for change.
  • If changing pkg/apis/, run make gen-files
  • If changing versions, run make gen-versions

For PR reviewers

A note for code reviewers - all pull requests must have the following:

  • Milestone set according to targeted release.
  • Appropriate labels:
    • kind/bug if this is a bugfix.
    • kind/enhancement if this is a a new feature.
    • enterprise if this PR applies to Calico Enterprise only.

components.GetReference currently ignores the image prefix and path
specified in the "Installation" spec.

It makes a copy of the image component, appends the configured image
path and prefix but then it uses the original component image when
looping over the image set.

The fix is trivial, we just have to use the modified image string.
@CLAassistant
Copy link

CLAassistant commented Sep 12, 2024

CLA assistant check
All committers have signed the CLA.

@petrutlucian94
Copy link
Author

petrutlucian94 commented Sep 12, 2024

Looks like there is another blocker that prevents us from using custom built images. We could pass in the install spec (or just the image path/prefix) and apply them just like GetReference does.

func ValidateImageSet(is *operator.ImageSet) error {

"ImageSet calico-v3.28.0: unexpected images: petrutlucian94/calico-node, petrutlucian94/calico-cni, petrutlucian94/calico-typha, petrutlucian94/calico-kube-controllers, petrutlucian94/calico-tigera-operator, petrutlucian94/calico-csi, petrutlucian94/calico-apiserver, petrutlucian94/calico-ctl, petrutlucian94/calico-pod2daemon-flexvol, petrutlucian94/calico-key-cert-provisioner",

@tmjd
Copy link
Member

tmjd commented Sep 23, 2024

For an ImageSet we need a reference to match an ImageSet Digest to the image it will be used with. I would suggest we document that the "Image" value in the ImageSet is the original (unmodified) image name and ignores any registry, path, or prefix changes.
This would ensure there is no change in behavior for users that have figured out this is the way the ImageSets work.

@radTuti radTuti modified the milestones: v1.36.0, v1.36.1 Oct 28, 2024
@caseydavenport caseydavenport modified the milestones: v1.36.1, v1.37.0 Oct 28, 2024
@petrutlucian94
Copy link
Author

Please let me know which document should be updated. The ImageSet docstring already covers this:

// Image is an image that the operator deploys and instead of using the built in tag
// the operator will use the Digest for the image identifier.
// The value should be the image name without registry or tag or digest.
// For the image `docker.io/calico/node:v3.17.1` it should be represented as `calico/node`
Image string `json:"image"`

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants