-
Notifications
You must be signed in to change notification settings - Fork 84
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Fix] Configuring SSL proxy via openapi_config object #321
Changes from all commits
6633a58
8e5effb
dfa9f5e
dbefd52
72615e6
7b47e8e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
from typing import NamedTuple, Optional, Dict | ||
import os | ||
import copy | ||
|
||
from pinecone.exceptions import PineconeConfigurationError | ||
from pinecone.config.openapi import OpenApiConfigFactory | ||
|
@@ -46,10 +47,11 @@ def build( | |
if not host: | ||
raise PineconeConfigurationError("You haven't specified a host.") | ||
|
||
openapi_config = ( | ||
openapi_config | ||
or kwargs.pop("openapi_config", None) | ||
or OpenApiConfigFactory.build(api_key=api_key, host=host) | ||
) | ||
if openapi_config: | ||
openapi_config = copy.deepcopy(openapi_config) | ||
openapi_config.host = host | ||
openapi_config.api_key = {"ApiKeyAuth": api_key} | ||
Comment on lines
+52
to
+53
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When the user provides this object with some configuration in it, we want to merge the api key and host settings into it rather than building a fresh object. |
||
else: | ||
openapi_config = OpenApiConfigFactory.build(api_key=api_key, host=host) | ||
|
||
return Config(api_key, host, openapi_config, additional_headers) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,8 +6,7 @@ | |
from pinecone.config import PineconeConfig, Config | ||
|
||
from pinecone.core.client.api.manage_indexes_api import ManageIndexesApi | ||
from pinecone.core.client.api_client import ApiClient | ||
from pinecone.utils import get_user_agent, normalize_host | ||
from pinecone.utils import normalize_host, setup_openapi_client | ||
from pinecone.core.client.models import ( | ||
CreateCollectionRequest, | ||
CreateIndexRequest, | ||
|
@@ -85,25 +84,20 @@ def __init__( | |
or share with Pinecone support. **Be very careful with this option, as it will print out | ||
your API key** which forms part of a required authentication header. Default: `false` | ||
""" | ||
if config or kwargs.get("config"): | ||
configKwarg = config or kwargs.get("config") | ||
if not isinstance(configKwarg, Config): | ||
if config: | ||
if not isinstance(config, Config): | ||
raise TypeError("config must be of type pinecone.config.Config") | ||
else: | ||
self.config = configKwarg | ||
Comment on lines
-88
to
-93
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was more cleanup since |
||
self.config = config | ||
else: | ||
self.config = PineconeConfig.build(api_key=api_key, host=host, additional_headers=additional_headers, **kwargs) | ||
|
||
self.pool_threads = pool_threads | ||
|
||
if index_api: | ||
self.index_api = index_api | ||
else: | ||
api_client = ApiClient(configuration=self.config.openapi_config, pool_threads=self.pool_threads) | ||
api_client.user_agent = get_user_agent() | ||
extra_headers = self.config.additional_headers or {} | ||
for key, value in extra_headers.items(): | ||
api_client.set_default_header(key, value) | ||
self.index_api = ManageIndexesApi(api_client) | ||
self.index_api = setup_openapi_client(ManageIndexesApi, self.config, pool_threads) | ||
|
||
self.index_host_store = IndexHostStore() | ||
""" @private """ | ||
|
@@ -521,12 +515,20 @@ def Index(self, name: str = '', host: str = '', **kwargs): | |
raise ValueError("Either name or host must be specified") | ||
|
||
pt = kwargs.pop('pool_threads', None) or self.pool_threads | ||
api_key = self.config.api_key | ||
openapi_config = self.config.openapi_config | ||
|
||
if host != '': | ||
# Use host url if it is provided | ||
return Index(api_key=self.config.api_key, host=normalize_host(host), pool_threads=pt, **kwargs) | ||
|
||
if name != '': | ||
index_host=normalize_host(host) | ||
else: | ||
# Otherwise, get host url from describe_index using the index name | ||
index_host = self.index_host_store.get_host(self.index_api, self.config, name) | ||
return Index(api_key=self.config.api_key, host=index_host, pool_threads=pt, **kwargs) | ||
|
||
return Index( | ||
host=index_host, | ||
api_key=api_key, | ||
pool_threads=pt, | ||
openapi_config=openapi_config, | ||
**kwargs | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
from pinecone.core.client.api_client import ApiClient | ||
from .user_agent import get_user_agent | ||
|
||
def setup_openapi_client(api_klass, config, pool_threads): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: |
||
api_client = ApiClient( | ||
configuration=config.openapi_config, | ||
pool_threads=pool_threads | ||
) | ||
api_client.user_agent = get_user_agent() | ||
extra_headers = config.additional_headers or {} | ||
for key, value in extra_headers.items(): | ||
api_client.set_default_header(key, value) | ||
client = api_klass(api_client) | ||
return client |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
import pytest | ||
import os | ||
|
||
from pinecone import Pinecone | ||
from pinecone.core.client.configuration import Configuration as OpenApiConfiguration | ||
from urllib3 import make_headers | ||
|
||
@pytest.mark.skipif(os.getenv('USE_GRPC') != 'false', reason='Only test when using REST') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice. 👍 |
||
class TestIndexOpenapiConfig: | ||
def test_passing_openapi_config(self, api_key_fixture, index_host): | ||
oai_config = OpenApiConfiguration.get_default_copy() | ||
p = Pinecone(api_key=api_key_fixture, openapi_config=oai_config) | ||
assert p.config.api_key == api_key_fixture | ||
p.list_indexes() # should not throw | ||
|
||
index = p.Index(host=index_host) | ||
assert index._config.api_key == api_key_fixture | ||
index.describe_index_stats() |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
import pytest | ||
|
||
from pinecone.core.client.configuration import Configuration as OpenApiConfiguration | ||
from pinecone.config import ConfigBuilder | ||
from pinecone import PineconeConfigurationError | ||
|
||
class TestConfigBuilder: | ||
def test_build_simple(self): | ||
config = ConfigBuilder.build(api_key="my-api-key", host="https://my-controller-host") | ||
assert config.api_key == "my-api-key" | ||
assert config.host == "https://my-controller-host" | ||
assert config.additional_headers == {} | ||
assert config.openapi_config.host == "https://my-controller-host" | ||
assert config.openapi_config.api_key == {"ApiKeyAuth": "my-api-key"} | ||
|
||
def test_build_merges_key_and_host_when_openapi_config_provided(self): | ||
config = ConfigBuilder.build( | ||
api_key="my-api-key", | ||
host="https://my-controller-host", | ||
openapi_config=OpenApiConfiguration() | ||
) | ||
assert config.api_key == "my-api-key" | ||
assert config.host == "https://my-controller-host" | ||
assert config.additional_headers == {} | ||
assert config.openapi_config.host == "https://my-controller-host" | ||
assert config.openapi_config.api_key == {"ApiKeyAuth": "my-api-key"} | ||
|
||
def test_build_errors_when_no_api_key_is_present(self): | ||
with pytest.raises(PineconeConfigurationError) as e: | ||
ConfigBuilder.build() | ||
assert str(e.value) == "You haven't specified an Api-Key." | ||
|
||
def test_build_errors_when_no_host_is_present(self): | ||
with pytest.raises(PineconeConfigurationError) as e: | ||
ConfigBuilder.build(api_key='my-api-key') | ||
assert str(e.value) == "You haven't specified a host." |
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.
or kwargs.pop("openapi_config", None)
wasn't doing anything, sinceopenapi_config
is a named param and the key should never appear inkwargs
.