helm: add imagePullSecrets for helm charts #3906

fungaren · 2023-06-14T12:01:15Z

Describe what this PR does

Currently the Helm chart does not contain a imagePullSecrets option when you are using private container registry, this is very inconvenient. This PR add this option for both CephFS and RBD.

Provide some context for the reviewer

For example, csi-driver-nfs already support this:

https://github.com/kubernetes-csi/csi-driver-nfs/blob/862cb036e3f0d7c3fe28d2b127a6f96335b15309/charts/v4.3.0/csi-driver-nfs/values.yaml#L127-L131

https://github.com/kubernetes-csi/csi-driver-nfs/blob/862cb036e3f0d7c3fe28d2b127a6f96335b15309/charts/v4.3.0/csi-driver-nfs/templates/csi-nfs-controller.yaml#L20-L23

Is there anything that requires special attention

To test this feature, you can use helm template and kubectl apply --dry-run to examine the YAML result and the syntax. For example:

~/ceph-csi/charts$ helm template -n default ceph-csi-rbd ./ceph-csi-cephfs \
--set provisioner.imagePullSecrets[0].name=aaaaa \
--set nodeplugin.imagePullSecrets[0].name=bbbbb | \
kubectl apply -f - --validate='strict' --dry-run=client

serviceaccount/ceph-csi-rbd-ceph-csi-cephfs-nodeplugin created (dry run)
serviceaccount/ceph-csi-rbd-ceph-csi-cephfs-provisioner created (dry run)
configmap/ceph-config created (dry run)
configmap/ceph-csi-config created (dry run)
clusterrole.rbac.authorization.k8s.io/ceph-csi-rbd-ceph-csi-cephfs-provisioner created (dry run)
clusterrolebinding.rbac.authorization.k8s.io/ceph-csi-rbd-ceph-csi-cephfs-provisioner created (dry run)
role.rbac.authorization.k8s.io/ceph-csi-rbd-ceph-csi-cephfs-provisioner created (dry run)
rolebinding.rbac.authorization.k8s.io/ceph-csi-rbd-ceph-csi-cephfs-provisioner created (dry run)
service/ceph-csi-rbd-ceph-csi-cephfs-nodeplugin-http-metrics created (dry run)
service/ceph-csi-rbd-ceph-csi-cephfs-provisioner-http-metrics created (dry run)
daemonset.apps/ceph-csi-rbd-ceph-csi-cephfs-nodeplugin created (dry run)
deployment.apps/ceph-csi-rbd-ceph-csi-cephfs-provisioner created (dry run)
csidriver.storage.k8s.io/cephfs.csi.ceph.com configured (dry run)

Do you have any questions?

Is the change backward compatible?

Are there concerns around backward compatibility?

Provide any external context for the change, if any.

For example:

Kubernetes links that explain why the change is required
CSI spec related changes/catch-up that necessitates this patch
golang related practices that necessitates this change

Related issues

Mention any github issues relevant to this PR. Adding below line
will help to auto close the issue once the PR is merged.

Fixes: #issue_number

Future concerns

List items that are not part of the PR and do not impact it's
functionality, but are work items that can be taken up subsequently.

Show available bot commands

These commands are normally not required, but in case of issues, leave any of
the following bot commands in an otherwise empty comment in this PR:

/retest ci/centos/<job-name>: retest the <job-name> after unrelated
failure (please report the failure too!)
/retest all: run this in case the CentOS CI failed to start/report any test
progress or results

nixpanic · 2023-06-14T12:40:57Z

Thanks!

Could you please add support for NFS as well? Also force-push your updated/rebased commit and include your signed-off-by approval. The subject of the commit should also start with deploy: or helm: .

fungaren · 2023-06-14T12:57:19Z

@nixpanic Well, but it seems that it does not provide a helm chart for NFS? Theres only ceph-csi-cephfs and ceph-csi-rbd in the charts directory.

nixpanic · 2023-06-14T15:11:16Z

@nixpanic Well, but it seems that it does not provide a helm chart for NFS? Theres only ceph-csi-cephfs and ceph-csi-rbd in the charts directory.

Ah, indeed, that seems to be missing 😞

nixpanic · 2023-06-14T15:17:20Z

charts/ceph-csi-cephfs/templates/nodeplugin-daemonset.yaml

@@ -37,6 +37,10 @@ spec:
      # to use e.g. Rook orchestrated cluster, and mons' FQDN is
      # resolved through k8s service, set dns policy to cluster first
      dnsPolicy: ClusterFirstWithHostNet
+{{- if .Values.nodeplugin.imagePullSecrets }}
+      imagePullSecrets:
+{{ toYaml .Values.nodeplugin.imagePullSecrets | indent 8 -}}


Could you show in an example what value you would pass? I would expect something like this instead:

- name: {{ .Values.nodeplugin.imagePullSecrets }}

Here you should provide an array like:

imagePullSecrets: - name: my-secret-a - name: my-secret-b

This part in fact is copied from csi-driver-nfs:

https://github.com/kubernetes-csi/csi-driver-nfs/blob/862cb036e3f0d7c3fe28d2b127a6f96335b15309/charts/v4.3.0/csi-driver-nfs/values.yaml#L127-L131

https://github.com/kubernetes-csi/csi-driver-nfs/blob/862cb036e3f0d7c3fe28d2b127a6f96335b15309/charts/v4.3.0/csi-driver-nfs/templates/csi-nfs-controller.yaml#L20-L23

And It is a recommended way rather than:

imagePullSecrets: - name: {{ .Values.nodeplugin.imagePullSecret }}

In this way you can not provide multiple secrets. And maybe there are other fields (no just name only).

ok, thanks for the clarification!

Madhu-1 · 2023-06-15T07:18:17Z

@fungaren can you please fix linter errors https://github.com/ceph/ceph-csi/actions/runs/5267388978/jobs/9526493600?pr=3906 and https://github.com/ceph/ceph-csi/pull/3906/checks?check_run_id=14256026729. you can follow https://github.com/ceph/ceph-csi/blob/devel/docs/development-guide.md#code-contribution-workflow to sign the commit.

nixpanic · 2023-06-15T07:59:27Z

@fungaren can you please fix linter errors https://github.com/ceph/ceph-csi/actions/runs/5267388978/jobs/9526493600?pr=3906 and https://github.com/ceph/ceph-csi/pull/3906/checks?check_run_id=14256026729. you can follow https://github.com/ceph/ceph-csi/blob/devel/docs/development-guide.md#code-contribution-workflow to sign the commit.

I think the failure is because there is no empty line between the subject of the commit, and the Signed-off-by line.

Pull request has been modified.

nixpanic · 2023-06-15T08:39:37Z

@megifyio queue

nixpanic · 2023-06-15T08:45:48Z

@Mergifyio queue

mergify · 2023-06-15T08:45:54Z

queue

🛑 The pull request has been removed from the queue

Pull request #3906 has been dequeued due to failing checks or checks timeout.

You can take a look at Queue: Embarked in merge train check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

nixpanic · 2023-06-15T11:40:34Z

Hi @fungaren, could you login (with your GitHub account) on the Mergify Dashboard? It seems that it is required for some reason before Mergify wants to run tests on this PR.

https://dashboard.mergify.com/github/ceph/repo/ceph-csi/queues?branch=devel&pull=3906&queues=default

Once done, leave a comment @mergifyio refresh in this PR. Thanks!

fungaren · 2023-06-15T11:51:50Z

Emm.... It sounds a bit strange... Anyway, I will try.

But I don't have permission to access it.

Rakshith-R · 2023-06-15T11:55:13Z

@fungaren Can you just rebase on top of devel now git pull --rebase upstream devel and force push the pr?
that should work too AFAIK.

Currently the Helm chart does not contain a imagePullSecrets option when you are using private container registry, this is very inconvenient. This PR add this option for both CephFS and RBD. Signed-off-by: Garen Fang <fungaren@qq.com>

Pull request has been modified.

Rakshith-R · 2023-06-15T12:10:25Z

@Mergifyio queue

ceph-csi-bot · 2023-06-15T13:22:50Z

/test ci/centos/k8s-e2e-external-storage/1.24

ceph-csi-bot · 2023-06-15T13:22:52Z

/test ci/centos/k8s-e2e-external-storage/1.25

ceph-csi-bot · 2023-06-15T13:22:53Z

/test ci/centos/k8s-e2e-external-storage/1.26

ceph-csi-bot · 2023-06-15T13:22:54Z

/test ci/centos/k8s-e2e-external-storage/1.27

ceph-csi-bot · 2023-06-15T13:22:55Z

/test ci/centos/mini-e2e-helm/k8s-1.24

ceph-csi-bot · 2023-06-15T13:22:56Z

/test ci/centos/mini-e2e-helm/k8s-1.25

ceph-csi-bot · 2023-06-15T13:22:57Z

/test ci/centos/mini-e2e-helm/k8s-1.26

ceph-csi-bot · 2023-06-15T13:22:58Z

/test ci/centos/mini-e2e-helm/k8s-1.27

ceph-csi-bot · 2023-06-15T13:22:59Z

/test ci/centos/mini-e2e/k8s-1.24

ceph-csi-bot · 2023-06-15T13:23:00Z

/test ci/centos/mini-e2e/k8s-1.25

ceph-csi-bot · 2023-06-15T13:23:01Z

/test ci/centos/mini-e2e/k8s-1.26

ceph-csi-bot · 2023-06-15T13:23:02Z

/test ci/centos/mini-e2e/k8s-1.27

ceph-csi-bot · 2023-06-15T13:23:04Z

/test ci/centos/upgrade-tests-cephfs

ceph-csi-bot · 2023-06-15T13:23:05Z

/test ci/centos/upgrade-tests-rbd

nixpanic · 2023-06-15T13:23:12Z

Thanks @fungaren , I think Mergify is finally also ready to accept+merge this PR.

ceph-csi-bot · 2023-06-16T02:57:05Z

/retest ci/centos/mini-e2e-helm/k8s-1.26

ceph-csi-bot · 2023-06-16T02:57:05Z

@fungaren "ci/centos/mini-e2e-helm/k8s-1.26" test failed. Logs are available at location for debugging

ceph-csi-bot · 2023-06-16T02:57:07Z

/retest ci/centos/upgrade-tests-rbd

ceph-csi-bot · 2023-06-16T02:57:08Z

@fungaren "ci/centos/upgrade-tests-rbd" test failed. Logs are available at location for debugging

ceph-csi-bot · 2023-06-16T02:57:09Z

/retest ci/centos/mini-e2e/k8s-1.27

ceph-csi-bot · 2023-06-16T02:57:10Z

@fungaren "ci/centos/mini-e2e/k8s-1.27" test failed. Logs are available at location for debugging

ceph-csi-bot · 2023-06-16T02:57:11Z

/retest ci/centos/upgrade-tests-cephfs

ceph-csi-bot · 2023-06-16T02:57:12Z

@fungaren "ci/centos/upgrade-tests-cephfs" test failed. Logs are available at location for debugging

ceph-csi-bot · 2023-06-16T02:57:13Z

@Mergifyio requeue

mergify · 2023-06-16T02:57:46Z

requeue

☑️ This pull request is already queued

mergify bot added the component/deployment Helm chart, kubernetes templates and configuration Issues/PRs label Jun 14, 2023

nixpanic added the ci/skip/multi-arch-build skip building on multiple architectures label Jun 14, 2023

fungaren force-pushed the support-image-pull-secrets branch from 260cbd0 to 118c456 Compare June 14, 2023 12:55

fungaren changed the title ~~add imagePullSecrets for helm charts~~ helm: add imagePullSecrets for helm charts Jun 14, 2023

nixpanic reviewed Jun 14, 2023

View reviewed changes

Madhu-1 previously approved these changes Jun 15, 2023

View reviewed changes

fungaren force-pushed the support-image-pull-secrets branch from 118c456 to 667beb6 Compare June 15, 2023 08:32

nixpanic previously approved these changes Jun 15, 2023

View reviewed changes

nixpanic requested a review from Madhu-1 June 15, 2023 08:35

Madhu-1 previously approved these changes Jun 15, 2023

View reviewed changes

helm: add imagePullSecrets option

c00b48a

Currently the Helm chart does not contain a imagePullSecrets option when you are using private container registry, this is very inconvenient. This PR add this option for both CephFS and RBD. Signed-off-by: Garen Fang <fungaren@qq.com>

fungaren force-pushed the support-image-pull-secrets branch from 667beb6 to c00b48a Compare June 15, 2023 11:59

Rakshith-R approved these changes Jun 15, 2023

View reviewed changes

Rakshith-R requested review from Madhu-1 and nixpanic June 15, 2023 12:07

nixpanic added the ok-to-test Label to trigger E2E tests label Jun 15, 2023

ceph-csi-bot removed the ok-to-test Label to trigger E2E tests label Jun 15, 2023

Rakshith-R added the ci/retry/e2e Label to retry e2e retesting on approved PR's label Jun 16, 2023

mergify bot merged commit 37018a2 into ceph:devel Jun 16, 2023

fungaren deleted the support-image-pull-secrets branch June 16, 2023 06:17

helm: add imagePullSecrets for helm charts #3906

helm: add imagePullSecrets for helm charts #3906

Conversation

fungaren commented Jun 14, 2023

Describe what this PR does

Is there anything that requires special attention

Related issues

Future concerns

nixpanic commented Jun 14, 2023

fungaren commented Jun 14, 2023

nixpanic commented Jun 14, 2023

nixpanic Jun 14, 2023

Choose a reason for hiding this comment

fungaren Jun 15, 2023

Choose a reason for hiding this comment

nixpanic Jun 15, 2023

Choose a reason for hiding this comment

Madhu-1 commented Jun 15, 2023

nixpanic commented Jun 15, 2023

nixpanic commented Jun 15, 2023

nixpanic commented Jun 15, 2023

mergify bot commented Jun 15, 2023 • edited Loading

🛑 The pull request has been removed from the queue

nixpanic commented Jun 15, 2023

fungaren commented Jun 15, 2023

Rakshith-R commented Jun 15, 2023

Rakshith-R commented Jun 15, 2023

ceph-csi-bot commented Jun 15, 2023

ceph-csi-bot commented Jun 15, 2023

ceph-csi-bot commented Jun 15, 2023

ceph-csi-bot commented Jun 15, 2023

ceph-csi-bot commented Jun 15, 2023

ceph-csi-bot commented Jun 15, 2023

ceph-csi-bot commented Jun 15, 2023

ceph-csi-bot commented Jun 15, 2023

ceph-csi-bot commented Jun 15, 2023

ceph-csi-bot commented Jun 15, 2023

ceph-csi-bot commented Jun 15, 2023

ceph-csi-bot commented Jun 15, 2023

ceph-csi-bot commented Jun 15, 2023

ceph-csi-bot commented Jun 15, 2023

nixpanic commented Jun 15, 2023

ceph-csi-bot commented Jun 16, 2023

ceph-csi-bot commented Jun 16, 2023

ceph-csi-bot commented Jun 16, 2023

ceph-csi-bot commented Jun 16, 2023

ceph-csi-bot commented Jun 16, 2023

ceph-csi-bot commented Jun 16, 2023

ceph-csi-bot commented Jun 16, 2023

ceph-csi-bot commented Jun 16, 2023

ceph-csi-bot commented Jun 16, 2023

mergify bot commented Jun 16, 2023

☑️ This pull request is already queued

mergify bot commented Jun 15, 2023 •

edited

Loading