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: cloudwatch_logs_subscription_filter resource #5996

Merged
merged 1 commit into from Apr 20, 2016

Conversation

@joshmyers
Copy link
Contributor

joshmyers commented Apr 3, 2016

Instead of #4813

@apparentlymart apparentlymart changed the title Add cloudwatch_logs_subscription_filter provider provider/aws: cloudwatch_logs_subscription_filter resource Apr 4, 2016
@joshmyers joshmyers force-pushed the joshmyers:cwl_subscription_filter branch from de0ea2a to 0bfd781 Apr 4, 2016
@stack72

This comment has been minimized.

Copy link
Contributor

stack72 commented Apr 5, 2016

Hi @joshmyers

thanks so much for the PR, on trying to start reviewing this, I ran the tests and got the following:

TF_LOG=1 make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSCloudwatchLogSubscriptionFilter' 2>~/tf.log
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /vendor/)
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSCloudwatchLogSubscriptionFilter -timeout 120m
=== RUN   TestAccAWSCloudwatchLogSubscriptionFilter_basic
--- FAIL: TestAccAWSCloudwatchLogSubscriptionFilter_basic (0.01s)
    testing.go:154: Step 0 error: Configuration is invalid.

        Warnings: []string(nil)

        Errors: []string{"aws_lambda_function.test_lambdafunction: \"role\": required field is not set"}
FAIL
exit status 1
FAIL    github.com/hashicorp/terraform/builtin/providers/aws    0.030s

Please can you have a look at these?

Thanks

Paul

@stack72 stack72 self-assigned this Apr 5, 2016
@joshmyers joshmyers force-pushed the joshmyers:cwl_subscription_filter branch from 0bfd781 to 719c93e Apr 6, 2016
@joshmyers

This comment has been minimized.

Copy link
Contributor Author

joshmyers commented Apr 6, 2016

@stack72 I've fixed up the tests and rebased

~/go/src/github.com/hashicorp/terraform(branch:cwl_subscription_filter) » TF_LOG=DEBUG make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSCloudwatchLogSubscriptionFilter' 2>~/tf.log
==> Checking that code complies with gofmt requirements...
/Users/joshmyers/go/bin/stringer
go generate $(go list ./... | grep -v /vendor/)
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSCloudwatchLogSubscriptionFilter -timeout 120m
=== RUN   TestAccAWSCloudwatchLogSubscriptionFilter_basic
--- PASS: TestAccAWSCloudwatchLogSubscriptionFilter_basic (19.26s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    19.290s
------------------------------------------------------------
@stack72

This comment has been minimized.

Copy link
Contributor

stack72 commented Apr 6, 2016

Hi @joshmyers

this all looks good - there is 1 issue that we see in a few places right now

F_LOG=DEBUG make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSCloudwatchLogSubscriptionFilter' 2>~/tf.log
==> Checking that code complies with gofmt requirements...
/Users/pstack/code/go/bin/stringer
go generate $(go list ./... | grep -v /vendor/)
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSCloudwatchLogSubscriptionFilter -timeout 120m
=== RUN   TestAccAWSCloudwatchLogSubscriptionFilter_basic
--- FAIL: TestAccAWSCloudwatchLogSubscriptionFilter_basic (18.78s)
    testing.go:154: Step 0 error: Error applying: 1 error(s) occurred:

        * aws_cloudwatch_log_subscription_filter.test_lambdafunction_logfilter: InvalidParameterException: Could not execute the lambda function. Make sure you have given CloudWatch Logs permission to execute your function.
            status code: 400, request id: 4e68910f-fc35-11e5-943c-b58a46d37bfe
FAIL
exit status 1
FAIL    github.com/hashicorp/terraform/builtin/providers/aws    18.798s

This is due to IAM eventual consistency issues. I will speak to some of HC team about that before I merge

I will keep you informed

Paul

@joshmyers

This comment has been minimized.

Copy link
Contributor Author

joshmyers commented Apr 7, 2016

@stack72 I can't reproduce this locally but I guess it is intermittent. Would be great to get this merged as we are using our fork of TF and have fallen behind a few releases.

@joshmyers

This comment has been minimized.

Copy link
Contributor Author

joshmyers commented Apr 15, 2016

@stack72 Good talk last night, was hoping to catch up with you after but it went on longer than expected. Is there any update on this?

@stack72

This comment has been minimized.

Copy link
Contributor

stack72 commented Apr 20, 2016

Hi @joshmyers

Glad you enjoyed the talk. I haven't been able to reproduce this error again so this is good to merge

Thanks for all the work here :)

