PaloAltoNetworks / pcs-sizing-scripts

Prisma Cloud sizing scripts
ISC License
46 stars 49 forks source link

The EC2 count is incorrect which case has multiple AMI Launch Index Value. #28

Closed hiroinfinity closed 2 years ago

hiroinfinity commented 2 years ago

Describe the bug

resource-count-aws.sh

If there is an AMI launch Index, the EC2 count is different.

About AMI Lunch Index. https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/AMI-launch-index-examples.html

Expected behavior

Multiple EC2 instances in the AMI Launch Index are counted.

Current behavior

Multiple EC2 instances in the AMI Launch Index are not counted.

Possible solution

I am not good at jq. For example, I think it is possible to implement the following using grep and wc.

resource-count-aws.sh line 293 RESOURCE_COUNT=$(aws_ec2_describe_instances "${i}" | grep InstanceId | wc -l' 2>/dev/null)

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

:tada: Thanks for opening your first issue here! Welcome to the community!

tkishel commented 2 years ago

Thank you for this report. I see that

https://docs.aws.amazon.com/cli/latest/reference/ec2/describe-instances.html#output

Prisma Cloud only counts running instances when calculating credit consumption. In your possible solution, you did not include this filter:

 --filters "Name=instance-state-name,Values=running"

https://github.com/PaloAltoNetworks/pcs-sizing-scripts/blob/main/aws/resource-count-aws.sh#L106

Could you explain why this script should count stopped instances?

I see that AmiLaunchIndex is documented as a property of a described instance:

https://docs.aws.amazon.com/cli/latest/reference/ec2/describe-instances.html#output

How does the existence of the AmiLaunchIndex property change the count of this script?

If you could provide example output (with any sensitive data redacted) from a call to describe-instances with the AmiLaunchIndex and State properties, similar to:

https://github.com/PaloAltoNetworks/pcs-sizing-scripts/blob/main/spec/resource-count-aws_spec.sh#L136

We could test this.

Thanks!

hiroinfinity commented 2 years ago

Thank you for your reply.

--filters "Name=instance-state-name,Values=running" My possible solution is include this filter, because this filter include the function aws_ec2_describe_instances. https://github.com/PaloAltoNetworks/pcs-sizing-scripts/blob/main/aws/resource-count-aws.sh#L107

I will provide an example output below. In this case, the desired count is 3, but the current count is 2.

{
    "Reservations": [
        {
            "Groups": [],
            "Instances": [
                {
                    "AmiLaunchIndex": 0,
                    "InstanceId": "i-01234567890",
                }
            ],
        },
        {
            "Groups": [],
            "Instances": [
                {
                    "AmiLaunchIndex": 0,
                    "InstanceId": "i-abcdefghijk",
                },
                {
                    "AmiLaunchIndex": 1,
                    "InstanceId": "i-567890abcde",
                }
            ]
        }
    ]
}

Would you please confirm? Thanks!

tkishel commented 2 years ago

Sorry, I missed that those filters are defined in the function.

This appears to be an issue with the output from aws ec2 describe-instances having Reservations as the outer object rather than Instances as the outer object, rather than the existence of AmiLaunchIndex.

I don't know if this is a recent change in the output, given ...

https://github.com/PaloAltoNetworks/pcs-sizing-scripts/commit/550a5e466014ceaada7acbee175d37390df72b75#diff-df255a31916be93e0b704bdc951dbbcd607fe60c7a9f25ac3513edb61dcf96e6R15

... or if it is somehow context-sensitive, but I'll submit a PR that expects:

{
    "Reservations": [{
            "Groups": [],
            "Instances": [{
                    "InstanceId": "0abcdef1234567890"
                },
                {
                    "InstanceId": "0abcdef1234567891"
                }
            ]
        },
        {
            "Groups": [],
            "Instances": [{
                "InstanceId": "1abcdef1234567890"
            }]
        }
    ]
}

Rather than:

{
    "Instances": [{
        "InstanceId": "0abcdef1234567890"
    }]
}

... from the aws ec2 describe-instances command.

tkishel commented 2 years ago

@heroinfinity could you confirm this fix?

hiroinfinity commented 2 years ago

@tkishel Great! I've solved it.

tkishel commented 2 years ago

Thank your for reporting and confirming!