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
Adds listen-address
CLI flags for all services servers.
#7675
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7675 +/- ##
==========================================
+ Coverage 61.39% 61.84% +0.45%
==========================================
Files 265 245 -20
Lines 22609 22449 -160
==========================================
+ Hits 13880 13884 +4
+ Misses 7579 7401 -178
- Partials 1150 1164 +14 ☔ View full report in Codecov by Sentry. |
cmd/daprd/options/options.go
Outdated
fs.StringVar(&opts.DaprAPIGRPCPort, "dapr-grpc-port", strconv.Itoa(runtime.DefaultDaprAPIGRPCPort), "gRPC port for the Dapr API to listen on") | ||
fs.StringVar(&opts.DaprInternalGRPCPort, "dapr-internal-grpc-port", "", "gRPC port for the Dapr Internal API to listen on") | ||
fs.StringVar(&opts.DaprInternalGRPCListenAddress, "dapr-internal-grpc-listen-address", "0.0.0.0", "gRPC listen address for the Dapr Internal API") |
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.
fs.StringVar(&opts.DaprInternalGRPCListenAddress, "dapr-internal-grpc-listen-address", "0.0.0.0", "gRPC listen address for the Dapr Internal API") | |
fs.StringVar(&opts.DaprInternalGRPCAddress, "dapr-internal-grpc-address", "0.0.0.0", "gRPC listen address for the Dapr Internal API") |
@@ -102,6 +102,7 @@ func Run() { | |||
|
|||
inj, err := service.NewInjector(service.Options{ | |||
Port: opts.Port, | |||
ListenAddress: opts.ListenAddress, |
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.
Same as above, do not repeat below.
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 have one important question about backwards compatibility.
@@ -174,7 +174,7 @@ func (s *server) StartNonBlocking() error { | |||
s.setupRoutes(publicR, s.api.PublicEndpoints()) | |||
|
|||
healthServer := &http.Server{ | |||
Addr: fmt.Sprintf(":%d", *s.config.PublicPort), | |||
Addr: fmt.Sprintf("%s:%d", s.config.PublicListenAddress, *s.config.PublicPort), |
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.
The old code was omitting the listening address while the new code is sending "0.0.0.0" instead. Does it mean that we now will only listen to IPv4 addresses while before it would listen also on IPv6? Can this be validated? I am asking because maybe the default value should be empty string to keep backwards compatibility.
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.
Thanks @artursouza, good catch! I've updated the default listen addresses CLI flags to be empty strings which will be the same value as before. PTAL.
Address corresponding `--listen-address` CLI flags for all services servers. This sets the listening address for these TCP servers. This is added to allow users to both bind the address to localhost to prevent access to a particular server on the network, as well as for integration testing as it is more correct to bind to localhost and prevents triggering MacOS firewall popups. Signed-off-by: joshvanl <[email protected]>
localhost Signed-off-by: joshvanl <[email protected]>
localhost Signed-off-by: joshvanl <[email protected]>
for default ipv6 support and keep backwards compat. Signed-off-by: joshvanl <[email protected]>
d6ea4da
to
522eb32
Compare
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.
LGTM
* Adds `listen-address` CLI flags for all services servers. Address corresponding `--listen-address` CLI flags for all services servers. This sets the listening address for these TCP servers. This is added to allow users to both bind the address to localhost to prevent access to a particular server on the network, as well as for integration testing as it is more correct to bind to localhost and prevents triggering MacOS firewall popups. Signed-off-by: joshvanl <[email protected]> * Report localhost to nameresolver when internal grpc listen address is localhost Signed-off-by: joshvanl <[email protected]> * Use localhost host for placement report when internal gRPC address is localhost Signed-off-by: joshvanl <[email protected]> * Change default listen-address from `0.0.0.0` to empty string to allow for default ipv6 support and keep backwards compat. Signed-off-by: joshvanl <[email protected]> --------- Signed-off-by: joshvanl <[email protected]> Co-authored-by: Dapr Bot <[email protected]>
Address corresponding
--listen-address
CLI flags for all services servers. This sets the listening address for these TCP servers.This is added to allow users to both bind the address to localhost to prevent access to a particular server on the network, as well as for integration testing as it is more correct to bind to localhost and prevents triggering MacOS firewall popups.