Overwrite source address with logging #272
Conversation
WalkthroughThe changes involve a significant update to the HTTP server configuration handling. The Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant Proxy
participant HTTPServer
participant Config
User->>Proxy: HTTP Request
Proxy->>HTTPServer: Forward Request with Address Header
HTTPServer->>Config: Load Configuration
Config-->>HTTPServer: Provide Config Object
HTTPServer->>HTTPServer: Overwrite Remote Address
HTTPServer->>User: Send Response
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
config/config.go (1)
107-108: The new command-line flags forRateLimitandAddressHeaderare correctly implemented. Ensure they are documented in the user guide.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- api/server.go (4 hunks)
- bootstrap/bootstrap.go (2 hunks)
- config/config.go (2 hunks)
Additional comments not posted (2)
bootstrap/bootstrap.go (1)
203-203: Ensure all calls toNewHTTPServerhave been updated to pass the newcfgparameter.Verification successful
The search results indicate that the
NewHTTPServerfunction is defined inapi/server.goand called inbootstrap/bootstrap.go. Let's verify the function definition inapi/server.goto ensure it matches the new signature and confirm there are no other calls toNewHTTPServerthat might have been missed.
The verification confirms that the
NewHTTPServerfunction inapi/server.gohas been updated to include thecfgparameter and that there are no other calls toNewHTTPServerthat might have been missed.
api/server.go:59: The function definition matches the new signature withcfgas a parameter.bootstrap/bootstrap.go:203: The call toNewHTTPServercorrectly passes thecfgparameter.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `NewHTTPServer` match the new signature. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type go $'NewHTTPServer'Length of output: 175
Script:
#!/bin/bash # Verify the function definition of `NewHTTPServer` in `api/server.go` ast-grep --lang go --pattern $'func NewHTTPServer(logger $_, cfg $_) $_' api/server.go # Confirm there are no other calls to `NewHTTPServer` that might have been missed rg --type go $'NewHTTPServer'Length of output: 1075
api/server.go (1)
52-52: The addition of theconfigfield to thehttpServerstruct is a crucial change for enhancing configurability.
| // RateLimit requests made by the client identified by IP over any protocol (ws/http). | ||
| RateLimit uint64 | ||
| // Address header used to identified clients, usually set by the proxy | ||
| AddressHeader string |
There was a problem hiding this comment.
Ensure that the new configuration fields RateLimit and AddressHeader are validated to prevent misconfigurations.
// Add validation for RateLimit and AddressHeader
if cfg.RateLimit < 0 {
return nil, fmt.Errorf("RateLimit cannot be negative")
}
if cfg.AddressHeader != "" && !strings.Contains(cfg.AddressHeader, "X-") {
return nil, fmt.Errorf("AddressHeader should start with 'X-'")
}| // this overwrites the remote address with the header value, this is used when the server is | ||
| // behind a proxy, and the true source address is overwritten by proxy, but retained in a header. | ||
| if h.config.AddressHeader != "" { | ||
| r.RemoteAddr = r.Header.Get(h.config.AddressHeader) | ||
| } |
There was a problem hiding this comment.
The implementation for overwriting the remote address using the AddressHeader is correct. Consider adding a unit test to ensure this behavior works as expected when the server is behind a proxy.
Would you like me to help by writing a unit test for this functionality?
devbugging
changed the title
| // RateLimit requests made by the client identified by IP over any protocol (ws/http). | ||
| RateLimit uint64 |
There was a problem hiding this comment.
Is this supposed to be in this PR?
nothing else mentions it
There was a problem hiding this comment.
not really, it's needed in the follow up PR #267
Which I will get in as soon as I validate this one.
Description
Allow defining header value used for determining the client source IP. This is useful when the EVM GW is behind a proxy that sets the source IP in a header.
Also add this source IP to logs so we can validate how this behaves on a deployment.
For contributor use:
masterbranchFiles changedin the Github PR explorer