SRV lookup done on every http request

[abh]

Expected Behavior

SRV lookup done when a new connection to the HTTPS server is necessary.

Current Behavior

SRV lookup is done on every HTTP request to Vault.

[averche]

Hi @abh,

Thanks for letting us know! The SRV lookup logic was added to replicate what we have in the existing vault/api library, which appears to also be doing SRV lookup on every request.

Do you have any suggested solutions in mind? If caching the host is required, do you know if it can be done at the net library level or what is the best approach here?

[abh]

My expectation as a user is that it'd follow the same logic the regular HTTP requests do for DNS lookups (so I guess a dialer function for the http.Transport that replaces the address and sets up the regular dialer with the new address? I'm sure it's more complicated than that sadly).

[maxb]

It's a bit surprising to me to hear that any SRV lookup occurs at all - as use of SRV with HTTP protocols was never standardised. There were a couple of Internet Drafts on the topic, but they didn't get enough momentum to ever graduate to a finished RFC.

With that as evidence, I'd claim that most users would be surprised to find an http: or https: URL attempting to use SRV records at all.

Since this is a relatively new library, I propose the way forward could be to just delete the SRV logic from vault-client-go entirely, and document it as a return towards common standard practice.

I would be very surprised to hear of any real Vault instances that needed SRV records to resolve them, as that would indicate they were incompatible with general purpose HTTP clients, and could only be reached using clients specifically customised to make use of SRV.

[averche]

use of SRV with HTTP protocols was never standardised. There were a couple of Internet Drafts on the topic, but they didn't get enough momentum to ever graduate to a finished RFC.

Thanks for this info! I did not know this and it's definitely a good argument in favour of removing the SRV support altogether from the new library.

For context, it looks like hashicorp/vault#3035 is the original PR that introduced SRV lookups in vault/api. Perhaps @jefferai can offer insight here :)

[maxb]

It appears @jefferai was just merging a contributed PR from @nrhall-deshaw, who appears to not have used GitHub in any way since January 2018...

The addition of SRV lookups caused pain for others - hashicorp/vault#5540 - leading to them being broken entirely by hashicorp/vault#8016 - and then put back as optional but off by default in hashicorp/vault#8520.

Since @spangenberg was interested in fixing them in vault/api in March 2020, maybe he knows of someone trying to actually use them?

Or perhaps @abh has a use-case in mind?

Unless someone comes forward with information about people actually using SRV records with Vault, I think we should just go ahead and drop the feature from the new client libraries.