feat(cmd-api-server): add ConnectRPC auto-registration for plugins #3183
Conversation
This file contains 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
petermetz
requested review from
takeutak,
izuru0,
jagpreetsinghsasan,
VRamakrishna,
sandeepnRES and
outSH
as code owners
petermetz
added a commit
to petermetz/cacti
that referenced
this pull request
Apr 4, 2024
1. This leverages the newly introduced methods in core-api that the API server is using to probe if a plugin has ConnectRPC support or not. 2. There is support for both HTTP 1.1 and HTTP 2. The caveat here is that HTTP 2 is not supported by ExpressJS so we pulled in Fastify to handle those type of requests and that means that HTTP 2 ConnectRPC traffic has to go through a different port compared to the HTTP 1.1 ConnectRPC traffic. 3. The lesson here is that we probably need to migrate away from ExpressJS longer term because it does not (and from the looks of it will not ever) support HTTP 2 which is probably going to be a bit of technical debt/ limiting factor in architectural decisions going forward for both Cacti maintainers and Cacti users. 4. A new code generator has been introduced by this commit as well which is @buf/build - the tool where ConnectRPC originates from. The scripts are structured in such a way that this should be seamlessly integrated into the existing `codegen` root level script and therefore also the CI. 5. There is test coverage for both HTTP 1.1 and HTTP 2 traffic in the file at ```sh packages/cactus-test-plugin-keychain-memory/src/test/typescript/integration/ test-keychain-memory-crpc-api-server.test.ts ``` 6. The test case referenced above is also the example on how to use the ConnectRPC client (very similar to the HTTP client we already had before) Depends on hyperledger-cacti#3183 Signed-off-by: Peter Somogyvari <[email protected]>
5 tasks
outSH
approved these changes
Apr 8, 2024
petermetz
force-pushed
the
feat-cmd-api-server-connect-grpc-flavor
branch
from
d27643c to
132c9af
Compare
1. This is enabling plugins to expose their operations via ConnectRPC services which is very similar to gRPC but it comes with a few extra bells and whistles that can come in very handy. 2. There is an upcoming pull request that makes it so that the keychain memory plugin implements and registers its services via this newly added hook of the API server. The importance of this is that test coverage for the code in this commit resides on another branch, meaning that even though there are no new test cases on this branch, the feature has been extensively tested and there is test-automation in place to continue verifying it as well. 3. The main difference between the hook methods are that for CRPC the API server expects an array of service definition+implementation pairs instead of just a single one. This was a design decision forced by the issues with implementing separate services in a single class: The compiler was hard to appease in a way that kept the code clean. gRPC did not suffer from this and therefore the registration methods defined for that only return a single gRPC service defintion+implementation pair which can combine any number of .proto services. Signed-off-by: Peter Somogyvari <[email protected]>
petermetz
force-pushed
the
feat-cmd-api-server-connect-grpc-flavor
branch
from
132c9af to
f422ab1
Compare
petermetz
added a commit
to petermetz/cacti
that referenced
this pull request
Apr 11, 2024
1. This leverages the newly introduced methods in core-api that the API server is using to probe if a plugin has ConnectRPC support or not. 2. There is support for both HTTP 1.1 and HTTP 2. The caveat here is that HTTP 2 is not supported by ExpressJS so we pulled in Fastify to handle those type of requests and that means that HTTP 2 ConnectRPC traffic has to go through a different port compared to the HTTP 1.1 ConnectRPC traffic. 3. The lesson here is that we probably need to migrate away from ExpressJS longer term because it does not (and from the looks of it will not ever) support HTTP 2 which is probably going to be a bit of technical debt/ limiting factor in architectural decisions going forward for both Cacti maintainers and Cacti users. 4. A new code generator has been introduced by this commit as well which is @buf/build - the tool where ConnectRPC originates from. The scripts are structured in such a way that this should be seamlessly integrated into the existing `codegen` root level script and therefore also the CI. 5. There is test coverage for both HTTP 1.1 and HTTP 2 traffic in the file at ```sh packages/cactus-test-plugin-keychain-memory/src/test/typescript/integration/ test-keychain-memory-crpc-api-server.test.ts ``` 6. The test case referenced above is also the example on how to use the ConnectRPC client (very similar to the HTTP client we already had before) Depends on hyperledger-cacti#3183 Signed-off-by: Peter Somogyvari <[email protected]>
petermetz
added a commit
to petermetz/cacti
that referenced
this pull request
Apr 11, 2024
1. This leverages the newly introduced methods in core-api that the API server is using to probe if a plugin has ConnectRPC support or not. 2. There is support for both HTTP 1.1 and HTTP 2. The caveat here is that HTTP 2 is not supported by ExpressJS so we pulled in Fastify to handle those type of requests and that means that HTTP 2 ConnectRPC traffic has to go through a different port compared to the HTTP 1.1 ConnectRPC traffic. 3. The lesson here is that we probably need to migrate away from ExpressJS longer term because it does not (and from the looks of it will not ever) support HTTP 2 which is probably going to be a bit of technical debt/ limiting factor in architectural decisions going forward for both Cacti maintainers and Cacti users. 4. A new code generator has been introduced by this commit as well which is @buf/build - the tool where ConnectRPC originates from. The scripts are structured in such a way that this should be seamlessly integrated into the existing `codegen` root level script and therefore also the CI. 5. There is test coverage for both HTTP 1.1 and HTTP 2 traffic in the file at ```sh packages/cactus-test-plugin-keychain-memory/src/test/typescript/integration/ test-keychain-memory-crpc-api-server.test.ts ``` 6. The test case referenced above is also the example on how to use the ConnectRPC client (very similar to the HTTP client we already had before) Depends on hyperledger-cacti#3183 Signed-off-by: Peter Somogyvari <[email protected]>
petermetz
added a commit
that referenced
this pull request
Apr 11, 2024
1. This leverages the newly introduced methods in core-api that the API server is using to probe if a plugin has ConnectRPC support or not. 2. There is support for both HTTP 1.1 and HTTP 2. The caveat here is that HTTP 2 is not supported by ExpressJS so we pulled in Fastify to handle those type of requests and that means that HTTP 2 ConnectRPC traffic has to go through a different port compared to the HTTP 1.1 ConnectRPC traffic. 3. The lesson here is that we probably need to migrate away from ExpressJS longer term because it does not (and from the looks of it will not ever) support HTTP 2 which is probably going to be a bit of technical debt/ limiting factor in architectural decisions going forward for both Cacti maintainers and Cacti users. 4. A new code generator has been introduced by this commit as well which is @buf/build - the tool where ConnectRPC originates from. The scripts are structured in such a way that this should be seamlessly integrated into the existing `codegen` root level script and therefore also the CI. 5. There is test coverage for both HTTP 1.1 and HTTP 2 traffic in the file at ```sh packages/cactus-test-plugin-keychain-memory/src/test/typescript/integration/ test-keychain-memory-crpc-api-server.test.ts ``` 6. The test case referenced above is also the example on how to use the ConnectRPC client (very similar to the HTTP client we already had before) Depends on #3183 Signed-off-by: Peter Somogyvari <[email protected]>
5 tasks
sandeepnRES
pushed a commit
to sandeepnRES/cacti
that referenced
this pull request
Jul 30, 2024
1. This leverages the newly introduced methods in core-api that the API server is using to probe if a plugin has ConnectRPC support or not. 2. There is support for both HTTP 1.1 and HTTP 2. The caveat here is that HTTP 2 is not supported by ExpressJS so we pulled in Fastify to handle those type of requests and that means that HTTP 2 ConnectRPC traffic has to go through a different port compared to the HTTP 1.1 ConnectRPC traffic. 3. The lesson here is that we probably need to migrate away from ExpressJS longer term because it does not (and from the looks of it will not ever) support HTTP 2 which is probably going to be a bit of technical debt/ limiting factor in architectural decisions going forward for both Cacti maintainers and Cacti users. 4. A new code generator has been introduced by this commit as well which is @buf/build - the tool where ConnectRPC originates from. The scripts are structured in such a way that this should be seamlessly integrated into the existing `codegen` root level script and therefore also the CI. 5. There is test coverage for both HTTP 1.1 and HTTP 2 traffic in the file at ```sh packages/cactus-test-plugin-keychain-memory/src/test/typescript/integration/ test-keychain-memory-crpc-api-server.test.ts ``` 6. The test case referenced above is also the example on how to use the ConnectRPC client (very similar to the HTTP client we already had before) Depends on hyperledger-cacti#3183 Signed-off-by: Peter Somogyvari <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
services which is very similar to gRPC but it comes with a few extra
bells and whistles that can come in very handy.
memory plugin implements and registers its services via this newly added
hook of the API server. The importance of this is that test coverage for
the code in this commit resides on another branch, meaning that even though
there are no new test cases on this branch, the feature has been extensively
tested and there is test-automation in place to continue verifying it
as well.
API server expects an array of service definition+implementation pairs
instead of just a single one. This was a design decision forced by the
issues with implementing separate services in a single class: The compiler
was hard to appease in a way that kept the code clean. gRPC did not suffer
from this and therefore the registration methods defined for that only
return a single gRPC service defintion+implementation pair which can combine
any number of .proto services.
Signed-off-by: Peter Somogyvari [email protected]
Pull Request Requirements
upstream/mainbranch and squashed into single commit to help maintainers review it more efficient and to avoid spaghetti git commit graphs that obfuscate which commit did exactly what change, when and, why.-sflag when usinggit commitcommand. You may refer to this link for more information.Character Limit
A Must Read for Beginners
For rebasing and squashing, here's a must read guide for beginners.