Skip to content
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

Fix incorrect local and remote address when using domain socket in abstract namespace #5096

Merged
merged 1 commit into from Aug 4, 2023

Conversation

trustin
Copy link
Member

@trustin trustin commented Aug 2, 2023

Related issue: #5080

Motivation:

ServiceRequestContext.localAddress() and
ServiceRequestContext.remoteAddress() currently returns an
InetSocketAddress(0.0.0.0:1) for domain sockets in abstract namespace.

Modifications:

  • Updated ChannelUtil to use the parent channel's local address for
    server-side domain socket channels to retrieve their socket addresses
    reliably
  • Added a special hostname <unknown> to HttpServerHandler.UNKNOWN_ADDR
    so that it's less confusing

Result:

  • ServiceRequestContext now returns the correct socket addresses when
    the request is being served via a domain socket in abstract namespace.
  • It should be easier to understand the situation when a similar issue
    (unknown address) arises again.

…stract namespace

Related issue: line#5080

Motivation:

`ServiceRequestContext.localAddress()` and
`ServiceRequestContext.remoteAddress()` currently returns an
`InetSocketAddress(0.0.0.0:1)` for domain sockets in abstract namespace.

Modifications:

- Updated `ChannelUtil` to use the parent channel's local address for
  server-side domain socket channels to retrieve their socket addresses
  reliably
- Added a special hostname `<unknown>` to `HttpServerHandler.UNKNOWN_ADDR`
  so that it's less confusing

Result:

- `ServiceRequestContext` now returns the correct socket addresses when
  the request is being served via a domain socket in abstract namespace.
- It should be easier to understand the situation when a similar issue
  (unknown address) arises again.
@codecov
Copy link

codecov bot commented Aug 2, 2023

Codecov Report

Patch coverage: 50.00% and project coverage change: -0.01% ⚠️

Comparison is base (32568f9) 74.19% compared to head (a462145) 74.19%.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #5096      +/-   ##
============================================
- Coverage     74.19%   74.19%   -0.01%     
- Complexity    19469    19471       +2     
============================================
  Files          1672     1672              
  Lines         71749    71762      +13     
  Branches       9189     9189              
============================================
+ Hits          53237    53243       +6     
- Misses        14186    14193       +7     
  Partials       4326     4326              
Files Changed Coverage Δ
...com/linecorp/armeria/server/HttpServerHandler.java 79.20% <30.00%> (-1.62%) ⬇️
...corp/armeria/internal/common/util/ChannelUtil.java 85.49% <66.66%> (-1.12%) ⬇️

... and 21 files with indirect coverage changes

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

@ikhoon ikhoon added the defect label Aug 2, 2023
@ikhoon ikhoon added this to the 1.25.0 milestone Aug 2, 2023
Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this, @trustin! 👍 💯

Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fix @trustin ! 🙇 👍 🙇

@jrhee17 jrhee17 merged commit 491841f into line:main Aug 4, 2023
13 of 15 checks passed
@trustin trustin deleted the absract_namespace_address branch August 4, 2023 05:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants