Configure apiKey, additionalHeaders, and host through environment variables
#22
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,3 @@ | ||
| PINECONE_API_KEY="<Project API Key>" | ||
| TEST_POD_INDEX_NAME="<Pod based Index name>" | ||
| TEST_SERVERLESS_INDEX_NAME="<Serverless based Index name>" |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,7 +5,10 @@ import ( | |
| "encoding/json" | ||
| "fmt" | ||
| "io" | ||
| "log" | ||
| "net/http" | ||
| "net/url" | ||
| "os" | ||
| "strings" | ||
|
|
||
| "github.com/deepmap/oapi-codegen/v2/pkg/securityprovider" | ||
|
|
@@ -16,25 +19,34 @@ import ( | |
|
|
||
| type Client struct { | ||
| apiKey string | ||
|
There was a problem hiding this comment. Is apiKey still needed in the There was a problem hiding this comment. Removed from |
||
| headers map[string]string | ||
| restClient *control.Client | ||
| sourceTag string | ||
| } | ||
|
|
||
| type NewClientParams struct { | ||
| ApiKey string // required unless Authorization header provided | ||
| Headers map[string]string // optional | ||
| Host string // optional | ||
| RestClient *http.Client // optional | ||
| SourceTag string // optional | ||
| } | ||
|
|
||
| func NewClient(in NewClientParams) (*Client, error) { | ||
|
There was a problem hiding this comment. I'd expect this func to just call There was a problem hiding this comment. It now ultimately calls |
||
| clientOptions, err := in.buildClientOptions() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| controlHostOverride := valueOrFallback(in.Host, os.Getenv("PINECONE_CONTROLLER_HOST")) | ||
|
There was a problem hiding this comment. "CONTROL" vs "CONTROLLER"? (Assuming this is talking about the control plane? There was a problem hiding this comment. I went with
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? There was a problem hiding this comment. Nice job throwing this in for consistency with other clients. |
||
| if controlHostOverride != "" { | ||
| controlHostOverride, err = ensureHTTP(controlHostOverride) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } | ||
|
|
||
| client, err := control.NewClient(valueOrFallback(controlHostOverride, "https://api.pinecone.io"), clientOptions...) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
@@ -421,26 +433,52 @@ func derefOrDefault[T any](ptr *T, defaultValue T) T { | |
| return *ptr | ||
| } | ||
|
|
||
| func (ncp *NewClientParams) buildClientOptions() ([]control.ClientOption, error) { | ||
|
There was a problem hiding this comment. could buildClientOptions to be a wrapper around buildClientBaseOptions? There was a problem hiding this comment. I slimmed things down to just |
||
| clientOptions := []control.ClientOption{} | ||
| hasAuthorizationHeader := false | ||
| osApiKey := os.Getenv("PINECONE_API_KEY") | ||
| hasApiKey := (ncp.ApiKey != "" || osApiKey != "") | ||
| envAdditionalHeaders, hasEnvAdditionalHeaders := os.LookupEnv("PINECONE_ADDITIONAL_HEADERS") | ||
|
|
||
| // build and apply user agent | ||
| userAgentProvider := provider.NewHeaderProvider("User-Agent", useragent.BuildUserAgent(ncp.SourceTag)) | ||
| clientOptions = append(clientOptions, control.WithRequestEditorFn(userAgentProvider.Intercept)) | ||
|
|
||
| // apply headers from parameters if passed, otherwise use environment headers | ||
| if ncp.Headers != nil { | ||
| for key, value := range ncp.Headers { | ||
| headerProvider := provider.NewHeaderProvider(key, value) | ||
|
|
||
| if strings.Contains(strings.ToLower(key), "authorization") { | ||
| hasAuthorizationHeader = true | ||
| } | ||
|
|
||
| clientOptions = append(clientOptions, control.WithRequestEditorFn(headerProvider.Intercept)) | ||
| } | ||
| } else if hasEnvAdditionalHeaders { | ||
| additionalHeaders := make(map[string]string) | ||
| err := json.Unmarshal([]byte(envAdditionalHeaders), &additionalHeaders) | ||
| if err != nil { | ||
| log.Printf("failed to parse PINECONE_ADDITIONAL_HEADERS: %v", err) | ||
| } else { | ||
| for header, value := range additionalHeaders { | ||
| headerProvider := provider.NewHeaderProvider(header, value) | ||
|
|
||
| if strings.Contains(strings.ToLower(header), "authorization") { | ||
| hasAuthorizationHeader = true | ||
| } | ||
|
|
||
| clientOptions = append(clientOptions, control.WithRequestEditorFn(headerProvider.Intercept)) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // if apiKey is provided and no auth header is set, add the apiKey as a header | ||
| // apiKey from parameters takes precedence over apiKey from environment | ||
| if hasApiKey && !hasAuthorizationHeader { | ||
| appliedApiKey := valueOrFallback(ncp.ApiKey, osApiKey) | ||
|
|
||
| apiKeyProvider, err := securityprovider.NewSecurityProviderApiKey("header", "Api-Key", appliedApiKey) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
@@ -451,9 +489,30 @@ func buildClientOptions(in NewClientParams) ([]control.ClientOption, error) { | |
| return nil, fmt.Errorf("no API key provided, please pass an API key for authorization") | ||
| } | ||
|
|
||
| if ncp.RestClient != nil { | ||
| clientOptions = append(clientOptions, control.WithHTTPClient(ncp.RestClient)) | ||
| } | ||
|
|
||
| return clientOptions, nil | ||
| } | ||
|
|
||
| func ensureHTTP(inputURL string) (string, error) { | ||
| parsedURL, err := url.Parse(inputURL) | ||
| if err != nil { | ||
| return "", fmt.Errorf("invalid URL: %v", err) | ||
| } | ||
|
|
||
| if parsedURL.Scheme == "" { | ||
| return "https://" + inputURL, nil | ||
| } | ||
| return inputURL, nil | ||
| } | ||
|
|
||
| func valueOrFallback[T comparable](value, fallback T) T { | ||
| var zero T | ||
| if value != zero { | ||
| return value | ||
| } else { | ||
| return fallback | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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..."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. 🤔