Add a web UI for the xDS control plane - #1317
Merged
Merged
Conversation
|
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:
📝 WalkthroughWalkthroughThis PR adds role-based write/delete authorization to all xDS resource services, restricts internal project visibility to ChangesxDS Authorization, Scoped Snapshots, and Web Application
Sequence Diagram(s)sequenceDiagram
participant Browser
participant NextjsApp
participant xdsApiSlice
participant ControlPlaneService
participant CentralDogmaXdsResources
participant MetadataService
Browser->>NextjsApp: GET /app/xds (xDS web UI)
NextjsApp->>xdsApiSlice: useIsXdsWebEnabledQuery()
xdsApiSlice->>ControlPlaneService: GET /api/v1/xds/web
ControlPlaneService-->>xdsApiSlice: {"enabled":true}
Browser->>xdsApiSlice: useGetGroupsQuery()
xdsApiSlice->>ControlPlaneService: GET /api/v1/xds/groups
Browser->>xdsApiSlice: useListResourcesQuery(group, type)
xdsApiSlice->>ControlPlaneService: GET /api/v1/projects/@xds/repos/{group}/contents/{type}
Note over Browser,MetadataService: Envoy discovery path
Browser->>ControlPlaneService: gRPC CDS/LDS/RDS (with app token)
ControlPlaneService->>MetadataService: groupsWithReadAccess(metadata, appId)
ControlPlaneService->>CentralDogmaXdsResources: snapshot(readableGroups)
CentralDogmaXdsResources-->>ControlPlaneService: filtered SnapshotResources
ControlPlaneService-->>Browser: scoped DiscoveryResponse
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (11)
xds/src/main/java/com/linecorp/centraldogma/xds/internal/CentralDogmaXdsResources.java-206-213 (1)
206-213: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMalformed resource names are treated as valid group names.
groupOf()returns"foo"forgroups/foo, even though the Javadoc says unexpected names should map to"". This can incorrectly include malformed resources in scoped snapshots.Suggested fix
private static String groupOf(String resourceName) { if (!resourceName.startsWith(GROUPS_PREFIX)) { return ""; } final int start = GROUPS_PREFIX.length(); final int end = resourceName.indexOf('/', start); - return end < 0 ? resourceName.substring(start) : resourceName.substring(start, end); + if (end <= start) { + return ""; + } + return resourceName.substring(start, end); }🤖 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/CentralDogmaXdsResources.java` around lines 206 - 213, The groupOf() method in CentralDogmaXdsResources is not properly validating the resource name format according to its Javadoc contract. When the resourceName starts with GROUPS_PREFIX, the method should ensure the resource follows the expected format (groups/{group}/...). Currently, it treats incomplete names like "groups/foo" as valid group names by returning the extracted substring. Fix this by returning an empty string when no "/" is found after the group name (when end < 0), since this indicates a malformed resource name that doesn't match the expected format with additional path segments.xds/src/test/java/com/linecorp/centraldogma/xds/internal/XdsGroupDeletePermissionTest.java-35-38 (1)
35-38: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd the missing READ-role denial assertion.
Line 36-37 documents READ as insufficient, but Line 57-83 never verifies that path. This leaves a permission regression hole in the test contract.
Suggested patch
@@ - final WebClient writer = client(baseUri, "writer"); + final WebClient writer = client(baseUri, "writer"); + final WebClient reader = client(baseUri, "reader"); @@ // A WRITE role is insufficient; deletion requires ADMIN. grantRole(admin, "foo", "writer", "WRITE"); assertThat(deleteGroup(writer, "foo").headers().get("grpc-status")).isEqualTo("7"); + + // A READ role is also insufficient. + grantRole(admin, "foo", "reader", "READ"); + assertThat(deleteGroup(reader, "foo").headers().get("grpc-status")).isEqualTo("7");Also applies to: 57-83
🤖 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/XdsGroupDeletePermissionTest.java` around lines 35 - 38, The javadoc in XdsGroupDeletePermissionTest documents that a READ role is insufficient for deleting an xDS group, but the test implementation at lines 57-83 does not include an assertion that actually verifies this READ role denial scenario. Add a test case or assertion within the test method that explicitly verifies a user with READ repository role cannot successfully delete the xDS group, ensuring the test contract matches the documented behavior and provides complete permission regression coverage.xds/build.gradle-42-45 (1)
42-45: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winTrack
webapp/package-lock.jsonas an input for web build tasks.
npm ciis lockfile-driven, but neithernpmInstallnorbuildWebcurrently trackswebapp/package-lock.json. If only the lockfile changes, Gradle can incorrectly treat these tasks as up-to-date and package stale dependencies/artifacts.Suggested diff
tasks.named('npmInstall').configure { inputs.file('webapp/package.json') + inputs.file('webapp/package-lock.json') outputs.dir('webapp/node_modules') } @@ tasks.register('buildWeb', NpmTask) { dependsOn(tasks.named('npmInstall')) args = ['run', 'build'] inputs.dir('webapp/src') inputs.file('webapp/package.json') + inputs.file('webapp/package-lock.json') inputs.file('webapp/next.config.js') inputs.file('webapp/tsconfig.json') outputs.dir('webapp/build/web') }Also applies to: 50-55
🤖 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/build.gradle` around lines 42 - 45, The npmInstall and buildWeb task configurations do not track the webapp/package-lock.json file as an input, causing Gradle to potentially cache stale results when only the lockfile changes. Add inputs.file('webapp/package-lock.json') to both the npmInstall task configuration (in the configure block that currently has inputs.file('webapp/package.json')) and the buildWeb task configuration (referenced at lines 50-55) to ensure these tasks re-run whenever the lockfile is modified, since npm ci relies on the lockfile for dependency resolution.xds/webapp/src/dogma/features/auth/LoginForm.tsx-62-62 (1)
62-62: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix invalid width unit on the root container.
100whis invalid CSS unit syntax; use100vw(or100%) to get the intended full-width layout.Suggested patch
- width="100wh" + width="100vw"🤖 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/webapp/src/dogma/features/auth/LoginForm.tsx` at line 62, The root container in LoginForm.tsx has an invalid CSS unit "100wh" set on the width attribute. Replace "100wh" with "100vw" to properly set the full viewport width for the container layout.xds/webapp/src/dogma/features/appidentity/DisplaySecretModal.tsx-78-81 (1)
78-81: 🩺 Stability & Availability | 🟡 MinorAdd error handling for clipboard copy failures.
The
navigator.clipboard.writeText()call can fail due to security restrictions (HTTPS requirement, user gesture context, permissions), and currently any rejection will go unhandled with no user feedback.Suggested fix
onClick={async () => { - await navigator.clipboard.writeText(response.secret || ''); - dispatch(newNotification('', 'Copied to clipboard', 'success')); + try { + await navigator.clipboard.writeText(response.secret || ''); + dispatch(newNotification('', 'Copied to clipboard', 'success')); + } catch { + dispatch(newNotification('Copy failed', 'Could not copy secret', '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 `@xds/webapp/src/dogma/features/appidentity/DisplaySecretModal.tsx` around lines 78 - 81, The clipboard write operation in the onClick handler lacks error handling for potential failures due to security restrictions or permission issues. Wrap the navigator.clipboard.writeText() call in a try-catch block or add a .catch() handler to the promise. When the clipboard operation fails, dispatch an error notification to inform the user that the copy operation was unsuccessful, instead of silently failing without user feedback.webapp/src/dogma/common/components/Navbar.tsx-108-112 (1)
108-112: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd the xDS entry to the mobile nav as well.
This change adds xDS only to desktop navigation; the mobile menu still omits it, so small-screen users lose discoverable access.
Suggested fix
{isOpen ? ( <Box pb={4} display={{ md: 'none' }}> <Stack as="nav" spacing={4}> {topMenus.map(({ path, name }) => ( <NavLink link={path} key={name}> {name} </NavLink> ))} + {xdsWebEnabled && ( + <Link href="/xds" px={2} py={1} rounded="md" _hover={{ textDecoration: 'none', bg: navHoverBg }}> + xDS + </Link> + )} </Stack> </Box> ) : null}🤖 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/dogma/common/components/Navbar.tsx` around lines 108 - 112, The xDS link has been added to the desktop navigation but is missing from the mobile navigation menu in the Navbar component. Locate the mobile navigation section in Navbar.tsx and add the same conditional xDS Link block with href="/xds" that appears in the desktop navigation. Ensure the mobile version also checks the xdsWebEnabled condition and apply appropriate mobile-friendly styling similar to other mobile nav items to maintain visual consistency.xds/webapp/src/pages/resource.tsx-22-27 (1)
22-27: 🎯 Functional Correctness | 🟡 MinorAvoid unsafe casts from
router.queryfor resource routing params.
router.querycan return arrays when query parameters are duplicated (e.g.,?id=a&id=b). The current casts and comparisons don't handle this:groupandtypecould be arrays that pass the truthy checks on line 28, whileisNewandk8scomparisons would silently fail if arrays are passed. Additionally,idis cast but never validated before reachingResourceEditor.Suggested fix
const ResourcePage = () => { const router = useRouter(); - const group = router.query.group as string; - const type = router.query.type as XdsResourceType; - const id = router.query.id as string | undefined; - const isNew = router.query.action === 'new'; - const k8s = router.query.k8s === 'true'; + const toSingle = (value: string | string[] | undefined): string | undefined => + Array.isArray(value) ? value[0] : value; + const group = toSingle(router.query.group); + const typeParam = toSingle(router.query.type); + const id = toSingle(router.query.id); + const isNew = toSingle(router.query.action) === 'new'; + const k8s = toSingle(router.query.k8s) === 'true'; - if (!group || !type || !XDS_RESOURCE_META[type]) { + if (!group || !typeParam || !XDS_RESOURCE_META[typeParam as XdsResourceType]) { return null; } + const type = typeParam as XdsResourceType; return <ResourceEditor group={group} type={type} id={id} isNew={isNew} k8s={k8s} />; };🤖 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/webapp/src/pages/resource.tsx` around lines 22 - 27, The code casts `router.query` parameters directly to strings without handling the case where these parameters can be arrays when duplicated in the URL. For the variables `group`, `type`, `id`, `isNew`, and `k8s`, add validation logic that checks if the value is an array and handles it appropriately by either taking the first element or rejecting it. Ensure that `group` and `type` are confirmed to be strings before being used in conditional checks, and validate that `id` is a valid string before passing it to the `ResourceEditor` component.xds/webapp/src/pages/k8s-aggregator.tsx-21-23 (1)
21-23: 🎯 Functional Correctness | 🟡 MinorNormalize query params before treating them as strings.
Direct
as stringcasts onrouter.querycan passstring[]values intoK8sAggregatorEditor, producing invalid IDs and group paths. The API calls expect strings for URL construction (e.g.,/groups/${group}/k8s/endpointAggregators/${id}.json) and will fail or produce incorrect requests if arrays are passed.Suggested fix
const K8sAggregatorPage = () => { const router = useRouter(); - const group = router.query.group as string; - const id = router.query.id as string | undefined; - const isNew = router.query.action === 'new'; + const toSingle = (value: string | string[] | undefined): string | undefined => + Array.isArray(value) ? value[0] : value; + const group = toSingle(router.query.group); + const id = toSingle(router.query.id); + const isNew = toSingle(router.query.action) === 'new';🤖 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/webapp/src/pages/k8s-aggregator.tsx` around lines 21 - 23, The direct type assertion with `as string` on router.query values for group and id parameters does not account for Next.js router.query returning string[] for certain query parameters, which will cause incorrect API calls. Normalize the group and id query parameters (and the action parameter used in the isNew check) by checking if they are arrays and extracting the first element if needed, ensuring they are treated as single string values before being passed to K8sAggregatorEditor or used in API endpoint construction.xds/webapp/src/dogma/common/useXdsRoute.ts-32-40 (1)
32-40: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winNormalize query params before string use.
Line 32 and Line 33 cast
router.queryvalues tostring, but Next.js can providestring[]. Repeated query params can then be parsed incorrectly.Suggested fix
export function useXdsRoute(): XdsRoute { const router = useRouter(); - const group = (router.query.name as string) || (router.query.group as string) || undefined; - const type = router.query.type as string | undefined; + const first = (v: string | string[] | undefined): string | undefined => (Array.isArray(v) ? v[0] : v); + const group = first(router.query.name) || first(router.query.group) || undefined; + const type = first(router.query.type); const section: XdsSection =🤖 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/webapp/src/dogma/common/useXdsRoute.ts` around lines 32 - 40, The issue is that router.query values are being cast directly to string without handling the case where Next.js returns string arrays for repeated query parameters. In the useXdsRoute function, normalize the router.query values by creating a helper that checks if each query value is an array and extracts the appropriate string (typically the first element), then apply this normalization to group, type, and other query parameters before using them in string comparisons and assignments.xds/webapp/src/pages/app-identities.tsx-48-56 (1)
48-56: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake
runreport success so failed delete does not close the modal.
runswallows errors, so the delete handler always executesonClose()even when deletion fails.Suggested fix
- const run = useCallback( - async (fn: () => Promise<unknown>, successTitle: string, successMsg: string, errorTitle: string) => { + const run = useCallback( + async ( + fn: () => Promise<unknown>, + successTitle: string, + successMsg: string, + errorTitle: string, + ): Promise<boolean> => { try { await fn(); dispatch(newNotification(successTitle, successMsg, 'success')); + return true; } catch (e) { dispatch(newNotification(errorTitle, ErrorMessageParser.parse(e), 'error')); + return false; } }, [dispatch], ); @@ - handleDelete={async () => { - await run( + handleDelete={async () => { + const ok = await run( () => deleteAppIdentity({ appId: target }).unwrap(), 'App identity deleted', `'${target}' is deleted`, 'Failed to delete', ); - onClose(); + if (ok) { + onClose(); + } }}Also applies to: 204-212
🤖 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/webapp/src/pages/app-identities.tsx` around lines 48 - 56, The `run` callback function catches errors and dispatches error notifications but does not report the success or failure status to its caller, which causes the delete handler to always execute `onClose()` even when the operation fails. Modify the `run` function to return a boolean value: return true after successfully dispatching the success notification, and return false in the catch block after handling the error. Then update the delete handler (and any other callers that use `run` as mentioned in the comment) to check the boolean return value before calling `onClose()`, ensuring the modal only closes when the operation actually succeeds.xds/webapp/src/dogma/common/components/Deferred.tsx-34-37 (1)
34-37: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winNormalize non-numeric error statuses before rendering
Error.Blind-casting
error.statusto number can pass invalid values and produce inconsistent error UI. Use a numeric fallback (e.g. 500).Suggested fix
if (props.error) { const error = props.error; const message = ErrorMessageParser.parse(error); - return <Error statusCode={error.status as number} withDarkMode={colorMode === 'dark'} title={message} />; + const rawStatus = (error as { status?: number | string }).status; + const statusCode = typeof rawStatus === 'number' ? rawStatus : 500; + return <Error statusCode={statusCode} withDarkMode={colorMode === 'dark'} title={message} />; }🤖 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/webapp/src/dogma/common/components/Deferred.tsx` around lines 34 - 37, The current code blindly casts error.status to a number without validation, which can pass invalid values to the Error component's statusCode prop. In the error handling block where the Error component is rendered, replace the direct cast of error.status with a normalized numeric value that validates the status is a valid number and provides a fallback value (such as 500) if it is not. This ensures the statusCode prop receives a consistent, valid numeric value for proper error UI rendering.
🧹 Nitpick comments (2)
xds/webapp/.gitignore (1)
13-13: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winBroaden env-file ignore coverage to reduce secret commit risk.
Line 13 only ignores
.env.local; other common env files can still be committed from this subproject.Suggested patch
-.env.local +.env* +!.env.example🤖 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/webapp/.gitignore` at line 13, The `.env.local` entry in the `.gitignore` file is too specific and only ignores that single file variant, leaving other common environment files at risk of being accidentally committed with secrets. Broaden the env-file ignore coverage by adding additional patterns to match other common environment file naming conventions such as `.env`, `.env.*.local`, `.env.production.local`, and similar variations that might contain sensitive configuration or credentials for this subproject.webapp/src/dogma/features/api/apiSlice.ts (1)
635-641: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the backend payload for
isXdsWebEnabledinstead of a constant.
transformResponse: () => trueignores the response body and can drift from server semantics ifenabledever becomes dynamic.Suggested refactor
isXdsWebEnabled: builder.query<boolean, void>({ query: () => ({ url: `/api/v1/xds/web`, method: 'GET', }), - transformResponse: () => true, + transformResponse: (response: { enabled?: boolean }) => Boolean(response?.enabled), }),🤖 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/dogma/features/api/apiSlice.ts` around lines 635 - 641, The isXdsWebEnabled query in the apiSlice ignores the backend response payload and always returns a hardcoded true value through the transformResponse callback. Update the transformResponse function in the isXdsWebEnabled builder.query to extract and return the actual enabled status from the backend response payload instead of returning a constant value. This will ensure the frontend reflects the server's actual XDS web enabled state, preventing drift if the backend value changes dynamically.
🤖 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/project/ProjectApiManager.java`:
- Around line 180-187: The exists() method is currently rejecting all internal
projects for non-system-admin users, but the isAccessibleInternalProject method
now allows authenticated users to access the xDS project. Update the exists()
method to check if the requested internal project is accessible using
isAccessibleInternalProject before rejecting access for non-admin users, so that
the xDS project is consistently accessible across all API operations for
authenticated users.
In
`@xds/src/main/java/com/linecorp/centraldogma/xds/group/v1/XdsGroupService.java`:
- Around line 105-119: The currentAuthor() method is being invoked inside the
handle() callback of the async findRepositoryRole() operation, which may execute
without the request context and cause the authorized delete to fail. Capture the
result of currentAuthor() at the beginning of the method alongside the existing
currentUser() call, and then pass this captured author value to the
removeRepository() method instead of calling currentAuthor() within the
callback.
In
`@xds/src/main/java/com/linecorp/centraldogma/xds/internal/ControlPlaneService.java`:
- Around line 345-351: The authorization handler in the ControlPlaneService
method is ignoring authorization failures. In the handle callback with
parameters (authorized, cause), the code unconditionally calls
unwrap().serve(ctx, req) regardless of whether cause indicates an authorization
failure. Fix this by checking if cause is not null before serving the request.
If authorization fails (cause is not null), return an appropriate error response
instead of proceeding to serve the request. Only call unwrap().serve(ctx, req)
when the authorization succeeds and cause is null.
In
`@xds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsResourceManager.java`:
- Around line 157-173: The createGroup() method in XdsGroupService currently
lacks authorization checks, allowing any authenticated user to create groups
regardless of their assigned roles. Add a call to checkWritePermission() at the
beginning of createGroup() before the createRepository() call, similar to how
createCluster(), createRoute(), createListener(), and createEndpoint() enforce
permissions. Reference how deleteGroup() properly enforces the ADMIN role
requirement to ensure consistency in authorization across group management
operations.
In
`@xds/src/test/java/com/linecorp/centraldogma/xds/internal/MtlsDiscoveryAuthorizationTest.java`:
- Around line 205-217: The onError method in the StreamObserver implementation
within the responseRecorder method is currently empty, which causes mTLS
discovery stream errors to be silently dropped instead of being propagated,
leading to timeout failures instead of immediate error detection. Implement the
onError method to properly handle and capture the Throwable parameter so that
stream errors are detected and made available for assertions rather than being
swallowed.
In
`@xds/src/test/java/com/linecorp/centraldogma/xds/internal/TokenDiscoveryAuthorizationTest.java`:
- Around line 202-214: The onError method in the StreamObserver returned by the
responseRecorder method is empty and swallows stream errors, causing failures to
only surface as delayed polling timeouts instead of failing fast. Modify the
onError method to properly handle the Throwable parameter by either adding error
context to the queue, throwing an exception, or failing the test to ensure
stream errors are detected immediately rather than masked by timeout delays.
In `@xds/webapp/src/dogma/common/components/DataTable.tsx`:
- Around line 15-30: The sort functionality is currently attached to the
non-focusable Th element, which prevents keyboard users from accessing the sort
feature. Replace the Th element's onClick handler with a keyboard-accessible
interactive element such as a button, and move the getToggleSortingHandler()
logic to that button. Ensure the button is properly labeled and the sort icons
(TriangleDownIcon and TriangleUpIcon) are contained within or adjacent to the
button so keyboard focus management and screen reader announcements work
correctly.
In `@xds/webapp/src/dogma/common/components/DateWithTooltip.tsx`:
- Around line 7-10: The DateWithTooltip component crashes when the date
parameter is malformed because toISOString() throws an error on invalid Date
objects. After creating the parsed Date object, add validation to check if the
date is valid by testing if parsed.getTime() returns NaN. If the date is
invalid, return a fallback UI (such as displaying an error message or
placeholder text) instead of attempting to call toISOString() or
toLocaleString() on the invalid date object. This ensures the component
gracefully handles invalid input without crashing.
In `@xds/webapp/src/dogma/common/components/JsonEditor.tsx`:
- Around line 34-51: The useEffect hook in JsonEditor.tsx lacks error handling
for the async operations (dynamic import and loader.init()). If either the
monaco-editor import or loader.init() call fails, setReady(true) is never
executed, leaving the component stuck in a loading state indefinitely. Wrap the
async code block in a try-catch statement to handle errors from the import and
initialization. When an error is caught, either set ready to true to exit the
loading state or add an error state variable and update the component to display
an error message instead of the Loading component while still respecting the
active flag.
In `@xds/webapp/src/dogma/common/useGroupExists.ts`:
- Around line 32-40: The useGroupExists hook needs to distinguish between fetch
failures and actual missing groups. Modify the useGetGroupsQuery call to
conditionally skip execution when no group is provided (by adding a skip
parameter that checks if group is falsy), capture any error state from the query
response, and update the return logic to handle three distinct cases: when the
query is loading, when the query has errored (return a state indicating the
fetch failed rather than group missing), and when data successfully loaded
(check if group exists in the data array). This prevents valid groups from being
incorrectly marked as non-existent when the fetch fails and avoids unnecessary
query execution when group is not selected.
In `@xds/webapp/src/dogma/features/auth/authSlice.ts`:
- Around line 109-115: The error handling in the checkSecurityEnabled thunk is
incorrectly treating all failures as 404 errors and switching to anonymous mode
for every failure. Instead of only showing the anonymous mode notification for
404 status codes, the current logic displays it regardless of the actual error
type, which causes the UI to proceed under a false security context for network
or 5xx errors. Modify the catch block to only dispatch the anonymous mode
notification when err.response.status is specifically 404, and for all other
errors, rejectWithValue should include a structured payload containing both the
error message and the status code so the error handler can make more
deterministic decisions about auth state. Apply this same fix to the other
location in the file that has this identical pattern.
In `@xds/webapp/src/dogma/features/notification/notificationSlice.ts`:
- Line 12: The timestamp property in the Notification interface is declared as
type number but the initialState object initializes it to null, creating a type
contract mismatch. Update the timestamp property declaration in the Notification
interface to use the union type number | null to accurately reflect that it can
be either a number or null, ensuring the type definition matches the actual
initial state value.
In `@xds/webapp/src/dogma/features/services/ErrorMessageParser.ts`:
- Around line 15-17: The parse() method promises to return a string but the
branch that checks if (object.error) exists returns object.error directly
without ensuring it's a string type. This branch can return non-string values
like objects or arrays. Convert object.error to a string in the return statement
of this branch, such as by using JSON.stringify() if it's an object/array, or
toString() to ensure the return value is always a string as promised by the
method signature.
In `@xds/webapp/src/dogma/features/xds/AddRepositoryRole.tsx`:
- Around line 100-143: The Select component's onChange handler guards with
option && which prevents clearing the id field when the select is cleared,
leaving stale values in form state. Additionally, when options is empty and the
Controller is not rendered, any previously selected id value remains in the form
state, allowing submission with an invalid identifier. Fix this by modifying the
onChange handler in the Select component to explicitly clear the form field
(call onChange with an appropriate empty value like undefined or empty string)
when option is null or falsy, ensuring the form state is properly reset when the
select is cleared or becomes unavailable.
In `@xds/webapp/src/dogma/features/xds/ResourceGraph.tsx`:
- Around line 119-124: The catch block is treating all fetch errors identically
by marking nodes as 'missing', but only HTTP 404 errors should result in a
'missing' status. Other errors like authentication failures, server errors, and
network issues need different handling. Modify the catch block to check if the
error is a 404 status code before setting node.status to 'missing', and either
re-throw or handle non-404 errors appropriately so they don't get silently
misrepresented as absent resources.
---
Minor comments:
In `@webapp/src/dogma/common/components/Navbar.tsx`:
- Around line 108-112: The xDS link has been added to the desktop navigation but
is missing from the mobile navigation menu in the Navbar component. Locate the
mobile navigation section in Navbar.tsx and add the same conditional xDS Link
block with href="/xds" that appears in the desktop navigation. Ensure the mobile
version also checks the xdsWebEnabled condition and apply appropriate
mobile-friendly styling similar to other mobile nav items to maintain visual
consistency.
In `@xds/build.gradle`:
- Around line 42-45: The npmInstall and buildWeb task configurations do not
track the webapp/package-lock.json file as an input, causing Gradle to
potentially cache stale results when only the lockfile changes. Add
inputs.file('webapp/package-lock.json') to both the npmInstall task
configuration (in the configure block that currently has
inputs.file('webapp/package.json')) and the buildWeb task configuration
(referenced at lines 50-55) to ensure these tasks re-run whenever the lockfile
is modified, since npm ci relies on the lockfile for dependency resolution.
In
`@xds/src/main/java/com/linecorp/centraldogma/xds/internal/CentralDogmaXdsResources.java`:
- Around line 206-213: The groupOf() method in CentralDogmaXdsResources is not
properly validating the resource name format according to its Javadoc contract.
When the resourceName starts with GROUPS_PREFIX, the method should ensure the
resource follows the expected format (groups/{group}/...). Currently, it treats
incomplete names like "groups/foo" as valid group names by returning the
extracted substring. Fix this by returning an empty string when no "/" is found
after the group name (when end < 0), since this indicates a malformed resource
name that doesn't match the expected format with additional path segments.
In
`@xds/src/test/java/com/linecorp/centraldogma/xds/internal/XdsGroupDeletePermissionTest.java`:
- Around line 35-38: The javadoc in XdsGroupDeletePermissionTest documents that
a READ role is insufficient for deleting an xDS group, but the test
implementation at lines 57-83 does not include an assertion that actually
verifies this READ role denial scenario. Add a test case or assertion within the
test method that explicitly verifies a user with READ repository role cannot
successfully delete the xDS group, ensuring the test contract matches the
documented behavior and provides complete permission regression coverage.
In `@xds/webapp/src/dogma/common/components/Deferred.tsx`:
- Around line 34-37: The current code blindly casts error.status to a number
without validation, which can pass invalid values to the Error component's
statusCode prop. In the error handling block where the Error component is
rendered, replace the direct cast of error.status with a normalized numeric
value that validates the status is a valid number and provides a fallback value
(such as 500) if it is not. This ensures the statusCode prop receives a
consistent, valid numeric value for proper error UI rendering.
In `@xds/webapp/src/dogma/common/useXdsRoute.ts`:
- Around line 32-40: The issue is that router.query values are being cast
directly to string without handling the case where Next.js returns string arrays
for repeated query parameters. In the useXdsRoute function, normalize the
router.query values by creating a helper that checks if each query value is an
array and extracts the appropriate string (typically the first element), then
apply this normalization to group, type, and other query parameters before using
them in string comparisons and assignments.
In `@xds/webapp/src/dogma/features/appidentity/DisplaySecretModal.tsx`:
- Around line 78-81: The clipboard write operation in the onClick handler lacks
error handling for potential failures due to security restrictions or permission
issues. Wrap the navigator.clipboard.writeText() call in a try-catch block or
add a .catch() handler to the promise. When the clipboard operation fails,
dispatch an error notification to inform the user that the copy operation was
unsuccessful, instead of silently failing without user feedback.
In `@xds/webapp/src/dogma/features/auth/LoginForm.tsx`:
- Line 62: The root container in LoginForm.tsx has an invalid CSS unit "100wh"
set on the width attribute. Replace "100wh" with "100vw" to properly set the
full viewport width for the container layout.
In `@xds/webapp/src/pages/app-identities.tsx`:
- Around line 48-56: The `run` callback function catches errors and dispatches
error notifications but does not report the success or failure status to its
caller, which causes the delete handler to always execute `onClose()` even when
the operation fails. Modify the `run` function to return a boolean value: return
true after successfully dispatching the success notification, and return false
in the catch block after handling the error. Then update the delete handler (and
any other callers that use `run` as mentioned in the comment) to check the
boolean return value before calling `onClose()`, ensuring the modal only closes
when the operation actually succeeds.
In `@xds/webapp/src/pages/k8s-aggregator.tsx`:
- Around line 21-23: The direct type assertion with `as string` on router.query
values for group and id parameters does not account for Next.js router.query
returning string[] for certain query parameters, which will cause incorrect API
calls. Normalize the group and id query parameters (and the action parameter
used in the isNew check) by checking if they are arrays and extracting the first
element if needed, ensuring they are treated as single string values before
being passed to K8sAggregatorEditor or used in API endpoint construction.
In `@xds/webapp/src/pages/resource.tsx`:
- Around line 22-27: The code casts `router.query` parameters directly to
strings without handling the case where these parameters can be arrays when
duplicated in the URL. For the variables `group`, `type`, `id`, `isNew`, and
`k8s`, add validation logic that checks if the value is an array and handles it
appropriately by either taking the first element or rejecting it. Ensure that
`group` and `type` are confirmed to be strings before being used in conditional
checks, and validate that `id` is a valid string before passing it to the
`ResourceEditor` component.
---
Nitpick comments:
In `@webapp/src/dogma/features/api/apiSlice.ts`:
- Around line 635-641: The isXdsWebEnabled query in the apiSlice ignores the
backend response payload and always returns a hardcoded true value through the
transformResponse callback. Update the transformResponse function in the
isXdsWebEnabled builder.query to extract and return the actual enabled status
from the backend response payload instead of returning a constant value. This
will ensure the frontend reflects the server's actual XDS web enabled state,
preventing drift if the backend value changes dynamically.
In `@xds/webapp/.gitignore`:
- Line 13: The `.env.local` entry in the `.gitignore` file is too specific and
only ignores that single file variant, leaving other common environment files at
risk of being accidentally committed with secrets. Broaden the env-file ignore
coverage by adding additional patterns to match other common environment file
naming conventions such as `.env`, `.env.*.local`, `.env.production.local`, and
similar variations that might contain sensitive configuration or credentials for
this subproject.
🪄 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: f6e4fd57-e9b7-4fb0-a8cf-adced0cc9f1d
⛔ Files ignored due to path filters (1)
xds/webapp/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (88)
it/xds-member-permission/src/test/java/com/linecorp/centraldogma/server/test/XdsMemberPermissionTest.javaserver/src/main/java/com/linecorp/centraldogma/server/CentralDogma.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/auth/RequiresRepositoryRoleDecorator.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/storage/project/ProjectApiManager.javaserver/src/main/java/com/linecorp/centraldogma/server/metadata/MetadataService.javawebapp/build.gradlewebapp/src/dogma/common/components/Navbar.tsxwebapp/src/dogma/features/api/apiSlice.tswebapp/tsconfig.tsbuildinfoxds/build.gradlexds/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/group/v1/XdsGroupService.javaxds/src/main/java/com/linecorp/centraldogma/xds/internal/CentralDogmaSnapshotResources.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/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/XdsTestServer.javaxds/src/test/java/com/linecorp/centraldogma/xds/internal/CreatingInternalGroupPlugin.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/XdsWebApplicationTest.javaxds/src/test/java/com/linecorp/centraldogma/xds/internal/XdsWritePermissionTest.javaxds/webapp/.eslintrc.jsonxds/webapp/.gitignorexds/webapp/.prettierrc.jsonxds/webapp/README.mdxds/webapp/next.config.jsxds/webapp/package.jsonxds/webapp/src/dogma/StoreProvider.tsxxds/webapp/src/dogma/common/components/DataTable.tsxxds/webapp/src/dogma/common/components/DateWithTooltip.tsxxds/webapp/src/dogma/common/components/Deferred.tsxxds/webapp/src/dogma/common/components/DeleteConfirmationModal.tsxxds/webapp/src/dogma/common/components/GroupSelector.tsxxds/webapp/src/dogma/common/components/JsonEditor.tsxxds/webapp/src/dogma/common/components/Layout.tsxxds/webapp/src/dogma/common/components/Loading.tsxxds/webapp/src/dogma/common/components/NotificationWrapper.tsxxds/webapp/src/dogma/common/components/Sidebar.tsxxds/webapp/src/dogma/common/components/TopBar.tsxxds/webapp/src/dogma/common/useGroupAdminAccess.tsxds/webapp/src/dogma/common/useGroupExists.tsxds/webapp/src/dogma/common/useGroupReadAccess.tsxds/webapp/src/dogma/common/useXdsRoute.tsxds/webapp/src/dogma/features/api/apiSlice.tsxds/webapp/src/dogma/features/appidentity/AppIdentityDto.tsxds/webapp/src/dogma/features/appidentity/DisplaySecretModal.tsxxds/webapp/src/dogma/features/appidentity/NewAppIdentity.tsxxds/webapp/src/dogma/features/auth/Authorized.tsxxds/webapp/src/dogma/features/auth/LoginForm.tsxxds/webapp/src/dogma/features/auth/UserDto.tsxds/webapp/src/dogma/features/auth/authSlice.tsxds/webapp/src/dogma/features/notification/notificationSlice.tsxds/webapp/src/dogma/features/services/ErrorMessageParser.tsxds/webapp/src/dogma/features/xds/AddRepositoryRole.tsxxds/webapp/src/dogma/features/xds/CredentialDto.tsxds/webapp/src/dogma/features/xds/CredentialsTab.tsxxds/webapp/src/dogma/features/xds/DangerZone.tsxxds/webapp/src/dogma/features/xds/GroupList.tsxxds/webapp/src/dogma/features/xds/K8sAggregatorEditor.tsxxds/webapp/src/dogma/features/xds/K8sAggregatorList.tsxxds/webapp/src/dogma/features/xds/MetadataDto.tsxds/webapp/src/dogma/features/xds/NewGroup.tsxxds/webapp/src/dogma/features/xds/PermissionsTab.tsxxds/webapp/src/dogma/features/xds/ResourceEditor.tsxxds/webapp/src/dogma/features/xds/ResourceGraph.tsxxds/webapp/src/dogma/features/xds/ResourceList.tsxxds/webapp/src/dogma/features/xds/RoleSection.tsxxds/webapp/src/dogma/features/xds/XdsTypes.tsxds/webapp/src/dogma/features/xds/xdsReferences.tsxds/webapp/src/dogma/hooks.tsxds/webapp/src/dogma/store.tsxds/webapp/src/dogma/util/auth.tsxds/webapp/src/pages/_app.tsxxds/webapp/src/pages/app-identities.tsxxds/webapp/src/pages/group.tsxxds/webapp/src/pages/index.tsxxds/webapp/src/pages/k8s-aggregator.tsxxds/webapp/src/pages/resource.tsxxds/webapp/tsconfig.json
There was a problem hiding this comment.
♻️ Duplicate comments (1)
xds/webapp/src/dogma/features/xds/AddRepositoryRole.tsx (1)
78-85: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTrim and validate
idbefore submission.At Line 81, whitespace-only IDs can still pass and be submitted in text-input mode. Normalize first, then guard and pass the trimmed value.
Suggested patch
const onSubmit = async (data: FormData) => { + const id = data.id?.trim() ?? ''; // Guard against a stale/empty id (e.g. a cleared select, or empty options) that form validation alone // would not catch. - if (!data.id || noSelectableOptions) { + if (!id || noSelectableOptions) { return; } - const ok = await onAdd(data.id, role); + const ok = await onAdd(id, role); if (ok) { reset(); setRole('READ'); onClose(); } };🤖 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/webapp/src/dogma/features/xds/AddRepositoryRole.tsx` around lines 78 - 85, In the onSubmit function, trim whitespace from the data.id value before checking if it is empty or falsy. Update the guard condition that checks "!data.id || noSelectableOptions" to first trim the ID, validate the trimmed value, and then ensure the trimmed ID is passed to the onAdd function instead of the original untrimmed value to prevent whitespace-only IDs from being submitted.
🤖 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.
Duplicate comments:
In `@xds/webapp/src/dogma/features/xds/AddRepositoryRole.tsx`:
- Around line 78-85: In the onSubmit function, trim whitespace from the data.id
value before checking if it is empty or falsy. Update the guard condition that
checks "!data.id || noSelectableOptions" to first trim the ID, validate the
trimmed value, and then ensure the trimmed ID is passed to the onAdd function
instead of the original untrimmed value to prevent whitespace-only IDs from
being submitted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b4d9636d-9fa8-422f-9c79-e6870206f321
📒 Files selected for processing (13)
webapp/tsconfig.tsbuildinfoxds/src/main/java/com/linecorp/centraldogma/xds/group/v1/XdsGroupService.javaxds/webapp/src/dogma/common/components/DataTable.tsxxds/webapp/src/dogma/common/components/DateWithTooltip.tsxxds/webapp/src/dogma/common/useGroupExists.tsxds/webapp/src/dogma/common/useGroupWriteAccess.tsxds/webapp/src/dogma/features/auth/authSlice.tsxds/webapp/src/dogma/features/notification/notificationSlice.tsxds/webapp/src/dogma/features/services/ErrorMessageParser.tsxds/webapp/src/dogma/features/xds/AddRepositoryRole.tsxxds/webapp/src/dogma/features/xds/MetadataDto.tsxds/webapp/src/dogma/features/xds/ResourceEditor.tsxxds/webapp/src/dogma/features/xds/ResourceGraph.tsx
🚧 Files skipped from review as they are similar to previous changes (9)
- xds/webapp/src/dogma/common/useGroupExists.ts
- xds/webapp/src/dogma/common/components/DateWithTooltip.tsx
- xds/webapp/src/dogma/features/notification/notificationSlice.ts
- xds/webapp/src/dogma/features/services/ErrorMessageParser.ts
- xds/webapp/src/dogma/features/xds/ResourceGraph.tsx
- xds/webapp/src/dogma/common/components/DataTable.tsx
- xds/src/main/java/com/linecorp/centraldogma/xds/group/v1/XdsGroupService.java
- xds/webapp/src/dogma/features/xds/ResourceEditor.tsx
- xds/webapp/src/dogma/features/auth/authSlice.ts
jrhee17
approved these changes
Jun 24, 2026
jrhee17
left a comment
jrhee17
left a comment
Contributor
There was a problem hiding this comment.
Understood the java-side main changes as:
- Metadata/User level permission is also managed for the
@xdsrepo - AppIdentity requests are scoped in terms of what resources are in it's group
|
|
||
| private void checkWritePermission0(String group) { | ||
| final User user = AuthUtil.currentUserOrNull(); | ||
| if (user == null || user.isSystemAdmin()) { |
Contributor
There was a problem hiding this comment.
Note) Understood that this is for no-auth mode. It might be clearer if XdsResourceManager were instantiated as a field.
Contributor
Author
There was a problem hiding this comment.
Understood that this is for no-auth mode.
That's correct. After the migration is done, it will require the auth.
It might be clearer if XdsResourceManager were instantiated as a field.
Could you elaborate more on this?
Contributor
There was a problem hiding this comment.
I assumed that no-auth mode is a server state, and hence it doesn't need to be calculated per auth validation.
Hence, I was imagining there was a field such as XdsResourceManager#authEnabled.
This is purely from a readability perspective, so feel free to ignore if you feel differently.
Contributor
Author
There was a problem hiding this comment.
Now I get it.
Let me just throw an exception if the user is null. We provide the example servers with authentication enabled, so it should be no problem.
|
|
||
| @Override | ||
| protected void onDiffHandled() { | ||
| updateAllSnapshots(); |
Contributor
There was a problem hiding this comment.
Note) Understood that all caches will be updated even if a single group within the project is updated
Contributor
Author
There was a problem hiding this comment.
That is true and it's on my todo list:
// TODO(minwoox): Implement better cache implementation that updates only changed resources
// instead of this snapshot based implementation.
| }); | ||
| } | ||
|
|
||
| private String cacheKey() { |
Contributor
There was a problem hiding this comment.
Note) Understood that cacheKey is used as the group name for each discovery req.
Hence, each group is scoped in terms of what groups it can subscribe to.
Contributor
Author
There was a problem hiding this comment.
That's correct. cacheKey is used for NodeGroup and it can subscribe to the Central Dogma groups.
ikhoon
approved these changes
Jun 24, 2026
| public HttpResponse serve(ServiceRequestContext ctx, HttpRequest req) { | ||
| // The authorizer sets the authenticated user on the context when the certificate or token is | ||
| // valid. The boolean result is ignored on purpose. | ||
| return HttpResponse.of(authorizer.authorize(ctx, req).handle((authorized, cause) -> { |
Contributor
There was a problem hiding this comment.
It was noted that authorizer does not complete exceptionally.
Contributor
Author
There was a problem hiding this comment.
It could be, I intentionally didn't check it because the authorization will be enabled anyway after the migration is done.
| } | ||
|
|
||
| /** | ||
| * Creates {@link SnapshotResources} from an already flattened (resource name to {@link VersionedResource}) |
Contributor
There was a problem hiding this comment.
For me, it took some time to understand what flattened means in this context. From my understanding, the snapshot are created from the filtered resources.
Suggested change
| * Creates {@link SnapshotResources} from an already flattened (resource name to {@link VersionedResource}) | |
| * Creates {@link SnapshotResources} from an already filtered (resource name to {@link VersionedResource}) |
Contributor
Author
There was a problem hiding this comment.
create method uses Map<String, Map<String, VersionedResource<T>>> resources but this method uses
Map<String, VersionedResource<T>> versionedResources which is flattened from Map<String, Map<String, VersionedResource<T>>> resources. However, it's filtered as well so let me update it as filtered and flattened
| import com.linecorp.centraldogma.server.storage.repository.Repository; | ||
|
|
||
| /** | ||
| * Serves read access to EDS (endpoint) resources of the xDS project without requiring a repository role. |
Contributor
There was a problem hiding this comment.
I’m a bit concerned that access control for CD may become fragmented, but I think this is the most appropriate implementation for now.
I’ll also think more about a more general implementation for file-level and directory-level access control.
Motivation: - The xds module exposes Envoy control plane features (LDS/RDS/CDS/EDS plus group, credential and Kubernetes-aggregator management) over gRPC and HTTP/JSON, but had no UI. Operators had to call the HTTP API by hand to create a group and manage its resources, which is error-prone and hard to inspect. Modifications: - Add a standalone Next.js web app under xds/webapp (Chakra UI, Redux Toolkit / RTK Query, Monaco editor), served under '/xds' with its own layout: a group selector, a per-group resource sidebar, and a JSON editor. Wire it into the Gradle build and serve it from ControlPlaneService when bundled. - Group management (a group is a repository under the internal '@xds' project): list with paging and search, create, and ADMIN-only deletion in a dedicated "Danger Zone" section. Per-group Permissions, Credentials and Kubernetes endpoint aggregator management. - Resource management for LDS/RDS/CDS/EDS: list, create from templates, read-only view with an explicit Edit toggle, update and delete. A "References" panel links a resource to the children it references (LDS -> RDS/CDS, RDS -> CDS, CDS -> EDS), and missing resources/groups are reported with explicit messages instead of raw exceptions. - Surface Kubernetes-aggregator-generated endpoints as read-only. - Authentication/authorization: reuse the main Central Dogma session auth, redirect unauthenticated users to the main login, and add an "xDS" navbar link in the main web app when the xDS web is enabled. Scope the discovery API per app identity (mTLS certificate or access token) and expose an ungated EDS read API so endpoints are readable without the READ role. - Add a runnable XdsTestServer (embedded fabric8 Kubernetes mock and sample data) for manual testing, plus integration tests for mTLS/token discovery scoping, EDS read permission, write permission, and web app serving. You can run the server via `./gradlew :xds:runXdsTestServer` Result: - Operators can manage xDS groups and their LDS/RDS/CDS/EDS resources, credentials, Kubernetes endpoint aggregators and permissions through a dedicated web UI served under '/xds', instead of hand-crafting HTTP calls.
There was a problem hiding this comment.
Actionable comments posted: 13
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/CentralDogmaXdsResources.java (1)
149-203: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPublish the snapshot and group indexes atomically.
Line 153, Line 170, and Line 179 publish per-group indexes before Line 185 publishes
currentSnapshot, whilesnapshot(Set)reads those volatile fields separately. A concurrent gRPC caller can observe new CDS/LDS/RDS indexes with an oldcurrentSnapshot, producing a mixed-revision scoped xDS snapshot. Consider replacing these independent volatile fields with a single immutable state holder, or synchronizesnapshot()andsnapshot(Set)so each scoped snapshot is assembled from one consistent state.🤖 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/CentralDogmaXdsResources.java` around lines 149 - 203, The snapshot publication in CentralDogmaXdsResources.snapshot() is split across multiple volatile fields, so snapshot(Set<String>) can observe updated group indexes with an older currentSnapshot and return a mixed-revision xDS view. Fix this by making snapshot() and snapshot(Set<String>) read from one consistent immutable state (for example, a single state holder containing currentSnapshot plus clustersByGroup/listenersByGroup/routesByGroup) or by synchronizing both methods so all resources are assembled from the same revision.
🧹 Nitpick comments (1)
xds/src/test/java/com/linecorp/centraldogma/xds/internal/XdsResourceWatchingServiceTest.java (1)
105-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the forwarded group name in this test.
The new callback contract is group-scoped, but this override drops
groupName, so the test would not catch a regression wherehandleDiff()passes the wrong group toonDiffHandled.Test coverage adjustment
- protected void onDiffHandled(String groupName) {} + protected void onDiffHandled(String groupName) { + queue.add("diff handled: " + groupName); + }Then consume the extra queue entry after each commit, e.g. assert
"diff handled: bar"/"diff handled: baz"alongside the existing resource events.🤖 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 105 - 106, The test override of onDiffHandled currently ignores the forwarded groupName, so it cannot verify that handleDiff passes the correct group through. Update XdsResourceWatchingServiceTest to assert the group-scoped callback argument from onDiffHandled and consume the corresponding queue entry after each commit, matching the existing resource event assertions so regressions in group forwarding are caught.
🤖 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 `@webapp/src/dogma/common/components/GroupSelector.tsx`:
- Around line 33-68: The GroupSelector menu currently treats any undefined
groups result as “No groups yet,” which mixes loading and error states into the
empty state. Update useGetGroupsQuery() handling in GroupSelector so the render
branches on isLoading and isError separately, and only show the no-groups
message when the query has completed successfully with an empty list; keep the
existing Menu/MenuItem structure and currentGroup behavior intact.
In `@webapp/src/dogma/common/useXdsRoute.ts`:
- Around line 34-40: The `useXdsRoute` type validation currently uses `type in
XDS_RESOURCE_META`, which can accept inherited keys like `toString` and produce
an invalid `section`. Update the `section` selection logic in `useXdsRoute` to
use an own-property check via
`Object.prototype.hasOwnProperty.call(XDS_RESOURCE_META, type)` while keeping
the existing explicit allowed values (`permissions`, `k8sAggregators`,
`credentials`, `dangerZone`) intact.
In `@webapp/src/dogma/features/services/ErrorMessageParser.ts`:
- Around line 12-18: `ErrorMessageParser.parse` still returns
`response.data.message` directly, so non-string payloads can escape the new
string-only contract. Update the `message` handling branches in
`ErrorMessageParser.parse` to normalize any object or array by routing them back
through `ErrorMessageParser.parse(...)`, matching the existing `error` fallback
behavior so `Deferred` and auth rendering always receive a string.
In `@webapp/src/dogma/features/xds/AddRepositoryRole.tsx`:
- Around line 78-84: The free-text ID path in AddRepositoryRole.onSubmit still
forwards untrimmed input, so whitespace-only or padded IDs can slip through
despite validation. Update the submit handling to normalize data.id by trimming
it before the empty-check and before calling onAdd, and make sure the same
behavior applies wherever the form submits manual IDs in this flow.
In `@webapp/src/dogma/features/xds/K8sAggregatorEditor.tsx`:
- Around line 142-147: The watcher numeric fields in K8sAggregatorEditor.tsx are
being coerced with Number(w.priority) and Number(w.loadBalancingWeight), which
can turn invalid input into NaN and get serialized incorrectly. Update the
watcher serialization logic in the code paths that build entry objects to
validate these fields before assignment, and reject or surface invalid values
instead of silently converting them; apply the same fix in both the current
watcher mapping and the duplicated block referenced in the comment.
- Around line 471-511: The NewK8sAggregatorEditor create flow currently skips
authorization checks, so users can open it directly and only fail on submit. Add
a write-access gate using useGroupWriteAccess(group) in NewK8sAggregatorEditor,
consistent with the resource editor, and prevent rendering or submitting the
form when WRITE is not available. Make sure the Create button and onSubmit path
are only reachable for authorized users.
In `@webapp/src/dogma/features/xds/PermissionsTab.tsx`:
- Around line 47-57: The app-identity loading/error state is being collapsed
into an empty list in PermissionsTab, which makes AddRepositoryRole treat
in-flight or failed fetches as “no app IDs” and disable submission. Update
PermissionsTab (and the related AddRepositoryRole usage around the app ID
options) to distinguish loading/error from a true empty result, and only render
the empty-state helper when the query has actually resolved with zero
identities. Keep the appIdOptions derivation tied to useGetAppIdentitiesQuery
data without defaulting to an empty array for non-success states.
In `@webapp/src/dogma/features/xds/ResourceEditor.tsx`:
- Around line 69-107: The NewResourceEditor flow currently allows users to open
and submit the create form without checking write permissions, so gate this
component on useGroupWriteAccess(group) before showing or enabling the create
UI. Add the access check alongside the existing hooks in NewResourceEditor, and
either return the existing no-access state or disable handleCreate/Create button
when write access is missing so read-only users are blocked before calling
useCreateResourceMutation.
In `@webapp/src/dogma/features/xds/xdsApiSlice.ts`:
- Around line 167-195: Encode the resource ID before using it as a path segment
in the xDS API methods, since `idFromPath()` can return nested values like
`foo/bar` and `updateResource`/`deleteResource` currently interpolate `id` raw
into the URL. Update the `query` builders in `xdsApiSlice` for the
`updateResource` and `deleteResource` mutations (and any similar path-based xDS
requests) to use an encoded `id`, matching the safer approach already used by
`createResource()`.
In `@webapp/src/pages/app/xds/group.tsx`:
- Around line 52-65: The access gating in group.tsx still renders protected
section content before the read-access check completes, even though
redirectToEndpoints waits on accessLoading. Update the section rendering logic
around redirectToEndpoints/useEffect so non-endpoints tabs do not mount or fetch
until hasAccess is known, and only render the gated tab content when
accessLoading is false and the user is allowed; keep the existing redirect
behavior for group, redirectToEndpoints, redirectFromAdminOnly, and the
section-specific tab components.
In
`@xds/src/main/java/com/linecorp/centraldogma/xds/group/v1/XdsGroupService.java`:
- Around line 19-20: The delete flow in XdsGroupService should check for a null
currentUser() before calling MetadataService.findRepositoryRole(...), since
unauthenticated requests can otherwise fail with an internal error. Keep the
system-admin fast path in place first, then guard the role lookup so only a
non-null User is passed into repository role resolution, using the existing
currentUser() and getAuthor() helpers in XdsGroupService.
In
`@xds/src/main/java/com/linecorp/centraldogma/xds/internal/ControlPlaneService.java`:
- Around line 282-286: Store the latest fetched xDS metadata even when
scopedClients is empty, instead of returning early before update. In
ControlPlaneService, make the listener path around fetchXdsMetadata() cache the
fresh metadata regardless of whether any ScopedClient exists, and then have the
first ScopedClient initialization use that cached xDS metadata instead of
Project.metadata() so stale readable groups are not seeded on first connection.
- Around line 366-372: Initialize the scoped snapshot before making the new
client visible in scopedClients. In ControlPlaneService’s client-registration
flow around ScopedClient, refreshScopedClient, and cache.setSnapshot, build the
scoped snapshot first and only then publish the ScopedClient with putIfAbsent so
a concurrent first request cannot observe the key before the snapshot exists.
---
Outside diff comments:
In
`@xds/src/main/java/com/linecorp/centraldogma/xds/internal/CentralDogmaXdsResources.java`:
- Around line 149-203: The snapshot publication in
CentralDogmaXdsResources.snapshot() is split across multiple volatile fields, so
snapshot(Set<String>) can observe updated group indexes with an older
currentSnapshot and return a mixed-revision xDS view. Fix this by making
snapshot() and snapshot(Set<String>) read from one consistent immutable state
(for example, a single state holder containing currentSnapshot plus
clustersByGroup/listenersByGroup/routesByGroup) or by synchronizing both methods
so all resources are assembled from the same revision.
---
Nitpick comments:
In
`@xds/src/test/java/com/linecorp/centraldogma/xds/internal/XdsResourceWatchingServiceTest.java`:
- Around line 105-106: The test override of onDiffHandled currently ignores the
forwarded groupName, so it cannot verify that handleDiff passes the correct
group through. Update XdsResourceWatchingServiceTest to assert the group-scoped
callback argument from onDiffHandled and consume the corresponding queue entry
after each commit, matching the existing resource event assertions so
regressions in group forwarding are caught.
🪄 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: 17e1d14d-1e65-49b2-8aa0-4fc7ba4f76dc
📒 Files selected for processing (74)
it/xds-member-permission/src/test/java/com/linecorp/centraldogma/server/test/XdsMemberPermissionTest.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/auth/RequiresRepositoryRoleDecorator.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/storage/project/ProjectApiManager.javaserver/src/main/java/com/linecorp/centraldogma/server/metadata/MetadataService.javawebapp/build.gradlewebapp/src/dogma/common/components/Deferred.tsxwebapp/src/dogma/common/components/DeleteConfirmationModal.tsxwebapp/src/dogma/common/components/GroupSelector.tsxwebapp/src/dogma/common/components/JsonEditor.tsxwebapp/src/dogma/common/components/Navbar.tsxwebapp/src/dogma/common/components/NotificationWrapper.tsxwebapp/src/dogma/common/components/Sidebar.tsxwebapp/src/dogma/common/components/XdsLayout.tsxwebapp/src/dogma/common/useGroupAdminAccess.tswebapp/src/dogma/common/useGroupExists.tswebapp/src/dogma/common/useGroupReadAccess.tswebapp/src/dogma/common/useGroupWriteAccess.tswebapp/src/dogma/common/useXdsRoute.tswebapp/src/dogma/features/api/apiSlice.tswebapp/src/dogma/features/api/baseQuery.tswebapp/src/dogma/features/appidentity/AppIdentityDto.tswebapp/src/dogma/features/auth/UserDto.tswebapp/src/dogma/features/auth/authSlice.tswebapp/src/dogma/features/notification/notificationSlice.tswebapp/src/dogma/features/services/ErrorMessageParser.tswebapp/src/dogma/features/xds/AddRepositoryRole.tsxwebapp/src/dogma/features/xds/CredentialDto.tswebapp/src/dogma/features/xds/CredentialsTab.tsxwebapp/src/dogma/features/xds/DangerZone.tsxwebapp/src/dogma/features/xds/DataTable.tsxwebapp/src/dogma/features/xds/GroupList.tsxwebapp/src/dogma/features/xds/K8sAggregatorEditor.tsxwebapp/src/dogma/features/xds/K8sAggregatorList.tsxwebapp/src/dogma/features/xds/MetadataDto.tswebapp/src/dogma/features/xds/NewGroup.tsxwebapp/src/dogma/features/xds/PermissionsTab.tsxwebapp/src/dogma/features/xds/ResourceEditor.tsxwebapp/src/dogma/features/xds/ResourceGraph.tsxwebapp/src/dogma/features/xds/ResourceList.tsxwebapp/src/dogma/features/xds/RoleSection.tsxwebapp/src/dogma/features/xds/XdsTypes.tswebapp/src/dogma/features/xds/xdsApiSlice.tswebapp/src/dogma/features/xds/xdsReferences.tswebapp/src/dogma/store.tswebapp/src/dogma/util/auth.tswebapp/src/pages/_app.tsxwebapp/src/pages/app/xds/group.tsxwebapp/src/pages/app/xds/index.tsxwebapp/src/pages/app/xds/k8s-aggregator.tsxwebapp/src/pages/app/xds/resource.tsxwebapp/tsconfig.tsbuildinfoxds/build.gradlexds/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/group/v1/XdsGroupService.javaxds/src/main/java/com/linecorp/centraldogma/xds/internal/CentralDogmaSnapshotResources.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/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/XdsTestServer.javaxds/src/test/java/com/linecorp/centraldogma/xds/internal/CreatingInternalGroupPlugin.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/XdsResourceWatchingServiceTest.javaxds/src/test/java/com/linecorp/centraldogma/xds/internal/XdsWebEnabledFlagTest.javaxds/src/test/java/com/linecorp/centraldogma/xds/internal/XdsWritePermissionTest.java
💤 Files with no reviewable changes (1)
- webapp/src/dogma/features/auth/UserDto.ts
✅ Files skipped from review due to trivial changes (10)
- xds/src/test/java/com/linecorp/centraldogma/xds/internal/XdsWebEnabledFlagTest.java
- webapp/src/dogma/common/useGroupReadAccess.ts
- webapp/src/dogma/common/useGroupWriteAccess.ts
- webapp/src/dogma/features/xds/GroupList.tsx
- webapp/src/dogma/features/xds/RoleSection.tsx
- webapp/src/dogma/common/components/Deferred.tsx
- webapp/src/dogma/util/auth.ts
- server/src/main/java/com/linecorp/centraldogma/server/internal/api/auth/RequiresRepositoryRoleDecorator.java
- webapp/src/dogma/features/xds/MetadataDto.ts
- it/xds-member-permission/src/test/java/com/linecorp/centraldogma/server/test/XdsMemberPermissionTest.java
🚧 Files skipped from review as they are similar to previous changes (12)
- xds/src/main/java/com/linecorp/centraldogma/xds/route/v1/XdsRouteService.java
- xds/src/main/java/com/linecorp/centraldogma/xds/internal/CentralDogmaSnapshotResources.java
- xds/src/main/java/com/linecorp/centraldogma/xds/k8s/v1/XdsKubernetesService.java
- xds/src/main/java/com/linecorp/centraldogma/xds/listener/v1/XdsListenerService.java
- xds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsResourceManager.java
- xds/src/main/java/com/linecorp/centraldogma/xds/cluster/v1/XdsClusterService.java
- xds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsEndpointReadService.java
- xds/src/test/java/com/linecorp/centraldogma/xds/internal/CreatingInternalGroupPlugin.java
- webapp/build.gradle
- server/src/main/java/com/linecorp/centraldogma/server/internal/storage/project/ProjectApiManager.java
- server/src/main/java/com/linecorp/centraldogma/server/metadata/MetadataService.java
- xds/src/main/java/com/linecorp/centraldogma/xds/endpoint/v1/XdsEndpointService.java
This was referenced Jul 1, 2026
This was referenced Jul 13, 2026
This was referenced Jul 21, 2026
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:
Modifications:
./gradlew :xds:runXdsTestServerResult: