PR#3269: return 400 codes when client fails to send a full request - koji

koji

#3269 return 400 codes when client fails to send a full request

Merged 2 years ago by tkopecek. Opened 2 years ago by mikem.

mikem/koji client-read-errors into master

also raise 400 errors when we can't read the client upload stream

Mike McLean • 2 years ago

323bb2f

fix indent

Mike McLean • 2 years ago

f51c502

return 400 codes when client fails to send a full request

Mike McLean • 2 years ago

5967391

hub/kojihub.py

file modified

+12 -1

		`@@ -62,6 +62,7 @@`
		`import koji.xmlrpcplus`
		`from koji.context import context`
		`from koji.daemon import SCM`
		`+ from koji.server import BadRequest, RequestTimeout`
		`from koji.util import (`
		`base64encode,`
		`decode_bytes,`
		`@@ -15406,7 +15407,17 @@`
		`os.ftruncate(fd, offset)`
		`os.lseek(fd, offset, 0)`
		`while True:`
		`- chunk = inf.read(65536)`
		`+ try:`
		`+ chunk = inf.read(65536)`
		`+ except OSError as e:`
		`+ str_e = str(e)`
		`+ logger.error(f"Error reading upload. Offset {offset}+{size}, path {fn}")`
		`+ if 'timeout' in str_e:`
		`+ logger.exception("Timed out reading input stream")`
		`+ raise RequestTimeout(str_e)`
		`+ else:`
		`+ logger.exception("Error reading input stream")`
		`+ raise BadRequest(str_e)`
		`if not chunk:`
		`break`
		`size += len(chunk)`

hub/kojixmlrpc.py

file modified

+38 -14

		`@@ -38,6 +38,7 @@`
		`import koji.util`
		`from koji.context import context`
		`# import xmlrpclib functions from koji to use tweaked Marshaller`
		`+ from koji.server import ServerError, BadRequest, RequestTimeout`
		`from koji.xmlrpcplus import ExtendedMarshaller, Fault, dumps, getparser`


		`@@ -213,7 +214,16 @@`
		`rlen = 0`
		`maxlen = opts.get('MaxRequestLength', None)`
		`while True:`
		`- chunk = stream.read(8192)`
		`+ try:`
		`+ chunk = stream.read(8192)`
		`+ except OSError as e:`
		`+ str_e = str(e)`
		`+ if 'timeout' in str_e:`
		`+ self.logger.exception("Timed out reading input stream")`
		`+ raise RequestTimeout(str_e)`
		`+ else:`
		`+ self.logger.exception("Error reading input stream")`
		`+ raise BadRequest(str_e)`
		`if not chunk:`
		`break`
		`rlen += len(chunk)`
		`@@ -255,6 +265,9 @@`
		`# wrap response in a singleton tuple`
		`response = (response,)`
		`response = dumps(response, methodresponse=1, marshaller=Marshaller)`
		`+ except ServerError:`
		`+ raise`
		`+ # these are handled higher up`
		`except Fault as fault:`
		`self.traceback = True`
		`response = dumps(fault, marshaller=Marshaller)`
		`@@ -382,6 +395,18 @@`
		`return [response]`


		`+ def error_reply(start_response, status, response, extra_headers=None):`
		`+ response = response.encode()`
		`+ headers = [`
		`+ ('Content-Length', str(len(response))),`
		`+ ('Content-Type', "text/plain"),`
		`+ ]`
		`+ if extra_headers:`
		`+ headers.extend(extra_headers)`
		`+ start_response(status, headers)`
		`+ return [response]`
		`+`
		`+`
		`def load_config(environ):`
		`"""Load configuration options`

		`@@ -744,18 +769,12 @@`
		`firstcall = False`
		`# XMLRPC uses POST only. Reject anything else`
		`if environ['REQUEST_METHOD'] != 'POST':`
		`- headers = [`
		`+ extra_headers = [`
		`('Allow', 'POST'),`
		`]`
		`- start_response('405 Method Not Allowed', headers)`
		`response = "Method Not Allowed\n" \`
		`- "This is an XML-RPC server. Only POST requests are accepted."`
		`- response = response.encode()`
		`- headers = [`
		`- ('Content-Length', str(len(response))),`
		`- ('Content-Type', "text/plain"),`
		`- ]`
		`- return [response]`
		`+ "This is an XML-RPC server. Only POST requests are accepted.\n"`
		`+ return error_reply(start_response, '405 Method Not Allowed', response, extra_headers)`
		`if opts.get('ServerOffline'):`
		`return offline_reply(start_response, msg=opts.get("OfflineMessage", None))`
		`# XXX check request length`
		`@@ -776,10 +795,15 @@`
		`except Exception:`
		`return offline_reply(start_response, msg="database outage")`
		`h = ModXMLRPCRequestHandler(registry)`
		`- if environ.get('CONTENT_TYPE') == 'application/octet-stream':`
		`- response = h._wrap_handler(h.handle_upload, environ)`
		`- else:`
		`- response = h._wrap_handler(h.handle_rpc, environ)`
		`+ try:`
		`+ if environ.get('CONTENT_TYPE') == 'application/octet-stream':`
		`+ response = h._wrap_handler(h.handle_upload, environ)`
		`+ else:`
		`+ response = h._wrap_handler(h.handle_rpc, environ)`
		`+ except BadRequest as e:`
		`+ return error_reply(start_response, '400 Bad Request', str(e) + '\n')`
		`+ except RequestTimeout as e:`
		`+ return error_reply(start_response, '408 Request Timeout', str(e) + '\n')`
		`response = response.encode()`
		`headers = [`
		`('Content-Length', str(len(response))),`

koji/server.py

file modified

		`@@ -26,3 +26,11 @@`

		`class ServerRedirect(ServerError):`
		`"""Used to handle redirects"""`
		`+`
		`+`
		`+ class BadRequest(ServerError):`
		`+ """Used to trigger an http 400 error"""`
		`+`
		`+`
		`+ class RequestTimeout(ServerError):`
		`+ """Used to trigger an http 408 error"""`

mikem commented 2 years ago

Fixes: https://pagure.io/koji/issue/3268

mikem commented 2 years ago

When we return an http 400 code instead of a fault, the client will retry the call if it has retries enabled. The only fault that is retried is ServerOffline and that does not seem appropriate here.

1 new commit added

fix indent

2 years ago

1 new commit added

also raise 400 errors when we can't read the client upload stream

2 years ago

mikem commented 2 years ago

Upon further testing, I realized the initial version of this PR did not address upload calls. The new commit extends the change to cover that as well.

This works well for me locally. If I simulate the OSErrors, the client upload code will retry.

Retrying an upload call that may have partially written the chunk might seem iffy, but each successive retry call is writing the same content to the same offset and the fastUpload code will perform an extra verify at the end if there are any retries (and it has always done so).