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

Implement missing HTTP host to ConsulResolver func for Connect SDK. #4392

Merged
merged 3 commits into from
Jul 16, 2018

Conversation

banks
Copy link
Member

@banks banks commented Jul 13, 2018

I managed to remove a TODO in Connect SDK that was not done. I thought it was because we have tests for HTTPClient but the part that we made pluggable to ease testing actually didn't have a non-test implementation 😱 .

This fixes that.

UX Gotcha

In general this will work OK as a basic experience. It's a bit of a gotcha that we only support .consul TLD in the HTTP client hostnames even if your actual Consul DNS is configured with a custom domain. The reason for that is that without making external calls and adding much complication, we don't actually know what the correct domain is, and in conjunction with the datacenter suffix being optional, it becomes impossible to unambiguously tell the difference between a custom domain segment and a datacenter name (which matters because we want to support explicit requests to other DCs).

We could potentially revisit this in the future if it seems to be a big issue for folks but it is simplest to just document it for now.

Docs updates to explain this:

image

@banks banks requested a review from mitchellh July 13, 2018 21:49
Copy link
Contributor

@mitchellh mitchellh left a comment

Choose a reason for hiding this comment

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

All looks good to me.

@banks banks merged commit 4ec8c48 into master Jul 16, 2018
@banks banks deleted the connect-sdk-http branch July 16, 2018 12:54
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.

2 participants