Skip to content

fix: Always set Content-Length field in non-100/204 responses #48

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 21, 2024
Merged

fix: Always set Content-Length field in non-100/204 responses #48

merged 1 commit into from
Feb 21, 2024

Conversation

zulinx86
Copy link
Contributor

@zulinx86 zulinx86 commented Feb 20, 2024 •
edited
Loading

Reason for This PR

Without having 'Content-Length: 0' for an empty content, a client may wait for the server to close the connection or for a timeout to occur like follows:

$ curl -s "http://169.254.169.254/latest/meta-data/message" --max-time 5 -v
*   Trying 169.254.169.254:80...
* Connected to 169.254.169.254 (169.254.169.254) port 80 (#0)
> GET /latest/meta-data/message HTTP/1.1
> Host: 169.254.169.254
> User-Agent: curl/7.81.0
> Accept: */*
> 
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 
< Server: Firecracker API
< Connection: keep-alive
* no chunk, no close, no size. Assume close to signal end
< 
* Operation timed out after 5001 milliseconds with 0 bytes received
* Closing connection 0

With this change, the client can close the connection soon after it gets the response.

# curl -s "http://169.254.169.254/latest/meta-data/message" --max-time 5 -v
*   Trying 169.254.169.254:80...
* Connected to 169.254.169.254 (169.254.169.254) port 80 (#0)
> GET /latest/meta-data/message HTTP/1.1
> Host: 169.254.169.254
> User-Agent: curl/7.81.0
> Accept: */*
>
* Mark bundle as not supporting multiuse
< HTTP/1.1 200
< Server: Firecracker API
< Connection: keep-alive
< Content-Type: application/json
< Content-Length: 0
<
* Connection #0 to host 169.254.169.254 left intact

Description of Changes

  • Always set Content-Length field in non-100/204 responses regardless of whether the body is empty. Although it doesn't cover all the patterns where the response must not set it, users can remove it by calling set_content_length(None) if needed.

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.

PR Checklist

[Author TODO: Meet these criteria.]
[Reviewer TODO: Verify that these criteria are met. Request changes if not]

  • All commits in this PR are signed (git commit -s).
  • The reason for this PR is clearly provided (issue no. or explanation).
  • The description of changes is clear and encompassing.
  • [ ] Any required documentation changes (code and docs) are included in this PR.
  • [ ] Any newly added unsafe code is properly documented.
  • Any user-facing changes are mentioned in CHANGELOG.md.

@zulinx86 zulinx86 force-pushed the optional_content_length branch from 2905bb5 to 1f9a265 Compare February 20, 2024 18:42
@zulinx86 zulinx86 self-assigned this Feb 20, 2024
@zulinx86 zulinx86 force-pushed the optional_content_length branch from 1f9a265 to 5293dea Compare February 20, 2024 18:45
pb8o
pb8o reviewed Feb 20, 2024
View reviewed changes
Copy link

@pb8o pb8o left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Why is it important to distinguish between no content and empty content? could we not just always return Content-Length: 0 ?

@zulinx86 zulinx86 force-pushed the optional_content_length branch from 5293dea to c295b53 Compare February 21, 2024 08:27
roypat
roypat reviewed Feb 21, 2024
View reviewed changes
@zulinx86
Copy link
Contributor Author

Why is it important to distinguish between no content and empty content? could we not just always return Content-Length: 0 ?

There are many cases where the server should not contain 'Content-Length' field in response. Please see RFC 9110.
https://datatracker.ietf.org/doc/html/rfc9110#name-content-length

A server MAY send a Content-Length header field in a response to a HEAD request (Section 9.3.2); a server MUST NOT send Content-Length in such a response unless its field value equals the decimal number of octets that would have been sent in the content of a response if the same request had used the GET method.

A server MAY send a Content-Length header field in a 304 (Not Modified) response to a conditional GET request (Section 15.4.5); a server MUST NOT send Content-Length in such a response unless its field value equals the decimal number of octets that would have been sent in the content of a 200 (OK) response to the same request.

A server MUST NOT send a Content-Length header field in any response with a status code of 1xx (Informational) or 204 (No Content). A server MUST NOT send a Content-Length header field in any 2xx (Successful) response to a CONNECT request (Section 9.3.6).

pb8o
pb8o reviewed Feb 21, 2024
View reviewed changes
Copy link

@pb8o pb8o left a comment

Choose a reason for hiding this comment

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

I think this would also benefit from an integration test in test_mmds.py? What do you think?

@zulinx86
Copy link
Contributor Author

I think this would also benefit from an integration test in test_mmds.py? What do you think?

@pb8o Yes, we can have it on the firecracker side. I'll implement it on firecracker repo (not on micro-http repo).

@zulinx86 zulinx86 force-pushed the optional_content_length branch 3 times, most recently from dc751a0 to 95c0794 Compare February 21, 2024 12:51
@zulinx86 zulinx86 changed the title fix: Make content_length optional fix: Always set Content-Length field in non-100/204 responses Feb 21, 2024
@zulinx86 zulinx86 force-pushed the optional_content_length branch from 95c0794 to d42feed Compare February 21, 2024 12:52
@zulinx86
fix: Always set Content-Length field in non-100/204 responses
0164349 
The client may wait for the server to close the connection or for
timeout to occur in some cases. This commit changes it to always set
Content-Length field in non-100/204 responses regardless of whether
the body is empty. Although it doesn't cover all the patterns where
the response must not set it, users can remove it by calling
`set_content_length(None)`.

Signed-off-by: Takahiro Itazuri <itazur@amazon.com>
@zulinx86 zulinx86 force-pushed the optional_content_length branch from d42feed to 0164349 Compare February 21, 2024 12:55
@roypat roypat merged commit 7e2684d into firecracker-microvm:main Feb 21, 2024
@zulinx86 zulinx86 deleted the optional_content_length branch February 21, 2024 13:23
zulinx86 added a commit to zulinx86/firecracker that referenced this pull request Feb 21, 2024
@zulinx86
fix(mmds): Update micro-http for empty MMDS path
b39b6a9 
When querying a MMDS path that has an empty content, the response didn't
contain 'Content-Length' field, resulting in the client waiting for the
connection closed by the server or timeout. To fix this issue,
micro-http made a change to have the field when the response status code
is not 1XX or 204. This commit updates Cargo.lock file to consume this
change and also adds an integration test to ensure that it doesn't
timeout for an empty MMDS path.

The micro-http's change is made on
firecracker-microvm/micro-http#48.

Signed-off-by: Takahiro Itazuri <itazur@amazon.com>
zulinx86 added a commit to zulinx86/firecracker that referenced this pull request Feb 21, 2024
@zulinx86
fix(mmds): Update micro-http for empty MMDS path
0831669 
When querying a MMDS path that has an empty content, the response didn't
contain 'Content-Length' field, resulting in the client waiting for the
connection closed by the server or timeout. To fix this issue,
micro-http made a change to have the field when the response status code
is not 1XX or 204. This commit updates Cargo.lock file to consume this
change and also adds an integration test to ensure that it doesn't
timeout for an empty MMDS path.

The micro-http's change was made on
firecracker-microvm/micro-http#48.

Signed-off-by: Takahiro Itazuri <itazur@amazon.com>
zulinx86 added a commit to zulinx86/firecracker that referenced this pull request Feb 21, 2024
@zulinx86
fix(mmds): Update micro-http for empty MMDS path
33d1efc 
When querying a MMDS path that has an empty content, the response didn't
contain 'Content-Length' field, resulting in the client waiting for the
connection closed by the server or timeout. To fix this issue,
micro-http made a change to have the field when the response status code
is not 1XX or 204. This commit updates Cargo.lock file to consume this
change and also adds an integration test to ensure that it doesn't
timeout for an empty MMDS path.

The micro-http's change was made on
firecracker-microvm/micro-http#48.

Signed-off-by: Takahiro Itazuri <itazur@amazon.com>
zulinx86 added a commit to firecracker-microvm/firecracker that referenced this pull request Feb 21, 2024
@zulinx86
fix(mmds): Update micro-http for empty MMDS path
e5e2fc3 
When querying a MMDS path that has an empty content, the response didn't
contain 'Content-Length' field, resulting in the client waiting for the
connection closed by the server or timeout. To fix this issue,
micro-http made a change to have the field when the response status code
is not 1XX or 204. This commit updates Cargo.lock file to consume this
change and also adds an integration test to ensure that it doesn't
timeout for an empty MMDS path.

The micro-http's change was made on
firecracker-microvm/micro-http#48.

Signed-off-by: Takahiro Itazuri <itazur@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pb8o pb8o pb8o approved these changes

@roypat roypat roypat approved these changes

Assignees

@zulinx86 zulinx86

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@zulinx86 @pb8o @roypat