Skip to content
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

feat: migrate to aws-sdk-go-v2 #186

Merged
merged 7 commits into from
Nov 22, 2023
Merged

feat: migrate to aws-sdk-go-v2 #186

merged 7 commits into from
Nov 22, 2023

Conversation

Bernix01
Copy link
Contributor

@Bernix01 Bernix01 commented Nov 6, 2023

Description

Hello :D

Problem

  • AWS SDK automatically detects credentials besides static ones. Right now it is manually forcing static credentials.
  • By default https is disabled which is required to be enabled under some circumstances (compliance)

Solution

  • Bump AWS SDK to version 2
  • Conditionally create config based on available parameters
  • Add a new s3-disabble-https config variable

Notes

This change is meant to be retro-compatible and to not require changes on the end user.

@Ivanello
Copy link

Ivanello commented Nov 7, 2023

thanks for resolving issue
waiting for merge

@dbarrosop
Copy link
Member

I don't think this PR is correct. Did you test it?

@Bernix01
Copy link
Contributor Author

Bernix01 commented Nov 8, 2023

Locally it doesn't throw compile errors and aws sdk documentation states that it will detect if not provided, maybe a new test case needs to be added for this?

@dbarrosop
Copy link
Member

I don't think a test is necessarily needed but I don't think the go SDK works like this. So if you could test in your environment and let me know where you read in the go SDK documentation this is correct that'd be very helpful. Also, if you are going to use the environment to configure the AWS I think you should configure everything, including region and endpoint.

@Bernix01 Bernix01 changed the title feat: allow aws credentials to be detected feat: bump aws-sdk-go Nov 9, 2023
@Bernix01
Copy link
Contributor Author

Bernix01 commented Nov 9, 2023

image Some local evidence of POC (using AWS SSO)

storage/s3.go Outdated Show resolved Hide resolved
storage/s3.go Outdated Show resolved Hide resolved
storage/s3_test.go Outdated Show resolved Hide resolved
storage/s3_test.go Outdated Show resolved Hide resolved
@dbarrosop
Copy link
Member

dbarrosop commented Nov 9, 2023

Ok, this looks more like what I expected, thanks a lot for the PR. I left a few minor comments, let me know when you address them so I can take a look again and do a few tests myself. You will also need to rebase the PR to fix the issue being raised by the CI (bumping go version due to a CVE).

@Bernix01
Copy link
Contributor Author

Bernix01 commented Nov 9, 2023

It's ready @dbarrosop

@dbarrosop
Copy link
Member

Good job, unfortunately, looks like tests aren't succeeding. I think you may have forgotten to update storage/s3_test.go.

@Bernix01
Copy link
Contributor Author

now it should be good 👍

@dbarrosop
Copy link
Member

Thanks, I will try to find time this week to do some thorough testing on a demo project

@dbarrosop dbarrosop self-assigned this Nov 14, 2023
@Bernix01
Copy link
Contributor Author

Hello, pinging to see if there's any news an about the testing on your end, this is an important feature for our company and we are looking forward for this to be released.

@dbarrosop
Copy link
Member

dbarrosop commented Nov 21, 2023

Yes, unfortunately presigned URLs weren't working but I fixed that issue. If you give me permissions to push to your fork I can push my fix to your branch.

Let me know when it's done. Thanks.

@dbarrosop dbarrosop changed the title feat: bump aws-sdk-go feat: migrate to aws-sdk-go-v2 Nov 22, 2023
@dbarrosop dbarrosop merged commit 51ccc70 into nhost:main Nov 22, 2023
3 checks passed
@dbarrosop
Copy link
Member

dbarrosop commented Nov 22, 2023

Thanks! Releasing v0.5.0 with this change.

@dbarrosop dbarrosop deleted the patch-1 branch November 22, 2023 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants