Allow use of imagePullSecrets on CDI operator, infra and workload pods

[shannonmitchell]

Is your feature request related to a problem? Please describe:

We are using kubevirt and cdi in a security environment that has only private registries and limited to no internet access. We are also limited on access to the kubernetes nodes, so the registry secrets can't be set up globally. This requires kubernetes imagePullSecrets to be used for both deployments and automated testing. Kubevirt allows for the setting of customizeComponents for dynamic infra/workload pods spun up by the operator. Kubevirt also supports imagePullSecrets direction in the virt-operator spec. CDI is currently missing the ability to do the same.

Describe the solution you'd like:

A similar solution to specify imagePullSecrets that KubeVirt has should work. See the customizeComponents and operator spec info from above. Hard coding imagePullSecrets similar to how you guys are doing the imagePullPolicy should work as well.

Describe alternatives you've considered:

We are currently using opa gatekeeper mutations to inject the imagePullSecrets into the CDI serviceaccounts, but a solution done through the CDI configs would be preferred.

Additional context:

Supporting imagePullSecrets should help with the adoption of CDI into more secure Enterprise spaces.

[awels]

Thanks for the report @mhenriks any thoughts on this? Seems the KubeVirt thing that allows this is https://github.com/kubevirt/kubevirt/pull/3612 and https://github.com/kubevirt/kubevirt/pull/5563

[mhenriks]

Thanks for the report @mhenriks any thoughts on this? Seems the KubeVirt thing that allows this is kubevirt/kubevirt#3612 and kubevirt/kubevirt#5563

Thanks for the request @shannonmitchell, CDI should definitely support this.

[bc185174]

In addition to the imagePullPolicy, we should probably include both imageRegistry and imageTag as well.

[kubevirt-bot]

Issues go stale after 90d of inactivity. Mark the issue as fresh with /remove-lifecycle stale. Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

/lifecycle stale

[andrewg-xyz]

/remove-lifecycle stale

[k8scoder192]

Hi @awels and @mhenriks any progress on this?

Looks like Kubevirt added support for this https://github.com/kubevirt/kubevirt/pull/5563

[barhoumikhaled]

@shannonmitchell do you have examples please on how to opa gatekeeper mutations to inject the imagePullSecrets into the CDI serviceaccounts? I tried below mutation, but it did not inject the imagepullsecrets to the SA

apiVersion: mutations.gatekeeper.sh/v1
kind: Assign
metadata:
  name: image-pull-secret
  namespace: cdi
spec:
  applyTo:
  - groups: [""]
    kinds: ["ServiceAccount"]
    versions: ["v1"]
  match:
    scope: Namespaced
    kinds:
    - apiGroups: ["*"]
      kinds: ["ServiceAccount"]
    namespaces: ["cdi"]
  location: "imagePullSecrets"
  parameters:
    assign:
      value:
        - name: pull-secret

[shannonmitchell]

The following worked for me. I added the 'inject-sa-imgpullsec: true' label to the namespace being used for the match. Make sure to enable mutations in the gatekeeper configs as it is disabled by default. The secrets used have to exist in that namespace as well.

apiVersion: mutations.gatekeeper.sh/v1beta1
kind: Assign
metadata:
  name: sa-image-pull-secrets
spec:
  applyTo:
    - groups: [""]
      kinds: ["ServiceAccount"]
      versions: ["v1"]
  match:
    scope: Namespaced
    kinds:
      - apiGroups: ["*"]
        kinds: ["ServiceAccount"]
    namespaceSelector:
      matchLabels:
        inject-sa-imgpullsec: "true"
  location: "imagePullSecrets"
  parameters:
    assign:
      value:
        - name: private-registry
        - name: repo1-read-creds

[mhenriks]

@ninjacoder80 @barhoumikhaled @shannonmitchell @andrewg-xyz. I'm looking into supporting imagePullSecrets in a way similar to KubeVirt as implemented here: https://github.com/kubevirt/kubevirt/pull/7761

It is pretty straightforward to add support for CDI infra components like cdi-deployment but it is trickier to do the "worker" pods like cdi-importer.

KubeVirt implements a nice little hack to support pullSecrets for virt-launcher pods which run in arbitrary namespaces. If imagePullSecrets are configured, virt-handler (a daemonset) includes the virt-launcher container as a sidecar. That way it gets downloaded to each node and as long as pull policy Always is not used, it works fine. CDI does not have a daemonset but we could create a dummy one just to get the worker images to nodes. Seems extra hacky compared to KubeVirt, but whatever. Let's call this option A.

Another option is to copy the pull secrets from cdi install namespace to each target namespace for each import/upload/clone operation. This will only require "create secret" RBAC which is okay I think. "Get secret" for every namespace would be unacceptable. A downside to this is that maybe you don't want to potentially make these secrets available in every namespace? To potentially any user that has "get secret" in a namespace? Let's call this option B.

Another option is that users can tell us the names of the secrets for worker pods but users are responsible for creating the secrets. Option C.

Any options I missed? Which one do you like the best?

[barhoumikhaled]

thanks @shannonmitchell

[barhoumikhaled]

Thanks @mhenriks for these options. I think for security reasons, you might not want to copy the secrets to every namespace. It is ok to have the secret in the CDI install namespace but you might not to want to have that secret available in a customer namespace (worker pods), with that being said, I think option A is the best of the 3.

[andrewg-xyz]

I don't think it's unreasonable to expect users to create secrets. In other projects that support imagePullSecrets I've set a reference to a namespace scoped secret.

However, the weight of my opinion is pretty small - as I'm unfamiliar with the CDI codebase. Excited to be engaged on this issue/topic and will help however I can :)

[k8scoder192]

@mhenriks thanks for looking into this. I would vote for option C, it's more secure and one less thing CDI has to manage/do with respect to secret replication.

[garonsky]

@mhenriks In our use case for starlingx - OpenDev, our framework copies the secret into the namespace or the end user can copy the secret. So, as I understand the choices, our preference would be C. I don’t think that CDI should be responsible for secret management across namespaces, in agreement with @ninjacoder80.

[aglitke]

Fixed by #2589