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: support the option to use aws-sdk-v2 #5

Closed
wants to merge 2 commits into from

Conversation

spennymac
Copy link

@spennymac spennymac commented Mar 26, 2024

With AWS announcing in January that the v1 AWS SDK for Go will have its support ending on July 31, 2025, I decide to take a stab at introducing non-breaking change to support the v2 SDK.

I chose to propagate all the AWS SDK options to the client and remove any support for parsing the region from key arn or loading credentials from csv/ini files. All of this extra functionality is still available through the sdk options, refer to AWS's documentation .

Let me know what you think.

@spennymac spennymac marked this pull request as ready for review March 26, 2024 20:14
@spennymac spennymac marked this pull request as draft March 26, 2024 22:05
@spennymac spennymac marked this pull request as ready for review March 27, 2024 02:05
@juergw
Copy link
Contributor

juergw commented Apr 3, 2024

Thanks for looking into implementing this. We certainly should have support for V2.

But this implementation looks overly complicated to me. Would it be better to simply have one new ClientOption "WithKMSV2"? And then require that the user always needs to create these KMS v2 client themselve. Then, we don't need to handle the options of these clients ourselves.

The timeout could be another ClientOption. (But I guess the timeout will not be needed anymore once tink-crypto/tink-go#6 is resolved.)

@spennymac
Copy link
Author

👋 Thank you for your feedback!

But this implementation looks overly complicated to me. Would it be better to simply have one new ClientOption "WithKMSV2"? And then require that the user always needs to create these KMS v2 client themselve. Then, we don't need to handle the options of these clients ourselves.

This is what I initially planned, and then pivoted when I realized the current v1 implementation offered up ways to specify credentials, and we now need the timeout. The implementation tries its best to separate all options, ensuring that the code is not burdened with the responsibility to constantly check whether a v1-specific option has been already been set, it only needs to check this one place.

Removing the load and kms options is a easy change, which leads us to the timeout 🤔

The timeout could be another ClientOption. (But I guess the timeout will not be needed anymore once tink-crypto/tink-go#6 is resolved.)

Would this timeout now apply to the v1-sdk as well, since it would become a 'v1 option'? If so, we would have to change the v1 Encrypt/Decrypt calls to use their WithContext variant and make sure we aren't changing any current timeout behavior. Or add new branches to use WithContext if the timeout is set. If not, I'm not sure of a good way to let users know this is only valid for v2, simply naming it with some v2 prefix/suffix?

Finally, would you say a v2 implementation would be blocked until [tink-crypto/tink-go#6] is complete?

@spennymac
Copy link
Author

Would this be more inline with what you are expecting? https://github.com/spennymac/tink-go-awskms/pull/2/files

@juergw
Copy link
Contributor

juergw commented Apr 8, 2024

Yes. But I think it should be even simpler. I don't think we want to add the timeout, because I think that will not be needed anymore once we add the context directly to the remote encrypt/decrypt calls, which we will add soon. I think the additional API really should just be a WithV2KMS option. And I don't think we need the "Aead builder", I'd prefer to not refactor the code, just add the new KMS and don't change the old code path.

@spennymac
Copy link
Author

spennymac commented Apr 10, 2024

Yes. But I think it should be even simpler. I don't think we want to add the timeout, because I think that will not be needed anymore once we add the context directly to the remote encrypt/decrypt calls, which we will add soon. I think the additional API really should just be a WithV2KMS option. And I don't think we need the "Aead builder", I'd prefer to not refactor the code, just add the new KMS and don't change the old code path.

Hmm.. alright. Seeing as how the timeout is needed, I'm not sure how best to do this. I'll hold off until the context change comes through, and then it wont be needed.

@tholenst tholenst requested a review from juergw April 17, 2024 12:34
@spennymac spennymac closed this Jul 17, 2024
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