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

Add support for tags to aws_vpcs #3672

Closed
wants to merge 1 commit into from

Conversation

rikturnbull
Copy link

Adds tag property to aws_vpcs to allow tag based queries:

  # check 'my-vpc' exists and is unique
  describe aws_vpcs.where{ !tags.nil? and tags.find{|h| h[:key] == 'Name'}[:value] == 'my-vpc'} do
    its('count') { should cmp 1 }
  end

Signed-off-by: Rik Turnbull <37418542+rikturnbull@users.noreply.github.com>
@clintoncwolfe
Copy link
Contributor

I'd like to hold off on this until we decide how to handle tags on plural resources. What you have is practical - and if you renamed the field to be something like 'raw_tags', I think we might accept it. We're wanting to reserve the method 'tags' for something that does something more sophisticated, hoping to avoid the awkward search within an array of hashes (which you include in your example, and we think would be a common use case).

See #3139 and discuss there if you have ideas.

@rikturnbull
Copy link
Author

The actual problem I'm trying to solve is a lookup of a single resource using a name (which is often a tag). I'm currently writing AWS tests against a fork and building up a worrying number which will need migrating in the future. That fork supports the following:

describe aws_subnet(subnet_name: 'my-subnet') do
  it { should exist }
end

describe aws_route_table(route_table_name: 'my-route-table') do
  it { should exist }
end

describe aws_vpc(vpc_name: 'my-vpc') do
  it { should exist }
end

This PR for the plural aws_vpcs resource was to create a pattern (to dispense with the fork) so that I can do a find against the plural resource and then examine the richer singular resource (second attempt at #3630).

Unfortunately, as you suggest this results in the rather inelegant:

# must first check 'my-vpc' exists and is unique
describe aws_vpcs.where{ !tags.nil? and tags.find{|h| h[:key] == "Name"}[:value] == "my-vpc"} do
  its('count') { should cmp 1 }
end

# now can check 'my-vpc' attributes
aws_vpcs.where{ !tags.nil? and tags.find{|h| h[:key] == "Name"}[:value] == "my-vpc"}.vpc_ids.each do |vpc_id|
  describe aws_vpc(vpc_id) do
    its ('state') { should eq 'available' }
  end
end

I can see a number of open discussions that are relevant here (in particular as this naming lookup seems to be a pattern used by awspec):

https://github.com/inspec/inspec/issues/3139
https://github.com/inspec/inspec/issues/3641
#3621

I'm not entirely sure how to move forward. Perhaps the pattern should indeed be as I've described:

describe aws_RESOURCE(RESOURCE_name: 'my-label') do
  it { should exist }
end

and also just:

describe aws_RESOURCE('my-label') do
  it { should exist }
end

I've resubmitted #3630 with unit tests.

@clintoncwolfe
Copy link
Contributor

I think we should keep in mind the goal of this change - to allow searching by VPC name. That's easy to approve and agree to. The fact that a VPC name comes from a tag is an implementation detail. So long as you don't expose the tag functionality, this PR stays out of the impasse of "how to query tags".

So, I would recommend #3630 be a simplified PR to add name-querying support to the singular aws_vpc resource (without exposing tags for now), and this PR to do the same for the plural resource.

@tbugfinder
Copy link

I think we should keep in mind the goal of this change - to allow searching by VPC name. That's easy to approve and agree to. The fact that a VPC name comes from a tag is an implementation detail. So long as you don't expose the tag functionality, this PR stays out of the impasse of "how to query tags".

So, I would recommend #3630 be a simplified PR to add name-querying support to the singular aws_vpc resource (without exposing tags for now), and this PR to do the same for the plural resource.

@clintoncwolfe I think tag:Name is just a single specific tag. I would prefer having access to all tags not just a specific one.

@ghost
Copy link

ghost commented Feb 26, 2019

This is being added via inspec/inspec-aws#25 which is currently WIP.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Platform: AWS Amazon Web Services-related issues Type: New Feature Adds new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants