Replace improbable-eng/grpc-web with grpc/web #6013
Assignees
Labels
component/apis-server
Issue related to kubeapps api-server
component/ui
Issue related to kubeapps UI
kind/enhancement
An issue that reports an enhancement for an implemented feature
Milestone
Comments
absoludity
added
component/ui
|
So bufbuild's
This would allow us to transition the code as follows:
If we ever change our minds and want to use envoy + gRPC-Web, it's a single-line change to switch back, so no lock-in. I'm investigating this as the way forward, looks much nicer to use as well, appears to be much more active, better documented and has the same Apache 2.0 license. |
absoludity
added a commit
that referenced
this issue
Mar 8, 2023
<!-- Before you open the request please review the following guidelines and tips to help it be more easily integrated: - Describe the scope of your change - i.e. what the change does. - Describe any known limitations with your change. - Please run any tests or examples that can exercise your modified code. Thank you for contributing! --> Part one of a series for #6013 ### Description of the change <!-- Describe the scope of your change - i.e. what the change does. --> Adds support for the typescript clients used by buf connect's gRPC-Web. ### Benefits <!-- What benefits will be realized by the code change? --> Just allows the next PR to be reviewable, only containing the actual code changes rather than the generated changes. ### Possible drawbacks <!-- Describe any known limitations with your change --> ### Applicable issues <!-- Enter any applicable Issues here (You can reference an issue using #) --> - ref #6013 ### Additional information <!-- If there's anything else that's important and relevant to your pull request, mention that information here.--> Once we've switched over, we can also look at removing this generated code altogether, since the buf connect system has support for remote plugins, enabling simple imports of the remotely generated code. --------- Signed-off-by: Michael Nelson <minelson@vmware.com>
absoludity
added a commit
that referenced
this issue
Mar 14, 2023
<!-- Before you open the request please review the following guidelines and tips to help it be more easily integrated: - Describe the scope of your change - i.e. what the change does. - Describe any known limitations with your change. - Please run any tests or examples that can exercise your modified code. Thank you for contributing! --> ### Description of the change This PR switches the dashboard over to use the new connect generated grpc-web client and associated generated classes. It was pretty tedious, with some complexity due to the different generated classes and client. ## Non-tedious actual changes - Updating the way the proto one-of (single choice for different values) work throughout to match the new library (which actually ensures there is only a single value, actually using a `OneOf` concept, whereas the old improbable library just allowed all the options at once). This was quite confusing to change in the actual code functionality as the code is written as if all options may be specified, but I didn't want to change that in this PR, just switch. - Updating the `shared/ResourceRef` class to be a child of the generated `ResourceRef` class so that we can continue to use it in place of the generated one as we're currently doing (compiler won't accept it otherwise now). Created #6062 to track removing our shared one if we can. - Updating the method used in the frontend to stream resources (and changes) for an installed package, since improbable used subscriptions while the connect one uses async iterators (need to verify this works, since I don't think we test it in CI, but it should just fall back to polling if it doesn't). - Update the `KubeappsGRPCClient` to use the new client (and the different way of setting headers/metadata). ## Tedious parts - Any object that is generated from the proto messages now needs to be instantiated with, for example, `new AvailablePackageSummary({...})` to pass the compilation, rather than the previous `{...} as AvailablePackageSummary` as this type assertion fails due to the missing methods. - Updating all the generated class imports with `_pb` to import the new versions (without deleting the old in this PR, as that blows it out by 16k lines). - Updating all the enums to the more sensible names used by the new lib - Update a lot of tests that used jest to spy on the old implementation. - Update all API method names since the connect one uses snake-case with an initial lower case :/ ### Benefits <!-- What benefits will be realized by the code change? --> Let's see what CI says, but possibly removing 16k lines of the old generated code (I'll do that in a follow-up PR). The main benefit is that we'll be using a supported grpc-web typescript client and can remove the old improbable generation in the kubeapps apis service. ### Possible drawbacks <!-- Describe any known limitations with your change --> It's a lot of changes, there maybe unexpected side-effects. ### Applicable issues <!-- Enter any applicable Issues here (You can reference an issue using #) --> - ref #6013 ### Additional information <!-- If there's anything else that's important and relevant to your pull request, mention that information here.--> --------- Signed-off-by: Michael Nelson <minelson@vmware.com>
absoludity
added a commit
that referenced
this issue
Mar 14, 2023
<!-- Before you open the request please review the following guidelines and tips to help it be more easily integrated: - Describe the scope of your change - i.e. what the change does. - Describe any known limitations with your change. - Please run any tests or examples that can exercise your modified code. Thank you for contributing! --> ### Description of the change <!-- Describe the scope of your change - i.e. what the change does. --> Just a follow-up to #6044 which removes the now unused code. ### Benefits <!-- What benefits will be realized by the code change? --> ### Possible drawbacks <!-- Describe any known limitations with your change --> ### Applicable issues <!-- Enter any applicable Issues here (You can reference an issue using #) --> - ref #6013 ### Additional information <!-- If there's anything else that's important and relevant to your pull request, mention that information here.--> Signed-off-by: Michael Nelson <minelson@vmware.com>
|
I've been investigating switching the backend over and realised that our use of grpc-gateway, to provide a ReST-ish version of the API (in addition to the grpc and grpc-web), won't be possible, since it is supported by the generated code of the tied with the standard gRPC registration of services. We don't use this outside of a liveness check (which we should replace with a grpc check anyway), but we have advertised it as an optional API (which could change at any time), and do currently use it to generate the openapi spec. This has been discussed on the connect-go project: https://github.com/bufbuild/connect-go/discussions/256 . We could potentially continue to generate the openapi spec by updating the http annotations to match the connect http. |
absoludity
added a commit
that referenced
this issue
Mar 20, 2023
<!-- Before you open the request please review the following guidelines and tips to help it be more easily integrated: - Describe the scope of your change - i.e. what the change does. - Describe any known limitations with your change. - Please run any tests or examples that can exercise your modified code. Thank you for contributing! --> ### Description of the change <!-- Describe the scope of your change - i.e. what the change does. --> This PR just adds the connect-go grpc tooling and code generation for the services. No integration is done here. ### Benefits <!-- What benefits will be realized by the code change? --> Step 1 towards replacing the improbably grpc-web library. ### Possible drawbacks <!-- Describe any known limitations with your change --> ### Applicable issues <!-- Enter any applicable Issues here (You can reference an issue using #) --> - ref #6013 ### Additional information <!-- If there's anything else that's important and relevant to your pull request, mention that information here.--> Signed-off-by: Michael Nelson <minelson@vmware.com>
This was referenced Mar 29, 2023
Actually, this isn't true. We are able to continue using the grpc-gateway.. we just need to forward the http requests to the new endpoint and it works fine. See #6150 |
absoludity
added a commit
that referenced
this issue
Mar 29, 2023
<!-- Before you open the request please review the following guidelines and tips to help it be more easily integrated: - Describe the scope of your change - i.e. what the change does. - Describe any known limitations with your change. - Please run any tests or examples that can exercise your modified code. Thank you for contributing! --> ### Description of the change <!-- Describe the scope of your change - i.e. what the change does. --> This is split out from the investigation done in #6097 and will be followed by a bunch more switching other services and plugins away from the improbable-eng gRPC backend (see #6013) ### Benefits <!-- What benefits will be realized by the code change? --> This PR sets a clear example of switching one of the gRPC servers, the core plugin service, away from the improbable-eng server to the connect gRPC server. The switch of the server is trivial, the difficulty was in getting our kubeapps-apis service working correctly with a combination of improbable and connect gRPC servers. As a side-effect, I updated the liveness and readiness checks to use a grpc request (since we no longer have the gateway endpoint for the core plugin service). Initially I'd tried using the existing server as the default and routing certain routes for transitioned servers to the new handler, but reading http2 headers requires writing the http2 settings frame, which meant the new handler, for some reason that I wasn't able to determine, did not accept the connections. I then switched to default to the new server and proxying to the old improbable for unhandled routes, which worked for all requests that are initiated by the dashboard, but not those initiated from within the kubeapps-apis server itself (like where the GetResources handler makes a gRPC request to the packages plugin for the resource refs). In those cases it failed. I initially assumed this was something to do with the proxying, but I couldn't find the issue at first, so instead transitioned the resources plugin over as well (in #6097), which ended up needing much more changes than I wanted in a PR, including handling the different ways auth creds are passed by the two libraries etc. But even after transitioning the resources plugin, I was still hitting an error for proxied requests (this time when the resources plugin called the packaging plugin). After much more digging, I finally found it was because the [golang httputil.ReverseProxy proxies requests not transports](golang/go#33452). So the gRPC (http2) requests were being proxied using an http1 transport which was resulting in a 404 as the server was not recognising the request (gRPC assumes http2). Once understood, the solution to use a different reverse proxy (just different transport) depending on the request was straight forward. Once I understood that, I was then able to go back to my original intention of a small PR demonstrating changing just a single service, the plugin service, which is what this PR is :) ### Possible drawbacks <!-- Describe any known limitations with your change --> ~~We lose the Gateway ReST-ish http endpoints. The only place we were using the gateway http endponts was in the liveness/readiness checks, which I've switched here too. We do publish the openapi docs for the gateway, but have not made any commitment to supporting it. Some devs have used the gateway with Postman for testing kubeapps-apis locally, but postman also supports gRPC now for a year.~~ Actually, I may even be able to re-use the proxy that I've setup here to keep the gateway available too. I'll check. EDIT: Indeed we can - #6150 ### Applicable issues <!-- Enter any applicable Issues here (You can reference an issue using #) --> - ref #6013 ### Additional information <!-- If there's anything else that's important and relevant to your pull request, mention that information here.--> I'll follow up with a PR for switching the resources gRPC service, then PRs for packaging, repository and plugins etc. --------- Signed-off-by: Michael Nelson <minelson@vmware.com>
absoludity
added a commit
that referenced
this issue
Mar 29, 2023
<!--
Before you open the request please review the following guidelines and
tips to help it be more easily integrated:
- Describe the scope of your change - i.e. what the change does.
- Describe any known limitations with your change.
- Please run any tests or examples that can exercise your modified code.
Thank you for contributing!
-->
### Description of the change
<!-- Describe the scope of your change - i.e. what the change does. -->
Just verifying that we actually don't need to lose the gRPC Gateway
functionality (of the ReST-ish API translation to gRPC).
In the end, all that was required was to update the gateway args so that
it points to the *new* gRPC connect service, and then it just continues
to work (I'd thought it was dependent on a piece that we'd be removing,
but it's not).
Local example check:
```
curl http://localhost:50051/core/plugins/v1alpha1/configured-plugins
{"plugins":[{"name":"helm.packages", "version":"v1alpha1"}, {"name":"resources", "version":"v1alpha1"}]}
```
I assume I'll be able to hook it up similarly when using other plugin
services with auth.
### Benefits
We keep the OpenAPI gateway API :)
### Possible drawbacks
<!-- Describe any known limitations with your change -->
None
### Applicable issues
<!-- Enter any applicable Issues here (You can reference an issue using
#) -->
- Ref #6013
### Additional information
<!-- If there's anything else that's important and relevant to your pull
request, mention that information here.-->
---------
Signed-off-by: Michael Nelson <minelson@vmware.com>
absoludity
added a commit
that referenced
this issue
Mar 29, 2023
<!-- Before you open the request please review the following guidelines and tips to help it be more easily integrated: - Describe the scope of your change - i.e. what the change does. - Describe any known limitations with your change. - Please run any tests or examples that can exercise your modified code. Thank you for contributing! --> ### Description of the change <!-- Describe the scope of your change - i.e. what the change does. --> This PR does some background prep, updating the config getter signature to include the http headers (which the connect gRPC uses). ### Benefits <!-- What benefits will be realized by the code change? --> Following PRs just have changes for actual plugins. ### Possible drawbacks <!-- Describe any known limitations with your change --> ### Applicable issues <!-- Enter any applicable Issues here (You can reference an issue using #) --> - ref #6013 ### Additional information <!-- If there's anything else that's important and relevant to your pull request, mention that information here.--> Signed-off-by: Michael Nelson <minelson@vmware.com>
absoludity
added a commit
that referenced
this issue
Mar 30, 2023
<!-- Before you open the request please review the following guidelines and tips to help it be more easily integrated: - Describe the scope of your change - i.e. what the change does. - Describe any known limitations with your change. - Please run any tests or examples that can exercise your modified code. Thank you for contributing! --> ### Description of the change <!-- Describe the scope of your change - i.e. what the change does. --> Switches the resources plugin (only) to the new connect gRPC. ### Benefits <!-- What benefits will be realized by the code change? --> One down, 4 to go. ### Possible drawbacks <!-- Describe any known limitations with your change --> ### Applicable issues <!-- Enter any applicable Issues here (You can reference an issue using #) --> -ref #6013 ### Additional information <!-- If there's anything else that's important and relevant to your pull request, mention that information here.--> One test, which was dependent on the implementation detail of the previous gRPC server, has been commented out. I want to revisit this before landing, to see whether I can create a similar double for the server, or remove that test. --------- Signed-off-by: Michael Nelson <minelson@vmware.com>
absoludity
added a commit
that referenced
this issue
Apr 11, 2023
<!-- Before you open the request please review the following guidelines and tips to help it be more easily integrated: - Describe the scope of your change - i.e. what the change does. - Describe any known limitations with your change. - Please run any tests or examples that can exercise your modified code. Thank you for contributing! --> ### Description of the change <!-- Describe the scope of your change - i.e. what the change does. --> This PR switches the core packages API to be using the connect gRPC for gRPC and gRPC-Web. The authz token is then set in the context when calling the (currently untransitioned) plugin implementations. ### Benefits <!-- What benefits will be realized by the code change? --> Step 2 of 4 done. ### Possible drawbacks <!-- Describe any known limitations with your change --> ### Applicable issues <!-- Enter any applicable Issues here (You can reference an issue using #) --> - ref #6013 ### Additional information <!-- If there's anything else that's important and relevant to your pull request, mention that information here.--> --------- Signed-off-by: Michael Nelson <minelson@vmware.com>
absoludity
added a commit
that referenced
this issue
Apr 13, 2023
<!-- Before you open the request please review the following guidelines and tips to help it be more easily integrated: - Describe the scope of your change - i.e. what the change does. - Describe any known limitations with your change. - Please run any tests or examples that can exercise your modified code. Thank you for contributing! --> ### Description of the change Updates the helm, kapp-controller and fluxv2 plugins to use the connect gRPC lib. Also updates the repository plugins. <!-- Describe the scope of your change - i.e. what the change does. --> ### Benefits <!-- What benefits will be realized by the code change? --> ### Possible drawbacks <!-- Describe any known limitations with your change --> ### Applicable issues <!-- Enter any applicable Issues here (You can reference an issue using #) --> - ref #6013 ### Additional information <!-- If there's anything else that's important and relevant to your pull request, mention that information here.--> --------- Signed-off-by: Michael Nelson <minelson@vmware.com>
absoludity
added a commit
that referenced
this issue
Apr 14, 2023
<!-- Before you open the request please review the following guidelines and tips to help it be more easily integrated: - Describe the scope of your change - i.e. what the change does. - Describe any known limitations with your change. - Please run any tests or examples that can exercise your modified code. Thank you for contributing! --> ### Description of the change <!-- Describe the scope of your change - i.e. what the change does. --> Removes all traces of improbable-eng's grpc-web handling as well as cmux which is no longer needed (as we're not multiplexing by header values). ### Benefits <!-- What benefits will be realized by the code change? --> Removes the unsupported improbable grpc to grpcWeb dependency! ### Possible drawbacks <!-- Describe any known limitations with your change --> ### Applicable issues <!-- Enter any applicable Issues here (You can reference an issue using #) --> - ref #6013 ### Additional information <!-- If there's anything else that's important and relevant to your pull request, mention that information here.--> --------- Signed-off-by: Michael Nelson <minelson@vmware.com>
absoludity
added a commit
that referenced
this issue
Apr 17, 2023
<!-- Before you open the request please review the following guidelines and tips to help it be more easily integrated: - Describe the scope of your change - i.e. what the change does. - Describe any known limitations with your change. - Please run any tests or examples that can exercise your modified code. Thank you for contributing! --> ### Description of the change <!-- Describe the scope of your change - i.e. what the change does. --> We no longer need to be passing the context down and around for authorization, as headers are used now. This PR isn't complete - I've got a bunch of TODOs that I need to followup. ### Benefits <!-- What benefits will be realized by the code change? --> ### Possible drawbacks <!-- Describe any known limitations with your change --> ### Applicable issues <!-- Enter any applicable Issues here (You can reference an issue using #) --> - ref #6013 ### Additional information <!-- If there's anything else that's important and relevant to your pull request, mention that information here.--> --------- Signed-off-by: Michael Nelson <minelson@vmware.com>
absoludity
added a commit
that referenced
this issue
Apr 18, 2023
… clean-ups. (#6204) ### Description of the change Removes the last redundant pieces of functionality after the switch to connect gRPC-Web. In particular: - Switches the resources plugin to use the connect gRPC client rather than the non-connect one, so that the header auth can be used, which meant - Remove the last use of auth in the go context and supporting function - Remove the `subscriptions` state in the dashboard, which was related to the non-connect gRPC-Web implementation that subscribed to events rather than using async/await. - Remove redundant `closeRequestResources` action. <!-- Describe the scope of your change - i.e. what the change does. --> ### Benefits Completes the work for 6013 ### Possible drawbacks ### Applicable issues - fixes #6013 ### Additional information <!-- If there's anything else that's important and relevant to your pull request, mention that information here.--> --------- Signed-off-by: Michael Nelson <minelson@vmware.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Summary
Improbable stopped supporting grpc-web a while back since the official repo was launched.
We should update to the official grpc-web version. Unfortunately it's not that simple though, since the official grpc-web version has the client-side implementation, but does not have the server-side in-process grpc-Web -> grpc translation (at least for go), instead utilising envoy to do the translation.
We may need to investigate depending on envoy for the translation.
The text was updated successfully, but these errors were encountered: