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

Add tagged addresses for services #5965

Merged
merged 7 commits into from
Jun 17, 2019
Merged

Conversation

mkeeler
Copy link
Member

@mkeeler mkeeler commented Jun 13, 2019

Currently these are not used anywhere but they will be eventually. I hooked it into the WAN address translation for both DNS and the HTTP server.

  • Service Definition Docs - Updated to show that the fields exist with no other mention of why you might use them. Currently nothing does but when they do I will update these docs some more.
  • consul services register - Added tagged address support
  • API - Added support in both agent and catalog/health APIs to allow usage of tagged addresses.
  • Config - Added necessary code to enable loading the tagged addresses for service definitions within a config.
  • Structs - Added the necessary fields to several structures and updated a bunch of functions to be aware of them - like IsSame methods and conversions to/from ServiceNode/NodeService.

Currently these are not used anywhere but they will be eventually.
@mkeeler mkeeler added this to the 1.6.0 milestone Jun 13, 2019
@mkeeler mkeeler requested a review from a team June 13, 2019 19:50
Copy link
Member

@banks banks left a comment

Choose a reason for hiding this comment

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

Looks good. I understand that while you can add these they basically don't do anything for now right? i.e. no way to have a tagged address return from DNS rather than the existing address logic? Is it confusing that translate_wan_addrs won't actually work at the service level for now?

A couple of places I could think of that weren't in this PR but might need considering:

  • TaggedAddresses will need camel case translation. It might be that it already works due to us using that field name elsewhere but an explicit test case in config tests seems important. (
    desc: "translated keys",
    and
    func TestAgent_RegisterService_TranslateKeys(t *testing.T) {
    )
  • API docs for the affected endpoints? We don't list all possible fields but we could show an example of a service result with tagged addresses?
  • Would be nice to have a test for the HTTP endpoint(s) that return services to show that we don't add an empty TaggedAddresses map into every response (e.g. show omitempty is set right in the relevant structs).

@mkeeler
Copy link
Member Author

mkeeler commented Jun 14, 2019

For the camel case thing, I tried to do it how the node level tagged addresses work. Which is in our config file you specify tagged_addresses but in the API and the main service definition in the structs package its only CamelCase. I went looking around but it didn't seem like we allowed snake case anywhere but the config. I guess the service registration is a bit different. Also we don't seem to translate keys when the v1/catalog/register endpoint is used instead of v1/agent/service/register.

I can make snake case work in those other situations easily enough as well as the rest of your points.

I went ahead and implemented the WAN addr translation as it was pretty easy. Still need a couple tests for that though.

@mkeeler
Copy link
Member Author

mkeeler commented Jun 14, 2019

Current TODO list:

  • DNS Service Address/Port translation tests
  • HTTP Service Address/Port translation tests
  • Should we synthesize the "lan" address to be the service address and port?
  • API docs where service tagged addresses can be used
  • HTTP endpoint tests to ensure we omit the empty map properly.
  • snake case for tagged addresses in the service definition.

@mkeeler mkeeler requested review from banks and a team June 14, 2019 19:35
@mkeeler
Copy link
Member Author

mkeeler commented Jun 14, 2019

@banks Not only did I add all the tests and ensure the snake case stuff was taken care of but I went ahead and plumbed the address translation for services into both the DNS and HTTP servers.

For DNS the precedence of addressing when querying a remote datacenter is now:

  1. "wan" tagged service address
  2. service address
  3. "wan" tagged node address
  4. node address

Before we just had layers 2-4.

Additionally for SRV records we now translate the port as well.

In the HTTP API the translation will change the service address returned and the services port. Essentially it just replaces them with the ones from the "wan" tagged address if that exists similar to how the HTTP API replaces the node address with the "wan" tagged node address.

Copy link
Member

@banks banks left a comment

Choose a reason for hiding this comment

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

Wow, this looks awesome!

I verified the a.config.TranslateWANAddrs is respected as all the translation remains encapsulated in TranslateAddress* methods.

Docs updates all look great!

Copy link
Member

@banks banks left a comment

Choose a reason for hiding this comment

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

Sigh... I meant the other radio button!

@mkeeler mkeeler changed the title Add tagged addresses into the data model for services Add tagged addresses for services Jun 17, 2019
@mkeeler mkeeler merged commit f3d9b99 into release/1-6 Jun 17, 2019
@mkeeler mkeeler deleted the tagged-service-addresses branch June 17, 2019 14:51
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