provider/aws: Glacier Vault #3169

stack72 · 2015-09-03T21:59:03Z

Started to build the resource to create an AWS Glacier Vault

Please note that I am pretty new to Go and this will take a little while to complete while I learn more about it

Resource for Vault Management
Documentation
Acceptance Tests

I have added a few basic acceptance tests:

terraform [aws-glacier-resource●●] % make testacc TEST=./builtin/providers/aws TESTARGS='-run=GlacierVault' 2>~/tf.log
go generate ./...
TF_ACC=1 go test ./builtin/providers/aws -v -run=GlacierVault -timeout 90m
=== RUN TestAccAWSGlacierVault_basic
--- PASS: TestAccAWSGlacierVault_basic (3.31s)
=== RUN TestAccAWSGlacierVault_full
--- PASS: TestAccAWSGlacierVault_full (3.40s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    6.718s

radeksimko · 2015-09-04T07:33:17Z

Interestingly enough, I started working on Glacier Vault yesterday as well 😃
I'll be more than happy to help you finish this.

So far I did not get farther than you, but I think you could get an inspiration for the schema:
c6f049b#diff-ba99ee1ccca56010bc69f1fd4d49ff35R15

I think the resource should also support access_policy & notification block.
Also I think that if we're going to support account_id, it should be optional and the default account ID would be the one from AWS provider.

stack72 · 2015-09-04T08:09:43Z

@radeksimko that is quite funny. I am happy to continue to build it if you want to move on to something else?

I'd love your comments as my commits start to build though - it would really help me learn :)

radeksimko · 2015-09-04T08:16:46Z

I am happy to continue to build it if you want to move on to something else?

Sure, I can take the role of the reviewer here. There's bunch of other things that I should finish (GCE backend service, EFS, CloudWatch Log Group, ...).

stack72 · 2015-09-04T08:36:10Z

That'd be awesome. Will ping you for some feedback :)

stack72 · 2015-09-04T13:24:25Z

@radeksimko I have a question about the tags work. I am a bit lost here. A lot of the other AWS api's (EC2, RDS etc) have a Tag type defined

therefore, we use []_ec2.Tag or []_rds.Tag

Glacier doesn't. So I am not quite sure what i need to do to add and remove the tags

I have the skeleton of your code. I have also managed to get the tags back from AWS

func resourceAwsGlacierVaultRead(d *schema.ResourceData, meta interface{}) error {
    glacierconn := meta.(*AWSClient).glacierconn

    input := &glacier.DescribeVaultInput{
        VaultName: aws.String(d.Get("name").(string)),
    }

    out, err := glacierconn.DescribeVault(input)
    if err != nil {
        return fmt.Errorf("Error reading Glacier Vault: %s", err)
    }

    d.Set("arn", *out.VaultARN)

    tags, err := getGlacierVaultTags(glacierconn, d.Get("name").(string))
    if err != nil {
        return err
    }

    return nil
}

func getGlacierVaultTags(glacierconn *glacier.Glacier, vaultName string) (map[string]*string, error) {
    request := &glacier.ListTagsForVaultInput{
        VaultName: aws.String(vaultName),
    }

    response, err := glacierconn.ListTagsForVault(request)
    //validation required here on the response

    return response.Tags, nil
}

Thoughts?

Paul

radeksimko · 2015-09-04T14:21:01Z

@stack72 Have a look at setGlacierVaultTags & diffGlacierVaultTags in here:
c6f049b#diff-ba99ee1ccca56010bc69f1fd4d49ff35R99

I think setGlacierVaultTags is almost ready, diffGlacierVaultTags needs implementation that will take old and new map, compares these two and generates two new maps - first for adding/changing tags, second for deleting tags.

glacier.AddTagsToVault also works for modifying values of existing tags.

I think that map[string]*string makes things actually easier compared to []ec2.Tag or []rds.Tag.

Does that make sense? :)

stack72 · 2015-09-04T16:08:48Z

ok @radeksimko we are closer to the implementation right now. I have managed to get the following:

aws_glacier_vault.test: Creating...
  arn:          "" => "<computed>"
  location:     "" => "<computed>"
  name:         "" => "test10"
  tags.#:       "" => "1"
  tags.testKey: "" => "testValue"
aws_glacier_vault.test: Creation complete

Then on the second run, I see the following:

No changes. Infrastructure is up-to-date. This means that Terraform
could not detect any differences between your configuration and
the real physical resources t....

We create the vault and terraform recognises that the tags exist, but the tags are not persisted to AWS.

One issue I can across was getting a lot of panics when using map[string]*string. So I started using map[string]string and then changed it to pointers at the point in which we interact with AWS

I guess i need to figure out how to write some tests / debug where this is now failing to do the write

P.

stack72 · 2015-09-04T21:45:19Z

@radeksimko I need a little schema suggestion for the notifications please :)

The set-vault-notifications has the following params:

"VaultNotificationConfig":{
      "type":"structure",
      "members":{
        "SNSTopic":{"shape":"string"},
        "Events":{"shape":"NotificationEventList"}
      }
    },

I was going to go with the style like policy - just accepting a JSON document. BUT the SDK won't be able to deal with that AFAICT

Thoughts?

radeksimko · 2015-09-04T22:14:29Z

builtin/providers/aws/resource_aws_glacier_vault.go

+ Computed: true,
+ },
+
+ "policy": &schema.Schema{


Do you think we could rename this to access_policy? I think it's not too long and better to comply with the API naming convention.

Noted and changed :)

radeksimko · 2015-09-04T22:34:56Z

I was going to go with the style like policy - just accepting a JSON document. BUT the SDK won't be able to deal with that AFAICT

I think policy as JSON is fine since that's a convention for all other IAM policies elsewhere, but here I think that a simple TypeSet, like aws_elb.listener will do:
https://github.com/hashicorp/terraform/blob/master/builtin/providers/aws/resource_aws_elb.go#L124-L156

radeksimko · 2015-09-04T22:41:12Z

builtin/providers/aws/resource_aws_glacier_vault.go

+ VaultName: aws.String(d.Id()),
+ })
+ if err != nil {
+ if err := d.Set("policy", ""); err != nil {


I don' think this is necessary. Also you're totally hiding the original error away. d.Set will hardly ever fail, we typically don't check that for errors, but errors coming from API are much more important.

Also if you're just going to return err, you can leave out the else block below, which will help readability.

stack72 · 2015-09-06T20:34:14Z

@radeksimko ok, from looking at the notifications, I have decided to go with this:

"vault_notifications_config": &schema.Schema{
                Type:     schema.TypeSet,
                Optional: true,
                Elem: &schema.Resource{
                    "sns_topic": &schema.Schema{
                        Type:     schema.TypeInt,
                        Required: true,
                    },

                    "eventlist": &schema.Schema{
                        Type:     schema.TypeSet,
                        Required: true,
                        Elem:     &schema.Schema{Type: schema.TypeString},
                        Set:      schema.HashString,
                    },
                },
                Set: resourceAwsGlacierVaultNotificationsHash,
            },

I get the following error:

builtin/providers/aws/resource_aws_glacier_vault.go:51: invalid field name "sns_topic" in struct initializer
builtin/providers/aws/resource_aws_glacier_vault.go:58: invalid field name "eventlist" in struct initializer

One other thing, I will have to create the Hash as per the Set. I am not quite sure how I would do that when the eventlist is a List of strings

Thoughts?

P.

radeksimko · 2015-09-06T21:16:49Z

builtin/providers/aws/resource_aws_glacier_vault.go

+ Type: schema.TypeString,
+ Required: true,
+ ForceNew: true,
+ ValidateFunc: func(v interface{}, k string) (ws []string, errors []error) {


Great to see some validation here! 👍

radeksimko · 2015-09-06T21:41:09Z

ok, from looking at the notifications, I have decided to go with this:

If I take the commit I linked previously & polish it, (1) it explains how to define TypeSet schema and (2) suggests an (IMO) more human-friendly and "API-compliant" naming convention.

When you see something like this:

schema.Resource{
    "key": value
}

then you're trying to initialise type Resource from package schema while setting key to value, but schema.Resource is a generic Resource type, hence it expects Schema field of type map[string]*schema.Schema.

"notifications": &schema.Schema{
    Type:     schema.TypeSet,
    Optional: true,
    Elem: &schema.Resource{
        Schema: map[string]*schema.Schema{
            "events": &schema.Schema{
                Type:     schema.TypeSet,
                Required: true,
                Elem:     &schema.Schema{Type: schema.TypeString},
                Set:      schema.HashString,
            },
            "sns_topic": &schema.Schema{
                Type:     schema.TypeString,
                Optional: true,
            },
        },
    },
},

Double check what needs to be Required and what doesn't. Either via AWS docs (which do not always tell the truth from my experience) or by trial and error.

stack72 · 2015-09-06T22:41:05Z

this makes much more sense :) thanks!

stack72 · 2015-09-08T13:52:56Z

@radeksimko so I have started getting a really weird error

When running a plan or apply, I see the following:

* aws_glacier_vault.test: ResourceNotFoundException: No vault access policy is set for: test12
    status code: 404, request id: pnG3m5DDaEQTFQTp7x7NwIKBUmTgUQlQmt0_qQWpQEQdL_g

The resource declaration looks as follows:

resource "aws_glacier_vault" "test" {
    name = "test12"
    tags {
      Test="Test1"
    }
}

Terraform successfully adds the vault but then throws that 404 error later. I think I have something wrong in my code since the refactor

radeksimko · 2015-09-08T15:46:33Z

@stack72 This I believe comes from here https://github.com/hashicorp/terraform/pull/3169/files#diff-ba99ee1ccca56010bc69f1fd4d49ff35R116

I think you should be differentiating real error and "not found" error there.

Double check what the real code is, but you're probably looking for something like this:

if ec2err, ok := err.(awserr.Error); ok && ec2err.Code() == "ResourceNotFoundException" {

stack72 · 2015-09-15T15:51:28Z

@radeksimko ok, I'm back on this and you were correct :) Will try and finish the notifications this week :)

stack72 · 2015-09-15T23:10:10Z

@radeksimko I think this may be ready for a final round of reviews. I just tested this works as expected and all looks good. I even added a couple of acceptance tests

radeksimko · 2015-09-16T06:33:59Z

builtin/providers/aws/provider.go

@@ -178,6 +178,7 @@ func Provider() terraform.ResourceProvider {
 "aws_elasticache_subnet_group": resourceAwsElasticacheSubnetGroup(),
 "aws_elb": resourceAwsElb(),
 "aws_flow_log": resourceAwsFlowLog(),
+ "aws_glacier_vault": resourceAwsGlacierVault(),


Can you try gofmt all the files you changed to avoid things like this, please? 😉

All sorted now :)

stack72 · 2015-09-16T13:17:57Z

is it therefore ok to add something like this:

~> **NOTE:** When trying to remove a Glacier Vault, the Vault must be empty.

radeksimko · 2015-09-16T14:14:11Z

is it therefore ok to add something like this:

yeah, I think that will do.

radeksimko · 2015-09-16T19:36:15Z

@stack72 So I think this is ready to be merged, isn't it?
If you will just kindly squash the commits, I will merge it 😃

stack72 · 2015-09-17T00:50:49Z

@radeksimko all ready - I will add unit tests later (i need to learn this part)

radeksimko · 2015-10-05T01:12:34Z

@stack72 I think you may need to rebase last changes from the master - there seems to be a conflict.

Also, do you plan on adding those unit tests? 😉

Updating the glacier docs to include a link to the AWS developer guide

stack72 · 2015-10-05T08:50:10Z

@radeksimko ok, I have rebased from origin/master

I will open another PR today to add those unit tests - I don't want to add more commits to this already hectic branch :)

…s expected

stack72 · 2015-10-08T10:59:23Z

@radeksimko apologies for not seeing the conflict issue earlier in your comment. This has been rectified and it's ready to go now :)

stack72 · 2015-10-12T19:19:36Z

@radeksimko can we get a merge on this? :p

radeksimko · 2015-10-12T21:27:07Z

Sorry to keep you waiting here, I'm currently on holiday, but I will have a final look at this tomorrow.

I will probably rather merge this manually (yet keep your authorship in git), since I don't feel confident pulling in those "merge-conflict commits" and it doesn't look very nice in the git log either.

If you can do some nice rebase until I get to it tomorrow, I'll be much happier and probably just click that big green button. If you won't, that's ok, I'll look at it anyway, but it means a little bit more effort for me 😃

Sorry for being a git pedant 😉

ghost · 2020-04-30T02:25:50Z

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.

radeksimko added enhancement provider/aws labels Sep 4, 2015

radeksimko added the wip label Sep 4, 2015

radeksimko reviewed Sep 4, 2015
View reviewed changes

radeksimko reviewed Sep 6, 2015
View reviewed changes

stack72 changed the title ~~[WIP] AWS Resource for Glacier Vault~~ AWS Resource for Glacier Vault Sep 15, 2015

radeksimko reviewed Sep 16, 2015
View reviewed changes

radeksimko removed the wip label Sep 16, 2015

stack72 force-pushed the aws-glacier-resource branch 2 times, most recently from 6d6c288 to ea4400b Compare September 17, 2015 00:46

stack72 changed the title ~~AWS Resource for Glacier Vault~~ provider/aws: Glacier Vault Sep 28, 2015

radeksimko added waiting-response An issue/pull request is waiting for a response from the community wip labels Oct 5, 2015

stack72 added 3 commits October 5, 2015 09:47

Adding the ability to manage a glacier vault

57f7437

Adding the the docs for the Glacier Vault resource

0ee0976

Updating the glacier docs to include a link to the AWS developer guide

Gofmt of the aws glacier vault resource

23b85ee

stack72 force-pushed the aws-glacier-resource branch from ea4400b to 23b85ee Compare October 5, 2015 08:49

stack72 added 2 commits October 5, 2015 10:37

Fixing the merge conflicts for the AWS config.go file

0290fdf

Adding a test to make sure that the diffGlacierVaultTags func works a…

5ff75f8

…s expected

stack72 mentioned this pull request Oct 5, 2015

AWS diffGlacierVaultTags Test #3410

Closed

radeksimko removed waiting-response An issue/pull request is waiting for a response from the community wip labels Oct 7, 2015

Fixing the merge conflict between branch and master

18d1a38

stack72 mentioned this pull request Oct 13, 2015

Aws glacier resource merge #3491

Merged

3 tasks

stack72 closed this Oct 13, 2015

stack72 deleted the aws-glacier-resource branch October 13, 2015 14:05

ghost locked and limited conversation to collaborators Apr 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

provider/aws: Glacier Vault #3169

provider/aws: Glacier Vault #3169

stack72 commented Sep 3, 2015

radeksimko commented Sep 4, 2015

stack72 commented Sep 4, 2015

radeksimko commented Sep 4, 2015

stack72 commented Sep 4, 2015

stack72 commented Sep 4, 2015

radeksimko commented Sep 4, 2015

stack72 commented Sep 4, 2015

stack72 commented Sep 4, 2015

radeksimko Sep 4, 2015

stack72 Sep 6, 2015

radeksimko commented Sep 4, 2015

radeksimko Sep 4, 2015

radeksimko Sep 4, 2015

stack72 commented Sep 6, 2015

radeksimko Sep 6, 2015

radeksimko commented Sep 6, 2015

stack72 commented Sep 6, 2015

stack72 commented Sep 8, 2015

radeksimko commented Sep 8, 2015

stack72 commented Sep 15, 2015

stack72 commented Sep 15, 2015

radeksimko Sep 16, 2015

stack72 Sep 16, 2015

stack72 commented Sep 16, 2015

radeksimko commented Sep 16, 2015

radeksimko commented Sep 16, 2015

stack72 commented Sep 17, 2015

radeksimko commented Oct 5, 2015

stack72 commented Oct 5, 2015

stack72 commented Oct 8, 2015

stack72 commented Oct 12, 2015

radeksimko commented Oct 12, 2015

ghost commented Apr 30, 2020

provider/aws: Glacier Vault #3169

provider/aws: Glacier Vault #3169

Conversation

stack72 commented Sep 3, 2015

radeksimko commented Sep 4, 2015

stack72 commented Sep 4, 2015

radeksimko commented Sep 4, 2015

stack72 commented Sep 4, 2015

stack72 commented Sep 4, 2015

radeksimko commented Sep 4, 2015

stack72 commented Sep 4, 2015

stack72 commented Sep 4, 2015

radeksimko Sep 4, 2015

Choose a reason for hiding this comment

stack72 Sep 6, 2015

Choose a reason for hiding this comment

radeksimko commented Sep 4, 2015

radeksimko Sep 4, 2015

Choose a reason for hiding this comment

radeksimko Sep 4, 2015

Choose a reason for hiding this comment

stack72 commented Sep 6, 2015

radeksimko Sep 6, 2015

Choose a reason for hiding this comment

radeksimko commented Sep 6, 2015

stack72 commented Sep 6, 2015

stack72 commented Sep 8, 2015

radeksimko commented Sep 8, 2015

stack72 commented Sep 15, 2015

stack72 commented Sep 15, 2015

radeksimko Sep 16, 2015

Choose a reason for hiding this comment

stack72 Sep 16, 2015

Choose a reason for hiding this comment

stack72 commented Sep 16, 2015

radeksimko commented Sep 16, 2015

radeksimko commented Sep 16, 2015

stack72 commented Sep 17, 2015

radeksimko commented Oct 5, 2015

stack72 commented Oct 5, 2015

stack72 commented Oct 8, 2015

stack72 commented Oct 12, 2015

radeksimko commented Oct 12, 2015

ghost commented Apr 30, 2020