Add Unix domain socket support #4846

trustin · 2023-04-24T16:22:51Z

Motivation:

In service mesh environment, it's quite common for a sidecar to
communicate with an app using a Unix domain socket. Armeria could be
used more efficiently than using TCP/IP in such a scenarios with
domain socket support.

Modifications:

Updated the various places to support domain sockets, as listed
below
Common changes:
- Added DomainSocketAddress
  - DomainSocketAddress is a subtype of InetSocketAddress for
    backward compatibility with the existing API that returns
    InetSocketAddress rather than SocketAddress.
- (Breaking) Changed the return type of RequestContext.localAddress()
  and remoteAddress() from SocketAddress and InetSocketAddress
  because DomainSocketAddress is now an InetSocketAddress.
- (Breaking) The parameter type of the following methods has been
  changed from SocketAddress to InetSocketAddress:
  - ClientRequestContextBuilder.localAddress()
  - ClientRequestContextBuilder.remoteAddress()
  - ServiceRequestContextBuilder.localAddress()
  - ClientRequestContextBuilder.remoteAddress()
- Added various getters to TransportType and TransportTypeProvider
  so it provides the transport-specific information about domain socket
  classes and whether the transport supports domain sockets
- Added ChannelUtil.localAddress() and remoteAddress() and replace
  the direct invocation of Channel.localAddress() and
  remoteAddress() to convert Netty's DomainSocketAddress into ours.
  - Updated Netty to 4.1.92 that contains the fix for the problem where
    DomainSocketChannel returns null addresses.
- Added ChannelUtil.isTcpOption() to determine whether the given
  ChannelOption is for TCP or not, so that a user doesn't get a WARN
  log when they use domain sockets
Server-side changes:
- Added ServerPort.isDomainSocket()
- Updated Server and HttpServerHandler to support domain sockets.
- Made ServerRule and ServerExtension not instantiate their default
  WebClients lazily, so that a user can start a serer that listens
  only on a domain socket
Client-side changes:
- Added domain socket address support to Endpoint:
  - Made Endpoint accept a domain socket address in its host field
  - Added Endpoint.of(SocketAddress) so that a user can easily create
    an Endpoint from an InetSocketAddress or a DomainSocketAddress
  - Added Endpoint.isDomainSocket()
- Updated HttpClientFactory, HttpChannelPool, HttpClientDelegate,
  HttpClientPipelineConfigurator and SessionProtocolNegotiationCache
  to support domain sockets

Result:

Armeria is now capable of serving/sending requests from/to a Unix domain
socket, making it more service-mesh-friendly.
(Breaking) Changed the return type of RequestContext.localAddress()
and remoteAddress() from SocketAddress and InetSocketAddress
because DomainSocketAddress is now an InetSocketAddress.
(Breaking) The parameter type of the following methods has been
changed from SocketAddress to InetSocketAddress:
- ClientRequestContextBuilder.localAddress()
- ClientRequestContextBuilder.remoteAddress()
- ServiceRequestContextBuilder.localAddress()
- ClientRequestContextBuilder.remoteAddress()

trustin · 2023-04-24T16:47:44Z

The memory leak is very likely from DomainSocketServerTest which I forgot to release the received buffers. Let me fix it later this week.

trustin · 2023-05-02T13:36:54Z

core/src/main/java/com/linecorp/armeria/client/DefaultWebClient.java

-                    req, new IllegalArgumentException("Invalid path: " + originalPath));
+                    req, new IllegalArgumentException("Invalid request target: " + originalPath));


This isn't always a path but can be an absolute URI, so I changed it to 'request target'.

trustin · 2023-05-02T13:40:05Z

core/src/main/java/com/linecorp/armeria/client/HttpClientPipelineConfigurator.java

    void addBeforeSessionHandler(ChannelPipeline pipeline, ChannelHandler handler) {
-        // Get the name of the HttpSessionHandler so that we can put our handlers before it.
        final ChannelHandlerContext lastContext = pipeline.lastContext();
-        assert lastContext.handler().getClass() == HttpSessionHandler.class;
-
-        pipeline.addBefore(lastContext.name(), null, handler);
+        if (lastContext.handler().getClass() == HttpSessionHandler.class) {
+            // Get the name of the HttpSessionHandler so that we can put our handlers before it.
+            pipeline.addBefore(lastContext.name(), null, handler);
+        } else {
+            // HttpSessionHandler was not added yet.
+            pipeline.addLast(handler);
+        }
    }


This one is a little bit tricky because a connect() operation in domain sockets succeeds immediately in Linux and thus this method can be invoked even before HttpSessionHandler is added.

minwoox

Looks good all in all. 👍

core/src/main/java/com/linecorp/armeria/client/HttpChannelPool.java

core/src/main/java/com/linecorp/armeria/client/HttpClientFactory.java

core/src/main/java/com/linecorp/armeria/client/HttpClientPipelineConfigurator.java

minwoox · 2023-05-08T03:30:44Z

core/src/main/java/com/linecorp/armeria/common/util/DomainSocketAddress.java

