xds: Listener.address field is not required when api_listener is populated#21132
Conversation
…lated Signed-off-by: Mark D. Roth <roth@google.com>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
| // Linux as the actual port will be allocated by the OS. | ||
| core.v3.Address address = 2 [(validate.rules).message = {required: true}]; | ||
| // Required unless *api_listener* or *listener_specifier* is populated. | ||
| core.v3.Address address = 2; |
There was a problem hiding this comment.
Should this be promoted to oneof in the future?
If so, we should move the other fields next to this one and add the oneof_promotion tag next to them.
Also this should probably also be reflected in the code. Looking at the ListenerImpl c'tor, it seems that the code will reject this config.
There was a problem hiding this comment.
See #15372 for the plan on eventually converting this to a oneof. It will wind up being restructured to make that happen.
Envoy does not currently support non-socket listeners, so the address field is required for Envoy. If the code rejects that config, then that sounds like the right behavior to me.
|
/retest |
|
Retrying Azure Pipelines: |
|
This CI failure is legit as there is a test that looks like it's relying on this. It will need to be switched to something else unfortunately. /wait |
|
Looking at the failing test, I don't quite know what the right way is to fix it. The test itself looks like it's just checking the PGV annotations on the Listener proto itself, and the annotation I'm removing here is basically the only one in that proto that could cause it to fail, which would suggest that maybe that test can just be removed. But on the other hand, the test is called Can someone please advise as to the right way to address this? Thanks! |
Yes the test is there is verify a specific type of failure case so we need to use some other proto to test the same thing. |
Signed-off-by: Mark D. Roth <roth@google.com>
Signed-off-by: Mark D. Roth <roth@google.com>
|
/retest |
|
Retrying Azure Pipelines: |
|
I'm not sure what's going on with the remaining CI failure here, but it doesn't look related to the PR. |
|
/retest |
|
Retrying Azure Pipelines: |
|
Looks like CI is green now. Can we get this merged? Thanks! |
…lated (envoyproxy#21132) Signed-off-by: Mark D. Roth <roth@google.com>
…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
Signed-off-by: Mark D. Roth roth@google.com
Commit Message: xds: Listener.address field is not required when api_listener is populated
Additional Description: This removes the overly restrictive PGV annotation that the address field is required, since there are cases where it does not need to be populated.
Risk Level: Low
Testing: N/A
Docs Changes: Included in PR
Release Notes: N/A
Platform Specific Features: N/A
The right long-term solution is to use a oneof for the different types of listeners, as described in #15372. But in the interim, it should not be necessary to populate the address field for non-socket listeners.
CC @costinm