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

feat: new field for image suffix in installation #2988

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

Conversation

stormqueen1990
Copy link

@stormqueen1990 stormqueen1990 commented Nov 7, 2023

Description

Create a new field imageSuffix to allow users to specify a suffix for images deployed. This suffix follows the same rules as the already-existing field imagePrefix. This affects the Installation resource in the operator.tigera.io/v1 group/version.

The goal of this change is to enable usage of images with Tigera Operator when the general format is

my.registry/project/<prefix>-<image-name>-<suffix>:<tag>

as it is currently not possible to use the operator with images for which names end with a standardized suffix (i.e. my.registry/project/calico-pod2daemon-flexvol-test).

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.

Create the new field imageSuffix to allow users to specify a suffix for images
deployed. This suffix follows the same rules as the already-existing field
imagePrefix.

Signed-off-by: Mauren Berti <mauren.berti@chainguard.dev>
@CLAassistant
Copy link

CLAassistant commented Nov 7, 2023

CLA assistant check
All committers have signed the CLA.

@caseydavenport caseydavenport marked this pull request as ready for review November 9, 2023 16:36
@caseydavenport caseydavenport requested a review from a team as a code owner November 9, 2023 16:36
@caseydavenport
Copy link
Member

@stormqueen1990 thanks for the PR! So far as the code goes, it looks sensible to me.

I am curious what your use-case is for a suffix here - is there a related issue? Or could you add a brief description of the use-case to the PR description?

@stormqueen1990
Copy link
Author

@stormqueen1990 thanks for the PR! So far as the code goes, it looks sensible to me.

I am curious what your use-case is for a suffix here - is there a related issue? Or could you add a brief description of the use-case to the PR description?

Hi there, @caseydavenport! The use case we have is a set of images in the format my.registry/project/a-prefix-<image-name>-a-suffix. Those can't be deployed with the current state of Installation without changing their tags, since it doesn't allow for a suffix to be appended.

I didn't file an issue. Will add this to the PR description.

@tmjd
Copy link
Member

tmjd commented Nov 9, 2023

Yeah the PR code looks sensible to me. My hesitation would be that I don't see what this would allow that couldn't also be achieved with images tagged like

my.registry/project/<prefix>-<suffix>-<image-name>:<tag>

So I hesitate to add a new field to satisfy a request to have a common suffix as there is no functional difference to my knowledge of <prefix>-<image-name>-<suffix> and <prefix>-<suffix>-<image-name>.

@caseydavenport
Copy link
Member

caseydavenport commented Nov 9, 2023

Yep, that is what I was thinking as well @tmjd. Basically by question is "why not use the existing prefix capability?"

If we want to introduce two ways of satisfying the same high level goal - distinguishing like images within a registry - it should be clear when one should be used vs. the other.

@stormqueen1990
Copy link
Author

Yeah the PR code looks sensible to me. My hesitation would be that I don't see what this would allow that couldn't also be achieved with images tagged like

my.registry/project/<prefix>-<suffix>-<image-name>:<tag>

So I hesitate to add a new field to satisfy a request to have a common suffix as there is no functional difference to my knowledge of <prefix>-<image-name>-<suffix> and <prefix>-<suffix>-<image-name>.

Hi there, @tmjd and @caseydavenport!

<prefix>-<suffix> doesn't really address the use case we have at Chainguard. We have a pattern of adding suffixes to identify specific image variants (for example, a FIPS variant of an image), so moving the suffix around when naming the images breaks our pattern.

@malavikabala
Copy link

Hello! It might be worth adding some context here that Chainguard is a popular security tool that many Calico users are using as part of their supply chain security efforts. It would be great if we could help @stormqueen1990 and the Chainguard team more easily manage the variants. :)

@tmjd
Copy link
Member

tmjd commented Nov 15, 2023

I think the issue here is that the operator is opinionated in that it isn't meant to install different variants of images. The reasoning for allowing a "prefix" was for cases where a registry was flat (no sub-directories) and a user wanted to be able to have something like <registry>/calico-<image>:<tag>.
So suggesting that a registry needs .../calico-<image>-<variant1>:<tag> and .../calico-<image>-<variant1>:<tag> is something the operator is specifically not intending to support.
If someone wants to build different variants of the calico images then it seems within reason that they could build their own version of an operator to install those versions.

@caseydavenport
Copy link
Member

caseydavenport commented Nov 15, 2023

I agree with @tmjd - a core principle of this project is to prevent installing versions of our code that we haven't blessed in combination with the operator code, as we have historically seen that to result in issues for users. I understand your desire to use a suffix here, and have no problem with it fundamentally - but I don't think it's an appropriate gun to put into the hands of the remaining 99% of this project's users.

If using one of the several other configurable options (prefix, tag) doesn't work for your flow, I'd probably suggest a custom build of this operator to support suffixes if you're already going to be rebuilding the other Calico images.

(for example, a FIPS variant of an image),

For FIPS in particular, we natively support FIPS and it can be enabled already via the operator's API:

// FIPSMode uses images and features only that are using FIPS 140-2 validated cryptographic modules and standards.
// Default: Disabled
// +kubebuilder:validation:Enum=Enabled;Disabled
// +optional
FIPSMode *FIPSMode `json:"fipsMode,omitempty"`

@danudey danudey modified the milestones: v1.32.0, v1.32.1, v1.32.2 Dec 2, 2023
@danudey danudey modified the milestones: v1.32.2, v1.32.3 Dec 15, 2023
@danudey danudey modified the milestones: v1.32.3, v1.32.4 Jan 18, 2024
@radTuti radTuti modified the milestones: v1.32.4, v1.32.5, v1.32.6 Feb 16, 2024
@radTuti radTuti modified the milestones: v1.32.6, v1.32.7 Mar 25, 2024
@danudey danudey removed this from the v1.32.7 milestone Apr 2, 2024
@danudey danudey added this to the v1.32.8 milestone Apr 2, 2024
@radTuti radTuti modified the milestones: v1.32.8, v1.32.9 Apr 26, 2024
@danudey danudey modified the milestones: v1.32.9, v1.32.10 Jun 14, 2024
@danudey danudey modified the milestones: v1.32.10, v1.32.11 Jul 9, 2024
@danudey danudey modified the milestones: v1.32.11, v1.32.12 Aug 19, 2024
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.

8 participants