feat(cli): change set name is now a constant, and --no-execute will always produce one (even if empty)

[swar8080]

closes #11075

Adds two commands to the deploy CLI command to make it easier to externally execute change sets when using the --no-execute flag:

--change-set-name: Optional name of the CloudFormation change set to create, instead of using a random one. An external script or the CodePipeline CloudFormation action can use this name to later deploy the changes.

--retain-empty-change-set: Optionally retain empty change sets instead of deleting them. This is useful for the (requested) CodePipeline use case, since the CodePipeline CloudFormation action always requires a change set, even if it's empty.

Questions for reviewer:

• Is --retain-empty-change-set needed? One workaround for the CodePipeline use case could be for users to write a lambda that generates the required empty change set whenever CDK doesn't generate one. Another idea would be to automatically retain change sets when using --no-execute to avoid this extra CLI flag, but this would be a small change in behavior.
• Are the new integration tests overkill? Also should unit tests be added or in-place of the integration tests?

[gitpod-io]

[swar8080]

This is piggy-backing off the existing integration test for the --no-execute-flag by also testing the use of a custom name for the change set. I think it's a safe change because it's not changing anything originally being tested, and at code level specifying a name shouldn't really change the previous behavior.

[rix0rrr]

Honestly -- I would be okay with drastically simplifying the behavior of the CLI. The things you are asking for seem like good default behavior.

• Let's just fix the name of the ChangeSet to be something like cdk-deploy-changeset. I don't think there's value in having a UUID as the name (but please correct me if I'm wrong). Is it even possible/feasible to have two independent change sets on the same stack anyway? It seems to me they would have to be serialized in any case, no? (git rebase, in essence)
• The behavior of not deleting an empty changeset in case of --no-execute seems like good default behavior. It seems weird to me to explicitly ask a tool to create a change set, and then have it not exist after the tool finishes.

The only added complication would be that we would potentially need to delete/replace an existing change set with the name cdk-deploy-changeset, but I don't see an issue with that.

Bottom line: let's do away with the flags and make your desired behavior the default. Yes, it's a behavior change, but since we are strengthening postconditions, all tools that potentially exist that currently wrap the CDK CLI and know how to deal with change sets with variable names that might or might not exist, will also successfully deal with a change set with a fixed name that always exists.

[swar8080]

@rix0rrr

The behavior of not deleting an empty changeset in case of --no-execute seems like good default behavior. It seems weird to me to explicitly ask a tool to create a change set, and then have it not exist after the tool finishes.

Okay sounds good, i'll make that change. The point about the impact on postconditions is interesting to think about for these kind of changes.

Let's just fix the name of the ChangeSet to be something like cdk-deploy-changeset. I don't think there's value in having a UUID as the name (but please correct me if I'm wrong). Is it even possible/feasible to have two independent change sets on the same stack anyway? It seems to me they would have to be serialized in any case, no? (git rebase, in essence)

The UUID was added because multiple change sets can exist at a time as long as their names are unique, which is currently possible by running cdk deploy --no-execute multiple times. However, only one change set can be executed at a time, which causes all the other change sets to be deleted.

The only added complication would be that we would potentially need to delete/replace an existing change set with the name cdk-deploy-changeset, but I don't see an issue with that.

Let me know if the above changes your opinion on deleting the previous change set, since the CloudFormation documentation mentions: "Creating multiple change sets helps you understand and evaluate how different changes will affect your resources. You can create as many change sets as you need."

Pull request has been modified.

[rix0rrr]

The UUID was added because multiple change sets can exist at a time as long as their names are unique, which is currently possible by running cdk deploy --no-execute multiple times. However, only one change set can be executed at a time, which causes all the other change sets to be deleted.

Right. I believe that's true, but what's the point? Do people actually do this in practice? I'd argue no.

So I'd be fully happy with making sure a changeset with a constant name is created, replacing a previous one with the same name (if it exists).

We can still add --change-set-name if you feel like you REALLY REALLY want to create 5 different changesets and pick your favorite one to execute... but honestly, let's not and wait until someone asks for it. Feels like that kind of workflow would go against how source controlled infra is supposed to work. Which version of those 5 would you commit? I'd hope the one that actually gets executed. But why wouldn't you have commited before deploying already?

(In short--I think this workflow is exceedingly rare, and I'd like to be proven wrong before I decide otherwise)

Pull request has been modified.

[mergify]

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
• Commit ID: cd3e62b
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[mergify]

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[Wenzil]

We can still add --change-set-name if you feel like you REALLY REALLY want to create 5 different changesets and pick your favorite one to execute... but honestly, let's not and wait until someone asks for it.

@rix0rrr We have a security use case for this.

We have the concept of "admin" Change Sets that execute under a very elevated CloudFormation execution role. Admins and normal CDK users both operate on the same stacks using CDK but for security, normal CDK users shouldn't be able to create or execute the "admin" Change Sets because of those elevated permissions.

In other words, we have two groups of users working with two mutually exclusive sets of Change Sets on the same stacks.

We want to enforce this using IAM policies in our custom bootstrap template. Like so:

# AdminDeploymentRole
# ...
- Action: cloudformation:CreateChangeSet
  Resource: '*'
  Effect: Allow
  Condition:
    StringEquals:
      cloudformation:ChangeSetName: "CDK-AdminChangeSet"
      cloudformation:RoleArn:
        Fn::GetAtt: [AdminCloudFormationExecutionRole, Arn] # Elevated role not intended for normal CDK users
- Action:
    - cloudformation:DeleteChangeSet
    - cloudformation:ExecuteChangeSet
  Resource: '*'
  Effect: Allow
  Condition:
    StringEquals:
      cloudformation:ChangeSetName: "CDK-AdminChangeSet"
# ...

# UserDeploymentRole
- Action: cloudformation:CreateChangeSet
  Resource: '*'
  Effect: Allow
  Condition:
    StringEquals:
      cloudformation:ChangeSetName: "CDK-UserChangeSet"
      cloudformation:RoleArn:
        Fn::GetAtt: [UserCloudFormationExecutionRole, Arn]
- Action:
    - cloudformation:DeleteChangeSet
    - cloudformation:ExecuteChangeSet
  Resource: '*'
  Effect: Allow
  Condition:
    StringEquals:
      cloudformation:ChangeSetName: "CDK-UserChangeSet"
# ...

Note: We are using new-style synthesis. At synth time we are able to control which deployment role to assume (restricted by the trust policy). We use a custom synthAsAdmin=true --context flag for that.

We are just missing the ability to instruct CDK to adequately name the Change Sets (for the above policy to work).

Unless I'm missing an alternative way to achieve the above, this feature is a must have for us.