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

New issue

Closed

opened 2011-11-19 02:38:01 +00:00 by daira · 11 comments

daira commented

2011-11-19 02:38:01 +00:00

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

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

daira added the

labels 2011-11-19 02:38:01 +00:00

daira added this to the 1.11.0 milestone 2011-11-19 02:38:01 +00:00

daira changed title from ~~S3 backend: when a txaws operation gets an error response, include the full request URI and response body in the exception message~~ to 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

2011-11-19 16:30:53 +00:00

daira commented

2012-02-14 20:52:55 +00:00

Author

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 source: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.

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 source: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.

daira self-assigned this 2012-02-14 20:52:55 +00:00

daira commented

2012-02-16 04:13:05 +00:00

Author

Attachment fix-1589.darcs.patch (62005 bytes) added

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

**Attachment** fix-1589.darcs.patch (62005 bytes) added Make S3 error tracebacks include the status code and response, and trigger an incident. fixes #1589

fix-1589.darcs.patch

61 KiB

daira removed their assignment 2012-02-16 04:13:35 +00:00

zancas was assigned by daira

2012-02-16 04:13:35 +00:00

daira commented

2012-02-16 04:19:16 +00:00

Author

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

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

zooko commented

2012-02-17 21:28:25 +00:00

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.

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.

zancas was unassigned by zooko

2012-02-17 21:28:47 +00:00

daira was assigned by zooko

2012-02-17 21:28:47 +00:00

daira commented

2012-02-17 21:59:27 +00:00

Author

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.

Replying to [zooko](/tahoe-lafs/trac/issues/1589#issuecomment-387730): > 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.

daira commented

2012-02-18 06:18:32 +00:00

Author

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 commented

2012-02-20 21:21:28 +00:00

The attached code is related to issues documented in ticket #1590. In particular, that ticket notes the need for more explicit handling/reporting of error information generated by the txaws S3 client's transactions with AWS S3 buckets.

daira commented

2012-02-20 21:40:45 +00:00

Author

Replying to zancas:

The attached code addresses issues documented in ticket #1590.

Actually it only improves the logging related to #1590.

Replying to [zancas](/tahoe-lafs/trac/issues/1589#issuecomment-387735): > The attached code addresses issues documented in ticket #1590. Actually it only improves the logging related to #1590.