Add acmpa ✨ #201

weitzj · 2021-07-13T20:33:31Z

This PR adds support to delete AWS ACM Private CAs (acmpca).

This needs a bit more testing.

Fixes #200

weitzjdevk · 2021-07-14T14:17:58Z

@brikis98 @yorinasub17 The tests work, and this PR is good to review (I cannot update the description or title)

yorinasub17

Thanks for your contribution! I had a few comments, most of them nits from defensive coding.

The biggest question mark for me prior to merging this in is how much it will cost to run the automated tests. Depending on that answer, we will need to come up with some strategy to limit the costs for us.

yorinasub17 · 2021-07-14T18:29:46Z

aws/acmpca.go

+	result, err := svc.ListCertificateAuthorities(&acmpca.ListCertificateAuthoritiesInput{})
+	if err != nil {
+		return nil, errors.WithStackTrace(err)
+	}


This should handle pagination.

yorinasub17 · 2021-07-14T18:31:15Z

aws/acmpca.go

+	var arns []*string
+	for _, ca := range result.CertificateAuthorities {
+		// one can only delete CAs if they are 'ACTIVE' or 'DISABLED'
+		isCandidateForDeletion := *ca.Status == acmpca.CertificateAuthorityStatusActive || *ca.Status == acmpca.CertificateAuthorityStatusDisabled


Use aws.StringValue to safe dereference pointers.

yorinasub17 · 2021-07-14T18:32:19Z

aws/acmpca.go

+		if !isCandidateForDeletion {
+			continue
+		}
+		if excludeAfter.After(*ca.CreatedAt) {


Use aws.TimeValue to safe dereference the pointer.

yorinasub17 · 2021-07-14T18:33:49Z

aws/acmpca.go

+)
+
+// getAllACMPCA returns a list of all arns of ACMPCA, which can be deleted.
+func getAllACMPCA(session *session.Session, region string, excludeAfter time.Time) ([]*string, error) {


It looks like this isn't implementing the config file based name filter using regexes. You need to plumb the config file through to here so that it properly filters out those that don't match the regex.

yorinasub17 · 2021-07-14T18:35:52Z

aws/acmpca.go

+	for _, arn := range arns {
+		logging.Logger.Infof("Setting status to 'DISABLED' for ACMPCA %s in region %s", *arn, *session.Config.Region)
+		if _, updateStatusErr := svc.UpdateCertificateAuthority(&acmpca.UpdateCertificateAuthorityInput{
+			CertificateAuthorityArn: arn,
+			Status:                  aws.String(acmpca.CertificateAuthorityStatusDisabled),
+		}); updateStatusErr != nil {
+			logging.Logger.Errorf("[Failed] %s", updateStatusErr)
+			continue
+		}
+		logging.Logger.Infof("Did set status to 'DISABLED' for ACMPCA: %s in region %s", *arn, *session.Config.Region)
+
+		if _, deleteErr := svc.DeleteCertificateAuthority(&acmpca.DeleteCertificateAuthorityInput{
+			CertificateAuthorityArn: arn,
+			// the range is 7 to 30 days.
+			// since cloud-nuke should not be used in production,
+			// we assume that the minimum (7 days) is fine.
+			PermanentDeletionTimeInDays: aws.Int64(7),
+		}); deleteErr != nil {
+			logging.Logger.Errorf("[Failed] %s", deleteErr)
+			continue
+		}
+		deletedARNs = append(deletedARNs, arn)
+		logging.Logger.Infof("Deleted ACMPCA: %s", *arn)
+	}


Since this is using single API calls to delete each one, this should be refactored to use goroutines so that multiple requests can be made concurrently. See the secrets manager deletion routine for reference: https://github.com/gruntwork-io/cloud-nuke/blob/master/aws/secrets_manager.go#L70-L89

yorinasub17 · 2021-07-14T18:39:56Z

aws/acmpca_test.go

+)
+
+// createTestACMPCA will create am ACMPCA and return its ARN.
+func createTestACMPCA(t *testing.T, session *session.Session, name string) *string {


Sanity check: Do you have a sense on how much it would cost to run this test? It appears that the cost of ACM Private CA is $400 / month, pro rated to the number of X (days or hours?) that it is active. Does that active period include the days it is waiting to be deleted (the 7 day minimum)? If so, that can rack up quickly for us if we are running cloud nuke tests for each PR.

Depending on how much it costs, can you add an environment variable flag so that we can control when we run the tests related to the ACM PCA functionality (and not on every PR)?

I am currently talking to our AWS Enterprise contact as indeed their documentation and support is not clear on this and racked up a significant amount of money just for starting/stopping a CA.

I will add some environment variable and this test will be opt-in in all cases so you have to make a concious decision to run it.

We heard back from our contact. The billing is pro-rated and does reconciliation after 24-48 hours and you only pay for those hours.

Hypothesis from Support-Agent

Hi Jan
It looks like the billing is fluctuating due to the 24-48 hour delay between the billing console and reality. When a certificate is created, the full amount for the month is added to the bill and this only changes to the pro-rated amount after the certificate has been deleted. I'll keep the internal ticket open with the service team just to confirm that this is exactly what is going on. In terms of your other questions, here are the answers:

How does AWS want to do pricing for short-lived CAs? (i.e. can we enable a testing pipeline or not)
->>>> I will forward this request to the service team and I'll update you as soon as I hear back from them.

Is there an absolute upper service quota for CAs in an account? (I see 200 quoted right now)
->>>> The quota of 200 is the default, and we can request an increase should you require it, but we would need a detailed use case for this request.

Do deleted CAs (with their 7 - 30 day pending period) count towards the service quota?
->>>> Yes, deleted Private CAs count towards the quota until the end of the restoration period.

Answer from AWS PCA Service Team confirming hypothesis from Support-Agent above

Quote

Thank you for your patience. I've just heard back from the team and my original answer about the delay between the billing console and reality is correct. Because the invoice for each month is issued a few days after the end of the month, by the time the bill is issued, everything will have refreshed and the bill will reflect correctly. For the current month, the "real" bill so far is around $7.

For some more context on how we charge for short term PCAs, the team provided the following:

AWS PCA charge customer based on the time period: from the time that CA was created to the time CA was deleted.

Facts:

Customer created 22 CAs in FRA region in July. 1 of them are in free trial.
Customer create CA, and delete them in a short period of time. --> this is fine, nothing wrong.

Customer check bill of July, which is not the final statement. There are delays between customer deleted the CA and the >billing team calculate the bill based on latest data.

If customer check right now, the bill will become about $168.

When customer finally get bill in August, the total charge will be around $8. Unless they created new CAs

Considering this information, the best way to monitor the bill for PCA specifically would be to log the hours and calculate this manually on your side throughout the month.

So how do we think about the pricing here?

If we have this test case and it runs, say, nightly for a month, what sort of fee are we talking? What if we get 20 PRs in this repo, and each one does 20 test runs in a month?

I just want to be extra sure we don't end up with a massive AWS bill as a surprise from adding this new functionality!

@brikis98 I think we can go with this for now. @weitzjdevk implemented a feature flag to disable the tests by default. We can merge with that disabled.

Separately, I'll run an experiment to enable that test locally once. Then, we can check how much our bill was for August and make our decision on whether to continually test it or not based on that bill cost. Does that seem reasonable?

If we have this test case and it runs, say, nightly for a month, what sort of fee are we talking? What if we get 20 PRs in this repo, and each one does 20 test runs in a month?

tl;dr: Price per run approx: $0.60 (if you issue a certificate + $0.75 therefore approx $1.50)
So the rough calculation how this works.

Monthly charge (for CA only): $400
Month has hours: 720

Price (CA) per hour approx. 400/720 approx $ 0.6
Issue a certificate charge: $0.75

Therefore roughly:

Price per run $0.60 + $0.75 approx. $1.50

Timeline (when starting the first day of the month)

Day1 (first of the month): $0

Run it

Delete it

Day1 bill: $0

Day2 bill: $400 + 0.75

Day3 bill: $400 + 0.75

Day4 bill: $400/720 + 0.75

When you do the run in the middle of the month

Day15 (middle of the month): $0

Run it

Delete it

Day15 bill: $0

Day16 bill (middle of month. Only 15 more days for pro-rating): $400 * 15 / 30 + 0.75 = $200 + 0.75

Day17 bill: $200 + 0.75

Day18 bill: $400/720 + 0.75

I have confirmed this cost in our account as well. I think 0.60 per run is a tad bit expensive to be continuously running the tests, but selectively running it everytime this feature changes seems reasonable, so the env var based feature switch is satisfactory.

weitzjdevk · 2021-07-14T22:28:21Z

Could you please have another look. I have addressed your comments in separate commits.

This adds support to delete AWS ACM Private CAs. Fixes gruntwork-io#200

This also refactors the logic for calculating excludeTime.

weitzjdevk · 2021-07-29T09:24:08Z

Please review, merge.

weitzjdevk · 2021-07-29T09:25:00Z

@yorinasub17 , @brikis98 I have rebased against the current master. The review comments are incorportated. See the answer from AWS Support how billing works.

weitzj · 2021-08-02T18:23:44Z

Any update on this?

brikis98 · 2021-08-03T16:00:53Z

@weitzj Sorry for the delay. We are very buried at the moment. Since we don't have a ton of customers using ACM PA, reviewing this is getting deprioritized in favor of things that many customers are using and requesting. We'll get to it as soon as we can.

yorinasub17

Updates LGTM except for the one nit about the name regex. Can you address that?

In the meantime, I'll go ahead and kick off a build in CI (without enabling ACM), and locally (with enabling ACM). That can give us a sense of how much the ACMPA costs for each test run, and we can use that information to base whether or not we can permanently enable the tests.

yorinasub17 · 2021-08-04T15:40:57Z

aws/acmpca.go

+		aws.StringValue(ca.Arn),
+		configObj.ACMPCA.IncludeRule.NamesRegExp,
+		configObj.ACMPCA.ExcludeRule.NamesRegExp,


This is checking the names regex against the ARN, which is unintuitive. Is there a way to just get the name out?

Yeah. I will think about it. You are correct that this does not feel good.
It should be something unique. And the name is not unique.
What is your plan about the regex behaviour for other resources? Is this "the final form"? Otherwise I would get rid of the regex in total.

I would need your guidance on this. Indeed this would be the ARN and there is nothing better than using the ARN for it. Name does not make much sense for this service.

I could introduce new types?

type Config struct { .... ACMPCA ResourceTypeARN `yaml:"acmcpa"` } type ResourceTypeARN struct { IncludeRule FilterRuleARN `yaml:"include"` ExcludeRule FilterRuleARN `yaml:"exclude"` } type FilterRuleARN struct { ARNsRegExp []Expression `yaml:"arns_regex"` }

Or I could do:

type FilterRule struct { NamesRegExp []Expression `yaml:"names_regex"` ARNsRegExp []Expression `yaml:"names_regex"` }

What is your plan about the regex behaviour for other resources? Is this "the final form"? Otherwise I would get rid of the regex in total.

This is not the final form, as we plan on supporting tags (something users have requested for other resources). Adding in ARN support or tag support just for ACM PCA makes sense to me, although that would be a lot more work to add considering the potential regressions with other resources.

If it isn't critical for your environment, I might suggest removing the regex filters for now and add that in later as a future feature when we support something that makes more sense, like ARN or tags.

Alright. I have removed the configObj support (see the latest commit)

yorinasub17 · 2021-08-04T15:51:34Z

aws/acmpca.go

+	logging.Logger.Infof("Setting status to 'DISABLED' for ACMPCA %s in region %s", *arn, region)
+	if _, updateStatusErr := svc.UpdateCertificateAuthority(&acmpca.UpdateCertificateAuthorityInput{
+		CertificateAuthorityArn: arn,
+		Status:                  aws.String(acmpca.CertificateAuthorityStatusDisabled),
+	}); updateStatusErr != nil {
+		errChan <- updateStatusErr
+		return
+	}
+	logging.Logger.Infof("Did set status to 'DISABLED' for ACMPCA: %s in region %s", *arn, region)


Is this step necessary? I just tried the test and it failed with:

=== RUN TestListACMPCA === PAUSE TestListACMPCA === CONT TestListACMPCA [cloud-nuke] time="2021-08-04T10:47:49-05:00" level=info msg="Random region chosen: eu-north-1" acmpca_test.go:93: Error Trace: acmpca_test.go:93 Error: []string{} does not contain "arn:aws:acm-pca:eu-north-1:738755648600:certificate-authority/3cb99176-55e7-427a-9fd7-2a0208d12c98" Test: TestListACMPCA [cloud-nuke] time="2021-08-04T10:47:50-05:00" level=info msg="Deleting all ACMPCA in region eu-north-1" [cloud-nuke] time="2021-08-04T10:47:50-05:00" level=info msg="Setting status to 'DISABLED' for ACMPCA arn:aws:acm-pca:eu-north-1:738755648600:certificate-authority/3cb99176-55e7-427a-9fd7-2a0208d12c98 in region eu-north-1" [cloud-nuke] time="2021-08-04T10:47:51-05:00" level=error msg="[Failed] InvalidStateException: The certificate authority must be in the ACTIVE or DISABLED state to be updated" --- FAIL: TestListACMPCA (2.36s)

The PCA was in state Pending Validation. However, I was able to delete it just fine using the UI, which suggests that it may not be necessary to disable the PCA prior to deleting it.

Oh. Sorry. I will make the listing more smart and test it

See the latest commits. Now the tests work as expected. I had to fix a race-condition, where the CA was in a "CREATING" state during test, which one could not see during the test.
Now the "createCA" command waits with retry until the status is the one we need to actually test.

This is fixed here.

Just a side-note how the terraform-provider-aws does it:

https://github.com/hashicorp/terraform-provider-aws/blob/main/aws/resource_aws_acmpca_certificate_authority.go#L322

https://github.com/hashicorp/terraform-provider-aws/blob/73484854207e4459a25a9b2a0da92d5963ceed59/aws/internal/service/acmpca/waiter/waiter.go#L13-L15

weitzjdevk · 2021-08-04T17:42:10Z

Everything is fixed (see the last commits).
The tests now actually work as expected :)
Just tell me if you want me to throw out the Regex stuff or how you want to have it.

weitzjdevk · 2021-08-06T08:58:16Z

@yorinasub17 I think everything is fine now. See the latest commits, where I have removed the configObj support. Also the last commit fixes some goimports -w .

Do you want me to rebase? squash-merge? Do you do this?

weitzj · 2021-08-17T11:15:05Z

@yorinasub17 Anything missing? Can we get this merged? Did you check your AWS Bill?

yorinasub17 · 2021-08-17T13:12:36Z

@weitzj Apologies for the delay here. I've been super swamped with other priorities and haven't been able to come back to this. I hope to get back to this later this week, or next week.

yorinasub17

Updates LGTM, the tests passed locally! The build failed, but I confirmed it was a fluke, so going to go ahead and merge + release this. Thanks for your patience in the review process!

weitzj requested review from brikis98 and yorinasub17 as code owners July 13, 2021 20:33

weitzj force-pushed the fix/200_aws_pca branch 6 times, most recently from a9444c2 to d9566af Compare July 14, 2021 14:15

yorinasub17 changed the title ~~WIP: Add acmpa ✨~~ Add acmpa ✨ Jul 14, 2021

yorinasub17 reviewed Jul 14, 2021

View reviewed changes

weitzj force-pushed the fix/200_aws_pca branch from 50b4e35 to ac2c7f9 Compare July 14, 2021 22:40

weitzjdevk added 7 commits July 29, 2021 11:23

Add acmpa ✨

5287cde

This adds support to delete AWS ACM Private CAs. Fixes gruntwork-io#200

Safely dereference the aws ca status 🔧

1ee809e

Safely dereference the aws time 🔧

92e6245

Add pagination 🔧

4209721

Add support to concurrently delete ACMPCA 🔧

8f9b7df

Implement configObj for acmpca 🔧

d678810

This also refactors the logic for calculating excludeTime.

Add opt-in feature for acmpca tests 🔧

542ee79

weitzj force-pushed the fix/200_aws_pca branch from ac2c7f9 to 542ee79 Compare July 29, 2021 09:23

yorinasub17 reviewed Aug 4, 2021

View reviewed changes

weitzjdevk added 3 commits August 4, 2021 19:27

Fix race-condition for CA to be ready in test

0a6016a

Fix deletion behaviour selector

2f018d9

Fix marking for deletion, fix waitGroup 🐛

f0b78f6

weitzjdevk added 2 commits August 6, 2021 10:53

Remove configObj support

97d6cac

gimports -w .

f028ccd

weitzj requested a review from yorinasub17 August 17, 2021 11:15

yorinasub17 approved these changes Aug 24, 2021

View reviewed changes

yorinasub17 merged commit 7c25147 into gruntwork-io:master Aug 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add acmpa ✨ #201

Add acmpa ✨ #201

weitzj commented Jul 13, 2021

weitzjdevk commented Jul 14, 2021

yorinasub17 left a comment

yorinasub17 Jul 14, 2021

yorinasub17 Jul 14, 2021

yorinasub17 Jul 14, 2021

yorinasub17 Jul 14, 2021

yorinasub17 Jul 14, 2021

yorinasub17 Jul 14, 2021

weitzjdevk Jul 14, 2021

weitzjdevk Jul 29, 2021

brikis98 Aug 3, 2021

yorinasub17 Aug 4, 2021

weitzjdevk Aug 4, 2021 •

edited

Loading

yorinasub17 Aug 24, 2021

weitzjdevk commented Jul 14, 2021

weitzjdevk commented Jul 29, 2021

weitzjdevk commented Jul 29, 2021

weitzj commented Aug 2, 2021

brikis98 commented Aug 3, 2021

yorinasub17 left a comment

yorinasub17 Aug 4, 2021

weitzjdevk Aug 4, 2021

weitzjdevk Aug 4, 2021 •

edited

Loading

yorinasub17 Aug 5, 2021

weitzjdevk Aug 6, 2021

yorinasub17 Aug 4, 2021

weitzjdevk Aug 4, 2021

weitzjdevk Aug 4, 2021

weitzjdevk Aug 5, 2021

weitzjdevk commented Aug 4, 2021

weitzjdevk commented Aug 6, 2021

weitzj commented Aug 17, 2021

yorinasub17 commented Aug 17, 2021

yorinasub17 left a comment

Add acmpa ✨ #201

Add acmpa ✨ #201

Conversation

weitzj commented Jul 13, 2021

weitzjdevk commented Jul 14, 2021

yorinasub17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hypothesis from Support-Agent

Answer from AWS PCA Service Team confirming hypothesis from Support-Agent above

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weitzjdevk Aug 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weitzjdevk commented Jul 14, 2021

weitzjdevk commented Jul 29, 2021

weitzjdevk commented Jul 29, 2021

weitzj commented Aug 2, 2021

brikis98 commented Aug 3, 2021

yorinasub17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weitzjdevk Aug 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weitzjdevk commented Aug 4, 2021

weitzjdevk commented Aug 6, 2021

weitzj commented Aug 17, 2021

yorinasub17 commented Aug 17, 2021

yorinasub17 left a comment

Choose a reason for hiding this comment

weitzjdevk Aug 4, 2021 •

edited

Loading

weitzjdevk Aug 4, 2021 •

edited

Loading