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

Translate Address to tagged WAN address in HTTP API when appropriate. #2118

Closed
wants to merge 1 commit into from
Closed

Translate Address to tagged WAN address in HTTP API when appropriate. #2118

wants to merge 1 commit into from

Conversation

dvgica
Copy link

@dvgica dvgica commented Jun 15, 2016

Likely of interest to @slackpad and @evan2645.

This PR builds on #1698. That PR translates a returned IP address to a WAN address when Consul is configured appropriately, but only through the DNS interface.

This PR additionally translates a node's address to the WAN address in the HTTP API. The rationale is:

  • the DNS and HTTP APIs should return consistent results
  • returning an IP local to DC1 to an HTTP client in DC2 is likely not useful (the client can't do anything with it)

The particular use case that motivated this change is "using the HTTP API in a NAT'd, multi-DC environment, I want to query for all nodes providing service X, and I want to be able to connect to those nodes using the returned Address".

The changes that this PR makes are:

  • adding TaggedAddresses to the ServiceNode struct (so that the catalog_endpoint.go, in particular, has access to them and co do the WAN translation before returning the result)
  • setting TaggedAddresses in the ServiceNode struct in state_store.go
  • moving translateAddr into agent.go and modifying the signature slightly so that it can be used from both HTTP and DNS handlers
  • translating node addresses in the following HTTP APIs (tests added for all)
    • /v1/catalog/nodes
    • /v1/catalog/node/<node>
    • /v1/catalog/service/<service>
    • /v1/health/service/<service>
    • /v1/query/<query or name>/execute

Added tests are passing, but it seems some of the existing tests are somewhat flaky and will occasionally fail. AFAICT, the failures don't have anything to do with this PR's additions.

First time Go user, view the code with suspicion :-).

@dvgica
Copy link
Author

dvgica commented Jun 15, 2016

Of course it occurs to me now that I completely neglected any documentation changes. I will be happy to do those once I've received some feedback on this :-).

@dvgica
Copy link
Author

dvgica commented Jun 22, 2016

I should also mention that I'm not really happy with all the places that I needed to insert TranslateAddr. Future committers will need to remember to do this, which is error-prone. Am very open to other ideas on how to do this (presenter layer, perhaps?).

@slackpad
Copy link
Contributor

Hey @DWvanGeest thanks for the PR and sorry for the delay. I did a little follow on work for this under #2275 and will be merging here in a sec. Code looked great!

@dvgica
Copy link
Author

dvgica commented Aug 16, 2016

Awesome, thanks for getting it merged!

@dvgica dvgica deleted the wan-translation-in-http-api branch May 9, 2017 21:14
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