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.

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(cli): cdk watch for Lambdas with Advanced Logging Controls do not stream logs to the terminal #29451

Merged
merged 33 commits into from
Mar 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
589e2d8
feat: use LoggingConfig.LogGroupName when lambda is configured with c…
onhate Mar 11, 2024
567adf5
Merge branch 'main' into fix-use-logconfig-for-cdk-watch
onhate Mar 11, 2024
d62dd4b
add tests
onhate Mar 11, 2024
49089d0
adding more tests
onhate Mar 11, 2024
ce4a051
remove invalid return value
onhate Mar 11, 2024
398732b
Merge branch 'main' into fix-use-logconfig-for-cdk-watch
onhate Mar 12, 2024
dcc39a0
improve isReferencedFromIgnoredResource logic
onhate Mar 12, 2024
f8bcdfc
Merge remote-tracking branch 'origin/fix-use-logconfig-for-cdk-watch'…
onhate Mar 12, 2024
1acf29d
Merge branch 'main' into fix-use-logconfig-for-cdk-watch
onhate Mar 12, 2024
6dc6827
Merge branch 'main' into fix-use-logconfig-for-cdk-watch
onhate Mar 12, 2024
95f7904
Merge branch 'main' into fix-use-logconfig-for-cdk-watch
onhate Mar 13, 2024
c22a3a2
Merge branch 'main' into fix-use-logconfig-for-cdk-watch
onhate Mar 13, 2024
b51b6fe
Merge branch 'main' into fix-use-logconfig-for-cdk-watch
onhate Mar 14, 2024
1d8d8a5
Merge branch 'main' into fix-use-logconfig-for-cdk-watch
onhate Mar 14, 2024
5cebbcf
Merge branch 'main' into fix-use-logconfig-for-cdk-watch
onhate Mar 14, 2024
e3f49b7
Merge branch 'main' into fix-use-logconfig-for-cdk-watch
onhate Mar 18, 2024
1d9bf61
code review changes
onhate Mar 18, 2024
0abae7e
Merge branch 'main' into fix-use-logconfig-for-cdk-watch
onhate Mar 18, 2024
802e1db
Merge branch 'main' into fix-use-logconfig-for-cdk-watch
onhate Mar 18, 2024
0fe76ca
Merge branch 'main' into fix-use-logconfig-for-cdk-watch
onhate Mar 18, 2024
cad211c
Merge branch 'main' into fix-use-logconfig-for-cdk-watch
onhate Mar 18, 2024
48a9b05
Merge branch 'main' into fix-use-logconfig-for-cdk-watch
onhate Mar 18, 2024
77105bd
Merge branch 'main' into fix-use-logconfig-for-cdk-watch
onhate Mar 19, 2024
28c6ecf
Merge branch 'main' into fix-use-logconfig-for-cdk-watch
onhate Mar 20, 2024
ae37db2
Merge branch 'main' into fix-use-logconfig-for-cdk-watch
onhate Mar 20, 2024
750aa2b
Merge branch 'main' into fix-use-logconfig-for-cdk-watch
mergify[bot] Mar 21, 2024
f5794c0
code review changes
onhate Mar 21, 2024
1b35bbf
Merge branch 'main' into fix-use-logconfig-for-cdk-watch
onhate Mar 21, 2024
5761504
Merge branch 'main' into fix-use-logconfig-for-cdk-watch
onhate Mar 25, 2024
830ee9c
Merge branch 'main' into fix-use-logconfig-for-cdk-watch
onhate Mar 25, 2024
33a37e9
Merge branch 'main' into fix-use-logconfig-for-cdk-watch
onhate Mar 25, 2024
32981b2
Merge branch 'main' into fix-use-logconfig-for-cdk-watch
mergify[bot] Mar 26, 2024
f941cde
Merge branch 'main' into fix-use-logconfig-for-cdk-watch
mergify[bot] Mar 27, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/aws-cdk/lib/api/evaluate-cloudformation-template.ts
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,10 @@ export class EvaluateCloudFormationTemplate {
return cfnExpression;
}

public getResourceProperty(logicalId: string, propertyName: string): any {
return this.template.Resources?.[logicalId]?.Properties?.[propertyName];
}

