Add YAML/JSON backward compatibility for xDS resource files - #1324
Conversation
Motivation: - xDS resources are currently stored as .json files, but are planned to be migrated to .yaml format. - To support safe rollout and rollback, the system must handle both .json and .yaml files coexisting in the same repository without errors. Modifications: - XdsResourceWatchingService: accept EntryType.YAML alongside JSON during the initial repository scan; handle UPSERT_YAML change type in the diff watcher; normalize YAML content to JSON before invoking handleXdsResource() so all implementations continue to use JSON_MESSAGE_MARSHALLER without format-specific logic. - XdsResourceManager: add alternativeFileName() helper that swaps between .json and .yaml extensions; change updateOrDelete() to search for both the requested filename and its alternative so that update and delete operations locate the file regardless of which format it was stored in; pass the resolved filename back to the task via Function<String, Runnable> so the correct file is mutated; make push() emit Change.ofYamlUpsert when the resolved filename ends with .yaml. Result: - .yaml and .json xDS resource files can coexist in the same group repository; the control plane, CRUD API, and endpoint read API all handle both formats transparently.
📝 WalkthroughWalkthroughThis PR extends YAML handling through content transformation, XDS resource watching, endpoint read/update flows, and related tests while preserving JSON behavior. ChangesYAML Support Across XDS and Change Application
Estimated code review effort: 4 (Complex) | ~60 minutes 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
xds/src/main/java/com/linecorp/centraldogma/xds/endpoint/v1/XdsEndpointUpdateScheduler.java (1)
183-202: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDetect duplicate endpoint files before transforming.
This lookup has the same ambiguity as the resource manager: if both endpoint variants exist,
FIND_ONE_WITHOUT_CONTENTmakes the batch update transform an arbitrary format/path. Fetch both candidates and reject the update when both*.jsonand*.yamlare present for the same endpoint.🤖 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/endpoint/v1/XdsEndpointUpdateScheduler.java` around lines 183 - 202, The endpoint lookup in XdsEndpointUpdateScheduler is ambiguous when both JSON and YAML variants exist because find(Revision.HEAD, fileName + ',' + altFileName, FIND_ONE_WITHOUT_CONTENT) returns an arbitrary match. Update this flow to explicitly inspect both candidates before creating the ContentTransformer and BatchUpdateTransformer, and reject the update with an error if both the primary and alternative endpoint files are present. Keep the fix localized around the repository.find handling and the resolvedFileName/entryType selection logic.xds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsResourceManager.java (1)
275-288: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDon’t hide duplicate JSON/YAML variants with
FIND_ONE.Using
FIND_ONE_WITHOUT_CONTENTmeans the code cannot tell whether bothfoo.jsonandfoo.yamlexist for the same logical resource; update/delete then mutates whichever entry is returned first. Fetch both candidates and fail with a deterministic conflict if both are present, or explicitly define precedence.🤖 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 275 - 288, Fetching with XdsResourceManager’s repository.find using FIND_ONE_WITHOUT_CONTENT hides when both the primary and alternative files exist, so update the resolution logic to inspect both candidates explicitly. In XdsResourceManager around alternativeFileName and the repository.find/handle flow, fetch both fileName and altFileName, detect when both entries are present, and return a deterministic conflict/error instead of silently choosing the first result. If only one exists, continue with that resolvedFileName; if neither exists, keep the NOT_FOUND behavior.xds/src/main/java/com/linecorp/centraldogma/xds/k8s/v1/XdsKubernetesEndpointFetchingService.java (1)
181-196: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRemove the generated
.jsonendpoint when deleting a YAML aggregator.Line 191 keeps the removed aggregator’s extension, but
pushK8sEndpoints()writes generated endpoint files as.jsonon Line 318. Deleting/k8s/endpointAggregators/foo.yamlwill try to remove/k8s/endpoints/foo.yamland leave the generated/k8s/endpoints/foo.jsonstale.Proposed fix
- final String aggregatorName = "groups/" + groupName + path.substring(0, path.length() - 5); + final String aggregatorPath = path.substring(0, path.length() - 5); + final String aggregatorName = "groups/" + groupName + aggregatorPath; if (updaters != null) { final KubernetesEndpointsUpdater updater = updaters.get(aggregatorName); if (updater != null) { updater.close(); } } // Remove corresponding endpoints. - final String endpointPath = AGGREGATORS_REPLCACE_PATTERN.matcher(path).replaceFirst("/endpoints/"); + final String endpointPath = + AGGREGATORS_REPLCACE_PATTERN.matcher(aggregatorPath).replaceFirst("/endpoints/") + ".json";🤖 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/k8s/v1/XdsKubernetesEndpointFetchingService.java` around lines 181 - 196, The endpoint removal path in XdsKubernetesEndpointFetchingService should match the generated filename used by pushK8sEndpoints(). When deleting an aggregator in the removal logic around AGGATORS_REPLCACE_PATTERN and endpointPath, strip the source extension from the aggregator name before building the corresponding /endpoints path so a deleted .yaml aggregator removes the generated .json endpoint instead of leaving it stale. Use the existing aggregatorName and endpointPath handling in XdsKubernetesEndpointFetchingService to normalize the target name consistently with pushK8sEndpoints().
🧹 Nitpick comments (1)
xds/src/test/java/com/linecorp/centraldogma/xds/internal/XdsResourceWatchingServiceTest.java (1)
56-121: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winExecutor threads leaked in new tests.
Both
yamlFilesAreHandledLikeJsonandyamlFilesLoadedDuringInitcreate a freshnewSingleThreadScheduledExecutor()per anonymous subclass instance but never shut it down, leaking a thread per test run (unlikeTestXdsResourceWatchingServicebelow, which reuses a single static executor).♻️ Proposed fix (apply to both tests)
- private final ScheduledExecutorService exec = Executors.newSingleThreadScheduledExecutor(); + private final ScheduledExecutorService exec = Executors.newSingleThreadScheduledExecutor(); + // ...Add cleanup, e.g.:
svc.init(); + try { // test body + } finally { + exec.shutdownNow(); + }Also applies to: 138-183
🤖 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/XdsResourceWatchingServiceTest.java` around lines 56 - 121, The anonymous XdsResourceWatchingService test instances leak their per-test ScheduledExecutorService because executor() returns a fresh newSingleThreadScheduledExecutor() with no shutdown. Update both yaml-related tests to either reuse a shared executor like TestXdsResourceWatchingService or add cleanup that shuts down the executor after the test completes, referencing the executor() override in XdsResourceWatchingServiceTest.
🤖 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/XdsEndpointReadService.java`:
- Around line 83-100: The endpoint selection in
XdsEndpointReadService.readByBase is relying on Repository.find() map iteration
order, so the returned format is not explicitly prioritized. Update readByBase
(and the getK8sEndpoint path it serves) to resolve the resource by checking the
.json variant first and then falling back to .yaml directly, instead of using
entries.values().iterator().next(); keep the existing get() fallback behavior
for the not-found case.
In
`@xds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsResourceManager.java`:
- Around line 187-190: The create path in XdsResourceManager is only checking
the requested file name, so it can miss an existing migrated *.yaml resource and
create a duplicate *.json entry instead of failing with ALREADY_EXISTS. Update
the create branch around Change.ofJsonPatch in XdsResourceManager to check both
the requested extension and the alternate extension before constructing the
change, using the existing resource lookup logic that handles fileName and the
.yaml/.json variants.
---
Outside diff comments:
In
`@xds/src/main/java/com/linecorp/centraldogma/xds/endpoint/v1/XdsEndpointUpdateScheduler.java`:
- Around line 183-202: The endpoint lookup in XdsEndpointUpdateScheduler is
ambiguous when both JSON and YAML variants exist because find(Revision.HEAD,
fileName + ',' + altFileName, FIND_ONE_WITHOUT_CONTENT) returns an arbitrary
match. Update this flow to explicitly inspect both candidates before creating
the ContentTransformer and BatchUpdateTransformer, and reject the update with an
error if both the primary and alternative endpoint files are present. Keep the
fix localized around the repository.find handling and the
resolvedFileName/entryType selection logic.
In
`@xds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsResourceManager.java`:
- Around line 275-288: Fetching with XdsResourceManager’s repository.find using
FIND_ONE_WITHOUT_CONTENT hides when both the primary and alternative files
exist, so update the resolution logic to inspect both candidates explicitly. In
XdsResourceManager around alternativeFileName and the repository.find/handle
flow, fetch both fileName and altFileName, detect when both entries are present,
and return a deterministic conflict/error instead of silently choosing the first
result. If only one exists, continue with that resolvedFileName; if neither
exists, keep the NOT_FOUND behavior.
In
`@xds/src/main/java/com/linecorp/centraldogma/xds/k8s/v1/XdsKubernetesEndpointFetchingService.java`:
- Around line 181-196: The endpoint removal path in
XdsKubernetesEndpointFetchingService should match the generated filename used by
pushK8sEndpoints(). When deleting an aggregator in the removal logic around
AGGATORS_REPLCACE_PATTERN and endpointPath, strip the source extension from the
aggregator name before building the corresponding /endpoints path so a deleted
.yaml aggregator removes the generated .json endpoint instead of leaving it
stale. Use the existing aggregatorName and endpointPath handling in
XdsKubernetesEndpointFetchingService to normalize the target name consistently
with pushK8sEndpoints().
---
Nitpick comments:
In
`@xds/src/test/java/com/linecorp/centraldogma/xds/internal/XdsResourceWatchingServiceTest.java`:
- Around line 56-121: The anonymous XdsResourceWatchingService test instances
leak their per-test ScheduledExecutorService because executor() returns a fresh
newSingleThreadScheduledExecutor() with no shutdown. Update both yaml-related
tests to either reuse a shared executor like TestXdsResourceWatchingService or
add cleanup that shuts down the executor after the test completes, referencing
the executor() override in XdsResourceWatchingServiceTest.
🪄 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: 918855da-c4ec-4f41-984c-c75afd583066
📒 Files selected for processing (14)
server/src/main/java/com/linecorp/centraldogma/server/command/ContentTransformer.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/TransformingChangesApplier.javaxds/src/main/java/com/linecorp/centraldogma/xds/endpoint/v1/XdsEndpointUpdateScheduler.javaxds/src/main/java/com/linecorp/centraldogma/xds/internal/CentralDogmaXdsResources.javaxds/src/main/java/com/linecorp/centraldogma/xds/internal/ControlPlaneService.javaxds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsEndpointReadService.javaxds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsResourceManager.javaxds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsResourceWatchingService.javaxds/src/main/java/com/linecorp/centraldogma/xds/k8s/v1/XdsKubernetesEndpointFetchingService.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/internal/XdsEndpointReadServiceTest.javaxds/src/test/java/com/linecorp/centraldogma/xds/internal/XdsResourceWatchingServiceTest.javaxds/src/test/java/com/linecorp/centraldogma/xds/internal/XdsYamlCompatibilityTest.java
| } | ||
| task.run(); | ||
| final String resolvedFileName = entries.keySet().iterator().next(); | ||
| taskProvider.apply(resolvedFileName).run(); |
There was a problem hiding this comment.
Question) If Runnable is invoked immediately, should we simply take Function<String, Void> or Consumer<String>?
| final JsonNode jsonNode = Jackson.readTree(jsonText); | ||
| if (create) { | ||
| change = Change.ofJsonPatch(fileName, null, jsonNode); | ||
| } else if (fileName.endsWith(".yaml")) { |
There was a problem hiding this comment.
I understand that only json files will be pushed in this version.
There was a problem hiding this comment.
That's correct because this is for compatibility.
| }); | ||
| return; | ||
| } | ||
| doPush(responseObserver, group, resourceName, fileName, summary, resource, author, false); |
There was a problem hiding this comment.
Question) I'm wondering if there is a case where a.json is updated while a.json is being migrated to a.yaml? It seems I may not have understood the current mechanism correctly.
There was a problem hiding this comment.
a.json will be removed and a.yaml will be created at once:
https://github.com/line/centraldogma/pull/1321/changes#diff-e5c2302f8f9692ff7ce5ea7df6b42d7a4241ac562a37195614ca7deba434154bR225-R228
There was a problem hiding this comment.
I understand a.json can't be modified due to the removal after a.yaml is created.
jrhee17
left a comment
jrhee17
left a comment
There was a problem hiding this comment.
👍 👍
Understood the changes as no migration logic, but just supporting read for both json/yaml formats
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 (1)
xds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsResourceManager.java (1)
180-208: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftResidual cross-extension TOCTOU on create, despite the new pre-check.
The new pre-check (lines 187-204) closes the gap flagged in the previous review for the common case, but a narrow race remains: between the
repository.find(...)check and the actual commit indoPush, a concurrent operation (e.g. a migration convertinga.json→a.yaml, or another create request for the alternate extension) can create the alternate file. SinceChange.ofJsonPatch(fileName, null, jsonNode)at line 218 only conflicts on the literalfileName, it will not detect thataltFileNamenow exists, allowing botha.jsonanda.yamlto co-exist for the same logical resource — defeating the dual-extension invariant this PR is meant to preserve.This is a narrow window and may be acceptable given how migrations are expected to run, but worth confirming it's an accepted tradeoff rather than an oversight.
🤖 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 180 - 208, The create-path pre-check in XdsResourceManager.push still has a TOCTOU gap because the alternate extension can appear after repository.find(...) but before doPush commits. Update the create flow so doPush (or the commit path it uses, including Change.ofJsonPatch) revalidates both fileName and alternativeFileName immediately before applying the change, or otherwise makes the existence check atomic with the write. Keep the dual-extension invariant enforced in the push/create path rather than relying only on the earlier pre-check.
🧹 Nitpick comments (1)
xds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsResourceManager.java (1)
187-189: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated dual-extension lookup logic.
The comma-joined
fileName + ',' + altFileNamelookup and itsfind(...)/isEmpty()handling in the create pre-check (187-189) is duplicated almost verbatim inupdateOrDelete(302-306). Consider extracting a shared private helper (e.g.findResolvedEntry(Repository, String fileName)returning theCompletableFuture<Map<String, Entry<?>>>) to avoid the two call sites drifting apart.♻️ Sketch of a shared helper
+ private CompletableFuture<Map<String, Entry<?>>> findWithAlternative( + Repository repository, String fileName) { + final String altFileName = alternativeFileName(fileName); + return repository.find(Revision.HEAD, fileName + ',' + altFileName, FIND_ONE_WITHOUT_CONTENT); + }Also applies to: 300-307
🤖 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 187 - 189, The dual-extension lookup logic is duplicated between the create pre-check and updateOrDelete paths. Extract the shared repository lookup into a private helper such as findResolvedEntry(Repository, String) in XdsResourceManager, and have both call sites use it for the fileName/alternativeFileName find(...) plus empty-check handling so the behavior stays consistent in one place.
🤖 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 180-208: The create-path pre-check in XdsResourceManager.push
still has a TOCTOU gap because the alternate extension can appear after
repository.find(...) but before doPush commits. Update the create flow so doPush
(or the commit path it uses, including Change.ofJsonPatch) revalidates both
fileName and alternativeFileName immediately before applying the change, or
otherwise makes the existence check atomic with the write. Keep the
dual-extension invariant enforced in the push/create path rather than relying
only on the earlier pre-check.
---
Nitpick comments:
In
`@xds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsResourceManager.java`:
- Around line 187-189: The dual-extension lookup logic is duplicated between the
create pre-check and updateOrDelete paths. Extract the shared repository lookup
into a private helper such as findResolvedEntry(Repository, String) in
XdsResourceManager, and have both call sites use it for the
fileName/alternativeFileName find(...) plus empty-check handling so the behavior
stays consistent in one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c2743ddc-d0e6-427e-9441-3cd5cf6ca3bb
📒 Files selected for processing (1)
xds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsResourceManager.java
3 participants
Motivation:
Modifications:
Result: