More changes in support of multiple orchestrations #32

netsettler · 2021-08-11T13:23:06Z

This is work in progress.

PLEASE IGNORE THE FOLLOWING, which I'll be cleaning up more:

Large blocks of commented out code.
Random print statements and pdb settings. (I think I removed them but I'll re-check before trying to make this final.)

…nd configs go.

willronchetti

These changes aside from some small things I think you're already working to clean up generally look great. One thing to look into though is why the security group rules for the application security group are being dropped. We saw in your test account that the security group is missing rules, though it is not obvious from the code changes why.

willronchetti · 2021-08-11T13:50:20Z

app.py

+class SingletonManager():
+
+    def __init__(self, singleton_class, *singleton_args, **singleton_kwargs):
+        self._singleton = None
+        self._singleton_class = singleton_class
+        self._singleton_args = singleton_args
+        self._singleton_kwargs = singleton_kwargs
+
+    @property
+    def singleton(self):
+        if not self._singleton:
+            self._singleton = self._singleton_class(*self._singleton_args or (), **self._singleton_kwargs or {})
+        return self._singleton


Should this go in utils?

Probably but not today. I'll make a note for the future.

willronchetti · 2021-08-11T13:50:47Z

app.py

-app_utils_obj = AppUtils()
+
+# app_utils_obj = AppUtils()
+app_utils_manager = SingletonManager(AppUtils)


Is the concern multiple initialization? I think chalice prevents that from happening, but admit I'm not sure. We have not had issues with this in the past though.

Yeah. We can look at that later. Seems harmless if it's done elsewhere too.

willronchetti · 2021-08-11T13:51:39Z

app.py



 ########################
 # Misc utility functions
 ########################


+def compute_valid_deploy_stages():
+    return list(Deploy.CONFIG_BASE['stages'].keys()) + ['test']


test is not a deploy stage I'm aware of.

It's allowed somewhere, and not due to me. We should hunt this down sometime. I'm going to leave this.

willronchetti · 2021-08-11T13:56:14Z

docs/deploy_new_account.rst

-    source ~/.aws_test/test_creds.sh
-    poetry run cli provision --trial --output_file out/foursight-dev-tmp/ --stage dev foursight --alpha --upload_change_set
+    source custom/aws_creds/test_creds.sh
+    poetry run cli provision --output_file out/foursight-dev-tmp/ --stage dev foursight --upload_change_set


A note on dev vs. prod stage is probably useful here.

I added a "to do" note in the doc.

willronchetti · 2021-08-11T13:59:33Z

src/base.py

+        secrets = self._load_config(self.SECRETS_FILE)
+        if True:
+            if not secrets.get(Secrets.S3_ENCRYPT_KEY):  # if missing or empty, try to default from file
+                secrets[Secrets.S3_ENCRYPT_KEY] = ConfigManager.get_s3_encrypt_key_from_file()
+        else:
+            print(f"WARNING: {Secrets.S3_ENCRYPT_KEY} is not in {self.SECRETS_FILE},"
+                  f" and the file {self.S3_ENCRYPT_KEY_FILE} does not exist.")


if True is this intended?

willronchetti · 2021-08-11T15:01:14Z

src/parts/ecs.py

+        ignorable(indexer)
        template.add_resource(self.ecs_ingester_task())
        ingester = template.add_resource(self.ecs_ingester_service())
+        ignorable(ingester)


These variables are from old code and can just be removed.

Fixed in sources to be uploaded shortl.

willronchetti · 2021-08-11T15:02:26Z

src/parts/ecs.py

+                        # VERY IMPORTANT - this environment variable determines which identity in the secrets manager
+                        # to use. If this secret does not exist, things will not start up correctly - this is ok in
+                        # the short term, but shortly after orchestration the secret value should be set.


Should probably be updated to indicate that ECS may not come online at all if the secret is not filled out fully.

willronchetti · 2021-08-11T15:03:11Z

src/parts/ecs.py

@@ -680,15 +696,17 @@ def ecs_deployment_task(self, cpus='256', mem='512', identity=None, initial=Fals
                    LogConfiguration=LogConfiguration(
                        LogDriver='awslogs',
                        Options={
-                            'awslogs-group': self.LOGGING_EXPORTS.import_value(C4LoggingExports.CGAP_APPLICATION_LOG_GROUP),
+                            'awslogs-group':
+                                self.LOGGING_EXPORTS.import_value(C4LoggingExports.CGAP_APPLICATION_LOG_GROUP),
                            'awslogs-region': Ref(AWS_REGION),
                            'awslogs-stream-prefix': 'cgap-deployment'


This prefix should probably be cgap-initial-deployment so that it can be distinguished easily from other deployment logs.

willronchetti · 2021-08-11T15:04:07Z

src/parts/iam.py

                "arn:aws:iam::aws:policy/service-role/AmazonEC2ContainerServiceforEC2Role",
-# Maybe?
-#                "arn:aws:iam::aws:policy/aws-service-role/AmazonECSServiceRolePolicy"
+                # Maybe?
+                # "arn:aws:iam::aws:policy/aws-service-role/AmazonECSServiceRolePolicy"


The ECS role policy is the desired one and we don't require the EC2 perms either IRC.

willronchetti · 2021-08-11T15:04:42Z

src/parts/network.py

+SUBNETS = {
+    'PrivateSubnetA': {'name': 'PrivateSubnetA', 'cidr_block': '10.0.0.0/18', 'az': 'us-east-1a', 'kind': 'private'},
+    'PublicSubnetA': {'name': 'PublicSubnetA', 'cidr_block': '10.0.64.0/18', 'az': 'us-east-1a', 'kind': 'public'},
+    'PrivateSubnetB': {'name': 'PrivateSubnetB', 'cidr_block': '10.0.128.0/18', 'az': 'us-east-1b', 'kind': 'private'},
+    'PublicSubnetB': {'name': 'PublicSubnetB', 'cidr_block': '10.0.192.0/18', 'az': 'us-east-1b', 'kind': 'public'},
+}


Might consider allowing some of this to come from settings, especially AZ's.

netsettler added 10 commits July 28, 2021 11:00

Remove some unused code.

8525126

Remove non --alpha functionality.

d80766a

Remove an unwanted import.

2cf6643

Add custom and custom.* to .gitignore for a change in where secrets a…

91e6426

…nd configs go.

New versions of dcicutils. Move some scripts to src/commands.

3b17385

json substitution code moved to utils

a2a0150

WIP

ed52076

WIP

275f388

WIP

96737d5

WIP

42914ff

willronchetti approved these changes Aug 11, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More changes in support of multiple orchestrations #32

More changes in support of multiple orchestrations #32

netsettler commented Aug 11, 2021

willronchetti left a comment

willronchetti Aug 11, 2021

netsettler Aug 17, 2021

willronchetti Aug 11, 2021

netsettler Aug 17, 2021

willronchetti Aug 11, 2021

netsettler Aug 17, 2021

willronchetti Aug 11, 2021

netsettler Aug 17, 2021

willronchetti Aug 11, 2021

willronchetti Aug 11, 2021

netsettler Aug 17, 2021

willronchetti Aug 11, 2021

netsettler Aug 17, 2021

willronchetti Aug 11, 2021

willronchetti Aug 11, 2021

willronchetti Aug 11, 2021

More changes in support of multiple orchestrations #32

Are you sure you want to change the base?

More changes in support of multiple orchestrations #32

Conversation

netsettler commented Aug 11, 2021

willronchetti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment