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

Fix crash on unsuccessful flow log creation #7528

Merged
merged 1 commit into from
Feb 19, 2019

Conversation

phy1729
Copy link
Contributor

@phy1729 phy1729 commented Feb 12, 2019

Changes proposed in this pull request:

  • Error instead of crashing when the flow log's destination S3 bucket is not accessible.

Crash reproducer:

resource "aws_vpc" "main" {
  cidr_block = "10.17.29.0/24"
}

resource "aws_flow_log" "main" {
  log_destination_type = "s3"
  log_destination      = "arn:aws:s3:::does-not-exist"
  vpc_id               = "${aws_vpc.main.id}"
  traffic_type         = "ALL"
}

Actual output:

panic: runtime error: index out of range
[... crash text ...]

due to https://github.com/terraform-providers/terraform-provider-aws/blob/master/aws/resource_aws_flow_log.go#L149 referencing the 0th element of the empty list resp.FlowLogIds

Output with PR:

Error: Error applying plan:

1 error(s) occurred:

* aws_flow_log.main: 1 error(s) occurred:

* aws_flow_log.main: Error creating Flow Log for (vpc-X), error: Access Denied for LogDestination: does-not-exist. Please check LogDestination permission

@ghost ghost added service/ec2 Issues and PRs that pertain to the ec2 service. size/XS Managed by automation to categorize the size of a PR. labels Feb 12, 2019
@bflad bflad added bug Addresses a defect in current functionality. crash Results from or addresses a Terraform crash or kernel panic. labels Feb 19, 2019
@bflad bflad added this to the v1.60.0 milestone Feb 19, 2019
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

Thanks for the detailed pull request summary and fix, @phy1729! Using your information, I was able to recreate the panic in an acceptance test and ensure that the fix is covered in the future. 🚀

func TestAccAWSFlowLog_LogDestinationType_S3_Invalid(t *testing.T) {
	rName := acctest.RandomWithPrefix("tf-acc-test-flow-log-s3-invalid")

	resource.ParallelTest(t, resource.TestCase{
		PreCheck:     func() { testAccPreCheck(t) },
		Providers:    testAccProviders,
		CheckDestroy: testAccCheckFlowLogDestroy,
		Steps: []resource.TestStep{
			{
				Config:      testAccFlowLogConfig_LogDestinationType_S3_Invalid(rName),
				ExpectError: regexp.MustCompile(`Access Denied for LogDestination`),
			},
		},
	})
}

func testAccFlowLogConfig_LogDestinationType_S3_Invalid(rName string) string {
	return fmt.Sprintf(`
resource "aws_vpc" "test" {
  cidr_block = "10.0.0.0/16"

  tags = {
    Name = %q
  }
}

resource "aws_flow_log" "test" {
  log_destination      = "arn:aws:s3:::does-not-exist"
  log_destination_type = "s3"
  traffic_type         = "ALL"
  vpc_id               = "${aws_vpc.test.id}"
}
`, rName)
}

Output from acceptance testing (before fix):

=== CONT  TestAccAWSFlowLog_LogDestinationType_S3_Invalid
panic: runtime error: index out of range

goroutine 249 [running]:
github.com/terraform-providers/terraform-provider-aws/aws.resourceAwsLogFlowCreate(0xc000a38fc0, 0x4838760, 0xc0004c1800, 0xc000a38fc0, 0x0)
	/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_flow_log.go:149 +0xc6e
github.com/hashicorp/terraform/helper/schema.(*Resource).Apply(0xc00024fc70, 0xc0004fad70, 0xc0007187a0, 0x4838760, 0xc0004c1800, 0xc000510e01, 0x31, 0x0)
	/Users/bflad/go/pkg/mod/github.com/hashicorp/[email protected]/helper/schema/resource.go:225 +0x351

Output from acceptance testing (after fix):

--- PASS: TestAccAWSFlowLog_LogDestinationType_S3_Invalid (74.41s)

@bflad bflad merged commit e30971c into hashicorp:master Feb 19, 2019
bflad added a commit that referenced this pull request Feb 19, 2019
Output from acceptance testing (before fix):

```
=== CONT  TestAccAWSFlowLog_LogDestinationType_S3_Invalid
panic: runtime error: index out of range

goroutine 249 [running]:
github.com/terraform-providers/terraform-provider-aws/aws.resourceAwsLogFlowCreate(0xc000a38fc0, 0x4838760, 0xc0004c1800, 0xc000a38fc0, 0x0)
	/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_flow_log.go:149 +0xc6e
github.com/hashicorp/terraform/helper/schema.(*Resource).Apply(0xc00024fc70, 0xc0004fad70, 0xc0007187a0, 0x4838760, 0xc0004c1800, 0xc000510e01, 0x31, 0x0)
	/Users/bflad/go/pkg/mod/github.com/hashicorp/[email protected]/helper/schema/resource.go:225 +0x351
```

Output from acceptance testing (after fix):

```
--- PASS: TestAccAWSFlowLog_LogDestinationType_S3_Invalid (74.41s)
```
bflad added a commit that referenced this pull request Feb 19, 2019
@bflad
Copy link
Contributor

bflad commented Feb 22, 2019

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

@phy1729 phy1729 deleted the flow-log-crash branch April 4, 2019 02:06
@ghost
Copy link

ghost commented Mar 30, 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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked and limited conversation to collaborators Mar 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Addresses a defect in current functionality. crash Results from or addresses a Terraform crash or kernel panic. service/ec2 Issues and PRs that pertain to the ec2 service. size/XS Managed by automation to categorize the size of a PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants