Added X-Ray support to s3crypto #2783
Conversation
alexbilbie
force-pushed
the
s3crypto-xray
branch
from
c8421c8
to
ab18bad
Compare
|
It looks like a few of the methods in the PR had their signature changed, adding |
| @@ -9,6 +9,7 @@ import ( | |||
| "github.com/aws/aws-sdk-go/service/kms" | |||
| "github.com/aws/aws-sdk-go/service/s3" | |||
| "github.com/aws/aws-sdk-go/service/s3/s3iface" | |||
| "golang.org/x/net/context" | |||
There was a problem hiding this comment.
Instead of importing golang.org/x/net/context The SDK should use aws.BackgroundContext() instead of context.Background()
| @@ -84,7 +85,7 @@ func NewDecryptionClient(prov client.ConfigProvider, options ...func(*Decryption | |||
| // Bucket: aws.String("testBucket"), | |||
| // }) | |||
| // err := req.Send() | |||
| func (c *DecryptionClient) GetObjectRequest(input *s3.GetObjectInput) (*request.Request, *s3.GetObjectOutput) { | |||
| func (c *DecryptionClient) GetObjectRequest(ctx aws.Context, input *s3.GetObjectInput) (*request.Request, *s3.GetObjectOutput) { | |||
There was a problem hiding this comment.
Adding Context to this method would be a breaking change.
I think we need to investigate how to refactor the Unmarshal handler. I think the c.contentCipherFromEnvelope should have access to the request.Request's Context method. This will prevent updating the GetObjectRequest's method signature.
| @@ -128,7 +129,7 @@ func (c *DecryptionClient) GetObject(input *s3.GetObjectInput) (*s3.GetObjectOut | |||
| // cause a panic. Use the Context to add deadlining, timeouts, etc. In the future | |||
| // this may create sub-contexts for individual underlying requests. | |||
| func (c *DecryptionClient) GetObjectWithContext(ctx aws.Context, input *s3.GetObjectInput, opts ...request.Option) (*s3.GetObjectOutput, error) { | |||
| req, out := c.GetObjectRequest(input) | |||
| req, out := c.GetObjectRequest(ctx, input) | |||
| req.SetContext(ctx) | |||
There was a problem hiding this comment.
With the above update to GetObjectRequest's Unmarshal handler adding the ctx to this method shouldn't be needed because the SetContext on the request will provide the value correctly.
| @@ -68,7 +69,7 @@ func NewEncryptionClient(prov client.ConfigProvider, builder ContentCipherBuilde | |||
| // Body: strings.NewReader("test data"), | |||
| // }) | |||
| // err := req.Send() | |||
| func (c *EncryptionClient) PutObjectRequest(input *s3.PutObjectInput) (*request.Request, *s3.PutObjectOutput) { | |||
| func (c *EncryptionClient) PutObjectRequest(ctx aws.Context, input *s3.PutObjectInput) (*request.Request, *s3.PutObjectOutput) { | |||
There was a problem hiding this comment.
Simpliar to GetObjectRequest above, I think the usage of the Context needs to happen with in a build handlers so the Context can be extracted from the Request.
| } | ||
|
|
||
| // CipherDataDecrypter is a handler to decrypt keys from the envelope. | ||
| type CipherDataDecrypter interface { | ||
| DecryptKey([]byte) ([]byte, error) | ||
| DecryptKey(aws.Context, []byte) ([]byte, error) |
There was a problem hiding this comment.
Updating these would be a breaking change. We should look at ways of supporting Context optionally.
|
I've closed this PR in favour of #2912 |
The s3crypto package was not passing a context object down to the KMS client calls so panics were being emitted by the X-Ray SDK.
The KMS client now calls
DecryptWithContextandGenerateDataKeyWithContext.This PR fixes with the absolute minimum number of changes I could make.
To support Go 1.5 and 1.6 which doesn't have a native
contextpackage I importedgolang.org/x/net/contextintodecryption_client.goandencryption_client.go- if there is a preferred solution please let me know.