feat(chart): add imagePullSecrets support

emaincourt commented 2 years ago

Hi,

This PR introduces support for imagePullSecrets for both the manager deployment and the daemon ds. One can either declare those values globally or locally for each component. Motivations behind this change are:

Authenticate to the registry in case of a custom, private image
Getting rid of rate limitations for services like Dockerhub when pulling images onto a large cluster

Please let me know if anything looks wrong to you.

Thanks in advance for reviewing the code.

Note on DCO:

If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.

Checklist:

[x] I have bumped the chart version according to versioning
[x] I have updated the chart changelog with all the changes that come with this pull request according to changelog.
[x] Any new values are backwards compatible and/or have sensible default.
[x] I have signed off all my commits as required by DCO.

Changes are automatically published when merged to master. They are not published on branches.

kruise-bot commented 2 years ago

Welcome @emaincourt! It looks like this is your first PR to openkruise/charts 🎉

FillZpp commented 2 years ago

@emaincourt Thanks. But question is, the imagePullSecrets of kruise-manager or kruise-daemon can only be secrets in kruise-system. How could you apply the secrets during helm install?

emaincourt commented 2 years ago

@FillZpp Sorry for the delay and thank you very much for reviewing the changes.

Actually yes those secrets must live in the kruise-system namespace, but that’s the responsibility of the owner of the cluster to make them available at deployment time.

In case you want to make it possible to optionally create the secret at install time, a secret could be added to the chart but I quite believe it would be outside of the scope.

What do you think ?

FillZpp commented 2 years ago

Actually yes those secrets must live in the kruise-system namespace, but that’s the responsibility of the owner of the cluster to make them available at deployment time.

Alright, that's fine. Maybe .Values.manager.imagePullSecrets is unnecessary since the images of kruise-manager and kruise-daemon are the same so .Values.imagePullSecrets is enough.

In case you want to make it possible to optionally create the secret at install time, a secret could be added to the chart but I quite believe it would be outside of the scope.

Actually, we can create the image in helm chart by https://helm.sh/docs/howto/charts_tips_and_tricks/#creating-image-pull-secrets , but this needs user to set registry/username/password parameters when helm install.

Do you think this is better than manually create secrets into kruise-system after helm install?

emaincourt commented 2 years ago

@FillZpp Thanks for the review !

Alright, that's fine. Maybe .Values.manager.imagePullSecrets is unnecessary since the images of kruise-manager and kruise-daemon are the same so .Values.imagePullSecrets is enough.

That's actually a pretty fair point. I did not realize it sorry. I updated the manifest to match it.

Actually, we can create the image in helm chart by https://helm.sh/docs/howto/charts_tips_and_tricks/#creating-image-pull-secrets , but this needs user to set registry/username/password parameters when helm install.

Do you think this is better than manually create secrets into kruise-system after helm install?

Well honestly I've never seen a chart allowing for imagePullSecret creation. I would expect secrets creation/update to be handled by a centralized/secured application, which would stand as a vault. Basically, in case I need to update this secret, I would not expect to have to update all the value files of all the charts I deploy, nor to share docker credentials with the whole team.

What do you think ?

kruise-bot commented 2 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FillZpp

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](https://github.com/openkruise/charts/blob/master/OWNERS)~~ [FillZpp] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment

openkruise / charts

feat(chart): add imagePullSecrets support #12