Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support muxing gRPC broker connections over a single net.Conn #288

Merged
merged 7 commits into from
Nov 13, 2023

Conversation

tomhjp
Copy link
Contributor

@tomhjp tomhjp commented Oct 16, 2023

Implements multiplexing for the gRPC broker so that additional gRPC connections are multiplexed onto the plugin's initial listener instead of creating a new Unix or TCP listener for each gRPC stream ID. The primary motivation is the simplification this gives containerised plugins which will only have to negotiate creating a single Unix socket that can be used by both sides, instead of one on each side - in particular this will greatly ease supporting rootless container engines running containers with a non-root user.

This will require both sides of the connection to update their go-plugin version, and as such is strictly opt-in via client config.

The implementation uses the same yamux.Session-based multiplexing that netrpc uses. However, given that we don't have control over when the gRPC library might redial a new connection, we introduce the concept of knocking (essentially a handshake, somewhat similar to TCP's SYN-ACK handshake) to synchronise the client and server on establishing a connection for a pre-agreed stream ID.

I briefly explored implementing gRPC's grpc.ClientConnInterface instead to give us full control over when a new connection is established, which would get rid of the need for knocking/pre-agreeing an ID, but I got put off by having to implement grpc.CallOptions, and also having to change the library's API, because GRPCBroker.Dial currently returns a concrete *grpc.ClientConn struct. However, I'm still open to exploring that further if people think those problems are preferable to the ones this PR currently solves.

Copy link
Contributor

@fairclothjm fairclothjm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I pulled this down and tested with a non-containerized plugin

Copy link
Contributor

@swenson swenson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, a few minor suggestions

client.go Outdated Show resolved Hide resolved
@@ -0,0 +1,48 @@
package grpcmux
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amusingly enough, this package doesn't seem to reference or use GRPC. :)

server.go Outdated Show resolved Hide resolved
server.go Outdated Show resolved Hide resolved
Copy link
Contributor

@swenson swenson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@tomhjp
Copy link
Contributor Author

tomhjp commented Nov 13, 2023

Thanks!

@tomhjp tomhjp merged commit d16cec3 into main Nov 13, 2023
@tomhjp tomhjp deleted the mux-grpc-broker-connections branch November 13, 2023 15:08
@tomhjp tomhjp mentioned this pull request Nov 13, 2023
wata727 added a commit to terraform-linters/tflint-plugin-sdk that referenced this pull request Feb 24, 2024
wata727 added a commit to terraform-linters/tflint-plugin-sdk that referenced this pull request Feb 24, 2024
* Bump github.com/hashicorp/go-plugin from 1.4.10 to 1.6.0

Bumps [github.com/hashicorp/go-plugin](https://github.com/hashicorp/go-plugin) from 1.4.10 to 1.6.0.
- [Release notes](https://github.com/hashicorp/go-plugin/releases)
- [Changelog](https://github.com/hashicorp/go-plugin/blob/main/CHANGELOG.md)
- [Commits](hashicorp/go-plugin@v1.4.10...v1.6.0)

---
updated-dependencies:
- dependency-name: github.com/hashicorp/go-plugin
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

* Follow up of hashicorp/go-plugin#288

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Kazuma Watanabe <[email protected]>
hanzei pushed a commit to hanzei/go-plugin that referenced this pull request Oct 17, 2024
…orp#288)

* Support muxing gRPC broker connections over a single net.Conn, via ClientConfig.GRPCBrokerMultiplex
* upgrade yamux, fix yamux config, and go mod tidy -compat=1.17
* Check for multiplexing support in protocol negotiation if enabled
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants