[pve-devel] [RFC PATCH http-server 0/2] improve error handling on api errors

[pve-devel] [RFC PATCH http-server 0/2] improve error handling on api errors

[Dominik Csapak]

these two patches improve the error handling for api errors:
* put the error in the body (so we can access them in the rust client)
* use the correct error code in some places (500 instead of 501)

the second patch is not 100% necessary now IMHO, but it is more correct,
than the status quo.

Both patches modify the api response so I send it as RFC since they're
possibly breaking API changes (not sure about how we'd interpret this
though, since it's not mentioned on [0]).

0: https://pve.proxmox.com/wiki/Proxmox_VE_API#API_Stability_&_Breakage

Dominik Csapak (2):
  add error message into http body
  use HTTP_INTERNAL_SERVER_ERROR were appropriate instead of '501'

 src/PVE/APIServer/AnyEvent.pm | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

-- 
2.39.5



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

[pve-devel] [RFC PATCH http-server 1/2] add error message into http body

[Dominik Csapak]

In our rust client, we can't access the http reason phrases[0], so let's
put them into the body itself if we don't specify an explicit content.

our proxmox-client code in rust already uses the body as message if
there is one [1], so we get that automatically.

0: https://github.com/hyperium/http/issues/737
1: https://git.proxmox.com/?p=proxmox.git;a=blob;f=proxmox-client/src/client.rs;h=9b078a9820405b22ca54c17ea4da4c586e0649b4;hb=refs/heads/master#l237

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PVE/APIServer/AnyEvent.pm | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/PVE/APIServer/AnyEvent.pm b/src/PVE/APIServer/AnyEvent.pm
index 24209a1..bd76488 100644
--- a/src/PVE/APIServer/AnyEvent.pm
+++ b/src/PVE/APIServer/AnyEvent.pm
@@ -388,6 +388,7 @@ sub error {
     my ($self, $reqstate, $code, $msg, $hdr, $content) = @_;
 
     eval {
+	$content //= $msg; # write error into body by default
 	my $resp = HTTP::Response->new($code, $msg, $hdr, $content);
 	$self->response($reqstate, $resp);
     };
-- 
2.39.5



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

[pve-devel] [RFC PATCH http-server 2/2] use HTTP_INTERNAL_SERVER_ERROR were appropriate instead of '501'

[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.

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'");
 	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'");
 	    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");
 	    return;
 	}
 
-- 
2.39.5



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

Re: [pve-devel] [RFC PATCH http-server 1/2] add error message into http body

[Thomas Lamprecht]

Am 08.01.25 um 09:45 schrieb Dominik Csapak:
> In our rust client, we can't access the http reason phrases[0], so let's
> put them into the body itself if we don't specify an explicit content.
> 
> our proxmox-client code in rust already uses the body as message if
> there is one [1], so we get that automatically.
> 
> 0: https://github.com/hyperium/http/issues/737
> 1: https://git.proxmox.com/?p=proxmox.git;a=blob;f=proxmox-client/src/client.rs;h=9b078a9820405b22ca54c17ea4da4c586e0649b4;hb=refs/heads/master#l237
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/PVE/APIServer/AnyEvent.pm | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/PVE/APIServer/AnyEvent.pm b/src/PVE/APIServer/AnyEvent.pm
> index 24209a1..bd76488 100644
> --- a/src/PVE/APIServer/AnyEvent.pm
> +++ b/src/PVE/APIServer/AnyEvent.pm
> @@ -388,6 +388,7 @@ sub error {
>      my ($self, $reqstate, $code, $msg, $hdr, $content) = @_;
>  
>      eval {
> +	$content //= $msg; # write error into body by default

might this need altering the content-type or is that already to an
OK default for a plain string? Just not that it's set to, e.g.,
application/json but contains the error that cannot be parsed
as JSON.

I can look myself, but I thought you might have already done so when
developing this.

If that's fine I'd have nothing against applying this now. 

>  	my $resp = HTTP::Response->new($code, $msg, $hdr, $content);
>  	$self->response($reqstate, $resp);
>      };



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

Re: [pve-devel] [RFC PATCH http-server 2/2] use HTTP_INTERNAL_SERVER_ERROR were appropriate instead of '501'

[Thomas Lamprecht]

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

Re: [pve-devel] [RFC PATCH http-server 1/2] add error message into http body

[Dominik Csapak]

On 1/15/25 17:08, Thomas Lamprecht wrote:
> Am 08.01.25 um 09:45 schrieb Dominik Csapak:
>> In our rust client, we can't access the http reason phrases[0], so let's
>> put them into the body itself if we don't specify an explicit content.
>>
>> our proxmox-client code in rust already uses the body as message if
>> there is one [1], so we get that automatically.
>>
>> 0: https://github.com/hyperium/http/issues/737
>> 1: https://git.proxmox.com/?p=proxmox.git;a=blob;f=proxmox-client/src/client.rs;h=9b078a9820405b22ca54c17ea4da4c586e0649b4;hb=refs/heads/master#l237
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>>   src/PVE/APIServer/AnyEvent.pm | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/src/PVE/APIServer/AnyEvent.pm b/src/PVE/APIServer/AnyEvent.pm
>> index 24209a1..bd76488 100644
>> --- a/src/PVE/APIServer/AnyEvent.pm
>> +++ b/src/PVE/APIServer/AnyEvent.pm
>> @@ -388,6 +388,7 @@ sub error {
>>       my ($self, $reqstate, $code, $msg, $hdr, $content) = @_;
>>   
>>       eval {
>> +	$content //= $msg; # write error into body by default
> 
> might this need altering the content-type or is that already to an
> OK default for a plain string? Just not that it's set to, e.g.,
> application/json but contains the error that cannot be parsed
> as JSON.
> 
> I can look myself, but I thought you might have already done so when
> developing this.
> 
> If that's fine I'd have nothing against applying this now.

The newly created response here is not setting an explicit content-type, and
according to the relevant part of the http spec[0], the client may assume
'application/octet-stream' or may try to identify the content-type
from the content itself.

While I guess most of the time we put strings in the error, it may happen
that this is something different here (since we sometimes simply
pass through the message from 'die') so explicitly setting
to text/plain could also be sometimes wrong here.

So I'd either leave it like this, or reformat the message to be e.g.
always text in some form, e.g. with

if (!defined($content)) {
     $content = "Some error occured: $msg";
     # set content type here
}

0: https://www.rfc-editor.org/rfc/rfc9110.html#name-content-type

> 
>>   	my $resp = HTTP::Response->new($code, $msg, $hdr, $content);
>>   	$self->response($reqstate, $resp);
>>       };
> 



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

Re: [pve-devel] [RFC PATCH http-server 2/2] use HTTP_INTERNAL_SERVER_ERROR were appropriate instead of '501'

[Dominik Csapak]

On 1/15/25 17:19, Thomas Lamprecht wrote:
> 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.

forgot that i talked with fabian off-list about this too, and
we said that the first 4 instances (where we simply pass through the error)
is fine, but for the last 4 (like you mentioned here) we should keep the 501
since we actually have not implemented some part of the request

I misunderstood the 501 error at first, thinking it's about the path of the request only,
but it's actually for any part of the request, so here the 'transfer-encoding' above
as well as the 'unexpected content' and 'data too large' below would qualify for a 501 error
IMO (though I'm fine with either of those be a 500 too)

So if it's fine with you, I'd send a new version with just the first 4 occurrences replaced.

> 
>>   	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