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.

Sign up for GitHub

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

Jump to bottom

r/redshift_cluster: Nest all logging fields #2230

Merged
merged 1 commit into from
Nov 9, 2017
Merged

r/redshift_cluster: Nest all logging fields #2230

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 71 additions & 18 deletions aws/resource_aws_redshift_cluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,22 +219,54 @@ func resourceAwsRedshiftCluster() *schema.Resource {
Set: schema.HashString,
},

"enable_logging": {
Type: schema.TypeBool,
"logging": {
Type: schema.TypeList,
MaxItems: 1,
Optional: true,
Default: false,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"enable": {
Type: schema.TypeBool,
Required: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

does this not need a State Migration?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so - it's only required within the context of logging block, i.e. you can't define an empty logging {}, it shouldn't affect any existing configs without this block.

},

"bucket_name": {
Type: schema.TypeString,
Optional: true,
Computed: true,
},

"s3_key_prefix": {
Type: schema.TypeString,
Optional: true,
Computed: true,
},
},
},
},

"enable_logging": {
Type: schema.TypeBool,
Optional: true,
Computed: true,
Deprecated: "Use enable in the 'logging' block instead",
ConflictsWith: []string{"logging"},
Copy link
Contributor

Choose a reason for hiding this comment

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

would it make more sense to make this logging.0.enabled?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think there's no harm in conflicting with the whole logging block. Either they use deprecated field or the whole new block. I'd rather not deal with mix of deprecated and non-deprecated fields. 😉

},

"bucket_name": {
Type: schema.TypeString,
Optional: true,
Computed: true,
Type: schema.TypeString,
Optional: true,
Computed: true,
Deprecated: "Use bucket_name in the 'logging' block instead",
ConflictsWith: []string{"logging"},
},

"s3_key_prefix": {
Type: schema.TypeString,
Optional: true,
Computed: true,
Type: schema.TypeString,
Optional: true,
Computed: true,
Deprecated: "Use s3_key_prefix in the 'logging' block instead",
ConflictsWith: []string{"logging"},
},

"snapshot_identifier": {
Expand Down Expand Up @@ -439,8 +471,9 @@ func resourceAwsRedshiftClusterCreate(d *schema.ResourceData, meta interface{})
return fmt.Errorf("[WARN] Error waiting for Redshift Cluster state to be \"available\": %s", err)
}

if _, ok := d.GetOk("enable_logging"); ok {

logging, ok := d.GetOk("logging.0.enable")
_, deprecatedOk := d.GetOk("enable_logging")
if (ok && logging.(bool)) || deprecatedOk {
loggingErr := enableRedshiftClusterLogging(d, conn)
if loggingErr != nil {
log.Printf("[ERROR] Error Enabling Logging on Redshift Cluster: %s", err)
Expand Down Expand Up @@ -553,10 +586,13 @@ func resourceAwsRedshiftClusterRead(d *schema.ResourceData, meta interface{}) er
d.Set("cluster_revision_number", rsc.ClusterRevisionNumber)
d.Set("tags", tagsToMapRedshift(rsc.Tags))

// TODO: Deprecated fields - remove in next major version
d.Set("bucket_name", loggingStatus.BucketName)
d.Set("enable_logging", loggingStatus.LoggingEnabled)
d.Set("s3_key_prefix", loggingStatus.S3KeyPrefix)

d.Set("logging", flattenRedshiftLogging(loggingStatus))

return nil
}

Expand Down Expand Up @@ -711,17 +747,20 @@ func resourceAwsRedshiftClusterUpdate(d *schema.ResourceData, meta interface{})
}
}

