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

Conversation

andy-b-84
Copy link
Contributor

@andy-b-84 andy-b-84 commented Jul 16, 2019

Answering #7972
This is my 1st go development ever, so please feel free to tell me if I
did something in a bad way, which is most probable :)

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

Fixes #7972

Release note for CHANGELOG:

Add support for AWS Config Remediation Configuration objects

Output from acceptance testing: (ran on a docker container with image golang:1.12.9-buster )

root@b12d15607224:/src# make testacc TESTARGS='-run=TestAccXXX'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -parallel 20 -run=TestAccXXX -timeout 120m
?       github.com/terraform-providers/terraform-provider-aws   [no test files]
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       0.046s [no tests to run]
...

@andy-b-84 andy-b-84 requested a review from a team July 16, 2019 09:38
@ghost ghost added size/L Managed by automation to categorize the size of a PR. service/configservice Issues and PRs that pertain to the configservice service. labels Jul 16, 2019
@andy-b-84
Copy link
Contributor Author

I'm starting to write acceptance tests ATM (and just saw Travis' failure, which I will fix ASAP)

@ghost ghost added size/XL Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. and removed size/L Managed by automation to categorize the size of a PR. labels Jul 16, 2019
@andy-b-84 andy-b-84 force-pushed the f-aws_config_remediation branch 2 times, most recently from f6d4338 to be57a6c Compare July 16, 2019 13:34
@ghost ghost added the provider Pertains to the provider itself, rather than any interaction with AWS. label Jul 16, 2019
@andy-b-84 andy-b-84 changed the title WIP feature(config): manage AWS Config Remediation Configuration [WIP] feature(config): manage AWS Config Remediation Configuration Jul 19, 2019
@andy-b-84
Copy link
Contributor Author

I've read through https://github.com/terraform-providers/terraform-provider-aws/blob/master/.github/CONTRIBUTING.md#common-review-items , and aside from points Uses AWS Go SDK Constants & Uses TypeList and MaxItems: 1 , I think I got all of them right.
Thus I delete the [WIP] flag and go on for validation.

@andy-b-84 andy-b-84 changed the title [WIP] feature(config): manage AWS Config Remediation Configuration feature(config): manage AWS Config Remediation Configuration Jul 22, 2019
@andy-b-84
Copy link
Contributor Author

andy-b-84 commented Jul 22, 2019

Note for the maintainers :
I messed up something in my go environment, I cannot run the tests on my machine anymore, neither with the make test command nor the go test command. If you could launch the make testacc TESTARGS='-run=TestAccXXX' you're talking about in the PR template, please, that would be great.

Patrick Laxton added 2 commits August 16, 2019 11:50
Answering hashicorp#7972
This is my 1st go development ever, so please feel free to tell me if I
did something in a bad way, which is most probable :)

feat(go): learn pointers usage

feat(config): parse all simple fields

chore(naming): replace ConfigRule by RemediationConfiguration

feat(config): add read flatten function

feat(config): add delete function

feat(config): use TypeSet instead of TypeList, & parse ResourceValue

feat(config): parse StaticValue

feat(config) flatten remediation config parameters to nil, just to get the function signature right

feat(config): flatten remediation config parameters
chore(tidy): remove unused import comments

test(config): copy-paste config rule test to start from something

feat(test): add shredder

feat(config): add a remediation configuration exemple

feat(test): add getter check

feat(config): add 1st finished acc test

feat(test): remove unadapted commented tests & add a second true test

feat(test): add static values tests

chore(fmt): format the file with make fmt

fixup! chore(fmt): format the file with make fmt

fix(test): add required on fields

chore(lint): proceed with linter results

chore(lint): use correct configuration
@andy-b-84
Copy link
Contributor Author

(force pushed because of a rebase on upstream)

@andy-b-84
Copy link
Contributor Author

I see you just added the code to support Organizations in AWS Config, @bflad .
Do you have any advice on my PR, or could you please paste the "Output from acceptance testing", as I cannot run this command on my computer?

@bflad bflad added help wanted new-resource Introduces a new resource. labels Aug 16, 2019
@bflad
Copy link
Member

bflad commented Aug 16, 2019

Hey @andy-b-84 👋 Thank you for contributing this! We will certainly want to get support for this new resource added at some point.

I can provide you a quick review (a few minutes behind this comment) and potentially try and help troubleshoot your local Go environment here, but unfortunately at this time the maintainers are pretty backlogged. We would prefer that contributors can appropriately run their own acceptance testing if possible as it saves a lot of small back-and-forth GitHub interactions for troubleshooting and reviews. 😅 We can certainly try to answer specific questions when time permits though.

It would be super helpful to know what operating system you are using, how Go was installed, which version of Go, and what errors you are seeing when running make test (these need to be fixed first) and make testacc.

For general assistance just beyond folks who may be able to help you on GitHub here, there are also other potentially helpful resources for Terraform Provider/Go development including the community forums or Slack workspaces such as the Gophers Slack.

Copy link
Member

@bflad bflad left a comment

Choose a reason for hiding this comment

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

Hi @andy-b-84 👋 As promised, here is an initial review of the submitted code. Please reach out with any questions or if you do not have time to implement the feedback items.

Please also let us know about your testing issues or if you would prefer to discuss them in another forum. Thanks.

aws/resource_aws_config_remediation_configuration.go Outdated Show resolved Hide resolved
aws/resource_aws_config_remediation_configuration.go Outdated Show resolved Hide resolved
aws/resource_aws_config_remediation_configuration_test.go Outdated Show resolved Hide resolved
aws/resource_aws_config_remediation_configuration_test.go Outdated Show resolved Hide resolved
aws/resource_aws_config_remediation_configuration_test.go Outdated Show resolved Hide resolved
aws/structure.go Outdated Show resolved Hide resolved
aws/provider.go Show resolved Hide resolved
@bflad bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Aug 16, 2019
@andy-b-84
Copy link
Contributor Author

Thanks for the extensive review and proposed help for my dev environment @bflad , and sorry to answer only now : as you may have guessed I was on vacation :)
I'm going to go through all of this ASAP.

@ghost ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Aug 26, 2019
@andy-b-84
Copy link
Contributor Author

I started a thread on gophers#newbies : https://gophers.slack.com/archives/C02A8LZKT/p1566895456146900 in order to get help getting my environment working.

@ghost ghost added the documentation Introduces or discusses updates to documentation. label Aug 27, 2019
@andy-b-84
Copy link
Contributor Author

I'd like to add terraform-providers/aws-provider as a reviewer, but I don't have the Add reviewer button on top of the reviewers list

@shannonrdunn
Copy link

What happened with this? Would love to see this implemented.

@aeschright aeschright requested a review from a team January 10, 2020 20:04
@sleerssen
Copy link

Would love to see this in a release.

@giuliocalzolari
Copy link

giuliocalzolari commented Feb 27, 2020

Would awesome to have this feature

@gdavison gdavison self-assigned this Jun 16, 2020
Copy link
Contributor

@gdavison gdavison left a comment

Choose a reason for hiding this comment

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

Hi @andy-b-84, thanks for the contribution! And thanks for your patience with our delay in getting back to you.

In the time since you made the PR, we've changed a number of things internally in our code. One major change is that the schema and resource definitions have move to their own SDK. The best approach would be to rebase your code onto the current provider and use the updated plugin SDK.

I've made a number of other suggestions if you want to continue working on this or if you'd like us to complete it. If not, we understand. Let us know in a reply here. I'll check back by 24 June

Comment on lines +16 to +70
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{}

if resourceValue, ok := detail["resource_value"].(string); ok {
rv := configservice.ResourceValue{
Value: &emptyString,
}
rpv.ResourceValue = &rv
results[resourceValue] = &rpv
}
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
}
}

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
}

