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

feat(autoscaling): add new check autoscaling_group_multiple_instance_types #5325

Conversation

danibarranqueroo
Copy link
Member

@danibarranqueroo danibarranqueroo commented Oct 8, 2024

Context

This new check ensures that Amazon EC2 Auto Scaling groups use multiple instance types and deploy instances across multiple Availability Zones. Using multiple instance types enhances availability by allowing the Auto Scaling group to launch alternative instance types if there is insufficient capacity in the chosen Availability Zones, improving resilience and high availability.

I have done the check following this guide, but I have some concerns. While testing the check on real infrastructure, I realized that there are two ways to add instance types: either by adding a new instance attached to the Auto Scaling group or by adding a new override to the Launch Template to include new supported instance types.

Also, for the unit tests I needed to use botocore because moto does not cover the attach_instance api call.

Description

Added new check autoscaling_group_multiple_instance_types with its unit tests.

Checklist

License

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@danibarranqueroo danibarranqueroo requested review from a team as code owners October 8, 2024 09:37
@github-actions github-actions bot added the provider/aws Issues/PRs related with the AWS provider label Oct 8, 2024
Copy link

codecov bot commented Oct 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.24%. Comparing base (bc1e6c0) to head (5a7636e).
Report is 8 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5325      +/-   ##
==========================================
+ Coverage   89.18%   89.24%   +0.05%     
==========================================
  Files        1037     1044       +7     
  Lines       32004    32225     +221     
==========================================
+ Hits        28544    28760     +216     
- Misses       3460     3465       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -0,0 +1,32 @@
{
"Provider": "aws",
"CheckID": "autoscaling_group_multiple_instances_types_in_multiaz",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"CheckID": "autoscaling_group_multiple_instances_types_in_multiaz",
"CheckID": "autoscaling_group_multiple_instance_types",

Copy link
Member

Choose a reason for hiding this comment

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

Can we check if an AZ has more than one instance types?
For example, if there are 2 AZs and in one there are multiple instance types but in the other one only one instance type, it should fail.

@danibarranqueroo danibarranqueroo added the no-merge Please, DO NOT MERGE this PR. label Oct 14, 2024
@danibarranqueroo danibarranqueroo changed the title feat(autoscaling): add new check autoscaling_group_multiple_instances_types_in_multiaz feat(autoscaling): add new check autoscaling_group_multiple_instances_types Oct 15, 2024
@danibarranqueroo danibarranqueroo removed the no-merge Please, DO NOT MERGE this PR. label Oct 15, 2024
@danibarranqueroo
Copy link
Member Author

Hi @sergargar!

These past few days, I’ve been working on other checks from this same service, and since they’re all merged now, I’ve finally made the changes you suggested. To do this, I needed to merge those checks into this branch and then start working on how to verify if each AZ has multiple instances. I decided to store each instance's AZ in a dictionary, using the AZ name as the key and the instance type as the value. During the check, I verify if each AZ has 2 or more values.

Since Moto still doesn’t support the attach_instances API call, I’ve kept the old describe_autoscaling_groups test, which covers the other Auto Scaling checks, and created a new test specifically for this Auto Scaling group with attached instances.

Hope this message helps you understand what I’ve done more quickly 🚀😄

@MrCloudSec MrCloudSec changed the title feat(autoscaling): add new check autoscaling_group_multiple_instances_types feat(autoscaling): add new check autoscaling_group_multiple_instance_types Oct 15, 2024
@MrCloudSec MrCloudSec self-requested a review October 15, 2024 18:47
@MrCloudSec MrCloudSec merged commit 45c32ab into master Oct 15, 2024
11 checks passed
@MrCloudSec MrCloudSec deleted the PRWLR-4447-ensure-auto-scaling-groups-use-multiple-instance-types-in-multiple-availability-zones branch October 15, 2024 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
provider/aws Issues/PRs related with the AWS provider
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants