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

Cleanup: Add new imagePullSecret field to CRD #99

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

Conversation

tcrawley-xilinx
Copy link
Contributor

@tcrawley-xilinx tcrawley-xilinx commented Dec 8, 2023

Users may want to pull from a private registry (such as ghcr) but without adding a global secret, this change allows users to specify a secret for the Onload CR which is then passed to the device plugin and the module(s).

I decided to only add a single value since I assumed that if a user is using a private registry then they will require the same credentials for all of the onload images and the device plugin. And since the Module CRD only exposes a single pull secret for pulling images I kept it to a single value.

Testing done

If omitted the operands are the same as before.
If present the value is present for each operand.

  • Test that the secrets actually work as intended - Tested externally in cluster

@tcrawley-xilinx tcrawley-xilinx force-pushed the reviews/tcrawley/imagepullsecret branch from f8f1280 to 1b50174 Compare January 8, 2024 08:46
Copy link
Collaborator

@pcolledg-amd pcolledg-amd left a comment

Choose a reason for hiding this comment

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

As with the insecure registry config, I'm more in favour of keeping Onload CRD clean and using built-in support. If a user wants to scope the token narrower than global cluster config, could they not configure in the ServiceAccount instead?

If we're going to deviate from KMM's unusual ImageRepoSecret, we should standarise on the Kubernetes plural property, ImagePullSecrets.

@tcrawley-xilinx
Copy link
Contributor Author

As with the insecure registry config, I'm more in favour of keeping Onload CRD clean and using built-in support. If a user wants to scope the token narrower than global cluster config, could they not configure in the ServiceAccount instead?

Potentially, I hadn't really thought about that approach. My (slight) concern with that approach is that both the Module CRD and PodTemplateSpec provide options for both ServiceAccountName and imagePullSecrets (or equivalent). I think service account permissions could work as a short term option while we decide what to do with the CRD.

If we're going to deviate from KMM's unusual ImageRepoSecret, we should standarise on the Kubernetes plural property, ImagePullSecrets.

I wanted to use the same data for both the device plugin (which uses ImagePullSecrets) and the modules (which uses ImageRepoSecret), so I don't think a plural solution would work. Regarding the name I don't have a strong opinion, I think I wanted to go for something more k8s-y since it isn't just used for the Module.

@tcrawley-xilinx tcrawley-xilinx force-pushed the reviews/tcrawley/imagepullsecret branch from 1b50174 to d6aa14d Compare January 11, 2024 17:31
@tcrawley-xilinx tcrawley-xilinx force-pushed the reviews/tcrawley/imagepullsecret branch from d6aa14d to db81820 Compare February 2, 2024 16:54
@tcrawley-xilinx tcrawley-xilinx marked this pull request as ready for review February 2, 2024 16:54
@tcrawley-xilinx tcrawley-xilinx requested a review from a team as a code owner February 2, 2024 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants