Conversation
📝 WalkthroughWalkthroughAdds transport-socket matching and per-endpoint TLS wiring, implements TLS peer verification (SPKI/hash pinning and SAN matching), threads transport-socket snapshots through endpoint/load-balancer wiring, removes GrpcClientBuilder customization hooks, and adds extensive unit and integration tests for TLS/transport-socket behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RouterFilter
participant XdsCommonUtil
participant Endpoint
participant TransportSocketSnapshot
participant HttpClient
Client->>RouterFilter: execute(ctx, req)
RouterFilter->>RouterFilter: select endpoint from group
RouterFilter->>Endpoint: getAttribute(TRANSPORT_SOCKET_SNAPSHOT_KEY)
Endpoint-->>RouterFilter: TransportSocketSnapshot
RouterFilter->>XdsCommonUtil: setTlsParams(ctx, endpoint)
XdsCommonUtil->>TransportSocketSnapshot: clientTlsSpec()
TransportSocketSnapshot-->>XdsCommonUtil: ClientTlsSpec
XdsCommonUtil->>ctx: setSessionProtocol(HTTPS) / setTlsSpec(ClientTlsSpec)
RouterFilter->>HttpClient: delegate(ctx, req)
HttpClient->>Client: perform TLS handshake using verifier factories (pinned/SAN)
HttpClient-->>Client: response
sequenceDiagram
participant LoadBalancer
participant TransportSocketMatchUtil
participant LbEndpoint
participant TransportSocketMatch
participant EndpointGroup
LoadBalancer->>TransportSocketMatchUtil: selectTransportSocket(socket, matches, lbEndpoint, locality)
TransportSocketMatchUtil->>LbEndpoint: extract endpoint metadata (envoy.transport_socket_match)
TransportSocketMatchUtil->>TransportSocketMatch: evaluate matches sequentially
alt Endpoint match found
TransportSocketMatchUtil->>LoadBalancer: return matched socket
else No endpoint match and locality present
TransportSocketMatchUtil->>LbEndpoint: extract locality metadata
TransportSocketMatchUtil->>TransportSocketMatch: evaluate locality matches
alt Locality match found
TransportSocketMatchUtil->>LoadBalancer: return locality matched socket
else No match
TransportSocketMatchUtil->>LoadBalancer: return default socket
end
end
LoadBalancer->>EndpointGroup: attach TRANSPORT_SOCKET_SNAPSHOT_KEY to endpoints
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In
`@xds/src/main/java/com/linecorp/armeria/xds/CertificateValidationContextSnapshot.java`:
- Around line 103-112: The decodeSpkiPins method currently calls
Base64.getDecoder().decode(pin) which can throw IllegalArgumentException for
malformed input; modify decodeSpkiPins to catch IllegalArgumentException around
each decode call (in the loop over pins), and rethrow a new
IllegalArgumentException (or IllegalStateException if preferred) that includes
the offending pin value and the original exception as the cause so snapshot
construction fails with a clear, contextual message; keep returning
ImmutableList.copyOf(decoded) on success and preserve behavior when pins is
empty.
In
`@xds/src/main/java/com/linecorp/armeria/xds/CertificateValidationContextStream.java`:
- Around line 55-66: The InputStream returned by bs.get().newInput() is never
closed, causing file descriptor leaks in CertificateValidationContextStream;
wrap the stream in a try-with-resources when calling
CertificateUtil.toX509Certificates(...) so the stream is always closed, e.g.
obtain the InputStream into a local variable (e.g. "in"), call
CertificateUtil.toX509Certificates(in) inside the try-with-resources, and adjust
the catch to handle the same exceptions (CertificateException and any
IOExceptions) and return Exceptions.throwUnsafely(...) on error; keep returning
new CertificateValidationContextSnapshot(validationContext, caCerts) on success.
In `@xds/src/main/java/com/linecorp/armeria/xds/ClusterSnapshot.java`:
- Around line 126-130: The toString representation in ClusterSnapshot uses the
label "tlsSnapshot" for the field transportSocket which is inconsistent with the
field name and other methods; update the toString builder to use
"transportSocket" instead of "tlsSnapshot" so it matches the transportSocket
field, its accessor, equals and hashCode and avoids confusion when logging or
debugging.
In `@xds/src/main/java/com/linecorp/armeria/xds/CombineLatest3Stream.java`:
- Around line 61-81: The dispose logic in CombineLatest3Stream's onStart sets
subA/subB/subC to null after calling close(), but callbacks onA/onB/onC can
still run and mutate latestA/latestB/latestC and call maybeEmit; add a volatile
boolean closed (or AtomicBoolean) field in the CombineLatest3Stream class, set
closed = true in the returned Subscription lambda (before/after closing the
subs) and have onA/onB/onC check and return immediately if closed is true,
preventing stale emissions; ensure the same guard is applied in
CombineLatest2Stream if present and reference the onStart method,
subA/subB/subC, onA/onB/onC, maybeEmit, and latestA/latestB/latestC when making
the change.
In `@xds/src/main/java/com/linecorp/armeria/xds/PinnedPeerVerifierFactory.java`:
- Around line 100-116: The equals/hashCode implementations in
PinnedPeerVerifierFactory incorrectly rely on List<byte[]>.equals()/hashCode()
(which uses identity for byte[]); update them to compare byte[] contents and
compute hashes by content: in equals(Object) iterate over spkiPins and
certHashPins and use Arrays.equals(byte[], byte[]) for each element (or compare
lists of ByteBuffer/ByteString wrappers created in the constructor), and in
hashCode() compute a combined hash by applying Arrays.hashCode(byte[]) per
element and combining those (or rely on the wrapper lists' hashCode). Ensure you
reference and update the existing methods equals and hashCode and use the fields
spkiPins and certHashPins when performing content-based comparisons/hashing.
In
`@xds/src/main/java/com/linecorp/armeria/xds/TransportSocketMatchSnapshot.java`:
- Line 26: The new public class TransportSocketMatchSnapshot is missing the
required `@UnstableApi` annotation; add the annotation to the
TransportSocketMatchSnapshot class declaration and import the annotation
(com.linecorp.armeria.common.annotation.UnstableApi) so the public API is marked
unstable per guidelines; update the class declaration for
TransportSocketMatchSnapshot to include `@UnstableApi` above the class and ensure
imports are correct.
🧹 Nitpick comments (24)
xds/src/main/java/com/linecorp/armeria/xds/VirtualHostSnapshot.java (1)
49-56: Nit: private constructor doesn't enforce immutability onrouteEntries.The package-private constructor (line 41) now wraps
routeEntriesinImmutableList.copyOf, but this private constructor assigns it directly. Currently safe since the only caller (withFilterConfigs, line 84) already produces anImmutableList, but wrapping here too would make the invariant self-documenting.Suggested diff
private VirtualHostSnapshot(VirtualHostXdsResource virtualHostXdsResource, List<RouteEntry> routeEntries, Map<String, ParsedFilterConfig> filterConfigs, int index) { this.virtualHostXdsResource = virtualHostXdsResource; - this.routeEntries = routeEntries; + this.routeEntries = ImmutableList.copyOf(routeEntries); this.filterConfigs = filterConfigs; this.index = index; }it/xds-client/src/test/java/com/linecorp/armeria/xds/it/XdsResourceReader.java (1)
68-77: Nit: consider moving themultiLineEscaperfield to the top of the class with the other static fields.The other static fields (
mapper,parser) are declared at the top (lines 36-42). PlacingmultiLineEscaperalongside them keeps a consistent field-then-method ordering.xds/src/main/java/com/linecorp/armeria/xds/TlsCertificateSnapshot.java (1)
36-39: MissingrequireNonNullfor theresourceparameter.The
resourcefield is always expected to be non-null (it's returned fromxdsResource()). Adding a null-check in the constructor would be consistent with defensive coding practices used elsewhere in Armeria.Proposed fix
+import static java.util.Objects.requireNonNull; + TlsCertificateSnapshot(TlsCertificate resource, `@Nullable` TlsKeyPair tlsKeyPair) { - this.resource = resource; + this.resource = requireNonNull(resource, "resource"); this.tlsKeyPair = tlsKeyPair; }it/xds-client/src/test/java/com/linecorp/armeria/xds/it/TlsPeerVerificationIntegrationTest.java (1)
351-368: Consider extracting shared test utilities forspkiPin,certHash, andsha256.These helper methods (
spkiPin,certHash,sha256) are duplicated identically inTransportSocketSnapshotTest.java. Extracting them into a shared test utility class would reduce duplication across the TLS test suite.Also applies to: 467-484
it/xds-client/src/test/java/com/linecorp/armeria/xds/it/TlsValidationContextSdsIntegrationTest.java (1)
249-261: Creating a newClientFactoryper fetch call is resource-intensive.Each call to
fetch()creates and closes a newClientFactory, which is expensive (new event loop group, SSL context, etc.). Consider creating a singleClientFactorywithtlsNoVerify()or a short-lived one at the test level to avoid excessive resource churn, especially invalidationContextRotationUpdatesTrustwherefetchis called in anawait()retry loop.That said, the intent to avoid connection reuse is clear and this is acceptable for integration tests.
xds/src/main/java/com/linecorp/armeria/xds/DataSourceStream.java (2)
58-61: Unnecessary intermediate copy forinlineBytes.
dataSource.getInlineBytes()already returns aByteString. Converting tobyte[]and back is a redundant copy.Proposed fix
if (dataSource.hasInlineBytes()) { - final byte[] bytes = dataSource.getInlineBytes().toByteArray(); - return SnapshotStream.just(Optional.of(ByteString.copyFrom(bytes))).subscribe(watcher); + return SnapshotStream.just(Optional.of(dataSource.getInlineBytes())).subscribe(watcher); }
62-65: Minor:ByteString.copyFromUtf8is more concise here.Proposed fix
if (dataSource.hasInlineString()) { - final byte[] bytes = dataSource.getInlineString().getBytes(StandardCharsets.UTF_8); - return SnapshotStream.just(Optional.of(ByteString.copyFrom(bytes))).subscribe(watcher); + return SnapshotStream.just(Optional.of(ByteString.copyFromUtf8(dataSource.getInlineString()))) + .subscribe(watcher); }it/xds-client/src/test/java/com/linecorp/armeria/xds/it/TransportSocketSnapshotTest.java (2)
57-57: Missingfinalmodifier on thecertificatefield.Other test files in this PR (e.g.,
TlsPeerVerificationIntegrationTest,TlsValidationContextSdsIntegrationTest) declare theirSelfSignedCertificateExtensionfields asstatic final. This should be consistent.Proposed fix
- static SelfSignedCertificateExtension certificate = new SelfSignedCertificateExtension(); + static final SelfSignedCertificateExtension certificate = new SelfSignedCertificateExtension();
381-421: String-based class name assertions are fragile.Lines 418-419 and 452 assert verifier types by fully-qualified class name strings. If the classes are renamed or relocated, these assertions will silently pass the compilation but fail at runtime. Consider using
isInstanceOfor comparing againstClassobjects directly.Example using Class references
assertThat(clientTlsSpec.verifierFactories()) .hasSize(2) - .extracting(factory -> factory.getClass().getName()) - .containsExactlyInAnyOrder( - "com.linecorp.armeria.xds.PinnedPeerVerifierFactory", - "com.linecorp.armeria.xds.SanPeerVerifierFactory"); + .extracting(Object::getClass) + .containsExactlyInAnyOrder( + PinnedPeerVerifierFactory.class, + SanPeerVerifierFactory.class);This requires importing the classes, but ensures compile-time safety if classes are renamed.
it/xds-client/src/test/java/com/linecorp/armeria/xds/it/ResourceNodeMetricTest.java (1)
87-91: Minor:certificate1andcertificate2are only used bylistenerRootWithSdsSecretUpdate.These extensions will be initialized for every test in the class, adding unnecessary self-signed certificate generation overhead for unrelated tests. Consider scoping them to the test method or extracting the SDS test into a dedicated test class. This is a minor concern since it's test code.
xds/src/main/java/com/linecorp/armeria/xds/XdsBootstrapImpl.java (1)
106-111: Pre-existing:close()chain is not exception-safe.If any of the earlier
close()calls throw,watchService.close()(and any subsequent calls) will be skipped, potentially leaking the watch service thread. This is a pre-existing pattern, but the addition ofwatchServicemakes it slightly more consequential since it holds OS-level file watch handles.Consider wrapping in a utility that ensures all resources are closed, or at minimum use try/finally:
♻️ Suggested improvement
`@Override` public void close() { - controlPlaneClientManager.close(); - clusterManager.close(); - listenerManager.close(); - watchService.close(); + try { + controlPlaneClientManager.close(); + } finally { + try { + clusterManager.close(); + } finally { + try { + listenerManager.close(); + } finally { + watchService.close(); + } + } + } }xds/src/main/java/com/linecorp/armeria/xds/CombineLatest2Stream.java (1)
88-97: Missing null assertions before applying combiner, inconsistent withCombineLatest3Stream.
CombineLatest3Stream.maybeEmit()assertslatestA != null,latestB != null,latestC != nullbefore callingcombiner.apply(...). This class omits those assertions despite following the same pattern. SinceaReady/bReadyguards ensure the values have been set, the assertions serve as safety documentation.Proposed fix
private void maybeEmit() { if (!aReady || !bReady) { return; } + assert latestA != null; + assert latestB != null; try { emit(combiner.apply(latestA, latestB), null); } catch (Throwable t) {xds/src/main/java/com/linecorp/armeria/xds/SanMatcher.java (1)
104-123: Verify: wildcard matching allows*.com-style TLD wildcards.
dnsExactMatchWithWildcard("*.com", "foo.com", false)returnstrue. This is consistent with Envoy's behavior but may be worth noting. If stricter validation is desired (e.g., requiring at least two dots in the expected name), it would need an additional check.xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/RouterFilter.java (1)
84-89: Consider replacingassertwithrequireNonNullfor the transport socket invariant.
assert transportSocket != nullis a no-op without-ea. If an endpoint ever lacks theTRANSPORT_SOCKET_SNAPSHOT_KEYattribute (e.g., due to a future code path or third-partyXdsLoadBalancerFactory), this silently passes and causes an NPE on the next line. ArequireNonNullwith a clear message would fail fast in all environments.♻️ Proposed fix
if (endpoint != null) { final TransportSocketSnapshot transportSocket = endpoint.attr(XdsAttributeKeys.TRANSPORT_SOCKET_SNAPSHOT_KEY); - assert transportSocket != null; + requireNonNull(transportSocket, + "TransportSocketSnapshot must be set on xDS-managed endpoints"); setTlsParams(ctx, transportSocket); }This requires adding
import static java.util.Objects.requireNonNull;if not already present.it/xds-client/src/test/java/com/linecorp/armeria/xds/it/BootstrapSecretsTest.java (1)
93-101: Route ordering:prefix: /will shadowprefix: /no-tlsin actual routing.In Envoy, routes are matched in declaration order (first match wins). Since
prefix: /matches all paths,/no-tlswould never be reached during live request routing. This doesn't affect the test since routes are accessed by index, but if this YAML is reused as a reference config, the route order should be swapped.xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/XdsLoadBalancerFactory.java (1)
69-73: Consider adding@paramJavadoc for the newtransportSocketandtransportSocketMatchesparameters.The existing
registermethod Javadoc describes its purpose but doesn't document the two new parameters, which are key to the transport socket matching feature.xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/XdsAttributeAssigningEndpointGroup.java (1)
42-42: Fieldmatchedshould beprivatefor consistency.All other instance fields (
localityLbEndpoints,lbEndpoint,delegate) areprivate final, butmatchedis package-private. Since it's only accessed within this class (in theacceptmethod), it should beprivate finalto maintain encapsulation.♻️ Proposed fix
- final TransportSocketSnapshot matched; + private final TransportSocketSnapshot matched;it/xds-client/src/test/java/com/linecorp/armeria/xds/it/DynamicSecretTest.java (2)
52-54: Shared mutable state across tests may cause flaky runs.
cacheandversionare static fields mutated by every test method. Although JUnit 5 runs methods within a class sequentially by default, test ordering is not guaranteed, and a snapshot left by one test could leak into the next viacache.setSnapshot. Consider resetting the cache/version in a@BeforeEachmethod (or use unique group keys per test) to make each test hermetic.
96-96: Nit: inconsistent language hint comment.Other YAML templates use
//language=YAML(IntelliJ language injection), but this one uses// YAML. Align for consistency so all templates get IDE syntax highlighting.- // YAML + //language=YAMLxds/src/main/java/com/linecorp/armeria/xds/CertificateValidationContextSnapshot.java (1)
90-101:peerVerifierFactories()rebuilds the list on every call.The verifier factories are derived from immutable fields (
verifyCertificateSpkiPins,verifyCertificateHashPins,typedSanMatchers) that never change after construction. Consider computing and caching the result in the constructor or as a lazily-initialized field to avoid repeated allocations.xds/src/main/java/com/linecorp/armeria/xds/TransportSocketStream.java (1)
85-96: Only the first element of TLS certificate lists is used.Lines 86 and 93 only process
get(0)fromgetTlsCertificatesList()andgetTlsCertificateSdsSecretConfigsList()respectively. This matches Envoy's typical behavior (it uses the first certificate for client connections), but a brief comment noting the intentional choice would help readers.it/xds-client/src/test/java/com/linecorp/armeria/xds/it/CertificateValidationContextTest.java (1)
54-56: Shared mutable state across test methods.
cacheandversionare static and shared across all test methods. This works when tests run sequentially (JUnit 5 default), but could cause flaky failures if parallel execution is ever enabled for this class. This is a common pattern in xDS tests, so noting it for awareness rather than requiring a change.it/xds-client/src/test/java/com/linecorp/armeria/xds/it/DataSourceTest.java (2)
76-1018: Significant YAML bootstrap template duplication across tests.The bootstrap YAML string (roughly 30 lines) is duplicated nearly verbatim in every test method — only the secret YAML and port differ. Consider extracting a shared helper method (e.g.,
bootstrapYaml(int port)) similar to howsdsBootstrapYamlis used inCertificateValidationContextTest, to reduce duplication and improve maintainability.♻️ Example helper extraction
private static String sdsBootstrapYaml(int port) { return """ dynamic_resources: ads_config: api_type: GRPC grpc_services: - envoy_grpc: cluster_name: bootstrap-cluster static_resources: clusters: - name: bootstrap-cluster type: STATIC load_assignment: cluster_name: bootstrap-cluster endpoints: - lb_endpoints: - endpoint: address: socket_address: address: 127.0.0.1 port_value: %s - name: my-cluster type: STATIC load_assignment: cluster_name: my-cluster endpoints: - lb_endpoints: - endpoint: address: socket_address: address: 127.0.0.1 port_value: 8080 transport_socket: name: envoy.transport_sockets.tls typed_config: "@type": type.googleapis.com/envoy.extensions.transport_sockets.tls.v3.UpstreamTlsContext common_tls_context: tls_certificate_sds_secret_configs: - name: my-cert sds_config: ads: {} listeners: - name: my-listener api_listener: api_listener: "@type": type.googleapis.com/envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager stat_prefix: http route_config: name: local_route virtual_hosts: - name: local_service1 domains: [ "*" ] routes: - match: prefix: / route: cluster: my-cluster http_filters: - name: envoy.filters.http.router typed_config: "@type": type.googleapis.com/envoy.extensions.filters.http.router.v3.Router """.formatted(port); }
926-1018: Consider asserting the error type foremptyDataSource.The test asserts an error is received but doesn't check its type. Adding a type assertion (e.g.,
isInstanceOf(...)) would make the test more robust and document the expected failure mode.🧪 Proposed assertion improvement
await().untilAsserted(() -> assertThat(errorRef.get()).isNotNull()); + // Optionally assert the specific error type + assertThat(errorRef.get()).isInstanceOf(IllegalArgumentException.class);
| private static List<byte[]> decodeSpkiPins(List<String> pins) { | ||
| if (pins.isEmpty()) { | ||
| return Collections.emptyList(); | ||
| } | ||
| final List<byte[]> decoded = new ArrayList<>(pins.size()); | ||
| for (String pin : pins) { | ||
| decoded.add(Base64.getDecoder().decode(pin)); | ||
| } | ||
| return ImmutableList.copyOf(decoded); | ||
| } |
There was a problem hiding this comment.
Uncaught IllegalArgumentException from Base64.getDecoder().decode(pin).
If a malformed Base64 string appears in verify_certificate_spki, Base64.getDecoder().decode() throws IllegalArgumentException. This would surface as an unhandled runtime exception during snapshot construction with no clear context about which pin was invalid. Consider wrapping this in a try-catch to provide a more descriptive error message.
Proposed fix
private static List<byte[]> decodeSpkiPins(List<String> pins) {
if (pins.isEmpty()) {
return Collections.emptyList();
}
final List<byte[]> decoded = new ArrayList<>(pins.size());
for (String pin : pins) {
- decoded.add(Base64.getDecoder().decode(pin));
+ try {
+ decoded.add(Base64.getDecoder().decode(pin));
+ } catch (IllegalArgumentException e) {
+ throw new IllegalArgumentException("Invalid Base64 SPKI pin: " + pin, e);
+ }
}
return ImmutableList.copyOf(decoded);
}📝 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.
| private static List<byte[]> decodeSpkiPins(List<String> pins) { | |
| if (pins.isEmpty()) { | |
| return Collections.emptyList(); | |
| } | |
| final List<byte[]> decoded = new ArrayList<>(pins.size()); | |
| for (String pin : pins) { | |
| decoded.add(Base64.getDecoder().decode(pin)); | |
| } | |
| return ImmutableList.copyOf(decoded); | |
| } | |
| private static List<byte[]> decodeSpkiPins(List<String> pins) { | |
| if (pins.isEmpty()) { | |
| return Collections.emptyList(); | |
| } | |
| final List<byte[]> decoded = new ArrayList<>(pins.size()); | |
| for (String pin : pins) { | |
| try { | |
| decoded.add(Base64.getDecoder().decode(pin)); | |
| } catch (IllegalArgumentException e) { | |
| throw new IllegalArgumentException("Invalid Base64 SPKI pin: " + pin, e); | |
| } | |
| } | |
| return ImmutableList.copyOf(decoded); | |
| } |
🤖 Prompt for AI Agents
In
`@xds/src/main/java/com/linecorp/armeria/xds/CertificateValidationContextSnapshot.java`
around lines 103 - 112, The decodeSpkiPins method currently calls
Base64.getDecoder().decode(pin) which can throw IllegalArgumentException for
malformed input; modify decodeSpkiPins to catch IllegalArgumentException around
each decode call (in the loop over pins), and rethrow a new
IllegalArgumentException (or IllegalStateException if preferred) that includes
the offending pin value and the original exception as the cause so snapshot
construction fails with a clear, contextual message; keep returning
ImmutableList.copyOf(decoded) on success and preserve behavior when pins is
empty.
xds/src/main/java/com/linecorp/armeria/xds/CertificateValidationContextStream.java
Show resolved
Hide resolved
| @Override | ||
| protected Subscription onStart(SnapshotWatcher<O> watcher) { | ||
| subA = streamA.subscribe(this::onA); | ||
| subB = streamB.subscribe(this::onB); | ||
| subC = streamC.subscribe(this::onC); | ||
|
|
||
| return () -> { | ||
| if (subA != null) { | ||
| subA.close(); | ||
| } | ||
| if (subB != null) { | ||
| subB.close(); | ||
| } | ||
| if (subC != null) { | ||
| subC.close(); | ||
| } | ||
| subA = null; | ||
| subB = null; | ||
| subC = null; | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find the CombineLatest files
fd -e java "CombineLatest" xds/src/main/java/Repository: line/armeria
Length of output: 193
🏁 Script executed:
#!/bin/bash
# Read CombineLatest2Stream to check the disposal pattern
cat -n xds/src/main/java/com/linecorp/armeria/xds/CombineLatest2Stream.javaRepository: line/armeria
Length of output: 3344
🏁 Script executed:
#!/bin/bash
# Read CombineLatest3Stream to compare
cat -n xds/src/main/java/com/linecorp/armeria/xds/CombineLatest3Stream.javaRepository: line/armeria
Length of output: 4347
🏁 Script executed:
#!/bin/bash
# Check RefCountedStream to understand the emit() behavior and subscription lifecycle
fd -e java "RefCountedStream" xds/src/main/java/ | head -5Repository: line/armeria
Length of output: 120
🏁 Script executed:
#!/bin/bash
# Read RefCountedStream to understand subscription lifecycle
cat -n xds/src/main/java/com/linecorp/armeria/xds/RefCountedStream.javaRepository: line/armeria
Length of output: 3231
Both CombineLatest2Stream and CombineLatest3Stream have the same disposal pattern without a closed guard.
After close() is called on the subscriptions, subA/subB/subC are nulled out, but in-flight callbacks (e.g., onA, onB, onC) could still mutate latestA/latestB/latestC and call maybeEmit(). If the underlying streams guarantee that callbacks stop before Subscription.close() returns, this is fine. Otherwise, adding a guard flag (e.g., a closed boolean checked in the callback methods) would prevent stale emissions.
🤖 Prompt for AI Agents
In `@xds/src/main/java/com/linecorp/armeria/xds/CombineLatest3Stream.java` around
lines 61 - 81, The dispose logic in CombineLatest3Stream's onStart sets
subA/subB/subC to null after calling close(), but callbacks onA/onB/onC can
still run and mutate latestA/latestB/latestC and call maybeEmit; add a volatile
boolean closed (or AtomicBoolean) field in the CombineLatest3Stream class, set
closed = true in the returned Subscription lambda (before/after closing the
subs) and have onA/onB/onC check and return immediately if closed is true,
preventing stale emissions; ensure the same guard is applied in
CombineLatest2Stream if present and reference the onStart method,
subA/subB/subC, onA/onB/onC, maybeEmit, and latestA/latestB/latestC when making
the change.
xds/src/main/java/com/linecorp/armeria/xds/PinnedPeerVerifierFactory.java
Show resolved
Hide resolved
xds/src/main/java/com/linecorp/armeria/xds/TransportSocketMatchSnapshot.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@xds/src/main/java/com/linecorp/armeria/xds/internal/XdsCommonUtil.java`:
- Around line 91-102: The assert on transportSocket in setTlsParams is unsafe;
replace it with an explicit null check on
endpoint.attr(TRANSPORT_SOCKET_SNAPSHOT_KEY) (the TransportSocketSnapshot
retrieved in setTlsParams) and handle the null case deterministically: if
transportSocket is null, set ctx.setSessionProtocol(SessionProtocol.HTTP) and
return (mirroring the clientTlsSpec==null branch) or throw an
IllegalStateException with a descriptive message; then continue to read
clientTlsSpec and set ctx.setSessionProtocol(SessionProtocol.HTTPS) and
ctx.setClientTlsSpec(clientTlsSpec) as before.
In
`@xds/src/main/java/com/linecorp/armeria/xds/TransportSocketMatchSnapshot.java`:
- Around line 29-50: TransportSocketMatchSnapshot is missing equals and hashCode
overrides so ClusterSnapshot's transportSocketMatches list comparison uses
identity equality; implement equals(Object) and hashCode() in
TransportSocketMatchSnapshot to perform structural equality by comparing
transportSocketMatch (TransportSocketMatch) and transportSocketSnapshot
(TransportSocketSnapshot) fields, mirroring the pattern used in
RouteSnapshot/EndpointSnapshot/ListenerSnapshot/VirtualHostSnapshot, ensuring
null-safe comparisons and consistent hash computation.
🧹 Nitpick comments (14)
it/xds-client/src/test/java/com/linecorp/armeria/xds/it/XdsEndpointGroupTest.java (3)
106-131: Both clusters are created without a transport socket —httpsClustermay need TLS config.
clusterYaml(HTTPS_CLUSTER_NAME, null)at Line 109 creates the "https" cluster without atransport_socket. This means the xDS config itself carries no TLS information for the HTTPS backend. IntestAllHttps, TLS only works becauseSessionProtocol.HTTPSandClientFactory.insecure()are used on the client side, bypassing xDS-driven TLS entirely.If the intent is to validate that xDS-provided TLS settings work end-to-end (which aligns with this PR's goal of integrating SDS/transport-socket support), consider configuring
httpsClusterwithhelloCert.certificateFile()as the trusted CA so the test exercises xDS transport socket resolution:final Cluster httpsCluster = clusterYaml(HTTPS_CLUSTER_NAME, helloCert.certificateFile());If the current behavior is intentional (i.e., testing that the client protocol override works independently of xDS TLS config), a brief comment clarifying this would help future readers.
148-163:testAllHttpsdoesn't exercise xDS-driven TLS — relies entirely on client-side overrides.This test connects to an HTTPS backend using
SessionProtocol.HTTPSwithClientFactory.insecure(), but the underlying xDS cluster has notransport_socket. The xDS TLS path is not tested here. Given the PR's objective to integrate SDS with the XDS preprocessor and validate transport socket handling, consider either:
- Adding a
transport_socketto the HTTPS cluster (as noted above) and removingClientFactory.insecure(), to verify that xDS-configured TLS works end-to-end.- If this test is intentionally validating the non-xDS-TLS path, rename it to clarify the intent (e.g.,
testHttpsWithClientOverride).
271-339: Consider extracting common YAML structure to reduce duplication.The
bootstrapYamlmethod has two large YAML blocks that differ only by the presence of thetransport_socketstanza (Lines 276-300 vs 303-336). The rest of the structure (dynamic_resources, static_resources, cluster definition) is identical.A minor refactor could build the base YAML and conditionally append the transport socket block, reducing duplication and making future changes easier. That said, for a test helper the current approach is readable enough — this is just a nice-to-have.
it/xds-client/src/test/java/com/linecorp/armeria/xds/it/ErrorHandlingTest.java (1)
411-412: Duplicate//language=comment.Line 411 has
//language=yaml(lowercase) which is immediately followed by//language=YAML(uppercase) on line 412. One of these should be removed.Proposed fix
- //language=yaml //language=YAML private static final String routeYaml =it/xds-client/src/test/java/com/linecorp/armeria/xds/it/ControlPlaneTlsIntegrationTest.java (1)
438-442:mutateLastCharmay produce invalid Base64 when applied to SPKI pins.SHA-256 digests (32 bytes) produce a 44-character Base64 string ending with
=(padding).mutateLastCharwill replace=withA, which corrupts the Base64 padding rather than changing the digest payload. Depending on how the downstream decoder handles this, it may throwIllegalArgumentExceptionduring pin parsing rather than failing during the TLS handshake.If the intent is to test SPKI pin mismatch (correct decode, wrong value), consider mutating a non-padding character instead:
Proposed fix
private static String mutateLastChar(String value) { - final char last = value.charAt(value.length() - 1); - final char replacement = last == 'A' ? 'B' : 'A'; - return value.substring(0, value.length() - 1) + replacement; + // Skip any trailing '=' padding characters + int idx = value.length() - 1; + while (idx >= 0 && value.charAt(idx) == '=') { + idx--; + } + final char last = value.charAt(idx); + final char replacement = last == 'A' ? 'B' : 'A'; + return value.substring(0, idx) + replacement + value.substring(idx + 1); }xds/src/main/java/com/linecorp/armeria/xds/CertificateValidationContextSnapshot.java (1)
87-98:peerVerifierFactories()creates new factory instances on every call.Since this is an immutable snapshot, the list of verifier factories could be computed once in the constructor and cached. This isn't a performance-critical path so it's a minor optimization, but it would be consistent with the eager-computation pattern used for
verifyCertificateSpkiPinsandtypedSanMatchers.xds/src/main/java/com/linecorp/armeria/xds/ClusterSnapshot.java (1)
71-79: Inconsistent@UnstableApion new accessors.
transportSocketMatches()(line 76) has@UnstableApiwhiletransportSocket()(line 111) does not. Since the enclosing class already carries@UnstableApi, neither method strictly needs it per the coding guidelines, but the inconsistency could confuse readers. Consider either adding it to both or removing it fromtransportSocketMatches()for consistency.Also applies to: 107-113
it/xds-client/src/test/java/com/linecorp/armeria/xds/it/TlsValidationContextSdsIntegrationTest.java (3)
89-91: Duplicate//language=YAMLcomment.Lines 89 and 90 both have the YAML language hint (one with a space, one without). Remove one.
🧹 Proposed fix
- // language=YAML //language=YAML private static final String bootstrapTemplate =
54-60: Shared mutable static state (cache,version) may cause flaky tests if test ordering changes.
cacheandversionarestaticand shared across all test methods. If tests run in a different order or if@RegisterExtensionlifecycle doesn't reset them, stale snapshots from a prior test could leak into the next one. Since JUnit 5 runs tests sequentially within a class by default, this is low-risk, but consider resetting state in a@BeforeEachto be safe.
252-264: Creating a newClientFactoryon everyfetch()call is heavyweight.Each invocation constructs and tears down a
ClientFactory. InvalidationContextRotationUpdatesTrust,fetch()is called multiple times (including withinawaitretries). This is acceptable for test code but could slow down CI. Consider creating the factory once per test if flakiness or slowness becomes an issue.it/xds-client/src/test/java/com/linecorp/armeria/xds/it/CertificateValidationContextTest.java (1)
293-304: Repeated deep snapshot navigation could be extracted into a helper.The chain
listenerSnapshot.routeSnapshot().virtualHostSnapshots().get(0).routeEntries().get(0).clusterSnapshot().transportSocket()is duplicated acrossmultipleCaCertificates,validationContextWithSds,mergeValidationContextWithBase, andoverrideBaseContextFields. Consider a small helper method to reduce duplication and make the tests more readable.♻️ Example helper extraction
private static TransportSocketSnapshot transportSocket(ListenerSnapshot snapshot) { return snapshot.routeSnapshot().virtualHostSnapshots().get(0) .routeEntries().get(0).clusterSnapshot().transportSocket(); }it/xds-client/src/test/java/com/linecorp/armeria/xds/client/endpoint/TransportSocketMatchUtilTest.java (1)
363-366: YAML indentation viaString.indent()is clever but fragile.If the template's structure changes, the indent values (8 and 4) must be manually adjusted. This is acceptable for test code, but a brief comment explaining the required indentation levels would help future maintainers.
xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/XdsAttributeAssigningEndpointGroup.java (1)
43-43: Makematchedprivate to keep state encapsulated.
matcheddoesn’t appear to be used outside this class; reducing visibility helps avoid accidental coupling.♻️ Suggested change
- final TransportSocketSnapshot matched; + private final TransportSocketSnapshot matched;it/xds-client/src/test/java/com/linecorp/armeria/xds/it/DynamicSecretTest.java (1)
52-55: Consider isolating shared state if tests run in parallel.The static cache/version can cause cross-test interference under parallel execution.
♻️ Optional test-stability guard
+import org.junit.jupiter.api.parallel.Execution; +import org.junit.jupiter.api.parallel.ExecutionMode; @@ -class DynamicSecretTest { +@Execution(ExecutionMode.SAME_THREAD) +class DynamicSecretTest {
xds/src/main/java/com/linecorp/armeria/xds/TransportSocketMatchSnapshot.java
Show resolved
Hide resolved
jrhee17
force-pushed
the
feat/xds-tls
branch
from
516fac4 to
dd73504
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (11)
xds/src/main/java/com/linecorp/armeria/xds/CertificateValidationContextSnapshot.java (1)
89-100:peerVerifierFactories()creates new instances on each call.This public method allocates new
PinnedPeerVerifierFactoryandSanPeerVerifierFactoryinstances per invocation. Currently it's only called once duringTransportSocketSnapshotconstruction, but since it's a public API, consider caching the result in a field to avoid surprises if called multiple times.🤖 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/CertificateValidationContextSnapshot.java` around lines 89 - 100, peerVerifierFactories() currently allocates new PinnedPeerVerifierFactory and SanPeerVerifierFactory instances on every call; change it to compute and cache the immutable result once in a private final (or lazily-initialized) field so subsequent calls return the same List instance. Implement a private field like cachedPeerVerifierFactories and populate it from the existing builder logic (or lazily initialize it on first call in peerVerifierFactories()), ensuring the returned list is immutable; reference peerVerifierFactories(), PinnedPeerVerifierFactory, SanPeerVerifierFactory and TransportSocketSnapshot usage to locate where to compute/store the cached value.it/xds-client/src/test/java/com/linecorp/armeria/xds/it/TransportSocketSnapshotTest.java (1)
476-493: Consider using a more appropriate exception type in thesha256helper.
NoSuchAlgorithmExceptionis wrapped inCertificateException(line 491), butCertificateExceptiondoesn't semantically relate to algorithm availability. Since SHA-256 is guaranteed by every compliant JVM, this is unlikely to trigger in practice, but usingIllegalStateExceptionorUncheckedSecurityExceptionwould be more idiomatic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@it/xds-client/src/test/java/com/linecorp/armeria/xds/it/TransportSocketSnapshotTest.java` around lines 476 - 493, The sha256 helper currently wraps NoSuchAlgorithmException in a CertificateException which is semantically incorrect; change sha256(byte[] input) to throw an unchecked exception (e.g. IllegalStateException or UncheckedSecurityException) instead of CertificateException, update its signature to remove "throws CertificateException", and replace the catch block to throw new IllegalStateException("SHA-256 not available.", e); keep spkiPin(X509Certificate) and certHash(X509Certificate) signatures as-is (they still legitimately declare CertificateException for certificate encoding errors) and ensure callers compile after this change.it/xds-client/src/test/java/com/linecorp/armeria/xds/it/TlsPeerVerificationIntegrationTest.java (2)
123-124: Nit: duplicate// language=YAMLcomments.Lines 123–124 and 170–171 each have two language injection hints (
// language=YAMLand//language=YAML). One per template is sufficient.Also applies to: 170-171
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@it/xds-client/src/test/java/com/linecorp/armeria/xds/it/TlsPeerVerificationIntegrationTest.java` around lines 123 - 124, In TlsPeerVerificationIntegrationTest remove the duplicate language injection comment so each YAML template only has a single "// language=YAML" hint; locate the duplicated pairs near the YAML template blocks in the test (the repeated "// language=YAML" / "//language=YAML" occurrences) and delete the extra comment in both places so each template has exactly one language hint.
709-722:transportSocketSnapshotmay silently miss snapshot if emitted before watcher registration.If
listenerRootemits the snapshot synchronously or on a fast path beforeaddSnapshotWatcheris called at Line 712, theAtomicReferencemay never be populated. In practice, Awaitility's default 10s timeout would surface this as a test failure rather than a hang, so this is low risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@it/xds-client/src/test/java/com/linecorp/armeria/xds/it/TlsPeerVerificationIntegrationTest.java` around lines 709 - 722, The watcher may miss a snapshot emitted before addSnapshotWatcher is called; after registering the watcher on listenerRoot, immediately query the current snapshot (e.g. call listenerRoot.snapshot() or listenerRoot.currentSnapshot() depending on the API) and if non-null set snapshotRef before waiting, so transportSocketSnapshot uses the snapshotRef populated either by the synchronous getter or the asynchronous watcher; keep references to listenerRoot, addSnapshotWatcher, snapshotRef and await() in your change.it/xds-client/src/test/java/com/linecorp/armeria/xds/it/TlsValidationContextSdsIntegrationTest.java (1)
89-90: Nit: duplicate// language=YAMLcomments.Same pattern as other test files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@it/xds-client/src/test/java/com/linecorp/armeria/xds/it/TlsValidationContextSdsIntegrationTest.java` around lines 89 - 90, Remove the duplicate comment line that repeats the language hint in the TlsValidationContextSdsIntegrationTest test (the consecutive "// language=YAML" comments); edit the test source in the TlsValidationContextSdsIntegrationTest class to keep a single "// language=YAML" marker where applicable to match the pattern used in other test files.it/xds-client/src/test/java/com/linecorp/armeria/xds/it/TransportSocketMatchesIntegrationTest.java (1)
57-58: Nit: duplicate// language=YAMLcomments.Same duplicate IDE hint as in the other test files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@it/xds-client/src/test/java/com/linecorp/armeria/xds/it/TransportSocketMatchesIntegrationTest.java` around lines 57 - 58, In TransportSocketMatchesIntegrationTest, remove the duplicated IDE hint so only a single "// language=YAML" comment remains; locate the duplicate comments near the top of the file (they appear before the YAML block in the TransportSocketMatchesIntegrationTest class) and keep one instance, deleting the extra to match the other test files.xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/XdsEndpointUtil.java (1)
221-229:attachTransportSocketduplicatesselectTransportSocketinvocation already inconvertToEndpoint; consider delegating to reduce code duplication.Both
convertToEndpoint(line 198) andattachTransportSocket(line 226) independently invokeTransportSocketMatchUtil.selectTransportSocketwith identical arguments, then attach the result viawithAttr(TRANSPORT_SOCKET_SNAPSHOT_KEY, ...). You can consolidate this by havingconvertToEndpointconstruct the endpoint first, then delegate the attach step to this helper:♻️ Optional refactor to consolidate selection+attach
private static Endpoint convertToEndpoint( LocalityLbEndpoints localityLbEndpoints, LbEndpoint lbEndpoint, TransportSocketSnapshot transportSocket, List<TransportSocketMatchSnapshot> transportSocketMatches) { final SocketAddress socketAddress = lbEndpoint.getEndpoint().getAddress().getSocketAddress(); final String hostname = lbEndpoint.getEndpoint().getHostname(); final int weight = endpointWeight(lbEndpoint); - final TransportSocketSnapshot matchedTransport = TransportSocketMatchUtil.selectTransportSocket( - transportSocket, transportSocketMatches, lbEndpoint, localityLbEndpoints); final Endpoint endpoint; if (!Strings.isNullOrEmpty(hostname)) { endpoint = Endpoint.of(hostname) .withIpAddr(socketAddress.getAddress()) .withAttr(XdsAttributeKeys.LB_ENDPOINT_KEY, lbEndpoint) .withAttr(XdsAttributeKeys.LOCALITY_LB_ENDPOINTS_KEY, localityLbEndpoints) - .withAttr(XdsCommonUtil.TRANSPORT_SOCKET_SNAPSHOT_KEY, matchedTransport) .withWeight(weight); } else { endpoint = Endpoint.of(socketAddress.getAddress()) .withAttr(XdsAttributeKeys.LB_ENDPOINT_KEY, lbEndpoint) .withAttr(XdsAttributeKeys.LOCALITY_LB_ENDPOINTS_KEY, localityLbEndpoints) - .withAttr(XdsCommonUtil.TRANSPORT_SOCKET_SNAPSHOT_KEY, matchedTransport) .withWeight(weight); } - if (socketAddress.hasPortValue()) { - return endpoint.withPort(socketAddress.getPortValue()); + final Endpoint withTransport = attachTransportSocket( + endpoint, transportSocket, transportSocketMatches, lbEndpoint, localityLbEndpoints); + if (socketAddress.hasPortValue()) { + return withTransport.withPort(socketAddress.getPortValue()); } - return endpoint; + return withTransport; }🤖 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 221 - 229, convertToEndpoint and attachTransportSocket both call TransportSocketMatchUtil.selectTransportSocket causing duplication; change attachTransportSocket to accept a pre-selected TransportSocketSnapshot (remove its internal call to selectTransportSocket) and have convertToEndpoint call TransportSocketMatchUtil.selectTransportSocket once, then call attachTransportSocket(endpoint, matchedSnapshot, lbEndpoint, localityLbEndpoints) to attach via withAttr(XdsCommonUtil.TRANSPORT_SOCKET_SNAPSHOT_KEY, matchedSnapshot). Update method signature of attachTransportSocket and all its call sites accordingly so selection is performed only in convertToEndpoint.it/xds-client/src/test/java/com/linecorp/armeria/xds/it/XdsPreprocessorTest.java (4)
136-136:testWithListeneris too vague — all three tests use a listener.A more descriptive name like
testAllHttportestNoTlswould make the intent immediately clear without reading the body.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@it/xds-client/src/test/java/com/linecorp/armeria/xds/it/XdsPreprocessorTest.java` at line 136, Rename the vague test method testWithListener in XdsPreprocessorTest to a descriptive name that reflects its intent (e.g., testAllHttp or testNoTls) so readers understand the scenario without inspecting the body; update the method declaration and any references/usages (test runners, annotations, or helper calls) to use the new name to keep tests runnable and consistent.
117-119: UsehttpsSocketAddress().getHostString()when combining withhttpsPort().
httpSocketAddress().getHostString()is used to obtain the hostname for what are explicitly HTTPS connections (paired withhttpsPort()). While both addresses typically resolve to the same host in test environments, the semantics are inconsistent. The same pattern recurs intestAllHttps(Lines 154–155) andtestControlPlaneOnlyHttps(Lines 169–170).♻️ Suggested fix
- final ClusterLoadAssignment httpsAssignment = - endpointYaml(HTTPS_CLUSTER_NAME, - helloServer.httpSocketAddress().getHostString(), - helloServer.httpsPort()); + final ClusterLoadAssignment httpsAssignment = + endpointYaml(HTTPS_CLUSTER_NAME, + helloServer.httpsSocketAddress().getHostString(), + helloServer.httpsPort());Apply the same change in
testAllHttpsandtestControlPlaneOnlyHttps:- bootstrapYaml(HTTPS_BOOTSTRAP_CLUSTER_NAME, - server.httpSocketAddress().getHostString(), - server.httpsPort(), + bootstrapYaml(HTTPS_BOOTSTRAP_CLUSTER_NAME, + server.httpsSocketAddress().getHostString(), + server.httpsPort(),Also applies to: 154-155, 169-170
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@it/xds-client/src/test/java/com/linecorp/armeria/xds/it/XdsPreprocessorTest.java` around lines 117 - 119, Replace uses of httpSocketAddress().getHostString() with httpsSocketAddress().getHostString() when constructing the HTTPS endpoint in XdsPreprocessorTest; specifically update the endpointYaml(...) calls in the test method where HTTPS is used (the call in the shown block and the calls in testAllHttps and testControlPlaneOnlyHttps) so the hostname is taken from helloServer.httpsSocketAddress() to match the paired httpsPort() and preserve correct semantics.
224-226: Use%dfor integerport_valueformat placeholders.
%sworks via autoboxing but%dis the conventional, type-safe specifier for integral values. The same applies to the twoport_value: %soccurrences inbootstrapYaml(Lines 297 and 324).♻️ Suggested fix (endpointYaml and bootstrapYaml)
- port_value: %s + port_value: %dUpdate the corresponding
.formatted(...)calls if the argument order/types change, though forintthis is a drop-in replacement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@it/xds-client/src/test/java/com/linecorp/armeria/xds/it/XdsPreprocessorTest.java` around lines 224 - 226, Update the formatted string placeholders to use the integer specifier for port values: in XdsPreprocessorTest replace the "%s" used for port_value in endpointYaml and the two occurrences in bootstrapYaml with "%d" and keep the same .formatted(...) argument order (clusterName, address, port) so the int port is formatted with "%d" for type-safety and clarity.
150-178: Extract shared HTTPS bootstrap construction to avoid duplication.
testAllHttpsandtestControlPlaneOnlyHttpsconstruct identicalBootstrapobjects (same cluster name, address, port, and cert file); the only difference is theofListenerargument. A private helper removes the duplication and makes the varying intent explicit.♻️ Suggested refactor
+ private static Bootstrap createHttpsBootstrap() { + return bootstrapYaml(HTTPS_BOOTSTRAP_CLUSTER_NAME, + server.httpsSocketAddress().getHostString(), + server.httpsPort(), + controlPlaneCert.certificateFile()); + } + `@Test` void testAllHttps() { - final Bootstrap bootstrap = - bootstrapYaml(HTTPS_BOOTSTRAP_CLUSTER_NAME, - server.httpSocketAddress().getHostString(), - server.httpsPort(), - controlPlaneCert.certificateFile()); + final Bootstrap bootstrap = createHttpsBootstrap(); try (XdsBootstrap xdsBootstrap = XdsBootstrap.of(bootstrap, eventLoop.get()); XdsHttpPreprocessor xdsPreprocessor = XdsHttpPreprocessor.ofListener(HTTPS_LISTENER_NAME, xdsBootstrap)) { ... } } `@Test` void testControlPlaneOnlyHttps() { - final Bootstrap bootstrap = - bootstrapYaml(HTTPS_BOOTSTRAP_CLUSTER_NAME, - server.httpSocketAddress().getHostString(), - server.httpsPort(), - controlPlaneCert.certificateFile()); + final Bootstrap bootstrap = createHttpsBootstrap(); try (XdsBootstrap xdsBootstrap = XdsBootstrap.of(bootstrap, eventLoop.get()); XdsHttpPreprocessor xdsPreprocessor = XdsHttpPreprocessor.ofListener(LISTENER_NAME, xdsBootstrap)) { ... } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@it/xds-client/src/test/java/com/linecorp/armeria/xds/it/XdsPreprocessorTest.java` around lines 150 - 178, Both testAllHttps and testControlPlaneOnlyHttps build the same Bootstrap; extract that construction into a private helper (e.g., buildHttpsBootstrap or createHttpsBootstrap) that calls bootstrapYaml with HTTPS_BOOTSTRAP_CLUSTER_NAME, server.httpSocketAddress().getHostString(), server.httpsPort(), and controlPlaneCert.certificateFile(), return the Bootstrap and use it in both tests when creating XdsBootstrap.of(bootstrap, eventLoop.get()); keep the differing call to XdsHttpPreprocessor.ofListener (LISTENER_NAME vs HTTPS_LISTENER_NAME) in each test so only the bootstrap construction is shared.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@xds/src/main/java/com/linecorp/armeria/xds/GrpcServicesPreprocessor.java`:
- Line 74: Replace the assertion in XdsCommonUtil.setTlsParams with a proper
null guard: check the endpoint (or the attribute fetched via
TRANSPORT_SOCKET_SNAPSHOT_KEY) for null and return (or handle) early instead of
using assert, so XdsServicesPreprocessor and any callers (e.g.,
GrpcServicesPreprocessor invoking XdsCommonUtil.setTlsParams) do not rely on JVM
assertions; ensure the method explicitly validates its inputs and logs or safely
exits when the transport-socket snapshot is missing to prevent NPEs if future
code paths omit attribute assignment.
---
Duplicate comments:
In
`@xds/src/main/java/com/linecorp/armeria/xds/CertificateValidationContextSnapshot.java`:
- Around line 102-111: decodeSpkiPins currently calls
Base64.getDecoder().decode(pin) which can throw IllegalArgumentException for
malformed input; wrap the decode call inside a try/catch in decodeSpkiPins and
when a decode fails rethrow a new IllegalArgumentException that includes
identifying info (the offending pin string or its index) and set the caught
exception as the cause so callers get a descriptive error (e.g., "Invalid Base64
SPKI pin: <pin>" or include pin index), keeping the rest of the method logic
unchanged and still returning ImmutableList.copyOf(decoded) on success.
In
`@xds/src/main/java/com/linecorp/armeria/xds/CertificateValidationContextStream.java`:
- Around line 62-67: The try-with-resources around bs.get().newInput() in
CertificateValidationContextStream correctly closes the InputStream and the
multi-catch of CertificateException | IOException covers the thrown exceptions
from CertificateUtil.toX509Certificates and ByteSource.newInput(); no code
changes required—leave the block using try (InputStream inputStream =
bs.get().newInput()) { caCerts =
CertificateUtil.toX509Certificates(inputStream); } catch (CertificateException |
IOException e) { return Exceptions.throwUnsafely(e); } and the subsequent return
new CertificateValidationContextSnapshot(validationContext, caCerts); as-is.
In
`@xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/RouterFilter.java`:
- Around line 93-95: The null-guard in XdsCommonUtil.setTlsParams currently uses
an assert which is disabled in production and allows a later NPE at
transportSocket.clientTlsSpec(); replace the assert by explicitly checking the
attribute (TRANSPORT_SOCKET_SNAPSHOT_KEY) for null and either throw a
descriptive IllegalStateException (including endpoint and cluster context) or
apply a safe fallback (e.g., skip TLS setup) so callers like
RouterFilter.execute and code paths via XdsLoadBalancer.selectNow(ctx) do not
NPE when an endpoint from clusters that bypass
XdsAttributeAssigningEndpointGroup lacks the attribute.
In `@xds/src/main/java/com/linecorp/armeria/xds/PinnedPeerVerifierFactory.java`:
- Around line 37-44: The previous equals/hashCode issue is resolved by changing
the fields to List<ByteString> in PinnedPeerVerifierFactory: ensure the
constructor continues to copy inputs using ImmutableList.copyOf for both
spkiPins and certHashPins so the fields are immutable and ByteString provides
content-based equality/hashing; no additional equals/hashCode overrides are
necessary for PinnedPeerVerifierFactory now because List<ByteString> items will
compare by content.
---
Nitpick comments:
In
`@it/xds-client/src/test/java/com/linecorp/armeria/xds/it/TlsPeerVerificationIntegrationTest.java`:
- Around line 123-124: In TlsPeerVerificationIntegrationTest remove the
duplicate language injection comment so each YAML template only has a single "//
language=YAML" hint; locate the duplicated pairs near the YAML template blocks
in the test (the repeated "// language=YAML" / "//language=YAML" occurrences)
and delete the extra comment in both places so each template has exactly one
language hint.
- Around line 709-722: The watcher may miss a snapshot emitted before
addSnapshotWatcher is called; after registering the watcher on listenerRoot,
immediately query the current snapshot (e.g. call listenerRoot.snapshot() or
listenerRoot.currentSnapshot() depending on the API) and if non-null set
snapshotRef before waiting, so transportSocketSnapshot uses the snapshotRef
populated either by the synchronous getter or the asynchronous watcher; keep
references to listenerRoot, addSnapshotWatcher, snapshotRef and await() in your
change.
In
`@it/xds-client/src/test/java/com/linecorp/armeria/xds/it/TlsValidationContextSdsIntegrationTest.java`:
- Around line 89-90: Remove the duplicate comment line that repeats the language
hint in the TlsValidationContextSdsIntegrationTest test (the consecutive "//
language=YAML" comments); edit the test source in the
TlsValidationContextSdsIntegrationTest class to keep a single "// language=YAML"
marker where applicable to match the pattern used in other test files.
In
`@it/xds-client/src/test/java/com/linecorp/armeria/xds/it/TransportSocketMatchesIntegrationTest.java`:
- Around line 57-58: In TransportSocketMatchesIntegrationTest, remove the
duplicated IDE hint so only a single "// language=YAML" comment remains; locate
the duplicate comments near the top of the file (they appear before the YAML
block in the TransportSocketMatchesIntegrationTest class) and keep one instance,
deleting the extra to match the other test files.
In
`@it/xds-client/src/test/java/com/linecorp/armeria/xds/it/TransportSocketSnapshotTest.java`:
- Around line 476-493: The sha256 helper currently wraps
NoSuchAlgorithmException in a CertificateException which is semantically
incorrect; change sha256(byte[] input) to throw an unchecked exception (e.g.
IllegalStateException or UncheckedSecurityException) instead of
CertificateException, update its signature to remove "throws
CertificateException", and replace the catch block to throw new
IllegalStateException("SHA-256 not available.", e); keep
spkiPin(X509Certificate) and certHash(X509Certificate) signatures as-is (they
still legitimately declare CertificateException for certificate encoding errors)
and ensure callers compile after this change.
In
`@it/xds-client/src/test/java/com/linecorp/armeria/xds/it/XdsPreprocessorTest.java`:
- Line 136: Rename the vague test method testWithListener in XdsPreprocessorTest
to a descriptive name that reflects its intent (e.g., testAllHttp or testNoTls)
so readers understand the scenario without inspecting the body; update the
method declaration and any references/usages (test runners, annotations, or
helper calls) to use the new name to keep tests runnable and consistent.
- Around line 117-119: Replace uses of httpSocketAddress().getHostString() with
httpsSocketAddress().getHostString() when constructing the HTTPS endpoint in
XdsPreprocessorTest; specifically update the endpointYaml(...) calls in the test
method where HTTPS is used (the call in the shown block and the calls in
testAllHttps and testControlPlaneOnlyHttps) so the hostname is taken from
helloServer.httpsSocketAddress() to match the paired httpsPort() and preserve
correct semantics.
- Around line 224-226: Update the formatted string placeholders to use the
integer specifier for port values: in XdsPreprocessorTest replace the "%s" used
for port_value in endpointYaml and the two occurrences in bootstrapYaml with
"%d" and keep the same .formatted(...) argument order (clusterName, address,
port) so the int port is formatted with "%d" for type-safety and clarity.
- Around line 150-178: Both testAllHttps and testControlPlaneOnlyHttps build the
same Bootstrap; extract that construction into a private helper (e.g.,
buildHttpsBootstrap or createHttpsBootstrap) that calls bootstrapYaml with
HTTPS_BOOTSTRAP_CLUSTER_NAME, server.httpSocketAddress().getHostString(),
server.httpsPort(), and controlPlaneCert.certificateFile(), return the Bootstrap
and use it in both tests when creating XdsBootstrap.of(bootstrap,
eventLoop.get()); keep the differing call to XdsHttpPreprocessor.ofListener
(LISTENER_NAME vs HTTPS_LISTENER_NAME) in each test so only the bootstrap
construction is shared.
In
`@xds/src/main/java/com/linecorp/armeria/xds/CertificateValidationContextSnapshot.java`:
- Around line 89-100: peerVerifierFactories() currently allocates new
PinnedPeerVerifierFactory and SanPeerVerifierFactory instances on every call;
change it to compute and cache the immutable result once in a private final (or
lazily-initialized) field so subsequent calls return the same List instance.
Implement a private field like cachedPeerVerifierFactories and populate it from
the existing builder logic (or lazily initialize it on first call in
peerVerifierFactories()), ensuring the returned list is immutable; reference
peerVerifierFactories(), PinnedPeerVerifierFactory, SanPeerVerifierFactory and
TransportSocketSnapshot usage to locate where to compute/store the cached value.
In
`@xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/XdsEndpointUtil.java`:
- Around line 221-229: convertToEndpoint and attachTransportSocket both call
TransportSocketMatchUtil.selectTransportSocket causing duplication; change
attachTransportSocket to accept a pre-selected TransportSocketSnapshot (remove
its internal call to selectTransportSocket) and have convertToEndpoint call
TransportSocketMatchUtil.selectTransportSocket once, then call
attachTransportSocket(endpoint, matchedSnapshot, lbEndpoint,
localityLbEndpoints) to attach via
withAttr(XdsCommonUtil.TRANSPORT_SOCKET_SNAPSHOT_KEY, matchedSnapshot). Update
method signature of attachTransportSocket and all its call sites accordingly so
selection is performed only in convertToEndpoint.
| checkArgument(loadBalancer != null, "No endpoints found for name: %s", clusterName); | ||
| final Endpoint endpoint = loadBalancer.selectNow(ctx); | ||
| checkArgument(endpoint != null, "Endpoint not selected found for name: %s", clusterName); | ||
| XdsCommonUtil.setTlsParams(ctx, endpoint); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the relevant XDS files
find . -path "*/xds/*" -name "*.java" -type f | grep -E "(XdsCommon|Processor|Bootstrap)" | head -20Repository: line/armeria
Length of output: 624
🏁 Script executed:
# Search for TRANSPORT_SOCKET_SNAPSHOT_KEY usage
rg -n --type java "TRANSPORT_SOCKET_SNAPSHOT_KEY" -A 3 -B 3Repository: line/armeria
Length of output: 6151
🏁 Script executed:
# Search for BootstrapClusters usage
rg -n --type java "BootstrapClusters" -A 5 -B 2Repository: line/armeria
Length of output: 7513
🏁 Script executed:
# Find XdsCommonUtil and examine setTlsParams method
fd -t f "XdsCommonUtil.java" -x cat -n {}Repository: line/armeria
Length of output: 4411
🏁 Script executed:
# Examine BootstrapClusters implementation
cat -n xds/src/main/java/com/linecorp/armeria/xds/BootstrapClusters.javaRepository: line/armeria
Length of output: 4028
🏁 Script executed:
# Check GrpcServicesPreprocessor to see the call path
cat -n xds/src/main/java/com/linecorp/armeria/xds/GrpcServicesPreprocessor.javaRepository: line/armeria
Length of output: 4440
🏁 Script executed:
# Search for how bootstrap endpoints are created and which EndpointGroup they use
rg -n --type java "class BootstrapClusters" -A 100 | head -150Repository: line/armeria
Length of output: 6484
🏁 Script executed:
# Look for where endpoints are obtained/selected from bootstrap cluster
rg -n --type java "bootstrapClusters.*getEndpoint\|getSnapshot\|selectEndpoint" -B 2 -A 2Repository: line/armeria
Length of output: 38
🏁 Script executed:
# Find XdsLoadBalancer class
fd -t f "XdsLoadBalancer.java" -x cat -n {}Repository: line/armeria
Length of output: 1611
🏁 Script executed:
# Understand how ClusterSnapshot provides the loadBalancer
rg -n --type java "class ClusterSnapshot" -A 30Repository: line/armeria
Length of output: 3366
🏁 Script executed:
# Search for where endpoints are created/obtained in the cluster management
rg -n --type java "selectNow" -B 3 -A 3Repository: line/armeria
Length of output: 50369
🏁 Script executed:
# Check how XdsAttributeAssigningEndpointGroup is used
rg -n --type java "XdsAttributeAssigningEndpointGroup" -B 2 -A 2Repository: line/armeria
Length of output: 2289
🏁 Script executed:
# Search for how bootstrap clusters create or manage endpoints
rg -n --type java "BootstrapClusters\|bootstrap.*Endpoint" -B 2 -A 5 | grep -A 5 -B 2 "Endpoint\|endpoint"Repository: line/armeria
Length of output: 38
🏁 Script executed:
# Look for where XdsEndpointUtil is called for bootstrap clusters
rg -n --type java "XdsEndpointUtil\|createEndpoints" | grep -v testRepository: line/armeria
Length of output: 38
🏁 Script executed:
# Check if bootstrap cluster endpoints are wrapped with XdsAttributeAssigningEndpointGroup
rg -n --type java "Cluster\|bootstrap" -A 10 | grep -A 10 "XdsAttributeAssigningEndpointGroup"Repository: line/armeria
Length of output: 38
🏁 Script executed:
# Find all places where TRANSPORT_SOCKET_SNAPSHOT_KEY is set - to understand the scope
rg -n --type java "withAttr.*TRANSPORT_SOCKET_SNAPSHOT_KEY" -B 5 -A 1Repository: line/armeria
Length of output: 4455
🏁 Script executed:
# Search for where bootstrap cluster endpoints are created - look at XdsClusterManager
fd -t f "XdsClusterManager.java" -x cat -n {}Repository: line/armeria
Length of output: 5581
🏁 Script executed:
# Look for how endpoints are obtained from bootstrap clusters
rg -n --type java "class DefaultLoadBalancer" -A 50 | head -100Repository: line/armeria
Length of output: 6694
🏁 Script executed:
# Search for where endpoints are selected from a cluster snapshot
rg -n --type java "endpointSnapshot\|createEndpoints\|loadEndpoints" | head -30Repository: line/armeria
Length of output: 38
🏁 Script executed:
# Look at DefaultLoadBalancer more carefully to understand endpoint handling
fd -t f "DefaultLoadBalancer.java" -x cat -n {}Repository: line/armeria
Length of output: 13681
🏁 Script executed:
# Look for how ClusterSnapshot creates its endpoints - check where endpoints come from
fd -t f "ClusterStream.java" -x cat -n {} | head -150Repository: line/armeria
Length of output: 9018
🏁 Script executed:
# Look for how endpoints are initially populated for bootstrap clusters
rg -n --type java "class ClusterStream" -A 80 | head -150Repository: line/armeria
Length of output: 10031
🏁 Script executed:
# Search for where endpoints are created - understand the flow from cluster to endpoints
rg -n --type java "createEndpoints\|PrioritySet\|EndpointSnapshot" | grep -v test | head -40Repository: line/armeria
Length of output: 38
🏁 Script executed:
# Look for EndpointSnapshotNode to see how endpoints are created
fd -t f "EndpointSnapshotNode.java" -x cat -n {} | head -200Repository: line/armeria
Length of output: 38
🏁 Script executed:
# Search for where TRANSPORT_SOCKET_SNAPSHOT_KEY might NOT be set
rg -n --type java "Endpoint\.of\|new Endpoint" -B 2 -A 2 | head -60Repository: line/armeria
Length of output: 38
🏁 Script executed:
# Check if bootstrap clusters use a different endpoint creation path
rg -n --type java "bootstrap.*[Ee]ndpoint\|static.*[Ee]ndpoint" -B 3 -A 3 | head -80Repository: line/armeria
Length of output: 38
🏁 Script executed:
# Find EndpointSnapshotNode
rg -n --type java "class EndpointSnapshotNode" -A 100 | head -150Repository: line/armeria
Length of output: 5182
🏁 Script executed:
# Look for LoadBalancerStream to see how it uses endpoints
rg -n --type java "class LoadBalancerStream" -A 80 | head -150Repository: line/armeria
Length of output: 9069
🏁 Script executed:
# Search for any special handling of bootstrap clusters vs other clusters
rg -n --type java "bootstrap\|static.*[Cc]luster" -i | grep -v test | head -40Repository: line/armeria
Length of output: 38
🏁 Script executed:
# Check for any endpoint creation without attribute assignment
rg -n --type java "Endpoint\.\w+\(" | grep -v "attr\|withAttr" | head -30Repository: line/armeria
Length of output: 5201
🏁 Script executed:
# Look at how LoadBalancerStream passes transportSocket to loadBalancerFactory
fd -t f "XdsLoadBalancerFactory.java" -x head -200 {}Repository: line/armeria
Length of output: 8072
🏁 Script executed:
# Check the complete flow: Does every endpoint path ensure TRANSPORT_SOCKET_SNAPSHOT_KEY is set?
# Search for all places endpoints are returned from load balancers
rg -n --type java "selectNow.*{" -A 5 | grep -v test | head -60Repository: line/armeria
Length of output: 143
🏁 Script executed:
# Look for convertEndpointGroup to see how it handles endpoints
rg -n --type java "convertEndpointGroup" -A 30Repository: line/armeria
Length of output: 9106
🏁 Script executed:
# Search for XdsEndpointUtil.convertEndpointGroup implementation
fd -t f "XdsEndpointUtil.java" -x cat -n {} | grep -A 50 "convertEndpointGroup"Repository: line/armeria
Length of output: 3237
🏁 Script executed:
# Find convertLoadAssignment to see if it sets TRANSPORT_SOCKET_SNAPSHOT_KEY
fd -t f "XdsEndpointUtil.java" -x cat -n {} | grep -A 80 "convertLoadAssignment"Repository: line/armeria
Length of output: 7393
🏁 Script executed:
# Check if there's any code path where endpoints are created without the attribute
rg -n --type java "private static.*convertLoadAssignment\|private static.*EndpointGroup" -A 50 | head -150Repository: line/armeria
Length of output: 38
🏁 Script executed:
# Check TransportSocketMatchUtil.selectTransportSocket to see if it can return null
fd -t f "TransportSocketMatchUtil.java" -x cat -n {}Repository: line/armeria
Length of output: 4847
🏁 Script executed:
# Check if there are any other paths where endpoints might be created without the attribute
rg -n --type java "endpoint\.attr.*TRANSPORT_SOCKET_SNAPSHOT_KEY" -cRepository: line/armeria
Length of output: 128
🏁 Script executed:
# Verify bootstrap clusters use the same endpoint creation path as regular clusters
# by confirming they go through convertEndpointGroup
rg -n --type java "convertEndpointGroup\|BootstrapClusters" | grep -A 5 -B 5 "convertEndpointGroup\|bootstrap"Repository: line/armeria
Length of output: 38
Replace assert with proper null guard in XdsCommonUtil.setTlsParams.
The use of assert for defensive checks is problematic because assertions are disabled by default in production JVMs (no -ea flag). Replace the assertion with a proper null check.
However, the scenario described in the review is inaccurate: bootstrap cluster endpoints do NOT bypass attribute assignment. All bootstrap clusters (whether using STATIC, EDS, or STRICT_DNS cluster types) go through the same XdsEndpointUtil.convertEndpointGroup() flow as regular clusters, which consistently sets TRANSPORT_SOCKET_SNAPSHOT_KEY on endpoints during creation via either convertLoadAssignment() or XdsAttributeAssigningEndpointGroup.
That said, the fix is still warranted as a defensive programming practice. If the codebase evolves or new endpoint creation paths are added, this null check ensures safety rather than relying on the current flow guarantees.
🤖 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/GrpcServicesPreprocessor.java` at
line 74, Replace the assertion in XdsCommonUtil.setTlsParams with a proper null
guard: check the endpoint (or the attribute fetched via
TRANSPORT_SOCKET_SNAPSHOT_KEY) for null and return (or handle) early instead of
using assert, so XdsServicesPreprocessor and any callers (e.g.,
GrpcServicesPreprocessor invoking XdsCommonUtil.setTlsParams) do not rely on JVM
assertions; ensure the method explicitly validates its inputs and logs or safely
exits when the transport-socket snapshot is missing to prevent NPEs if future
code paths omit attribute assignment.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
xds/src/main/java/com/linecorp/armeria/xds/PinnedPeerVerifierFactory.java (1)
54-54: Remove the unusedengineparameter fromverifyPins.
SSLEngine engineis never referenced insideverifyPins. Removing it clarifies intent and avoids misleading readers into thinking the engine state affects pin validation.♻️ Proposed fix
- private void verifyPins(X509Certificate[] chain, SSLEngine engine) throws CertificateException { + private void verifyPins(X509Certificate[] chain) throws CertificateException {return (chain, authType, engine) -> { delegate.verify(chain, authType, engine); - verifyPins(chain, engine); + verifyPins(chain); };🤖 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/PinnedPeerVerifierFactory.java` at line 54, The verifyPins method declares an unused SSLEngine engine parameter—remove the engine parameter from the verifyPins(X509Certificate[] chain, SSLEngine engine) signature and adjust all call sites to call verifyPins(X509Certificate[] chain) instead; update any overrides or references (e.g., in PinnedPeerVerifierFactory and any classes referencing verifyPins) so the compiler and callers match the new signature and rebuild.xds/src/main/java/com/linecorp/armeria/xds/SanMatcher.java (1)
52-54: Local variabletypeshadows fieldthis.type— consider renaming for clarity.The local variable
type(anIntegerSAN type code from the JDK cert API) shadows the fieldthis.type(aSubjectAltNameMatcher.SanType). While the types differ so there's no functional ambiguity, a name likealtNameTypeCodewould avoid any potential confusion for future readers.♻️ Proposed rename
- final Integer type = altName.size() >= 2 ? (Integer) altName.get(0) : null; - if (type == null || !matchesType(type)) { + final Integer altNameTypeCode = altName.size() >= 2 ? (Integer) altName.get(0) : null; + if (altNameTypeCode == null || !matchesType(altNameTypeCode)) {🤖 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/SanMatcher.java` around lines 52 - 54, In SanMatcher, the local variable named type inside the loop over altNames shadows the field this.type; rename the local variable (e.g., altNameTypeCode) wherever it’s used in the loop so it no longer conflicts with the class field, and update the checks that use it (the creation from altName.get(0) and the subsequent matchesType(…) call) to use the new name to preserve behavior and clarity.xds/src/main/java/com/linecorp/armeria/xds/TransportSocketSnapshot.java (1)
172-180: Consider includingclientTlsSpecintoString()for easier TLS debugging.
equals()/hashCode()correctly excludeclientTlsSpecsince it's derived. However, when diagnosing TLS handshake issues, having the resolvedClientTlsSpecvisible in log output would be helpful without impacting correctness.♻️ Proposed addition to `toString()`
`@Override` public String toString() { return MoreObjects.toStringHelper(this) .omitNullValues() .add("transportSocket", transportSocket) .add("tlsCertificate", tlsCertificate) .add("validationContext", validationContext) + .add("clientTlsSpec", clientTlsSpec) .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/TransportSocketSnapshot.java` around lines 172 - 180, The current TransportSocketSnapshot.toString() omits the derived clientTlsSpec, making TLS troubleshooting harder; update the toString() implementation in TransportSocketSnapshot to include the clientTlsSpec field (e.g., add("clientTlsSpec", clientTlsSpec)) while keeping equals()/hashCode() unchanged so behavior stays correct; locate the toString() method in the TransportSocketSnapshot class and append the clientTlsSpec to the MoreObjects.toStringHelper chain, using omitNullValues() as appropriate.xds/src/main/java/com/linecorp/armeria/xds/internal/XdsCommonUtil.java (1)
93-105:checkArgumentvscheckStatesemantics.
checkArgumentsignals that the caller passed a bad value, but here the violated invariant is a pipeline guarantee (every endpoint in the xDS path must haveTRANSPORT_SOCKET_SNAPSHOT_KEYset).checkState(also from Guava) more accurately conveys "the system is in an unexpected state" and throwsIllegalStateExceptioninstead ofIllegalArgumentException, which is more meaningful to future debuggers. This is a very minor semantic distinction.♻️ Proposed change
- checkArgument(transportSocket != null, - "TransportSocket not set for selected endpoint: %s", endpoint); + checkState(transportSocket != null, + "TransportSocket not set for selected endpoint: %s", endpoint);🤖 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/internal/XdsCommonUtil.java` around lines 93 - 105, Replace the use of Guava's checkArgument in setTlsParams with checkState to reflect that missing TRANSPORT_SOCKET_SNAPSHOT_KEY on the Endpoint is an unexpected pipeline/state invariant (not a bad caller argument); update the call in XdsCommonUtil.setTlsParams to call checkState(transportSocket != null, "TransportSocket not set for selected endpoint: %s", endpoint) so an IllegalStateException is thrown and the intent is clearer while leaving the rest of the method (ClientTlsSpec handling and ctx updates) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@xds/src/main/java/com/linecorp/armeria/xds/SanMatcher.java`:
- Around line 89-102: The code in SanMatcher.toValueString converts byte[] SANs
to InetAddress.getHostAddress() but the xDS config string may be in expanded or
uppercase IPv6 form, causing mismatches; update the matching logic so both sides
are normalized to the same canonical form: when the configured expected SAN
string is used (wherever the comparison happens) attempt to parse it with
InetAddress.getByName(expected) and replace it with its getHostAddress() (or
alternatively compare InetAddress objects/byte[] directly), and ensure
toValueString(Object) still returns the canonical hostAddress for byte[] inputs
so comparisons are done against normalized IPv4/IPv6 representations (reference:
SanMatcher.toValueString).
In `@xds/src/main/java/com/linecorp/armeria/xds/TransportSocketSnapshot.java`:
- Line 48: The static field warnedNoVerify lacks proper cross-thread visibility;
update its declaration or usage so writes are visible to other threads—either
mark warnedNoVerify as volatile or replace it with an AtomicBoolean and change
warnNoVerifyOnce() to use atomic compareAndSet(false, true) to guarantee the log
emits only once; ensure the unique symbols warnedNoVerify and warnNoVerifyOnce()
are updated accordingly and remove any unsynchronized reads/writes.
---
Duplicate comments:
In `@xds/src/main/java/com/linecorp/armeria/xds/PinnedPeerVerifierFactory.java`:
- Around line 100-116: The equals/hashCode implementation in
PinnedPeerVerifierFactory was corrected to use content-based comparison by
switching spkiPins and certHashPins to List<ByteString>; confirm that the fields
referenced in equals(Object) and hashCode() are the immutable ByteString lists
(spkiPins, certHashPins) and leave the implementations of equals and hashCode as
shown (equals compares spkiPins.equals(that.spkiPins) &&
certHashPins.equals(that.certHashPins) and hashCode returns
Objects.hash(spkiPins, certHashPins)); no further changes are needed but ensure
the fields are instantiated as ImmutableList<ByteString> to preserve correct
semantics and immutability.
---
Nitpick comments:
In `@xds/src/main/java/com/linecorp/armeria/xds/internal/XdsCommonUtil.java`:
- Around line 93-105: Replace the use of Guava's checkArgument in setTlsParams
with checkState to reflect that missing TRANSPORT_SOCKET_SNAPSHOT_KEY on the
Endpoint is an unexpected pipeline/state invariant (not a bad caller argument);
update the call in XdsCommonUtil.setTlsParams to call checkState(transportSocket
!= null, "TransportSocket not set for selected endpoint: %s", endpoint) so an
IllegalStateException is thrown and the intent is clearer while leaving the rest
of the method (ClientTlsSpec handling and ctx updates) unchanged.
In `@xds/src/main/java/com/linecorp/armeria/xds/PinnedPeerVerifierFactory.java`:
- Line 54: The verifyPins method declares an unused SSLEngine engine
parameter—remove the engine parameter from the verifyPins(X509Certificate[]
chain, SSLEngine engine) signature and adjust all call sites to call
verifyPins(X509Certificate[] chain) instead; update any overrides or references
(e.g., in PinnedPeerVerifierFactory and any classes referencing verifyPins) so
the compiler and callers match the new signature and rebuild.
In `@xds/src/main/java/com/linecorp/armeria/xds/SanMatcher.java`:
- Around line 52-54: In SanMatcher, the local variable named type inside the
loop over altNames shadows the field this.type; rename the local variable (e.g.,
altNameTypeCode) wherever it’s used in the loop so it no longer conflicts with
the class field, and update the checks that use it (the creation from
altName.get(0) and the subsequent matchesType(…) call) to use the new name to
preserve behavior and clarity.
In `@xds/src/main/java/com/linecorp/armeria/xds/TransportSocketSnapshot.java`:
- Around line 172-180: The current TransportSocketSnapshot.toString() omits the
derived clientTlsSpec, making TLS troubleshooting harder; update the toString()
implementation in TransportSocketSnapshot to include the clientTlsSpec field
(e.g., add("clientTlsSpec", clientTlsSpec)) while keeping equals()/hashCode()
unchanged so behavior stays correct; locate the toString() method in the
TransportSocketSnapshot class and append the clientTlsSpec to the
MoreObjects.toStringHelper chain, using omitNullValues() as appropriate.
| @Nullable | ||
| private static String toValueString(Object value) throws CertificateException { | ||
| if (value instanceof String) { | ||
| return (String) value; | ||
| } | ||
| if (value instanceof byte[]) { | ||
| try { | ||
| return InetAddress.getByAddress((byte[]) value).getHostAddress(); | ||
| } catch (Exception e) { | ||
| throw new CertificateException("Failed to parse IP address SAN", e); | ||
| } | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
IPv6 SAN format mismatch risk: InetAddress.getHostAddress() output must exactly match the configured string.
For IP_ADDRESS SANs, the JDK returns a byte[] which is converted via InetAddress.getHostAddress(). For IPv4 this is unambiguous, but for IPv6 the JDK produces RFC 5952 compressed lowercase notation (e.g., "::1" or "fe80::1"). If the xDS config contains an IPv6 address in expanded form ("0:0:0:0:0:0:0:1") or uppercase hex ("FE80::1"), the comparison will silently return false — no error is raised, just a missed match. Consider normalizing the expected string as well, or documenting this constraint.
🤖 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/SanMatcher.java` around lines 89 -
102, The code in SanMatcher.toValueString converts byte[] SANs to
InetAddress.getHostAddress() but the xDS config string may be in expanded or
uppercase IPv6 form, causing mismatches; update the matching logic so both sides
are normalized to the same canonical form: when the configured expected SAN
string is used (wherever the comparison happens) attempt to parse it with
InetAddress.getByName(expected) and replace it with its getHostAddress() (or
alternatively compare InetAddress objects/byte[] directly), and ensure
toValueString(Object) still returns the canonical hostAddress for byte[] inputs
so comparisons are done against normalized IPv4/IPv6 representations (reference:
SanMatcher.toValueString).
There was a problem hiding this comment.
This matches upstream envoy's implementation where the canonical form of ipv6 is used
There was a problem hiding this comment.
@jrhee17, thank you for clarifying! I understand now—the implementation intentionally uses canonical IPv6 form to match Envoy's behavior. This is the correct approach for xDS compatibility.
✏️ Learnings added
Learnt from: jrhee17
Repo: line/armeria PR: 6628
File: xds/src/main/java/com/linecorp/armeria/xds/SanMatcher.java:89-102
Timestamp: 2026-02-19T06:03:17.212Z
Learning: In SanMatcher.java IPv6 SAN matching, the canonical form (as returned by InetAddress.getHostAddress()) is intentionally used to match upstream Envoy's implementation. This means IPv6 addresses must be configured in canonical lowercase compressed notation in xDS config.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| public final class TransportSocketSnapshot implements Snapshot<TransportSocket> { | ||
|
|
||
| private static final Logger logger = LoggerFactory.getLogger(TransportSocketSnapshot.class); | ||
| private static boolean warnedNoVerify; |
There was a problem hiding this comment.
warnedNoVerify is missing volatile — JMM visibility is not guaranteed across threads.
static boolean warnedNoVerify is read and written from warnNoVerifyOnce() without any synchronization or volatile declaration. The JMM does not guarantee that a write by one thread is visible to another, so multiple threads can observe false after the first write, causing duplicate log emissions indefinitely. Use volatile for visibility, or AtomicBoolean.compareAndSet(false, true) for a true once-only guarantee.
🔒 Proposed fix using `AtomicBoolean`
- private static boolean warnedNoVerify;
+ private static final java.util.concurrent.atomic.AtomicBoolean warnedNoVerify =
+ new java.util.concurrent.atomic.AtomicBoolean(); private static void warnNoVerifyOnce() {
- if (!warnedNoVerify) {
- warnedNoVerify = true;
+ if (warnedNoVerify.compareAndSet(false, true)) {
logger.warn("TLS peer verification is disabled ...");
}
}Also applies to: 144-151
🤖 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 48, The static field warnedNoVerify lacks proper cross-thread visibility;
update its declaration or usage so writes are visible to other threads—either
mark warnedNoVerify as volatile or replace it with an AtomicBoolean and change
warnNoVerifyOnce() to use atomic compareAndSet(false, true) to guarantee the log
emits only once; ensure the unique symbols warnedNoVerify and warnNoVerifyOnce()
are updated accordingly and remove any unsynchronized reads/writes.
There was a problem hiding this comment.
This path is always called from the event loop
There was a problem hiding this comment.
@jrhee17, thank you for the clarification! If TransportSocketSnapshot is always accessed from the same event loop thread, then concurrent access is impossible and the synchronization concern doesn't apply. The simple boolean is sufficient in a single-threaded execution model.
✏️ Learnings added
Learnt from: jrhee17
Repo: line/armeria PR: 6628
File: xds/src/main/java/com/linecorp/armeria/xds/TransportSocketSnapshot.java:48-48
Timestamp: 2026-02-19T06:03:54.715Z
Learning: In the XDS module (package `com.linecorp.armeria.xds`), XDS-related code paths including snapshot classes like `TransportSocketSnapshot` are always executed on the event loop, which provides single-threaded execution guarantees. Thread-safety mechanisms like `volatile` or `AtomicBoolean` are not required for static fields accessed only within these execution paths.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| //language=yaml | ||
| //language=YAML |
There was a problem hiding this comment.
nit:
| //language=yaml | |
| //language=YAML | |
| //language=YAML |
| } | ||
| final X509Certificate leaf = chain[0]; | ||
| final boolean spkiMatched = !spkiPins.isEmpty() && matchesSpki(leaf); | ||
| final boolean certMatched = !certHashPins.isEmpty() && matchesCertHash(leaf); |
There was a problem hiding this comment.
Minor) We may lazily check certHashPins if spkiMatched == false
| LoadBalancerInput::new) | ||
| .switchMap(input -> { | ||
| if (!input.endpointSnapshot.isPresent()) { | ||
| return SnapshotStream.just(Optional.empty()); |
There was a problem hiding this comment.
nit: SnapshotStream.empty()
| for (String pin : pins) { | ||
| decoded.add(ByteString.copyFrom(Base64.getDecoder().decode(pin))); | ||
| } | ||
| return ImmutableList.copyOf(decoded); |
There was a problem hiding this comment.
nit: Should we use ImmutableList.Builder rather than copying the list?
| for (String pin : pins) { | ||
| decoded.add(decodeHex(pin)); | ||
| } | ||
| return ImmutableList.copyOf(decoded); |
| if (normalized.length() % 2 != 0) { | ||
| throw new IllegalArgumentException("Invalid certificate hash length: " + pin); | ||
| } | ||
| return ByteString.copyFrom(BaseEncoding.base16().decode(normalized)); |
There was a problem hiding this comment.
Noted that verify_certificate_hash is a hex-encoded value.
2 participants
Motivation:
This PR extends xDS to fully support transport-socket TLS (including SAN/pin verification) for control-plane and upstream calls. It also tightens endpoint/LB wiring around transport socket metadata and consolidates string matching logic used by SAN validation. Integration tests move to the
itmodule to cover the new TLS and matching flows end-to-end.Modifications:
Transport socket TLS + verification (core xDS)
TransportSocketSnapshot,TransportSocketMatchSnapshot).ClientTlsSpecfrom certs + validation context + optional SAN/pin verifiers; warns once when falling back tonoVerify()iftrusted_cais absent andsystem_root_certsis unset.PinnedPeerVerifierFactory,SanPeerVerifierFactory,SanMatcher).SanMatcherJavadoc).Endpoint/LB wiring for transport sockets
XdsEndpointUtil,XdsAttributeAssigningEndpointGroup,XdsAttributeKeys).TransportSocketMatchUtil).DefaultXdsLoadBalancerFactory,XdsLoadBalancerFactory).Peer verifier components
StringMatcherImplis extracted as a common matcher used by SAN verification.Bootstrap + module moves
XdsBootstrapImpl,XdsBootstrapBuilder).XdsEndpointGroup/XdsPreprocessortests moved to theitmodule.Misc
equals/hashCode/toStringimplemented on snapshot types.Testing:
XdsPreprocessoris added