Add proposal documentation for xDS server-side integration#6766
Open
jrhee17 wants to merge 1 commit into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR introduces two design and specification documents that establish the architectural foundation and public API surface for server-side xDS integration in Armeria. The changes define how xDS resources map to Armeria server components, the semantic constraints governing decorator and filter execution, the core APIs for plugin registration and connection management, and the entry point for integrating xDS control plane provisioning. ChangesServer-Side xDS Design and API Specification
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 (8)
xds/docs/SERVER_PROPOSAL.md (3)
17-34: ⚡ Quick winAdd language specifier to code block.
The fenced code block lacks a language specifier. Adding
textwill improve readability and fix the markdownlint warning.📝 Proposed fix
-``` +```text Connection arrives |🤖 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/docs/SERVER_PROPOSAL.md` around lines 17 - 34, The fenced code block in SERVER_PROPOSAL.md should include a language specifier to satisfy markdownlint and improve readability; edit the block that documents the flow (containing ConnectionAcceptor.accept, ConnectionContext, ServerTlsProvider.serverTlsSpec, ServerTlsSpec, TLS handshake, XdsRootDecorator.serve, and ServiceRequestContext.connectionContext()) and change the opening fence from ``` to ```text so the diagram is marked as plain text.
10-15: ⚡ Quick winAdd language specifier to code block.
The fenced code block lacks a language specifier. Adding
javawill improve syntax highlighting and fix the markdownlint warning.📝 Proposed fix
-``` +```java Server.builder() .addPlugin(new XdsServerPlugin(bootstrap, "listener", 8080)) -- [ServerPlugin, XdsServerPlugin]🤖 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/docs/SERVER_PROPOSAL.md` around lines 10 - 15, Add the missing language specifier to the fenced code block in SERVER_PROPOSAL.md so markdownlint highlights it as Java; update the opening fence from ``` to ```java for the block containing Server.builder(), .addPlugin(new XdsServerPlugin(...)), .service("/api", myService) and .build() so the snippet (including XdsServerPlugin and Server.builder) is rendered with Java syntax highlighting.
174-180: Document timeout and failure handling for the blockinginstall()call.This design proposal describes
install()blocking until the first xDS snapshot is resolved, but lacks documentation on several operational aspects:
- No timeout is specified for the blocking operation, which could cause server startup to hang indefinitely if the xDS control plane is unreachable
- No guidance on how deployments should handle control plane unavailability scenarios (retry logic, timeouts, fallback behaviors)
- No recommendations for health check configuration during the blocking initialization phase
Consider clarifying in the design:
- Whether a configurable timeout should be added
- How users should handle xDS control plane connectivity issues at startup
- Best practices for monitoring and health checks during the blocking phase
🤖 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/docs/SERVER_PROPOSAL.md` around lines 174 - 180, The docs lack operational guidance for the blocking install() call; update SERVER_PROPOSAL.md near the install() description to specify a configurable timeout (e.g., installTimeoutMs) and default behavior, document retry/backoff strategies and fallback options when the xDS control plane is unavailable (e.g., fail fast vs continue with cached/default snapshot), and add recommendations for health checks and startup probes during the blocking phase (e.g., return STARTING vs UNHEALTHY, expose a non-blocking readiness endpoint). Reference the install() entry, ConnectionAcceptor behavior, and ServerTlsProvider resolution so readers know where timeouts/retries apply and how health-check semantics should map during initialization.xds/docs/SERVER_DESIGN.md (5)
74-94: ⚡ Quick winAdd language specifier to code block.
The fenced code block lacks a language specifier. Adding
textwill improve readability and fix the markdownlint warning.📝 Proposed fix
-``` +```text xDS Listener (port 8080) ├── FilterChain: mTLS🤖 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/docs/SERVER_DESIGN.md` around lines 74 - 94, The fenced code block showing the xDS Listener diagram is missing a language specifier; update the opening fence for that block (the triple backticks before the "xDS Listener (port 8080)" diagram) to include the language token `text` (i.e., change ``` to ```text) so the markdownlint warning is resolved and the diagram renders with the correct highlighting/formatting.
72-72: ⚡ Quick winFix heading formatting.
Remove extra spaces after the hash to comply with markdown style guidelines.
📝 Proposed fix
-### Sample listener +### Sample listener🤖 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/docs/SERVER_DESIGN.md` at line 72, The markdown heading "### Sample listener" has an extra space after the hashes; update the heading in SERVER_DESIGN.md (the "Sample listener" heading) to remove the extra space so it reads "### Sample listener" to comply with markdown style guidelines.
53-70: ⚡ Quick winAdd language specifier to code block.
The fenced code block lacks a language specifier. Adding
textoryamlwill improve syntax highlighting and fix the markdownlint warning.📝 Proposed fix
-``` +```text Listener (name: "inbound_0.0.0.0_8080") └── FilterChain(s)🤖 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/docs/SERVER_DESIGN.md` around lines 53 - 70, Add a language specifier to the fenced code block that documents the Listener/FilterChain example (the block starting with ``` and containing "Listener (name: \"inbound_0.0.0.0_8080\")" and the FilterChain tree) by changing the opening fence to include a language such as ```text or ```yaml so markdownlint highlighting is enabled; keep the existing content and closing ``` unchanged.
172-172: ⚡ Quick winUse American English phrasing.
Replace "In future" with "In the future" for consistency with American English conventions.
📝 Proposed fix
-In future, we could support dynamically binding ports to listeners via wildcard listener +In the future, we could support dynamically binding ports to listeners via wildcard listener🤖 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/docs/SERVER_DESIGN.md` at line 172, Replace the phrase "In future" with "In the future" wherever it appears (exact string "In future") in the SERVER_DESIGN.md content to conform to American English phrasing; look for the occurrence in the documentation text and update it to "In the future" ensuring spacing and punctuation remain correct.
149-151: ⚡ Quick winAdd language specifier to code block.
The fenced code block lacks a language specifier. Adding
textwill improve readability and fix the markdownlint warning.📝 Proposed fix
-``` +```text [xDS RBAC] → [xDS authn] → [user's service decorators] → service🤖 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/docs/SERVER_DESIGN.md` around lines 149 - 151, Add a language specifier to the fenced code block containing the arrow diagram "[xDS RBAC] → [xDS authn] → [user's service decorators] → service" by changing the opening triple-backticks to include "text" (i.e., ```text) so the block declares its language and satisfies markdownlint; update the fenced block around that exact diagram in SERVER_DESIGN.md.
🤖 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/docs/SERVER_PROPOSAL.md`:
- Around line 99-108: Align the null-handling semantics by making sniHostname()
nullable: change ConnectionContext.sniHostname() to return "@Nullable String"
(instead of empty string) to match alpnProtocols() semantics; update the
interface signature, any javadoc/docs in SERVER_PROPOSAL.md, all implementations
of ConnectionContext (and their constructors/builders) to return null when SNI
is absent, and adjust callers/tests to handle null rather than empty string.
Ensure Attribute: reference sniHostname() and alpnProtocols() in the same style
across the codebase and run tests to catch call sites that assumed an empty
string.
---
Nitpick comments:
In `@xds/docs/SERVER_DESIGN.md`:
- Around line 74-94: The fenced code block showing the xDS Listener diagram is
missing a language specifier; update the opening fence for that block (the
triple backticks before the "xDS Listener (port 8080)" diagram) to include the
language token `text` (i.e., change ``` to ```text) so the markdownlint warning
is resolved and the diagram renders with the correct highlighting/formatting.
- Line 72: The markdown heading "### Sample listener" has an extra space after
the hashes; update the heading in SERVER_DESIGN.md (the "Sample listener"
heading) to remove the extra space so it reads "### Sample listener" to comply
with markdown style guidelines.
- Around line 53-70: Add a language specifier to the fenced code block that
documents the Listener/FilterChain example (the block starting with ``` and
containing "Listener (name: \"inbound_0.0.0.0_8080\")" and the FilterChain tree)
by changing the opening fence to include a language such as ```text or ```yaml
so markdownlint highlighting is enabled; keep the existing content and closing
``` unchanged.
- Line 172: Replace the phrase "In future" with "In the future" wherever it
appears (exact string "In future") in the SERVER_DESIGN.md content to conform to
American English phrasing; look for the occurrence in the documentation text and
update it to "In the future" ensuring spacing and punctuation remain correct.
- Around line 149-151: Add a language specifier to the fenced code block
containing the arrow diagram "[xDS RBAC] → [xDS authn] → [user's service
decorators] → service" by changing the opening triple-backticks to include
"text" (i.e., ```text) so the block declares its language and satisfies
markdownlint; update the fenced block around that exact diagram in
SERVER_DESIGN.md.
In `@xds/docs/SERVER_PROPOSAL.md`:
- Around line 17-34: The fenced code block in SERVER_PROPOSAL.md should include
a language specifier to satisfy markdownlint and improve readability; edit the
block that documents the flow (containing ConnectionAcceptor.accept,
ConnectionContext, ServerTlsProvider.serverTlsSpec, ServerTlsSpec, TLS
handshake, XdsRootDecorator.serve, and
ServiceRequestContext.connectionContext()) and change the opening fence from ```
to ```text so the diagram is marked as plain text.
- Around line 10-15: Add the missing language specifier to the fenced code block
in SERVER_PROPOSAL.md so markdownlint highlights it as Java; update the opening
fence from ``` to ```java for the block containing Server.builder(),
.addPlugin(new XdsServerPlugin(...)), .service("/api", myService) and .build()
so the snippet (including XdsServerPlugin and Server.builder) is rendered with
Java syntax highlighting.
- Around line 174-180: The docs lack operational guidance for the blocking
install() call; update SERVER_PROPOSAL.md near the install() description to
specify a configurable timeout (e.g., installTimeoutMs) and default behavior,
document retry/backoff strategies and fallback options when the xDS control
plane is unavailable (e.g., fail fast vs continue with cached/default snapshot),
and add recommendations for health checks and startup probes during the blocking
phase (e.g., return STARTING vs UNHEALTHY, expose a non-blocking readiness
endpoint). Reference the install() entry, ConnectionAcceptor behavior, and
ServerTlsProvider resolution so readers know where timeouts/retries apply and
how health-check semantics should map during initialization.
🪄 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: 0e9ed98d-f238-4f3a-a2fb-8949e72e1bae
📒 Files selected for processing (2)
xds/docs/SERVER_DESIGN.mdxds/docs/SERVER_PROPOSAL.md
Comment on lines
+99
to
+108
| public final class ConnectionContext { | ||
| public SessionProtocol sessionProtocol(); | ||
| public String sniHostname(); // empty string if no SNI | ||
| @Nullable public List<String> alpnProtocols(); // null if no ALPN | ||
| public InetSocketAddress localAddress(); | ||
| public InetSocketAddress remoteAddress(); | ||
| @Nullable public <T> T attr(AttributeKey<T> key); | ||
| public <T> void setAttr(AttributeKey<T> key, @Nullable T value); | ||
| } | ||
| ``` |
Contributor
There was a problem hiding this comment.
Inconsistent null-handling in ConnectionContext API.
The API has inconsistent semantics for "not present" values:
sniHostname()returns an empty string when SNI is absent (non-null)alpnProtocols()returnsnullwhen ALPN is absent (@Nullable)
This inconsistency can confuse API users and lead to inconsistent null-checking patterns. Consider aligning the semantics—either both use null to indicate absence, or both use empty/empty-list to indicate absence.
Recommendation: Use @Nullable String for sniHostname() to match alpnProtocols(), since null is a clearer signal of "not present" for optional connection attributes.
🔄 Proposed fix
public final class ConnectionContext {
public SessionProtocol sessionProtocol();
- public String sniHostname(); // empty string if no SNI
+ `@Nullable` public String sniHostname(); // null if no SNI
`@Nullable` public List<String> alpnProtocols(); // null if no ALPN📝 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 final class ConnectionContext { | |
| public SessionProtocol sessionProtocol(); | |
| public String sniHostname(); // empty string if no SNI | |
| @Nullable public List<String> alpnProtocols(); // null if no ALPN | |
| public InetSocketAddress localAddress(); | |
| public InetSocketAddress remoteAddress(); | |
| @Nullable public <T> T attr(AttributeKey<T> key); | |
| public <T> void setAttr(AttributeKey<T> key, @Nullable T value); | |
| } | |
| ``` | |
| public final class ConnectionContext { | |
| public SessionProtocol sessionProtocol(); | |
| `@Nullable` public String sniHostname(); // null if no SNI | |
| `@Nullable` public List<String> alpnProtocols(); // null if no ALPN | |
| public InetSocketAddress localAddress(); | |
| public InetSocketAddress remoteAddress(); | |
| `@Nullable` public <T> T attr(AttributeKey<T> key); | |
| public <T> void setAttr(AttributeKey<T> key, `@Nullable` T 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/docs/SERVER_PROPOSAL.md` around lines 99 - 108, Align the null-handling
semantics by making sniHostname() nullable: change
ConnectionContext.sniHostname() to return "@Nullable String" (instead of empty
string) to match alpnProtocols() semantics; update the interface signature, any
javadoc/docs in SERVER_PROPOSAL.md, all implementations of ConnectionContext
(and their constructors/builders) to return null when SNI is absent, and adjust
callers/tests to handle null rather than empty string. Ensure Attribute:
reference sniHostname() and alpnProtocols() in the same style across the
codebase and run tests to catch call sites that assumed an empty string.
Contributor
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6766 +/- ##
============================================
- 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:
|
ikhoon
approved these changes
May 15, 2026
ikhoon
left a comment
ikhoon
left a comment
Contributor
There was a problem hiding this comment.
The proposed implementation direction looks good to me.
| ```java | ||
| package com.linecorp.armeria.server; | ||
|
|
||
| public final class ConnectionContext { |
Contributor
There was a problem hiding this comment.
nit)
- Should we make it an interface so that we can wrap it and override methods easily?
- Expose Netty
Channelas well?
|
|
||
| @FunctionalInterface | ||
| public interface ConnectionAcceptor { | ||
| boolean accept(ConnectionContext ctx); |
Contributor
There was a problem hiding this comment.
Question) What does the returned boolean value indicate?
|
Tick the box to add this pull request to the merge queue (same as
|
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 existing xDS support is client-side only:
XdsBootstrapfetches Listener/Route/Cluster/Endpoint resources and feeds them intoXdsEndpointGroupfor service discovery and load balancing.Server-side xDS is a different use case. Instead of discovering upstream services, the Armeria server itself becomes an xDS-managed workload. A control plane (Istio, CentralDogma, etc.) pushes configuration that governs how the server accepts connections, negotiates TLS, and enforces policies like RBAC — while the user's services and routing remain entirely under their control.
This PR proposes a design for server-side xDS integration, adding two documents that describe the architecture and public API surface.