-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add aws #11
Conversation
Given that we have versions in jslib, maybe we should merge this under the "better than nothing" v.0.0.1, with only the most necessary changes made? I haven't looked at the actual code, but similar to #20, we can always polish the API and implementation later. |
I was really hoping that someone will take interest and push it through with some more testing ... |
Sorry this issue dropped off my radar a few months ago. I am definitely interested in this, and will try to take a look at it tomorrow. |
Is there a doc somewhere that describes how to use this? I'm more of an SRE-type, and don't spend much time in JS-land. I'm sure I could just vendor the JS files in my branch for testing, but I'll want to know how to do it "right" :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still trying to get to the point where I can test, but I'm close. This is feedback from things I've encountered along the way.
lib/awsv4/core.js
Outdated
options.sessionToken = options.sessionToken || __ENV.AWS_SESSION_TOKEN; | ||
options.protocol = options.protocol || "https"; | ||
options.timestamp = options.timestamp || Date.now(); | ||
options.region = options.region || __ENV.AWS_REGION || "us-east-1"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AWS_DEFAULT_REGION
is also something I have seen in my day. Perhaps
options.region = options.region || __ENV.AWS_REGION || __ENV.AWS_DEFAULT_REGION
I also removed the fallback to us-east-
, because it seems to me that it's better (i.e. less confusing) for the code flow to break here rather than having to reason about why a request that "should have" worked didn't.
lib/awsv4/header.js
Outdated
options.sessionToken = options.sessionToken || __ENV.AWS_SESSION_TOKEN; | ||
options.protocol = options.protocol || "https"; | ||
options.timestamp = options.timestamp || Date.now(); | ||
options.region = options.region || __ENV.AWS_REGION || "us-east-1"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same feedback here as in core.js
lib/awsv4/header.js
Outdated
return options; | ||
} | ||
|
||
function signWithHeaders(method, service, region, target, path="", body=null, query="", headers= {}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
region=""
, so it can be filled byfillOptions
when not suppliedtarget=""
, because it's not always relevant to setX-Amz-Target
header
var options = {headers: headers } | ||
options = fillOptions(options); | ||
|
||
options.headers["X-Amz-Target"] = target; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrap in if (target)
lib/awsv4/header.js
Outdated
options = fillOptions(options); | ||
|
||
options.headers["X-Amz-Target"] = target; | ||
options.headers["Content-Type"] = "application/x-amz-json-1.1"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is always necessary. We send text/csv
content type to many of our Sagemaker endpoints.
Perhaps content type should be left up to the K6 script?
Hi @alanbrent , Sorry for the slow response and thanks for the help! 🎉 Unfortunately, I don't have all that much time to work on this currently (as always, this is why it has been postponed forever) case in point this is the third day I meant to look at this, and once again I am unlikely to have the time to do it. So if you can directly make a PR against my branch (or even directly against master on top of my changes) I think that will speed things up a bit, as I can just review the changes instead of also having to make them ;). And you can use the code you write to both test and then commit to the repo. |
Encapsulate AWS request signing feedback
FYI I actually have NOT been able to successfully test this. I thought I had but then I realized all the requests were failing. I will open another PR when I get a chance to iron it out. I am currently this far:
|
I've been having a play with AWS Signature Version 4 and was able to get it working with a (real) Lambda endpoint using a browserified version of the aws4 library. I also saw the I'm now trying to repeat the process using this
That is resolved by simply adding it to the options object in
The second
Ideally, we'd also generate the necessary (minimum) headers automatically. Below is the eventual working example. Note I had to convert the
What slightly puzzles me here is that I'm not actually using the returned |
I think
signWithHeaders does set the header on the heders object/map you provide. I guess from UX perspective it's probably better to not change the original headers 🤔 |
Continue feedback on AWS signing
This was my original idea in #29 (comment)
as mentioned in #29 (comment)
tried to use current version with
browserify'ed aws4 works fine. I with someone creates |
closed in favor of #46 |
This is very much WIP that is based on:
https://gist.github.com/MStoykov/38cc1293daa9080b11e26053589a6865
Unfortunately, S3 seems to be the one AWS service that is okay with query signing, the rest needs headers signing. Which is what is now ... suppose to be supported ?!?
I tried using localstack to test it, with mixed results - it definitely does check for some of the headers and stuff(probably not in the same way AWS does though), but it definitely doesn't check that I've signed the requests correctly .. as changing credentials still works.
The other ideas were:
https://aws.amazon.com/blogs/developer/mocking-out-then-aws-sdk-for-go-for-unit-testing/
https://www.npmjs.com/package/aws-sdk-mock
But both will probably need an even harder set up to be usable. So I am looking for ideas
Priorities: