Skip to content

Commit

Permalink
Fix normalizeHost, allow for non-secure connections (#74)
Browse files Browse the repository at this point in the history
## Problem
When working with specific environments and hosts that require a
non-secure connection, calling
`pc.Index(pinecone.NewIndexConnParams{Host: idx.Host})` with a `Host`
value that is insecure, you'll run into unexpected behavior seeing
`:443` attached as a port, or gRPC errors around connecting insecurely
without explicitly passing a `grpc.DialOption`.

Our `normalizeHost` function has a bug in that it applies port `:443` to
`Host` values which are passed without an explicit scheme.

## Solution
- We can simplify the `normalizeHost` function to only deal with
stripping away any provided scheme, and then checking to see if the
connection is explicitly insecure. This is accomplished by providing
`http://` as a part of the `NewIndexConnParams{ Host: yourHost }` value,
which would need to be accounted for in documentation, etc.

This felt most straightforward because of how the `grpc` module handles
host addresses. Ultimately, we need to strip the scheme off any hosts
that are passed, but we also need to decide whether we're dialing in a
secure or insecure fashion. I thought of providing an explicit `bool` as
a part of the `NewIndexConnParams`, but it felt more unwieldy adding new
fields.

## Type of Change
- [X] 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

I have a local test harness that I'm using to interact with a locally
hosted index. I was overriding the `go-pinecone` package in `go.mod` to
work against my local copy of the SDK. You can pull this branch down and
do something similar locally. Here are some of my results:

### Before
![Screenshot 2024-09-12 at 10 09
45 PM](https://github.com/user-attachments/assets/1ba87d57-24ee-44ab-bb14-08b7fe98a683)

### After
![Screenshot 2024-09-12 at 10 33
59 PM](https://github.com/user-attachments/assets/c26ad177-3ca6-4664-a708-31ea4fccff3e)
  • Loading branch information
austin-denoble authored Sep 13, 2024
1 parent b0e8307 commit 64f1d68
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 32 deletions.
31 changes: 17 additions & 14 deletions pinecone/index_connection.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"github.com/pinecone-io/go-pinecone/internal/useragent"
"google.golang.org/grpc"
"google.golang.org/grpc/credentials"
"google.golang.org/grpc/credentials/insecure"
"google.golang.org/grpc/metadata"
)

Expand All @@ -37,19 +38,19 @@ type newIndexParameters struct {
}

func newIndexConnection(in newIndexParameters, dialOpts ...grpc.DialOption) (*IndexConnection, error) {
target := normalizeHost(in.host)
target, isSecure := normalizeHost(in.host)

// configure default gRPC DialOptions
grpcOptions := []grpc.DialOption{
grpc.WithAuthority(target),
grpc.WithUserAgent(useragent.BuildUserAgentGRPC(in.sourceTag)),
}

// if the target includes an http:// address, don't include TLS
// otherwise we need to add transport credentials
if !strings.HasPrefix(target, "http://") {
if isSecure {
config := &tls.Config{}
grpcOptions = append(grpcOptions, grpc.WithTransportCredentials(credentials.NewTLS(config)))
} else {
grpcOptions = append(grpcOptions, grpc.WithTransportCredentials(insecure.NewCredentials()))
}

// if we have user-provided dialOpts, append them to the defaults here
Expand Down Expand Up @@ -1104,24 +1105,26 @@ func sparseValToGrpc(sv *SparseValues) *data.SparseValues {
}
}

func normalizeHost(host string) string {
func normalizeHost(host string) (string, bool) {
// default to secure unless http is specified
isSecure := true

parsedHost, err := url.Parse(host)
if err != nil {
log.Default().Printf("Failed to parse host %s: %v", host, err)
return host
return host, isSecure
}

if parsedHost.Scheme == "http" {
isSecure = false
}

// if https:// or http:// without a port, strip the scheme
// the gRPC client is not expecting a scheme so we strip that out
if parsedHost.Scheme == "https" {
host = strings.TrimPrefix(host, "https://")
} else if parsedHost.Scheme == "http" && parsedHost.Port() == "" {
} else if parsedHost.Scheme == "http" {
host = strings.TrimPrefix(host, "http://")
}

// if a port was provided leave it, otherwise we append :443
if parsedHost.Port() == "" {
host = host + ":443"
}

return host
return host, isSecure
}
32 changes: 14 additions & 18 deletions pinecone/index_connection_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1000,33 +1000,29 @@ func TestToUsageUnit(t *testing.T) {

func TestNormalizeHostUnit(t *testing.T) {
tests := []struct {
name string
host string
expectedHost string
name string
host string
expectedHost string
expectedIsSecure bool
}{
{
name: "https:// scheme should be removed",
host: "https://this-is-my-host.io",
expectedHost: "this-is-my-host.io:443",
}, {
name: "https:// scheme with a port should be removed",
host: "https://this-is-my-host.io:33445",
expectedHost: "this-is-my-host.io:33445",
}, {
name: "http:// scheme without a port should be removed",
host: "http://this-is-my-host.io",
expectedHost: "this-is-my-host.io:443",
name: "https:// scheme should be removed",
host: "https://this-is-my-host.io",
expectedHost: "this-is-my-host.io",
expectedIsSecure: true,
}, {
name: "http:// scheme and port should be maintained",
host: "http://this-is-my-host.io:8080",
expectedHost: "http://this-is-my-host.io:8080",
name: "https:// scheme should be removed",
host: "https://this-is-my-host.io:33445",
expectedHost: "this-is-my-host.io:33445",
expectedIsSecure: true,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
result := normalizeHost(tt.host)
result, isSecure := normalizeHost(tt.host)
assert.Equal(t, tt.expectedHost, result, "Expected result to be '%s', but got '%s'", tt.expectedHost, result)
assert.Equal(t, tt.expectedIsSecure, isSecure, "Expected isSecure to be '%t', but got '%t'", tt.expectedIsSecure, isSecure)
})
}
}
Expand Down

0 comments on commit 64f1d68

Please sign in to comment.