Skip to content

Commit

Permalink
fix(stack): Stack tags are separate from other tags (under feature flag)
Browse files Browse the repository at this point in the history
Stacks are considered taggable, and so `Tags.of(this).add('key',
'value')` used to add tags to Stacks in scope. Usually this happens if
`this` is an instance of `Stack`, which it commonly is in user code.

Since `Tags.of(...)` walks the construct tree, it will add tags to the
stack *and* to all the resources in the stack. Then, come deploy time,
CloudFormation will also try and apply all the stack tags to the
resources again. This is silly and unnecessary.

In #28017, someone tries to use a CloudFormation intrisinc in a tag
applied using `Tags.of(this)`; that will work for resources as the tags
are rendered into the template, but it will not work for the stack
tags as those are passed via an API call, and intrinsics don't work
there.

IN THIS CHANGE

The *correct* solution to tagging all resources with an intrinsic
would be to tag each of them individually, as tagging a Stack
with an intrinsic is not possible. That's a poor user experience.

Resolve both the silly duplicate work as well as the "tagging with
an intrinsic" use case as follows:

- Stacks no longer participate in the hierarchical `Tags.of(...)`
  tagging.
- Instead, only tags explicitly applied at the stack level (using
  the `tags` constructor property) are renderd as stack tags.

This requires a user to make a conscious decision between resource-level
and stack-level tagging: either apply tags to the stack, which will
apply it to all resources; or apply tags to (groups of) resources inside
the template.
  • Loading branch information
rix0rrr committed Sep 13, 2024
1 parent 4392ab4 commit a91bdee
Show file tree
Hide file tree
Showing 5 changed files with 261 additions and 15 deletions.
17 changes: 14 additions & 3 deletions packages/aws-cdk-lib/core/lib/stack-synthesizers/_shared.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { Node, IConstruct } from 'constructs';
import { ISynthesisSession } from './types';
import * as cxschema from '../../../cloud-assembly-schema';
import { Stack } from '../stack';
import { Token } from '../token';

/**
* Shared logic of writing stack artifact to the Cloud Assembly
Expand All @@ -20,10 +21,20 @@ export function addStackArtifactToAssembly(
stackProps: Partial<cxschema.AwsCloudFormationStackProperties>,
additionalStackDependencies: string[]) {

const stackTags = stack.stackTags;

// nested stack tags are applied at the AWS::CloudFormation::Stack resource
// level and are not needed in the cloud assembly.
if (stack.tags.hasTags()) {
stack.node.addMetadata(cxschema.ArtifactMetadataEntryType.STACK_TAGS, stack.tags.renderTags());
if (Object.entries(stackTags).length > 0) {
stack.node.addMetadata(
cxschema.ArtifactMetadataEntryType.STACK_TAGS,
Object.entries(stackTags).map(([key, value]) => ({ Key: key, Value: value })));

for (const [k, v] of Object.entries(stackTags)) {
if (Token.isUnresolved(k) || Token.isUnresolved(v)) {
throw new Error(`Stack tags may not contain deploy-time values (tag: ${k}=${v}). Apply tags like this to resources inside the template instead.`);
}
}
}

const deps = [
Expand All @@ -46,7 +57,7 @@ export function addStackArtifactToAssembly(
const properties: cxschema.AwsCloudFormationStackProperties = {
templateFile: stack.templateFile,
terminationProtection: stack.terminationProtection,
tags: nonEmptyDict(stack.tags.tagValues()),
tags: nonEmptyDict(stackTags),
validateOnSynth: session.validateOnSynth,
...stackProps,
...stackNameProperty,
Expand Down
60 changes: 59 additions & 1 deletion packages/aws-cdk-lib/core/lib/stack.ts
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,15 @@ export interface StackProps {
readonly stackName?: string;

/**
* Stack tags that will be applied to all the taggable resources and the stack itself.
* Stack tags that will be applied to the stack
*
* The behavior of this property depends on the `@aws-cdk/core:explicitStackTags` feature
* flag:
*
* - If unset, tags are applied to all resources in the stack by CDK (changing
* the template).
* - If set, tags are applied to all resources in the stack by CloudFormation (the
* template will not contain them).
*
* @default {}
*/
Expand Down Expand Up @@ -244,6 +252,12 @@ export class Stack extends Construct implements ITaggable {

/**
* Tags to be applied to the stack.
*
* The behavior of the TagManager, and tags applied using `Tags.of()` in
* general, depends on the `@aws-cdk/core:explicitStackTags` feature flag.
*
* If `@aws-cdk/core:explicitStackTags` is set, tags set on this tag manager
* are ignored.
*/
public readonly tags: TagManager;

Expand Down Expand Up @@ -395,6 +409,16 @@ export class Stack extends Construct implements ITaggable {
*/
private readonly _suppressTemplateIndentation: boolean;

/**
* The value of the "explicit stack tags" feature flag.
*/
private readonly _explicitStackTags: boolean;

/**
* A copy of the stack tags
*/
private readonly _stackTags: Record<string, string> = {};

private _terminationProtection: boolean;

/**
Expand Down Expand Up @@ -424,6 +448,7 @@ export class Stack extends Construct implements ITaggable {
this.templateOptions = { };
this._crossRegionReferences = !!props.crossRegionReferences;
this._suppressTemplateIndentation = props.suppressTemplateIndentation ?? this.node.tryGetContext(SUPPRESS_TEMPLATE_INDENTATION_CONTEXT) ?? false;
this._explicitStackTags = FeatureFlags.of(this).isEnabled(cxapi.EXPLICIT_STACK_TAGS) ?? false;

Object.defineProperty(this, STACK_SYMBOL, { value: true });

Expand All @@ -449,6 +474,7 @@ export class Stack extends Construct implements ITaggable {
if (this._stackName.length > 128) {
throw new Error(`Stack name must be <= 128 characters. Stack name: '${this._stackName}'`);
}
this._stackTags = { ...props.tags };
this.tags = new TagManager(TagType.KEY_VALUE, 'aws:cdk:stack', props.tags);

if (!VALID_STACK_NAME_REGEX.test(this.stackName)) {
Expand Down Expand Up @@ -1565,6 +1591,38 @@ export class Stack extends Construct implements ITaggable {
pattern,
));
}

/**
* Returns the stack tags
*
* These tags are applied to the stack, and CloudFormation will apply
* them to all resources in the stack.
*/
public get stackTags(): Record<string, string> {
if (this._explicitStackTags) {
// New behavior, only return the explicit stack tags
return { ...this._stackTags };
} else {
// Old behavior, return the accumulated tags from the TagManager
return this.tags.tagValues();
}
}

/**
* Configure a stack tag
*/
public addStackTag(tagName: string, tagValue: string) {
this._stackTags[tagName] = tagValue;
this.tags.setTag(tagName, tagValue);
}

/**
* Remove a stack tag
*/
public removeStackTag(tagName: string) {
delete this._stackTags[tagName];
this.tags.removeTag(tagName, 0);
}
}

function merge(template: any, fragment: any): void {
Expand Down
129 changes: 129 additions & 0 deletions packages/aws-cdk-lib/core/test/stack.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@ import {
PERMISSIONS_BOUNDARY_CONTEXT_KEY,
Aspects,
Stage,
TagManager,
Resource,
TagType,
ITaggable,
ITaggableV2,
} from '../lib';
import { Intrinsic } from '../lib/private/intrinsic';
import { resolveReferences } from '../lib/private/refs';
Expand Down Expand Up @@ -2075,6 +2080,114 @@ describe('stack', () => {
expect(asm.getStackArtifact(stack2.artifactId).tags).toEqual(expected);
});

test.each([false, true])('stack tags added in constructor are in metadata and artifact properties (ussing feature flag: %p)', (explicitStackTags) => {
// GIVEN
const app = new App({
stackTraces: false,
context: {
[cxapi.NEW_STYLE_STACK_SYNTHESIS_CONTEXT]: false,
[cxapi.EXPLICIT_STACK_TAGS]: explicitStackTags,
},
});

const stack = new Stack(app, 'stack1', {
tags: {
foo: 'bar',
},
});

// THEN
const asm = app.synth();

const stackArtifact = asm.getStackArtifact(stack.artifactId);
expect(stackArtifact.manifest.metadata).toEqual({
'/stack1': [
{
type: 'aws:cdk:stack-tags',
data: [{ key: 'foo', value: 'bar' }],
},
],
});
expect(stackArtifact.tags).toEqual({ foo: 'bar' });
});

test('stack tags are not applied to resources', () => {
// GIVEN
const app = new App({
stackTraces: false,
context: {
[cxapi.NEW_STYLE_STACK_SYNTHESIS_CONTEXT]: false,
[cxapi.EXPLICIT_STACK_TAGS]: true,
},
});

const stack = new Stack(app, 'stack1', {
tags: {
foo: 'bar',
},
});
new TaggableResource(stack, 'res');

// THEN
const asm = app.synth();
const stackArtifact = asm.getStackArtifact(stack.artifactId);
expect(stackArtifact.template.Resources.res).toEqual({
Type: 'AWS::Taggable::Resource',
Properties: {
R: 1,
},
});
});

test('with explicitStackTags enabled, tags added using Tags.of() are only applied to resources', () => {
// GIVEN
const app = new App({
stackTraces: false,
context: {
[cxapi.NEW_STYLE_STACK_SYNTHESIS_CONTEXT]: false,
[cxapi.EXPLICIT_STACK_TAGS]: true,
},
});

const stack = new Stack(app, 'stack1', {
tags: {
foo: 'bar',
},
});
new TaggableResource(stack, 'res');
Tags.of(stack).add('resourceTag', 'resourceValue');

// THEN
const asm = app.synth();
const stackArtifact = asm.getStackArtifact(stack.artifactId);
expect(stackArtifact.template.Resources.res).toEqual({
Type: 'AWS::Taggable::Resource',
Properties: {
R: 1,
Tags: [
{ Key: 'resourceTag', Value: 'resourceValue' },
],
},
});
// resourceTag tag is not added to stack tags
expect(stackArtifact.tags).toEqual({ foo: 'bar' });
});

test('stack tags may not contain tokens', () => {
// GIVEN
const app = new App({
stackTraces: false,
});

const stack = new Stack(app, 'stack1', {
tags: {
foo: Lazy.string({ produce: () => 'lazy' }),
},
});

expect(() => app.synth()).toThrow(/Stack tags may not contain deploy-time values/);
});

test('Termination Protection is reflected in Cloud Assembly artifact', () => {
// if the root is an app, invoke "synth" to avoid double synthesis
const app = new App();
Expand Down Expand Up @@ -2428,3 +2541,19 @@ class StackWithPostProcessor extends Stack {
return template;
}
}

class TaggableResource extends CfnResource implements ITaggableV2 {
public readonly cdkTagManager = new TagManager(TagType.KEY_VALUE, 'TaggableResource', {}, {
tagPropertyName: 'Tags',
});

constructor(scope: Construct, id: string) {
super(scope, id, {
type: 'AWS::Taggable::Resource',
properties: {
R: 1,
Tags: Lazy.any({ produce: () => this.cdkTagManager.renderTags() }),
},
});
}
}
34 changes: 30 additions & 4 deletions packages/aws-cdk-lib/cx-api/FEATURE_FLAGS.md
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ Flags come in three types:
| [@aws-cdk/custom-resources:logApiResponseDataPropertyTrueDefault](#aws-cdkcustom-resourceslogapiresponsedatapropertytruedefault) | When enabled, the custom resource used for `AwsCustomResource` will configure the `logApiResponseData` property as true by default | 2.145.0 | (fix) |
| [@aws-cdk/aws-s3:keepNotificationInImportedBucket](#aws-cdkaws-s3keepnotificationinimportedbucket) | When enabled, Adding notifications to a bucket in the current stack will not remove notification from imported stack. | 2.155.0 | (fix) |
| [@aws-cdk/aws-stepfunctions-tasks:useNewS3UriParametersForBedrockInvokeModelTask](#aws-cdkaws-stepfunctions-tasksusenews3uriparametersforbedrockinvokemodeltask) | When enabled, use new props for S3 URI field in task definition of state machine for bedrock invoke model. | 2.156.0 | (fix) |
| [@aws-cdk/core:explicitStackTags](#aws-cdkcoreexplicitstacktags) | When enabled, stack tags need to be assigned explicitly on a Stack. | V2NEXT | (default) |

<!-- END table -->

Expand Down Expand Up @@ -134,7 +135,8 @@ The following json shows the current recommended set of flags, as `cdk init` wou
"@aws-cdk/aws-ec2:ebsDefaultGp3Volume": true,
"@aws-cdk/aws-ecs:removeDefaultDeploymentAlarm": true,
"@aws-cdk/custom-resources:logApiResponseDataPropertyTrueDefault": false,
"@aws-cdk/aws-s3:keepNotificationInImportedBucket": false
"@aws-cdk/aws-s3:keepNotificationInImportedBucket": false,
"@aws-cdk/core:explicitStackTags": true
}
}
```
Expand Down Expand Up @@ -1131,7 +1133,7 @@ shipped as part of the runtime environment.

*When enabled, will always use the arn for identifiers for CfnSourceApiAssociation in the GraphqlApi construct rather than id.* (fix)

When this feature flag is enabled, we use the IGraphqlApi ARN rather than ID when creating or updating CfnSourceApiAssociation in
When this feature flag is enabled, we use the IGraphqlApi ARN rather than ID when creating or updating CfnSourceApiAssociation in
the GraphqlApi construct. Using the ARN allows the association to support an association with a source api or merged api in another account.
Note that for existing source api associations created with this flag disabled, enabling the flag will lead to a resource replacement.

Expand Down Expand Up @@ -1188,7 +1190,7 @@ database cluster from a snapshot.

*When enabled, the CodeCommit source action is using the default branch name 'main'.* (fix)

When setting up a CodeCommit source action for the source stage of a pipeline, please note that the
When setting up a CodeCommit source action for the source stage of a pipeline, please note that the
default branch is 'master'.
However, with the activation of this feature flag, the default branch is updated to 'main'.

Expand Down Expand Up @@ -1366,7 +1368,7 @@ Other notifications that are not managed by this stack will be kept.
Currently, 'inputPath' and 'outputPath' from the TaskStateBase Props is being used under BedrockInvokeModelProps to define S3URI under 'input' and 'output' fields
of State Machine Task definition.

When this feature flag is enabled, specify newly introduced props 's3InputUri' and
When this feature flag is enabled, specify newly introduced props 's3InputUri' and
's3OutputUri' to populate S3 uri under input and output fields in state machine task definition for Bedrock invoke model.


Expand All @@ -1378,4 +1380,28 @@ When this feature flag is enabled, specify newly introduced props 's3InputUri' a
**Compatibility with old behavior:** Disable the feature flag to use input and output path fields for s3 URI


### @aws-cdk/core:explicitStackTags

*When enabled, stack tags need to be assigned explicitly on a Stack.* (default)

Without this feature flag enabled, if tags are added to a Stack using
`Tags.of(scope).add(...)`, they will be added to both the stack and all resources
in the Stack.

With this flag enabled, tags added to a stack using `Tags.of(...)` are ignored,
and Stack tags must be configured explicitly on the Stack object.

Tags configured on the Stack will be propagated to all resources automatically
by CloudFormation, so there is no need for the automatic propagation that
`Tags.of(...)` does.


| Since | Default | Recommended |
| ----- | ----- | ----- |
| (not in v1) | | |
| V2NEXT | `false` | `true` |

**Compatibility with old behavior:** Configure stack-level tags using `new Stack(..., { tags: { ... } })`.


<!-- END details -->
Loading

0 comments on commit a91bdee

Please sign in to comment.