Closed Anurag252 closed 2 months ago
This is quite a lot of new code and I am concerned about supporting this.
@szuecs should we also consider
* adding AWS sdk as a dependency * copy-paste AWS signer code (vendor-in) * use AWS sdk with a build tag to exclude for lib users by default
Good question, I thought it makes sense to not put AWS SDK into dependency because this is really huge. The contributor copies basically code (option 2) that won't be able to be changed anyways into the repository, because AWS can't really change the SDK signing code, because if so they likely need to change validation, too and then you would break half the internet that uses AWS SDK to have to update.
Adding an AWS build tag would be something we can do to have this feature as a more "experimental" implementation.
Added review comments except the unresolved open discussions.
will add changes for these once I have more information
@Anurag252 I tried to answer all comments that seem to have an open question. If there is something missing please point us to it.
@Anurag252 I tried to answer all comments that seem to have an open question. If there is something missing please point us to it.
Thanks @szuecs . A comment about keeping/removing caching layer is still remaining. That was copied from sdk code and I do not have any opinion on keeping or removing it. I am open to both.
Can you also suggest next steps, as PR appears feature complete to me ( if test
and check-race
passes)?
re-requesting review. Added fixes
disableURIPathEscaping
disableSessionToken
behaviour I didn't check but please check the CodeQL warning. I think we should fix it.
I didn't check but please check the CodeQL warning. I think we should fix it.
removed logging, so CodeQL warning should go away
@szuecs @AlexanderYastrebov This PR is feature complete. Could we approve the workflow ? If there are any more issues in workflow, I can take a look at them.
From my side looks good, only some godoc related errors
:+1:
:+1:
https://github.com/zalando/skipper/issues/2911
Example 1 with default body {}
actual sdk code
see https://go.dev/play/p/nPiTzwAl6Ut for a signed sample request with sdk. response : -
skipper
./bin/skipper -inline-routes='r: * -> status(200) -> sigv4( "us-east-1","dynamodb", "false", "false", "false") -> "http://httpbin.org"'
curl -
Response from curl -
Example 2 with non-default body
skipper
curl
actual sdk code
https://go.dev/play/p/Ixx-ozxeWfv Response