1636 long aws check #1919

VakarisZ · 2022-04-28T06:23:18Z

What does this PR do?

Fixes #1636

Add any further explanations here.

PR Checklist

Have you added an explanation of what your changes do and why you'd like to include them?
Is the TravisCI build passing?
Was the CHANGELOG.md updated to reflect the changes?
Was the documentation framework updated to reflect the changes?
Have you checked that you haven't introduced any duplicate code?

Testing Checklist

Added relevant unit tests?
Have you successfully tested your changes locally? Elaborate:

Tested by setting long timeout, going to run page and making sure loading icon appears while request is loading
Tested by going to report and making sure aws exporter doesn't crash

ilija-lazoroski

Good job! Just make sure it is tested on actual AWS instance.

mssalvatore

Just a few small suggestions.

monkey/common/cloud/aws/aws_instance.py

monkey/monkey_island/cc/services/remote_run_aws.py

codecov · 2022-04-28T15:02:05Z

Codecov Report

Merging #1919 (4a1b336) into develop (f65e009) will decrease coverage by 0.06%.
The diff coverage is 76.19%.

❗ Current head 4a1b336 differs from pull request most recent head 3a98fdb. Consider uploading reports for the commit 3a98fdb to get more accurate results

@@             Coverage Diff             @@
##           develop    #1919      +/-   ##
===========================================
- Coverage    55.43%   55.37%   -0.07%     
===========================================
  Files          449      448       -1     
  Lines        12816    12927     +111     
===========================================
+ Hits          7105     7158      +53     
- Misses        5711     5769      +58

Impacted Files	Coverage Δ
...ey/infection_monkey/utils/aws_environment_check.py	`0.00% <0.00%> (ø)`
monkey/monkey_island/cc/server_setup.py	`0.00% <0.00%> (ø)`
...onkey_island/cc/services/reporting/aws_exporter.py	`0.00% <0.00%> (ø)`
monkey/common/aws/aws_service.py	`62.06% <50.00%> (ø)`
monkey/monkey_island/cc/app.py	`78.03% <50.00%> (+0.16%)`	⬆️
monkey/common/utils/code_utils.py	`76.92% <57.14%> (-23.08%)`	⬇️
monkey/monkey_island/cc/services/remote_run_aws.py	`58.97% <62.50%> (+5.12%)`	⬆️
monkey/common/aws/aws_instance.py	`98.33% <100.00%> (ø)`
monkey/common/cmd/aws/aws_cmd_runner.py	`46.42% <100.00%> (ø)`
monkey/monkey_island/cc/resources/remote_run.py	`33.33% <100.00%> (ø)`
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fdf8198...3a98fdb. Read the comment docs.

mssalvatore

Given the complications that have arisen, I think an overall better approach is provide a function called get_aws_info() -> AWSInfo where AWSInfo is a named tuple or dataclass. We can use @lru_cache to cache the results of get_aws_info() to prevent the slow calls from being made more than once.

See the comments for more detail.

monkey/common/utils/code_utils.py

monkey/tests/unit_tests/common/cloud/aws/test_aws_instance.py

monkey/common/cloud/aws/aws_instance.py

Singleton is a common pattern, potentially usable in the Agent so it belongs in common

AWS related services call AWS metadata service which might take a long time to timeout, that's why they are ran on a separate thread

Locks will avoid the situation where is_running_on_aws is called before this service finished initializing

Update the IAM role change screenshot and add a note about firewall rules

This change simplifies the codebase by removing unnecessary inheritance and nested directory structure

This also changes AwsInstance from singleton and instead the aws_service package is used as one

VakarisZ requested review from ilija-lazoroski and mssalvatore April 28, 2022 06:26

ilija-lazoroski approved these changes Apr 28, 2022

View reviewed changes

mssalvatore approved these changes Apr 28, 2022

View reviewed changes

monkey/common/cloud/aws/aws_instance.py Show resolved Hide resolved

monkey/common/cloud/aws/aws_instance.py Outdated Show resolved Hide resolved

monkey/monkey_island/cc/services/remote_run_aws.py Outdated Show resolved Hide resolved

mssalvatore requested changes Apr 29, 2022

View reviewed changes

mssalvatore reviewed Apr 29, 2022

View reviewed changes

monkey/common/cloud/aws/aws_instance.py Outdated Show resolved Hide resolved

VakarisZ and others added 15 commits May 2, 2022 15:55

Island, Common: Move singleton to code_utils.py in common

65eb9b1

Singleton is a common pattern, potentially usable in the Agent so it belongs in common

Common: Change AwsInstance to be a Singleton

0e1ffb4

Island: Run AWS services on separate threads

d3c1ff8

AWS related services call AWS metadata service which might take a long time to timeout, that's why they are ran on a separate thread

Island: Add locks to remote_run_aws.py

8535118

Locks will avoid the situation where is_running_on_aws is called before this service finished initializing

Changelog: Add entry about fixed long AWS check on island startup

946c394

Common: Fix aws_instance.py formatting

f63bc77

Island, Common: Change AwsInstance properties to private, add getter

fead7f6

Island: Refactor lock to event in remote_run_aws.py

b58d847

Common: Replace protected attributes with read-only properties

797482a

Docs: Improve running on AWS ec2 documentation

cea49b6

Update the IAM role change screenshot and add a note about firewall rules

Common: Improve singleton to use WeakValueDictionary

e51b760

UT: Fix AWS instance unit tests

c5a126f

Common: Remove CloudInstance since aws is the only cloud supported

7b2ff1e

This change simplifies the codebase by removing unnecessary inheritance and nested directory structure

Agent, Island, Common: Refactor AwsService from class to package

f3a5a70

This also changes AwsInstance from singleton and instead the aws_service package is used as one

Common: Rever singleton from WeakValueDictionary to simple dictionary

e895897

VakarisZ force-pushed the 1636-long-aws-check branch from 501827a to e895897 Compare May 2, 2022 13:02

mssalvatore added 5 commits May 2, 2022 09:10

Agent: Use AwsInstance instead of aws_service

2714617

Island: Relocate aws_service from common to monkey_island

ae83c2e

Island: Add get_account_id() to aws_service

56ea170

Island: Use aws_service.get_account_id() in AWSExporter

ceebdea

Island: Move AwsCmdRunner to monkey_island/cc/server_utils/

a466e97

mssalvatore approved these changes May 2, 2022

View reviewed changes

UT: Use MappingProxyType for defaults in get_test_aws_instance()

3a98fdb

mssalvatore merged commit 8d65fa3 into develop May 2, 2022

mssalvatore deleted the 1636-long-aws-check branch May 2, 2022 13:53

SarmadBytes mentioned this pull request May 16, 2022

[Snyk] Security upgrade marked from 1.1.1 to 2.0.0 SarmadBytes/monkey#11

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

1636 long aws check #1919

1636 long aws check #1919

Uh oh!

VakarisZ commented Apr 28, 2022 •

edited by mssalvatore

Loading

Uh oh!

ilija-lazoroski left a comment

Uh oh!

mssalvatore left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Apr 28, 2022 •

edited

Loading

Uh oh!

mssalvatore left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

1636 long aws check #1919

1636 long aws check #1919

Uh oh!

Conversation

VakarisZ commented Apr 28, 2022 • edited by mssalvatore Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

PR Checklist

Testing Checklist

Uh oh!

ilija-lazoroski left a comment

Choose a reason for hiding this comment

Uh oh!

mssalvatore left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Apr 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mssalvatore left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

VakarisZ commented Apr 28, 2022 •

edited by mssalvatore

Loading

codecov bot commented Apr 28, 2022 •

edited

Loading