eds: allow duplicate internal addresses#22420
eds: allow duplicate internal addresses#22420stevenctl wants to merge 1 commit intoenvoyproxy:mainfrom
Conversation
|
Hi @stevenctl, welcome and thank you for your contribution. We will try to review your Pull Request as quickly as possible. In the meantime, please take a look at the contribution guidelines if you have not done so already. |
|
You'll need to fix the DCO: https://github.com/envoyproxy/envoy/blob/main/CONTRIBUTING.md#dco-sign-your-work |
e5359fd to
70a65da
Compare
source/common/upstream/eds.cc
Outdated
| // An exception is made for internal addresses as passthrough metadata is usually what makes them unique. | ||
| const auto address_as_string = address->asString(); | ||
| if (all_new_hosts.count(address_as_string) > 0) { | ||
| if (address->envoyInternalAddress() == nullptr && all_new_hosts.count(address_as_string) > 0) { |
There was a problem hiding this comment.
I think this could be a bit improved:
if (address->type() != Type::EnvoyInternal) {
const auto address_as_string = address->asString();
if (all_new_hosts.count(...)) {
return;
}
all_new_hosts.emplace(address_as_string);
}
That will avoid counting internal address and unnecessarily creating a string.
There was a problem hiding this comment.
as a cpp newb.. wouldn't internaladdr != null already avoid the counting? Just double checking.
There was a problem hiding this comment.
Yeah, but you're still putting internal hosts into a map for no reason.
There was a problem hiding this comment.
The emplace call has to happen regardless of address->type(), so seems we should always create the string.
There was a problem hiding this comment.
Didn't see this comment ^
Are we certain this map is only used for dedupe? It eventually gets passed here:
envoy/source/common/upstream/upstream_impl.cc
Lines 1814 to 1824 in 86eef10
There was a problem hiding this comment.
It looks like it's trying to correlate the priority set and the new hosts map, so we should keep them consistent. I wonder if we should make the map be from a pointer instead. Let me look deeper at the usage.
kyessenov
left a comment
There was a problem hiding this comment.
I suspect this is not going to work as expected for addition/removal of internal address hosts with the same address. All the complex logic for diffing host sets relies on HostMap which itself relies on addressAsString. So something is likely to break (either removal not causing drain, or addition not affecting priority set). I don't see how we can easily change to not use addressAsString because hosts are re-created it seems on every EDS update. So what we need is a structural hash hash(internal_address_name, endpoint metadata) to be the address-as-string. I think we could simply append hash of the endpoint metadata to internal addresses? WDYT?
|
Another idea: change host address to support "extensions" to internal address identifier. That would be a protobuf change, but we could just add an opaque string as a designator of "host uniqueness" without affecting "network identity". |
Adds a "logical port" to internal addresses to help differentiate them from each other when they have differences in metadata. Could also be accomplished by appending a hash to the dedupe key. Signed-off-by: Steven Landow <landow@google.com>
I think changing asString on address has other implications. |
21b78ba to
e435db9
Compare
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
|
I can't tell what the issue is from the log. Can you reproduce it in a test with a standalone envoy? |
|
There's a difference between downstream and upstream. You don't want to have logical port on the downstream, so we need to strip the port there. |
|
I think we need to separate listener binding from address like in https://github.com/envoyproxy/envoy/pull/22274/files |
| * For IPv4 addresses: "1.2.3.4:80" | ||
| * For IPv6 addresses: "[1234:5678::9]:443" | ||
| * For pipe addresses: "/foo" | ||
| * For internal addresses: "serverListenerName-<hash of metadata>" |
There was a problem hiding this comment.
Is the port directly available as a property of the Address?
There was a problem hiding this comment.
I have another API PR that also changes how binding works #22274, because we don't want to bind to individual "ports". It's merely an endpoint identifier for diff tracking.
There was a problem hiding this comment.
@kyessenov will let you guide the review for API here and let me know when ready for another pass. It seems #22274 is solving the broader problem here.
|
Seems like #22274 is the fix |
…er binding (#22274) Signed-off-by: Kuat Yessenov <kuat@google.com> Commit Message: Expose internal listener functionality. Marks the fields as implemented. Adds the following functional changes prior to publishing: * do not use the `address` field per #21132, and instead use the listener `name` field as originally expected. * add an `endpoint_id` field to upstream address to fix #22420; it's not possible to have multiple internal addresses to the same listener in a single pool without some extra identifier beyond the listener name, because pools identify addresses using `asString()`. * fixes #20665 by using stat prefix `listener.envoy_internal_<listener_name>`. Adds a few sample configs (and fixes #20335): * minimal two chained `tcp_proxy` listeners; * encap HTTP in HTTP CONNECT; * decap HTTP in HTTP CONNECT; Risk Level: low, hidden prior Testing: yes Docs Changes: yes Release Notes: yes
…er binding (#22274) Signed-off-by: Kuat Yessenov <kuat@google.com> Commit Message: Expose internal listener functionality. Marks the fields as implemented. Adds the following functional changes prior to publishing: * do not use the `address` field per envoyproxy/envoy#21132, and instead use the listener `name` field as originally expected. * add an `endpoint_id` field to upstream address to fix envoyproxy/envoy#22420; it's not possible to have multiple internal addresses to the same listener in a single pool without some extra identifier beyond the listener name, because pools identify addresses using `asString()`. * fixes envoyproxy/envoy#20665 by using stat prefix `listener.envoy_internal_<listener_name>`. Adds a few sample configs (and fixes envoyproxy/envoy#20335): * minimal two chained `tcp_proxy` listeners; * encap HTTP in HTTP CONNECT; * decap HTTP in HTTP CONNECT; Risk Level: low, hidden prior Testing: yes Docs Changes: yes Release Notes: yes Mirrored from https://github.com/envoyproxy/envoy @ 02489bbf8990faddb6be1f2e0f22851b64c3fc85
…er binding (#22274) Signed-off-by: Kuat Yessenov <kuat@google.com> Commit Message: Expose internal listener functionality. Marks the fields as implemented. Adds the following functional changes prior to publishing: * do not use the `address` field per envoyproxy/envoy#21132, and instead use the listener `name` field as originally expected. * add an `endpoint_id` field to upstream address to fix envoyproxy/envoy#22420; it's not possible to have multiple internal addresses to the same listener in a single pool without some extra identifier beyond the listener name, because pools identify addresses using `asString()`. * fixes envoyproxy/envoy#20665 by using stat prefix `listener.envoy_internal_<listener_name>`. Adds a few sample configs (and fixes envoyproxy/envoy#20335): * minimal two chained `tcp_proxy` listeners; * encap HTTP in HTTP CONNECT; * decap HTTP in HTTP CONNECT; Risk Level: low, hidden prior Testing: yes Docs Changes: yes Release Notes: yes
Commit Message: allow duplicate internal endpoints
Additional Description: endpoints that use internal address should not be subject to deduplication as passthrough metadata is usually what distinguishes them
Risk Level: Low
Testing: TODO unit test
Docs Changes: N/A
Release Notes: should I add one? are there releases including internal address at this point?
Platform Specific Features: N/A
Fixes #22417