search

quarkiverse / quarkus-vault

Quarkus HashiCorp Vault extension
Apache License 2.0
19 stars 24 forks source link

Pure JDK Vault Client for Vault Config Source #235

Closed vsevel closed 6 months ago

vsevel commented 6 months ago

Fixes https://github.com/quarkiverse/quarkus-vault/issues/226

kdubb commented 6 months ago

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 commented 6 months ago

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

vsevel commented 6 months ago

this started as a POC. I would like to fix https://github.com/quarkiverse/quarkus-vault/issues/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 https://github.com/quarkiverse/quarkus-vault/pull/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 commented 6 months ago

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

kdubb commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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.

kdubb commented 6 months ago

Implemted via #215