Add filter chain matching and SNI-based TLS selection for server-side xDS - #6837
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds Envoy filter-chain matching support and SNI-based server TLS certificate selection. Listener snapshots now use a matcher for chain resolution, transport sockets use a selector for certificate choice, and tests cover matching behavior and YAML fixture quoting. ChangesXDS filter chain and TLS spec matching
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ConnectionContext
participant ListenerSnapshot
participant FilterChainMatcher
ConnectionContext->>ListenerSnapshot: matchFilterChain(ctx)
ListenerSnapshot->>FilterChainMatcher: match(defaultFilterChain, ctx)
FilterChainMatcher->>FilterChainMatcher: narrow by destination port
FilterChainMatcher->>FilterChainMatcher: narrow by server names
FilterChainMatcher->>FilterChainMatcher: narrow by transport protocol
FilterChainMatcher->>FilterChainMatcher: narrow by application protocols
FilterChainMatcher-->>ListenerSnapshot: matched or default filter chain
sequenceDiagram
participant ConnectionContext
participant TransportSocketSnapshot
participant ServerTlsSpecSelector
ConnectionContext->>TransportSocketSnapshot: serverTlsSpec(ctx)
TransportSocketSnapshot->>ServerTlsSpecSelector: select(ctx)
ServerTlsSpecSelector->>ServerTlsSpecSelector: exact SNI lookup
ServerTlsSpecSelector->>ServerTlsSpecSelector: wildcard SNI lookup
ServerTlsSpecSelector-->>TransportSocketSnapshot: selected ServerTlsSpec
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 2
🧹 Nitpick comments (5)
xds/src/main/java/com/linecorp/armeria/xds/ServerTlsSpecSelector.java (1)
136-145: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winParsing the DN with
split(",")is fragile; preferLdapName.
X500Principal.getName()returns an RFC 2253 string where commas inside attribute values are escaped (e.g.CN=Doe\, John) and RDNs can be multi-valued (+). Splitting on,will mis-parse such CNs and yield a truncated/incorrect name, leading to wrong SNI keying for that certificate. Usejavax.naming.ldap.LdapNameto parse robustly.♻️ Proposed refactor using
LdapNameimport javax.naming.InvalidNameException; import javax.naming.ldap.LdapName; import javax.naming.ldap.Rdn;`@Nullable` private static String extractCn(X509Certificate cert) { - final String dn = cert.getSubjectX500Principal().getName(); - for (String rdn : dn.split(",")) { - final String trimmed = rdn.trim(); - if (trimmed.toUpperCase(Locale.ROOT).startsWith("CN=")) { - return trimmed.substring(3); - } - } - return null; + final String dn = cert.getSubjectX500Principal().getName(); + try { + final LdapName ldapName = new LdapName(dn); + for (Rdn rdn : ldapName.getRdns()) { + if ("CN".equalsIgnoreCase(rdn.getType())) { + return String.valueOf(rdn.getValue()); + } + } + } catch (InvalidNameException e) { + // Ignore malformed DN and fall through. + } + return null; }Please confirm
javax.naming.ldap.LdapNameis acceptable within this module's allowed dependencies.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@xds/src/main/java/com/linecorp/armeria/xds/ServerTlsSpecSelector.java` around lines 136 - 145, The CN extraction in extractCn() is parsing the X500Principal DN string by splitting on commas, which can break on escaped commas and multi-valued RDNs. Replace that logic with robust DN parsing using javax.naming.ldap.LdapName (and Rdn iteration) inside ServerTlsSpecSelector.extractCn(), and handle InvalidNameException appropriately; this will preserve the correct CN for SNI keying. Please also confirm that javax.naming.ldap is allowed for this module before applying the refactor.Source: Path instructions
it/xds-client/src/test/java/com/linecorp/armeria/xds/it/server/ServerFilterChainMatchTest.java (1)
90-144: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared YAML boilerplate to reduce duplication.
The
envoy.filters.network.http_connection_managerblock (route config, virtual hosts, http_filters) is copy-pasted near-identically across all three tests (and twice withinmatchByTransportProtocol/defaultFilterChainFallback). Consider a small helper that builds this repeated block (and optionally thetls_certificatessnippet) given parameters, then compose the filter-chain YAML from it. This would make future edits (e.g., adding a new match field) less error-prone.As per path instructions, dev-guide recommends preferring "top-down organization, early-return, avoid redundancy" style.
Also applies to: 176-242, 276-313
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@it/xds-client/src/test/java/com/linecorp/armeria/xds/it/server/ServerFilterChainMatchTest.java` around lines 90 - 144, The YAML in ServerFilterChainMatchTest repeats the same envoy.filters.network.http_connection_manager setup across multiple test cases, so extract that shared route/virtual_hosts/http_filters block into a helper and reuse it when building the listener YAML. Use the existing test methods such as matchByTransportProtocol and defaultFilterChainFallback to compose the per-chain parts (including the optional tls_certificates snippet) from the shared builder so future changes only need to be made once.Source: Path instructions
xds/src/main/java/com/linecorp/armeria/xds/ListenerSnapshot.java (1)
109-117: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueJavadoc slightly understates the new matching semantics.
"Returns the first FilterChainSnapshot that matches" no longer fully captures the multi-criteria, priority-ordered narrowing now performed by
FilterChainMatcher. Consider linking toFilterChainMatcher's more detailed doc for clarity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@xds/src/main/java/com/linecorp/armeria/xds/ListenerSnapshot.java` around lines 109 - 117, Update the Javadoc on ListenerSnapshot.matchFilterChain to reflect the newer multi-criteria, priority-ordered matching behavior instead of saying it simply returns the “first” matching FilterChainSnapshot. Clarify that the method delegates to FilterChainMatcher.match, and reference FilterChainMatcher in the comment so readers can find the detailed matching semantics. Keep the default-filter-chain fallback mention intact.it/xds-client/src/test/java/com/linecorp/armeria/xds/it/server/ServerTlsSpecSelectorTest.java (2)
120-188: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting shared request/assert boilerplate.
All three tests repeat the same build-client/execute/assert-OK-and-"hello" sequence. A small private helper (e.g.
assertHelloOk(ClientTlsSpec, Endpoint)) would reduce duplication and make each test method focus on its distinguishing setup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@it/xds-client/src/test/java/com/linecorp/armeria/xds/it/server/ServerTlsSpecSelectorTest.java` around lines 120 - 188, The three tests in ServerTlsSpecSelectorTest repeat the same WebClient build, execute, and OK/"hello" assertions, so extract that shared flow into a small private helper such as assertHelloOk(ClientTlsSpec, Endpoint). Update exactSniMatch, fallbackToFirstCert, and noSniReturnsFirstCert to call the helper after their distinct TLS/Endpoint setup so the test methods only describe their scenario-specific differences.
153-188: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winMissing wildcard-match test coverage.
ServerTlsSpecSelector.select()implements three selection paths (exact, wildcard, fallback-to-first), but the tests here only exercise exact match and fallback. The wildcard branch (dotPossubstring logic inServerTlsSpecSelector.select) is untested, which is exactly the kind of subtle string-index logic that benefits from a dedicated test.Suggested addition
`@Test` void wildcardSniMatch() { // requires a cert with a wildcard SAN, e.g. "*.example.com" // SNI "www.example.com" → should resolve via wildcard match, not fallback. }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@it/xds-client/src/test/java/com/linecorp/armeria/xds/it/server/ServerTlsSpecSelectorTest.java` around lines 153 - 188, Add a dedicated test in ServerTlsSpecSelectorTest to cover the wildcard SNI branch in ServerTlsSpecSelector.select(), not just exact-match and fallback behavior. Create a TLS setup with a wildcard certificate (for example a cert whose SAN matches *.example.com), then send a request using an SNI like www.example.com and assert that the connection succeeds via wildcard selection rather than falling back to the first cert. Keep the test рядом with fallbackToFirstCert and noSniReturnsFirstCert so the three selection paths are covered together.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@it/xds-client/src/test/java/com/linecorp/armeria/xds/it/server/ServerFilterChainMatchTest.java`:
- Around line 258-267: The TLS failure assertion in ServerFilterChainMatchTest
is too engine-specific because Flags.tlsEngineType() can switch between JDK and
OpenSSL, making the SignatureException root-cause check brittle. Update the
assertion around wrongClient.execute(...) to use a provider-agnostic TLS failure
expectation, such as checking for SSLHandshakeException as the cause, or remove
the root-cause type check entirely if UnprocessedRequestException is sufficient.
Keep the change localized to the wrongTlsSpec / wrongClient test path.
In `@xds/src/main/java/com/linecorp/armeria/xds/FilterChainMatcher.java`:
- Around line 170-195: Normalize the SNI value before server name matching in
ServerNamesStep so casing differences do not prevent a match. In
FilterChainMatcher.ServerNamesStep.matchSpecifics, use the
ConnectionContext.sniHostname() value in a case-insensitive way (for example,
normalize it once before checking against
FilterChainSnapshot.filterChainMatch().getServerNamesList()), while keeping the
existing empty/null guard.
---
Nitpick comments:
In
`@it/xds-client/src/test/java/com/linecorp/armeria/xds/it/server/ServerFilterChainMatchTest.java`:
- Around line 90-144: The YAML in ServerFilterChainMatchTest repeats the same
envoy.filters.network.http_connection_manager setup across multiple test cases,
so extract that shared route/virtual_hosts/http_filters block into a helper and
reuse it when building the listener YAML. Use the existing test methods such as
matchByTransportProtocol and defaultFilterChainFallback to compose the per-chain
parts (including the optional tls_certificates snippet) from the shared builder
so future changes only need to be made once.
In
`@it/xds-client/src/test/java/com/linecorp/armeria/xds/it/server/ServerTlsSpecSelectorTest.java`:
- Around line 120-188: The three tests in ServerTlsSpecSelectorTest repeat the
same WebClient build, execute, and OK/"hello" assertions, so extract that shared
flow into a small private helper such as assertHelloOk(ClientTlsSpec, Endpoint).
Update exactSniMatch, fallbackToFirstCert, and noSniReturnsFirstCert to call the
helper after their distinct TLS/Endpoint setup so the test methods only describe
their scenario-specific differences.
- Around line 153-188: Add a dedicated test in ServerTlsSpecSelectorTest to
cover the wildcard SNI branch in ServerTlsSpecSelector.select(), not just
exact-match and fallback behavior. Create a TLS setup with a wildcard
certificate (for example a cert whose SAN matches *.example.com), then send a
request using an SNI like www.example.com and assert that the connection
succeeds via wildcard selection rather than falling back to the first cert. Keep
the test рядом with fallbackToFirstCert and noSniReturnsFirstCert so the three
selection paths are covered together.
In `@xds/src/main/java/com/linecorp/armeria/xds/ListenerSnapshot.java`:
- Around line 109-117: Update the Javadoc on ListenerSnapshot.matchFilterChain
to reflect the newer multi-criteria, priority-ordered matching behavior instead
of saying it simply returns the “first” matching FilterChainSnapshot. Clarify
that the method delegates to FilterChainMatcher.match, and reference
FilterChainMatcher in the comment so readers can find the detailed matching
semantics. Keep the default-filter-chain fallback mention intact.
In `@xds/src/main/java/com/linecorp/armeria/xds/ServerTlsSpecSelector.java`:
- Around line 136-145: The CN extraction in extractCn() is parsing the
X500Principal DN string by splitting on commas, which can break on escaped
commas and multi-valued RDNs. Replace that logic with robust DN parsing using
javax.naming.ldap.LdapName (and Rdn iteration) inside
ServerTlsSpecSelector.extractCn(), and handle InvalidNameException
appropriately; this will preserve the correct CN for SNI keying. Please also
confirm that javax.naming.ldap is allowed for this module before applying the
refactor.
🪄 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: 2bbb6185-fb2a-4cf0-a721-3c313edb0a4a
📒 Files selected for processing (7)
it/xds-client/src/test/java/com/linecorp/armeria/xds/it/server/ServerFilterChainMatchTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/server/ServerTlsSpecSelectorTest.javaxds-api/src/main/proto/envoy/config/listener/v3/listener_components.protoxds/src/main/java/com/linecorp/armeria/xds/FilterChainMatcher.javaxds/src/main/java/com/linecorp/armeria/xds/ListenerSnapshot.javaxds/src/main/java/com/linecorp/armeria/xds/ServerTlsSpecSelector.javaxds/src/main/java/com/linecorp/armeria/xds/TransportSocketSnapshot.java
Contributor
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
xds/src/main/java/com/linecorp/armeria/xds/TransportSocketSnapshot.java (1)
97-102: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winBuild server specs from the copied certificate snapshot.
ServerTlsSpecSelectorindexesspecsandtlsCertificatestogether. Buildspecsfromthis.tlsCertificatesso the selector always receives two lists from the same immutable snapshot.Proposed fix
final List<ServerTlsSpec> specs = - tlsCertificates.stream() + this.tlsCertificates.stream() .map(cert -> buildServerTlsSpec(downstreamTlsContext, cert, this.validationContext)) .collect(ImmutableList.toImmutableList());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@xds/src/main/java/com/linecorp/armeria/xds/TransportSocketSnapshot.java` around lines 97 - 102, Build the server TLS specs from the copied certificate snapshot instead of the original list so ServerTlsSpecSelector always receives matching immutable data. In TransportSocketSnapshot, update the spec creation in the constructor/initializer that builds specs and passes them to ServerTlsSpecSelector so it iterates over this.tlsCertificates, keeping the specs list aligned with the same snapshot used by the selector.it/xds-client/src/test/java/com/linecorp/armeria/xds/it/server/ServerTlsSpecSelectorTest.java (1)
120-188: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd wildcard-SNI coverage in
ServerTlsSpecSelectorTest. The existingTlsPeerVerificationIntegrationTestwildcard cert case covers SAN validation, notServerTlsSpecSelector’s wildcard lookup branch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@it/xds-client/src/test/java/com/linecorp/armeria/xds/it/server/ServerTlsSpecSelectorTest.java` around lines 120 - 188, Add a new test in ServerTlsSpecSelectorTest to cover wildcard SNI selection, since exactSniMatch(), fallbackToFirstCert(), and noSniReturnsFirstCert() do not exercise the wildcard lookup branch. Create a client/server setup using a wildcard hostname like “*.example.com” and verify the server presents the wildcard certificate when the SNI is a matching subdomain, using the existing helpers such as ClientTlsSpec, Endpoint, and WebClient. Keep the test focused on ServerTlsSpecSelector behavior rather than SAN validation, which is already covered elsewhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@xds/src/main/java/com/linecorp/armeria/xds/FilterChainMatcher.java`:
- Around line 171-198: The ServerNamesStep in FilterChainMatcher only normalizes
the incoming SNI, so mixed-case configured server_names still fail to match.
Update matchSpecifics to compare against a normalized form of each
FilterChainSnapshot’s filterChainMatch().getServerNamesList() entry (or
normalize once when building the match set) so the lookup is truly
case-insensitive while preserving the existing exact-match behavior.
In `@xds/src/main/java/com/linecorp/armeria/xds/ServerTlsSpecSelector.java`:
- Around line 116-130: In ServerTlsSpecSelector, the SAN parsing logic in the
method that extracts subject names should not fall back to the CN when a SAN
extension is present but contains no DNS entries. Update the existing
SAN-handling branch so that the presence of any SAN extension returns only DNS
SANs if available, and otherwise returns an empty result instead of continuing
to extractCn and using the CN. Keep the CN fallback only for certificates with
no SAN extension at all.
---
Nitpick comments:
In
`@it/xds-client/src/test/java/com/linecorp/armeria/xds/it/server/ServerTlsSpecSelectorTest.java`:
- Around line 120-188: Add a new test in ServerTlsSpecSelectorTest to cover
wildcard SNI selection, since exactSniMatch(), fallbackToFirstCert(), and
noSniReturnsFirstCert() do not exercise the wildcard lookup branch. Create a
client/server setup using a wildcard hostname like “*.example.com” and verify
the server presents the wildcard certificate when the SNI is a matching
subdomain, using the existing helpers such as ClientTlsSpec, Endpoint, and
WebClient. Keep the test focused on ServerTlsSpecSelector behavior rather than
SAN validation, which is already covered elsewhere.
In `@xds/src/main/java/com/linecorp/armeria/xds/TransportSocketSnapshot.java`:
- Around line 97-102: Build the server TLS specs from the copied certificate
snapshot instead of the original list so ServerTlsSpecSelector always receives
matching immutable data. In TransportSocketSnapshot, update the spec creation in
the constructor/initializer that builds specs and passes them to
ServerTlsSpecSelector so it iterates over this.tlsCertificates, keeping the
specs list aligned with the same snapshot used by the selector.
🪄 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: da0cb38e-3845-4445-b8ee-b2c1a11fc99e
📒 Files selected for processing (7)
it/xds-client/src/test/java/com/linecorp/armeria/xds/it/server/ServerFilterChainMatchTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/server/ServerTlsSpecSelectorTest.javaxds-api/src/main/proto/envoy/config/listener/v3/listener_components.protoxds/src/main/java/com/linecorp/armeria/xds/FilterChainMatcher.javaxds/src/main/java/com/linecorp/armeria/xds/ListenerSnapshot.javaxds/src/main/java/com/linecorp/armeria/xds/ServerTlsSpecSelector.javaxds/src/main/java/com/linecorp/armeria/xds/TransportSocketSnapshot.java
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6837 +/- ##
============================================
+ Coverage 74.46% 75.11% +0.65%
- Complexity 22234 25275 +3041
============================================
Files 1963 2246 +283
Lines 82437 93770 +11333
Branches 10764 12260 +1496
============================================
+ Hits 61385 70440 +9055
- Misses 15918 17498 +1580
- Partials 5134 5832 +698 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
xds/src/main/java/com/linecorp/armeria/xds/ServerTlsSpecSelector.java (1)
132-142: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winParse the subject DN with
LdapNameinstead of splitting on commas.dn.split(",")can mis-read escaped commas in RFC 2253 names likeCN=Doe\, John,O=Example, soextractCnmay return the wrong value.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@xds/src/main/java/com/linecorp/armeria/xds/ServerTlsSpecSelector.java` around lines 132 - 142, The CN extraction in extractCn currently splits the subject DN on commas, which can misparse RFC 2253 names with escaped commas. Update ServerTlsSpecSelector.extractCn to parse the DN using LdapName and iterate its RDNs instead of string-splitting, preserving correct CN handling for names like CN=Doe\, John,O=Example.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@xds/src/main/java/com/linecorp/armeria/xds/ServerTlsSpecSelector.java`:
- Around line 132-142: The CN extraction in extractCn currently splits the
subject DN on commas, which can misparse RFC 2253 names with escaped commas.
Update ServerTlsSpecSelector.extractCn to parse the DN using LdapName and
iterate its RDNs instead of string-splitting, preserving correct CN handling for
names like CN=Doe\, John,O=Example.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3c16bb8d-78a6-476c-80bd-61ab1e0693bc
📒 Files selected for processing (1)
xds/src/main/java/com/linecorp/armeria/xds/ServerTlsSpecSelector.java
jrhee17
marked this pull request as ready for review
July 2, 2026 08:39
|
Tick the box to add this pull request to the merge queue (same as
|
ikhoon
approved these changes
Jul 3, 2026
| } | ||
|
|
||
| // Uses DNS SANs if present; falls back to CN per RFC 6125 §6.4.4. | ||
| static List<String> extractDnsNames(X509Certificate cert) { |
Contributor
There was a problem hiding this comment.
Optional) Would it be possible to reuse CertificateUtil with a small refactoring?
Contributor
Author
There was a problem hiding this comment.
Moved to CertificateUtil and shares extractCommonName now
minwoox
approved these changes
Jul 3, 2026
| } | ||
| // Wildcard match: "www.example.com" → ".example.com" | ||
| final int dotPos = normalizedSni.indexOf('.', 1); | ||
| if (dotPos > 0 && dotPos < normalizedSni.length() - 1) { |
Contributor
There was a problem hiding this comment.
Is this from Envoy? it looks weird to try with .com when the normalizedSni is example.com
Contributor
Author
There was a problem hiding this comment.
Left a comment on the code with a reference to envoy's implementation
| final String normalizedSni = sniHostname.toLowerCase(Locale.ROOT); | ||
| final ImmutableList.Builder<FilterChainSnapshot> matched = ImmutableList.builder(); | ||
| for (FilterChainSnapshot fcs : specifics) { | ||
| if (fcs.filterChainMatch().getServerNamesList().contains(normalizedSni)) { |
Contributor
There was a problem hiding this comment.
Question: We don't have to consider a wildcard here. Right?
Contributor
Author
There was a problem hiding this comment.
I had dropped support initially for wildcard as I thought the implementation would become complex.
https://github.com/line/armeria/pull/6837/changes/BASE..1deec8e6ede7f862a185a3a156bd9fb534164c4b#diff-84f432e7c5ecb6f5e87b83f1bd0060923bcc570df872f0c0d25c628e8374f874R171
Added support at 33cf876
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@xds/src/main/java/com/linecorp/armeria/xds/FilterChainMatcher.java`:
- Around line 223-233: The hasWildcardServerName helper in FilterChainMatcher
should defensively handle empty server_names entries before calling
name.charAt(0). Add a guard for empty strings inside the loop so a blank entry
is skipped rather than throwing StringIndexOutOfBoundsException, while
preserving the existing wildcard matching logic for valid names.
🪄 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: 18b3ba3f-7dcd-47d6-b538-c6d960db4a14
📒 Files selected for processing (22)
core/src/main/java/com/linecorp/armeria/internal/common/util/CertificateUtil.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/BootstrapSecretsTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/CertificateValidationContextTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/ControlPlaneTlsIntegrationTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/DataSourcePolicyTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/DataSourceTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/DynamicSecretTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/ErrorHandlingTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/PipeEndpointTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/ResourceNodeMetricTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/TlsPeerVerificationIntegrationTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/XdsEndpointGroupTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/XdsPreprocessorTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/server/ServerDecoratorTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/server/ServerFallbackTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/server/ServerFilterChainMatchTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/server/ServerMultiPortTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/server/ServerMultiplePluginTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/server/ServerTlsSpecSelectorTest.javait/xds-client/src/test/java/com/linecorp/armeria/xds/it/server/ServerXdsTest.javaxds/src/main/java/com/linecorp/armeria/xds/FilterChainMatcher.javaxds/src/main/java/com/linecorp/armeria/xds/ServerTlsSpecSelector.java
✅ Files skipped from review due to trivial changes (5)
- it/xds-client/src/test/java/com/linecorp/armeria/xds/it/PipeEndpointTest.java
- it/xds-client/src/test/java/com/linecorp/armeria/xds/it/server/ServerMultiPortTest.java
- it/xds-client/src/test/java/com/linecorp/armeria/xds/it/server/ServerFallbackTest.java
- it/xds-client/src/test/java/com/linecorp/armeria/xds/it/server/ServerMultiplePluginTest.java
- it/xds-client/src/test/java/com/linecorp/armeria/xds/it/server/ServerDecoratorTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- it/xds-client/src/test/java/com/linecorp/armeria/xds/it/server/ServerTlsSpecSelectorTest.java
- it/xds-client/src/test/java/com/linecorp/armeria/xds/it/server/ServerFilterChainMatchTest.java
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:
This is a subset of #6797 that adds server-side filter chain matching and SNI-based TLS certificate selection to the xDS module. When Armeria acts as an xDS-managed server, incoming connections need to be matched against the listener's
filter_chainsto determine which TLS configuration and routing to apply — replicating Envoy's filter chain matching semantics.Modifications:
FilterChainMatcherwhich implements Envoy's BFS-style multi-pass narrowing algorithm. Supported match dimensions (in priority order):destination_port,server_names,transport_protocol,application_protocols. Each level precomputes specific/wildcard partitions to avoid redundant iteration.ServerTlsSpecSelectorwhich implements SNI-based certificate selection: exact DNS SAN match → wildcard match (e.g.*.example.com) → fallback to first certificate. Follows Envoy'sDefaultTlsCertificateSelectorbehavior.ListenerSnapshot.matchFilterChain(ConnectionContext)that delegates toFilterChainMatcher.TransportSocketSnapshot.serverTlsSpec(ConnectionContext)that delegates toServerTlsSpecSelector. The downstream constructor builds aServerTlsSpecper certificate using the sharedapplyCommonTlsConfigmethod (same trust/verifier logic as the client path).destination_port,server_names,transport_protocol, andapplication_protocolsfields as supported inlistener_components.proto.ServerFilterChainMatchTest— verifies transport protocol matching, default filter chain fallback, and unmatched connection rejection.ServerTlsSpecSelectorTest— verifies exact SNI match, wildcard fallback, and no-SNI fallback.Result:
ListenerSnapshotcan now select the correct filter chain for an incoming connection based on destination port, SNI hostname, transport protocol, and ALPN.TransportSocketSnapshotcan select the correct server TLS certificate based on SNI, supporting multi-certificate downstream listeners.