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.

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

New Resource: r/aws_config_aggregator #4262

Merged
merged 1 commit into from Jun 3, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
30 changes: 30 additions & 0 deletions aws/import_aws_config_aggregator_test.go
@@ -0,0 +1,30 @@
package aws

import (
"testing"

"github.com/hashicorp/terraform/helper/acctest"
"github.com/hashicorp/terraform/helper/resource"
)

func TestAccConfigAggregator_import(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

Minor nitpick: we're going to move away from separate import test files and tests (#4705). We can simply add the final TestStep the existing _basic test 👍

resourceName := "aws_config_aggregator.example"
rString := acctest.RandString(10)

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckConfigAggregatorDestroy,
Steps: []resource.TestStep{
resource.TestStep{
Config: testAccConfigAggregatorConfig_account(rString),
},

resource.TestStep{
ResourceName: resourceName,
ImportState: true,
ImportStateVerify: true,
},
},
})
}
1 change: 1 addition & 0 deletions aws/provider.go
Expand Up @@ -303,6 +303,7 @@ func Provider() terraform.ResourceProvider {
"aws_cloudwatch_log_resource_policy": resourceAwsCloudWatchLogResourcePolicy(),
"aws_cloudwatch_log_stream": resourceAwsCloudWatchLogStream(),
"aws_cloudwatch_log_subscription_filter": resourceAwsCloudwatchLogSubscriptionFilter(),
"aws_config_aggregator": resourceAwsConfigAggregator(),
Copy link
Member

Choose a reason for hiding this comment

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

Similar to #4263 we should prefer to clarify that this is a "configuration" aggregator to match the API, especially if they later introduce some other type of aggregator:

"aws_config_configuration_aggregator": resourceAwsConfigConfigurationAggregator(),

"aws_config_config_rule": resourceAwsConfigConfigRule(),
"aws_config_configuration_recorder": resourceAwsConfigConfigurationRecorder(),
"aws_config_configuration_recorder_status": resourceAwsConfigConfigurationRecorderStatus(),
Expand Down
193 changes: 193 additions & 0 deletions aws/resource_aws_config_aggregator.go
@@ -0,0 +1,193 @@
package aws

import (
"fmt"
"log"
"strings"

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/service/configservice"

"github.com/hashicorp/terraform/helper/customdiff"
"github.com/hashicorp/terraform/helper/schema"
)

func resourceAwsConfigAggregator() *schema.Resource {
return &schema.Resource{
Create: resourceAwsConfigAggregatorPut,
Read: resourceAwsConfigAggregatorRead,
Update: resourceAwsConfigAggregatorPut,
Delete: resourceAwsConfigAggregatorDelete,

Importer: &schema.ResourceImporter{
State: schema.ImportStatePassthrough,
},

CustomizeDiff: customdiff.Sequence(
customdiff.ForceNewIfChange("account_aggregation_source", func(old, new, meta interface{}) bool {
Copy link
Member

Choose a reason for hiding this comment

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

I believe we can just set ForceNew: true on the root attributes instead of implementing CustomizeDiff here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was trying to minimise delete/recreate operations - it's OK to call PutConfigurationAggregatorInput to make changes to an existing aggregator as long as you aren't switching between organization or account aggregation sources.

Copy link
Member

Choose a reason for hiding this comment

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

Ah: its because of this behavior (seemingly buggy as ForceNew: doesn't necessarily apply to all nested attributes anymore):

All fields are ForceNew or Computed w/out Optional, Update is superfluous

I'll add this in a comment in there.

return len(old.([]interface{})) == 0 && len(new.([]interface{})) > 0
}),
customdiff.ForceNewIfChange("organization_aggregation_source", func(old, new, meta interface{}) bool {
return len(old.([]interface{})) == 0 && len(new.([]interface{})) > 0
}),
),

Schema: map[string]*schema.Schema{
"arn": {
Type: schema.TypeString,
Computed: true,
},
"name": {
Type: schema.TypeString,
Required: true,
ForceNew: true,
ValidateFunc: validateMaxLength(256),
},
"account_aggregation_source": {
Type: schema.TypeList,
Optional: true,
MaxItems: 1,
ConflictsWith: []string{"organization_aggregation_source"},
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"account_ids": {
Type: schema.TypeList,
Required: true,
MinItems: 1,
Elem: &schema.Schema{
Type: schema.TypeString,
ValidateFunc: validateAwsAccountId,
},
},
"all_regions": {
Type: schema.TypeBool,
Default: false,
Optional: true,
},
"regions": {
Type: schema.TypeList,
Optional: true,
MinItems: 1,
Elem: &schema.Schema{
Type: schema.TypeString,
},
},
},
},
},
"organization_aggregation_source": {
Type: schema.TypeList,
Optional: true,
MaxItems: 1,
ConflictsWith: []string{"account_aggregation_source"},
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"all_regions": {
Type: schema.TypeBool,
Default: false,
Optional: true,
},
"regions": {
Type: schema.TypeList,
Optional: true,
MinItems: 1,
Elem: &schema.Schema{
Type: schema.TypeString,
},
},
"role_arn": {
Type: schema.TypeString,
Required: true,
ValidateFunc: validateArn,
},
},
},
},
},
}
}

func resourceAwsConfigAggregatorPut(d *schema.ResourceData, meta interface{}) error {
conn := meta.(*AWSClient).configconn

name := d.Get("name").(string)

req := &configservice.PutConfigurationAggregatorInput{
ConfigurationAggregatorName: aws.String(name),
}

account_aggregation_sources := d.Get("account_aggregation_source").([]interface{})
if len(account_aggregation_sources) > 0 {
req.AccountAggregationSources = expandConfigAccountAggregationSources(account_aggregation_sources)
}

organization_aggregation_sources := d.Get("organization_aggregation_source").([]interface{})
if len(organization_aggregation_sources) > 0 {
req.OrganizationAggregationSource = expandConfigOrganizationAggregationSource(organization_aggregation_sources[0].(map[string]interface{}))
}

_, err := conn.PutConfigurationAggregator(req)
if err != nil {
return fmt.Errorf("Error creating aggregator: %s", err)
}

d.SetId(strings.ToLower(name))

return resourceAwsConfigAggregatorRead(d, meta)
}

func resourceAwsConfigAggregatorRead(d *schema.ResourceData, meta interface{}) error {
conn := meta.(*AWSClient).configconn
req := &configservice.DescribeConfigurationAggregatorsInput{
ConfigurationAggregatorNames: []*string{aws.String(d.Id())},
}

res, err := conn.DescribeConfigurationAggregators(req)
if err != nil {
if isAWSErr(err, configservice.ErrCodeNoSuchConfigurationAggregatorException, "") {
log.Printf("[WARN] No such configuration aggregator (%s), removing from state", d.Id())
d.SetId("")
return nil
}
return err
}

if len(res.ConfigurationAggregators) == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

While trying to prevent panics here, we should check res == nil as well 👍

log.Printf("[WARN] No aggregators returned (%s), removing from state", d.Id())
d.SetId("")
return nil
}

aggregator := res.ConfigurationAggregators[0]
d.Set("arn", aggregator.ConfigurationAggregatorArn)
d.Set("name", aggregator.ConfigurationAggregatorName)

if aggregator.AccountAggregationSources != nil {
d.Set("account_aggregation_source", flattenConfigAccountAggregationSources(aggregator.AccountAggregationSources))
Copy link
Member

Choose a reason for hiding this comment

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

Two notes:

  • The flatten function should handle nil input and return a zero length slice so we do not need to wrap this with a conditional
  • When setting non-scalar attributes in the Terraform state, we prefer to perform error checking:
if err := d.Set("account_aggregation_source", flattenConfigAccountAggregationSources(aggregator.AccountAggregationSources)); err != nil {
  return fmt.Errorf("error setting account_aggregation_source: %s", err)
}

} else {
d.Set("account_aggregation_source", nil)
}

if aggregator.OrganizationAggregationSource != nil {
d.Set("organization_aggregation_source", flattenConfigOrganizationAggregationSource(aggregator.OrganizationAggregationSource))
} else {
d.Set("organization_aggregation_source", nil)
}

return nil
}

func resourceAwsConfigAggregatorDelete(d *schema.ResourceData, meta interface{}) error {
conn := meta.(*AWSClient).configconn

req := &configservice.DeleteConfigurationAggregatorInput{
ConfigurationAggregatorName: aws.String(d.Id()),
}
_, err := conn.DeleteConfigurationAggregator(req)
if err != nil {
return err
}

d.SetId("")
Copy link
Member

Choose a reason for hiding this comment

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

d.SetId("") is extraneous in delete functions

return nil
}