feat(cfn): implement env controller #1508

iamhopaul123 · 2020-10-15T03:02:04Z

Implement env controller for env upgrade, allowing svc deploy to update env stack to create/delete optional resources (e.g., ALB).

Note that this feature is only available starting from env version v1.0.0.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

iamhopaul123 · 2020-10-15T03:06:54Z

Manual Tests

Create an environment. Then deploy a backend service. LB should not be created. Then create an LB service, and the LB should be created. Then delete those services, and LB resources should be removed from env stack.
Create an environment. Then deploy two LB services to it simultaneously. Both deployments should succeed. Then delete those two services from the environment, and the LB should be removed.

cf-custom-resources/lib/env-controller.js

cf-custom-resources/test/env-controller-test.js

templates/workloads/common/cf/env-controller.yml

efekarakus · 2020-10-15T23:55:00Z

templates/workloads/common/cf/envvars.yml

-  Value:
-    Fn::ImportValue:
-      !Sub "${AppName}-${EnvName}-PublicLoadBalancerDNS" {{if .Variables}}{{range $name, $value := .Variables}}
+  Value: !GetAtt EnvControllerAction.PublicLoadBalancerDNSName{{if .Variables}}{{range $name, $value := .Variables}}


I think this will break backend services since the Lambda is not included there!
Mmm this is interesting 🤔

I feel like what we need is a way of telling if an environment variable should exist because the feature is enabled. For example:

{{- if has .RequiredFeatures "ALBWorkloads" }} - Name: COPILOT_LB_DNS Value: !GetAtt EnvControllerAction.PublicLoadBalancerDNSName {{- end }}

or maybe only display it when it is an LB service?

yea, we could do that for now. But in the future the LB Web Service pattern will support NLB.
So instead of having http maybe users will specify only udp or tcp and that should still give them a service that is internet-facing. In that scenario we should not create this environment variable.

But yes let's do your suggestion for now! If it's a LBWebService, let's add the environment variable and remove it from the other types that don't force the resource creation.

efekarakus · 2020-10-16T00:09:55Z

templates/workloads/services/lb-web/cf.yml

@@ -168,6 +167,31 @@ Resources:
              Action:
                - elasticloadbalancing:DescribeRules
              Resource: "*"
+        - PolicyName: "EnvControllerStackUpdate"


Hmmm we are increasing the permission scope here! Which would require a security review.

I'm trying to understand how come this prototype worked: efekarakus@66a2c27 without these permission changes. I feel like my prototype shouldn't have worked :O strange!

How do you perform those actions without permission 🤔 And looks like the prototype doesn't pass CFN execution role to update the stack, which should fail when updating the environment stack...

ok instead of increasing the scope of the other Lambda, let's create a new role:
EnvControllerLambdaRole which has the permissions that you've listed.

Can we try being specific on the resources? For example:

the stack

Resource: !Sub 'arn:aws:cloudformation:${AWS::Region}:${AWS::AccountId}:stack/${AppName}-${EnvName}/*'

the role

Resource: !Sub 'arn:aws:iam::${AWS::AccountId}:role/${AppName}-${EnvName}-CFNExecutionRole'

Agree on creating a new role otherwise the other lambda scope might be expanded as well. However, I don't get why we want to be specific on the resources? I feel they are about the same since the only stack with both copilot app and env tags are in format of those two. But one benefit for using tags is it is agnostic about the arn format, whereas we are very unlikely to change our copilot tags. What do you think?

I guess we also need to move the role for auto scaling out right? i'll do that in the next PR

The slight difference is that if we use only tags, then the role can also update other service stacks in the environment. Whereas specifying the ARN limits it strictly to the environment stack and not to the services.

ah got it good point!

efekarakus · 2020-11-02T22:42:27Z

cf-custom-resources/lib/env-controller.js

+  try {
+    switch (event.RequestType) {
+      case "Create":
+        responseData = await controlEnv(


can we increase it to the max 15 mins plz? 🙏

I could see env upgrades taking a while just in case, and we should handle the previous comment above as well

efekarakus · 2020-11-02T22:46:44Z

cf-custom-resources/lib/env-controller.js

+    } catch (err) {
+      if (
+        !err.message.match(
+          /^Stack.*is in UPDATE_IN_PROGRESS state and can not be updated/


Yeah, I don't think they can change it either. OK as long as you validated that this is indeed the error message, I'm fine with it.

templates/environment/versions/cf-v1.0.0.yml

efekarakus · 2020-11-02T22:58:03Z

templates/workloads/common/cf/envvars.yml

-  Value:
-    Fn::ImportValue:
-      !Sub "${AppName}-${EnvName}-PublicLoadBalancerDNS" {{if .Variables}}{{range $name, $value := .Variables}}
+  Value: !GetAtt EnvControllerAction.PublicLoadBalancerDNSName{{if .Variables}}{{range $name, $value := .Variables}}


yea, we could do that for now. But in the future the LB Web Service pattern will support NLB.
So instead of having http maybe users will specify only udp or tcp and that should still give them a service that is internet-facing. In that scenario we should not create this environment variable.

But yes let's do your suggestion for now! If it's a LBWebService, let's add the environment variable and remove it from the other types that don't force the resource creation.

efekarakus · 2020-11-02T23:10:30Z

templates/workloads/services/lb-web/cf.yml

@@ -168,6 +167,31 @@ Resources:
              Action:
                - elasticloadbalancing:DescribeRules
              Resource: "*"
+        - PolicyName: "EnvControllerStackUpdate"


ok instead of increasing the scope of the other Lambda, let's create a new role:
EnvControllerLambdaRole which has the permissions that you've listed.

Can we try being specific on the resources? For example:

the stack

Resource: !Sub 'arn:aws:cloudformation:${AWS::Region}:${AWS::AccountId}:stack/${AppName}-${EnvName}/*'

the role

Resource: !Sub 'arn:aws:iam::${AWS::AccountId}:role/${AppName}-${EnvName}-CFNExecutionRole'

efekarakus

Looks good! just one more request related to the env variable

templates/workloads/common/cf/envvars.yml

Address this [comment](#1508 (comment)) left in #1508. Add retry to EnvController when we call `cfn.waitFor()`, so that it won't immediately error out when the request gets throttled.  By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Implement env controller for env upgrade, allowing `svc deploy` to update env stack to create/delete optional resources (e.g., ALB). Note that this feature is only available starting from env version v1.0.0.  By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Address this [comment](aws#1508 (comment)) left in aws#1508. Add retry to EnvController when we call `cfn.waitFor()`, so that it won't immediately error out when the request gets throttled.  By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

iamhopaul123 requested a review from a team as a code owner October 15, 2020 03:02

iamhopaul123 requested a review from kohidave October 15, 2020 03:02

amazon-ecs-cli-v2-pr-manager requested a review from efekarakus October 15, 2020 03:02

efekarakus added the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Oct 15, 2020

iamhopaul123 force-pushed the env-upgrade/env-controller branch from 3066f93 to d9edc87 Compare October 15, 2020 18:58

efekarakus reviewed Oct 16, 2020

View reviewed changes

bvtujo mentioned this pull request Oct 22, 2020

EFS Support for Copilot #1559

Closed

8 tasks

efekarakus reviewed Nov 2, 2020

View reviewed changes

iamhopaul123 force-pushed the env-upgrade/env-controller branch from ea73c34 to a135bd1 Compare November 3, 2020 05:58

efekarakus approved these changes Nov 3, 2020

View reviewed changes

templates/workloads/common/cf/envvars.yml Outdated Show resolved Hide resolved

iamhopaul123 force-pushed the env-upgrade/env-controller branch from 81455a5 to 07320c7 Compare November 3, 2020 22:50

efekarakus approved these changes Nov 3, 2020

View reviewed changes

iamhopaul123 removed the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Nov 3, 2020

iamhopaul123 added 5 commits November 3, 2020 15:08

feat(cfn): implement env controller

10e2777

Address feedback

e7f0600

Address feedback

0f2cc90

Scope down env controller lambda policy

c62b16b

Address feedback

59a58f7

iamhopaul123 force-pushed the env-upgrade/env-controller branch from 07320c7 to 59a58f7 Compare November 3, 2020 23:08

huanjani approved these changes Nov 3, 2020

View reviewed changes

Merge branch 'mainline' into env-upgrade/env-controller

c9e8241

mergify bot merged commit e4aeca0 into aws:mainline Nov 3, 2020

iamhopaul123 mentioned this pull request Nov 4, 2020

chore: add retry to EnvController waitFor requests #1632

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cfn): implement env controller #1508

feat(cfn): implement env controller #1508

iamhopaul123 commented Oct 15, 2020 •

edited

Loading

iamhopaul123 commented Oct 15, 2020 •

edited

Loading

efekarakus Oct 15, 2020

iamhopaul123 Oct 16, 2020

efekarakus Nov 2, 2020

iamhopaul123 Nov 3, 2020

efekarakus Oct 16, 2020

iamhopaul123 Oct 16, 2020 •

edited

Loading

efekarakus Nov 2, 2020

iamhopaul123 Nov 3, 2020

iamhopaul123 Nov 3, 2020

efekarakus Nov 3, 2020

iamhopaul123 Nov 3, 2020

efekarakus Nov 2, 2020

efekarakus Nov 2, 2020

efekarakus Nov 2, 2020

efekarakus Nov 2, 2020

efekarakus left a comment

feat(cfn): implement env controller #1508

feat(cfn): implement env controller #1508

Conversation

iamhopaul123 commented Oct 15, 2020 • edited Loading

iamhopaul123 commented Oct 15, 2020 • edited Loading

Manual Tests

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

iamhopaul123 Oct 16, 2020 • edited Loading

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

efekarakus left a comment

Choose a reason for hiding this comment

iamhopaul123 commented Oct 15, 2020 •

edited

Loading

iamhopaul123 commented Oct 15, 2020 •

edited

Loading

iamhopaul123 Oct 16, 2020 •

edited

Loading