admin: Add socket options#7562
Conversation
Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>
|
@mattklein123 I need some guidance how to test this change: |
mattklein123
left a comment
There was a problem hiding this comment.
Thanks, a few small nits.
For testing, we have various integration tests that start the admin server. Is there any socket option that you could set that we could verify somehow? Another thought, maybe you could actually configure reuse port and spawn 2 envoys and see that it works? https://github.com/envoyproxy/envoy/blob/master/test/integration/hotrestart_test.sh or some variant of that?
/wait
- use const reference for socket options argument - add test Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>
Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>
mattklein123
left a comment
There was a problem hiding this comment.
Nice, LGTM with a small nit.
/wait
include/envoy/server/admin.h
Outdated
| const std::string& address_out_path, | ||
| Network::Address::InstanceConstSharedPtr address, | ||
| Network::Socket::OptionsSharedPtr socketOptions, | ||
| const Network::Socket::OptionsSharedPtr& socketOptions, |
There was a problem hiding this comment.
nit: s/socketOptions/socket_options here and elsewhere.
Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>
Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>
Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>
|
Can you merge master? It should fix coverage. /wait |
Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>
|
Sorry I think CI is wedged. Can you push an empty commit or merge master again? /wait |
Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>
Description: Add ability to configure socket options for admin listener socket
Risk Level: LOW (opt-in change)
Testing: unit and integration test to ensure parsing and propagation of new protobuf field
Docs Changes: inline documentation
Release Notes: updated
Fixes #7524