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

feat(providers): Add support for fetching from k8s secrets #186

Merged
merged 7 commits into from
Dec 14, 2023

Conversation

bersalazar
Copy link
Contributor

@bersalazar bersalazar commented Dec 11, 2023

Allows fetching values from Kubernetes secrets.

Authentication to the Kubernetes cluster is managed by referencing the local kubeconfig.

Covers #185

Allows fetching values from Kubernetes secrets.

Authentication to the Kubernetes cluster is managed by
referencing the local kubeconfig.

Signed-off-by: Bernardo Salazar <[email protected]>
pkg/providers/k8s/k8s.go Outdated Show resolved Hide resolved
@bersalazar bersalazar force-pushed the add-k8s-secrets-support branch from bbbcf33 to ea63792 Compare December 12, 2023 12:21
pkg/providers/k8s/k8s.go Outdated Show resolved Hide resolved
@yxxhero
Copy link
Member

yxxhero commented Dec 12, 2023

@bersalazar could you add some tests for this feature?

@aslafy-z
Copy link
Contributor

aslafy-z commented Dec 12, 2023

What about config maps? Maybe we can prep to support them in the future by leaving room in the api for them?

Maybe /[namespace/][apiVersion/kind/]name[#path.to.key]

pkg/providers/k8s/k8s.go Outdated Show resolved Hide resolved
@bersalazar
Copy link
Contributor Author

What about config maps? Maybe we can prep to support them in the future by leaving room in the api for them?

Maybe /[namespace/][apiVersion/kind/]name[#path.to.key]

Definitely worth considering. Maybe in a future PR, happy to raise an issue for it once this one's merged.

@aslafy-z
Copy link
Contributor

What about config maps? Maybe we can prep to support them in the future by leaving room in the api for them?

Maybe /[namespace/][apiVersion/kind/]name[#path.to.key]

Definitely worth considering. Maybe in a future PR, happy to raise an issue for it once this one's merged.

I still think we should ensure this implementation is future proof to reduce chances of breaking changes when we implement this.

@bersalazar
Copy link
Contributor Author

@bersalazar could you add some tests for this feature?

@yxxhero added it

pkg/providers/k8s/k8s.go Outdated Show resolved Hide resolved
pkg/providers/k8s/k8s.go Outdated Show resolved Hide resolved
pkg/providers/k8s/k8s.go Outdated Show resolved Hide resolved
@yxxhero
Copy link
Member

yxxhero commented Dec 13, 2023

@bersalazar please see my newest comments. it will be perfect after you update.

Updates the reference to allow other Kubernetes objects
like ConfigMaps, which can become a valid use case.

Also adds unit tests.

Signed-off-by: Bernardo Salazar <[email protected]>
@bersalazar
Copy link
Contributor Author

What about config maps? Maybe we can prep to support them in the future by leaving room in the api for them?
Maybe /[namespace/][apiVersion/kind/]name[#path.to.key]

Definitely worth considering. Maybe in a future PR, happy to raise an issue for it once this one's merged.

I still think we should ensure this implementation is future proof to reduce chances of breaking changes when we implement this.

@aslafy-z sure thing, I have modified the proposed API so its more generic and can cover ConfigMaps as well, eventually. Thanks for suggesting! it makes sense to avoid future breaking changes, as you mentioned.

@bersalazar
Copy link
Contributor Author

@bersalazar please see my newest comments. it will be perfect after you update.

@yxxhero I've had a look at them and pushed updates, including unit tests.

Copy link
Contributor

@aslafy-z aslafy-z 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 taking my point into account! Here's two little nit so we make the doc obvious for the secretref case

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@yxxhero
Copy link
Member

yxxhero commented Dec 14, 2023

@bersalazar please fix lint error.

@bersalazar
Copy link
Contributor Author

@bersalazar please fix lint error.

@yxxhero all good now, thanks.

Do you mind having a look at helmfile/helmfile#1212?
It's not directly related to this PR, but it's one of the first use cases for this new k8s provider.
Would appreciate understanding how to deal with passing the kube context down to other helmfiles, or at least see if we need to raise an issue for it.

@yxxhero yxxhero linked an issue Dec 14, 2023 that may be closed by this pull request
Signed-off-by: Bernardo Salazar <[email protected]>
@yxxhero yxxhero merged commit b4e9a52 into helmfile:main Dec 14, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: add support for Kubernetes provider
3 participants