[Fix] Configuring SSL proxy via openapi_config object #321

jhamon · 2024-03-12T20:06:12Z

Problem

A few different problems being solved here:

Pinecone class accepts an optional param, openapi_config. When this is passed (usually as a vehicle for SSL configurations), it currently clobbers the api_key param so the user sees an error message about not providing an api_key (even though they did pass it) if they attempt to perform a control plane operation

from pinecone import Pinecone
from pinecone.core.client.configuration import Configuration as OpenApiConfiguration

openapi_config = OpenApiConfiguration()
openapi_config.ssl_ca_cert = '/path/to/cert'

pc = Pinecone(api_key='key, openapi_config=openapi_config)
pc.list_indexes() // error: No api-key provided

In a related issue, the openapi_config (with SSL configs) was not being correctly passed through to the underlying DataPlaneApi for data calls. So users with custom network configurations requiring SSL config would see SSL validation failures when attempting data operations.

from pinecone import Pinecone
from pinecone.core.client.configuration import Configuration as OpenApiConfiguration

openapi_config = OpenApiConfiguration()
openapi_config.ssl_ca_cert = '/path/to/cert'

pc = Pinecone(api_key='key, openapi_config=openapi_config)
pc.list_indexes() // error: No api-key provided

Solution

Adjust the ConfigBuilder to avoid clobbering API key
Move some logic into a util function so that behavior will be consistent across both data and control planes
Ensure configuration is passed to from the Pinecone object to the index client
deepcopy the openapi_config object before modifying it so that index-specific host changes do clobber control plane or calls to other indexes.

Future work

In the future, we should deprecate openapi_config and have some way of passing SSL config without all the baggage that comes with this OpenApiConfiguration object. This config object is an undocumented holdover from earlier versions of the client and breaks the abstraction the client is trying to provide to smooth out the UX of the generated SDK code.

Type of Change

Bug fix (non-breaking change which fixes an issue)

Test Plan

Added tests

jhamon · 2024-03-12T22:51:06Z

pinecone/config/config.py

@@ -46,10 +46,10 @@ def build(
        if not host:
            raise PineconeConfigurationError("You haven't specified a host.")

-        openapi_config = (
-            openapi_config
-            or kwargs.pop("openapi_config", None)


or kwargs.pop("openapi_config", None) wasn't doing anything, since openapi_config is a named param and the key should never appear in kwargs.

jhamon · 2024-03-12T22:51:54Z

pinecone/config/config.py

+            openapi_config.host = host
+            openapi_config.api_key = {"ApiKeyAuth": api_key}


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.

jhamon · 2024-03-12T22:52:27Z

pinecone/control/pinecone.py

-        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


This was more cleanup since config is a named param that should never appear in kwargs

ssmith-pc · 2024-03-12T23:04:38Z

pinecone/control/pinecone.py

-            return Index(api_key=self.config.api_key, host=normalize_host(host), pool_threads=pt, **kwargs)
+            return Index(
+                host=normalize_host(host),
+                api_key=api_key,
+                pool_threads=pt,
+                openapi_config=openapi_config,
+                **kwargs
+            )

        if name != '':
            # 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
+            )


This may be out of the scope for this change since you're just adding the openapi_config param, but it looks like these two blocks are largely identical, differing only by how the host param is init'd

austin-denoble

Nice job tracking this one down and cleaning that up. 🚢

austin-denoble · 2024-03-12T23:04:38Z

pinecone/utils/setup_openapi_client.py

+from pinecone.core.client.api_client import ApiClient
+from .user_agent import get_user_agent
+
+def setup_openapi_client(api_klass, config, pool_threads):


nit: _klass?

austin-denoble · 2024-03-12T23:05:59Z

tests/integration/data/test_openapi_configuration.py

+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')


* 'main' of github.com:pinecone-io/pinecone-python-client: [skip ci] Bump version to v4.1.0 Bump tqdm from 4.66.1 to 4.66.3 (pinecone-io#344) Bump idna from 3.4 to 3.7 (pinecone-io#345) Bump jinja2 from 3.1.3 to 3.1.4 (pinecone-io#343) Add better error messages for mistaken `from_texts` and `from_documents` (pinecone-io#342) Support proxy_url and ssl_ca_certs options for gRPC (pinecone-io#341) Remove serverless public preview warnings (pinecone-io#340) [skip ci] Bump version to v4.0.0 Improve upsert throughput by 3x (pinecone-io#334) Remove `merge` workflow and update `build-and-publish-docs` workflow to be manually runnable (pinecone-io#335) [skip ci] Bump version to v3.2.2 [Fix] openapi_config deprecation warning incorrectly shown (pinecone-io#327) Add grpc unit test run, expand testing of VectorFactory (pinecone-io#326) [skip ci] Bump version to v3.2.1 Allow clients to tag requests with a source_tag (pinecone-io#324) [skip ci] Bump version to v3.2.0 Revise proxy configuration, add integration testing (pinecone-io#325) [Fix] Configuring SSL proxy via openapi_config object (pinecone-io#321) Update README.md (pinecone-io#323)

jhamon added 3 commits March 12, 2024 16:04

Move api client setup into helper function

6633a58

Correctly build config using openapi_config

8e5effb

Thread SSL configuration through to index client

dfa9f5e

jhamon marked this pull request as ready for review March 12, 2024 22:50

jhamon requested a review from austin-denoble March 12, 2024 22:50

jhamon commented Mar 12, 2024

View reviewed changes

ssmith-pc reviewed Mar 12, 2024

View reviewed changes

austin-denoble approved these changes Mar 12, 2024

View reviewed changes

jhamon added 3 commits March 14, 2024 11:54

Copy openapi config object before modifying it

dbefd52

Fix failing config unit test

72615e6

Review feedback: Cleanup duplication in Index instantiation

7b47e8e

jhamon merged commit a201cb6 into main Mar 14, 2024
124 checks passed

jhamon deleted the jhamon/ssl-config branch March 14, 2024 17:11

jhamon mentioned this pull request Mar 14, 2024

Pass the openapi_config to the Index object #318

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fix] Configuring SSL proxy via openapi_config object #321

[Fix] Configuring SSL proxy via openapi_config object #321

jhamon commented Mar 12, 2024 •

edited

Loading

jhamon Mar 12, 2024

jhamon Mar 12, 2024

jhamon Mar 12, 2024

ssmith-pc Mar 12, 2024

austin-denoble left a comment

austin-denoble Mar 12, 2024

austin-denoble Mar 12, 2024

		openapi_config.host = host
		openapi_config.api_key = {"ApiKeyAuth": api_key}

[Fix] Configuring SSL proxy via openapi_config object #321

[Fix] Configuring SSL proxy via openapi_config object #321

Conversation

jhamon commented Mar 12, 2024 • edited Loading

Problem

Solution

Future work

Type of Change

Test Plan

jhamon Mar 12, 2024

Choose a reason for hiding this comment

jhamon Mar 12, 2024

Choose a reason for hiding this comment

jhamon Mar 12, 2024

Choose a reason for hiding this comment

ssmith-pc Mar 12, 2024

Choose a reason for hiding this comment

austin-denoble left a comment

Choose a reason for hiding this comment

austin-denoble Mar 12, 2024

Choose a reason for hiding this comment

austin-denoble Mar 12, 2024

Choose a reason for hiding this comment

jhamon commented Mar 12, 2024 •

edited

Loading