aws_flow_log - adding support for sending to S3 #5509

ryandeivert · 2018-08-10T23:43:17Z

Changes proposed in this pull request:

Updating aws_flow_log resource for s3 delivery support, also improving schema validation.
Adding state migration for aws_flow_log resource from v0 to v1
Adding test for aws_flow_log state migration
Updating docs for new arguments in aws_flow_log resource
Updates to the 1-to-2 upgrade guide for this resource (thanks @ewbankkit)
Way better tests from @ewbankkit

Output from acceptance testing (from @ewbankkit):

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSFlowLog_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -parallel 20 -run=TestAccAWSFlowLog_ -timeout 120m
=== RUN   TestAccAWSFlowLog_VPCID
=== PAUSE TestAccAWSFlowLog_VPCID
=== RUN   TestAccAWSFlowLog_SubnetID
--- PASS: TestAccAWSFlowLog_SubnetID (30.24s)
=== RUN   TestAccAWSFlowLog_LogDestinationType_CloudWatchLogs
--- PASS: TestAccAWSFlowLog_LogDestinationType_CloudWatchLogs (24.43s)
=== RUN   TestAccAWSFlowLog_LogDestinationType_S3
--- PASS: TestAccAWSFlowLog_LogDestinationType_S3 (28.18s)
=== CONT  TestAccAWSFlowLog_VPCID
--- PASS: TestAccAWSFlowLog_VPCID (40.73s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	124.117s

ewbankkit · 2018-08-11T11:42:58Z

aws/resource_aws_flow_log.go

- Optional: true,
- ForceNew: true,
- ConflictsWith: []string{"eni_id", "vpc_id"},
+ "resource_id": &schema.Schema{


Should be named resource_ids as potentially multiple IDs can be specified?

I did this to conform to the AWS cli argument name*, but will make the change

ewbankkit · 2018-08-11T11:47:03Z

aws/resource_aws_flow_log.go

 LogGroupName: aws.String(d.Get("log_group_name").(string)),
- ResourceIds: []*string{aws.String(resourceId)},
- ResourceType: aws.String(resourceType),
+ ResourceIds: aws.StringSlice(d.Get("resource_id").([]string)),


As resource_id is declared as schema.TypeSet, d.Get("resource_id") is I think of type *schema.Set, so some manipulation is required to pass to aws.StringSlice.

ewbankkit · 2018-08-11T11:49:51Z

aws/resource_aws_flow_log.go


- var resourceKey string
 if strings.HasPrefix(*fl.ResourceId, "vpc-") {


I have been thinking of various places this kind of code is used/useful (e.g. I've been thinking about solutions for #26).
Do you think we should add some generic EC2 resource ID parsing function that returns the resource type from the ID (e.g. based on rules here)?

ewbankkit · 2018-08-11T11:55:04Z

Do we need to consider a diff suppression for the case

resource "aws_flow_log" "foo" {
  log_group_name = "bar"
}

=>

resource "aws_flow_log" "foo" {
  log_destination_type = "cloud-watch-logs"
  log_destination      = "bar"
}

and vice-versa?

ewbankkit · 2018-08-11T21:05:31Z

aws/resource_aws_flow_log.go

 },

- "vpc_id": &schema.Schema{


Removing existing attributes will require a deprecation warning and support of the attributes until deprecation.

ryandeivert · 2018-08-14T00:15:29Z

thanks @ewbankkit I've addressed some of your suggestions. going to take a look at the diff suppression. I'm not sure how to address your suggestion of generic ec2 resource parsing but can look later as well

ryandeivert · 2018-08-14T20:48:40Z

@ewbankkit I addressed the diff suppression for log destination, do you think I also have to add diff suppression to check if the old format matches the new format.. ie:

old format:

resource "aws_flow_log" "foo" {
  vpc_id = "vpc-foo"
}

vs new format:

resource "aws_flow_log" "foo" {
  resource_ids = ["vpc-foo"]
  resource_type = "VPC"
}

ewbankkit · 2018-08-15T19:21:23Z

@ryandeivert RE #5509 (comment), don't worry; I was just noodling some thoughts.

ryandeivert · 2018-08-15T19:23:34Z

thanks @ewbankkit - I'm running acceptance tests and fixing some more things. will update soon

ewbankkit · 2018-08-15T19:26:49Z

Yes, strictly speaking those resource_type diffs should be suppressed also.
The deprecations should get onto the Terraform AWS Provider Version 2 Upgrade Guide also. Not sure where the source for that is.

ryandeivert · 2018-08-15T19:27:57Z

good to know - I'll hunt for it. thanks for the pointers

mikecook · 2018-08-25T20:39:12Z

aws/resource_aws_flow_log.go now has conflicts, could you rebase please?

tomelliff · 2018-08-28T10:32:17Z

Upgrade guide is at website/docs/guides/version-2-upgrade.html.md.

mikecook · 2018-08-31T00:27:16Z

@ryandeivert aws/resource_aws_flow_log.go now has conflicts, could you rebase please?

ryandeivert · 2018-08-31T22:23:31Z

hey @mikecook sorry I haven't gotten to touch this in a bit. I have some local changes I hope to get upstream soon and will rebase when I take care of that

ewbankkit · 2018-09-02T16:49:04Z

@ryandeivert I can help with running acceptance tests when you're ready.

ryandeivert · 2018-09-04T04:17:45Z

thanks @ewbankkit I've actually been running them now, trying to sort out issues. will follow up soon

uovobw · 2018-09-05T14:11:39Z

greetings, this might turn out to be very useful, any way i can help? if not, is there an estimated release date? thanks all for the great work!

ryandeivert · 2018-09-05T16:52:54Z

hi @uovobw I'm going to try to dedicate some time to this later today. I'm having some issues with acceptance tests (thinking related to the diff suppression) so I'll probably just commit what I have and outsource some assistance from someone here. thanks for your patience!

uovobw · 2018-09-05T17:05:00Z

@ryandeivert ok, in the coming days i'll provide what help i can, thanks for the update and for your work.

AzCii · 2018-09-11T13:02:14Z

exactly what i'm looking for :-) any ETA on this?

vmartin01 · 2018-10-05T19:49:57Z

I'm interested in this as well, VPC Flow Logs through cloudwatch are very costly and would reduce the cost tremendously.

bflad · 2018-10-05T20:03:21Z

This will get a first review next week. 👍

bflad

Hi @ryandeivert and @ewbankkit 👋 Thanks for submitting this and thanks everyone for your patience. This took longer to review because there are two sets of changes occurring here with the usage of some more advanced resource functionality (state migrations and customizing the difference). We will keep the feedback loop tight on this going forward.

I'm a little curious why the resource_id and resource_type changes need to be included with the addition of log_destination and log_destination_type arguments? While they help the resource look more like the EC2 API, I'm not sure the change is necessary either. I feel like we can treat those changes separately (or not at all) since they complicate this PR review and are unrelated, but I will defer to the authors if they can adjust given the feedback or would like to treat them separately.

That said, I did find some items below that required me to mark this as breaking-change since it will affect existing configurations, mostly the Read function nil'ing out the old attributes. Resources should continue to have existing attributes available, even after deprecation, so configuration references to those attributes continue working.

Please let me know how you would like to proceed here or if you have any questions regarding any of these items, thanks.

bflad · 2018-10-15T12:09:57Z

aws/resource_aws_flow_log_migrate.go

+ Partition: meta.(*AWSClient).partition,
+ Region: meta.(*AWSClient).region,
+ Service: "logs",
+ AccountID: meta.(*AWSClient).accountid,


For environments that use skip_requesting_account_id this could silently break the configuration.

bflad · 2018-10-15T12:11:51Z

aws/resource_aws_flow_log.go

+ Partition: meta.(*AWSClient).partition,
+ Region: meta.(*AWSClient).region,
+ Service: "logs",
+ AccountID: meta.(*AWSClient).accountid,


We have the same problem here, that configurations using skip_requesting_account_id may not work in this scenario.

bflad · 2018-10-15T12:15:51Z

aws/resource_aws_flow_log.go

+ Partition: meta.(*AWSClient).partition,
+ Region: meta.(*AWSClient).region,
+ Service: "logs",
+ AccountID: meta.(*AWSClient).accountid,


We have the same problem here, that configurations using skip_requesting_account_id may not work in this scenario.

bflad · 2018-10-15T12:15:59Z

aws/resource_aws_flow_log.go

+ Partition: meta.(*AWSClient).partition,
+ Region: meta.(*AWSClient).region,
+ Service: "logs",
+ AccountID: meta.(*AWSClient).accountid,


We have the same problem here, that configurations using skip_requesting_account_id may not work in this scenario.

bflad · 2018-10-15T12:28:05Z

aws/resource_aws_flow_log.go

@@ -18,45 +20,114 @@ func resourceAwsFlowLog() *schema.Resource {
 Importer: &schema.ResourceImporter{
 State: schema.ImportStatePassthrough,
 },
+ CustomizeDiff: resourceAwsLogFlowCustomizeDiff,


Just noting here that CustomizeDiff is not the most reliable method to work with in Terraform resources. It will silently skip computed values (e.g. Computed attributes from references like aws_vpc.XXX.id). If we can avoid its usage here, I think we should.

bflad · 2018-10-15T12:35:08Z

aws/resource_aws_flow_log.go

 d.SetId("")
 return nil
 }

 fl := resp.FlowLogs[0]
- d.Set("traffic_type", fl.TrafficType)
- d.Set("log_group_name", fl.LogGroupName)


I think we need to keep this for backwards compatibility. See note below.

bflad · 2018-10-15T12:35:14Z

aws/resource_aws_flow_log.go

 }
- if resourceKey != "" {
- d.Set(resourceKey, fl.ResourceId)


I think we need to keep this for backwards compatibility. See note below.

bflad · 2018-10-15T12:42:25Z

aws/resource_aws_flow_log_test.go

 Providers: testAccProviders,
 CheckDestroy: testAccCheckFlowLogDestroy,
 Steps: []resource.TestStep{
 {
- Config: testAccFlowLogConfig_basic(rInt),
+ Config: testAccFlowLogConfig_vpcOldSyntaxCloudWatch(rInt),


For configurations using the "old" syntax, we should ensure that the "old" attributes are still available as expected. The same can also be used to verify it available in the "new" attribute. e.g.

resource.TestCheckResourceAttrPair("aws_flow_log.test_flow_log_vpc", "log_group_name", "aws_cloudwatch_log_group.foobar", "name"), resource.TestCheckResourceAttrPair("aws_flow_log.test_flow_log_vpc", "resource_id", "aws_vpc.default", "id"), resource.TestCheckResourceAttrPair("aws_flow_log.test_flow_log_vpc", "vpc_id", "aws_vpc.default", "id"),

bflad · 2018-10-15T12:45:56Z

aws/resource_aws_flow_log_test.go

@@ -68,16 +104,152 @@ func TestAccAWSFlowLog_subnet(t *testing.T) {
 CheckDestroy: testAccCheckFlowLogDestroy,
 Steps: []resource.TestStep{
 {
- Config: testAccFlowLogConfig_subnet(rInt),
+ Config: testAccFlowLogConfig_subnetOldSyntaxCloudWatch(rInt),


Same here -- especially for configurations using the "old" syntax, we should ensure that the "old" attributes are still available as expected. The same can also be used to verify it available in the "new" attribute. e.g.

resource.TestCheckResourceAttrPair("aws_flow_log.test_flow_log_vpc", "log_group_name", "aws_cloudwatch_log_group.foobar", "name"), resource.TestCheckResourceAttrPair("aws_flow_log.test_flow_log_vpc", "resource_id", "aws_subnet.test_subnet", "id"), resource.TestCheckResourceAttrPair("aws_flow_log.test_flow_log_vpc", "subnet_id", "aws_subnet.test_subnet", "id"),

bflad · 2018-10-15T12:48:05Z

aws/resource_aws_flow_log_test.go

+ CheckDestroy: testAccCheckFlowLogDestroy,
+ Steps: []resource.TestStep{
+ {
+ Config: testAccFlowLogConfig_eniOldSyntaxCloudWatch(rInt),


Same here -- especially for configurations using the "old" syntax, we should ensure that the "old" attributes are still available as expected. The same can also be used to verify it available in the "new" attribute. e.g.

resource.TestCheckResourceAttrPair("aws_flow_log.test_flow_log_vpc", "log_group_name", "aws_cloudwatch_log_group.foobar", "name"), resource.TestCheckResourceAttrPair("aws_flow_log.test_flow_log_vpc", "eni_id", "aws_network_interface.test_eni", "id"), resource.TestCheckResourceAttrPair("aws_flow_log.test_flow_log_vpc", "resource_id", "aws_network_interface.test_eni", "id"),

bflad · 2018-10-15T14:24:14Z

For a sample of the minimum required changes to support S3 logging, please see: https://github.com/terraform-providers/terraform-provider-aws/compare/f-aws_flow_log-log_destination?expand=1

ewbankkit · 2018-10-16T11:53:15Z

@bflad Agreed that the currently proposed changes are large and a more lightweight approach may be more appropriate to land this much-requested feature as soon as makes sense.
The proposed alternative changes look sensible.
I'll let @ryandeivert make the call as he's the owner of the PR.

ryandeivert · 2018-10-19T17:11:27Z

@bflad / @ewbankkit I'm more than happy with a more minimal approach to this. would you suggest that I modify this PR or open a new one with those changes?

bflad · 2018-10-19T17:25:56Z

@ryandeivert I'll leave that up to you, but I'm sure folks here would probably appreciate whatever is quickest -- you can steal any of my branch if you wish or you/I can just create a new PR 😄

* Add `log_destination` and `log_destination_type` arguments * Deprecate `log_group_name` and conflict it with `log_destination` * Mark `iam_role_arn` as Optional ``` $ make testacc TEST=./aws TESTARGS='-run=TestAccAWSFlowLog_' ==> Checking that code complies with gofmt requirements... TF_ACC=1 go test ./aws -v -run=TestAccAWSFlowLog_ -timeout 120m === RUN TestAccAWSFlowLog_VPCID --- PASS: TestAccAWSFlowLog_VPCID (75.46s) === RUN TestAccAWSFlowLog_SubnetID --- PASS: TestAccAWSFlowLog_SubnetID (28.62s) === RUN TestAccAWSFlowLog_LogDestinationType_CloudWatchLogs --- PASS: TestAccAWSFlowLog_LogDestinationType_CloudWatchLogs (28.68s) === RUN TestAccAWSFlowLog_LogDestinationType_S3 --- PASS: TestAccAWSFlowLog_LogDestinationType_S3 (153.81s) PASS ok github.com/terraform-providers/terraform-provider-aws/aws 287.945s ```

ryandeivert · 2018-10-19T21:33:31Z

thanks @bflad I just reset my commits here and cherry picked your commit. output of acceptance tests:

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSFlowLog_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -parallel 20 -run=TestAccAWSFlowLog_ -timeout 120m
=== RUN   TestAccAWSFlowLog_VPCID
=== PAUSE TestAccAWSFlowLog_VPCID
=== RUN   TestAccAWSFlowLog_SubnetID
--- PASS: TestAccAWSFlowLog_SubnetID (30.24s)
=== RUN   TestAccAWSFlowLog_LogDestinationType_CloudWatchLogs
--- PASS: TestAccAWSFlowLog_LogDestinationType_CloudWatchLogs (24.43s)
=== RUN   TestAccAWSFlowLog_LogDestinationType_S3
--- PASS: TestAccAWSFlowLog_LogDestinationType_S3 (28.18s)
=== CONT  TestAccAWSFlowLog_VPCID
--- PASS: TestAccAWSFlowLog_VPCID (40.73s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	124.117s

amine250 · 2018-10-29T15:56:08Z

Hi, when can we expect a Terraform release that's having this change?

bflad · 2018-10-29T16:19:55Z

@amine250 we typically release on Wednesdays, see also: https://github.com/terraform-providers/terraform-provider-aws/blob/master/CHANGELOG.md

bflad · 2018-11-01T00:21:52Z

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

amine250 · 2018-11-06T16:44:36Z

How can I make use of a aws_flow_log that logs on S3 inside a aws_subnet resource?
I have multiple subnets like:

resource "aws_subnet" "webapp" {
  availability_zone = "${data.aws_region.current.name}a"
  tags {
    Name               = "MyWebAppSubnetName"
  }
}

Shouldn't there be an argument for aws_subnet such as :

resource "aws_subnet" "webapp" {
  flow_logs {
    log_destination      = "${aws_s3_bucket.example.arn}"
    log_destination_type = "s3"
    traffic_type         = "ALL"
    vpc_id               = "${aws_vpc.example.id}"
  }
}

tomelliff · 2018-11-08T11:47:09Z

@amine250 What makes you think it should be a configuration option on the aws_subnet resource instead of just being created in the aws_flow_log resource?

amine250 · 2018-11-12T16:03:16Z

@tomelliff I'm new to Terraform so I could be saying BS.
Would you please show me a working example? Thanks.

tomelliff · 2018-11-12T16:17:53Z

flow_logs isn't a parameter for the aws_subnet resource. If you want to add a flow log for a subnet then you should use the aws_flow_log resource and specify a subnet_id to point to your subnet.

If you're running into issues with that then you should raise a separate issue rather than reply to this pull request which added the S3 support for the aws_flow_log resource.

amine250 · 2018-11-13T08:44:45Z

@tomelliff I think we are missing an attribute to specify S3 prefix for an aws_flow_log resource.

ghost · 2020-04-02T17:29:45Z

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 added the size/L Managed by automation to categorize the size of a PR. label Aug 10, 2018

ewbankkit reviewed Aug 11, 2018

View reviewed changes

radeksimko added enhancement Requests to existing resources that expand the functionality or scope. service/ec2 Issues and PRs that pertain to the ec2 service. labels Aug 13, 2018

ghost added size/L Managed by automation to categorize the size of a PR. labels Aug 13, 2018

ghost added size/L Managed by automation to categorize the size of a PR. labels Aug 14, 2018

ewbankkit mentioned this pull request Aug 20, 2018

Add send VPC flow logs to S3 AWS Provider #5600

Closed

ghost added the size/L Managed by automation to categorize the size of a PR. label Sep 13, 2018

bflad self-assigned this Oct 5, 2018

bflad reviewed Oct 15, 2018

View reviewed changes

bflad added waiting-response Maintainers are waiting on response from community or contributor. breaking-change Introduces a breaking change in current functionality; usually deferred to the next major release. labels Oct 15, 2018

ghost added size/XL Managed by automation to categorize the size of a PR. and removed size/XXL Managed by automation to categorize the size of a PR. labels Oct 19, 2018

bflad removed breaking-change Introduces a breaking change in current functionality; usually deferred to the next major release. waiting-response Maintainers are waiting on response from community or contributor. labels Oct 24, 2018

bflad added this to the v1.42.0 milestone Oct 25, 2018

paultyng approved these changes Oct 25, 2018

View reviewed changes

bflad merged commit 18aa08b into hashicorp:master Oct 25, 2018

bflad added a commit that referenced this pull request Oct 25, 2018

Update CHANGELOG for #5509

22f93ab

ghost locked and limited conversation to collaborators Apr 2, 2020


		var resourceKey string
		if strings.HasPrefix(*fl.ResourceId, "vpc-") {

aws_flow_log - adding support for sending to S3 #5509

aws_flow_log - adding support for sending to S3 #5509

Conversation

ryandeivert commented Aug 10, 2018 • edited Loading

Choose a reason for hiding this comment

ryandeivert Aug 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ewbankkit commented Aug 11, 2018

Choose a reason for hiding this comment

ryandeivert commented Aug 14, 2018

ryandeivert commented Aug 14, 2018 • edited Loading

ewbankkit commented Aug 15, 2018

ryandeivert commented Aug 15, 2018

ewbankkit commented Aug 15, 2018

ryandeivert commented Aug 15, 2018

mikecook commented Aug 25, 2018

tomelliff commented Aug 28, 2018

mikecook commented Aug 31, 2018

ryandeivert commented Aug 31, 2018

ewbankkit commented Sep 2, 2018

ryandeivert commented Sep 4, 2018

uovobw commented Sep 5, 2018

ryandeivert commented Sep 5, 2018

uovobw commented Sep 5, 2018

AzCii commented Sep 11, 2018

vmartin01 commented Oct 5, 2018

bflad commented Oct 5, 2018

bflad 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad commented Oct 15, 2018

ewbankkit commented Oct 16, 2018

ryandeivert commented Oct 19, 2018

bflad commented Oct 19, 2018

ryandeivert commented Oct 19, 2018

amine250 commented Oct 29, 2018

bflad commented Oct 29, 2018

bflad commented Nov 1, 2018

amine250 commented Nov 6, 2018

tomelliff commented Nov 8, 2018

amine250 commented Nov 12, 2018

tomelliff commented Nov 12, 2018

amine250 commented Nov 13, 2018 • edited Loading

ghost commented Apr 2, 2020

ryandeivert commented Aug 10, 2018 •

edited

Loading

ryandeivert Aug 13, 2018 •

edited

Loading

ryandeivert commented Aug 14, 2018 •

edited

Loading

amine250 commented Nov 13, 2018 •

edited

Loading