Migrate xDS resource services from gRPC to YAML-over-HTTP annotated services - #1337
Merged
Conversation
…ervices
Motivation:
The xDS resource management APIs (cluster, endpoint, listener, route,
group, kubernetes) were implemented as gRPC services backed by
service-specific `.proto` files. Migrating them to Armeria annotated
HTTP services that consume `application/yaml` removes the protobuf
service layer, simplifies the API surface, and makes the YAML-native
storage format a first-class concern — clients send YAML directly and
it is stored as-is (with `name`/`clusterName` injected).
Modifications:
- Convert `XdsClusterService`, `XdsEndpointService`,
`XdsListenerService`, `XdsRouteService` from `*ImplBase` gRPC stubs
to plain annotated HTTP services (`@Post`/`@Put`/`@Delete`,
`@Consumes("application/yaml")`).
- Add `RequiresXdsGroupRole` annotation and
`RequiresXdsGroupRoleDecorator` to replace the inline
`checkWritePermission` calls with a declarative, per-method
authorization mechanism.
- Refactor `XdsResourceManager` so that `push`/`update`/`delete`
operate on raw YAML strings instead of protobuf messages, and add
helpers `parseYaml`, `normalizeYamlKeys`, `injectYamlField`, and
`errorResponse`.
Result:
- xDS resource CRUD is now served via plain HTTP with YAML bodies,
removing the gRPC service layer and the associated proto wrapper
definitions.
|
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:
📝 WalkthroughWalkthroughXDS mutation APIs were migrated from gRPC and JSON request handling to Armeria HTTP endpoints with YAML payloads, asynchronous responses, centralized YAML-backed resource management, group-role authorization, updated control-plane wiring, and revised tests. ChangesXDS HTTP and YAML migration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ControlPlaneService
participant XdsGroupRoleDecorator
participant XdsClusterService
participant XdsResourceManager
Client->>ControlPlaneService: Send YAML mutation request
ControlPlaneService->>XdsGroupRoleDecorator: Resolve group role
XdsGroupRoleDecorator->>XdsClusterService: Forward authorized request
XdsClusterService->>XdsResourceManager: Parse, normalize, and persist YAML
XdsResourceManager-->>Client: Return HTTP response
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
it/xds-k8s-node-ip-extractor/src/test/java/com/linecorp/centraldogma/it/xds/k8s/XdsKubernetesNodeIpExtractorTest.java (1)
321-323: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse existing YAML serialization helper.
Instead of manually parsing and re-serializing via
JacksonandYaml, you can use the built-in helperXdsResourceManager.toYamlBodyString().♻️ Proposed refactor
- final String yaml = Yaml.writeValueAsString( - Jackson.readTree(XdsResourceManager.JSON_MESSAGE_MARSHALLER.writeValueAsString(aggregator))); + final String yaml = XdsResourceManager.toYamlBodyString(aggregator);🤖 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-k8s-node-ip-extractor/src/test/java/com/linecorp/centraldogma/it/xds/k8s/XdsKubernetesNodeIpExtractorTest.java` around lines 321 - 323, Update the YAML request-body construction in the test helper to use XdsResourceManager.toYamlBodyString() directly, replacing the manual JSON serialization, Jackson parsing, and Yaml re-serialization while preserving the existing execute call and request behavior.xds/src/test/java/com/linecorp/centraldogma/xds/endpoint/v1/XdsRegisterEndpointTest.java (1)
252-254: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse existing YAML serialization helper.
You can simplify the YAML conversion by delegating to
XdsTestUtil.toYaml().♻️ Proposed refactor
- final String yaml = Yaml.writeValueAsString( - Jackson.readTree(JSON_MESSAGE_MARSHALLER.writeValueAsString(localityLbEndpoint))); + final String yaml = XdsTestUtil.toYaml(localityLbEndpoint);(Make sure to import
com.linecorp.centraldogma.xds.internal.XdsTestUtilif not already imported).🤖 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/test/java/com/linecorp/centraldogma/xds/endpoint/v1/XdsRegisterEndpointTest.java` around lines 252 - 254, Update the YAML conversion in the test helper to call XdsTestUtil.toYaml() directly instead of manually serializing with JSON_MESSAGE_MARSHALLER, Jackson.readTree, and Yaml.writeValueAsString; add the XdsTestUtil import if needed and preserve the resulting request payload.xds/src/test/java/com/linecorp/centraldogma/xds/endpoint/v1/XdsEndpointServiceTest.java (1)
94-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDelegate YAML response parsing to
XdsResourceManager.parseYaml.Across several tests, the HTTP response body is manually parsed as a YAML tree and merged into a builder using
JSON_MESSAGE_MARSHALLER. You can simplify this logic significantly by using the existingXdsResourceManager.parseYaml()helper.
xds/src/test/java/com/linecorp/centraldogma/xds/endpoint/v1/XdsEndpointServiceTest.java#L94-L96: Replace the manual tree traversal and builder merge withfinal ClusterLoadAssignment actualEndpoint = XdsResourceManager.parseYaml(response.contentUtf8(), ClusterLoadAssignment.newBuilder());.xds/src/test/java/com/linecorp/centraldogma/xds/endpoint/v1/XdsEndpointServiceTest.java#L160-L162: Simplify parsing for the first update test step.xds/src/test/java/com/linecorp/centraldogma/xds/endpoint/v1/XdsEndpointServiceTest.java#L174-L176: Simplify parsing for the second update test step.xds/src/test/java/com/linecorp/centraldogma/xds/endpoint/v1/XdsEndpointServiceTest.java#L240-L242: Pass the parsedClusterLoadAssignmentdirectly tocheckEndpointsViaDiscoveryRequest.xds/src/test/java/com/linecorp/centraldogma/xds/k8s/v1/XdsKubernetesServiceTest.java#L318-L320: Replace the builder merge withfinal KubernetesEndpointAggregator actual = XdsResourceManager.parseYaml(json, KubernetesEndpointAggregator.newBuilder());and assert on it directly.xds/src/test/java/com/linecorp/centraldogma/xds/listener/v1/XdsListenerServiceTest.java#L82-L84: Simplify parsing foractualListener.xds/src/test/java/com/linecorp/centraldogma/xds/listener/v1/XdsListenerServiceTest.java#L146-L148: Simplify parsing foractualListenerin the update test.xds/src/test/java/com/linecorp/centraldogma/xds/listener/v1/XdsListenerServiceTest.java#L158-L160: Simplify parsing foractualListener2.xds/src/test/java/com/linecorp/centraldogma/xds/route/v1/XdsRouteServiceTest.java#L82-L84: Simplify parsing foractualRoute.xds/src/test/java/com/linecorp/centraldogma/xds/route/v1/XdsRouteServiceTest.java#L145-L147: Simplify parsing foractualRoutein the update test.xds/src/test/java/com/linecorp/centraldogma/xds/route/v1/XdsRouteServiceTest.java#L157-L159: Simplify parsing foractualRoute2.🤖 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/test/java/com/linecorp/centraldogma/xds/endpoint/v1/XdsEndpointServiceTest.java` around lines 94 - 96, Replace manual YAML tree traversal and JSON_MESSAGE_MARSHALLER builder merges with XdsResourceManager.parseYaml in XdsEndpointServiceTest.java at lines 94-96, 160-162, 174-176, and 240-242, passing parsed ClusterLoadAssignment values directly to assertions or checkEndpointsViaDiscoveryRequest; in XdsKubernetesServiceTest.java at lines 318-320, parse json directly into KubernetesEndpointAggregator; and in XdsListenerServiceTest.java at lines 82-84, 146-148, 158-160 and XdsRouteServiceTest.java at lines 82-84, 145-147, 157-159, parse each response directly into the corresponding actualListener or actualRoute values using the existing builders.
🤖 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/centraldogma/xds/internal/ControlPlaneService.java`:
- Around line 152-155: Update ControlPlaneService.start() where
serverBuilder.dependencyInjector() is configured to compose
RequiresXdsGroupRoleDecoratorFactory with the ServerBuilder’s existing injector
chain rather than replacing it. Preserve CentralDogma’s installed injector and
reflective fallback while adding the XDS role decorator behavior.
In
`@xds/src/main/java/com/linecorp/centraldogma/xds/internal/RequiresXdsGroupRoleDecorator.java`:
- Around line 62-93: In RequiresXdsGroupRoleDecorator.serve, perform the
AuthUtil.currentUser authentication check immediately after validating the group
path format and before calling xdsProject.repos().exists(group). Return
UNAUTHORIZED for unauthenticated requests without revealing whether the group
exists, while preserving the existing not-found, system-admin, and role
authorization behavior for authenticated users.
In
`@xds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsResourceManager.java`:
- Around line 207-229: Update normalizeYamlKeys and collectSnakeCaseKeyRanges so
normalization targets only YAML keys corresponding to actual proto field names,
not arbitrary nested mapping keys in map<string, *> or Struct-backed values.
Pass or derive the relevant proto descriptor while traversing nodes, and only
collect ranges for keys declared as proto fields; preserve traversal of nested
message fields while leaving map and Struct contents unchanged.
---
Nitpick comments:
In
`@it/xds-k8s-node-ip-extractor/src/test/java/com/linecorp/centraldogma/it/xds/k8s/XdsKubernetesNodeIpExtractorTest.java`:
- Around line 321-323: Update the YAML request-body construction in the test
helper to use XdsResourceManager.toYamlBodyString() directly, replacing the
manual JSON serialization, Jackson parsing, and Yaml re-serialization while
preserving the existing execute call and request behavior.
In
`@xds/src/test/java/com/linecorp/centraldogma/xds/endpoint/v1/XdsEndpointServiceTest.java`:
- Around line 94-96: Replace manual YAML tree traversal and
JSON_MESSAGE_MARSHALLER builder merges with XdsResourceManager.parseYaml in
XdsEndpointServiceTest.java at lines 94-96, 160-162, 174-176, and 240-242,
passing parsed ClusterLoadAssignment values directly to assertions or
checkEndpointsViaDiscoveryRequest; in XdsKubernetesServiceTest.java at lines
318-320, parse json directly into KubernetesEndpointAggregator; and in
XdsListenerServiceTest.java at lines 82-84, 146-148, 158-160 and
XdsRouteServiceTest.java at lines 82-84, 145-147, 157-159, parse each response
directly into the corresponding actualListener or actualRoute values using the
existing builders.
In
`@xds/src/test/java/com/linecorp/centraldogma/xds/endpoint/v1/XdsRegisterEndpointTest.java`:
- Around line 252-254: Update the YAML conversion in the test helper to call
XdsTestUtil.toYaml() directly instead of manually serializing with
JSON_MESSAGE_MARSHALLER, Jackson.readTree, and Yaml.writeValueAsString; add the
XdsTestUtil import if needed and preserve the resulting request payload.
🪄 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: 881d83c2-ccd3-4af9-a38f-561ec551716f
📒 Files selected for processing (35)
it/xds-k8s-node-ip-extractor/src/test/java/com/linecorp/centraldogma/it/xds/k8s/XdsKubernetesNodeIpExtractorTest.javawebapp/src/dogma/features/xds/XdsTypes.tsxds/src/main/java/com/linecorp/centraldogma/xds/cluster/v1/XdsClusterService.javaxds/src/main/java/com/linecorp/centraldogma/xds/endpoint/v1/XdsEndpointService.javaxds/src/main/java/com/linecorp/centraldogma/xds/endpoint/v1/XdsEndpointUpdateScheduler.javaxds/src/main/java/com/linecorp/centraldogma/xds/group/v1/XdsGroupService.javaxds/src/main/java/com/linecorp/centraldogma/xds/internal/ControlPlaneService.javaxds/src/main/java/com/linecorp/centraldogma/xds/internal/RequiresXdsGroupRole.javaxds/src/main/java/com/linecorp/centraldogma/xds/internal/RequiresXdsGroupRoleDecorator.javaxds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsResourceManager.javaxds/src/main/java/com/linecorp/centraldogma/xds/k8s/v1/XdsKubernetesService.javaxds/src/main/java/com/linecorp/centraldogma/xds/listener/v1/XdsListenerService.javaxds/src/main/java/com/linecorp/centraldogma/xds/route/v1/XdsRouteService.javaxds/src/main/proto/centraldogma/xds/cluster/v1/xds_cluster.protoxds/src/main/proto/centraldogma/xds/endpoint/v1/xds_endpoint.protoxds/src/main/proto/centraldogma/xds/group/v1/xds_group.protoxds/src/main/proto/centraldogma/xds/listener/v1/xds_listener.protoxds/src/main/proto/centraldogma/xds/route/v1/xds_route.protoxds/src/test/java/com/linecorp/centraldogma/xds/cluster/v1/XdsClusterServiceTest.javaxds/src/test/java/com/linecorp/centraldogma/xds/endpoint/v1/XdsEndpointServiceTest.javaxds/src/test/java/com/linecorp/centraldogma/xds/endpoint/v1/XdsRegisterEndpointTest.javaxds/src/test/java/com/linecorp/centraldogma/xds/group/v1/XdsGroupServiceTest.javaxds/src/test/java/com/linecorp/centraldogma/xds/internal/MtlsDiscoveryAuthorizationTest.javaxds/src/test/java/com/linecorp/centraldogma/xds/internal/TokenDiscoveryAuthorizationTest.javaxds/src/test/java/com/linecorp/centraldogma/xds/internal/XdsEndpointReadPermissionTest.javaxds/src/test/java/com/linecorp/centraldogma/xds/internal/XdsGroupDeletePermissionTest.javaxds/src/test/java/com/linecorp/centraldogma/xds/internal/XdsLegacyJsonCompatibilityTest.javaxds/src/test/java/com/linecorp/centraldogma/xds/internal/XdsResourceManagerTest.javaxds/src/test/java/com/linecorp/centraldogma/xds/internal/XdsTestUtil.javaxds/src/test/java/com/linecorp/centraldogma/xds/internal/XdsWritePermissionTest.javaxds/src/test/java/com/linecorp/centraldogma/xds/internal/XdsYamlCompatibilityTest.javaxds/src/test/java/com/linecorp/centraldogma/xds/k8s/v1/KubernetesEndpointMetadataTest.javaxds/src/test/java/com/linecorp/centraldogma/xds/k8s/v1/XdsKubernetesServiceTest.javaxds/src/test/java/com/linecorp/centraldogma/xds/listener/v1/XdsListenerServiceTest.javaxds/src/test/java/com/linecorp/centraldogma/xds/route/v1/XdsRouteServiceTest.java
💤 Files with no reviewable changes (6)
- xds/src/main/proto/centraldogma/xds/group/v1/xds_group.proto
- xds/src/main/proto/centraldogma/xds/cluster/v1/xds_cluster.proto
- xds/src/main/proto/centraldogma/xds/listener/v1/xds_listener.proto
- xds/src/main/proto/centraldogma/xds/route/v1/xds_route.proto
- xds/src/test/java/com/linecorp/centraldogma/xds/k8s/v1/KubernetesEndpointMetadataTest.java
- webapp/src/dogma/features/xds/XdsTypes.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
xds/src/test/java/com/linecorp/centraldogma/xds/internal/XdsResourceManagerTest.java (2)
26-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for edge cases (missing trailing newline and Windows line endings).
To ensure
injectYamlFieldremains robust—especially in light of the regex adjustments for different line endings and files lacking a trailing newline—it would be beneficial to explicitly test these scenarios.💡 Proposed test cases
`@Test` void injectYamlField_missingTrailingNewline() { final String yaml = "name: old\ntype: EDS"; assertThat(XdsResourceManager.injectYamlField(yaml, "name", "new_name")) .isEqualTo("name: new_name\ntype: EDS"); } `@Test` void injectYamlField_windowsLineEndings() { final String yaml = "name: old\r\ntype: EDS\r\n"; // The replacement string injected by injectYamlField uses \n, so we expect a mix if the original had \r\n assertThat(XdsResourceManager.injectYamlField(yaml, "name", "new_name")) .isEqualTo("name: new_name\ntype: EDS\r\n"); }🤖 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/test/java/com/linecorp/centraldogma/xds/internal/XdsResourceManagerTest.java` around lines 26 - 31, Add tests alongside injectYamlField_replacesExistingField for inputs without a trailing newline and with Windows \r\n line endings. Assert the exact resulting strings, preserving the expected newline behavior described for each case.
26-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for edge cases (missing trailing newline and Windows line endings).
To ensure
injectYamlFieldremains robust—especially if the regex is updated—it would be beneficial to test YAML strings that use\r\nand those that do not end with a trailing newline.💡 Proposed test cases
`@Test` void injectYamlField_missingTrailingNewline() { final String yaml = "name: old\ntype: EDS"; assertThat(XdsResourceManager.injectYamlField(yaml, "name", "new_name")) .isEqualTo("name: new_name\ntype: EDS"); } `@Test` void injectYamlField_windowsLineEndings() { final String yaml = "name: old\r\ntype: EDS\r\n"; assertThat(XdsResourceManager.injectYamlField(yaml, "name", "new_name")) .isEqualTo("name: new_name\ntype: EDS\r\n"); }🤖 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/test/java/com/linecorp/centraldogma/xds/internal/XdsResourceManagerTest.java` around lines 26 - 31, Add edge-case tests alongside injectYamlField_replacesExistingField for YAML without a trailing newline and YAML using Windows \r\n line endings. Assert that the target field is replaced while the input’s newline style and trailing-newline presence are preserved.
🤖 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/centraldogma/xds/internal/XdsResourceManager.java`:
- Around line 172-184: The YAML matcher in the replacement logic should support
CRLF line endings and keys reaching EOF without a trailing newline. Update the
regex in the `pattern` used by the surrounding replacement method so both the
key line and indented continuation lines terminate with `(?:\r?\n|$)`, while
preserving camelCase/snake_case matching and block-scalar handling.
- Around line 172-184: Update the pattern in the YAML replacement logic around
the matcher to support a top-level field whose value ends at EOF without a
trailing newline, while preserving matching of newline-terminated fields and
indented continuation lines. Ensure name replacement does not prepend a
duplicate key for newline-less input.
---
Nitpick comments:
In
`@xds/src/test/java/com/linecorp/centraldogma/xds/internal/XdsResourceManagerTest.java`:
- Around line 26-31: Add tests alongside injectYamlField_replacesExistingField
for inputs without a trailing newline and with Windows \r\n line endings. Assert
the exact resulting strings, preserving the expected newline behavior described
for each case.
- Around line 26-31: Add edge-case tests alongside
injectYamlField_replacesExistingField for YAML without a trailing newline and
YAML using Windows \r\n line endings. Assert that the target field is replaced
while the input’s newline style and trailing-newline presence are preserved.
🪄 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: 78765cc7-1910-49b1-bbee-7beafdcb25c9
📒 Files selected for processing (8)
xds/src/main/java/com/linecorp/centraldogma/xds/cluster/v1/XdsClusterService.javaxds/src/main/java/com/linecorp/centraldogma/xds/endpoint/v1/XdsEndpointService.javaxds/src/main/java/com/linecorp/centraldogma/xds/internal/RequiresXdsGroupRoleDecorator.javaxds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsResourceManager.javaxds/src/main/java/com/linecorp/centraldogma/xds/k8s/v1/XdsKubernetesService.javaxds/src/main/java/com/linecorp/centraldogma/xds/listener/v1/XdsListenerService.javaxds/src/main/java/com/linecorp/centraldogma/xds/route/v1/XdsRouteService.javaxds/src/test/java/com/linecorp/centraldogma/xds/internal/XdsResourceManagerTest.java
🚧 Files skipped from review as they are similar to previous changes (6)
- xds/src/main/java/com/linecorp/centraldogma/xds/internal/RequiresXdsGroupRoleDecorator.java
- xds/src/main/java/com/linecorp/centraldogma/xds/listener/v1/XdsListenerService.java
- xds/src/main/java/com/linecorp/centraldogma/xds/endpoint/v1/XdsEndpointService.java
- xds/src/main/java/com/linecorp/centraldogma/xds/route/v1/XdsRouteService.java
- xds/src/main/java/com/linecorp/centraldogma/xds/k8s/v1/XdsKubernetesService.java
- xds/src/main/java/com/linecorp/centraldogma/xds/cluster/v1/XdsClusterService.java
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
xds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsResourceManager.java (2)
290-299: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPrefer the requested filename when both variants exist.
When both
.yamland legacy.jsonfiles are present,keySet().iterator().next()selects one based on map iteration order. An update or delete can therefore modify the wrong variant. ResolvefileNamefirst and use the alternative only when the requested file is absent.🤖 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/centraldogma/xds/internal/XdsResourceManager.java` around lines 290 - 299, Update the resource resolution logic around alternativeFileName and taskProvider.apply so it explicitly selects the requested fileName when entries contains both variants, falling back to altFileName only when fileName is absent. Preserve the existing not-found response when neither variant exists and pass the selected filename to taskProvider.
208-227: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake resource creation atomic.
The existence check and subsequent
Change.ofYamlUpsertare separate operations. Two concurrent creates can both pass the check, after which the second request silently overwrites the first instead of returning409 Conflict. Enforce create-if-absent semantics in the repository operation or detect the conflict during the write.🤖 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/centraldogma/xds/internal/XdsResourceManager.java` around lines 208 - 227, Update the create path around the existence check and doPush so creation is atomic: enforce create-if-absent semantics in the repository write, or detect a concurrent-write conflict and return HttpStatus.CONFLICT instead of overwriting the existing resource. Preserve normal updates and the existing Resource already exists response for conflicts.
🤖 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.
Outside diff comments:
In
`@xds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsResourceManager.java`:
- Around line 290-299: Update the resource resolution logic around
alternativeFileName and taskProvider.apply so it explicitly selects the
requested fileName when entries contains both variants, falling back to
altFileName only when fileName is absent. Preserve the existing not-found
response when neither variant exists and pass the selected filename to
taskProvider.
- Around line 208-227: Update the create path around the existence check and
doPush so creation is atomic: enforce create-if-absent semantics in the
repository write, or detect a concurrent-write conflict and return
HttpStatus.CONFLICT instead of overwriting the existing resource. Preserve
normal updates and the existing Resource already exists response for conflicts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bdc03f68-2b7c-427c-a733-b6127a8f74b9
📒 Files selected for processing (2)
xds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsResourceManager.javaxds/src/test/java/com/linecorp/centraldogma/xds/internal/XdsResourceManagerTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- xds/src/test/java/com/linecorp/centraldogma/xds/internal/XdsResourceManagerTest.java
minwoox
marked this pull request as ready for review
July 21, 2026 05:23
Contributor
Author
|
Will make changes for the UI in the follow-up PR. 😉 |
ikhoon
approved these changes
Jul 21, 2026
| .set(HttpHeaderNames.AUTHORIZATION, "Bearer anonymous") | ||
| .contentType(MediaType.JSON_UTF_8).build(); | ||
| return webClient.execute(headers, JSON_MESSAGE_MARSHALLER.writeValueAsString(endpoint)) | ||
| .contentType(MediaType.parse("application/yaml")).build(); |
Contributor
There was a problem hiding this comment.
Should we create a constant value of MediaType.parse("application/yaml")?
Contributor
Author
There was a problem hiding this comment.
It's added for production code:
https://github.com/line/centraldogma/pull/1337/changes#diff-e5c2302f8f9692ff7ce5ea7df6b42d7a4241ac562a37195614ca7deba434154bR75
Also, I'm planning to add this to Armeria.
| public CompletableFuture<HttpResponse> createCluster( | ||
| @Param("group") String group, | ||
| @Param("cluster_id") String clusterId, | ||
| @Param("summary") @Nullable String summary, |
Contributor
There was a problem hiding this comment.
A summary seems like an arbitrary value set by users. Is it safe to set in the query parameters? I'm concerned about potential issues related to encoding or string length.
Contributor
Author
There was a problem hiding this comment.
Is it safe to set in the query parameters? I'm concerned about potential issues related to encoding or string length.
Yeah, it's going to be encoded. Also the length limit is more than 2000 chars so I think it should be fine.
https://medium.com/suyeonme/effective-strategies-for-handling-long-query-strings-b790e1fddd65
Let's revisit this if this becomes an issue. 😉
| "Invalid cluster name: " + clusterName)); | ||
| } | ||
| try { | ||
| XdsResourceManager.parseYaml(body, Cluster.newBuilder()); |
Contributor
There was a problem hiding this comment.
Question) Would it make sense to fork ProtobufRequestConverterFunction so that YAML request-body parsing can be handled as a cross-cutting concern?
Contributor
Author
There was a problem hiding this comment.
I think there's not so much gain for forking the class. If you think, this will be useful, let's consider adding yaml support to the ProtobufRequestConverterFunction on upstream.
| * <p>Previews the endpoints that would be resolved for a Kubernetes endpoint aggregator | ||
| * without persisting it. | ||
| */ | ||
| @Blocking |
Contributor
There was a problem hiding this comment.
Question) Is @Blocking still necessary? The code seems to run asynchronously.
Contributor
Author
There was a problem hiding this comment.
We don't need it anymore. 😉
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:
The xDS resource management APIs (cluster, endpoint, listener, route, group, kubernetes) were implemented as gRPC services backed by service-specific
.protofiles. Migrating them to Armeria annotated HTTP services that consumeapplication/yamlremoves the protobuf service layer, simplifies the API surface, and makes the YAML-native storage format a first-class concern — clients send YAML directly and it is stored as-is (withname/clusterNameinjected).Modifications:
XdsClusterService,XdsEndpointService,XdsListenerService,XdsRouteServicefrom*ImplBasegRPC stubs to plain annotated HTTP services (@Post/@Put/@Delete,@Consumes("application/yaml")).RequiresXdsGroupRoleannotation andRequiresXdsGroupRoleDecoratorto replace the inlinecheckWritePermissioncalls with a declarative, per-method authorization mechanism.XdsResourceManagerso thatpush/update/deleteoperate on raw YAML strings instead of protobuf messages, and add helpersparseYaml,normalizeYamlKeys,injectYamlField, anderrorResponse.Result: