Configure `apiKey`, `additionalHeaders`, and `host` through environment variables #22

austin-denoble · 2024-05-08T23:01:36Z

Problem

There was a request to add additional configuration validation to the Go client as you could make requests with an empty apiKey. An empty check and early error return were added in a previous PR when passing apiKey directly: https://github.com/pinecone-io/go-pinecone/pull/18/files#diff-37c3a14781b4b6e74bcc0cfc8b2462ee2ae243a720a42669256248a3cb005f01R442-R444

We're also lacking the ability to specify an api key or headers through environment variables like the Python client, which can be a nice UX tweak for options in how you configure the client. Additionally, there's no way to specify a control plane host override, which is useful in situations where we need to hit a staging URL.

Solution

Refactor buildClientOptions() into a method on the NewClientParams struct rather than a function which receives the struct.
Add ability to specify PINECONE_API_KEY, PINECONE_ADDITIONAL_HEADERS, and PINECONE_CONTROLLER_HOST through environment variables. I kept the naming aligned with Python for now.
- For these values, if the corresponding field has been passed through NewClientParams directly, the passed value will override anything set in the environment.
Add unit tests to validate override behavior, and headers being added as expected.

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

CI and new unit tests.

To see the specific tasks where the Asana app for GitHub is being used, see below:
- https://app.asana.com/0/0/1207208972408325
- https://app.asana.com/0/0/1207208968992757

…ent variables, add logic for using passed params over environment variables where applicable, add new unit tests

…ough NewClientParams or environment variables, add tests

haruska

Good improvement. It might be more clear to have multiple constructors vs ApiKey xor an Authorization header as a part of a headers array.

Technically, the ApiKey option is also just setting an Authorization header.

haruska · 2024-05-09T15:19:56Z

pinecone/client.go

Could we have two different constructors? One that requires an ApiKey and the other that requires a different auth mechanism (bearer token? oauth token?). It feels clunky that the "ApiKey is required unless..."

That does seem reasonable. I'm less clear on what this would look like though since we don't officially offer alternative methods of authentication quite yet. The other clients require an API key for instance.

If the user is able to supply headers directly then they could technically always pass their own auth headers. I think this would also be an issue on Python and TypeScript since they don't do any checking of the headers that are provided. You could also consider this a user-error that they'd need to resolve themselves.

I'm a bit unsure of what would make the most sense here to be honest. 🤔

austin-denoble · 2024-05-10T16:44:59Z

Good improvement. It might be more clear to have multiple constructors vs ApiKey xor an Authorization header as a part of a headers array.
Technically, the ApiKey option is also just setting an Authorization header.

True, responded here: #22 (comment)

haruska · 2024-05-14T17:18:44Z

.github/workflows/ci.yaml

This doesn't seem to match up with .env.example above

I split things out a bit and there are 2 environment keys for setting the API key:

PINECONE_API_KEY

INTEGRATION_PINECONE_API_KEY

Let me know if that's too confusing. Trying to test properly while one env value was used in both situations (the client vs. the tests) felt trickier than just having explicit values for it.

To keep the "test" key usage more clear, I updated the name to TEST_PINECONE_API_KEY.

.env.example should also be updated:

PINECONE_API_KEY="<Project API Key>" TEST_PINECONE_API_KEY="<Test Project API Key>" TEST_POD_INDEX_NAME="<Pod based Index name>" TEST_SERVERLESS_INDEX_NAME="<Serverless based Index name>"

haruska · 2024-05-14T17:20:04Z

pinecone/client.go

 	if err != nil {
 		return nil, err
 	}

-	client, err := control.NewClient("https://api.pinecone.io", clientOptions...)
+	controlHostOverride := valueOrFallback(in.Host, os.Getenv("PINECONE_CONTROLLER_HOST"))


"CONTROL" vs "CONTROLLER"? (Assuming this is talking about the control plane?

I went with PINECONE_CONTROLLER_HOST because that's the env key that's used in the Python and TypeScript clients for the same config value:

https://github.com/pinecone-io/pinecone-python-client/blob/1fad23f00c114aa8ef33242b91b969da2825c360/pinecone/config/pinecone_config.py#L15

https://github.com/pinecone-io/pinecone-ts-client/blob/00d7c36bb34867a933170312070a91306f94c058/src/pinecone.ts#L165

I suppose it is a bit confusing given it's applied as an override to the control plane. I think staying consistent makes sense, although the variable name here is confusing. What do you think?

haruska · 2024-05-14T17:20:54Z

pinecone/client.go

@@ -16,25 +18,41 @@ import (

 type Client struct {
 	apiKey     string


Is apiKey still needed in the Client struct?

Removed from Client as well as IndexConnection.

haruska · 2024-05-14T17:22:03Z

pinecone/client.go

+	Headers    map[string]string
+	Host       string
+	RestClient *http.Client
+	SourceTag  string
 }

 func NewClient(in NewClientParams) (*Client, error) {


I'd expect this func to just call NewClientBase and configure the ApiKey as an auth header

It now ultimately calls NewClientBase and passes the ApiKey through as part of the Headers map.

haruska · 2024-05-14T17:23:18Z

pinecone/client.go

@@ -421,39 +460,82 @@ func derefOrDefault[T any](ptr *T, defaultValue T) T {
 	return *ptr
 }

-func buildClientOptions(in NewClientParams) ([]control.ClientOption, error) {
+func (ncp *NewClientParams) buildClientOptions() ([]control.ClientOption, error) {


could buildClientOptions to be a wrapper around buildClientBaseOptions?

I slimmed things down to just buildClientBaseOptions, lemme know if that makes sense.

haruska · 2024-05-14T17:23:52Z

pinecone/client_test.go

+	os.Unsetenv("PINECONE_ADDITIONAL_HEADERS")
+}
+
+// func (ts *ClientTests) TestAuthorizationHeaderOverridesApiKey() {


meant to comment out?

Meant to remove entirely, the PR wasn't really in a finished state. 😅 The client no longer does any explicit juggling of ApiKey and any auth header you may have passed.

…unc and add NewClientBase, update tests to support the new interface for creating clients, remove apiKey from Client and IndexConnection, handle turning apiKey into a header early, and allowing for complete control over headers without using apiKey

…iated env values

haruska

Minor suggestions for your consideration. Good work!

haruska · 2024-05-15T14:34:48Z

.env.example

@@ -1,3 +1,4 @@
-API_KEY="<Project API Key>"
+PINECONE_API_KEY="<Project API Key>"
+TEST_PINECONE_API_KEY="<Test Project API Key>"


Good point. 😄

haruska · 2024-05-15T14:38:30Z

pinecone/client.go

-	if err != nil {
-		return nil, err
+	osApiKey := os.Getenv("PINECONE_API_KEY")
+	hasApiKey := (in.ApiKey != "" || osApiKey != "")


This functionality (of having an environment variable) instead of requiring the in.ApiKey seems unclear. On L26 it says that the ApiKey is required. Here, it seems it's optional if I set an environment variable.

True, I updated the comment to call that out, and also the error that's returned if you don't pass a key. I think adding docstrings we can make this a bit more clear and explicit.

haruska · 2024-05-15T14:39:17Z

pinecone/client.go

+		return nil, fmt.Errorf("no API key provided, please pass an API key for authorization")
+	}
+
+	apiKeyHeader := struct{ Key, Value string }{"Api-Key", valueOrFallback(in.ApiKey, osApiKey)}


Seems that valueOrFallback could be utilized for the "hasApiKey" check?

haruska · 2024-05-15T14:46:19Z

pinecone/client.go

-func minOne(x int32) int32 {
-	if x < 1 {
-		return 1
+func ensureHTTPS(inputURL string) (string, error) {


can we add the option to disable https? (i'm thinking for local dev)

The way it's written currently means if you provide a Host or PINECONE_CONTROLLER_HOST, you're able to use whatever scheme as long as url.Parse() can read it out. https is only applied if there's no scheme, do you think that's sufficient?

For example, you should be able to pass http://localhost:9999 properly without "https" being added.

yup that works. The function name is a bit confusing...

jhamon · 2024-05-15T15:08:41Z

pinecone/client.go

+	clientOptions := buildClientBaseOptions(in)
+	var err error
+
+	controlHostOverride := valueOrFallback(in.Host, os.Getenv("PINECONE_CONTROLLER_HOST"))


Nice job throwing this in for consistency with other clients.

… env value, remove unnecessary test

add ability to provide apiKey and additional headers through environm…

8eac891

…ent variables, add logic for using passed params over environment variables where applicable, add new unit tests

austin-denoble requested review from haruska and aulorbe May 8, 2024 23:01

austin-denoble added 3 commits May 8, 2024 19:02

remove printf statements()

8446df9

update CI workflow env variable

995829d

add ability to specify host override for control plane operations thr…

6eb91a3

…ough NewClientParams or environment variables, add tests

austin-denoble changed the title ~~Configure API Key and headers through environment variables~~ Configure apiKey, additionalHeaders, and host through environment variables May 9, 2024

haruska approved these changes May 9, 2024

View reviewed changes

refactor into NewClientParams and NewClientBaseParams

9f56ded

haruska reviewed May 14, 2024

View reviewed changes

austin-denoble added 4 commits May 14, 2024 19:57

add comments back to NewClientParams struct

fc13ea1

update INTEGREATION_PINECONE_API_KEY to TEST_ to match with the assoc…

db00a6d

…iated env values

update client_test

99e6972

haruska approved these changes May 15, 2024

View reviewed changes

jhamon reviewed May 15, 2024

View reviewed changes

jhamon approved these changes May 15, 2024

View reviewed changes

austin-denoble added 2 commits May 16, 2024 02:57

more review feedback: rename URL scheme checker, remove extra API key…

7dea994

… env value, remove unnecessary test

move buildClientBaseOptions back down

2e394d8

austin-denoble merged commit c7eda0a into main May 16, 2024
3 checks passed

austin-denoble deleted the adenoble/configuration-validation branch May 16, 2024 07:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configure `apiKey`, `additionalHeaders`, and `host` through environment variables #22

Configure `apiKey`, `additionalHeaders`, and `host` through environment variables #22

austin-denoble commented May 8, 2024 •

edited

Loading

haruska left a comment

haruska May 9, 2024

austin-denoble May 10, 2024

austin-denoble commented May 10, 2024

haruska May 14, 2024

austin-denoble May 15, 2024

austin-denoble May 15, 2024 •

edited

Loading

haruska May 14, 2024

austin-denoble May 15, 2024 •

edited

Loading

haruska May 14, 2024

austin-denoble May 15, 2024

haruska May 14, 2024

austin-denoble May 15, 2024

haruska May 14, 2024

austin-denoble May 15, 2024

haruska May 14, 2024

austin-denoble May 15, 2024

haruska left a comment

haruska May 15, 2024

austin-denoble May 16, 2024

haruska May 15, 2024

austin-denoble May 16, 2024

haruska May 15, 2024

haruska May 15, 2024

austin-denoble May 15, 2024

haruska May 15, 2024

jhamon May 15, 2024

		@@ -16,25 +18,41 @@ import (

		type Client struct {
		apiKey string

Configure apiKey, additionalHeaders, and host through environment variables #22

Configure apiKey, additionalHeaders, and host through environment variables #22

Conversation

austin-denoble commented May 8, 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

austin-denoble commented May 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austin-denoble May 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austin-denoble May 15, 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

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

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

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

Configure `apiKey`, `additionalHeaders`, and `host` through environment variables #22

Configure `apiKey`, `additionalHeaders`, and `host` through environment variables #22

austin-denoble commented May 8, 2024 •

edited

Loading

austin-denoble May 15, 2024 •

edited

Loading

austin-denoble May 15, 2024 •

edited

Loading