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

Pure JDK Vault Client for Vault Config Source #235

Closed
wants to merge 1 commit into from

Conversation

vsevel
Copy link
Contributor

@vsevel vsevel commented Feb 13, 2024

Fixes #226

@kdubb
Copy link
Contributor

kdubb commented Feb 13, 2024

How are you setting up the proxy configuraiton for the JDK client? Also, wouldn't it be better to just merge #215?

The only outstanding issue I ran into for using the JDK for configurtion in the new client was setting SSL and proxy to match the Vert.x implementation exactly.

@kdubb
Copy link
Contributor

kdubb commented Feb 13, 2024

Merging 215 has the added advantage that we can close all but 1 PR 👍

@vsevel
Copy link
Contributor Author

vsevel commented Feb 13, 2024

this started as a POC. I would like to fix #226, as we are spending a lot of time trying to chase situations where dev mode does not work.

How are you setting up the proxy configuration for the JDK client?

not done at this point.

Also, wouldn't it be better to just merge #215?

yes. definitely if it is ready. is it? you could grab the ssl stuff from my PR? not sure about the proxy configuration (I did not do it).

we have hit so many issues related to the fact that the vault config source was using quarkus stuff, that I would like to get that behind us.

@vsevel
Copy link
Contributor Author

vsevel commented Feb 13, 2024

@kdubb
Copy link
Contributor

kdubb commented Feb 13, 2024

For me #215 is ready, there are a couple feedback items unresolved that you might want to close or comment further on.

I can move your SSL configuration into #215 easily and we can use the JDK client for configuration request.

The proxy configuration is what was troublesome to me for using the JDK with Quarkus's configuration.

@kdubb
Copy link
Contributor

kdubb commented Feb 13, 2024

are we talking about this? https://openjdk.org/groups/net/httpclient/recipes-incubating.html#proxy

Kind of. I saw no way of building a proxy selector using Vert.x proxy configuration items.

@vsevel
Copy link
Contributor Author

vsevel commented Feb 13, 2024

I like better what you did to be honest. we need it to go through.
did you check that it supports dev mode restart?
I am missing something. we have everything we need for vertx? don't we? i.e. io.vertx.ext.web.client.WebClientOptions
it is actually for the http client that we are missing a way to configure the non-proxy-hosts (is it supposed to be in the proxy selector?)

@vsevel
Copy link
Contributor Author

vsevel commented Feb 13, 2024

one thing to consider is that we do not need to support an entire jdk vault client for the config source. if we do, good. but it is not a requirement. we just need login and read secret (v1 and v2).
so if that makes it difficult to reuse the same approach between the config source and the vault api use cases, I would rather completely separate them.

@kdubb
Copy link
Contributor

kdubb commented Feb 13, 2024

The issue is, as I found it (I may be wrong), configuring a JDK HttpClient to work nearly identicaally to the Vert.x WebClient/HttpClient. I found most of the configuration to be easily transferrable or not required to produce similar functionality (e.g. timeouts just need to be correct in aggregrate). The two issues I ran into were configuring the JDK's SSLContext and ProxtySelector. I knew the SSLContext configuration was doable but stumbled at the ProxySelector; given the proxy issue I stopped at that point and returned to using Vert.x clients even for the configuration requests. The specific issue is that the JDK's DefaultProxySelector doesn't support the same configuration as Vert.x.

Give this we have a few options:

  1. Configure the proxy as well as possible using the available Vert.x settings
  2. Write our own, or find an available, ProxySelector implementation that supports all the options that Vert.x does.
  3. Ignore the proxy configuration until some point in the future where we implement 1 or 2.

Given that you have tackled the SSL configuration. The proxy configuration is the only remaining hurdle for both this PR and #215.

This seems to mean that we are in agreement that whatever path we decide we should use #215.

@kdubb
Copy link
Contributor

kdubb commented Feb 13, 2024

one thing to consider is that we do not need to support an entire jdk vault client for the config source. if we do, good. but it is not a requirement. we just need login and read secret (v1 and v2).

#215 does implement the entie thing and as noted is only block by the proxy configuration problem. There is no other reason not to use it's JDK client in Quarkus.

@vsevel vsevel marked this pull request as draft February 16, 2024 09:25
@kdubb
Copy link
Contributor

kdubb commented Mar 1, 2024

Implemted via #215

@kdubb kdubb closed this Mar 1, 2024
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.

Unable to restart application when vault extension is present
2 participants