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

CAA: add support to look up imagePullSecrets for pods #2232

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

Conversation

squarti
Copy link
Contributor

@squarti squarti commented Jan 9, 2025

This PR initializes auth.json with the imagePullSecrets listed on the pod and service account.

Fixes: #2231

@squarti squarti requested a review from a team as a code owner January 9, 2025 16:41
@stevenhorsman stevenhorsman added the test_e2e_libvirt Run Libvirt e2e tests label Jan 9, 2025
Copy link
Collaborator

@mkulke mkulke left a comment

Choose a reason for hiding this comment

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

thx, looks good! the override semantics are interesting here, I'd intuitively expect that a cluster-wide auth.json is overridden by a pod-specific auth.json, but we're doing the opposite here. some questions:

does it make sense to retire the existing caa-wide "auth.json" mechanism in favor of passing this from kubernetes? the caa-wide param is roughly comparable to a node-wide auth.json in the kubelet config, which is discouraged (and which we don't consider it here, either).

I would suggest to at least deprecate the parameter and mark it for later removal.

If we don't want to remove the caa-wide auth.json, would we be able to "merge" both sources?

if we don't want to remove the caa-wide auth.json, would it make sense for pod-specific auth.json to take precedence?

What are the implications for auth.json that come from KBS?

@@ -259,6 +259,17 @@ func (s *cloudService) CreateVM(ctx context.Context, req *pb.CreateVMRequest) (r
_, err = os.Stat(SrcAuthfilePath)
if err != nil {
logger.Printf("credential file %s is not present, skipping image auth config", SrcAuthfilePath)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess the log message needs to be adjusted, to indicate that we fall back on image-pull-secrets from the pod

@stevenhorsman
Copy link
Member

@bpradipt @snir911 - I think you added the CAA wide auth.json, so wanted to check if you had any concerned with this. I'm inclined to agree with Magnus, that it would be good to replace the CAA-wide approach with a pod specific one, but am unsure if this would cause challenges with the pause image pull on OpenShift?

@bpradipt
Copy link
Member

@bpradipt @snir911 - I think you added the CAA wide auth.json, so wanted to check if you had any concerned with this. I'm inclined to agree with Magnus, that it would be good to replace the CAA-wide approach with a pod specific one, but am unsure if this would cause challenges with the pause image pull on OpenShift?

Afaicr, the pause image is embedded now.
Overall I like this approach, but I was thinking that imagePullSecrets, if present, would replace the default cluster wide auth.json as Magnus had mentioned.
In the current of using serviceAccount and imagePullSecrets may be we can have a flexibility to specify which service account to retrieve from. That could be a CAA config with default being the default SA. This will avoid any need for cluster wide auth.json to be used.

@squarti
Copy link
Contributor Author

squarti commented Jan 10, 2025

thx, looks good! the override semantics are interesting here, I'd intuitively expect that a cluster-wide auth.json is overridden by a pod-specific auth.json, but we're doing the opposite here. some questions:

I left the cluster wide auth.json overwriting the pod specific secrets because this way current code that uses cluster wide auth.json would not be broken. I think merging the secrets would also be ok as long as the pod secrets do not conflict with the cluster wide secret.

Let me know which approach to take and I will adjust the changes.

@mkulke
Copy link
Collaborator

mkulke commented Jan 10, 2025

thx, looks good! the override semantics are interesting here, I'd intuitively expect that a cluster-wide auth.json is overridden by a pod-specific auth.json, but we're doing the opposite here. some questions:

I left the cluster wide auth.json overwriting the pod specific secrets because this way current code that uses cluster wide auth.json would not be broken. I think merging the secrets would also be ok as long as the pod secrets do not conflict with the cluster wide secret.

Let me know which approach to take and I will adjust the changes.

I would opt for removing the option to have a cluster-wide auth.json in this case. If I understood it correctly there is no use case for that anymore. Propagating it from the cluster is the right call, IMO. since the manifest will still be pulled on the node different maintaining different sets of registry credentials in k8s and caa config can be pretty confusing.

@squarti squarti force-pushed the pull-secret branch 7 times, most recently from fdf8aa6 to 42e1cf8 Compare January 10, 2025 23:16
This PR initializes auth.json with the imagePullSecrets listed on the
pod and service account.

Fixes: confidential-containers#2231


Signed-off-by: Silenio Quarti <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test_e2e_libvirt Run Libvirt e2e tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CAA: imagePullSecrets are not supported
4 participants