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

provider/aws: Add aws_lambda_permission #4826

Merged
merged 7 commits into from
Feb 17, 2016

Conversation

radeksimko
Copy link
Member

This should allow users to schedule Lambda functions via CloudWatch events (essentially crontab functionality) and/or connect functions to other AWS products, like SNS, S3 etc.

It is closing #2885 & #4454

TODO list follows:

  • Implement basic CRD functionality
  • Document new resource
  • Work around ResourceConflictException (retry) - similar problem to provider/aws: Retry Listener Creation for ELBs #4825
  • Tests for all validators
  • Acceptance test for minimal config
  • Acceptance test for full config (including qualifier)
  • Acceptance test with multiple permissions
  • Acceptance test with function_name as myFunction (as opposed to ARN)
  • Acceptance test with SNS
  • Acceptance test with S3
  • Acceptance test for ResourceConflictException
  • Document example with SNS

@radeksimko radeksimko force-pushed the f-aws-lambda-permission branch 12 times, most recently from 76f057a to 1d6d9ee Compare January 27, 2016 22:40
@radeksimko radeksimko removed the wip label Jan 27, 2016
@radeksimko radeksimko changed the title [WIP] provider/aws: Add aws_lambda_permission provider/aws: Add aws_lambda_permission Jan 27, 2016
@radeksimko
Copy link
Member Author

This is now ready for review.

It is possible to use this with SNS.
However some extra work will need to be done on the aws_s3_bucket (to allow events like new version uploads to be sent to Lambda) and I'm working on getting CloudWatch Events implemented as a new resource too.

Acceptance tests may intermittently fail due to IAM - see #4447

if awsErr, ok := err.(awserr.Error); ok {
// IAM is eventually consistent :/
if awsErr.Code() == "ResourceConflictException" {
return fmt.Errorf("[WARN] Error creating ELB Listener with SSL Cert, retrying: %s", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we Warning on creating ELB Listener with SSL Cert? - copy/ paste error I think?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is for a case where you rename the reference name, e.g.

resource "aws_lambda_permission" "one" {

to

resource "aws_lambda_permission" "two" {

It should probably be dealt with in core too, but Terraform now treats these as two different resources and the new one may be created before the old one gets deleted. I thought WARN was the right level for this log message, but i can make it INFO or DEBUG - what do you suggest?

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, I see, the message is obviously wrong! d'oh

Copy link
Contributor

Choose a reason for hiding this comment

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

@radeksimko I mean more the actual error itself - creating ELB Listener with SSL Cert - this resource has nothing to do with ELBs :)

@stack72
Copy link
Contributor

stack72 commented Feb 12, 2016

1 small comment made but tests look good:

make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSLambdaPermission' 2>~/tf.log
==> Checking that code complies with gofmt requirements...
/Users/stacko/Code/go/bin/stringer
GO15VENDOREXPERIMENT=1 go generate $(GO15VENDOREXPERIMENT=1 go list ./... | grep -v /vendor/)
TF_ACC=1 GO15VENDOREXPERIMENT=1 go test ./builtin/providers/aws -v -run=TestAccAWSLambdaPermission -timeout 120m
=== RUN   TestAccAWSLambdaPermission_basic
--- PASS: TestAccAWSLambdaPermission_basic (19.03s)
=== RUN   TestAccAWSLambdaPermission_withRawFunctionName
--- PASS: TestAccAWSLambdaPermission_withRawFunctionName (18.89s)
=== RUN   TestAccAWSLambdaPermission_withQualifier
--- PASS: TestAccAWSLambdaPermission_withQualifier (16.04s)
=== RUN   TestAccAWSLambdaPermission_multiplePerms
--- PASS: TestAccAWSLambdaPermission_multiplePerms (26.43s)
=== RUN   TestAccAWSLambdaPermission_withS3
--- PASS: TestAccAWSLambdaPermission_withS3 (19.09s)
=== RUN   TestAccAWSLambdaPermission_withSNS
--- PASS: TestAccAWSLambdaPermission_withSNS (16.61s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    116.112s

@radeksimko
Copy link
Member Author

I will need to work around the intermittent issues with permissions being lost.

Here's the full explanation including isolated reproduction case sent as support ticket to AWS:
https://gist.github.com/radeksimko/07e5f8ea3e873143666e

I reckon they will say it's the nature of the API (inconsistent in the same way as IAM). The solution I plan to implement includes mutex to prevent creating more permissions for the same function at the same time. It inherently makes creation of multiple resources of this type less efficient (slower), but there doesn't seem to be any other way around it.

We already use mutex in AWS provider, as @stack72 mentioned - in aws_security_group_rule.

@radeksimko radeksimko changed the title provider/aws: Add aws_lambda_permission [WIP] provider/aws: Add aws_lambda_permission Feb 14, 2016
@radeksimko
Copy link
Member Author

Just a quick update from AWS support this morning - a bug on the API level has been confirmed by the Lambda team. They're working on a fix.

I will try to make it work with the mutex anyway, so we can get this resource into master asap.

@radeksimko radeksimko force-pushed the f-aws-lambda-permission branch 4 times, most recently from 22d9515 to 674f4a7 Compare February 17, 2016 11:15
@stack72
Copy link
Contributor

stack72 commented Feb 17, 2016

Just verified that the tests all work now 4 times :)

make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSLambdaPermission' 2>~/tf.log
==> Checking that code complies with gofmt requirements...
/Users/stacko/Code/go/bin/stringer
GO15VENDOREXPERIMENT=1 go generate $(GO15VENDOREXPERIMENT=1 go list ./... | grep -v /vendor/)
TF_ACC=1 GO15VENDOREXPERIMENT=1 go test ./builtin/providers/aws -v -run=TestAccAWSLambdaPermission -timeout 120m
=== RUN   TestAccAWSLambdaPermission_basic
--- PASS: TestAccAWSLambdaPermission_basic (21.52s)
=== RUN   TestAccAWSLambdaPermission_withRawFunctionName
--- PASS: TestAccAWSLambdaPermission_withRawFunctionName (17.23s)
=== RUN   TestAccAWSLambdaPermission_withQualifier
--- PASS: TestAccAWSLambdaPermission_withQualifier (19.38s)
=== RUN   TestAccAWSLambdaPermission_multiplePerms
--- PASS: TestAccAWSLambdaPermission_multiplePerms (40.58s)
=== RUN   TestAccAWSLambdaPermission_withS3
--- PASS: TestAccAWSLambdaPermission_withS3 (32.52s)
=== RUN   TestAccAWSLambdaPermission_withSNS
--- PASS: TestAccAWSLambdaPermission_withSNS (21.80s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    153.053s

Very nice work @radeksimko

stack72 added a commit that referenced this pull request Feb 17, 2016
[WIP] provider/aws: Add aws_lambda_permission
@stack72 stack72 merged commit cd28433 into hashicorp:master Feb 17, 2016
@stack72 stack72 changed the title [WIP] provider/aws: Add aws_lambda_permission provider/aws: Add aws_lambda_permission Feb 17, 2016
@radeksimko radeksimko deleted the f-aws-lambda-permission branch February 17, 2016 13:33
@ghost
Copy link

ghost commented Apr 28, 2020

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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants