From 52db098292e3768208ac914f6a5240a8d5e74f9e Mon Sep 17 00:00:00 2001 From: Paul Forman Date: Fri, 20 Nov 2015 22:51:40 -0700 Subject: [PATCH 1/6] Add enable_logging to AWS CloudTrail 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`. --- .../providers/aws/resource_aws_cloudtrail.go | 64 +++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/builtin/providers/aws/resource_aws_cloudtrail.go b/builtin/providers/aws/resource_aws_cloudtrail.go index eed420edf..82a9172c0 100644 --- a/builtin/providers/aws/resource_aws_cloudtrail.go +++ b/builtin/providers/aws/resource_aws_cloudtrail.go @@ -22,6 +22,11 @@ func resourceAwsCloudTrail() *schema.Resource { Required: true, ForceNew: true, }, + "enable_logging": &schema.Schema{ + Type: schema.TypeBool, + Optional: true, + Default: false, + }, "s3_bucket_name": &schema.Schema{ Type: schema.TypeString, Required: true, @@ -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()) + } + return resourceAwsCloudTrailRead(d, meta) } @@ -115,6 +125,12 @@ func resourceAwsCloudTrailRead(d *schema.ResourceData, meta interface{}) error { d.Set("include_global_service_events", trail.IncludeGlobalServiceEvents) d.Set("sns_topic_name", trail.SnsTopicName) + logstatus, err := cloudTrailGetLoggingStatus(conn, *trail.Name) + if err != nil { + return err + } + d.Set("enable_logging", logstatus) + return nil } @@ -149,6 +165,15 @@ func resourceAwsCloudTrailUpdate(d *schema.ResourceData, meta interface{}) error if err != nil { return err } + + if d.HasChange("enable_logging") { + log.Printf("[DEBUG] Updating logging on CloudTrail: %s", input) + err := cloudTrailSetLogging(conn, d.Get("enable_logging").(bool), *input.Name) + if err != nil { + return err + } + } + log.Printf("[DEBUG] CloudTrail updated: %s", t) return resourceAwsCloudTrailRead(d, meta) @@ -165,3 +190,42 @@ func resourceAwsCloudTrailDelete(d *schema.ResourceData, meta interface{}) error return err } + +func cloudTrailGetLoggingStatus(conn *cloudtrail.CloudTrail, id string) (bool, error) { + GetTrailStatusOpts := &cloudtrail.GetTrailStatusInput{ + Name: aws.String(id), + } + resp, err := conn.GetTrailStatus(GetTrailStatusOpts) + + return *resp.IsLogging, err +} + +func cloudTrailSetLogging(conn *cloudtrail.CloudTrail, enabled bool, id string) error { + if enabled { + log.Printf( + "[DEBUG] Starting logging on CloudTrail (%s)", + id) + StartLoggingOpts := &cloudtrail.StartLoggingInput{ + Name: aws.String(id), + } + if _, err := conn.StartLogging(StartLoggingOpts); err != nil { + return fmt.Errorf( + "Error starting logging on CloudTrail (%s): %s", + id, err) + } + } else { + log.Printf( + "[DEBUG] Stopping logging on CloudTrail (%s)", + id) + StopLoggingOpts := &cloudtrail.StopLoggingInput{ + Name: aws.String(id), + } + if _, err := conn.StopLogging(StopLoggingOpts); err != nil { + return fmt.Errorf( + "Error stopping logging on CloudTrail (%s): %s", + id, err) + } + } + + return nil +} From f98dbbb580b078214e4412333e3314182d38684b Mon Sep 17 00:00:00 2001 From: Paul Forman Date: Sat, 21 Nov 2015 00:15:29 -0700 Subject: [PATCH 2/6] Tests and docs for AWS CloudTrail "enable_logging" Add acceptance tests for creation, enable, and disable logging. Add option to docs and example. --- .../aws/resource_aws_cloudtrail_test.go | 58 +++++++++++++++++++ .../providers/aws/r/cloudtrail.html.markdown | 2 + 2 files changed, 60 insertions(+) diff --git a/builtin/providers/aws/resource_aws_cloudtrail_test.go b/builtin/providers/aws/resource_aws_cloudtrail_test.go index 10ed17a5b..7bf76219b 100644 --- a/builtin/providers/aws/resource_aws_cloudtrail_test.go +++ b/builtin/providers/aws/resource_aws_cloudtrail_test.go @@ -39,6 +39,39 @@ func TestAccAWSCloudTrail_basic(t *testing.T) { }) } +func TestAccAWSCloudTrail_enable_logging(t *testing.T) { + var trail cloudtrail.Trail + + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckAWSCloudTrailDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: testAccAWSCloudTrailConfig, + Check: resource.ComposeTestCheckFunc( + testAccCheckCloudTrailExists("aws_cloudtrail.foobar", &trail), + testAccCheckCloudTrailLoggingEnabled("aws_cloudtrail.foobar", false, &trail), + ), + }, + resource.TestStep{ + Config: testAccAWSCloudTrailConfigModified, + Check: resource.ComposeTestCheckFunc( + testAccCheckCloudTrailExists("aws_cloudtrail.foobar", &trail), + testAccCheckCloudTrailLoggingEnabled("aws_cloudtrail.foobar", true, &trail), + ), + }, + resource.TestStep{ + Config: testAccAWSCloudTrailConfig, + Check: resource.ComposeTestCheckFunc( + testAccCheckCloudTrailExists("aws_cloudtrail.foobar", &trail), + testAccCheckCloudTrailLoggingEnabled("aws_cloudtrail.foobar", false, &trail), + ), + }, + }, + }) +} + func testAccCheckCloudTrailExists(n string, trail *cloudtrail.Trail) resource.TestCheckFunc { return func(s *terraform.State) error { rs, ok := s.RootModule().Resources[n] @@ -63,6 +96,30 @@ func testAccCheckCloudTrailExists(n string, trail *cloudtrail.Trail) resource.Te } } +func testAccCheckCloudTrailLoggingEnabled(n string, desired bool, trail *cloudtrail.Trail) resource.TestCheckFunc { + return func(s *terraform.State) error { + rs, ok := s.RootModule().Resources[n] + if !ok { + return fmt.Errorf("Not found: %s", n) + } + + conn := testAccProvider.Meta().(*AWSClient).cloudtrailconn + params := cloudtrail.GetTrailStatusInput{ + Name: aws.String(rs.Primary.ID), + } + resp, err := conn.GetTrailStatus(¶ms) + + if err != nil { + return err + } + if *resp.IsLogging != desired { + return fmt.Errorf("Logging status is incorrect") + } + + return nil + } +} + func testAccCheckAWSCloudTrailDestroy(s *terraform.State) error { conn := testAccProvider.Meta().(*AWSClient).cloudtrailconn @@ -134,6 +191,7 @@ resource "aws_cloudtrail" "foobar" { s3_bucket_name = "${aws_s3_bucket.foo.id}" s3_key_prefix = "/prefix" include_global_service_events = false + enable_logging = true } resource "aws_s3_bucket" "foo" { diff --git a/website/source/docs/providers/aws/r/cloudtrail.html.markdown b/website/source/docs/providers/aws/r/cloudtrail.html.markdown index d4ba604fc..0f21f4670 100644 --- a/website/source/docs/providers/aws/r/cloudtrail.html.markdown +++ b/website/source/docs/providers/aws/r/cloudtrail.html.markdown @@ -16,6 +16,7 @@ resource "aws_cloudtrail" "foobar" { name = "tf-trail-foobar" s3_bucket_name = "${aws_s3_bucket.foo.id}" s3_key_prefix = "/prefix" + enable_logging = true include_global_service_events = false } @@ -63,6 +64,7 @@ The following arguments are supported: endpoint to assume to write to a user’s log group. * `cloud_watch_logs_group_arn` - (Optional) Specifies a log group name using an Amazon Resource Name (ARN), that represents the log group to which CloudTrail logs will be delivered. +* `enable_logging` - (Optional) Enables logging for the trail. Defaults to `false`. * `include_global_service_events` - (Optional) Specifies whether the trail is publishing events from global services such as IAM to the log files. Defaults to `true`. * `sns_topic_name` - (Optional) Specifies the name of the Amazon SNS topic From c9eeb161e0d25732902cb59d06362addd54fb52d Mon Sep 17 00:00:00 2001 From: Paul Forman Date: Sat, 21 Nov 2015 14:55:08 -0700 Subject: [PATCH 3/6] Add a comment in tests The purpose of the first test of enable_logging wasn't quite clear. It's future-proofing against the assumptions made about AWS behavior. --- builtin/providers/aws/resource_aws_cloudtrail_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/builtin/providers/aws/resource_aws_cloudtrail_test.go b/builtin/providers/aws/resource_aws_cloudtrail_test.go index 7bf76219b..47a89083c 100644 --- a/builtin/providers/aws/resource_aws_cloudtrail_test.go +++ b/builtin/providers/aws/resource_aws_cloudtrail_test.go @@ -51,6 +51,8 @@ func TestAccAWSCloudTrail_enable_logging(t *testing.T) { Config: testAccAWSCloudTrailConfig, Check: resource.ComposeTestCheckFunc( testAccCheckCloudTrailExists("aws_cloudtrail.foobar", &trail), + // This is a warning test. AWS sets up new trails with logging disabled + // Should that change in the future, this test should fail. testAccCheckCloudTrailLoggingEnabled("aws_cloudtrail.foobar", false, &trail), ), }, From 484887c0c5585b5db336da6f5484234bda1c853f Mon Sep 17 00:00:00 2001 From: Paul Forman Date: Sun, 22 Nov 2015 10:47:23 -0700 Subject: [PATCH 4/6] Change default for logging in CloudTrail to true 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. --- builtin/providers/aws/resource_aws_cloudtrail.go | 2 +- .../providers/aws/resource_aws_cloudtrail_test.go | 12 ++++++------ .../docs/providers/aws/r/cloudtrail.html.markdown | 3 ++- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/builtin/providers/aws/resource_aws_cloudtrail.go b/builtin/providers/aws/resource_aws_cloudtrail.go index 82a9172c0..303073422 100644 --- a/builtin/providers/aws/resource_aws_cloudtrail.go +++ b/builtin/providers/aws/resource_aws_cloudtrail.go @@ -25,7 +25,7 @@ func resourceAwsCloudTrail() *schema.Resource { "enable_logging": &schema.Schema{ Type: schema.TypeBool, Optional: true, - Default: false, + Default: true, }, "s3_bucket_name": &schema.Schema{ Type: schema.TypeString, diff --git a/builtin/providers/aws/resource_aws_cloudtrail_test.go b/builtin/providers/aws/resource_aws_cloudtrail_test.go index 47a89083c..2d3e807c6 100644 --- a/builtin/providers/aws/resource_aws_cloudtrail_test.go +++ b/builtin/providers/aws/resource_aws_cloudtrail_test.go @@ -51,23 +51,23 @@ func TestAccAWSCloudTrail_enable_logging(t *testing.T) { Config: testAccAWSCloudTrailConfig, Check: resource.ComposeTestCheckFunc( testAccCheckCloudTrailExists("aws_cloudtrail.foobar", &trail), - // This is a warning test. AWS sets up new trails with logging disabled - // Should that change in the future, this test should fail. - testAccCheckCloudTrailLoggingEnabled("aws_cloudtrail.foobar", false, &trail), + // AWS will create the trail with logging turned off. + // Test that "enable_logging" default works. + testAccCheckCloudTrailLoggingEnabled("aws_cloudtrail.foobar", true, &trail), ), }, resource.TestStep{ Config: testAccAWSCloudTrailConfigModified, Check: resource.ComposeTestCheckFunc( testAccCheckCloudTrailExists("aws_cloudtrail.foobar", &trail), - testAccCheckCloudTrailLoggingEnabled("aws_cloudtrail.foobar", true, &trail), + testAccCheckCloudTrailLoggingEnabled("aws_cloudtrail.foobar", false, &trail), ), }, resource.TestStep{ Config: testAccAWSCloudTrailConfig, Check: resource.ComposeTestCheckFunc( testAccCheckCloudTrailExists("aws_cloudtrail.foobar", &trail), - testAccCheckCloudTrailLoggingEnabled("aws_cloudtrail.foobar", false, &trail), + testAccCheckCloudTrailLoggingEnabled("aws_cloudtrail.foobar", true, &trail), ), }, }, @@ -193,7 +193,7 @@ resource "aws_cloudtrail" "foobar" { s3_bucket_name = "${aws_s3_bucket.foo.id}" s3_key_prefix = "/prefix" include_global_service_events = false - enable_logging = true + enable_logging = false } resource "aws_s3_bucket" "foo" { diff --git a/website/source/docs/providers/aws/r/cloudtrail.html.markdown b/website/source/docs/providers/aws/r/cloudtrail.html.markdown index 0f21f4670..e63a22dd2 100644 --- a/website/source/docs/providers/aws/r/cloudtrail.html.markdown +++ b/website/source/docs/providers/aws/r/cloudtrail.html.markdown @@ -64,7 +64,8 @@ The following arguments are supported: endpoint to assume to write to a user’s log group. * `cloud_watch_logs_group_arn` - (Optional) Specifies a log group name using an Amazon Resource Name (ARN), that represents the log group to which CloudTrail logs will be delivered. -* `enable_logging` - (Optional) Enables logging for the trail. Defaults to `false`. +* `enable_logging` - (Optional) Enables logging for the trail. Defaults to `true`. + Setting this to `false` will pause logging. * `include_global_service_events` - (Optional) Specifies whether the trail is publishing events from global services such as IAM to the log files. Defaults to `true`. * `sns_topic_name` - (Optional) Specifies the name of the Amazon SNS topic From 9cec40ea3cdac35f4dde5453cfa7dc674f437c8b Mon Sep 17 00:00:00 2001 From: Paul Forman Date: Sun, 22 Nov 2015 12:54:11 -0700 Subject: [PATCH 5/6] Add missing error-checks from code review 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. --- builtin/providers/aws/resource_aws_cloudtrail.go | 14 ++++++++++---- .../providers/aws/resource_aws_cloudtrail_test.go | 2 +- .../docs/providers/aws/r/cloudtrail.html.markdown | 1 - 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/builtin/providers/aws/resource_aws_cloudtrail.go b/builtin/providers/aws/resource_aws_cloudtrail.go index 303073422..53def2fb1 100644 --- a/builtin/providers/aws/resource_aws_cloudtrail.go +++ b/builtin/providers/aws/resource_aws_cloudtrail.go @@ -91,7 +91,10 @@ func resourceAwsCloudTrailCreate(d *schema.ResourceData, meta interface{}) error // AWS CloudTrail sets newly-created trails to false. if v, ok := d.GetOk("enable_logging"); ok && v.(bool) { - cloudTrailSetLogging(conn, v.(bool), d.Id()) + err := cloudTrailSetLogging(conn, v.(bool), d.Id()) + if err != nil { + return err + } } return resourceAwsCloudTrailRead(d, meta) @@ -125,7 +128,7 @@ func resourceAwsCloudTrailRead(d *schema.ResourceData, meta interface{}) error { d.Set("include_global_service_events", trail.IncludeGlobalServiceEvents) d.Set("sns_topic_name", trail.SnsTopicName) - logstatus, err := cloudTrailGetLoggingStatus(conn, *trail.Name) + logstatus, err := cloudTrailGetLoggingStatus(conn, trail.Name) if err != nil { return err } @@ -191,11 +194,14 @@ func resourceAwsCloudTrailDelete(d *schema.ResourceData, meta interface{}) error return err } -func cloudTrailGetLoggingStatus(conn *cloudtrail.CloudTrail, id string) (bool, error) { +func cloudTrailGetLoggingStatus(conn *cloudtrail.CloudTrail, id *string) (bool, error) { GetTrailStatusOpts := &cloudtrail.GetTrailStatusInput{ - Name: aws.String(id), + Name: id, } resp, err := conn.GetTrailStatus(GetTrailStatusOpts) + if err != nil { + return false, fmt.Errorf("Error retrieving logging status of CloudTrail (%s): %s", id, err) + } return *resp.IsLogging, err } diff --git a/builtin/providers/aws/resource_aws_cloudtrail_test.go b/builtin/providers/aws/resource_aws_cloudtrail_test.go index 2d3e807c6..c276135ce 100644 --- a/builtin/providers/aws/resource_aws_cloudtrail_test.go +++ b/builtin/providers/aws/resource_aws_cloudtrail_test.go @@ -115,7 +115,7 @@ func testAccCheckCloudTrailLoggingEnabled(n string, desired bool, trail *cloudtr return err } if *resp.IsLogging != desired { - return fmt.Errorf("Logging status is incorrect") + return fmt.Errorf("Expected logging status %t, given %t", desired, *resp.IsLogging) } return nil diff --git a/website/source/docs/providers/aws/r/cloudtrail.html.markdown b/website/source/docs/providers/aws/r/cloudtrail.html.markdown index e63a22dd2..6bffee09e 100644 --- a/website/source/docs/providers/aws/r/cloudtrail.html.markdown +++ b/website/source/docs/providers/aws/r/cloudtrail.html.markdown @@ -16,7 +16,6 @@ resource "aws_cloudtrail" "foobar" { name = "tf-trail-foobar" s3_bucket_name = "${aws_s3_bucket.foo.id}" s3_key_prefix = "/prefix" - enable_logging = true include_global_service_events = false } From 52aad0493086ca393a73beb2e77012856b391101 Mon Sep 17 00:00:00 2001 From: Paul Forman Date: Sun, 22 Nov 2015 13:23:08 -0700 Subject: [PATCH 6/6] Mistake in type refactor in cloudTrailGetLoggingStatus When adjusting the types to prevent casting, I didn't change the error message to handle the pointer change. "go tool vet" caught this. --- builtin/providers/aws/resource_aws_cloudtrail.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/providers/aws/resource_aws_cloudtrail.go b/builtin/providers/aws/resource_aws_cloudtrail.go index 53def2fb1..5041a3741 100644 --- a/builtin/providers/aws/resource_aws_cloudtrail.go +++ b/builtin/providers/aws/resource_aws_cloudtrail.go @@ -200,7 +200,7 @@ func cloudTrailGetLoggingStatus(conn *cloudtrail.CloudTrail, id *string) (bool, } resp, err := conn.GetTrailStatus(GetTrailStatusOpts) if err != nil { - return false, fmt.Errorf("Error retrieving logging status of CloudTrail (%s): %s", id, err) + return false, fmt.Errorf("Error retrieving logging status of CloudTrail (%s): %s", *id, err) } return *resp.IsLogging, err