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

Support Kubernetes ConfigMap and Secrets as configuration/value sources #8376

Merged
merged 4 commits into from
Apr 6, 2020

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Apr 3, 2020

Still to do:

  • Add Secret support
  • Add documentation (added in Add docs for Kubernetes Config extension #8739)
  • Figure out whether we should support namespaced ConfigMaps (and Secrets) - the current implementation only looks for them in the current namespace
  • Figure out whether this should be a separate extension or not (perhaps it should because then that would open up opportunities to use a slimmer version of the Kubernetes Model in the future)

Fixes: #6745

@geoand
Copy link
Contributor Author

geoand commented Apr 3, 2020

@Ladicek
Copy link
Contributor

Ladicek commented Apr 3, 2020

IMHO, this totally needs to be a separate extension.

Also, I'm leaving for your consideration whether this should or shouldn't be part of SmallRye Config. For example, here's a PR that adds a Kubernetes config source in SRye Config: smallrye/smallrye-config#238 Except it's a fancy filesystem config source :-/ This one is what I'd expect from a Kubernetes config source.

@geoand
Copy link
Contributor Author

geoand commented Apr 3, 2020

We can certainly start out having this here and move the core parts into the SmallryeConfig if that makes sense.

@gastaldi
Copy link
Contributor

gastaldi commented Apr 3, 2020

i agree with @Ladicek ,that should be a separate extension (like the config YAML extension). Preferably having SM Config support should be the first step (to avoid API inconsistencies, etc)

@geoand
Copy link
Contributor Author

geoand commented Apr 3, 2020

I don't think that SR Config support should be a prerequisite for this, but a useful future avenue to explore.

@boring-cyborg boring-cyborg bot added the area/dependencies Pull requests that update a dependency file label Apr 3, 2020
@geoand
Copy link
Contributor Author

geoand commented Apr 3, 2020

I moved the code into its own extension.

Also, I think that item no.2 can be addressed later on if people request it, while I would like to do the documentation in a subsequent PR.

@geoand geoand changed the title WIP: Support Kubernetes ConfigMap and Secrets as configuration/value sources Support Kubernetes ConfigMap and Secrets as configuration/value sources Apr 3, 2020
@gastaldi gastaldi requested a review from dmlloyd April 3, 2020 13:50
@Ladicek
Copy link
Contributor

Ladicek commented Apr 3, 2020

Don't be lazy, documentation is as important as tests 😛 😁

@geoand
Copy link
Contributor Author

geoand commented Apr 3, 2020

Sure, but I don't want the PR to be help up by typos or my very frequent habit of forgetting to type words as I think about that I need to write 😂

Copy link
Member

@dmlloyd dmlloyd left a comment

Choose a reason for hiding this comment

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

The config source stuff looks OK to me, I don't know anything about the Kubernetes part though. It might be a good idea to explain (in the commit message) why (if I understand correctly) the "volume" approach which io.smallrye.config.source.file.FileSystemConfigSource exploits was not chosen.

@geoand
Copy link
Contributor Author

geoand commented Apr 3, 2020

@dmlloyd the idea is to provide users the ability to read ConfigMap or Secret via the Kubernetes API instead of the file system.
The advantage of this approach is that the user doesn't need to alter the Kubernetes manifest to configure it to mount a ConfigMap or Secret.

So it's basically complementary to what users can already do with regular config sources.

@geoand
Copy link
Contributor Author

geoand commented Apr 3, 2020

A follow-up for @iocanel:

When this extension is active, generate the proper RoleBinding for reading ConfigMap, and Secret from the kubernetes extension.

@geoand
Copy link
Contributor Author

geoand commented Apr 4, 2020

@maxandersen any comments on this?

@maxandersen
Copy link
Contributor

@maxandersen any comments on this?

discussed here https://quarkusio.zulipchat.com/#narrow/stream/187038-dev/topic/k8s.20config/near/193046063

but in short: looks good, its default off which is good as its set to also fails if config/secrets not reachable. I haven't tried to run it in practice (had build issues today ;/ ) but looking at code and expected semantics looks good imo.

@geoand
Copy link
Contributor Author

geoand commented Apr 6, 2020

Thanks for checking @maxandersen.

I'll write some documentation within the week.

@iocanel mind looking at the RoleBinding enhancement I mentioned above?

@geoand geoand merged commit 99b5ddc into quarkusio:master Apr 6, 2020
@geoand geoand deleted the #6745 branch April 6, 2020 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dependencies Pull requests that update a dependency file area/kubernetes area/testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Kubernetes ConfigMap and Secrets as configuration/value sources
5 participants