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

Added a default limit = 1 value to aws_cloudwatch_log_group.rb #928

Merged

Conversation

so1omon563
Copy link
Contributor

@so1omon563 so1omon563 commented Jun 22, 2022

Description

The resource is using the describe_log_groups api call, which is actually doing a prefix lookup. This means that it can easily return multiple log groups that have the same prefix.

For example, if I have a log group called /my-environment/my-app that I want to check, but I also have a log group called /my-environment/my-app-testing, it is impossible to check the /my-environment/my-app log group because it returns multiple groups.

This change includes adding a defaulted limit: 1 value to the call. That will return only the first log group match, which in examples as shown above, will be the exact match.

It allows for an override if desired by simply adding a limit: value option to the resource.

I believe that since this is a singular resource, that this new behavior is more in line with expected results.

Issues Resolved

Resolves issue #885

Check List

Please fill box or appropriate ([x]) or mark N/A.

…at can be overridden. Updated associated documentation.

Signed-off-by: Jedidiah Foster <[email protected]>
@netlify
Copy link

netlify bot commented Jun 22, 2022

Deploy Preview for inspec-aws ready!

Name Link
🔨 Latest commit a1b06db
🔍 Latest deploy log https://app.netlify.com/sites/inspec-aws/deploys/6321825344f5990008fab4d1
😎 Deploy Preview https://deploy-preview-928--inspec-aws.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@so1omon563
Copy link
Contributor Author

Unsure if unit / integration tests need to be modified. I believe the existing tests for the resource will pass.

@so1omon563 so1omon563 marked this pull request as ready for review June 22, 2022 19:59
@so1omon563 so1omon563 requested a review from a team as a code owner June 22, 2022 19:59
@so1omon563
Copy link
Contributor Author

Any updates on this request?

@soumyo13
Copy link
Contributor

In a particular region, we cant create two log_groups with the same name.

I tried to replicate the issue. I created a cloud-log-group in region ‘us-east-2’ and created another cloud-log-group with the same name in the region 'us-west-2'.

Then I tried running the same in both the two regions.

Test Checked:

aws_cloud_watch_log_group_name = 'test1'

control 'aws-cloudwatch-log-group-1.0' do
  impact 1.0
  title 'Ensure AWS Cloudwatch Log Group has the correct properties.'
  
  describe aws_cloudwatch_log_group(log_group_name:  aws_cloud_watch_log_group_name) do
    it { should exist }
    its('retention_in_days') { should eq nil }
    its('kms_key_id') { should eq nil }
    its('tags') { should be_empty }
  end
end

I used the below command to execute the test in different regions:

This test I have executed for region 'us-east-2'

inspec exec . -t aws://us-east-2

And here is the result:
image

This test I have executed for region 'us-west-2'

inspec exec . -t aws://us-west-2

And here is the result:
image

@so1omon563
Copy link
Contributor Author

The issue isn't two log groups with the same name. It's two log groups that start with the same prefix. This information is in my description.

To replicate the issue, try creating a log group called test, and a log group called test1, both in the same region. Then run your check against only the test log group.

It will fail, because there are now two log groups with a prefix of test, and the check only works with one result, since it's a singular resource.

My submitted fix will prevent this from happening, as it would default to only looking at the first match.

Here is sample output of the output from running a basic should exist test against a test log group exactly as I have described above.

  RuntimeError:
    Found multiple CloudWatch Log Groups. The following matched: {:log_group_name=>"test", :creation_time=>1663082860460, :retention_in_days=>14, :metric_filter_count=>0, :arn=>"arn:aws:logs:us-east-1:508551232565:log-group:test:*", :stored_bytes=>0, :kms_key_id=>nil}, {:log_group_name=>"test1", :creation_time=>1663082968352, :retention_in_days=>14, :metric_filter_count=>0, :arn=>"arn:aws:logs:us-east-1:508551232565:log-group:test1:*", :stored_bytes=>0, :kms_key_id=>nil}.  Try to restrict your resource criteria.
  # ./vendor/bundle/ruby/2.7.0/gems/inspec-core-5.17.4/lib/inspec/rule.rb:61:in `block (2 levels) in initialize'
  # ./vendor/bundle/ruby/2.7.0/gems/inspec-core-5.17.4/lib/inspec/runner_rspec.rb:97:in `run'
  # ./vendor/bundle/ruby/2.7.0/gems/inspec-core-5.17.4/lib/inspec/runner.rb:183:in `run_tests'
  # ./vendor/bundle/ruby/2.7.0/gems/inspec-core-5.17.4/lib/inspec/runner.rb:154:in `run'
  # ./vendor/bundle/ruby/2.7.0/gems/inspec-core-5.17.4/lib/inspec/cli.rb:313:in `exec'
  # ./vendor/bundle/ruby/2.7.0/gems/thor-1.2.1/lib/thor/command.rb:27:in `run'
  # ./vendor/bundle/ruby/2.7.0/gems/thor-1.2.1/lib/thor/invocation.rb:127:in `invoke_command'
  # ./vendor/bundle/ruby/2.7.0/gems/thor-1.2.1/lib/thor.rb:392:in `dispatch'
  # ./vendor/bundle/ruby/2.7.0/gems/thor-1.2.1/lib/thor/base.rb:485:in `start'
  # ./vendor/bundle/ruby/2.7.0/gems/inspec-core-5.17.4/lib/inspec/base_cli.rb:35:in `start'
./vendor/bundle/ruby/2.7.0/gems/inspec-core-5.17.4/lib/inspec/rule.rb:61:in `block (2 levels) in initialize'
./vendor/bundle/ruby/2.7.0/gems/inspec-core-5.17.4/lib/inspec/runner_rspec.rb:97:in `run'
./vendor/bundle/ruby/2.7.0/gems/inspec-core-5.17.4/lib/inspec/runner.rb:183:in `run_tests'
./vendor/bundle/ruby/2.7.0/gems/inspec-core-5.17.4/lib/inspec/runner.rb:154:in `run'
./vendor/bundle/ruby/2.7.0/gems/inspec-core-5.17.4/lib/inspec/cli.rb:313:in `exec'
./vendor/bundle/ruby/2.7.0/gems/thor-1.2.1/lib/thor/command.rb:27:in `run'
./vendor/bundle/ruby/2.7.0/gems/thor-1.2.1/lib/thor/invocation.rb:127:in `invoke_command'
./vendor/bundle/ruby/2.7.0/gems/thor-1.2.1/lib/thor.rb:392:in `dispatch'
./vendor/bundle/ruby/2.7.0/gems/thor-1.2.1/lib/thor/base.rb:485:in `start'
./vendor/bundle/ruby/2.7.0/gems/inspec-core-5.17.4/lib/inspec/base_cli.rb:35:in `start'

@soumyo13
Copy link
Contributor

The issue isn't two log groups with the same name. It's two log groups that start with the same prefix. This information is in my description.

To replicate the issue, try creating a log group called test, and a log group called test1, both in the same region. Then run your check against only the test log group.

It will fail, because there are now two log groups with a prefix of test, and the check only works with one result, since it's a singular resource.

My submitted fix will prevent this from happening, as it would default to only looking at the first match.

Here is sample output of the output from running a basic should exist test against a test log group exactly as I have described above.

  RuntimeError:
    Found multiple CloudWatch Log Groups. The following matched: {:log_group_name=>"test", :creation_time=>1663082860460, :retention_in_days=>14, :metric_filter_count=>0, :arn=>"arn:aws:logs:us-east-1:508551232565:log-group:test:*", :stored_bytes=>0, :kms_key_id=>nil}, {:log_group_name=>"test1", :creation_time=>1663082968352, :retention_in_days=>14, :metric_filter_count=>0, :arn=>"arn:aws:logs:us-east-1:508551232565:log-group:test1:*", :stored_bytes=>0, :kms_key_id=>nil}.  Try to restrict your resource criteria.
  # ./vendor/bundle/ruby/2.7.0/gems/inspec-core-5.17.4/lib/inspec/rule.rb:61:in `block (2 levels) in initialize'
  # ./vendor/bundle/ruby/2.7.0/gems/inspec-core-5.17.4/lib/inspec/runner_rspec.rb:97:in `run'
  # ./vendor/bundle/ruby/2.7.0/gems/inspec-core-5.17.4/lib/inspec/runner.rb:183:in `run_tests'
  # ./vendor/bundle/ruby/2.7.0/gems/inspec-core-5.17.4/lib/inspec/runner.rb:154:in `run'
  # ./vendor/bundle/ruby/2.7.0/gems/inspec-core-5.17.4/lib/inspec/cli.rb:313:in `exec'
  # ./vendor/bundle/ruby/2.7.0/gems/thor-1.2.1/lib/thor/command.rb:27:in `run'
  # ./vendor/bundle/ruby/2.7.0/gems/thor-1.2.1/lib/thor/invocation.rb:127:in `invoke_command'
  # ./vendor/bundle/ruby/2.7.0/gems/thor-1.2.1/lib/thor.rb:392:in `dispatch'
  # ./vendor/bundle/ruby/2.7.0/gems/thor-1.2.1/lib/thor/base.rb:485:in `start'
  # ./vendor/bundle/ruby/2.7.0/gems/inspec-core-5.17.4/lib/inspec/base_cli.rb:35:in `start'
./vendor/bundle/ruby/2.7.0/gems/inspec-core-5.17.4/lib/inspec/rule.rb:61:in `block (2 levels) in initialize'
./vendor/bundle/ruby/2.7.0/gems/inspec-core-5.17.4/lib/inspec/runner_rspec.rb:97:in `run'
./vendor/bundle/ruby/2.7.0/gems/inspec-core-5.17.4/lib/inspec/runner.rb:183:in `run_tests'
./vendor/bundle/ruby/2.7.0/gems/inspec-core-5.17.4/lib/inspec/runner.rb:154:in `run'
./vendor/bundle/ruby/2.7.0/gems/inspec-core-5.17.4/lib/inspec/cli.rb:313:in `exec'
./vendor/bundle/ruby/2.7.0/gems/thor-1.2.1/lib/thor/command.rb:27:in `run'
./vendor/bundle/ruby/2.7.0/gems/thor-1.2.1/lib/thor/invocation.rb:127:in `invoke_command'
./vendor/bundle/ruby/2.7.0/gems/thor-1.2.1/lib/thor.rb:392:in `dispatch'
./vendor/bundle/ruby/2.7.0/gems/thor-1.2.1/lib/thor/base.rb:485:in `start'
./vendor/bundle/ruby/2.7.0/gems/inspec-core-5.17.4/lib/inspec/base_cli.rb:35:in `start'

Thanks got the problem.

Copy link
Contributor

@soumyo13 soumyo13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@soumyo13 soumyo13 merged commit 1b84c5e into inspec:main Sep 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants