search

quarkiverse / quarkus-vault

Quarkus HashiCorp Vault extension
Apache License 2.0
18 stars 22 forks source link

First time fetch in Vault Config Source done multiple times #280

Closed vsevel closed 2 months ago

vsevel commented 2 months ago

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

moved firstTime = false; above the call to fetchSecretsFirstTime added a finally block on cache.set(new VaultCacheEntry<>(properties)); caught HttpTimeoutException

to reproduce, used toxiproxy with:

toxiproxy-cli create -l localhost:18200 -u localhost:8200  vault
toxiproxy-cli toxic add -t latency -a latency=5000 -a jitter=2000 vault

and configuration:

quarkus.vault.url=http://localhost:18200
quarkus.vault.mp-config-initial-attempts=3
quarkus.vault.secret-config-kv-path=myapps/vault-quickstart/config,myapps/vault-quickstart/config2
kdubb commented 2 months ago

@vsevel With #257 you should be catching TimeoutException, correct?

vsevel commented 2 months ago

Well no. I tested. That IS what we are getting. Or we need to convert it when it is raised somewhere

kdubb commented 2 months ago

Then #257 wasn't implemented correctly. I think we should be fixing that problem rather than attempting to catch multiple timeouts all over the place; it's just easier to remember.

kdubb commented 2 months ago

I'm guessing that https://github.com/quarkiverse/quarkus-vault/blob/b2690ed4f943928e7a6484c7006139d2203fee4b/client/src/main/java/io/quarkus/vault/client/http/jdk/JDKVaultHttpClient.java#L51 needs to unwrap an exception.

Otherwise I don't see how this exception is being passed up the chain.

vsevel commented 2 months ago

Ok I will give it a try. It just I feel I have been chasing that one up so many times...

vsevel commented 2 months ago

@kdubb this was located in mapError indeed. I left the original catch, and added the handling of CompletionException. I do not know if exceptionallyCompose will always pass a CompletionException? that is not the case from the signature. but may be in our case that would be?

kdubb commented 2 months ago

That's what I thought when I mentioned unwrapping (I couldn't think of which exception it was probably throwing). I seem to remember the CompletionStage always wraps the exception in java.util.concurrent.CompletionException.

kdubb commented 2 months ago

I'd leave it as is or maybe make it recursive to make it easier to add exceptions to. Of course, then you're relying on Throwable.cause not being recursive, and I think it sometimes is.

vsevel commented 2 months ago

make it recursive to make it easier to add exceptions to. Of course, then you're relying on Throwable.cause not being recursive, and I think it sometimes is.

that is what I was concerned of. I think we can live with the repetition. I will squash then if you are ok.

kdubb commented 2 months ago

👍