S3 backend: when a txaws operation gets an error response, include the full request URI and response body in the exception message, and maybe trigger an incident

[davidsarah]

The exception message shouldn't include secrets (AWS secret key, user token or product token), or data, but the request URI and the body of error responses are short enough to include in full. In particular the response code, which txaws currently drops, would be useful. We don't send any caps in S3 requests.

It would also be useful to trigger an incident for errors that we don't understand or haven't seen before (probably based on the HTTP response code).

[davidsarah]

txaws is preserving the xml_bytes, status, and response in the args of the S3Error object (so we don't need a txaws patch), but they are not being included in the stringified form of the exception (line 58 of txaws/exception.py).

So, we need an errback, probably after each operation in ticket999-S3-backend/src/allmydata/storage/backends/s3/s3_bucket.py, that extracts this information, logs it, and raises another exception that includes it in the error message.

[davidsarah]

Make S3 error tracebacks include the status code and response, and trigger an incident. fixes #1589

[davidsarah]

On line 313 of the patch, the error string should be: "was supposed to raise TahoeS3Error, not get %r"

[zooko]

patch review:

I guess it is redundant but not harmful to say that class S3Bucket implements(IS3Bucket), since it now subclasses S3BucketMixin which implements(IS3Bucket). I would tend to err on the side of removing that declaration from class S3Bucket, but I could go either way.

Why do we need both the f.trap(self.S3Error) and the except self.S3Error? Don't those both mean the same thing -- one for twisted Failure instances and the other for Python Error instances? Don't we know which of those two types we might encounter there?

Other than that, I saw no problems with it.

[davidsarah]

Replying to zooko:

patch review:

I guess it is redundant but not harmful to say that class S3Bucket implements(IS3Bucket), since it now subclasses S3BucketMixin which implements(IS3Bucket).

S3BucketMixin doesn't actually implement S3Bucket (and shouldn't, since it only implements one helper method).

Why do we need both the f.trap(self.S3Error) and the except self.S3Error?

The intention is to preserve the exception traceback. The trap call does so when the exception is not a self.S3Error (because it's preserving the same Failure), and the except clause does so (using the 3-arg form of raise) when it is. The except clause depends on the exception object being an S3Error or MockS3Error. I don't know of any other way to preserve the traceback than to raise and catch f.value.

(I attempted to just change the behaviour of stringification by setting f.value.__str__ rather than using a different exception class, but was stymied by what I consider to be a bug in CPython -- str(x) is equivalent to x.__class__.__str__(x) and not to x.__str__() as the documentation implies, so setting __str__ on an instance does not work.)

The other alternative would be to change AWSError.__str__ in txaws. That might be more elegant, but I would prefer to minimize the changes in our txaws fork.

[davidsarah]

This is ready to push to the ticket999-S3-backend branch, but I'm going to delay that until I also push the patch on #1678.

[zancas]

The attached code addresses issues documented in ticket #1590.

[davidsarah]

Replying to zancas:

The attached code addresses issues documented in ticket #1590.

Actually it only improves the logging related to #1590.

[davidsarah]

This was fixed in [5548/ticket999-S3-backend] and improved most recently in [5647/ticket999-S3-backend].

[warner]

Milestone renamed

[warner]

renaming milestone