Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[common]: Enable mounting of existing secrets #148

Merged
merged 2 commits into from
Feb 27, 2025

Conversation

wthhub
Copy link
Contributor

@wthhub wthhub commented Feb 13, 2025

What this PR does:

Enable mounting of existing secrets and enable optional flag when mounting an existing ressources.

https://kubernetes.io/docs/concepts/configuration/secret/#restriction-secret-must-exist

Notes for Reviewer:
mostly copied from: #147

Checklist:

  • Pull Request title in format [chart]: Changed Something
  • Updated documentation in the README.md.gotmpl file and executed helm-docs
  • Chart Version bumped
  • All commits are signed-off

@wthhub wthhub requested a review from a team as a code owner February 13, 2025 15:22
@wthhub wthhub requested a review from brjos February 13, 2025 15:22
@github-actions github-actions bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 13, 2025
@wthhub wthhub requested review from fanks4 and removed request for brjos February 13, 2025 15:24
Copy link

@fanks4 fanks4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your pull request @wthhub
Sorry for my late answer. I was a little bit confused about the "existing" flag.
There are a couple of similar functions available. Have you considered whether components.[*].controller.envFrom.secretRef ,components.[*].controller.containers.[*].envFrom.secretRef or components.[*].controller.containers.[*].configFiles fits your usecase?

What do you think abourt the _files.yaml template? I guess it is saver to exclude existing secrets and configMaps somewhere around line 9. Unless .filePath is not set, everything should work anyway.
Sorry, that I did not realize this when I did the review for #147.

I added notices to two more comment lines.

Since I tested both types and optional flag I can confirm that the basic functionally is given with your change.

@wthhub
Copy link
Contributor Author

wthhub commented Feb 27, 2025

Thank you for your pull request @wthhub Sorry for my late answer. I was a little bit confused about the "existing" flag. There are a couple of similar functions available. Have you considered whether components.[*].controller.envFrom.secretRef ,components.[*].controller.containers.[*].envFrom.secretRef or components.[*].controller.containers.[*].configFiles fits your usecase?

What do you think abourt the _files.yaml template? I guess it is saver to exclude existing secrets and configMaps somewhere around line 9. Unless .filePath is not set, everything should work anyway. Sorry, that I did not realize this when I did the review for #147.

I added notices to two more comment lines.

Since I tested both types and optional flag I can confirm that the basic functionally is given with your change.

@fanks4, thanks for your review!

The requirement is that the files can be mounted as files, so the options as environment variables are not applicable.
With the existing configFiles options, only specific formats or Base64 encoded files can be mounted.
However, since we want to offer the possibility to mount all file types readably without overloading the Helm values file, we have decided on this extension. In addition, external secrets can thus be used multiple times.

The filePath attribute has nothing to do with the two PRs. This allows you to create a secret from a file. However, this requires that the file is "local" available. For the intended use case, however, an existing secret should be mountable.

@fanks4 fanks4 self-requested a review February 27, 2025 16:07
Copy link

@fanks4 fanks4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the explanations and your changes @wthhub.
I guess it's acceptable to let the user decide whether a secret should be generated or not.
If not filePath must not be set.

@wthhub
Copy link
Contributor Author

wthhub commented Feb 27, 2025

Thank you for the explanations and your changes @wthhub. I guess it's acceptable to let the user decide whether a secret should be generated or not. If not filePath must not be set.

opened an internal follow up task to clearify this

@wthhub wthhub merged commit c3ea5c8 into master Feb 27, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants