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

🌐 Print "Empty" Route53 Hosted Zones #49

Merged
merged 3 commits into from
Jun 27, 2024
Merged

Conversation

jasonBirchall
Copy link
Contributor

@jasonBirchall jasonBirchall commented Jun 26, 2024

👀 Purpose

  • This PR will add a new procedural script that reports on Hosted Zones with ONLY an SOA and NS record.
  • It is understood that at this point, either the zone is incorrectly configured or should be considered for removal.
  • I don't want to add an alarm just yet, but this would be useful as a script that can be triggered by anyone in the organisation.

♻️ What's changed

  • A new script that checks R53 for hosted zones with only an SOA and NS record.
  • A makefile command with a help entry for quick and easy execution.
  • A test file that ensures the script performs as expected.
  • A GitHub Action that will trigger the script with the required IAM permissions.

📝 Notes

  • This PR also contains a small formatting change to the makefile's help argument. It simply sorts the entries for improved readability.

Copy link

github-actions bot commented Jun 26, 2024

🦙 MegaLinter status: ✅ SUCCESS

Descriptor Linter Files Fixed Errors Elapsed time
✅ YAML prettier 1 0 0 0.25s
✅ YAML yamllint 1 0 0.18s

See detailed report in MegaLinter reports
Set VALIDATE_ALL_CODEBASE: true in mega-linter.yml to validate all sources, not only the diff

MegaLinter is graciously provided by OX Security

This will allows one and all to run the action using the click of a button.
@jasonBirchall jasonBirchall changed the title check empty zones 🌐 Print "Empty" Route53 Hosted Zones Jun 26, 2024
@jasonBirchall jasonBirchall marked this pull request as ready for review June 26, 2024 19:35
@jasonBirchall jasonBirchall requested a review from a team as a code owner June 26, 2024 19:35
print("The following hosted zones are empty:")
for zone in empty_zones:
print(f" - {zone}")
sys.exit(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think we should exit with an error here? 🤔 The program has been completed successfully at this point and printed empty hosted zones ✅

Seeing the CI/CD go red may lead to some wild goose chases for a problem that doesn't exist 🪿

Suggested change
sys.exit(1)

sys.exit(1)
else:
print("No empty hosted zones found.")
sys.exit(0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Think we can remove this since it's implied there were no errors if we don't raise any errors? 👀

Suggested change
sys.exit(0)

Comment on lines +6 to +7
def get_aws_zones():
route53 = boto3.client("route53")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd recommend instantiating the Route53Client at the top level and passing it through to these functions 🤝

Mainly because it shows at a high level what the external dependencies the script requires - and also what dependencies each functions require 🥂

Can imagine reading this function get_aws_zones() and thinking it could be reading from an S3Bucket, using a local variable or reading from a local file - but get_aws_zones(route53Client) is a bit clearer where the information is coming from 👀

Also makes testing a bit easier 🧪

Comment on lines +7 to +10
@pytest.fixture
def mock_boto3_client():
with patch('boto3.client') as mock_client:
yield mock_client
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels like a good method to use for testing at scale 🚀

I believe sticking with the explicit patches on each test is more readable in this case 👀 But maybe I'm stuck in my ways 😅

Copy link
Contributor

@connormaglynn connormaglynn left a comment

Choose a reason for hiding this comment

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

🚀 Looks Good To Me! - just a few comments 💬

@jasonBirchall jasonBirchall merged commit 13a83f6 into main Jun 27, 2024
2 checks passed
@jasonBirchall jasonBirchall deleted the check-empty-zones branch June 27, 2024 15:37
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