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

squarti · 2025-01-09T16:41:49Z

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

mkulke

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?

mkulke · 2025-01-10T08:49:06Z

src/cloud-api-adaptor/pkg/adaptor/cloud/cloud.go

@@ -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)


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

stevenhorsman · 2025-01-10T10:15:33Z

@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 · 2025-01-10T12:15:40Z

@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 · 2025-01-10T14:20:03Z

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 · 2025-01-10T14:31:26Z

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.

snir911 · 2025-01-12T07:56:21Z

I like the idea, from the pause image pulling POV i don't think it would be an issue in Openshift..
Another point is that there're node/runtime level pull credentials also IIRC, if keeping auth.json is the only option to (roughly) mimic this behavior i'd keep it.

src/cloud-api-adaptor/pkg/adaptor/k8sops/node.go

bpradipt

/lgtm
Thanks @squarti

stevenhorsman · 2025-01-23T15:09:03Z

@squarti - are you able to rebase this please to pick up the latest changes. Also I'd like to understand where we ended up with on the user experience here: I think the consensus was having support for node wide secret that is overridden with a deployment specific imagePullSecret?

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]>

squarti · 2025-01-23T15:49:14Z

@squarti - are you able to rebase this please to pick up the latest changes. Also I'd like to understand where we ended up with on the user experience here: I think the consensus was having support for node wide secret that is overridden with a deployment specific imagePullSecret?

I think the consensus was to remove the option to have a cluster-wide auth.json.

stevenhorsman · 2025-01-23T16:02:43Z

I think the consensus was to remove the option to have a cluster-wide auth.json.

Ok, that's not what was discussed on the community call, which I appreciate you weren't present at. From a quick scan above I think it looks like mostly @mkulke was for removing the cluster-wide option, but was supporting it on the call?

I don't really mind either way, but I'd like some community agreement here,

bpradipt · 2025-01-23T16:16:36Z

I think the consensus was to remove the option to have a cluster-wide auth.json.

Ok, that's not what was discussed on the community call, which I appreciate you weren't present at. From a quick scan above I think it looks like mostly @mkulke was for removing the cluster-wide option, but was supporting it on the call?

I don't really mind either way, but I'd like some community agreement here,

Fwiw, I'm ok to remove the cluster-wide auth option since imagepullSecret can be provided via the SA. Also with this change it decouples the pod image pull secret from node specific secret.
Also to be clear, I don't have any hard preference either way..

mkulke · 2025-01-27T10:04:59Z

afair, in the discussion the rationale for the keeping a cluster-wide option was: we currently don't know how to propagate credentials that are specified "out-of-band" on the node to the podvm, e.g. as a kubelet option or via other container runtime means.

an alternative that I can imagine would be to provide a simple docs explaining how an "auth.json" can be embedded in in the image.

I think in both cases we need logic to merge auth.json and a specified behaviour for overlaps / preference. maybe we can leverage existing libraries.

squarti requested a review from a team as a code owner January 9, 2025 16:41

stevenhorsman added the test_e2e_libvirt Run Libvirt e2e tests label Jan 9, 2025

squarti force-pushed the pull-secret branch from 72ee02e to a85d777 Compare January 9, 2025 22:13

mkulke reviewed Jan 10, 2025

View reviewed changes

squarti force-pushed the pull-secret branch 8 times, most recently from 42e1cf8 to 3630bc7 Compare January 11, 2025 00:31

squarti force-pushed the pull-secret branch 5 times, most recently from f3a0d8b to c9c2e02 Compare January 14, 2025 16:45

bpradipt reviewed Jan 15, 2025

View reviewed changes

src/cloud-api-adaptor/pkg/adaptor/k8sops/node.go Show resolved Hide resolved

bpradipt requested a review from mkulke January 16, 2025 13:24

squarti force-pushed the pull-secret branch from c9c2e02 to 8302f8b Compare January 16, 2025 23:34

bpradipt approved these changes Jan 23, 2025

View reviewed changes

bpradipt requested a review from stevenhorsman January 23, 2025 04:45

CAA: add support to look up imagePullSecrets for pods

10a1bf6

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]>

squarti force-pushed the pull-secret branch from 8302f8b to 10a1bf6 Compare January 23, 2025 15:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

squarti commented Jan 9, 2025

mkulke left a comment

mkulke Jan 10, 2025

stevenhorsman commented Jan 10, 2025

bpradipt commented Jan 10, 2025

squarti commented Jan 10, 2025

mkulke commented Jan 10, 2025

snir911 commented Jan 12, 2025

bpradipt left a comment

stevenhorsman commented Jan 23, 2025

squarti commented Jan 23, 2025 •

edited

Loading

stevenhorsman commented Jan 23, 2025

bpradipt commented Jan 23, 2025

mkulke commented Jan 27, 2025

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

Are you sure you want to change the base?

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

Conversation

squarti commented Jan 9, 2025

mkulke left a comment

Choose a reason for hiding this comment

mkulke Jan 10, 2025

Choose a reason for hiding this comment

stevenhorsman commented Jan 10, 2025

bpradipt commented Jan 10, 2025

squarti commented Jan 10, 2025

mkulke commented Jan 10, 2025

snir911 commented Jan 12, 2025

bpradipt left a comment

Choose a reason for hiding this comment

stevenhorsman commented Jan 23, 2025

squarti commented Jan 23, 2025 • edited Loading

stevenhorsman commented Jan 23, 2025

bpradipt commented Jan 23, 2025

mkulke commented Jan 27, 2025

squarti commented Jan 23, 2025 •

edited

Loading