Skip to content

Commit

Permalink
Allow configuration of gRPC.ClientConn through grpc.DialOption, r…
Browse files Browse the repository at this point in the history
…efactor `Client.Index()`, bump `grpc` dep to 1.64.0 (#35)

We currently do not give users many options for configuring the
underlying `grpc.ClientConn` that's used by the `VectorServiceClient`
for interacting with the data plane. This will be important for allowing
configuration of things like timeouts and proxies, and also for allowing
us to unit test aspects of the data plane layer.

We have an open issue to allow more configuration of our gRPC
implementation: #13

I ended up implementing a few different things here. Ultimately, I feel
like these changes move us more in the right direction for allowing
configuration of our gRPC service for data plane operations.

I decided on leaving things a bit open-ended for the consumer. We give
them the option to pass variadic `grpc.DialOption` arguments when
calling `Client.Index(NewIndexConnParams{...})`. This makes providing
these options optional, and you can continue calling the function
without them as before.

If someone wants to configure the gRPC connection themselves, they have
some latitude to do that via the `Host` and `grpc.DialOption` helper
functions: https://pkg.go.dev/google.golang.org/[email protected]#DialOption.
However, since we're also applying some dial options of our own when
constructing the client, allowing this flexibility may require more care
when merging things in `newIndexConnection`. Specifically looking for
feedback on this if people have opinions. Being able to provide these
directly made it possible to unit test some of the behavior as well.

- Bump grpc version from `1.62.0` -> `1.64.0`. @aulorbe and I noticed
there was an issue with how we were calling `grpc.Dial()` for setting up
the `grpc.ClientConn`. If you passed in an invalid `host`, the `Dial`
operation would hang without error'ing for some period of time. I think
ultimately we should have been using `grpc.DialContext` to configure the
timeout, or using `grpc.WithTimeout()`. When looking at fixing this I
bumped our grpc dependency, and noticed the `Dial` and `DialContext`
methods are under deprecation notice - they want people using
`grpc.NewClient`. It felt like it made sense to go ahead and make this
change now:
  - `grpc.Dial` - https://pkg.go.dev/google.golang.org/[email protected]#Dial
- `grpc.NewClient` -
https://pkg.go.dev/google.golang.org/[email protected]#NewClient
- Refactor `Client` to have only one `Index` method add
`NewIndexConnParams` struct for passing to `Index` rather than discrete
arguments. It already felt like our functions for establishing an
`IndexConnection` were getting a bit bloated, and when I was looking at
adding in gRPC configuration options, it felt like we should go ahead
and update this before v1.0.0.
- Add unit test to validate metadata in outgoing RPC requests, and unit
tests to validate Client applies the appropriate values to
`IndexConnection` through `Client.Index`, `along with
Client.extractAuthHeader()`.
- Fix formatting in doc comment code blocks in both `client.go` and
`index_connection.go`.
- Bump `google.golang.org/grpc` to v1.65.0 to correct snyk security
vulnerability:
https://security.snyk.io/vuln/SNYK-GOLANG-GOOGLEGOLANGORGGRPCMETADATA-7430177

- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [X] Breaking change (fix or feature that would cause existing
functionality to not work as expected)
- [ ] This change requires a documentation update
- [ ] Infrastructure change (CI configs, etc)
- [ ] Non-code change (docs, etc)
- [ ] None of the above: (explain here)

Make sure CI for unit + integration tests is passing.

I've added unit tests to specifically target gRPC metadata, testing
passing a `grpc.DialOption`, and creating an `IndexConnection` via
`Client.Index()`.
  • Loading branch information
austin-denoble authored and aulorbe committed Jul 11, 2024
1 parent 5b77ef9 commit ec1d912
Showing 0 changed files with 0 additions and 0 deletions.

0 comments on commit ec1d912

Please sign in to comment.