Optional configs for setting custom headers / metadata for REST / gRPC operations #18

austin-denoble · 2024-04-30T16:37:47Z

Problem

Currently, the Go SDK does not support specifying custom headers or metadata for control & data plane operations (REST and gRPC). This is a useful feature to aid in debugging or tracking a specific request, and we'd like to enable this functionality in the Go SDK.

Solution

Update NewClientParams and Client structs to support headers, also allow passing a custom RestClient to the new client. This is primarily for allowing mocking in unit tests, but we have a similar approach in other clients allowing users to customize the HTTP module.
Add new buildClientOptions function.
Update NewClient to support appending Headers and RestClient to clientOptions.
Add error message for empty ApiKey without any Authorization header provided. If the user passes both, avoid applying the Api-Key header in favor of the Authorization header.
Add new Client.IndexWithAdditionalMetadata constructor.
Update IndexConnection struct to include additionalMetadata, and update newIndexConnection to accept additionalMetadata as an argument.
Update IndexConnection.akCtx to handle appending the api-key and any additionalMetadata to requests.
Update unit tests, add new mocks package and mock_transport to facilitate validating requests made via Client including.

I spent some time trying to figure out how to mock / test IndexConnection but I think it'll be a bit trickier than Client, so somewhat saving for a future PR atm.

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
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)

Test Plan

just test to run the test suite locally. Validate tests pass in CI.

haruska

Good addition overall. Might want to consider some breaking changes pre-1.0 to clean up some of the options in the constructors.

pinecone/client.go

haruska · 2024-04-30T16:54:47Z

pinecone/client.go

+}
+
+func (c *Client) IndexWithAdditionalMetadata(host string, namespace string, additionalMetadata map[string]string) (*IndexConnection, error) {
+	idx, err := newIndexConnection(c.apiKey, host, namespace, c.sourceTag, additionalMetadata)


The newIndexConnection input is getting a bit long. Consider refactoring this into a breaking change where it takes a struct similar to other constructors.

Agreed, will go ahead and take a pass on that now.

See newIndexParameters struct type which newIndexConnection should consume.

haruska · 2024-04-30T17:00:46Z

pinecone/client.go

 	apiKeyProvider, err := securityprovider.NewSecurityProviderApiKey("header", "Api-Key", in.ApiKey)
 	if err != nil {
 		return nil, err
 	}
-
+	clientOptions = append(clientOptions, control.WithRequestEditorFn(apiKeyProvider.Intercept))
+


Is an API Key now optional with the addition of custom headers? Still seems required here.

If a caller was attempting to use a JWT Bearer token instead of an API Key for auth, would this now work? Seems I'd need to pass in an API Key (which gets added to the headers) and a custom header with the Bearer token. Also guessing that breaks the request (two auth schemes in the same request headers)

Good point, I didn't change anything regarding API key being optional vs. required so I think it still expects it to be populated.

The point about JWT Bearer is intersting, I'd maybe need to defer to @jhamon regarding specifically what we need to support here, but maybe I should look at removing the API key if they've passed a header for auth, for example. 🤔

aulorbe · 2024-04-30T17:09:02Z

pinecone/client_test.go

@@ -66,8 +68,11 @@ func (ts *ClientTests) TestNewClientParamsSet() {
 	if client.sourceTag != "" {
 		ts.FailNow(fmt.Sprintf("Expected client to have empty sourceTag, but got '%s'", client.sourceTag))
 	}
+	if client.headers != nil {
+		ts.FailNow(fmt.Sprintf("Expected client to have nil headers, but got '%v'", client.headers))


Total nit, but when I read this, I ask myself "what are nil headers?" because nil sounds like a quantity. Maybe rephrase this stmt to be "Expected client headers to be nil".

(B/c at least if I'm understanding stuff about Go correctly online, nil isn't really a quantity, it's sorta just a pointer that indicates there's no object or or no value/concrete type, right?)

aulorbe · 2024-04-30T17:09:59Z

pinecone/client_test.go

+		ts.FailNow(err.Error())
+	}
+	if client.apiKey != apiKey {
+		ts.FailNow(fmt.Sprintf("Expected client to have apiKey '%s', but got '%s'", apiKey, client.apiKey))


Is this potentially dangerous b/c someone could reveal an API key through this error msg?

(I mean, I know it's a mocked API key in the test, but if this function outputs this error msg IRL, that could be a dangerous vulnerability for users, no?)

This is test code and thus not compiled into the actual built package, as far as I understand. Given that and the usage of a fake key in the tests where we're doing this, I feel like this is probably safe. Users would need to be developing the client themselves locally or in CI for it to be an issue I think.

aulorbe · 2024-04-30T17:11:23Z

pinecone/index_connection_test.go

@@ -93,19 +106,47 @@ func (ts *IndexConnectionTests) TestNewIndexConnection() {
 	if idxConn.Namespace != "" {
 		ts.FailNow(fmt.Sprintf("Expected idxConn to have empty namespace, but got '%s'", idxConn.Namespace))
 	}
+	if idxConn.additionalMetadata != nil {
+		ts.FailNow(fmt.Sprintf("Expected idxConn to have nil additionalMetadata, but got '%+v'", idxConn.additionalMetadata))


Same comment re: the usage of nil here, as w/my previous comment

aulorbe

Just lurking :) Giving an approval so I don't block!

…ta plane requests, update tests, add verbose to just test, update NewClientParams to take in a custom rest client to allow for mocking in unit tests

…ection

…f both Api-Key and Authorization header have been provided, add unit tests

austin-denoble · 2024-05-03T21:51:06Z

justfile

@@ -3,7 +3,7 @@ test:
  set -o allexport
  source .env
  set +o allexport
-  go test -count=1 ./pinecone
+  go test -count=1 -v ./pinecone


I like the additional logging output when running these locally, etc.

ssmith-pc · 2024-05-06T19:03:11Z

pinecone/client.go

+	for key, value := range in.Headers {
+		headerProvider := provider.NewHeaderProvider(key, value)
+
+		if strings.Contains(key, "Authorization") {
+			hasAuthorizationHeader = true
+		}
+
+		clientOptions = append(clientOptions, control.WithRequestEditorFn(headerProvider.Intercept))
+	}


Are there any headers we'd want to prevent them from setting? I'm not sure there, just asking the question.

At the moment I don't think there are, good question though I'm also not sure.

ssmith-pc · 2024-05-06T19:09:05Z

pinecone/index_connection.go

+type newIndexParameters struct {
+	apiKey string
+	host string
+	namespace string
+	sourceTag string
+	additionalMetadata map[string]string
+}
+
+func newIndexConnection(in newIndexParameters) (*IndexConnection, error) {


Nice, I like this

Props to @haruska for the suggestion.

ssmith-pc · 2024-05-06T19:09:56Z

pinecone/models.go

-type Metadata = structpb.Struct
+type Metadata = structpb.Struct


accidental change?

I think it was just auto-formatting removing the line at the end of the file.

ssmith-pc · 2024-05-06T19:10:58Z

pinecone/client.go

 }

 type NewClientParams struct {
-	ApiKey    string
-	SourceTag string // optional
+	ApiKey     string            // optional unless no Authorization header provided


Suggest rephrasing this to "required unless Authorization header provided"

ssmith-pc · 2024-05-06T19:12:18Z

pinecone/client.go

+		if strings.Contains(key, "Authorization") {
+			hasAuthorizationHeader = true
+		}


http headers are typically case in-sensitive so we probably want to relax this check

Updated to use strings.ToLower(key), "authorization"), thanks!

… comment

gitguardian · 2024-05-06T20:05:40Z

️✅ There are no secrets present in this pull request anymore.

If these secrets were true positive and are still valid, we highly recommend you to revoke them.
Once a secret has been leaked into a git repository, you should consider it compromised, even if it was deleted immediately.
Find here more information about risks.

^{_{🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.}}

haruska

Good improvements. The creation of the index connection still seems a bit clunky but I think that's just an artifact of the public constructor(s) being off Client. Overall, the PR is much better than the current impl in main.

austin-denoble requested review from haruska, aulorbe and ssmith-pc April 30, 2024 16:37

austin-denoble changed the title ~~Optional configs to custom headers & metadata for REST & gRPC operations~~ Optional configs for setting custom headers / metadata for REST / gRPC operations Apr 30, 2024

haruska reviewed Apr 30, 2024

View reviewed changes

aulorbe reviewed Apr 30, 2024

View reviewed changes

aulorbe approved these changes Apr 30, 2024

View reviewed changes

austin-denoble added 5 commits May 1, 2024 17:30

add ability to specify custom headers and metadata for control and da…

fcfac96

…ta plane requests, update tests, add verbose to just test, update NewClientParams to take in a custom rest client to allow for mocking in unit tests

remove unneeded variable declaration

b311770

remove print statement

233a473

tweak testing strings

395be59

add newIndexParameters struct and swap to using that for newIndexConn…

e5753fb

…ection

austin-denoble force-pushed the adenoble/custom-request-headers branch from 8293e2b to 505a4ff Compare May 3, 2024 21:28

add new buildClientOptions function, do not pass the Api-Key header i…

59a6918

…f both Api-Key and Authorization header have been provided, add unit tests

austin-denoble force-pushed the adenoble/custom-request-headers branch from 67b7090 to 59a6918 Compare May 3, 2024 21:48

pinecone-io deleted a comment from gitguardian bot May 3, 2024

austin-denoble commented May 3, 2024

View reviewed changes

ssmith-pc reviewed May 6, 2024

View reviewed changes

review feedback, relax check on authorization string, update required…

89fc762

… comment

haruska approved these changes May 6, 2024

View reviewed changes

ssmith-pc approved these changes May 6, 2024

View reviewed changes

austin-denoble merged commit 5a9897e into main May 6, 2024
3 checks passed

austin-denoble deleted the adenoble/custom-request-headers branch May 6, 2024 20:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optional configs for setting custom headers / metadata for REST / gRPC operations #18

Optional configs for setting custom headers / metadata for REST / gRPC operations #18

austin-denoble commented Apr 30, 2024 •

edited

Loading

haruska left a comment

haruska Apr 30, 2024

austin-denoble Apr 30, 2024

austin-denoble May 6, 2024

haruska Apr 30, 2024

austin-denoble Apr 30, 2024

aulorbe Apr 30, 2024 •

edited

Loading

aulorbe Apr 30, 2024

aulorbe Apr 30, 2024

aulorbe Apr 30, 2024

austin-denoble Apr 30, 2024 •

edited

Loading

aulorbe Apr 30, 2024

aulorbe left a comment

austin-denoble May 3, 2024

ssmith-pc May 6, 2024

austin-denoble May 6, 2024

ssmith-pc May 6, 2024

austin-denoble May 6, 2024

ssmith-pc May 6, 2024

austin-denoble May 6, 2024 •

edited

Loading

ssmith-pc May 6, 2024

ssmith-pc May 6, 2024

austin-denoble May 6, 2024

gitguardian bot commented May 6, 2024

haruska left a comment

		type Metadata = structpb.Struct
		type Metadata = structpb.Struct

Optional configs for setting custom headers / metadata for REST / gRPC operations #18

Optional configs for setting custom headers / metadata for REST / gRPC operations #18

Conversation

austin-denoble commented Apr 30, 2024 • edited Loading

Problem

Solution

Type of Change

Test Plan

haruska left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aulorbe Apr 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austin-denoble Apr 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aulorbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austin-denoble May 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gitguardian bot commented May 6, 2024

️✅ There are no secrets present in this pull request anymore.

haruska left a comment

Choose a reason for hiding this comment

austin-denoble commented Apr 30, 2024 •

edited

Loading

aulorbe Apr 30, 2024 •

edited

Loading

austin-denoble Apr 30, 2024 •

edited

Loading

austin-denoble May 6, 2024 •

edited

Loading