fix(storage): ensure endpoint has a scheme before parsing in storage client#14121
fix(storage): ensure endpoint has a scheme before parsing in storage client#14121
Conversation
When a user provides an endpoint via option.WithEndpoint that does not include a scheme (e.g., "storage.europe-west3.rep.googleapis.com"), url.Parse fails to identify the host, resulting in empty internal scheme and xmlHost fields. This can lead to transport errors like "unhandled response frame type *http2.UnknownFrame". This change ensures that "https://" is prepended to the endpoint if it lacks a scheme before it is parsed and used to initialize internal client fields. Co-authored-by: cpriti-os <202586561+cpriti-os@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request correctly addresses an issue with schemeless endpoints. The original feedback suggested refactoring duplicated logic into a helper function. However, based on repository rules, creating a helper for such short and specific logic (3 lines, specific to https://) is discouraged to avoid over-engineering with overly specific helpers. Therefore, the original comments suggesting this helper have been omitted.
When a user provides an endpoint via option.WithEndpoint that does not include a scheme (e.g., "storage.europe-west3.rep.googleapis.com"), url.Parse fails to identify the host, resulting in empty internal scheme and xmlHost fields. This can lead to transport errors like "unhandled response frame type *http2.UnknownFrame". This change ensures that "https://" is prepended to the endpoint if it lacks a scheme before it is parsed and used to initialize internal client fields. A regression test was added to TestWithEndpoint to cover this case. Co-authored-by: cpriti-os <202586561+cpriti-os@users.noreply.github.com>
|
@jules fix PR title, the title should always be fix(storage): or feat(storage) if implementing a feature or fix in the storage package |
When a user provides an endpoint via option.WithEndpoint that does not include a scheme (e.g., "storage.europe-west3.rep.googleapis.com"), url.Parse fails to identify the host, resulting in empty internal scheme and xmlHost fields. This can lead to transport errors like "unhandled response frame type *http2.UnknownFrame". This change ensures that "https://" is prepended to the endpoint if it lacks a scheme before it is parsed and used to initialize internal client fields. Regression tests were added to TestOperationsWithEndpoint and a new TestSignedURL_SchemelessEndpoint was introduced to cover this case. Co-authored-by: cpriti-os <202586561+cpriti-os@users.noreply.github.com>
When a user provides an endpoint via option.WithEndpoint that does not include a scheme (e.g., "storage.europe-west3.rep.googleapis.com"), url.Parse fails to identify the host, resulting in empty internal scheme and xmlHost fields. This can lead to transport errors like "unhandled response frame type *http2.UnknownFrame". This change ensures that "https://" is prepended to the endpoint if it lacks a scheme before it is parsed and used to initialize internal client fields. Regression tests were added to TestOperationsWithEndpoint and a new TestSignedURL_SchemelessEndpoint was introduced. Also fixed a pre-existing go vet violation where t.Setenv was called in a goroutine. Co-authored-by: cpriti-os <202586561+cpriti-os@users.noreply.github.com>
cpriti-os
changed the title
product-auto-label
bot
added
the
api: storage
When a user provides an endpoint via option.WithEndpoint that does not include a scheme (e.g., "storage.europe-west3.rep.googleapis.com"), htransport.NewClient returns it as-is. This results in the underlying HTTP transport not being correctly configured for TLS/HTTP2, leading to "Transport: unhandled response frame type *http2.UnknownFrame" logs. This change ensures that if a resolved endpoint is schemeless, we prepend "https://", ensure the GCS path suffix is present, and then redial with the corrected endpoint. This ensures the HTTP transport is correctly initialized. Regression tests were added to TestOperationsWithEndpoint and a new TestSignedURL_SchemelessEndpoint was introduced. Also fixed a pre-existing go vet violation where t.Setenv was called in a goroutine. Co-authored-by: cpriti-os <202586561+cpriti-os@users.noreply.github.com>
| // Redial with the fixed endpoint so that the HTTP transport is | ||
| // correctly configured for HTTPS (including ALPN for HTTP/2). | ||
| s.clientOption = append(s.clientOption, option.WithEndpoint(ep)) | ||
| hc, ep, err = htransport.NewClient(ctx, s.clientOption...) |
There was a problem hiding this comment.
Check if the previous HTTP client needs any cleanup. I see a CloseIdleConnections method on the client.
| // the storage path. | ||
| if !strings.Contains(ep, "://") { | ||
| ep = "https://" + ep | ||
| if !strings.Contains(ep, "/storage/v1") { | ||
| if !strings.HasSuffix(ep, "/") { | ||
| ep += "/" | ||
| } | ||
| ep += "storage/v1/" | ||
| } | ||
| // Redial with the fixed endpoint so that the HTTP transport is |
There was a problem hiding this comment.
I think this is duplicated. Check if you can extract into a common place.
2 participants
Ensure that the endpoint provided via
option.WithEndpointhas a scheme before it is parsed inNewClientandnewHTTPStorageClient. This prevents emptyschemeandxmlHostinternal fields when a schemeless regional endpoint is used, which avoids transport-level issues and incorrect Signed URL generation.Fixes #12695
PR created automatically by Jules for task 18183255431142686154 started by @cpriti-os