-
Notifications
You must be signed in to change notification settings - Fork 5.4k
listener: support multiple addresses #19367
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
Changes from all commits
c3e5b6e
40c2080
84094cf
cf23597
0aa830b
091bb27
37c5260
8982d2a
f08fd45
6ca527f
c8d220a
33d81ca
7fa7666
415b6dd
8a1bb30
eb464d1
cab7dcc
889b9fc
2b2f729
b913699
d3587dc
ccc2c73
9d6959e
2563b05
96eb8bb
59594e4
aedc178
65b8ed1
6841036
4f04597
cee07ab
3dc7635
bbb7057
7de085f
ad3ecee
0021a9b
365b584
6faa8f2
a1823c5
c5409dd
5b5577f
0921df1
871eb36
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,7 +31,7 @@ message ListenerStatus { | |
| // Name of the listener | ||
| string name = 1; | ||
|
|
||
| // The actual local address that the listener is listening on. If a listener was configured | ||
| // The actual local addresses that the listener is listening on. If a listener was configured | ||
| // to listen on port 0, then this address has the port that was allocated by the OS. | ||
| config.core.v3.Address local_address = 2; | ||
| repeated config.core.v3.Address local_addresses = 2; | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we have backward-compatibility rule for the admin API?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, this is a breaking change, and will not be approved. |
||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -38,7 +38,7 @@ message ListenerCollection { | |||||
| repeated xds.core.v3.CollectionEntry entries = 1; | ||||||
| } | ||||||
|
|
||||||
| // [#next-free-field: 33] | ||||||
| // [#next-free-field: 34] | ||||||
| message Listener { | ||||||
| option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.Listener"; | ||||||
|
|
||||||
|
|
@@ -107,8 +107,15 @@ message Listener { | |||||
| // that is governed by the bind rules of the OS. E.g., multiple listeners can listen on port 0 on | ||||||
| // Linux as the actual port will be allocated by the OS. | ||||||
| // Required unless *api_listener* or *listener_specifier* is populated. | ||||||
| // TODO (soulxu): deprecated the `address` field after `addresses` implemented. | ||||||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't mark the old field as
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suggest splitting this PR, to a few PRs.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks! got it. I will mark it as For other parts, also want to hear others' opinions if they are happy to separate the PR, since before people may want to see the full picture. I'm ok with both ways, if we want to separate the whole thing, I will work on it.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given that literally every envoy configuration uses these fields, we'll want to support both the old and the new forever. And I'd adopt the semantic that it's a config load error if both of them are set. So it logically turns into a As far as separating the PR or doing it all at once, I'm fine either way. I like seeing how the config will all be put together in one shot, but if the PR gets too big for that I'm fine splitting it. It may be worth defining the entire proto, but just not writing all the code yet, which should be fine if it's
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I tend to agree with this, but from previous experience, when a configuration server supports both "old" and "new" Envoy clients, and sends them the same config, then the "new" clients will reject it.
If possible, I think that it makes sense to break this to multiple smaller PRs (not only API vs non-API) to make it more reviewable. That said, if the PR includes mostly new tests, then it should be ok.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In this case, I think it could be as simple as if there is only one address, config server populates the old field. If there are multiple addresses, config server populates the new field. If config server doesn't know if the client supports the new field, it can do the old behavior of completely duplicate listeners (filter chains etc) except with a different address.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I also have thing about keeping the old
Let me see what the separate PR looks like. I found I can't change
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Just to clarify, breaking to multiple PRs is not a must, and in some cases it may make sense to have everything in one PR.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I should be able to separate the PR as the commits have in the PR (maybe merge a few of them together) https://github.com/envoyproxy/envoy/pull/19367/commits, I checked each of major commit has unittest covered. I will merge those cleanup and format change commits in the bottom to the top commits. The last PR should be for the integration tests. Let me know if you guys happy with that. |
||||||
| core.v3.Address address = 2; | ||||||
|
|
||||||
| // A list of addresses that the listener should listen on. In general, the address must be unique, though | ||||||
| // that is governed by the bind rules of the OS. E.g., multiple listeners can listen on port 0 on | ||||||
| // Linux as the actual port will be allocated by the OS. For multiple addresses in single listener, | ||||||
| // all addresses are the same protocol, and multiple internal addresses don't support now. | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| repeated core.v3.Address addresses = 33; | ||||||
|
|
||||||
| // Optional prefix to use on listener stats. If empty, the stats will be rooted at | ||||||
| // `listener.<address as string>.`. If non-empty, stats will be rooted at | ||||||
| // `listener.<stat_prefix>.`. | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this v2 should not be modified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea, I modify the wrong file and then forget to revert back.