api: mark internal listener as implemented and change internal listener binding#22274
Conversation
Signed-off-by: Kuat Yessenov <kuat@google.com>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
Signed-off-by: Kuat Yessenov <kuat@google.com>
|
@adisuissa would you mind taking a look /wait-any |
adisuissa
left a comment
There was a problem hiding this comment.
Thanks for cleaning this up, left a few minor comments.
/wait
| oneof address_name_specifier { | ||
| option (validate.required) = true; | ||
|
|
||
| // [#not-implemented-hide:] The :ref:`listener name <envoy_v3_api_field_config.listener.v3.Listener.name>` of the destination internal listener. |
There was a problem hiding this comment.
Please keep the comment describing the field.
There was a problem hiding this comment.
This comment is not true, address has nothing to do with the listener name.
There was a problem hiding this comment.
IIUC the value here needs to point to another listener.
Would it be possible to add a reference to the field that this value will be matched to?
There was a problem hiding this comment.
seems like it changed what the value expect to from the beginning. Maybe worth to ask again whether this field still name correctly, this can be a chance to change the name before expose to the end user.
There was a problem hiding this comment.
Yes, I agree we need to fix this. I think we should use a dedicated field in InternalListener instead of internal address in the generic address field in Listener. WDYT?
There was a problem hiding this comment.
The simplest thing is to just really use the listener name. That keeps LDS to a minimum and I can't think of any reason why more flexibility would be needed here (multiple addresses don't work anyways, but can always be added to InternalListener).
There was a problem hiding this comment.
I'm not sure the usecase of using multiple internal addresses, it seems useless. I guess the multiple internal addresses usecase is internal address mix with IP address. From this point of view, just using the listener name should be fine.
I little prefer just use a string for the internal address, needn't limit that to the listener name. The only reason I have is a soft reason, it seems more straightforward for me when configuring the internal address, just matching the upstream endpoint internal address with the listener internal address. And we needn't add a lot of checks in the code like the user can't specific addresses when this is internal listener...etc. But anyway those are soft reasons.
There was a problem hiding this comment.
I'm not sure the usecase of using multiple internal addresses, it seems useless. I guess the multiple addresses usecase is internal address mix with IP address. From this point of view, just use the listener name should be fine.
suddenly found this may be the multiple internal addresses usecase https://github.com/envoyproxy/envoy/pull/22274/files#r935253240
|
Looks like a simple API change: throwing over to @htuch as Adi is out. |
|
no relevant owners for "api," |
|
@kyessenov following Alyssa's comment, if the |
|
/wait on review notes - I'm happy to do that pass. I think if we don't have example configs we're going to want that as part of making this non-hidden. |
|
Affected by #22483. |
|
/retest |
|
Retrying Azure Pipelines: |
|
/retest |
|
Retrying Azure Pipelines: |
snowp
left a comment
There was a problem hiding this comment.
Not familiar enough with the internal listener work to give a true in depth review, but high level this looks good to me, just a docs comment
| In case there are multiple endpoints referencing the same internal listener in a single upstream cluster, use | ||
| :ref:`endpoint ID field <envoy_v3_api_field_config.core.v3.EnvoyInternalAddress.endpoint_id>` to improve change tracking | ||
| in the cluster pool. This field in combination with the internal listener name uniquely identify an endpoint in the |
There was a problem hiding this comment.
What happens if you don't use the endpoint id in case? What do you end up losing out on? Or does it just not work?
There was a problem hiding this comment.
It'll de-duplicate the internal addresses. The same happens for IP:PORTs, but not well-documented. This is due to HostMap using strings as a key for host tracking. Should we add this comment here?
There was a problem hiding this comment.
I think it would be helpful to either make the language prescriptive (you should do this when multiple IPs to ensure X) or help the reader understand what this does if we want to talk about this as an optional thing to do
|
/retest |
|
Retrying Azure Pipelines: |
|
/retest |
|
Retrying Azure Pipelines: |
|
/retest |
|
Retrying Azure Pipelines: |
|
Thanks @lambdai for doing the actual work! Hopefully we can get more usage and value now that it's public and easily available. |
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:
addressfield per xds: Listener.address field is not required when api_listener is populated #21132, and instead use the listenernamefield as originally expected.endpoint_idfield to upstream address to fix eds: allow duplicate internal addresses #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 usingasString().listener.envoy_internal_<listener_name>.Adds a few sample configs (and fixes #20335):
tcp_proxylisteners;Risk Level: low, hidden prior
Testing: yes
Docs Changes: yes
Release Notes: yes