render imagePullSecrets in Deployment/StatefulSet
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.
Tested on on-prem Kubernetes. The Deployment and StatefulSet now support a list of imagePullSecrets defined in values.yaml.
@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 , I'm working on it, I'll let you know once done.
@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 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, This is the change I was proposing earlier. Let me know what you think, and I can also update the README.md accordingly.
@czaloumis Very nice change, please update the README.md, please let me know when you done. Thx
Looks strange for me.
Instead of using native yaml with toYaml function, you render the secret in a strange way.
Hello @jurim76, thank you for your feedback. Could you please be more specific about your comments?
Hi @czaloumis
I've added imagePullSecrets support in this PR https://github.com/rustfs/rustfs/pull/1122
@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.
Hmm..never seen this approach, usually image.repository, image.tag and imagesPullSecrets is enough, sometimes chart uses service account.
@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.
Looks like it was added to main branch and it breaks possibility to use already existing secrets
@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.
/LGTM
/cc @houseme @loverustfs