re-create lambda policy when permission sid not found #11924

ansoni · 2020-02-06T08:01:03Z

Hello,
Please find the small change to blank out the Id for proper remediation when the policy SID goes missing.

Thanks,

Anthony

Community Note

Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Release note for CHANGELOG:

lambda policy SIDs that are deleted out-of-band are now properly drift detected and restored.

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestLambdaPermission'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -count 1 -parallel 20 -run=TestLambdaPermission -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestLambdaPermissionUnmarshalling
--- PASS: TestLambdaPermissionUnmarshalling (0.00s)
=== RUN   TestLambdaPermissionGetQualifierFromLambdaAliasOrVersionArn_alias
--- PASS: TestLambdaPermissionGetQualifierFromLambdaAliasOrVersionArn_alias (0.00s)
=== RUN   TestLambdaPermissionGetQualifierFromLambdaAliasOrVersionArn_govcloud
--- PASS: TestLambdaPermissionGetQualifierFromLambdaAliasOrVersionArn_govcloud (0.00s)
=== RUN   TestLambdaPermissionGetQualifierFromLambdaAliasOrVersionArn_version
--- PASS: TestLambdaPermissionGetQualifierFromLambdaAliasOrVersionArn_version (0.00s)
=== RUN   TestLambdaPermissionGetQualifierFromLambdaAliasOrVersionArn_invalid
--- PASS: TestLambdaPermissionGetQualifierFromLambdaAliasOrVersionArn_invalid (0.00s)
=== RUN   TestLambdaPermissionGetFunctionNameFromLambdaArn_invalid
--- PASS: TestLambdaPermissionGetFunctionNameFromLambdaArn_invalid (0.00s)
=== RUN   TestLambdaPermissionGetFunctionNameFromLambdaArn_valid
--- PASS: TestLambdaPermissionGetFunctionNameFromLambdaArn_valid (0.00s)
=== RUN   TestLambdaPermissionGetFunctionNameFromGovCloudLambdaArn
--- PASS: TestLambdaPermissionGetFunctionNameFromGovCloudLambdaArn (0.00s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	0.062s
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws/internal/flatmap	0.023s [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws/internal/keyvaluetags	0.101s [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws/internal/service/batch/equivalency	0.034s [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws/internal/service/eks/token	0.035s [no tests to run]
?   	github.com/terraform-providers/terraform-provider-aws/awsproviderlint	[no test files]
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/awsproviderlint/passes	0.068s [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/awsproviderlint/passes/AWSAT001	0.048s [no tests to run]
...

ansoni · 2020-02-06T08:07:48Z

aws/resource_aws_lambda_permission.go

-		return nil, fmt.Errorf("Error finding Lambda policy statement: %s", psErr)
-	}
-	return statement, nil
+	return findLambdaPolicyStatementById(&policy, statemendId)


re-casting the error to a human readable string is unnecessary since we are a tool that can do something about it.

Agreed, there's no need to wrap this again

ansoni · 2020-02-06T08:09:00Z

aws/resource_aws_lambda_permission.go

@@ -225,6 +225,16 @@ func resourceAwsLambdaPermissionRead(d *schema.ResourceData, meta interface{}) e
 		if err == nil {
 			var psErr error
 			statement, psErr = getLambdaPolicyStatement(out, d.Id())
+
+			// handle the resource not existing
+			if awsErr, ok := psErr.(awserr.Error); ok {


Pattern taken from existing code a few lines below. I don't believe we were ever hitting that case, but left just to be sure.

ansoni · 2020-02-06T08:12:11Z

FYI @bflad, this hard fail caused us a lot of grief today. If we can get this drift-remediation update included then our work-around for the modified lambda policies will be a terraform upgrade instead of us having to modify 40-50 TFE project state files or out-of-band edit of lambda permissions.

gdavison

Thanks for the PR, @ansoni. Could you add acceptance tests for this, please?

You can probably use direct AWS API calls to make the out-of-band changes. Take a look at some of the <something>_disappears acceptance tests for inspiration.

gdavison · 2020-02-10T22:28:37Z

aws/resource_aws_lambda_permission.go

-		return nil, fmt.Errorf("Error finding Lambda policy statement: %s", psErr)
-	}
-	return statement, nil
+	return findLambdaPolicyStatementById(&policy, statemendId)


Agreed, there's no need to wrap this again

scottwinkler · 2020-02-14T22:48:04Z

@gdavison I wrote an acceptance test for this

Running tool: /usr/local/bin/go test -timeout 30s github.com/terraform-providers/terraform-provider-aws/aws -run ^(TestAccAWSLambdaPermission_disappears)$
ok github.com/terraform-providers/terraform-provider-aws/aws 2.518s
Success: Tests passed.

gdavison

Thanks for the test, @scottwinkler. I have a couple changes.

gdavison · 2020-02-18T22:42:22Z

aws/resource_aws_lambda_permission_test.go

+			{
+				Config: testAccAWSLambdaPermissionConfig(funcName, roleName),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckLambdaPermissionExists("aws_lambda_permission.allow_cloudwatch", &statement),
+				),
+			},
+			// Here we delete the Lambda permission to verify the follow-on refresh after this step
+			// should not error.


This first step can be removed

gdavison · 2020-02-18T22:46:02Z

aws/resource_aws_lambda_permission_test.go

+			// Here we delete the Lambda permission to verify the follow-on refresh after this step
+			// should not error.
+			{
+				Config: testAccAWSLambdaPermissionConfig(funcName, roleName),


The error only shows up when there are still permission elements left. We can use testAccAWSLambdaPermissionConfig_multiplePerms() instead, and then remove aws_lambda_permission.first.

scottwinkler · 2020-02-18T23:07:55Z

@gdavison I made the changes you requested. Please let me know if there is anything else you require

gdavison · 2020-02-19T01:10:56Z

LGTM 🚀

--- PASS: TestAccAWSLambdaPermission_basic (24.64s)
--- PASS: TestAccAWSLambdaPermission_withIAMRole (33.33s)
--- PASS: TestAccAWSLambdaPermission_withQualifier (33.23s)
--- PASS: TestAccAWSLambdaPermission_withStatementIdPrefix (33.43s)
--- PASS: TestAccAWSLambdaPermission_withRawFunctionName (33.53s)
--- PASS: TestAccAWSLambdaPermission_withSNS (33.73s)
--- PASS: TestAccAWSLambdaPermission_withS3 (33.70s)
--- PASS: TestAccAWSLambdaPermission_multiplePerms (40.61s)
--- PASS: TestAccAWSLambdaPermission_StatementId_Duplicate (89.63s)
--- PASS: TestAccAWSLambdaPermission_disappears (91.56s)

ghost · 2020-02-21T00:23:30Z

This has been released in version 2.50.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

ghost · 2020-03-20T14:50:58Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

Anthony Johnson added 2 commits February 5, 2020 23:40

update to re-create resoure when lambda policy sid not found

703cce7

go fmt

c4b6a42

ansoni requested a review from a team February 6, 2020 08:01

ghost added needs-triage Waiting for first response or review from a maintainer. size/XS Managed by automation to categorize the size of a PR. service/lambda Issues and PRs that pertain to the lambda service. labels Feb 6, 2020

ansoni commented Feb 6, 2020

View reviewed changes

gdavison self-assigned this Feb 10, 2020

gdavison requested changes Feb 10, 2020

View reviewed changes

gdavison added waiting-response Maintainers are waiting on response from community or contributor. and removed needs-triage Waiting for first response or review from a maintainer. labels Feb 10, 2020

add acceptance tests

da0ae7d

ghost added tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. size/M Managed by automation to categorize the size of a PR. and removed size/XS Managed by automation to categorize the size of a PR. labels Feb 14, 2020

ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Feb 14, 2020

gdavison requested changes Feb 18, 2020

View reviewed changes

gdavison added the waiting-response Maintainers are waiting on response from community or contributor. label Feb 18, 2020

changes

4820e6f

ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Feb 18, 2020

gdavison approved these changes Feb 19, 2020

View reviewed changes

gdavison added this to the v2.50.0 milestone Feb 19, 2020

gdavison merged commit a3c7502 into hashicorp:master Feb 19, 2020

gdavison added a commit that referenced this pull request Feb 19, 2020

Update CHANGELOG for #11924

9f67e78

ghost locked and limited conversation to collaborators Mar 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

re-create lambda policy when permission sid not found #11924

re-create lambda policy when permission sid not found #11924

ansoni commented Feb 6, 2020

ansoni Feb 6, 2020

gdavison Feb 10, 2020

ansoni Feb 6, 2020

ansoni commented Feb 6, 2020

gdavison left a comment

gdavison Feb 10, 2020

scottwinkler commented Feb 14, 2020

gdavison left a comment

gdavison Feb 18, 2020

scottwinkler Feb 18, 2020

gdavison Feb 18, 2020

scottwinkler commented Feb 18, 2020 •

edited

Loading

gdavison commented Feb 19, 2020

ghost commented Feb 21, 2020

ghost commented Mar 20, 2020

re-create lambda policy when permission sid not found #11924

re-create lambda policy when permission sid not found #11924

Conversation

ansoni commented Feb 6, 2020

Community Note

ansoni Feb 6, 2020

Choose a reason for hiding this comment

gdavison Feb 10, 2020

Choose a reason for hiding this comment

ansoni Feb 6, 2020

Choose a reason for hiding this comment

ansoni commented Feb 6, 2020

gdavison left a comment

Choose a reason for hiding this comment

gdavison Feb 10, 2020

Choose a reason for hiding this comment

scottwinkler commented Feb 14, 2020

gdavison left a comment

Choose a reason for hiding this comment

gdavison Feb 18, 2020

Choose a reason for hiding this comment

scottwinkler Feb 18, 2020

Choose a reason for hiding this comment

gdavison Feb 18, 2020

Choose a reason for hiding this comment

scottwinkler commented Feb 18, 2020 • edited Loading

gdavison commented Feb 19, 2020

ghost commented Feb 21, 2020

ghost commented Mar 20, 2020

scottwinkler commented Feb 18, 2020 •

edited

Loading