GODRIVER-1973 create internal copy of aws v4 signing code #657
Conversation
iwysiu
requested review from
kevinAlbs,
divjotarora,
matthewdale and
benjirewis
| // Based on github.com/aws/aws-sdk-go by Amazon.com, Inc. | ||
| // See THIRD-PARTY-NOTICES for original license terms | ||
|
|
||
| package awsv4 |
There was a problem hiding this comment.
I noticed you decided to simplify the aws/signer/v4 API while copying it into this package. What tradeoffs did you consider when making that decision?
(I think reducing the API surface area to only what is needed is a good idea overall, I'm just curious about your decision process.)
Also, for all the Go files in the awsv4 package, consider adding a link to the Go file in the AWS SDK that it's based on (permalink, with the commit). That will make some questions later easier to answer, like "What version of the Go AWS SDK is this based on?" and "What file should I look at to update the code in case I have to later?"
There was a problem hiding this comment.
The main things I was considering in terms of being pro-reduction was that removing options and such that we didn't use would reduced the package dependencies in addition to the surface area and in terms of being anti-reduction, the more i change it the harder it gets to update later and i could introduce errors. Ultimately I decided that the signature calculation code is unlikely to change, and since the calculation we use it for is the same every time, the tests should be sufficient to catch any errors.
Done. I used links to the version tag rather than the commit because it seemed neater.
There was a problem hiding this comment.
Thank you for the explanation here. That is a good trade-off to consider.
The more code changed in vendoring means the more difficult it will be in upgrading later.
FWIW drivers that have written their own AWS signature v4 implementation (C and Rust) will need to update if anything were to ever change. I imagine that is more work than updating this vendored version.
| // a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Based on github.com/aws/aws-sdk-go by Amazon.com, Inc. with code from: | ||
| // - github.com/aws/aws-sdk-go/blob/v1.34.28/aws/credentials/static_provider.go |
There was a problem hiding this comment.
To check, has anything significant changed in 1.34.28 to the latest stable release (looks like 1.38.37) in the files we are vendoring?
There was a problem hiding this comment.
nothing changed in any code that we use
| // Based on github.com/aws/aws-sdk-go by Amazon.com, Inc. | ||
| // See THIRD-PARTY-NOTICES for original license terms | ||
|
|
||
| package awsv4 |
There was a problem hiding this comment.
Thank you for the explanation here. That is a good trade-off to consider.
The more code changed in vendoring means the more difficult it will be in upgrading later.
FWIW drivers that have written their own AWS signature v4 implementation (C and Rust) will need to update if anything were to ever change. I imagine that is more work than updating this vendored version.
| // a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Based on github.com/aws/aws-sdk-go by Amazon.com, Inc. | ||
| // See THIRD-PARTY-NOTICES for original license terms |
There was a problem hiding this comment.
FWIW as some additional prior art, @jyemin also pointed me to file attributions for vendored libraries in the Java driver: https://github.com/mongodb/mongo-java-driver/blob/master/driver-core/src/main/com/mongodb/internal/connection/tlschannel/BufferAllocator.java#L1-L18. It looks very similar.
* 'master' of https://github.com/mongodb/mongo-go-driver: (39 commits) GODRIVER-2004 Add Versioned API connection examples for Docs (mongodb#665) GODRIVER-1961 Run OCSP tests against RHEL 7.0 (mongodb#664) GODRIVER-1844 finer precision for getSecondsSinceEpoch (mongodb#666) GODRIVER-1973 create internal copy of aws v4 signing code (mongodb#657) GODRIVER-1951 Update the Go version for Evergreen builds to 1.16 (mongodb#663) GODRIVER-1949 add more ignored killAllSessions errors for unified tes… (mongodb#658) GODRIVER-1963 remove dropDatabase result (mongodb#660) GODRIVER-1180 Remove legacy transform functions from mongo (mongodb#583) GODRIVER-1937 Update legacy ListCollections to support the BatchSize option for server version 2.6 (mongodb#656) GODRIVER-1933 remove xtrace from shell scripts (mongodb#661) fix README error handling of FindOne (mongodb#636) GODRIVER-1938 update mongocryptd serverSelectionTimeout to 10 seconds (mongodb#659) GODRIVER-1925 Surface cursor errors in DownloadStream fillBuffer (mongodb#653) GODRIVER-1955 create labeledError interface (mongodb#651) GODRIVER-1947 Unmarshal unicode surrogate pairs correctly in UnmarshalExtJSON. (mongodb#649) Changed order of actions in ObjectIDFromHex func (mongodb#637) GODRIVER-1750 Ensure contexts are always cancelled during server monitoring (mongodb#654) GODRIVER-1931 Sync new cursors and SDAM LB tests (mongodb#655) GODRIVER-1981 Sync new transactions tests (mongodb#652) GODRIVER-1931 Run tests against LBs in Evergreen (mongodb#648) ...
The SDK is pulled due to go.mongodb.org/mongo-driver. Luckily mongo-driver has removed the SDK in mongodb/mongo-go-driver#657. So we can remove the SDK by just upgrading go.mongodb.org/mongo-driver. Signed-off-by: Kazuyoshi Kato <[email protected]>
So I was able to reduce it by a lot! There are some changes I wasn't sure about, for example I left a lot of the functions I took from the amazon sdk public, since it's in internal and I didn't want to change too much, but I can make those private if we want.
I did have to make edits when I took the files, because there were a lot of inter-package dependencies. Most of the files in awsv4 are named after the package that the code in it comes from.
Also I think I credited them correctly, but please tell me if I got it wrong.