api: add listener_address_restriction_hint in Node to hint known listening ports on the node by sanjaypujare · Pull Request #10370 · envoyproxy/envoy

sanjaypujare · 2020-03-12T23:15:16Z

Description: Added an optional field in Node as a generic hint to the management server about known listening ports on the node

Risk Level: Low
Testing: manually tested by compiling protos into generated Java code and using the code in a Java project
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Sanjay Pujare sanjaypujare@users.noreply.github.com

repokitteh-read-only · 2020-03-12T23:15:20Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #10370 was opened by sanjaypujare.

see: more, trace.

sanjaypujare · 2020-03-12T23:16:05Z

@htuch pls review

ejona86 · 2020-03-12T23:23:34Z

To be more clear about the use-case: This is for cases where the server has already decided which ports it will listen on and needs configuration for those ports.

sanjaypujare · 2020-03-13T04:29:39Z

Looked into the envoy-linux (format) failure:

-import "envoy/config/core/v3/base.proto";
+import "envoy/api/v3/core/socket_option.proto";

For some reason socket_option.proto was not properly placed in the v3 hierarchy. It should go into envoy/config/core/v3. I think I need to update some file to include socket_option.proto there.

alyssawilk · 2020-03-13T13:24:41Z

@htuch @lizan given this is API work one of you up for taking this?

markdroth · 2020-03-13T14:06:36Z

This was suggested by @htuch, so I assume he'll review.

sanjaypujare · 2020-03-16T19:14:43Z

@htuch Just FYI I did try to test the changes on my Mac but there are infra/tooling problems I am unable to debug. Pls let me know if the failures are related to my changes.

htuch

LGTM, but you need to run fix_format (to forward propagate into v3) and would be good to hear from at least one other @envoyproxy/api-shepherds
/wait

htuch · 2020-03-17T00:24:46Z

api/envoy/api/v2/core/base.proto

This PR looks good, just trying to see if we can maybe name this a bit more descriptively (I know I was responsible for the original suggestion :P)

How about listener_bound_addresses, listening_addresses, listener_available_addresses?

I'll pick listening_addresses as the shortest of them all.

sanjaypujare · 2020-03-17T04:05:34Z

LGTM, but you need to run fix_format (to forward propagate into v3) ...

So I ran

./ci/run_envoy_docker.sh './ci/do_ci.sh fix_format'

And that finished successfully and generated following changes:

	modified:   api/BUILD
	modified:   api/envoy/api/v2/core/base.proto
	modified:   api/envoy/api/v2/core/socket_option.proto
	modified:   api/envoy/api/v2/listener.proto
	modified:   api/envoy/config/bootstrap/v2/bootstrap.proto
	modified:   api/envoy/config/bootstrap/v3/BUILD
	modified:   api/envoy/config/bootstrap/v3/bootstrap.proto
	modified:   api/envoy/config/core/v3/BUILD
	modified:   api/envoy/config/core/v3/address.proto
	modified:   api/envoy/config/core/v3/base.proto
	modified:   api/envoy/config/listener/v3/BUILD
	modified:   api/envoy/config/listener/v3/listener.proto
	modified:   generated_api_shadow/envoy/api/v2/core/address.proto
	modified:   generated_api_shadow/envoy/api/v2/core/base.proto
	modified:   generated_api_shadow/envoy/api/v2/listener.proto
	modified:   generated_api_shadow/envoy/config/bootstrap/v2/bootstrap.proto
	modified:   generated_api_shadow/envoy/config/bootstrap/v3/BUILD
	modified:   generated_api_shadow/envoy/config/bootstrap/v3/bootstrap.proto
	modified:   generated_api_shadow/envoy/config/core/v3/BUILD
	modified:   generated_api_shadow/envoy/config/core/v3/address.proto
	modified:   generated_api_shadow/envoy/config/core/v3/base.proto
	modified:   generated_api_shadow/envoy/config/listener/v3/BUILD
	modified:   generated_api_shadow/envoy/config/listener/v3/listener.proto

Untracked files:
  (use "git add <file>..." to include in what will be committed)
	api/envoy/api/v3/
	generated_api_shadow/envoy/api/v2/core/socket_option.proto
	generated_api_shadow/envoy/api/v3/

Should I commit all these changes? And more importantly, why is socket_options under api/envoy/api/v3/core/ instead of api/envoy/config/core/v3 like other similar files?

htuch

@sanjaypujare good catch on the v3 file move:

Yes, please add all the new shadow files.
See my comment on how to ensure the new socket options file is in the right place.
/wait

htuch · 2020-03-17T18:34:46Z

api/envoy/api/v2/core/socket_option.proto

You'll need to add option (udpa.annotations.file_migrate).move_to_package = "envoy.config.core.v3"; (as done in other files in this package) for the correct file movement in v3.
`

mattklein123

LGTM at a high level as an alternate API reviewer. Thank you!

sanjaypujare · 2020-03-17T21:25:18Z

@sanjaypujare good catch on the v3 file move:

Yes, please add all the new shadow files.

See my comment on how to ensure the new socket options file is in the right place.
/wait

@htuch done. PTAL.

htuch · 2020-03-17T22:23:04Z

@sanjaypujare I think you ended up colliding with a known master breakage. Can you sync past #10426, merge master and try again?

You will also need to fix DCO: https://github.com/envoyproxy/envoy/blob/master/CONTRIBUTING.md#fixing-dco

…t about known listening ports on the node Signed-off-by: Sanjay Pujare <sanjaypujare@users.noreply.github.com>

Signed-off-by: Sanjay Pujare <sanjaypujare@users.noreply.github.com>

sanjaypujare · 2020-03-18T00:03:03Z

@sanjaypujare I think you ended up colliding with a known master breakage. Can you sync past #10426, merge master and try again?

You will also need to fix DCO: https://github.com/envoyproxy/envoy/blob/master/CONTRIBUTING.md#fixing-dco

Done both. PTAL.

htuch · 2020-03-18T15:22:10Z

@sanjaypujare see the docs warning:

checking consistency... /source/generated/rst/api-v3/config/core/v3/socket_option.proto.rst: WARNING: document isn't included in any toctree

I suggest adding to https://github.com/envoyproxy/envoy/blob/master/docs/root/api-v3/common_messages/common_messages.rst and https://github.com/envoyproxy/envoy/blob/master/docs/root/api-v2/common_messages/common_messages.rst as appropriate.

Signed-off-by: Sanjay Pujare <sanjaypujare@users.noreply.github.com>

sanjaypujare · 2020-03-18T15:49:54Z

@htuch v3 common_messages.rst updated (v2 was already done).

api/envoy/api/v2/core/base.proto

htuch

LGTM

sanjaypujare · 2020-03-19T15:21:49Z

LGTM

@htuch thanks! Once this is merged the downstream design/implementation activities will continue.

repokitteh-read-only bot added the api label Mar 12, 2020

mattklein123 assigned htuch Mar 15, 2020

htuch suggested changes Mar 17, 2020

View reviewed changes

repokitteh-read-only bot added waiting and removed waiting labels Mar 17, 2020

htuch suggested changes Mar 17, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Mar 17, 2020

mattklein123 reviewed Mar 17, 2020

View reviewed changes

repokitteh-read-only bot removed the waiting label Mar 17, 2020

sanjaypujare added 5 commits March 17, 2020 16:50

api: added listener_address_restriction_hint to Node as a generic hin…

15924da

…t about known listening ports on the node Signed-off-by: Sanjay Pujare <sanjaypujare@users.noreply.github.com>

add socket_option.proto to common_messages file

1ec5688

Signed-off-by: Sanjay Pujare <sanjaypujare@users.noreply.github.com>

remove socket_option.proto from v3 common_messages file

ff27b46

Signed-off-by: Sanjay Pujare <sanjaypujare@users.noreply.github.com>

rename the new field to listening_addresses

2eb5116

Signed-off-by: Sanjay Pujare <sanjaypujare@users.noreply.github.com>

check in file generated by fix_format

15ff0cc

Signed-off-by: Sanjay Pujare <sanjaypujare@users.noreply.github.com>

sanjaypujare force-pushed the listener_address_restriction1 branch from af49960 to 15ff0cc Compare March 17, 2020 23:59

add socket_option.proto to v3 common_messages.rst

c4383a7

Signed-off-by: Sanjay Pujare <sanjaypujare@users.noreply.github.com>

lizan reviewed Mar 18, 2020

View reviewed changes

api/envoy/api/v2/core/base.proto Show resolved Hide resolved

htuch approved these changes Mar 18, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Mar 18, 2020

mattklein123 merged commit 404d73c into envoyproxy:master Mar 19, 2020

Conversation

sanjaypujare commented Mar 12, 2020

Uh oh!

repokitteh-read-only bot commented Mar 12, 2020

Uh oh!

sanjaypujare commented Mar 12, 2020

Uh oh!

ejona86 commented Mar 12, 2020

Uh oh!

sanjaypujare commented Mar 13, 2020

Uh oh!

alyssawilk commented Mar 13, 2020

Uh oh!

markdroth commented Mar 13, 2020

Uh oh!

sanjaypujare commented Mar 16, 2020

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

htuch Mar 17, 2020

Choose a reason for hiding this comment

Uh oh!

sanjaypujare Mar 17, 2020

Choose a reason for hiding this comment

Uh oh!

sanjaypujare commented Mar 17, 2020

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

htuch Mar 17, 2020

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

sanjaypujare commented Mar 17, 2020

Uh oh!

htuch commented Mar 17, 2020

Uh oh!

sanjaypujare commented Mar 18, 2020

Uh oh!

htuch commented Mar 18, 2020

Uh oh!

sanjaypujare commented Mar 18, 2020

Uh oh!

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

sanjaypujare commented Mar 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants