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

Happy Eyeballs for EDS clusters #27881

Merged
merged 9 commits into from
Jun 18, 2023
Merged

Conversation

pradeepcrao
Copy link
Contributor

@pradeepcrao pradeepcrao commented Jun 8, 2023

Commit Message:
Additional Description:

This change adds the ability to associate multiple IP addresses with one host for an EDS cluster. The connection to the upstream will be made using Happy Eyeballs if the Endpoint has more than one address.

The primary use case for this is to allow dual-stack (i.e. with both IPv4 and IPv6 addresses) endpoints as a means to migrate from IPv4 to IPv6 backends. See https://docs.google.com/document/d/1AjmTcMWwb7nia4rAgqE-iqIbSbfiXCI4h1vk-FONFdM/edit#heading=h.xzptrog8pyxf for a more detailed description.

Risk Level: Low
Testing: Added test
Docs Changes: Added Doc
Release Notes: Added release note
Platform Specific Features: N/A

Signed-off-by: pcrao <pcrao@google.com>
Signed-off-by: pcrao <pcrao@google.com>
Signed-off-by: pcrao <pcrao@google.com>
@repokitteh-read-only
Copy link

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #27881 was opened by pradeepcrao.

see: more, trace.

@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @mattklein123
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #27881 was opened by pradeepcrao.

see: more, trace.

Signed-off-by: pcrao <pcrao@google.com>
Signed-off-by: pcrao <pcrao@google.com>
@pradeepcrao pradeepcrao marked this pull request as ready for review June 12, 2023 18:28
@pradeepcrao
Copy link
Contributor Author

/assign @RyanTheOptimist

Signed-off-by: pcrao <pcrao@google.com>
Copy link
Contributor

@RyanTheOptimist RyanTheOptimist left a comment

Choose a reason for hiding this comment

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

Nice! Though I find the idea of Happy Eyeballs without DNS to be a bit mind blowing :)

@pradeepcrao
Copy link
Contributor Author

Nice! Though I find the idea of Happy Eyeballs without DNS to be a bit mind blowing :)

Thank you so much for the review @RyanTheOptimist !

@pradeepcrao
Copy link
Contributor Author

/retest

@pradeepcrao
Copy link
Contributor Author

/assign-from @envoyproxy/senior-maintainers

@repokitteh-read-only
Copy link

@envoyproxy/senior-maintainers assignee is @mattklein123

🐱

Caused by: a #27881 (comment) was created by @pradeepcrao.

see: more, trace.

@pradeepcrao
Copy link
Contributor Author

/review @adisuissa @markdroth

mattklein123
mattklein123 previously approved these changes Jun 15, 2023
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Neat!

adisuissa
adisuissa previously approved these changes Jun 15, 2023
Copy link
Contributor

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

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

Changes LGTM!

Can you please add a comment next to the address_list_ data member, stating the underlying assumption that the first entry in the list must be the same host as address_ (better yet, add this as an assertion somewhere if possible).

Signed-off-by: pcrao <pcrao@google.com>
@pradeepcrao
Copy link
Contributor Author

Changes LGTM!

Can you please add a comment next to the address_list_ data member, stating the underlying assumption that the first entry in the list must be the same host as address_ (better yet, add this as an assertion somewhere if possible).

The following ASSERT fails for me in the ProtocolIntegrationTest.LogicalDns test. It appears that this assumption is not valid for LOGICAL_DNS

    void setAddressList(const std::vector<Network::Address::InstanceConstSharedPtr>& address_list) {
      address_list_ = address_list;
      ASSERT(address_list_.empty() ||
             std::find(address_list_.begin(), address_list_.end(), address_) != address_list_.end());
    }

@pradeepcrao
Copy link
Contributor Author

Changes LGTM!
Can you please add a comment next to the address_list_ data member, stating the underlying assumption that the first entry in the list must be the same host as address_ (better yet, add this as an assertion somewhere if possible).

The following ASSERT fails for me in the ProtocolIntegrationTest.LogicalDns test. It appears that this assumption is not valid for LOGICAL_DNS

    void setAddressList(const std::vector<Network::Address::InstanceConstSharedPtr>& address_list) {
      address_list_ = address_list;
      ASSERT(address_list_.empty() ||
             std::find(address_list_.begin(), address_list_.end(), address_) != address_list_.end());
    }

Never mind, I should be dereferencing the shared_ptrs for comparison because of how these are constructed

Signed-off-by: pcrao <pcrao@google.com>
Copy link
Contributor

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM!

@pradeepcrao
Copy link
Contributor Author

/restest

Signed-off-by: pcrao <pcrao@google.com>
@mattklein123 mattklein123 merged commit 652d23d into envoyproxy:main Jun 18, 2023
91 checks passed
asheryerm pushed a commit to asheryerm/envoy that referenced this pull request Jul 5, 2023
Signed-off-by: pcrao <pcrao@google.com>
Signed-off-by: asheryer <asheryer@amazon.com>
reskin89 pushed a commit to reskin89/envoy that referenced this pull request Jul 11, 2023
Signed-off-by: pcrao <pcrao@google.com>
Signed-off-by: Ryan Eskin <ryan.eskin89@protonmail.com>
soulxu pushed a commit that referenced this pull request Sep 8, 2023
* Allow custom local address resolvers.

#27881 introduces the concept of EDS clusters with hosts that have multiple (potentially > 2) IP addresses.

The current implementation of UpstreamLocalAddressSelector limits the number of source addresses in BindConfig artificially to 2, and further requires that the addresses be of different address families.

The workaround for this (if we need to specify more than 2 source addresses or have multiple addresses from the same family) is to use a custom address resolver that resolves the bind config address to nullptr (and therefore ignore it) and call bind in a customised SocketInterfaceImpl to a local source address determined by the SocketInterfaceImpl specialisation.

This PR makes it possible to define a custom local address selector, that makes it easy to work with a custom address resolver to pick the right source address based on the upstream address selected by HappyEyeballsConnectionImpl

Signed-off-by: pcrao <pcrao@google.com>
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.

None yet

4 participants