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

Cleanup AWS CloudFormation checks examples #118

Merged
merged 13 commits into from
May 16, 2024
Merged

Cleanup AWS CloudFormation checks examples #118

merged 13 commits into from
May 16, 2024

Conversation

StevenSmiley
Copy link
Contributor

This PR fixes a number of minor issues with the examples provided for AWS CloudFormation checks, including:

  • There were "good" examples with "bad" in the title, which was confusing
  • There were examples with titles that didn't match the resource type, like "Queue" when describing an RDS Instance
  • There was some odd placeholder text
  • There were some incomplete examples
  • Some examples included extraneous info that distracted from their purpose, like the CF template version

I've been using trivy lately and ran into a bunch of these, so I wanted to help fix them.

@StevenSmiley StevenSmiley requested a review from simar7 as a code owner April 22, 2024 22:44
@CLAassistant
Copy link

CLAassistant commented Apr 22, 2024

CLA assistant check
All committers have signed the CLA.

Comment on lines 14 to 21
AWSTemplateFormatVersion: 2010-09-09
Description: Bad example of redshift sgr
Resources:
Queue:
Type: AWS::Redshift::ClusterSecurityGroup
Properties:
Description: ""

Copy link
Member

Choose a reason for hiding this comment

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

was this intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. This check is related to resources using EC2-Classic, so the example provided is not relevant. Additionally, AWS has completely retired EC2-Classic as of August 23, 2023, so this check should probably be retired altogether.

Comment on lines 5 to 8
AWSTemplateFormatVersion: 2010-09-09
Description: Good example of redshift sgr
Resources:

Copy link
Member

Choose a reason for hiding this comment

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

was this intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. This check is related to resources using EC2-Classic, so the example provided is not relevant. Additionally, AWS has completely retired EC2-Classic as of August 23, 2023, so this check should probably be retired altogether.

Copy link
Member

Choose a reason for hiding this comment

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

I understand. Your point about deprecating the check makes sense but I do think we should do that in a separate PR once we have added support for deprecating a check and for each such deprecated check we can update the docs separately. I've opened this to track adding support for deprecation of checks.

For the time being I would bring back these examples and as I mentioned we can open a separate PR to update both check and its docs.

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I will restore these to their previous state.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @StevenSmiley - looks like the tests are red. If you could update them we can merge this PR in. Thanks for the patience.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated the tests

Copy link
Member

Choose a reason for hiding this comment

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

Still seem to be failing. You can run make test locally to run the tests.

Comment on lines 5 to 8
AWSTemplateFormatVersion: 2010-09-09
Description: Good example of rds sgr
Resources:

Copy link
Member

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. This check is related to resources using EC2-Classic, so the example provided is not relevant. Additionally, AWS has completely retired EC2-Classic as of August 23, 2023, so this check should probably be retired altogether.

Comment on lines 14 to 21
AWSTemplateFormatVersion: 2010-09-09
Description: Bad example of rds sgr
Resources:
Queue:
Type: AWS::RDS::DBSecurityGroup
Properties:
Description: ""

Copy link
Member

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. This check is related to resources using EC2-Classic, so the example provided is not relevant. Additionally, AWS has completely retired EC2-Classic as of August 23, 2023, so this check should probably be retired altogether.

Copy link
Member

@simar7 simar7 left a comment

Choose a reason for hiding this comment

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

thanks for the PR! I left some comments.

@StevenSmiley StevenSmiley requested a review from simar7 April 23, 2024 14:20
@StevenSmiley
Copy link
Contributor Author

I see that a test is failing during docs generation, can you provide guidance on how to resolve that? Am I supposed to copy each changed example into the docs files?

@nikpivkin
Copy link
Contributor

Hi @StevenSmiley !

Just run make docs and push the changes.

@StevenSmiley
Copy link
Contributor Author

Ah, easy. Done.

@simar7 simar7 merged commit 4b6f7cc into aquasecurity:main May 16, 2024
5 checks passed
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.

4 participants