Added Support for ECS Fargate Tasks - Assuming each task runs only one container

erickrazr commented 2 years ago

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate)

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist

[ ] I have updated the documentation accordingly.
[ ] I have read the CONTRIBUTING document.
[ ] I have added tests to cover my changes if appropriate.
[ ] All new and existing tests passed.

welcome-to-palo-alto-networks[bot] commented 2 years ago

:tada: Thanks for opening this pull request! We really appreciate contributors like you! :raised_hands:

tkishel commented 2 years ago

Could you test this with https://www.shellcheck.net/ Especially important since this code runs in the customer's environment. Thanks!

tkishel commented 2 years ago

The shellspec spec test is failing because its actually trying to execute aws ecs list-clusters and aws ecs list-tasks and both of those are executed directly in aws_ecs_list_ecs_fargate_containers_running() rather than being wrapped in utility functions. All of the utility functions in this script make exactly one aws cli call, checking the error code, and only returning the result. For example:

aws_lambda_get_account_settings() {
  RESULT=$(aws lambda get-account-settings --region="${1}" --output json 2>/dev/null)
  if [ $? -eq 0 ]; then
    echo "${RESULT}"
  else
    echo '{"Error": [] }'
  fi
}

Any transformation (in this case, piping to jq) of the result occurs in the code calling the utility function:

        RESOURCE_COUNT=$(aws_lambda_get_account_settings "${i}" | jq '.AccountUsage.FunctionCount' 2>/dev/null)
        echo " Count of Lambda Functions in Region ${i}: ${RESOURCE_COUNT}"
        LAMBDA_COUNT=$((LAMBDA_COUNT + RESOURCE_COUNT))

This allows for easier spec testing and avoids issues with process piping (aws ecs list-clusters --output json | jq -r '.clusterArns[]') obscuring the root cause of an error code.

Please refactor to use utility methods for each new aws cli call in aws/resource-count-aws.sh and add mocks for those utility functions (It 'returns a list of ECS Clusters' and It 'returns a list of ECS Tasks') and count tests count tests to It 'counts compute resources' in spec/resource-count-aws.spec.

You can run the spec test locally with:

shellspec spec/resource-count-aws.spec

Tips:

Your mocks will be similar to aws_lambda_get_account_settings() so consider using it (and/or aws_ec2_describe_regions()) as a reference mock. I used the AWS documentation to create mocks:

https://docs.aws.amazon.com/cli/latest/reference/lambda/get-account-settings.html

But you could also call the aws cli directly, and I thinned out the mock to focus on what is being tested, for comprehension.

If your mocks return 4 ECS Tasks, your count test would be 16, since the mock for aws_ec2_describe_regions returns 4 regions, and the script loops through regions.

tkishel commented 2 years ago

This branch incorporates the suggestions above.

https://github.com/PaloAltoNetworks/pcs-sizing-scripts/compare/main...tkishel:pcs-sizing-scripts:refactor_s3_add_fargate

Please set and either refactor and submit the same changes, or I can merge it.

tkishel commented 2 years ago

I realize that the above is a lot of work for a new contributor. Incorporated and merged as:

https://github.com/PaloAltoNetworks/pcs-sizing-scripts/pull/41

PaloAltoNetworks / pcs-sizing-scripts