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.

Sign up for GitHub

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

Jump to bottom

data-source/aws_ami and data-source/aws_ami_ids: Require owners argument #5576

Merged
merged 1 commit into from
Feb 22, 2019
Merged

data-source/aws_ami and data-source/aws_ami_ids: Require owners argument #5576

merged 1 commit into from
Feb 22, 2019

Conversation

bflad
Copy link
Contributor

@bflad bflad commented Aug 16, 2018

The lookup is insecure by default and there have been numerous security events relating to referencing unexpected AMIs. This implementation explicitly requires the root level owners argument to use Terraform's built-in schema validation, however concessions might be necessary to support the owner-alias and owner-id filters (back to leaving owners optional in the schema).

Reference: hashicorp/packer#6584

Changes proposed in this pull request:

  • data-source/aws_ami: Switch owners argument from Optional to Required
  • data-source/aws_ami_ids: Switch owners argument from Optional to Required
  • tests: Update aws_ami data sources to use owners instead of filter > name = "owner-alias"

Output from acceptance testing: (others will be handled via daily acceptance testing)

$ make testacc TEST=./aws TESTARGS='-run=TestAcc\(AWSAmiDataSource\|DataSourceAwsAmiIds\)_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAcc\(AWSAmiDataSource\|DataSourceAwsAmiIds\)_ -timeout 120m
=== RUN   TestAccDataSourceAwsAmiIds_basic
--- PASS: TestAccDataSourceAwsAmiIds_basic (11.55s)
=== RUN   TestAccDataSourceAwsAmiIds_sorted
--- PASS: TestAccDataSourceAwsAmiIds_sorted (251.95s)
=== RUN   TestAccAWSAmiDataSource_natInstance
--- PASS: TestAccAWSAmiDataSource_natInstance (11.88s)
=== RUN   TestAccAWSAmiDataSource_windowsInstance
--- PASS: TestAccAWSAmiDataSource_windowsInstance (22.03s)
=== RUN   TestAccAWSAmiDataSource_instanceStore
--- PASS: TestAccAWSAmiDataSource_instanceStore (9.35s)
=== RUN   TestAccAWSAmiDataSource_localNameFilter
--- PASS: TestAccAWSAmiDataSource_localNameFilter (16.45s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	323.842s

@bflad bflad added breaking-change Introduces a breaking change in current functionality; usually deferred to the next major release. proposal Proposes new design or functionality. service/ec2 Issues and PRs that pertain to the ec2 service. labels Aug 16, 2018
@bflad bflad added this to the v2.0.0 milestone Aug 16, 2018
@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Aug 16, 2018
This was referenced Aug 21, 2018
Merged
Merged
@bflad bflad force-pushed the s-aws_ami-require-owners branch from c74ad5a to 1b9cd62 Compare August 22, 2018 18:09
@ghost ghost added size/XL Managed by automation to categorize the size of a PR. and removed size/L Managed by automation to categorize the size of a PR. labels Aug 22, 2018
@bflad bflad removed the proposal Proposes new design or functionality. label Aug 22, 2018
@bflad
Copy link
Contributor Author

bflad commented Aug 22, 2018

This PR has been rebased and updated to remove documentation/warning logging introduced in 1.X by #5626 and #5653

This was referenced Sep 13, 2018
Merged
Merged
Merged
@bflad
data-source/aws_ami and data-source/aws_ami_ids: Require owners argument
493e23f 
* data-source/aws_ami: Switch owners argument from Optional to Required
* data-source/aws_ami_ids: Switch owners argument from Optional to Required
* tests: Update aws_ami data sources to use owners instead of filter > name = "owner-alias"

Output from acceptance testing:

```
--- PASS: TestAccAWSAmiDataSource_instanceStore (8.89s)
--- PASS: TestAccDataSourceAwsAmiIds_basic (9.72s)
--- PASS: TestAccAWSAmiDataSource_natInstance (9.72s)
--- PASS: TestAccAWSAmiDataSource_localNameFilter (12.65s)
--- PASS: TestAccDataSourceAwsAmiIds_sorted (14.02s)
--- PASS: TestAccAWSAmiDataSource_windowsInstance (16.91s)
```
@bflad bflad force-pushed the s-aws_ami-require-owners branch from 1b9cd62 to 493e23f Compare February 22, 2019 21:11
@ghost ghost added service/autoscaling Issues and PRs that pertain to the autoscaling service. service/storagegateway Issues and PRs that pertain to the storagegateway service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. documentation Introduces or discusses updates to documentation. labels Feb 22, 2019
@bflad
Copy link
Contributor Author

bflad commented Feb 22, 2019

This pull request has been rebased with master and passes acceptance testing:

--- PASS: TestAccAWSAmiDataSource_instanceStore (8.89s)
--- PASS: TestAccDataSourceAwsAmiIds_basic (9.72s)
--- PASS: TestAccAWSAmiDataSource_natInstance (9.72s)
--- PASS: TestAccAWSAmiDataSource_localNameFilter (12.65s)
--- PASS: TestAccDataSourceAwsAmiIds_sorted (14.02s)
--- PASS: TestAccAWSAmiDataSource_windowsInstance (16.91s)

Once TravisCI agrees, merging and adding CHANGELOG notes. The Version 2 Upgrade Guide has documented this change for a few months as well.

@bflad bflad merged commit 39d6664 into master Feb 22, 2019
@bflad bflad deleted the s-aws_ami-require-owners branch February 22, 2019 21:20
bflad added a commit that referenced this pull request Feb 22, 2019
@bflad
Update CHANGELOG for #5576
6b154ed
@garyellis garyellis mentioned this pull request Mar 7, 2019
Open
This was referenced Mar 8, 2019
Closed
Closed
kostyrev added a commit to kostyrev/elasticsearch-cloud-deploy that referenced this pull request Mar 28, 2019
@kostyrev
Add owners to data aws_ami
0931f23 
See hashicorp/terraform-provider-aws#5576 for more info
@ghost
Copy link

ghost commented Mar 31, 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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked and limited conversation to collaborators Mar 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@vancluever vancluever vancluever approved these changes

Assignees
No one assigned
Labels
breaking-change Introduces a breaking change in current functionality; usually deferred to the next major release. documentation Introduces or discusses updates to documentation. service/autoscaling Issues and PRs that pertain to the autoscaling service. service/ec2 Issues and PRs that pertain to the ec2 service. service/storagegateway Issues and PRs that pertain to the storagegateway service. size/XL Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Milestone
v2.0.0
Development

Successfully merging this pull request may close these issues.
2 participants
@bflad @vancluever