Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR enhances client TLS configuration with ALPN protocol override and endpoint identification algorithm support, extends xDS snapshot classes with debug string methods, adds comprehensive Istio integration testing infrastructure (including gRPC proxyless mode), and introduces test utilities for xDS resource parsing and filter factories. ALPN override allows runtime protocol selection, replacing the static defaults derived from protocol configuration. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Armeria Client
participant CTLS as ClientTlsSpec
participant Builder as ClientTlsSpecBuilder
participant SslUtil as SslContextUtil
participant OpenSSL as OpenSSL Context
Client->>Builder: alpnProtocols(customProtocols)
Builder->>Builder: store overrideAlpnProtocols
Client->>Builder: endpointIdentificationAlgorithm(algo)
Builder->>Builder: store algorithm
Client->>Builder: build()
Builder->>CTLS: new ClientTlsSpec(...,overrides,algo)
CTLS->>CTLS: initialize baseAlpnProtocols,overrideAlpnProtocols
Client->>SslUtil: createClientSslContext(ClientTlsSpec)
SslUtil->>CTLS: alpnProtocols()
CTLS-->>SslUtil: return overrideAlpnProtocols (if non-empty)
SslUtil->>CTLS: endpointIdentificationAlgorithm()
CTLS-->>SslUtil: return algorithm
SslUtil->>OpenSSL: setAlpnProtocols(protocols)
SslUtil->>OpenSSL: setEndpointIdentificationAlgorithm(algo)
SslUtil-->>Client: SslContext
sequenceDiagram
participant Test as Integration Test
participant Istio as Istio Cluster
participant Pod as gRPC Proxyless Pod
participant Agent as gRPC Agent
participant XDS as XDS Server
Test->>Istio: start cluster with GrpcProxylessPodCustomizer
Istio->>Pod: create pod
Pod->>Pod: inject grpc-agent template annotation
Istio->>Agent: start lightweight pilot-agent
Agent->>XDS: fetch gRPC bootstrap config
XDS-->>Agent: bootstrap.json
Test->>Pod: wait for pod healthy
Pod->>Pod: check istio-proxy container ready
Pod-->>Test: healthy
Test->>Pod: send request via xDS listener
Pod->>Agent: route through agent
Agent->>Pod: deliver to target
Pod-->>Test: response 200 OK
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
xds/src/main/java/com/linecorp/armeria/xds/TransportSocketSnapshot.java (1)
159-166:⚠️ Potential issue | 🟡 MinorLog message interpolation is broken.
The message uses
"\\{}", which emits a literal backslash followed by{}rather than an SLF4J placeholder — and there are no arguments passed tologger.warnanyway, so the{}would not be substituted regardless. Users will seeSet system_root_certs: \{} to use ...in logs. Drop the placeholder (it isn't parameterized) and clean the string up.Suggested change
- logger.warn("TLS peer verification is disabled because validation context has no trusted CA " + - "and system_root_certs is unset. Set system_root_certs: \\{} to use the " + - "default Java TLS roots."); + logger.warn("TLS peer verification is disabled because the validation context has no trusted " + + "CA and system_root_certs is unset. Set system_root_certs: {} to use the default " + + "Java TLS roots.", "{}");Or, if the intent is just to document the YAML snippet, hard-code it in the string without any placeholder:
- logger.warn("TLS peer verification is disabled because validation context has no trusted CA " + - "and system_root_certs is unset. Set system_root_certs: \\{} to use the " + - "default Java TLS roots."); + logger.warn("TLS peer verification is disabled because the validation context has no trusted " + + "CA and system_root_certs is unset. Set 'system_root_certs: {}' to use the " + + "default Java TLS roots.");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xds/src/main/java/com/linecorp/armeria/xds/TransportSocketSnapshot.java` around lines 159 - 166, The log message in TransportSocketSnapshot.warnNoVerifyOnce wrongly includes an escaped SLF4J placeholder ("\\{}") and no arguments, producing a literal "\{}" in logs; update the logger.warn call in warnNoVerifyOnce (and leave warnedNoVerify handling) to use a plain string without any placeholder so the YAML snippet appears correctly (e.g., "Set system_root_certs: {} to use the default Java TLS roots." with the braces literal, or better hard-code the snippet "Set system_root_certs: true to use the default Java TLS roots." depending on intended wording).
🧹 Nitpick comments (11)
xds/src/main/java/com/linecorp/armeria/xds/TlsCertificateSnapshot.java (1)
76-79:toString()is now effectively empty.After removing the
resourcefield (and droppingomitNullValues()), this returns justTlsCertificateSnapshot{}. Consider including at least a non-verbose identifier (e.g.tlsKeyPairpresence or a resource name) so logs using the defaulttoString()remain useful; the full protobuf already lives intoDebugString().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xds/src/main/java/com/linecorp/armeria/xds/TlsCertificateSnapshot.java` around lines 76 - 79, The current TlsCertificateSnapshot.toString() returns an empty brace; update toString() to include a compact identifier such as whether tlsKeyPair is present and/or a resource name (e.g. add("tlsKeyPairPresent", tlsKeyPair != null) and/or add("resourceName", resourceName)) using MoreObjects.toStringHelper(this) so logs remain useful while keeping the full protobuf output in toDebugString(); locate the toString() method in class TlsCertificateSnapshot and add these non-verbose fields.xds/src/main/java/com/linecorp/armeria/xds/ClusterSnapshot.java (1)
141-142: Render transport socket matches with their debug strings.
transportSocketMatchescurrently falls back toTransportSocketMatchSnapshot.toString(), so the nested transport sockets do not get the new protobuf-focused debug expansion.Proposed fix
.add("transportSocket", transportSocket.toDebugString()) - .add("transportSocketMatches", transportSocketMatches) + .add("transportSocketMatches", + SnapshotUtil.debugStrings(transportSocketMatches, + TransportSocketMatchSnapshot::toDebugString)) .toString();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xds/src/main/java/com/linecorp/armeria/xds/ClusterSnapshot.java` around lines 141 - 142, ClusterSnapshot currently logs transportSocketMatches using its default toString, losing the protobuf-focused debug expansion; update the pretty-printing to map each TransportSocketMatchSnapshot to its debug string (e.g., replace the direct use of transportSocketMatches in the StringBuilder/MoreObjects.add call with a transformed collection produced by transportSocketMatches.stream().map(TransportSocketMatchSnapshot::toDebugString).collect(...)) so nested transport sockets render with their debug strings; locate the code around the .add("transportSocket", transportSocket.toDebugString()) and change the .add("transportSocketMatches", transportSocketMatches) usage to the mapped-toDebugString version.xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/XdsEndpointUtil.java (2)
208-216: VerifyPaths.get(...).toAbsolutePath()behavior for relative pipe paths.When the Envoy
Pipe.pathis absolute (the common/expected case), this is a no-op. When it is relative,toAbsolutePath()resolves against the JVM's working directory, which likely differs from whatever the control plane intended — silently producing a bogus endpoint rather than surfacing a configuration error. Consider either:
- requiring an absolute path and throwing an
IllegalArgumentExceptionwith the offending value if it is relative, or- dropping the
toAbsolutePath()call and lettingDomainSocketAddress.of(path)receive the path verbatim (it already rejects empty paths).If the absolute-path resolution is intentional (e.g., to normalize
./sock), a brief comment would help.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/XdsEndpointUtil.java` around lines 208 - 216, The current use of Paths.get(address.getPipe().getPath()).toAbsolutePath() can silently resolve relative pipe paths against the JVM working dir and produce bogus endpoints; update XdsEndpointUtil so that when address.hasPipe() is true you validate the provided pipe path is absolute and throw an IllegalArgumentException containing the offending value if Paths.get(address.getPipe().getPath()).isAbsolute() is false, then keep using DomainSocketAddress.of(pipePath).asEndpoint().withAttr(...) for the validated absolute path (or add a short comment explaining the deliberate normalization if you prefer to retain toAbsolutePath()).
144-149: Include cluster context in the error message.Other unsupported cases in this file include the cluster name and type (see the
defaultbranch ofconvertEndpointGroup). Mirroring that here makes diagnostics consistent.Suggested change
- if (address.hasPipe()) { - throw new UnsupportedOperationException( - "Pipe addresses are not supported for STRICT_DNS cluster type"); - } + if (address.hasPipe()) { + throw new UnsupportedOperationException( + "Cluster (" + cluster.getName() + ") uses a pipe address with " + + "STRICT_DNS; pipe addresses are only supported with STATIC or EDS."); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/XdsEndpointUtil.java` around lines 144 - 149, The UnsupportedOperationException thrown when an lbEndpoint has a pipe address lacks cluster context; update the exception message inside XdsEndpointUtil (the block handling lbEndpoint.getEndpoint().getAddress() where it checks address.hasPipe()) to include the cluster name and cluster type (same fields used in convertEndpointGroup) so diagnostics match other unsupported-case messages—retrieve the cluster name and type available in the surrounding scope and interpolate them into the error message (e.g., "Pipe addresses are not supported for STRICT_DNS cluster type: cluster=<name> type=<type>").xds/src/main/java/com/linecorp/armeria/xds/TransportSocketSnapshot.java (2)
107-120: Minor: preferImmutableList.toImmutableList()for ALPN filtering.
ImmutableListis already imported and the rest of the Armeria codebase prefers Guava immutable collectors overCollectors.toList()(which returns a mutableArrayList). Also consider short-circuiting whenalpnis empty to avoid the stream allocation on the common no-ALPN case.Suggested change
- if (upstreamTlsContext != null) { - // Armeria doesn't implement "istio-peer-exchange" for now. - final List<String> alpn = upstreamTlsContext.getCommonTlsContext().getAlpnProtocolsList(); - final List<String> filteredAlpn = alpn.stream() - .filter(a -> !istioPeerExchange.equals(a)) - .collect(Collectors.toList()); - specBuilder.alpnProtocols(filteredAlpn); - } + if (upstreamTlsContext != null) { + // Armeria doesn't implement "istio-peer-exchange" for now. + final List<String> alpn = upstreamTlsContext.getCommonTlsContext().getAlpnProtocolsList(); + if (!alpn.isEmpty()) { + final List<String> filteredAlpn = alpn.stream() + .filter(a -> !ISTIO_PEER_EXCHANGE.equals(a)) + .collect(ImmutableList.toImmutableList()); + specBuilder.alpnProtocols(filteredAlpn); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xds/src/main/java/com/linecorp/armeria/xds/TransportSocketSnapshot.java` around lines 107 - 120, In buildClientTlsSpec, avoid Collectors.toList() and unnecessary stream allocation: after obtaining upstreamTlsContext.getCommonTlsContext().getAlpnProtocolsList(), short-circuit if alpn is empty (skip filtering and don't call specBuilder.alpnProtocols), otherwise use ImmutableList.toImmutableList() to collect filtered values (filtering out istioPeerExchange) and pass the resulting immutable list to specBuilder.alpnProtocols(filteredAlpn). This replaces the mutable ArrayList creation with Guava's immutable collector and avoids allocating a stream for the common empty case.
50-50: Constant naming.
istioPeerExchangeisprivate static finaland should follow Java'sUPPER_SNAKE_CASEconvention used throughout this module (e.g.,HTTP_CONNECTION_MANAGER_TYPE_URLinListenerXdsResource). Rename toISTIO_PEER_EXCHANGE.Suggested change
- private static final String istioPeerExchange = "istio-peer-exchange"; + private static final String ISTIO_PEER_EXCHANGE = "istio-peer-exchange";And update the reference in
buildClientTlsSpecaccordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xds/src/main/java/com/linecorp/armeria/xds/TransportSocketSnapshot.java` at line 50, Rename the private static final field istioPeerExchange in TransportSocketSnapshot to follow Java constant naming (ISTIO_PEER_EXCHANGE) and update all references, particularly the usage inside buildClientTlsSpec, to use the new constant name; ensure the field remains static final and update any imports or qualifiers as necessary so compilation and behavior are unchanged.core/src/main/java/com/linecorp/armeria/client/ClientTlsSpec.java (1)
101-117: Minor: missingthis == oshort-circuit inequals.Functionally correct (super.equals handles identity correctly), but the idiomatic pattern in Armeria and across this codebase is to short-circuit on reference equality before doing class and field comparisons.
Suggested change
`@Override` public boolean equals(`@Nullable` Object o) { + if (this == o) { + return true; + } if (o == null || getClass() != o.getClass()) { return false; } if (!super.equals(o)) { return false; } final ClientTlsSpec that = (ClientTlsSpec) o; return Objects.equal(overrideAlpnProtocols, that.overrideAlpnProtocols) && Objects.equal(endpointIdentificationAlgorithm, that.endpointIdentificationAlgorithm); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/linecorp/armeria/client/ClientTlsSpec.java` around lines 101 - 117, The equals implementation in ClientTlsSpec should short-circuit on reference equality; update the equals(`@Nullable` Object o) method in class ClientTlsSpec to first check "if (this == o) return true;" before the null/class checks so identity is handled immediately, then proceed with the existing getClass() check, call to super.equals(o), and the field comparisons for overrideAlpnProtocols and endpointIdentificationAlgorithm; no other logic changes required (ensure hashCode remains unchanged).xds/src/main/java/com/linecorp/armeria/xds/ListenerXdsResource.java (1)
138-152: Consider scanningdefault_filter_chainas well.
extractHcmFromFilterChainsonly iterateslistener.getFilterChainsList()and ignoreslistener.getDefaultFilterChain(). Envoy listeners can carry the HCM on the default filter chain (used when noFilterChainMatchmatches), so a listener that only configures HCM there would end up withconnectionManager == nulland therefore no routing. If this isn't expected to occur in the targeted (Istio sidecar / gRPC proxyless) scenarios, a brief javadoc note clarifying the scope would help.Also note that returning only the first HCM across all filter chains silently drops any per-filter-chain HCM divergence (different RDS/route configs per SNI/transport match). That seems intentional for the proxyless path but is worth documenting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xds/src/main/java/com/linecorp/armeria/xds/ListenerXdsResource.java` around lines 138 - 152, The extractHcmFromFilterChains method currently only inspects listener.getFilterChainsList(), missing listener.getDefaultFilterChain(); update extractHcmFromFilterChains to also check listener.getDefaultFilterChain() for an HttpConnectionManager (same name/type-url checks and unpack via XdsValidatorIndexRegistry.unpack) and return it if found; additionally, add a brief javadoc to extractHcmFromFilterChains explaining that it returns the first HCM encountered (scanning defaultFilterChain then explicit filterChains) and that per-filter-chain HCM divergence (different RDS/routes) will be ignored by this method.core/src/main/java/com/linecorp/armeria/client/ClientTlsSpecBuilder.java (2)
76-80: Consider null-checking individual ALPN entries and rejecting empty strings.
ImmutableSet.copyOfaccepts any non-nullCollectionbut throws NPE only if an element is null — a blank/empty element would silently propagate all the way to the NettyApplicationProtocolConfig. Validating here gives a clearer error surface for a public API.Suggested change
public ClientTlsSpecBuilder alpnProtocols(Collection<String> alpnProtocols) { requireNonNull(alpnProtocols, "alpnProtocols"); - overrideAlpnProtocols = ImmutableSet.copyOf(alpnProtocols); + final ImmutableSet.Builder<String> builder = ImmutableSet.builder(); + for (String p : alpnProtocols) { + requireNonNull(p, "alpnProtocols contains null"); + checkArgument(!p.isEmpty(), "alpnProtocols contains an empty string"); + builder.add(p); + } + overrideAlpnProtocols = builder.build(); return this; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/linecorp/armeria/client/ClientTlsSpecBuilder.java` around lines 76 - 80, The alpnProtocols method currently allows blank/empty strings to be passed through to overrideAlpnProtocols; update ClientTlsSpecBuilder.alpnProtocols to validate each element: after requireNonNull(alpnProtocols), iterate the collection and requireNonNull each protocol string and reject empty or all-whitespace strings (throw IllegalArgumentException with a clear message), then set overrideAlpnProtocols = ImmutableSet.copyOf(alpnProtocols); this ensures null entries and blank ALPN identifiers are rejected at the public API boundary.
92-102: Document and/or validate accepted values forendpointIdentificationAlgorithm.JSSE currently recognizes only
"HTTPS","LDAPS", and""forSSLParameters.setEndpointIdentificationAlgorithm. Any other value will only fail later during SSL handshake. For a public,@UnstableApimethod it'd be helpful to either:
- explicitly enumerate acceptable values (or at least reject obvious garbage like whitespace-only), or
- expand the Javadoc to note that the value is passed through to JSSE verbatim and unsupported values will fail at connect time.
The current doc only mentions
"HTTPS"and"", which may leave users guessing whether e.g."LDAPS"is supported.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/linecorp/armeria/client/ClientTlsSpecBuilder.java` around lines 92 - 102, Update ClientTlsSpecBuilder.endpointIdentificationAlgorithm to either validate or document accepted JSSE values: add input validation that trims and rejects null/blank strings and only allows the known set {"HTTPS","LDAPS",""} (throw IllegalArgumentException for others), or expand the method Javadoc to explicitly enumerate the accepted values and state that the string is passed verbatim to SSLParameters.setEndpointIdentificationAlgorithm and unsupported values will fail at connect time; reference the endpointIdentificationAlgorithm field, the endpointIdentificationAlgorithm(String) setter, and SSLParameters.setEndpointIdentificationAlgorithm in the doc or validation error message so callers can locate and understand the behavior.it/xds-istio/src/test/java/com/linecorp/armeria/it/xds/XdsClientToSidecarTest.java (1)
222-235: Remove the leftover fixed sleep.
Thread.sleep(10_000)runs after the Awaitility assertions have already passed, so it only slows the Istio integration suite by 10 seconds per run.Proposed fix
}); logger.info("Outbound listener snapshot loaded: {}", snapshotRef.get()); - Thread.sleep(10_000); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@it/xds-istio/src/test/java/com/linecorp/armeria/it/xds/XdsClientToSidecarTest.java` around lines 222 - 235, A leftover Thread.sleep(10_000) after the Awaitility assertions is causing an unnecessary 10s delay; remove the Thread.sleep(10_000) call in XdsClientToSidecarTest so the test proceeds immediately after the await().untilAsserted block finishes (look for the await block that checks errorRef/get, snapshotRef/get and asserts snapshot.routeSnapshot().xdsResource().resource().getName() contains server.serviceName(), and delete the Thread.sleep invocation that follows it).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/src/main/java/com/linecorp/armeria/common/AbstractTlsSpec.java`:
- Around line 90-91: AbstractTlsSpec exposes an overridable alpnProtocols()
accessor but equals() and hashCode() read the alpnProtocols field directly,
causing mismatches when subclasses (e.g., ClientTlsSpec) override ALPN; update
equals() and hashCode() in AbstractTlsSpec to consult the public alpnProtocols()
accessor (and normalize the returned collection consistently, e.g., treat
null/empty the same and compare as a Set) instead of referencing the private
field, ensuring both methods reflect the overridable public state.
In `@it/xds-istio/build.gradle`:
- Around line 89-92: The task that currently includes
from(sourceSets.test.output) (the block that also dependsOn
tasks.named('copyShadedTestClasses') and uses
configurations.shadedJarTestRuntime) must use the shaded test-class output
produced by the copyShadedTestClasses task instead of the original test classes;
update that task (the one containing dependsOn
tasks.named('copyShadedTestClasses')) to replace from(sourceSets.test.output)
with the output files of tasks.named('copyShadedTestClasses') (e.g., use the
task's outputs.files or destination directory exposed by copyShadedTestClasses)
so the copied runtime uses relocated/shaded test classes rather than
sourceSets.test.output.
In
`@it/xds-istio/src/test/java/com/linecorp/armeria/it/xds/XdsResourceReader.java`:
- Around line 1-15: The file-level copyright header in XdsResourceReader.java
uses "LINE Corporation" but the project standard and other new files use "LY
Corporation"; update the header comment at the top of XdsResourceReader.java to
replace "LINE Corporation" with "LY Corporation" so it matches the required
project header (ensure the rest of the Apache 2.0 boilerplate remains
unchanged).
- Around line 43-95: The class XdsResourceReader is unnecessarily public for
test-only use; remove the public modifier from the class declaration (change
"public final class XdsResourceReader" to "final class XdsResourceReader") so it
becomes package-private. Leave the existing public static helper methods
(fromYaml, fromYaml<T>, fromJson) as-is; if instead you prefer to keep the class
public, add appropriate Javadoc on the class and its public methods
(XdsResourceReader, fromYaml, fromYaml(Class<T>), fromJson) describing their
test-only purpose—prefer the package-private change per the review.
In
`@xds/src/main/java/com/linecorp/armeria/xds/CertificateValidationContextSnapshot.java`:
- Around line 160-163: The current toString() in
CertificateValidationContextSnapshot now returns an empty object and loses
helpful fields; restore a concise summary (e.g., include the resource identifier
or a short summary field) in toString() while keeping the full, verbose output
in toDebugString(). Update CertificateValidationContextSnapshot.toString() to
use MoreObjects.toStringHelper(this).add("resource", resourceSummary) (or
another small summary field) instead of the empty helper, and keep the detailed
fields only in toDebugString() so logs using toString() remain informative
without exposing verbose content.
In `@xds/src/main/java/com/linecorp/armeria/xds/VirtualHostSnapshot.java`:
- Around line 90-97: The new public debug API method toDebugString() in
VirtualHostSnapshot must be annotated with `@UnstableApi`; add the `@UnstableApi`
annotation to the toDebugString() method (or the enclosing VirtualHostSnapshot
class if preferred) and add the corresponding import for the annotation so the
new public API complies with the project's unstable-API coding guideline.
---
Outside diff comments:
In `@xds/src/main/java/com/linecorp/armeria/xds/TransportSocketSnapshot.java`:
- Around line 159-166: The log message in
TransportSocketSnapshot.warnNoVerifyOnce wrongly includes an escaped SLF4J
placeholder ("\\{}") and no arguments, producing a literal "\{}" in logs; update
the logger.warn call in warnNoVerifyOnce (and leave warnedNoVerify handling) to
use a plain string without any placeholder so the YAML snippet appears correctly
(e.g., "Set system_root_certs: {} to use the default Java TLS roots." with the
braces literal, or better hard-code the snippet "Set system_root_certs: true to
use the default Java TLS roots." depending on intended wording).
---
Nitpick comments:
In `@core/src/main/java/com/linecorp/armeria/client/ClientTlsSpec.java`:
- Around line 101-117: The equals implementation in ClientTlsSpec should
short-circuit on reference equality; update the equals(`@Nullable` Object o)
method in class ClientTlsSpec to first check "if (this == o) return true;"
before the null/class checks so identity is handled immediately, then proceed
with the existing getClass() check, call to super.equals(o), and the field
comparisons for overrideAlpnProtocols and endpointIdentificationAlgorithm; no
other logic changes required (ensure hashCode remains unchanged).
In `@core/src/main/java/com/linecorp/armeria/client/ClientTlsSpecBuilder.java`:
- Around line 76-80: The alpnProtocols method currently allows blank/empty
strings to be passed through to overrideAlpnProtocols; update
ClientTlsSpecBuilder.alpnProtocols to validate each element: after
requireNonNull(alpnProtocols), iterate the collection and requireNonNull each
protocol string and reject empty or all-whitespace strings (throw
IllegalArgumentException with a clear message), then set overrideAlpnProtocols =
ImmutableSet.copyOf(alpnProtocols); this ensures null entries and blank ALPN
identifiers are rejected at the public API boundary.
- Around line 92-102: Update
ClientTlsSpecBuilder.endpointIdentificationAlgorithm to either validate or
document accepted JSSE values: add input validation that trims and rejects
null/blank strings and only allows the known set {"HTTPS","LDAPS",""} (throw
IllegalArgumentException for others), or expand the method Javadoc to explicitly
enumerate the accepted values and state that the string is passed verbatim to
SSLParameters.setEndpointIdentificationAlgorithm and unsupported values will
fail at connect time; reference the endpointIdentificationAlgorithm field, the
endpointIdentificationAlgorithm(String) setter, and
SSLParameters.setEndpointIdentificationAlgorithm in the doc or validation error
message so callers can locate and understand the behavior.
In
`@it/xds-istio/src/test/java/com/linecorp/armeria/it/xds/XdsClientToSidecarTest.java`:
- Around line 222-235: A leftover Thread.sleep(10_000) after the Awaitility
assertions is causing an unnecessary 10s delay; remove the Thread.sleep(10_000)
call in XdsClientToSidecarTest so the test proceeds immediately after the
await().untilAsserted block finishes (look for the await block that checks
errorRef/get, snapshotRef/get and asserts
snapshot.routeSnapshot().xdsResource().resource().getName() contains
server.serviceName(), and delete the Thread.sleep invocation that follows it).
In
`@xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/XdsEndpointUtil.java`:
- Around line 208-216: The current use of
Paths.get(address.getPipe().getPath()).toAbsolutePath() can silently resolve
relative pipe paths against the JVM working dir and produce bogus endpoints;
update XdsEndpointUtil so that when address.hasPipe() is true you validate the
provided pipe path is absolute and throw an IllegalArgumentException containing
the offending value if Paths.get(address.getPipe().getPath()).isAbsolute() is
false, then keep using
DomainSocketAddress.of(pipePath).asEndpoint().withAttr(...) for the validated
absolute path (or add a short comment explaining the deliberate normalization if
you prefer to retain toAbsolutePath()).
- Around line 144-149: The UnsupportedOperationException thrown when an
lbEndpoint has a pipe address lacks cluster context; update the exception
message inside XdsEndpointUtil (the block handling
lbEndpoint.getEndpoint().getAddress() where it checks address.hasPipe()) to
include the cluster name and cluster type (same fields used in
convertEndpointGroup) so diagnostics match other unsupported-case
messages—retrieve the cluster name and type available in the surrounding scope
and interpolate them into the error message (e.g., "Pipe addresses are not
supported for STRICT_DNS cluster type: cluster=<name> type=<type>").
In `@xds/src/main/java/com/linecorp/armeria/xds/ClusterSnapshot.java`:
- Around line 141-142: ClusterSnapshot currently logs transportSocketMatches
using its default toString, losing the protobuf-focused debug expansion; update
the pretty-printing to map each TransportSocketMatchSnapshot to its debug string
(e.g., replace the direct use of transportSocketMatches in the
StringBuilder/MoreObjects.add call with a transformed collection produced by
transportSocketMatches.stream().map(TransportSocketMatchSnapshot::toDebugString).collect(...))
so nested transport sockets render with their debug strings; locate the code
around the .add("transportSocket", transportSocket.toDebugString()) and change
the .add("transportSocketMatches", transportSocketMatches) usage to the
mapped-toDebugString version.
In `@xds/src/main/java/com/linecorp/armeria/xds/ListenerXdsResource.java`:
- Around line 138-152: The extractHcmFromFilterChains method currently only
inspects listener.getFilterChainsList(), missing
listener.getDefaultFilterChain(); update extractHcmFromFilterChains to also
check listener.getDefaultFilterChain() for an HttpConnectionManager (same
name/type-url checks and unpack via XdsValidatorIndexRegistry.unpack) and return
it if found; additionally, add a brief javadoc to extractHcmFromFilterChains
explaining that it returns the first HCM encountered (scanning
defaultFilterChain then explicit filterChains) and that per-filter-chain HCM
divergence (different RDS/routes) will be ignored by this method.
In `@xds/src/main/java/com/linecorp/armeria/xds/TlsCertificateSnapshot.java`:
- Around line 76-79: The current TlsCertificateSnapshot.toString() returns an
empty brace; update toString() to include a compact identifier such as whether
tlsKeyPair is present and/or a resource name (e.g. add("tlsKeyPairPresent",
tlsKeyPair != null) and/or add("resourceName", resourceName)) using
MoreObjects.toStringHelper(this) so logs remain useful while keeping the full
protobuf output in toDebugString(); locate the toString() method in class
TlsCertificateSnapshot and add these non-verbose fields.
In `@xds/src/main/java/com/linecorp/armeria/xds/TransportSocketSnapshot.java`:
- Around line 107-120: In buildClientTlsSpec, avoid Collectors.toList() and
unnecessary stream allocation: after obtaining
upstreamTlsContext.getCommonTlsContext().getAlpnProtocolsList(), short-circuit
if alpn is empty (skip filtering and don't call specBuilder.alpnProtocols),
otherwise use ImmutableList.toImmutableList() to collect filtered values
(filtering out istioPeerExchange) and pass the resulting immutable list to
specBuilder.alpnProtocols(filteredAlpn). This replaces the mutable ArrayList
creation with Guava's immutable collector and avoids allocating a stream for the
common empty case.
- Line 50: Rename the private static final field istioPeerExchange in
TransportSocketSnapshot to follow Java constant naming (ISTIO_PEER_EXCHANGE) and
update all references, particularly the usage inside buildClientTlsSpec, to use
the new constant name; ensure the field remains static final and update any
imports or qualifiers as necessary so compilation and behavior are unchanged.
🪄 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: e231dc35-c87d-46db-a5e7-77673459b1f6
📒 Files selected for processing (40)
core/src/main/java/com/linecorp/armeria/client/ClientTlsSpec.javacore/src/main/java/com/linecorp/armeria/client/ClientTlsSpecBuilder.javacore/src/main/java/com/linecorp/armeria/common/AbstractTlsSpec.javacore/src/main/java/com/linecorp/armeria/internal/common/util/SslContextUtil.javadependencies.tomlit/xds-client/build.gradleit/xds-client/src/main/java/com/linecorp/armeria/xds/it/XdsResourceReader.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/PipeEndpointTest.javait/xds-istio/build.gradleit/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/GrpcProxylessPodCustomizer.javait/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioPodCustomizer.javait/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/K8sClusterHelper.javait/xds-istio/src/test/java/com/linecorp/armeria/it/xds/EnvoyDebugTest.javait/xds-istio/src/test/java/com/linecorp/armeria/it/xds/NoopXdsValidatorIndex.javait/xds-istio/src/test/java/com/linecorp/armeria/it/xds/XdsClientToSidecarTest.javait/xds-istio/src/test/java/com/linecorp/armeria/it/xds/XdsGrpcProxylessTest.javait/xds-istio/src/test/java/com/linecorp/armeria/it/xds/XdsNativeGrpcProxylessTest.javait/xds-istio/src/test/java/com/linecorp/armeria/it/xds/XdsResourceReader.javait/xds-istio/src/test/java/com/linecorp/armeria/it/xds/filter/IstioNoOpFilterFactories.javait/xds-istio/src/test/resources/META-INF/services/com.linecorp.armeria.xds.filter.HttpFilterFactoryit/xds-istio/src/test/resources/META-INF/services/com.linecorp.armeria.xds.validator.XdsValidatorIndexsettings.gradlexds-api/build.gradlexds/build.gradlexds/src/main/java/com/linecorp/armeria/xds/CertificateValidationContextSnapshot.javaxds/src/main/java/com/linecorp/armeria/xds/ClusterSnapshot.javaxds/src/main/java/com/linecorp/armeria/xds/EndpointSnapshot.javaxds/src/main/java/com/linecorp/armeria/xds/ListenerSnapshot.javaxds/src/main/java/com/linecorp/armeria/xds/ListenerXdsResource.javaxds/src/main/java/com/linecorp/armeria/xds/RouteEntry.javaxds/src/main/java/com/linecorp/armeria/xds/RouteSnapshot.javaxds/src/main/java/com/linecorp/armeria/xds/Snapshot.javaxds/src/main/java/com/linecorp/armeria/xds/SnapshotUtil.javaxds/src/main/java/com/linecorp/armeria/xds/TlsCertificateSnapshot.javaxds/src/main/java/com/linecorp/armeria/xds/TransportSocketMatchSnapshot.javaxds/src/main/java/com/linecorp/armeria/xds/TransportSocketSnapshot.javaxds/src/main/java/com/linecorp/armeria/xds/TransportSocketStream.javaxds/src/main/java/com/linecorp/armeria/xds/VirtualHostSnapshot.javaxds/src/main/java/com/linecorp/armeria/xds/client/endpoint/XdsEndpointUtil.javaxds/src/main/java/com/linecorp/armeria/xds/client/endpoint/XdsHealthCheckedEndpointGroupBuilder.java
💤 Files with no reviewable changes (3)
- xds-api/build.gradle
- xds/build.gradle
- it/xds-client/build.gradle
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6730 +/- ##
============================================
- Coverage 74.46% 73.66% -0.81%
- Complexity 22234 24293 +2059
============================================
Files 1963 2194 +231
Lines 82437 91561 +9124
Branches 10764 11960 +1196
============================================
+ Hits 61385 67444 +6059
- Misses 15918 18415 +2497
- Partials 5134 5702 +568 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/src/main/java/com/linecorp/armeria/common/AbstractTlsSpec.java (1)
90-91: KeeptoString()aligned with overridable ALPN.Now that
alpnProtocols()can be overridden, Line 184 still prints the backing field instead of the effective ALPN set. This can make diagnostics misleading for subclasses that override ALPN.Proposed diagnostic consistency fix
.add("tlsVersions", tlsVersions) - .add("alpnProtocols", alpnProtocols) + .add("alpnProtocols", alpnProtocols()) .add("ciphers", ciphers)Also applies to: 181-185
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/linecorp/armeria/common/AbstractTlsSpec.java` around lines 90 - 91, The toString() implementation prints the backing field instead of the overridable accessor; update toString() to call the alpnProtocols() method (not the alpnProtocols field) so subclasses that override alpnProtocols() are represented correctly—locate the toString() method in AbstractTlsSpec and replace uses of the alpnProtocols field with a call to the alpnProtocols() accessor (ensure any formatting/joins still operate on the returned Set).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@core/src/main/java/com/linecorp/armeria/common/AbstractTlsSpec.java`:
- Around line 90-91: The toString() implementation prints the backing field
instead of the overridable accessor; update toString() to call the
alpnProtocols() method (not the alpnProtocols field) so subclasses that override
alpnProtocols() are represented correctly—locate the toString() method in
AbstractTlsSpec and replace uses of the alpnProtocols field with a call to the
alpnProtocols() accessor (ensure any formatting/joins still operate on the
returned Set).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3e5f7910-0bc2-4feb-89df-7ce193ef28df
📒 Files selected for processing (7)
core/src/main/java/com/linecorp/armeria/client/ClientTlsSpec.javacore/src/main/java/com/linecorp/armeria/common/AbstractTlsSpec.javait/xds-istio/build.gradleit/xds-istio/src/test/java/com/linecorp/armeria/it/xds/XdsClientToSidecarTest.javait/xds-istio/src/test/java/com/linecorp/armeria/it/xds/XdsResourceReader.javaxds/src/main/java/com/linecorp/armeria/xds/ClusterSnapshot.javaxds/src/main/java/com/linecorp/armeria/xds/TransportSocketSnapshot.java
✅ Files skipped from review due to trivial changes (1)
- xds/src/main/java/com/linecorp/armeria/xds/TransportSocketSnapshot.java
🚧 Files skipped from review as they are similar to previous changes (4)
- xds/src/main/java/com/linecorp/armeria/xds/ClusterSnapshot.java
- it/xds-istio/build.gradle
- core/src/main/java/com/linecorp/armeria/client/ClientTlsSpec.java
- it/xds-istio/src/test/java/com/linecorp/armeria/it/xds/XdsClientToSidecarTest.java
Contributor
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
xds/src/main/java/com/linecorp/armeria/xds/TransportSocketSnapshot.java (1)
105-116: Minor: defaultendpointIdentificationAlgorithm("")is set unconditionally before the TLS-context branch.The builder is initialized with
endpointIdentificationAlgorithm("")(disabling hostname verification) and only re-enabled to"HTTPS"in thesystemRootCertsbranch. For thetrustedCa != nullpath — which is the common SDS-provisioned case — this silently disables hostname verification even when a proper trust anchor is configured. If that's the intended behavior for xDS transport sockets (where SANs are matched viapeerVerifierFactoriesderived frommatch_typed_subject_alt_names), a brief comment explaining the rationale would help future readers; otherwise consider enabling"HTTPS"whenever SAN matching isn't expressed via verifier factories.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xds/src/main/java/com/linecorp/armeria/xds/TransportSocketSnapshot.java` around lines 105 - 116, The ClientTlsSpec builder currently disables hostname verification unconditionally by calling endpointIdentificationAlgorithm("") at initialization in TransportSocketSnapshot.buildClientTlsSpec; change this so hostname verification is enabled by default (set endpointIdentificationAlgorithm("HTTPS") on the initial ClientTlsSpecBuilder) and only override to "" in the specific branch where you intentionally want to disable it (e.g., the systemRootCerts or other branch that explicitly derives peer verifier factories), and ensure the trustedCa path does not leave endpointIdentificationAlgorithm("") unintentionally; update the logic around ClientTlsSpec.builder(), endpointIdentificationAlgorithm("..."), and the branches that handle systemRootCerts, trustedCa, and validationContext so the default is "HTTPS" unless a branch explicitly requires disabling it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@it/xds-istio/src/test/java/com/linecorp/armeria/it/xds/filter/IstioFilterFactories.java`:
- Around line 30-35: Update the class Javadoc for IstioFilterFactories to remove
the claim that all implementations are no-op and to document that the IstioAlpn
factory returns a real HttpFilter which mutates request context state; keep note
that the other factories still return null from create() and are registered via
SPI for HttpFilterFactoryRegistry, and briefly describe the behavior difference
for IstioAlpn (i.e., it produces a filter that alters request context rather
than being silently skipped).
- Around line 69-77: The AlpnFilter currently only sets the ALPN override in
httpPreprocessor, so implement rpcPreprocessor() in the same AlpnFilter class to
mirror that behavior: in rpcPreprocessor() set
ctx.setAttr(XdsCommonUtil.ALPN_OVERRIDE_KEY, ImmutableSet.of("istio-h2",
"istio", "h2")) and then call and return delegate.execute(ctx, req) (use the
same delegate parameter names as in httpPreprocessor); ensure the method
signature matches XdsHttpFilter.rpcPreprocessor so gRPC/proxyless paths receive
the same istio.alpn override.
- Around line 102-120: The three classes EnvoyGzipCompressor,
EnvoyZstdCompressor, and EnvoyBrotliCompressor return non-canonical filter names
and must be consolidated: replace them with a single EnvoyCompressor class
(extending Base) whose filterName() returns "envoy.filters.http.compressor", and
update the SPI/service registration entry that currently lists the three classes
to reference the new EnvoyCompressor instead; ensure no other code expects the
old per-algorithm class names.
---
Nitpick comments:
In `@xds/src/main/java/com/linecorp/armeria/xds/TransportSocketSnapshot.java`:
- Around line 105-116: The ClientTlsSpec builder currently disables hostname
verification unconditionally by calling endpointIdentificationAlgorithm("") at
initialization in TransportSocketSnapshot.buildClientTlsSpec; change this so
hostname verification is enabled by default (set
endpointIdentificationAlgorithm("HTTPS") on the initial ClientTlsSpecBuilder)
and only override to "" in the specific branch where you intentionally want to
disable it (e.g., the systemRootCerts or other branch that explicitly derives
peer verifier factories), and ensure the trustedCa path does not leave
endpointIdentificationAlgorithm("") unintentionally; update the logic around
ClientTlsSpec.builder(), endpointIdentificationAlgorithm("..."), and the
branches that handle systemRootCerts, trustedCa, and validationContext so the
default is "HTTPS" unless a branch explicitly requires disabling it.
🪄 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: d0b69fe5-8af6-430f-a84d-f51efea3c86c
📒 Files selected for processing (6)
it/xds-istio/src/test/java/com/linecorp/armeria/it/xds/XdsClientToSidecarTest.javait/xds-istio/src/test/java/com/linecorp/armeria/it/xds/filter/IstioFilterFactories.javait/xds-istio/src/test/resources/META-INF/services/com.linecorp.armeria.xds.filter.HttpFilterFactoryxds/src/main/java/com/linecorp/armeria/xds/ListenerXdsResource.javaxds/src/main/java/com/linecorp/armeria/xds/TransportSocketSnapshot.javaxds/src/main/java/com/linecorp/armeria/xds/internal/XdsCommonUtil.java
🚧 Files skipped from review as they are similar to previous changes (2)
- it/xds-istio/src/test/resources/META-INF/services/com.linecorp.armeria.xds.filter.HttpFilterFactory
- xds/src/main/java/com/linecorp/armeria/xds/ListenerXdsResource.java
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.
This PR should be reviewed after #6708
Motivation:
Armeria's xDS client could not communicate with a real Istio control plane end-to-end. Several gaps prevented this:
ClientTlsSpecfrom xDSUpstreamTlsContext, Armeria ignored the ALPN protocols specified in the xDS config and always enforced JSSE hostname verification (HTTPSalgorithm). Updated so that Istio's alpn filter is mirrored, and verifies peer identity via SPIFFE rather than hostname, so JSSE verification must be disabled for mTLS to succeed.ListenerXdsResourceonly extractedHttpConnectionManagerfromapi_listener, ignoring non-API listeners that carry HCM insidefilter_chains. Istio's sidecar-generated listeners use filter chains exclusively.it/xds-istiomodule only tested basic cluster startup and Envoy sidecar reachability, but never exercised Armeria's xDS client against Istiod.Modifications:
Core (
core)overrideAlpnProtocolsandendpointIdentificationAlgorithmtoClientTlsSpec/ClientTlsSpecBuilder, allowing xDS-derived TLS configs to override session-protocol-based ALPN and disable JSSE hostname verification.AbstractTlsSpec.alpnProtocols()non-final soClientTlsSpeccan override it.clientTlsSpec.endpointIdentificationAlgorithm()inSslContextUtilinstead of hardcoded"HTTPS".xDS (
xds)TransportSocketSnapshotreceives theUpstreamTlsContextand uses its ALPN list (filtering outistio-peer-exchange). Endpoint identification algorithm defaults to""(disabled) for xDS transport sockets, re-enabled to"HTTPS"only when system root CAs are used.ListenerXdsResource.extractHcmFromFilterChains()scans filter chains forenvoy.filters.network.http_connection_managerto support non-API listeners.toDebugString()to theSnapshotinterface and all implementations for protobuf-level introspection.protoc-pgv-java-stubdependency fromxdsandxds-api.Istio integration tests (
it/xds-istio)XdsClientToSidecarTest— bootstraps Armeria's xDS client from the Istio-generated bootstrap, loads cluster/listener snapshots, and makes HTTP requests routed via xDS through the Envoy sidecar.XdsGrpcProxylessTest— tests Armeria's xDS client in Istio gRPC proxyless mode (grpc-agenttemplate, no Envoy).XdsNativeGrpcProxylessTest— tests native gRPC proxyless mode viagrpc-javafor comparison.GrpcProxylessPodCustomizer,NoopXdsValidatorIndex, andIstioNoOpFilterFactoriesas supporting infrastructure for the new tests.IstioPodCustomizerto avoid a hot-loop caused by Istio's wildcard CDS/LDS behavior.ArmeriaHttpClientFactorywith retry for improved reliability.Result:
ClientTlsSpecgainsoverrideAlpnProtocols()andendpointIdentificationAlgorithm()for fine-grained TLS control needed by xDS-based mTLS.