+    // Use an IPv6 address that falls into RFC 6666 (IPv6 Discard Prefix),
+    // which will never reach anywhere. See https://datatracker.ietf.org/doc/rfc6666/
+    private static final byte[] IPV6_DISCARD_ADDR = {
+            1, 0, 0, 0, 0, 0, 0, 0, // 0100::/64


Question: Isn't it 0, 0, 0, 1, 0, 0, 0, 0?

I guess not:

jshell> var addr = InetAddress.getByAddress(new byte[] { 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}) addr ==> /100:0:0:0:0:0:0:0 jshell> var addr = InetAddress.getByAddress(new byte[] { 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}) addr ==> /0:1:0:0:0:0:0:0

Thanks for the explanation. 🙇

core/src/main/java/com/linecorp/armeria/common/util/TransportType.java

core/src/main/java/com/linecorp/armeria/internal/common/ArmeriaHttpUtil.java

ikhoon

Great! I learned a lot from this work.

core/src/main/java/com/linecorp/armeria/client/Endpoint.java

core/src/test/java/com/linecorp/armeria/client/DomainSocketClientTest.java

core/src/main/java/com/linecorp/armeria/client/HttpChannelPool.java

core/src/main/java/com/linecorp/armeria/client/SessionProtocolNegotiationCache.java

core/src/main/java/com/linecorp/armeria/internal/client/DefaultClientRequestContext.java

core/src/main/java/com/linecorp/armeria/server/HttpServerHandler.java

Motivation: In service mesh environment, it's quite common for a sidecar to communicate with an app using a Unix domain socket. Armeria could be used in such a scenarios if it is capable of serving requests on a Unix domain socket. Modifications: - Added `DomainSocketAddress` - Added `ServerPort.isDomainSocket()` - Added various getters to `TransportType` and `TransportTypeProvider` so it provides the transport-specific information about domain socket classes and whether the transport supports domain sockets - Added `ChannelUtil.localAddress()` and `remoteAddress()` and replace the direct invocation of `Channel.localAddress()` and `remoteAddress()` to work around the issue where Netty's domain socket channel returns `null` - See: netty/netty#13323 - Added `ChannelUtil.isTcpOption()` to determine whether the given `ChannelOption` is for TCP or not, so that a user doesn't get a WARN log when they use domain sockets - Made `ServerRule` and `ServerExtension`not instantiate their default `WebClient`s lazily, so that a user can start a serer that listens only on a domain socket Result: - Armeria is now capable of serving requests from a Unix domain socket, making it more service-mesh-friendly. Fix leaks Update Netty to 4.1.92 Cache remote/localAddress / SocketAddress -> InetSocketAddress Add client-side domain socket support Lint Windows Formatting Windows / Lint Review comments Remove an unused field

ikhoon

Thanks, @trustin! 💯 👍

ikhoon · 2023-05-22T15:06:52Z

core/build.gradle

+    }
+    ['osx-x86_64', 'osx-aarch_64'].each { arch ->
+        implementation(variantOf(libs.netty.transport.native.unix.common) { classifier(arch) })
+        implementation(variantOf(libs.netty.transport.native.kqueue) { classifier(arch) })


🙇‍♂️

core/src/main/java/com/linecorp/armeria/client/Endpoint.java

core/src/main/java/com/linecorp/armeria/client/HttpChannelPool.java

core/src/test/java/com/linecorp/armeria/client/SessionProtocolNegotiationCacheTest.java

@ikhoon

… the comments from @ikhoon

ikhoon

Awesome feature! 🚀 💡

trustin · 2023-05-30T15:19:09Z

I see the following test failures on my Mac:

FileWatcherRegistryTest. runnableWithExceptionContinuesRun
PropertiesEndpointGroupTest. endpointChangePropagatesToListeners
PropertiesEndpointGroupTest. propertiesFileRestart
PropertiesEndpointGroupTest. propertiesFileUpdatesCorrectly

Do you get the same failures, @ikhoon? They look irrelevant to this pull request 🤔

trustin · 2023-05-30T16:14:33Z

I see the following test failures on my Mac

This should fix them: #4903

trustin · 2023-05-30T16:58:33Z

I also see some flaky tests, which may or may not be related to this PR:

HttpServerKeepAliveHandlerTest
PortUnificationServerTest
ServerMaxConnectionAgeTest

…tPort(SessionProtocol)`

jrhee17

Left a minor suggestion but looks good to me 👍 Thanks @trustin ! 🙇 👍 🙇

core/src/main/java/com/linecorp/armeria/client/HttpChannelPool.java

minwoox

Still LGTM. 👍

core/src/main/java/com/linecorp/armeria/client/Endpoint.java

Co-authored-by: minux <songmw725@gmail.com>

core/src/main/java/com/linecorp/armeria/client/Endpoint.java

core/src/main/java/com/linecorp/armeria/server/Server.java

Co-authored-by: Ikhun Um <ih.pert@gmail.com>

….java Co-authored-by: jrhee17 <guins_j@guins.org>

trustin · 2023-06-07T10:28:00Z

Addressed all comments 😆

ikhoon · 2023-06-07T10:30:12Z

Looks good with the new changes.

trustin · 2023-06-08T06:58:57Z

Thanks for reviewing this large PR, folks! 🙇

Motivation: In service mesh environment, it's quite common for a sidecar to communicate with an app using a Unix domain socket. Armeria could be used more efficiently than using TCP/IP in such a scenarios with domain socket support. Modifications: - Updated the various places to support domain sockets, as listed below - Common changes: - Added `DomainSocketAddress` - `DomainSocketAddress` is a subtype of `InetSocketAddress` for backward compatibility with the existing API that returns `InetSocketAddress` rather than `SocketAddress`. - (Breaking) Changed the return type of `RequestContext.localAddress()` and `remoteAddress()` from `SocketAddress` and `InetSocketAddress` because `DomainSocketAddress` is now an `InetSocketAddress`. - (Breaking) The parameter type of the following methods has been changed from `SocketAddress` to `InetSocketAddress`: - `ClientRequestContextBuilder.localAddress()` - `ClientRequestContextBuilder.remoteAddress()` - `ServiceRequestContextBuilder.localAddress()` - `ClientRequestContextBuilder.remoteAddress()` - Added various getters to `TransportType` and `TransportTypeProvider` so it provides the transport-specific information about domain socket classes and whether the transport supports domain sockets - Added `ChannelUtil.localAddress()` and `remoteAddress()` and replace the direct invocation of `Channel.localAddress()` and `remoteAddress()` to convert Netty's `DomainSocketAddress` into ours. - Updated Netty to 4.1.92 that contains the fix for the problem where `DomainSocketChannel` returns `null` addresses. - Added `ChannelUtil.isTcpOption()` to determine whether the given `ChannelOption` is for TCP or not, so that a user doesn't get a WARN log when they use domain sockets - Server-side changes: - Added `ServerPort.isDomainSocket()` - Updated `Server` and `HttpServerHandler` to support domain sockets. - Made `ServerRule` and `ServerExtension` not instantiate their default `WebClient`s lazily, so that a user can start a serer that listens only on a domain socket - Client-side changes: - Added domain socket address support to `Endpoint`: - Made `Endpoint` accept a domain socket address in its `host` field - Added `Endpoint.of(SocketAddress)` so that a user can easily create an `Endpoint` from an `InetSocketAddress` or a `DomainSocketAddress` - Added `Endpoint.isDomainSocket()` - Updated `HttpClientFactory`, `HttpChannelPool`, `HttpClientDelegate`, `HttpClientPipelineConfigurator` and `SessionProtocolNegotiationCache` to support domain sockets Result: - Armeria is now capable of serving/sending requests from/to a Unix domain socket, making it more service-mesh-friendly. - (Breaking) Changed the return type of `RequestContext.localAddress()` and `remoteAddress()` from `SocketAddress` and `InetSocketAddress` because `DomainSocketAddress` is now an `InetSocketAddress`. - (Breaking) The parameter type of the following methods has been changed from `SocketAddress` to `InetSocketAddress`: - `ClientRequestContextBuilder.localAddress()` - `ClientRequestContextBuilder.remoteAddress()` - `ServiceRequestContextBuilder.localAddress()` - `ClientRequestContextBuilder.remoteAddress()` --------- Co-authored-by: jrhee17 <guins_j@guins.org> Co-authored-by: Ikhun Um <ih.pert@gmail.com> Co-authored-by: minux <songmw725@gmail.com>

trustin requested review from ikhoon, jrhee17 and minwoox as code owners April 24, 2023 16:22

trustin added the new feature label Apr 24, 2023

trustin added this to the 1.24.0 milestone Apr 24, 2023

trustin added breaking change dependencies labels Apr 26, 2023

trustin changed the title ~~Add server-side Unix domain socket support~~ Add Unix domain socket support May 2, 2023

trustin commented May 2, 2023

View reviewed changes

minwoox reviewed May 8, 2023

View reviewed changes

trustin requested a review from minwoox May 8, 2023 11:28

ikhoon reviewed May 9, 2023

View reviewed changes

trustin force-pushed the uds branch 2 times, most recently from 41c1ce6 to e65ad13 Compare May 19, 2023 03:38

trustin force-pushed the uds branch from 780844b to 1d3f630 Compare May 20, 2023 09:45

trustin added 2 commits May 20, 2023 20:13

Address the comments from @ikhoon

41ee453

Domain socket support on macOS

999eca6

trustin requested a review from ikhoon May 22, 2023 02:59

trustin added 2 commits May 22, 2023 12:15

Lint

d473068

Add Endpoint.toSocketAddress() and DomainSocketAddress.asEndpoint()

6ea5bee

ikhoon approved these changes May 22, 2023

View reviewed changes

trustin force-pushed the uds branch from 2b9c844 to ad3e99d Compare May 23, 2023 13:10

Make Endpoint cache InetSocketAddress more aggressively / Address…

20c6709

… the comments from @ikhoon

trustin force-pushed the uds branch from ad3e99d to 20c6709 Compare May 23, 2023 13:16

Simplify SessionProtocolNegotiationCache key generation

a5aa7fe

trustin requested a review from ikhoon May 23, 2023 14:11

ikhoon approved these changes May 24, 2023

View reviewed changes

trustin mentioned this pull request May 30, 2023

Assertion failure in HttpSessionHandler.isAcquirable() #4902

Closed

Address the comment from @ikhoon / Add `Endpoint.{with,without}Defaul…

92ebac1

…tPort(SessionProtocol)`

jrhee17 approved these changes Jun 1, 2023

View reviewed changes

core/src/main/java/com/linecorp/armeria/client/HttpChannelPool.java Outdated Show resolved Hide resolved

minwoox approved these changes Jun 2, 2023

View reviewed changes

core/src/main/java/com/linecorp/armeria/client/Endpoint.java Outdated Show resolved Hide resolved

ikhoon and others added 3 commits June 3, 2023 17:23

Update core/src/main/java/com/linecorp/armeria/client/Endpoint.java

54aee42

Co-authored-by: minux <songmw725@gmail.com>

Merge branch 'main' into pr-4846-trustin-uds

324d470

resolve conflicts

37cbac2

ikhoon reviewed Jun 3, 2023

View reviewed changes

ikhoon and others added 9 commits June 3, 2023 19:04

Remove an unused import

ee13b32

Merge branch 'main' into uds

80e7329

Merge branch 'main' into uds

a4bcd25

Update core/src/main/java/com/linecorp/armeria/client/Endpoint.java

5939891

Co-authored-by: Ikhun Um <ih.pert@gmail.com>

Update core/src/main/java/com/linecorp/armeria/client/Endpoint.java

22f637d

Co-authored-by: Ikhun Um <ih.pert@gmail.com>

Update core/src/main/java/com/linecorp/armeria/client/Endpoint.java

0e7bc01

Co-authored-by: Ikhun Um <ih.pert@gmail.com>

Add unix: prefix to the boss thread names

fb84c46

Update core/src/main/java/com/linecorp/armeria/client/HttpChannelPool…

7834c7d

….java Co-authored-by: jrhee17 <guins_j@guins.org>

Address the comment by @jrhee17

ac354c1

trustin added 3 commits June 8, 2023 01:18

Lint

1ffd2ec

Oops

168e71c

Lint

4de1794

trustin merged commit 9a8c0a0 into line:main Jun 8, 2023

trustin deleted the uds branch June 8, 2023 06:58

ikhoon mentioned this pull request Jun 12, 2023

UNIX domain socket support #1837

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Unix domain socket support #4846

Add Unix domain socket support #4846

trustin commented Apr 24, 2023 •

edited

Loading

trustin commented Apr 24, 2023 •

edited

Loading

trustin May 2, 2023

trustin May 2, 2023

minwoox left a comment

minwoox May 8, 2023

trustin May 8, 2023

minwoox May 8, 2023

ikhoon left a comment

ikhoon left a comment

ikhoon May 22, 2023

ikhoon left a comment

trustin commented May 30, 2023

trustin commented May 30, 2023

trustin commented May 30, 2023

jrhee17 left a comment

minwoox left a comment

trustin commented Jun 7, 2023

ikhoon commented Jun 7, 2023

trustin commented Jun 8, 2023

		req, new IllegalArgumentException("Invalid path: " + originalPath));
		req, new IllegalArgumentException("Invalid request target: " + originalPath));

Add Unix domain socket support #4846

Add Unix domain socket support #4846

Conversation

trustin commented Apr 24, 2023 • edited Loading

trustin commented Apr 24, 2023 • edited Loading

trustin May 2, 2023

Choose a reason for hiding this comment

trustin May 2, 2023

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

minwoox May 8, 2023

Choose a reason for hiding this comment

trustin May 8, 2023

Choose a reason for hiding this comment

minwoox May 8, 2023

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

ikhoon May 22, 2023

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

trustin commented May 30, 2023

trustin commented May 30, 2023

trustin commented May 30, 2023

jrhee17 left a comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

trustin commented Jun 7, 2023

ikhoon commented Jun 7, 2023

trustin commented Jun 8, 2023

trustin commented Apr 24, 2023 •

edited

Loading

trustin commented Apr 24, 2023 •

edited

Loading