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

sdk/client: the Content-Length header should be set for DELETE/PATCH requests #448

Closed
wants to merge 1 commit into from

Conversation

tombuildsstuff
Copy link
Contributor

and match the size of the payload

…H requests too, and match the size of the payload
@tombuildsstuff tombuildsstuff requested a review from a team as a code owner May 4, 2023 07:07
Copy link
Member

@jackofallops jackofallops left a comment

Choose a reason for hiding this comment

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

LGTM 👍

// Ensure the Content-Length header is set for methods that define a meaning for enclosed content, i.e. POST and PUT.
// https://www.rfc-editor.org/rfc/rfc9110#section-8.6-5
if strings.EqualFold(req.Method, http.MethodDelete) || strings.EqualFold(req.Method, http.MethodPatch) || strings.EqualFold(req.Method, http.MethodPost) || strings.EqualFold(req.Method, http.MethodPut) {
req.Header.Set("Content-Length", fmt.Sprintf("%d", contentLength))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

looks like this header is being stripped out when it's explicitly set to a 0 value, setting this (and the original logic) fails when it's set to 0, but this works fine if another header is used

Copy link
Contributor

@manicminer manicminer left a comment

Choose a reason for hiding this comment

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

It seems like the stripping of the Content-Length and Transfer-Encoding headers is due to hashicorp/go-retryablehttp#194 - neither of these headers need to be (or should be) manually set in the Header field, they are determined by net/http as part of the transport handling for HTTP/1.1 and HTTP/2.0. Accordingly, I think we should stop trying to set them manually (since it has no effect anyway due to them being automatically replaced by net/http) and wait for hashicorp/go-retryablehttp#194.

manicminer

This comment was marked as duplicate.

@manicminer
Copy link
Contributor

Gonna close this out in favor of #500 now that the bug is resolved upstream

@manicminer manicminer closed this Jun 6, 2023
@manicminer manicminer deleted the b/content-length-should-be-set branch June 6, 2023 18:23
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.

3 participants