return 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 prefer that the resource schema function (i.e. resourceAwsConfigRemediationConfiguration()) is at the top of the file

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


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)
}

Comment on lines +8 to +10
"github.com/hashicorp/terraform/helper/resource"
"github.com/hashicorp/terraform/helper/schema"
"github.com/hashicorp/terraform/helper/validation"
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.

Comment on lines +34 to +41
value := staticValue["value"]
staticValues = make([]*string, 0)
staticValues = append(staticValues, &value)
sv := configservice.StaticValue{
Values: staticValues,
}
rpv.StaticValue = &sv
results[staticValue["key"]] = &rpv
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"])},
    },
}

* `config_rule_name` - (Required) The name of the AWS Config rule
* `resource_type` - (Optional) The type of a resource
* `target_id` - (Required) Target ID is the name of the public document
* `target_type` - (Required) The type of the target. Target executes remediation. For example, SSM document
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this field only accepts a specific list of values, we should list the valid values. In this case, there's only one valid value, so it could read "The only current valid value is SSM_DOCUMENT"

* `target_id` - (Required) Target ID is the name of the public document
* `target_type` - (Required) The type of the target. Target executes remediation. For example, SSM document
* `target_version` - (Required) Version of the target. For example, version of the SSM document

Copy link
Contributor

Choose a reason for hiding this comment

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

We should add an entry for parameters here at the top level, and have a description like "Up to 25 parameter blocks. Parameters are documented below."

* `target_type` - (Required) The type of the target. Target executes remediation. For example, SSM document
* `target_version` - (Required) Version of the target. For example, version of the SSM document

### `parameters`
Copy link
Contributor

Choose a reason for hiding this comment

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

This heading should read "A parameters block supports the following arguments:"


AWS managed rules can be used by setting the source owner to `AWS` and the source identifier to the name of the managed rule. More information about AWS managed rules can be found in the [AWS Config Developer Guide](https://docs.aws.amazon.com/config/latest/developerguide/evaluate-config_use-managed-rules.html).

```hcl
Copy link
Contributor

Choose a reason for hiding this comment

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

For examples in the documentation, it's better for them to be short so that the essential parts are more obvious. In this case, we'd be ok to remove the IAM resources, or possibly leave the definition but remove the attributes. It does have the trade-off that these long examples aren't executable as-is.


Provides an AWS Config Remediation Configuration.

~> **Note:** Config Remediation Configuration requires an existing [Config Rule](/docs/providers/aws/r/config_config_rule.html) to be present. Use of `depends_on` is recommended (as shown below) to avoid race conditions.
Copy link
Contributor

Choose a reason for hiding this comment

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

If we set the config_rule_name of the remediation to the name valueof the rule, then we can get the built-in Terraform dependency management without usingdepends_on`

@mikeokner
Copy link

I'm open to picking up the torch if necessary here.

@cgetzen
Copy link
Contributor

cgetzen commented Jun 22, 2020

Hello. I had to get this rolling quick for my org. This is rebased and added too. This is everything squashed (I figured there may be an issue with signing Andy's rebased commits with my key).

Feel free to merge either, or cherry-pick changes over to this PR, or just use my changes as refs. Thank you and looking forward to easily creating remediation configs :)

@gdavison
Copy link
Contributor

This work was completed in #13884. Thanks, @andy-b-84 and @cgetzen!

@gdavison gdavison closed this Sep 16, 2020
@ghost
Copy link

ghost commented Sep 17, 2020

This has been released in version 3.7.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

@ghost
Copy link

ghost commented Oct 17, 2020

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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked as resolved and limited conversation to collaborators Oct 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Introduces or discusses updates to documentation. new-resource Introduces a new resource. provider Pertains to the provider itself, rather than any interaction with AWS. service/configservice Issues and PRs that pertain to the configservice service. size/XL Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support ConfigService Remediation Configuration
8 participants