-
Notifications
You must be signed in to change notification settings - Fork 918
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
feat(rpc): default binding to localhost vs 0.0.0.0 open to world #2955
Conversation
…efault bind address refs #2824 and add some tests
… defaults as localhost for bind/listen address
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
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2955 +/- ##
==========================================
+ Coverage 50.57% 50.93% +0.36%
==========================================
Files 168 176 +8
Lines 11022 11193 +171
==========================================
+ Hits 5574 5701 +127
- Misses 4942 4988 +46
+ Partials 506 504 -2 ☔ View full report in Codecov by Sentry. |
…estiaorg#2955) fixes celestiaorg#2824 which requests we bind to localhost by default (previously was world accessible) - sets defaults to bind to localhost properly (previously flags SAID the defaults were localhost but were actually 0.0.0.0) - removes an unused `DefaultConfig` defined in `api/gateway/package` - moves defaults (bind address and port) to vars so we can use them to set in flag usage messages so they don't drift out of sync with reality - adds tests for the defaults - adds test cases for flag definitions - adds test cases for flag parsing Other skips suggestion of binding vhosts, we don't do any kind of middleware checking in rpc (i believe in geth this would be a way to bind to 0.0.0.0 then limit to allowed remote IPs making requests). I'd suggest this is a more advanced configuration use case and belongs outside of the node itself to be managed by the infra operator where it'd be handled by a ingress gateway or routing rule to the machine/instance/orchestrator where the node software is running
I think this was actually a breaking change. If you run node in docker, you now need to add |
fixes #2824 which requests we bind to localhost by default (previously was world accessible)
DefaultConfig
defined inapi/gateway/package
Other
skips suggestion of binding vhosts, we don't do any kind of middleware checking in rpc (i believe in geth this would be a way to bind to 0.0.0.0 then limit to allowed remote IPs making requests). I'd suggest this is a more advanced configuration use case and belongs outside of the node itself to be managed by the infra operator where it'd be handled by a ingress gateway or routing rule to the machine/instance/orchestrator where the node software is running