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.

Sign up for GitHub

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

fix(toolkit-lib): message including tokens from annotations cannot output correctly #101

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
+94 −8
Open

fix(toolkit-lib): message including tokens from annotations cannot output correctly #101

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
78ada79
fix(toolkit-lib): message including tokens from annotations cannot ou…
go-to-k Feb 22, 2025
42570c4
change test names
go-to-k Feb 22, 2025
05d3b92
nit
go-to-k Feb 22, 2025
89815b2
tests
go-to-k Feb 22, 2025
4811e4d
style
go-to-k Feb 22, 2025
83a2d48
missed await
go-to-k Feb 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 17 additions & 7 deletions packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -757,13 +757,23 @@ export class Toolkit extends CloudAssemblySourceBuilder implements AsyncDisposab
default: return 'CDK_ASSEMBLY_I9999';
}
};
await stacks.validateMetadata(this.props.assemblyFailureAt, async (level, msg) => ioHost.notify({
time: new Date(),
level,
code: code(level),
message: `[${level} at ${msg.id}] ${msg.entry.data}`,
data: msg,
}));

await stacks.validateMetadata(this.props.assemblyFailureAt, async (level, msg) => {
// Data comes from Annotations and the data can be of object type containing 'Fn::Join' or 'Ref' when tokens are included in Annotations.
// Therefore, we use JSON.stringify to convert it to a string when the data is of object type.
// see: https://github.com/aws/aws-cdk/issues/33527
const data = msg.entry.data !== null && typeof msg.entry.data === 'object'
? JSON.stringify(msg.entry.data)
: msg.entry.data;

await ioHost.notify({
time: new Date(),
level,
code: code(level),
message: `[${level} at ${msg.id}] ${data}`,
data: msg,
});
});
}

/**
Expand Down
78 changes: 77 additions & 1 deletion packages/@aws-cdk/toolkit-lib/test/toolkit/toolkit.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

import * as chalk from 'chalk';
import { Toolkit } from '../../lib';
import { TestIoHost } from '../_helpers';
import { TestCloudAssemblySource, TestIoHost } from '../_helpers';

describe('message formatting', () => {
test('emojis can be stripped from message', async () => {
Expand Down Expand Up @@ -70,3 +70,79 @@ describe('message formatting', () => {
}));
});
});

describe('metadata message formatting', () => {
test('converts object data for log message to string', async () => {
const ioHost = new TestIoHost();
const toolkit = new Toolkit({ ioHost });

const source = new TestCloudAssemblySource({
stacks: [{
stackName: 'test-stack',
metadata: {
'test-stack': [{
type: 'aws:cdk:warning',
data: {
'Fn::Join': [
'',
[
'stackId: ',
{
Ref: 'AWS::StackId',
},
],
],
} as any,
}],
},
}],
});

await toolkit.synth(source);

expect(ioHost.notifySpy).toHaveBeenCalledWith(expect.objectContaining({
level: 'warn',
message: expect.stringContaining('{"Fn::Join":["",["stackId: ",{"Ref":"AWS::StackId"}]]}'),
Copy link
Contributor

@rix0rrr rix0rrr Feb 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is a particularly nice rendering.

How do these { Fn::Join } objects get into the annotations in the first place? They only have meaning inside a CFN template, so if they are produced outside the context of a CFN template that is wrong.

Are they resolve()d by any chance? Because they probably shouldn't be.

Copy link
Author

@go-to-k go-to-k Feb 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rix0rrr

How do these { Fn::Join } objects get into the annotations in the first place?

The issue mentioned about a situation where the user calls Annotations like the following:

(this is Stack class and ${this.stackId} is a token. So stackId: ${this.stackId} will be a string with { Fn::Join } after resolve.)

Annotations.of(this).addInfo(`stackId: ${this.stackId}`);

Are they resolve()d by any chance?

It seems that the Stack Synthesizer resolves the metadatas including them added by Annotations.

We can avoid resolving tokens added by Annotations by changing this code as follows.

    if (node.node.metadata.length > 0) {
      // Make the path absolute
      output[Node.PATH_SEP + node.node.path] = node.node.metadata.map(md => {
        if ([
          cxschema.ArtifactMetadataEntryType.ERROR,
          cxschema.ArtifactMetadataEntryType.WARN,
          cxschema.ArtifactMetadataEntryType.INFO,
        ].includes(md.type as cxschema.ArtifactMetadataEntryType)) {
          return md;
        }

        const resolved = stack.resolve(md);
        return resolved as cxschema.MetadataEntry;
      });

However, in that case, the metadatas would be written as the token format ("stackId: ${Token[AWS::StackId.1119]}") in manifest.json. I feel uncomfortable with this because I think the tokens are for CDK Apps and should be resolved in the synth phase, so I didn't decide it for now. (see the "Alternative Approach" in the description of this PR). But if this approach is fine, then I can adopt the approach.

Or what do you think of a way to make it an error if the user tries to put a token in Annotations?
(But in that case, the CDK application that the user including the issue author is currently running may not work, so a feature flag is needed...?)

Copy link
Contributor

@rix0rrr rix0rrr Mar 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or what do you think of a way to make it an error if the user tries to put a token in Annotations?

Yes this would have been ideal, but we can't do that anymore for the reason you already mentioned. People are already putting tokens into annotations (as useless as it is), otherwise you wouldn't have had to file this PR.

But yes, the change should be made inside the construct library. The data payload of an annotation should be a string, whatever the string is. So the library should make sure it's a string, and we don't have to deal with the situation where it's a non-string in the CLI.

I feel comfortable leaving the token unrendered for now; if people come to complain that they were parsing this data structure we will come up with a correct behavior then.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel comfortable leaving the token unrendered for now; if people come to complain that they were parsing this data structure we will come up with a correct behavior then.

Yes, got it. I will submit the another PR to not resolve the tokens by annotations in aws-cdk-lib.

Thanks.

data: {
entry: {
type: 'aws:cdk:warning',
data: { 'Fn::Join': ['', ['stackId: ', { Ref: 'AWS::StackId' }]] },
},
id: 'test-stack',
level: 'warning',
},
}));
});

test('keeps non-object data for log message as-is', async () => {
const ioHost = new TestIoHost();
const toolkit = new Toolkit({ ioHost });

const source = new TestCloudAssemblySource({
stacks: [{
stackName: 'test-stack',
metadata: {
'test-stack': [{
type: 'aws:cdk:info',
data: 'simple string message',
}],
},
}],
});

await toolkit.synth(source);

expect(ioHost.notifySpy).toHaveBeenCalledWith(expect.objectContaining({
level: 'info',
message: expect.stringContaining('simple string message'),
data: {
entry: {
type: 'aws:cdk:info',
data: 'simple string message',
},
id: 'test-stack',
level: 'info',
},
}));
});
});