Paul

@stack72 stack72 merged commit 3be66aa into hashicorp:master Apr 20, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@catsby

This comment has been minimized.

Copy link
Member

catsby commented May 11, 2016

Hello friends –

TestAccAWSCloudwatchLogSubscriptionFilter_basic has been failing consistently in our nightly acceptances tests since 2016-04-21 when it was introduced:

--- FAIL: TestAccAWSCloudwatchLogSubscriptionFilter_basic (16.47s)
    testing.go:174: Step 0 error: Error applying: 1 error(s) occurred:

        * aws_cloudwatch_log_subscription_filter.test_lambdafunction_logfilter: InvalidParameterException: Could not execute the lambda function. Make sure you have given CloudWatch Logs permission to execute your function.

Unfortunately, I am not a IAM / Policy Guru, can anyone shed some light on what we're missing here in the config?

resource "aws_cloudwatch_log_subscription_filter" "test_lambdafunction_logfilter" {
  name            = "test_lambdafunction_logfilter"
  log_group_name  = "example_lambda_name"
  filter_pattern  = "logtype test"
  destination_arn = "${aws_lambda_function.test_lambdafunction.arn}"
}

resource "aws_lambda_function" "test_lambdafunction" {
  filename      = "test-fixtures/lambdatest.zip"
  function_name = "example_lambda_name"
  role          = "${aws_iam_role.iam_for_lambda.arn}"
  handler       = "exports.handler"
}

resource "aws_cloudwatch_log_group" "logs" {
  name              = "example_lambda_name"
  retention_in_days = 1
}

resource "aws_lambda_permission" "allow_cloudwatch_logs" {
  statement_id  = "AllowExecutionFromCloudWatchLogs"
  action        = "lambda:*"
  function_name = "${aws_lambda_function.test_lambdafunction.arn}"
  principal     = "logs.eu-west-1.amazonaws.com"
}

resource "aws_iam_role" "iam_for_lambda" {
  name = "test_lambdafuntion_iam_role"

  assume_role_policy = <<EOF
{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Action": "sts:AssumeRole",
      "Principal": {
        "Service": "lambda.amazonaws.com"
      },
      "Effect": "Allow",
      "Sid": ""
    }
  ]
}
EOF
}

resource "aws_iam_role_policy" "test_lambdafunction_iam_policy" {
  name = "test_lambdafunction_iam_policy"
  role = "${aws_iam_role.iam_for_lambda.id}"

  policy = <<EOF
{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Sid": "Stmt1441111030000",
      "Effect": "Allow",
      "Action": [
        "dynamodb:*"
      ],
      "Resource": [
        "*"
      ]
    }
  ]
}
EOF
}
@radeksimko

This comment has been minimized.

Copy link
Member

radeksimko commented May 11, 2016

@catsby I originally thought this would be YAIAMECI™ (yet another IAM eventual consistency issue), but probably not this time.

I took the exact config you pasted above, just changed eu-west-1 to us-east-1, pointed Lambda function to a valid ZIP file and got to this:

Apply complete! Resources: 6 added, 0 changed, 0 destroyed.

All of our acceptance tests run in us-west-2 AFAIK, so that's the issue.

@stack72

This comment has been minimized.

Copy link
Contributor

stack72 commented May 11, 2016

Oh no! Hard coded string :( my AWS creds have a default of region of eu-west so I didn't even notice that. Doh!

@catsby

This comment has been minimized.

Copy link
Member

catsby commented May 11, 2016

@radeksimko BRILLIANT thank you :)

@joshmyers

This comment has been minimized.

Copy link
Contributor Author

joshmyers commented May 13, 2016

Hi folks, just seen this.While writing tests I saw others which used a hardcoded string so I did the same. I thought it doesn't feel right but didn't get picked up. Apologies!

@catsby

This comment has been minimized.

Copy link
Member

catsby commented Jun 21, 2016

@joshmyers no worries! (sorry for the very late reply)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.