Move Mirroring Configuration to Individual Repositories #1086

minwoox · 2025-01-03T11:36:11Z

Motivation:
Mirroring configurations should belong to the repository where the mirroring is executed to ensure proper ownership and management alignment.

Modifications:

Implemented migration job that relocated mirroring configurations to their respective repositories.
- Updated Mirroring REST API to reflect these changes.
Updated credential IDs in mirroring configurations to use a full resource ID format:
- Example for a project credential: projects/foo/credentials/foo-credential.
- Example for a repository credential: projects/foo/repos/bar/credentials/bar-credential.
Adjusted role requirements for credential management:
- Project OWNER: Can create, update, and delete credentials.
- Project MEMBER: Can read credentials.
- Removed dependency on Meta repository access roles.

Result:

Mirroring configurations are now repository-specific.
Dependency on Meta repository access roles has been eliminated.

minwoox · 2025-01-07T11:18:35Z

Will continue after #1085 is merged. 😉

codecov · 2025-01-17T01:35:15Z

Codecov Report

Attention: Patch coverage is 78.86905% with 71 lines in your changes missing coverage. Please review.

Project coverage is 70.37%. Comparing base (f4dda40) to head (a8188b2).
Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
...nal/mirror/MigratingMirrorToRepositoryService.java	72.94%	17 Missing and 6 partials ⚠️
...rp/centraldogma/internal/api/v1/MirrorRequest.java	61.90%	15 Missing and 1 partial ⚠️
...traldogma/server/internal/mirror/MirrorRunner.java	68.75%	8 Missing and 2 partials ⚠️
...rnal/storage/repository/DefaultMetaRepository.java	90.58%	6 Missing and 2 partials ⚠️
...rver/internal/storage/repository/MirrorConfig.java	77.14%	3 Missing and 5 partials ⚠️
...ldogma/server/internal/api/MirroringServiceV1.java	87.50%	2 Missing and 1 partial ⚠️
...erver/internal/mirror/MirrorSchedulingService.java	57.14%	3 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1086      +/-   ##
============================================
+ Coverage     70.29%   70.37%   +0.08%     
- Complexity     4284     4443     +159     
============================================
  Files           427      442      +15     
  Lines         17258    17882     +624     
  Branches       1915     1974      +59     
============================================
+ Hits          12131    12585     +454     
- Misses         4077     4240     +163     
- Partials       1050     1057       +7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

minwoox · 2025-01-17T02:10:57Z

...ava/com/linecorp/centraldogma/server/internal/mirror/MigratingMirrorToRepositoryService.java

+        commandExecutor.execute(Command.updateServerStatus(ServerStatus.REPLICATION_ONLY))
+                       .get(1, TimeUnit.MINUTES);
+        logger.info("Starting Mirrors migration ...");
+        if (commandExecutor instanceof ZooKeeperCommandExecutor) {
+            logger.debug("Waiting for 10 seconds to make sure that all cluster have been notified of the " +
+                         "read-only mode ...");
+            Thread.sleep(10000);
+        }


I prefer migrating without entering read-only mode because

It's not critical if mirroring is temporarily unavailable for a minute.

Any issues can be promptly addressed if they arise.

If this is acceptable, I will remove these lines of code. Please let me know your opinion.

jrhee17

Understood the changes

jrhee17 · 2025-01-21T02:13:51Z

...ava/com/linecorp/centraldogma/server/internal/mirror/MigratingMirrorToRepositoryService.java

+        if (entries.isEmpty()) {
+            return false;
+        }
+        repository.parent().name();


Suggested change

repository.parent().name();

...java/com/linecorp/centraldogma/server/internal/storage/repository/DefaultMetaRepository.java

ikhoon · 2025-01-21T08:13:58Z

