rustfs icon indicating copy to clipboard operation
rustfs copied to clipboard

render imagePullSecrets in Deployment/StatefulSet

Open czaloumis opened this issue 2 weeks ago • 15 comments

Type of Change

  • [ ] New Feature
  • [ ] Bug Fix
  • [ ] Documentation
  • [ ] Performance Improvement
  • [ ] Test/CI
  • [ ] Refactor
  • [ ] Other:

Related Issues

#1123

Summary of Changes

Checklist

  • [ ] I have read and followed the CONTRIBUTING.md guidelines
  • [ ] Passed make pre-commit
  • [ ] Added/updated necessary tests
  • [ ] Documentation updated (if needed)
  • [ ] CI/CD passed (if applicable)

Impact

  • [ ] Breaking change (compatibility)
  • [ ] Requires doc/config/deployment update
  • [ ] Other impact:

Additional Notes


Thank you for your contribution! Please ensure your PR follows the community standards (CODE_OF_CONDUCT.md) and sign the CLA if this is your first contribution.

czaloumis avatar Dec 12 '25 10:12 czaloumis

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Dec 12 '25 10:12 CLAassistant

Tested on on-prem Kubernetes. The Deployment and StatefulSet now support a list of imagePullSecrets defined in values.yaml.

czaloumis avatar Dec 12 '25 10:12 czaloumis

@czaloumis After https://github.com/rustfs/rustfs/pull/1127 is merged, and leads to the conflict for this pr, could y please help to resolve the conflict and then we will merge this pr.

majinghe avatar Dec 12 '25 12:12 majinghe

@majinghe , I'm working on it, I'll let you know once done.

czaloumis avatar Dec 12 '25 12:12 czaloumis

@majinghe, Tested on on-prem Kubernetes cluster and confirmed it works in both cases:

  • Using predefined imagePullSecrets
  • Creating one automatically from the imageRegistryCredentials implementation

Also verified compatibility with standard and distributed deployment modes.

I combine the logic you create with the creation of a default pull secret, along with the ability for users to add their own secrets. Please let me know what you think. Maybe we could improve it further by automatically appending the generated imageRegistryCredentials secret to the user's imagePullSecrets array (if provided), creating a unified list that includes both.

czaloumis avatar Dec 12 '25 13:12 czaloumis

@czaloumis Cool,

Maybe we could improve it further by automatically appending the generated imageRegistryCredentials secret to the user's imagePullSecrets array (if provided), creating a unified list that includes both.

Sounds good, please go ahead.

majinghe avatar Dec 12 '25 13:12 majinghe

@majinghe, This is the change I was proposing earlier. Let me know what you think, and I can also update the README.md accordingly.

czaloumis avatar Dec 12 '25 14:12 czaloumis

@czaloumis Very nice change, please update the README.md, please let me know when you done. Thx

majinghe avatar Dec 12 '25 14:12 majinghe

Looks strange for me. Instead of using native yaml with toYaml function, you render the secret in a strange way.

jurim76 avatar Dec 12 '25 16:12 jurim76

Hello @jurim76, thank you for your feedback. Could you please be more specific about your comments?

czaloumis avatar Dec 12 '25 16:12 czaloumis

Hi @czaloumis I've added imagePullSecrets support in this PR https://github.com/rustfs/rustfs/pull/1122

jurim76 avatar Dec 12 '25 16:12 jurim76

@jurim76, I've added support for both existing pull secrets and auto-generated ones from credentials, as discussed with @majinghe before.

Need feedback from a contributor before any other action.

czaloumis avatar Dec 12 '25 16:12 czaloumis

Hmm..never seen this approach, usually image.repository, image.tag and imagesPullSecrets is enough, sometimes chart uses service account.

jurim76 avatar Dec 12 '25 16:12 jurim76

@jurim76, I agree that most Helm charts follow what you mentioned here. However, @majinghe's new approach for creating imagePullSecrets from credentials addresses the issue I raised in #1123, which is related to this PR.

Maintainer review and feedback are crucial for project sustainability—let's hear their thoughts.

czaloumis avatar Dec 12 '25 16:12 czaloumis

Looks like it was added to main branch and it breaks possibility to use already existing secrets

jurim76 avatar Dec 12 '25 20:12 jurim76

@jurim76

Hmm..never seen this approach, usually image.repository, image.tag and imagesPullSecrets is enough, sometimes chart uses service account.

At the beginning, I have the same thought with y, then i checked the helm docs and found the append approach is a nice way to deal with imagepullsecret, and I have tested on local env, and worked fine with/without imagepullsecret as well as existing imagepullsecret.

Looks like it was added to main branch and it breaks possibility to use already existing secrets

Existing secrets logic before seems only related to AK&SK

{{- if not .Values.secret.existingSecret }}
apiVersion: v1
kind: Secret
metadata:
  name: {{ include "rustfs.secretName" . }}
type: Opaque
data:
  RUSTFS_ACCESS_KEY: {{ .Values.secret.rustfs.access_key | b64enc | quote }}
  RUSTFS_SECRET_KEY: {{ .Values.secret.rustfs.secret_key | b64enc | quote }}
{{- end }}

I think has no conflict with imagepullsecret which is another secret.

I think we should merge this code change, due to some change code already merged into main, and then let us discuss https://github.com/rustfs/rustfs/pull/1122, due to this PR has a lot of change, we will discuss and test one by one to make sure all the change has no break change for current helm chart.

majinghe avatar Dec 13 '25 02:12 majinghe

/LGTM

/cc @houseme @loverustfs

majinghe avatar Dec 13 '25 03:12 majinghe