[POC] Server-side integration for xDS#6797
Draft
jrhee17 wants to merge 1 commit into
Draft
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR introduces server-side xDS integration for Armeria by adding connection-level infrastructure ( ChangesServer-side xDS integration with connection-level TLS and filter chains
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/src/main/java/com/linecorp/armeria/server/HttpServerPipelineConfigurator.java (1)
199-233:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply per-connection max-age overrides on plaintext connections.
Line 221 uses global
config.maxConnectionAgeMillis()infinishConfigureHttp(...), so values set viaConnectionContext.setMaxConnectionAgeMillis(...)in the acceptor are ignored for non-TLS connections.Suggested fix
private void configureHttp(ChannelPipeline p, `@Nullable` ProxiedAddresses proxiedAddresses) { final Channel ch = p.channel(); final ConnectionContext connectionContext = new ConnectionContext(H1C, "", null, proxiedAddresses, ch); + ch.attr(ConnectionContext.ATTR).set(connectionContext); @@ private void finishConfigureHttp(ChannelPipeline p, ConnectionContext connectionContext) { final long idleTimeoutMillis = config.idleTimeoutMillis(); - final long maxConnectionAgeMillis = config.maxConnectionAgeMillis(); + final long maxConnectionAgeMillis = resolveMaxConnectionAge(p.channel());Also applies to: 253-259
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/src/main/java/com/linecorp/armeria/server/HttpServerPipelineConfigurator.java` around lines 199 - 233, configureHttp/finishConfigureHttp currently use global config values (e.g., config.maxConnectionAgeMillis(), config.maxNumRequestsPerConnection()) so per-connection overrides set via ConnectionContext.setMaxConnectionAgeMillis(...) are ignored for plaintext connections; change finishConfigureHttp to read the effective values from the provided ConnectionContext (e.g., use connectionContext.maxConnectionAgeMillis() and connectionContext.maxNumRequestsPerConnection(), falling back to config.* if those are unset) before calling needsKeepAliveHandler and when constructing Http1ServerKeepAliveHandler so the per-connection overrides are honored (also apply the same change to the later block referenced around the second occurrence).
🧹 Nitpick comments (2)
core/src/test/java/com/linecorp/armeria/server/ServerTlsProviderTest.java (1)
148-156: ⚡ Quick winExercise the null-provider fallback, not just
build().
TlsProvider.of(TlsKeyPair.ofSelfSigned())never returnsnull, so this never hits thetls()fallback, and the server is never started or used for a TLS handshake. A regression inFallbackServerTlsProviderwould still pass here. Use a provider that returnsnullfor at least one hostname and assert the negotiated certificate over HTTPS.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/src/test/java/com/linecorp/armeria/server/ServerTlsProviderTest.java` around lines 148 - 156, The test currently uses a non-null provider so it never exercises the tls() fallback; update ServerTlsProviderTest.allowTlsProviderWithTlsSettings() to use a TlsProvider implementation (or a FallbackServerTlsProvider configuration) that returns null for at least one hostname so the Server will fall back to tls(TlsKeyPair.ofSelfSigned()), start the server (call server.start() or ensure it accepts requests), perform an HTTPS request against that hostname, and assert the negotiated certificate is the self-signed one; keep references to Server.builder(), tls(), tlsProvider(TlsProvider), and the test method name to locate where to change.xds/src/main/java/com/linecorp/armeria/xds/ListenerSnapshot.java (1)
122-130: ⚡ Quick winConsider including filter chain snapshots in debug output.
The
toDebugString()method does not includefilterChainSnapshotsordefaultFilterChainSnapshot, whiletoString()does (lines 117-118). Including these in the debug string would provide more complete debugging information.📋 Proposed addition to debug output
`@Override` public String toDebugString() { return MoreObjects.toStringHelper(this) .omitNullValues() .add("listener", listenerXdsResource.resource()) .add("routeSnapshot", SnapshotUtil.debugString(routeSnapshot, RouteSnapshot::toDebugString)) + .add("filterChainSnapshots", filterChainSnapshots) + .add("defaultFilterChainSnapshot", defaultFilterChainSnapshot) .toString(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@xds/src/main/java/com/linecorp/armeria/xds/ListenerSnapshot.java` around lines 122 - 130, The toDebugString() implementation in ListenerSnapshot omits filterChainSnapshots and defaultFilterChainSnapshot (present in toString()), so update ListenerSnapshot.toDebugString() to also include these fields: add entries for "filterChainSnapshots" with SnapshotUtil.debugString(filterChainSnapshots, FilterChainSnapshot::toDebugString) and "defaultFilterChainSnapshot" with defaultFilterChainSnapshot (or SnapshotUtil.debugString if needed) alongside the existing listenerXdsResource.resource() and routeSnapshot entries to produce a more complete debug output.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@core/src/main/java/com/linecorp/armeria/internal/server/DefaultServiceRequestContext.java`:
- Around line 269-272: Add a Javadoc comment to the public method
connectionContext() in class DefaultServiceRequestContext explaining what it
returns (the associated ConnectionContext for this request), any
threading/ownership assumptions or lifecycle notes (e.g., non-null and valid for
the lifetime of the request context), and include the `@return` tag describing the
ConnectionContext; reference the method name connectionContext() and the class
DefaultServiceRequestContext when locating where to add the comment.
- Around line 264-267: The channel() accessor is wrongly annotated `@Nullable`
despite ch being required non-null in the constructor; remove the `@Nullable`
annotation from the channel() method (or replace it with a non-null annotation
if your codebase uses one) so the signature correctly reflects that channel()
always returns a non-null Channel; locate the method named channel() and the ch
field/constructor check to make the change.
In `@core/src/main/java/com/linecorp/armeria/server/ConnectionContext.java`:
- Around line 63-64: Add explicit null checks using Objects.requireNonNull(...,
"...") for all public API parameters in ConnectionContext: at minimum add
Objects.requireNonNull(channel, "channel") at the start of public static
ConnectionContext get(Channel channel), and likewise add
Objects.requireNonNull(...) for the user-facing parameters in the other public
methods shown around lines 143 and 150 (use the exact parameter names from those
method signatures and provide clear messages), so each public method validates
its inputs with the required project convention.
- Around line 113-121: The two methods localAddress() and remoteAddress() should
not cast channel.localAddress()/channel.remoteAddress() directly to
InetSocketAddress because domain-socket channels can return other SocketAddress
types; change both method signatures to return java.net.SocketAddress (remove
the direct cast and return channel.localAddress()/channel.remoteAddress() as-is)
and add optional helpers like inetLocalAddress()/inetRemoteAddress() that
perform an instanceof check and return an InetSocketAddress or null (or
Optional<InetSocketAddress>) to preserve callers that need InetSocketAddress
while avoiding ClassCastException.
In `@core/src/main/java/com/linecorp/armeria/server/DefaultServerConfig.java`:
- Around line 170-178: The constructor for DefaultServerConfig must enforce the
HTTPS wiring invariant: ensure serverTlsProvider and sslContextFactory are
either both non-null or both null; if one is null and the other is non-null,
throw an IllegalArgumentException describing the mismatch. Add the same
validation to the other constructor/overload referenced around lines 285-296 so
the class cannot be constructed into a partially-populated HTTPS state (check
the serverTlsProvider and sslContextFactory parameters in those constructors and
reject the XOR case).
In
`@core/src/main/java/com/linecorp/armeria/server/HttpServerPipelineConfigurator.java`:
- Around line 207-212: The acceptor callback currently only closes the channel
when accepted equals Boolean.FALSE, allowing null to be treated as acceptance;
change the logic in the whenCompleteAsync handler for
acceptor.accept(connectionContext) to treat any non-TRUE result as rejection by
using a strict Boolean.TRUE check (i.e., replace the
Boolean.FALSE.equals(accepted) check with !Boolean.TRUE.equals(accepted)) so
that on (t != null || !Boolean.TRUE.equals(accepted)) you call ch.close(),
otherwise call finishConfigureHttp(p, connectionContext).
In `@core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java`:
- Around line 348-349: The new public API methods ports() and
connectionAcceptor() in ServerBuilder are missing the required `@UnstableApi`
annotation; update their declarations to add the `@UnstableApi` annotation (same
as other newly added methods) so both ServerBuilder.ports() and
ServerBuilder.connectionAcceptor() are annotated accordingly and import the
annotation if missing.
In `@core/src/main/java/com/linecorp/armeria/server/ServerTlsProvider.java`:
- Around line 51-62: The Javadoc for ServerTlsProvider.serverTlsSpec is
incorrect: the method must never return null but must return a non-null
CompletableFuture that may complete with null to indicate fallthrough; update
the doc to state “returns a non-null CompletableFuture that completes with a
ServerTlsSpec or with null to indicate this provider does not handle the
connection,” and mention examples like FallbackServerTlsProvider which expect a
non-null future; ensure the contract clearly requires implementations
(serverTlsSpec) to return CompletableFuture.completedFuture(null) for
fallthrough rather than returning null.
In
`@core/src/main/java/com/linecorp/armeria/server/ServiceRequestContextBuilder.java`:
- Around line 246-247: The synthetic ConnectionContext is being created with
null proxied addresses causing ctx.connectionContext().proxiedAddresses() to be
incorrect; in ServiceRequestContextBuilder replace the null proxied-address
argument when constructing new ConnectionContext(...) with the resolved
proxiedAddresses variable (i.e. new ConnectionContext(sessionProtocol(), "",
proxiedAddresses, null, ch) or the correct parameter position for proxied
addresses), ensuring the existing proxiedAddresses field is passed through to
the ConnectionContext constructor.
In
`@core/src/main/java/com/linecorp/armeria/server/ServiceRequestContextWrapper.java`:
- Around line 78-81: Add a Javadoc comment to the public method
ServiceRequestContextWrapper.connectionContext() describing that it returns the
current ConnectionContext and that the implementation delegates to
unwrap().connectionContext(); include a brief description, an `@return` tag
specifying it returns the ConnectionContext for this request, and note that it
forwards to the underlying context via unwrap() so callers understand
delegation.
In `@core/src/main/java/com/linecorp/armeria/server/StaticTlsProvider.java`:
- Around line 69-82: The mapping currently keys TLS specs only by
originalHostnamePattern (in buildMapping) which collapses different
ServerTlsSpec for the same hostname on different ports; change the key to
include the listening port (e.g., combine vh.originalHostnamePattern() with the
ServerPort or its port number) when calling builder.add in buildMapping (and/or
maintain a separate DomainWildcardMappingBuilder per ServerPort), and update the
resolution logic that reads ctx.sniHostname() (the StaticTlsProvider lookup code
around lines 90–97) to lookup by the same port+hostname composite key (extract
the port from the channel/ctx/local address or ServerPort context) so each
port+hostname maps to its own ServerTlsSpec.
In
`@it/xds-client/src/test/java/com/linecorp/armeria/xds/it/ServerMultiplePluginTest.java`:
- Around line 167-173: In ServerMultiplePluginTest, avoid brittle root-cause
assertion on TLS handshake failures: update the assertion on the
client3.execute(...) call to only assert the outer exception type
(UnprocessedRequestException) and remove the
hasRootCauseInstanceOf(SignatureException.class) check so the test does not
depend on provider/JDK-specific TLS exception types.
- Around line 133-139: The test currently assumes ordering from
server.server().activePorts() maps ports to certs; instead, probe each HTTPS
port (from ServerPort entries filtered by ServerPort::hasHttps and mapped via
localAddress().getPort()) by attempting TLS connections using both ClientTlsSpec
variants for cert1 and cert2, record which ClientTlsSpec succeeds for each port,
and then assert that one port accepts cert1 and the other accepts cert2; update
ServerMultiplePluginTest to replace the index-based httpsPorts.get(...) checks
with this probe-and-map approach (use the existing ClientTlsSpec instances and
the test helper that makes a TLS request to a port to determine success).
In
`@it/xds-istio/src/test/java/com/linecorp/armeria/it/xds/XdsEchoConfigurator.java`:
- Around line 33-41: Add Javadoc comments for the public class
XdsEchoConfigurator and its public override method reconfigure(ServerBuilder
sb): document the purpose of XdsEchoConfigurator (what it configures, e.g., sets
up the test echo server, constants like LISTENER_NAME and SERVER_PORT) and
describe the behavior and parameters of reconfigure(ServerBuilder sb) (what the
ServerBuilder is configured to do and any side effects). Keep comments concise,
include `@param` for the ServerBuilder parameter and `@throws` if the method can
throw checked exceptions, and follow the project's Javadoc style.
In `@xds/src/main/java/com/linecorp/armeria/xds/ServerSnapshotWatcher.java`:
- Around line 220-254: ResolvedFilterChain.matches(): the server_name
restriction currently only runs when sniHostname is non-empty, allowing chains
with configured serverNames to match connections that provide no SNI; change the
logic using filterChainMatch.getServerNamesList()/serverNames and sniHostname so
that if serverNames is non-empty and sniHostname is empty the method returns
false, and otherwise require serverNames.contains(sniHostname) to match; update
the block that references serverNames/sniHostname so it reads: if
(!serverNames.isEmpty()) { if (sniHostname.isEmpty() ||
!serverNames.contains(sniHostname)) return false; }.
In `@xds/src/main/java/com/linecorp/armeria/xds/TransportSocketSnapshot.java`:
- Around line 182-202: The server mTLS code in buildServerTlsSpec currently sets
ClientAuth.REQUIRE without mirroring the client-side handling of trusted CA /
verifier factories and system-root/no-verify semantics; update
buildServerTlsSpec (and ServerTlsSpecBuilder usage) so that when
DownstreamTlsTransportSocketFactory.requireClientCertificate(...) is true you
only set clientAuth and the trust configuration in the same way
buildClientTlsSpec does: copy trustedCertificates from
validationContext.trustedCa when present, apply
validationContext.peerVerifierFactories() (SPKI/hash/SAN match verifiers) to the
ServerTlsSpec so server-side verifierFactories are used, and handle the
validationContext cases for system_root_certs or “no-verify” exactly as the
client path does instead of relying on JVM default trust managers; in short,
reuse or mirror the client-side trust-selection logic (trustedCa,
peerVerifierFactories, system-root/no-verify handling) when constructing the
ServerTlsSpec in buildServerTlsSpec so server mTLS semantics match client mTLS
semantics.
In `@xds/src/main/java/com/linecorp/armeria/xds/XdsServerPlugin.java`:
- Around line 131-142: Add Javadoc to the public install(ServerBuilder sb)
method (in class XdsServerPlugin) describing its behavior: explain that it
blocks until the first xDS snapshot is resolved by calling
watcher.whenReady().get(readyTimeout.toMillis(), TimeUnit.MILLISECONDS), state
what happens when the timeout is exceeded (throws RuntimeException wrapping the
caught exception), and note that after readiness it registers configured server
ports (serverPorts) with the provided ServerBuilder and installs xDS-related
policies/decorators; ensure the Javadoc mentions the method implements
ServerPlugin.install and documents parameters and thrown behavior concisely.
- Around line 187-190: The public close() method in XdsServerPlugin lacks
Javadoc; add a Javadoc comment above the close() method in class XdsServerPlugin
that states this method closes the underlying xDS listener and cleans up
resources (e.g., "Closes the underlying xDS listener and releases any associated
resources."), mention any important behavior such as idempotence or exceptions
if applicable, and reference listenerRoot as the resource being closed.
- Around line 72-106: Add Javadoc for all public XdsServerPlugin constructors:
document the constructor that takes (XdsBootstrap, String listenerName) as
creating a plugin that subscribes to the named xDS listener and listens on an
ephemeral port (HTTP/HTTPS) and indicate the DEFAULT_READY_TIMEOUT used;
document the (XdsBootstrap, String listenerName, int... ports) overload and
explain that the varargs ports are converted via toServerPorts(...) and
represent numeric ports to listen on; document the (XdsBootstrap, String
listenerName, ServerPort) overload and clarify that ServerPort allows explicit
SessionProtocol settings; and document the (XdsBootstrap, String listenerName,
ServerPort, Duration readyTimeout) overload including the readyTimeout behavior
(how long to wait for the first xDS snapshot before starting) and that the other
constructors delegate to it. Include `@param` entries for bootstrap, listenerName
(purpose), ports/ serverPort, readyTimeout (units and default), and a brief
`@throws` or `@see` if applicable.
---
Outside diff comments:
In
`@core/src/main/java/com/linecorp/armeria/server/HttpServerPipelineConfigurator.java`:
- Around line 199-233: configureHttp/finishConfigureHttp currently use global
config values (e.g., config.maxConnectionAgeMillis(),
config.maxNumRequestsPerConnection()) so per-connection overrides set via
ConnectionContext.setMaxConnectionAgeMillis(...) are ignored for plaintext
connections; change finishConfigureHttp to read the effective values from the
provided ConnectionContext (e.g., use connectionContext.maxConnectionAgeMillis()
and connectionContext.maxNumRequestsPerConnection(), falling back to config.* if
those are unset) before calling needsKeepAliveHandler and when constructing
Http1ServerKeepAliveHandler so the per-connection overrides are honored (also
apply the same change to the later block referenced around the second
occurrence).
---
Nitpick comments:
In `@core/src/test/java/com/linecorp/armeria/server/ServerTlsProviderTest.java`:
- Around line 148-156: The test currently uses a non-null provider so it never
exercises the tls() fallback; update
ServerTlsProviderTest.allowTlsProviderWithTlsSettings() to use a TlsProvider
implementation (or a FallbackServerTlsProvider configuration) that returns null
for at least one hostname so the Server will fall back to
tls(TlsKeyPair.ofSelfSigned()), start the server (call server.start() or ensure
it accepts requests), perform an HTTPS request against that hostname, and assert
the negotiated certificate is the self-signed one; keep references to
Server.builder(), tls(), tlsProvider(TlsProvider), and the test method name to
locate where to change.
In `@xds/src/main/java/com/linecorp/armeria/xds/ListenerSnapshot.java`:
- Around line 122-130: The toDebugString() implementation in ListenerSnapshot
omits filterChainSnapshots and defaultFilterChainSnapshot (present in
toString()), so update ListenerSnapshot.toDebugString() to also include these
fields: add entries for "filterChainSnapshots" with
SnapshotUtil.debugString(filterChainSnapshots,
FilterChainSnapshot::toDebugString) and "defaultFilterChainSnapshot" with
defaultFilterChainSnapshot (or SnapshotUtil.debugString if needed) alongside the
existing listenerXdsResource.resource() and routeSnapshot entries to produce a
more complete debug output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a6ea0757-a07d-4bf4-acde-2831345667a4
📒 Files selected for processing (60)
benchmarks/jmh/benchmarks/jmh/build/results/jmh/aggregate-results.txtbenchmarks/jmh/run-benchmark.shcore/src/main/java/com/linecorp/armeria/internal/server/DefaultServiceRequestContext.javacore/src/main/java/com/linecorp/armeria/server/ConnectionAcceptHandler.javacore/src/main/java/com/linecorp/armeria/server/ConnectionAcceptor.javacore/src/main/java/com/linecorp/armeria/server/ConnectionContext.javacore/src/main/java/com/linecorp/armeria/server/ConnectionLevelSetters.javacore/src/main/java/com/linecorp/armeria/server/DefaultServerConfig.javacore/src/main/java/com/linecorp/armeria/server/FallbackServerTlsProvider.javacore/src/main/java/com/linecorp/armeria/server/Http2ServerConnectionHandler.javacore/src/main/java/com/linecorp/armeria/server/HttpServerHandler.javacore/src/main/java/com/linecorp/armeria/server/HttpServerPipelineConfigurator.javacore/src/main/java/com/linecorp/armeria/server/Server.javacore/src/main/java/com/linecorp/armeria/server/ServerBuilder.javacore/src/main/java/com/linecorp/armeria/server/ServerPlugin.javacore/src/main/java/com/linecorp/armeria/server/ServerTlsConfig.javacore/src/main/java/com/linecorp/armeria/server/ServerTlsConfigBuilder.javacore/src/main/java/com/linecorp/armeria/server/ServerTlsProvider.javacore/src/main/java/com/linecorp/armeria/server/ServerTlsSpec.javacore/src/main/java/com/linecorp/armeria/server/ServiceRequestContext.javacore/src/main/java/com/linecorp/armeria/server/ServiceRequestContextBuilder.javacore/src/main/java/com/linecorp/armeria/server/ServiceRequestContextWrapper.javacore/src/main/java/com/linecorp/armeria/server/StaticTlsProvider.javacore/src/main/java/com/linecorp/armeria/server/TlsProviderAdapter.javacore/src/main/java/com/linecorp/armeria/server/TlsProviderMapping.javacore/src/main/java/com/linecorp/armeria/server/UpdatableServerConfig.javacore/src/main/java/com/linecorp/armeria/server/VirtualHost.javacore/src/main/java/com/linecorp/armeria/server/VirtualHostBuilder.javacore/src/test/java/com/linecorp/armeria/server/ServerTlsProviderTest.javacore/src/test/java/com/linecorp/armeria/server/TlsProviderMappingTest.javacore/src/test/java/com/linecorp/armeria/server/VirtualHostAnnotatedServiceBindingBuilderTest.javacore/src/test/java/com/linecorp/armeria/server/VirtualHostBuilderTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/ServerConnectionConfigTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/ServerDecoratorTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/ServerMultiPortTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/ServerMultiplePluginTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/TestHeaderFilterFactory.javait/xds-client/src/test/resources/META-INF/services/com.linecorp.armeria.xds.filter.HttpFilterFactoryit/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioPodCustomizer.javait/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioServerExtension.javait/xds-istio/src/test/java/com/linecorp/armeria/it/xds/XdsClientToServerTest.javait/xds-istio/src/test/java/com/linecorp/armeria/it/xds/XdsEchoConfigurator.javait/xds-istio/src/test/java/com/linecorp/armeria/it/xds/XdsResourceReader.javaxds-api/src/main/proto/envoy/config/listener/v3/listener.protoxds-api/src/main/proto/envoy/config/listener/v3/listener_components.protoxds-api/src/main/proto/envoy/extensions/transport_sockets/tls/v3/tls.protoxds/docs/SERVER_DESIGN.mdxds/docs/SERVER_PROPOSAL.mdxds/src/main/java/com/linecorp/armeria/xds/DelegatingHttpService.javaxds/src/main/java/com/linecorp/armeria/xds/DownstreamTlsTransportSocketFactory.javaxds/src/main/java/com/linecorp/armeria/xds/FilterChainSnapshot.javaxds/src/main/java/com/linecorp/armeria/xds/FilterUtil.javaxds/src/main/java/com/linecorp/armeria/xds/ListenerSnapshot.javaxds/src/main/java/com/linecorp/armeria/xds/ListenerStream.javaxds/src/main/java/com/linecorp/armeria/xds/ParsedFilterChain.javaxds/src/main/java/com/linecorp/armeria/xds/ServerSnapshotWatcher.javaxds/src/main/java/com/linecorp/armeria/xds/TransportSocketSnapshot.javaxds/src/main/java/com/linecorp/armeria/xds/XdsExtensionRegistry.javaxds/src/main/java/com/linecorp/armeria/xds/XdsServerPlugin.javaxds/src/main/java/com/linecorp/armeria/xds/filter/XdsHttpFilter.java
💤 Files with no reviewable changes (2)
- core/src/test/java/com/linecorp/armeria/server/TlsProviderMappingTest.java
- core/src/main/java/com/linecorp/armeria/server/TlsProviderMapping.java
Comment on lines
+264
to
267
| @Nullable | ||
| protected Channel channel() { | ||
| return ch; | ||
| } |
Contributor
There was a problem hiding this comment.
Remove incorrect @Nullable annotation from channel().
The channel() method is annotated with @Nullable, but the ch field is validated as non-null in the constructor (line 185: requireNonNull(ch, "ch")), so this method always returns a non-null value.
🔧 Proposed fix
- `@Nullable`
protected Channel channel() {
return ch;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @Nullable | |
| protected Channel channel() { | |
| return ch; | |
| } | |
| protected Channel channel() { | |
| return ch; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@core/src/main/java/com/linecorp/armeria/internal/server/DefaultServiceRequestContext.java`
around lines 264 - 267, The channel() accessor is wrongly annotated `@Nullable`
despite ch being required non-null in the constructor; remove the `@Nullable`
annotation from the channel() method (or replace it with a non-null annotation
if your codebase uses one) so the signature correctly reflects that channel()
always returns a non-null Channel; locate the method named channel() and the ch
field/constructor check to make the change.
Comment on lines
+269
to
+272
| @Override | ||
| public ConnectionContext connectionContext() { | ||
| return connectionContext; | ||
| } |
Contributor
There was a problem hiding this comment.
Add Javadoc for the new public method.
The connectionContext() method is public and requires Javadoc documentation. As per coding guidelines, all public and protected methods must have Javadoc.
📝 Suggested Javadoc
+ /**
+ * Returns the {`@link` ConnectionContext} for this request.
+ */
`@Override`
public ConnectionContext connectionContext() {As per coding guidelines: "ensure all public classes and public/protected methods have Javadoc."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @Override | |
| public ConnectionContext connectionContext() { | |
| return connectionContext; | |
| } | |
| /** | |
| * Returns the {`@link` ConnectionContext} for this request. | |
| */ | |
| `@Override` | |
| public ConnectionContext connectionContext() { | |
| return connectionContext; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@core/src/main/java/com/linecorp/armeria/internal/server/DefaultServiceRequestContext.java`
around lines 269 - 272, Add a Javadoc comment to the public method
connectionContext() in class DefaultServiceRequestContext explaining what it
returns (the associated ConnectionContext for this request), any
threading/ownership assumptions or lifecycle notes (e.g., non-null and valid for
the lifetime of the request context), and include the `@return` tag describing the
ConnectionContext; reference the method name connectionContext() and the class
DefaultServiceRequestContext when locating where to add the comment.
Comment on lines
+63
to
+64
| public static ConnectionContext get(Channel channel) { | ||
| return channel.attr(ATTR).get(); |
Contributor
There was a problem hiding this comment.
Add explicit null checks for new public API parameters.
Line 63, Line 143, and Line 150 accept user-facing parameters without requireNonNull(...), which weakens error messages and violates the project’s public API validation convention.
Suggested fix
`@Nullable`
public static ConnectionContext get(Channel channel) {
+ requireNonNull(channel, "channel");
return channel.attr(ATTR).get();
}
@@
`@Nullable`
public <T> T attr(AttributeKey<T> key) {
+ requireNonNull(key, "key");
return attrs.attr(key);
}
@@
public <T> void setAttr(AttributeKey<T> key, `@Nullable` T value) {
+ requireNonNull(key, "key");
attrs.set(key, value);
}As per coding guidelines: "use Objects.requireNonNull(..., "...") for explicit null checks on user-facing public parameters."
Also applies to: 143-151
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/src/main/java/com/linecorp/armeria/server/ConnectionContext.java` around
lines 63 - 64, Add explicit null checks using Objects.requireNonNull(..., "...")
for all public API parameters in ConnectionContext: at minimum add
Objects.requireNonNull(channel, "channel") at the start of public static
ConnectionContext get(Channel channel), and likewise add
Objects.requireNonNull(...) for the user-facing parameters in the other public
methods shown around lines 143 and 150 (use the exact parameter names from those
method signatures and provide clear messages), so each public method validates
its inputs with the required project convention.
Comment on lines
+113
to
+121
| public InetSocketAddress localAddress() { | ||
| return (InetSocketAddress) channel.localAddress(); | ||
| } | ||
|
|
||
| /** | ||
| * Returns the remote address of this connection. | ||
| */ | ||
| public InetSocketAddress remoteAddress() { | ||
| return (InetSocketAddress) channel.remoteAddress(); |
Contributor
There was a problem hiding this comment.
Avoid raw Channel address casts in ConnectionContext.
Line 114 and Line 121 cast Netty socket addresses directly to InetSocketAddress. For domain-socket channels this can throw ClassCastException at runtime when plugins/services read ConnectionContext addresses.
Suggested fix
+import com.linecorp.armeria.internal.common.util.ChannelUtil;
@@
public InetSocketAddress localAddress() {
- return (InetSocketAddress) channel.localAddress();
+ final InetSocketAddress localAddress = ChannelUtil.localAddress(channel);
+ assert localAddress != null;
+ return localAddress;
}
@@
public InetSocketAddress remoteAddress() {
- return (InetSocketAddress) channel.remoteAddress();
+ final InetSocketAddress remoteAddress = ChannelUtil.remoteAddress(channel);
+ assert remoteAddress != null;
+ return remoteAddress;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/src/main/java/com/linecorp/armeria/server/ConnectionContext.java` around
lines 113 - 121, The two methods localAddress() and remoteAddress() should not
cast channel.localAddress()/channel.remoteAddress() directly to
InetSocketAddress because domain-socket channels can return other SocketAddress
types; change both method signatures to return java.net.SocketAddress (remove
the direct cast and return channel.localAddress()/channel.remoteAddress() as-is)
and add optional helpers like inetLocalAddress()/inetRemoteAddress() that
perform an instanceof check and return an InetSocketAddress or null (or
Optional<InetSocketAddress>) to preserve callers that need InetSocketAddress
while avoiding ClassCastException.
Comment on lines
+170
to
+178
| @Nullable ServerTlsProvider serverTlsProvider, | ||
| @Nullable SslContextFactory sslContextFactory, | ||
| Http1HeaderNaming http1HeaderNaming, | ||
| DependencyInjector dependencyInjector, | ||
| Function<? super String, String> absoluteUriTransformer, | ||
| long unloggedExceptionsReportIntervalMillis, | ||
| List<ShutdownSupport> shutdownSupports, | ||
| @Nullable Function<? super String, ? extends EventLoopGroup> bossGroupFactory) { | ||
| @Nullable Function<? super String, ? extends EventLoopGroup> bossGroupFactory, | ||
| @Nullable ConnectionAcceptor connectionAcceptor) { |
Contributor
There was a problem hiding this comment.
Validate the HTTPS wiring invariant in the config constructor.
core/src/main/java/com/linecorp/armeria/server/HttpServerPipelineConfigurator.java, Lines 261-268, assumes serverTlsProvider and sslContextFactory are present together and only protects that with assert. This constructor now allows them to diverge, so a partially populated DefaultServerConfig will survive construction and fail later on the HTTPS path once assertions are off. Reject that partial state here.
Suggested invariant check
this.errorHandler = requireNonNull(errorHandler, "errorHandler");
+ if ((serverTlsProvider == null) != (sslContextFactory == null)) {
+ throw new IllegalArgumentException(
+ "serverTlsProvider and sslContextFactory must be set together");
+ }
this.serverTlsProvider = serverTlsProvider;
this.sslContextFactory = sslContextFactory;Also applies to: 285-296
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/src/main/java/com/linecorp/armeria/server/DefaultServerConfig.java`
around lines 170 - 178, The constructor for DefaultServerConfig must enforce the
HTTPS wiring invariant: ensure serverTlsProvider and sslContextFactory are
either both non-null or both null; if one is null and the other is non-null,
throw an IllegalArgumentException describing the mismatch. Add the same
validation to the other constructor/overload referenced around lines 285-296 so
the class cannot be constructed into a partially-populated HTTPS state (check
the serverTlsProvider and sslContextFactory parameters in those constructors and
reject the XOR case).
Comment on lines
+220
to
+254
| boolean matches(int destinationPort, String transportProtocol, | ||
| String sniHostname, @Nullable List<String> alpnProtocols) { | ||
| if (filterChainMatch.hasDestinationPort() && | ||
| filterChainMatch.getDestinationPort().getValue() != destinationPort) { | ||
| return false; | ||
| } | ||
|
|
||
| final String matchTransport = filterChainMatch.getTransportProtocol(); | ||
| if (!matchTransport.isEmpty() && !matchTransport.equals(transportProtocol)) { | ||
| return false; | ||
| } | ||
|
|
||
| final List<String> serverNames = filterChainMatch.getServerNamesList(); | ||
| if (!serverNames.isEmpty() && !sniHostname.isEmpty()) { | ||
| if (!serverNames.contains(sniHostname)) { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| final List<String> matchAlpn = filterChainMatch.getApplicationProtocolsList(); | ||
| if (!matchAlpn.isEmpty() && alpnProtocols != null) { | ||
| boolean found = false; | ||
| for (String offered : alpnProtocols) { | ||
| if (matchAlpn.contains(offered)) { | ||
| found = true; | ||
| break; | ||
| } | ||
| } | ||
| if (!found) { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| return true; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
How does Envoy filter chain matching handle server_names when SNI is not provided by the client?
💡 Result:
In Envoy, the server_name field in a filter_chain_match specifically refers to the SNI (Server Name Indication) extracted during a TLS handshake [1][2]. When a client does not provide SNI—or when the connection is not TLS—Envoy cannot populate this field [3][4]. How Envoy handles connections missing an SNI depends on your filter chain configuration: 1. Matching Behavior: If you configure a filter chain with a server_names requirement, Envoy will only select that filter chain if the SNI matches one of the specified values [1][2]. If no SNI is provided, that specific filter chain will not be selected [5]. 2. Default/Fallback Chains: To handle connections without an SNI (or with an unknown SNI), you must define a catch-all filter chain that does not include a server_names requirement [5][1]. Envoy prioritizes more specific matches; if an incoming connection does not have an SNI, it will fall back to a filter chain where server_names is omitted (left empty), as this is treated as the least specific match [1][2]. 3. Detecting Missing SNI: Because filter chain matching is a static selection process based on available metadata, you cannot explicitly match against an "empty" SNI within the filter_chain_match criteria [6][5]. To handle these cases, you should: - Provide a default filter chain without a server_names field to capture traffic that fails to match your specific SNI-based chains [5]. - If you need to distinguish between connections with and without SNI for logic later in the filter stack, you can use a regular expression match on server_names (e.g., ^.+$) in a specific chain to capture all connections where SNI is present, leaving a separate, non-matching chain to handle the rest [6]. In summary, when SNI is missing, Envoy simply skips any filter chain that requires a specific server_name and proceeds to evaluate other available chains. If no non-server-name-restricted chain exists, the connection will fail with a "no matching filter chain found" error [5].
Citations:
- 1: https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/listener/v3/listener_components.proto.html?highlight=filterchainmatch
- 2: https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/listener/v3/listener_components.proto.html
- 3: filter_chain_match.server_names is not working for TCP filter envoyproxy/envoy#3615
- 4: http inspector filter is not filling
server_nameenvoyproxy/envoy#22612 - 5: Connecting with IP when the listener is configured with SNI envoyproxy/envoy#15869
- 6: How to detect if server_name is populated for TLS transport protocol envoyproxy/envoy#37256
🏁 Script executed:
#!/bin/bash
set -euo pipefail
repo_root="$(pwd)"
file="xds/src/main/java/com/linecorp/armeria/xds/ServerSnapshotWatcher.java"
# Show the matches method and surrounding call site context
python3 - <<PY
import itertools,sys
path="${file}"
start=130
end=280
with open(path,'r',encoding='utf-8') as f:
for i,line in enumerate(f, start=1):
if start <= i <= end:
sys.stdout.write(f"{i:5d}:{line}")
PY
# Also locate ctx.sniHostname() definition / usage in this file
rg -n "sniHostname\\(" "$file" || true
rg -n "getServerNamesList\\(" "$file" || trueRepository: line/armeria
Length of output: 6320
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="xds/src/main/java/com/linecorp/armeria/xds/ServerSnapshotWatcher.java"
python3 - <<'PY'
import sys
path="xds/src/main/java/com/linecorp/armeria/xds/ServerSnapshotWatcher.java"
start=140
end=270
with open(path,'r',encoding='utf-8') as f:
for lineno,line in enumerate(f, start=1):
if start <= lineno <= end:
sys.stdout.write(f"{lineno:5d}:{line}")
PY
echo "---- sniHostname usages ----"
rg -n "sniHostname" "$file" || true
echo "---- serverNamesList usages ----"
rg -n "getServerNamesList" "$file" || trueRepository: line/armeria
Length of output: 6169
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="xds/src/main/java/com/linecorp/armeria/xds/ServerSnapshotWatcher.java"
python3 - <<'PY'
import sys
path="xds/src/main/java/com/linecorp/armeria/xds/ServerSnapshotWatcher.java"
start=140
end=270
with open(path,'r',encoding='utf-8') as f:
for lineno,line in enumerate(f, start=1):
if start <= lineno <= end:
sys.stdout.write(f"{lineno:5d}:{line}")
PY
echo "---- sniHostname usages ----"
rg -n "sniHostname" "$file" || true
echo "---- serverNamesList usages ----"
rg -n "getServerNamesList" "$file" || trueRepository: line/armeria
Length of output: 6169
Fix SNI server_names matching when SNI is missing
In ResolvedFilterChain.matches() the serverNames restriction only runs when !sniHostname.isEmpty() (lines 232-237). If a filter chain specifies server_names but the connection provides no SNI (empty sniHostname), the match currently “passes” by default, which diverges from Envoy’s behavior (server_name-restricted chains are skipped when SNI isn’t provided).
- Adjust logic so that when
serverNamesis non-empty, an emptysniHostnamecausesreturn falseunless it matches one of the configured names.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@xds/src/main/java/com/linecorp/armeria/xds/ServerSnapshotWatcher.java` around
lines 220 - 254, ResolvedFilterChain.matches(): the server_name restriction
currently only runs when sniHostname is non-empty, allowing chains with
configured serverNames to match connections that provide no SNI; change the
logic using filterChainMatch.getServerNamesList()/serverNames and sniHostname so
that if serverNames is non-empty and sniHostname is empty the method returns
false, and otherwise require serverNames.contains(sniHostname) to match; update
the block that references serverNames/sniHostname so it reads: if
(!serverNames.isEmpty()) { if (sniHostname.isEmpty() ||
!serverNames.contains(sniHostname)) return false; }.
Comment on lines
+182
to
+202
| @Nullable | ||
| private static ServerTlsSpec buildServerTlsSpec( | ||
| DownstreamTlsContext downstreamTlsContext, | ||
| @Nullable TlsCertificateSnapshot tlsCertificate, | ||
| @Nullable CertificateValidationContextSnapshot validationContext) { | ||
| if (tlsCertificate == null || tlsCertificate.tlsKeyPair() == null) { | ||
| return null; | ||
| } | ||
| final ServerTlsSpecBuilder builder = ServerTlsSpec.builder(); | ||
| builder.tlsKeyPair(tlsCertificate.tlsKeyPair()); | ||
| if (DownstreamTlsTransportSocketFactory.requireClientCertificate(downstreamTlsContext)) { | ||
| builder.clientAuth(ClientAuth.REQUIRE); | ||
| } | ||
| if (validationContext != null) { | ||
| final List<X509Certificate> trustedCa = validationContext.trustedCa(); | ||
| if (trustedCa != null) { | ||
| builder.trustedCertificates(trustedCa); | ||
| } | ||
| } | ||
| return builder.build(); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for ServerTlsSpec usage and ClientAuth.REQUIRE behavior
rg -n -C3 'ClientAuth\.REQUIRE' --type=javaRepository: line/armeria
Length of output: 19762
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate ServerTlsSpec / ServerTlsSpecBuilder implementations
rg -n "class ServerTlsSpecBuilder|interface ServerTlsSpecBuilder|ServerTlsSpecBuilder" --type=java .
rg -n "class ServerTlsSpec\b|interface ServerTlsSpec\b|ServerTlsSpec\.builder\(" --type=java .
# Inspect how clientAuth and trustedCertificates are used inside ServerTlsSpec
rg -n "clientAuth\(|trustedCertificates\(" --type=java core/src/main/java xds/src/main/java .
# Find any validation/enforcement around REQUIRED client auth and trust material
rg -n "REQUIRE.*trusted|trusted.*REQUIRE|clientAuth.*REQUIRE|REQUIRED.*clientAuth" --type=java core/src/main/java xds/src/main/java .
# Inspect DownstreamTlsTransportSocketFactory.requireClientCertificate
rg -n "requireClientCertificate\(" --type=java xds/src/main/java .
# Inspect CertificateValidationContextSnapshot
rg -n "class CertificateValidationContextSnapshot|interface CertificateValidationContextSnapshot" --type=java xds/src/main/java .Repository: line/armeria
Length of output: 31569
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show DownstreamTlsTransportSocketFactory.requireClientCertificate implementation
sed -n '1,220p' xds/src/main/java/com/linecorp/armeria/xds/DownstreamTlsTransportSocketFactory.java | nl -ba | sed -n '90,180p'
# Show TransportSocketSnapshot around buildServerTlsSpec for context
sed -n '140,230p' xds/src/main/java/com/linecorp/armeria/xds/TransportSocketSnapshot.java | nl -ba
# Inspect ServerTlsSpec builder/clientAuth/trustedCertificates fields
sed -n '1,260p' core/src/main/java/com/linecorp/armeria/server/ServerTlsSpec.java | nl -ba | sed -n '50,230p'
# Inspect SslContextUtil trustManager/clientAuth config
sed -n '120,290p' core/src/main/java/com/linecorp/armeria/internal/common/util/SslContextUtil.java | nl -ba
# Inspect SslContextFactory trustedCertificates handling
sed -n '1,220p' core/src/main/java/com/linecorp/armeria/internal/common/SslContextFactory.java | nl -ba | sed -n '60,160p'
# Inspect AbstractTlsSpec trustedCertificates default
sed -n '1,220p' core/src/main/java/com/linecorp/armeria/common/AbstractTlsSpec.java | nl -ba | sed -n '90,160p'Repository: line/armeria
Length of output: 99
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show DownstreamTlsTransportSocketFactory.requireClientCertificate implementation
cat -n xds/src/main/java/com/linecorp/armeria/xds/DownstreamTlsTransportSocketFactory.java | sed -n '90,170p'
# Show TransportSocketSnapshot around buildServerTlsSpec
cat -n xds/src/main/java/com/linecorp/armeria/xds/TransportSocketSnapshot.java | sed -n '150,230p'
# Inspect ServerTlsSpec builder/clientAuth/trustedCertificates fields and defaults
cat -n core/src/main/java/com/linecorp/armeria/server/ServerTlsSpec.java | sed -n '60,230p'
# Inspect SslContextUtil for clientAuth and trust manager usage
cat -n core/src/main/java/com/linecorp/armeria/internal/common/util/SslContextUtil.java | sed -n '120,290p'
# Inspect SslContextFactory trustedCertificates handling
cat -n core/src/main/java/com/linecorp/armeria/internal/common/SslContextFactory.java | sed -n '70,150p'
# Inspect AbstractTlsSpec trustedCertificates default
cat -n core/src/main/java/com/linecorp/armeria/common/AbstractTlsSpec.java | sed -n '90,170p'Repository: line/armeria
Length of output: 27905
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect defaults for verifierFactories/trustedCertificates in AbstractTlsSpecBuilder
sed -n '1,240p' core/src/main/java/com/linecorp/armeria/common/AbstractTlsSpecBuilder.java | cat -n
echo "----"
sed -n '1,260p' core/src/main/java/com/linecorp/armeria/common/AbstractTlsSpec.java | cat -n
echo "----"
# Inspect ServerTlsSpec constructor/fields for defaults/empty behavior
sed -n '1,240p' core/src/main/java/com/linecorp/armeria/server/ServerTlsSpec.java | cat -n
echo "----"
# Inspect TransportSocketSnapshotTest for mTLS / trusted CA missing cases
rg -n "TransportSocketSnapshotTest" -S it/xds-client/src/test/java/com/linecorp/armeria/xds/it/TransportSocketSnapshotTest.java
sed -n '360,520p' it/xds-client/src/test/java/com/linecorp/armeria/xds/it/TransportSocketSnapshotTest.java | cat -n
echo "----"
# Search for requireClientCertificate/validation context absence in xds ITs
rg -n "requireClientCertificate|require_client_certificate|ClientAuth\\.REQUIRE|trustedCa\\(\\)|trustedCa" it/xds-client/src/test/java/com/linecorp/armeria/xds/it -SRepository: line/armeria
Length of output: 38070
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect requireClientCertificate implementation
sed -n '1,220p' xds/src/main/java/com/linecorp/armeria/xds/DownstreamTlsTransportSocketFactory.java | cat -n
# 2) Inspect the rest of TransportSocketSnapshotTest around where server/client specs are asserted
sed -n '1,260p' it/xds-client/src/test/java/com/linecorp/armeria/xds/it/TransportSocketSnapshotTest.java | cat -n
echo "----"
sed -n '260,560p' it/xds-client/src/test/java/com/linecorp/armeria/xds/it/TransportSocketSnapshotTest.java | cat -n
# 3) Search for server-side TLS spec assertions in xds-client ITs
rg -n "serverTlsSpec|TransportSocketSnapshot.*server|tlsSnapshot.*server|clientAuth\\(ClientAuth\\.REQUIRE\\)|requireClientCertificate\\(" it/xds-client/src/test/java/com/linecorp/armeria/xds/it -S
# 4) Search for cases where trustedCa / trustedCertificates are empty while client auth is REQUIRED
rg -n "trustedCa\\(\\)\\s*\\)\\.isEmpty\\(|trustedCa.*isEmpty|trustedCertificates\\(\\)\\.isEmpty\\(" it/xds-client/src/test/java/com/linecorp/armeria/xds/it -SRepository: line/armeria
Length of output: 35995
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find clientAuth REQUIRE usages and check nearby for trustedCertificates/trustManager
rg -n -C5 "clientAuth\\(ClientAuth\\.REQUIRE\\)" --type=java core xds spring jetty tomcat10 it | head -n 200
# Also search for ServerTlsConfigBuilder.clientAuth without trustedCertificates nearby
rg -n -C5 "ServerTlsConfig\\.builder\\(\\)|ServerTlsConfigBuilder.*clientAuth\\(" --type=java core xds | head -n 200Repository: line/armeria
Length of output: 41334
🌐 Web query:
Netty SslContextBuilder clientAuth REQUIRE without trustManager() default behavior
💡 Result:
In Netty's SslContextBuilder, the default behavior for the trust manager—when it is not explicitly configured via any of the trustManager methods—is to use the system default trust manager [1][2][3][4]. If you set clientAuth(ClientAuth.REQUIRE) without explicitly providing a trust manager, the SslContextBuilder will fall back to using this system default [1][5]. The resulting SSL context will therefore attempt to authenticate client certificates against the trust store managed by the underlying SSL provider (typically the JDK's default trust store, such as the cacerts file in your Java installation, unless otherwise configured by system properties like javax.net.ssl.trustStore) [1][2][5]. If no certificate in the client's provided chain can be verified against the system's default trust store, the TLS handshake will fail as expected when ClientAuth.REQUIRE is enabled [6].
Citations:
- 1: https://netty.io/4.2/api/io/netty/handler/ssl/SslContextBuilder.html
- 2: https://netty.io/4.1/api/io/netty/handler/ssl/SslContextBuilder.html
- 3: https://javadoc.io/static/io.netty/netty-all/4.1.52.Final/io/netty/handler/ssl/SslContextBuilder.html
- 4: https://www.javadoc.io/static/io.netty/netty-handler/4.1.115.Final/io/netty/handler/ssl/SslContextBuilder.html
- 5: https://netty.io/4.1/xref/io/netty/handler/ssl/SslContextBuilder.html
- 6: TLS negotiation continues with optional client authentication when a client has a bogus certificate with Netty 4.1.7 and a native SSL provider netty/netty#6312
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,200p' xds/src/main/java/com/linecorp/armeria/xds/CertificateValidationContextSnapshot.java | cat -n
rg -n "trustedCa\\(" xds/src/main/java/com/linecorp/armeria/xds -S
rg -n "class CertificateValidationContext" xds/src/main/java/com/linecorp/armeria/xds -SRepository: line/armeria
Length of output: 8971
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,260p' xds/src/main/java/com/linecorp/armeria/xds/TransportSocketSnapshot.java | cat -nRepository: line/armeria
Length of output: 12613
Correct server-side client-cert verification semantics in buildServerTlsSpec
ClientAuth.REQUIREwithoutvalidationContext.trustedCadoes not mean “accept any client certificate”: sincebuildServerTlsSpecdoesn’t settrustedCertificates/verifierFactories,SslContextUtilwon’t callSslContextBuilder.trustManager(...), so Netty falls back to the system default trust manager and the handshake will fail if the client cert isn’t trusted.- The server path still diverges from the client path:
validationContext.peerVerifierFactories()(SPKI/hash/SAN match verifiers) are ignored for server-side mTLS, andsystem_root_certs/ “no-verify when no trusted CA” handling isn’t applied—so behavior can be incorrect relative to xDS expectations.
xds/src/main/java/com/linecorp/armeria/xds/TransportSocketSnapshot.java (buildServerTlsSpec)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@xds/src/main/java/com/linecorp/armeria/xds/TransportSocketSnapshot.java`
around lines 182 - 202, The server mTLS code in buildServerTlsSpec currently
sets ClientAuth.REQUIRE without mirroring the client-side handling of trusted CA
/ verifier factories and system-root/no-verify semantics; update
buildServerTlsSpec (and ServerTlsSpecBuilder usage) so that when
DownstreamTlsTransportSocketFactory.requireClientCertificate(...) is true you
only set clientAuth and the trust configuration in the same way
buildClientTlsSpec does: copy trustedCertificates from
validationContext.trustedCa when present, apply
validationContext.peerVerifierFactories() (SPKI/hash/SAN match verifiers) to the
ServerTlsSpec so server-side verifierFactories are used, and handle the
validationContext cases for system_root_certs or “no-verify” exactly as the
client path does instead of relying on JVM default trust managers; in short,
reuse or mirror the client-side trust-selection logic (trustedCa,
peerVerifierFactories, system-root/no-verify handling) when constructing the
ServerTlsSpec in buildServerTlsSpec so server mTLS semantics match client mTLS
semantics.
Comment on lines
+72
to
+106
| /** | ||
| * Creates a new {@link XdsServerPlugin} that subscribes to the given listener | ||
| * and listens on an ephemeral port with HTTP and HTTPS. | ||
| */ | ||
| public XdsServerPlugin(XdsBootstrap bootstrap, String listenerName) { | ||
| this(bootstrap, listenerName, | ||
| ImmutableList.of(new ServerPort(0, SessionProtocol.HTTP, SessionProtocol.HTTPS)), | ||
| DEFAULT_READY_TIMEOUT); | ||
| } | ||
|
|
||
| /** | ||
| * Creates a new {@link XdsServerPlugin} that subscribes to the given listener | ||
| * and listens on the specified port(s) with HTTP and HTTPS. | ||
| */ | ||
| public XdsServerPlugin(XdsBootstrap bootstrap, String listenerName, int... ports) { | ||
| this(bootstrap, listenerName, toServerPorts(ports), DEFAULT_READY_TIMEOUT); | ||
| } | ||
|
|
||
| /** | ||
| * Creates a new {@link XdsServerPlugin} that subscribes to the given listener | ||
| * and listens on the specified {@link ServerPort}. | ||
| */ | ||
| public XdsServerPlugin(XdsBootstrap bootstrap, String listenerName, ServerPort serverPort) { | ||
| this(bootstrap, listenerName, ImmutableList.of(serverPort), DEFAULT_READY_TIMEOUT); | ||
| } | ||
|
|
||
| /** | ||
| * Creates a new {@link XdsServerPlugin} that subscribes to the given listener | ||
| * and listens on the specified {@link ServerPort}, waiting up to {@code readyTimeout} | ||
| * for the first xDS snapshot to be resolved before the server starts. | ||
| */ | ||
| public XdsServerPlugin(XdsBootstrap bootstrap, String listenerName, ServerPort serverPort, | ||
| Duration readyTimeout) { | ||
| this(bootstrap, listenerName, ImmutableList.of(serverPort), readyTimeout); | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add Javadoc to public constructors.
All public constructors lack Javadoc comments. As per coding guidelines, all public methods (including constructors) should have Javadoc.
Consider adding Javadoc to each public constructor describing the parameters and behavior, especially:
- The
listenerNameparameter purpose - The
readyTimeoutparameter behavior and default value - The
portsvsServerPortdifferences
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@xds/src/main/java/com/linecorp/armeria/xds/XdsServerPlugin.java` around lines
72 - 106, Add Javadoc for all public XdsServerPlugin constructors: document the
constructor that takes (XdsBootstrap, String listenerName) as creating a plugin
that subscribes to the named xDS listener and listens on an ephemeral port
(HTTP/HTTPS) and indicate the DEFAULT_READY_TIMEOUT used; document the
(XdsBootstrap, String listenerName, int... ports) overload and explain that the
varargs ports are converted via toServerPorts(...) and represent numeric ports
to listen on; document the (XdsBootstrap, String listenerName, ServerPort)
overload and clarify that ServerPort allows explicit SessionProtocol settings;
and document the (XdsBootstrap, String listenerName, ServerPort, Duration
readyTimeout) overload including the readyTimeout behavior (how long to wait for
the first xDS snapshot before starting) and that the other constructors delegate
to it. Include `@param` entries for bootstrap, listenerName (purpose), ports/
serverPort, readyTimeout (units and default), and a brief `@throws` or `@see` if
applicable.
Comment on lines
+131
to
+142
| @Override | ||
| public void install(ServerBuilder sb) { | ||
| // Block until the first xDS snapshot is resolved so TLS and decorators | ||
| // are available before the server is built. | ||
| try { | ||
| watcher.whenReady().get(readyTimeout.toMillis(), TimeUnit.MILLISECONDS); | ||
| } catch (Exception e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| for (ServerPort serverPort : serverPorts) { | ||
| sb.port(serverPort); | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add Javadoc to public install method.
The install(ServerBuilder) method is public (from ServerPlugin interface) and lacks Javadoc. As per coding guidelines, all public methods should have Javadoc.
Add Javadoc describing:
- That this method blocks until the first xDS snapshot is resolved
- The behavior when the timeout is exceeded
- The registration of server ports and xDS policies
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@xds/src/main/java/com/linecorp/armeria/xds/XdsServerPlugin.java` around lines
131 - 142, Add Javadoc to the public install(ServerBuilder sb) method (in class
XdsServerPlugin) describing its behavior: explain that it blocks until the first
xDS snapshot is resolved by calling
watcher.whenReady().get(readyTimeout.toMillis(), TimeUnit.MILLISECONDS), state
what happens when the timeout is exceeded (throws RuntimeException wrapping the
caught exception), and note that after readiness it registers configured server
ports (serverPorts) with the provided ServerBuilder and installs xDS-related
policies/decorators; ensure the Javadoc mentions the method implements
ServerPlugin.install and documents parameters and thrown behavior concisely.
Comment on lines
+187
to
+190
| @Override | ||
| public void close() { | ||
| listenerRoot.close(); | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add Javadoc to public close method.
The close() method is public and lacks Javadoc. As per coding guidelines, all public methods should have Javadoc.
Add Javadoc describing that this method closes the underlying xDS listener and cleans up resources.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@xds/src/main/java/com/linecorp/armeria/xds/XdsServerPlugin.java` around lines
187 - 190, The public close() method in XdsServerPlugin lacks Javadoc; add a
Javadoc comment above the close() method in class XdsServerPlugin that states
this method closes the underlying xDS listener and cleans up resources (e.g.,
"Closes the underlying xDS listener and releases any associated resources."),
mention any important behavior such as idempotence or exceptions if applicable,
and reference listenerRoot as the resource being closed.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6797 +/- ##
============================================
+ Coverage 74.46% 74.93% +0.47%
- Complexity 22234 25028 +2794
============================================
Files 1963 2225 +262
Lines 82437 93321 +10884
Branches 10764 12198 +1434
============================================
+ Hits 61385 69933 +8548
- Misses 15918 17535 +1617
- Partials 5134 5853 +719 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation:
Explain why you're making this change and what problem you're trying to solve.
Modifications:
Result: