provider/aws: Implementing aws_ami_launch_permission resource #7365

BSick7 · 2016-06-27T14:06:41Z

Overview

There is currently no way to manage AMI Launch Permissions in terraform.
This PR introduces aws_ami_launch_permission which allows a user to provision launch permissions to other aws accounts.

Syntax

provider "aws" {
    //...
}

resource "aws_ami_launch_permission" {
    image_id = "${var.image_id}"
    account_id = "${var.aws_account_id}"
}

Checklist

Implement aws_ami_launch_permission.
Add resource to provider.
Test aws_ami_launch_permission.
Add resource to website docs.

BSick7 · 2016-06-27T20:11:16Z

@stack72 ready for review

stack72 · 2016-06-28T01:06:02Z

Hi @BSick7

Unfortunately, when running the tests, I get the following:

make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSAMILaunchPermission'
==> Checking that code complies with gofmt requirements...
/Users/stacko/Code/go/bin/stringer
go generate $(go list ./... | grep -v /vendor/)
2016/06/28 02:04:48 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSAMILaunchPermission -timeout 120m
=== RUN   TestAccAWSAMILaunchPermission_Basic
--- FAIL: TestAccAWSAMILaunchPermission_Basic (0.00s)
panic: interface conversion: interface {} is nil, not *aws.AWSClient [recovered]
    panic: interface conversion: interface {} is nil, not *aws.AWSClient

goroutine 6 [running]:
panic(0x13fa6c0, 0xc8200f2f00)
    /opt/boxen/homebrew/Cellar/go/1.6.2/libexec/src/runtime/panic.go:481 +0x3e6
testing.tRunner.func1(0xc820084fc0)
    /opt/boxen/homebrew/Cellar/go/1.6.2/libexec/src/testing/testing.go:467 +0x192
panic(0x13fa6c0, 0xc8200f2f00)
    /opt/boxen/homebrew/Cellar/go/1.6.2/libexec/src/runtime/panic.go:443 +0x4e9
github.com/hashicorp/terraform/builtin/providers/aws.TestAccAWSAMILaunchPermission_Basic.func1()
    /Users/stacko/Code/go/src/github.com/hashicorp/terraform/builtin/providers/aws/resource_aws_ami_launch_permission_test.go:21 +0x262
github.com/hashicorp/terraform/helper/resource.Test(0x2b18f18, 0xc820084fc0, 0x0, 0xc820144880, 0xc8202430b0, 0x0, 0x0, 0xc82019f3a0, 0xc820274000, 0x1, ...)
    /Users/stacko/Code/go/src/github.com/hashicorp/terraform/helper/resource/testing.go:216 +0x6aa
github.com/hashicorp/terraform/builtin/providers/aws.TestAccAWSAMILaunchPermission_Basic(0xc820084fc0)
    /Users/stacko/Code/go/src/github.com/hashicorp/terraform/builtin/providers/aws/resource_aws_ami_launch_permission_test.go:39 +0x35c
testing.tRunner(0xc820084fc0, 0x2446128)
    /opt/boxen/homebrew/Cellar/go/1.6.2/libexec/src/testing/testing.go:473 +0x98
created by testing.RunTests
    /opt/boxen/homebrew/Cellar/go/1.6.2/libexec/src/testing/testing.go:582 +0x892
exit status 2
FAIL    github.com/hashicorp/terraform/builtin/providers/aws    0.020s
make: *** [testacc] Error 1

BSick7 · 2016-06-28T12:53:43Z

@stack72 That's odd that the travis build succeeded, but yours didn't
Looking into fix now.

stack72 · 2016-06-28T12:59:05Z

@BSick7 the travis build doesn't run the acceptance tests :) The cmd i ran does. So there may be someone missed

P.

BSick7 · 2016-06-28T12:59:55Z

Ahh right on. Thanks! Fixing.

BSick7 · 2016-06-28T13:19:24Z

@stack72 I found the issue. I thought I could rely on the aws provider being "ready" to issue aws calls in my test. The only reason I am doing this is to use the account id from the current aws caller identity to verify this resource. Any thoughts on other ways to get the account id?

stack72 · 2016-06-28T14:02:21Z

Hey @BSick7

Tests still fail for me I'm afraid

make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSAMILaunchPermission'                                  1 ↵
==> Checking that code complies with gofmt requirements...
/Users/stacko/Code/go/bin/stringer
go generate $(go list ./... | grep -v /vendor/)
2016/06/28 14:58:03 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSAMILaunchPermission -timeout 120m
=== RUN   TestAccAWSAMILaunchPermission_Basic
--- FAIL: TestAccAWSAMILaunchPermission_Basic (157.96s)
    testing.go:255: Step 1 error: Error applying: 1 error(s) occurred:

        * aws_ami_launch_permission.self-test: error creating ami launch permission: InvalidAMIAttributeItemValue: Invalid attribute item value " " for userId item type.
            status code: 400, request id: d026f5eb-939a-45ec-8b0c-75bda1177e32
    testing.go:318: Error destroying resource! WARNING: Dangling resources
        may exist. The full state and error is shown below.

        Error: Check failed: MissingParameter: The request must contain the parameter ImageId
            status code: 400, request id: 3afbc378-b56d-40cd-9ae8-13cc1b7dfb83

        State: <no state>
FAIL
exit status 1
FAIL    github.com/hashicorp/terraform/builtin/providers/aws    157.988s

Is there something missing here?

P.

BSick7 · 2016-06-28T14:12:39Z

@stack72 Ya, I still haven't fixed it. The resource relies on an "account_id". I suppose I could use the env vars to retrieve the account id without the use of a provider.

stack72 · 2016-06-28T15:02:54Z

@BSick7 some of the other code we have already has an Env Var for Account Id. Therefore, there is a chance that we could effectively add our own AccountID to this resource and share the AMI with ourselves :)

BSick7 · 2016-06-28T15:12:30Z

@stack72 I was looking for that! Can you point me to that account id?

stack72 · 2016-06-28T15:14:46Z

@BSick7 look for resource_aws_vpc_peering_connection.go

BSick7 · 2016-06-28T17:46:29Z

@stack72 Finally got my tests running the way I intended. It's annoying when it takes almost 5-10 minutes to run the test.

BSick7 · 2016-06-28T17:47:01Z

TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSAMILaunchPermission -timeout 120m
=== RUN   TestAccAWSAMILaunchPermission_Basic
--- PASS: TestAccAWSAMILaunchPermission_Basic (355.01s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    355.019s

stack72 · 2016-06-29T07:41:46Z

Hi @BSick7

So I am slightly confused here, in the test, you are doing the following:

account_id := os.Getenv("AWS_ACCOUNT_ID")

AWS_ACCOUNT_ID is being used already in Terraform to denote the account you are running terraform against. This test therefore seems to suggest sharing the AMI with the same account it was created with

Thoughts?

Paul

BSick7 · 2016-06-29T12:02:36Z

@stack72 The acceptance test is really just ensuring that communication with aws is configured properly. I believe a better acceptance test would share with another account and build an instance using that account; however, I didn't think terraform acceptance tests were in a position to do that.

stack72 · 2016-06-29T12:16:01Z

builtin/providers/aws/resource_aws_ami_launch_permission_test.go

+
+func testAccAWSAMILaunchPermissionConfig(account_id string, includeLaunchPermission bool) string {
+	base := `
+provider "aws" {


we don't need to worry about specifying the provider block and region here :)

We only specify it if we need to run it against a particular env

Thanks! Dropping. I originally was using an AMI in us-east-1, but upon massive changes yesterday, forgot to remove that block.

steve-jansen · 2016-07-20T21:18:36Z

@stack72 do you think this PR will make the 0.7 release?

stack72 · 2016-07-21T22:08:26Z

Hi @BSick7

This is looking good now :)

% make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSAMILaunchPermission'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSAMILaunchPermission -timeout 120m
=== RUN   TestAccAWSAMILaunchPermission_Basic
--- PASS: TestAccAWSAMILaunchPermission_Basic (344.70s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    344.719s

* master: (34 commits) Update CHANGELOG.md provider/aws: Delete access keys before deleting IAM user (hashicorp#7766) Fix broken link to Consul demo (hashicorp#7789) provider/aws: `aws_redshift_cluster` `number_of_nodes` was having the (hashicorp#7771) provider/aws: Restore lost client.simpledbconn initialization Update vendored atlas client Make using `ssl_verify_mode` more robust (hashicorp#7769) Update CHANGELOG.md provider/aws: Rename the ECS Container Data Source test docs/azure: Small changes to remove the use of double Update docs to centralize on ARM-based Azure provider (hashicorp#7767) Update CHANGELOG.md Update CHANGELOG.md Add support for Kinesis streams shard-level metrics (hashicorp#7684) Update CHANGELOG.md Implementing aws_ami_launch_permission. (hashicorp#7365) Update CHANGELOG.md Add VersionString provider/aws: Set `storage_encrypted` to state in (hashicorp#7751) provider/fastly: Update go-fastly SDK (hashicorp#7747) ...

ghost · 2020-04-24T01:59:52Z

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.

BSick7 changed the title ~~provider/aws: Implementing aws_ami_launch_permission resource~~ [wip] provider/aws: Implementing aws_ami_launch_permission resource Jun 27, 2016

BSick7 force-pushed the aws-ami-permission branch 4 times, most recently from f1f4bd0 to 0b17c69 Compare June 27, 2016 15:13

stack72 added provider/aws enhancement labels Jun 27, 2016

BSick7 force-pushed the aws-ami-permission branch 5 times, most recently from 375b4a7 to 7837786 Compare June 27, 2016 20:10

BSick7 changed the title ~~[wip] provider/aws: Implementing aws_ami_launch_permission resource~~ provider/aws: Implementing aws_ami_launch_permission resource Jun 27, 2016

BSick7 force-pushed the aws-ami-permission branch from c78efa1 to 3009830 Compare June 28, 2016 13:28

BSick7 force-pushed the aws-ami-permission branch from 3009830 to ef77f92 Compare June 28, 2016 14:02

BSick7 force-pushed the aws-ami-permission branch from ef77f92 to 03547e5 Compare June 28, 2016 14:35

BSick7 force-pushed the aws-ami-permission branch 3 times, most recently from 2de31e7 to 7521f90 Compare June 28, 2016 17:45

stack72 added the waiting-response An issue/pull request is waiting for a response from the community label Jun 29, 2016

stack72 reviewed Jun 29, 2016
View reviewed changes

Implementing aws_ami_launch_permission.

390d3b9

BSick7 force-pushed the aws-ami-permission branch from 7521f90 to 390d3b9 Compare June 29, 2016 12:36

stack72 merged commit 732b8d3 into hashicorp:master Jul 21, 2016

steve-jansen mentioned this pull request Jul 27, 2016

Lookup of AMIs by region in atlas_artifact ? #4239

Closed

ghost locked and limited conversation to collaborators Apr 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

provider/aws: Implementing aws_ami_launch_permission resource #7365

provider/aws: Implementing aws_ami_launch_permission resource #7365

BSick7 commented Jun 27, 2016 •

edited

Loading

BSick7 commented Jun 27, 2016

stack72 commented Jun 28, 2016

BSick7 commented Jun 28, 2016

stack72 commented Jun 28, 2016

BSick7 commented Jun 28, 2016

BSick7 commented Jun 28, 2016

stack72 commented Jun 28, 2016

BSick7 commented Jun 28, 2016

stack72 commented Jun 28, 2016

BSick7 commented Jun 28, 2016

stack72 commented Jun 28, 2016

BSick7 commented Jun 28, 2016

BSick7 commented Jun 28, 2016

stack72 commented Jun 29, 2016

BSick7 commented Jun 29, 2016

stack72 Jun 29, 2016

BSick7 Jun 29, 2016

steve-jansen commented Jul 20, 2016

stack72 commented Jul 21, 2016

ghost commented Apr 24, 2020

provider/aws: Implementing aws_ami_launch_permission resource #7365

provider/aws: Implementing aws_ami_launch_permission resource #7365

Conversation

BSick7 commented Jun 27, 2016 • edited Loading

Overview

Syntax

Checklist

BSick7 commented Jun 27, 2016

stack72 commented Jun 28, 2016

BSick7 commented Jun 28, 2016

stack72 commented Jun 28, 2016

BSick7 commented Jun 28, 2016

BSick7 commented Jun 28, 2016

stack72 commented Jun 28, 2016

BSick7 commented Jun 28, 2016

stack72 commented Jun 28, 2016

BSick7 commented Jun 28, 2016

stack72 commented Jun 28, 2016

BSick7 commented Jun 28, 2016

BSick7 commented Jun 28, 2016

stack72 commented Jun 29, 2016

BSick7 commented Jun 29, 2016

stack72 Jun 29, 2016

Choose a reason for hiding this comment

BSick7 Jun 29, 2016

Choose a reason for hiding this comment

steve-jansen commented Jul 20, 2016

stack72 commented Jul 21, 2016

ghost commented Apr 24, 2020

BSick7 commented Jun 27, 2016 •

edited

Loading