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: make HEAD responses emtpy #600

Merged
merged 1 commit into from
Jan 9, 2025
Merged

fix: make HEAD responses emtpy #600

merged 1 commit into from
Jan 9, 2025

Conversation

Panaetius
Copy link
Member

closes #582

/deploy

@Panaetius Panaetius requested a review from a team as a code owner January 8, 2025 15:15
@RenkuBot
Copy link
Contributor

RenkuBot commented Jan 8, 2025

You can access the deployment of this PR at https://renku-ci-ds-600.dev.renku.ch

@Panaetius
Copy link
Member Author

tested with https://renku-ci-ds-600.dev.renku.ch/ui-server/api/data/data_connectors/01JH51XNPC2EJBQ4CJAWHX9G8J on the PR deployment.

❯ curl -X HEAD -v https://renku-ci-ds-600.dev.renku.ch/ui-server/api/data/data_connectors/01JH51XNPC2EJBQ4CJAWHX9G8J
Warning: Setting custom HTTP method to HEAD with -X/--request may not work the way you want. Consider using
Warning: -I/--head instead.
* Host renku-ci-ds-600.dev.renku.ch:443 was resolved.
* IPv6: (none)
* IPv4: 86.119.36.34
*   Trying 86.119.36.34:443...
* ALPN: curl offers h2,http/1.1
* TLSv1.3 (OUT), TLS handshake, Client hello (1):
*  CAfile: /etc/ssl/certs/ca-certificates.crt
*  CApath: none
* TLSv1.3 (IN), TLS handshake, Server hello (2):
* TLSv1.3 (IN), TLS change cipher, Change cipher spec (1):
* TLSv1.3 (IN), TLS handshake, Encrypted Extensions (8):
* TLSv1.3 (IN), TLS handshake, Certificate (11):
* TLSv1.3 (IN), TLS handshake, CERT verify (15):
* TLSv1.3 (IN), TLS handshake, Finished (20):
* TLSv1.3 (OUT), TLS change cipher, Change cipher spec (1):
* TLSv1.3 (OUT), TLS handshake, Finished (20):
* SSL connection using TLSv1.3 / TLS_AES_256_GCM_SHA384 / x25519 / RSASSA-PSS
* ALPN: server accepted h2
* Server certificate:
*  subject: CN=renku-ci-ds-600.dev.renku.ch
*  start date: Jan  8 14:23:46 2025 GMT
*  expire date: Apr  8 14:23:45 2025 GMT
*  subjectAltName: host "renku-ci-ds-600.dev.renku.ch" matched cert's "renku-ci-ds-600.dev.renku.ch"
*  issuer: C=US; O=Let's Encrypt; CN=R10
*  SSL certificate verify ok.
*   Certificate level 0: Public key type RSA (2048/112 Bits/secBits), signed using sha256WithRSAEncryption
*   Certificate level 1: Public key type RSA (2048/112 Bits/secBits), signed using sha256WithRSAEncryption
*   Certificate level 2: Public key type RSA (4096/152 Bits/secBits), signed using sha256WithRSAEncryption
* Connected to renku-ci-ds-600.dev.renku.ch (86.119.36.34) port 443
* using HTTP/2
* [HTTP/2] [1] OPENED stream for https://renku-ci-ds-600.dev.renku.ch/ui-server/api/data/data_connectors/01JH51XNPC2EJBQ4CJAWHX9G8J
* [HTTP/2] [1] [:method: HEAD]
* [HTTP/2] [1] [:scheme: https]
* [HTTP/2] [1] [:authority: renku-ci-ds-600.dev.renku.ch]
* [HTTP/2] [1] [:path: /ui-server/api/data/data_connectors/01JH51XNPC2EJBQ4CJAWHX9G8J]
* [HTTP/2] [1] [user-agent: curl/8.11.1]
* [HTTP/2] [1] [accept: */*]
> HEAD /ui-server/api/data/data_connectors/01JH51XNPC2EJBQ4CJAWHX9G8J HTTP/2
> Host: renku-ci-ds-600.dev.renku.ch
> User-Agent: curl/8.11.1
> Accept: */*
>
* Request completely sent off
* TLSv1.3 (IN), TLS handshake, Newsession Ticket (4):
* TLSv1.3 (IN), TLS handshake, Newsession Ticket (4):
< HTTP/2 200
< date: Thu, 09 Jan 2025 08:14:10 GMT
< content-type: application/json
< content-length: 0
< etag: "5C712DF7B5AA007C01927F5CCB7621B1"
< set-cookie: _renku_session=MTczNjQxMDQ1MHxGTTN0RzVBUXpndnhZRmIwUElPVGhrZWdLVHZpQWR2WU1GYnU1VTFScUlkcm5KMmNRWTZ3SHpPR0FIRU9HczlET01kMFFxemJWMUtSa0oyUnp2YUt8FhRDHgGlBibdRkzjb39_Ci4eJ1EY0a9kPlG-7r16lu4=; Path=/; HttpOnly; Secure; SameSite=Lax
< x-request-id: e5fc201f2e6a3138ab1c0641411ab293
< strict-transport-security: max-age=31536000; includeSubDomains
< content-security-policy: frame-ancestors 'self' https://*.renku-ci-ds-600.dev.renku.ch https://renku-ci-ds-600.dev.renku.ch
<
* Connection #0 to host renku-ci-ds-600.dev.renku.ch left intact

etag and other headers are sent, content-length is 0

async def handle_head(request: Request, response: BaseHTTPResponse) -> None:
"""Make sure HEAD requests return an empty body."""
if request.method == "HEAD":
response.body = None
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean, the response has already been created and is now discarded before being rendered?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes. the response needs to be created in some cases as we return the etag in the headers (which gets calculated based on DB state) and a client getting the etag without having to receive the whole entity is one of the main use-cases for head requests in our case.

So it doesn't save on server processing, just unnecessary network traffic.

There's not really a better way to solve this with Sanic from what I could tell, other than implementing individual "HEAD" handlers for every endpoint we have.

@Panaetius Panaetius merged commit d3329d7 into main Jan 9, 2025
16 checks passed
@Panaetius Panaetius deleted the emtpy-head branch January 9, 2025 13:52
@RenkuBot
Copy link
Contributor

RenkuBot commented Jan 9, 2025

Tearing down the temporary RenkuLab deplyoment for this PR.

@coveralls
Copy link

coveralls commented Jan 9, 2025

Pull Request Test Coverage Report for Build 12686153226

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 3 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.01%) to 86.768%

Files with Coverage Reduction New Missed Lines %
components/renku_data_services/secrets/db.py 3 92.68%
Totals Coverage Status
Change from base Build 12672357807: -0.01%
Covered Lines: 15174
Relevant Lines: 17488

💛 - Coveralls

@leafty
Copy link
Member

leafty commented Jan 13, 2025

< content-length: 0

This is not correct. The response headers SHOULD be the same as if the request verb was GET.

See:

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.

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.

HEAD responses should have no body content
5 participants