(POC) Verify xDS integration with istiod#6700
Draft
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:
📝 WalkthroughWalkthroughAdds delta xDS support and rewrites XDS streaming to an ADS-driven model with StateCoordinator and ActualStream implementations; refactors filter factory/config handling; exposes endpointIdentificationAlgorithm on ClientTlsSpec; and adds a new it:xds-istio integration test module with Istio/K3s helpers and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AdsXdsStream
participant ActualStream
participant StateCoordinator
participant Subscriber
Client->>AdsXdsStream: register interest(type,name) / start()
AdsXdsStream->>ActualStream: ensure stream exists / forward resourcesUpdated(type)
Note right of ActualStream: receives DiscoveryResponse / DeltaDiscoveryResponse
ActualStream->>StateCoordinator: onResourceUpdated(type,name,resource) / onAck(...)
StateCoordinator->>Subscriber: replay or onChanged / onResourceDoesNotExist
Subscriber->>Client: deliver snapshot / trigger downstream behavior
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
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/SwitchMapEagerStream.java (1)
51-67:⚠️ Potential issue | 🟠 MajorDon't invalidate the current inner stream before the replacement exists.
Line 53 advances
innerEpochbeforemapper.apply(snapshot)succeeds. If mapping fails, the oldinnerSubstays open but all of its future emissions are filtered out by the stale epoch, so this stream can stop forwarding the last good inner state until another upstream snapshot arrives.Suggested fix
final Subscription prevInnerSub = innerSub; - final long mappedEpoch = ++innerEpoch; + final long mappedEpoch = innerEpoch + 1; final SnapshotStream<? extends O> innerStream; try { innerStream = mapper.apply(snapshot); } catch (Throwable t) { emit(null, t); return; } + innerEpoch = mappedEpoch; innerSub = innerStream.subscribe((value, error) -> { if (mappedEpoch != innerEpoch) { return; } emit(value, error);🤖 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/SwitchMapEagerStream.java` around lines 51 - 67, The code increments innerEpoch before calling mapper.apply(snapshot), which causes the existing innerSub to be treated as stale if mapping throws and prevents forwarding of the last good inner state; change the sequence in SwitchMapEagerStream so you call mapper.apply(snapshot) first (capture innerStream), then increment innerEpoch and compute mappedEpoch, subscribe to innerStream (assigning innerSub) and only after a successful new subscription cancel or replace the previous innerSub; reference innerEpoch, mapper.apply(snapshot), innerStream, innerSub, prevInnerSub and emit when making the change.
🟡 Minor comments (17)
it/xds-istio/src/test/java/com/linecorp/armeria/it/xds/XdsResourceReader.java-1-2 (1)
1-2:⚠️ Potential issue | 🟡 MinorUpdate copyright year to 2026.
This is a new file and the copyright year should reflect the current year. As per coding guidelines, modified Java source files must start with
Copyright $today.year LY Corporation.Proposed fix
-/* - * Copyright 2024 LINE Corporation +/* + * Copyright 2026 LINE Corporation🤖 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/XdsResourceReader.java` around lines 1 - 2, Update the file header in XdsResourceReader to use the current year: replace the existing copyright line with "Copyright 2026 LINE Corporation" so the top-of-file comment matches the required format for modified Java sources.it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/PodCustomizer.java-42-44 (1)
42-44:⚠️ Potential issue | 🟡 MinorPotential NPE if
pod.getStatus()returns null.The default
isPodHealthyimplementation callspod.getStatus().getPhase()without a null check. While unlikely in normal Kubernetes operations, a freshly created pod might have a null status.🛡️ Proposed defensive fix
default boolean isPodHealthy(Pod pod) { - return "Running".equals(pod.getStatus().getPhase()); + return pod.getStatus() != null && "Running".equals(pod.getStatus().getPhase()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/PodCustomizer.java` around lines 42 - 44, The default isPodHealthy(Pod pod) may NPE when pod.getStatus() is null; update the isPodHealthy implementation to defensively check pod and pod.getStatus() (and optionally pod.getStatus().getPhase()) for null before calling getPhase(), and return false (or an explicit unhealthy value) when status or phase is missing so calling isPodHealthy won’t throw an NPE.it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioPodCustomizer.java-52-59 (1)
52-59:⚠️ Potential issue | 🟡 MinorPotential NPE if
pod.getStatus()returns null.Similar to the base interface,
pod.getStatus()could be null for a freshly created pod, causing an NPE at line 53 or 56.🛡️ Proposed defensive fix
`@Override` public boolean isPodHealthy(Pod pod) { + if (pod.getStatus() == null) { + return false; + } if (!"Running".equals(pod.getStatus().getPhase())) { return false; } final var statuses = pod.getStatus().getContainerStatuses();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioPodCustomizer.java` around lines 52 - 59, The isPodHealthy(Pod pod) method can NPE if pod.getStatus() is null; add a defensive null-check for pod.getStatus() at the start of isPodHealthy(Pod pod) (return false if null) before calling pod.getStatus().getPhase() or pod.getStatus().getContainerStatuses(), so subsequent uses of getPhase() and getContainerStatuses() are safe; update the logic in IstioPodCustomizer.isPodHealthy to perform that early null guard.it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioInstaller.java-80-86 (1)
80-86:⚠️ Potential issue | 🟡 MinorPotential NPE when namespace has no labels.
ns.getMetadata().getLabels()can returnnullfor namespaces that have no labels defined, causing an NPE when callingput().Proposed fix
private static void enableNamespaceInjection(KubernetesClient client, String namespaceName) { client.namespaces().withName(namespaceName).edit(ns -> { + if (ns.getMetadata().getLabels() == null) { + ns.getMetadata().setLabels(new java.util.HashMap<>()); + } ns.getMetadata().getLabels().put("istio-injection", "enabled"); return ns; }); logger.info("Labeled namespace '{}' with istio-injection=enabled", namespaceName); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioInstaller.java` around lines 80 - 86, enableNamespaceInjection may NPE when ns.getMetadata().getLabels() is null; update the edit lambda in enableNamespaceInjection to obtain the Namespace metadata, ensure labels is non-null (create and set a new Map when metadata.getLabels() == null) before calling put("istio-injection","enabled"), and then return the modified namespace; keep the logger.info call as-is to report namespaceName.it/xds-istio/src/test/java/com/linecorp/armeria/it/xds/XdsClientToSidecarTest.java-73-81 (1)
73-81:⚠️ Potential issue | 🟡 MinorLogging format issue: throwable not properly formatted.
The log statement
logger.info("Cluster snapshot: {}, t: ", snapshot, t)has a format string mismatch. Thetthrowable won't be logged with its stack trace because there's no{}placeholder for it, and SLF4J only treats the last argument as a throwable when there are no remaining placeholders.Proposed fix (applies to lines 74, 116, 156 similarly)
clusterRoot.addSnapshotWatcher((snapshot, t) -> { - logger.info("Cluster snapshot: {}, t: ", snapshot, t); + logger.info("Cluster snapshot: {}, error: {}", snapshot, t); if (snapshot != null) { snapshotRef.compareAndSet(null, snapshot); }Or if you want the full stack trace logged:
- logger.info("Cluster snapshot: {}, t: ", snapshot, t); + if (t != null) { + logger.info("Cluster snapshot: {}, error:", snapshot, t); + } else { + logger.info("Cluster snapshot: {}", snapshot); + }🤖 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 73 - 81, The log call inside clusterRoot.addSnapshotWatcher uses a mismatched format string so the throwable won't be logged correctly; update the logger calls (e.g. in the lambda passed to clusterRoot.addSnapshotWatcher) to either include a second {} placeholder if you want t.toString() (logger.info("Cluster snapshot: {}, t: {}", snapshot, t)) or, to log the full stack trace, remove the extra literal and pass the throwable as the last argument consumed by SLF4J (logger.info("Cluster snapshot: {}", snapshot, t)); apply the same change to the other similar logger calls referenced at the locations around lines with the snapshot/watchers (the other lambdas at the same sites).it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioTestExtension.java-1-14 (1)
1-14:⚠️ Potential issue | 🟡 MinorUse the current copyright year for this new file.
Line 2 should be
2026, since this file is being introduced in this April 2026 PR. As per coding guidelines, "modified Java source files must start withCopyright $today.year LY Corporation(do not churn the year on every edit)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioTestExtension.java` around lines 1 - 14, Update the file header year to the current year: change the copyright line in IstioTestExtension.java (the top-of-file comment for the class IstioTestExtension) from "2025" to "2026" so the file starts with "Copyright 2026 LY Corporation" per the project's header policy.it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioTestExtension.java-32-34 (1)
32-34:⚠️ Potential issue | 🟡 MinorJavadoc should say
Pod, notJob.The implementation creates a
Poddirectly and cleans up that pod infinally, so the current class doc points readers at the wrong Kubernetes resource type.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioTestExtension.java` around lines 32 - 34, The class Javadoc incorrectly states that tests run inside a Kubernetes Job; update the documentation on IstioTestExtension to say it runs tests inside a Kubernetes Pod (and reference IstioPodTest) to match the implementation which creates and cleans up a Pod; change “Job” to “Pod” and any related wording in the class-level comment to accurately describe the resource type and lifecycle managed by IstioTestExtension.xds/src/main/java/com/linecorp/armeria/xds/XdsConverterUtil.java-41-44 (1)
41-44:⚠️ Potential issue | 🟡 MinorUse
apiTypein the exception format argument.The message says “Unsupported apiType %s” but currently prints
configSource, which makes failures harder to debug.Suggested patch
- checkArgument(apiType == ApiType.GRPC || apiType == ApiType.DELTA_GRPC || - apiType == ApiType.AGGREGATED_GRPC || apiType == ApiType.AGGREGATED_DELTA_GRPC, - "Unsupported apiType %s. Only GRPC, DELTA_GRPC, AGGREGATED_GRPC and " + - "AGGREGATED_DELTA_GRPC are supported.", configSource); + checkArgument(apiType == ApiType.GRPC || apiType == ApiType.DELTA_GRPC || + apiType == ApiType.AGGREGATED_GRPC || apiType == ApiType.AGGREGATED_DELTA_GRPC, + "Unsupported apiType %s. Only GRPC, DELTA_GRPC, AGGREGATED_GRPC and " + + "AGGREGATED_DELTA_GRPC are supported.", apiType);As per coding guidelines: “Prefer early-return style and meaningful exception messages; use Guava
Preconditionswhere appropriate.”🤖 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/XdsConverterUtil.java` around lines 41 - 44, The checkArgument call in XdsConverterUtil.java uses the wrong format argument: the message "Unsupported apiType %s..." currently substitutes configSource instead of apiType; update the checkArgument invocation (the one calling com.google.common.base.Preconditions.checkArgument) to pass apiType as the format argument so the exception message prints the actual apiType value (keep the same condition and message text, only replace configSource with apiType) to improve diagnostics.xds/src/main/java/com/linecorp/armeria/xds/ListenerSnapshot.java-39-49 (1)
39-49:⚠️ Potential issue | 🟡 MinorRemove the unused
SubscriptionContext contextparameter from both constructors or store it as a field.The
contextparameter is accepted but never stored or used. This is inconsistent with other stream and snapshot classes in the codebase (RouteStream, EndpointStream, ClusterStream, etc.) which store context as a field. Either add a field to store it or remove the parameter entirely.🤖 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/ListenerSnapshot.java` around lines 39 - 49, The constructors for ListenerSnapshot accept a SubscriptionContext parameter but never store it; add a private final SubscriptionContext field to the ListenerSnapshot class (e.g., subscriptionContext) and assign it from the SubscriptionContext argument in both constructors (the one-arg delegating constructor and the main constructor), keeping existing initialization of listenerXdsResource, routeSnapshot, and downstreamFilter via FilterUtil.buildDownstreamFilter(listenerXdsResource.connectionManager()); ensure imports/annotations match existing code style (e.g., `@Nullable` if used) so the field is consistently available like other stream/snapshot classes.xds/src/main/java/com/linecorp/armeria/xds/SotwActualStream.java-1-15 (1)
1-15:⚠️ Potential issue | 🟡 MinorUse the current LY header on this new file.
This class still carries the old
LINE Corporationheader, so it doesn't match the repository template.As per coding guidelines, "modified Java source files must start with
Copyright $today.year LY Corporation."🤖 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/SotwActualStream.java` around lines 1 - 15, Update the file header to the current LY header template: replace the old "LINE Corporation" copyright block at the top of SotwActualStream.java with the repository's canonical header that begins "Copyright $today.year LY Corporation" and includes the standard Apache License 2.0 notice; keep the rest of the file (class SotwActualStream and its package/imports) unchanged.xds/src/main/java/com/linecorp/armeria/xds/RouteEntry.java-66-75 (1)
66-75:⚠️ Potential issue | 🟡 MinorUpdate
filterConfig()docs to match the merged lookup.
filterConfigsnow includes route-config and virtual-host entries as well, but the publicfilterConfig(String)contract still reads as route-only.As per coding guidelines, "Add Javadoc for all public classes and public/protected methods (except
com.linecorp.armeria.internal)."🤖 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/RouteEntry.java` around lines 66 - 75, The Javadoc for the public method filterConfig(String) in RouteEntry is stale: filterConfigs now merges route-config, virtual-host (vhost) and route-level per-filter configs via toParsedFilterConfigs and FilterUtil.mergeFilterConfigs, so update the filterConfig(String) doc to state it looks up merged config across route-config, vhost, and route levels and to describe the precedence (route-level overrides vhost, which overrides route-config). Also mention that filterConfigs is populated from routeResource.resource().getTypedPerFilterConfigMap(), vhostResource.resource().getTypedPerFilterConfigMap(), and route.getTypedPerFilterConfigMap() so callers understand the sources.it/xds-client/src/test/java/com/linecorp/armeria/xds/it/DeltaXdsResourceWatcherTest.java-260-269 (1)
260-269:⚠️ Potential issue | 🟡 MinorAssert the expected missing-resource error on delete.
isNotEmpty()will also pass on unrelated transport/setup failures, so this step can go green even if the CLUSTER/ENDPOINT missing signal regresses.Suggested fix
- await().untilAsserted(() -> assertThat(errors).isNotEmpty()); + await().untilAsserted(() -> assertThat(errors).anyMatch(error -> { + if (!(error instanceof MissingXdsResourceException)) { + return false; + } + final MissingXdsResourceException exception = (MissingXdsResourceException) error; + return (exception.type() == XdsType.CLUSTER || + exception.type() == XdsType.ENDPOINT) && + exception.name().equals(CLUSTER_NAME); + }));🤖 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/DeltaXdsResourceWatcherTest.java` around lines 260 - 269, Replace the broad existence check on errors after cache.setSnapshot(GROUP, Snapshot.create(..., "4")) with an assertion that at least one recorded error is the expected "missing-resource" error for the deleted CLUSTER/ENDPOINT; locate the errors collection used in the test and change await().untilAsserted(() -> assertThat(errors).isNotEmpty()) to assert that errors contains a throwable (or status/code) that identifies a missing-resource condition (e.g., check the exception type or status/message), so the test specifically verifies the CLUSTER/ENDPOINT missing-resource signal rather than any unrelated failure.xds/src/main/java/com/linecorp/armeria/xds/DeltaActualStream.java-1-2 (1)
1-2:⚠️ Potential issue | 🟡 MinorUpdate copyright year to 2026.
This is a new file created in 2026, but the copyright shows 2025.
Proposed fix
/* - * Copyright 2025 LY Corporation + * Copyright 2026 LY Corporation🤖 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/DeltaActualStream.java` around lines 1 - 2, Update the copyright header in DeltaActualStream.java from 2025 to 2026; open the file containing the class DeltaActualStream and change the year in the top-of-file copyright comment block to 2026 so the file reflects the correct creation year.xds/src/main/java/com/linecorp/armeria/xds/AdsXdsStream.java-1-2 (1)
1-2:⚠️ Potential issue | 🟡 MinorUpdate copyright year to 2026.
This is a new file, but the copyright header shows 2023. As per the coding guidelines, new Java source files should use the current year (
Copyright 2026 LY Corporation).Proposed fix
/* - * Copyright 2023 LINE Corporation + * Copyright 2026 LY Corporation * - * LINE Corporation licenses this file to you under the Apache License, + * LY Corporation licenses this file to you under the Apache License,🤖 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/AdsXdsStream.java` around lines 1 - 2, Update the file header in AdsXdsStream.java to use the current year and organization name per guidelines: replace the existing copyright line that reads "Copyright 2023 LINE Corporation" with "Copyright 2026 LY Corporation" (i.e., update the year to 2026 and the organization to LY Corporation) at the top of the file/class AdsXdsStream.xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/XdsEndpointUtil.java-150-151 (1)
150-151:⚠️ Potential issue | 🟡 MinorAdd
hasSocketAddress()validation to both locations to ensure explicit failure on missing socket_address.Since protobuf's
getSocketAddress()returns a default empty instance when socket_address is not set in the oneof, the code should validate its presence before use. This makes failures deterministic and prevents silent use of default values from malformed XDS data.Suggested fix
if (address.hasPipe()) { throw new UnsupportedOperationException( "Pipe addresses are not supported for STRICT_DNS cluster type"); } + checkArgument(address.hasSocketAddress(), + "lbEndpoint.endpoint.address.socket_address is missing"); final SocketAddress socketAddress = address.getSocketAddress();if (address.hasPipe()) { ... } + checkArgument(address.hasSocketAddress(), + "lbEndpoint.endpoint.address.socket_address is missing"); final SocketAddress socketAddress = address.getSocketAddress();Also applies to: 218-219
🤖 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 150 - 151, The code calls address.getSocketAddress() and then uses socketAddress.getAddress() (e.g., assigning dnsAddress) but does not validate that socket_address is actually present; add explicit hasSocketAddress() checks before calling getSocketAddress() in both places where address.getSocketAddress() is used (the two occurrences in XdsEndpointUtil.java), and if hasSocketAddress() is false throw a clear, deterministic exception (for example IllegalArgumentException or a custom parse/validation exception) with a message like "missing socket_address in Address" so malformed XDS data fails explicitly rather than using a default empty instance.it/xds-client/src/test/java/com/linecorp/armeria/xds/it/PipeEndpointTest.java-250-253 (1)
250-253:⚠️ Potential issue | 🟡 MinorClose explicitly created
ClientFactoryinstances in tests.These factories hold resources (event-loop threads) that should be released via try-with-resources or equivalent cleanup to prevent leaks across the test suite.
Suggested fix
final String bootstrapYaml = bootstrapTemplate.formatted(SOCKET_PATH); final Bootstrap bootstrap = XdsResourceReader.fromYaml(bootstrapYaml, Bootstrap.class); try (XdsBootstrap xdsBootstrap = XdsBootstrap.of(bootstrap); - XdsHttpPreprocessor preprocessor = - XdsHttpPreprocessor.ofListener("my-listener", xdsBootstrap)) { + XdsHttpPreprocessor preprocessor = + XdsHttpPreprocessor.ofListener("my-listener", xdsBootstrap); + ClientFactory factory = ClientFactory.insecure()) { final String response = WebClient.builder(preprocessor) - .factory(ClientFactory.insecure()) + .factory(factory) .build() .blocking() .get("/hello") .contentUtf8();try (XdsBootstrap xdsBootstrap = XdsBootstrap.of(bootstrap); XdsHttpPreprocessor preprocessor = - XdsHttpPreprocessor.ofListener("my-listener", xdsBootstrap)) { + XdsHttpPreprocessor.ofListener("my-listener", xdsBootstrap); + ClientFactory factory = ClientFactory.builder() + .connectTimeoutMillis(3000) + .build()) { // Wait until the listener snapshot is fully assembled (SDS secrets resolved) ... final AggregatedHttpResponse response = WebClient.builder(preprocessor) - .factory(ClientFactory.builder() - .connectTimeoutMillis(3000) - .build()) + .factory(factory) .build() .blocking() .get("/hello");Also applies to: 318-323
🤖 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/PipeEndpointTest.java` around lines 250 - 253, The test creates a ClientFactory via ClientFactory.insecure() inline in the WebClient builder (seen around the WebClient.builder(preprocessor).factory(ClientFactory.insecure()).build().blocking() call) and never closes it; change each occurrence to allocate the factory to a local variable (e.g., ClientFactory cf = ClientFactory.insecure()) and use try-with-resources or try/finally to call cf.close() after the WebClient/blocking call completes so the event-loop threads are released (apply same change to the other occurrence around the second WebClient.builder(...) block).xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/XdsEndpointUtil.java-208-210 (1)
208-210:⚠️ Potential issue | 🟡 MinorGuard against empty pipe paths before
Paths.get(...).An empty
pipe.pathwill resolve to the current working directory when converted to absolute path, which produces a misleading endpoint instead of a clear config error. Validate the path usingcheckArgumentwithStrings.isNullOrEmpty()before passing toPaths.get().Suggested fix
if (address.hasPipe()) { - final String pipePath = Paths.get(address.getPipe().getPath()).toAbsolutePath().toString(); + final String rawPipePath = address.getPipe().getPath(); + checkArgument(!Strings.isNullOrEmpty(rawPipePath), + "lbEndpoint.endpoint.address.pipe.path is empty"); + final String pipePath = Paths.get(rawPipePath).toAbsolutePath().toString(); return DomainSocketAddress.of(pipePath) .asEndpoint() .withAttr(XdsAttributeKeys.LB_ENDPOINT_KEY, lbEndpoint)🤖 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 - 210, Guard against empty pipe paths before calling Paths.get(...) in the XdsEndpointUtil code that handles address.hasPipe(): validate the pipe path string (the value used to create pipePath and passed into DomainSocketAddress.of) with a checkArgument(!Strings.isNullOrEmpty(pipe.getPath()), "...") and throw a clear IllegalArgumentException if empty; perform this validation before Paths.get(...) and keep using DomainSocketAddress.of(pipePath) after the check so an empty pipe.path yields a clear config error instead of resolving to the CWD.
🧹 Nitpick comments (18)
it/xds-istio/src/test/java/com/linecorp/armeria/it/xds/XdsResourceReader.java (2)
41-41: Consider adding a class-level Javadoc.While this is test utility code, a brief class-level Javadoc explaining the purpose (e.g., "Utility for parsing xDS/Envoy protobuf messages from YAML or JSON") would improve discoverability and maintainability.
🤖 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/XdsResourceReader.java` at line 41, Add a concise class-level Javadoc above the XdsResourceReader class declaration that describes its purpose (e.g., utility for parsing xDS/Envoy protobuf messages from YAML or JSON for tests), expected input formats, and typical usage; place it immediately above the "public final class XdsResourceReader" line and keep it brief (one or two sentences) to improve discoverability and maintainability.
79-103: Consider extracting common logic to reduce duplication.The
fromYamlandfromJsonmethods share identical reflection and merge logic. This could be consolidated into a private helper method.Proposed refactor
+ private static <T extends Message> T parse(JsonNode jsonNode, Class<T> clazz) { + try { + `@SuppressWarnings`("unchecked") + final Message.Builder builder = (Message.Builder) clazz.getMethod("newBuilder").invoke(null); + parser.merge(jsonNode.toString(), builder); + `@SuppressWarnings`("unchecked") + final T result = (T) builder.build(); + return result; + } catch (Exception e) { + throw new RuntimeException(e); + } + } + - `@SuppressWarnings`("unchecked") public static <T extends Message> T fromYaml(String yaml, Class<T> clazz) { - final Message.Builder builder; try { - builder = (Message.Builder) clazz.getMethod("newBuilder").invoke(null); final JsonNode jsonNode = mapper.reader().readTree(yaml); - parser.merge(jsonNode.toString(), builder); + return parse(jsonNode, clazz); } catch (Exception e) { throw new RuntimeException(e); } - return (T) builder.build(); } - `@SuppressWarnings`("unchecked") public static <T extends Message> T fromJson(String json, Class<T> clazz) { - final Message.Builder builder; try { - builder = (Message.Builder) clazz.getMethod("newBuilder").invoke(null); final JsonNode jsonNode = jsonMapper.reader().readTree(json); - parser.merge(jsonNode.toString(), builder); + return parse(jsonNode, clazz); } catch (Exception e) { throw new RuntimeException(e); } - return (T) builder.build(); }🤖 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/XdsResourceReader.java` around lines 79 - 103, Extract the duplicated reflection + merge logic from fromYaml and fromJson into a private helper (e.g., a static method like parseMessage or buildFromText) that takes the input text, Class<T> clazz and the Json mapper to use (mapper or jsonMapper), obtains the Message.Builder via clazz.getMethod("newBuilder").invoke(null), reads the JsonNode with the provided mapper.reader().readTree(text) and calls parser.merge(jsonNode.toString(), builder), then returns builder.build(); update fromYaml and fromJson to simply call this helper with the correct mapper (mapper vs jsonMapper) and remove the duplicated try/catch/reflection/merge blocks while keeping exception handling consistent.it/xds-client/src/test/java/com/linecorp/armeria/xds/it/DeltaXdsPreprocessorTest.java (2)
93-95: Addfinalmodifier for consistency with other extension declarations.All other
@RegisterExtensionfields in this class are declared asstatic final, buteventLoopis missing thefinalmodifier.Suggested fix
`@RegisterExtension` `@Order`(3) - static EventLoopExtension eventLoop = new EventLoopExtension(); + static final EventLoopExtension eventLoop = new EventLoopExtension();🤖 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/DeltaXdsPreprocessorTest.java` around lines 93 - 95, The EventLoopExtension field declaration annotated with `@RegisterExtension` and `@Order`(3) is missing the final modifier; update the declaration for the symbol eventLoop (type EventLoopExtension) to be static final to match the other extension fields for consistency.
168-173: Consider adding an explicit timeout for test reliability.The
await()call uses Awaitility's default timeout. Specifying an explicit timeout improves test readability and provides clearer control over the expected propagation time.Suggested fix
+import java.time.Duration; + // ... // Wait until requests reach helloServer2 final int newPort = helloServer2.httpPort(); - await().untilAsserted(() -> { + await().atMost(Duration.ofSeconds(10)).untilAsserted(() -> { assertThat(blockingClient.get("/hello").contentUtf8()).isEqualTo("world"); assertThat(ports).contains(newPort); });🤖 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/DeltaXdsPreprocessorTest.java` around lines 168 - 173, The Awaitility call uses the default timeout which can make the test flaky; update the await().untilAsserted(...) invocation in DeltaXdsPreprocessorTest (the block that checks blockingClient.get("/hello") and ports contains helloServer2.httpPort()) to specify an explicit timeout (e.g. await().atMost(Duration.ofSeconds(...)) or await().atMost(..., TimeUnit.SECONDS)) so the test fails fast and is more deterministic; ensure you add the necessary import for Duration or TimeUnit as needed.it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/DockerAvailableCondition.java (1)
25-33: Consider clarifying the double negative logic.The condition
!HostOnlyExtension.notRunningInPod()is a double negative that may be hard to follow. Consider extracting to a local variable or renaming for clarity.♻️ Optional refactor for readability
public ConditionEvaluationResult evaluateExecutionCondition(ExtensionContext context) { // When running inside a K8s pod, Docker is not available but the test should still run. - if (!HostOnlyExtension.notRunningInPod()) { + final boolean runningInPod = !HostOnlyExtension.notRunningInPod(); + if (runningInPod) { return ConditionEvaluationResult.enabled("Running inside K8s pod"); }Alternatively, consider adding a
runningInPod()method toHostOnlyExtensionfor clearer semantics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/DockerAvailableCondition.java` around lines 25 - 33, The code uses a confusing double-negative check `!HostOnlyExtension.notRunningInPod()` inside evaluateExecutionCondition; change this to clearer logic by introducing a local boolean like `runningInPod = HostOnlyExtension.notRunningInPod() == false` or (better) add/use a positively named method `HostOnlyExtension.runningInPod()` and replace the expression with `if (runningInPod)` to make semantics explicit when returning ConditionEvaluationResult.enabled("Running inside K8s pod"); keep the rest of evaluateExecutionCondition (DockerClientFactory.instance().isDockerAvailable() and the enabled/disabled returns) unchanged.it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/EnabledIfDockerAvailable.java (1)
25-28: Add Javadoc for the public annotation.This public annotation is missing Javadoc documentation. As per coding guidelines, public classes should have Javadoc (the
.testingpackage exempts@UnstableApibut not documentation requirements).📝 Proposed Javadoc
import org.junit.jupiter.api.extension.ExtendWith; +/** + * Enables the annotated test class or method only if Docker is available. + * When running inside a Kubernetes pod, the test is enabled regardless of Docker availability. + */ `@Target`({ElementType.TYPE, ElementType.METHOD}) `@Retention`(RetentionPolicy.RUNTIME) `@ExtendWith`(DockerAvailableCondition.class) public `@interface` EnabledIfDockerAvailable {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/EnabledIfDockerAvailable.java` around lines 25 - 28, Add a Javadoc comment for the public annotation EnabledIfDockerAvailable describing its purpose (marks tests to run only when Docker is available), scope (can be applied to types and methods), retention policy, and interaction with DockerAvailableCondition; place the Javadoc immediately above the annotation declaration (public `@interface` EnabledIfDockerAvailable {}) and reference DockerAvailableCondition.class in the description so readers understand the runtime behavior.it/xds-istio/build.gradle (1)
15-15: Consider usingimplementationinstead ofapifor test module dependencies.The
apiconfiguration exposesprotoc.pgv.java.stubtransitively to consumers. Since this is an integration test module (:it:xds-istio) that won't have downstream consumers,implementationortestImplementationwould be more appropriate.♻️ Suggested change
- api libs.protoc.pgv.java.stub + implementation libs.protoc.pgv.java.stub🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@it/xds-istio/build.gradle` at line 15, The dependency declaration "api libs.protoc.pgv.java.stub" in the integration test module :it:xds-istio should not use the public "api" configuration; change it to a non-transitive configuration (replace "api" with "implementation" or "testImplementation" depending on whether the stub is needed at runtime or only for tests) so the protoc.pgv.java.stub artifact is not exposed to consumers.it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioTestImage.java (1)
61-66: Include stderr in the error message fordocker savefailures.When
docker savefails, the current error message doesn't include any output from the process, making debugging difficult.♻️ Proposed improvement for better diagnostics
- final Process saveProcess = new ProcessBuilder("docker", "save", IMAGE_NAME) - .redirectOutput(tempFile.toFile()) - .start(); - if (saveProcess.waitFor() != 0) { - throw new IllegalStateException("docker save exited with non-zero status"); - } + final ProcessBuilder pb = new ProcessBuilder("docker", "save", IMAGE_NAME) + .redirectOutput(tempFile.toFile()) + .redirectErrorStream(false); + final Process saveProcess = pb.start(); + final int exitCode = saveProcess.waitFor(); + if (exitCode != 0) { + final String stderr = new String(saveProcess.getErrorStream().readAllBytes()); + throw new IllegalStateException( + "docker save exited with status " + exitCode + ": " + stderr); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioTestImage.java` around lines 61 - 66, The docker save failure throws an IllegalStateException without the process stderr; update the block that runs Process saveProcess (the ProcessBuilder("docker", "save", IMAGE_NAME")... start() and saveProcess.waitFor() check) to capture the process error stream when waitFor() != 0 and include that stderr text in the exception message (e.g., read saveProcess.getErrorStream() into a UTF-8 string, ensure streams are consumed/closed) so the thrown IllegalStateException contains the docker save stderr for diagnostics.it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioPodEntryPoint.java (2)
41-56: Argument parsing could miss edge cases.The loop condition
i < args.length - 1is correct for preventing out-of-bounds access, but if someone passes--portas the very last argument without a value, the error won't be caught—it'll silently remain at-1and fail later with a less clear message. Consider validating that expected arguments have their values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioPodEntryPoint.java` around lines 41 - 56, The argument parsing in main may silently leave className/methodName/serverFactory null or serverPort as -1 if a flag is last or has an invalid value; update the parsing and validation in main to check that for each flag (--class, --method, --server-factory, --port) there is a following token before reading args[i+1] (or iterate i < args.length and guard i+1 < args.length), catch NumberFormatException when parsing serverPort, and after the loop validate that className, methodName, serverFactory are non-null and serverPort != -1, otherwise throw an IllegalArgumentException (or print a clear error) indicating which flag is missing or invalid so the failure points to the correct argument.
69-71: Consider using a clearer idiom to keep the server running.
Thread.currentThread().join()blocks indefinitely by joining the current thread to itself. While this works, it's an unusual pattern. Consider using the server's built-in mechanism for clearer intent.♻️ Proposed clearer alternative
- sb.build().start().join(); - Thread.currentThread().join(); + final Server server = sb.build(); + server.start().join(); + server.blockUntilShutdown();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioPodEntryPoint.java` around lines 69 - 71, Replace the unusual Thread.currentThread().join() with the server's shutdown-wait mechanism: assign sb.build().start() to a Server variable (e.g., Server server = sb.build().start()) and then wait on server.closeFuture().join() (or server.blockUntilShutdown() if available) instead of joining the current thread; update the code around sb.build().start().join() and Thread.currentThread().join() in IstioPodEntryPoint to use the server variable and its close/wait method to keep the process running.it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioServerExtension.java (1)
198-216: Pod readiness check may be insufficient.Checking only
pod.getStatus().getPhase() == "Running"doesn't guarantee containers are ready to serve traffic. A pod can be in "Running" phase while its containers are still initializing or failing readiness probes.Consider checking container readiness conditions for more reliable detection:
Proposed enhancement
return pods.stream().anyMatch(pod -> { final String phase = pod.getStatus() != null ? pod.getStatus().getPhase() : null; - return "Running".equals(phase); + if (!"Running".equals(phase)) { + return false; + } + // Also verify containers are ready + if (pod.getStatus().getContainerStatuses() == null) { + return false; + } + return pod.getStatus().getContainerStatuses().stream() + .allMatch(cs -> Boolean.TRUE.equals(cs.getReady())); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioServerExtension.java` around lines 198 - 216, The pod readiness check in waitForReady currently only tests pod.getStatus().getPhase() == "Running" which can be insufficient; update the lambda inside K8sClusterHelper.poll in IstioServerExtension.waitForReady to also inspect each Pod's container statuses (pod.getStatus().getContainerStatuses()) and require that containerStatus.getReady() (or isReady) is true for all containers (in addition to phase == "Running"), ignoring null lists; ensure the poll returns true only when at least one pod meets both running phase and all containers ready.it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioState.java (1)
37-40: Add null validation for constructor parameters.Per coding guidelines, validate non-null inputs with
Objects.requireNonNull. While this is package-private, defensive validation prevents subtle bugs.Proposed fix
+import static java.util.Objects.requireNonNull; + final class IstioState implements SafeCloseable { ... private IstioState(KubernetesClient client, Path kubeconfigPath) { - this.client = client; - this.kubeconfigPath = kubeconfigPath; + this.client = requireNonNull(client, "client"); + this.kubeconfigPath = requireNonNull(kubeconfigPath, "kubeconfigPath"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioState.java` around lines 37 - 40, The IstioState constructor should defensively validate its inputs: in the IstioState(KubernetesClient client, Path kubeconfigPath) constructor, call Objects.requireNonNull on both client and kubeconfigPath before assigning to this.client and this.kubeconfigPath so nulls are rejected early and clear NPEs are thrown with informative messages.it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioClusterExtension.java (1)
62-64: Consider adding Javadoc for public static fields.
NAMESPACEandK8S_CLIENT_KEYare public fields used by other extensions to share state via the JUnit extension context store. Brief Javadoc would clarify their purpose for users extending this framework.Proposed documentation
+ /** + * The JUnit extension context namespace used for sharing state between Istio extensions. + */ public static final ExtensionContext.Namespace NAMESPACE = ExtensionContext.Namespace.create(IstioClusterExtension.class); + /** + * The key used to store the shared {`@link` KubernetesClient} in the extension context store. + */ public static final String K8S_CLIENT_KEY = "kubernetesClient";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioClusterExtension.java` around lines 62 - 64, Add Javadoc comments to the public static fields NAMESPACE and K8S_CLIENT_KEY in IstioClusterExtension: describe that NAMESPACE is the ExtensionContext.Namespace used by this extension (and collaborators) to store/retrieve shared state in the JUnit ExtensionContext store, and that K8S_CLIENT_KEY is the key name used to store the KubernetesClient instance in that store; keep the comments short and mention intended consumers (other extensions/tests) and the stored value types for clarity.xds/src/main/java/com/linecorp/armeria/xds/CompositeXdsStream.java (1)
28-34: Add an explicit null check forstreamSupplier.Failing early with a clear message here is better than an indirect NPE later in the loop.
Suggested patch
+import java.util.Objects; import java.util.Map; import java.util.function.Function; @@ CompositeXdsStream(Function<XdsType, XdsStream> streamSupplier) { + Objects.requireNonNull(streamSupplier, "streamSupplier"); final ImmutableMap.Builder<XdsType, XdsStream> streamMapBuilder = ImmutableMap.builder(); for (XdsType type : XdsType.discoverableTypes()) { streamMapBuilder.put(type, streamSupplier.apply(type)); } streamMap = streamMapBuilder.build(); }🤖 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/CompositeXdsStream.java` around lines 28 - 34, The CompositeXdsStream constructor should validate its streamSupplier parameter up front: add an explicit null check (e.g., Objects.requireNonNull(streamSupplier, "streamSupplier") or an if-check that throws a NullPointerException/IllegalArgumentException with a clear message) at the start of the constructor before using it in the loop that builds streamMap via XdsType.discoverableTypes() and streamMapBuilder; this ensures failures surface immediately and clearly when streamSupplier is null.xds/src/main/java/com/linecorp/armeria/xds/ListenerXdsResource.java (1)
91-97: Javadoc is now inaccurate after the filter chain extraction change.The Javadoc at line 92 states the
HttpConnectionManageris "contained in theListener#getListenerFiltersList()", but the new implementation also extracts it fromfilterChainsviaextractHcmFromFilterChains. Consider updating the documentation.📝 Suggested Javadoc update
/** - * The {`@link` HttpConnectionManager} contained in the {`@link` Listener#getListenerFiltersList()}. + * The {`@link` HttpConnectionManager} extracted from the {`@link` Listener}'s API listener + * or filter chains. */ `@Nullable` public HttpConnectionManager connectionManager() {🤖 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 91 - 97, Update the outdated Javadoc on ListenerXdsResource.connectionManager(): change the description so it no longer says the HttpConnectionManager is only "contained in the Listener#getListenerFiltersList()", and instead documents that the HttpConnectionManager may be obtained from either the listener's listenerFilters list or from filterChains via extractHcmFromFilterChains; reference the connectionManager() accessor and the extractHcmFromFilterChains extraction path to clarify both sources.xds/src/main/java/com/linecorp/armeria/xds/StateCoordinator.java (1)
131-142: Use static import forrequireNonNull.Line 134 uses fully-qualified
java.util.Objects.requireNonNull. Add a static import to match the pattern used in other files in this PR.Proposed fix
Add to imports:
import static java.util.Objects.requireNonNull;Then update line 134:
- watcher.onChanged((T) java.util.Objects.requireNonNull(state.resource(), "resource")); + watcher.onChanged((T) requireNonNull(state.resource(), "resource"));🤖 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/StateCoordinator.java` around lines 131 - 142, Replace the fully-qualified java.util.Objects.requireNonNull usage with a static import and call to requireNonNull: add the static import "import static java.util.Objects.requireNonNull;" and update the call in StateCoordinator where watcher.onChanged((T) java.util.Objects.requireNonNull(state.resource(), "resource")); to use requireNonNull(state.resource(), "resource") so it matches the project's import/style pattern around watcher.onChanged and state.resource().xds/src/main/java/com/linecorp/armeria/xds/DeltaActualStream.java (1)
275-279: Remove redundantinstanceofcheck.The generic type
OinhandleResponseextendsXdsResource, soholder.parsedResources()returnsMap<String, O>where values are alwaysXdsResourceinstances. Theinstanceof XdsResourcecheck is always true.Proposed fix
holder.parsedResources().forEach((name, resource) -> { - if (resource instanceof XdsResource) { - stateCoordinator.onResourceUpdated(type, name, (XdsResource) resource); - } + stateCoordinator.onResourceUpdated(type, name, resource); });🤖 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/DeltaActualStream.java` around lines 275 - 279, Remove the redundant runtime check in handleResponse: holder.parsedResources() returns values of generic type O which extends XdsResource, so the "instanceof XdsResource" guard is always true; change the lambda to directly cast or pass the resource as (XdsResource) where needed and call stateCoordinator.onResourceUpdated(type, name, (XdsResource)resource) (or drop the cast if signature accepts O), removing the instanceof branch around holder.parsedResources() and using the existing resource variable directly in the call to stateCoordinator.onResourceUpdated.xds/src/main/java/com/linecorp/armeria/xds/AdsXdsStream.java (1)
135-139: Handle exhausted backoff more explicitly.When
backoff.nextDelayMillis()returns a negative value (backoff exhausted), the method silently returns without scheduling a retry. The stream is left in a state wherestopped=falsebutactualStream=nullwith no pending retry. Consider closing the stream or logging this terminal condition.Proposed fix
final long nextDelayMillis = backoff.nextDelayMillis(connBackoffAttempts); if (nextDelayMillis < 0) { + // Backoff exhausted; close the stream + stop(); return; } eventLoop.schedule(this::reset, nextDelayMillis, TimeUnit.MILLISECONDS);🤖 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/AdsXdsStream.java` around lines 135 - 139, When backoff.nextDelayMillis(connBackoffAttempts) returns a negative value, explicitly handle the exhausted-backoff terminal state instead of silently returning: detect the negative nextDelayMillis and then mark the AdsXdsStream as stopped (set stopped = true), cancel/close any existing actualStream (if actualStream != null) and clear it, and log or emit an error indicating the backoff is exhausted; remove the silent return and only call eventLoop.schedule(this::reset, ...) when nextDelayMillis >= 0 so reset is never left pending when backoff is exhausted.
Comment on lines
+44
to
45
| private Set<String> alpnProtocols = ImmutableSet.of(); | ||
| private Consumer<? super SslContextBuilder> tlsCustomizer = SslContextUtil.DEFAULT_NOOP_CUSTOMIZER; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and read the ClientTlsSpecBuilder.java file
find . -name "ClientTlsSpecBuilder.java" -type fRepository: line/armeria
Length of output: 130
🏁 Script executed:
# Let's also locate SslContextUtil to verify the applyCommonConfigs behavior
find . -name "SslContextUtil.java" -type fRepository: line/armeria
Length of output: 138
🏁 Script executed:
# Check the current state of ClientTlsSpecBuilder.java around the specified lines
cat -n core/src/main/java/com/linecorp/armeria/client/ClientTlsSpecBuilder.java | sed -n '1,120p'Repository: line/armeria
Length of output: 5445
🏁 Script executed:
# Let's find and examine SslContextUtil.applyCommonConfigs
cat -n core/src/main/java/com/linecorp/armeria/internal/client/SslContextUtil.java | head -200Repository: line/armeria
Length of output: 163
🏁 Script executed:
# Let's read the actual SslContextUtil file
cat -n core/src/main/java/com/linecorp/armeria/internal/common/util/SslContextUtil.java | head -300Repository: line/armeria
Length of output: 15942
🏁 Script executed:
# Search for the applyCommonConfigs method specifically
rg -A 30 "applyCommonConfigs" core/src/main/java/com/linecorp/armeria/internal/common/util/SslContextUtil.javaRepository: line/armeria
Length of output: 4133
🏁 Script executed:
# Let's also check ClientTlsSpec to understand the full flow
find . -name "ClientTlsSpec.java" -type f -not -path "*/test/*"Repository: line/armeria
Length of output: 123
🏁 Script executed:
# Check the ClientTlsSpec.java implementation
cat -n core/src/main/java/com/linecorp/armeria/client/ClientTlsSpec.java | head -200Repository: line/armeria
Length of output: 3810
🏁 Script executed:
# Search for tests using ClientTlsSpec or testing the default spec
find . -path "*/test/*" -name "*ClientTlsSpec*" -type fRepository: line/armeria
Length of output: 127
🏁 Script executed:
# Search for usages of ClientTlsSpec.of() or builder().build()
rg -A 3 "ClientTlsSpec\.of\(\)" --type javaRepository: line/armeria
Length of output: 3893
🏁 Script executed:
# Search for tests that might exercise the ALPN configuration
rg -B 3 -A 5 "alpnProtocols|ALPN" core/src/test --type java | head -100Repository: line/armeria
Length of output: 4663
🏁 Script executed:
# Check if there's any guard against empty ALPN in ClientTlsSpec or AbstractTlsSpec
cat -n core/src/main/java/com/linecorp/armeria/common/AbstractTlsSpec.java | head -100Repository: line/armeria
Length of output: 4448
🏁 Script executed:
# Check the ClientTlsSpecTest to see what's tested
cat -n core/src/test/java/com/linecorp/armeria/client/ClientTlsSpecTest.javaRepository: line/armeria
Length of output: 9350
🏁 Script executed:
# Look at TlsSpecPerRequestTest to understand the empty ALPN handling
rg -B 10 -A 10 "alpnProtocols\(\)\.isEmpty" core/src/test/java/com/linecorp/armeria/common/tls/TlsSpecPerRequestTest.javaRepository: line/armeria
Length of output: 38
🏁 Script executed:
# Search for any existing handling of empty ALPN in the codebase
rg -B 5 -A 5 "ApplicationProtocolConfig.DISABLED|isEmpty.*alpn" --type javaRepository: line/armeria
Length of output: 2914
🏁 Script executed:
# Check if there are any recent changes or TODOs related to ALPN
rg -B 3 -A 3 "TODO|FIXME|BUG" core/src/main/java/com/linecorp/armeria/client/ClientTlsSpecBuilder.javaRepository: line/armeria
Length of output: 38
🏁 Script executed:
# Check if tests actually create SslContext from default spec
rg -B 5 -A 10 "toSslContext.*ClientTlsSpec\.of\(\)|ClientTlsSpec\.of\(\).*toSslContext" --type javaRepository: line/armeria
Length of output: 1924
🏁 Script executed:
# Look at the TlsSpecPerRequestTest more carefully
cat -n core/src/test/java/com/linecorp/armeria/common/tls/TlsSpecPerRequestTest.java | grep -A 30 -B 5 "isEmpty"Repository: line/armeria
Length of output: 2354
🏁 Script executed:
# Check Netty ApplicationProtocolConfig to see how it handles empty protocols
# Look for any validation or special handling in the codebase
rg "ApplicationProtocolConfig" core/src/main/java --type java -A 5 | head -50Repository: line/armeria
Length of output: 2233
🏁 Script executed:
# Search for any recent changes or comments about ALPN empty handling
git log --oneline --all -S "alpnProtocols" --max-count=20 2>/dev/null || rg "alpnProtocols" core/src/main/java/com/linecorp/armeria/client/ClientTlsSpecBuilder.java -B 2 -A 2Repository: line/armeria
Length of output: 142
🏁 Script executed:
# Let's try to understand if there's any transformation of empty ALPN before it reaches ApplicationProtocolConfig
rg -B 10 -A 10 "alpnProtocols\(\)" core/src/main/java/com/linecorp/armeria/internal/common/util/SslContextUtil.javaRepository: line/armeria
Length of output: 1105
🏁 Script executed:
# Check if the test that asserts empty ALPN actually successfully creates an SslContext
cat -n core/src/test/java/com/linecorp/armeria/common/tls/TlsSpecPerRequestTest.java | sed -n '260,280p'Repository: line/armeria
Length of output: 1466
🏁 Script executed:
# Let's verify what happens when we create an SslContext from the default spec
# by looking at actual test execution paths
rg -B 5 -A 15 "class TlsSpecPerRequestTest" core/src/test/java/com/linecorp/armeria/common/tls/TlsSpecPerRequestTest.java | head -50Repository: line/armeria
Length of output: 704
🏁 Script executed:
# Check if there's a test that directly tests SslContext creation with empty ALPN
rg -B 3 -A 10 "toSslContext.*ClientTlsSpec" core/src/test --type java | head -50Repository: line/armeria
Length of output: 1848
🏁 Script executed:
# Let's check if there's special handling when ALPN is used with SessionProtocol
# Search for where SessionProtocol affects ALPN
rg -B 5 -A 10 "SessionProtocol" core/src/main/java/com/linecorp/armeria/client/ClientTlsSpecBuilder.javaRepository: line/armeria
Length of output: 1744
🏁 Script executed:
# Let's look at how the WebClient actually uses the ClientTlsSpec
# Check if it overwrites ALPN based on SessionProtocol before creating SslContext
rg -B 3 -A 10 "alpnProtocols\(SessionProtocol" core/src/main/java --type javaRepository: line/armeria
Length of output: 1576
🏁 Script executed:
# Let's check if there's any code that sets ALPN based on SessionProtocol when building the connection
find . -name "*.java" -path "*/client/*" -type f -not -path "*/test/*" | xargs grep -l "SessionProtocol" | head -5Repository: line/armeria
Length of output: 542
🏁 Script executed:
# Check HttpClientFactory or similar to see how ALPN is actually set
rg -B 5 -A 10 "toSslContext.*ClientTlsSpec" core/src/main/java/com/linecorp/armeria/client --type java | head -80Repository: line/armeria
Length of output: 38
🏁 Script executed:
# Search for where alpnProtocols(SessionProtocol) is called
rg "alpnProtocols\(.*SessionProtocol\)" core/src/main/java --type javaRepository: line/armeria
Length of output: 38
🏁 Script executed:
# Let's look for HttpClientConnectionFactory or similar to see where ALPN is set
find . -name "*ClientConnectionFactory.java" -o -name "*HttpClient*.java" | grep -v test | head -10Repository: line/armeria
Length of output: 897
🏁 Script executed:
# Search for where ClientTlsSpec is actually used when creating connections
rg -B 5 -A 10 "ClientTlsSpec\b" core/src/main/java/com/linecorp/armeria/client/HttpClientFactory.java 2>/dev/null || echo "File not found, searching alternatives"Repository: line/armeria
Length of output: 1930
🏁 Script executed:
# Let's search broadly for where ALPN gets set from SessionProtocol
rg -B 3 -A 8 "\.alpnProtocols\(sessionProtocol\)" core/src/main/java --type javaRepository: line/armeria
Length of output: 4582
🏁 Script executed:
# Now let's check the exact flow in BootstrapSslContexts to confirm
cat -n core/src/main/java/com/linecorp/armeria/client/BootstrapSslContexts.java | head -80Repository: line/armeria
Length of output: 3720
🏁 Script executed:
# Check HttpClientDelegate more carefully
cat -n core/src/main/java/com/linecorp/armeria/client/HttpClientDelegate.java | sed -n '1,100p'Repository: line/armeria
Length of output: 5548
🏁 Script executed:
# Search for direct calls to SslContextUtil.toSslContext with empty ALPN
rg -B 5 -A 5 "SslContextUtil\.toSslContext\(ClientTlsSpec" core/src/main --type javaRepository: line/armeria
Length of output: 1430
🌐 Web query:
Netty ApplicationProtocolConfig empty supportedProtocols constructor validation
💡 Result:
The Netty ApplicationProtocolConfig class constructors do not perform validation that rejects empty or null supportedProtocols lists. The official Netty API documentation for versions 4.0, 4.1, and 4.2 shows two constructors: one accepting Iterable supportedProtocols and one accepting varargs String... supportedProtocols. No javadoc or description mentions throwing exceptions for empty inputs, and no explicit null/empty checks are visible in the provided API excerpts or xref sources. Instead, use the static field ApplicationProtocolConfig.DISABLED to explicitly disable application protocol negotiation, which is the recommended way when no protocols are supported. For example: SslContextBuilder.forServer(...) .applicationProtocolConfig(ApplicationProtocolConfig.DISABLED) .build; Passing an empty list to the constructors likely results in an empty supportedProtocols list returned by the instance, but protocol negotiation would effectively be disabled or fail depending on the SslProvider (JDK/OpenSSL) and behaviors specified. No sources indicate runtime validation errors in the constructor itself; any issues would arise later during SSL handshake if no protocols match. This is confirmed across Netty API references (4.2.6.Final, 4.1.130.Final, 4.0.56.Final) and GitHub repository details (latest 4.2 branch as of 2026-03-27).
Citations:
- 1: https://netty.io/4.2/api/io/netty/handler/ssl/ApplicationProtocolConfig.html
- 2: https://netty.io/4.2/api/io/netty/handler/ssl/ApplicationProtocolConfig.Protocol.html
- 3: https://netty.io/4.1/api/index.html?io%2Fnetty%2Fhandler%2Fssl%2FApplicationProtocolConfig.html=
- 4: https://netty.io/4.0/api/index.html?io%2Fnetty%2Fhandler%2Fssl%2FApplicationProtocolConfig.html=
- 5: https://github.com/netty/netty
- 6: https://netty.io/4.0/api/io/netty/handler/ssl/ApplicationProtocolConfig.html
SslContextUtil.applyCommonConfigs() must guard against empty ALPN protocols.
When ClientTlsSpec.of() or any spec with empty alpnProtocols reaches applyCommonConfigs(), the code unconditionally creates new ApplicationProtocolConfig(..., tlsSpec.alpnProtocols()) with an empty protocol list. Netty's documented best practice is to use ApplicationProtocolConfig.DISABLED when ALPN is not configured, not an empty protocol list. Passing an empty list bypasses this and results in undefined protocol negotiation behavior at handshake time. This causes issues in code paths that create SslContext from a default or partially configured spec, such as in Flags.java when dumpOpenSslInfo is enabled.
Suggested fix in SslContextUtil.applyCommonConfigs()
- final ApplicationProtocolConfig alpnConfig = new ApplicationProtocolConfig(
- Protocol.ALPN,
- // NO_ADVERTISE is currently the only mode supported by both OpenSsl and JDK providers.
- SelectorFailureBehavior.NO_ADVERTISE,
- // ACCEPT is currently the only mode supported by both OpenSsl and JDK providers.
- SelectedListenerFailureBehavior.ACCEPT, tlsSpec.alpnProtocols());
- builder.applicationProtocolConfig(alpnConfig);
+ if (tlsSpec.alpnProtocols().isEmpty()) {
+ builder.applicationProtocolConfig(ApplicationProtocolConfig.DISABLED);
+ } else {
+ final ApplicationProtocolConfig alpnConfig = new ApplicationProtocolConfig(
+ Protocol.ALPN,
+ // NO_ADVERTISE is currently the only mode supported by both OpenSsl and JDK providers.
+ SelectorFailureBehavior.NO_ADVERTISE,
+ // ACCEPT is currently the only mode supported by both OpenSsl and JDK providers.
+ SelectedListenerFailureBehavior.ACCEPT, tlsSpec.alpnProtocols());
+ builder.applicationProtocolConfig(alpnConfig);
+ }🤖 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 44 - 45, SslContextUtil.applyCommonConfigs() currently constructs
an ApplicationProtocolConfig using tlsSpec.alpnProtocols() even when that set is
empty; change the logic to detect an empty or null alpnProtocols and use
ApplicationProtocolConfig.DISABLED instead of creating a new
ApplicationProtocolConfig with an empty list. In practice, update the branch
that builds the ApplicationProtocolConfig (referencing
SslContextUtil.applyCommonConfigs and ClientTlsSpec.alpnProtocols()) to check
CollectionUtils.isEmpty(tlsSpec.alpnProtocols()) (or equivalent) and substitute
ApplicationProtocolConfig.DISABLED so Netty uses the documented disabled ALPN
behavior rather than an empty protocol list.
Comment on lines
+120
to
+152
| private static void applyOperation(Protocol protocol, Scenario scenario, Target target, Operation operation, | ||
| RecordingWatcher<ListenerSnapshot> listenerWatcher) throws Exception { | ||
| final ResourceVariants baseline = ResourceVariants.v1(); | ||
| cache.setSnapshot(GROUP, snapshotFor(protocol, scenario, baseline, null)); | ||
| awaitExpectedState(baseline, listenerWatcher); | ||
|
|
||
| switch (operation) { | ||
| case ADD: | ||
| return; | ||
| case MODIFY: | ||
| cache.setSnapshot(GROUP, snapshotFor(protocol, scenario, | ||
| baseline.with(target, Variant.V2), null)); | ||
| awaitExpectedState(baseline.with(target, Variant.V2), listenerWatcher); | ||
| return; | ||
| case DELETE: | ||
| listenerWatcher.clearErrors(); | ||
| cache.setSnapshot(GROUP, snapshotFor(protocol, scenario, baseline, target)); | ||
| if (deleteSignalsMissing(protocol) && isDynamicTarget(scenario, target)) { | ||
| awaitMissing(target, listenerWatcher); | ||
| } | ||
| return; | ||
| case READD: | ||
| // Treat re-add as a direct update; delete+readd is not reliably observable for all types. | ||
| cache.setSnapshot(GROUP, snapshotFor(protocol, scenario, | ||
| baseline.with(target, Variant.V2), null)); | ||
| awaitExpectedState(baseline.with(target, Variant.V2), listenerWatcher); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| private static boolean deleteSignalsMissing(Protocol protocol) { | ||
| return protocol == Protocol.ADS_DELTA || protocol == Protocol.DELTA_GRPC; | ||
| } |
Contributor
There was a problem hiding this comment.
Several matrix rows don't execute the transition named by the case.
ADD returns after the full baseline is already applied, READD is just another modify, and deleteSignalsMissing() skips the SOTW listener/cluster delete assertions. A sizeable part of the matrix can therefore pass without validating the add/re-add/delete behavior it claims to cover.
🤖 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/XdsControlPlaneMatrixTest.java`
around lines 120 - 152, applyOperation currently returns without exercising ADD
and treats READD as MODIFY while DELETE short-circuits some assertion paths;
update applyOperation so ADD actually applies a snapshot that includes the new
target (use cache.setSnapshot with baseline.with(target, Variant.V2) and then
awaitExpectedState), change READD to perform a true re-add flow (simulate delete
then re-add: set snapshot removing the target, awaitMissing if applicable, then
set snapshot adding target back and awaitExpectedState), and ensure DELETE
always runs the SOTW listener/cluster delete assertions rather than skipping
them when deleteSignalsMissing(protocol); adjust calls to
deleteSignalsMissing(protocol) and isDynamicTarget(scenario, target) so
awaitMissing is only conditional for protocols that signal missing, but the SOTW
assertions path still executes for all protocols; refer to applyOperation, ADD,
READD, DELETE, deleteSignalsMissing, isDynamicTarget, awaitExpectedState, and
awaitMissing when making changes.
Comment on lines
+141
to
+152
| final boolean healthy = K8sClusterHelper.poll( | ||
| Duration.ofMinutes(5), K8sClusterHelper.DEFAULT_POLL_INTERVAL, () -> { | ||
| final Pod pod = client.pods().inNamespace(namespace).withName(podName).get(); | ||
| if (pod == null || pod.getStatus() == null) { | ||
| return false; | ||
| } | ||
| return podCustomizer.isPodHealthy(pod); | ||
| }); | ||
| if (!healthy) { | ||
| throw new IllegalStateException( | ||
| "Timed out waiting for pod '" + podName + "' to become healthy"); | ||
| } |
Contributor
There was a problem hiding this comment.
Fail fast on startup failure states.
This poll treats every non-healthy state the same. If the pod reaches an unrecoverable startup state, the test burns the full five-minute timeout and then deletes the pod with only a generic health-timeout error, which makes infra failures slow and hard to diagnose. Capture terminal status and logs here before cleanup.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioTestExtension.java`
around lines 141 - 152, The poll currently treats every non-healthy state the
same and waits the full timeout; update the polling lambda used by
K8sClusterHelper.poll to detect terminal/unrecoverable pod states (e.g.,
Pod.getStatus().getPhase() == "Failed" or containerStatuses showing
CrashLoopBackOff/terminated with non-zero exit code) by retrieving the Pod via
client.pods().inNamespace(namespace).withName(podName).get(), and when such a
terminal state is observed immediately log or capture container logs (via
client.pods().inNamespace(namespace).withName(podName).getLog(...) or
equivalent) and either include those details in the thrown IllegalStateException
or return a special flag to short-circuit the wait so cleanup can inspect logs
before deletion; keep using podCustomizer.isPodHealthy(pod) for healthy checks
but add this terminal-state branch to fail fast with collected logs/context
instead of waiting the full timeout.
Comment on lines
+71
to
+76
| // Also handle the legacy match_subject_alt_names field (used by older Istio versions). | ||
| // SPIFFE IDs are URIs, so each legacy StringMatcher is treated as a URI SAN matcher. | ||
| for (StringMatcher matcher : resource.getMatchSubjectAltNamesList()) { | ||
| parsedSanMatchers.add(new SanMatcher(SubjectAltNameMatcher.SanType.URI, | ||
| new StringMatcherImpl(matcher))); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the CertificateValidationContextSnapshot.java file to understand the context
fd -t f "CertificateValidationContextSnapshot.java" xds/Repository: line/armeria
Length of output: 140
🏁 Script executed:
# Let's look at the full method context around lines 71-76
cat -n xds/src/main/java/com/linecorp/armeria/xds/CertificateValidationContextSnapshot.java | head -100Repository: line/armeria
Length of output: 5304
🏁 Script executed:
# Examine SanMatcher class to understand its implementation
fd -t f "SanMatcher.java" xds/Repository: line/armeria
Length of output: 114
🏁 Script executed:
# Look at SubjectAltNameMatcher to understand SAN type handling
fd -t f "SubjectAltNameMatcher.java" xds/Repository: line/armeria
Length of output: 38
🏁 Script executed:
# Get full SanMatcher.java
cat xds/src/main/java/com/linecorp/armeria/xds/SanMatcher.javaRepository: line/armeria
Length of output: 5564
🏁 Script executed:
# Find SubjectAltNameMatcher references to understand the class
fd -t f "SubjectAltNameMatcher.java"Repository: line/armeria
Length of output: 38
🏁 Script executed:
# Look for proto files that define CertificateValidationContext
fd -t f "common.proto"Repository: line/armeria
Length of output: 785
🏁 Script executed:
# Search for match_subject_alt_names references in the codebase
rg "match_subject_alt_names" -i --type javaRepository: line/armeria
Length of output: 520
🏁 Script executed:
# Check the proto definition for CertificateValidationContext fields
cat xds-api/src/main/proto/envoy/extensions/transport_sockets/tls/v3/common.proto | grep -A 20 -B 5 "match_subject_alt_names"Repository: line/armeria
Length of output: 1819
🏁 Script executed:
# Look at the test file to see how both fields are used
cat -n it/xds-client/src/test/java/com/linecorp/armeria/xds/it/CertificateValidationContextTest.java | head -200Repository: line/armeria
Length of output: 9947
🏁 Script executed:
# Check if there are any comments or documentation about Envoy behavior in the tests
rg "match_subject_alt_names|match_typed_subject_alt_names" -B 5 -A 5 --type java it/xds-client/src/test/Repository: line/armeria
Length of output: 33352
🏁 Script executed:
# Look at the test that uses match_subject_alt_names to understand the scenario
cat -n it/xds-client/src/test/java/com/linecorp/armeria/xds/it/CertificateValidationContextTest.java | sed -n '200,350p'Repository: line/armeria
Length of output: 8669
🏁 Script executed:
# Find the test that uses match_subject_alt_names specifically
grep -n "match_subject_alt_names" it/xds-client/src/test/java/com/linecorp/armeria/xds/it/CertificateValidationContextTest.javaRepository: line/armeria
Length of output: 159
🏁 Script executed:
# Get more context around that test
cat -n it/xds-client/src/test/java/com/linecorp/armeria/xds/it/CertificateValidationContextTest.java | sed -n '350,450p'Repository: line/armeria
Length of output: 5125
🏁 Script executed:
# Let's see the rest of the mergeValidationContextWithBase test
cat -n it/xds-client/src/test/java/com/linecorp/armeria/xds/it/CertificateValidationContextTest.java | sed -n '450,550p'Repository: line/armeria
Length of output: 4794
🏁 Script executed:
# Check if there's any logic in the code that handles combined_validation_context or field merging
rg "combined_validation_context|mergeValidationContext" -i --type javaRepository: line/armeria
Length of output: 525
🏁 Script executed:
# Check if there are any test cases that have BOTH match_typed_subject_alt_names and match_subject_alt_names in the same validation context
rg -B 10 "match_typed_subject_alt_names" it/xds-client/src/test/ | grep -A 10 "match_subject_alt_names"Repository: line/armeria
Length of output: 38
Skip legacy match_subject_alt_names when match_typed_subject_alt_names is present.
According to the Envoy protobuf, the legacy field is deprecated with the note: "if both this field and match_typed_subject_alt_names are specified, the former (deprecated field) is ignored." The current code unconditionally processes both fields into the same parsedSanMatchers list (lines 73–76), which means it will validate certificates differently than Envoy when both are configured in the same CertificateValidationContext. Add a check to skip the legacy field if the typed field is non-empty.
🤖 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 71 - 76, The loop that unconditionally processes
resource.getMatchSubjectAltNamesList() into parsedSanMatchers should be skipped
when the typed SANs are present; in CertificateValidationContextSnapshot, check
the typed field (e.g., resource.getMatchTypedSubjectAltNamesCount() or
getMatchTypedSubjectAltNamesList().isEmpty()) before iterating legacy
match_subject_alt_names and only add the legacy SanMatcher entries when the
typed list is empty/absent so the deprecated field is ignored when
match_typed_subject_alt_names is provided.
Comment on lines
+62
to
+74
| boolean completed; | ||
| private boolean draining; | ||
|
|
||
| DeltaActualStream(DeltaDiscoveryStub stub, AdsXdsStream owner, StateCoordinator stateCoordinator, | ||
| EventExecutor eventLoop, ConfigSourceLifecycleObserver lifecycleObserver, Node node) { | ||
| this.owner = owner; | ||
| this.stateCoordinator = stateCoordinator; | ||
| this.eventLoop = eventLoop; | ||
| this.lifecycleObserver = lifecycleObserver; | ||
| this.node = node; | ||
| requestObserver = stub.stream(this); | ||
| lifecycleObserver.streamOpened(); | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Make completed field private and add null validation to constructor.
The completed field lacks an access modifier (defaulting to package-private) but should be private like other fields. Additionally, the constructor should validate parameters with requireNonNull for consistency with AdsXdsStream.
Proposed fix
- boolean completed;
+ private boolean completed;
private boolean draining;
DeltaActualStream(DeltaDiscoveryStub stub, AdsXdsStream owner, StateCoordinator stateCoordinator,
EventExecutor eventLoop, ConfigSourceLifecycleObserver lifecycleObserver, Node node) {
- this.owner = owner;
- this.stateCoordinator = stateCoordinator;
- this.eventLoop = eventLoop;
- this.lifecycleObserver = lifecycleObserver;
- this.node = node;
- requestObserver = stub.stream(this);
+ this.owner = java.util.Objects.requireNonNull(owner, "owner");
+ this.stateCoordinator = java.util.Objects.requireNonNull(stateCoordinator, "stateCoordinator");
+ this.eventLoop = java.util.Objects.requireNonNull(eventLoop, "eventLoop");
+ this.lifecycleObserver = java.util.Objects.requireNonNull(lifecycleObserver, "lifecycleObserver");
+ this.node = java.util.Objects.requireNonNull(node, "node");
+ requestObserver = java.util.Objects.requireNonNull(stub, "stub").stream(this);
lifecycleObserver.streamOpened();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| boolean completed; | |
| private boolean draining; | |
| DeltaActualStream(DeltaDiscoveryStub stub, AdsXdsStream owner, StateCoordinator stateCoordinator, | |
| EventExecutor eventLoop, ConfigSourceLifecycleObserver lifecycleObserver, Node node) { | |
| this.owner = owner; | |
| this.stateCoordinator = stateCoordinator; | |
| this.eventLoop = eventLoop; | |
| this.lifecycleObserver = lifecycleObserver; | |
| this.node = node; | |
| requestObserver = stub.stream(this); | |
| lifecycleObserver.streamOpened(); | |
| } | |
| private boolean completed; | |
| private boolean draining; | |
| DeltaActualStream(DeltaDiscoveryStub stub, AdsXdsStream owner, StateCoordinator stateCoordinator, | |
| EventExecutor eventLoop, ConfigSourceLifecycleObserver lifecycleObserver, Node node) { | |
| this.owner = java.util.Objects.requireNonNull(owner, "owner"); | |
| this.stateCoordinator = java.util.Objects.requireNonNull(stateCoordinator, "stateCoordinator"); | |
| this.eventLoop = java.util.Objects.requireNonNull(eventLoop, "eventLoop"); | |
| this.lifecycleObserver = java.util.Objects.requireNonNull(lifecycleObserver, "lifecycleObserver"); | |
| this.node = java.util.Objects.requireNonNull(node, "node"); | |
| requestObserver = java.util.Objects.requireNonNull(stub, "stub").stream(this); | |
| lifecycleObserver.streamOpened(); | |
| } |
🤖 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/DeltaActualStream.java` around
lines 62 - 74, Change the package-private boolean field completed in class
DeltaActualStream to private to match other fields, and add null checks using
Objects.requireNonNull for all reference parameters in the DeltaActualStream
constructor (DeltaActualStream(DeltaDiscoveryStub stub, AdsXdsStream owner,
StateCoordinator stateCoordinator, EventExecutor eventLoop,
ConfigSourceLifecycleObserver lifecycleObserver, Node node)) similar to
AdsXdsStream so the constructor validates stub, owner, stateCoordinator,
eventLoop, lifecycleObserver, and node before assigning and calling
stub.stream(this) and lifecycleObserver.streamOpened().
Comment on lines
+41
to
+45
| StateCoordinator(EventExecutor eventLoop, long timeoutMillis, boolean delta) { | ||
| this.delta = delta; | ||
| subscriberStorage = new SubscriberStorage(eventLoop, timeoutMillis, delta); | ||
| stateStore = new ResourceStateStore(); | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add null validation for eventLoop parameter.
The constructor should validate the eventLoop parameter with requireNonNull for consistency with other classes in this PR.
Proposed fix
+ import static java.util.Objects.requireNonNull;
+
StateCoordinator(EventExecutor eventLoop, long timeoutMillis, boolean delta) {
this.delta = delta;
- subscriberStorage = new SubscriberStorage(eventLoop, timeoutMillis, delta);
+ subscriberStorage = new SubscriberStorage(requireNonNull(eventLoop, "eventLoop"), timeoutMillis, delta);
stateStore = new ResourceStateStore();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| StateCoordinator(EventExecutor eventLoop, long timeoutMillis, boolean delta) { | |
| this.delta = delta; | |
| subscriberStorage = new SubscriberStorage(eventLoop, timeoutMillis, delta); | |
| stateStore = new ResourceStateStore(); | |
| } | |
| StateCoordinator(EventExecutor eventLoop, long timeoutMillis, boolean delta) { | |
| this.delta = delta; | |
| subscriberStorage = new SubscriberStorage(requireNonNull(eventLoop, "eventLoop"), timeoutMillis, delta); | |
| stateStore = new ResourceStateStore(); | |
| } |
🤖 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/StateCoordinator.java` around
lines 41 - 45, The StateCoordinator constructor currently doesn't validate its
eventLoop parameter; add Objects.requireNonNull(eventLoop, "eventLoop") at the
top of the constructor (before creating SubscriberStorage) to match other
classes' null checks; reference the EventExecutor parameter and the
StateCoordinator(EventExecutor eventLoop, long timeoutMillis, boolean delta)
constructor and ensure the check occurs prior to instantiating new
SubscriberStorage(eventLoop, timeoutMillis, delta).
xds/src/main/java/com/linecorp/armeria/xds/TransportSocketSnapshot.java
Outdated
Show resolved
Hide resolved
Comment on lines
+211
to
+261
| void nackResponse() throws Exception { | ||
| final DeltaDiscoveryStub stub = DeltaDiscoveryStub.ads(GrpcClients.builder(server.httpUri())); | ||
| final DummyResourceWatcher watcher = new DummyResourceWatcher(); | ||
| final StateCoordinator stateCoordinator = new StateCoordinator(eventLoop.get(), 15_000, true); | ||
| final AtomicBoolean ackRef = new AtomicBoolean(); | ||
| final AtomicInteger nackCount = new AtomicInteger(); | ||
| final RecordingLifecycleObserver lifecycleObserver = new RecordingLifecycleObserver() { | ||
| @Override | ||
| public void responseReceived(DeltaDiscoveryResponse value) { | ||
| if (ackRef.get()) { | ||
| super.responseReceived(value); | ||
| } else { | ||
| nackCount.incrementAndGet(); | ||
| } | ||
| } | ||
| }; | ||
| final Backoff backoff = Backoff.ofDefault(); | ||
|
|
||
| try (AdsXdsStream stream = | ||
| AdsXdsStream.of( | ||
| owner -> new DeltaActualStream(stub, owner, stateCoordinator, eventLoop.get(), | ||
| lifecycleObserver, SERVER_INFO), | ||
| backoff, eventLoop.get(), stateCoordinator, lifecycleObserver, | ||
| XdsType.discoverableTypes())) { | ||
|
|
||
| await().pollDelay(100, TimeUnit.MILLISECONDS) | ||
| .untilAsserted(() -> assertThat(lifecycleObserver.responses()).isEmpty()); | ||
|
|
||
| stateCoordinator.register(XdsType.CLUSTER, clusterName, watcher); | ||
| stream.start(); | ||
|
|
||
| // Wait for the first NACK. | ||
| await().untilAtomic(nackCount, Matchers.greaterThanOrEqualTo(1)); | ||
| assertThat(lifecycleObserver.responses()).isEmpty(); | ||
|
|
||
| // Push a new snapshot to trigger the server to send an updated response, | ||
| // and start ACKing from here. | ||
| ackRef.set(true); | ||
| cache.setSnapshot( | ||
| GROUP, | ||
| Snapshot.create( | ||
| ImmutableList.of(createCluster(clusterName, 2)), | ||
| ImmutableList.of(), ImmutableList.of(), ImmutableList.of(), | ||
| ImmutableList.of(), "2")); | ||
|
|
||
| // Once the handler acks, the response is recorded. | ||
| await().until(() -> !lifecycleObserver.responses().isEmpty()); | ||
| assertThat(lifecycleObserver.responses()).allSatisfy(res -> { | ||
| assertThat(res.getResourcesList()).isNotEmpty(); | ||
| assertThat(res.getResources(0).getName()).isEqualTo(clusterName); | ||
| }); |
Contributor
There was a problem hiding this comment.
This test never proves a NACK was sent.
The fixture stays on a valid snapshot from beforeEach(), and the override only counts inbound DeltaDiscoveryResponses until ackRef flips. That shows a response arrived, not that the client emitted a NACK. To cover the NACK path, seed an invalid resource and assert the outbound DeltaDiscoveryRequest carries error_detail.
Contributor
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/src/main/java/com/linecorp/armeria/server/ServerTlsSpec.java (1)
53-63:⚠️ Potential issue | 🟡 MinorAdd documentation explaining why
ServerTlsSpecpassesalpnProtocolstwice to the parent constructor.The constructor at line 59 passes
alpnProtocolsfor both thealpnProtocolsandfallbackAlpnProtocolsparameters, unlikeClientTlsSpecwhich allows separate configuration. This design appears intentional—servers don't expose fallback ALPN as configurable since they advertise capabilities rather than negotiate fallback protocols. Either add a brief comment explaining this design choice or reconsider if servers should support separate fallback ALPN configuration.🤖 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/server/ServerTlsSpec.java` around lines 53 - 63, Explain in a brief comment inside the ServerTlsSpec constructor why alpnProtocols is passed for both the alpnProtocols and fallbackAlpnProtocols parameters when calling super: note that servers advertise supported ALPNs rather than negotiate a separate fallback set (unlike ClientTlsSpec which accepts separate fallback configuration), so the server intentionally reuses the same set; add the comment near the ServerTlsSpec(...) constructor call to clarify the design choice and reference the parent constructor parameters (alpnProtocols, fallbackAlpnProtocols) and the contrasting ClientTlsSpec behavior.
🧹 Nitpick comments (3)
core/src/main/java/com/linecorp/armeria/common/AbstractTlsSpec.java (1)
104-106: Missing Javadoc for public methodeffectiveAlpnProtocols().Per coding guidelines, all public methods require Javadoc documentation.
Suggested Javadoc
+ /** + * Returns the effective ALPN protocols to use for the TLS handshake. + * Returns {`@link` `#alpnProtocols`()} if non-empty, otherwise {`@link` `#fallbackAlpnProtocols`()}. + */ public Set<String> effectiveAlpnProtocols() { return alpnProtocols.isEmpty() ? fallbackAlpnProtocols : alpnProtocols; }🤖 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 104 - 106, Add Javadoc for the public method effectiveAlpnProtocols() in AbstractTlsSpec: document the method purpose (returns the ALPN protocols to use, preferring alpnProtocols over fallbackAlpnProtocols when non-empty), describe return value (Set<String> of protocols), and note any immutability/ownership semantics or null-safety (that it never returns null and which internal fields it references: alpnProtocols and fallbackAlpnProtocols). Keep the style consistent with the project's existing Javadoc conventions and place the comment immediately above the method signature.xds/src/main/java/com/linecorp/armeria/xds/TransportSocketSnapshot.java (1)
130-132: Consider debug logging the ignored exception for observability.Silently ignoring the
InvalidProtocolBufferExceptionis acceptable when the typed config isn't anUpstreamTlsContext, but debug-level logging would help troubleshoot unexpected configurations.Optional improvement
} catch (com.google.protobuf.InvalidProtocolBufferException ignored) { - // not an UpstreamTlsContext — leave defaults + // not an UpstreamTlsContext — leave defaults + logger.debug("Transport socket typed config is not UpstreamTlsContext, using defaults", ignored); }🤖 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 130 - 132, In TransportSocketSnapshot, the catch block for com.google.protobuf.InvalidProtocolBufferException currently ignores the exception; update it to emit a debug-level log including the exception and a short contextual message (e.g., using the class logger) so unexpected typed-config parsing failures are observable—keep behavior unchanged for non-UpstreamTlsContext cases but include the exception in the debug log for troubleshooting.it/xds-istio/src/test/java/com/linecorp/armeria/it/xds/XdsClientToSidecarTest.java (1)
67-68: Centralize the service FQDN and listener naming.
echo-server.default.svc.cluster.local,8080, and the{clusterIP}_{port}format are repeated across the class. A small helper derived fromserver.serviceName()andserver.port()will keep the fixture aligned if the test server config changes.Also applies to: 97-99, 144-146, 182-184
🤖 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 67 - 68, Replace repeated hard-coded FQDN, port and listener/cluster naming by introducing a small helper that derives the service FQDN and the listener/cluster identifiers from the test server instance (use server.serviceName() and server.port()). Update occurrences where clusterName is defined (variable clusterName), the listener name and any "{clusterIP}_{port}" usages to call the helper or use its returned values so the values in XdsClientToSidecarTest (including the places around lines 97-99, 144-146, 182-184) stay in sync with server configuration; ensure helper produces exactly the strings currently expected (e.g., "echo-server.default.svc.cluster.local", "8080", and the combined "{clusterIP}_{port}" format) but computed from server.serviceName() and server.port().
🤖 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/XdsClientToSidecarTest.java`:
- Around line 73-80: The watcher callbacks currently use
snapshotRef.compareAndSet(null, ...) and errorRef.compareAndSet(null, ...) which
pins the first emission; change them to always update to the latest state by
using snapshotRef.set(snapshot) and errorRef.set(t) (or
snapshotRef.setIfPresent/overwrite as appropriate) in the three watcher blocks
(clusterRoot.addSnapshotWatcher, routeRoot.addSnapshotWatcher,
listenerRoot.addSnapshotWatcher) so the refs reflect the most recent
snapshot/error rather than the first callback.
- Line 2: The new source file XdsClientToSidecarTest.java contains a copyright
header showing year "2025" which is stale for a file added in 2026; update the
header year to "2026" in the file header comment (the top-of-file copyright
line) so the header matches the file creation year and project guidelines.
---
Outside diff comments:
In `@core/src/main/java/com/linecorp/armeria/server/ServerTlsSpec.java`:
- Around line 53-63: Explain in a brief comment inside the ServerTlsSpec
constructor why alpnProtocols is passed for both the alpnProtocols and
fallbackAlpnProtocols parameters when calling super: note that servers advertise
supported ALPNs rather than negotiate a separate fallback set (unlike
ClientTlsSpec which accepts separate fallback configuration), so the server
intentionally reuses the same set; add the comment near the ServerTlsSpec(...)
constructor call to clarify the design choice and reference the parent
constructor parameters (alpnProtocols, fallbackAlpnProtocols) and the
contrasting ClientTlsSpec behavior.
---
Nitpick comments:
In `@core/src/main/java/com/linecorp/armeria/common/AbstractTlsSpec.java`:
- Around line 104-106: Add Javadoc for the public method
effectiveAlpnProtocols() in AbstractTlsSpec: document the method purpose
(returns the ALPN protocols to use, preferring alpnProtocols over
fallbackAlpnProtocols when non-empty), describe return value (Set<String> of
protocols), and note any immutability/ownership semantics or null-safety (that
it never returns null and which internal fields it references: alpnProtocols and
fallbackAlpnProtocols). Keep the style consistent with the project's existing
Javadoc conventions and place the comment immediately above the method
signature.
In
`@it/xds-istio/src/test/java/com/linecorp/armeria/it/xds/XdsClientToSidecarTest.java`:
- Around line 67-68: Replace repeated hard-coded FQDN, port and listener/cluster
naming by introducing a small helper that derives the service FQDN and the
listener/cluster identifiers from the test server instance (use
server.serviceName() and server.port()). Update occurrences where clusterName is
defined (variable clusterName), the listener name and any "{clusterIP}_{port}"
usages to call the helper or use its returned values so the values in
XdsClientToSidecarTest (including the places around lines 97-99, 144-146,
182-184) stay in sync with server configuration; ensure helper produces exactly
the strings currently expected (e.g., "echo-server.default.svc.cluster.local",
"8080", and the combined "{clusterIP}_{port}" format) but computed from
server.serviceName() and server.port().
In `@xds/src/main/java/com/linecorp/armeria/xds/TransportSocketSnapshot.java`:
- Around line 130-132: In TransportSocketSnapshot, the catch block for
com.google.protobuf.InvalidProtocolBufferException currently ignores the
exception; update it to emit a debug-level log including the exception and a
short contextual message (e.g., using the class logger) so unexpected
typed-config parsing failures are observable—keep behavior unchanged for
non-UpstreamTlsContext cases but include the exception in the debug log for
troubleshooting.
🪄 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: 52f08350-55b3-4afd-bd42-baf27be2902d
📒 Files selected for processing (18)
core/src/main/java/com/linecorp/armeria/client/BootstrapSslContexts.javacore/src/main/java/com/linecorp/armeria/client/ClientTlsSpec.javacore/src/main/java/com/linecorp/armeria/client/ClientTlsSpecBuilder.javacore/src/main/java/com/linecorp/armeria/client/HttpClientDelegate.javacore/src/main/java/com/linecorp/armeria/common/AbstractTlsSpec.javacore/src/main/java/com/linecorp/armeria/internal/common/util/SslContextUtil.javacore/src/main/java/com/linecorp/armeria/server/ServerTlsSpec.javacore/src/test/java/com/linecorp/armeria/common/tls/TlsSpecPerRequestTest.javait/xds-istio/src/test/java/com/linecorp/armeria/it/xds/XdsClientToSidecarTest.javait/xds-istio/src/test/java/com/linecorp/armeria/it/xds/filter/IstioNoOpFilterFactories.javaxds/src/main/java/com/linecorp/armeria/xds/FilterUtil.javaxds/src/main/java/com/linecorp/armeria/xds/ParsedFilterConfig.javaxds/src/main/java/com/linecorp/armeria/xds/RouteEntry.javaxds/src/main/java/com/linecorp/armeria/xds/TransportSocketSnapshot.javaxds/src/main/java/com/linecorp/armeria/xds/client/endpoint/RouterFilterFactory.javaxds/src/main/java/com/linecorp/armeria/xds/filter/HttpFilterFactory.javaxds/src/main/java/com/linecorp/armeria/xds/filter/HttpFilterFactoryRegistry.javaxds/src/main/java/com/linecorp/armeria/xds/filter/XdsHttpFilter.java
💤 Files with no reviewable changes (1)
- xds/src/main/java/com/linecorp/armeria/xds/ParsedFilterConfig.java
✅ Files skipped from review due to trivial changes (1)
- core/src/test/java/com/linecorp/armeria/common/tls/TlsSpecPerRequestTest.java
🚧 Files skipped from review as they are similar to previous changes (4)
- core/src/main/java/com/linecorp/armeria/client/HttpClientDelegate.java
- core/src/main/java/com/linecorp/armeria/client/ClientTlsSpec.java
- core/src/main/java/com/linecorp/armeria/internal/common/util/SslContextUtil.java
- it/xds-istio/src/test/java/com/linecorp/armeria/it/xds/filter/IstioNoOpFilterFactories.java
| @@ -0,0 +1,236 @@ | |||
| /* | |||
| * Copyright 2025 LY Corporation | |||
Contributor
There was a problem hiding this comment.
Update the new file’s header year.
This file is being added in 2026, so the copied 2025 header looks stale for a new source file.
📝 Proposed fix
- * Copyright 2025 LY Corporation
+ * Copyright 2026 LY Corporation📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| * Copyright 2025 LY Corporation | |
| * Copyright 2026 LY Corporation |
🤖 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`
at line 2, The new source file XdsClientToSidecarTest.java contains a copyright
header showing year "2025" which is stale for a file added in 2026; update the
header year to "2026" in the file header comment (the top-of-file copyright
line) so the header matches the file creation year and project guidelines.
Comment on lines
+73
to
+80
| clusterRoot.addSnapshotWatcher((snapshot, t) -> { | ||
| logger.info("Cluster snapshot: {}, t: ", snapshot, t); | ||
| if (snapshot != null) { | ||
| snapshotRef.compareAndSet(null, snapshot); | ||
| } | ||
| if (t != null) { | ||
| errorRef.compareAndSet(null, t); | ||
| } |
Contributor
There was a problem hiding this comment.
Keep the latest watcher state instead of the first callback.
These callbacks are streams, not one-shot futures. compareAndSet(null, snapshot) pins the first emission, so these tests can get stuck on an intermediate update even when a later callback reaches the ready state. This is especially brittle in routeLoad() and listenerLoad(), where the route snapshot can land after the listener snapshot. If these watchers can recover from transient errors, the same caveat applies to errorRef.
🔄 Representative fix
Apply the same replacement to the three watcher blocks.
- if (snapshot != null) {
- snapshotRef.compareAndSet(null, snapshot);
- }
+ if (snapshot != null) {
+ snapshotRef.set(snapshot);
+ }Also applies to: 116-123, 156-163
🤖 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 73 - 80, The watcher callbacks currently use
snapshotRef.compareAndSet(null, ...) and errorRef.compareAndSet(null, ...) which
pins the first emission; change them to always update to the latest state by
using snapshotRef.set(snapshot) and errorRef.set(t) (or
snapshotRef.setIfPresent/overwrite as appropriate) in the three watcher blocks
(clusterRoot.addSnapshotWatcher, routeRoot.addSnapshotWatcher,
listenerRoot.addSnapshotWatcher) so the refs reflect the most recent
snapshot/error rather than the first callback.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioTestExtension.java (1)
145-156:⚠️ Potential issue | 🟠 MajorStill missing a fast-fail path for terminal startup failures.
This poll still waits the full timeout for pods that have already entered terminal states like
Failed/CrashLoopBackOff, and this path never captures the startup logs before cleanup. That makes infra failures much slower to diagnose.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioTestExtension.java` around lines 145 - 156, The poll must fast-fail when the pod has entered terminal/irrecoverable states instead of waiting the full timeout: inside the K8sClusterHelper.poll lambda in IstioTestExtension (the block that reads pod = client.pods().inNamespace(namespace).withName(podName).get()), check pod.getStatus().getPhase() and the pod.getStatus().getContainerStatuses() for terminal indicators (e.g., "Failed", "Succeeded", CrashLoopBackOff in container state/reason) before calling podCustomizer.isPodHealthy(pod); if a terminal state is detected, immediately collect the pod startup logs (so they are not lost during cleanup) and throw an IllegalStateException (or return a fast-fail signal) with a clear message including podName and the gathered log/error details so the poll does not wait the full timeout.
🧹 Nitpick comments (2)
it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioServerExtension.java (2)
67-72: Add validation for theportparameter.The constructor validates
serviceNameandconfiguratorClassbut notport. Invalid port values (e.g., 0, negative, or > 65535) would cause failures during deployment.Proposed fix
+import com.google.common.base.Preconditions; + public IstioServerExtension(String serviceName, int port, Class<? extends ServerConfigurator> configuratorClass) { this.serviceName = requireNonNull(serviceName, "serviceName"); - this.port = port; + Preconditions.checkArgument(port > 0 && port <= 65535, "port: %s (expected: 1-65535)", port); + this.port = port; this.configuratorClass = requireNonNull(configuratorClass, "configuratorClass"); }As per coding guidelines, "validate public method parameters with requireNonNull / argument checks and meaningful exception messages".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioServerExtension.java` around lines 67 - 72, The IstioServerExtension constructor validates serviceName and configuratorClass but not port; add an argument check in the constructor to ensure the port is within the valid TCP range (1–65535) and throw an IllegalArgumentException with a clear message for invalid values. Modify the IstioServerExtension(String serviceName, int port, Class<? extends ServerConfigurator> configuratorClass) constructor to call requireNonNull for serviceName and configuratorClass as already done and then validate port (e.g., if (port <= 0 || port > 65535) throw new IllegalArgumentException("port: must be in range 1-65535, was " + port)); keep existing field assignments (this.port = port) only after the check so invalid ports are rejected early.
202-220: Consider checking container readiness, not just pod phase.The current check only verifies
pod.getStatus().getPhase() == "Running", but this doesn't guarantee all containers are ready. With Istio sidecar injection, pods have 2 containers (app + istio-proxy), and tests could be flaky if they proceed before both are ready.Proposed fix for more robust readiness check
private void waitForReady(KubernetesClient client) { logger.info("Waiting for deployment '{}' to be ready...", serviceName); final boolean ready = K8sClusterHelper.poll( READY_TIMEOUT, K8sClusterHelper.DEFAULT_POLL_INTERVAL, () -> { final List<Pod> pods = client.pods().inNamespace(NAMESPACE) .withLabel("app", serviceName) .list() .getItems(); return pods.stream().anyMatch(pod -> { - final String phase = pod.getStatus() != null ? pod.getStatus().getPhase() : null; - return "Running".equals(phase); + if (pod.getStatus() == null || !"Running".equals(pod.getStatus().getPhase())) { + return false; + } + final var containerStatuses = pod.getStatus().getContainerStatuses(); + if (containerStatuses == null || containerStatuses.isEmpty()) { + return false; + } + return containerStatuses.stream().allMatch(cs -> Boolean.TRUE.equals(cs.getReady())); }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioServerExtension.java` around lines 202 - 220, The current waitForReady method only checks pod phase "Running" which can be true before all containers (app + istio-proxy) are actually ready; update the K8sClusterHelper.poll predicate inside waitForReady to inspect each Pod's PodStatus.containerStatuses (and optionally initContainerStatuses) and return true only when all containerStatuses exist and each ContainerStatus.getReady() is true for the pod (instead of just checking pod.getStatus().getPhase()); keep using serviceName and NAMESPACE to select pods and preserve the READY_TIMEOUT and K8sClusterHelper.DEFAULT_POLL_INTERVAL behavior.
🤖 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/client/ClientTlsSpecBuilder.java`:
- Around line 69-77: The Javadoc on ClientTlsSpecBuilder.alpnProtocols is
outdated — update the method comment to state that the default is an empty ALPN
set (no automatic h2/http/1.1 advertisement) and explain that ALPN values may
still be derived from the SessionProtocol elsewhere (e.g., ClientTlsSpec.of() /
builder().build() applies session-derived ALPN when appropriate); reference
ClientTlsSpecBuilder, the alpnProtocols(Collection<String>) method, and
ClientTlsSpec.of()/builder().build() so readers know where session-derived ALPN
is applied.
In
`@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioTestExtension.java`:
- Line 82: createTestPod currently returns only podName while
PodCustomizer.customizePod(builder) may change metadata.namespace, causing
subsequent operations to use the original namespace; update createTestPod (and
its callers) to return both the resolved podName and the resolved namespace
(e.g., a PodInfo/Pair or a small record) so callers (the code that calls
createTestPod and then runs health checks, collectLogs, and cleanup) use the
returned namespace instead of the original namespace variable; ensure all usages
around where podName is assigned (and in the block covering lines ~107-139) are
updated to read both podName and namespace from the new return value and
propagate the resolved namespace into health check, log collection, and deletion
calls.
In `@xds/src/main/java/com/linecorp/armeria/xds/TransportSocketSnapshot.java`:
- Around line 110-111: In TransportSocketSnapshot, do not unconditionally call
ClientTlsSpec.builder().endpointIdentificationAlgorithm(""); instead check
whether CertificateValidationContextSnapshot.peerVerifierFactories() will
install a SAN/pin verifier and only clear the JSSE endpoint identification when
such a custom peer verifier is present; otherwise leave the default "HTTPS"
(i.e., do not call endpointIdentificationAlgorithm("")) so CA-only configs still
perform hostname/SAN peer identity checks; alternatively add a clear comment in
TransportSocketSnapshot explaining that CA-only verification intentionally omits
peer-identity checks if you choose to keep the current behavior.
---
Duplicate comments:
In
`@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioTestExtension.java`:
- Around line 145-156: The poll must fast-fail when the pod has entered
terminal/irrecoverable states instead of waiting the full timeout: inside the
K8sClusterHelper.poll lambda in IstioTestExtension (the block that reads pod =
client.pods().inNamespace(namespace).withName(podName).get()), check
pod.getStatus().getPhase() and the pod.getStatus().getContainerStatuses() for
terminal indicators (e.g., "Failed", "Succeeded", CrashLoopBackOff in container
state/reason) before calling podCustomizer.isPodHealthy(pod); if a terminal
state is detected, immediately collect the pod startup logs (so they are not
lost during cleanup) and throw an IllegalStateException (or return a fast-fail
signal) with a clear message including podName and the gathered log/error
details so the poll does not wait the full timeout.
---
Nitpick comments:
In
`@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioServerExtension.java`:
- Around line 67-72: The IstioServerExtension constructor validates serviceName
and configuratorClass but not port; add an argument check in the constructor to
ensure the port is within the valid TCP range (1–65535) and throw an
IllegalArgumentException with a clear message for invalid values. Modify the
IstioServerExtension(String serviceName, int port, Class<? extends
ServerConfigurator> configuratorClass) constructor to call requireNonNull for
serviceName and configuratorClass as already done and then validate port (e.g.,
if (port <= 0 || port > 65535) throw new IllegalArgumentException("port: must be
in range 1-65535, was " + port)); keep existing field assignments (this.port =
port) only after the check so invalid ports are rejected early.
- Around line 202-220: The current waitForReady method only checks pod phase
"Running" which can be true before all containers (app + istio-proxy) are
actually ready; update the K8sClusterHelper.poll predicate inside waitForReady
to inspect each Pod's PodStatus.containerStatuses (and optionally
initContainerStatuses) and return true only when all containerStatuses exist and
each ContainerStatus.getReady() is true for the pod (instead of just checking
pod.getStatus().getPhase()); keep using serviceName and NAMESPACE to select pods
and preserve the READY_TIMEOUT and K8sClusterHelper.DEFAULT_POLL_INTERVAL
behavior.
🪄 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: 58589585-88ef-49c1-8577-0b54d6213c61
📒 Files selected for processing (14)
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/client/HttpClientDelegate.javacore/src/main/java/com/linecorp/armeria/internal/common/util/SslContextUtil.javacore/src/main/java/com/linecorp/armeria/server/ServerTlsSpec.javacore/src/test/java/com/linecorp/armeria/common/tls/TlsSpecPerRequestTest.javait/xds-istio/build.gradleit/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioEnv.javait/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioServerExtension.javait/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioTestExtension.javaxds/src/main/java/com/linecorp/armeria/xds/CertificateValidationContextSnapshot.javaxds/src/main/java/com/linecorp/armeria/xds/TlsCertificateSnapshot.javaxds/src/main/java/com/linecorp/armeria/xds/TransportSocketSnapshot.javaxds/src/main/java/com/linecorp/armeria/xds/TransportSocketStream.java
💤 Files with no reviewable changes (1)
- xds/src/main/java/com/linecorp/armeria/xds/TlsCertificateSnapshot.java
✅ Files skipped from review due to trivial changes (3)
- core/src/test/java/com/linecorp/armeria/common/tls/TlsSpecPerRequestTest.java
- core/src/main/java/com/linecorp/armeria/server/ServerTlsSpec.java
- it/xds-istio/build.gradle
🚧 Files skipped from review as they are similar to previous changes (5)
- core/src/main/java/com/linecorp/armeria/client/HttpClientDelegate.java
- core/src/main/java/com/linecorp/armeria/internal/common/util/SslContextUtil.java
- xds/src/main/java/com/linecorp/armeria/xds/TransportSocketStream.java
- xds/src/main/java/com/linecorp/armeria/xds/CertificateValidationContextSnapshot.java
- it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioEnv.java
core/src/main/java/com/linecorp/armeria/client/ClientTlsSpecBuilder.java
Show resolved
Hide resolved
| final String testClass = invocationContext.getTargetClass().getName(); | ||
| final String testMethod = invocationContext.getExecutable().getName(); | ||
| final String namespace = "default"; | ||
| final String podName = createTestPod(client, namespace, testClass, testMethod, podCustomizer); |
Contributor
There was a problem hiding this comment.
Propagate the resolved namespace after customizePod().
PodCustomizer.customizePod(builder) can legally move the pod out of "default", but createTestPod() only returns podName. Every later lookup still uses the original namespace, so a customizer that overrides metadata.namespace will make the health check, log collection, and cleanup all target the wrong pod.
💡 One way to keep the namespace in sync
- final String podName = createTestPod(client, namespace, testClass, testMethod, podCustomizer);
+ final TestPodRef podRef =
+ createTestPod(client, namespace, testClass, testMethod, podCustomizer);
- logger.info("Created K8s Pod '{}' for {}.{}", podName, testClass, testMethod);
+ logger.info("Created K8s Pod '{}' for {}.{}", podRef.podName(), testClass, testMethod);
try {
- waitForPodHealthy(client, podName, namespace, podCustomizer);
- final int exitCode = waitForTestContainerTerminated(client, podName, namespace);
- final String logs = collectPodLogs(client, podName, namespace);
+ waitForPodHealthy(client, podRef.podName(), podRef.namespace(), podCustomizer);
+ final int exitCode = waitForTestContainerTerminated(
+ client, podRef.podName(), podRef.namespace());
+ final String logs = collectPodLogs(client, podRef.podName(), podRef.namespace());
if (exitCode == 0) {
...
}
} finally {
- client.pods().inNamespace(namespace).withName(podName).delete();
+ client.pods().inNamespace(podRef.namespace()).withName(podRef.podName()).delete();
}
- private static String createTestPod(KubernetesClient client, String namespace,
+ private static TestPodRef createTestPod(KubernetesClient client, String namespace,
String testClass, String testMethod,
PodCustomizer podCustomizer) {
...
- client.pods().inNamespace(namespace).resource(builder.build()).create();
- return podName;
+ final Pod pod = builder.build();
+ final String resolvedNamespace = pod.getMetadata().getNamespace();
+ client.pods().inNamespace(resolvedNamespace).resource(pod).create();
+ return new TestPodRef(resolvedNamespace, podName);
}Also applies to: 107-139
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioTestExtension.java`
at line 82, createTestPod currently returns only podName while
PodCustomizer.customizePod(builder) may change metadata.namespace, causing
subsequent operations to use the original namespace; update createTestPod (and
its callers) to return both the resolved podName and the resolved namespace
(e.g., a PodInfo/Pair or a small record) so callers (the code that calls
createTestPod and then runs health checks, collectLogs, and cleanup) use the
returned namespace instead of the original namespace variable; ensure all usages
around where podName is assigned (and in the block covering lines ~107-139) are
updated to read both podName and namespace from the new return value and
propagate the resolved namespace into health check, log collection, and deletion
calls.
Comment on lines
+110
to
+111
| final ClientTlsSpecBuilder specBuilder = ClientTlsSpec.builder() | ||
| .endpointIdentificationAlgorithm(""); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Envoy/Istio UpstreamTlsContext, when CommonTlsContext.certificate_validation_context configures trusted_ca but does not set match_typed_subject_alt_names, auto_sni_san_validation, or pinned certificate checks, is upstream peer identity verified by default, or is only the certificate chain validated?
💡 Result:
In Envoy/Istio UpstreamTlsContext, when CommonTlsContext.certificate_validation_context configures trusted_ca but does not set match_typed_subject_alt_names, auto_sni_san_validation, or pinned certificate checks, only the certificate chain is validated against the trusted CA. The upstream peer identity (hostname/SAN verification) is not verified by default.
Citations:
- 1: https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/security/ssl.html?highlight=default+certificate
- 2: https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/security/ssl
- 3: https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/security/ssl.html?highlight=tls+certificates
- 4: Envoy accepts any upstream certificate as long as the CA is trusted envoyproxy/envoy#6763
Unconditional hostname-verification disablement leaves CA-only configs without a peer-identity check.
Line 111 clears JSSE endpoint identification before it is known whether CertificateValidationContextSnapshot.peerVerifierFactories() will install a SAN/pin verifier. On a CA-only path (trusted CA with no SAN or pin checks), no custom verifier is installed, so peer identity goes unverified—only the certificate chain is validated. Envoy/Istio exhibit the same behavior in equivalent scenarios. Either keep "HTTPS" endpoint identification for this path, or document that xDS intentionally allows CA-only verification without peer-identity verification.
🤖 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 110 - 111, In TransportSocketSnapshot, do not unconditionally call
ClientTlsSpec.builder().endpointIdentificationAlgorithm(""); instead check
whether CertificateValidationContextSnapshot.peerVerifierFactories() will
install a SAN/pin verifier and only clear the JSSE endpoint identification when
such a custom peer verifier is present; otherwise leave the default "HTTPS"
(i.e., do not call endpointIdentificationAlgorithm("")) so CA-only configs still
perform hostname/SAN peer identity checks; alternatively add a clear comment in
TransportSocketSnapshot explaining that CA-only verification intentionally omits
peer-identity checks if you choose to keep the current behavior.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6700 +/- ##
============================================
- Coverage 74.46% 73.86% -0.60%
- Complexity 22234 24257 +2023
============================================
Files 1963 2187 +224
Lines 82437 91171 +8734
Branches 10764 11927 +1163
============================================
+ Hits 61385 67345 +5960
- Misses 15918 18144 +2226
- Partials 5134 5682 +548 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
it/xds-client/src/test/java/com/linecorp/armeria/xds/it/PipeEndpointTest.java (1)
306-314: Consider capturing errors in the watcher to improve test debuggability.If SDS resolution fails, the watcher silently ignores the error and
snapshotRefremains null. The test will then timeout on theawait()assertion with a generic Awaitility timeout message rather than surfacing the actual failure cause.♻️ Suggested improvement
final AtomicReference<ListenerSnapshot> snapshotRef = new AtomicReference<>(); + final AtomicReference<Throwable> errorRef = new AtomicReference<>(); xdsBootstrap.listenerRoot("my-listener") .addSnapshotWatcher((snapshot, t) -> { if (snapshot != null) { snapshotRef.set(snapshot); + } else if (t != null) { + errorRef.set(t); } }); - await().untilAsserted(() -> assertThat(snapshotRef.get()).isNotNull()); + await().untilAsserted(() -> { + assertThat(errorRef.get()).isNull(); + assertThat(snapshotRef.get()).isNotNull(); + });🤖 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/PipeEndpointTest.java` around lines 306 - 314, The watcher passed to xdsBootstrap.listenerRoot("my-listener").addSnapshotWatcher currently ignores the Throwable parameter so SDS resolution failures are swallowed; modify the watcher in PipeEndpointTest to record any non-null Throwable (e.g., store it in an AtomicReference<Throwable> alongside snapshotRef or log it) and make the await assertion check that either snapshotRef.get() is non-null or the recorded Throwable is null/absent, so test failures surface the actual exception from the watcher (refer to ListenerSnapshot, snapshotRef, addSnapshotWatcher, and await()).xds/src/test/java/com/linecorp/armeria/xds/SotwXdsStreamTest.java (1)
58-59: Remove unused static field.The static
lifecycleObserverfield is never used—each test method creates its own localRecordingLifecycleObserverinstance. This appears to be leftover code from the previous implementation.Suggested fix
- private static final ConfigSourceLifecycleObserver lifecycleObserver = - new ConfigSourceLifecycleObserver() {};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xds/src/test/java/com/linecorp/armeria/xds/SotwXdsStreamTest.java` around lines 58 - 59, Remove the unused static field lifecycleObserver from SotwXdsStreamTest: locate the declaration "private static final ConfigSourceLifecycleObserver lifecycleObserver = new ConfigSourceLifecycleObserver() {};" and delete it, since tests create local RecordingLifecycleObserver instances and the static field is unused.it/xds-istio/src/test/java/com/linecorp/armeria/it/xds/XdsClientToSidecarTest.java (1)
133-137: RedundantsetDynamicResourcescall.Since
parsedBootstrap.toBuilder()already preserves the dynamic resources, explicitly setting them to the same value (line 136) is unnecessary.♻️ Proposed simplification
- final Bootstrap.DynamicResources dynamicResources = parsedBootstrap.getDynamicResources(); final Bootstrap bootstrap = parsedBootstrap.toBuilder() .setStaticResources(staticResources) - .setDynamicResources(dynamicResources) .build();🤖 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 133 - 137, The code redundantly retrieves dynamicResources from parsedBootstrap and then calls setDynamicResources on parsedBootstrap.toBuilder(), which already preserves dynamic resources; remove the unnecessary variable and the setDynamicResources(...) call and simply build the new Bootstrap via parsedBootstrap.toBuilder().setStaticResources(staticResources).build(), leaving parsedBootstrap and its dynamic resources intact (references: parsedBootstrap, toBuilder(), setStaticResources, setDynamicResources, Bootstrap.DynamicResources).
🤖 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/XdsClientToSidecarTest.java`:
- Line 74: The logger.info call in XdsClientToSidecarTest uses two arguments
(snapshot and t) but only one {} placeholder, so the throwable won't be logged;
update the logger.info invocation (the logger.info call) to include a
placeholder for the throwable (or pass the throwable as the dedicated Throwable
parameter form) so that t is properly recorded in the log.
- Line 144: The log call using logger.info("Listener snapshot: {}, t: ",
snapshot, t) is missing a placeholder for the second argument; update the format
string to include a {} for t (e.g. "Listener snapshot: {}, t: {}") and pass
snapshot and t as the two parameters to logger.info so both values are rendered
correctly (same fix as applied in clusterLoad()).
- Line 213: Update the logger call so the throwable is included in the formatted
message: change the logger.info invocation that currently reads
logger.info("Outbound listener snapshot: {}, error: ", snapshot, t); to include
a placeholder for the error (e.g. "Outbound listener snapshot: {}, error: {}")
and pass the throwable (t) as the corresponding argument (logger.info("Outbound
listener snapshot: {}, error: {}", snapshot, t)); locate the call using the
logger variable inside the XdsClientToSidecarTest watcher callback where
"Outbound listener snapshot" is logged.
In
`@xds/src/main/java/com/linecorp/armeria/xds/ResourceNodeMeterBinderFactory.java`:
- Around line 34-35: Document or enforce event-loop confinement for mutations in
ResourceNodeMeterBinderFactory and ResourceNodeMeterBinder: add a class- or
method-level Javadoc on ResourceNodeMeterBinderFactory and on
ResourceNodeMeterBinder.acquire()/close() stating that callers must be on the
event loop, or add runtime assertions (e.g., check eventLoop.inEventLoop()) at
the start of acquire() and close() to protect mutations of metersMap, refCount
and closed; also ensure the subscription returned from RefCountedStream is
invoked on the event loop or wrapped to schedule the close() call on the event
loop so removals cannot mutate shared state from arbitrary threads.
In `@xds/src/test/java/com/linecorp/armeria/xds/SotwXdsStreamTest.java`:
- Around line 97-99: The requests() accessor currently returns the internal
mutable List requests, exposing internal state and risking concurrent
modification; change requests() to return an immutable copy (e.g., use
ImmutableList.copyOf(requests) or Collections.unmodifiableList(new
ArrayList<>(requests))) to match responses() and ensure callers cannot mutate
the underlying list when iterating in tests referencing requests()/stream().
---
Nitpick comments:
In
`@it/xds-client/src/test/java/com/linecorp/armeria/xds/it/PipeEndpointTest.java`:
- Around line 306-314: The watcher passed to
xdsBootstrap.listenerRoot("my-listener").addSnapshotWatcher currently ignores
the Throwable parameter so SDS resolution failures are swallowed; modify the
watcher in PipeEndpointTest to record any non-null Throwable (e.g., store it in
an AtomicReference<Throwable> alongside snapshotRef or log it) and make the
await assertion check that either snapshotRef.get() is non-null or the recorded
Throwable is null/absent, so test failures surface the actual exception from the
watcher (refer to ListenerSnapshot, snapshotRef, addSnapshotWatcher, and
await()).
In
`@it/xds-istio/src/test/java/com/linecorp/armeria/it/xds/XdsClientToSidecarTest.java`:
- Around line 133-137: The code redundantly retrieves dynamicResources from
parsedBootstrap and then calls setDynamicResources on
parsedBootstrap.toBuilder(), which already preserves dynamic resources; remove
the unnecessary variable and the setDynamicResources(...) call and simply build
the new Bootstrap via
parsedBootstrap.toBuilder().setStaticResources(staticResources).build(), leaving
parsedBootstrap and its dynamic resources intact (references: parsedBootstrap,
toBuilder(), setStaticResources, setDynamicResources,
Bootstrap.DynamicResources).
In `@xds/src/test/java/com/linecorp/armeria/xds/SotwXdsStreamTest.java`:
- Around line 58-59: Remove the unused static field lifecycleObserver from
SotwXdsStreamTest: locate the declaration "private static final
ConfigSourceLifecycleObserver lifecycleObserver = new
ConfigSourceLifecycleObserver() {};" and delete it, since tests create local
RecordingLifecycleObserver instances and the static field is unused.
🪄 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: ef353d0b-05d0-4a1c-b6eb-82b6a766c91f
📒 Files selected for processing (9)
it/xds-client/src/test/java/com/linecorp/armeria/xds/it/PipeEndpointTest.javait/xds-istio/src/test/java/com/linecorp/armeria/it/xds/XdsClientToSidecarTest.javatesting-internal/src/main/java/com/linecorp/armeria/internal/testing/InternalTestingBlockHoundIntegration.javaxds/src/main/java/com/linecorp/armeria/xds/DefaultSubscriptionContext.javaxds/src/main/java/com/linecorp/armeria/xds/ResourceNodeAdapter.javaxds/src/main/java/com/linecorp/armeria/xds/ResourceNodeMeterBinderFactory.javaxds/src/main/java/com/linecorp/armeria/xds/SubscriptionContext.javaxds/src/test/java/com/linecorp/armeria/xds/DeltaXdsStreamTest.javaxds/src/test/java/com/linecorp/armeria/xds/SotwXdsStreamTest.java
✅ Files skipped from review due to trivial changes (3)
- testing-internal/src/main/java/com/linecorp/armeria/internal/testing/InternalTestingBlockHoundIntegration.java
- xds/src/main/java/com/linecorp/armeria/xds/ResourceNodeAdapter.java
- xds/src/test/java/com/linecorp/armeria/xds/DeltaXdsStreamTest.java
| final AtomicReference<ClusterSnapshot> snapshotRef = new AtomicReference<>(); | ||
| final AtomicReference<Throwable> errorRef = new AtomicReference<>(); | ||
| clusterRoot.addSnapshotWatcher((snapshot, t) -> { | ||
| logger.info("Cluster snapshot: {}, t: ", snapshot, t); |
Contributor
There was a problem hiding this comment.
Logger format string is missing a placeholder for the throwable.
The format string "Cluster snapshot: {}, t: " has two arguments (snapshot, t) but only one {} placeholder. The throwable t will not be logged, making debugging failures harder.
🔧 Proposed fix
- logger.info("Cluster snapshot: {}, t: ", snapshot, t);
+ logger.info("Cluster snapshot: {}, t: {}", snapshot, t);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| logger.info("Cluster snapshot: {}, t: ", snapshot, t); | |
| logger.info("Cluster snapshot: {}, t: {}", snapshot, t); |
🤖 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`
at line 74, The logger.info call in XdsClientToSidecarTest uses two arguments
(snapshot and t) but only one {} placeholder, so the throwable won't be logged;
update the logger.info invocation (the logger.info call) to include a
placeholder for the throwable (or pass the throwable as the dedicated Throwable
parameter form) so that t is properly recorded in the log.
| final AtomicReference<ListenerSnapshot> snapshotRef = new AtomicReference<>(); | ||
| final AtomicReference<Throwable> errorRef = new AtomicReference<>(); | ||
| listenerRoot.addSnapshotWatcher((snapshot, t) -> { | ||
| logger.info("Listener snapshot: {}, t: ", snapshot, t); |
Contributor
There was a problem hiding this comment.
Logger format string is missing a placeholder for the throwable.
Same issue as in clusterLoad().
🔧 Proposed fix
- logger.info("Listener snapshot: {}, t: ", snapshot, t);
+ logger.info("Listener snapshot: {}, t: {}", snapshot, t);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| logger.info("Listener snapshot: {}, t: ", snapshot, t); | |
| logger.info("Listener snapshot: {}, t: {}", snapshot, t); |
🤖 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`
at line 144, The log call using logger.info("Listener snapshot: {}, t: ",
snapshot, t) is missing a placeholder for the second argument; update the format
string to include a {} for t (e.g. "Listener snapshot: {}, t: {}") and pass
snapshot and t as the two parameters to logger.info so both values are rendered
correctly (same fix as applied in clusterLoad()).
| final AtomicReference<ListenerSnapshot> snapshotRef = new AtomicReference<>(); | ||
| final AtomicReference<Throwable> errorRef = new AtomicReference<>(); | ||
| listenerRoot.addSnapshotWatcher((snapshot, t) -> { | ||
| logger.info("Outbound listener snapshot: {}, error: ", snapshot, t); |
Contributor
There was a problem hiding this comment.
Logger format string is missing a placeholder for the throwable.
Same issue as in other watcher callbacks.
🔧 Proposed fix
- logger.info("Outbound listener snapshot: {}, error: ", snapshot, t);
+ logger.info("Outbound listener snapshot: {}, error: {}", snapshot, t);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| logger.info("Outbound listener snapshot: {}, error: ", snapshot, t); | |
| logger.info("Outbound listener snapshot: {}, error: {}", snapshot, t); |
🤖 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`
at line 213, Update the logger call so the throwable is included in the
formatted message: change the logger.info invocation that currently reads
logger.info("Outbound listener snapshot: {}, error: ", snapshot, t); to include
a placeholder for the error (e.g. "Outbound listener snapshot: {}, error: {}")
and pass the throwable (t) as the corresponding argument (logger.info("Outbound
listener snapshot: {}, error: {}", snapshot, t)); locate the call using the
logger variable inside the XdsClientToSidecarTest watcher callback where
"Outbound listener snapshot" is logged.
Comment on lines
+34
to
+35
| private final Map<Key, ResourceNodeMeterBinder> metersMap = new HashMap<>(); | ||
| private final MeterRegistry meterRegistry; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Acquire call sites"
rg -n --type=java -C2 '\bmeterBinderFactory\(\)\.acquire\s*\('
echo
echo "2) ResourceNodeMeterBinder close call sites"
rg -n --type=java -C2 '\bResourceNodeMeterBinder\b|\.close\s*\(\s*\)\s*;' xds/src/main/java/com/linecorp/armeria/xds
echo
echo "3) Existing event-loop assertions in XDS package"
rg -n --type=java -C1 '\binEventLoop\s*\(' xds/src/main/java/com/linecorp/armeria/xdsRepository: line/armeria
Length of output: 29038
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== ResourceNodeAdapter instantiation sites ==="
rg -n --type=java 'new ResourceNodeAdapter\s*\(' xds/src/main/java/com/linecorp/armeria/xds
echo
echo "=== ResourceNodeAdapter class declaration and context ==="
head -50 xds/src/main/java/com/linecorp/armeria/xds/ResourceNodeAdapter.java | cat -n
echo
echo "=== Check if ResourceNodeAdapter's closeSubscription() or similar is guarded ==="
rg -n --type=java -A5 'closeSubscription\|onClose' xds/src/main/java/com/linecorp/armeria/xds/ResourceNodeAdapter.javaRepository: line/armeria
Length of output: 103
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Look for ResourceNodeAdapter in the codebase ==="
fd -t f "ResourceNodeAdapter.java" xds/src
echo
echo "=== ResourceNodeAdapter instantiation sites (broader search) ==="
rg -n "ResourceNodeAdapter" xds/src/main/java/com/linecorp/armeria/xds/ -A2 -B2 | head -80
echo
echo "=== Check which classes call acquire() ==="
rg -n "\.acquire\s*\(" xds/src/main/java/com/linecorp/armeria/xds/ -B3 -A1Repository: line/armeria
Length of output: 5129
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check SubscriptionContext interface/class ==="
fd -t f "SubscriptionContext.java" xds/src
echo
echo "=== SubscriptionContext definition ==="
rg -n "interface SubscriptionContext|class SubscriptionContext" xds/src/main/java/com/linecorp/armeria/xds/ -A10
echo
echo "=== Check where ListenerStream.subscribe() is called ==="
rg -n "ListenerStream.*subscribe\|\.subscribe.*watcher" xds/src/main/java/com/linecorp/armeria/xds/ -B3 | head -60
echo
echo "=== Check ResourceNodeMeterBinderFactory class context ==="
head -55 xds/src/main/java/com/linecorp/armeria/xds/ResourceNodeMeterBinderFactory.java | cat -nRepository: line/armeria
Length of output: 1351
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check DefaultSubscriptionContext to see how it's initialized ==="
rg -n "class DefaultSubscriptionContext" xds/src/main/java/com/linecorp/armeria/xds/ -A20
echo
echo "=== Check where ResourceNodeAdapter/streams are subscribed from ==="
rg -n "ListenerStream\|ClusterStream\|EndpointStream" xds/src/main/java/com/linecorp/armeria/xds/ --type=java | grep -E "(subscribe|subscribe\()" | head -20
echo
echo "=== Look at ListenerManager/ClusterManager subscribe context ==="
sed -n '40,100p' xds/src/main/java/com/linecorp/armeria/xds/ListenerManager.java | cat -n
</script>Repository: line/armeria
Length of output: 3039
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check RefCountedStream (parent of ResourceNodeAdapter) ==="
head -100 xds/src/main/java/com/linecorp/armeria/xds/RefCountedStream.java | cat -n
echo
echo "=== Check if streams' subscribe() methods are event-loop guarded ==="
rg -n "class ListenerStream\|class ClusterStream\|class EndpointStream" xds/src/main/java/com/linecorp/armeria/xds/ -A40 | grep -E "(subscribe\(|eventLoop)" | head -30
echo
echo "=== Check ListenerManager.subscribe and if it's event-loop confined ==="
sed -n '55,90p' xds/src/main/java/com/linecorp/armeria/xds/ListenerManager.java | cat -nRepository: line/armeria
Length of output: 3364
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check ListenerManager.subscribe() fully ==="
sed -n '75,90p' xds/src/main/java/com/linecorp/armeria/xds/ListenerManager.java | cat -n
echo
echo "=== Check ListenerManager.subscribe call context (is it guarded) ==="
sed -n '65,105p' xds/src/main/java/com/linecorp/armeria/xds/ListenerManager.java | cat -n
echo
echo "=== Check if ResourceNodeMeterBinderFactory or acquire has any thread safety docs ==="
grep -n "acquire\|thread\|event" xds/src/main/java/com/linecorp/armeria/xds/ResourceNodeMeterBinderFactory.java | head -20
echo
echo "=== Full ResourceNodeMeterBinderFactory.acquire() method ==="
sed -n '43,52p' xds/src/main/java/com/linecorp/armeria/xds/ResourceNodeMeterBinderFactory.java | cat -nRepository: line/armeria
Length of output: 3019
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Where is ListenerManager.register() called from? ==="
rg -n "\.register\s*\(" xds/src/main/java/com/linecorp/armeria/xds/ -B4 -A1 | head -80
echo
echo "=== Check if XdsClusterManager has similar pattern ==="
sed -n '75,110p' xds/src/main/java/com/linecorp/armeria/xds/XdsClusterManager.java | cat -n
echo
echo "=== Check the close() path in ResourceNodeMeterBinder ==="
sed -n '88,115p' xds/src/main/java/com/linecorp/armeria/xds/ResourceNodeMeterBinderFactory.java | cat -nRepository: line/armeria
Length of output: 13460
Add event-loop confinement documentation or enforcement to ResourceNodeMeterBinderFactory.
The acquire() method and the close() path in ResourceNodeMeterBinder mutate shared state (metersMap, refCount, closed) without explicit event-loop guards. While current code paths likely execute on the event loop, the returned subscription closure from RefCountedStream can be invoked from arbitrary contexts when watchers are removed. Explicitly document the event-loop confinement requirement (via class-level Javadoc or method-level comments) or add runtime checks (assert eventLoop.inEventLoop()) to match the pattern used in ControlPlaneClientManager, ListenerManager, and XdsClusterManager.
🤖 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/ResourceNodeMeterBinderFactory.java`
around lines 34 - 35, Document or enforce event-loop confinement for mutations
in ResourceNodeMeterBinderFactory and ResourceNodeMeterBinder: add a class- or
method-level Javadoc on ResourceNodeMeterBinderFactory and on
ResourceNodeMeterBinder.acquire()/close() stating that callers must be on the
event loop, or add runtime assertions (e.g., check eventLoop.inEventLoop()) at
the start of acquire() and close() to protect mutations of metersMap, refCount
and closed; also ensure the subscription returned from RefCountedStream is
invoked on the event loop or wrapped to schedule the close() call on the event
loop so removals cannot mutate shared state from arbitrary threads.
Comment on lines
+97
to
+99
| List<DiscoveryRequest> requests() { | ||
| return requests; | ||
| } |
Contributor
There was a problem hiding this comment.
Inconsistent defensive copying between requests() and responses().
responses() returns an immutable copy (ImmutableList.copyOf), but requests() returns the raw mutable list. This inconsistency exposes internal state and makes the thread-safety issue worse—callers can inadvertently modify the list, and the stream() calls in tests (lines 256-257, 289-290) iterate over a potentially mutating collection.
Suggested fix
List<DiscoveryRequest> requests() {
- return requests;
+ return ImmutableList.copyOf(requests);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| List<DiscoveryRequest> requests() { | |
| return requests; | |
| } | |
| List<DiscoveryRequest> requests() { | |
| return ImmutableList.copyOf(requests); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@xds/src/test/java/com/linecorp/armeria/xds/SotwXdsStreamTest.java` around
lines 97 - 99, The requests() accessor currently returns the internal mutable
List requests, exposing internal state and risking concurrent modification;
change requests() to return an immutable copy (e.g., use
ImmutableList.copyOf(requests) or Collections.unmodifiableList(new
ArrayList<>(requests))) to match responses() and ensure callers cannot mutate
the underlying list when iterating in tests referencing requests()/stream().
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
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/XdsStreamSubscriber.java (1)
56-64:⚠️ Potential issue | 🟠 MajorKeep timeout-based absence replayable for late watchers.
The initial timeout still calls
onAbsent(), but this class no longer remembers that result andregisterWatcher()no longer replays any local state.StateCoordinatoronly replays absences it recorded itself, so a watcher added after the timeout now stays inWAITING_FOR_SERVERuntil another control-plane event arrives. Persist the timeout-driven absent state somewhere that late registrations can observe.Also applies to: 103-121
🤖 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/XdsStreamSubscriber.java` around lines 56 - 64, The timeout task in restartTimer() calls onAbsent() but doesn't persist that absent state for late watchers; modify restartTimer() (and the similar timer logic around lines 103-121) to record the timeout-driven absence into a durable local state (e.g., set an absentDueToTimeout flag or call StateCoordinator.recordAbsent with the same identity used for other absences) so that registerWatcher() can replay it for late registrations; ensure the flag/state is cleared when a real control-plane update arrives or when the timer is cancelled, and make registerWatcher() check this stored absent state and replay by invoking the watcher or delegating to StateCoordinator just as it does for other recorded absences.
🧹 Nitpick comments (10)
xds/src/main/java/com/linecorp/armeria/xds/ResourceNodeMeterBinderFactory.java (2)
38-41: Consider adding null validation for constructor parameters.While this is a package-private class, adding null checks would provide clearer error messages if invalid arguments are passed.
🛡️ Suggested defensive null checks
ResourceNodeMeterBinderFactory(MeterRegistry meterRegistry, MeterIdPrefix meterIdPrefix) { - this.meterRegistry = meterRegistry; - this.meterIdPrefix = meterIdPrefix; + this.meterRegistry = Objects.requireNonNull(meterRegistry, "meterRegistry"); + this.meterIdPrefix = Objects.requireNonNull(meterIdPrefix, "meterIdPrefix"); }🤖 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/ResourceNodeMeterBinderFactory.java` around lines 38 - 41, Add defensive null validation to the ResourceNodeMeterBinderFactory constructor: validate the meterRegistry and meterIdPrefix parameters (e.g., via Objects.requireNonNull or explicit checks) and throw a clear NPE with a descriptive message if either is null so callers get immediate, readable errors when constructing ResourceNodeMeterBinderFactory.
121-124: Consider null validation inKeyconstructor for safety.While this is an internal class,
resourceName.equals()in theequalsmethod would throw NPE ifresourceNameis null. Adding validation ensures fail-fast behavior.🛡️ Suggested null checks
private Key(XdsType type, String resourceName) { - this.type = type; - this.resourceName = resourceName; + this.type = Objects.requireNonNull(type, "type"); + this.resourceName = Objects.requireNonNull(resourceName, "resourceName"); }🤖 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/ResourceNodeMeterBinderFactory.java` around lines 121 - 124, The Key constructor currently assigns fields without null checks which can cause resourceName.equals(...) in Key.equals(...) to NPE; update the Key(XdsType type, String resourceName) constructor to validate inputs (e.g., use Objects.requireNonNull or explicit checks) for both type and resourceName so instances never hold nulls, and ensure hashCode/equals logic can rely on non-null fields (no further changes to equals required once constructor validation is added).xds/src/main/java/com/linecorp/armeria/xds/RouteEntry.java (1)
46-46: Consider adding@UnstableApito the class.The
RouteEntryclass is public but lacks the@UnstableApiannotation. Per the coding guidelines, newly added public classes in the xDS module should have this annotation. If this class predates this PR, this may be pre-existing, but it's worth addressing for API consistency.Suggested fix
+@UnstableApi public final class RouteEntry {🤖 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/RouteEntry.java` at line 46, Add the `@UnstableApi` annotation to the public RouteEntry class: annotate the RouteEntry class declaration with com.linecorp.armeria.common.annotation.UnstableApi (and add the corresponding import) so the public API is marked as unstable per xDS module guidelines.it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioInstaller.java (1)
112-129: Consider adding a timeout for process execution.The
runCommandmethod waits indefinitely for the process to complete viaprocess.waitFor(). Ifistioctlhangs, the test will hang forever. Consider usingprocess.waitFor(timeout, TimeUnit)to prevent indefinite blocking.♻️ Suggested enhancement
- final int exitCode = process.waitFor(); + final boolean finished = process.waitFor(10, java.util.concurrent.TimeUnit.MINUTES); + if (!finished) { + process.destroyForcibly(); + throw new IllegalStateException("Command timed out: " + command); + } + final int exitCode = process.exitValue();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioInstaller.java` around lines 112 - 129, The runCommand method currently blocks indefinitely on process.waitFor(); modify runCommand to use process.waitFor(timeout, TimeUnit.SECONDS) (pick a sensible timeout), and if it returns false (timed out) call process.destroyForcibly(), wait again for termination, and throw an IllegalStateException indicating a timeout; ensure you still consume the process output via the existing BufferedReader and include the command and exit/timeout info in the thrown message so callers of runCommand can fail fast when istioctl hangs.it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioServerExtension.java (1)
205-223: Consider checking container readiness conditions, not just pod phase.A pod in
"Running"phase may still have containers that are not ready (e.g., liveness/readiness probes not yet passing). For Istio sidecar injection scenarios, the sidecar container may take additional time to initialize. This could lead to flaky tests where requests fail because the Envoy sidecar isn't ready yet.Consider also checking
pod.getStatus().getContainerStatuses()to ensure all containers reportready = true.♻️ Suggested enhancement
return pods.stream().anyMatch(pod -> { final String phase = pod.getStatus() != null ? pod.getStatus().getPhase() : null; - return "Running".equals(phase); + if (!"Running".equals(phase)) { + return false; + } + // Also check that all containers are ready + if (pod.getStatus().getContainerStatuses() == null) { + return false; + } + return pod.getStatus().getContainerStatuses().stream() + .allMatch(cs -> Boolean.TRUE.equals(cs.getReady())); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioServerExtension.java` around lines 205 - 223, The current waitForReady method only checks pod phase ("Running") which can be true even if containers (including Istio sidecar) aren't ready; update the readiness predicate inside waitForReady (used by K8sClusterHelper.poll) to also inspect each Pod's container statuses via pod.getStatus().getContainerStatuses() and verify every ContainerStatus reports ready == true for all containers for the pod matching withLabel("app", serviceName), and only return true when at least one pod both is in "Running" phase and all its container statuses have ready==true; keep the same timeout behavior and throw the same IllegalStateException on timeout.it/xds-istio/src/test/java/com/linecorp/armeria/it/xds/XdsClientToSidecarTest.java (1)
83-89: Consider specifying explicit timeout forawait()assertions.The
await().untilAsserted()calls use Awaitility's default timeout (10 seconds). For xDS resource loading from Istio, this might be too tight, leading to flaky tests. Consider using explicit timeouts likeawait().atMost(Duration.ofMinutes(1))to provide adequate time for xDS resources to propagate.♻️ Suggested enhancement
- await().untilAsserted(() -> { + await().atMost(Duration.ofMinutes(1)).untilAsserted(() -> {Also applies to: 153-163, 222-232
🤖 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 83 - 89, The Awaitility waits in XdsClientToSidecarTest use the default timeout; update the await() calls (the ones invoking untilAsserted in this test class) to set an explicit longer timeout such as await().atMost(Duration.ofMinutes(1)).untilAsserted(...), and add the necessary import for java.time.Duration; apply the same change to the other await().untilAsserted usages in this class (the subsequent assertions that wait for xDS snapshots and load completion).xds/src/main/java/com/linecorp/armeria/xds/TransportSocketSnapshot.java (1)
68-74: Tighten theupstreamTlsContextcontract.The new parameter is nullable, but
buildClientTlsSpec()disables endpoint identification even when it is absent. That makesbuildClientTlsSpec(null, ...)a surprising footgun for any future caller. Either require a non-nullUpstreamTlsContexthere, or keep the override inside the non-null branch so the behavior stays scoped to actual xDS upstream TLS configs.Also applies to: 106-111
🤖 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 68 - 74, The constructor currently accepts a nullable UpstreamTlsContext but calls buildClientTlsSpec(upstreamTlsContext, ...) unconditionally, which lets the method apply endpoint-identification overrides even when upstreamTlsContext is null; change the contract so upstreamTlsContext is non-null (remove `@Nullable` and validate callers) or only call/compute the endpoint-identification override when upstreamTlsContext != null (move the override logic inside the non-null branch) so buildClientTlsSpec is only passed a real UpstreamTlsContext; apply the same fix to the other constructor/usage at the block around the second occurrence (lines 106-111) referencing buildClientTlsSpec and clientTlsSpec.it/xds-client/src/test/java/com/linecorp/armeria/xds/it/PipeEndpointTest.java (1)
307-314: Fail fast on listener watcher errors.This callback ignores
t, so an SDS/listener assembly failure degrades into an Awaitility timeout with no useful cause. Capture the throwable and assert it stays null while waiting forsnapshotRef.🛠️ Suggested change
- final AtomicReference<ListenerSnapshot> snapshotRef = new AtomicReference<>(); + final AtomicReference<ListenerSnapshot> snapshotRef = new AtomicReference<>(); + final AtomicReference<Throwable> errorRef = new AtomicReference<>(); xdsBootstrap.listenerRoot("my-listener") .addSnapshotWatcher((snapshot, t) -> { + if (t != null) { + errorRef.set(t); + return; + } if (snapshot != null) { snapshotRef.set(snapshot); } }); - await().untilAsserted(() -> assertThat(snapshotRef.get()).isNotNull()); + await().untilAsserted(() -> { + assertThat(errorRef.get()).isNull(); + assertThat(snapshotRef.get()).isNotNull(); + });🤖 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/PipeEndpointTest.java` around lines 307 - 314, The listener watcher callback for xdsBootstrap.listenerRoot("my-listener").addSnapshotWatcher ignores the throwable parameter (t), causing SDS/listener assembly failures to manifest as Awaitility timeouts; update the watcher (and the await assertion) to capture the throwable alongside the ListenerSnapshot (snapshotRef/optional throwableRef), and in the await().untilAsserted block assert that snapshotRef.get() is not null and that the captured throwable is null so failures fail fast and surface the real error from addSnapshotWatcher.xds/src/main/java/com/linecorp/armeria/xds/ClusterXdsResource.java (1)
41-46: Avoid re-parsing TLS config on revision-only copies.
withRevision()currently goes back through the validating constructor, so every internal revision bump re-runsXdsValidatorIndexRegistry.assertValid()and unpacks the same transport-socket config again. A cheap copy path would keep this hot path lighter.♻️ Suggested copy-path refactor
- ClusterXdsResource(Cluster cluster, String version) { - this(cluster, version, 0); + ClusterXdsResource(Cluster cluster, String version) { + this(validate(cluster), version, 0, upstreamTlsContext(cluster)); } - private ClusterXdsResource(Cluster cluster, String version, long revision) { + private ClusterXdsResource(Cluster cluster, String version, long revision, + `@Nullable` UpstreamTlsContext upstreamTlsContext) { super(version, revision); - XdsValidatorIndexRegistry.assertValid(cluster); this.cluster = cluster; - upstreamTlsContext = upstreamTlsContext(cluster); + this.upstreamTlsContext = upstreamTlsContext; + } + + private static Cluster validate(Cluster cluster) { + XdsValidatorIndexRegistry.assertValid(cluster); + return cluster; } @@ ClusterXdsResource withRevision(long revision) { if (revision == revision()) { return this; } - return new ClusterXdsResource(cluster, version(), revision); + return new ClusterXdsResource(cluster, version(), revision, upstreamTlsContext); }Also applies to: 91-97
🤖 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/ClusterXdsResource.java` around lines 41 - 46, The current ClusterXdsResource constructors cause withRevision() to call the validating constructor which re-invokes XdsValidatorIndexRegistry.assertValid() and re-unpacks the transport-socket on every revision bump; add a cheap copy-path constructor (e.g., a private ClusterXdsResource(Cluster cluster, String version, long revision, ParsedTransportSocket parsedTls) or a constructor/flag that skips validation) that reuses already-parsed TLS/transport-socket state and does not call XdsValidatorIndexRegistry.assertValid(), and update withRevision() (and the similar copy path at the other occurrence) to use this new constructor so revision-only copies avoid re-parsing/unpacking the TLS config.xds/src/main/java/com/linecorp/armeria/xds/DeltaActualStream.java (1)
275-279: Redundantinstanceofcheck.The
resourcevariable is already of typeO extends XdsResourcefromParsedResourcesHolder<O>, so theinstanceof XdsResourcecheck will always be true. This can be simplified.Proposed simplification
-holder.parsedResources().forEach((name, resource) -> { - if (resource instanceof XdsResource) { - stateCoordinator.onResourceUpdated(type, name, (XdsResource) resource); - } -}); +holder.parsedResources().forEach((name, resource) -> + stateCoordinator.onResourceUpdated(type, name, resource));🤖 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/DeltaActualStream.java` around lines 275 - 279, The loop in DeltaActualStream uses an unnecessary instanceof check: inside holder.parsedResources().forEach((name, resource) -> { if (resource instanceof XdsResource) { stateCoordinator.onResourceUpdated(type, name, (XdsResource) resource); } }); resource is already typed as O extends XdsResource from ParsedResourcesHolder<O>, so remove the instanceof guard and the cast—invoke stateCoordinator.onResourceUpdated(type, name, resource) directly to simplify and avoid redundant checks.
🤖 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-client/src/test/java/com/linecorp/armeria/xds/it/DeltaXdsResourceWatcherTest.java`:
- Around line 260-269: Before deleting the snapshot, record the current
errors.size(), then call cache.setSnapshot(GROUP, Snapshot.create(..., "4")) and
replace the broad assertThat(errors).isNotEmpty() with an await().untilAsserted
that waits for errors.size() to increase by one and contains a new
MissingXdsResourceException for resource "cluster1" (originating from CLUSTER or
ENDPOINT handlers); use the existing errors collection and assert the newest
throwable is an instance of MissingXdsResourceException and references
"cluster1" to ensure the failure is due to the deletion triggered by
cache.setSnapshot rather than an earlier error.
In
`@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioClusterExtension.java`:
- Around line 31-33: Update the stale Javadoc in IstioClusterExtension.java to
refer to K3s instead of Kind: edit the class-level or method Javadoc lines that
currently say "Kind container" or "Kind" (the description items mentioning reuse
or creation of a Kind container) so they correctly state "K3s container" or "K3s
cluster/container" and keep the rest of the text (e.g., that kubeconfig is used,
or Istio is always installed) unchanged.
In
`@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioInstaller.java`:
- Around line 80-86: In enableNamespaceInjection(KubernetesClient client, String
namespaceName) protect against null metadata/labels: fetch ns.getMetadata(), if
it is null create a new ObjectMeta and set it on ns, then getLabels() and if
labels is null create a new HashMap, set it back via
ns.getMetadata().setLabels(...), then put("istio-injection","enabled"); keep the
logger.info call. This ensures no NPE when editing the namespace labels.
In
`@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioTestImage.java`:
- Around line 61-65: The docker save invocation in IstioTestImage.java (the
ProcessBuilder that constructs saveProcess for IMAGE_NAME and writes to
tempFile) currently calls saveProcess.waitFor() with no timeout and doesn't
handle stderr; change the ProcessBuilder to redirectErrorStream(true) (or
otherwise consume the error stream) and replace the blocking waitFor() with a
timed wait e.g. saveProcess.waitFor(<reasonableTimeout>, TimeUnit.SECONDS); if
the timed wait returns false, destroy the process
(saveProcess.destroyForcibly()), wait briefly, then throw an
IllegalStateException indicating timeout; also check the exit value after a
successful timed wait and throw with stderr/exit details if non-zero.
In
`@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/K8sClusterHelper.java`:
- Around line 59-68: The method startK3sAndWaitReady starts a K3sContainer (k3s)
but never closes it on failure paths; ensure the container is closed when
readiness check or image loading fails by wrapping the startup sequence in a
try/catch that on any Exception/Throwable calls k3s.close() (or k3s.stop()/close
equivalent) and rethrows the original error; specifically, keep the existing
calls to startK3s(), waitForReadyNode(...), and IstioTestImage.loadIntoK3s(k3s)
but add error handling around them so that if waitForReadyNode returns false or
loadIntoK3s throws, you first close k3s (using the K3sContainer instance) and
then throw the IllegalStateException or propagate the original exception.
In
`@it/xds-istio/src/test/java/com/linecorp/armeria/it/istio/testing/IstioTestingBlockHoundIntegration.java`:
- Line 22: Add a class-level Javadoc comment above the public SPI class
IstioTestingBlockHoundIntegration that briefly describes its purpose
(integrating BlockHound checks for Istio testing), its scope (public SPI
intended for consumers to register integration behavior), and any important
usage or stability notes; ensure the Javadoc follows project conventions (short
summary sentence, optionally a longer description and relevant tags such as
`@since` or `@see` if applicable).
In `@it/xds-istio/src/test/java/com/linecorp/armeria/it/xds/EnvoyDebugTest.java`:
- Around line 82-83: The test is logging full Envoy config payloads at INFO
which can leak sensitive data and bloat CI logs; change the logger calls in
EnvoyDebugTest that call logger.info("Envoy config dump: {}",
response.contentUtf8()) (and the similar calls around lines 97-99) to either
logger.debug and/or log a truncated/summary version (e.g., first N chars or
specific summary fields) by using response.contentUtf8().substring(0, N) or
extracting key fields before logging, ensuring you still use the existing logger
instance and avoid printing the full payload.
In
`@it/xds-istio/src/test/java/com/linecorp/armeria/it/xds/XdsResourceReader.java`:
- Line 41: Add concise Javadoc to the public class XdsResourceReader and to each
public/protected helper method exposed in the same file (the methods around
lines 68-103) describing the purpose, supported inputs (allowed parameter
types/values), return value, and failure behavior (exceptions thrown or error
conditions). For each method, mention nullability expectations for
parameters/returns and any side effects or threading assumptions; for the
class-level Javadoc, include a short summary of its role and typical usage.
Ensure comments follow project style and are added to all public/protected APIs
in this file.
- Around line 68-103: Add explicit null checks at the start of the public
methods fromYaml(String), fromYaml(String, Class<T>), and fromJson(String,
Class<T>) by calling Objects.requireNonNull for each public parameter (yaml,
json, clazz) with a helpful message; e.g., in fromYaml(String yaml) validate
yaml, in fromYaml(String yaml, Class<T> clazz) validate both yaml and clazz, and
in fromJson(String json, Class<T> clazz) validate json and clazz before any
processing (so the subsequent mapper/parser calls and reflection on clazz.run
only when inputs are non-null).
In `@xds/src/main/java/com/linecorp/armeria/xds/AdsXdsStream.java`:
- Around line 110-114: The close() method must run its entire shutdown sequence
on the eventLoop to preserve ordering between stop() (which enqueues the stream
shutdown) and lifecycleObserver.close() so it doesn't race with
actualStream.closeStream() callbacks; modify AdsXdsStream.close() to check if
the current thread is the eventLoop and if so call stop() then
lifecycleObserver.close() directly, otherwise schedule a Runnable on eventLoop
(eventLoop.execute(...)) that calls stop() followed by
lifecycleObserver.close(), and ensure any thrown exceptions are caught/logged
inside the scheduled task.
In `@xds/src/main/java/com/linecorp/armeria/xds/ListenerXdsResource.java`:
- Around line 138-151: extractHcmFromFilterChains currently returns the first
HCM found and ignores others; update it to explicitly validate and reject
multi-chain or multi-HCM listeners: iterate listener.getFilterChainsList(),
count filter chains and count matches where filter.getName() equals
HTTP_CONNECTION_MANAGER_FILTER_NAME and typedConfig type URL equals
HTTP_CONNECTION_MANAGER_TYPE_URL (the place you call
XdsValidatorIndexRegistry.unpack for HttpConnectionManager), and if
filterChains.size() > 1 or matchedHcmCount > 1, fail fast by logging an error
and returning null or throwing an IllegalArgumentException (choose consistent
error behavior used elsewhere), otherwise unpack and return the single
HttpConnectionManager as before.
In `@xds/src/main/java/com/linecorp/armeria/xds/RouteEntry.java`:
- Around line 85-91: The comparison retryPolicy ==
RetryPolicy.getDefaultInstance() is using reference equality on a protobuf
message; change it to use .equals(), i.e. call
RetryPolicy.getDefaultInstance().equals(retryPolicy) (or
retryPolicy.equals(...)) when computing effectiveRetryPolicy for the value
produced by route.getRoute().getRetryPolicy(), so effectiveRetryPolicy is null
only when the message is equal to the default instance before passing it into
FilterUtil.buildUpstreamFilter and creating
clientDecoration/httpClient/rpcClient.
In `@xds/src/main/java/com/linecorp/armeria/xds/StateCoordinator.java`:
- Around line 47-50: The replayToWatcher(...) call in register(...) can throw
and currently propagates after subscriberStorage.register(...) has already
succeeded; wrap the replayToWatcher(type, resourceName, watcher) invocation in a
try-catch that catches Throwable, logs the error (consistent with how
XdsStreamSubscriber handles watcher failures), and suppresses the exception so
the method still returns the boolean from subscriberStorage.register(...)
unchanged; apply the same try-catch/log-and-suppress pattern to the other
replayToWatcher(...) invocation(s) in the file (the block around the 124-141
region) so watcher exceptions never leave the replay path.
In `@xds/src/main/java/com/linecorp/armeria/xds/SubscriberStorage.java`:
- Around line 52-57: Update the misleading comment around the timeout logic so
it no longer states that explicit 0s equals omitted/unset; instead state that
timeoutMillis == 0 means "no timeout" (indefinite wait) and that an omitted
initial_fetch_timeout is handled elsewhere (see
ConfigSourceClient.initialFetchTimeoutMillis(), which defaults to 15_000ms);
ensure the boolean enableAbsentOnTimeout (computed as !delta && timeoutMillis >
0) is described as enabling the absent-on-timeout behavior only when a positive
explicit timeout is provided, and add unit tests verifying explicit 0s versus
omitted timeout behavior flows through XdsStreamSubscriber and
ConfigSourceClient.initialFetchTimeoutMillis().
In `@xds/src/test/java/com/linecorp/armeria/xds/SubscriberStorageTest.java`:
- Around line 68-70: The second assertion reading watcher.missingName is not
guaranteed to be visible to the test thread because onResourceDoesNotExist()
runs on the event loop; move the
assertThat(watcher.missingName).isEqualTo(ROUTE_NAME) into the Awaitility
untilAsserted lambda so both assertions
(assertThat(watcher.missingType).isEqualTo(XdsType.ROUTE) and the missingName
check) execute under the same synchronized wait; apply the same change for the
other similar blocks that assert watcher.missingType/missingName so all
visibility-sensitive assertions are inside untilAsserted.
---
Outside diff comments:
In `@xds/src/main/java/com/linecorp/armeria/xds/XdsStreamSubscriber.java`:
- Around line 56-64: The timeout task in restartTimer() calls onAbsent() but
doesn't persist that absent state for late watchers; modify restartTimer() (and
the similar timer logic around lines 103-121) to record the timeout-driven
absence into a durable local state (e.g., set an absentDueToTimeout flag or call
StateCoordinator.recordAbsent with the same identity used for other absences) so
that registerWatcher() can replay it for late registrations; ensure the
flag/state is cleared when a real control-plane update arrives or when the timer
is cancelled, and make registerWatcher() check this stored absent state and
replay by invoking the watcher or delegating to StateCoordinator just as it does
for other recorded absences.
---
Nitpick comments:
In
`@it/xds-client/src/test/java/com/linecorp/armeria/xds/it/PipeEndpointTest.java`:
- Around line 307-314: The listener watcher callback for
xdsBootstrap.listenerRoot("my-listener").addSnapshotWatcher ignores the
throwable parameter (t), causing SDS/listener assembly failures to manifest as
Awaitility timeouts; update the watcher (and the await assertion) to capture the
throwable alongside the ListenerSnapshot (snapshotRef/optional throwableRef),
and in the await().untilAsserted block assert that snapshotRef.get() is not null
and that the captured throwable is null so failures fail fast and surface the
real error from addSnapshotWatcher.
In
`@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioInstaller.java`:
- Around line 112-129: The runCommand method currently blocks indefinitely on
process.waitFor(); modify runCommand to use process.waitFor(timeout,
TimeUnit.SECONDS) (pick a sensible timeout), and if it returns false (timed out)
call process.destroyForcibly(), wait again for termination, and throw an
IllegalStateException indicating a timeout; ensure you still consume the process
output via the existing BufferedReader and include the command and exit/timeout
info in the thrown message so callers of runCommand can fail fast when istioctl
hangs.
In
`@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioServerExtension.java`:
- Around line 205-223: The current waitForReady method only checks pod phase
("Running") which can be true even if containers (including Istio sidecar)
aren't ready; update the readiness predicate inside waitForReady (used by
K8sClusterHelper.poll) to also inspect each Pod's container statuses via
pod.getStatus().getContainerStatuses() and verify every ContainerStatus reports
ready == true for all containers for the pod matching withLabel("app",
serviceName), and only return true when at least one pod both is in "Running"
phase and all its container statuses have ready==true; keep the same timeout
behavior and throw the same IllegalStateException on timeout.
In
`@it/xds-istio/src/test/java/com/linecorp/armeria/it/xds/XdsClientToSidecarTest.java`:
- Around line 83-89: The Awaitility waits in XdsClientToSidecarTest use the
default timeout; update the await() calls (the ones invoking untilAsserted in
this test class) to set an explicit longer timeout such as
await().atMost(Duration.ofMinutes(1)).untilAsserted(...), and add the necessary
import for java.time.Duration; apply the same change to the other
await().untilAsserted usages in this class (the subsequent assertions that wait
for xDS snapshots and load completion).
In `@xds/src/main/java/com/linecorp/armeria/xds/ClusterXdsResource.java`:
- Around line 41-46: The current ClusterXdsResource constructors cause
withRevision() to call the validating constructor which re-invokes
XdsValidatorIndexRegistry.assertValid() and re-unpacks the transport-socket on
every revision bump; add a cheap copy-path constructor (e.g., a private
ClusterXdsResource(Cluster cluster, String version, long revision,
ParsedTransportSocket parsedTls) or a constructor/flag that skips validation)
that reuses already-parsed TLS/transport-socket state and does not call
XdsValidatorIndexRegistry.assertValid(), and update withRevision() (and the
similar copy path at the other occurrence) to use this new constructor so
revision-only copies avoid re-parsing/unpacking the TLS config.
In `@xds/src/main/java/com/linecorp/armeria/xds/DeltaActualStream.java`:
- Around line 275-279: The loop in DeltaActualStream uses an unnecessary
instanceof check: inside holder.parsedResources().forEach((name, resource) -> {
if (resource instanceof XdsResource) { stateCoordinator.onResourceUpdated(type,
name, (XdsResource) resource); } }); resource is already typed as O extends
XdsResource from ParsedResourcesHolder<O>, so remove the instanceof guard and
the cast—invoke stateCoordinator.onResourceUpdated(type, name, resource)
directly to simplify and avoid redundant checks.
In
`@xds/src/main/java/com/linecorp/armeria/xds/ResourceNodeMeterBinderFactory.java`:
- Around line 38-41: Add defensive null validation to the
ResourceNodeMeterBinderFactory constructor: validate the meterRegistry and
meterIdPrefix parameters (e.g., via Objects.requireNonNull or explicit checks)
and throw a clear NPE with a descriptive message if either is null so callers
get immediate, readable errors when constructing ResourceNodeMeterBinderFactory.
- Around line 121-124: The Key constructor currently assigns fields without null
checks which can cause resourceName.equals(...) in Key.equals(...) to NPE;
update the Key(XdsType type, String resourceName) constructor to validate inputs
(e.g., use Objects.requireNonNull or explicit checks) for both type and
resourceName so instances never hold nulls, and ensure hashCode/equals logic can
rely on non-null fields (no further changes to equals required once constructor
validation is added).
In `@xds/src/main/java/com/linecorp/armeria/xds/RouteEntry.java`:
- Line 46: Add the `@UnstableApi` annotation to the public RouteEntry class:
annotate the RouteEntry class declaration with
com.linecorp.armeria.common.annotation.UnstableApi (and add the corresponding
import) so the public API is marked as unstable per xDS module guidelines.
In `@xds/src/main/java/com/linecorp/armeria/xds/TransportSocketSnapshot.java`:
- Around line 68-74: The constructor currently accepts a nullable
UpstreamTlsContext but calls buildClientTlsSpec(upstreamTlsContext, ...)
unconditionally, which lets the method apply endpoint-identification overrides
even when upstreamTlsContext is null; change the contract so upstreamTlsContext
is non-null (remove `@Nullable` and validate callers) or only call/compute the
endpoint-identification override when upstreamTlsContext != null (move the
override logic inside the non-null branch) so buildClientTlsSpec is only passed
a real UpstreamTlsContext; apply the same fix to the other constructor/usage at
the block around the second occurrence (lines 106-111) referencing
buildClientTlsSpec and clientTlsSpec.
🪄 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: 4678eb6f-8f2e-4904-8eea-e5396d899ea3
📒 Files selected for processing (106)
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/client/HttpClientDelegate.javacore/src/main/java/com/linecorp/armeria/internal/common/util/SslContextUtil.javacore/src/main/java/com/linecorp/armeria/server/ServerTlsSpec.javacore/src/test/java/com/linecorp/armeria/common/tls/TlsSpecPerRequestTest.javadependencies.tomlit/xds-client/src/test/java/com/linecorp/armeria/xds/it/ConfigSourceLifecycleObserverTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/DeltaXdsPreprocessorTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/DeltaXdsResourceWatcherTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/PipeEndpointTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/XdsControlPlaneMatrixTest.javait/xds-istio/build.gradleit/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/DockerAvailableCondition.javait/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/EnabledIfDockerAvailable.javait/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/HostOnlyExtension.javait/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioClusterExtension.javait/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioEnv.javait/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioInstaller.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/IstioPodEntryPoint.javait/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioPodTest.javait/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioServerExtension.javait/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioState.javait/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioTestExtension.javait/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioTestImage.javait/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/K8sClusterHelper.javait/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/PodCustomizer.javait/xds-istio/src/test/java/com/linecorp/armeria/it/istio/testing/IstioImageBuildTest.javait/xds-istio/src/test/java/com/linecorp/armeria/it/istio/testing/IstioStartupTest.javait/xds-istio/src/test/java/com/linecorp/armeria/it/istio/testing/IstioTestingBlockHoundIntegration.javait/xds-istio/src/test/java/com/linecorp/armeria/it/xds/EchoConfigurator.javait/xds-istio/src/test/java/com/linecorp/armeria/it/xds/EnvoyDebugTest.javait/xds-istio/src/test/java/com/linecorp/armeria/it/xds/XdsClientToSidecarTest.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/reactor.blockhound.integration.BlockHoundIntegrationlicenses/web-licenses.txtsettings.gradletesting-internal/src/main/java/com/linecorp/armeria/internal/testing/InternalTestingBlockHoundIntegration.javaxds/src/main/java/com/linecorp/armeria/xds/AbstractXdsResource.javaxds/src/main/java/com/linecorp/armeria/xds/AdsXdsStream.javaxds/src/main/java/com/linecorp/armeria/xds/CertificateValidationContextSnapshot.javaxds/src/main/java/com/linecorp/armeria/xds/ClusterResourceParser.javaxds/src/main/java/com/linecorp/armeria/xds/ClusterStream.javaxds/src/main/java/com/linecorp/armeria/xds/ClusterXdsResource.javaxds/src/main/java/com/linecorp/armeria/xds/CompositeXdsStream.javaxds/src/main/java/com/linecorp/armeria/xds/ConfigSourceClient.javaxds/src/main/java/com/linecorp/armeria/xds/ConfigSourceLifecycleObserver.javaxds/src/main/java/com/linecorp/armeria/xds/DefaultConfigSourceLifecycleObserver.javaxds/src/main/java/com/linecorp/armeria/xds/DefaultResponseHandler.javaxds/src/main/java/com/linecorp/armeria/xds/DefaultSubscriptionContext.javaxds/src/main/java/com/linecorp/armeria/xds/DeltaActualStream.javaxds/src/main/java/com/linecorp/armeria/xds/DeltaDiscoveryStub.javaxds/src/main/java/com/linecorp/armeria/xds/EndpointResourceParser.javaxds/src/main/java/com/linecorp/armeria/xds/EndpointStream.javaxds/src/main/java/com/linecorp/armeria/xds/EndpointXdsResource.javaxds/src/main/java/com/linecorp/armeria/xds/FilterUtil.javaxds/src/main/java/com/linecorp/armeria/xds/ListenerResourceParser.javaxds/src/main/java/com/linecorp/armeria/xds/ListenerSnapshot.javaxds/src/main/java/com/linecorp/armeria/xds/ListenerStream.javaxds/src/main/java/com/linecorp/armeria/xds/ListenerXdsResource.javaxds/src/main/java/com/linecorp/armeria/xds/ParsedFilterConfig.javaxds/src/main/java/com/linecorp/armeria/xds/ResourceNodeAdapter.javaxds/src/main/java/com/linecorp/armeria/xds/ResourceNodeMeterBinder.javaxds/src/main/java/com/linecorp/armeria/xds/ResourceNodeMeterBinderFactory.javaxds/src/main/java/com/linecorp/armeria/xds/ResourceParser.javaxds/src/main/java/com/linecorp/armeria/xds/ResourceStateStore.javaxds/src/main/java/com/linecorp/armeria/xds/RouteEntry.javaxds/src/main/java/com/linecorp/armeria/xds/RouteResourceParser.javaxds/src/main/java/com/linecorp/armeria/xds/RouteSnapshot.javaxds/src/main/java/com/linecorp/armeria/xds/RouteStream.javaxds/src/main/java/com/linecorp/armeria/xds/RouteXdsResource.javaxds/src/main/java/com/linecorp/armeria/xds/SecretResourceParser.javaxds/src/main/java/com/linecorp/armeria/xds/SecretXdsResource.javaxds/src/main/java/com/linecorp/armeria/xds/SnapshotStream.javaxds/src/main/java/com/linecorp/armeria/xds/SotwActualStream.javaxds/src/main/java/com/linecorp/armeria/xds/SotwXdsStream.javaxds/src/main/java/com/linecorp/armeria/xds/StateCoordinator.javaxds/src/main/java/com/linecorp/armeria/xds/SubscriberStorage.javaxds/src/main/java/com/linecorp/armeria/xds/SubscriptionContext.javaxds/src/main/java/com/linecorp/armeria/xds/SwitchMapEagerStream.javaxds/src/main/java/com/linecorp/armeria/xds/TlsCertificateSnapshot.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/VirtualHostXdsResource.javaxds/src/main/java/com/linecorp/armeria/xds/XdsConverterUtil.javaxds/src/main/java/com/linecorp/armeria/xds/XdsResource.javaxds/src/main/java/com/linecorp/armeria/xds/XdsResourceException.javaxds/src/main/java/com/linecorp/armeria/xds/XdsResponseHandler.javaxds/src/main/java/com/linecorp/armeria/xds/XdsStream.javaxds/src/main/java/com/linecorp/armeria/xds/XdsStreamSubscriber.javaxds/src/main/java/com/linecorp/armeria/xds/client/endpoint/RouterFilterFactory.javaxds/src/main/java/com/linecorp/armeria/xds/client/endpoint/XdsEndpointUtil.javaxds/src/main/java/com/linecorp/armeria/xds/client/endpoint/XdsHealthCheckedEndpointGroupBuilder.javaxds/src/main/java/com/linecorp/armeria/xds/filter/HttpFilterFactory.javaxds/src/main/java/com/linecorp/armeria/xds/filter/HttpFilterFactoryRegistry.javaxds/src/main/java/com/linecorp/armeria/xds/filter/XdsHttpFilter.javaxds/src/test/java/com/linecorp/armeria/xds/DeltaXdsStreamTest.javaxds/src/test/java/com/linecorp/armeria/xds/SotwXdsStreamTest.javaxds/src/test/java/com/linecorp/armeria/xds/StateCoordinatorTest.javaxds/src/test/java/com/linecorp/armeria/xds/StreamSwitchMapEagerTest.javaxds/src/test/java/com/linecorp/armeria/xds/SubscriberStorageTest.javaxds/src/test/java/com/linecorp/armeria/xds/XdsClientCleanupTest.java
💤 Files with no reviewable changes (7)
- xds/src/main/java/com/linecorp/armeria/xds/TlsCertificateSnapshot.java
- xds/src/main/java/com/linecorp/armeria/xds/DefaultResponseHandler.java
- xds/src/main/java/com/linecorp/armeria/xds/XdsResponseHandler.java
- xds/src/main/java/com/linecorp/armeria/xds/VirtualHostSnapshot.java
- xds/src/main/java/com/linecorp/armeria/xds/ParsedFilterConfig.java
- xds/src/main/java/com/linecorp/armeria/xds/SotwXdsStream.java
- xds/src/main/java/com/linecorp/armeria/xds/ResourceNodeMeterBinder.java
✅ Files skipped from review due to trivial changes (27)
- it/xds-istio/src/test/resources/META-INF/services/reactor.blockhound.integration.BlockHoundIntegration
- core/src/test/java/com/linecorp/armeria/common/tls/TlsSpecPerRequestTest.java
- core/src/main/java/com/linecorp/armeria/server/ServerTlsSpec.java
- xds/src/main/java/com/linecorp/armeria/xds/EndpointStream.java
- xds/src/main/java/com/linecorp/armeria/xds/XdsStream.java
- it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/EnabledIfDockerAvailable.java
- it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/PodCustomizer.java
- xds/src/main/java/com/linecorp/armeria/xds/ClusterStream.java
- testing-internal/src/main/java/com/linecorp/armeria/internal/testing/InternalTestingBlockHoundIntegration.java
- it/xds-istio/src/test/java/com/linecorp/armeria/it/xds/EchoConfigurator.java
- xds/src/main/java/com/linecorp/armeria/xds/SubscriptionContext.java
- it/xds-istio/src/test/resources/META-INF/services/com.linecorp.armeria.xds.filter.HttpFilterFactory
- xds/src/main/java/com/linecorp/armeria/xds/XdsResource.java
- core/src/main/java/com/linecorp/armeria/client/ClientTlsSpecBuilder.java
- xds/src/main/java/com/linecorp/armeria/xds/SecretResourceParser.java
- it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioPodEntryPoint.java
- xds/src/test/java/com/linecorp/armeria/xds/XdsClientCleanupTest.java
- xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/XdsHealthCheckedEndpointGroupBuilder.java
- xds/src/main/java/com/linecorp/armeria/xds/XdsConverterUtil.java
- xds/src/main/java/com/linecorp/armeria/xds/ResourceStateStore.java
- xds/src/test/java/com/linecorp/armeria/xds/StreamSwitchMapEagerTest.java
- xds/src/main/java/com/linecorp/armeria/xds/DeltaDiscoveryStub.java
- dependencies.toml
- it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioState.java
- licenses/web-licenses.txt
- xds/src/main/java/com/linecorp/armeria/xds/filter/XdsHttpFilter.java
- xds/src/test/java/com/linecorp/armeria/xds/DeltaXdsStreamTest.java
🚧 Files skipped from review as they are similar to previous changes (32)
- core/src/main/java/com/linecorp/armeria/internal/common/util/SslContextUtil.java
- xds/src/main/java/com/linecorp/armeria/xds/RouteResourceParser.java
- core/src/main/java/com/linecorp/armeria/client/ClientTlsSpec.java
- settings.gradle
- it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/DockerAvailableCondition.java
- it/xds-istio/src/test/java/com/linecorp/armeria/it/istio/testing/IstioStartupTest.java
- xds/src/main/java/com/linecorp/armeria/xds/DefaultSubscriptionContext.java
- xds/src/main/java/com/linecorp/armeria/xds/XdsResourceException.java
- xds/src/main/java/com/linecorp/armeria/xds/ListenerStream.java
- xds/src/main/java/com/linecorp/armeria/xds/ListenerResourceParser.java
- xds/src/main/java/com/linecorp/armeria/xds/SnapshotStream.java
- xds/src/main/java/com/linecorp/armeria/xds/ConfigSourceLifecycleObserver.java
- it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioEnv.java
- it/xds-client/src/test/java/com/linecorp/armeria/xds/it/DeltaXdsPreprocessorTest.java
- xds/src/main/java/com/linecorp/armeria/xds/VirtualHostXdsResource.java
- xds/src/main/java/com/linecorp/armeria/xds/ListenerSnapshot.java
- xds/src/main/java/com/linecorp/armeria/xds/CompositeXdsStream.java
- it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioPodCustomizer.java
- xds/src/main/java/com/linecorp/armeria/xds/CertificateValidationContextSnapshot.java
- xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/XdsEndpointUtil.java
- xds/src/main/java/com/linecorp/armeria/xds/ResourceNodeAdapter.java
- xds/src/test/java/com/linecorp/armeria/xds/StateCoordinatorTest.java
- it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioTestExtension.java
- xds/src/main/java/com/linecorp/armeria/xds/SotwActualStream.java
- it/xds-istio/src/test/java/com/linecorp/armeria/it/xds/filter/IstioNoOpFilterFactories.java
- xds/src/test/java/com/linecorp/armeria/xds/SotwXdsStreamTest.java
- xds/src/main/java/com/linecorp/armeria/xds/DefaultConfigSourceLifecycleObserver.java
- xds/src/main/java/com/linecorp/armeria/xds/SwitchMapEagerStream.java
- it/xds-istio/src/test/java/com/linecorp/armeria/it/istio/testing/IstioImageBuildTest.java
- it/xds-client/src/test/java/com/linecorp/armeria/xds/it/XdsControlPlaneMatrixTest.java
- it/xds-istio/build.gradle
- xds/src/main/java/com/linecorp/armeria/xds/RouteXdsResource.java
Comment on lines
+260
to
+269
| // Step 4: Delete cluster — both CLUSTER and ENDPOINT deletions fire, | ||
| // so errors gets at least one throwable. | ||
| cache.setSnapshot(GROUP, Snapshot.create( | ||
| ImmutableList.of(), | ||
| ImmutableList.of(), | ||
| ImmutableList.of(), | ||
| ImmutableList.of(), | ||
| ImmutableList.of(), | ||
| "4")); | ||
| await().untilAsserted(() -> assertThat(errors).isNotEmpty()); |
Contributor
There was a problem hiding this comment.
Assert the new deletion error, not any accumulated error.
errors is never cleared, so assertThat(errors).isNotEmpty() can pass because of an earlier unrelated failure. Capture the pre-delete size and wait for a new MissingXdsResourceException for cluster1 (CLUSTER or ENDPOINT) after the snapshot removal.
Proposed tightening
// Step 4: Delete cluster — both CLUSTER and ENDPOINT deletions fire,
- // so errors gets at least one throwable.
+ // so wait for a new missing-resource error for this cluster.
+ final int errorsBeforeRemoval = errors.size();
cache.setSnapshot(GROUP, Snapshot.create(
ImmutableList.of(),
ImmutableList.of(),
ImmutableList.of(),
ImmutableList.of(),
ImmutableList.of(),
"4"));
- await().untilAsserted(() -> assertThat(errors).isNotEmpty());
+ await().untilAsserted(() -> assertThat(
+ errors.stream().skip(errorsBeforeRemoval).anyMatch(error -> {
+ if (!(error instanceof MissingXdsResourceException)) {
+ return false;
+ }
+ final MissingXdsResourceException exception =
+ (MissingXdsResourceException) error;
+ return (exception.type() == XdsType.CLUSTER ||
+ exception.type() == XdsType.ENDPOINT) &&
+ exception.name().equals(CLUSTER_NAME);
+ })).isTrue());🤖 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/DeltaXdsResourceWatcherTest.java`
around lines 260 - 269, Before deleting the snapshot, record the current
errors.size(), then call cache.setSnapshot(GROUP, Snapshot.create(..., "4")) and
replace the broad assertThat(errors).isNotEmpty() with an await().untilAsserted
that waits for errors.size() to increase by one and contains a new
MissingXdsResourceException for resource "cluster1" (originating from CLUSTER or
ENDPOINT handlers); use the existing errors collection and assert the newest
throwable is an instance of MissingXdsResourceException and references
"cluster1" to ensure the failure is due to the deletion triggered by
cache.setSnapshot rather than an earlier error.
Comment on lines
+31
to
+33
| * <li>Reuses an existing Kind container if one is running and kubeconfig is available</li> | ||
| * <li>Otherwise, creates a new Kind container</li> | ||
| * <li>Istio is always installed on the cluster</li> |
Contributor
There was a problem hiding this comment.
Fix stale Javadoc terminology (Kind vs K3s).
Lines 31-33 mention Kind containers, but this extension path is K3s-based. Updating this text will prevent confusion for test authors.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioClusterExtension.java`
around lines 31 - 33, Update the stale Javadoc in IstioClusterExtension.java to
refer to K3s instead of Kind: edit the class-level or method Javadoc lines that
currently say "Kind container" or "Kind" (the description items mentioning reuse
or creation of a Kind container) so they correctly state "K3s container" or "K3s
cluster/container" and keep the rest of the text (e.g., that kubeconfig is used,
or Istio is always installed) unchanged.
Comment on lines
+80
to
+86
| private static void enableNamespaceInjection(KubernetesClient client, String namespaceName) { | ||
| client.namespaces().withName(namespaceName).edit(ns -> { | ||
| ns.getMetadata().getLabels().put("istio-injection", "enabled"); | ||
| return ns; | ||
| }); | ||
| logger.info("Labeled namespace '{}' with istio-injection=enabled", namespaceName); | ||
| } |
Contributor
There was a problem hiding this comment.
Potential NPE if namespace has no labels.
ns.getMetadata().getLabels() can return null if the namespace has no labels set. Calling put() on a null map will throw a NullPointerException.
🐛 Proposed fix
private static void enableNamespaceInjection(KubernetesClient client, String namespaceName) {
client.namespaces().withName(namespaceName).edit(ns -> {
- ns.getMetadata().getLabels().put("istio-injection", "enabled");
+ java.util.Map<String, String> labels = ns.getMetadata().getLabels();
+ if (labels == null) {
+ labels = new java.util.HashMap<>();
+ ns.getMetadata().setLabels(labels);
+ }
+ labels.put("istio-injection", "enabled");
return ns;
});
logger.info("Labeled namespace '{}' with istio-injection=enabled", namespaceName);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private static void enableNamespaceInjection(KubernetesClient client, String namespaceName) { | |
| client.namespaces().withName(namespaceName).edit(ns -> { | |
| ns.getMetadata().getLabels().put("istio-injection", "enabled"); | |
| return ns; | |
| }); | |
| logger.info("Labeled namespace '{}' with istio-injection=enabled", namespaceName); | |
| } | |
| private static void enableNamespaceInjection(KubernetesClient client, String namespaceName) { | |
| client.namespaces().withName(namespaceName).edit(ns -> { | |
| java.util.Map<String, String> labels = ns.getMetadata().getLabels(); | |
| if (labels == null) { | |
| labels = new java.util.HashMap<>(); | |
| ns.getMetadata().setLabels(labels); | |
| } | |
| labels.put("istio-injection", "enabled"); | |
| return ns; | |
| }); | |
| logger.info("Labeled namespace '{}' with istio-injection=enabled", namespaceName); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioInstaller.java`
around lines 80 - 86, In enableNamespaceInjection(KubernetesClient client,
String namespaceName) protect against null metadata/labels: fetch
ns.getMetadata(), if it is null create a new ObjectMeta and set it on ns, then
getLabels() and if labels is null create a new HashMap, set it back via
ns.getMetadata().setLabels(...), then put("istio-injection","enabled"); keep the
logger.info call. This ensures no NPE when editing the namespace labels.
Comment on lines
+61
to
+65
| final Process saveProcess = new ProcessBuilder("docker", "save", IMAGE_NAME) | ||
| .redirectOutput(tempFile.toFile()) | ||
| .start(); | ||
| if (saveProcess.waitFor() != 0) { | ||
| throw new IllegalStateException("docker save exited with non-zero status"); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP --type=java -C2 'new ProcessBuilder|waitFor\(\)'Repository: line/armeria
Length of output: 2566
🏁 Script executed:
cd it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing && \
head -n 80 IstioTestImage.javaRepository: line/armeria
Length of output: 3743
Add timeout and stderr handling for docker save.
Line 64 uses waitFor() without timeout, and lines 61–63 do not consume stderr. Without a timeout, the docker save process can hang indefinitely. Additionally, if stderr is not drained, a full buffer can block process completion. The sibling class IstioInstaller.java in the same module demonstrates the correct pattern with redirectErrorStream(true).
Suggested change
- final Process saveProcess = new ProcessBuilder("docker", "save", IMAGE_NAME)
+ final Process saveProcess = new ProcessBuilder("docker", "save", IMAGE_NAME)
+ .redirectErrorStream(true)
.redirectOutput(tempFile.toFile())
.start();
- if (saveProcess.waitFor() != 0) {
+ if (!saveProcess.waitFor(2, java.util.concurrent.TimeUnit.MINUTES)) {
+ saveProcess.destroyForcibly();
+ throw new IllegalStateException("docker save timed out");
+ }
+ if (saveProcess.exitValue() != 0) {
throw new IllegalStateException("docker save exited with non-zero status");
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/IstioTestImage.java`
around lines 61 - 65, The docker save invocation in IstioTestImage.java (the
ProcessBuilder that constructs saveProcess for IMAGE_NAME and writes to
tempFile) currently calls saveProcess.waitFor() with no timeout and doesn't
handle stderr; change the ProcessBuilder to redirectErrorStream(true) (or
otherwise consume the error stream) and replace the blocking waitFor() with a
timed wait e.g. saveProcess.waitFor(<reasonableTimeout>, TimeUnit.SECONDS); if
the timed wait returns false, destroy the process
(saveProcess.destroyForcibly()), wait briefly, then throw an
IllegalStateException indicating timeout; also check the exit value after a
successful timed wait and throw with stderr/exit details if non-zero.
Comment on lines
+59
to
+68
| static K3sContainer startK3sAndWaitReady(Duration timeout, Duration pollInterval) throws Exception { | ||
| final K3sContainer k3s = startK3s(); | ||
| try (KubernetesClient client = createClient(k3s.getKubeConfigYaml())) { | ||
| if (!waitForReadyNode(client, timeout, pollInterval)) { | ||
| k3sLogger.warn("Failed to start K3s cluster within timeout: {}", k3s.getLogs()); | ||
| throw new IllegalStateException("Timed out waiting for K3s cluster to be Ready."); | ||
| } | ||
| } | ||
| IstioTestImage.loadIntoK3s(k3s); | ||
| return k3s; |
Contributor
There was a problem hiding this comment.
Close the K3s container on startup/import failures.
If readiness check fails (Line 62) or image loading fails (Line 67), the started container is left running. This leaks resources and can destabilize subsequent tests.
Suggested change
static K3sContainer startK3sAndWaitReady(Duration timeout, Duration pollInterval) throws Exception {
final K3sContainer k3s = startK3s();
+ boolean success = false;
try (KubernetesClient client = createClient(k3s.getKubeConfigYaml())) {
if (!waitForReadyNode(client, timeout, pollInterval)) {
k3sLogger.warn("Failed to start K3s cluster within timeout: {}", k3s.getLogs());
throw new IllegalStateException("Timed out waiting for K3s cluster to be Ready.");
}
+ IstioTestImage.loadIntoK3s(k3s);
+ success = true;
+ return k3s;
+ } finally {
+ if (!success) {
+ k3s.close();
+ }
}
- IstioTestImage.loadIntoK3s(k3s);
- return k3s;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@it/xds-istio/src/main/java/com/linecorp/armeria/it/istio/testing/K8sClusterHelper.java`
around lines 59 - 68, The method startK3sAndWaitReady starts a K3sContainer
(k3s) but never closes it on failure paths; ensure the container is closed when
readiness check or image loading fails by wrapping the startup sequence in a
try/catch that on any Exception/Throwable calls k3s.close() (or k3s.stop()/close
equivalent) and rethrows the original error; specifically, keep the existing
calls to startK3s(), waitForReadyNode(...), and IstioTestImage.loadIntoK3s(k3s)
but add error handling around them so that if waitForReadyNode returns false or
loadIntoK3s throws, you first close k3s (using the K3sContainer instance) and
then throw the IllegalStateException or propagate the original exception.
Comment on lines
+138
to
+151
| @Nullable | ||
| private static HttpConnectionManager extractHcmFromFilterChains(Listener listener) { | ||
| for (FilterChain fc : listener.getFilterChainsList()) { | ||
| for (Filter filter : fc.getFiltersList()) { | ||
| if (HTTP_CONNECTION_MANAGER_FILTER_NAME.equals(filter.getName()) && | ||
| filter.hasTypedConfig() && | ||
| HTTP_CONNECTION_MANAGER_TYPE_URL.equals( | ||
| filter.getTypedConfig().getTypeUrl())) { | ||
| return XdsValidatorIndexRegistry.unpack( | ||
| filter.getTypedConfig(), HttpConnectionManager.class); | ||
| } | ||
| } | ||
| } | ||
| return null; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Envoy v3 Listeners, when multiple filter_chains contain envoy.filters.network.http_connection_manager, what determines which HttpConnectionManager applies to a connection, and is choosing the first matching filter chain semantically correct?
💡 Result:
For any downstream connection, Envoy first selects exactly one listener filter chain, and the envoy.filters.network.http_connection_manager inside that selected chain is the one that will run for the lifetime of the connection (filter-chain selection is done once per connection). [2]
What determines the selected filter chain (and thus which HCM applies):
- If you’re using the classic
filter_chain_matchfields, Envoy prunes candidate chains by match criteria in a defined precedence order (destination port → destination IP → SNI/server name → transport protocol → ALPN/application protocols → directly connected source IP → source type → source IP → source port), and at each step prefers the most specific match; Envoy’s model is that after all steps, at most 1 filter chain remains. [1] - If you’re using
filter_chain_matcher(the newer matching API), the matcher tree evaluates connection inputs and returns the name of the filter chain to use; if no chain matches, Envoy uses the configured default filter chain (if any) or rejects the connection. [2]
Is “choose the first matching filter chain” semantically correct?
No. Envoy’s semantics are most-specific match (per the ordered pruning algorithm / matcher), not “first match in the list.” Also, configurations that create ambiguity (e.g., effectively duplicate match rules) are rejected rather than relying on list order to break ties. [1] [3]
Sources: Envoy FilterChainMatch selection rules and “most specific” guarantee [1]; Envoy listener filter_chain_matcher behavior and “once per connection” [2]; example of Envoy rejecting duplicate/ambiguous filter-chain matches [3].
🏁 Script executed:
# Find test files related to ListenerXdsResource
find . -type f -name "*ListenerXdsResource*Test*.java" -o -name "*Test*ListenerXdsResource*.java"Repository: line/armeria
Length of output: 38
🏁 Script executed:
# Search for uses of extractHcmFromFilterChains to understand context
rg "extractHcmFromFilterChains" -A 5 -B 5Repository: line/armeria
Length of output: 2462
🏁 Script executed:
# Look for filter chain handling and comments in the file
rg -A 3 "filter.*chain|FilterChain" xds/src/main/java/com/linecorp/armeria/xds/ListenerXdsResource.javaRepository: line/armeria
Length of output: 819
🏁 Script executed:
# Check for any documentation or inline comments about multi-chain behavior
rg "multiple.*filter.*chain|multi.*chain" xds/src/ --type javaRepository: line/armeria
Length of output: 38
🏁 Script executed:
# Search for ListenerXdsResource tests in all test locations
find . -type d -name "*test*" | head -20Repository: line/armeria
Length of output: 727
🏁 Script executed:
# Search for any test files that might test ListenerXdsResource
rg "ListenerXdsResource" -lRepository: line/armeria
Length of output: 505
🏁 Script executed:
# Look for TODO, FIXME, or comments about limitations
rg "TODO|FIXME|limitation|support" xds/src/main/java/com/linecorp/armeria/xds/ListenerXdsResource.java -iRepository: line/armeria
Length of output: 329
🏁 Script executed:
# Check the broader context of the file - class declaration and overall design
head -100 xds/src/main/java/com/linecorp/armeria/xds/ListenerXdsResource.java | cat -nRepository: line/armeria
Length of output: 4293
🏁 Script executed:
# Get more of the file to understand the full design
sed -n '100,160p' xds/src/main/java/com/linecorp/armeria/xds/ListenerXdsResource.java | cat -nRepository: line/armeria
Length of output: 2373
🏁 Script executed:
# Search for tests in xds module
find xds/src/test -type f -name "*.java" | head -20Repository: line/armeria
Length of output: 1444
🏁 Script executed:
# Search for any tests that create listeners with multiple filter chains
rg "FilterChain|getFilterChainsList" xds/src/test --type java -lRepository: line/armeria
Length of output: 38
🏁 Script executed:
# Check if there's documentation about listener support/limitations
rg "listener|filter.*chain" xds/src/main/java --type java -g "*.md*" -lRepository: line/armeria
Length of output: 38
🏁 Script executed:
# Search for tests that directly test or use ListenerXdsResource
rg "ListenerXdsResource" xds/src/test --type java -A 3Repository: line/armeria
Length of output: 38
🏁 Script executed:
# Look for test files that test the listener resource parsing/creation
rg "class.*Listener.*Test|new Listener|Listener\.Builder" xds/src/test --type java -lRepository: line/armeria
Length of output: 38
🏁 Script executed:
# Check ListenerResourceParser for any relevant logic
cat xds/src/main/java/com/linecorp/armeria/xds/ListenerResourceParser.javaRepository: line/armeria
Length of output: 2051
🏁 Script executed:
# Check if there are any tests that create Listeners with multiple FilterChains
rg "filterChain|FilterChain" xds/src/test --type java -B 2 -A 2 | head -50Repository: line/armeria
Length of output: 38
🏁 Script executed:
# Look for any documentation files in xds module
find xds -type f \( -name "*.md" -o -name "*.adoc" -o -name "README*" \)Repository: line/armeria
Length of output: 77
🏁 Script executed:
# Check the XDS developer documentation
cat xds/docs/DEVELOPER.mdRepository: line/armeria
Length of output: 10082
🏁 Script executed:
# Search for any comments or documentation about filter chains in the codebase
rg "filter.*chain|multiple.*listener" xds/src/main --type java -i -B 2 -A 2Repository: line/armeria
Length of output: 2403
🏁 Script executed:
# Check when this limitation was introduced or if it's documented elsewhere
rg "filter.*not.*implement|Filter.*ignore" xds/src --type java -i -B 2 -A 2Repository: line/armeria
Length of output: 810
🏁 Script executed:
# Check if there are any open issues or comments about listener/filter limitations
rg "TODO|FIXME|XXX" xds/src/main/java/com/linecorp/armeria/xds --type java -B 1 -A 1Repository: line/armeria
Length of output: 38
Filter chain selection should be explicit or validated.
extractHcmFromFilterChains() returns the first envoy.filters.network.http_connection_manager found across all filter chains and silently discards the rest. This diverges from Envoy's semantics, where filter chain selection per connection is determined by match criteria (destination IP/port, SNI, ALPN, etc.), not iteration order.
Given the DEVELOPER.md states that filters are not yet fully implemented, either:
- Add an explicit check to reject listeners with multiple filter chains and document this limitation clearly, or
- Document that only the first filter chain's HCM will be used and clarify under what conditions this is acceptable.
Silently collapsing multi-chain listeners to a single HCM without validation or clear documentation risks unexpected behavior.
🤖 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 - 151, extractHcmFromFilterChains currently returns the first HCM
found and ignores others; update it to explicitly validate and reject
multi-chain or multi-HCM listeners: iterate listener.getFilterChainsList(),
count filter chains and count matches where filter.getName() equals
HTTP_CONNECTION_MANAGER_FILTER_NAME and typedConfig type URL equals
HTTP_CONNECTION_MANAGER_TYPE_URL (the place you call
XdsValidatorIndexRegistry.unpack for HttpConnectionManager), and if
filterChains.size() > 1 or matchedHcmCount > 1, fail fast by logging an error
and returning null or throwing an IllegalArgumentException (choose consistent
error behavior used elsewhere), otherwise unpack and return the single
HttpConnectionManager as before.
Comment on lines
+85
to
+91
| final RetryPolicy retryPolicy = route.getRoute().getRetryPolicy(); | ||
| final RetryPolicy effectiveRetryPolicy = | ||
| retryPolicy == RetryPolicy.getDefaultInstance() ? null : retryPolicy; | ||
| final ClientDecoration clientDecoration = FilterUtil.buildUpstreamFilter( | ||
| upstreamFilters, filterConfigs, effectiveRetryPolicy); | ||
| httpClient = clientDecoration.decorate(DelegatingHttpClient.of()); | ||
| rpcClient = clientDecoration.rpcDecorate(DelegatingRpcClient.of()); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Protocol Buffers Java, when comparing a message to getDefaultInstance(), should you use == or equals()?
💡 Result:
Use equals(), not ==, unless you specifically want an identity check.
getDefaultInstance()returns a singleton “all fields unset/empty” instance.[1]Message.equals()is defined to compare field values (same type + identical values for all fields).[2]==only checks whether both references point to the exact same object, so it may befalseeven if the message is “default/empty”.
So, for “is this message default/empty?” do:
msg.equals(Foo.getDefaultInstance())Sources: [1] [2]
🏁 Script executed:
# First, let's check the RouteEntry.java file to understand the context
head -100 xds/src/main/java/com/linecorp/armeria/xds/RouteEntry.java | tail -30Repository: line/armeria
Length of output: 1326
🏁 Script executed:
# Check if RetryPolicy is imported and from which package
rg "import.*RetryPolicy" xds/src/main/java/com/linecorp/armeria/xds/RouteEntry.javaRepository: line/armeria
Length of output: 111
🏁 Script executed:
# Search for other instances of == comparison with getDefaultInstance() in the codebase
rg "==.*getDefaultInstance\(\)" --type javaRepository: line/armeria
Length of output: 951
🏁 Script executed:
# Check if there are other instances in the same file
rg "getDefaultInstance" xds/src/main/java/com/linecorp/armeria/xds/RouteEntry.javaRepository: line/armeria
Length of output: 141
Use .equals() instead of == for protobuf message comparison.
RetryPolicy is a protobuf message, and getDefaultInstance() returns a singleton. Using == checks object identity rather than equality, which fails if a newly parsed empty message is compared against the cached default instance. Use .equals() instead:
Suggested fix
// Determine retry policy
final RetryPolicy retryPolicy = route.getRoute().getRetryPolicy();
final RetryPolicy effectiveRetryPolicy =
- retryPolicy == RetryPolicy.getDefaultInstance() ? null : retryPolicy;
+ retryPolicy.equals(RetryPolicy.getDefaultInstance()) ? null : retryPolicy;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| final RetryPolicy retryPolicy = route.getRoute().getRetryPolicy(); | |
| final RetryPolicy effectiveRetryPolicy = | |
| retryPolicy == RetryPolicy.getDefaultInstance() ? null : retryPolicy; | |
| final ClientDecoration clientDecoration = FilterUtil.buildUpstreamFilter( | |
| upstreamFilters, filterConfigs, effectiveRetryPolicy); | |
| httpClient = clientDecoration.decorate(DelegatingHttpClient.of()); | |
| rpcClient = clientDecoration.rpcDecorate(DelegatingRpcClient.of()); | |
| final RetryPolicy retryPolicy = route.getRoute().getRetryPolicy(); | |
| final RetryPolicy effectiveRetryPolicy = | |
| retryPolicy.equals(RetryPolicy.getDefaultInstance()) ? null : retryPolicy; | |
| final ClientDecoration clientDecoration = FilterUtil.buildUpstreamFilter( | |
| upstreamFilters, filterConfigs, effectiveRetryPolicy); | |
| httpClient = clientDecoration.decorate(DelegatingHttpClient.of()); | |
| rpcClient = clientDecoration.rpcDecorate(DelegatingRpcClient.of()); |
🤖 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/RouteEntry.java` around lines 85 -
91, The comparison retryPolicy == RetryPolicy.getDefaultInstance() is using
reference equality on a protobuf message; change it to use .equals(), i.e. call
RetryPolicy.getDefaultInstance().equals(retryPolicy) (or
retryPolicy.equals(...)) when computing effectiveRetryPolicy for the value
produced by route.getRoute().getRetryPolicy(), so effectiveRetryPolicy is null
only when the message is equal to the default instance before passing it into
FilterUtil.buildUpstreamFilter and creating
clientDecoration/httpClient/rpcClient.
Comment on lines
+47
to
+50
| <T extends XdsResource> boolean register(XdsType type, String resourceName, ResourceWatcher<T> watcher) { | ||
| final boolean updated = subscriberStorage.register(type, resourceName, watcher); | ||
| replayToWatcher(type, resourceName, watcher); | ||
| return updated; |
Contributor
There was a problem hiding this comment.
Shield replay callbacks from watcher exceptions.
replayToWatcher() invokes user callbacks directly, unlike XdsStreamSubscriber, which catches and logs watcher failures during live updates. A throwing watcher here will break registration after the watcher has already been added, leaving the caller with a partial registration state. Apply the same exception isolation on the replay path.
Also applies to: 124-141
🤖 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/StateCoordinator.java` around
lines 47 - 50, The replayToWatcher(...) call in register(...) can throw and
currently propagates after subscriberStorage.register(...) has already
succeeded; wrap the replayToWatcher(type, resourceName, watcher) invocation in a
try-catch that catches Throwable, logs the error (consistent with how
XdsStreamSubscriber handles watcher failures), and suppresses the exception so
the method still returns the boolean from subscriberStorage.register(...)
unchanged; apply the same try-catch/log-and-suppress pattern to the other
replayToWatcher(...) invocation(s) in the file (the block around the 124-141
region) so watcher exceptions never leave the replay path.
Comment on lines
+68
to
+70
| await().atMost(1, TimeUnit.SECONDS) | ||
| .untilAsserted(() -> assertThat(watcher.missingType).isEqualTo(XdsType.ROUTE)); | ||
| assertThat(watcher.missingName).isEqualTo(ROUTE_NAME); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the test file
git ls-files | grep SubscriberStorageTest.javaRepository: line/armeria
Length of output: 125
🏁 Script executed:
# Check the test file content around the relevant lines
cat -n xds/src/test/java/com/linecorp/armeria/xds/SubscriberStorageTest.java | sed -n '1,100p'Repository: line/armeria
Length of output: 4035
🏁 Script executed:
# Check if EventLoopExtension is used and how it's configured
rg "EventLoopExtension" xds/src/test/java/com/linecorp/armeria/xds/SubscriberStorageTest.java -B 2 -A 2Repository: line/armeria
Length of output: 321
🏁 Script executed:
# Look for how CapturingWatcher is being used and registered
rg "CapturingWatcher\|onResourceDoesNotExist" xds/src/test/java/com/linecorp/armeria/xds/SubscriberStorageTest.java -B 3 -A 3Repository: line/armeria
Length of output: 38
🏁 Script executed:
# Check SubscriberStorage to see if it invokes callbacks on the event loop
fd -t f "SubscriberStorage.java" xds/src/main/javaRepository: line/armeria
Length of output: 121
🏁 Script executed:
# Examine the SubscriberStorage implementation to see callback invocation
cat -n xds/src/main/java/com/linecorp/armeria/xds/SubscriberStorage.java | head -150Repository: line/armeria
Length of output: 5132
🏁 Script executed:
# Find and examine XdsStreamSubscriber to see where callbacks are invoked
fd -t f "XdsStreamSubscriber.java" xds/src/main/javaRepository: line/armeria
Length of output: 123
🏁 Script executed:
# Check the XdsStreamSubscriber implementation, especially callback invocations
cat -n xds/src/main/java/com/linecorp/armeria/xds/XdsStreamSubscriber.java | head -200Repository: line/armeria
Length of output: 5128
🏁 Script executed:
# Verify that the scheduled task in XdsStreamSubscriber runs on the event loop
# by checking the eventLoop.schedule usage context
rg "eventLoop.schedule" xds/src/main/java/com/linecorp/armeria/xds/XdsStreamSubscriber.java -B 5 -A 5Repository: line/armeria
Length of output: 328
🏁 Script executed:
# Check if there are other similar patterns in the XDS test suite where volatile is used
rg "volatile" xds/src/test/java/ -t javaRepository: line/armeria
Length of output: 38
🏁 Script executed:
# Check if there are other patterns in the XDS codebase for thread-safe field access
rg "volatile" xds/src/main/java/com/linecorp/armeria/xds/ -t javaRepository: line/armeria
Length of output: 1753
Move the second assertion inside the untilAsserted block to ensure thread-safe visibility.
The onResourceDoesNotExist() callback runs on the event loop, while Awaitility polls from the test thread. The second assertion on line 70 reads missingName without synchronization guarantees, risking visibility issues or intermittent failures.
🧪 Suggested fix
await().atMost(1, TimeUnit.SECONDS)
.untilAsserted(() -> {
assertThat(watcher.missingType).isEqualTo(XdsType.ROUTE);
+ assertThat(watcher.missingName).isEqualTo(ROUTE_NAME);
});
- assertThat(watcher.missingName).isEqualTo(ROUTE_NAME);Also applies to: 73-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@xds/src/test/java/com/linecorp/armeria/xds/SubscriberStorageTest.java` around
lines 68 - 70, The second assertion reading watcher.missingName is not
guaranteed to be visible to the test thread because onResourceDoesNotExist()
runs on the event loop; move the
assertThat(watcher.missingName).isEqualTo(ROUTE_NAME) into the Awaitility
untilAsserted lambda so both assertions
(assertThat(watcher.missingType).isEqualTo(XdsType.ROUTE) and the missingName
check) execute under the same synchronized wait; apply the same change for the
other similar blocks that assert watcher.missingType/missingName so all
visibility-sensitive assertions are inside untilAsserted.
This was referenced Apr 6, 2026
Merged
jrhee17
added a commit
that referenced
this pull request
Apr 9, 2026
This PR is a subset of #6700 Motivation The xDS resource stream layer used a lazy `switchMap` operator when mapping an upstream resource to a downstream snapshot stream (e.g., a `ListenerXdsResource` → `ListenerSnapshot`). On every upstream change, the lazy implementation would unsubscribe from the old inner stream *before* creating and subscribing to the new one. This caused the entire downstream `ResourceNode` chain — including SDS secret subscriptions — to be torn down and rebuilt on every xDS update. This is inconsistent with [Envoy's behavior](https://github.com/envoyproxy/envoy), which maintains existing resource watchers across xDS updates and only replaces them when a genuinely different resource is received. A secondary consequence was that `ResourceNodeMeterBinder` — which records revision, error, and missing-resource metrics per resource node — was allocated and deregistered per `ResourceNodeAdapter` instance. With the old lazy teardown, gauges and counters reset to zero on every xDS update, producing noisy metric churn. Modifications - Replace `SwitchMapStream` (lazy) with `SwitchMapEagerStream` (eager): the new operator subscribes to the new inner stream *before* closing the old one, so downstream resource subscriptions survive across upstream changes. An epoch counter discards events from superseded inner streams. `SwitchMapStream` is deleted. - `SnapshotStream`: `switchMap()` → `switchMapEager()`, backed by `SwitchMapEagerStream` - Call sites updated: `ClusterStream`, `TransportSocketStream`, `ListenerStream`, `RouteStream` - Replace `ResourceNodeMeterBinder` with `ResourceNodeMeterBinderFactory`: the factory maintains a reference-counted map of binder instances keyed by `(XdsType, resourceName)`. `acquire()` returns the shared binder (incrementing the ref count); `close()` only deregisters meters when the last reference drops to zero, preventing metric resets during the brief overlap when two `ResourceNodeAdapter` instances exist for the same resource during an eager switch. - `ResourceNodeMeterBinder` deleted (moved as inner class of `ResourceNodeMeterBinderFactory`) - `ResourceNodeMeterBinderFactory` added; exposed via `SubscriptionContext.meterBinderFactory()` - `DefaultSubscriptionContext`: constructs and holds the factory - `ResourceNodeAdapter`: calls `context.meterBinderFactory().acquire()` on construction, `meterBinder.close()` on unsubscription - `SubscriberStorage#ResourceCache` is cleaned up ## Result - xDS resource updates no longer cause unnecessary teardown and recreation of downstream resource subscriptions, consistent with Envoy's resource-cache behavior. - Resource node metrics (`resource.node.revision`, `resource.node.error`, `resource.node.missing`) remain continuous across xDS updates and only deregister when all watchers for a given resource name have closed. - Preparation for Delta stream is done --------- Co-authored-by: Ikhun Um <[email protected]>
jrhee17
added a commit
that referenced
this pull request
Apr 9, 2026
Subset of #6700 Motivation `HttpFilterFactory<T extends Message>` forced each factory implementation to declare a specific protobuf `Message` type for its config, and required the framework to own config parsing and `FilterConfig` envelope unwrapping. This made it difficult to handle unknown or dynamically-typed xDS filter configs (e.g. Istio-specific filters), and leaked framework concerns into the factory API. Additionally, `ParsedFilterConfig` wrapped per-route filter configs and required the framework to merge and propagate them through the snapshot hierarchy (`RouteSnapshot` → `VirtualHostSnapshot` → `RouteEntry`), coupling snapshot construction to filter logic. Modifications - Replace `HttpFilterFactory<T extends Message>` with an untyped `HttpFilterFactory` interface; `create(HttpFilter, Any)` now receives the raw `Any` config directly, and factories are responsible for all parsing including `FilterConfig` envelope unwrapping. Returning `null` skips the filter. - Introduce `XdsHttpFilter` as the return type of `create()`, with default no-op `httpPreprocessor()`, `rpcPreprocessor()`, `httpDecorator()`, and `rpcDecorator()` methods. - Delete `ParsedFilterConfig`; per-route filter configs are now stored and merged as `Map<String, Any>` directly. - Move 3-level `typed_per_filter_config` merging (route-config → vhost → route) into `RouteEntry`'s constructor, eliminating `withFilterConfigs()` from `VirtualHostSnapshot` and `withRouter()` from `RouteSnapshot`. - Thread `@Nullable ListenerXdsResource` through `RouteStream` inner classes so `RouteEntry` can access upstream HTTP filters from the Router directly. - Update `RouterFilterFactory` to implement the new untyped interface. - Simplify `FilterUtil`: remove `toParsedFilterConfigs()`, update `mergeFilterConfigs` to operate on `Map<String, Any>`, `buildUpstreamFilter` takes `@Nullable RetryPolicy` directly. Result - `HttpFilterFactory` implementations no longer need to declare a protobuf type parameter, and can handle any xDS filter config including unknown or Istio-specific ones. - Per-route filter config merging is self-contained in `RouteEntry` rather than spread across the snapshot hierarchy. - `RouteEntry.filterConfig(String)` now returns `@Nullable Any` instead of `@Nullable ParsedFilterConfig`. Breaking - `HttpFilterFactory` implementations must now implement `XdsHttpFilter create(HttpFilter httpFilter, Any config)` for custom filter implementations
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation:
This PR is a collective set of changes which targets introduction of https://github.com/jrhee17/armeria/blob/dd70a0aade88aaefc0d83a0d3241890e7022b9ef/it/xds-istio/src/test/java/com/linecorp/armeria/it/xds/XdsClientToSidecarTest.java
Modifications:
Result: