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

provider/aws: New SSM Parameter resource #14043

Closed
wants to merge 2 commits into from
Closed

provider/aws: New SSM Parameter resource #14043

wants to merge 2 commits into from

Conversation

tomelliff
Copy link
Contributor

@tomelliff tomelliff commented Apr 27, 2017

This new resource can be used for creating parameters in AWS' SSM Parameter Store that can then be used by other applications that have access to AWS and necessary IAM permissions.

Acceptance tests are currently passing:

$ make testacc TEST=./builtin/providers/aws TESTARGS="-run=TestAccAWSSSMParameter_"
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/04/27 16:16:06 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSSSMParameter_ -timeout 120m
=== RUN   TestAccAWSSSMParameter_basic
--- PASS: TestAccAWSSSMParameter_basic (33.45s)
=== RUN   TestAccAWSSSMParameter_update
--- PASS: TestAccAWSSSMParameter_update (58.78s)
=== RUN   TestAccAWSSSMParameter_secure
--- PASS: TestAccAWSSSMParameter_secure (32.97s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/aws	125.224s

$ make testacc TEST=./builtin/providers/aws TESTARGS="-run=TestValidateSsmParameterType"
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/04/27 16:13:52 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestValidateSsmParameterType -timeout 120m
=== RUN   TestValidateSsmParameterType
--- PASS: TestValidateSsmParameterType (0.00s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/aws	0.021s

This is my first stab at creating a new resource and I'm not all that used to Go just yet either so I might be missing a whole bunch of things here and would love any feedback I get from this.

This was previously requested in #11433. Would be good to add a data source for this at some point as well but this solves an immediate use case I have right now (providing a secret to a Lambda function that I deploy using Terraform).

TODO:

  • Documentation
  • Optionally use CMK to encrypt SecureString - might be left to a later date
  • Any missing tests?

Can be used for creating parameters in AWS' SSM Parameter Store that can then be used by other applications that have access to AWS and necessary IAM permissions.
@tomelliff
Copy link
Contributor Author

I'm unsure if I should be marking value as Sensitive because it's only really sensitive for SecureStrings. Am I right in thinking this will just hide it from logs but still leave it in plain text in state? And is there a downside to it or should I just do it here because some of the time it will be sensitive?

@tomelliff
Copy link
Contributor Author

@radeksimko sorry to nag you but is there any chance you could take a look at this as is please? I'm starting a new job on Monday so probably won't get much time for a while to look at this again after this week.

My testing with it seems to do everything I need it to do (including working with the file interpolation which is my main requirement for storing SSH keys in SSM Parameter store) but not sure if it needs any more automated tests at all?

I'm happy without the CMK option for SecureStrings right now but that might be useful to be added in later -
did you want me to raise an issue for this for completeness?

@radeksimko radeksimko self-assigned this May 3, 2017
@tyjonesAncestry
Copy link

@radeksimko Can you approve and merge this? My org also needs this ASAP.

Thanks!

@tomelliff
Copy link
Contributor Author

tomelliff commented May 10, 2017

Just noticed that there's merge conflicts on the branch now. I'll sort those as soon as the rest of the code has been reviewed.

EDIT: Conflicts look fine, just need to move this parameter store stuff after aws_ssm_maintenance_window in the provider and website docs and take both sides of the validation files.

@wyaeld
Copy link

wyaeld commented May 15, 2017

@tomelliff Would you also be interested to produce a data source version of the parameter_store for reading existing values? I think you already have most of the implementation code

similar to https://www.terraform.io/docs/providers/aws/d/kms_alias.html
eg.

data "aws_ssm_parameter" "db_master_password" {
  name = "some_project/some_environment/rds/master_password"
}

We work in sensitive environments, so use parameter store for generic secrets management since we can pair it with IAM policies that limit application access to them.

@tyjonesAncestry
Copy link

+1. If the data source and the ability to create them were in Terraform that would be fantastic.

@tomelliff
Copy link
Contributor Author

Yeah, happy to add a data source once this has been reviewed and I get some free time. Would like to check if this is missing anything glaring first though.

Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

Hi @tomelliff
thank you for submitting this PR.

This looks overall 👌 I just left you a few questions there. Would you mind looking over those?

},
"value": {
Type: schema.TypeString,
Required: true,
Copy link
Member

Choose a reason for hiding this comment

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

Is it worth marking this field as Sensitive as it may contain sensitive data?

Name: aws.String(d.Get("name").(string)),
Type: aws.String(d.Get("type").(string)),
Value: aws.String(d.Get("value").(string)),
Overwrite: aws.Bool(overwrite),
Copy link
Member

Choose a reason for hiding this comment

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

Just FYI - there's d.IsNewResource() which can basically tell you whether this is being called from Create() or Update() and therefore you could reduce the number of arguments as you already have *schema.ResourceData there.

}

log.Printf("[DEBUG] Waiting for SSM Parameter %q to be updated", d.Get("name").(string))
err := resource.Retry(5*time.Minute, func() *resource.RetryError {
Copy link
Member

Choose a reason for hiding this comment

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

What exactly is the reason for retrying this operation if there's no retryable error? 🤔

@radeksimko
Copy link
Member

Also would you mind rebasing your branch & resolving conflicts? Thanks.

@radeksimko radeksimko added the waiting-response An issue/pull request is waiting for a response from the community label May 16, 2017
@analytically
Copy link

Will this be part of 0.9.7?

pmorton added a commit to pmorton/terraform that referenced this pull request Jun 2, 2017
- Addressed all issues in hashicorp#14043
- Added ForceNew directive to type
- Added the ability to specify a KMS key for encryption and decryption
@wyaeld
Copy link

wyaeld commented Jun 5, 2017

@tomelliff even if you're busy, could you please prioritize spending a couple of hours and finishing this.

radeksimko pushed a commit that referenced this pull request Jun 6, 2017
* New SSM Parameter resource

Can be used for creating parameters in AWS' SSM Parameter Store that can then be used by other applications that have access to AWS and necessary IAM permissions.

* Add docs for new SSM Parameter resource

* Code Review and Bug Hunt and KMS Key
- Addressed all issues in #14043
- Added ForceNew directive to type
- Added the ability to specify a KMS key for encryption and decryption

* Add SSM Parameter Data Source

* Fix bad merge

* Fix SSM Parameter Integration Tests

* docs/aws: Fix typo in SSM sidebar link
@radeksimko
Copy link
Member

#15035 was just merged. @tomelliff thank you for all the initial work and @pmorton for taking it to the finish line 🎉

@radeksimko radeksimko closed this Jun 6, 2017
@radeksimko radeksimko removed the waiting-response An issue/pull request is waiting for a response from the community label Jun 6, 2017
@ghost
Copy link

ghost commented Apr 11, 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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants