codebuild: PipelineProject Construct: Add option to suppress creation of a AWS::Logs::ResourcePolicy when using logging via aws-logs.LogGroups or be descriptive about the behavior

[pepito-j]

Describe the feature

Add in an optional parameter to "disable" (or remove) the auto-generated Resource Policy, or add to the description of the CloudWatchLoggingOptions interface that a AWS::Logs::ResourcePolicy will be auto-generated (just to help others avoid the headache of the weird behavior)

• https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_codebuild.CloudWatchLoggingOptions.html

Use Case

Related to issues "#17615" and "#17544", I believe it may be a good workaround to the unexpected Resource Policy being added without the user's knowledge when using the PipelineProject Construct.

Proposed Solution

We can add another parameter to the CloudWatchLoggingOptions interface (something like "suppressResourcePolicy")

• https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_codebuild.CloudWatchLoggingOptions.html

On the handler, it simply checks if logging is configured and if it's enabled/disabled.

• aws-cdk/packages/@aws-cdk/aws-codebuild/lib/project.ts

  Lines 1452 to 1466 in 60a5b2a

  	if (props.cloudWatch) {
  	const cloudWatchLogs = props.cloudWatch;
  	const status = (cloudWatchLogs.enabled ?? true) ? 'ENABLED' : 'DISABLED';
  	if (status === 'ENABLED' && !(cloudWatchLogs.logGroup)) {
  	throw new Error('Specifying a LogGroup is required if CloudWatch logging for CodeBuild is enabled');
  	}
  	cloudWatchLogs.logGroup?.grantWrite(this);
  	cloudwatchConfig = {
  	status,
  	groupName: cloudWatchLogs.logGroup?.logGroupName,
  	streamName: cloudWatchLogs.prefix,
  	};
  	}

After "cloudWatchLogs.logGroup?.grantWrite(this);" is executed, we can then do another check to see if we should do remove it or not:

if ( cloudWatchLogs.suppressResourcePolicy )
   cloudWatchLogs.logGroup?.node.tryRemoveChild("Policy")

I wasn't able to find any another potential ways that '.node.tryRemoveChild("Policy")' wouldn't be applicable except if the "Id" may not be "Policy", but I wasn't see where the Id may be changed either.

In the case that the resource Id on tree.json isn't consistent, it can also just be explicitly stated on the documentation that this will be auto-populated given some certain conditions. This way users don't get too confused with the unexpected resource and can try to remediate it themselves by removing it from the LogGroup that it's tied to.

Other Information

I believe the issue of a ResourcePolicy being added unexpectedly is due to the behavior of the "grantWrite()" method being called when checking if logging is being defined or not.

• aws-cdk/packages/@aws-cdk/aws-codebuild/lib/project.ts

  Lines 1452 to 1466 in 60a5b2a

  	if (props.cloudWatch) {
  	const cloudWatchLogs = props.cloudWatch;
  	const status = (cloudWatchLogs.enabled ?? true) ? 'ENABLED' : 'DISABLED';
  	if (status === 'ENABLED' && !(cloudWatchLogs.logGroup)) {
  	throw new Error('Specifying a LogGroup is required if CloudWatch logging for CodeBuild is enabled');
  	}
  	cloudWatchLogs.logGroup?.grantWrite(this);
  	cloudwatchConfig = {
  	status,
  	groupName: cloudWatchLogs.logGroup?.logGroupName,
  	streamName: cloudWatchLogs.prefix,
  	};
  	}

To my understanding of how this works, essentially grantWrite() will try to do the right thing by applying some policy to a valid Grantable-type object, and in this case it would be some IAM Role.

Given that the stack is account agnostic and an IAM Role is being used similar to the following:

#!/usr/bin/env node
import 'source-map-support/register';
import * as cdk from 'aws-cdk-lib';
import * as logs from 'aws-cdk-lib/aws-logs';
import * as iam from 'aws-cdk-lib/aws-iam';
import * as codebuild from 'aws-cdk-lib/aws-codebuild';

const app = new cdk.App();

const stack = new cdk.Stack(app, "LogLimitStack", {
    env:{ region: "us-west-2" }
})
const logGroup = new logs.LogGroup(this, 'my-log-group-label', {
    logGroupName: 'my-log-group-name',
    removalPolicy: cdk.RemovalPolicy.RETAIN,
    retention: logs.RetentionDays.ONE_WEEK,
});
const importedRole = iam.Role.fromRoleArn(this, 'ImportedRole',
    `arn:aws:iam::<SomeAccountId>:role/my-codebuild-role`
);
const project = new codebuild.PipelineProject(this, 'my-build-project-label', {
    projectName: 'my-build-project-name',
    role: importedRole,
    buildSpec: codebuild.BuildSpec.fromObject({
        version: '0.2',
        phases: {
            build: {
                commands: [ 'echo "Hello, CodeBuild!"' ],
            },
        },
    }),
    logging : {
        cloudWatch: { logGroup: logGroup }
    }
});

The synthesized template will include the AWS::Logs::ResourcePolicy under the above conditions:

  myloggrouplabel0A41AD36:
    Type: AWS::Logs::LogGroup
    Properties:
      LogGroupName: my-log-group-name
      RetentionInDays: 7
    UpdateReplacePolicy: Retain
    DeletionPolicy: Retain
    Metadata:
      aws:cdk:path: LogLimitStack/my-log-group-label/Resource
  myloggrouplabelPolicyResourcePolicy95FDC6C6:
    Type: AWS::Logs::ResourcePolicy
    Properties:
      PolicyDocument:
        Fn::Join:
          - ""
          - - '{"Statement":[{"Action":["logs:CreateLogStream","logs:PutLogEvents"],"Effect":"Allow","Principal":{"AWS":"<SomeAccountId>"},"Resource":"'
            - Fn::GetAtt:
                - myloggrouplabel0A41AD36
                - Arn
            - '"}],"Version":"2012-10-17"}'
      PolicyName: LogLimitStackmyloggrouplabelPolicy23DE1910
    Metadata:
      aws:cdk:path: LogLimitStack/my-log-group-label/Policy/ResourcePolicy

the PipelineProject will invoke the grantWrite() under the hood, and grantWrite() will infer information based on what role information is currently passed to the PipelineProject where it'll be created if an aws_iam.IRole is supplied or not:

• https://github.com/aws/aws-cdk/blob/main/packages/@aws-cdk/aws-codebuild/lib/project.ts#L1035-L1041

In the case of an imported role, to my understanding of the grant() method being used by "grantWrite()" this seems to try inferring if the IAM Role to reference is part of the same account or not via "addToPrincipalOrResource()". Because the stack is account agnostic, it then infers this to potentially be cross-account since we can't guarantee that the imported IAM role will be from the same target account.

Testing this out with a simple stack reflects this behavior when just isolating it to executing "grantWrite()" against an imported role resulting in the same ResourcePolicy to appear without my consent

const app = new cdk.App();

const stack = new cdk.Stack(app, "LogLimitStack", {
    env:{ region: "us-west-2" }
})
const logGroup = new logs.LogGroup(stack, 'my-log-group-label', {
    logGroupName: 'my-log-group-name',
    removalPolicy: cdk.RemovalPolicy.RETAIN,
    retention: logs.RetentionDays.ONE_WEEK,
});
const importedRole = iam.Role.fromRoleArn(stack, 'ImportedRole',
    `arn:aws:iam::<SomeAccountId>:role/my-codebuild-role`
);
logGroup.grantWrite(importedRole)

But when manually creating a role and granting it write permissions, the resource policy disappears within the account agnostic stack:

const app = new cdk.App();

const stack = new cdk.Stack(app, "LogLimitStack", {
    env:{ region: "us-west-2" }
})
const logGroup = new logs.LogGroup(stack, 'my-log-group-label', {
    logGroupName: 'my-log-group-name',
    removalPolicy: cdk.RemovalPolicy.RETAIN,
    retention: logs.RetentionDays.ONE_WEEK,
});
const actualRole = new iam.Role(stack, 'ActualCBRole', {
    assumedBy: new iam.ServicePrincipal('codebuild.amazonaws.com'),
});
logGroup.grantWrite(actualRole)

Because this stems from the LogGroup Construct primarily, I was expecting this to potentially be documented explicitly on the aws_logs overview, or the LogGroup Construct document. I found on the overview that this may be implied:

Be aware that any ARNs or tokenized values passed to the resource policy will be converted into AWS Account IDs.

• https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_logs-readme.html

But given the unexpected results not being explicit/implied on the aws_codebuild.PipelineProject docs, it's understanding how this may be missed as users would have to dig into the source to eventually see this. But this is assuming my findings are correct and would like to hopefully get this cross-checked by others to confirm my understanding of this behavior.

Overall, this seems to be an expected but unfortunate outcome due to how "addToPrincipalOrResource()" infers information about the CDK Stack.

• aws-cdk/packages/@aws-cdk/aws-logs/lib/log-group.ts

  Lines 173 to 175 in 60a5b2a

  	public grantWrite(grantee: iam.IGrantable) {
  	return this.grant(grantee, 'logs:CreateLogStream', 'logs:PutLogEvents');
  	}

• aws-cdk/packages/@aws-cdk/aws-logs/lib/log-group.ts

  Lines 193 to 202 in 60a5b2a

  	public grant(grantee: iam.IGrantable, ...actions: string[]) {
  	return iam.Grant.addToPrincipalOrResource({
  	grantee,
  	actions,
  	// A LogGroup ARN out of CloudFormation already includes a ':*' at the end to include the log streams under the group.
  	// See https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-logs-loggroup.html#w2ab1c21c10c63c43c11
  	resourceArns: [this.logGroupArn],
  	resource: this,
  	});
  	}

• aws-cdk/packages/@aws-cdk/aws-iam/lib/grant.ts

  Lines 122 to 158 in 9d1093f

  	public static addToPrincipalOrResource(options: GrantWithResourceOptions): Grant {
  	const result = Grant.addToPrincipal({
  	...options,
  	scope: options.resource,
  	});
  	const resourceAndPrincipalAccountComparison = options.grantee.grantPrincipal.principalAccount
  	? cdk.Token.compareStrings(options.resource.env.account, options.grantee.grantPrincipal.principalAccount)
  	: undefined;
  	// if both accounts are tokens, we assume here they are the same
  	const equalOrBothUnresolved = resourceAndPrincipalAccountComparison === cdk.TokenComparison.SAME
  	|| resourceAndPrincipalAccountComparison == cdk.TokenComparison.BOTH_UNRESOLVED;
  	const sameAccount: boolean = resourceAndPrincipalAccountComparison
  	? equalOrBothUnresolved
  	// if the principal doesn't have an account (for example, a service principal),
  	// we should modify the resource's trust policy
  	: false;
  	// If we added to the principal AND we're in the same account, then we're done.
  	// If not, it's a different account and we must also add a trust policy on the resource.
  	if (result.success && sameAccount) {
  	return result;
  	}
  	const statement = new PolicyStatement({
  	actions: options.actions,
  	resources: (options.resourceSelfArns || options.resourceArns),
  	principals: [options.grantee!.grantPrincipal],
  	});
  	const resourceResult = options.resource.addToResourcePolicy(statement);
  	return new Grant({
  	resourceStatement: statement,
  	options,
  	policyDependable: resourceResult.statementAdded ? resourceResult.policyDependable ?? options.resource : undefined,
  	});
  	}

So, instead of modifying the behavior of "addToPrincipalOrResource()", instead it may just be easier to be clear with how the PipelineProject Construct configures logging automatically on behalf of the user. Otherwise, it may be easier to give users the option to disable it themselves by adding another property and checking if it's set to true or not.

Acknowledgements

• I may be able to implement this feature request
• This feature might incur a breaking change

CDK version used

2.69.0

Environment details (OS name and version, etc.)

Windows 10 | Node v16.13.0

[pahud]

Thank you for all the details. This seems to be a good option. Feel free to submit your PR when ready and we will be happy to review and dive deep in the PR.

[nrledo]

@pepito-j Are you planning on submitting / reworking your PR? I'm impacted by this issue and I don't mind tackling it if you don't mind...

[nathmahale]

Looks like this PR is stale and was closed off by the bot. I am having the same issue when using python cdk v2.93.0.

@nrledo -- Have you managed to solve this?
Any pointers would help. I am from python world, i can donate my time to help solve this, if others have not solved it already.

Cheers

[mrgrain]

Hi all, can you help me out and explain why this resource policy is not needed?
The issue and PR list the reasons why it might be unwanted as a) unexpected and b) very strict limit on resource policies.
But why is the policy added in the first place? What feature will break if we don't have it?

---

I appreciate the suggested API removes the policy, but it doesn't quite help anyone to figure out why they would want to do this, nor does it inform about consequences.

As far as I can tell this workaround works perfectly well:

cloudWatchLogs.logGroup?.node.tryRemoveChild('Policy'); 

[nrledo]

@nathmahale What I finally ended up doing is creating an iam role on the same project instead of importing it, and then assigning this role to the project. This made the ResourcePolicy not to be created, as stated by @pepito-j on their previous post

// Create a log group for the build project
const logGroup = new logs.LogGroup(this, id + "logGroup", {
logGroupName: "/aws/codebuild/" + stackSpecific(id) + "LogGroup",
retention: logs.RetentionDays.TWO_WEEKS,
});

// Create the role for the build project
const actualRole = new iam.Role(this, id + 'ActualCBRole', {
  assumedBy: new iam.ServicePrincipal('codebuild.amazonaws.com'),
});
logGroup.grantWrite(actualRole);