provider/aws: Add FlowLog resource #2384

catsby · 2015-06-17T18:04:11Z

This is a proposal config/work for adding a FlowLog resource.

Syntax:

resource "aws_flow_log" "test_flow_log" {
  log_group_name = "tf-test-log-group"
  iam_role_arn = "arn:aws:iam::470663696735:role/tf-test-cloud"
  vpc_id = "${aws_vpc.default.id}"
  traffic_type = "ALL"
}

API Docs:

CreateFlowLogs
Describe FlowLogs
Delete FlowLogs
There is no Update or Modify, things will be ForceNew.

A FlowLog can be attached to either a VPC, Subnet, or ENI. The API call is the same, you just supply the resource id and type. Here I propose separate vpc_id, subnet_id, and eni_id syntax, to avoid a user having to specify both a resource id and the type.

Additionally, the API supports creating multiple flows in a single call, but this creates multiple FlowLogs, , so our implementation will be 1:1.

Remaining:

Docs
Implementation
Acceptance tests

phinze · 2015-06-17T21:29:56Z

builtin/providers/aws/provider.go

@@ -85,12 +85,13 @@ func Provider() terraform.ResourceProvider {
 		ResourcesMap: map[string]*schema.Resource{
 			"aws_app_cookie_stickiness_policy": resourceAwsAppCookieStickinessPolicy(),
 			"aws_autoscaling_group":            resourceAwsAutoscalingGroup(),
+			"aws_flow_log":                     resourceAwsFlowLog(),


Alphabetize por favor! 🔠 😁

catsby · 2015-06-17T21:45:43Z

cc @radeksimko since I know he has at least looked into Flow Logs

phinze · 2015-06-17T21:46:13Z

builtin/providers/aws/resource_aws_flow_log.go

+	return nil
+}
+
+func flowLogStateRefreshFunc(conn *ec2.EC2, sn string) resource.StateRefreshFunc {


Looks like this guy is unused.

phinze · 2015-06-17T21:50:42Z

Review complete - Looking great overall!

radeksimko · 2015-06-17T22:29:14Z

builtin/providers/aws/resource_aws_flow_log_test.go

+	"testing"
+
+	"github.com/aws/aws-sdk-go/service/ec2"
+	"github.com/hashicorp/aws-sdk-go/aws"


why this import?

Wow, great catch! My editor must have auto populated that 😱

Fixed in 0bf127a

catsby · 2015-06-18T13:37:44Z

@phinze cleaned up, sorry for the sloppy 😐

radeksimko · 2015-06-18T14:30:46Z

builtin/providers/aws/resource_aws_flow_log.go

+	}
+
+	log.Printf(
+		"[DEBUG] Flow Log Create configuration: %s", awsutil.StringValue(opts))


Nice! 👍 Isn't this basically addressing #2179 ?

awsutil.StringValue does, yes, but to close #2179 we need to use it in All The Places™

radeksimko · 2015-06-18T15:01:36Z

@catsby It looks ok, just one question here:
What is the benefit behind saving flow_log_status and deliver_log_status?

I thought that change in the state should be changed because user/human has done something in order to change the state. My understanding is that these two things will change without user's interaction over time and so far I don't see how would any other Terraform resource react on that.

radeksimko · 2015-06-18T15:10:30Z

I reckon this can be fixed later on after merging, but just FYI regarding example in docs:

http://docs.aws.amazon.com/cli/latest/reference/ec2/create-flow-logs.html

In your request, you must also specify an IAM role that has permission to publish logs to CloudWatch Logs.

We're not defining that permission anywhere.

* master: Update CHANGELOG.md Update CHANGELOG.md Added affinity group resource. update link to actually work provider/azure: Fix SQL client name to match upstream add warning message to explain scenario of conflicting rules typo remove debugging Update CHANGELOG.md provider/aws: Add docs for autoscaling_policy + cloudwatch_metric_alarm provider/aws: Add autoscaling_policy provider/aws: Add cloudwatch_metric_alarm rename method, update docs clean up some conflicts with clean up old, incompatible test update tests with another example update test remove meta usage, stub test fix existing tests Consider security groups with source security groups when hashing

catsby · 2015-06-22T15:09:05Z

@radeksimko Thanks for reviewing

What is the benefit behind saving flow_log_status and deliver_log_status?

I thought it would be useful, but in hindsight agree, so I removed them in 87c7f63

We're not defining that permission anywhere.

I updated the documentation to include the needed IAM things to get Flow Logs working, and verified they work. The Logs must Flow

phinze · 2015-06-23T20:07:37Z

LGTM

provider/aws: Add FlowLog resource

ghost · 2020-05-02T02:01:46Z

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.

catsby added enhancement provider/aws proposal labels Jun 17, 2015

catsby added 2 commits June 17, 2015 16:29

provider/aws: Docs for FlowLog resource

cbb6b97

additional attributes exported

641a86e

phinze reviewed Jun 17, 2015
View reviewed changes

provider/aws: Implement AWS Flow Logs

bfdf11c

catsby force-pushed the f-aws-flow-logs branch from dd4c228 to 6c5c9b8 Compare June 17, 2015 21:32

Log Group Name is required/forcenew

1a6aef0

catsby force-pushed the f-aws-flow-logs branch from 6c5c9b8 to 1a6aef0 Compare June 17, 2015 21:33

fix go vet error

b3d7bb2

phinze reviewed Jun 17, 2015
View reviewed changes

radeksimko reviewed Jun 17, 2015
View reviewed changes

catsby changed the title ~~Proposal provider/aws: Docs for FlowLog resource~~ provider/aws: Add FlowLog resource Jun 18, 2015

catsby added 2 commits June 18, 2015 08:28

code cleanups

285a88b

update test; fix import, interpolate ENV var for log name

0bf127a

radeksimko reviewed Jun 18, 2015
View reviewed changes

radeksimko mentioned this pull request Jun 21, 2015

provider/aws: Add cloudwatch_log_group #2415

Merged

5 tasks

catsby added 2 commits June 22, 2015 09:25

clean up extra indentation

d5f962b

remove flow/deliver log status attributes

87c7f63

catsby added 2 commits June 22, 2015 09:33

update docs to include IAM roles

8e23607

catsby added a commit that referenced this pull request Jun 23, 2015

Merge pull request #2384 from hashicorp/f-aws-flow-logs

24c4c55

provider/aws: Add FlowLog resource

catsby merged commit 24c4c55 into master Jun 23, 2015

catsby deleted the f-aws-flow-logs branch June 23, 2015 20:08

ghost locked and limited conversation to collaborators May 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

provider/aws: Add FlowLog resource #2384

provider/aws: Add FlowLog resource #2384

catsby commented Jun 17, 2015

phinze Jun 17, 2015

catsby commented Jun 17, 2015

phinze Jun 17, 2015

catsby Jun 18, 2015

phinze commented Jun 17, 2015

radeksimko Jun 17, 2015

catsby Jun 18, 2015

catsby commented Jun 18, 2015

radeksimko Jun 18, 2015

catsby Jun 18, 2015

radeksimko commented Jun 18, 2015

radeksimko commented Jun 18, 2015

catsby commented Jun 22, 2015

phinze commented Jun 23, 2015

ghost commented May 2, 2020

provider/aws: Add FlowLog resource #2384

provider/aws: Add FlowLog resource #2384

Conversation

catsby commented Jun 17, 2015

phinze Jun 17, 2015

Choose a reason for hiding this comment

catsby commented Jun 17, 2015

phinze Jun 17, 2015

Choose a reason for hiding this comment

catsby Jun 18, 2015

Choose a reason for hiding this comment

phinze commented Jun 17, 2015

radeksimko Jun 17, 2015

Choose a reason for hiding this comment

catsby Jun 18, 2015

Choose a reason for hiding this comment

catsby commented Jun 18, 2015

radeksimko Jun 18, 2015

Choose a reason for hiding this comment

catsby Jun 18, 2015

Choose a reason for hiding this comment

radeksimko commented Jun 18, 2015

radeksimko commented Jun 18, 2015

catsby commented Jun 22, 2015

phinze commented Jun 23, 2015

ghost commented May 2, 2020