-
Notifications
You must be signed in to change notification settings - Fork 635
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
New HTTP demo to generate a pre-signed URL to an S3 object file #1901
Conversation
Thank you for this contribution! I will review your PR today and send this out to the team for further review. |
demos/http/http_demo_s3_generate_presigned_url/http_demo_s3_generate_presigned_url.c
Outdated
Show resolved
Hide resolved
demos/http/http_demo_s3_generate_presigned_url/http_demo_s3_generate_presigned_url.c
Outdated
Show resolved
Hide resolved
demos/http/http_demo_s3_generate_presigned_url/http_demo_s3_generate_presigned_url.c
Outdated
Show resolved
Hide resolved
demos/http/http_demo_s3_generate_presigned_url/http_demo_s3_generate_presigned_url.c
Outdated
Show resolved
Hide resolved
demos/http/http_demo_s3_generate_presigned_url/http_demo_s3_generate_presigned_url.c
Outdated
Show resolved
Hide resolved
demos/http/http_demo_s3_generate_presigned_url/http_demo_s3_generate_presigned_url.c
Show resolved
Hide resolved
Do you intend for this demo to only generate the pre-signed URL? Or would you also like it to download the item using the pre-signed URL? |
There is a lot of duplicated code between your demo and the download demo (which you based it off of). Would you mind refactoring some of these functions to a common location like this? |
Thanks for your review @kstribrnAmzn , I will push new commits with the requested changes. |
Thank you so much @giuspen! Looking forward to reviewing when you push the changes. Keeping pre-signed URL creation and download as separate demos works for me. It should be far simpler for a new user to understand. |
I addressed all the points of the review except for the refactoring to share code with the other demo, will do that in one of the next few days... |
@kstribrnAmzn I refactored most of the common code in a shared .h/.c please let me know your thoughts |
I'll take a look later today. Thank you for taking the time to refactor this :) |
On first pass I don't see anything troubling. I will make a more thorough review tomorrow morning with fresh eyes and see if I can fix the failing workflows (or can make suggestions for how you may). Thank you for your hard work on this! Its really awesome to see how the refactor you did shrunk both demos by over 1k lines combined. I'd says that's a huge win :) |
/bot run uncrustify |
Make an HTTPS request to the credentials provider to fetch a security token. You have to supply the following information: | ||
|
||
Certificate and key pair: Because this is an HTTP request over TLS mutual authentication, you have to provide the certificate and the corresponding key pair to your client while making the request. Use the same certificate and key pair that you used during certificate registration with AWS IoT. | ||
RoleAlias: Provide the role alias (in this example, Thermostat-dynamodb-access-role-alias) to be assumed in the request. | ||
ThingName: Provide the thing name that you created earlier in the AWS IoT thing registry database. This is passed as a header with the name, x-amzn-iot-thingname. Note that the thing name is mandatory only if you have thing attributes as policy variables in AWS IoT or IAM policies. |
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.
Should these instructions be there? i'm not seeing how they are used in the step
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.
Correct
/* Initialize the request object. */ | ||
requestInfo.pHost = pHost; | ||
requestInfo.hostLen = hostLen; | ||
requestInfo.pMethod = HTTP_METHOD_GET; | ||
requestInfo.methodLen = sizeof( HTTP_METHOD_GET ) - 1; | ||
requestInfo.pPath = pPath; | ||
requestInfo.pathLen = strlen( pPath ); |
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.
Why set these values to a local variable here when generateS3ObjectFilePresignedURL
sets them to a global?
I've pushed a commit applying some fixes from the workflow failures. I doubt I've got them all so I may be applying a further commit. |
/* Initialize all HTTP Client library API structs to 0. */ | ||
( void ) memset( &requestHeaders, 0, sizeof( requestHeaders ) ); | ||
( void ) memset( &requestInfo, 0, sizeof( requestInfo ) ); | ||
( void ) memset( &response, 0, sizeof( response ) ); | ||
|
||
/* Initialize the request object. */ | ||
requestInfo.pHost = serverHost; | ||
requestInfo.hostLen = serverHostLength; | ||
requestInfo.pMethod = HTTP_METHOD_GET; | ||
requestInfo.methodLen = HTTP_METHOD_GET_LENGTH; | ||
requestInfo.pPath = pPath; | ||
requestInfo.pathLen = strlen( pPath ); | ||
|
||
/* Set "Connection" HTTP header to "keep-alive" so that multiple requests | ||
* can be sent over the same established TCP connection. This is done in | ||
* order to download the file in parts. */ | ||
requestInfo.reqFlags = HTTP_REQUEST_KEEP_ALIVE_FLAG; | ||
|
||
/* Set the buffer used for storing request headers. */ | ||
requestHeaders.pBuffer = userBuffer; | ||
requestHeaders.bufferLen = USER_BUFFER_LENGTH; | ||
|
||
/* Initialize the response object. The same buffer used for storing request | ||
* headers is reused here. */ | ||
response.pBuffer = userBuffer; | ||
response.bufferLen = USER_BUFFER_LENGTH; |
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.
Did a bit of testing and found all of these lines (and their corresponding globals) to be unneeded. Could you remove these?
Thanks for your review @kstribrnAmzn I'm pretty busy for a couple of days but then I'll process your comments! |
@kstribrnAmzn thanks again for your work on my PR, I removed the unneeded code that you spotted and the leftover from copy/paste in the README |
Merged. Thank you for your hard work on this one @giuspen 😄 |
Cool, thanks to you @kstribrnAmzn take care ;) |
@giuspen First of all, thanks for your contribution of 'http_demo_s3_generate_presigned_url'. I had a problem using it:
|
Hey @yinlonglyl my advice is to first have the demo working with your parameters / valid keys and certificate in a linux desktop.
And you get the URL printed out. If you copy and paste the URL in a browser and it works / you can download, then your bug is in porting the code from the demo to your embedded system. |
@giuspen Thanks very much! |
I'm glad you sorted out @yinlonglyl ;) |
New HTTP demo to generate a pre-signed URL to an S3 object file.
The setup is identical to the demo HTTP S3 download and in fact I implemented this based on that demo / starting from a duplication of demos/http/http_demo_s3_download.
By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.