From ec1d91282f5e1731c71929e720a34ba6120a3bbc Mon Sep 17 00:00:00 2001 From: Austin DeNoble Date: Wed, 10 Jul 2024 02:43:21 -0400 Subject: [PATCH] Allow configuration of `gRPC.ClientConn` through `grpc.DialOption`, refactor `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: https://github.com/pinecone-io/go-pinecone/issues/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/grpc@v1.64.0#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/grpc@v1.64.0#Dial - `grpc.NewClient` - https://pkg.go.dev/google.golang.org/grpc@v1.64.0#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()`.