fix: apply resource conditions to DeploymentPreference resources #1578

keetonian · 2020-04-28T05:30:02Z

Issue #, if available:
N/A

Description of changes:
Bug fix. Adds conditions from the Serverless::Function resource to the CodeDeploy resources that are created when using DeploymentPreference. Previously, these resources were added even if the resources that use them were not included based on resource conditions.

Description of how you validated changes:
Updated tests, deployed sample template and verified that resources were created correctly.

Breaking Change?
Potentially a breaking change if there is a dependency on the resources that were created before hand (when there was a bug, and conditions were not passed through to deployment preference resources). With this change, those resources that were previously created will be up for deletion. If those resources are being used elsewhere, those will start breaking now.

Checklist:

Write/update tests
make pr passes
Update documentation
Verify transformed template deploys and application functions as expected
Add/update example to examples/2016-10-31

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

keetonian · 2020-04-28T05:35:35Z

Deployed the following template (after a local transform) and verified that it created the correct resources (and didn't create others).

NOTE: This will cause some redeployments for customers where some resources will be removed from their templates. In the case of the following template:

Resources created:

CodeDeployServiceRole AWS::IAM::Role
MinimalFunctionAliaslive AWS::Lambda::Alias
MinimalFunctionDeploymentGroup AWS::CodeDeploy::DeploymentGroup
MinimalFunctionRole AWS::IAM::Role
MinimalFunctionVersion76c37f65fb AWS::Lambda::Version
MinimalFunction AWS::Lambda::Function
ServerlessDeploymentApplication AWS::CodeDeploy::Application

Resources no longer created (will be deleted):

MinimalFunctionWithDeploymentPreferenceWithHooksAndAlarmsDeploymentGroup AWS::CodeDeploy::DeploymentGroup
MinimalFunctionWithMinimalDeploymentPreferenceDeploymentGroup AWS::CodeDeploy::DeploymentGroup

Transform: AWS::Serverless-2016-10-31

Conditions:
  Condition1:
    Fn::Equals:
      - true
      - true
  Condition2:
    Fn::Equals:
      - true
      - false
  Condition3:
    Fn::Equals:
      - true
      - false

Parameters:
  MyFalseParameter:
    Type: String
    Default: False

Resources:
  MinimalFunction:
    Condition: Condition1
    Type: 'AWS::Serverless::Function'
    Properties:
      Handler: hello.handler
      InlineCode: |
        def hi():
          print("hi")
      Runtime: python2.7
      AutoPublishAlias: live
      DeploymentPreference:
        Type: Linear10PercentEvery2Minutes

  MinimalFunctionWithMinimalDeploymentPreference:
    Type: 'AWS::Serverless::Function'
    Condition: Condition2
    Properties:
      InlineCode: |
        def hi():
          print("hi")
      Handler: hello.handler
      Runtime: python2.7
      AutoPublishAlias: livewithdeployment
      DeploymentPreference:
        Type: Canary10Percent5Minutes

  MinimalFunctionWithDeploymentPreferenceWithHooksAndAlarms:
    Type: 'AWS::Serverless::Function'
    Condition: Condition2
    Properties:
      InlineCode: |
        def hi():
          print("hi")
      Handler: hello.handler
      Runtime: python2.7
      AutoPublishAlias: livewithdeploymentwithhooksandalarms
      DeploymentPreference:
        Type: Linear10PercentEvery2Minutes
        Hooks:
          PreTraffic: !Ref MySanityTestFunction
          PostTraffic: !Ref MyValidationTestFunction

  MySanityTestFunction:
    Type: 'AWS::Serverless::Function'
    Condition: Condition3
    Properties:
      Handler: hello.handler
      Runtime: python2.7
      InlineCode: |
        def hi():
          print("hi")
      DeploymentPreference:
        Enabled: False

  MyValidationTestFunction:
    Type: 'AWS::Serverless::Function'
    Condition: Condition3
    Properties:
      Handler: hello.handler
      Runtime: python2.7
      InlineCode: |
        def hi():
          print("hi")
      DeploymentPreference:
        Enabled: !Ref MyFalseParameter

sriram-mv · 2020-07-02T18:03:08Z

This PR potentially removes resources on a new deployment, the resources that will be removed should have not have been used. This PR does fixes an underlying bug and should have been the expected behavior to start with.

Any concerns on this PR?

…_iam_role

hawflau · 2022-05-06T17:14:30Z

Pushed new changes to enable the condition fix only if the feature flag "deployment_preference_condition_fix" is enabled. Existing behavior will be used for any account ID that is not allow-listed (see test case "tests/translator/input/function_with_deployment_preference_condition_without_condition_fix.yaml").

As the above comment mentioned, the resources that will be removed should have not been used. We decided to use a feature flag to gate it before we are certain that removing the resources are safe. More investigation will be done but is not under the current scope

jfuss · 2022-05-06T18:11:44Z

@hawflau Can we just add the removal of these into a new Property? That way back compat is kept but customers have a way to remove the behavior as needed?

codecov-commenter · 2022-05-07T17:25:22Z

Codecov Report

Merging #1578 (d39ef47) into develop (e7a1496) will increase coverage by 0.86%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #1578      +/-   ##
===========================================
+ Coverage    93.58%   94.44%   +0.86%     
===========================================
  Files           90       98       +8     
  Lines         6124     7254    +1130     
  Branches      1260     1505     +245     
===========================================
+ Hits          5731     6851    +1120     
- Misses         183      195      +12     
+ Partials       210      208       -2

Impacted Files	Coverage Δ
samtranslator/region_configuration.py	`77.77% <0.00%> (-22.23%)`	⬇️
samtranslator/model/codedeploy.py	`90.90% <0.00%> (-9.10%)`	⬇️
samtranslator/validator/validator.py	`91.80% <0.00%> (-3.85%)`	⬇️
samtranslator/model/exceptions.py	`97.67% <0.00%> (-2.33%)`	⬇️
samtranslator/open_api/open_api.py	`90.03% <0.00%> (-1.94%)`	⬇️
samtranslator/model/s3_utils/uri_parser.py	`68.42% <0.00%> (-0.81%)`	⬇️
samtranslator/yaml_helper.py	`89.47% <0.00%> (-0.53%)`	⬇️
samtranslator/translator/logical_id_generator.py	`90.62% <0.00%> (-0.29%)`	⬇️
samtranslator/model/api/api_generator.py	`94.08% <0.00%> (-0.28%)`	⬇️
samtranslator/model/apigateway.py	`96.98% <0.00%> (-0.18%)`	⬇️
... and 44 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 2be47df...d39ef47. Read the comment docs.

hawflau · 2022-05-09T16:20:05Z

Updated to add a new property PassthroughCondition for customer to opt-in this fix. The fix will be applied on if PassthroughCondition is set to true, or if the account ID is allow-listed. Default is false. Intrinsic is not supported

Examples:

...
# Condition will be passed through to CodeDeploy resources
  MinimalFunctionWithMinimalDeploymentPreference1:
    Type: 'AWS::Serverless::Function'
    Condition: Condition2
    Properties:
      InlineCode: |
        def hi():
          print("hi")
      Handler: hello.handler
      Runtime: python3.9
      AutoPublishAlias: livewithdeployment
      DeploymentPreference:
        Type: Canary10Percent5Minutes
        PassthroughCondition: true

# Condition will not be passed through to CodeDeploy resources
  MinimalFunctionWithMinimalDeploymentPreference2:
    Type: 'AWS::Serverless::Function'
    Condition: Condition2
    Properties:
      InlineCode: |
        def hi():
          print("hi")
      Handler: hello.handler
      Runtime: python3.9
      AutoPublishAlias: livewithdeployment
      DeploymentPreference:
        Type: Canary10Percent5Minutes
        PassthroughCondition: false

# Default is false. Condition will not be passed through to CodeDeploy resources unless account is allowlisted
  MinimalFunctionWithMinimalDeploymentPreference3:
    Type: 'AWS::Serverless::Function'
    Condition: Condition2
    Properties:
      InlineCode: |
        def hi():
          print("hi")
      Handler: hello.handler
      Runtime: python3.9
      AutoPublishAlias: livewithdeployment
      DeploymentPreference:
        Type: Canary10Percent5Minutes

@jfuss Can you check if that makes sense to you?

torresxb1 · 2022-05-17T16:43:02Z

samtranslator/model/preferences/deployment_preference.py

-        # FIXME: We should support not only true/false, but also yes/no, on/off? See https://yaml.org/type/bool.html 
-        passthrough_condition = True if passthrough_condition in [True, "true", "True"] else False


moved to sam_resources

hawflau · 2022-05-17T18:09:46Z

samtranslator/model/sam_resources.py

@@ -860,6 +889,40 @@ def _validate_deployment_preference_and_add_update_policy(
                "UpdatePolicy", deployment_preference_collection.update_policy(self.logical_id).to_dict()
            )

+    def _resolve_property_to_boolean(
+        self,
+        property_value: Union[bool, str, dict],


nit: Can it be anything else? like int and float?

nope, can't they only be string, boolean or intrinsic?
https://yaml.org/type/bool.html

sriram-mv requested review from sriram-mv and c2tarun June 15, 2020 20:04

sriram-mv approved these changes Jun 16, 2020

View reviewed changes

CoshUS added the pr/external label Mar 22, 2021

hawflau force-pushed the code-deploy-condition-fix branch from 8edfda3 to 10c0b99 Compare April 22, 2022 22:29

keetonian and others added 3 commits April 25, 2022 17:45

fix code deploy conditions bug

1c2e550

Cover all condition cases, update tests

7aacc65

black reformat

2ec132f

hawflau force-pushed the code-deploy-condition-fix branch from 0de6ed0 to 2ec132f Compare April 26, 2022 00:45

hawflau added 4 commits May 4, 2022 09:42

Update tests to use .get_codedeploy_iam_role() instead of .codedeploy…

b2736f4

…_iam_role

Fixing unit tests after merging Condition fix commit

38fcc48

Use feature toggle to gate deployment preference condition fix

2a56189

Add tests

f14ef77

torresxb1 approved these changes May 6, 2022

View reviewed changes

hawflau added 2 commits May 6, 2022 16:33

Revert using feature toggle

cdcde39

Add property to opt-in deployment preference condition passthrough

7b672e7

hawflau added 2 commits May 7, 2022 16:02

Add tests for PassthroughCondition

8585bf8

Fix passthrough condition logic and add tests

c7cbf79

hawflau and others added 2 commits May 10, 2022 13:51

Update PassthroughCondition to support intrinsic

8405853

intrinscs support + tests

82ffbd9

torresxb1 reviewed May 17, 2022

View reviewed changes

torresxb1 added 2 commits May 17, 2022 09:54

update invalid intrinsics end-to-end test

d39ef47

uncomment and update invalid value unit test

4597514

hawflau reviewed May 17, 2022

View reviewed changes

black

bdaded1

hawflau approved these changes May 17, 2022

View reviewed changes

moelasmar approved these changes May 19, 2022

View reviewed changes

torresxb1 merged commit 8d0e058 into aws:develop May 19, 2022

hawflau mentioned this pull request Sep 21, 2022

feat: add UnescapeMappingTemplate to state machine Api event #2495

Closed

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: apply resource conditions to DeploymentPreference resources #1578

fix: apply resource conditions to DeploymentPreference resources #1578

keetonian commented Apr 28, 2020 •

edited by sriram-mv

Loading

keetonian commented Apr 28, 2020

sriram-mv commented Jul 2, 2020

hawflau commented May 6, 2022

jfuss commented May 6, 2022

codecov-commenter commented May 7, 2022 •

edited

Loading

hawflau commented May 9, 2022

torresxb1 May 17, 2022

hawflau May 17, 2022

torresxb1 May 17, 2022

		# FIXME: We should support not only true/false, but also yes/no, on/off? See https://yaml.org/type/bool.html
		passthrough_condition = True if passthrough_condition in [True, "true", "True"] else False

fix: apply resource conditions to DeploymentPreference resources #1578

fix: apply resource conditions to DeploymentPreference resources #1578

Conversation

keetonian commented Apr 28, 2020 • edited by sriram-mv Loading

keetonian commented Apr 28, 2020

sriram-mv commented Jul 2, 2020

hawflau commented May 6, 2022

jfuss commented May 6, 2022

codecov-commenter commented May 7, 2022 • edited Loading

Codecov Report

hawflau commented May 9, 2022

torresxb1 May 17, 2022

Choose a reason for hiding this comment

hawflau May 17, 2022

Choose a reason for hiding this comment

torresxb1 May 17, 2022

Choose a reason for hiding this comment

keetonian commented Apr 28, 2020 •

edited by sriram-mv

Loading

codecov-commenter commented May 7, 2022 •

edited

Loading