provider/aws: Add "enable_logging" to CloudTrail resource #4010

pforman · 2015-11-21T22:09:03Z

I was working with the CloudTrail resource (#3094, merged for release in 0.6.7) and discovered that AWS creates CloudTrail trails with logging disabled by default. The user needs to click the button in the console, or send a cloudtrail:StartLogging API call to begin using the trail.

Terraform will create the trail, but had no ability to enable (or monitor the ongoing state of) logging on the trail resource.

I didn't want that to be managed or changed outside my TF config, so I wrote this PR to add an enable_logging parameter to the CloudTrail resource. It requires extra API calls (StartLogging to enable or StopLogging to disable), so I wrapped those in helper functions inside resource_aws_cloudtrail.go. The CRUD operations use these to try to do the right thing.

The default is still false to match existing AWS behavior. I wrote some acceptance tests for enabling and disabling logging, and added the option to the documentation.

Please look through the implementation and let me know if there's something that looks off, I'm happy to fix it or rework it if necessary. Big thanks to @radeksimko for writing the resource in the first place.

make testacc TEST=./builtin/providers/aws TESTARGS='-run=CloudTrail' 2>/dev/null
go generate ./...
TF_ACC=1 go test ./builtin/providers/aws -v -run=CloudTrail -timeout 90m
=== RUN TestAccAWSCloudTrail_basic
--- PASS: TestAccAWSCloudTrail_basic (15.53s)
=== RUN TestAccAWSCloudTrail_enable_logging
--- PASS: TestAccAWSCloudTrail_enable_logging (19.95s)
PASS
ok github.com/hashicorp/terraform/builtin/providers/aws 35.499s

The AWS CloudTrail resource is capable of creating CloudTrail resources, but AWS defaults the actual logging of the trails to `false`, and Terraform has no method to enable or monitor the status of logging. CloudTrail trails that are inactive aren't very useful, and it's a surprise to discover they aren't logging on creation. Added an `enable_logging` parameter to resource_aws_cloudtrail to enable logging. This requires some extra API calls, which are wrapped in new internal functions. For compatibility with AWS, the default of `enable_logging` is set to `false`.

Add acceptance tests for creation, enable, and disable logging. Add option to docs and example.

The purpose of the first test of enable_logging wasn't quite clear. It's future-proofing against the assumptions made about AWS behavior.

radeksimko · 2015-11-21T22:16:52Z

Thanks a lot for catching this!

Since I wrote the implementation for TF, I didn't get to actually test it thoroughly in practice yet (except in the acceptance test), hence I did not realise it's disabled by default. 😮

Even though it may be considered as a breaking change, I would rather enable it by default.
I think it's better to enable by accident than disable.

radeksimko · 2015-11-21T22:19:40Z

builtin/providers/aws/resource_aws_cloudtrail.go

+			"enable_logging": &schema.Schema{
+				Type:     schema.TypeBool,
+				Optional: true,
+				Default:  false,


You can keep the field name, just change this to Default: true and it should be good to go. 😃

pforman · 2015-11-21T22:29:28Z

I waffled on whether to enable by default or not. It seems like the right thing to do, under the assumption that if you declare a trail you'd like to use it.

However, I also figured that matching AWS behavior and not being surprising was a good thing. (of course, AWS behavior was the real surprise here...)

I ended up deciding to just add it with the AWS behavior, and note the default. Changing the default to true will require a small update to the acceptance tests, there's a future-proof test in there to make sure AWS doesn't change their default behavior.

However, if it's desired to default to true, I can adjust it. Fine either way to me.

radeksimko · 2015-11-22T08:47:27Z

I would enable it by default. Also CloudTrail wasn't released yet, so it won't actually be breaking anything as long as we'll make it until Monday, when we're most likely cutting the next release.

I think many people rely on CloudTrail as one of the security mechanisms and I believe they'd rather have this enabled than disabled. The pricing is also a good argument why rather enable it and why it shouldn't matter too much if you'd enable it by accident:

https://aws.amazon.com/cloudtrail/pricing/

There is no additional charge for CloudTrail, but standard rates for Amazon S3 and Amazon SNS usage apply. Monthly Amazon S3 charges for CloudTrail usage are typically less than $3 per account for most customers¹. Monthly Amazon SNS charges for CloudTrail usage are typically less than $1 per account for most customers².

pforman · 2015-11-22T17:02:59Z

Yes, I agree. Default true is a sensible position.

I'll rework the tests to expect that default and change the doc entry. I'll get an update to the PR in today so hopefully the CloudTrail stuff can all go together on Monday.

The default for `enable_logging`, which defines whether CloudTrail actually logs events was originally written as defaulting to `false`, since that's how AWS creates trails. `true` is likely a better default for Terraform users. Changed the default and updated the docs. Changed the acceptance tests to verify new default behavior.

pforman · 2015-11-22T17:52:14Z

Tested on my own configs, does what it should. Also updated the docs in that commit.

Reworked the acceptance tests, they still look good:

make testacc TEST=./builtin/providers/aws TESTARGS='-run=CloudTrail' 2>/dev/null
go generate ./...
TF_ACC=1 go test ./builtin/providers/aws -v -run=CloudTrail -timeout 90m
=== RUN   TestAccAWSCloudTrail_basic
--- PASS: TestAccAWSCloudTrail_basic (19.62s)
=== RUN   TestAccAWSCloudTrail_enable_logging
--- PASS: TestAccAWSCloudTrail_enable_logging (26.48s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    46.122s

radeksimko · 2015-11-22T18:41:32Z

builtin/providers/aws/resource_aws_cloudtrail.go

@@ -84,6 +89,11 @@ func resourceAwsCloudTrailCreate(d *schema.ResourceData, meta interface{}) error

 	d.SetId(*t.Name)

+	// AWS CloudTrail sets newly-created trails to false.
+	if v, ok := d.GetOk("enable_logging"); ok && v.(bool) {
+		cloudTrailSetLogging(conn, v.(bool), d.Id())


Can you add error checking here, please? Otherwise any error coming out from cloudTrailSetLogging would be ignored.

radeksimko · 2015-11-22T18:55:44Z

@pforman Thanks for the quick update, I appreciate it. Acceptance tests are 👌

I left you some comments there, some are more serious (error checking) some are just nitpicks. It would be nice if you managed to fix the error checking at least.

I will check this PR again tomorrow morning (UK time) and may eventually pick it up (in case you would not get chance to look at it by that time).

pforman · 2015-11-22T19:13:01Z

Thanks for the thorough review, looks like I dropped some errors there, which is bad.

I'll see if I can get an update in. If not (or if I do and there's still something off), go ahead and pick it up. I'm heading out of town for a while tomorrow, so I won't be back immediately to work on it anyway. It would be good to see it in 0.6.7 with the problems fixed.

Some error-checking was omitted. Specifically, the cloudTrailSetLogging call in the Create function was ignoring the return and cloudTrailGetLoggingStatus could crash on a nil-dereference during the return. Fixed both. Fixed some needless casting in cloudTrailGetLoggingStatus. Clarified error message in acceptance tests. Removed needless option from example in docs.

When adjusting the types to prevent casting, I didn't change the error message to handle the pointer change. "go tool vet" caught this.

pforman · 2015-11-22T20:26:01Z

Okay, I think I handled everything in the review.

Added the SetLogging error check in Create (Update had this, but Create didn't...)
Added the explicit error check in GetLoggingStatus to avoid nil-dereference
Changed the types in GetLoggingStatus to avoid unecessary casting
Clarified the error message in the acceptance test
Removed needless option in the example.

If there's anything else that you see, please feel free to fix it, I'll be on the road tomorrow and won't have a chance to update it before 0.6.7.

provider/aws: Add "enable_logging" to CloudTrail resource

ghost · 2020-04-29T02:36: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 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.

Paul Forman added 3 commits November 21, 2015 00:18

Tests and docs for AWS CloudTrail "enable_logging"

f98dbbb

Add acceptance tests for creation, enable, and disable logging. Add option to docs and example.

Add a comment in tests

c9eeb16

The purpose of the first test of enable_logging wasn't quite clear. It's future-proofing against the assumptions made about AWS behavior.

radeksimko added enhancement provider/aws labels Nov 21, 2015

radeksimko reviewed Nov 21, 2015
View reviewed changes

radeksimko reviewed Nov 22, 2015
View reviewed changes

Paul Forman added 2 commits November 22, 2015 12:54

Mistake in type refactor in cloudTrailGetLoggingStatus

52aad04

When adjusting the types to prevent casting, I didn't change the error message to handle the pointer change. "go tool vet" caught this.

radeksimko added a commit that referenced this pull request Nov 23, 2015

Merge pull request #4010 from pforman/cloudtrail-enable

749fcd4

provider/aws: Add "enable_logging" to CloudTrail resource

radeksimko merged commit 749fcd4 into hashicorp:master Nov 23, 2015

radeksimko mentioned this pull request Nov 23, 2015

v0.6.7 Release Process #3964

Closed

ghost locked and limited conversation to collaborators Apr 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

provider/aws: Add "enable_logging" to CloudTrail resource #4010

provider/aws: Add "enable_logging" to CloudTrail resource #4010

pforman commented Nov 21, 2015

radeksimko commented Nov 21, 2015

radeksimko Nov 21, 2015

pforman commented Nov 21, 2015

radeksimko commented Nov 22, 2015

pforman commented Nov 22, 2015

pforman commented Nov 22, 2015

radeksimko Nov 22, 2015

radeksimko commented Nov 22, 2015

pforman commented Nov 22, 2015

pforman commented Nov 22, 2015

ghost commented Apr 29, 2020

provider/aws: Add "enable_logging" to CloudTrail resource #4010

provider/aws: Add "enable_logging" to CloudTrail resource #4010

Conversation

pforman commented Nov 21, 2015

radeksimko commented Nov 21, 2015

radeksimko Nov 21, 2015

Choose a reason for hiding this comment

pforman commented Nov 21, 2015

radeksimko commented Nov 22, 2015

pforman commented Nov 22, 2015

pforman commented Nov 22, 2015

radeksimko Nov 22, 2015

Choose a reason for hiding this comment

radeksimko commented Nov 22, 2015

pforman commented Nov 22, 2015

pforman commented Nov 22, 2015

ghost commented Apr 29, 2020