public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
From: Thomas Lamprecht <t.lamprecht@proxmox.com>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>,
	Dominik Csapak <d.csapak@proxmox.com>
Subject: Re: [pve-devel] [RFC PATCH http-server 2/2] use HTTP_INTERNAL_SERVER_ERROR were appropriate instead of '501'
Date: Wed, 15 Jan 2025 17:19:27 +0100	[thread overview]
Message-ID: <4d7a8ece-96dd-4742-a0f4-011e54258d4c@proxmox.com> (raw)
In-Reply-To: <20250108084558.390324-3-d.csapak@proxmox.com>

Am 08.01.25 um 09:45 schrieb Dominik Csapak:
> The http status code 501 is meant to be 'Not Implemented'[0] but that
> clearly does not fit here as the default error when we encounter a
> problem during handling an api request or upload.

Not sure about the clearly; 501 is not a 404 like error but one where
some functionality is not implemented.

So if the error stems from an side effect of the actual code handling
the request switching over to 500 seems OK, but if it's a error from
some header flag not being supported then 501 seems alright to me,
I looked into a few hunks inline with more comments.

> 
> So instead use '500' (HTTP_INTERNAL_SERVER_ERROR) which we already use
> in other places where it fits.
> 
> 0: https://datatracker.ietf.org/doc/html/rfc9110#name-501-not-implemented
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/PVE/APIServer/AnyEvent.pm | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/src/PVE/APIServer/AnyEvent.pm b/src/PVE/APIServer/AnyEvent.pm
> index bd76488..3b96d2a 100644
> --- a/src/PVE/APIServer/AnyEvent.pm
> +++ b/src/PVE/APIServer/AnyEvent.pm
> @@ -504,7 +504,7 @@ sub send_file_start {
>  	    $self->response($reqstate, $resp, $mtime, $nocomp);
>  	};
>  	if (my $err = $@) {
> -	    $self->error($reqstate, 501, $err);
> +	    $self->error($reqstate, HTTP_INTERNAL_SERVER_ERROR, $err);
>  	}
>      };
>  
> @@ -1020,7 +1020,7 @@ sub handle_api2_request {
>  	$self->response($reqstate, $resp, undef, $nocomp, $delay);
>      };
>      if (my $err = $@) {
> -	$self->error($reqstate, 501, $err);
> +	$self->error($reqstate, HTTP_INTERNAL_SERVER_ERROR, $err);
>      }
>  }
>  
> @@ -1214,7 +1214,7 @@ sub handle_request {
>  	die "no such file '$path'\n";
>      };
>      if (my $err = $@) {
> -	$self->error($reqstate, 501, $err);
> +	$self->error($reqstate, HTTP_INTERNAL_SERVER_ERROR, $err);
>      }
>  }
>  
> @@ -1304,7 +1304,7 @@ sub file_upload_multipart {
>      };
>      if (my $err = $@) {
>  	syslog('err', $err);
> -	$self->error($reqstate, 501, $err);
> +	$self->error($reqstate, HTTP_INTERNAL_SERVER_ERROR, $err);
>      }
>  }
>  
> @@ -1402,10 +1402,10 @@ sub process_header {
>      my $te  = $request->header('Transfer-Encoding');
>      if ($te && lc($te) eq 'chunked') {
>  	# Handle chunked transfer encoding
> -	$self->error($reqstate, 501, "chunked transfer encoding not supported");
> +	$self->error($reqstate, HTTP_INTERNAL_SERVER_ERROR, "chunked transfer encoding not supported");
>  	return 0;
>      } elsif ($te) {
> -	$self->error($reqstate, 501, "Unknown transfer encoding '$te'");
> +	$self->error($reqstate, HTTP_INTERNAL_SERVER_ERROR, "Unknown transfer encoding '$te'");

both above seem to fulfill the "server does not support the functionality
required to fulfill the request" part of the 501 Not implemented error
though?

While it follows "This is the appropriate response when the server does not
recognize the request method and is not capable of supporting it for any
resource", this rather reads as example to me, but not deep into the HTTP
lore as of now, just not 100$ sure this counts as unexpected condition, as
I can trigger it quite expectedly.

>  	return 0;
>      }
>  
> @@ -1574,7 +1574,7 @@ sub authenticate_and_handle_request {
>      if ($len) {
>  
>  	if (!($method eq 'PUT' || $method eq 'POST')) {
> -	    $self->error($reqstate, 501, "Unexpected content for method '$method'");
> +	    $self->error($reqstate, HTTP_INTERNAL_SERVER_ERROR, "Unexpected content for method '$method'");

not 100% sure here either, one could support a body for GET, but tbh. I'd
be fine with 500 here, it's even less of a a clear cut.

>  	    return;
>  	}
>  
> @@ -1624,7 +1624,7 @@ sub authenticate_and_handle_request {
>  	}
>  
>  	if ($len > $limit_max_post) {
> -	    $self->error($reqstate, 501, "for data too large");
> +	    $self->error($reqstate, HTTP_INTERNAL_SERVER_ERROR, "for data too large");

501 could be OK here, we explicitly do not implement handling bigger
data.

>  	    return;
>  	}
>  



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


  reply	other threads:[~2025-01-15 16:20 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-01-08  8:45 [pve-devel] [RFC PATCH http-server 0/2] improve error handling on api errors Dominik Csapak
2025-01-08  8:45 ` [pve-devel] [RFC PATCH http-server 1/2] add error message into http body Dominik Csapak
2025-01-15 16:08   ` Thomas Lamprecht
2025-01-16  7:31     ` Dominik Csapak
2025-01-27 12:44       ` Dominik Csapak
2025-01-28 14:24         ` Thomas Lamprecht
2025-01-28 14:46           ` Dominik Csapak
2025-01-29 17:51             ` Thomas Lamprecht
2025-01-08  8:45 ` [pve-devel] [RFC PATCH http-server 2/2] use HTTP_INTERNAL_SERVER_ERROR were appropriate instead of '501' Dominik Csapak
2025-01-15 16:19   ` Thomas Lamprecht [this message]
2025-01-16  7:36     ` Dominik Csapak

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4d7a8ece-96dd-4742-a0f4-011e54258d4c@proxmox.com \
    --to=t.lamprecht@proxmox.com \
    --cc=d.csapak@proxmox.com \
    --cc=pve-devel@lists.proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal