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

Update usage of AWS SDK in aws_pca UpstreamAuthority plugin to v2 #2766

Merged
merged 16 commits into from
Jul 7, 2022

Conversation

rturner3
Copy link
Collaborator

@rturner3 rturner3 commented Feb 9, 2022

This is primarily motivated by trying to consolidate to a single AWS SDK dependency in SPIRE.

The max wait time for certificate issuance introduced in this change matches the default behavior used in the v1 SDK.

This is primarily motivated by trying to consolidate to a single AWS SDK
dependency in SPIRE.

The max wait time for certificate issuance introduced in this change matches
the default behavior used in the v1 SDK.

Signed-off-by: Ryan Turner <[email protected]>
Signed-off-by: Ryan Turner <[email protected]>
@rturner3 rturner3 marked this pull request as draft February 9, 2022 03:24
@rturner3
Copy link
Collaborator Author

rturner3 commented Feb 9, 2022

Keeping in draft state until I get a chance to test this in AWS.

}

func newCertificateIssuedWaiter(client acmpca.GetCertificateAPIClient, optFns ...func(*acmpca.CertificateIssuedWaiterOptions)) certificateIssuedWaiter {
return acmpca.NewCertificateIssuedWaiter(client, optFns...)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we shouldn't set our own custom Retryable option until aws/aws-sdk-go-v2#1585 is fixed...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe? It is also something that we need to maintain over time though. I think I'd rather see the fix go into the AWS SDK and pick it up from there so we don't have a Retryable that gets stale and out of sync with AWS API call patterns.

I'm trying to think about the worst that will happen with the existing Retryable behavior. It seems like it will just take longer to fail an operation than is necessary? With an upstream CA, rotation windows should in practice be much larger than this retry window, so we might be technically ok.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would probably be good to get confirmation from the AWS SDK maintainers that the behavior is actually a bug as well. It seems like it is, but I'm not so familiar with these APIs.

@azdagron azdagron added this to the 1.2.2 milestone Mar 3, 2022
@azdagron azdagron self-assigned this Mar 21, 2022
@azdagron azdagron modified the milestones: 1.2.2, 1.3.0 Mar 30, 2022
@evan2645
Copy link
Member

Hey @bcelenza, I hope you are doing well 🤗

We have had a really hard time trying to get access to an AWS account so that we can test these changes (and others). We're nearly out of options, so thought we would ping you (as the original contributor of this plugin) to see if you or anyone you know may be able to help out?

@bcelenza
Copy link
Contributor

Hey @evan2645, absolutely!

I'm no longer with AWS, but can hopefully connect you to the right folks there. Can you send me an email (bcelenza at gmail) from your preferred address?

@evan2645
Copy link
Member

Amazing, thank you @bcelenza! Email incoming

@azdagron azdagron modified the milestones: 1.3.0, 1.3.1 May 2, 2022
@azdagron azdagron modified the milestones: 1.3.1, 1.4.0 Jun 1, 2022
@azdagron azdagron removed their assignment Jun 22, 2022
@rturner3 rturner3 marked this pull request as ready for review July 7, 2022 16:01
@rturner3 rturner3 requested a review from MarcosDY as a code owner July 7, 2022 16:01
Copy link
Member

@azdagron azdagron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good. Just one small comment...

pkg/server/plugin/upstreamauthority/awspca/pca_client.go Outdated Show resolved Hide resolved
Copy link
Member

@azdagron azdagron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @rturner3 !

@rturner3 rturner3 merged commit 12cf52e into spiffe:main Jul 7, 2022
@rturner3 rturner3 deleted the update-aws-sdk-v2-awspca branch September 2, 2022 00:11
stevend-uber pushed a commit to stevend-uber/spire that referenced this pull request Oct 16, 2023
…iffe#2766)

* Update usage of AWS SDK in aws_pca UpstreamAuthority plugin to v2

This is primarily motivated by trying to consolidate to a single AWS SDK
dependency in SPIRE.

The max wait time for certificate issuance introduced in this change matches
the default behavior used in the v1 SDK.

Signed-off-by: Ryan Turner <[email protected]>
Co-authored-by: Marcos Yacob <[email protected]>
stevend-uber pushed a commit to stevend-uber/spire that referenced this pull request Oct 16, 2023
…iffe#2766)

* Update usage of AWS SDK in aws_pca UpstreamAuthority plugin to v2

This is primarily motivated by trying to consolidate to a single AWS SDK
dependency in SPIRE.

The max wait time for certificate issuance introduced in this change matches
the default behavior used in the v1 SDK.

Signed-off-by: Ryan Turner <[email protected]>
Co-authored-by: Marcos Yacob <[email protected]>
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.

5 participants