if d.HasChange("enable_logging") || d.HasChange("bucket_name") || d.HasChange("s3_key_prefix") {
deprecatedHasChange := (d.HasChange("enable_logging") || d.HasChange("bucket_name") || d.HasChange("s3_key_prefix"))
if d.HasChange("logging") || deprecatedHasChange {
var loggingErr error
if _, ok := d.GetOk("enable_logging"); ok {

logging, ok := d.GetOk("logging.0.enable")
_, deprecatedOk := d.GetOk("enable_logging")

if (ok && logging.(bool)) || deprecatedOk {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you want to check the value of enable_logging, rather than that it's set to a value?

Copy link
Member Author

Choose a reason for hiding this comment

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

As it is a boolean field it shouldn't matter, b/c it will always be present - that is in fact why we introduced d.GetOkExists, secondly I intentionally left the implementation as is since it will be deprecated eventually.

log.Printf("[INFO] Enabling Logging for Redshift Cluster %q", d.Id())
loggingErr = enableRedshiftClusterLogging(d, conn)
if loggingErr != nil {
return loggingErr
}
} else {

log.Printf("[INFO] Disabling Logging for Redshift Cluster %q", d.Id())
_, loggingErr = conn.DisableLogging(&redshift.DisableLoggingInput{
ClusterIdentifier: aws.String(d.Id()),
Expand All @@ -732,6 +771,7 @@ func resourceAwsRedshiftClusterUpdate(d *schema.ResourceData, meta interface{})
}

d.SetPartial("enable_logging")
d.SetPartial("logging")
}

d.Partial(false)
Expand All @@ -740,17 +780,30 @@ func resourceAwsRedshiftClusterUpdate(d *schema.ResourceData, meta interface{})
}

func enableRedshiftClusterLogging(d *schema.ResourceData, conn *redshift.Redshift) error {
if _, ok := d.GetOk("bucket_name"); !ok {
var bucketName, v interface{}
var deprecatedOk, ok bool
bucketName, deprecatedOk = d.GetOk("bucket_name")
v, ok = d.GetOk("logging.0.bucket_name")
if ok {
bucketName = v
}
if !ok && !deprecatedOk {
return fmt.Errorf("bucket_name must be set when enabling logging for Redshift Clusters")
}

params := &redshift.EnableLoggingInput{
ClusterIdentifier: aws.String(d.Id()),
BucketName: aws.String(d.Get("bucket_name").(string)),
BucketName: aws.String(bucketName.(string)),
}

if v, ok := d.GetOk("s3_key_prefix"); ok {
params.S3KeyPrefix = aws.String(v.(string))
var s3KeyPrefix interface{}
s3KeyPrefix, deprecatedOk = d.GetOk("s3_key_prefix")
v, ok = d.GetOk("logging.0.s3_key_prefix")
if ok {
s3KeyPrefix = v
}
if ok || deprecatedOk {
params.S3KeyPrefix = aws.String(s3KeyPrefix.(string))
}

_, loggingErr := conn.EnableLogging(params)
Expand Down
110 changes: 105 additions & 5 deletions aws/resource_aws_redshift_cluster_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ func TestAccAWSRedshiftCluster_enhancedVpcRoutingEnabled(t *testing.T) {
})
}

func TestAccAWSRedshiftCluster_loggingEnabled(t *testing.T) {
func TestAccAWSRedshiftCluster_loggingEnabledDeprecated(t *testing.T) {
var v redshift.Cluster
rInt := acctest.RandInt()

Expand All @@ -167,7 +167,7 @@ func TestAccAWSRedshiftCluster_loggingEnabled(t *testing.T) {
CheckDestroy: testAccCheckAWSRedshiftClusterDestroy,
Steps: []resource.TestStep{
{
Config: testAccAWSRedshiftClusterConfig_loggingEnabled(rInt),
Config: testAccAWSRedshiftClusterConfig_loggingEnabledDeprecated(rInt),
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSRedshiftClusterExists("aws_redshift_cluster.default", &v),
resource.TestCheckResourceAttr(
Expand All @@ -178,7 +178,7 @@ func TestAccAWSRedshiftCluster_loggingEnabled(t *testing.T) {
},

{
Config: testAccAWSRedshiftClusterConfig_loggingDisabled(rInt),
Config: testAccAWSRedshiftClusterConfig_loggingDisabledDeprecated(rInt),
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSRedshiftClusterExists("aws_redshift_cluster.default", &v),
resource.TestCheckResourceAttr(
Expand All @@ -189,6 +189,38 @@ func TestAccAWSRedshiftCluster_loggingEnabled(t *testing.T) {
})
}

func TestAccAWSRedshiftCluster_loggingEnabled(t *testing.T) {
var v redshift.Cluster
rInt := acctest.RandInt()

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckAWSRedshiftClusterDestroy,
Steps: []resource.TestStep{
{
Config: testAccAWSRedshiftClusterConfig_loggingEnabled(rInt),
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSRedshiftClusterExists("aws_redshift_cluster.default", &v),
resource.TestCheckResourceAttr(
"aws_redshift_cluster.default", "logging.0.enable", "true"),
resource.TestCheckResourceAttr(
"aws_redshift_cluster.default", "logging.0.bucket_name", fmt.Sprintf("tf-redshift-logging-%d", rInt)),
),
},

{
Config: testAccAWSRedshiftClusterConfig_loggingDisabled(rInt),
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSRedshiftClusterExists("aws_redshift_cluster.default", &v),
resource.TestCheckResourceAttr(
"aws_redshift_cluster.default", "logging.0.enable", "false"),
),
},
},
})
}

func TestAccAWSRedshiftCluster_iamRoles(t *testing.T) {
var v redshift.Cluster

Expand Down Expand Up @@ -743,7 +775,7 @@ resource "aws_redshift_cluster" "default" {
`, rInt)
}

func testAccAWSRedshiftClusterConfig_loggingDisabled(rInt int) string {
func testAccAWSRedshiftClusterConfig_loggingDisabledDeprecated(rInt int) string {
return fmt.Sprintf(`
resource "aws_redshift_cluster" "default" {
cluster_identifier = "tf-redshift-cluster-%d"
Expand All @@ -759,7 +791,7 @@ func testAccAWSRedshiftClusterConfig_loggingDisabled(rInt int) string {
}`, rInt)
}

func testAccAWSRedshiftClusterConfig_loggingEnabled(rInt int) string {
func testAccAWSRedshiftClusterConfig_loggingEnabledDeprecated(rInt int) string {
return fmt.Sprintf(`
resource "aws_s3_bucket" "bucket" {
bucket = "tf-redshift-logging-%d"
Expand Down Expand Up @@ -807,6 +839,74 @@ EOF
}`, rInt, rInt, rInt, rInt)
}

func testAccAWSRedshiftClusterConfig_loggingDisabled(rInt int) string {
return fmt.Sprintf(`
resource "aws_redshift_cluster" "default" {
cluster_identifier = "tf-redshift-cluster-%d"
availability_zone = "us-west-2a"
database_name = "mydb"
master_username = "foo_test"
master_password = "Mustbe8characters"
node_type = "dc1.large"
automated_snapshot_retention_period = 0
allow_version_upgrade = false
logging {
enable = false
}
skip_final_snapshot = true
}`, rInt)
}

func testAccAWSRedshiftClusterConfig_loggingEnabled(rInt int) string {
return fmt.Sprintf(`
resource "aws_s3_bucket" "bucket" {
bucket = "tf-redshift-logging-%d"
force_destroy = true
policy = <<EOF
{
"Version": "2008-10-17",
"Statement": [
{
"Sid": "Stmt1376526643067",
"Effect": "Allow",
"Principal": {
"AWS": "arn:aws:iam::902366379725:user/logs"
},
"Action": "s3:PutObject",
"Resource": "arn:aws:s3:::tf-redshift-logging-%d/*"
},
{
"Sid": "Stmt137652664067",
"Effect": "Allow",
"Principal": {
"AWS": "arn:aws:iam::902366379725:user/logs"
},
"Action": "s3:GetBucketAcl",
"Resource": "arn:aws:s3:::tf-redshift-logging-%d"
}
]
}
EOF
}


resource "aws_redshift_cluster" "default" {
cluster_identifier = "tf-redshift-cluster-%d"
availability_zone = "us-west-2a"
database_name = "mydb"
master_username = "foo_test"
master_password = "Mustbe8characters"
node_type = "dc1.large"
automated_snapshot_retention_period = 0
allow_version_upgrade = false
logging {
enable = true
bucket_name = "${aws_s3_bucket.bucket.bucket}"
}
skip_final_snapshot = true
}`, rInt, rInt, rInt, rInt)
}

func testAccAWSRedshiftClusterConfig_tags(rInt int) string {
return fmt.Sprintf(`
resource "aws_redshift_cluster" "default" {
Expand Down
16 changes: 16 additions & 0 deletions aws/structure.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2340,3 +2340,19 @@ func flattenCognitoIdentityPoolRolesAttachmentMappingRules(d []*cognitoidentity.

return rules
}

func flattenRedshiftLogging(ls *redshift.LoggingStatus) []interface{} {
if ls == nil {
return []interface{}{}
}

cfg := make(map[string]interface{}, 0)
cfg["enabled"] = *ls.LoggingEnabled
if ls.BucketName != nil {
cfg["bucket_name"] = *ls.BucketName
}
if ls.S3KeyPrefix != nil {
cfg["s3_key_prefix"] = *ls.S3KeyPrefix
}
return []interface{}{cfg}
}