Skip to content

Fix minor xDS bugs in filter chain TLS matching, locality routing, and state replay - #6887

Merged
jrhee17 merged 3 commits into
line:mainfrom
jrhee17:feat/xds-minor-bugs
Jul 29, 2026
Merged

Fix minor xDS bugs in filter chain TLS matching, locality routing, and state replay#6887
jrhee17 merged 3 commits into
line:mainfrom
jrhee17:feat/xds-minor-bugs

Conversation

@jrhee17

@jrhee17 jrhee17 commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Motivation:

While implementing the xds example, several minor bugs were discovered in the xDS module:

  1. The xDS server plugin did not verify that a matched filter chain's TLS configuration is consistent with the actual connection protocol. A TLS-configured filter chain would incorrectly accept plaintext connections and vice versa.
  2. The locality routing percentage calculation used 100.0 instead of 1.0, producing inconsistent routing percentages.
  3. When a new watcher registered in StateCoordinator and a cached resource already existed, the cached value was replayed directly to the individual watcher rather than through the CompositeSnapshotWatcher. Since CompositeSnapshotWatcher tracks whether an update has been received, bypassing it caused it to still consider the resource missing and emit a spurious MissingXdsResourceException.

Misc) Added a XdsResourceReader.typeRegistry() public API so JsonFormat.Printer can also use loaded protos

Modifications:

  • Added a TLS/plaintext consistency check in XdsServerPlugin that rejects connections when the matched filter chain's TLS configuration doesn't match the session protocol (e.g., plaintext request on a TLS-required chain, or HTTPS request on a plaintext-only chain).
  • Fixed the locality routing percentage multiplier from 100.0 to 1.0 in LocalityRoutingStateFactory.
  • Updated StateCoordinator.replayToWatcher() to replay cached resources through CompositeSnapshotWatcher for new subscribers, so that the composite correctly records the update and avoids emitting a false MissingXdsResourceException.
  • Exposed XdsResourceReader.typeRegistry() as a public API so that external code can create a JsonFormat.Printer that correctly serializes Any-wrapped xDS messages.
  • Added integration tests (tlsFilterChainRejectsPlaintextConnection, plaintextFilterChainRejectsHttpsConnection) to verify that mismatched TLS/plaintext connections are properly rejected.

Result:

  • xDS server-side filter chain matching now correctly rejects connections where the protocol doesn't match the filter chain's TLS configuration.
  • Locality-aware routing uses correct percentages for routing decisions.
  • Watchers no longer receive false MissingXdsResourceException when a cached resource exists at the time of registration for path-type config sources

@jrhee17 jrhee17 added this to the 1.41.0 milestone Jul 28, 2026
@jrhee17 jrhee17 added the defect label Jul 28, 2026
@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Adds cached xDS type-registry access, adjusts subscriber replay and locality percentage handling, and validates TLS consistency during server filter-chain acceptance with integration tests for TLS and plaintext default chains. The unmanaged-port test now targets its explicitly configured port.

Changes

xDS behavior updates

Layer / File(s) Summary
Resource type registry and parsing
xds/src/main/java/com/linecorp/armeria/xds/XdsResourceReader.java
Caches a TypeRegistry, exposes it through typeRegistry(), and uses it for string-based parsing.
Subscriber replay coordination
xds/src/main/java/com/linecorp/armeria/xds/StateCoordinator.java
Passes new-subscriber state into cached watcher replay and updates the appropriate watcher.
Locality percentage calculation
xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/LocalityRoutingStateFactory.java
Changes the direct locality routing percentage returned by the routing state.
TLS filter-chain acceptance
xds/src/main/java/com/linecorp/armeria/xds/server/XdsServerPlugin.java, it/xds-client/src/test/java/com/linecorp/armeria/xds/it/server/ServerFilterChainMatchTest.java
Rejects TLS/session-protocol mismatches and tests TLS-required and plaintext default filter chains.
Unmanaged port test wiring
it/xds-client/src/test/java/com/linecorp/armeria/xds/it/server/ServerXdsTest.java
Configures an explicit unmanaged port and sends the unaffected-port request directly to it.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant XdsServerPlugin
  participant ExistingAcceptor
  Client->>XdsServerPlugin: Accept connection
  XdsServerPlugin->>XdsServerPlugin: Resolve filter-chain TLS spec
  XdsServerPlugin->>XdsServerPlugin: Compare TLS spec with session protocol
  XdsServerPlugin->>ExistingAcceptor: Delegate matching connection
  XdsServerPlugin-->>Client: Reject protocol mismatch
Loading

Possibly related PRs

  • line/armeria#6833: Modifies the server-side XdsServerPlugin connection flow used by this TLS consistency check.
  • line/armeria#6837: Covers the related xDS server filter-chain matching and TLS behavior tested here.
  • line/armeria#6838: Modifies the xDS TypeRegistry construction and parser handling used by this registry update.

Suggested reviewers: trustin, ikhoon, minwoox

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the main xDS bug fixes in TLS matching, locality routing, and state replay.
Description check ✅ Passed The description is directly related to the changeset and accurately describes the implemented fixes and tests.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@jrhee17
jrhee17 marked this pull request as ready for review July 28, 2026 07:33
@jrhee17
jrhee17 requested review from ikhoon and minwoox as code owners July 28, 2026 07:33
@codecov

codecov Bot commented Jul 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 0.00%. Comparing base (8150425) to head (28085c9).
⚠️ Report is 548 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main   #6887       +/-   ##
============================================
- 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 Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jrhee17 jrhee17 added cleanup and removed defect labels Jul 28, 2026
@mergify

mergify Bot commented Jul 28, 2026

Copy link
Copy Markdown

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

  • Queue this pull request

@minwoox minwoox left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 👍 👍

@ikhoon ikhoon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 👍

@jrhee17
jrhee17 merged commit d338ed3 into line:main Jul 29, 2026
17 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants