nextcloud
Fixed redis password in clear text
Pull Request
Description of the change
Always read redis password form secret.
Benefits
Avoid printing the redis secret in clear text when applying the chart
Possible drawbacks
Unknowns
Applicable issues
None
Additional information
None
Checklist
- [X] DCO has been signed off on the commit.
- [X] Chart version bumped in
Chart.yamlaccording to semver. - [ ] (optional) Variables are documented in the README.md
Hoi!
I'm not sure we should do this, as this would break the user's ability to utilize the bitnami redis helm chart's existing features defined in their values.yaml, which I think we want to follow as closely as possible, unless it would break nextcloud in some way. If we merged this, it would make testing quickly a bit harder, as people would need to create secrets ahead of time, which is indeed better infosec practice, but this is also not consistent with the rest of our password management. If the bitnami hosted redis helm chart updates to remove their plain text password field, it would make sense to remove it here, but I feel like until then, we should still maintain compatibility with their chart.
I'm open to feedback and others opinions whenever you or others get a chance.
Kind regards, Jesse
@tvories or @provokateurin am I missing anything here? Should we close this?
This removes some of the bitnami redis chart's existing features though, and makes testing a bit harder. I think if we merged this, we might get complaints.
Ok, I'm back after re-reading the bitnami redis helm chart some more. So, this change actually won't break anything. Sorry, @slombard2011, for my misunderstanding earlier!
I looked at the secret in the bitnami redis helm chart templates, here and it looks like they do create this secret if you pass in a plain text password :) So yeah, we just need to bump the chart version.
@slombard2011 if you have the time, could you please bump the helm chart version.
You can also create another PR where you allow edits by maintainers. This will allow us to bump the helm chart version or commit specific minor suggestions to get your PR up to date for merging.
I'm going to close this, as it's been a couple of years without a response. I, or another nextcloud/helm collaborator/maintainer, can reopen if @slombard2011 comes back to bump the chart version.
If others in the community still want this feature, you can submit a new PR for this feature, but please make sure to bump the chart version and allow edits by maintainers, so we can always move the PR forward even in your absence. See more info in our contributing doc.