private references(logicalId: string, templateElement: any): boolean {
if (typeof templateElement === 'string') {
return logicalId === templateElement;
Expand Down
86 changes: 53 additions & 33 deletions packages/aws-cdk/lib/api/logs/find-cloudwatch-logs.ts
Original file line number Diff line number Diff line change
@@ -1,22 +1,12 @@
import * as cxapi from '@aws-cdk/cx-api';
import { CloudFormation } from 'aws-sdk';
import { Mode, SdkProvider, ISDK } from '../aws-auth';
import { ISDK, Mode, SdkProvider } from '../aws-auth';
import { Deployments } from '../deployments';
import { EvaluateCloudFormationTemplate, LazyListStackResources } from '../evaluate-cloudformation-template';

// resource types that have associated CloudWatch Log Groups that should _not_ be monitored
const IGNORE_LOGS_RESOURCE_TYPES = ['AWS::EC2::FlowLog', 'AWS::CloudTrail::Trail', 'AWS::CodeBuild::Project'];

// Resource types that will create a CloudWatch log group with a specific name if one is not provided.
// The keys are CFN resource types, and the values are the name of the physical name property of that resource
// and the service name that is used in the automatically created CloudWatch log group.
const RESOURCE_TYPES_WITH_IMPLICIT_LOGS: { [cfnResourceType: string]: { [key: string]: string } } = {
'AWS::Lambda::Function': {
PhysicalNamePropertyName: 'FunctionName',
LogGroupServiceName: 'lambda',
},
};

/**
* Configuration needed to monitor CloudWatch Log Groups
* found in a given CloudFormation Stack
Expand Down Expand Up @@ -84,38 +74,68 @@ function isReferencedFromIgnoredResource(
logGroupResource: CloudFormation.StackResourceSummary,
evaluateCfnTemplate: EvaluateCloudFormationTemplate,
): boolean {
let foundReference = false;
const resourcesReferencingLogGroup = evaluateCfnTemplate.findReferencesTo(logGroupResource.LogicalResourceId);
for (const reference of resourcesReferencingLogGroup) {
if (IGNORE_LOGS_RESOURCE_TYPES.includes(reference.Type)) {
foundReference = true;
}
}
return foundReference;
return resourcesReferencingLogGroup.some(reference => {
return IGNORE_LOGS_RESOURCE_TYPES.includes(reference.Type);
});
Comment on lines +78 to +80
Copy link
Contributor

Choose a reason for hiding this comment

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

This has no functionality change, correct?

Copy link
Contributor Author

@onhate onhate Mar 21, 2024

Choose a reason for hiding this comment

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

correct, it was looping the resourcesReferencingLogGroup and returning true if some Type is in the IGNORE_LOGS_RESOURCE_TYPES.

Which summarizes as a .some.

let foundReference = false;
  const resourcesReferencingLogGroup = evaluateCfnTemplate.findReferencesTo(logGroupResource.LogicalResourceId);
  for (const reference of resourcesReferencingLogGroup) {
    if (IGNORE_LOGS_RESOURCE_TYPES.includes(reference.Type)) {
      foundReference = true;
    }
  }
  return foundReference;

}

type CloudWatchLogsResolver = (
resource: CloudFormation.StackResourceSummary,
evaluateCfnTemplate: EvaluateCloudFormationTemplate
) => string | undefined;

const cloudWatchLogsResolvers: Record<string, CloudWatchLogsResolver> = {
'AWS::Logs::LogGroup': (resource, evaluateCfnTemplate) => {
if (isReferencedFromIgnoredResource(resource, evaluateCfnTemplate)) {
return undefined;
}
return resource.PhysicalResourceId?.toString();
},

// Resource types that will create a CloudWatch log group with a specific name if one is not provided.
// The keys are CFN resource types, and the values are the name of the physical name property of that resource
// and the service name that is used in the automatically created CloudWatch log group.
'AWS::Lambda::Function': (resource, evaluateCfnTemplate) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Lambda is not the only service that creates custom logGroupNames. We should note that similar functionality may be needed for several other services (rds, neptune, docdb, etc). However, Lambda Function was the only resource in RESOURCE_TYPES_WITH_IMPLICIT_LOGS, so we definitely do not need to add that functionality.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I though about that while making this change but as the only reference I had was the Lambda one I thought in not adding more stuff than the things that already exists, but I can definitely work on that extras on upcoming PRs now that I got familiarity with the codebase.

const loggingConfig = evaluateCfnTemplate.getResourceProperty(resource.LogicalResourceId, 'LoggingConfig');
if (loggingConfig?.LogGroup) {
// if LogGroup is a string then use it as the LogGroupName as it is referred by LogGroup.fromLogGroupArn in CDK
if (typeof loggingConfig.LogGroup === 'string') {
return loggingConfig.LogGroup;
}

// if { Ref: '...' } is used then try to resolve the LogGroupName from the referenced resource in the template
if (typeof loggingConfig.LogGroup === 'object') {
if (loggingConfig.LogGroup.Ref) {
return evaluateCfnTemplate.getResourceProperty(loggingConfig.LogGroup.Ref, 'LogGroupName');
}
}
}

return `/aws/lambda/${resource.PhysicalResourceId}`;
},
};

/**
* Find all CloudWatch Log Groups in the deployed template.
* This will find both explicitely created Log Groups (excluding those associated with ignored resources)
* as well as Log Groups created implicitely (i.e. Lambda Functions)
* This will find both explicitly created Log Groups (excluding those associated with ignored resources)
* and Log Groups created implicitly (i.e. Lambda Functions)
*/
function findAllLogGroupNames(
stackResources: CloudFormation.StackResourceSummary[],
evaluateCfnTemplate: EvaluateCloudFormationTemplate,
): string[] {
return stackResources.reduce((logGroupNames: string[], resource) => {
let logGroupName;
if (resource.ResourceType === 'AWS::Logs::LogGroup') {
if (!isReferencedFromIgnoredResource(resource, evaluateCfnTemplate)) {
logGroupName = resource.PhysicalResourceId;
const logGroupNames: string[] = [];

for (const resource of stackResources) {
const logGroupResolver = cloudWatchLogsResolvers[resource.ResourceType];
if (logGroupResolver) {
const logGroupName = logGroupResolver(resource, evaluateCfnTemplate);
if (logGroupName) {
logGroupNames.push(logGroupName);
}
} else if (RESOURCE_TYPES_WITH_IMPLICIT_LOGS[resource.ResourceType]) {
const servicePart = RESOURCE_TYPES_WITH_IMPLICIT_LOGS[resource.ResourceType].LogGroupServiceName;
logGroupName = `/aws/${servicePart}/${resource.PhysicalResourceId}`;
}
if (logGroupName) {
logGroupNames.push(logGroupName);
}
return logGroupNames;
}, []);
}

return logGroupNames;
}
60 changes: 60 additions & 0 deletions packages/aws-cdk/test/api/logs/find-cloudwatch-logs.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,66 @@ test('add log groups from lambda function', async () => {
expect(result.logGroupNames).toEqual(['/aws/lambda/my-function']);
});

test('add log groups from lambda function when using custom LoggingConfig', async () => {
// GIVEN
const cdkStackArtifact = cdkStackArtifactOf({
template: {
Resources: {
Func: {
Type: 'AWS::Lambda::Function',
Properties: {
FunctionName: 'my-function',
LoggingConfig: {
LogGroup: '/this/custom/my-custom-log-group',
},
},
},
},
},
});
pushStackResourceSummaries(stackSummaryOf('Func', 'AWS::Lambda::Function', 'my-function'));

// WHEN
const result = await findCloudWatchLogGroups(logsMockSdkProvider.mockSdkProvider, cdkStackArtifact);

// THEN
expect(result.logGroupNames).toEqual(['/this/custom/my-custom-log-group']);
});

test('add log groups from lambda function when using custom LoggingConfig using Ref', async () => {
// GIVEN
const cdkStackArtifact = cdkStackArtifactOf({
template: {
Resources: {
MyCustomLogGroupLogicalId: {
Type: 'AWS::Logs::LogGroup',
Properties: {
LogGroupName: '/this/custom/my-custom-log-group',
},
},
Func: {
Type: 'AWS::Lambda::Function',
Properties: {
FunctionName: 'my-function',
LoggingConfig: {
LogGroup: {
Ref: 'MyCustomLogGroupLogicalId',
},
},
},
},
},
},
});
pushStackResourceSummaries(stackSummaryOf('Func', 'AWS::Lambda::Function', 'my-function'));

// WHEN
const result = await findCloudWatchLogGroups(logsMockSdkProvider.mockSdkProvider, cdkStackArtifact);

// THEN
expect(result.logGroupNames).toEqual(['/this/custom/my-custom-log-group']);
});

test('add log groups from lambda function without physical name', async () => {
// GIVEN
const cdkStackArtifact = cdkStackArtifactOf({
Expand Down