Allow listening for go debug on all interfaces, not just localhost #1088
Conversation
matthew1001
requested review from
peterbroadhurst,
nguyer,
awrichar and
shorsher
as code owners
internal/coreconfig/coreconfig.go
Outdated
| @@ -359,6 +361,7 @@ func setDefaults() { | |||
| viper.SetDefault(string(CacheOperationsTTL), "5m") | |||
| viper.SetDefault(string(HistogramsMaxChartRows), 100) | |||
| viper.SetDefault(string(DebugPort), -1) | |||
| viper.SetDefault(string(DebugAddress), "") | |||
There was a problem hiding this comment.
The suggest the defaults stays as localhost, as exposing the debug server externally is not something I believe users would want to do outside of a dockerized/k8s environment where they have carefully considered the security implications.
A follow-up PR with firefly-cli to have a flag to allocate ports to the debug ports of various services (ethconnect/evmconnect/core etc.) might be valuable for dev environments.
There was a problem hiding this comment.
Yeah I almost left the default as localhost - I'll revert it back. Happy to look at the firefly-cli/firefly-* PRs after this one.
| @@ -209,6 +209,8 @@ var ( | |||
| PluginsIdentityList = ffc("plugins.identity") | |||
| // DebugPort a HTTP port on which to enable the go debugger | |||
| DebugPort = ffc("debug.port") | |||
| // DebugAddress the HTTP interface for the debugger to listen on | |||
| DebugAddress = ffc("debug.address") | |||
There was a problem hiding this comment.
Think you'll need to add a documentation entry for this one too.
There was a problem hiding this comment.
Good point, let me do that
There was a problem hiding this comment.
Here's where to add the config doc:
Then make reference before you check in.
Signed-off-by: Matthew Whitehead <matthew.whitehead@kaleido.io>
… config test Signed-off-by: Matthew Whitehead <matthew.whitehead@kaleido.io>
|
Both comments should be addressed in the new commit. I notice that we have a config test so I updated that, although it feels like it's of limited use as it's just testing the defaults haven't been changed. Didn't seem to do any harm to add a couple of tests to it though. |
peterbroadhurst
changed the title
| @@ -29,4 +29,6 @@ func TestInitConfigOK(t *testing.T) { | |||
| Reset() | |||
|
|
|||
| assert.Equal(t, 25, config.GetInt(APIDefaultFilterLimit)) | |||
| assert.Equal(t, "localhost", config.GetString(DebugAddress)) | |||
There was a problem hiding this comment.
Yeah, this test is for the config scaffolding code, rather than for individual fields - but all good 👍
It's difficult using the go pprof debug utility when running in a container since the network interface it binds to defaults to localhost. This PR makes 2 changes:
It still requires changes to the docker run command or docker compose overrides file to get a route through to the debug listener, but doesn't require you to rebuild firefly.