chore(cli): statically guarantee that assets aren't forgotten around makeBodyParameter #31750

rix0rrr · 2024-10-14T11:04:47Z

The protocol of makeBodyParameter has 2 requirements:

Stack assets must already be uploaded (because the stack template will be among them).
It may produce a new stack template asset to be uploaded.

This protocol was easy to misuse, so change the API to make it more resilient to misuse:

Before calling, makeBodyParameter now requires an AssetsPublishedProof value. This is a value that can only be produced by calling certain functions that publish assets, and cannot be constructed from any other source location than the module that defines it. This ensures that makeBodyParameter cannot be called without the uploading of assets.
The return value of makeBodyParameter no longer is just CloudFormation API parameters, with the implicit assumption that any assets added to the AssetManifestBuilder would be uploaded as well: instead, the return value is either CloudFormation parameters, or an object with a function that trades adding to an AssetManifestBuilder for the CloudFormation parameters.

After merging with #31904, this also moves the allowCrossAccountAssetPublishing check into ToolkitInfo, next to all the other bootstrap stack routines. In practice it is indirected through EnvironmentResources, and has a different implementation for no-environment-resources.

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

…`makeBodyParameter` The protocol of `makeBodyParameter` has 2 requirements: - Stack assets must already be uploaded (because the stack template will be among them). - It may produce a new stack template asset to be uploaded. This protocol was easy to misuse, so change the API to make it more resilient to misuse: - Before calling, `makeBodyParameter` now requires an `AssetsPublishedProof` value. This is a value that can only be produced by calling certain functions that publish assets, and cannot be constructed from any other source location than the module that defines it. This ensures that `makeBodyParameter` cannot be called without the uploading of assets. - The return value of `makeBodyParameter` no longer is just CloudFormation API parameters, with the implicit assumption that any assets added to the `AssetManifestBuilder` would be uploaded as well: instead, the return value is either CloudFormation parameters, or an object with a function that trades adding to an `AssetManifestBuilder` for the CloudFormation parameters.

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

mrgrain · 2024-10-29T10:45:46Z

packages/aws-cdk/lib/util/asset-publishing.ts

+const PUBLISH_ASSET_PROOF_SYM = Symbol('publish_assets_proof');
+
+/**
+ * This interface represents proof that assets have been published.
+ *
+ * Objects of this type can only be obtained by calling functions in this module.
+ */
+export interface AssetsPublishedProof {
+  [PUBLISH_ASSET_PROOF_SYM]: true;
+}


Using Symbol vs Symbol.for() will prevent anyone else from implementing AssetsPublishedProof - which might well be what you want. But than why make it an interface and/or export it. This feels like a recipe for confusion in the future.

This is exactly what I want -- I want any bit of code (also in other modules) be able to require an AssetsPublishedProof, while only this module can generate an instance of one.

mrgrain · 2024-10-29T10:46:51Z

packages/aws-cdk/lib/util/asset-publishing.ts

+ * This is only allowed in one location: the function `deployStack` in
+ * `deploy-stack.ts`.  Do not call this function anywhere but there.


Meh. So this is the Get-out-of-jail-free card if you pinky promise to not misuse it?

I mean. Yes. It's a pragmatic compromise without completely overhauling the codebase. The name should be clear enough to be flagged in code reviews if used for any other purpose.

mrgrain · 2024-10-29T10:52:44Z

packages/aws-cdk/lib/api/deployments.ts

@@ -419,27 +419,26 @@ export class Deployments {
    const { stackSdk, resolvedEnvironment, envResources } = await this.prepareSdkFor(stackArtifact, undefined, Mode.ForReading);
    const cfn = stackSdk.cloudFormation();

-    await uploadStackTemplateAssets(stackArtifact, this);
+    const proof = await uploadStackTemplateAssets(stackArtifact, this.sdkProvider, resolvedEnvironment);


Unrelated, but why does this method even publish assets?

Every API that calls CloudFormation with a template may need to publish assets, because with a LegacyStackSynthesizer the stack template is not automatically written to disk and published; the CLI will need to do that just-in-time.

mrgrain · 2024-10-29T10:56:21Z

packages/aws-cdk/lib/api/util/template-body-parameter.ts

  resources: EnvironmentResources,
  sdk: ISDK,
-  overrideTemplate?: any,
-): Promise<TemplateBodyParameter> {
+  _assetsPublishedProof: AssetsPublishedProof,


Unused? Don't we need to check this?

The fact that you can pass an object that passes the type checker is proof that you have one.

Yes someone could do as any, but we're not trying to protect against someone who is willingly trying to bypass the system, we are protecting against accidental misuse.

mrgrain

I like the direction, but I'm not wild about the implementation.

Good: makeBodyParameter is has fewer side-effects now.
Bad: we now make every caller of makeBodyParameter implement the (more or less) same switch (bodyAction.type) {}.

You should also add unit tests to this.

rix0rrr · 2024-10-29T14:28:17Z

Bad: we now make every caller of makeBodyParameter implement the (more or less) same switch (bodyAction.type) {}.

I suppose we could wrap that in a helper function with a callback. But the current code has the advantage of being simple and obvious.

rix0rrr · 2024-10-30T11:06:15Z

Note: when merging this from main, there are a lot of things that need to be resolved that conflict with #31904, so we need to merge that first.

…ntees

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

…ntees

aws-cdk-automation · 2024-11-06T15:00:29Z

➡️ PR build request submitted to test-main-pipeline ⬅️

A maintainer must now check the pipeline and add the pr-linter/cli-integ-tested label once the pipeline succeeds.

…ntees

…nto huijbers/static-guarantees

aws-cdk-automation · 2024-11-15T16:28:15Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 2a81ee4
Result: FAILED
Build Logs (available for 30 days)

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

rix0rrr requested a review from a team October 14, 2024 11:04

aws-cdk-automation requested a review from a team October 14, 2024 11:05

github-actions bot added the p2 label Oct 14, 2024

mergify bot added the contribution/core label Oct 14, 2024

rix0rrr had a problem deploying to test-pipeline October 14, 2024 11:05 — with GitHub Actions Failure

aws-cdk-automation previously requested changes Oct 14, 2024

View reviewed changes

aws-cdk-automation added the pr/needs-cli-test-run label Oct 14, 2024

mrgrain reviewed Oct 29, 2024

View reviewed changes

mrgrain requested changes Oct 29, 2024

View reviewed changes

github-actions bot mentioned this pull request Nov 1, 2024

Monthly PR metrics report #31972

Closed

rix0rrr added 2 commits November 4, 2024 15:00

Merge remote-tracking branch 'origin/main' into huijbers/static-guara…

8b6f902

…ntees

Refactoring where "allowCrossAccountAssetPublishing" lives

78be3c6

rix0rrr temporarily deployed to test-pipeline November 5, 2024 09:32 — with GitHub Actions Inactive

rix0rrr added the pr-linter/cli-integ-tested label Nov 6, 2024

aws-cdk-automation removed the pr/needs-cli-test-run label Nov 6, 2024

Merge remote-tracking branch 'origin/main' into huijbers/static-guara…

9470d90

…ntees

rix0rrr temporarily deployed to test-pipeline November 6, 2024 10:46 — with GitHub Actions Inactive

rix0rrr self-assigned this Nov 6, 2024

Merge remote-tracking branch 'origin/main' into huijbers/static-guara…

8a56277

…ntees

rix0rrr had a problem deploying to test-pipeline November 15, 2024 15:18 — with GitHub Actions Failure

Merge branch 'main' into huijbers/static-guarantees

10492b4

rix0rrr had a problem deploying to test-pipeline November 15, 2024 15:30 — with GitHub Actions Failure

rix0rrr added 2 commits November 15, 2024 16:43

Lint

981a6ec

Merge branch 'huijbers/static-guarantees' of github.com:aws/aws-cdk i…

2a81ee4

…nto huijbers/static-guarantees

rix0rrr had a problem deploying to test-pipeline November 15, 2024 16:15 — with GitHub Actions Failure

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(cli): statically guarantee that assets aren't forgotten around makeBodyParameter #31750

chore(cli): statically guarantee that assets aren't forgotten around makeBodyParameter #31750

rix0rrr commented Oct 14, 2024 •

edited

Loading

aws-cdk-automation left a comment

mrgrain Oct 29, 2024

rix0rrr Oct 29, 2024

mrgrain Oct 29, 2024

rix0rrr Oct 29, 2024

mrgrain Oct 29, 2024

rix0rrr Oct 29, 2024

mrgrain Oct 29, 2024

rix0rrr Oct 29, 2024

mrgrain left a comment

rix0rrr commented Oct 29, 2024

rix0rrr commented Oct 30, 2024

aws-cdk-automation commented Nov 6, 2024

aws-cdk-automation commented Nov 15, 2024

		* This is only allowed in one location: the function `deployStack` in
		* `deploy-stack.ts`. Do not call this function anywhere but there.

chore(cli): statically guarantee that assets aren't forgotten around makeBodyParameter #31750

Are you sure you want to change the base?

chore(cli): statically guarantee that assets aren't forgotten around makeBodyParameter #31750

Conversation

rix0rrr commented Oct 14, 2024 • edited Loading

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrgrain left a comment

Choose a reason for hiding this comment

rix0rrr commented Oct 29, 2024

rix0rrr commented Oct 30, 2024

aws-cdk-automation commented Nov 6, 2024

aws-cdk-automation commented Nov 15, 2024

AWS CodeBuild CI Report

rix0rrr commented Oct 14, 2024 •

edited

Loading