Add plural resources #18

skpaterson · 2018-06-08T09:59:29Z

Includes resources required for upcoming blog post.

…zones; google_compute_firewalls as well as example controls and documentation. Also added helpers for retrieving compute instance labels. Signed-off-by: Stuart Paterson <spaterson@chef.io>

…Extra protection against null result in plural resources. Signed-off-by: Stuart Paterson <spaterson@chef.io>

miah

This looks good, can we remove those un-needed methods before merging?

miah · 2018-06-11T16:35:40Z

libraries/google_compute_firewalls.rb

+    filter_table_config.add_accessor(:where)
+    filter_table_config.add_accessor(:entries)
+    filter_table_config.add(:exist?) { |filter_table| !filter_table.entries.empty? }
+    filter_table_config.add(:count) { |filter_table| filter_table.entries.count }


With inspec/inspec#3104 we don't need to add where, entries, exist? or count properties to filter tables, they are now automatic.

Happy to do this but worth noting all profiles using inspec-gcp will now have to update the minimum InSpec version because of inspec/inspec#3066

miah · 2018-06-11T16:36:11Z

libraries/google_compute_zones.rb

+    filter_table_config.add_accessor(:where)
+    filter_table_config.add_accessor(:entries)
+    filter_table_config.add(:exist?) { |filter_table| !filter_table.entries.empty? }
+    filter_table_config.add(:count) { |filter_table| filter_table.entries.count }


clintoncwolfe

The FilterTable docs were in flight while this work was going on; now that it is available, I hope its users will have a better experience. So, mostly docs changes around that, and some testing coverage asks as well. Great work overall, really appreciate the effort!

clintoncwolfe · 2018-06-11T22:42:39Z

docs/resources/google_compute_firewalls.md.erb

+### Test that there are no more than a specified number of firewalls available for the project
+
+    describe google_compute_firewalls(project: 'chef-inspec-gcp') do
+      its('entries.count') { should be <= 100}


You can just use its('count') ... here.

sure, will update

clintoncwolfe · 2018-06-11T22:44:02Z

docs/resources/google_compute_firewalls.md.erb

+### Test the exact number of firewalls in the project
+
+    describe google_compute_firewalls(project: 'chef-inspec-gcp') do
+      its('firewall_ids.count') { should cmp 8 }


I'm not clear on the difference between this example and the previous one. If the resource matches 5 firewalls, are you not expected to have 5 firewall IDs?

agreed, will remove

clintoncwolfe · 2018-06-11T22:47:43Z

docs/resources/google_compute_firewalls.md.erb

+      its('firewall_names') { should_not include "default-allow-ssh" }
+    end
+
+### Test there are no firewalls for the "INGRESS" direction


Hrm, this is usually expressed like this:

describe google_compute_firewalls(project: 'chef-inspec-gcp').where(firewall_direction: 'INGRESS') do it { should_not exist } end

thanks, will update the example

clintoncwolfe · 2018-06-11T22:51:04Z

docs/resources/google_compute_firewalls.md.erb

+
+## Filter Criteria
+
+This resource currently does not support any filter criteria; it will always fetch all available firewalls.


Actually, you have firewall_id, firewall_name, and firewall_direction. You can use any of them with where, as a block or as a method.

Please read https://github.com/inspec/inspec/blob/master/docs/dev/filtertable-usage.md

thanks, likely another copy/paste oversight, will update accordingly

clintoncwolfe · 2018-06-11T22:51:48Z

docs/resources/google_compute_firewalls.md.erb

+
+## Properties
+
+*  `firewall_id`, `firewall_name`, `firewall_directions`


The properties are all the plural forms - so firewall_ids, firewall_names. Typical would be to explicitly document each, describing what they are (likely arrays of strings, but what do the Strings mean?)

clintoncwolfe · 2018-06-11T22:56:43Z

docs/resources/google_compute_zones.md.erb

+
+# google\_compute\_zones
+
+Use the `google_compute_zones` InSpec audit resource to test properties of all GCP compute zones for a project in a particular zone.


...all or a filtered group of ...

clintoncwolfe · 2018-06-11T22:58:30Z

docs/resources/google_compute_zones.md.erb

+
+### Test that there are no more than a specified number of zones available for the project
+
+    describe google_compute_zones(project: 'chef-inspec-gcp') do


So, similar comments as on firewalls. Use count instead of entries.count. You do actually have filter criteria. Your properties are plural.

clintoncwolfe · 2018-06-11T22:59:33Z

libraries/google_compute_firewalls.rb

+    filter_table_config.add(:firewall_ids, field: :firewall_id)
+    filter_table_config.add(:firewall_names, field: :firewall_name)
+    filter_table_config.add(:firewall_directions, field: :firewall_direction)
+    filter_table_config.add(:colors, field: :color, type: :simple)


Colors appears to be copypasta. Check your other plurals as well.

yup, removed

clintoncwolfe · 2018-06-11T23:00:09Z

libraries/google_compute_firewalls.rb

+    def fetch_data
+      firewall_rows = []
+      catch_gcp_errors do
+        @firewalls = @gcp.gcp_compute_client.list_firewalls(@project)


Do you need to paginate?

The API supports pagination [https://cloud.google.com/compute/docs/reference/rest/v1/firewalls/list] but the default is 500 so it's probably fine to skip that for now.

thanks - just saw all the comments, will add pagination as it shouldn't take too much

clintoncwolfe · 2018-06-11T23:03:20Z

test/integration/verify/controls/google_compute_firewalls.rb

+    it { should exist }
+    its('entries.count') { should be <= 100}
+    # assume this is a development setup for a moment
+    its('firewall_names') { should include "default-allow-ssh" }


You should also test firewall_ids - any property you expose. To obtain a value to check for, you can use the "output the ID in terraform, translate to YAML, read as an attribute" approach. AWS does this. Other methods also work.

added tests for the plural resource id parameters

…r tables as they are now automatic, also removing copy paste error “color” property. Signed-off-by: Stuart Paterson <spaterson@chef.io>

erjohnso

Thanks for taking this on @skpaterson! I recognize that this is early work, but some general comments:

I suspect a user will care about doing checks against FW rules to ensure protocols/ports/labels are present/absent. I'm new to InSpec, but stating intent such as 'ensure tcp:22 does not allow 0.0.0.0/0' would be very valuable for users.
I don't think there is any real utility for customers in checking kind (e.g. compute#zone). It's extra work to strip it out, so it's probably fine to leave as-is. However, I'd just change up the examples to not do any checks against it.

erjohnso · 2018-06-12T13:28:05Z

docs/resources/google_compute_zone.md.erb

+
+The following examples show how to use this InSpec audit resource.
+
+### Test that a GCP compute zone does not exist


Suggest flipping logic to test that a specified zone does exist.

sure can do - thanks

erjohnso · 2018-06-12T13:38:40Z

libraries/google_compute_firewalls.rb

+    def fetch_data
+      firewall_rows = []
+      catch_gcp_errors do
+        @firewalls = @gcp.gcp_compute_client.list_firewalls(@project)


The API supports pagination [https://cloud.google.com/compute/docs/reference/rest/v1/firewalls/list] but the default is 500 so it's probably fine to skip that for now.

…roperties across firewalls, zones and instances. * Updated tests to use count instead of entries.count. * Added an up? method to google_compute_zone and associated example. Also included in test. Signed-off-by: Stuart Paterson <spaterson@chef.io>

skpaterson · 2018-06-12T14:53:09Z

Thanks @erjohnso for your comments - created #27 to track the firewall rule improvements.

…s. Also, flip logic to test for existence of a zone in documented example. Signed-off-by: Stuart Paterson <spaterson@chef.io>

Add configuration flag for optionally executing tests that rely on gcloud/grep. Correct zones filter criteria and properties documentation. Signed-off-by: Stuart Paterson <spaterson@chef.io>

skpaterson · 2018-06-12T18:34:30Z

Thanks for the feedback, hopefully the comments should all now be addressed (famous last words...)

clintoncwolfe

Let's ship it :-) I still have some comments, but mainly aimed at getting it to be core-mergable.

Like core is perfect quality :-/

clintoncwolfe · 2018-06-13T18:40:59Z

docs/resources/google_compute_firewalls.md.erb

+
+This resource supports the following filter criteria:  `firewall_id`; `firewall_name`; and `firewall_direction`. Any of these may be used with `where`, as a block or as a method.
+
+## Properties


Let's go ahead and ship this, but typically we include one example per property. Users links straight to the property, then copy and paste the example.

clintoncwolfe · 2018-06-13T18:42:26Z

docs/resources/google_compute_instance.md.erb

 <br>

 ## Properties

-*  `cpu_platform`, `creation_timestamp`, `deletion_protection`, `disks`, `id`, `kind`, `label_fingerprint`, `machine_type`, `metadata`, `name`, `network_interfaces`, `scheduling`, `start_restricted`, `status`, `tags`, `zone`
+*  `cpu_platform`, `creation_timestamp`, `deletion_protection`, `disks`, `id`, `kind`, `label_fingerprint`, `machine_type`, `metadata`, `name`, `network_interfaces`, `scheduling`, `start_restricted`, `status`, `tags`, `zone`, `labels_keys`, `labels_values`


I'd be reluctant to add anything you want to remove later. It's very hard to deprecate once thing are in the wild. Since this isn't in core, let's ship it, but keep in mind if we have doubts about something, better to not expose it.

clintoncwolfe · 2018-06-13T18:42:50Z

docs/resources/google_compute_instance.md.erb

 <br>

 ## Properties

-*  `cpu_platform`, `creation_timestamp`, `deletion_protection`, `disks`, `id`, `kind`, `label_fingerprint`, `machine_type`, `metadata`, `name`, `network_interfaces`, `scheduling`, `start_restricted`, `status`, `tags`, `zone`
+*  `cpu_platform`, `creation_timestamp`, `deletion_protection`, `disks`, `id`, `kind`, `label_fingerprint`, `machine_type`, `metadata`, `name`, `network_interfaces`, `scheduling`, `start_restricted`, `status`, `tags`, `zone`, `labels_keys`, `labels_values`


See https://github.com/inspec/inspec/issues/3139

clintoncwolfe · 2018-06-13T18:43:44Z

docs/resources/google_compute_instances.md.erb

@@ -57,11 +57,12 @@ The following examples show how to use this InSpec audit resource.

 ## Filter Criteria

-This resource currently does not support any filter criteria; it will always fetch all instances in the zone.
+This resource supports the following filter criteria:  `instance_id` and `instance_name`. Either of these may be used with `where`, as a block or as a method.


If you need it for a blog post, let's ship it, but please make a tech debt issue - people fiind where very confusing, and need examples.

clintoncwolfe · 2018-06-13T18:45:17Z

libraries/google_compute_firewalls.rb

+    filter_table_config = FilterTable.create
+    filter_table_config.add(:firewall_ids, field: :firewall_id)
+    filter_table_config.add(:firewall_names, field: :firewall_name)
+    filter_table_config.add(:firewall_directions, field: :firewall_direction)


This is good as-is, but you might consider adding style: :simple - having this de-duplicated might be useful (or at least having individual entries for each rule isn't very useful).

Stuart Paterson added 2 commits June 7, 2018 14:58

Created the following resources: google_compute_zone; google_compute_…

c0ef3c9

…zones; google_compute_firewalls as well as example controls and documentation. Also added helpers for retrieving compute instance labels. Signed-off-by: Stuart Paterson <spaterson@chef.io>

New example control to check for label keys across VMs in all zones. …

19f23cd

…Extra protection against null result in plural resources. Signed-off-by: Stuart Paterson <spaterson@chef.io>

skpaterson requested a review from chris-rock June 8, 2018 09:59

chris-rock requested review from clintoncwolfe, miah, jquick and jerryaldrichiii June 8, 2018 10:01

skpaterson mentioned this pull request Jun 11, 2018

Enhancement: Support Fuzzy Match with Instance Group Name #23

Closed

miah suggested changes Jun 11, 2018

View reviewed changes

clintoncwolfe requested changes Jun 11, 2018

View reviewed changes

Removing “where”, “entries”, “exist?” and “count” properties to filte…

afb560d

…r tables as they are now automatic, also removing copy paste error “color” property. Signed-off-by: Stuart Paterson <spaterson@chef.io>

erjohnso reviewed Jun 12, 2018

View reviewed changes

Stuart Paterson added 2 commits June 12, 2018 16:31

Add pagination for retrieving firewalls and instances plural resource…

07013b1

…s. Also, flip logic to test for existence of a zone in documented example. Signed-off-by: Stuart Paterson <spaterson@chef.io>

Ensure “ids” parameters for plural resource are tested.

30e03bf

Add configuration flag for optionally executing tests that rely on gcloud/grep. Correct zones filter criteria and properties documentation. Signed-off-by: Stuart Paterson <spaterson@chef.io>

skpaterson mentioned this pull request Jun 12, 2018

found the documentation #17

Closed

miah approved these changes Jun 13, 2018

View reviewed changes

clintoncwolfe approved these changes Jun 13, 2018

View reviewed changes

davymcaleer merged commit 66b06a7 into master Jun 14, 2018

skpaterson deleted the add-plural-resources branch June 19, 2018 16:28


		## Filter Criteria

		This resource currently does not support any filter criteria; it will always fetch all available firewalls.


		## Properties

		* `firewall_id`, `firewall_name`, `firewall_directions`


		# google\_compute\_zones

		Use the `google_compute_zones` InSpec audit resource to test properties of all GCP compute zones for a project in a particular zone.


		### Test that there are no more than a specified number of zones available for the project

		describe google_compute_zones(project: 'chef-inspec-gcp') do


		The following examples show how to use this InSpec audit resource.

		### Test that a GCP compute zone does not exist


		This resource supports the following filter criteria: `firewall_id`; `firewall_name`; and `firewall_direction`. Any of these may be used with `where`, as a block or as a method.

		## Properties

Add plural resources #18

Add plural resources #18

Conversation

skpaterson commented Jun 8, 2018

miah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clintoncwolfe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skpaterson Jun 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erjohnso left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skpaterson commented Jun 12, 2018

skpaterson commented Jun 12, 2018

clintoncwolfe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skpaterson Jun 12, 2018 •

edited

Loading