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: aws_ssm_resource_data_sync #1895

Merged
merged 6 commits into from
Nov 10, 2017
Merged
Show file tree
Hide file tree
Changes from 2 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 @@ -450,6 +450,7 @@ func Provider() terraform.ResourceProvider {
"aws_ssm_patch_baseline": resourceAwsSsmPatchBaseline(),
"aws_ssm_patch_group": resourceAwsSsmPatchGroup(),
"aws_ssm_parameter": resourceAwsSsmParameter(),
"aws_ssm_resource_data_sync": resourceAwsSsmResourceDataSync(),
"aws_spot_datafeed_subscription": resourceAwsSpotDataFeedSubscription(),
"aws_spot_instance_request": resourceAwsSpotInstanceRequest(),
"aws_spot_fleet_request": resourceAwsSpotFleetRequest(),
Expand Down
180 changes: 180 additions & 0 deletions aws/resource_aws_ssm_resource_data_sync.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,180 @@
package aws

import (
"bytes"
"fmt"
"log"
"strings"

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/aws/awserr"
"github.com/aws/aws-sdk-go/service/ssm"
"github.com/hashicorp/terraform/helper/hashcode"
"github.com/hashicorp/terraform/helper/schema"
)

func resourceAwsSsmResourceDataSync() *schema.Resource {
return &schema.Resource{
Create: resourceAwsSsmResourceDataSyncCreate,
Read: resourceAwsSsmResourceDataSyncRead,
Delete: resourceAwsSsmResourceDataSyncDelete,

Schema: map[string]*schema.Schema{
"name": &schema.Schema{
Type: schema.TypeString,
Required: true,
ForceNew: true,
},
"destination": {
Copy link
Member

Choose a reason for hiding this comment

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

Is there any reason we should drift away from the API here? i.e. how do you feel about renaming this to s3_destination?

Type: schema.TypeSet,
Copy link
Member

Choose a reason for hiding this comment

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

Since this is a singleton I think we can just make it TypeList as there's no value in computing the hash and it would also complicate referencing. TypeList can be referenced as s3_destination.0.key etc., whereas TypeSet is non-trivial to reference because the address is s3_destination.<computed-idx>.key.

Required: true,
ForceNew: true,
MaxItems: 1,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"key": {
Copy link
Member

Choose a reason for hiding this comment

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

As above - kms_key_arn?

Type: schema.TypeString,
Optional: true,
},
"bucket": {
Copy link
Member

Choose a reason for hiding this comment

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

As above - bucket_name?

Type: schema.TypeString,
Required: true,
},
"prefix": {
Type: schema.TypeString,
Optional: true,
},
"region": {
Type: schema.TypeString,
Required: true,
},
},
},
Set: func(v interface{}) int {
var buf bytes.Buffer
m := v.(map[string]interface{})
buf.WriteString(fmt.Sprintf("%s-", strings.ToLower(m["bucket"].(string))))
buf.WriteString(fmt.Sprintf("%s-", strings.ToLower(m["region"].(string))))
if val, ok := m["key"]; ok {
buf.WriteString(fmt.Sprintf("%s-", strings.ToLower(val.(string))))
}
if val, ok := m["prefix"]; ok {
buf.WriteString(fmt.Sprintf("%s-", strings.ToLower(val.(string))))
}
return hashcode.String(buf.String())
Copy link
Member

Choose a reason for hiding this comment

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

Can you please explain why do we need to lowercase all the values here? It seems unnecessary to me. 🤔

},
},
},
}
}

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

input := &ssm.CreateResourceDataSyncInput{
S3Destination: &ssm.ResourceDataSyncS3Destination{
SyncFormat: aws.String(ssm.ResourceDataSyncS3FormatJsonSerDe),
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps a nitpick, but how do you feel about exposing this argument via schema too? It would help us to be prepared for time when AWS introduces new sync format(s).

},
SyncName: aws.String(d.Get("name").(string)),
}
destination := d.Get("destination").(*schema.Set).List()
for _, v := range destination {
dest := v.(map[string]interface{})
input.S3Destination.SetBucketName(dest["bucket"].(string))
input.S3Destination.SetRegion(dest["region"].(string))
if val, ok := dest["key"].(string); ok && val != "" {
input.S3Destination.SetAWSKMSKeyARN(val)
}
if val, ok := dest["prefix"].(string); ok && val != "" {
input.S3Destination.SetPrefix(val)
}
}

_, err := conn.CreateResourceDataSync(input)
if err != nil {
if aerr, ok := err.(awserr.Error); ok {
switch aerr.Code() {
case ssm.ErrCodeResourceDataSyncAlreadyExistsException:
if err := resourceAwsSsmResourceDataSyncDeleteAndCreate(meta, input); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Can you please explain why we should ever recreate the resource? The common convention (and user's expectations) is to just error out in case there's a conflict.

return err
}
default:
return err
}
} else {
return err
}
}
d.SetId(d.Get("name").(string))
return resourceAwsSsmResourceDataSyncRead(d, meta)
}

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

nextToken := ""
found := false
for {
input := &ssm.ListResourceDataSyncInput{}
if nextToken != "" {
input.NextToken = aws.String(nextToken)
}
resp, err := conn.ListResourceDataSync(input)
if err != nil {
return err
}
Copy link
Member

@radeksimko radeksimko Nov 10, 2017

Choose a reason for hiding this comment

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

I'm slightly worried about this conditional here, because we may add more errors to findResourceDataSyncItem in the future which aren't awserr and those may have nothing to do with "resource not found".
How do you feel about tweaking findResourceDataSyncItem to return nil, nil if no resource was found and then checking the nil here, instead of checking the error isn't awserr.Error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure👍 I'll apply that idea!

for _, v := range resp.ResourceDataSyncItems {
if *v.SyncName == d.Get("name").(string) {
found = true
}
}
if found || *resp.NextToken == "" {
break
}
nextToken = *resp.NextToken
}
Copy link
Member

Choose a reason for hiding this comment

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

Do you mind moving this for loop into a separate function, e.g. findResourceDataSync(conn *ssm.SSM, name string) *ssm.ResourceDataSyncItem?

if !found {
log.Printf("[INFO] No Resource Data Sync found for SyncName: %s", d.Get("name").(string))
}
Copy link
Member

Choose a reason for hiding this comment

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

There's a few things missing here - we need to re-set all the fields here based on what came from the API, e.g.

d.Set("s3_destination", flattenS3Destination(ds.S3Destination))

where the flattener will turn the SDK object into []interface{}

and secondly if there's no sync found we should remove it from state by calling d.SetId("").

return nil
}

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

input := &ssm.DeleteResourceDataSyncInput{
SyncName: aws.String(d.Get("name").(string)),
}

_, err := conn.DeleteResourceDataSync(input)
if err != nil {
if aerr, ok := err.(awserr.Error); ok {
switch aerr.Code() {
case ssm.ErrCodeResourceDataSyncNotFoundException:
return nil
default:
return err
}
}
return err
}
Copy link
Member

Choose a reason for hiding this comment

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

All of the above can be IMO simplified to something like this:

if isAWSErr(err, ssm.ErrCodeResourceDataSyncNotFoundException, "") {
    return nil
}
return err

return nil
}

func resourceAwsSsmResourceDataSyncDeleteAndCreate(meta interface{}, input *ssm.CreateResourceDataSyncInput) error {
conn := meta.(*AWSClient).ssmconn

delinput := &ssm.DeleteResourceDataSyncInput{
SyncName: input.SyncName,
}

_, err := conn.DeleteResourceDataSync(delinput)
if err != nil {
return err
}
_, err = conn.CreateResourceDataSync(input)
if err != nil {
return err
}
return nil
}
130 changes: 130 additions & 0 deletions aws/resource_aws_ssm_resource_data_sync_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
package aws

import (
"fmt"
"log"
"testing"

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/service/ssm"
"github.com/hashicorp/terraform/helper/acctest"
"github.com/hashicorp/terraform/helper/resource"
"github.com/hashicorp/terraform/terraform"
)

