Skip to content
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

Allow Google storage endpoints without http/s #179

Merged
merged 1 commit into from
Oct 10, 2024

Conversation

jcortejoso
Copy link
Contributor

Fixes Issue

The line endpointURL, err := url.Parse(cfg.Endpoint) does not properly parse the URL if the parameter does not include the protocol (http/https). This is problematic because the protocol cannot be included when initializing the S3/MinIO client; otherwise, you get the following error:

t=2024-10-09T08:31:27+0000 lvl=crit msg="Application failed" role=eigenda_proxy message="failed to create store: failed to create S3 store: Endpoint url cannot have fully qualified paths."

Changes proposed

A small refactor to simplify the Google Storage endpoint checker.

Screenshots (Optional)

Note to reviewers

For the simple tests added, I’ve only focused on the isGoogleEndpoint() method. While https://storage.googleapis.com is not a valid URL for minio.New(), I skipped checking for that in this function. If you'd prefer, I can include a check in server/config.go to ensure that the protocol is absent in the config provided.
Sorry about the noise/multiple PRs.

Comment on lines +60 to +63
func isGoogleEndpoint(endpoint string) bool {
return strings.Contains(endpoint, "storage.googleapis.com")
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems fine. if it's always going to be https://storage.googleapis.com, maybe trim the protocol instead of doing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So right now it requires to be submitted without https for minio/client (i.e.: --s3.endpoint=storage.googleapis.com).

Another solution would be ensuring that the protocol is present before using url.Parse() and keep s3utils.IsGoogleEndpoint. In minio, they use a helper method for this.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When/why do you ever need the full protocol url? And why does minio not allow it (I’ve noticed this also and found it annoying). I’m just curious at this point, but I’ll approve this, sounds fine for now

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it's just needed for url.Parse() here. I think it may depend on each client implementation, but seems minio-go client don't handle if the endpoint has the protocol on it.

@samlaf samlaf merged commit 1395dfa into Layr-Labs:main Oct 10, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants