Skip to content

network: fixes to public address support#5851

Merged
algorandskiy merged 6 commits intoalgorand:masterfrom
algorandskiy:pavel/proxyfix
Dec 7, 2023
Merged

network: fixes to public address support#5851
algorandskiy merged 6 commits intoalgorand:masterfrom
algorandskiy:pavel/proxyfix

Conversation

@algorandskiy
Copy link
Copy Markdown
Contributor

@algorandskiy algorandskiy commented Dec 2, 2023

Summary

  1. requestTracker changes:
  • Remove http.Request.RemoteAddr overwriting in request tracker
  • Remove http.Request from request tracker
  • Add a new remoteAddresss() method that returns a most suitable address
  1. public addr changes:
  • no otherPublicAddr outside of requestTracker
  • new remoteAddresss() method provides most meaningful address for incoming requests

Rationale:
There is a chain http.Handler's in wsNetwork: request tracker and wsNetwork itself.
Tracked request is created/updated in the first ServeHTTP and used to save a pointer to http.Request object.
This request object is actually gets copied in mux.ServeHTTP that calls downstream wsNetwork's ServeHTTP making
the request non-usable in the main wsNetwork's ServeHTTP so removed.

otherPublicAddr is an address reported by a peer via our custom X-Algorand-Location header that the only
correct value for wsPeer.rootURL (host:port accepting connections) but cannot be trusted.
Unconditionally rewriting it with remoteAddr obtained from a tcp connection is too much and a new
remoteAddresss() method attempts to provide the most meaningful trusted value.

Fixes #5713

Test Plan

  1. Existing test must pass
  2. Added a new unit test
  3. Updated some connection dedup TestPeering tests since they relied on a buggy otherPublicAddr overwriting by remoteAddr.

Comment thread network/requestTracker.go Outdated
@codecov
Copy link
Copy Markdown

codecov Bot commented Dec 2, 2023

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (f48be99) 55.95% compared to head (793f8ca) 55.92%.
Report is 16 commits behind head on master.

Files Patch % Lines
network/requestTracker.go 73.68% 3 Missing and 2 partials ⚠️
network/wsNetwork.go 54.54% 4 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5851      +/-   ##
==========================================
- Coverage   55.95%   55.92%   -0.03%     
==========================================
  Files         477      477              
  Lines       67346    67355       +9     
==========================================
- Hits        37683    37671      -12     
- Misses      27117    27131      +14     
- Partials     2546     2553       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@algorandskiy algorandskiy marked this pull request as draft December 4, 2023 15:23
1. requestTracker changes:
  - Remove http.Request.RemoteAddr overwriting in request tracker
  - Remove http.Request from request tracker
  - Add a new remoteAddresss() method that returns a most suitable address

2. public addr changes:
  - no otherPublicAddr outside of requestTracker
  - new remoteAddresss() method provides most meaningful address for incoming requests

Rationale:
There is a chain http.Handler's in wsNetwork: request tracker and wsNetwork itself.
Tracked request is created/updated in the first ServeHTTP and used to save a pointer to http.Request object.
This request object is actually gets copied in mux.ServeHTTP that calls downstream wsNetwork's ServeHTTP making
the request non-usable in the main wsNetwork's ServeHTTP so removed.

otherPublicAddr is an address reported by a peer via our custom X-Algorand-Location header that the only
correct value for wsPeer.rootURL (host:port accepting connections) but cannot be trusted.
Unconditionally rewriting it with remoteAddr obtained from a tcp connection is too much and a new
remoteAddresss() method attempts to provide the most meaningful trusted value.

Fixes algorand#5713
Comment thread network/requestTracker.go Outdated
Comment thread network/requestTracker.go
Comment thread network/requestTracker.go Outdated
Comment thread network/wsNetwork.go
Comment thread network/wsNetwork.go
Comment thread network/wsNetwork_test.go
Comment thread network/wsNetwork_test.go Outdated
Comment thread network/requestTracker.go Outdated
Co-authored-by: Gary <982483+gmalouf@users.noreply.github.com>
Comment thread network/wsNetwork_test.go Outdated
Comment thread network/wsNetwork_test.go
@algorandskiy algorandskiy requested a review from gmalouf December 6, 2023 22:09
jasonpaulos
jasonpaulos previously approved these changes Dec 7, 2023
Copy link
Copy Markdown
Contributor

@jasonpaulos jasonpaulos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems better. Just putting in some minor corrections

Comment thread network/requestTracker_test.go Outdated
Comment thread network/requestTracker.go Outdated
Co-authored-by: Jason Paulos <jasonpaulos@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Incorrect Remote Address in wsNetwork logs when algod behind proxy

3 participants