-
Notifications
You must be signed in to change notification settings - Fork 6
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
PR: Feature to add elb scan and filter security groups for elbs #18
base: master
Are you sure you want to change the base?
PR: Feature to add elb scan and filter security groups for elbs #18
Conversation
…is attached to elb
def get_all_elbs(region=None): | ||
if not region: | ||
region = config.REGIONS | ||
data = "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please initialize the variable with the correct variable type. This should probably be data = []
data = "" | ||
for region in config.REGIONS: | ||
client = boto3.client("elb", region_name=region) | ||
data = client.describe_load_balancers() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You overwrite data
in every iteration of the loop. This way you will always only get the ELBs from the last region in the list. You need to append the ELBs from a region onto the list and return the final list in the end
flags.append(crayons.yellow(" Not used")) | ||
|
||
for elb in not_used: | ||
if len(flags) > 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You use one global flags
array for all ELBs. How should this work when you have more than one ELB ?
|
||
response = get_all_elbs() | ||
for ELB in response['LoadBalancerDescriptions']: | ||
if len(ELB['Instances']) == 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stuff like this should already be done in the helper function. get_all_elbs
should just return a list of all ELBs where you don't need to do any more checks or processing of the data
print("Found {} security groups".format(len(all_sg))) | ||
|
||
not_used = [] | ||
for group in all_sg: | ||
id = group["GroupId"] | ||
if id not in used_groups: | ||
not_used.append(group) | ||
if len(elb_sg) > 0: | ||
if id not in elb_sg: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have a used_groups
array with all SGs that are in use by anything. Just append the list of your SGs from the ELBs to this list and then you don't need to modify this part of the code
Feature add for issue 17
Type of Change