Make names for AWS Config service objects optional #2928

clintoncwolfe · 2018-04-09T17:40:41Z

As described on #2900, we were unnecessarily requiring an AWS resource name for the two Config Service inspec resources, aws_config_recoorder and aws_config_delivery_resource. If you don't provide a name in the query, the API gives you all config resources, and since they are singletons, it's easy to pick the right one.

Also found we had no unit tests for aws_config_delivery_resource; added them.

Also some general code cleanup in both affected resources.

Fixes #2900

jquick

Looks good! Thanks.

TrevorBramble

Loads of great additions and improvements. I have some suggestions to offer but I think only the typo is a blocker. ;^)

TrevorBramble · 2018-04-12T16:41:41Z

docs/resources/aws_config_delivery_channel.md.erb

@@ -23,57 +27,70 @@ An `aws_config_delivery_channel` resource block declares the tests for a single
      it { should exist }
    end

+However, since you may only have one DDelivery Channel per region, and InSpec connections are per-region, you may also omit the `channel_name` to obtain the one Delivery Channel (if any) that exists:


TrevorBramble · 2018-04-12T16:55:42Z

lib/resources/aws/aws_config_delivery_channel.rb

-    catch_aws_errors do
-      @resp = backend.describe_delivery_channels(query)
+    query = @channel_name ? { delivery_channel_names: [@channel_name] } : {}
+    response = {}


Does the assignment on line 39 sometimes return nil? If so, maybe just response = backend.describe_delivery_channels(query) || {} instead?

No, it never returns a {}.

inspec> c = inspec.backend.aws_client(Aws::ConfigService::Client) => #<Aws::ConfigService::Client> inspec> c.describe_delivery_channels => #<struct Aws::ConfigService::Types::DescribeDeliveryChannelsResponse delivery_channels=[]> inspec> c.describe_delivery_channels({}) => #<struct Aws::ConfigService::Types::DescribeDeliveryChannelsResponse delivery_channels=[]> inspec> c.describe_delivery_channels(delivery_channel_names:['nonesuch']) Aws::ConfigService::Errors::NoSuchDeliveryChannelException: Cannot find delivery channel with specified name 'nonesuch'. from /Users/cwolfe/.rbenv/versions/2.4.2/lib/ruby/gems/2.4.0/gems/aws-sdk-core-2.11.21/lib/seahorse/client/plugins/raise_response_errors.rb:15:in `call' inspec> c.describe_delivery_channels.delivery_channels => [] inspec> c.describe_delivery_channels.delivery_channels.empty? => true

Thanks for mentioning it, though - that means this is wrong:

@exists = !response.empty?

Should be:

@exists = !response.delivery_channels.empty?

TrevorBramble · 2018-04-12T17:01:52Z

lib/resources/aws/aws_config_delivery_channel.rb

+    response = {}
+    begin
+      response = backend.describe_delivery_channels(query)
+    rescue Aws::ConfigService::Errors::NoSuchDeliveryChannelException


Dropping this rescue to the bottom of the method would clear up the "happy path" for readers and removes the need to early return when handling the exception.

def fetch_from_api # ... rescue Aws::ConfigService::Errors::NoSuchDeliveryChannelException @exists = false end

I've moved the rescue to be method-level, as you asked. I generally dislike doing that, however. By moving the rescue away from the "dangerous" code, it obscures which line(s) are potentially dangerous. While this code only has one such dangerous expression, other methods may need to call multiple APIs that may fail.

There is an argument there for shorter, more granular methods.

TrevorBramble · 2018-04-12T17:07:57Z

lib/resources/aws/aws_config_delivery_channel.rb

+    @s3_bucket_name = channel[:s3_bucket_name]
+    @s3_key_prefix = channel[:s3_key_prefix]
+    @sns_topic_arn = channel[:sns_topic_arn]
+    @delivery_frequency_in_hours = channel[:config_snapshot_delivery_properties][:delivery_frequency] unless channel[:config_snapshot_delivery_properties].nil?


You can remove the conditional clause with dig. =^)

@delivery_frequency_in_hours = channel.dig(:config_snapshot_delivery_properties, :delivery_frequency)

TrevorBramble · 2018-04-12T17:12:39Z

lib/resources/aws/aws_config_recorder.rb

+    rescue Aws::ConfigService::Errors::NoSuchConfigurationRecorderException
+      @exists = false
+      return
+    end


Same as the other fetch_from_api, re: rescue at method level instead of introducing a begin block.

TrevorBramble · 2018-04-12T17:17:25Z

test/unit/resources/aws_config_delivery_channel_test.rb

+    def describe_delivery_channels(query = {})
+      fixtures = {
+        'default' => Aws::ConfigService::Types::DescribeDeliveryChannelsResponse.new(
+          :delivery_channels => [


Sometimes I see {:foo => :bar} and sometimes I see {foo: :bar}, particularly in the Rubocop-ignored test code. Shouldn't we always use JSON style for symbol keys?

If it ignored by rubocop, that means we as a team have decided it is code that is not worthwhile to enforce style conventions. We can open that for discussion if you'd like.

TrevorBramble · 2018-04-12T17:18:15Z

test/unit/resources/aws_config_delivery_channel_test.rb

+      }
+      return fixtures['default'] if query.empty?
+      return fixtures[query[:delivery_channel_names][0]] unless fixtures[query[:delivery_channel_names][0]].nil?
+      raise Aws::ConfigService::Errors::NoSuchDeliveryChannelException.new(nil, nil)


These look like three cases to me. ;^)

Signed-off-by: Clinton Wolfe <[email protected]>

…eanup Signed-off-by: Clinton Wolfe <[email protected]>

…oking for empty results Signed-off-by: Clinton Wolfe <[email protected]>

TrevorBramble

Thanks for considering my feedback!

clintoncwolfe requested a review from a team as a code owner April 9, 2018 17:40

adamhjk added the in progress label Apr 9, 2018

clintoncwolfe added Type: Enhancement Improves an existing feature Platform: AWS Amazon Web Services-related issues and removed in progress labels Apr 9, 2018

clintoncwolfe force-pushed the cw/aws-config-name-by-default branch from 319948b to 64d5dff Compare April 9, 2018 19:00

jquick approved these changes Apr 12, 2018

View reviewed changes

TrevorBramble reviewed Apr 12, 2018

View reviewed changes

clintoncwolfe added 4 commits April 17, 2018 13:23

Update tests and docs to assume one recorder per region

9cccf09

Signed-off-by: Clinton Wolfe <[email protected]>

Config recorder supports singleton fetch

5cdeab3

Signed-off-by: Clinton Wolfe <[email protected]>

Docs and tests for singleton mode delivery_channel

4d5d724

Signed-off-by: Clinton Wolfe <[email protected]>

Implementation for singleton delivery channel, and some other code cl…

73372cc

…eanup Signed-off-by: Clinton Wolfe <[email protected]>

clintoncwolfe force-pushed the cw/aws-config-name-by-default branch from 64d5dff to 49a5031 Compare April 17, 2018 20:22

Implement some feedback, and fix a bug in traversing the struct in lo…

6f4c356

…oking for empty results Signed-off-by: Clinton Wolfe <[email protected]>

clintoncwolfe force-pushed the cw/aws-config-name-by-default branch from 49a5031 to 6f4c356 Compare April 17, 2018 20:23

TrevorBramble approved these changes Apr 19, 2018

View reviewed changes

jquick merged commit 8934352 into master Apr 19, 2018

jquick deleted the cw/aws-config-name-by-default branch April 19, 2018 17:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make names for AWS Config service objects optional #2928

Make names for AWS Config service objects optional #2928

clintoncwolfe commented Apr 9, 2018

jquick left a comment

TrevorBramble left a comment

TrevorBramble Apr 12, 2018

TrevorBramble Apr 12, 2018

clintoncwolfe Apr 17, 2018

TrevorBramble Apr 12, 2018

clintoncwolfe Apr 17, 2018

TrevorBramble Apr 12, 2018

TrevorBramble Apr 12, 2018

TrevorBramble Apr 12, 2018

clintoncwolfe Apr 17, 2018

TrevorBramble Apr 12, 2018

TrevorBramble left a comment

Make names for AWS Config service objects optional #2928

Make names for AWS Config service objects optional #2928

Conversation

clintoncwolfe commented Apr 9, 2018

jquick left a comment

Choose a reason for hiding this comment

TrevorBramble 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

TrevorBramble left a comment

Choose a reason for hiding this comment