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.

Sign up for GitHub

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

Jump to bottom

Add support for dual stack IPv4/IPv6 network #6640

Merged
merged 4 commits into from
Jan 17, 2020
Merged

Add support for dual stack IPv4/IPv6 network #6640

merged 4 commits into from
Jan 17, 2020

Conversation

ShimmerGlass
Copy link
Contributor

Implementation for #6531

As described in the issue, we can only set one address for a service or a node. In cases were the network is dual stack IPv4/IPv6 we would like to specify addresses for both protocols.

We add config options for node :

  • advertise_addr_ipv4
  • advertise_addr_ipv6
  • advertise_addr_wan_ipv4
  • advertise_addr_wan_ipv6

These addresses are set in the TaggedAddress map and available in HTTP endpoints

For services, TaggedAddresses can be set directly when registering the service.

The keys for TaggedAddress entries are defined in the structs package :

	TaggedAddressWAN     = "wan"
	TaggedAddressWANIPv4 = "wan_ipv4"
	TaggedAddressWANIPv6 = "wan_ipv6"
	TaggedAddressLAN     = "lan"
	TaggedAddressLANIPv4 = "lan_ipv4"
	TaggedAddressLANIPv6 = "lan_ipv6"

For both nodes and services, tagged addresses take default from the "address" field when compatible. For example, running an agent with advertise_addr = "1.2.3.4" will automatically set lan_ipv4 and wan_ipv4, unless they are explicitly specified. For services the "Address" field is used.

Left to do :

pierresouchay
pierresouchay reviewed Oct 18, 2019
View reviewed changes
Copy link
Contributor

@pierresouchay pierresouchay left a comment

Choose a reason for hiding this comment

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

@mkeeler is it what you had in mind? (modulo the missing DNS)

@ShimmerGlass ShimmerGlass mentioned this pull request Nov 20, 2019
Closed
mkeeler
mkeeler reviewed Dec 3, 2019
View reviewed changes
Copy link
Member

@mkeeler mkeeler left a comment

Choose a reason for hiding this comment

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

This is exactly what I had in mind. There are a few places in the PR where we need some additional logic to chose the "best" address which will be needed in particular for DNS. I have added comments to a few of those locations.

Right now we select the best address in the following order:

For WAN:

  1. "wan" tagged address of the NodeService
  2. Address field on the NodeService
  3. "wan" tagged address of the Node
  4. Address field on the Node

For LAN

  1. Address field on the NodeService
  2. Address field on the Node

I actually think my original implementation missed using a tagged "lan" address. Regardless I think we want Address selection to now have the following precedence.

For WAN + IPv6 Preference

  1. "wan_ipv6" tagged address of the NodeService
  2. "wan" tagged address of the service (if IPv6)
  3. Address field of the NodeService (if its IPv6)
  4. "wan_ipv6" tagged address of the Node
  5. "wan" tagged address of the Node (if its IPv6)
  6. Address field on the Node (if its IPv6)

For WAN + No IPv6 Preference

  1. "wan_ipv4" tagged address of the NodeService
  2. "wan" tagged address of the service (if IPv4)
  3. Address field of the NodeService (if its IPv4)
  4. "wan_ipv4" tagged address of the Node
  5. "wan" tagged address of the Node (if its IPv4)
  6. Address field on the Node (if its IPv4)

For LAN + IPv6 Preference

  1. "lan_ipv6" tagged address of the NodeService
  2. "lan" tagged address of the service (if IPv6)
  3. Address field of the NodeService (if its IPv6)
  4. "lan_ipv6" tagged address of the Node
  5. "lan" tagged address of the Node (if its IPv6)
  6. Address field on the Node (if its IPv6)

For LAN + No IPv6 Preference

  1. "lan_ipv4" tagged address of the NodeService
  2. "lan" tagged address of the service (if IPv4)
  3. Address field of the NodeService (if its IPv4)
  4. "lan_ipv4" tagged address of the Node
  5. "lan" tagged address of the Node (if its IPv4)
  6. Address field on the Node (if its IPv4)

Also for xDS/Envoy I don't think there is anything you need to do at least for Mesh Gateways. The gateways already have the option to bind to all tagged addresses. If you are looking to add dual-stack support to a connect proxies listener then that will take a bit more effort.

agent/structs/structs.go
@@ -565,7 +574,7 @@ type Node struct {

func (n *Node) BestAddress(wan bool) string {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it would be useful to add a second argument to indicate whether IPv4 or IPv6 should be preferred.

agent/translate_addr.go Show resolved Hide resolved
@hanshasselberg hanshasselberg added the waiting-reply Waiting on response from Original Poster or another individual in the thread label Dec 13, 2019
@hanshasselberg hanshasselberg self-assigned this Dec 18, 2019
@stale stale bot removed the waiting-reply Waiting on response from Original Poster or another individual in the thread label Dec 18, 2019
@hanshasselberg hanshasselberg added the waiting-reply Waiting on response from Original Poster or another individual in the thread label Dec 18, 2019
@stale stale bot removed the waiting-reply Waiting on response from Original Poster or another individual in the thread label Dec 20, 2019
@pierresouchay
Copy link
Contributor

@Aestek Possible to fix the conflict?

@hanshasselberg
Copy link
Member

@Aestek let me know when you think this is good to go for another review.

@hanshasselberg hanshasselberg added the waiting-reply Waiting on response from Original Poster or another individual in the thread label Jan 13, 2020
@stale stale bot removed the waiting-reply Waiting on response from Original Poster or another individual in the thread label Jan 13, 2020
@ShimmerGlass
Copy link
Contributor Author

@i0rek Good for review. I also added the DNS part.

mkeeler
mkeeler approved these changes Jan 17, 2020
View reviewed changes
Copy link
Member

@mkeeler mkeeler left a comment

Choose a reason for hiding this comment

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

LGTM

I only had the one comment but seeing as no other changes are needed I don't see a reason for that one thing to prevent merging.

agent/agent.go
@@ -2451,6 +2471,34 @@ func (a *Agent) validateService(service *structs.NodeService, chkTypes []*struct
}
}

// Check IPv4/IPv6 tagged addresses
if service.TaggedAddresses != nil {
Copy link
Member

Choose a reason for hiding this comment

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

The nil check here should not be necessary as val, ok := service.TaggedAddresses[...] will work fine even on a nil map.

This shouldn't need to block merging though.

agent/translate_addr.go Show resolved Hide resolved
@mkeeler mkeeler merged commit ba8fd82 into hashicorp:master Jan 17, 2020
@mkeeler mkeeler mentioned this pull request Jan 17, 2020
Closed
@hanshasselberg hanshasselberg mentioned this pull request Jul 30, 2020
Merged
@freddygv freddygv mentioned this pull request Aug 17, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pierresouchay pierresouchay pierresouchay left review comments

@mkeeler mkeeler mkeeler approved these changes

Assignees

@hanshasselberg hanshasselberg

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ShimmerGlass @pierresouchay @hanshasselberg @mkeeler