-
Notifications
You must be signed in to change notification settings - Fork 13
Add support for disabling TLS #150
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
Conversation
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.
Couple considerations:
- Is this TLS flag needed? In the example code:
pinecone-java-client/src/main/java/io/pinecone/configs/PineconeConfig.java
Lines 30 to 38 in da7ebc2
* // Custom channel with timeouts | |
* try { | |
* builder = builder.overrideAuthority(endpoint) | |
* .negotiationType(NegotiationType.TLS) | |
* .keepAliveTimeout(5, TimeUnit.SECONDS) | |
* .sslContext(GrpcSslContexts.forClient().build()); | |
* } catch (SSLException e) { | |
* throw new PineconeException("SSL error opening gRPC channel", e); | |
* } |
It looks like you can set negotiationType(NegotiationType.PLAINTEXT)
to turn off TLS?
- Is the flag really global to PineconeClient? Or is it per-connection to an index?
- Another example had the host with a
http(s)://
prefix and:PORT
suffix. Would the http vs https prefix on the host be enough without setting a specific flag? Or is this considered bad practice?
|
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.
Looks straightforward, few minor comments
Problem
Add support for enabling/disabling TLS.
Solution
Added enableTls boolean flag which is set to true by default but when set to false, it will be disabled.
Type of Change
Test Plan
Since the flag is set to true by default, the existing integration tests should work as it is.