Log S3 API error message

Background: When the S3 API returns for example code 403, it is hard to
guess what went wrong without looking at the more verbose message, as
seen for example in #29912.

When S3 API returns an error code in the 400 or 500 range, put message
contents into debug log for easier troubleshooting.

Requests doesn't raise HTTPError automatically - for that
raise_for_status() is commonly used. Therefore the try clause never
caught any exceptions.

This change replaces the never activated except block with a response
code check, as seen in utils/aws.py

Q&A:
Should we raise an error instead? Potentially Salt will generate a big
amount of these log lines, for example when updating every single states
file from S3. It is less scary to put it in debug log.

Why did we lose the "return False"? It is inconsistent with the error
handling below where return value does not matter.

Why aren't we alerting the user that something went wrong? That requires
a go-through through the whole s3.py to see which log.debug() should
really be log.error() and is left for a future change.
This commit is contained in:
Stanislav Blokhin 2016-01-02 22:40:18 +01:00
parent 941bcaed07
commit 8c4a101c8f

View file

@ -119,17 +119,15 @@ def query(key, keyid, method='GET', params=None, headers=None,
if not data:
data = None
try:
result = requests.request(method, requesturl, headers=headers,
data=data,
verify=verify_ssl)
response = result.content
except requests.exceptions.HTTPError as exc:
log.error('Failed to {0} {1}::'.format(method, requesturl))
log.error(' Exception: {0}'.format(exc))
if exc.response:
log.error(' Response content: {0}'.format(exc.response.content))
return False
result = requests.request(method,
requesturl,
headers=headers,
data=data,
verify=verify_ssl)
response = result.content
if result.status_code >= 400:
# On error the S3 API response should contain error message
log.debug(' Response content: {0}'.format(response))
log.debug('S3 Response Status Code: {0}'.format(result.status_code))