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

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

Merged
merged 1 commit into from
Feb 21, 2024

Conversation

zulinx86
Copy link
Contributor

@zulinx86 zulinx86 commented Feb 20, 2024

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.

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.

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

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
src/response.rs Outdated Show resolved Hide resolved
src/response.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@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).

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
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>
@roypat roypat merged commit 7e2684d into firecracker-microvm:main Feb 21, 2024
2 checks passed
@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
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
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
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
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants