Page MenuHomePhabricator

[Identity] Rename IdentityService service to IdentityKeyserverService
ClosedPublic

Authored by jon on Mar 8 2023, 9:05 AM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 23 2024, 7:39 AM
Unknown Object (File)
Feb 23 2024, 7:14 AM
Unknown Object (File)
Feb 23 2024, 5:01 AM
Unknown Object (File)
Feb 23 2024, 12:49 AM
Unknown Object (File)
Feb 21 2024, 5:25 AM
Unknown Object (File)
Feb 21 2024, 4:06 AM
Unknown Object (File)
Feb 21 2024, 1:58 AM
Unknown Object (File)
Feb 20 2024, 11:41 PM
Subscribers

Details

Summary

We are onboarding a client <-> identity gRPC service, and in order
to distinguish between the existing keyserver <-> identity gRPC service it
would be great for the identity service reflect the scenario
it's being used in.

Part of https://linear.app/comm/issue/ENG-3264

Test Plan
nix develop

comm-dev services start # start localstack

# Configure "aws" resources on localstack
(cd services/terraform && ./run.sh)

# start identity service and run mobile workflow
(cd services/identity && cargo run -- server &)
(cd keyserver && yarn dev &)
(cd native && yarn dev &)

Then you should be able to create a new user, update the password,
and delete a user with the actions being reflected in dynamodb.
Output of the actions should also be appearing in identity logs.

Diff Detail

Repository
rCOMM Comm
Branch
jonringer/rename-grpc-keyserver
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

identity.proto should probably be renamed as identity_keyserver.proto. As I would like to add the client <-> identity protobuf file as identity_client.proto, which is confusing as we refer to the keyserver protos as "identity_client" in some parts of the code base. In other words, align the naming with the "comm" defintion of terms and move away with the more general gRPC definition of terms.

This honestly feels like bike-shedding… I’d love if we could start out the work on this project with a focus on derisking the big risks, rather than renaming things

This revision is now accepted and ready to land.Mar 8 2023, 12:30 PM

This honestly feels like bike-shedding… I’d love if we could start out the work on this project with a focus on derisking the big risks, rather than renaming things

Not changing it we would have IdentityService and IdentityClientService. In the renamed world it's IdentityKeyserverService and IdentityClientService.

rather than renaming things

I would like to lessen the mental burden of moving around the identity code. The following diff D7003 will add another service, and would like to distinguish between the two in a more meaningful way.

Also, this diff didn't take much time, typing everything out for arc diff probably took more time than renaming, building, and doing a quick test.

I'm more confused as to why are we're separating the protos / services out at all

the set of RPCs that devices will call is different than the set that ashoat's keyserver can call today. also, the actual messages are slightly different, as the linear issue i linked explains. we think it makes more sense to just have a separate service for devices, rather than introduce a bunch of conditional logic in the current service. of course, both services will be served by the Identity Service (this is pretty simple). eventually, when we switch to the identity service being the source of truth for user info, we can deprecate the original service (the one being renamed here)

we can deprecate the original service (the one being renamed here)

We can deprecate parts of it, keyserver-specific things like VerifyUser will still remain. But pretty much everything dealing with account creation, updating, or deletion can eventually be removed. Other items such as https://linear.app/comm/issue/ENG-3260 will require keyserver-specific additions to the service as well.

I'm more confused as to why are we're separating the protos / services out at all

One thing not mentioned by @varun is that the current AUTH_TOKEN hack we use for keyserver is implemented on a per-service basis. So we can roll out a new service without "authorization headers", while maintaining the current keyserver paradigm.

One thing not mentioned by @varun is that the current AUTH_TOKEN hack we use for keyserver is implemented on a per-service basis. So we can roll out a new service without "authorization headers", while maintaining the current keyserver paradigm.

Thanks for explaining, that makes sense – the two services need to be different between the auth for them is different