Add support for imagePullSecrets #2589

garonsky · 2023-02-14T22:48:57Z

Add support for imagePullSecrets in the CDI CR to support pulling images from repositories that require secrets.

The imagePullSecrets is propagated to the following components: cdi-apiserver, cdi-deployment, and cdi-uploadproxy. The definition of imagePullSecrets in cdi-operator must be done manually.

Does this PR introduce a user-facing change?

Added support for imagePullSecrets to the CDI custom resource and the cdi-operator resource.  Updates to the imagePullSecrets value in the custom resource are automatically propagated to cdi-apiserver, cdi-deployment, and cdi-uploadproxy.  The cdi-operator resource should be manually updated.

kubevirt-bot · 2023-02-14T22:49:06Z

Hi @garonsky. Thanks for your PR.

I'm waiting for a kubevirt member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

mhenriks · 2023-02-15T00:46:47Z

thanks for the pr @garonsky!

/ok-to-test

mhenriks · 2023-02-15T00:54:15Z

@garonsky have you seen the discussion in #2395 on support for secrets in "worker" pods? Mind sharing your thoughts there? Looks like you intend to support clone source pods in this pr? What are your thoughts on the other workers?

alromeros · 2023-02-15T13:10:03Z

/test pull-containerized-data-importer-e2e-ceph

mhenriks

@garonsky thanks for the PR, great start! Looks like we mostly just have to flush out the support for worker pods and.

mhenriks · 2023-02-17T21:49:01Z

cmd/cdi-controller/controller.go

@@ -57,6 +58,7 @@ var (
 	uploadProxyServiceName string
 	configName             string
 	pullPolicy             string
+	imagePullSecrets       []corev1.LocalObjectReference


This variable is not assigned to. But I suggest a different approach for setting imagePullSecrets on worker pods.

There are a bunch of places in pkg/controller where pods are created. We should handle imagePullSecrets similar to how podResourceRequirements are handled by looking up the value in CDIConfig at pod creation time.

mhenriks · 2023-02-17T22:01:11Z

staging/src/kubevirt.io/containerized-data-importer-api/pkg/apis/core/v1beta1/types.go

-	ImagePullPolicy corev1.PullPolicy `json:"imagePullPolicy,omitempty" valid:"required"`
+	ImagePullPolicy  corev1.PullPolicy             `json:"imagePullPolicy,omitempty" valid:"required"`
+	// The imagePullSecrets to pull the container images
+	ImagePullSecrets []corev1.LocalObjectReference `json:"imagePullSecrets,omitempty"`


I assume that we want regular users to know the names of secrets we expect them to create? So I suggest moving ImagePullSecrets to CDIConfig. So CDI will look like this:

... kind: CDI spec: config: imagePullSecrets: - secret-name ...

And CDIConfig should have imagePullSecrets in the status like so:

... kind: CDIConfig metadata: name: config spec: imagePullSecrets: #set by config-controller - secret-name status: imagePullSecrets: - secret-name ...

CDIConfig has a singleton (named config) that is created by the config-controller and readable by all users. The spec section is spec.config from the CDI resource.

I refer you again to how PodResourceRequirements are handled

garonsky · 2023-02-23T23:28:14Z

@mhenriks thank you for the great feedback! I tried to implement the suggestions, let me know if this is more of what you had in mind.

images from repositories that require secrets. The imagePullSecrets is propagated to the following components: cdi-apiserver, cdi-deployment, and cdi-uploadproxy. The definition of imagePullSecrets in cdi-operator must be done manually. Signed-off-by: Gleb Aronsky <gleb.aronsky@windriver.com>

mhenriks · 2023-03-08T01:57:45Z

@garonsky I did a quick scan and I think this looks great! I will dig a little deeper tomorrow and keep you posted. Sorry for the delayed response. Been on PTO.

mhenriks

Great work! Couple places that need pull secrets:

https://github.com/garonsky/containerized-data-importer/blob/add_imagePullSecrets/pkg/controller/dataimportcron-controller.go#L877-L883

https://github.com/garonsky/containerized-data-importer/blob/add_imagePullSecrets/pkg/controller/datavolume/pvc-clone-controller.go#L1246-L1258

https://github.com/garonsky/containerized-data-importer/blob/add_imagePullSecrets/pkg/controller/import-controller.go#L1082-L1092

I think that's it!

mhenriks · 2023-03-08T23:35:07Z

pkg/controller/import-controller.go

@@ -867,6 +868,8 @@ func createImporterPod(log logr.Logger, client client.Client, args *importerPodA
 		return nil, err
 	}

+	args.imagePullSecrets = cdiv1.CDIConfig{}.Spec.ImagePullSecrets


Should be calling GetImagePullSecrets here, right?

garonsky · 2023-03-13T01:32:52Z

@mhenriks Thank you so much for the guidance! I made some changes. Let me know if I captured your suggestions with the latest changes.

mhenriks

@garonsky seems to work well, just one bug I found

mhenriks · 2023-03-14T01:03:22Z

pkg/controller/config-controller.go

+func (r *CDIConfigReconciler) reconcileImagePullSecrets(config *cdiv1.CDIConfig) error {
+	if config.Spec.ImagePullSecrets != nil {
+		config.Status.ImagePullSecrets = config.Spec.ImagePullSecrets
+	}


config.Status.ImagePullSecrets will not get unset if config.Spec.ImagePullSecrets deleted

Signed-off-by: Gleb Aronsky <gleb.aronsky@windriver.com>

mhenriks · 2023-03-17T21:39:55Z

/lgtm
/approve

Thanks @garonsky!

kubevirt-bot · 2023-03-17T21:40:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mhenriks

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [mhenriks]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

aglitke · 2023-03-27T12:28:27Z

Fixes #2395

kubevirt-bot requested review from aglitke and maya-r February 14, 2023 22:49

kubevirt-bot added the size/L label Feb 14, 2023

kubevirt-bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 15, 2023

kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Feb 17, 2023

mhenriks reviewed Feb 17, 2023

View reviewed changes

kubevirt-bot added dco-signoff: no Indicates the PR's author has not DCO signed all their commits. and removed dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Feb 21, 2023

garonsky force-pushed the add_imagePullSecrets branch from aa3d219 to 68c8ab4 Compare February 21, 2023 22:15

garonsky force-pushed the add_imagePullSecrets branch from 117408e to 9c33139 Compare February 23, 2023 23:30

garonsky force-pushed the add_imagePullSecrets branch from 9c33139 to 15e33dc Compare February 23, 2023 23:44

kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Feb 23, 2023

garonsky force-pushed the add_imagePullSecrets branch from 15e33dc to 523fd40 Compare February 27, 2023 22:06

mhenriks reviewed Mar 8, 2023

View reviewed changes

garonsky force-pushed the add_imagePullSecrets branch from 523fd40 to 7dbfbb8 Compare March 12, 2023 21:45

mhenriks reviewed Mar 14, 2023

View reviewed changes

Modifying code to incorporate review comments.

d3490cb

Signed-off-by: Gleb Aronsky <gleb.aronsky@windriver.com>

garonsky force-pushed the add_imagePullSecrets branch from 7dbfbb8 to d3490cb Compare March 16, 2023 19:48

kubevirt-bot assigned mhenriks Mar 17, 2023

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 17, 2023

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 17, 2023

kubevirt-bot merged commit 4e3ee4c into kubevirt:main Mar 18, 2023

aglitke mentioned this pull request Mar 27, 2023

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for imagePullSecrets #2589

Add support for imagePullSecrets #2589

garonsky commented Feb 14, 2023 •

edited by awels

Loading

kubevirt-bot commented Feb 14, 2023

mhenriks commented Feb 15, 2023

mhenriks commented Feb 15, 2023

alromeros commented Feb 15, 2023

mhenriks left a comment

mhenriks Feb 17, 2023

mhenriks Feb 17, 2023 •

edited

Loading

garonsky commented Feb 23, 2023

mhenriks commented Mar 8, 2023

mhenriks left a comment

mhenriks Mar 8, 2023

garonsky commented Mar 13, 2023

mhenriks left a comment

mhenriks Mar 14, 2023

mhenriks commented Mar 17, 2023

kubevirt-bot commented Mar 17, 2023

aglitke commented Mar 27, 2023

Add support for imagePullSecrets #2589

Add support for imagePullSecrets #2589

Conversation

garonsky commented Feb 14, 2023 • edited by awels Loading

kubevirt-bot commented Feb 14, 2023

mhenriks commented Feb 15, 2023

mhenriks commented Feb 15, 2023

alromeros commented Feb 15, 2023

mhenriks left a comment

Choose a reason for hiding this comment

mhenriks Feb 17, 2023

Choose a reason for hiding this comment

mhenriks Feb 17, 2023 • edited Loading

Choose a reason for hiding this comment

garonsky commented Feb 23, 2023

mhenriks commented Mar 8, 2023

mhenriks left a comment

Choose a reason for hiding this comment

mhenriks Mar 8, 2023

Choose a reason for hiding this comment

garonsky commented Mar 13, 2023

mhenriks left a comment

Choose a reason for hiding this comment

mhenriks Mar 14, 2023

Choose a reason for hiding this comment

mhenriks commented Mar 17, 2023

kubevirt-bot commented Mar 17, 2023

aglitke commented Mar 27, 2023

garonsky commented Feb 14, 2023 •

edited by awels

Loading

mhenriks Feb 17, 2023 •

edited

Loading