While reviewing this PR, I found out two bugs in DefaultMirrorAccessControler. Fixed at 5cae78a (#1086)

ikhoon

I couldn't fully review this PR. I will do another round tomorrow.

ikhoon · 2025-01-21T05:31:31Z

server/src/main/java/com/linecorp/centraldogma/server/internal/api/CredentialServiceV1.java

@@ -66,7 +67,7 @@ public CredentialServiceV1(ProjectApiManager projectApiManager, CommandExecutor
     *
     * <p>Returns the list of the credentials in the project.
     */
-    @RequiresRepositoryRole(value = RepositoryRole.READ, repository = Project.REPO_META)
+    @RequiresProjectRole(ProjectRole.MEMBER)


Currently, credentials are only exposed to owners in the UI. Do you want to allow members to read credentials? It may not be harmful since all sensitive information is masked.

We need to allow the admin of each repository to read the credentials. Since it's read, I thought we could give the read permission to all members.

server/src/main/java/com/linecorp/centraldogma/server/internal/api/MirroringServiceV1.java

ikhoon · 2025-01-21T08:27:22Z

server/src/main/java/com/linecorp/centraldogma/server/mirror/Mirror.java

+     * It is in the form of {@code "projects/<project>/credentials/<credential>"} or
+     * {@code "projects/<project>/repos/<repo>/credentials/<credential>"}.
+     */
+    String mirrorCredentialId();


What do you think of making Credential return the resource name instead?
The ownership of the resource belongs to Credential so it would make more sense to delegate it such as Credential.resourceName() or Credential.name()?
https://google.aip.dev/122#fields-representing-resource-names

Alright, let me try that. 😉

@ikhoon
I tried that approach, but it seems to make the logic and codebase more complicated.
What do you think of just removing credential.id() and using crendential.name() as we do for representing an xDS resource?

Never mind. Let me just add the resourceName field to the classes. 😉

ikhoon · 2025-01-21T14:08:36Z

server/src/main/java/com/linecorp/centraldogma/server/internal/mirror/MirrorRunner.java

+                                        throw new MirrorAccessException(
+                                                "The mirroring from " + mirror.remoteRepoUri() +
+                                                " is not allowed: " +
+                                                mirrorKey.projectName + '/' + mirrorKey.mirrorId);


To apply the same approach as above here

Suggested change

mirrorKey.projectName + '/' + mirrorKey.mirrorId);

mirrorKey.repoName + '/' + mirrorKey.mirrorId);

ikhoon · 2025-01-21T14:19:20Z

...java/com/linecorp/centraldogma/server/internal/storage/repository/DefaultMetaRepository.java

@@ -125,57 +152,124 @@ public CompletableFuture<Mirror> mirror(String id, Revision revision) {
                throw new RepositoryMetadataException("failed to load the mirror configuration", e);
            }

-            final CompletableFuture<List<Credential>> credentials;
            if (Strings.isNullOrEmpty(c.credentialId())) {


nit:

Suggested change

if (Strings.isNullOrEmpty(c.credentialId())) {

if (c.credentialId().isEmpty()) {

ikhoon · 2025-01-21T14:34:46Z

...src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/MirrorConfig.java

-            for (Credential c : credentials) {
+    private Credential findCredential(Map<String, List<Credential>> repoCredentials,
+                                      List<Credential> projectCredentials) {
+        if (repoCredential) {


Suggestion) This logic seems complicated to be included in a data class. What do you think of extracting the methods to MirrorConverter?

ikhoon · 2025-01-21T14:46:16Z

...ava/com/linecorp/centraldogma/server/internal/mirror/MigratingMirrorToRepositoryService.java

+                    continue;
+                }
+                final MirrorConfig newMirrorConfig = mirrorConfig.withCredentialId(
+                        projectMirrorCredentialId(repository.parent().name(), mirrorConfig.credentialId()));


Should we skip migration for a MirrorConfig whose the credential ID starts with projects/? Because we don't rollback if the migration process is halt somehow. This migration could be partially applied.

ikhoon · 2025-01-21T14:48:56Z

...java/com/linecorp/centraldogma/server/internal/storage/repository/DefaultMetaRepository.java

+
+            final List<String> credentialRepoNames;
+            final boolean hasProjectCredential;
+            if (c.repoCredential()) {


Suggestion) What do you think of adding a new resource repository layer to find a credential from a given ID? This logic could be simplified as we don't have to care if the ID is a repo credential.

interface MetaResourceRepository { T find(String resourceName, Class<T> clazz); } Credential credential = repositoryRepository.find(mirror.credentialId(), Credential.class);

I've used this logic not to fetch the resource one by one when multiple credentials are needed to be found. Let me encapsulate the logic into another class then.

ikhoon · 2025-01-21T14:51:02Z

...ava/com/linecorp/centraldogma/server/internal/mirror/MigratingMirrorToRepositoryService.java

+        commandExecutor.execute(Command.updateServerStatus(ServerStatus.REPLICATION_ONLY))
+                       .get(1, TimeUnit.MINUTES);
+        logger.info("Starting Mirrors migration ...");
+        if (commandExecutor instanceof ZooKeeperCommandExecutor) {
+            logger.debug("Waiting for 10 seconds to make sure that all cluster have been notified of the " +
+                         "read-only mode ...");
+            Thread.sleep(10000);
+        }


Move Mirror Config to Each Repository

e2af4e4

minwoox requested review from ikhoon, jrhee17 and trustin as code owners January 3, 2025 11:36

minwoox added 2 commits January 13, 2025 23:20

Merge branch 'main' into repo_mirror

0e9f087

WIP

a8188b2

minwoox force-pushed the repo_mirror branch from f653824 to a8188b2 Compare January 17, 2025 01:18

minwoox added 2 commits January 17, 2025 10:36

Fix

d0182e5

Fix2

e81bdc3

minwoox changed the title ~~Move Mirror Config to Each Repository~~ Move Mirroring Configuration to Individual Repositories Jan 17, 2025

minwoox added this to the 0.74.0 milestone Jan 17, 2025

minwoox added the breaking change label Jan 17, 2025

minwoox commented Jan 17, 2025

View reviewed changes

jrhee17 approved these changes Jan 21, 2025

View reviewed changes

fix bugs in DefaultMirrorAccessController

5cae78a

Address the comment from @jrhee17

4379f84

ikhoon reviewed Jan 21, 2025

View reviewed changes

minwoox added 3 commits January 22, 2025 17:53

Add resourceName

dd8ad22

WIP

a6dd836

Fix

ba3e6f7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move Mirroring Configuration to Individual Repositories #1086

Move Mirroring Configuration to Individual Repositories #1086

minwoox commented Jan 3, 2025 •

edited

Loading

minwoox commented Jan 7, 2025

codecov bot commented Jan 17, 2025

minwoox Jan 17, 2025

ikhoon Jan 21, 2025

jrhee17 left a comment

jrhee17 Jan 21, 2025

ikhoon commented Jan 21, 2025

ikhoon left a comment

ikhoon Jan 21, 2025

minwoox Jan 22, 2025

ikhoon Jan 21, 2025

minwoox Jan 22, 2025

minwoox Jan 22, 2025

minwoox Jan 22, 2025

ikhoon Jan 21, 2025

ikhoon Jan 21, 2025

ikhoon Jan 21, 2025

ikhoon Jan 21, 2025

ikhoon Jan 21, 2025

minwoox Jan 22, 2025

ikhoon Jan 21, 2025

	mirrorKey.projectName + '/' + mirrorKey.mirrorId);
	mirrorKey.repoName + '/' + mirrorKey.mirrorId);

	if (Strings.isNullOrEmpty(c.credentialId())) {
	if (c.credentialId().isEmpty()) {

Move Mirroring Configuration to Individual Repositories #1086

Are you sure you want to change the base?

Move Mirroring Configuration to Individual Repositories #1086

Conversation

minwoox commented Jan 3, 2025 • edited Loading

minwoox commented Jan 7, 2025

codecov bot commented Jan 17, 2025

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrhee17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikhoon commented Jan 21, 2025

ikhoon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox commented Jan 3, 2025 •

edited

Loading