func TestAccAWSSsmResourceDataSync(t *testing.T) {
rInt := acctest.RandInt()
resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckAWSSsmResourceDataSyncDestroy,
Steps: []resource.TestStep{
{
Config: testAccSsmResourceDataSyncConfig(rInt),
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSSsmResourceDataSyncExists("aws_ssm_resource_data_sync.foo"),
),
},
},
})
}

func testAccCheckAWSSsmResourceDataSyncDestroy(s *terraform.State) error {
conn := testAccProvider.Meta().(*AWSClient).ssmconn

for _, rs := range s.RootModule().Resources {
if rs.Type != "aws_ssm_resource_data_sync" {
continue
}
nextToken := ""
found := false
for {
input := &ssm.ListResourceDataSyncInput{}
if nextToken != "" {
input.NextToken = aws.String(nextToken)
}
resp, err := conn.ListResourceDataSync(input)
if err != nil {
return err
}
Copy link
Member

Choose a reason for hiding this comment

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

^ I share the same concerns here as above

for _, v := range resp.ResourceDataSyncItems {
if *v.SyncName == rs.Primary.Attributes["name"] {
found = true
}
}
if resp.NextToken != nil {
nextToken = *resp.NextToken
}
if found || nextToken == "" {
break
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Once you have decoupled the logic for finding the right data sync, it can also be reused here 😉

if found {
return fmt.Errorf("[DELETE ERROR] Resource Data Sync found for SyncName: %s", rs.Primary.Attributes["name"])
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick, but there's no reason to prefix the message with [DELETE ERROR] - it's already clear that it's error.

}
}
return nil
}

func testAccCheckAWSSsmResourceDataSyncExists(name string) resource.TestCheckFunc {
return func(s *terraform.State) error {
log.Println(s.RootModule().Resources)
_, ok := s.RootModule().Resources[name]
if !ok {
return fmt.Errorf("Not found: %s", name)
}
return nil
}
}

func testAccSsmResourceDataSyncConfig(randInt int) string {
return fmt.Sprintf(`
resource "aws_s3_bucket" "hoge" {
bucket = "tf-test-bucket-%d"
region = "us-east-1"
Copy link
Member

Choose a reason for hiding this comment

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

Any particular reason why this bucket should be in us-east-1? It's certainly no big deal, just that our primary test region is us-west-2.

force_destroy = true
}

resource "aws_s3_bucket_policy" "hoge" {
bucket = "${aws_s3_bucket.hoge.bucket}"
policy = <<EOF
{
"Version": "2012-10-17",
"Statement": [
{
"Sid": "SSMBucketPermissionsCheck",
"Effect": "Allow",
"Principal": {
"Service": "ssm.amazonaws.com"
},
"Action": "s3:GetBucketAcl",
"Resource": "arn:aws:s3:::tf-test-bucket-%d"
},
{
"Sid": " SSMBucketDelivery",
"Effect": "Allow",
"Principal": {
"Service": "ssm.amazonaws.com"
},
"Action": "s3:PutObject",
"Resource": ["arn:aws:s3:::tf-test-bucket-%d/*"],
"Condition": {
"StringEquals": {
"s3:x-amz-acl": "bucket-owner-full-control"
}
}
}
]
}
EOF
}

resource "aws_ssm_resource_data_sync" "foo" {
name = "foo"
Copy link
Member

Choose a reason for hiding this comment

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

Do you mind randomizing the name here?

destination = {
bucket = "${aws_s3_bucket.hoge.bucket}"
region = "${aws_s3_bucket.hoge.region}"
}
}
`, randInt, randInt, randInt)
}
4 changes: 4 additions & 0 deletions website/aws.erb
Original file line number Diff line number Diff line change
Expand Up @@ -1425,6 +1425,10 @@
<a href="/docs/providers/aws/r/ssm_parameter.html">aws_ssm_parameter</a>
</li>

<li<%= sidebar_current("docs-aws-resource-ssm-resource-data-sync") %>>
<a href="/docs/providers/aws/r/ssm_resource_data_sync.html">aws_ssm_resource_data_sync</a>
</li>

</ul>
</li>

Expand Down
Loading