Add MirrorFileValidator SPI and xDS mirroring UI - #1330
Merged
Conversation
Motivation: When a Git repository is mirrored into an @xds repository, there was no mechanism to reject invalid xDS resource files before they were committed. A typo or structurally-wrong protobuf file would silently land in the repository and break the control plane at push time rather than at the source. Additionally, the xDS UI had no way to manage the mirrors and credentials that back an xDS group. Modifications: - Add `MirrorFileValidator`, a `@FunctionalInterface` SPI loaded via `ServiceLoader`. Implementations are invoked in `AbstractMirror` inside a new `validateChanges()` method called by both `AbstractGitMirror` and `CentralDogmaMirror` before each push. All validation errors are collected and surfaced as a single `MirrorException`; mirror state files are excluded from validation. - Add `XdsMirrorFileValidator`, the xDS-specific implementation. It rejects changes to the `@xds` project that target paths reserved for the Kubernetes controller (`/k8s/endpoints/`), paths outside the recognised xDS resource directories, or content that cannot be parsed as the expected protobuf message type (Cluster, Listener, RouteConfiguration, ClusterLoadAssignment, KubernetesEndpointAggregator). - Enforce in `DefaultMetaRepository` that xDS mirrors must use `localPath: /`. - Add `XdsMirroringTab` and xDS-scoped pages for creating, viewing, and editing mirrors (`/app/xds/mirrors/*`) and credentials (`/app/xds/credentials/*`), reusing the existing `MirrorForm`, `MirrorList`, `MirrorView`, `CredentialForm`, and `CredentialList` components with xDS-specific URL routing. Result: - Mirroring an invalid or structurally-wrong file into an @xds repository now fails fast. - Users can create, view, and edit xDS mirrors and credentials directly from the xDS group UI.
|
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:
📝 WalkthroughWalkthroughAdds pluggable validation before mirror pushes, xDS resource validation and project-constant centralization, root-path enforcement for xDS mirrors, and an admin UI for managing xDS mirrors and credentials. ChangesXDS mirroring
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Admin
participant XdsMirroringTab
participant MirrorList
participant MirrorAPI
Admin->>XdsMirroringTab: open mirroring section
XdsMirroringTab->>MirrorList: load group mirrors
MirrorList->>MirrorAPI: request mirror data
Admin->>MirrorAPI: create or update mirror
MirrorAPI-->>Admin: return result
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
🧹 Nitpick comments (4)
server/src/main/java/com/linecorp/centraldogma/server/mirror/MirrorFileValidator.java (1)
21-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winJavadoc overstates validation scope; missing
@throwsdoc.The javadoc says validators are Implementations are loaded via {
@linkjava.util.ServiceLoader} and invoked in {@linkcom.linecorp.centraldogma.server.internal.mirror.AbstractMirror} before each push. ButvalidateChangesis only called frommirrorRemoteToLocalin bothCentralDogmaMirrorandAbstractGitMirror— never frommirrorLocalToRemote. A validator author reading this doc could reasonably assume LOCAL_TO_REMOTE changes are also checked, which is not the case. Also worth documenting thatvalidate()signals failure viaMirrorException.✏️ Suggested doc fix
/** * Validates file changes before they are committed to a repository during mirroring. * - * <p>Implementations are loaded via {`@link` java.util.ServiceLoader} and invoked in - * {`@link` com.linecorp.centraldogma.server.internal.mirror.AbstractMirror} before each push.</p> + * <p>Implementations are loaded via {`@link` java.util.ServiceLoader} and invoked in + * {`@link` com.linecorp.centraldogma.server.internal.mirror.AbstractMirror} before a + * remote-to-local push commits changes to the local repository.</p> */ `@FunctionalInterface` public interface MirrorFileValidator { /** * Validates a file change before it is committed to a repository during mirroring. + * + * `@throws` com.linecorp.centraldogma.common.MirrorException if the change is invalid */ void validate(String projectName, String repoName, Change<?> change); }🤖 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 `@server/src/main/java/com/linecorp/centraldogma/server/mirror/MirrorFileValidator.java` around lines 21 - 34, Update the Javadoc for MirrorFileValidator to state that validation applies only to changes mirrored from remote to local, not before every push or during local-to-remote mirroring. Document that validate(String projectName, String repoName, Change<?> change) signals validation failure by throwing MirrorException.xds/src/test/java/com/linecorp/centraldogma/xds/internal/XdsMirrorFileValidatorTest.java (1)
57-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a test for the text-content path with a valid file path.
All tests with valid paths use
yamlChangeOf(which producesJsonNodecontent), exercising theif (content instanceof JsonNode)branch. The only test usingChange.ofTextUpsert(unexpectedPath_rejected) fails at path validation before reaching content parsing. Theelsebranch at line 88 ofXdsMirrorFileValidator(text content with a valid path) is not covered.♻️ Suggested additional test
`@Test` void jsonTextContent_validPath_passes() { final String json = JSON_MESSAGE_MARSHALLER.writeValueAsString(sampleCluster()); assertThatCode(() -> VALIDATOR.validate( XDS_CENTRAL_DOGMA_PROJECT, REPO_NAME, Change.ofJsonUpsert("/clusters/my-cluster.json", json))) .doesNotThrowAnyException(); }Also applies to: 206-214
🤖 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/XdsMirrorFileValidatorTest.java` around lines 57 - 64, Add a test in XdsMirrorFileValidatorTest covering text content with a valid path, such as a JSON string created from sampleCluster() and passed to Change.ofJsonUpsert("/clusters/my-cluster.json", ...), asserting that VALIDATOR.validate does not throw. This should exercise the non-JsonNode content branch in XdsMirrorFileValidator, unlike unexpectedPath_rejected.webapp/src/pages/app/xds/credentials/[id]/edit/index.tsx (1)
46-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant error check after
.unwrap().RTK Query's
.unwrap()(line 54) already throws on mutation error, so the manual.errorcheck on lines 55-57 is dead code — the catch block on line 61 handles all failures. This appears to be an existing pattern in the codebase, so it's not a regression, but consider simplifying in a follow-up.♻️ Simplified onSubmit
const onSubmit = async (credential: CredentialDto, onSuccess: () => void) => { try { credential.name = `projects/@xds/repos/${group}/credentials/${credential.id}`; - const response = await updateCredential({ + await updateCredential({ projectName: '`@xds`', id, credential, repoName: group, }).unwrap(); - if ((response as { error: FetchBaseQueryError | SerializedError }).error) { - throw (response as { error: FetchBaseQueryError | SerializedError }).error; - } dispatch(newNotification(`Credential '${credential.id}' is updated`, 'Successfully updated', 'success')); onSuccess(); Router.push(`/app/xds/credentials/${encodeURIComponent(id)}?group=${encodeURIComponent(group)}`); } catch (error) { dispatch(newNotification('Failed to update the credential', ErrorMessageParser.parse(error), 'error')); } };🤖 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 `@webapp/src/pages/app/xds/credentials/`[id]/edit/index.tsx around lines 46 - 64, Remove the redundant response.error check and cast after updateCredential(...).unwrap() in onSubmit; rely on unwrap() to throw mutation failures and let the existing catch block handle them, while retaining the success notification, callback, and redirect behavior.webapp/src/pages/app/xds/mirrors/new.tsx (1)
67-70: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead error-check after
.unwrap()— consider simplifying.
addNewMirror(formData).unwrap()rejects on error, so by line 68responseis the success data. Checkingresponse.erroris unreachable in practice. Not harmful, but adds noise.♻️ Suggested simplification
- const response = await addNewMirror(formData).unwrap(); - if ((response as { error: FetchBaseQueryError | SerializedError }).error) { - throw (response as { error: FetchBaseQueryError | SerializedError }).error; - } + await addNewMirror(formData).unwrap();🤖 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 `@webapp/src/pages/app/xds/mirrors/new.tsx` around lines 67 - 70, Remove the redundant response.error check and cast after addNewMirror(formData).unwrap() in the relevant submit handler; rely on unwrap() to reject failures and treat response directly as successful data.
🤖 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
`@server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/DefaultMetaRepository.java`:
- Around line 421-425: Update the xDS validation in DefaultMetaRepository to
account for AbstractMirror normalizing an empty localPath to the root path. In
the XDS_PROJECT_NAME branch, compare the normalized path or accept both "/" and
"" before throwing the argument error, while preserving the existing rejection
of other paths.
In `@webapp/src/pages/app/xds/credentials/`[id]/index.tsx:
- Around line 28-31: Add non-null assertions to both route parameters in the
useGetRepoCredentialQuery call: pass group! and id! while retaining the existing
skip condition. This aligns the credentials page with the mirror pages and
satisfies the query hook’s required string types.
---
Nitpick comments:
In
`@server/src/main/java/com/linecorp/centraldogma/server/mirror/MirrorFileValidator.java`:
- Around line 21-34: Update the Javadoc for MirrorFileValidator to state that
validation applies only to changes mirrored from remote to local, not before
every push or during local-to-remote mirroring. Document that validate(String
projectName, String repoName, Change<?> change) signals validation failure by
throwing MirrorException.
In `@webapp/src/pages/app/xds/credentials/`[id]/edit/index.tsx:
- Around line 46-64: Remove the redundant response.error check and cast after
updateCredential(...).unwrap() in onSubmit; rely on unwrap() to throw mutation
failures and let the existing catch block handle them, while retaining the
success notification, callback, and redirect behavior.
In `@webapp/src/pages/app/xds/mirrors/new.tsx`:
- Around line 67-70: Remove the redundant response.error check and cast after
addNewMirror(formData).unwrap() in the relevant submit handler; rely on unwrap()
to reject failures and treat response directly as successful data.
In
`@xds/src/test/java/com/linecorp/centraldogma/xds/internal/XdsMirrorFileValidatorTest.java`:
- Around line 57-64: Add a test in XdsMirrorFileValidatorTest covering text
content with a valid path, such as a JSON string created from sampleCluster()
and passed to Change.ofJsonUpsert("/clusters/my-cluster.json", ...), asserting
that VALIDATOR.validate does not throw. This should exercise the non-JsonNode
content branch in XdsMirrorFileValidator, unlike unexpectedPath_rejected.
🪄 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: 610295f1-5460-4a4c-9e07-fb5ddee6eb68
📒 Files selected for processing (28)
server-mirror-dogma/src/main/java/com/linecorp/centraldogma/server/internal/mirror/CentralDogmaMirror.javaserver-mirror-git/src/main/java/com/linecorp/centraldogma/server/internal/mirror/AbstractGitMirror.javaserver-mirror-git/src/test/java/com/linecorp/centraldogma/server/internal/mirror/DefaultMetaRepositoryWithMirrorTest.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/mirror/AbstractMirror.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/DefaultMetaRepository.javaserver/src/main/java/com/linecorp/centraldogma/server/mirror/MirrorFileValidator.javawebapp/src/dogma/features/project/settings/credentials/CredentialForm.tsxwebapp/src/dogma/features/project/settings/credentials/CredentialList.tsxwebapp/src/dogma/features/project/settings/credentials/CredentialView.tsxwebapp/src/dogma/features/repo/settings/mirrors/MirrorForm.tsxwebapp/src/dogma/features/repo/settings/mirrors/MirrorList.tsxwebapp/src/dogma/features/repo/settings/mirrors/MirrorView.tsxwebapp/src/dogma/features/xds/CredentialsTab.tsxwebapp/src/dogma/features/xds/Sidebar.tsxwebapp/src/dogma/features/xds/XdsMirroringTab.tsxwebapp/src/dogma/features/xds/useXdsRoute.tswebapp/src/pages/app/xds/credentials/[id]/edit/index.tsxwebapp/src/pages/app/xds/credentials/[id]/index.tsxwebapp/src/pages/app/xds/credentials/new.tsxwebapp/src/pages/app/xds/group.tsxwebapp/src/pages/app/xds/mirrors/[id]/edit/index.tsxwebapp/src/pages/app/xds/mirrors/[id]/index.tsxwebapp/src/pages/app/xds/mirrors/new.tsxxds/build.gradlexds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsMirrorFileValidator.javaxds/src/main/java/com/linecorp/centraldogma/xds/k8s/v1/XdsKubernetesService.javaxds/src/main/resources/META-INF/services/com.linecorp.centraldogma.server.mirror.MirrorFileValidatorxds/src/test/java/com/linecorp/centraldogma/xds/internal/XdsMirrorFileValidatorTest.java
minwoox
marked this pull request as ready for review
July 10, 2026 07:48
ikhoon
approved these changes
Jul 10, 2026
| try { | ||
| validator.validate(projectName, repoName, change); | ||
| } catch (MirrorException e) { | ||
| errors.add(e.getMessage()); |
Contributor
There was a problem hiding this comment.
Should we add the MirrorException as a suppressed exception so that we could see the original exception in the centraldogma.log?
| } | ||
| } | ||
|
|
||
| private static Message.@Nullable Builder builderForPath(String filePath) { |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
xds/src/main/java/com/linecorp/centraldogma/xds/k8s/v1/XdsKubernetesEndpointFetchingService.java (1)
198-199: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSafeguard against unsupported file extensions.
path.substring(0, path.length() - 5)strictly assumes that the removed file has a 5-character extension (.yamlor.json). Because the watcher observes all files matching**, removing a file with a different extension (e.g.,.ymlor.txt) will either compute a corrupted endpoint path or throw aStringIndexOutOfBoundsException, which could disrupt the watcher thread.Consider adding an early return at the beginning of the
onFileRemovedmethod to safely ignore unsupported file formats.🛡️ Proposed safeguard
`@Override` protected void onFileRemoved(String groupName, String path) { + if (!path.endsWith(".yaml") && !path.endsWith(".json")) { + return; + } final Map<String, KubernetesEndpointsUpdater> updaters = kubernetesEndpointsUpdaters.get(groupName);🤖 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 198 - 199, Add an early guard at the start of onFileRemoved to return for paths that do not end with the supported .yaml or .json extensions, before the path.substring operation. Preserve existing endpoint removal behavior for supported files.
🤖 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/centraldogma/xds/k8s/v1/XdsKubernetesEndpointFetchingService.java`:
- Around line 198-199: Add an early guard at the start of onFileRemoved to
return for paths that do not end with the supported .yaml or .json extensions,
before the path.substring operation. Preserve existing endpoint removal behavior
for supported files.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0ac8e94b-f737-48ec-b5a5-466cb30d430f
📒 Files selected for processing (11)
it/xds-k8s-node-ip-extractor/src/test/java/com/linecorp/centraldogma/it/xds/k8s/XdsKubernetesNodeIpExtractorTest.javaxds/src/main/java/com/linecorp/centraldogma/xds/endpoint/v1/XdsEndpointUpdateScheduler.javaxds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsResourceManager.javaxds/src/main/java/com/linecorp/centraldogma/xds/k8s/v1/XdsKubernetesEndpointFetchingService.javaxds/src/main/java/com/linecorp/centraldogma/xds/k8s/v1/XdsKubernetesService.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/XdsLegacyJsonCompatibilityTest.javaxds/src/test/java/com/linecorp/centraldogma/xds/internal/XdsMirrorFileValidatorTest.javaxds/src/test/java/com/linecorp/centraldogma/xds/k8s/v1/AggregatingMultipleKubernetesTest.javaxds/src/test/java/com/linecorp/centraldogma/xds/k8s/v1/XdsKubernetesServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (8)
- xds/src/test/java/com/linecorp/centraldogma/xds/k8s/v1/AggregatingMultipleKubernetesTest.java
- xds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsResourceManager.java
- xds/src/main/java/com/linecorp/centraldogma/xds/endpoint/v1/XdsEndpointUpdateScheduler.java
- xds/src/test/java/com/linecorp/centraldogma/xds/endpoint/v1/XdsRegisterEndpointTest.java
- xds/src/test/java/com/linecorp/centraldogma/xds/internal/XdsEndpointReadServiceTest.java
- xds/src/test/java/com/linecorp/centraldogma/xds/k8s/v1/XdsKubernetesServiceTest.java
- xds/src/main/java/com/linecorp/centraldogma/xds/k8s/v1/XdsKubernetesService.java
- it/xds-k8s-node-ip-extractor/src/test/java/com/linecorp/centraldogma/it/xds/k8s/XdsKubernetesNodeIpExtractorTest.java
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1330 +/- ##
============================================
- Coverage 69.46% 68.98% -0.49%
+ Complexity 5709 5679 -30
============================================
Files 540 541 +1
Lines 24207 24267 +60
Branches 2771 2786 +15
============================================
- Hits 16816 16740 -76
- Misses 5880 6003 +123
- Partials 1511 1524 +13 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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:
When a Git repository is mirrored into an @xds repository, there was no mechanism to reject invalid xDS resource files before they were committed. A typo or structurally-wrong protobuf file would silently land in the repository and break the control plane at push time rather than at the source. Additionally, the xDS UI had no way to manage the mirrors and credentials that back an xDS group.
Modifications:
MirrorFileValidator, a@FunctionalInterfaceSPI loaded viaServiceLoader. Implementations are invoked inAbstractMirrorinside a newvalidateChanges()method called by bothAbstractGitMirrorandCentralDogmaMirrorbefore each push. All validation errors are collected and surfaced as a singleMirrorException; mirror state files are excluded from validation.XdsMirrorFileValidator, the xDS-specific implementation. It rejects changes to the@xdsproject that target paths reserved for the Kubernetes controller (/k8s/endpoints/), paths outside the recognised xDS resource directories, or content that cannot be parsed as the expected protobuf message type (Cluster, Listener, RouteConfiguration, ClusterLoadAssignment, KubernetesEndpointAggregator).DefaultMetaRepositorythat xDS mirrors must uselocalPath: /.XdsMirroringTaband xDS-scoped pages for creating, viewing, and editing mirrors (/app/xds/mirrors/*) and credentials (/app/xds/credentials/*), reusing the existingMirrorForm,MirrorList,MirrorView,CredentialForm, andCredentialListcomponents with xDS-specific URL routing.Result:
@xdsrepository now fails fast.