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_media_store_container #2448

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
3 changes: 3 additions & 0 deletions aws/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ import (
"github.com/aws/aws-sdk-go/service/kms"
"github.com/aws/aws-sdk-go/service/lambda"
"github.com/aws/aws-sdk-go/service/lightsail"
"github.com/aws/aws-sdk-go/service/mediastore"
"github.com/aws/aws-sdk-go/service/mq"
"github.com/aws/aws-sdk-go/service/opsworks"
"github.com/aws/aws-sdk-go/service/rds"
Expand Down Expand Up @@ -188,6 +189,7 @@ type AWSClient struct {
batchconn *batch.Batch
athenaconn *athena.Athena
dxconn *directconnect.DirectConnect
mediastoreconn *mediastore.MediaStore
}

func (c *AWSClient) S3() *s3.S3 {
Expand Down Expand Up @@ -426,6 +428,7 @@ func (c *Config) Client() (interface{}, error) {
client.batchconn = batch.New(sess)
client.athenaconn = athena.New(sess)
client.dxconn = directconnect.New(sess)
client.mediastoreconn = mediastore.New(sess)

// Workaround for https://github.com/aws/aws-sdk-go/issues/1376
client.kinesisconn.Handlers.Retry.PushBack(func(r *request.Request) {
Expand Down
1 change: 1 addition & 0 deletions aws/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -400,6 +400,7 @@ func Provider() terraform.ResourceProvider {
"aws_main_route_table_association": resourceAwsMainRouteTableAssociation(),
"aws_mq_broker": resourceAwsMqBroker(),
"aws_mq_configuration": resourceAwsMqConfiguration(),
"aws_media_store_container": resourceAwsMediaStoreContainer(),
"aws_nat_gateway": resourceAwsNatGateway(),
"aws_network_acl": resourceAwsNetworkAcl(),
"aws_default_network_acl": resourceAwsDefaultNetworkAcl(),
Expand Down
149 changes: 149 additions & 0 deletions aws/resource_aws_media_store_container.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,149 @@
package aws

import (
"fmt"
"regexp"
"time"

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

func resourceAwsMediaStoreContainer() *schema.Resource {
return &schema.Resource{
Create: resourceAwsMediaStoreContainerCreate,
Read: resourceAwsMediaStoreContainerRead,
Update: resourceAwsMediaStoreContainerUpdate,
Delete: resourceAwsMediaStoreContainerDelete,

Schema: map[string]*schema.Schema{
"name": &schema.Schema{
Type: schema.TypeString,
Required: true,
ForceNew: true,
ValidateFunc: func(v interface{}, k string) (ws []string, errors []error) {
value := v.(string)
if !regexp.MustCompile("^\\w+$").MatchString(value) {
errors = append(errors, fmt.Errorf("%q must match \\w+", k))
Copy link
Member

Choose a reason for hiding this comment

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

Can we make this error message a bit more human friendly for folks who don't know regular expressions? e.g. must contain alphanumeric characters or underscores?

}
return
},
},
"policy": {
Type: schema.TypeString,
Optional: true,
ValidateFunc: validateIAMPolicyJson,
DiffSuppressFunc: suppressEquivalentAwsPolicyDiffs,
},
"arn": {
Type: schema.TypeString,
Computed: true,
},
"endpoint": {
Type: schema.TypeString,
Computed: true,
},
},
}
}

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

input := &mediastore.CreateContainerInput{
ContainerName: aws.String(d.Get("name").(string)),
}

_, err := conn.CreateContainer(input)
if err != nil {
return err
}
stateConf := &resource.StateChangeConf{
Pending: []string{mediastore.ContainerStatusCreating},
Target: []string{mediastore.ContainerStatusActive},
Refresh: mediaStoreContainerRefreshStatusFunc(conn, d.Get("name").(string)),
Timeout: 10 * time.Minute,
Delay: 10 * time.Second,
MinTimeout: 3 * time.Second,
}

_, err = stateConf.WaitForState()
if err != nil {
return fmt.Errorf("[WARN] Error waiting for MediaStore Container status to be \"ACTIVE\": %s", err)
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 I think the error which comes out of WaitForState is sufficiently detailed so we don't need to duplicate the context here.

}

d.SetId(d.Get("name").(string))
return resourceAwsMediaStoreContainerUpdate(d, meta)
}

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

input := &mediastore.DescribeContainerInput{
ContainerName: aws.String(d.Id()),
}
resp, err := conn.DescribeContainer(input)
if err != nil {
return err
}
d.Set("arn", resp.Container.ARN)
d.Set("endpoint", resp.Container.Endpoint)
return nil
}

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

if !d.HasChange("policy") {
return resourceAwsMediaStoreContainerRead(d, meta)
}
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 the above 3 lines will never be executed since all fields are ForceNew (so the only way the function ever gets executed is when policy has changed), unless I'm mistaken?

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 called resourceAwsMediaStoreContainerUpdate from the end of resourceAwsMediaStoreContainerCreate so I needed the above 3 lines.
Separating policy as aws_media_store_container_policy will solve this complication.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see - that's confusing. It actually makes me reconsider what I said in my other comment - I think we should remove all the policy related logic from this PR and implement it as a separate resource in another PR.

Do you agree?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree👍 I'll remove


input := &mediastore.PutContainerPolicyInput{
ContainerName: aws.String(d.Id()),
Policy: aws.String(d.Get("policy").(string)),
}
_, err := conn.PutContainerPolicy(input)
if err != nil {
return err
}

return resourceAwsMediaStoreContainerRead(d, meta)
}

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

input := &mediastore.DeleteContainerInput{
ContainerName: aws.String(d.Id()),
}
_, err := conn.DeleteContainer(input)
if err != nil {
if ecrerr, ok := err.(awserr.Error); ok {
switch ecrerr.Code() {
case mediastore.ErrCodeContainerNotFoundException:
d.SetId("")
return nil
}
}
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 I think we could simplify the above error handling to something like

if isAWSErr(err, mediastore.ErrCodeContainerNotFoundException, "") {
    d.SetId("")
    return 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.

As this resource is stateful shouldn't we also wait here until it's deleted instead of leaving it in "deleting" state?


d.SetId("")
return nil
}

func mediaStoreContainerRefreshStatusFunc(conn *mediastore.MediaStore, cn string) resource.StateRefreshFunc {
return func() (interface{}, string, error) {
input := &mediastore.DescribeContainerInput{
ContainerName: aws.String(cn),
}
resp, err := conn.DescribeContainer(input)
if err != nil {
return nil, "failed", err
}
return resp, *resp.Container.Status, nil
}
}
109 changes: 109 additions & 0 deletions aws/resource_aws_media_store_container_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
package aws

import (
"fmt"
"testing"

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

func TestAccAwsMediaStoreContainer_basic(t *testing.T) {
rName := acctest.RandString(5)
resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckAwsMediaStoreContainerDestroy,
Steps: []resource.TestStep{
{
Config: testAccMediaStoreContainerConfig(rName),
Check: resource.ComposeTestCheckFunc(
testAccCheckAwsMediaStoreContainerExists("aws_media_store_container.test"),
),
},
{
Config: testAccMediaStoreContainerConfig_Update(rName),
Check: resource.ComposeTestCheckFunc(
testAccCheckAwsMediaStoreContainerExists("aws_media_store_container.test"),
resource.TestCheckResourceAttrSet("aws_media_store_container.test", "policy"),
),
},
},
})
}

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

for _, rs := range s.RootModule().Resources {
if rs.Type != "aws_media_store_container" {
continue
}

input := &mediastore.DescribeContainerInput{
ContainerName: aws.String(rs.Primary.ID),
}

resp, err := conn.DescribeContainer(input)
if err != nil {
if ecrerr, ok := err.(awserr.Error); ok {
switch ecrerr.Code() {
case mediastore.ErrCodeContainerNotFoundException:
return nil
}
}
Copy link
Member

Choose a reason for hiding this comment

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

As mentioned elsewhere - I think the error handling can be simplified a bit.

return err
}

if *resp.Container.Status != mediastore.ContainerStatusDeleting {
return fmt.Errorf("Media Store Container (%s) not deleted", rs.Primary.ID)
}
}
return nil
}

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

return nil
}
}

func testAccMediaStoreContainerConfig(rName string) string {
return fmt.Sprintf(`
resource "aws_media_store_container" "test" {
name = "tf_mediastore_%s"
}`, rName)
}

func testAccMediaStoreContainerConfig_Update(rName string) string {
return fmt.Sprintf(`
data "aws_caller_identity" "test" {}

resource "aws_media_store_container" "test" {
name = "tf_mediastore_%s"
policy = <<POLICY
{
"Version": "2012-10-17",
"Statement": [{
"Sid": "MediaStoreFullAccess",
"Action": [ "mediastore:*" ],
"Principal": {"AWS" : "*"},
"Effect": "Allow",
"Resource": "arn:aws:mediastore:us-west-2:${data.aws_caller_identity.test.account_id}:container/tf_mediastore_%s/*",
Copy link
Member

Choose a reason for hiding this comment

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

It looks like we'd benefit from having aws_media_store_container_policy so we can reference the container ARN instead of building it like this. 🤔

It's certainly outside of scope of this PR, just thinking out loud as I'm reading the code...

Copy link
Contributor Author

@atsushi-ishibashi atsushi-ishibashi Dec 3, 2017

Choose a reason for hiding this comment

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

Yeah, I agree with you...
"Resource" in policy must be scope of arn:aws:mediastore:us-west-2:1234567890:container/tf_mediastore_%s/ (I got error if I set * to Resource) so I thought the same when implementing.
If we'll develop aws_media_store_container_policy, should I remove policy from aws_media_store_container to simplify relation and these roles?

Copy link
Member

Choose a reason for hiding this comment

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

No, keep it here... as I said it's outside of scope of this PR 😃

Copy link
Member

Choose a reason for hiding this comment

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

I'm taking back what I said - see my other comment about policy.

"Condition": {
"Bool": { "aws:SecureTransport": "true" }
}
}]
}
POLICY
}`, rName, rName)
}
12 changes: 12 additions & 0 deletions website/aws.erb
Original file line number Diff line number Diff line change
Expand Up @@ -1118,6 +1118,18 @@
<li<%= sidebar_current("docs-aws-resource-mq-configuration") %>>
<a href="/docs/providers/aws/r/mq_configuration.html">aws_mq_configuration</a>
</li>

</ul>
</li>

<li<%= sidebar_current("docs-aws-resource-media-store") %>>
<a href="#">MediaStore Resources</a>
<ul class="nav nav-visible">

<li<%= sidebar_current("docs-aws-resource-media-store-container") %>>
<a href="/docs/providers/aws/r/media_store_container.html">aws_media_store_container</a>
</li>

</ul>
</li>

Expand Down
33 changes: 33 additions & 0 deletions website/docs/r/media_store_container.html.markdown
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
---
layout: "aws"
page_title: "AWS: aws_media_store_container"
sidebar_current: "docs-aws-resource-media-store-container"
description: |-
Provides a MediaStore Container.
---

# aws_media_store_container

Provides a MediaStore Container.

## Example Usage

```hcl
resource "aws_media_store_container" "example" {
name = "example"
}
```

## Argument Reference

The following arguments are supported:

* `name` - (Required) The name of the container. Must atisfy regular expression pattern: `\w+`
Copy link
Member

Choose a reason for hiding this comment

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

Can we make this a little bit more human-readable for folks who don't know regular expressions or omit this sentence (as the validation error message will tell them anyways)?

* `policy` - (Optional) The policy of the container.

## Attributes Reference

The following attributes are exported:

* `arn` - The ARN of the container.
* `endpoint` - The DNS endpoint of the container.