Add xDS group service. #981

minwoox · 2024-07-15T14:43:09Z

Motivation:
A user might want to manage her own xDS resources with a group of people who have permission to create and update these resources. By creating an xDS group (a Central Dogma repository), she can easily manage these xDS resources and grant write permissions to specific people, allowing them to manage the resources under Central Dogma's authorization system.

Modifications:

Added XdsGroupService to create a group, which is a Central Dogma repository.
- With this group, a user can easily manage privileged people under the Central Dogma authorization system.
- xds_group.proto file is also added.
  - All proto files might be published in a separate module later for other clients such as LEGY.
Removed MetadataServiceInjector.
- MetadataService is injected when a decorator is created using a DependencyInjector.

Result:

You can now create xDS groups to manage your xDS resources under the Central Dogma authorization system.

Motivation: A user might want to manage her own xDS resources with a group of people who have the permission to create and update these resources. By creating an xDS application (a Central Dogma repository), she can easily manage these xDS resources and grant write permissions to specific people, allowing them to manage the resources under Central Dogma's authorization system. Modifications: - Added `XdsApplicationService` to create an application, which is a Central Dogma repository. - With this application, a user can easily manage privileged people under the Central Dogma authorization system. Result: - You can now create xDS applications to manage your xDS resources under the Central Dogma authorization system.

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

...ain/java/com/linecorp/centraldogma/server/internal/api/auth/RequiresPermissionDecorator.java

xds/src/main/proto/centraldogma/xds/application/v1/xds_application.proto

server/src/main/java/com/linecorp/centraldogma/server/plugin/PluginInitContext.java

xds/src/main/java/com/linecorp/centraldogma/xds/application/v1/XdsApplicationService.java

xds/src/main/proto/centraldogma/xds/application/v1/xds_application.proto

jrhee17

Looks good overall, the /groups prefix is the only issue bothering me. The rest looks good 👍

server/src/main/java/com/linecorp/centraldogma/server/plugin/PluginInitContext.java

jrhee17 · 2024-07-18T10:04:01Z

xds/src/main/java/com/linecorp/centraldogma/xds/group/v1/XdsGroupService.java

+    public void createGroup(CreateGroupRequest request,
+                            StreamObserver<Group> responseObserver) {
+        final String groupName = request.getGroup().getName();
+        final String name = removePrefix("groups/", groupName);


Taking another look at this PR, it seems like we're going need to extra logic to remove/add groups/ in all user-facing locations. i.e. API paths, DTOs, UI, metrics, etc.. I'm not sure if it's realistic and worth it to ensure this.

Do you mind explaining why we need this prefix again?

Like I said before, I'm trying to adhere to the AIP as much as possible:
https://google.aip.dev/122
It recommends in this way:

publishers/123/books/les-miserables users/vhugo1802 Resource name components should usually alternate between collection identifiers (example: publishers, books, users) and resource IDs (example: 123, les-miserables, vhugo1802).

we're going need to extra logic to remove/add groups/ in all user-facing locations.

Yeah, that is true. But the logic isn't so many, and we still need that logic for validating and removing group name from the cluster name.
I thought the format (groups/{group}/clusters/{cluster}), which follows the guideline, was much better than the original format ({group}/{cluster}).

If you're strong on following the AIP, then sure. Ideally I prefer that there is a cross-cutting solution to apply this, but I guess we'll be doing this manually then.

e.g. If a new API is added where groupName is the parameter, we should be conscious to add/remove the prefix.

Ideally I prefer that there is a cross-cutting solution to apply this

I also prefer applying the cross-cutting solution, which is why I hesitated to follow the AIP at first. 😕 However, I've decided to try using gRPC for this API, and it’s not easy to apply AOP even if I don’t follow AIP because it needs parameters in the message.
I also considered abandoning gRPC and just using REST, but decided against it. So, I followed the AIP.

jrhee17 · 2024-07-18T10:06:38Z

xds/src/main/java/com/linecorp/centraldogma/xds/internal/ControlPlanePlugin.java

+                           .addService(new XdsGroupService(
+                                   projectManager, pluginInitContext.commandExecutor()))
+                           .enableHttpJsonTranscoding(true).build();
+        sb.service(xdsApplicationService, pluginInitContext.authService());


Question) Just so I understand the current state, IIUC only admins have access to internal projects. Hence, am I understanding correctly that only admins will be able to use XdsGroupService for now?

Anyone can use the XdsGroupService for now as I talked before.
https://github.com/line/centraldogma/pull/981/files#diff-6530b293347a15e8133c26cfb4221f0b565c32f0a6cccb8c1d44c0d78108c8bfR62

After modifying the authorization system similar to GitHub, anyone can create a group, and the one who created the group can add anyone or tokens to the group who will have the write permission for the resources in the group.

I see, I must have glanced through the diffset. I understood that for now, anyone can create/delete anyone's group 👍

jrhee17 · 2024-07-18T10:08:41Z

xds/src/main/java/com/linecorp/centraldogma/xds/group/v1/XdsGroupService.java

+                            responseObserver.onError(alreadyExistsException(name));
+                        } else {
+                            responseObserver.onError(
+                                    Status.INTERNAL.withCause(cause).asRuntimeException());


Note: I'm curious on how we'll deal with handling exceptions later on as sometimes we perform special operations like in ReplicationLagTolerantCentralDogma for RevisionNotFoundException.

For now, I assume creating a repository and getting the repository immediately won't necessary succeed if the get is from a different replica.

Anyways, I guess the current approach is fine for now

That's a good question. I will add the gRPC exception handler for the xDS services after I all set up the services.

jrhee17 · 2024-07-18T10:10:03Z

xds/src/main/proto/centraldogma/xds/group/v1/xds_group.proto

+  }
+
+  // Deletes an group.
+  rpc DeleteGroup(DeleteGroupRequest) returns (google.protobuf.Empty) {


Note: This is a red flag personally since I think there is always the possibility that additional fields will need to be added. Having said this, I guess it's fine to try adhering to the AIP like you proposed for now.

This is a red flag personally since I think there is always the possibility that additional fields will need to be added.

Would you mind explaining which fields are likely to be added later?
If that's reasonable, I will define a message and return it instead of the Empty.

Let's just follow the AIP for now since the point is to ease these types of discussions.

jrhee17

👍 👍 👍

ikhoon

Thanks!

ikhoon · 2024-07-19T05:59:34Z

xds/src/main/java/com/linecorp/centraldogma/xds/group/v1/XdsGroupService.java

+
+    @Override
+    public void createGroup(CreateGroupRequest request,
+                            StreamObserver<Group> responseObserver) {


Optional) What do you think of using Reactive-gRPC instead of the vanilla gRPC? It is only used for server modules so we are relatively free of dependencies. Reactive-gRPC could remove boilerplate code such as .onNext() + .onCopmlete().

Thanks for the suggestion. 👍
Well, honestly, I didn't want to use any other gRPC libraries because the logic is relatively simple (All methods are unary) and it might take more time (maybe just for me) when troubleshooting.

However, that doesn't mean that I will never use grpc-reactor. I will definitely use it when I think it gives more advantages or when I want to use it. 🤣

minwoox added the new feature label Jul 15, 2024

minwoox added this to the 0.68.0 milestone Jul 15, 2024

minwoox requested review from ikhoon, jrhee17 and trustin as code owners July 15, 2024 14:43

minwoox changed the title ~~Add xDS application service.~~ [WIP] Add xDS application service. Jul 15, 2024

minwoox added 2 commits July 16, 2024 09:57

Merge branch 'main' into xds_application

d3ef4d6

Update

99f4ecd

minwoox changed the title ~~[WIP] Add xDS application service.~~ Add xDS application service. Jul 16, 2024

minwoox added 2 commits July 16, 2024 15:46

Update

4a5b661

rename package

71c9706

ikhoon reviewed Jul 16, 2024

View reviewed changes

Address comments from @ikhoon

b4f2dbb

minwoox changed the title ~~Add xDS application service.~~ Add xDS group service. Jul 17, 2024

rename to group

ded0ff2

jrhee17 reviewed Jul 18, 2024

View reviewed changes

jrhee17 approved these changes Jul 19, 2024

View reviewed changes

ikhoon approved these changes Jul 19, 2024

View reviewed changes

Merge branch 'main' into xds_application

1e20507

minwoox modified the milestones: 0.68.0, 0.69.0 Jul 23, 2024

minwoox merged commit 5912bf7 into line:main Aug 4, 2024
8 of 10 checks passed

minwoox deleted the xds_application branch August 4, 2024 07:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add xDS group service. #981

Add xDS group service. #981

minwoox commented Jul 15, 2024 •

edited

Loading

jrhee17 left a comment

jrhee17 Jul 18, 2024

minwoox Jul 18, 2024

jrhee17 Jul 19, 2024

minwoox Jul 19, 2024 •

edited

Loading

jrhee17 Jul 18, 2024

minwoox Jul 18, 2024

jrhee17 Jul 19, 2024

jrhee17 Jul 18, 2024

minwoox Jul 18, 2024

jrhee17 Jul 18, 2024

minwoox Jul 18, 2024

jrhee17 Jul 19, 2024

jrhee17 left a comment

ikhoon left a comment

ikhoon Jul 19, 2024

minwoox Jul 19, 2024

Add xDS group service. #981

Add xDS group service. #981

Conversation

minwoox commented Jul 15, 2024 • edited Loading

jrhee17 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

minwoox Jul 19, 2024 • edited Loading

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

jrhee17 left a comment

Choose a reason for hiding this comment

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

minwoox commented Jul 15, 2024 •

edited

Loading

minwoox Jul 19, 2024 •

edited

Loading