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

feature(config): manage AWS Config Remediation Configuration #9348

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions aws/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,7 @@ func Provider() terraform.ResourceProvider {
"aws_config_delivery_channel": resourceAwsConfigDeliveryChannel(),
"aws_config_organization_custom_rule": resourceAwsConfigOrganizationCustomRule(),
"aws_config_organization_managed_rule": resourceAwsConfigOrganizationManagedRule(),
"aws_config_remediation_configuration": resourceAwsConfigRemediationConfiguration(),
andy-b-84 marked this conversation as resolved.
Show resolved Hide resolved
"aws_cognito_identity_pool": resourceAwsCognitoIdentityPool(),
"aws_cognito_identity_pool_roles_attachment": resourceAwsCognitoIdentityPoolRolesAttachment(),
"aws_cognito_identity_provider": resourceAwsCognitoIdentityProvider(),
Expand Down
257 changes: 257 additions & 0 deletions aws/resource_aws_config_remediation_configuration.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,257 @@
package aws

import (
"fmt"
"log"
"time"

"github.com/hashicorp/terraform/helper/resource"
"github.com/hashicorp/terraform/helper/schema"
"github.com/hashicorp/terraform/helper/validation"
Comment on lines +8 to +10
Copy link
Contributor

Choose a reason for hiding this comment

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

We've separated the resource and schema functions into their own package, github.com/hashicorp/terraform-plugin-sdk/helper/*. Please update to use those packages.


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

func expandConfigRemediationConfigurationParameters(configured *schema.Set) map[string]*configservice.RemediationParameterValue {
var staticValues []*string
results := make(map[string]*configservice.RemediationParameterValue)

emptyString := ""

for _, item := range configured.List() {
detail := item.(map[string]interface{})
rpv := configservice.RemediationParameterValue{}

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to have a check here that would return an error if neither resource_value nor static_value is set. You could use github.com/hashicorp/go-multierror to collect all errors instead of returning on the first.

if resourceValue, ok := detail["resource_value"].(string); ok {
rv := configservice.ResourceValue{
Value: &emptyString,
Copy link
Contributor

Choose a reason for hiding this comment

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

Does Value need to be set to configservice.ResourceValueTypeResourceId?

}
rpv.ResourceValue = &rv
results[resourceValue] = &rpv
Comment on lines +27 to +31
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be inlined, something like

results[resourceValue] = &configservice.RemediationParameterValue{
    ResourceValue: &configservice.ResourceValue{
        Value: aws.String(""),
    },
}

}
if staticValue, ok := detail["static_value"].(map[string]string); ok {
value := staticValue["value"]
staticValues = make([]*string, 0)
staticValues = append(staticValues, &value)
sv := configservice.StaticValue{
Values: staticValues,
}
rpv.StaticValue = &sv
results[staticValue["key"]] = &rpv
Comment on lines +34 to +41
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be inlined, something like

results[staticValue["key"]] = &configservice.RemediationParameterValue{
    StaticValue: &configservice.StaticValue{
        Values: []*string{aws.String(staticValue["value"])},
    },
}

}
}

return results
}

func flattenRemediationConfigurations(c []*configservice.RemediationConfiguration) []map[string]interface{} {
configurations := make([]map[string]interface{}, 0)

for _, bd := range c {
if bd.ConfigRuleName != nil && bd.Parameters != nil {
configuration := make(map[string]interface{})
configuration["config_rule_name"] = *bd.ConfigRuleName
configuration["parameters"] = flattenRemediationConfigurationParameters(bd.Parameters)
configuration["resource_type"] = *bd.ResourceType
configuration["target_id"] = *bd.TargetId
configuration["target_type"] = *bd.TargetType
configuration["target_version"] = *bd.TargetVersion

configurations = append(configurations, configuration)
}
}

if len(configurations) > 0 {
return configurations
}
Comment on lines +65 to +67
Copy link
Contributor

Choose a reason for hiding this comment

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

This check could be moved to the top and test len(c) > 0


return nil
}
Comment on lines +16 to +70
Copy link
Contributor

Choose a reason for hiding this comment

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

We prefer that the resource schema function (i.e. resourceAwsConfigRemediationConfiguration()) is at the top of the file


func resourceAwsConfigRemediationConfiguration() *schema.Resource {
return &schema.Resource{
Create: resourceAwsConfigRemediationConfigurationPut,
Read: resourceAwsConfigRemediationConfigurationRead,
Update: resourceAwsConfigRemediationConfigurationPut,
Delete: resourceAwsConfigRemediationConfigurationDelete,

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

Schema: map[string]*schema.Schema{
Copy link
Contributor

Choose a reason for hiding this comment

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

The remediation configuration has an ARN returned by the AWS API that can be used to uniquely identify it. We should have a Computed attribute called arn that is set to that value

"config_rule_name": {
andy-b-84 marked this conversation as resolved.
Show resolved Hide resolved
Type: schema.TypeString,
Required: true,
ForceNew: true,
ValidateFunc: validation.StringLenBetween(1, 64),
},
"resource_type": {
Type: schema.TypeString,
Optional: true,
},
"target_id": {
andy-b-84 marked this conversation as resolved.
Show resolved Hide resolved
Type: schema.TypeString,
Required: true,
ValidateFunc: validation.StringLenBetween(1, 256),
},
"target_type": {
andy-b-84 marked this conversation as resolved.
Show resolved Hide resolved
Type: schema.TypeString,
Required: true,
ValidateFunc: validation.StringInSlice([]string{
configservice.RemediationTargetTypeSsmDocument,
}, false),
},
"target_version": {
Type: schema.TypeString,
Optional: true,
},
"parameters": {
Type: schema.TypeSet,
MaxItems: 25,
Optional: true,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"resource_value": {
Type: schema.TypeString,
Optional: true,
ValidateFunc: validation.StringLenBetween(0, 256),
},
"static_value": {
Type: schema.TypeSet,
Optional: true,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"key": {
Type: schema.TypeString,
Required: true,
},
"value": {
Type: schema.TypeString,
Required: true,
},
},
},
},
},
},
},
},
}
}

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

name := d.Get("config_rule_name").(string)
remediationConfigurationInput := configservice.RemediationConfiguration{
ConfigRuleName: aws.String(name),
}

if v, ok := d.GetOk("parameters"); ok {
remediationConfigurationInput.Parameters = expandConfigRemediationConfigurationParameters(v.(*schema.Set))
}

if v, ok := d.GetOk("resource_type"); ok {
remediationConfigurationInput.ResourceType = aws.String(v.(string))
}
if v, ok := d.GetOk("target_id"); ok && v.(string) != "" {
remediationConfigurationInput.TargetId = aws.String(v.(string))
}
if v, ok := d.GetOk("target_type"); ok && v.(string) != "" {
remediationConfigurationInput.TargetType = aws.String(v.(string))
}
if v, ok := d.GetOk("target_version"); ok && v.(string) != "" {
remediationConfigurationInput.TargetVersion = aws.String(v.(string))
}

input := configservice.PutRemediationConfigurationsInput{
RemediationConfigurations: []*configservice.RemediationConfiguration{&remediationConfigurationInput},
}
log.Printf("[DEBUG] Creating AWSConfig remediation configuration: %s", input)
err := resource.Retry(2*time.Minute, func() *resource.RetryError {
Copy link
Contributor

Choose a reason for hiding this comment

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

We prefer named constants for timeout values

_, err := conn.PutRemediationConfigurations(&input)
if err != nil {
if isAWSErr(err, configservice.ErrCodeInsufficientPermissionsException, "") {
// IAM is eventually consistent
return resource.RetryableError(err)
}

return resource.NonRetryableError(fmt.Errorf("Failed to create AWSConfig remediation configuration: %s", err))
Copy link
Contributor

Choose a reason for hiding this comment

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

We're updating our error messages to use the Go 1.13 error wrapping verb %w

}

return nil
})
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

We've started adding one last try after resource.Retry() in case of a timeout.

if isResourceTimeoutError(err) {
	_, err = conn.PutRemediationConfigurations(&input)
}

return err
}

d.SetId(name)
Copy link
Contributor

Choose a reason for hiding this comment

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

The resource ID should be set to the ARN value, since that uniquely identifies it

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a really tough one -- https://godoc.org/github.com/aws/aws-sdk-go/service/configservice#PutRemediationConfigurationsOutput doesn't return the ARN. We could use "describe" to get the arn, but it could return multiple configurations, and we'd have to make some assumptions to get a single arn.


log.Printf("[DEBUG] AWSConfig config remediation configuration for rule %q created", name)

return resourceAwsConfigRemediationConfigurationRead(d, meta)
}

func resourceAwsConfigRemediationConfigurationRead(d *schema.ResourceData, meta interface{}) error {
conn := meta.(*AWSClient).configconn
out, err := conn.DescribeRemediationConfigurations(&configservice.DescribeRemediationConfigurationsInput{
ConfigRuleNames: []*string{aws.String(d.Id())},
})
if err != nil {
if isAWSErr(err, configservice.ErrCodeNoSuchConfigRuleException, "") {
log.Printf("[WARN] Config Rule %q is gone (NoSuchConfigRuleException)", d.Id())
d.SetId("")
return nil
}
return err
}

numberOfRemediationConfigurations := len(out.RemediationConfigurations)
if numberOfRemediationConfigurations < 1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

The call to len() can be inlined in the check

log.Printf("[WARN] No Remediation Configuration for Config Rule %q (no remediation configuration found)", d.Id())
d.SetId("")
return nil
}

log.Printf("[DEBUG] AWS Config remediation configurations received: %s", out)

remediationConfigurations := out.RemediationConfigurations
d.Set("remediationConfigurations", flattenRemediationConfigurations(remediationConfigurations))
Copy link
Contributor

Choose a reason for hiding this comment

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

This appears like it could have been a previous iteration of the schema. We'd expect to see d.Set() calls like the below with the current schema so Terraform can appropriately save each of the attributes into the Terraform state to perform drift detection and support import:

d.Set("config_rule_name", /* ... */)

if err := d.Set("parameters", flattenRemediationConfigurationParameters(/* ... */)); err != nil {
	return fmt.Errorf("error setting parameters: %s", err)
}

d.Set("resource_type", /* ... */)
d.Set("target_id", /* ... */)
d.Set("target_type", /* ... */)
d.Set("target_version", /* ... */)

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'll have to put this error handling code in the flattenRemediationConfigurations, then, as this code handles lists of RemediationConfiguration objects.
Unless you mean I have to handle RemediationConfiguration one by one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The more I read this specific review, my code, and AWS documentation (website and man), the more I feel like there is something to clarify before getting forward :
Remediation Configuration objects can only be identified by their corresponding Config Rule, which makes sense as they dannot exist without a Config Rule on top of them.
aws CLI comand reflects that fact :

$ aws configservice help
...
AVAILABLE COMMANDS
...
delete-remediation-configuration
...
describe-remediation-configurations
...
put-remediation-configurations
...
$ aws configservice describe-config-rules help
...
SYNOPSIS
            describe-config-rules
          [--config-rule-names <value>]
          [--cli-input-json <value>]
          [--starting-token <value>]
          [--max-items <value>]
          [--generate-cli-skeleton <value>]

OPTIONS
...

Which appears the same way in the GO API this provider is using to communicate with AWS, if I understood correctly.

At this point I'm not sure I wrote a code that matches these behaviors : maybe it would be best to only handle lists of Remediation Configuraion objects, be it when reading or writing them, instead of allowing to write a single Remediation Configuration object outside of any kind of collection.

Copy link
Contributor

Choose a reason for hiding this comment

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

When creating resources that depend on other resources, we tend to separate them out when the dependent resource has its own APIs. This lets them be managed separately, for example if different teams define the config rules and the remediation actions they could be defined in separate Terraform configurations


return nil
}

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

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

deleteRemediationConfigurationInput := configservice.DeleteRemediationConfigurationInput{
ConfigRuleName: aws.String(name),
}

if v, ok := d.GetOk("resource_type"); ok && v.(string) != "" {
deleteRemediationConfigurationInput.ResourceType = aws.String(v.(string))
}

log.Printf("[DEBUG] Deleting AWS Config remediation configurations for rule %q", name)
err := resource.Retry(2*time.Minute, func() *resource.RetryError {
_, err := conn.DeleteRemediationConfiguration(&deleteRemediationConfigurationInput)
if err != nil {
if isAWSErr(err, configservice.ErrCodeResourceInUseException, "") {
return resource.RetryableError(err)
}
return resource.NonRetryableError(err)
}
return nil
})
if err != nil {
return fmt.Errorf("Deleting Remediation Configurations failed: %s", err)
}

log.Printf("[DEBUG] AWS Config remediation configurations for rule %q deleted", name)

return nil
}
Loading