Conversation
github-actions
Bot
added
p2
beginning-contributor
|
PRs without a linked issue will receive lower priority for review and merging. Please update the description to follow the PR template and include a line like |
satyakigh
changed the title
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
8da308b to
517cab1
Compare
mrgrain
left a comment
mrgrain
left a comment
There was a problem hiding this comment.
Hey, since the feature is already stable, the Mixin should go straight into aws-cdk-lib as well. The preview package is being phased out.
aws-cdk-automation
added
the
pr/needs-community-review
GitHub Actions
Inactive
GitHub Actions
Inactive
b362a0a to
b6106b6
Compare
Sounds good, updated |
|
Draft until RFC 972: Structured design context in synthesized templates is approved. |
22f9fba to
f69068b
Compare
|
|
||||||||||||||
|
|
||||||||||||||
|
Automated review A maintainer will still review this — treat the notes below as a starting point. This PR adds a new, purely additive public core surface for authoring CloudFormation's advisory The change is well built and faithful to the published schema — property names and enum values match it, the facade pattern mirrors 🔴 0 blocking · 🟡 2 recommended · ⚪ 3 optional Files with findings (2)
Generated automatically. React 👍 or 👎 to tell us whether this review helped, so we can improve these reviews. |
![maintainer-for-aws[bot]](https://avatars.githubusercontent.com/b/1290?s=60&v=4){kind=small}
There was a problem hiding this comment.
See the review summary comment for the overview; the notes below are inline.
| */ | ||
| readonly applyToDescendants?: boolean; | ||
|
|
||
| /** | ||
| * Apply the context block to every CloudFormation resource in scope, | ||
| * primary and incidental helper resources alike. | ||
| * | ||
| * This is not a "helpers only" selector: it disables the primary-resource | ||
| * filter, so every `CfnResource` beneath the scope receives the block. To | ||
| * reach helpers of a particular kind, combine it with | ||
| * `includeResourceTypes` (e.g. `['AWS::IAM::Role']`), or target an exposed | ||
| * helper construct directly. Implies descendant traversal regardless of | ||
| * `applyToDescendants`. Traversal never crosses a `Stage` assembly | ||
| * boundary. | ||
| * | ||
| * @default false | ||
| */ |
There was a problem hiding this comment.
🟡 Recommended — The three targeting modes this option set supports — self/defaultChild chain only, primary descendants, and all resources including helpers — form a single graduated choice, but they are encoded as two independent booleans (applyToDescendants, applyToAllResources) where one silently implies the other ("Implies descendant traversal regardless of applyToDescendants"). That coupling makes representable-but-meaningless states possible (both true, where the first flag is dead), forces the reader to hold the implication rule in their head, and boxes the API in: a future fourth mode can only arrive as yet another interacting boolean. Multiple related choices like this are meant to be modeled as an enum (DESIGN_GUIDELINES.md#enums). Because this is a brand-new public core surface, the boolean shape cannot be corrected later without a breaking change.
Suggested change: Replace the two booleans with a single enum, e.g. readonly targeting?: MetadataContextTargeting with members DEFAULT_CHILD_CHAIN (default), PRIMARY_DESCENDANTS, and ALL_RESOURCES, so the modes are exclusive by construction and a further mode can be added additively.
|
|
||
| constructor(context: ResourceContextProps) { | ||
| super(); |
There was a problem hiding this comment.
🟡 Recommended — The MetadataContextMixin constructor stores its context without validating it, deferring all checks to applyTo → ResourceMetadataContext.of(construct).add(). A caller who builds new MetadataContextMixin({ trust: { ... } }) with a missing trust source/confidence, or a propertyMutability entry that repeats defaultMutability, gets no feedback at construction — the error surfaces only at apply/synth time, further from the offending call and after the mixin may have been passed around or applied in bulk via Mixins.of(scope).apply(...). Input properties are meant to be validated in the constructor so invalid combinations fail where they are authored (MIXINS_DESIGN_GUIDELINES.md#validation).
Suggested change: Call validateResourceContext(context) from the constructor so invalid inputs fail at the point they are authored, in addition to the validation still performed by add().
| } | ||
|
|
||
| if (applicableEntries.some((entry) => !entry.options.inheritAncestorContext)) { | ||
| // Opt out of inherited ancestor context once before processing this | ||
| // scope, preserving all declarations made on the scope itself. | ||
| merged = undefined; | ||
| } | ||
| for (const staged of applicableEntries) { | ||
| merged = mergeResourceContext(merged, renderResourceContext(staged.context)); | ||
| } | ||
| } | ||
|
|
||
| if (merged === undefined || Object.keys(merged).length === 0) { | ||
| return; | ||
| } | ||
|
|
||
| setResourceMetadataContext(node, merged); | ||
| } | ||
|
|
||
| private applies(resource: CfnResource, appliedScope: IConstruct, staged: StagedEntry): boolean { | ||
| const include = staged.options.includeResourceTypes; | ||
| if (include && include.length > 0 && !include.includes(resource.cfnResourceType)) { | ||
| return false; | ||
| } | ||
| const exclude = staged.options.excludeResourceTypes; | ||
| if (exclude && exclude.length > 0 && exclude.includes(resource.cfnResourceType)) { | ||
| return false; | ||
| } | ||
| if (staged.options.applyToAllResources) { | ||
| // Every resource beneath the scope, helpers included. | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🟡 Recommended — The Stage assembly boundary is a deliberately engineered branch of the traversal that no test exercises. MetadataContextAspect.visit() computes an assemblyRootIndex from the nearest marked Stage so a declaration made above the stage is intentionally excluded, and both isOnDefaultChildChain and isPrimaryDescendant short-circuit at a Stage boundary; the zero-match error message even tells users to "declare context inside each Stage." The unit suite imports Stage but never constructs one, so neither the exclusion (a declaration above a stage must not cascade into it) nor the inclusion (a declaration inside a stage must apply) is asserted. If this boundary logic regressed, context could leak across a stage boundary and attach the wrong rationale/change-safety block to resources in a different assembly, with no test turning red.
Suggested change: Add two unit tests: (1) a declaration on a scope above a Stage with applyToDescendants asserts it matches nothing inside the stage's resources (and the resources carry no context); (2) a declaration inside a Stage asserts the stage's resources carry it after synth.
| }); | ||
|
|
||
| const aspectOptions: AspectOptions = { priority: options.priority ?? AspectPriority.MUTATING }; | ||
| const aspects = Aspects.of(this.scope); | ||
| if (!aspects.all.some((aspect) => aspect instanceof MetadataContextAspect)) { | ||
| aspects.add(new MetadataContextAspect(), aspectOptions); |
There was a problem hiding this comment.
⚪ Optional — The priority option on add() is forwarded to Aspects.of(scope).add(..., { priority }) (defaulting to AspectPriority.MUTATING) but no test sets a custom priority or asserts ordering relative to another mutating aspect. The harm is low since it only affects aspect ordering and the default path is well covered, but a single ordering test would close the gap.
Suggested change: Add one test that applies the context aspect alongside another mutating aspect at a different priority and asserts the resulting evaluation order.
There was a problem hiding this comment.
See the review summary comment for the overview; the notes below are inline.
| } | ||
| const exclude = staged.options.excludeResourceTypes; | ||
| if (exclude !== undefined && exclude.includes(resource.cfnResourceType)) { | ||
| return false; |
There was a problem hiding this comment.
🟡 Recommended — PropagationFilter ships two public static factories — includeResourceTypes and excludeResourceTypes — and applies() has a distinct branch for each, but across the unit suite, the mixin suite, and both integ tests every propagation-filter case uses includeResourceTypes; the excludeResourceTypes branch is never exercised. If it regressed (inverted condition, wrong list read, or the exclude list silently ignored), no test would fail and a user's excludeResourceTypes([...]) would write context onto resources it was told to skip. Because applies() runs only at synth via the aspect, the branch is not covered even indirectly. The sibling negative-validation branch that throws when propagationFilter is supplied without propagate: true is likewise untested.
Suggested change: Add a test mirroring the existing include case that applies excludeResourceTypes(['AWS::IAM::Role']) with propagate: true and asserts the excluded resource has no Context block while a non-excluded resource does; and a one-line test asserting add(props, { propagationFilter: ... }) without propagate: true throws.
…and aspect + mixins to set context on a resource and template
…ib core The mixins-preview package is being phased out and the feature is stable, so the Mixin ships directly in aws-cdk-lib alongside the MetadataContext facade (review feedback). - MetadataContextMixin now lives in core/lib/mixins/ and is exported flat from aws-cdk-lib (a top-level 'mixins' jsii submodule is not possible: JSII5011 name conflict with the Mixins class), with an awslint exclusion for the mixin-namespace rule. - Unit test moved to core/test/mixins/, adapted to core's toCloudFormation convention. - Integ test moved to @aws-cdk-testing/framework-integ test/core/test/ with regenerated snapshot. - aws-cdk-lib README documents the mixin inline; all mixins-preview changes reverted.
There was a problem hiding this comment.
See the review summary comment for the overview; the notes below are inline.
| Transform: transform, | ||
| AWSTemplateFormatVersion: this.templateOptions.templateFormatVersion, | ||
| Metadata: this.templateOptions.metadata, | ||
| Metadata: renderTemplateMetadata(this, this.templateOptions.metadata), |
There was a problem hiding this comment.
⚪ Optional — For a stack where a user has explicitly set templateOptions.metadata = {} (an empty object), the previous code assigned Metadata: {} into the template and resolve() preserved it (it strips undefined, not empty objects), so the synthesized template carried an empty Metadata: {}. The new render path returns undefined when the merged result has no keys, so that empty object is now dropped. The change is negligible and arguably more correct, but it is a subtle synthesized-template delta for that specific input.
Suggested change: If strict transparency for the empty-metadata path is desired, short-circuit the render helper to return the original metadata reference unchanged when no API-produced context is present, e.g. if (contextFromApi === undefined) return metadata;.
| export class PropagationFilter { | ||
| /** | ||
| * Only resources whose CloudFormation type is in `resourceTypes` receive the | ||
| * context (e.g. `['AWS::SQS::Queue']`). | ||
| */ | ||
| public static includeResourceTypes(resourceTypes: string[]): PropagationFilter { | ||
| return new PropagationFilter({ includeResourceTypes: [...resourceTypes] }); | ||
| } | ||
|
|
||
| /** | ||
| * Every resource except those whose CloudFormation type is in | ||
| * `resourceTypes` receives the context (e.g. `['AWS::IAM::Role']`). | ||
| */ | ||
| public static excludeResourceTypes(resourceTypes: string[]): PropagationFilter { | ||
| return new PropagationFilter({ excludeResourceTypes: [...resourceTypes] }); | ||
| } | ||
|
|
||
| private constructor(private readonly spec: PropagationFilterSpec) { | ||
| } | ||
|
|
||
| /** | ||
| * The JSON-serializable form of this filter. | ||
| * | ||
| * @internal | ||
| */ | ||
| public _toSpec(): PropagationFilterSpec { |
There was a problem hiding this comment.
⚪ Optional — Resource-type filtering here diverges in shape from the closest established core facade. RemovalPolicies.of(scope).retain({ applyToResourceTypes: [...], excludeResourceTypes: [...] }) carries the include/exclude lists as inline props on the options object, whereas this PR introduces a dedicated PropagationFilter helper class with includeResourceTypes()/excludeResourceTypes() static factories passed as options.propagationFilter. A developer who already knows RemovalPolicies meets a second core facade doing conceptually the same resource-type scoping under a different name and structural shape. The harm is low and the two-mode targeting arguably justifies a separate object, but it is a small consistency tax across sibling APIs a reader expects to rhyme (DESIGN_GUIDELINES.md#tags).
Suggested change: Consider making the include/exclude lists inline props on ResourceMetadataContextOptions (guarded by propagate: true), matching RemovalPolicyProps, e.g. add(context, { propagate: true, applyToResourceTypes: ['AWS::SQS::Queue'] }). If the helper class is retained, aligning the method name with the existing applyToResourceTypes vocabulary would still reduce the divergence.
This branch was successfully deployed
5 participants
Reason for this change
CloudFormation templates capture what infrastructure exists but not why: rationale, invariants and change-safety live in comments, wikis and people's heads, and are gone by the time someone (or an agent) modifies the deployed template. CloudFormation publishes an advisory Metadata Context schema under the
com.aws.cloudformation.Contextmetadata key for exactly this, but CDK users have no way to author it. Most production templates are CDK-synthesized, so CDK needs a first-class authoring surface.Description of changes
Two facades in
aws-cdk-libcore, following theTags/RemovalPoliciespattern. Property names are the schema's field names, so code and template use one vocabulary.ResourceMetadataContext.of(scope).add(props, options?)(aspect-backed) — resource-level context (why,must,mutable, sparsemutabilitymap,trust { src, conf, cite, note },deps). By default targets the scope's primary resource: the scope itself when it is aCfnResource, otherwise theCfnResourceat the end of itsdefaultChildchain, skipping helper resources. Options:propagate: true— target everyCfnResourcebeneath the scope instead (crossesNestedStack, neverStage).propagationFilter: PropagationFilter.includeResourceTypes([...])/.excludeResourceTypes([...])— narrows propagation by type; requirespropagate: true.inheritAncestorContext: false— discard context merged from ancestor scopes.priority— aspect priority (defaultAspectPriority.MUTATING).A declaration that matches no resource fails synthesis. Declarations merge ancestor-to-resource: closest wins
why/mutable/trust;must/depsunion and de-duplicate;mutabilitymerges per property.TemplateMetadataContext.of(stack).add(props)— template-level context (arch,must,ref,owner) written once to the stack's top-levelMetadata. Repeated calls merge (laterarch/ownerwin;must/refaccumulate).Public types: enums
ContextMutability,ContextTrustSource(AUTHORED | COMMENT | COMMIT | INFER),ContextTrustConfidence; structsResourceContextProps,TemplateContextProps,ContextTrust,ContextRef,ResourceMetadataContextOptions; classesPropagationFilter,ResourceMetadataContext,TemplateMetadataContext.Files:
core/lib/metadata-context.ts(public API, aspect, targeting),core/lib/private/metadata-context-internal.ts(schema rendering, merge, validation),core/lib/private/metadata-context-metadata.ts(writes the block intoCfnResource/Stackmetadata and detects collisions; wired in via one-line seams incfn-resource.tsandstack.ts).Validation:
trustrequiressrcandconf; amutabilityentry may not repeatmutable; arefentry requiresat. CDK never addstrustautomatically. A manually addedcom.aws.cloudformation.Contextblock colliding with an API-produced one fails synthesis with aValidationErrorrather than being overwritten or merged. Templates that do not use the API render exactly as before.Docs: new "Metadata Context" section in the
aws-cdk-libREADME.Describe any new or updated permissions being added
None. The change writes only to template
Metadataat synthesis time; no IAM policies, roles, grants, trust relationships or resource policies are created or modified, and no runtime AWS calls are made.Description of how you validated changes
core/test/metadata-context.test.ts: rendering under the schema's field names, default targeting through multi-hopdefaultChildchains,propagatewith include and exclude filters, rejection of a filter withoutpropagate, zero-match failures,NestedStackandStageboundaries, merge andinheritAncestorContextsemantics, trust/mutability/ref validation, manual-metadata collision, and a frozen-enum drift check against the published schema. All assertions run against synthesized templates.integ.metadata-contextin@aws-cdk-testing/framework-integwith snapshot.jsii-rosetta extract --compilepasses for all README examples.Checklist
RFC aws/aws-cdk-rfcs#981
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license