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

Auto-populated protocol-specific tagged addresses do not respect default tags #8525

Open
freddygv opened this issue Aug 17, 2020 · 7 comments
Labels
type/bug Feature does not function as expected

Comments

@freddygv
Copy link
Contributor

We currently populate the tagged addresses for ipv4 and ipv6 from the service address if the service address is ipv4/6 and there is no existing address tagged as wan_ipv4/6.

That means that if someone adds a plain wan tagged address, the wan_ipv4/6 will be inherited from the service address and not the user-provided wan address.

Repro steps:

  1. consul agent -dev
  2. consul connect envoy -gateway=mesh -register -wan-address 192.168.0.4:8080 -address '{{ GetInterfaceIP "en0" }}'
  3. curl http://127.0.0.1:8500/v1/agent/services

Notice that the wan and wan_ipv4 addresses differ.

One potential solution here is to revert the tagged address defaulting in addServiceInternal that was added in #6640. This way only addresses specified by the user are present in the TaggedAddresses map. Modifying a provided service registration has been surprising to some users.

@freddygv freddygv added the type/bug Feature does not function as expected label Aug 17, 2020
@freddygv freddygv changed the title Prefer lan and wan tagged addresses over service address when populating IPv4/6 addresses Auto-populated protocol-specific tagged addresses do not respect default tags Aug 17, 2020
@freddygv
Copy link
Contributor Author

@pierresouchay do you have any thoughts about this?

@pierresouchay
Copy link
Contributor

@ShimmerGlass Any opinion on this?

@ShimmerGlass
Copy link
Contributor

This looks like an oversight in the original PR. Reverting the change would indeed solve the issue, but would be a step back for dual-stack support in Consul which is needed in our shop. Additionally, the wan_ipv{4,6} fields are pretty new and I would be surprised if many people use them already.

I see two ways to fix that :

  • Store the source for each tagged address, if a more specific one comes along, override it as needed. Eg: first populate wan_ipv4 from address, if wan_address is specified wan_ipv4 get overridden.
  • Apply the addresses in more-specific to less specific order: wan_address is applied first and populates wan_ipv4, address is applied but doesn't change wan_ipv4 because it is already set.

andrewoelfing added a commit to andrewoelfing/consul that referenced this issue Sep 7, 2020
@andrewoelfing
Copy link

Hello @pierresouchay, hello @ShimmerGlass, could this commit be a possible solution? At least it fixes the problem with the incorrect/differing WAN IP

@ShimmerGlass
Copy link
Contributor

@andrewoelfing It might, do you handle the case where the WAN address is a domain, not an IP?

@andrewoelfing
Copy link

Hello @ShimmerGlass, the documentation says the parameter only takes ip and port (or go-sockaddr templates).
Consul-Connect#wan-address

@freddygv
Copy link
Contributor Author

freddygv commented Sep 18, 2020

Hi @andrewoelfing, thank you for the reference commit. After taking a look at this I think it's preferable to address this bug in the agent code rather than in the connect sub-command because it affects all service registrations (as well as the lan tag, which I missed when I created this issue).

I pushed a PR to address this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug Feature does not function as expected
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants