Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Functions as DeletionPolicy/UpdateReplacePolicy Values #58

Closed
jthomerson opened this issue Sep 6, 2019 · 33 comments
Closed

Support Functions as DeletionPolicy/UpdateReplacePolicy Values #58

jthomerson opened this issue Sep 6, 2019 · 33 comments
Labels
feature request Proposal for new features or requests

Comments

@jthomerson
Copy link

Currently the DeletionPolicy attribute for any resource can only be a string. This means we can't use functions like !If [ IsProduction, 'Retain', 'Delete' ].

I'd expect to be able to use conditions and other CloudFormation functions as a value for a DeletionPolicy the same as I can elsewhere in the template.

Others who have asked the same thing:

@bdrx312
Copy link

bdrx312 commented Dec 9, 2019

It would be nice if the DeletionPolicy also accepted a parameter from the Parameters section although you could work around that if DeletionPolicy at least supported !If

@PatMyron PatMyron changed the title All Resources: Support Functions as DeletionPolicy Values All Resources: Support Functions as DeletionPolicy/UpdateReplacePolicy Values Dec 12, 2019
@PatMyron
Copy link

PatMyron commented Dec 14, 2019

A workaround is having multiple duplicate resources with different policies and using Conditions to create only resources with correct policies if anyone needs a workaround until this is properly supported

@PatMyron PatMyron changed the title All Resources: Support Functions as DeletionPolicy/UpdateReplacePolicy Values Support Functions as DeletionPolicy/UpdateReplacePolicy Values Dec 14, 2019
@jthomerson
Copy link
Author

Yeah, @PatMyron, that works, but I'm sure you'd agree that "copy and paste your resources and hope you always remember to keep them in sync" is a pretty bad pattern. 😜 Glad you added "until this is properly supported" ... that gives me hope!

@bdrx312
Copy link

bdrx312 commented Dec 15, 2019

That work around is really bad if you have an resources that have dependencies (DependsOn) on the resources.

@danieljamesscott
Copy link

I think that a better work around is to use AWS::Include and parameterise the name of the included snippet:

"Fn::Transform" : {
  "Name" : "AWS::Include",
  "Parameters" : {
    "Location" : {"Fn::Sub": "s3://${S3Bucket}/deletionPolicy/${RDSDeletionPolicy}.json" }
  }
}

It does mean you have to have snippets for all possible values (and a separate set for UpdateReplacePolicy), but better than duplicating resources.

@PatMyron
Copy link

PatMyron commented Dec 15, 2019

@danieljamesscott due to AWS::Include regional availability, I've used Jinja to avoid maintaining duplicate source code

@bdrx312
Copy link

bdrx312 commented Dec 17, 2019

The AWS::Include that @danieljamesscott mentioned is a better work around than having duplicate resources but as @PatMyron mentioned is not available in all regions.

@danieljamesscott
Copy link

I've also just discovered that you can't use that work around for multiple policies (DeletionPolicy and UpdateReplacePolicy for example) because it results in duplicate keys in the JSON. The work around for that is to generate transforms with each combination of the 3 possibilities.

If AWS would just let us parameterise the policies it would save all this craziness... :)

@PatMyron
Copy link

PatMyron commented Dec 17, 2019

@danieljamesscott duplicate resources should have different Logical IDs
As @bdrx312 mentioned, painful workaround if you have resources with DependsOn because of that

And agreed, temporary workarounds are not to say this isn't still a most important feature request :)

@danieljamesscott
Copy link

danieljamesscott commented Dec 17, 2019

@PatMyron I'll admit that I haven't tested it for 'real' (my editor and cfn-lint complained) but is this really valid CF?

"S3Bucket": {
  "Type": "AWS::S3::Bucket",
  "Properties": {
    "BucketName": "dan-test-bucket4",
    "VersioningConfiguration": {
      "Status": "Suspended"
    }
  },
  "Fn::Transform" : {
    "Name" : "AWS::Include",
    "Parameters" : {
      "Location" : {"Fn::Sub": "s3://cloudformation-partials/deletePolicy/${DeletePolicy}.json" }
    }
  },
  "Fn::Transform" : {
    "Name" : "AWS::Include",
    "Parameters" : {
      "Location" : {"Fn::Sub": "s3://cloudformation-partials/updateReplacePolicy/${UpdateReplacePolicy}.json" }
    }
  }
}

@mrowles
Copy link

mrowles commented May 1, 2020

This has a high level of user experience, automation, cost and security implications. This was first raised in 2014 and we, as paying customers, still have no way to set DeletionPolicy dynamically.

@jobarjo
Copy link

jobarjo commented Jul 19, 2020

Has there been any feedback from AWS on this topic and at least a tentative target date for completion?

@zetas
Copy link

zetas commented Aug 19, 2020

@PatMyron I'll admit that I haven't tested it for 'real' (my editor and cfn-lint complained) but is this really valid CF?

"S3Bucket": {
  "Type": "AWS::S3::Bucket",
  "Properties": {
    "BucketName": "dan-test-bucket4",
    "VersioningConfiguration": {
      "Status": "Suspended"
    }
  },
  "Fn::Transform" : {
    "Name" : "AWS::Include",
    "Parameters" : {
      "Location" : {"Fn::Sub": "s3://cloudformation-partials/deletePolicy/${DeletePolicy}.json" }
    }
  },
  "Fn::Transform" : {
    "Name" : "AWS::Include",
    "Parameters" : {
      "Location" : {"Fn::Sub": "s3://cloudformation-partials/updateReplacePolicy/${UpdateReplacePolicy}.json" }
    }
  }
}

I'm not sure if using 2 transforms like this is necessary or works, I'm doing something similar and combining both policies in one include like so:

Resources:
  Instance:
    Type: 'AWS::RDS::DBInstance'
    Fn::Transform:
      Name: AWS::Include
      Parameters:
        Location: !Sub 's3://${S3SourceBucketName}/${CPVersion}/templates/customer/rds-instance-${StackType}-policy.yaml'
    Properties:
      Engine: MySQL
      EngineVersion: '5.7'
...

and then in the rds-instance-dev-policy.yaml:

DeletionPolicy: Delete
UpdateReplacePolicy: Delete

and all seems to work fine. I am however getting an error with cfn-lint: E3001 Invalid resource attribute Fn::Transform for resource Instance rds.yaml:64:5 which is super annoying because I use taskcat for CF testing and it relies on the cfn linter under the hood so I can't complete my CI pipeline now because it fails the linter. I've posted about it on their issue tracker here but I think this is an aws-cfn-bootstrap issue which is why I'm posting here. I hope someone with knowledge with that package can read this and help.

Also as an aside in that issue I linked there is the processed json template directly from the CF ui so you can see definitively that those directives are being included properly.

@PatMyron
Copy link

@zetas AWS::Include transform isn't fully supported by the Linter yet, but can workaround with resource-level ignores:

  Instance:
    Type: AWS::RDS::DBInstance
    Metadata:
      cfn-lint:
        config:
          ignore_checks:
          - E3001

@zetas
Copy link

zetas commented Aug 20, 2020

@PatMyron omg you are amazing, thank you so much! That worked great.

@nefkor
Copy link

nefkor commented Sep 28, 2020

It would be important to fix this issue soon, since many customer projects required parametrize the deletion policies, and making "copies" with conditional is not an option when cfn template increases in size beyond the limit

@mariaines
Copy link

Kinda hacky workaround: specify 'Retain' for your DeletionPolicy, then use a custom resource to delete the resource(s) on stack deletion when desired. This avoids duplication and works without any transforms.

E.g.:

  Table:
    Type: AWS::DynamoDB::Table
    DeletionPolicy: Retain
    ...
  CleanupTableOnStackDelete:
    Type: AWS::Serverless::Function
    Condition: ShouldDeleteTableOnStackDelete
    Properties:
      InlineCode:
        !Sub |
          import json, boto3, logging, uuid
          import cfnresponse
          logger = logging.getLogger()
          logger.setLevel(logging.INFO)

          def lambda_handler(event, context):
              logger.info("event: {}".format(event))
              try:
                  tableName = event['ResourceProperties']['TableName']
                  logger.info("tableName: {}, event['RequestType']: {}".format(tableName,event['RequestType']))
                  if event['RequestType'] == 'Delete':
                      dynamodb = boto3.client('dynamodb')
                      dynamodb.delete_table(TableName=tableName)

                  sendResponseCfn(event, context, cfnresponse.SUCCESS)
              except Exception as e:
                  logger.info("Exception: {}".format(e))
                  sendResponseCfn(event, context, cfnresponse.FAILED)

          def sendResponseCfn(event, context, responseStatus):
              responseData = {}
              responseData['Data'] = {}
              physicalResourceId = event.get('PhysicalResourceId', str(uuid.uuid4()))
              cfnresponse.send(event, context, responseStatus, responseData, physicalResourceId)

      Handler: "index.lambda_handler"
      Runtime: python3.8
      MemorySize: 128
      Timeout: 60
      Role: "arn:aws:iam::...:role/DeleteTestTableRole"
  DoCleanupTableOnStackDelete:
    Type: Custom::CleanupTableOnStackDelete
    Condition: ShouldDeleteTableOnStackDelete
    Properties:
      ServiceToken: !GetAtt CleanupTableOnStackDelete.Arn
      TableName: !Ref Table
    DependsOn:
      - Table
      - CleanupTableOnStackDelete

@sairoko12
Copy link

@mariaines Your approach is very helpful 🥇 , but I feel this is very complex for future maintenance because the behavior depends on the other resource that is the lambda, and maybe this produces a little confuses between other developers 😬 but right now is a good option

@bdrx312
Copy link

bdrx312 commented Sep 2, 2021

I have a hard time imagining how the back end code for cloudformation could have been written that makes this such a difficult feature (bug fix in my opinion) to implement. This is one of the oldest feature requests on the cloudformation coverage roadmap with one of the top number of thumbs up, yet there has been no response from AWS to even acknowledge that they are looking at it and give an idea of why it can't be implemented in any kind of reasonable time period.

@andrewlytle
Copy link

Please get this fixed, this causes a huge amount of wasted effort and time.

@bogdantimes
Copy link

bogdantimes commented Dec 17, 2021

Agreed, this has to be addressed ASAP by the AWS CloudFormation team... c'mon

@wesleymooiman
Copy link

Still waiting on this....

@uncledru-veho
Copy link

Would love some sort of response from AWS/CloudFormation team!

@lejiati
Copy link
Contributor

lejiati commented May 10, 2022

@jthomerson Thank you very much for your feedback! Since this repository is focused on resource coverage, I'm transferring this issue over to a new GitHub repository dedicated to CloudFormation template language issues.

FYI, We have an RFC #74 proposed that allows intrinsic function inDeletionPolicy/UpdateReplacePolicy.

@lejiati lejiati transferred this issue from aws-cloudformation/cloudformation-coverage-roadmap May 10, 2022
@muneebar muneebar added feature request Proposal for new features or requests and removed enhancement New feature or request labels May 10, 2022
@lejiati
Copy link
Contributor

lejiati commented Aug 25, 2022

Hi all, really appreciate all the feedback and I'm happy to share that intrinsic functions such as Fn::If is supported now with the launch of AWS::LanguageExtensions transform. Please feel free to try it out and leave you feedback here.

@MalikAtalla-AWS
Copy link
Contributor

This functionality is now available, so we can close this issue.

@lukepafford
Copy link

lukepafford commented Aug 31, 2022

Hopefully this issue is just isolated to my environment. Let me know if anyone else experiences this problem with the new AWS::LanguageExtensions Transform.

Potential Bug : When combining the transform with the AWS::Serverless-2016-10-31 transform, only the first transform in the list is applied.


AWS::Serverless-2016-10-31 listed first:

AWSTemplateFormatVersion: '2010-09-09'
Transform:
- AWS::Serverless-2016-10-31
- AWS::LanguageExtensions

Produces the error:

Creating ChangeSet-2022-08-31T20-19-32Z in --- failed
"StatusReason": "Transform AWS::Serverless-2016-10-31 failed with: Invalid Serverless Application Specification document. Number of errors found: 1. Structure of the SAM template is invalid. Every DeletionPolicy member must be a string."

This error occurs when creating the ChangeSet, indicating that the AWS::LanguageExtensions transform is being ignored.


AWS::LanguageExtensions listed first:

AWSTemplateFormatVersion: '2010-09-09'
Transform:
- AWS::LanguageExtensions
- AWS::Serverless-2016-10-31

Produces the error:

Policy statement must contain actions. (Service: AmazonIdentityManagement; Status Code: 400; Error Code: MalformedPolicyDocument; Request ID: 9dcda1f8-0cbc-472d-891c-5e72378b500e; Proxy: null)

This error is referring to a role that previously deployed successfully, which appears to indicate that the AWS::Serverless-2016-10-31 transform is being ignored.


If others can confirm that the AWS::LanguageExtensions transform plays nice with AWS::Serverless-2016-10-31 then that will be helpful


Minimal Template Which Reproduces Bug

Sorry for being lazy, here's a functional template which displays the issue:

---
# Deployed with
# aws cloudformation deploy --template-file ~/cloudformation-language-extension-with-serverless-test.yaml --stack "LanguageExtensionsTest" --capabilities CAPABILITY_IAM

# The following template deploys successfully AS IS.
# However if you uncomment the `AWS::LanguageExtensions` transform, and the corresponding `TestLogGroup` resource, then the 
# LambdaRole resource fails to deploy with the error:
# > Policy statement must contain actions. (Service: AmazonIdentityManagement; Status Code: 400; Error Code: MalformedPolicyDocument; Request ID: 08df462c-2472-4b76-91a0-87d293c285ee; Proxy: null)

AWSTemplateFormatVersion: 2010-09-09
Transform:
# - 'AWS::LanguageExtensions'
- 'AWS::Serverless-2016-10-31'
Parameters:
  Stage:
    Type: String
    AllowedValues:
      - Prod
      - Staging
      - Dev
    Default: Dev
  RunLambdaInVPC:
    Type: String
    Default: false
    AllowedValues: [true, false]
Conditions:
  IsProd: !Equals 
    - !Ref Stage
    - Prod
  RunLambdaInVPC: !Equals
    - !Ref RunLambdaInVPC
    - true
Resources:
  # TestLogGroup:
  #   Type: "AWS::Logs::LogGroup"
  #   DeletionPolicy: !If [IsProd, Retain, Delete]
  #   Properties:
  #     LogGroupName: "TestLogGroup"
  LambdaRole:
    Type: AWS::IAM::Role
    Properties:
      AssumeRolePolicyDocument:
        Statement:
        - Action: ['sts:AssumeRole']
          Effect: Allow
          Principal:
            Service: [lambda.amazonaws.com]
        Version: '2012-10-17'
      Policies:
      - PolicyDocument:
          Statement:
          - Action: ['cloudwatch:*', 'logs:*']
            Effect: Allow
            Resource: '*'
          - Fn::If:
            - RunLambdaInVPC
            - Action: ['ec2:CreateNetworkInterface', 'ec2:DescribeNetworkInterfaces', 'ec2:DeleteNetworkInterface']
              Effect: Allow
              Resource: '*'
            - {Ref: 'AWS::NoValue'}
          Version: '2012-10-17'
        PolicyName: lambdaRole

@lejiati
Copy link
Contributor

lejiati commented Sep 8, 2022

Hey @lukepafford , thank you for submitting this issue! The issue should be fixed now and please give it another try and let us know if you still have any questions.

@jlhood
Copy link
Contributor

jlhood commented Sep 8, 2022

@lukepafford Note, you must put the AWS::LanguageExtensions transform before the AWS::Serverless transform in the list. We are working on updating our documentation to indicate this ordering.

@benkehoe
Copy link

benkehoe commented Sep 8, 2022

If this is known for AWS transforms, does it make sense for CloudFormation to even accept a template with the transforms out of order?

@jlhood
Copy link
Contributor

jlhood commented Sep 8, 2022

@benkehoe Thanks for the feedback. We've had internal discussions on how to help guide customers here. First step is documentation. We're also planning to add a rule to cfn-lint to check for this. We'll discuss the possibility of a server-side check.

@benkehoe
Copy link

benkehoe commented Sep 8, 2022

ooh, adding it to cfn-lint is a great idea!

@lukepafford
Copy link

@jlhood @lejiati
I confirmed that my fully uncommented minimal template successfully deploys from both the UPDATE_ROLLBACK_COMPLETE state, and from a fresh deploy.

Thanks for the quick turnaround!

Screen Shot 2022-09-09 at 12 49 26 AM

Screen Shot 2022-09-09 at 12 49 51 AM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Proposal for new features or requests
Projects
None yet
Development

No branches or pull requests