Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (3)
📝 WalkthroughWalkthroughAdds protobuf extensions for marking supported fields/enum values and a new SupportedFieldValidator that traverses protobuf Messages to detect unsupported fields/enum values, delegating reporting to a configurable UnsupportedFieldHandler (warn/reject/ignore). Includes a no-op handler, tests, test protos, and a minor SPI annotation update. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as "Client"
participant Validator as "SupportedFieldValidator\n(runs validation)"
participant Cache as "Descriptor Cache"
participant Message as "protobuf.Message"
participant Handler as "UnsupportedFieldHandler"
participant Logger as "Logger/Throw"
Client->>Validator: validate(message)
Validator->>Message: getDescriptor()/getAllFields()
Validator->>Cache: lookupSupportedFields(descriptor)
loop for each field
Validator->>Validator: build fieldPath
alt field not supported
Validator->>Handler: handle(descriptorName, fieldPath, value)
Handler->>Logger: warn / throw / nop
else supported and is message/map/enum
Validator->>Message: recurse into nested message / iterate map entries
Validator->>Cache: (re)use supported set for nested descriptor
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
xds-api/src/test/java/com/linecorp/armeria/xds/api/SupportedFieldValidatorTest.java (1)
206-239: These tests don't assert the behaviors their names claim.
rejectFailsFast()only proves that someIllegalArgumentExceptionis thrown, andignoreEarlyExit()only proves thatignore()suppresses errors. Both still pass if fail-fast ordering or the early-return optimization regresses. Either strengthen the assertions to cover those behaviors, or rename the tests to the weaker contract they actually verify.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xds-api/src/test/java/com/linecorp/armeria/xds/api/SupportedFieldValidatorTest.java` around lines 206 - 239, The tests currently only check for any exception or lack thereof; strengthen them to assert the specific fast-fail/early-exit behaviors: for rejectFailsFast(), construct a TestCluster that has a top-level unsupported field (setUnsupportedField("bad")) AND an invalid nested field in TestOutlierDetection (e.g., setConsecutiveErrors(-1)) then call SupportedFieldValidator.of(UnsupportedFieldHandler.reject()).validate(cluster) and assert that an IllegalArgumentException is thrown whose message references the top-level unsupported field (e.g., "unsupportedField") and does NOT reference the nested field (e.g., "consecutiveErrors"), proving fail-fast; for ignoreEarlyExit(), similarly build a cluster with setUnsupportedField("bad") and an invalid nested outlier (setConsecutiveErrors(-1)), use SupportedFieldValidator.of(UnsupportedFieldHandler.ignore()) and assert that validate(cluster) completes without throwing (verifying early exit/skip), referencing SupportedFieldValidator.validate, UnsupportedFieldHandler.reject()/ignore(), TestCluster and TestOutlierDetection to locate the code.xds-api/src/main/java/com/linecorp/armeria/xds/api/UnsupportedFieldHandler.java (1)
34-63: Move the logger field into thewarn()method to avoid exposing an internal implementation detail through the public interface.Fields declared in an interface are implicitly
public static final, makingUnsupportedFieldHandler.loggerpart of the public API surface even though it is only used internally bywarn(). Keep the logger local to the method instead.♻️ Proposed fix
- Logger logger = LoggerFactory.getLogger(UnsupportedFieldHandler.class); - /** * Returns a handler that logs a warning for each unsupported field path. */ static UnsupportedFieldHandler warn() { + final Logger logger = LoggerFactory.getLogger(UnsupportedFieldHandler.class); return (descriptorName, fieldPath, value) -> logger.warn("Unsupported xDS field detected in {}: {}", descriptorName, fieldPath); }🤖 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/UnsupportedFieldHandler.java` around lines 34 - 63, The interface currently exposes a public static final Logger field named logger, leaking an internal implementation detail; move the logger declaration inside the static factory method warn() so the logger becomes a private local variable scoped to warn(), update warn() to create and use that local logger (keep the same log message), and remove the top-level Logger field from UnsupportedFieldHandler to avoid exposing it via the public interface.
🤖 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-validator/src/main/java/com/linecorp/armeria/xds/validator/XdsValidatorIndex.java`:
- Line 2: Revert the header change so the original copyright year is preserved:
update the top-of-file header in XdsValidatorIndex.java back to the original LY
Corporation year (keep the existing LY header unchanged) instead of modifying it
to 2026; locate the file/class XdsValidatorIndex and restore the prior copyright
line.
---
Nitpick comments:
In
`@xds-api/src/main/java/com/linecorp/armeria/xds/api/UnsupportedFieldHandler.java`:
- Around line 34-63: The interface currently exposes a public static final
Logger field named logger, leaking an internal implementation detail; move the
logger declaration inside the static factory method warn() so the logger becomes
a private local variable scoped to warn(), update warn() to create and use that
local logger (keep the same log message), and remove the top-level Logger field
from UnsupportedFieldHandler to avoid exposing it via the public interface.
In
`@xds-api/src/test/java/com/linecorp/armeria/xds/api/SupportedFieldValidatorTest.java`:
- Around line 206-239: The tests currently only check for any exception or lack
thereof; strengthen them to assert the specific fast-fail/early-exit behaviors:
for rejectFailsFast(), construct a TestCluster that has a top-level unsupported
field (setUnsupportedField("bad")) AND an invalid nested field in
TestOutlierDetection (e.g., setConsecutiveErrors(-1)) then call
SupportedFieldValidator.of(UnsupportedFieldHandler.reject()).validate(cluster)
and assert that an IllegalArgumentException is thrown whose message references
the top-level unsupported field (e.g., "unsupportedField") and does NOT
reference the nested field (e.g., "consecutiveErrors"), proving fail-fast; for
ignoreEarlyExit(), similarly build a cluster with setUnsupportedField("bad") and
an invalid nested outlier (setConsecutiveErrors(-1)), use
SupportedFieldValidator.of(UnsupportedFieldHandler.ignore()) and assert that
validate(cluster) completes without throwing (verifying early exit/skip),
referencing SupportedFieldValidator.validate,
UnsupportedFieldHandler.reject()/ignore(), TestCluster and TestOutlierDetection
to locate the code.
🪄 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: a5ec0c61-91b3-4da2-9968-1fea783b6e74
📒 Files selected for processing (7)
xds-api/src/main/java/com/linecorp/armeria/xds/api/IgnoreUnsupportedFieldHandler.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/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.java
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6722 +/- ##
============================================
- Coverage 74.46% 0 -74.47%
============================================
Files 1963 0 -1963
Lines 82437 0 -82437
Branches 10764 0 -10764
============================================
- Hits 61385 0 -61385
+ Misses 15918 0 -15918
+ Partials 5134 0 -5134 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
xds-api/src/main/java/com/linecorp/armeria/xds/api/SupportedFieldValidator.java (1)
131-146: Enum values in map fields are not validated.When a map field has an enum as its value type, the condition
valueField.getJavaType() == FieldDescriptor.JavaType.MESSAGEis false, so the loop is skipped andcontinueexecutes immediately. This means enum values within maps don't get checked against(armeria.xds.supported_value).If this is intentional for v1, consider adding a comment; otherwise, add enum handling for map values.
♻️ Suggested enhancement to validate enum map values
if (fd.isMapField()) { final FieldDescriptor valueField = fd.getMessageType().findFieldByNumber(2); if (valueField != null && valueField.getJavaType() == FieldDescriptor.JavaType.MESSAGE) { int i = 0; for (Message mapEntry : (Iterable<Message>) value) { final Object mapValue = mapEntry.getField(valueField); if (mapValue instanceof Message) { doValidate((Message) mapValue, descriptorName, fieldPath + '[' + i + "].value"); } i++; } + } else if (valueField != null && + valueField.getJavaType() == FieldDescriptor.JavaType.ENUM && + !unsupportedPackage(valueField.getEnumType().getFile().getPackage())) { + int i = 0; + for (Message mapEntry : (Iterable<Message>) value) { + final EnumValueDescriptor ev = (EnumValueDescriptor) mapEntry.getField(valueField); + if (unsupportedEnumValue(ev)) { + handler.handle(descriptorName, fieldPath + '[' + i + "].value", ev); + } + i++; + } } continue; }🤖 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 131 - 146, The map-field branch in SupportedFieldValidator skips non-MESSAGE value types, so enum map values aren’t validated; update the map handling inside SupportedFieldValidator (the block that inspects fd.isMapField()) to also handle valueField.getJavaType() == FieldDescriptor.JavaType.ENUM: iterate the same map entries, extract mapValue from mapEntry.getField(valueField), detect enum values (e.g., EnumValueDescriptor or the enum representation returned by reflection), and invoke the same enum-validation logic used elsewhere in this class (the code path used for single enum fields) to check against the (armeria.xds.supported_value) annotation; keep the existing MESSAGE handling and only add an ENUM branch before the continue.
🤖 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-api/src/main/java/com/linecorp/armeria/xds/api/SupportedFieldValidator.java`:
- Around line 131-146: The map-field branch in SupportedFieldValidator skips
non-MESSAGE value types, so enum map values aren’t validated; update the map
handling inside SupportedFieldValidator (the block that inspects
fd.isMapField()) to also handle valueField.getJavaType() ==
FieldDescriptor.JavaType.ENUM: iterate the same map entries, extract mapValue
from mapEntry.getField(valueField), detect enum values (e.g.,
EnumValueDescriptor or the enum representation returned by reflection), and
invoke the same enum-validation logic used elsewhere in this class (the code
path used for single enum fields) to check against the
(armeria.xds.supported_value) annotation; keep the existing MESSAGE handling and
only add an ENUM branch before the continue.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bbf528ee-584f-454f-9de2-b8446dd37159
📒 Files selected for processing (3)
xds-api/src/main/java/com/linecorp/armeria/xds/api/SupportedFieldValidator.javaxds-api/src/main/java/com/linecorp/armeria/xds/api/UnsupportedFieldHandler.javaxds-api/src/test/java/com/linecorp/armeria/xds/api/SupportedFieldValidatorTest.java
✅ Files skipped from review due to trivial changes (1)
- xds-api/src/test/java/com/linecorp/armeria/xds/api/SupportedFieldValidatorTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- xds-api/src/main/java/com/linecorp/armeria/xds/api/UnsupportedFieldHandler.java
ikhoon
approved these changes
Apr 15, 2026
| void handle(String descriptorName, String fieldPath, Object value); | ||
|
|
||
| /** | ||
| * Returns a composed handler that first invokes this handler, then the {@code after} handler. |
Contributor
There was a problem hiding this comment.
Noted that after won't be invoked if the first handler rejects the unsupported field.
Contributor
Author
|
Gentle ping in case this PR was forgotten @minwoox 🙇 |
minwoox
approved these changes
Apr 21, 2026
| } | ||
|
|
||
| private static boolean unsupportedPackage(String pkg) { | ||
| return !(pkg.startsWith("envoy.") || pkg.startsWith("xds.") || pkg.startsWith("armeria.")); |
Contributor
There was a problem hiding this comment.
Is there any chance that a user wants to customize this?
Contributor
Author
There was a problem hiding this comment.
This may be possible if users want to validate their custom proto, but we can always create an extension point later on rather than prematurely adding APIs.
jrhee17
added a commit
that referenced
this pull request
May 4, 2026
Motivation: The `(armeria.xds.supported)` proto annotation was introduced in #6722 to detect unsupported xDS field usage, but the annotation was not yet applied to the proto field definitions. Additionally, `XdsValidatorIndex.assertValid()` accepted `Object` instead of the more type-safe `Message`, and a `StrictXdsValidatorIndex` (which collects all violations and throws a single exception) was missing for use in test environments. Modifications: - Changed `XdsValidatorIndex.assertValid(Object)` to `assertValid(Message)` for type safety, and updated all call sites (`XdsValidatorIndexRegistry`, `XdsResourceValidator`, `XdsExtensionRegistry`, `DefaultXdsValidatorIndex`). - Added `protobuf-java` dependency to `xds-validator` module to support the `Message` parameter type. - Extracted pgv validation logic from `DefaultXdsValidatorIndex` into a dedicated `PgvValidator` class. - Refactored `DefaultXdsValidatorIndex` to compose `PgvValidator` and `SupportedFieldValidator` (with `warn()` handler). - Added `StrictXdsValidatorIndex` with priority `1` for test environments, which collects all unsupported field violations and reports them in a single `IllegalArgumentException`. - Annotated supported fields with `(armeria.xds.supported) = true` across all xDS proto files. Result: - In production, `DefaultXdsValidatorIndex` logs warnings for unsupported xDS fields via `SupportedFieldValidator`. - In test environments (where `StrictXdsValidatorIndex` is on the classpath via SPI), unsupported field usage fails fast with a clear error listing all violations. - Proto field annotations serve as living documentation of which xDS fields Armeria actively supports.
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
3 participants
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:
Armeria's xDS implementation supports a subset of envoy proto fields, but there is no
mechanism to detect when a user's xDS configuration sets fields that Armeria silently ignores.
This can lead to confusing behavior where config appears correct but has no effect.
Modifications:
armeria/xds/supported.protowith custom proto extensions:(armeria.xds.supported)for field options(armeria.xds.supported_value)for enum value optionsSupportedFieldValidatorthat recursively walks protobuf messages and reportsany set field lacking the
(armeria.xds.supported) = trueannotation$.edsConfig.serviceName)google.protobuf, etc.) to avoid false positivesUnsupportedFieldHandlerstrategy interface with built-in implementations:warn()— logs unsupported fieldsreject()— throws on first unsupported fieldignore()— no-op sentinel enabling early-exit optimizationandThen()— composition supportIgnoreUnsupportedFieldHandleras a package-private enum singleton@FunctionalInterfacetoXdsValidatorIndextest_supported.proto) andSupportedFieldValidatorTestResult:
This PR does not wire the validator into the production path or annotate the
envoy proto files — those will follow in subsequent PRs.