[POC] Add validation for supported fields, Support customization of config_source#6721
Draft
[POC] Add validation for supported fields, Support customization of config_source#6721
config_source#6721Conversation
Contributor
📝 WalkthroughWalkthroughIntroduces delta xDS support and a pluggable extension/validation framework: new delta/Sotw stream implementations, centralized extension registry, subscription/stream factory abstractions, resource state coordination, transport-socket factory model, broad proto-supported-field annotations, and accompanying tests and SPI registrations. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Armeria Client
participant Registry as XdsExtensionRegistry
participant Factory as GrpcConfigSourceStreamFactory
participant AdsStream as AdsXdsStream
participant DeltaActual as DeltaActualStream
participant ControlPlane as Envoy Control Plane
Client->>Registry: XdsExtensionRegistry.of(...)
Client->>Factory: create(ConfigSource)
Factory->>AdsStream: construct (ActualStreamFactory + StateCoordinator)
AdsStream->>DeltaActual: ensureStream() / create ActualStream
DeltaActual->>ControlPlane: stream(responseObserver)
ControlPlane->>DeltaActual: DeltaDiscoveryResponse
DeltaActual->>Registry: parser = query(typeUrl)
DeltaActual->>DeltaActual: parseDeltaResources()
DeltaActual->>StateCoordinator: onResourceUpdated/onResourceMissing/onResourceError
DeltaActual->>AdsStream: ackResponse()/nackResponse()
AdsStream->>AdsStream: retryOrClose() (backoff scheduling)
sequenceDiagram
participant App as Application
participant Registry as XdsExtensionRegistry
participant TlsFactory as UpstreamTlsTransportSocketFactory
participant Stream as TransportSocketStream
participant Watcher as SnapshotWatcher
App->>Registry: query(transportSocket.typeUrl | name, TransportSocketFactory)
Registry-->>TlsFactory: resolve factory
App->>Stream: create(transportSocket, watcher)
Stream->>TlsFactory: factory.create(context, configSource, transportSocket)
TlsFactory->>Stream: SnapshotStream<TransportSocketSnapshot>
Stream->>Watcher: subscribe(watcher)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
jrhee17
force-pushed
the
feat/xds-support-field
branch
from
89c2909 to
37b1b1c
Compare
![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: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
it/xds-client/src/test/java/com/linecorp/armeria/xds/it/ConfigSourceLifecycleObserverTest.java (1)
531-578:⚠️ Potential issue | 🟡 MinorStatic
versionincrements during test discovery may cause flaky behavior.The
resourceRejection_args()method is annotated with@MethodSourceand is called during JUnit test discovery. It increments the staticversionfield (lines 558, 564, 570, 576) each time it's invoked. Since test discovery can happen multiple times or in different orders, this may lead to non-deterministic version strings in the snapshots.Consider computing version strings inside the test method instead, or using a fresh version counter per test invocation.
🛠️ Suggested approach
static Stream<Arguments> resourceRejection_args() { // Create valid resources ... return Stream.of( - Arguments.of(XdsType.LISTENER, Snapshot.create( - ... - ImmutableList.of(), String.valueOf(version.incrementAndGet()))), + Arguments.of(XdsType.LISTENER, /* pass resource objects, compute snapshot in test */), ... ); }Or pass the malformed resource type and construct the snapshot inside
resourceRejection()using a fresh version increment.🤖 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/ConfigSourceLifecycleObserverTest.java` around lines 531 - 578, The static version field is being incremented inside the MethodSource provider resourceRejection_args(), causing non-deterministic version strings during test discovery; move version increments out of the method source by returning argument tuples that identify the malformed resource type (e.g., enum or marker) rather than prebuilt Snapshot versions, and then inside the test method resourceRejection(...) construct the Snapshot with Snapshot.create(...) using a fresh version string generated there (e.g., version.incrementAndGet() or a local counter) so each test invocation computes its own deterministic version instead of changing a shared static during discovery.
🧹 Nitpick comments (5)
xds/src/main/java/com/linecorp/armeria/xds/ResourceStateStore.java (1)
76-85: Enforcetype/resourceNameinvariants inputVersioned(...).
putVersioned(...)currently trusts caller-provided key/type without validating againstresource. A mismatch can silently corrupt state buckets.As per coding guidelines, keep exception messages meaningful (include offending values + expected constraints).Proposed hardening diff
`@Nullable` XdsResource putVersioned(XdsType type, String resourceName, XdsResource resource) { + if (resource.type() != type) { + throw new IllegalArgumentException( + "Mismatched resource type: expected=" + type + ", actual=" + resource.type()); + } + if (!resourceName.equals(resource.name())) { + throw new IllegalArgumentException( + "Mismatched resource name: key=" + resourceName + ", actual=" + resource.name()); + } final ResourceState prev = state(type, resourceName); if (isDuplicateEntry(resource, prev)) { return null; }🤖 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/ResourceStateStore.java` around lines 76 - 85, putVersioned(...) currently accepts a caller-supplied XdsType and resourceName without verifying they match the provided XdsResource, which can corrupt the per-type state buckets; before calling isDuplicateEntry(...) validate that resource is non-null and that its identifying/type fields (use XdsResource's accessor(s) — e.g. the resource's name/type or, for AbstractXdsResource, the values returned by its getters) equal the supplied resourceName and type, and if not throw an IllegalArgumentException that includes both the offending values and the expected type/resourceName; perform this check at the top of putVersioned (before computing revision and inserting into statesFor(...)) so incorrect calls fail fast and preserve bucket invariants.xds/src/main/java/com/linecorp/armeria/xds/XdsResourceFactory.java (1)
47-65: DerivetypeUrl()fromtype()to remove a drift point.Every implementation here maps
typeUrl()straight from the sameXdsType. Keeping both abstract means one bad override can register a factory under the wrong URL while still reporting the right type.Suggested change
/** * Returns the type URL for this resource factory. Used as both the registry key * and the name. */ - String typeUrl(); + default String typeUrl() { + return type().typeUrl(); + }If a factory ever needs a different URL, it can still override this default.
🤖 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/XdsResourceFactory.java` around lines 47 - 65, The implementation exposes both abstract type() and typeUrl(), which allows inconsistent overrides; change typeUrl() to be a default method that derives its value from type() (i.e., return type().url() or however XdsType exposes its URL) so the registry key/name and reported XdsType cannot drift, and keep the ability for implementations to override typeUrl() if needed; update XdsResourceFactory's typeUrl() default and leave name() and typeUrls() as-is to continue using typeUrl().xds/src/main/java/com/linecorp/armeria/xds/XdsExtensionRegistry.java (1)
94-101: Silent overwriting of factories with duplicate typeUrls.The
register()method puts factories into the maps without checking for duplicates. If an SPI-loaded factory shares a typeUrl with a built-in factory (or another SPI factory), the later registration silently overwrites the earlier one. This could lead to unexpected behavior if multiple factories claim the same typeUrl.Consider either logging a warning when overwriting occurs, or throwing an exception to detect conflicts early during initialization.
🔧 Proposed fix to detect duplicates
private static void register(XdsExtensionFactory factory, Map<String, XdsExtensionFactory> byName, Map<String, XdsExtensionFactory> byTypeUrl) { - byName.put(factory.name(), factory); + final XdsExtensionFactory existingByName = byName.put(factory.name(), factory); + if (existingByName != null) { + logger.warn("Factory for name '{}' overwritten: {} -> {}", + factory.name(), existingByName.getClass(), factory.getClass()); + } for (String typeUrl : factory.typeUrls()) { - byTypeUrl.put(typeUrl, factory); + final XdsExtensionFactory existingByTypeUrl = byTypeUrl.put(typeUrl, factory); + if (existingByTypeUrl != null) { + logger.warn("Factory for typeUrl '{}' overwritten: {} -> {}", + typeUrl, existingByTypeUrl.getClass(), factory.getClass()); + } } }🤖 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/XdsExtensionRegistry.java` around lines 94 - 101, The register(...) helper currently blindly overwrites existing entries in byName and byTypeUrl when an XdsExtensionFactory (from XdsExtensionFactory.name() / typeUrls()) collides; update register to detect duplicates by checking byTypeUrl.containsKey(typeUrl) and byName.containsKey(factory.name()) for each factory, and either log a clear warning (including both factory.name() and the existing factory identity) or throw an IllegalStateException to fail fast on conflicts; apply this check for every factory.typeUrls() and ensure the chosen behavior (warning vs exception) is used consistently so SPI-loaded factories cannot silently replace built-ins.xds-api/src/main/java/com/linecorp/armeria/xds/api/SupportedFieldValidator.java (1)
156-164: Consider using map key in error path for better debuggability.The current implementation uses an index-based path like
fieldPath[0].value, but for maps, the iteration order is not guaranteed to be meaningful. Including the actual map key in the path would make error messages more useful for debugging.♻️ Suggested improvement
if (valueField != null && valueField.getJavaType() == FieldDescriptor.JavaType.MESSAGE && isAnnotatablePackage(valueField.getMessageType().getFile().getPackage())) { - int i = 0; + final FieldDescriptor keyField = fd.getMessageType().findFieldByName("key"); for (Message mapEntry : (List<Message>) value) { final Object mapValue = mapEntry.getField(valueField); if (mapValue instanceof Message) { - doValidate((Message) mapValue, fieldPath + '[' + i + "].value"); + final Object mapKey = keyField != null ? mapEntry.getField(keyField) : null; + final String keyStr = mapKey != null ? String.valueOf(mapKey) : "?"; + doValidate((Message) mapValue, fieldPath + '[' + keyStr + ']'); } - i++; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xds-api/src/main/java/com/linecorp/armeria/xds/api/SupportedFieldValidator.java` around lines 156 - 164, The loop in SupportedFieldValidator (inside doValidate) uses an index for map entries which is unhelpful for maps; change the iteration over Message mapEntry to extract the actual map key via mapEntry.getField(keyField) (or equivalent key field used for map entries), convert it to a string (handling nulls and non-String keys), and use that key in the recursive path call (e.g., fieldPath + '[' + keyString + "].value"); if the key cannot be obtained, fall back to the index to preserve behavior; update references to valueField and mapEntry accordingly and ensure no NPEs when key is missing.xds/src/main/java/com/linecorp/armeria/xds/SubscriptionCallbacks.java (1)
85-98: Consider simplifying the redundant condition check.The condition on lines 87-88 checks both
factory.isFullStateOfTheWorld()AND(type == LISTENER || type == CLUSTER). Since "full state of the world" semantics in xDS are specific to LDS and CDS types, this double-check appears redundant. IfisFullStateOfTheWorld()correctly identifies these types, the explicit type check is unnecessary.♻️ Suggested simplification
// SotW absent detection for full-state types (LDS/CDS) final XdsResourceFactory factory = extensionRegistry.queryByTypeUrl(type.typeUrl(), XdsResourceFactory.class); - if (factory != null && factory.isFullStateOfTheWorld() && - (type == XdsType.LISTENER || type == XdsType.CLUSTER)) { + if (factory != null && factory.isFullStateOfTheWorld()) { final Set<String> currentSubscribers = stateCoordinator.interestedResources(type);🤖 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/SubscriptionCallbacks.java` around lines 85 - 98, Remove the redundant explicit type check and rely on factory.isFullStateOfTheWorld() to determine full-state semantics: replace the condition "factory != null && factory.isFullStateOfTheWorld() && (type == XdsType.LISTENER || type == XdsType.CLUSTER)" with "factory != null && factory.isFullStateOfTheWorld()". Leave the subsequent logic that fetches currentSubscribers via stateCoordinator.interestedResources(type) and iterates to call stateCoordinator.onResourceMissing(type, name) when parsedResources and invalidResources do not contain the name; ensure parsedResources and invalidResources checks remain intact.
🤖 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 261-268: The test currently only checks that errors is non-empty
after calling cache.setSnapshot(GROUP, Snapshot.create(..., "4")), which can
hide unrelated async failures; change the assertion in the await().untilAsserted
block to verify the specific deletion error type or message for cluster/endpoint
deletion rather than assertThat(errors).isNotEmpty(). Locate the
await().untilAsserted usage that wraps assertThat(errors) and replace the
predicate to assert that errors contains an entry matching the expected
exception/class/message (e.g., specific deletion error type or a known error
string) so the test validates the correct deletion behavior for the Snapshot
version "4".
- Around line 147-163: The test only waits for MissingXdsResourceException but
doesn't assert that no new snapshots were added; before calling
cache.setSnapshot(...) capture the current snapshots.size() (e.g.,
sizeBeforeRemoval), then after awaiting the MissingXdsResourceException assert
that snapshots.size() == sizeBeforeRemoval to ensure no new snapshot was
emitted, and optionally verify the last snapshot (or lack thereof) does not
contain a ListenerSnapshot for LISTENER_NAME; refer to
DeltaXdsResourceWatcherTest, cache.setSnapshot(...), errors,
MissingXdsResourceException, XdsType.LISTENER, LISTENER_NAME and snapshots when
making these checks.
In
`@xds-api/src/main/java/com/linecorp/armeria/xds/api/MeterUnsupportedFieldHandler.java`:
- Around line 53-57: In MeterUnsupportedFieldHandler, add an explicit null-check
at the start of the public method handle(String fieldPath) by calling
Objects.requireNonNull(fieldPath, "fieldPath") so fieldPath is validated before
being used to build tags (Counter.builder(...).tag("field", fieldPath)...); this
ensures the public parameter is non-null per coding guidelines and avoids NPEs
when registering the counter with meterRegistry.
In `@xds-api/src/main/java/com/linecorp/armeria/xds/api/PgvValidator.java`:
- Around line 49-52: The public method PgvValidator.assertValid(Object message)
must perform an explicit null check on the incoming parameter before delegating;
add Objects.requireNonNull(message, "message") at the start of assertValid to
enforce the contract and keep the existing try/catch (catching
ValidationException) and delegation to delegate.validatorFor(message). Ensure
the method throws a clear NPE immediately for null input rather than relying on
delegate behavior.
In
`@xds-api/src/main/java/com/linecorp/armeria/xds/api/StrictXdsValidatorIndex.java`:
- Around line 40-46: The public method StrictXdsValidatorIndex.assertValid
currently allows a null message to pass to downstream validators; add an
explicit null-check using Objects.requireNonNull(message, "message") at the
start of assertValid to fail fast and give a stable error to callers before
calling pgvValidator.assertValid(message) and
SupportedFieldValidator.assertValid(message); update imports if needed to
reference java.util.Objects.
In `@xds-api/src/main/proto/envoy/config/cluster/v3/cluster.proto`:
- Around line 226-229: The enum value DEFAULT_SUBSET is annotated as supported
but its companion field google.protobuf.Struct default_subset is missing the
(armeria.xds.supported) annotation causing SupportedFieldValidator to reject
configs; update the default_subset field declaration in cluster.proto (and the
analogous default_subset in
extensions/load_balancing_policies/subset/v3/subset.proto) to include
[(armeria.xds.supported) = true], so the SupportedFieldValidator will treat the
nested field as supported when DEFAULT_SUBSET is used.
In `@xds/src/main/java/com/linecorp/armeria/xds/AdsXdsStream.java`:
- Around line 117-120: resourcesUpdated currently calls
ensureStream().resourcesUpdated(type) off the event loop, which can race with
stop()/retryOrClose() and mutate actualStream concurrently; change
resourcesUpdated(XdsType) to marshal its body onto the existing eventLoop (e.g.,
check eventLoop.inEventLoop and either run directly or schedule via
eventLoop.execute) so that ensureStream() and the subsequent
resourcesUpdated(type) invoke happen only on the eventLoop thread, preserving
the single-threaded invariants around actualStream, stop(), and retryOrClose().
In `@xds/src/main/java/com/linecorp/armeria/xds/ConfigSourceHandler.java`:
- Around line 45-47: The close() method currently calls stream.close() then
stateCoordinator.close(), which can skip closing the coordinator if
stream.close() throws; update ConfigSourceHandler.close() to ensure
stateCoordinator.close() always runs by wrapping stream.close() in a try block
with stateCoordinator.close() in a finally block (i.e., try { stream.close(); }
finally { stateCoordinator.close(); }) so both resources are guaranteed closed
even if ConfigSourceSubscription.close()/stream.close() throws.
In `@xds/src/main/java/com/linecorp/armeria/xds/DeltaActualStream.java`:
- Around line 88-95: The local close path flips the completed flag in
DeltaActualStream.closeStream() but onError() and onCompleted() still proceed to
notify the lifecycle observer and call owner.retryOrClose(...), causing
duplicate/undesired retry/close behavior; modify DeltaActualStream.onError(...)
and DeltaActualStream.onCompleted(...) to first check the completed boolean and
return immediately if true (i.e., ignore terminal callbacks after an intentional
local close), so they do not call lifecycleObserver.* or
owner.retryOrClose(...); apply the same guard to the analogous terminal-callback
handlers in the other block referenced (the second onError/onCompleted
implementations around the later range) to ensure consistent behavior.
- Around line 221-226: When DeltaActualStream finds factory == null for
extensionRegistry.queryByTypeUrl(value.getTypeUrl(), XdsResourceFactory.class),
do not return silently; treat it as a protocol error and NACK the response with
an explicit error message and the incoming nonce. Replace the early return in
DeltaActualStream with code that builds a descriptive error string containing
value.getTypeUrl() and value.getNonce(), logs it (logger.error/ warn as
appropriate), and calls the existing NACK path (e.g., the method that sends a
negative ACK for delta responses in this class or the control-plane sender) so
the ADS control plane receives the nonce + error details instead of leaving the
request unanswered.
In `@xds/src/main/java/com/linecorp/armeria/xds/DeltaDiscoveryStub.java`:
- Around line 41-65: The default branch in DeltaDiscoveryStub.basic(XdsType
type, GrpcClientBuilder builder) throws java.lang.Error for an unexpected enum
value; replace that with throwing an IllegalArgumentException (e.g. throw new
IllegalArgumentException("Unexpected value: " + type)) so callers can handle it
normally. Locate the switch in the basic(...) method and change the default
throw Error(...) to throw new IllegalArgumentException(...) preserving the
message content.
In
`@xds/src/main/java/com/linecorp/armeria/xds/GrpcConfigSourceStreamFactory.java`:
- Around line 97-104: The code currently only validates the first GrpcService
(firstGrpcService) for envoyGrpc but must validate every configured GrpcService:
iterate over apiConfigSource.getGrpcServicesList() (or stream it) and assert
each GrpcService.hasEnvoyGrpc() using the same checkArgument-style message
(include which configSource or index for clarity), failing if any entry is not
envoyGrpc; after that keep using grpcServices.get(0).getEnvoyGrpc() as before
(or assign envoyGrpc from the first element) so behavior is unchanged except
that all entries are validated.
In
`@xds/src/main/java/com/linecorp/armeria/xds/HttpConnectionManagerFactory.java`:
- Around line 29-32: The NAME constant in HttpConnectionManagerFactory is using
the old v2 identifier and should be updated to the canonical Envoy v3 extension
name; locate the static field NAME in class HttpConnectionManagerFactory and
change its value from "envoy.http_connection_manager" to
"envoy.filters.network.http_connection_manager" (leave TYPE_URL unchanged) so
name-based registry lookups (e.g., queryByName()) will succeed and remain
consistent with other factories.
In
`@xds/src/main/java/com/linecorp/armeria/xds/PathSotwConfigSourceSubscriptionFactory.java`:
- Around line 110-112: The code in PathSotwConfigSourceSubscriptionFactory
builds a DiscoveryResponse by parsing JSON from a byte array using new
String(bytes), which relies on the platform default charset; change the
conversion to use an explicit charset (e.g., StandardCharsets.UTF_8) when
calling JsonFormat.parser().merge so the JSON is always decoded as UTF-8, and
add the necessary import for java.nio.charset.StandardCharsets.
In `@xds/src/main/java/com/linecorp/armeria/xds/SotwActualStream.java`:
- Around line 89-96: closeStream() sets completed = true but onError() and
onCompleted() still call retryOrClose(...), causing an intentional local close
to be treated as a remote termination; update onError(...) and onCompleted(...)
in SotwActualStream to first check the completed flag and return immediately if
true (same pattern used in the delta implementation), so that terminal callbacks
are ignored after closeStream() and do not invoke retryOrClose(...); apply the
same guard to the other occurrence of these handlers in this class.
- Around line 145-149: SotwActualStream currently logs and returns when
extensionRegistry.queryByTypeUrl(value.getTypeUrl(), XdsResourceFactory.class)
yields null, which drops the response without ACK/NACK; instead, treat this as a
protocol error by sending a NACK for the received DiscoveryResponse (including
value.getTypeUrl() and a clear error message) back to the management server and
then return. Locate the null-check around XdsResourceFactory factory in
SotwActualStream, replace the silent return with logic that builds/sends a NACK
for the incoming response (use the stream's existing ACK/NACK helper or call the
method that sends discovery-response errors), include the typeUrl and the reason
string in the NACK, and keep the logger.warn for visibility. Ensure you
reference the same value (value.getTypeUrl()) and
extensionRegistry.queryByTypeUrl semantics when constructing the NACK so the
server receives an explicit protocol failure for unknown types.
In `@xds/src/main/java/com/linecorp/armeria/xds/XdsResourceValidator.java`:
- Around line 76-85: The public method unpack(Any message, Class<T> clazz) lacks
null checks for its parameters; add explicit checks at the start of the method
using Objects.requireNonNull(message, "message") and
Objects.requireNonNull(clazz, "clazz") so callers get clear NPEs and the method
never passes null into message.unpack or assertValid(unpacked).
In `@xds/src/main/java/com/linecorp/armeria/xds/XdsUnpackUtil.java`:
- Around line 36-44: Replace the unsafe assert in XdsUnpackUtil: after obtaining
apiListener and calling registry.queryByTypeUrl(...,
HttpConnectionManagerFactory.class) check if the returned
HttpConnectionManagerFactory (the variable factory) is null and throw a clear
exception (e.g., IllegalStateException or a custom descriptive exception) or
return a handled error instead of relying on assert; ensure the null-check
message references the apiListener.getTypeUrl() and that
factory.create(apiListener, registry.validator()) is only invoked when factory
is non-null.
In `@xds/src/test/java/com/linecorp/armeria/xds/SubscriberStorageTest.java`:
- Around line 68-70: The test is racy because watcher.missingType and
watcher.missingName are set on the event-loop thread and the test waits only for
missingType then reads missingName separately; fix it by making the read happen
inside the same await().untilAsserted() to ensure both watcher.missingType and
watcher.missingName are observed atomically from the test thread (or
alternatively make the fields thread-safe like volatile/AtomicReference), e.g.
assert both watcher.missingType and watcher.missingName within the same
untilAsserted lambda (also apply the same change for the later assertions
covering the other case).
In `@xds/src/test/java/com/linecorp/armeria/xds/XdsExtensionRegistryTest.java`:
- Around line 34-41: Add a static teardown to close the DirectoryWatchService
static field: in XdsExtensionRegistryTest add a public static method (e.g.,
tearDown or cleanup) annotated with `@AfterClass` that calls WATCH_SERVICE.close()
(or SafeCloseable.close()) to release the resource after all tests; ensure the
method is static and handles/propagates any checked exceptions as required by
the test framework so WATCH_SERVICE is properly closed.
---
Outside diff comments:
In
`@it/xds-client/src/test/java/com/linecorp/armeria/xds/it/ConfigSourceLifecycleObserverTest.java`:
- Around line 531-578: The static version field is being incremented inside the
MethodSource provider resourceRejection_args(), causing non-deterministic
version strings during test discovery; move version increments out of the method
source by returning argument tuples that identify the malformed resource type
(e.g., enum or marker) rather than prebuilt Snapshot versions, and then inside
the test method resourceRejection(...) construct the Snapshot with
Snapshot.create(...) using a fresh version string generated there (e.g.,
version.incrementAndGet() or a local counter) so each test invocation computes
its own deterministic version instead of changing a shared static during
discovery.
---
Nitpick comments:
In
`@xds-api/src/main/java/com/linecorp/armeria/xds/api/SupportedFieldValidator.java`:
- Around line 156-164: The loop in SupportedFieldValidator (inside doValidate)
uses an index for map entries which is unhelpful for maps; change the iteration
over Message mapEntry to extract the actual map key via
mapEntry.getField(keyField) (or equivalent key field used for map entries),
convert it to a string (handling nulls and non-String keys), and use that key in
the recursive path call (e.g., fieldPath + '[' + keyString + "].value"); if the
key cannot be obtained, fall back to the index to preserve behavior; update
references to valueField and mapEntry accordingly and ensure no NPEs when key is
missing.
In `@xds/src/main/java/com/linecorp/armeria/xds/ResourceStateStore.java`:
- Around line 76-85: putVersioned(...) currently accepts a caller-supplied
XdsType and resourceName without verifying they match the provided XdsResource,
which can corrupt the per-type state buckets; before calling
isDuplicateEntry(...) validate that resource is non-null and that its
identifying/type fields (use XdsResource's accessor(s) — e.g. the resource's
name/type or, for AbstractXdsResource, the values returned by its getters) equal
the supplied resourceName and type, and if not throw an IllegalArgumentException
that includes both the offending values and the expected type/resourceName;
perform this check at the top of putVersioned (before computing revision and
inserting into statesFor(...)) so incorrect calls fail fast and preserve bucket
invariants.
In `@xds/src/main/java/com/linecorp/armeria/xds/SubscriptionCallbacks.java`:
- Around line 85-98: Remove the redundant explicit type check and rely on
factory.isFullStateOfTheWorld() to determine full-state semantics: replace the
condition "factory != null && factory.isFullStateOfTheWorld() && (type ==
XdsType.LISTENER || type == XdsType.CLUSTER)" with "factory != null &&
factory.isFullStateOfTheWorld()". Leave the subsequent logic that fetches
currentSubscribers via stateCoordinator.interestedResources(type) and iterates
to call stateCoordinator.onResourceMissing(type, name) when parsedResources and
invalidResources do not contain the name; ensure parsedResources and
invalidResources checks remain intact.
In `@xds/src/main/java/com/linecorp/armeria/xds/XdsExtensionRegistry.java`:
- Around line 94-101: The register(...) helper currently blindly overwrites
existing entries in byName and byTypeUrl when an XdsExtensionFactory (from
XdsExtensionFactory.name() / typeUrls()) collides; update register to detect
duplicates by checking byTypeUrl.containsKey(typeUrl) and
byName.containsKey(factory.name()) for each factory, and either log a clear
warning (including both factory.name() and the existing factory identity) or
throw an IllegalStateException to fail fast on conflicts; apply this check for
every factory.typeUrls() and ensure the chosen behavior (warning vs exception)
is used consistently so SPI-loaded factories cannot silently replace built-ins.
In `@xds/src/main/java/com/linecorp/armeria/xds/XdsResourceFactory.java`:
- Around line 47-65: The implementation exposes both abstract type() and
typeUrl(), which allows inconsistent overrides; change typeUrl() to be a default
method that derives its value from type() (i.e., return type().url() or however
XdsType exposes its URL) so the registry key/name and reported XdsType cannot
drift, and keep the ability for implementations to override typeUrl() if needed;
update XdsResourceFactory's typeUrl() default and leave name() and typeUrls()
as-is to continue using typeUrl().
🪄 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: e49ce6d5-1efc-4f35-8f56-f9f940188a3c
📒 Files selected for processing (120)
it/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/XdsControlPlaneErrorHandlingTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/XdsControlPlaneMatrixTest.javait/xds-client/src/test/resources/META-INF/services/com.linecorp.armeria.xds.validator.XdsValidatorIndextesting-internal/src/main/java/com/linecorp/armeria/internal/testing/InternalTestingBlockHoundIntegration.javaxds-api/src/main/java/com/linecorp/armeria/xds/api/DefaultXdsValidatorIndex.javaxds-api/src/main/java/com/linecorp/armeria/xds/api/IgnoreUnsupportedFieldHandler.javaxds-api/src/main/java/com/linecorp/armeria/xds/api/MeterUnsupportedFieldHandler.javaxds-api/src/main/java/com/linecorp/armeria/xds/api/PgvValidator.javaxds-api/src/main/java/com/linecorp/armeria/xds/api/StrictXdsValidatorIndex.javaxds-api/src/main/java/com/linecorp/armeria/xds/api/SupportedFieldValidator.javaxds-api/src/main/java/com/linecorp/armeria/xds/api/UnsupportedFieldHandler.javaxds-api/src/main/proto/armeria/xds/supported.protoxds-api/src/main/proto/envoy/config/bootstrap/v3/bootstrap.protoxds-api/src/main/proto/envoy/config/cluster/v3/cluster.protoxds-api/src/main/proto/envoy/config/core/v3/address.protoxds-api/src/main/proto/envoy/config/core/v3/base.protoxds-api/src/main/proto/envoy/config/core/v3/config_source.protoxds-api/src/main/proto/envoy/config/core/v3/grpc_service.protoxds-api/src/main/proto/envoy/config/core/v3/health_check.protoxds-api/src/main/proto/envoy/config/endpoint/v3/endpoint.protoxds-api/src/main/proto/envoy/config/endpoint/v3/endpoint_components.protoxds-api/src/main/proto/envoy/config/listener/v3/api_listener.protoxds-api/src/main/proto/envoy/config/listener/v3/listener.protoxds-api/src/main/proto/envoy/config/route/v3/route.protoxds-api/src/main/proto/envoy/config/route/v3/route_components.protoxds-api/src/main/proto/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.protoxds-api/src/main/proto/envoy/extensions/transport_sockets/tls/v3/common.protoxds-api/src/main/proto/envoy/extensions/transport_sockets/tls/v3/secret.protoxds-api/src/main/proto/envoy/extensions/transport_sockets/tls/v3/tls.protoxds-api/src/main/proto/envoy/type/matcher/v3/regex.protoxds-api/src/main/proto/envoy/type/matcher/v3/string.protoxds-api/src/main/proto/envoy/type/v3/percent.protoxds-api/src/main/proto/envoy/type/v3/range.protoxds-api/src/test/java/com/linecorp/armeria/xds/api/MeterUnsupportedFieldHandlerTest.javaxds-api/src/test/java/com/linecorp/armeria/xds/api/SupportedFieldValidatorTest.javaxds-api/src/test/proto/armeria/xds/testing/test_supported.protoxds-validator/src/main/java/com/linecorp/armeria/xds/validator/XdsValidatorIndex.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/ClusterResourceFactory.javaxds/src/main/java/com/linecorp/armeria/xds/ClusterResourceParser.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/ConfigSourceHandler.javaxds/src/main/java/com/linecorp/armeria/xds/ConfigSourceLifecycleObserver.javaxds/src/main/java/com/linecorp/armeria/xds/ConfigSourceSubscription.javaxds/src/main/java/com/linecorp/armeria/xds/ControlPlaneClientManager.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/EndpointResourceFactory.javaxds/src/main/java/com/linecorp/armeria/xds/EndpointResourceParser.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/GrpcConfigSourceStreamFactory.javaxds/src/main/java/com/linecorp/armeria/xds/HttpConnectionManagerFactory.javaxds/src/main/java/com/linecorp/armeria/xds/ListenerManager.javaxds/src/main/java/com/linecorp/armeria/xds/ListenerResourceFactory.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/ListenerXdsResource.javaxds/src/main/java/com/linecorp/armeria/xds/PathSotwConfigSourceSubscriptionFactory.javaxds/src/main/java/com/linecorp/armeria/xds/RawBufferTransportSocketFactory.javaxds/src/main/java/com/linecorp/armeria/xds/ResourceNodeAdapter.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/RouteResourceFactory.javaxds/src/main/java/com/linecorp/armeria/xds/RouteResourceParser.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/SecretResourceFactory.javaxds/src/main/java/com/linecorp/armeria/xds/SecretXdsResource.javaxds/src/main/java/com/linecorp/armeria/xds/SotwActualStream.javaxds/src/main/java/com/linecorp/armeria/xds/SotwConfigSourceSubscriptionFactory.javaxds/src/main/java/com/linecorp/armeria/xds/SotwSubscriptionCallbacks.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/SubscriptionCallbacks.javaxds/src/main/java/com/linecorp/armeria/xds/SubscriptionContext.javaxds/src/main/java/com/linecorp/armeria/xds/SubscriptionHandler.javaxds/src/main/java/com/linecorp/armeria/xds/TransportSocketFactory.javaxds/src/main/java/com/linecorp/armeria/xds/TransportSocketStream.javaxds/src/main/java/com/linecorp/armeria/xds/UpstreamTlsTransportSocketFactory.javaxds/src/main/java/com/linecorp/armeria/xds/VirtualHostXdsResource.javaxds/src/main/java/com/linecorp/armeria/xds/XdsBootstrapImpl.javaxds/src/main/java/com/linecorp/armeria/xds/XdsClusterManager.javaxds/src/main/java/com/linecorp/armeria/xds/XdsConverterUtil.javaxds/src/main/java/com/linecorp/armeria/xds/XdsExtensionFactory.javaxds/src/main/java/com/linecorp/armeria/xds/XdsExtensionRegistry.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/XdsResourceFactory.javaxds/src/main/java/com/linecorp/armeria/xds/XdsResourceParseUtil.javaxds/src/main/java/com/linecorp/armeria/xds/XdsResourceParserUtil.javaxds/src/main/java/com/linecorp/armeria/xds/XdsResourceValidator.javaxds/src/main/java/com/linecorp/armeria/xds/XdsResponseHandler.javaxds/src/main/java/com/linecorp/armeria/xds/XdsStreamSubscriber.javaxds/src/main/java/com/linecorp/armeria/xds/XdsUnpackUtil.javaxds/src/main/java/com/linecorp/armeria/xds/XdsValidatorIndexRegistry.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/test/java/com/linecorp/armeria/xds/SotwXdsStreamTest.javaxds/src/test/java/com/linecorp/armeria/xds/StateCoordinatorTest.javaxds/src/test/java/com/linecorp/armeria/xds/SubscriberStorageTest.javaxds/src/test/java/com/linecorp/armeria/xds/TestResourceWatcher.javaxds/src/test/java/com/linecorp/armeria/xds/XdsClientCleanupTest.javaxds/src/test/java/com/linecorp/armeria/xds/XdsClientIntegrationTest.javaxds/src/test/java/com/linecorp/armeria/xds/XdsExtensionRegistryTest.javaxds/src/test/java/com/linecorp/armeria/xds/XdsResourceValidatorTest.javaxds/src/test/java/com/linecorp/armeria/xds/XdsValidatorIndexRegistryTest.javaxds/src/test/resources/META-INF/services/com.linecorp.armeria.xds.validator.XdsValidatorIndex
💤 Files with no reviewable changes (15)
- xds/src/test/java/com/linecorp/armeria/xds/XdsValidatorIndexRegistryTest.java
- xds/src/main/java/com/linecorp/armeria/xds/ResourceParser.java
- xds/src/main/java/com/linecorp/armeria/xds/filter/HttpFilterFactoryRegistry.java
- xds/src/main/java/com/linecorp/armeria/xds/XdsResponseHandler.java
- xds/src/main/java/com/linecorp/armeria/xds/XdsValidatorIndexRegistry.java
- xds/src/main/java/com/linecorp/armeria/xds/ClusterResourceParser.java
- xds/src/main/java/com/linecorp/armeria/xds/ListenerResourceParser.java
- xds/src/main/java/com/linecorp/armeria/xds/EndpointResourceParser.java
- xds/src/test/java/com/linecorp/armeria/xds/SotwXdsStreamTest.java
- xds/src/main/java/com/linecorp/armeria/xds/XdsResourceParserUtil.java
- xds/src/main/java/com/linecorp/armeria/xds/RouteResourceParser.java
- xds/src/main/java/com/linecorp/armeria/xds/SotwXdsStream.java
- xds/src/main/java/com/linecorp/armeria/xds/DefaultResponseHandler.java
- xds/src/main/java/com/linecorp/armeria/xds/XdsConverterUtil.java
- xds/src/main/java/com/linecorp/armeria/xds/ConfigSourceClient.java
Comment on lines
+147
to
+163
| cache.setSnapshot(GROUP, Snapshot.create( | ||
| ImmutableList.of(staticClusterYaml(CLUSTER_NAME)), | ||
| ImmutableList.of(), | ||
| ImmutableList.of(), | ||
| ImmutableList.of(routeYaml(ROUTE_NAME, CLUSTER_NAME)), | ||
| ImmutableList.of(), | ||
| "3")); | ||
| await().untilAsserted(() -> assertThat(errors).anyMatch(error -> { | ||
| if (!(error instanceof MissingXdsResourceException)) { | ||
| return false; | ||
| } | ||
| final MissingXdsResourceException exception = (MissingXdsResourceException) error; | ||
| return exception.type() == XdsType.LISTENER && | ||
| exception.name().equals(LISTENER_NAME); | ||
| })); | ||
| final int sizeAfterRemoval = snapshots.size(); | ||
|
|
Contributor
There was a problem hiding this comment.
Deletion step doesn't verify the “no new snapshot” contract.
This only waits for a MissingXdsResourceException and then snapshots the current size. If deletion accidentally emits another ListenerSnapshot, the test still passes and the regression slips through.
Suggested tightening
- cache.setSnapshot(GROUP, Snapshot.create(
+ final int sizeBeforeRemoval = snapshots.size();
+ cache.setSnapshot(GROUP, Snapshot.create(
ImmutableList.of(staticClusterYaml(CLUSTER_NAME)),
ImmutableList.of(),
ImmutableList.of(),
ImmutableList.of(routeYaml(ROUTE_NAME, CLUSTER_NAME)),
ImmutableList.of(),
"3"));
- await().untilAsserted(() -> assertThat(errors).anyMatch(error -> {
- if (!(error instanceof MissingXdsResourceException)) {
- return false;
- }
- final MissingXdsResourceException exception = (MissingXdsResourceException) error;
- return exception.type() == XdsType.LISTENER &&
- exception.name().equals(LISTENER_NAME);
- }));
- final int sizeAfterRemoval = snapshots.size();
+ await().untilAsserted(() -> {
+ assertThat(errors).anyMatch(error -> {
+ if (!(error instanceof MissingXdsResourceException)) {
+ return false;
+ }
+ final MissingXdsResourceException exception =
+ (MissingXdsResourceException) error;
+ return exception.type() == XdsType.LISTENER &&
+ exception.name().equals(LISTENER_NAME);
+ });
+ assertThat(snapshots).hasSize(sizeBeforeRemoval);
+ });
+ final int sizeAfterRemoval = sizeBeforeRemoval;🤖 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 147 - 163, The test only waits for MissingXdsResourceException but
doesn't assert that no new snapshots were added; before calling
cache.setSnapshot(...) capture the current snapshots.size() (e.g.,
sizeBeforeRemoval), then after awaiting the MissingXdsResourceException assert
that snapshots.size() == sizeBeforeRemoval to ensure no new snapshot was
emitted, and optionally verify the last snapshot (or lack thereof) does not
contain a ListenerSnapshot for LISTENER_NAME; refer to
DeltaXdsResourceWatcherTest, cache.setSnapshot(...), errors,
MissingXdsResourceException, XdsType.LISTENER, LISTENER_NAME and snapshots when
making these checks.
Comment on lines
+261
to
+268
| 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 deletion error type, not just “some error happened”.
isNotEmpty() passes on any asynchronous failure, so this step can go green without proving cluster/endpoint deletion behaved correctly.
Suggested tightening
- 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;
+ }));📝 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
| cache.setSnapshot(GROUP, Snapshot.create( | |
| ImmutableList.of(), | |
| ImmutableList.of(), | |
| ImmutableList.of(), | |
| ImmutableList.of(), | |
| ImmutableList.of(), | |
| "4")); | |
| await().untilAsserted(() -> assertThat(errors).isNotEmpty()); | |
| cache.setSnapshot(GROUP, Snapshot.create( | |
| ImmutableList.of(), | |
| ImmutableList.of(), | |
| ImmutableList.of(), | |
| ImmutableList.of(), | |
| ImmutableList.of(), | |
| "4")); | |
| 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; | |
| })); |
🤖 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 261 - 268, The test currently only checks that errors is non-empty
after calling cache.setSnapshot(GROUP, Snapshot.create(..., "4")), which can
hide unrelated async failures; change the assertion in the await().untilAsserted
block to verify the specific deletion error type or message for cluster/endpoint
deletion rather than assertThat(errors).isNotEmpty(). Locate the
await().untilAsserted usage that wraps assertThat(errors) and replace the
predicate to assert that errors contains an entry matching the expected
exception/class/message (e.g., specific deletion error type or a known error
string) so the test validates the correct deletion behavior for the Snapshot
version "4".
Comment on lines
+53
to
+57
| public void handle(String fieldPath) { | ||
| final Counter counter = Counter.builder(meterIdPrefix.name("unsupported.fields")) | ||
| .tags(meterIdPrefix.tags()) | ||
| .tag("field", fieldPath) | ||
| .register(meterRegistry); |
Contributor
There was a problem hiding this comment.
Add explicit null-check for fieldPath in the public handle(...) method.
At Line 53, fieldPath should be validated with requireNonNull before using it to build tags.
Suggested fix
`@Override`
public void handle(String fieldPath) {
+ fieldPath = requireNonNull(fieldPath, "fieldPath");
final Counter counter = Counter.builder(meterIdPrefix.name("unsupported.fields"))
.tags(meterIdPrefix.tags())
.tag("field", fieldPath)
.register(meterRegistry);
counter.increment();
}As per coding guidelines: "Do explicit null checks on user-facing public parameters using Objects.requireNonNull(obj, "name")".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@xds-api/src/main/java/com/linecorp/armeria/xds/api/MeterUnsupportedFieldHandler.java`
around lines 53 - 57, In MeterUnsupportedFieldHandler, add an explicit
null-check at the start of the public method handle(String fieldPath) by calling
Objects.requireNonNull(fieldPath, "fieldPath") so fieldPath is validated before
being used to build tags (Counter.builder(...).tag("field", fieldPath)...); this
ensures the public parameter is non-null per coding guidelines and avoids NPEs
when registering the counter with meterRegistry.
Comment on lines
+49
to
+52
| public void assertValid(Object message) { | ||
| try { | ||
| delegate.validatorFor(message).assertValid(message); | ||
| } catch (ValidationException e) { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "PgvValidator.java" -type fRepository: line/armeria
Length of output: 126
🏁 Script executed:
cat -n xds-api/src/main/java/com/linecorp/armeria/xds/api/PgvValidator.javaRepository: line/armeria
Length of output: 2173
Add explicit null validation in public assertValid API.
The assertValid(Object message) method must include an explicit null check per coding guidelines: "Do explicit null checks on user-facing public parameters using Objects.requireNonNull(obj, "name")." Currently, null handling is delegated indirectly, making the error contract non-deterministic.
Proposed fix
+import java.util.Objects;
+
import com.linecorp.armeria.common.annotation.UnstableApi;
@@
public void assertValid(Object message) {
+ Objects.requireNonNull(message, "message");
try {
delegate.validatorFor(message).assertValid(message);
} catch (ValidationException e) {
throw new IllegalArgumentException(e);
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@xds-api/src/main/java/com/linecorp/armeria/xds/api/PgvValidator.java` around
lines 49 - 52, The public method PgvValidator.assertValid(Object message) must
perform an explicit null check on the incoming parameter before delegating; add
Objects.requireNonNull(message, "message") at the start of assertValid to
enforce the contract and keep the existing try/catch (catching
ValidationException) and delegation to delegate.validatorFor(message). Ensure
the method throws a clear NPE immediately for null input rather than relying on
delegate behavior.
Comment on lines
+40
to
+46
| @Override | ||
| public void assertValid(Object message) { | ||
| pgvValidator.assertValid(message); | ||
| final List<String> violations = new ArrayList<>(); | ||
| final SupportedFieldValidator validator = | ||
| new SupportedFieldValidator(violations::add); | ||
| validator.assertValid(message); |
Contributor
There was a problem hiding this comment.
Guard message at the public API boundary.
assertValid(null) currently falls through to downstream validators, so callers won't get a stable, intentional failure from this new public API.
Suggested fix
+import java.util.Objects;
@@
`@Override`
public void assertValid(Object message) {
+ Objects.requireNonNull(message, "message");
pgvValidator.assertValid(message);
final List<String> violations = new ArrayList<>();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@xds-api/src/main/java/com/linecorp/armeria/xds/api/StrictXdsValidatorIndex.java`
around lines 40 - 46, The public method StrictXdsValidatorIndex.assertValid
currently allows a null message to pass to downstream validators; add an
explicit null-check using Objects.requireNonNull(message, "message") at the
start of assertValid to fail fast and give a stable error to callers before
calling pgvValidator.assertValid(message) and
SupportedFieldValidator.assertValid(message); update imports if needed to
reference java.util.Objects.
Comment on lines
+145
to
+149
| final XdsResourceFactory factory = | ||
| extensionRegistry.queryByTypeUrl(value.getTypeUrl(), XdsResourceFactory.class); | ||
| if (factory == null) { | ||
| logger.warn("XDS stream Received unexpected type: {}", value.getTypeUrl()); | ||
| return; |
Contributor
There was a problem hiding this comment.
Don't silently drop unknown SotW response types.
If the client can't resolve typeUrl, logging and returning means the management server never gets an ACK/NACK for that response. This should be treated as a protocol failure, not 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/SotwActualStream.java` around
lines 145 - 149, SotwActualStream currently logs and returns when
extensionRegistry.queryByTypeUrl(value.getTypeUrl(), XdsResourceFactory.class)
yields null, which drops the response without ACK/NACK; instead, treat this as a
protocol error by sending a NACK for the received DiscoveryResponse (including
value.getTypeUrl() and a clear error message) back to the management server and
then return. Locate the null-check around XdsResourceFactory factory in
SotwActualStream, replace the silent return with logic that builds/sends a NACK
for the incoming response (use the stream's existing ACK/NACK helper or call the
method that sends discovery-response errors), include the typeUrl and the reason
string in the NACK, and keep the logger.warn for visibility. Ensure you
reference the same value (value.getTypeUrl()) and
extensionRegistry.queryByTypeUrl semantics when constructing the NACK so the
server receives an explicit protocol failure for unknown types.
Comment on lines
+76
to
+85
| public <T extends Message> T unpack(Any message, Class<T> clazz) { | ||
| final T unpacked; | ||
| try { | ||
| unpacked = message.unpack(clazz); | ||
| } catch (InvalidProtocolBufferException e) { | ||
| throw new IllegalArgumentException("Error unpacking: " + clazz.getName(), e); | ||
| } | ||
| assertValid(unpacked); | ||
| return unpacked; | ||
| } |
Contributor
There was a problem hiding this comment.
Add null checks for public method parameters.
The unpack method is public but doesn't validate its parameters. Per coding guidelines, public methods should perform explicit null checks on user-facing parameters.
🛡️ Proposed fix
+import java.util.Objects;
+
...
public <T extends Message> T unpack(Any message, Class<T> clazz) {
+ Objects.requireNonNull(message, "message");
+ Objects.requireNonNull(clazz, "clazz");
final T unpacked;
try {
unpacked = message.unpack(clazz);As per coding guidelines: "Do explicit null checks on user-facing public parameters using Objects.requireNonNull(obj, \"name\")."
📝 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
| public <T extends Message> T unpack(Any message, Class<T> clazz) { | |
| final T unpacked; | |
| try { | |
| unpacked = message.unpack(clazz); | |
| } catch (InvalidProtocolBufferException e) { | |
| throw new IllegalArgumentException("Error unpacking: " + clazz.getName(), e); | |
| } | |
| assertValid(unpacked); | |
| return unpacked; | |
| } | |
| public <T extends Message> T unpack(Any message, Class<T> clazz) { | |
| Objects.requireNonNull(message, "message"); | |
| Objects.requireNonNull(clazz, "clazz"); | |
| final T unpacked; | |
| try { | |
| unpacked = message.unpack(clazz); | |
| } catch (InvalidProtocolBufferException e) { | |
| throw new IllegalArgumentException("Error unpacking: " + clazz.getName(), e); | |
| } | |
| assertValid(unpacked); | |
| return unpacked; | |
| } |
🤖 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/XdsResourceValidator.java` around
lines 76 - 85, The public method unpack(Any message, Class<T> clazz) lacks null
checks for its parameters; add explicit checks at the start of the method using
Objects.requireNonNull(message, "message") and Objects.requireNonNull(clazz,
"clazz") so callers get clear NPEs and the method never passes null into
message.unpack or assertValid(unpacked).
Comment on lines
+36
to
+44
| if (listener.getApiListener().hasApiListener()) { | ||
| final Any apiListener = listener.getApiListener().getApiListener(); | ||
| final HttpConnectionManagerFactory factory = | ||
| registry.queryByTypeUrl(apiListener.getTypeUrl(), | ||
| HttpConnectionManagerFactory.class); | ||
| assert factory != null; | ||
| return factory.create(apiListener, registry.validator()); | ||
| } | ||
| return null; |
Contributor
There was a problem hiding this comment.
Replace assert with explicit null check.
Using assert factory != null is unsafe because assertions can be disabled at runtime (-da flag). If the type URL is unregistered, this silently passes in production, leading to an NPE at factory.create(...).
🛡️ Proposed fix
if (listener.getApiListener().hasApiListener()) {
final Any apiListener = listener.getApiListener().getApiListener();
final HttpConnectionManagerFactory factory =
registry.queryByTypeUrl(apiListener.getTypeUrl(),
HttpConnectionManagerFactory.class);
- assert factory != null;
+ if (factory == null) {
+ throw new IllegalArgumentException(
+ "No HttpConnectionManagerFactory registered for type URL: " +
+ apiListener.getTypeUrl());
+ }
return factory.create(apiListener, registry.validator());
}📝 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
| if (listener.getApiListener().hasApiListener()) { | |
| final Any apiListener = listener.getApiListener().getApiListener(); | |
| final HttpConnectionManagerFactory factory = | |
| registry.queryByTypeUrl(apiListener.getTypeUrl(), | |
| HttpConnectionManagerFactory.class); | |
| assert factory != null; | |
| return factory.create(apiListener, registry.validator()); | |
| } | |
| return null; | |
| if (listener.getApiListener().hasApiListener()) { | |
| final Any apiListener = listener.getApiListener().getApiListener(); | |
| final HttpConnectionManagerFactory factory = | |
| registry.queryByTypeUrl(apiListener.getTypeUrl(), | |
| HttpConnectionManagerFactory.class); | |
| if (factory == null) { | |
| throw new IllegalArgumentException( | |
| "No HttpConnectionManagerFactory registered for type URL: " + | |
| apiListener.getTypeUrl()); | |
| } | |
| return factory.create(apiListener, registry.validator()); | |
| } | |
| return null; |
🤖 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/XdsUnpackUtil.java` around lines
36 - 44, Replace the unsafe assert in XdsUnpackUtil: after obtaining apiListener
and calling registry.queryByTypeUrl(..., HttpConnectionManagerFactory.class)
check if the returned HttpConnectionManagerFactory (the variable factory) is
null and throw a clear exception (e.g., IllegalStateException or a custom
descriptive exception) or return a handled error instead of relying on assert;
ensure the null-check message references the apiListener.getTypeUrl() and that
factory.create(apiListener, registry.validator()) is only invoked when factory
is non-null.
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.
Make the captured timeout result safe to read from the test thread.
onResourceDoesNotExist() updates missingType and missingName from the event-loop thread, but this test waits only for missingType and then reads missingName separately. That can observe the callback half-written and make the test flaky.
Suggested fix
- await().atMost(1, TimeUnit.SECONDS)
- .untilAsserted(() -> assertThat(watcher.missingType).isEqualTo(XdsType.ROUTE));
- assertThat(watcher.missingName).isEqualTo(ROUTE_NAME);
+ await().atMost(1, TimeUnit.SECONDS)
+ .untilAsserted(() -> {
+ assertThat(watcher.missingType).isEqualTo(XdsType.ROUTE);
+ assertThat(watcher.missingName).isEqualTo(ROUTE_NAME);
+ });
@@
- private XdsType missingType;
- private String missingName;
+ private volatile XdsType missingType;
+ private volatile String missingName;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 test is racy because watcher.missingType and
watcher.missingName are set on the event-loop thread and the test waits only for
missingType then reads missingName separately; fix it by making the read happen
inside the same await().untilAsserted() to ensure both watcher.missingType and
watcher.missingName are observed atomically from the test thread (or
alternatively make the fields thread-safe like volatile/AtomicReference), e.g.
assert both watcher.missingType and watcher.missingName within the same
untilAsserted lambda (also apply the same change for the later assertions
covering the other case).
Comment on lines
+34
to
+41
| private static final XdsResourceValidator VALIDATOR = new XdsResourceValidator(); | ||
| private static final DirectoryWatchService WATCH_SERVICE = new DirectoryWatchService(); | ||
|
|
||
| private static XdsExtensionRegistry createRegistry() { | ||
| return XdsExtensionRegistry.of(VALIDATOR, WATCH_SERVICE, | ||
| Flags.meterRegistry(), | ||
| new MeterIdPrefix("armeria.xds")); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if DirectoryWatchService implements AutoCloseable or Closeable
ast-grep --pattern 'class DirectoryWatchService $$$implements $_ {
$$$
}'
# Also check parent classes
rg -n "class DirectoryWatchService" --type java -A 5Repository: line/armeria
Length of output: 1446
🏁 Script executed:
cat -n xds/src/test/java/com/linecorp/armeria/xds/XdsExtensionRegistryTest.javaRepository: line/armeria
Length of output: 6250
🏁 Script executed:
# Check SafeCloseable interface
rg -n "interface SafeCloseable" --type java -A 10
# Check how DirectoryWatchService is used in other tests
rg -n "DirectoryWatchService" --type java -B 2 -A 2Repository: line/armeria
Length of output: 19402
🏁 Script executed:
rg -n "class XdsExtensionRegistry" --type java -A 5Repository: line/armeria
Length of output: 1426
Add cleanup for DirectoryWatchService static field.
DirectoryWatchService implements SafeCloseable and should be closed after all tests complete. Add a static teardown method to properly clean up the resource.
🤖 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/XdsExtensionRegistryTest.java`
around lines 34 - 41, Add a static teardown to close the DirectoryWatchService
static field: in XdsExtensionRegistryTest add a public static method (e.g.,
tearDown or cleanup) annotated with `@AfterClass` that calls WATCH_SERVICE.close()
(or SafeCloseable.close()) to release the resource after all tests; ensure the
method is static and handles/propagates any checked exceptions as required by
the test framework so WATCH_SERVICE is properly closed.
Contributor
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
xds/src/main/java/com/linecorp/armeria/xds/SubscriptionCallbacks.java (2)
43-47: Add null checks for constructor parameters.Per coding guidelines, constructor parameters should be validated with
Objects.requireNonNull().Proposed fix
+import static java.util.Objects.requireNonNull; + SubscriptionCallbacks(StateCoordinator stateCoordinator, - XdsExtensionRegistry extensionRegistry) { - this.stateCoordinator = stateCoordinator; - this.extensionRegistry = extensionRegistry; + XdsExtensionRegistry extensionRegistry) { + this.stateCoordinator = requireNonNull(stateCoordinator, "stateCoordinator"); + this.extensionRegistry = requireNonNull(extensionRegistry, "extensionRegistry"); }As per coding guidelines: "add null checks for user-facing public methods using
Objects.requireNonNull(..., \"paramName\")".🤖 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/SubscriptionCallbacks.java` around lines 43 - 47, Add null checks in the SubscriptionCallbacks constructor by validating its parameters using Objects.requireNonNull: call Objects.requireNonNull(stateCoordinator, "stateCoordinator") and Objects.requireNonNull(extensionRegistry, "extensionRegistry") before assigning to the fields stateCoordinator and extensionRegistry so the constructor fails fast on null inputs.
98-100: Unused parametersystemVersionInfo.The
systemVersionInfoparameter is declared but never used in the method body. If this is intentionally reserved for future use or API consistency with delta protocol, consider adding a brief comment. Otherwise, consider removing it.🤖 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/SubscriptionCallbacks.java` around lines 98 - 100, The method SubscriptionCallbacks.onConfigUpdate currently declares a parameter systemVersionInfo that is unused; either remove the parameter from the onConfigUpdate signature (and update all callers and implementations) or, if it must remain for API compatibility, add a brief inline comment in the onConfigUpdate declaration explaining it is intentionally reserved for future/delta-protocol use (e.g., "reserved for future use" or "kept for API compatibility") and annotate with `@SuppressWarnings`("unused") if your style permits to avoid warnings; update any implementing classes to match the chosen approach.xds/src/main/java/com/linecorp/armeria/xds/XdsExtensionRegistry.java (2)
87-94: Silent overwriting of factories may mask configuration errors.
register()silently overwrites existing factory entries. If an SPI-loaded factory shares a name or typeUrl with a built-in, the built-in wins without warning. Consider logging a warning or throwing if a duplicate is detected.Proposed fix to detect duplicates
private static void register(XdsExtensionFactory factory, Map<String, XdsExtensionFactory> byName, Map<String, XdsExtensionFactory> byTypeUrl) { - byName.put(factory.name(), factory); + final XdsExtensionFactory existing = byName.put(factory.name(), factory); + if (existing != null) { + // Log or handle duplicate - built-in overriding SPI is intentional + } for (String typeUrl : factory.typeUrls()) { byTypeUrl.put(typeUrl, factory); } }🤖 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/XdsExtensionRegistry.java` around lines 87 - 94, The register method currently overwrites existing entries in byName/byTypeUrl without notice; update XdsExtensionRegistry.register to detect duplicates before inserting: check byName.containsKey(factory.name()) and byTypeUrl.containsKey(typeUrl) for each factory.typeUrls(), and either log a warning including the conflicting key and existing factory or throw an IllegalStateException depending on policy; use the existing process/logger used elsewhere in this class (or a static logger) and reference factory.name(), factory.typeUrls(), byName and byTypeUrl when building the diagnostic message so duplicate SPI vs built-in registrations are surfaced instead of silently overwritten.
47-53: Add null checks for constructor parameters.Per coding guidelines, user-facing or API-level constructor parameters should validate nulls. Even though this is package-private, defensive null checks prevent obscure NPEs downstream.
Proposed fix
+import static java.util.Objects.requireNonNull; + private XdsExtensionRegistry(Map<String, XdsExtensionFactory> byTypeUrl, Map<String, XdsExtensionFactory> byName, XdsResourceValidator validator) { - this.byTypeUrl = byTypeUrl; - this.byName = byName; - this.validator = validator; + this.byTypeUrl = requireNonNull(byTypeUrl, "byTypeUrl"); + this.byName = requireNonNull(byName, "byName"); + this.validator = requireNonNull(validator, "validator"); }🤖 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/XdsExtensionRegistry.java` around lines 47 - 53, The XdsExtensionRegistry constructor should validate its parameters to avoid later NPEs: in the XdsExtensionRegistry(Map<String, XdsExtensionFactory> byTypeUrl, Map<String, XdsExtensionFactory> byName, XdsResourceValidator validator) constructor, add null checks for byTypeUrl, byName, and validator (e.g., via Objects.requireNonNull or explicit checks) and throw a clear NullPointerException with a message identifying the missing parameter before assigning to this.byTypeUrl, this.byName, and this.validator.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@xds/src/main/java/com/linecorp/armeria/xds/SubscriptionCallbacks.java`:
- Around line 43-47: Add null checks in the SubscriptionCallbacks constructor by
validating its parameters using Objects.requireNonNull: call
Objects.requireNonNull(stateCoordinator, "stateCoordinator") and
Objects.requireNonNull(extensionRegistry, "extensionRegistry") before assigning
to the fields stateCoordinator and extensionRegistry so the constructor fails
fast on null inputs.
- Around line 98-100: The method SubscriptionCallbacks.onConfigUpdate currently
declares a parameter systemVersionInfo that is unused; either remove the
parameter from the onConfigUpdate signature (and update all callers and
implementations) or, if it must remain for API compatibility, add a brief inline
comment in the onConfigUpdate declaration explaining it is intentionally
reserved for future/delta-protocol use (e.g., "reserved for future use" or "kept
for API compatibility") and annotate with `@SuppressWarnings`("unused") if your
style permits to avoid warnings; update any implementing classes to match the
chosen approach.
In `@xds/src/main/java/com/linecorp/armeria/xds/XdsExtensionRegistry.java`:
- Around line 87-94: The register method currently overwrites existing entries
in byName/byTypeUrl without notice; update XdsExtensionRegistry.register to
detect duplicates before inserting: check byName.containsKey(factory.name()) and
byTypeUrl.containsKey(typeUrl) for each factory.typeUrls(), and either log a
warning including the conflicting key and existing factory or throw an
IllegalStateException depending on policy; use the existing process/logger used
elsewhere in this class (or a static logger) and reference factory.name(),
factory.typeUrls(), byName and byTypeUrl when building the diagnostic message so
duplicate SPI vs built-in registrations are surfaced instead of silently
overwritten.
- Around line 47-53: The XdsExtensionRegistry constructor should validate its
parameters to avoid later NPEs: in the XdsExtensionRegistry(Map<String,
XdsExtensionFactory> byTypeUrl, Map<String, XdsExtensionFactory> byName,
XdsResourceValidator validator) constructor, add null checks for byTypeUrl,
byName, and validator (e.g., via Objects.requireNonNull or explicit checks) and
throw a clear NullPointerException with a message identifying the missing
parameter before assigning to this.byTypeUrl, this.byName, and this.validator.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6c8280e1-5953-4bef-b6ce-c25d2920bfe6
📒 Files selected for processing (11)
xds/src/main/java/com/linecorp/armeria/xds/ClusterResourceParser.javaxds/src/main/java/com/linecorp/armeria/xds/DeltaActualStream.javaxds/src/main/java/com/linecorp/armeria/xds/EndpointResourceParser.javaxds/src/main/java/com/linecorp/armeria/xds/ListenerResourceParser.javaxds/src/main/java/com/linecorp/armeria/xds/ResourceParser.javaxds/src/main/java/com/linecorp/armeria/xds/RouteResourceParser.javaxds/src/main/java/com/linecorp/armeria/xds/SecretResourceParser.javaxds/src/main/java/com/linecorp/armeria/xds/SotwActualStream.javaxds/src/main/java/com/linecorp/armeria/xds/SubscriptionCallbacks.javaxds/src/main/java/com/linecorp/armeria/xds/XdsExtensionRegistry.javaxds/src/main/java/com/linecorp/armeria/xds/XdsResourceParserUtil.java
✅ Files skipped from review due to trivial changes (1)
- xds/src/main/java/com/linecorp/armeria/xds/DeltaActualStream.java
🚧 Files skipped from review as they are similar to previous changes (2)
- xds/src/main/java/com/linecorp/armeria/xds/ClusterResourceParser.java
- xds/src/main/java/com/linecorp/armeria/xds/SotwActualStream.java
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6721 +/- ##
============================================
- Coverage 74.46% 73.70% -0.77%
- Complexity 22234 24298 +2064
============================================
Files 1963 2199 +236
Lines 82437 91593 +9156
Branches 10764 11957 +1193
============================================
+ Hits 61385 67505 +6120
- Misses 15918 18400 +2482
- Partials 5134 5688 +554 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jrhee17
force-pushed
the
feat/xds-support-field
branch
from
2631fa8 to
1e93d34
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
xds-api/src/main/java/com/linecorp/armeria/xds/api/StrictXdsValidatorIndex.java (1)
19-20:⚠️ Potential issue | 🟡 MinorStill missing a
nullguard at the public API boundary.
assertValid(null)still falls through to downstream validators instead of failing fast from this new public API.As per coding guidelines, "Use null-safety conventions: explicit requireNonNull checks for user-facing public methods."Suggested fix
import java.util.ArrayList; import java.util.List; +import java.util.Objects; @@ `@Override` public void assertValid(Object message) { + Objects.requireNonNull(message, "message"); pgvValidator.assertValid(message); final List<String> violations = new ArrayList<>();Also applies to: 40-46
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xds-api/src/main/java/com/linecorp/armeria/xds/api/StrictXdsValidatorIndex.java` around lines 19 - 20, StrictXdsValidatorIndex's public API lacks a null-guard: add an explicit null check (e.g., Objects.requireNonNull(...)) at the start of the public assertValid(...) method(s) in class StrictXdsValidatorIndex so assertValid(null) throws immediately rather than delegating to downstream validators; place the requireNonNull call at the top of the public method(s) referenced (assertValid) to enforce null-safety for the API boundary.xds/src/main/java/com/linecorp/armeria/xds/GrpcConfigSourceStreamFactory.java (1)
97-103:⚠️ Potential issue | 🟠 MajorValidate every configured
GrpcService.Lines 100-102 only check the first entry, so a config with
grpc_services[0].envoy_grpcand an unsupported fallback entry still passes this gate.Proposed fix
final List<GrpcService> grpcServices = apiConfigSource.getGrpcServicesList(); checkArgument(!grpcServices.isEmpty(), "At least one GrpcService should be specified for '%s'", configSource); + for (GrpcService grpcService : grpcServices) { + checkArgument(grpcService.hasEnvoyGrpc(), + "Only envoyGrpc is supported for '%s'", configSource); + } final GrpcService firstGrpcService = grpcServices.get(0); - checkArgument(firstGrpcService.hasEnvoyGrpc(), - "Only envoyGrpc is supported for '%s'", configSource); final EnvoyGrpc envoyGrpc = firstGrpcService.getEnvoyGrpc();🤖 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/GrpcConfigSourceStreamFactory.java` around lines 97 - 103, The code only validates grpc_services[0]; instead iterate apiConfigSource.getGrpcServicesList() and assert each GrpcService.hasEnvoyGrpc() (e.g., for (GrpcService gs : grpcServices) checkArgument(gs.hasEnvoyGrpc(), "Only envoyGrpc is supported for '%s'", configSource)); then keep the existing firstGrpcService/getEnvoyGrpc() usage to obtain envoyGrpc after the per-entry validation. This ensures every configured GrpcService is validated, not just the first.xds/src/main/java/com/linecorp/armeria/xds/SotwActualStream.java (2)
153-172:⚠️ Potential issue | 🟠 MajorIgnore terminal callbacks after
closeStream().
closeStream()marks the stream completed on Line 94, but lateronError()/onCompleted()still driveretryOrClose(...). That turns an intentional local shutdown into a reconnect.Proposed fix
`@Override` public void onError(Throwable throwable) { if (!eventLoop.inEventLoop()) { eventLoop.execute(() -> onError(throwable)); return; } + if (completed) { + return; + } completed = true; lifecycleObserver.streamError(throwable); owner.retryOrClose(true); } `@Override` public void onCompleted() { if (!eventLoop.inEventLoop()) { eventLoop.execute(this::onCompleted); return; } + if (completed) { + return; + } completed = true; lifecycleObserver.streamCompleted(); owner.retryOrClose(false); }🤖 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 153 - 172, The onError/onCompleted paths must ignore terminal callbacks if closeStream() already marked the stream completed; add an early guard in both onError(Throwable) and onCompleted() that returns immediately when completed is true (before scheduling onto eventLoop) so you don't reschedule work or call lifecycleObserver.streamError/streamCompleted or owner.retryOrClose(...) after a local close; reference the existing completed flag, the onError/onCompleted methods, lifecycleObserver, owner.retryOrClose, and closeStream() when making this change.
145-149:⚠️ Potential issue | 🟠 MajorNACK unknown SotW response types instead of dropping them.
Logging and returning here means the management server never gets an ACK/NACK for that response. This should be treated as a protocol error, not 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/SotwActualStream.java` around lines 145 - 149, The code in SotwActualStream currently logs and returns when XdsResourceParserUtil.fromTypeUrl(value.getTypeUrl()) returns null, which drops the response; instead construct and send a NACK for this DiscoveryResponse: build an error Status (e.g., INVALID_ARGUMENT) with a descriptive message including value.getTypeUrl(), then call the class's existing NACK pathway using the DiscoveryResponse identifiers (value.getVersionInfo(), value.getNonce(), and optionally resource names from value.getResourcesList()) — reuse the SotwActualStream method that sends NACKs (the existing nack/sendNack/onNack handler in this class) so the management server receives an explicit NACK rather than silently ignoring the unknown type.
🧹 Nitpick comments (4)
xds/src/main/java/com/linecorp/armeria/xds/ListenerManager.java (1)
65-72: ReuseListenerResourceParserfor bootstrap listeners.This is the same unpack/resolve sequence now implemented in
ListenerResourceParser.parse(...), so the static-listener path can drift from the xDS parsing path.Suggested refactor
- final XdsExtensionRegistry registry = context.extensionRegistry(); - final HttpConnectionManager connectionManager = - XdsUnpackUtil.unpackConnectionManager(listener, registry); - final ListenerXdsResource listenerResource = - new ListenerXdsResource(listener, connectionManager, - XdsUnpackUtil.resolveDownstreamFilters(connectionManager, registry), - "", 0); + final ListenerXdsResource listenerResource = + ListenerResourceParser.INSTANCE.parse(listener, context.extensionRegistry(), "", 0);🤖 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/ListenerManager.java` around lines 65 - 72, The static-listener path duplicates the unpack/resolve logic; replace the manual calls to XdsUnpackUtil.unpackConnectionManager and XdsUnpackUtil.resolveDownstreamFilters and the direct construction of ListenerXdsResource in ListenerManager with a call to ListenerResourceParser.parse(...) to produce the ListenerXdsResource, then pass that resource into new ListenerStream(listenerResource, context); locate the block that currently uses XdsExtensionRegistry registry, XdsUnpackUtil.unpackConnectionManager(listener,...), XdsUnpackUtil.resolveDownstreamFilters(...), and ListenerXdsResource(...) and swap it to use ListenerResourceParser.parse(listener, registry, ...) (matching the available parse signature) to ensure both paths share the same parsing logic.xds/src/main/java/com/linecorp/armeria/xds/SubscriptionCallbacks.java (1)
82-95: Redundant condition in absent detection logic.The condition on line 84-85 has redundancy:
parser.isFullStateOfTheWorld()should already imply LDS or CDS types, making the explicit type check redundant- The guard on line 87
!parsedResources.isEmpty() || !currentSubscribers.isEmpty()is unnecessary since an emptycurrentSubscribersset means the for-loop simply won't execute♻️ Suggested simplification
// SotW absent detection for full-state types (LDS/CDS) final ResourceParser<?, ?> parser = XdsResourceParserUtil.fromType(type); - if (parser != null && parser.isFullStateOfTheWorld() && - (type == XdsType.LISTENER || type == XdsType.CLUSTER)) { + if (parser != null && parser.isFullStateOfTheWorld()) { final Set<String> currentSubscribers = stateCoordinator.interestedResources(type); - if (!parsedResources.isEmpty() || !currentSubscribers.isEmpty()) { - for (String name : currentSubscribers) { - if (parsedResources.containsKey(name) || invalidResources.containsKey(name)) { - continue; - } - stateCoordinator.onResourceMissing(type, name); + for (String name : currentSubscribers) { + if (parsedResources.containsKey(name) || invalidResources.containsKey(name)) { + continue; } + stateCoordinator.onResourceMissing(type, name); } }🤖 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/SubscriptionCallbacks.java` around lines 82 - 95, The absent-detection logic in SubscriptionCallbacks currently redundantly checks the XdsType and guards on parsedResources/currentSubscribers; change the block to rely on XdsResourceParserUtil.fromType(type) and parser.isFullStateOfTheWorld() only (remove the explicit type check for XdsType.LISTENER/XdsType.CLUSTER) and drop the unnecessary if (!parsedResources.isEmpty() || !currentSubscribers.isEmpty()) guard; keep obtaining currentSubscribers via stateCoordinator.interestedResources(type) and iterate it, calling stateCoordinator.onResourceMissing(type, name) for names not present in parsedResources or invalidResources to preserve behavior.xds/src/main/java/com/linecorp/armeria/xds/XdsExtensionRegistry.java (1)
87-94: Consider logging or failing on duplicate factory registrations.The
registermethod silently overwrites existing entries if the samenameortypeUrlis registered twice. This could hide configuration errors where two factories claim the same identifier.♻️ Suggested improvement
private static void register(XdsExtensionFactory factory, Map<String, XdsExtensionFactory> byName, Map<String, XdsExtensionFactory> byTypeUrl) { - byName.put(factory.name(), factory); + final XdsExtensionFactory prev = byName.put(factory.name(), factory); + if (prev != null) { + logger.warn("Duplicate factory registration for name '{}': {} replaced by {}", + factory.name(), prev.getClass().getName(), factory.getClass().getName()); + } for (String typeUrl : factory.typeUrls()) { - byTypeUrl.put(typeUrl, factory); + final XdsExtensionFactory prevUrl = byTypeUrl.put(typeUrl, factory); + if (prevUrl != null) { + logger.warn("Duplicate factory registration for typeUrl '{}': {} replaced by {}", + typeUrl, prevUrl.getClass().getName(), factory.getClass().getName()); + } } }This would require adding a logger field:
private static final Logger logger = LoggerFactory.getLogger(XdsExtensionRegistry.class);🤖 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/XdsExtensionRegistry.java` around lines 87 - 94, The register method currently overwrites existing entries; add a private static final Logger (LoggerFactory.getLogger(XdsExtensionRegistry.class)) and in register check byName.containsKey(factory.name()) and byTypeUrl.containsKey(typeUrl) before putting; when a duplicate is detected log a clear warning including the conflicting name/typeUrl and both factory identifiers and then fail fast by throwing an IllegalStateException to surface configuration errors instead of silently overwriting; reference the register method, XdsExtensionFactory.name(), XdsExtensionFactory.typeUrls(), and the byName/byTypeUrl maps when implementing the checks.xds-api/src/main/proto/envoy/extensions/transport_sockets/tls/v3/common.proto (1)
338-341: Document canonical IPv6 format forIP_ADDRESSSAN matching.Since
IP_ADDRESSis now explicitly supported, add a brief proto note that IPv6 SANs should use canonical lowercase compressed notation to prevent unexpected mismatch behavior.Suggested doc-only patch
enum SanType { SAN_TYPE_UNSPECIFIED = 0; EMAIL = 1 [(armeria.xds.supported_value) = true]; DNS = 2 [(armeria.xds.supported_value) = true]; URI = 3 [(armeria.xds.supported_value) = true]; + // For IPv6, use canonical lowercase compressed form (e.g. "2001:db8::1") + // to match runtime SAN normalization behavior. IP_ADDRESS = 4 [(armeria.xds.supported_value) = true]; OTHER_NAME = 5; }Based on learnings: In
SanMatcher.java, IPv6 SAN matching intentionally uses canonicalInetAddress.getHostAddress()form, requiring canonical lowercase compressed notation in xDS config.Also applies to: 346-347, 364-365
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xds-api/src/main/proto/envoy/extensions/transport_sockets/tls/v3/common.proto` around lines 338 - 341, Add a proto-side note documenting the canonical IPv6 format required for IP_ADDRESS SAN matching: update the enum block containing EMAIL, DNS, URI, IP_ADDRESS to include a brief comment stating that IPv6 SANs must use canonical lowercase compressed notation (the same form returned by InetAddress.getHostAddress()) to avoid mismatches with SanMatcher.java; reference the IP_ADDRESS enum symbol and mention the behavior in SanMatcher.java that performs matching using InetAddress.getHostAddress() so config authors know to supply canonical IPv6 addresses.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@xds-api/src/main/java/com/linecorp/armeria/xds/api/DefaultXdsValidatorIndex.java`:
- Around line 46-49: The public API DefaultXdsValidatorIndex.assertValid
currently passes a null through to pgvValidator; add an explicit null-safety
check at the start of assertValid(Object message) to reject null inputs (throw a
NullPointerException with a clear message) before calling
pgvValidator.assertValid(message) and
supportedFieldValidator.assertValid(message), so that the method fails fast and
documents the contract at the API boundary.
In
`@xds-api/src/main/java/com/linecorp/armeria/xds/api/SupportedFieldValidator.java`:
- Around line 46-60: The javadoc describes a message-level option
(armeria.xds.supported_fields) but SupportedFieldValidator.supportedFields(...)
only reads per-field (supported) annotations, so message-level declared
supported field numbers must be read too; update supportedFields(...) in
SupportedFieldValidator to also inspect the message descriptor's options for the
repeated int32 armeria.xds.supported_fields extension (merge its values with any
per-field supported flags), treating those numbers as supported field indices
and preventing them from being reported unsupported; ensure you correctly access
the message options extension (the generated extension identifier for
armeria.xds.supported_fields) and handle duplicates and out-of-range numbers
consistently with existing validation logic.
- Around line 101-105: The public method SupportedFieldValidator.assertValid
currently silently ignores a null argument; add an explicit null-check at the
start (e.g., Objects.requireNonNull or equivalent) to throw a
NullPointerException for null inputs so callers fail fast and match
validate(Message) expectations, then continue to cast and call
validate((Message) message) as before.
In `@xds/src/main/java/com/linecorp/armeria/xds/ListenerXdsResource.java`:
- Around line 49-56: The constructor stores downstreamFilters by reference then
computes router from it, so subsequent mutations to the stored list can make
router stale and withRevision reuses the same mutable list; to fix, create an
immutable/defensive copy of downstreamFilters (e.g., via
Collections.unmodifiableList or List.copyOf) as the stored field before calling
findRouter(...) and ensure withRevision(...) also passes that immutable copy (or
creates a fresh immutable copy) so router is always derived from an immutable
list; update the ListenerXdsResource constructor, the downstreamFilters field
initialization, and withRevision() to use the frozen list and compute router
from that frozen list via findRouter.
In `@xds/src/main/java/com/linecorp/armeria/xds/StateCoordinator.java`:
- Around line 66-69: The replay logic currently calls the watcher directly
(inside register -> replayToWatcher), which bypasses the XdsStreamSubscriber
lifecycle and leaves its initial-absent timeout active; update replayToWatcher
(and the other similar replay spot) to invoke the subscriber's handlers instead
of calling the watcher directly—specifically call
XdsStreamSubscriber.onData(...) when delivering a VERSIONED state and
XdsStreamSubscriber.onAbsent(...) when delivering an ABSENT state—or, if you
prefer to keep watcher invocation, cancel the subscriber's initial-absent
timeout before delivering cached state so the subscriber never later fires
onAbsent; apply the same change to the other replay code path referenced in this
file to ensure timeouts are not left running.
---
Duplicate comments:
In
`@xds-api/src/main/java/com/linecorp/armeria/xds/api/StrictXdsValidatorIndex.java`:
- Around line 19-20: StrictXdsValidatorIndex's public API lacks a null-guard:
add an explicit null check (e.g., Objects.requireNonNull(...)) at the start of
the public assertValid(...) method(s) in class StrictXdsValidatorIndex so
assertValid(null) throws immediately rather than delegating to downstream
validators; place the requireNonNull call at the top of the public method(s)
referenced (assertValid) to enforce null-safety for the API boundary.
In
`@xds/src/main/java/com/linecorp/armeria/xds/GrpcConfigSourceStreamFactory.java`:
- Around line 97-103: The code only validates grpc_services[0]; instead iterate
apiConfigSource.getGrpcServicesList() and assert each GrpcService.hasEnvoyGrpc()
(e.g., for (GrpcService gs : grpcServices) checkArgument(gs.hasEnvoyGrpc(),
"Only envoyGrpc is supported for '%s'", configSource)); then keep the existing
firstGrpcService/getEnvoyGrpc() usage to obtain envoyGrpc after the per-entry
validation. This ensures every configured GrpcService is validated, not just the
first.
In `@xds/src/main/java/com/linecorp/armeria/xds/SotwActualStream.java`:
- Around line 153-172: The onError/onCompleted paths must ignore terminal
callbacks if closeStream() already marked the stream completed; add an early
guard in both onError(Throwable) and onCompleted() that returns immediately when
completed is true (before scheduling onto eventLoop) so you don't reschedule
work or call lifecycleObserver.streamError/streamCompleted or
owner.retryOrClose(...) after a local close; reference the existing completed
flag, the onError/onCompleted methods, lifecycleObserver, owner.retryOrClose,
and closeStream() when making this change.
- Around line 145-149: The code in SotwActualStream currently logs and returns
when XdsResourceParserUtil.fromTypeUrl(value.getTypeUrl()) returns null, which
drops the response; instead construct and send a NACK for this
DiscoveryResponse: build an error Status (e.g., INVALID_ARGUMENT) with a
descriptive message including value.getTypeUrl(), then call the class's existing
NACK pathway using the DiscoveryResponse identifiers (value.getVersionInfo(),
value.getNonce(), and optionally resource names from value.getResourcesList()) —
reuse the SotwActualStream method that sends NACKs (the existing
nack/sendNack/onNack handler in this class) so the management server receives an
explicit NACK rather than silently ignoring the unknown type.
---
Nitpick comments:
In
`@xds-api/src/main/proto/envoy/extensions/transport_sockets/tls/v3/common.proto`:
- Around line 338-341: Add a proto-side note documenting the canonical IPv6
format required for IP_ADDRESS SAN matching: update the enum block containing
EMAIL, DNS, URI, IP_ADDRESS to include a brief comment stating that IPv6 SANs
must use canonical lowercase compressed notation (the same form returned by
InetAddress.getHostAddress()) to avoid mismatches with SanMatcher.java;
reference the IP_ADDRESS enum symbol and mention the behavior in SanMatcher.java
that performs matching using InetAddress.getHostAddress() so config authors know
to supply canonical IPv6 addresses.
In `@xds/src/main/java/com/linecorp/armeria/xds/ListenerManager.java`:
- Around line 65-72: The static-listener path duplicates the unpack/resolve
logic; replace the manual calls to XdsUnpackUtil.unpackConnectionManager and
XdsUnpackUtil.resolveDownstreamFilters and the direct construction of
ListenerXdsResource in ListenerManager with a call to
ListenerResourceParser.parse(...) to produce the ListenerXdsResource, then pass
that resource into new ListenerStream(listenerResource, context); locate the
block that currently uses XdsExtensionRegistry registry,
XdsUnpackUtil.unpackConnectionManager(listener,...),
XdsUnpackUtil.resolveDownstreamFilters(...), and ListenerXdsResource(...) and
swap it to use ListenerResourceParser.parse(listener, registry, ...) (matching
the available parse signature) to ensure both paths share the same parsing
logic.
In `@xds/src/main/java/com/linecorp/armeria/xds/SubscriptionCallbacks.java`:
- Around line 82-95: The absent-detection logic in SubscriptionCallbacks
currently redundantly checks the XdsType and guards on
parsedResources/currentSubscribers; change the block to rely on
XdsResourceParserUtil.fromType(type) and parser.isFullStateOfTheWorld() only
(remove the explicit type check for XdsType.LISTENER/XdsType.CLUSTER) and drop
the unnecessary if (!parsedResources.isEmpty() || !currentSubscribers.isEmpty())
guard; keep obtaining currentSubscribers via
stateCoordinator.interestedResources(type) and iterate it, calling
stateCoordinator.onResourceMissing(type, name) for names not present in
parsedResources or invalidResources to preserve behavior.
In `@xds/src/main/java/com/linecorp/armeria/xds/XdsExtensionRegistry.java`:
- Around line 87-94: The register method currently overwrites existing entries;
add a private static final Logger
(LoggerFactory.getLogger(XdsExtensionRegistry.class)) and in register check
byName.containsKey(factory.name()) and byTypeUrl.containsKey(typeUrl) before
putting; when a duplicate is detected log a clear warning including the
conflicting name/typeUrl and both factory identifiers and then fail fast by
throwing an IllegalStateException to surface configuration errors instead of
silently overwriting; reference the register method, XdsExtensionFactory.name(),
XdsExtensionFactory.typeUrls(), and the byName/byTypeUrl maps when implementing
the checks.
🪄 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: 7a43eb57-43a0-41aa-bcbd-4c28685d5027
📒 Files selected for processing (89)
it/xds-client/src/test/resources/META-INF/services/com.linecorp.armeria.xds.validator.XdsValidatorIndexxds-api/src/main/java/com/linecorp/armeria/xds/api/DefaultXdsValidatorIndex.javaxds-api/src/main/java/com/linecorp/armeria/xds/api/IgnoreUnsupportedFieldHandler.javaxds-api/src/main/java/com/linecorp/armeria/xds/api/MeterUnsupportedFieldHandler.javaxds-api/src/main/java/com/linecorp/armeria/xds/api/PgvValidator.javaxds-api/src/main/java/com/linecorp/armeria/xds/api/StrictXdsValidatorIndex.javaxds-api/src/main/java/com/linecorp/armeria/xds/api/SupportedFieldValidator.javaxds-api/src/main/java/com/linecorp/armeria/xds/api/UnsupportedFieldHandler.javaxds-api/src/main/proto/armeria/xds/supported.protoxds-api/src/main/proto/envoy/config/bootstrap/v3/bootstrap.protoxds-api/src/main/proto/envoy/config/cluster/v3/cluster.protoxds-api/src/main/proto/envoy/config/core/v3/address.protoxds-api/src/main/proto/envoy/config/core/v3/base.protoxds-api/src/main/proto/envoy/config/core/v3/config_source.protoxds-api/src/main/proto/envoy/config/core/v3/grpc_service.protoxds-api/src/main/proto/envoy/config/core/v3/health_check.protoxds-api/src/main/proto/envoy/config/endpoint/v3/endpoint.protoxds-api/src/main/proto/envoy/config/endpoint/v3/endpoint_components.protoxds-api/src/main/proto/envoy/config/listener/v3/api_listener.protoxds-api/src/main/proto/envoy/config/listener/v3/listener.protoxds-api/src/main/proto/envoy/config/route/v3/route.protoxds-api/src/main/proto/envoy/config/route/v3/route_components.protoxds-api/src/main/proto/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.protoxds-api/src/main/proto/envoy/extensions/transport_sockets/tls/v3/common.protoxds-api/src/main/proto/envoy/extensions/transport_sockets/tls/v3/secret.protoxds-api/src/main/proto/envoy/extensions/transport_sockets/tls/v3/tls.protoxds-api/src/main/proto/envoy/type/matcher/v3/regex.protoxds-api/src/main/proto/envoy/type/matcher/v3/string.protoxds-api/src/main/proto/envoy/type/v3/percent.protoxds-api/src/main/proto/envoy/type/v3/range.protoxds-api/src/test/java/com/linecorp/armeria/xds/api/MeterUnsupportedFieldHandlerTest.javaxds-api/src/test/java/com/linecorp/armeria/xds/api/SupportedFieldValidatorTest.javaxds-api/src/test/proto/armeria/xds/testing/test_supported.protoxds-validator/src/main/java/com/linecorp/armeria/xds/validator/XdsValidatorIndex.javaxds/src/main/java/com/linecorp/armeria/xds/ClusterResourceParser.javaxds/src/main/java/com/linecorp/armeria/xds/ClusterXdsResource.javaxds/src/main/java/com/linecorp/armeria/xds/ConfigSourceClient.javaxds/src/main/java/com/linecorp/armeria/xds/ConfigSourceHandler.javaxds/src/main/java/com/linecorp/armeria/xds/ConfigSourceSubscription.javaxds/src/main/java/com/linecorp/armeria/xds/ControlPlaneClientManager.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/EndpointResourceParser.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/GrpcConfigSourceStreamFactory.javaxds/src/main/java/com/linecorp/armeria/xds/HttpConnectionManagerFactory.javaxds/src/main/java/com/linecorp/armeria/xds/ListenerManager.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/ListenerXdsResource.javaxds/src/main/java/com/linecorp/armeria/xds/PathSotwConfigSourceSubscriptionFactory.javaxds/src/main/java/com/linecorp/armeria/xds/RawBufferTransportSocketFactory.javaxds/src/main/java/com/linecorp/armeria/xds/ResourceNodeAdapter.javaxds/src/main/java/com/linecorp/armeria/xds/ResourceParser.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/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/SotwActualStream.javaxds/src/main/java/com/linecorp/armeria/xds/SotwConfigSourceSubscriptionFactory.javaxds/src/main/java/com/linecorp/armeria/xds/SotwSubscriptionCallbacks.javaxds/src/main/java/com/linecorp/armeria/xds/StateCoordinator.javaxds/src/main/java/com/linecorp/armeria/xds/SubscriptionCallbacks.javaxds/src/main/java/com/linecorp/armeria/xds/SubscriptionContext.javaxds/src/main/java/com/linecorp/armeria/xds/SubscriptionHandler.javaxds/src/main/java/com/linecorp/armeria/xds/TransportSocketFactory.javaxds/src/main/java/com/linecorp/armeria/xds/TransportSocketStream.javaxds/src/main/java/com/linecorp/armeria/xds/UpstreamTlsTransportSocketFactory.javaxds/src/main/java/com/linecorp/armeria/xds/VirtualHostXdsResource.javaxds/src/main/java/com/linecorp/armeria/xds/XdsBootstrapImpl.javaxds/src/main/java/com/linecorp/armeria/xds/XdsClusterManager.javaxds/src/main/java/com/linecorp/armeria/xds/XdsConverterUtil.javaxds/src/main/java/com/linecorp/armeria/xds/XdsExtensionFactory.javaxds/src/main/java/com/linecorp/armeria/xds/XdsExtensionRegistry.javaxds/src/main/java/com/linecorp/armeria/xds/XdsResourceParserUtil.javaxds/src/main/java/com/linecorp/armeria/xds/XdsResourceValidator.javaxds/src/main/java/com/linecorp/armeria/xds/XdsUnpackUtil.javaxds/src/main/java/com/linecorp/armeria/xds/XdsValidatorIndexRegistry.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/test/java/com/linecorp/armeria/xds/XdsClientCleanupTest.javaxds/src/test/java/com/linecorp/armeria/xds/XdsExtensionRegistryTest.javaxds/src/test/java/com/linecorp/armeria/xds/XdsResourceValidatorTest.javaxds/src/test/java/com/linecorp/armeria/xds/XdsValidatorIndexRegistryTest.javaxds/src/test/resources/META-INF/services/com.linecorp.armeria.xds.validator.XdsValidatorIndex
💤 Files with no reviewable changes (5)
- xds/src/test/java/com/linecorp/armeria/xds/XdsValidatorIndexRegistryTest.java
- xds/src/main/java/com/linecorp/armeria/xds/filter/HttpFilterFactoryRegistry.java
- xds/src/main/java/com/linecorp/armeria/xds/XdsConverterUtil.java
- xds/src/main/java/com/linecorp/armeria/xds/XdsValidatorIndexRegistry.java
- xds/src/main/java/com/linecorp/armeria/xds/ConfigSourceClient.java
✅ Files skipped from review due to trivial changes (17)
- it/xds-client/src/test/resources/META-INF/services/com.linecorp.armeria.xds.validator.XdsValidatorIndex
- xds/src/main/java/com/linecorp/armeria/xds/XdsClusterManager.java
- xds/src/test/resources/META-INF/services/com.linecorp.armeria.xds.validator.XdsValidatorIndex
- xds-validator/src/main/java/com/linecorp/armeria/xds/validator/XdsValidatorIndex.java
- xds/src/main/java/com/linecorp/armeria/xds/SotwSubscriptionCallbacks.java
- xds/src/main/java/com/linecorp/armeria/xds/XdsExtensionFactory.java
- xds/src/main/java/com/linecorp/armeria/xds/SubscriptionHandler.java
- xds/src/main/java/com/linecorp/armeria/xds/ConfigSourceSubscription.java
- xds-api/src/test/java/com/linecorp/armeria/xds/api/MeterUnsupportedFieldHandlerTest.java
- xds-api/src/main/proto/armeria/xds/supported.proto
- xds-api/src/main/java/com/linecorp/armeria/xds/api/PgvValidator.java
- xds-api/src/main/proto/envoy/type/v3/percent.proto
- xds/src/main/java/com/linecorp/armeria/xds/PathSotwConfigSourceSubscriptionFactory.java
- xds-api/src/main/proto/envoy/config/route/v3/route.proto
- xds-api/src/main/proto/envoy/config/endpoint/v3/endpoint.proto
- xds-api/src/test/proto/armeria/xds/testing/test_supported.proto
- xds/src/main/java/com/linecorp/armeria/xds/DeltaActualStream.java
🚧 Files skipped from review as they are similar to previous changes (34)
- xds/src/main/java/com/linecorp/armeria/xds/SubscriptionContext.java
- xds/src/main/java/com/linecorp/armeria/xds/ListenerSnapshot.java
- xds/src/main/java/com/linecorp/armeria/xds/ResourceNodeAdapter.java
- xds-api/src/main/proto/envoy/config/listener/v3/api_listener.proto
- xds/src/main/java/com/linecorp/armeria/xds/EndpointResourceParser.java
- xds/src/main/java/com/linecorp/armeria/xds/SecretResourceParser.java
- xds/src/main/java/com/linecorp/armeria/xds/RouteStream.java
- xds-api/src/main/java/com/linecorp/armeria/xds/api/MeterUnsupportedFieldHandler.java
- xds/src/main/java/com/linecorp/armeria/xds/RouteResourceParser.java
- xds/src/main/java/com/linecorp/armeria/xds/HttpConnectionManagerFactory.java
- xds-api/src/main/proto/envoy/type/v3/range.proto
- xds/src/main/java/com/linecorp/armeria/xds/XdsBootstrapImpl.java
- xds/src/main/java/com/linecorp/armeria/xds/RawBufferTransportSocketFactory.java
- xds-api/src/main/proto/envoy/config/listener/v3/listener.proto
- xds/src/main/java/com/linecorp/armeria/xds/RouteEntry.java
- xds/src/main/java/com/linecorp/armeria/xds/SotwConfigSourceSubscriptionFactory.java
- xds/src/main/java/com/linecorp/armeria/xds/RouteXdsResource.java
- xds/src/main/java/com/linecorp/armeria/xds/XdsResourceValidator.java
- xds/src/main/java/com/linecorp/armeria/xds/ConfigSourceHandler.java
- xds/src/main/java/com/linecorp/armeria/xds/XdsResourceParserUtil.java
- xds/src/test/java/com/linecorp/armeria/xds/XdsResourceValidatorTest.java
- xds/src/main/java/com/linecorp/armeria/xds/XdsUnpackUtil.java
- xds-api/src/main/java/com/linecorp/armeria/xds/api/UnsupportedFieldHandler.java
- xds-api/src/main/proto/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto
- xds-api/src/main/proto/envoy/type/matcher/v3/string.proto
- xds/src/main/java/com/linecorp/armeria/xds/ClusterResourceParser.java
- xds/src/main/java/com/linecorp/armeria/xds/VirtualHostXdsResource.java
- xds-api/src/main/proto/envoy/config/bootstrap/v3/bootstrap.proto
- xds/src/test/java/com/linecorp/armeria/xds/XdsExtensionRegistryTest.java
- xds-api/src/main/proto/envoy/config/core/v3/base.proto
- xds/src/main/java/com/linecorp/armeria/xds/EndpointXdsResource.java
- xds-api/src/main/proto/envoy/config/core/v3/health_check.proto
- xds/src/main/java/com/linecorp/armeria/xds/FilterUtil.java
- xds-api/src/main/proto/envoy/extensions/transport_sockets/tls/v3/tls.proto
Comment on lines
46
to
+49
| @Override | ||
| public void assertValid(Object message) { | ||
| try { | ||
| validatorFor(message).assertValid(message); | ||
| } catch (ValidationException e) { | ||
| throw new IllegalArgumentException(e); | ||
| } | ||
| pgvValidator.assertValid(message); | ||
| supportedFieldValidator.assertValid(message); |
Contributor
There was a problem hiding this comment.
Reject null at the API boundary.
assertValid(null) currently fails somewhere inside PgvValidator, which makes the public API harder to reason about. Add an explicit null check here.
Proposed fix
`@Override`
public void assertValid(Object message) {
+ java.util.Objects.requireNonNull(message, "message");
pgvValidator.assertValid(message);
supportedFieldValidator.assertValid(message);
}As per coding guidelines, "Use null-safety conventions: explicit requireNonNull checks for user-facing public methods; prefer @Nullable for nullable params/returns (no @Nonnull)."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @Override | |
| public void assertValid(Object message) { | |
| try { | |
| validatorFor(message).assertValid(message); | |
| } catch (ValidationException e) { | |
| throw new IllegalArgumentException(e); | |
| } | |
| pgvValidator.assertValid(message); | |
| supportedFieldValidator.assertValid(message); | |
| `@Override` | |
| public void assertValid(Object message) { | |
| java.util.Objects.requireNonNull(message, "message"); | |
| pgvValidator.assertValid(message); | |
| supportedFieldValidator.assertValid(message); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@xds-api/src/main/java/com/linecorp/armeria/xds/api/DefaultXdsValidatorIndex.java`
around lines 46 - 49, The public API DefaultXdsValidatorIndex.assertValid
currently passes a null through to pgvValidator; add an explicit null-safety
check at the start of assertValid(Object message) to reject null inputs (throw a
NullPointerException with a clear message) before calling
pgvValidator.assertValid(message) and
supportedFieldValidator.assertValid(message), so that the method fails fast and
documents the contract at the API boundary.
Comment on lines
+46
to
+60
| * <p>If inline annotations cause frequent merge conflicts when syncing upstream proto files, | ||
| * an alternative is to use a message-level option listing supported field numbers: | ||
| * <pre>{@code | ||
| * extend google.protobuf.MessageOptions { | ||
| * repeated int32 supported_fields = 50000; | ||
| * } | ||
| * | ||
| * message StringMatcher { | ||
| * option (armeria.xds.supported_fields) = 1; // exact | ||
| * option (armeria.xds.supported_fields) = 2; // prefix | ||
| * string exact = 1; | ||
| * string prefix = 2 [(validate.rules).string = {min_len: 1}]; | ||
| * } | ||
| * }</pre> | ||
| * This keeps upstream field declarations untouched, at the cost of reduced readability. |
Contributor
There was a problem hiding this comment.
supported_fields is documented but never consulted.
The Javadoc advertises a message-level supported_fields option, but supportedFields(...) only inspects per-field (supported) options. Any proto that follows the documented message-option pattern will still be reported unsupported.
Also applies to: 209-218
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@xds-api/src/main/java/com/linecorp/armeria/xds/api/SupportedFieldValidator.java`
around lines 46 - 60, The javadoc describes a message-level option
(armeria.xds.supported_fields) but SupportedFieldValidator.supportedFields(...)
only reads per-field (supported) annotations, so message-level declared
supported field numbers must be read too; update supportedFields(...) in
SupportedFieldValidator to also inspect the message descriptor's options for the
repeated int32 armeria.xds.supported_fields extension (merge its values with any
per-field supported flags), treating those numbers as supported field indices
and preventing them from being reported unsupported; ensure you correctly access
the message options extension (the generated extension identifier for
armeria.xds.supported_fields) and handle duplicates and out-of-range numbers
consistently with existing validation logic.
Comment on lines
+101
to
+105
| public void assertValid(Object message) { | ||
| if (message instanceof Message) { | ||
| validate((Message) message); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Fail fast on null in assertValid.
assertValid(null) is a silent no-op today, which makes this public API inconsistent with validate(Message) and hides caller mistakes.
Suggested fix
public void assertValid(Object message) {
+ requireNonNull(message, "message");
if (message instanceof Message) {
validate((Message) message);
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@xds-api/src/main/java/com/linecorp/armeria/xds/api/SupportedFieldValidator.java`
around lines 101 - 105, The public method SupportedFieldValidator.assertValid
currently silently ignores a null argument; add an explicit null-check at the
start (e.g., Objects.requireNonNull or equivalent) to throw a
NullPointerException for null inputs so callers fail fast and match
validate(Message) expectations, then continue to cast and call
validate((Message) message) as before.
Comment on lines
+49
to
+56
| ListenerXdsResource(Listener listener, @Nullable HttpConnectionManager connectionManager, | ||
| List<XdsHttpFilter> downstreamFilters, | ||
| String version, long revision) { | ||
| super(version, revision); | ||
| XdsValidatorIndexRegistry.assertValid(listener); | ||
| this.listener = listener; | ||
| this.connectionManager = connectionManager; | ||
| this.downstreamFilters = downstreamFilters; | ||
| this.router = findRouter(downstreamFilters); |
Contributor
There was a problem hiding this comment.
Freeze downstreamFilters before deriving router.
router is computed once, but downstreamFilters is stored and exposed by reference. A later mutation makes router() stale relative to downstreamFilters(), and withRevision() keeps reusing the same mutable list.
Proposed fix
- this.downstreamFilters = downstreamFilters;
- this.router = findRouter(downstreamFilters);
+ this.downstreamFilters = ImmutableList.copyOf(downstreamFilters);
+ this.router = findRouter(this.downstreamFilters);📝 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
| ListenerXdsResource(Listener listener, @Nullable HttpConnectionManager connectionManager, | |
| List<XdsHttpFilter> downstreamFilters, | |
| String version, long revision) { | |
| super(version, revision); | |
| XdsValidatorIndexRegistry.assertValid(listener); | |
| this.listener = listener; | |
| this.connectionManager = connectionManager; | |
| this.downstreamFilters = downstreamFilters; | |
| this.router = findRouter(downstreamFilters); | |
| ListenerXdsResource(Listener listener, `@Nullable` HttpConnectionManager connectionManager, | |
| List<XdsHttpFilter> downstreamFilters, | |
| String version, long revision) { | |
| super(version, revision); | |
| this.listener = listener; | |
| this.connectionManager = connectionManager; | |
| this.downstreamFilters = ImmutableList.copyOf(downstreamFilters); | |
| this.router = findRouter(this.downstreamFilters); |
🤖 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 49 - 56, The constructor stores downstreamFilters by reference then
computes router from it, so subsequent mutations to the stored list can make
router stale and withRevision reuses the same mutable list; to fix, create an
immutable/defensive copy of downstreamFilters (e.g., via
Collections.unmodifiableList or List.copyOf) as the stored field before calling
findRouter(...) and ensure withRevision(...) also passes that immutable copy (or
creates a fresh immutable copy) so router is always derived from an immutable
list; update the ListenerXdsResource constructor, the downstreamFilters field
initialization, and withRevision() to use the frozen list and compute router
from that frozen list via findRouter.
Comment on lines
+66
to
+69
| <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.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== SubscriberStorage register/unregister =="
fd -p 'SubscriberStorage.java' | while read -r file; do
echo "-- $file --"
rg -n -C4 'register\(|unregister\(' "$file"
done
echo
echo "== XdsStreamSubscriber lifecycle =="
fd -p 'XdsStreamSubscriber.java' | while read -r file; do
echo "-- $file --"
rg -n -C4 'class XdsStreamSubscriber|registerWatcher\(|onData\(|onAbsent\(|onError\(|timeout|schedule' "$file"
doneRepository: line/armeria
Length of output: 4000
🏁 Script executed:
fd -p 'StateCoordinator.java' | head -1 | xargs wc -lRepository: line/armeria
Length of output: 124
🏁 Script executed:
fd -p 'StateCoordinator.java' | head -1 | xargs cat -n | sed -n '1,200p'Repository: line/armeria
Length of output: 7150
🏁 Script executed:
fd -p 'StateCoordinator.java' | head -1 | xargs rg -n 'replayToWatcher|register\(' -A 10 -B 2Repository: line/armeria
Length of output: 1631
Replay paths should invoke subscriber state methods, not watcher directly.
When register() creates a fresh XdsStreamSubscriber, it unconditionally schedules an initial-absent timeout in the constructor. However, replayToWatcher() immediately delivers cached VERSIONED or ABSENT state directly to the watcher, bypassing the subscriber's onData()/onAbsent() methods. This leaves the timeout running, so if the server never sends a new update, the timeout fires and invokes subscriber.onAbsent(), causing the watcher to receive an unexpected onResourceDoesNotExist() notification after already receiving a cached onChanged() or onResourceDoesNotExist().
Replay should either call subscriber.onData() / subscriber.onAbsent() instead of invoking the watcher directly, or cancel the timeout when cached state is delivered.
Also applies to: 135-153
🤖 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 66 - 69, The replay logic currently calls the watcher directly (inside
register -> replayToWatcher), which bypasses the XdsStreamSubscriber lifecycle
and leaves its initial-absent timeout active; update replayToWatcher (and the
other similar replay spot) to invoke the subscriber's handlers instead of
calling the watcher directly—specifically call XdsStreamSubscriber.onData(...)
when delivering a VERSIONED state and XdsStreamSubscriber.onAbsent(...) when
delivering an ABSENT state—or, if you prefer to keep watcher invocation, cancel
the subscriber's initial-absent timeout before delivering cached state so the
subscriber never later fires onAbsent; apply the same change to the other replay
code path referenced in this file to ensure timeouts are not left running.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation:
Explain why you're making this change and what problem you're trying to solve.
Modifications:
Result: