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

logs: Support Resource policies #5343

Closed
cmckni3 opened this issue Dec 8, 2019 · 26 comments · Fixed by #17015
Closed

logs: Support Resource policies #5343

cmckni3 opened this issue Dec 8, 2019 · 26 comments · Fixed by #17015
Labels
@aws-cdk/aws-iam Related to AWS Identity and Access Management @aws-cdk/aws-logs Related to Amazon CloudWatch Logs effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. needs-cfn This issue is waiting on changes to CloudFormation before it can be addressed.

Comments

@cmckni3
Copy link
Contributor

cmckni3 commented Dec 8, 2019

Reproduction Steps

const cloudWatchLogGroup = new logs.LogGroup(this, 'ElasticSearchLogGroup', {
  retention: logs.RetentionDays.THREE_MONTHS,
});

cloudWatchLogGroup.grantWrite(new iam.ServicePrincipal('es.amazonaws.com')).assertSuccess();

Also fails with the resource policy that I need.

const cloudWatchLogGroup = new logs.LogGroup(this, 'ElasticSearchLogGroup', {
  retention: logs.RetentionDays.THREE_MONTHS,
});

cloudWatchLogGroup
  .grant(
    new iam.ServicePrincipal('es.amazonaws.com'),
    'logs:PutLogEvents',
    'logs:PutLogEventsBatch',
    'logs:CreateLogStream'
  )
  .assertSuccess();

Error Log

Error: Permissions for 'ServicePrincipal(es.amazonaws.com)' to call 'logs:CreateLogStream,logs:PutLogEvents' on '${Token[TOKEN.507]}' could not be added on either identity or resource policy.
Error: Permissions for 'ServicePrincipal(es.amazonaws.com)' to call 'logs:PutLogEvents,logs:PutLogEventsBatch,logs:CreateLogStream' on '${Token[TOKEN.507]}' could not be added on either identity or resource policy.

Environment

  • CLI Version :1.18.0
  • Framework Version:1.18.0
  • OS :macOS
  • Language :TypeScript

Other


This is 🐛 Bug Report

@cmckni3 cmckni3 added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Dec 8, 2019
@cmckni3
Copy link
Contributor Author

cmckni3 commented Dec 8, 2019

maybe LogGroup is missing addToResourcePolicy (IResourceWithPolicy)?

@SomayaB SomayaB added @aws-cdk/aws-logs Related to Amazon CloudWatch Logs @aws-cdk/aws-iam Related to AWS Identity and Access Management labels Dec 9, 2019
@rix0rrr
Copy link
Contributor

rix0rrr commented Dec 12, 2019

Correct, but as far as I know CloudWatch Log Groups don't have resource policies?

@rix0rrr
Copy link
Contributor

rix0rrr commented Dec 12, 2019

So what seems to be the case is that CloudWatch supports account-wide log policies, but CloudFormation doesn't support creating them.

These log policies seem to apply to all log groups at the same time though, not individual ones. Marking this as a "needs cfn" feature request.

@rix0rrr rix0rrr added feature-request A feature should be added or improved. needs-cfn This issue is waiting on changes to CloudFormation before it can be addressed. and removed bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Dec 12, 2019
@rix0rrr rix0rrr changed the title [logs] grant/grantWrite to ServicePrincipal fails logs: Support Resource policies Dec 12, 2019
@cmckni3
Copy link
Contributor Author

cmckni3 commented Dec 12, 2019

@rix0rrr
Copy link
Contributor

rix0rrr commented Dec 13, 2019

Your workaround for any feature that is not supported by CFN will be "custom resource".

@cmckni3
Copy link
Contributor Author

cmckni3 commented Dec 13, 2019

Yeah I created 2 custom resources yesterday. This is lower priority for me.

@cmckni3
Copy link
Contributor Author

cmckni3 commented Dec 13, 2019

Also, I noticed that CDK uses a custom resource to set log retention on lambda log groups.

Is that a related piece to cfn not supporting it? Should I open an issue to the CloudFormation team somewhere?

@rix0rrr rix0rrr added the effort/medium Medium work item – several days of effort label Jan 23, 2020
@rix0rrr
Copy link
Contributor

rix0rrr commented Jan 23, 2020

I believe CFN does not support this. An issue to CFN team would always be appreciate to try and get movement on things.

@cmckni3
Copy link
Contributor Author

cmckni3 commented Jan 23, 2020

Sure thing @rix0rrr. I have been creating some other GitHub issues on the Cloudformation roadmap.

@cmckni3
Copy link
Contributor Author

cmckni3 commented Jan 24, 2020

Created at aws-cloudformation/cloudformation-coverage-roadmap#359

Feel free to suggest updates to my description.

@paul42
Copy link

paul42 commented Mar 5, 2020

hey @cmckni3 are there any good examples out there for how to make a custom resource like you describe? i'm still learning CF and such.
Thanks in advance!

@cmckni3
Copy link
Contributor Author

cmckni3 commented Mar 7, 2020

@blimmer
Copy link
Contributor

blimmer commented Apr 23, 2020

In case others run into this, I used the AwsCustomResource construct to help accomplish our goal here.

Here's some code to hopefully help out if folks need to use this workaround in the meantime. Please note that this code is very specific to our use-case, so you'll almost undoubtedly need to modify it to work for you.

// Cloudwatch logs have global resource policies that allow EventBridge to
// write logs to a given Cloudwatch Log group. That's currently not exposed
// via CloudFormation, so we use a Custom Resource here.
// See https://github.com/aws/aws-cdk/issues/5343
const policyName = '<fill in your unique policy name>'
new cr.AwsCustomResource(this, "CloudwatchLogResourcePolicy", {
  resourceType: "Custom::CloudwatchLogResourcePolicy",
  onUpdate: {
    service: "CloudWatchLogs",
    action: "putResourcePolicy",
    parameters: {
      policyName,
      // PolicyDocument must be provided as a string, so we can't use the iam.PolicyDocument provisions
      // or other CDK niceties here.
      policyDocument: JSON.stringify({
        Version: "2012-10-17",
        Statement: [
          {
            Sid: policyName,
            Effect: "Allow",
            Principal: {
              Service: ["delivery.logs.amazonaws.com", "events.amazonaws.com"],
            },
            Action: ["logs:CreateLogStream", "logs:PutLogEvents"],
            // I'd prefer to use cdk.Arn.format() here, but that creates Fn::Join's in the template,
            // which AwsCustomResource can't handle.
            Resource: '<fill in your arn here>',
          },
        ],
      }),
    },
    physicalResourceId: cr.PhysicalResourceId.of(policyName),
  },
  onDelete: {
    service: "CloudWatchLogs",
    action: "deleteResourcePolicy",
    parameters: {
      policyName,
    },
  },
  policy: cr.AwsCustomResourcePolicy.fromStatements([
    new iam.PolicyStatement({
      actions: ["logs:PutResourcePolicy", "logs:DeleteResourcePolicy"],
      // Resource Policies are global in Cloudwatch Logs per-region, per-account.
      resources: ["*"],
    }),
  ]),
});

@cmckni3
Copy link
Contributor Author

cmckni3 commented Apr 23, 2020

@blimmer same here. Have been using AwsCustomResource heavily for other API calls.

Switched logs and resource policies over to AwsCustomResource 2 weeks ago.

@JoHuang
Copy link

JoHuang commented Jun 14, 2020

I'm also trying to create a log group to ES domain.
Is there any complete example that equivalent to this tutorial https://aws.amazon.com/blogs/big-data/viewing-amazon-elasticsearch-service-error-logs/ ?

Here is my code combining @blimmer 's custom resource example.
It worked without logPublishingOptions.
The error was:
The Resource Access Policy specified for the CloudWatch Logs log group /aws/aes/domains/monitordevstack/application-logs/ does not grant sufficient permissions for Amazon Elasticsearch Service to create a log stream. Please check the Resource Access Policy.

import cdk = require('@aws-cdk/core');
import {CfnDomain} from '@aws-cdk/aws-elasticsearch';
import {LogGroup, RetentionDays} from '@aws-cdk/aws-logs';
import {PolicyStatement} from '@aws-cdk/aws-iam';
import {AwsCustomResource, PhysicalResourceId, AwsCustomResourcePolicy} from '@aws-cdk/custom-resources'

const region = "us-west-2";

export class MonitorDevStack extends cdk.Stack {

  constructor(scope: cdk.App) {
    super(scope, 'MonitorDevStack', {
      env : { 
        account: process.env.CDK_DEFAULT_ACCOUNT, 
        region: region || process.env.CDK_DEFAULT_REGION
      }
    });
    const domainName = this.stackName.toLowerCase();

    const ES_APPLICATION_LOGS = new LogGroup(this, 'ES_APPLICATION_LOGS', {
      logGroupName: `/aws/aes/domains/${domainName}/application-logs/`,
      retention: RetentionDays.THREE_MONTHS
    });

    // Cloudwatch logs have global resource policies that allow EventBridge to
    // write logs to a given Cloudwatch Log group. That's currently not exposed
    // via CloudFormation, so we use a Custom Resource here.
    // See https://github.com/aws/aws-cdk/issues/5343
    const policyName = `${this.stackName}-ElastichsearchToCloudWatchPolicy`;
    const policy = new AwsCustomResource(this, "CloudwatchLogResourcePolicy", {
      resourceType: "Custom::CloudwatchLogResourcePolicy",
      onUpdate: {
        service: "CloudWatchLogs",
        action: "putResourcePolicy",
        parameters: {
          policyName,
          // PolicyDocument must be provided as a string, so we can't use the iam.PolicyDocument provisions
          // or other CDK niceties here.
          policyDocument: JSON.stringify({
            Version: "2012-10-17",
            Statement: [
              {
                Sid: policyName,
                Effect: "Allow",
                Principal: {
                  Service: ["es.amazonaws.com"],
                },
                Action: ["logs:CreateLogStream", "logs:PutLogEvents", "logs:PutLogEventsBatch"],
                // I'd prefer to use cdk.Arn.format() here, but that creates Fn::Join's in the template,
                // which AwsCustomResource can't handle.
                Resource: ES_APPLICATION_LOGS.logGroupArn,
              },
            ],
          }),
        },
        physicalResourceId: PhysicalResourceId.of(policyName),
      },
      onDelete: {
        service: "CloudWatchLogs",
        action: "deleteResourcePolicy",
        parameters: {
          policyName,
        },
      },
      policy: AwsCustomResourcePolicy.fromStatements([
        new PolicyStatement({
          actions: ["logs:PutResourcePolicy", "logs:DeleteResourcePolicy"],
          // Resource Policies are global in Cloudwatch Logs per-region, per-account.
          resources: ["*"],
        }),
      ]),
    });

    const elasticsearchDomain = new CfnDomain(
      this,
      'ElasticsearchDomain',
      {
        accessPolicies: {
          Version: '2012-10-17',
          Statement: [
            {
              Effect: 'Allow',
              Principal: {
                AWS: '*',
              },
              Action: [
                "es:ESHttp*"
              ],
              Resource: `arn:aws:es:${this.region}:${this.account}:domain/${domainName}/*`,
            },
          ],
        },
        ebsOptions: {
          ebsEnabled: true,
          volumeSize: 20,
          volumeType: 'standard',
        },
        elasticsearchClusterConfig: {
          instanceCount: 2,
          instanceType: 't2.medium.elasticsearch',
          zoneAwarenessConfig: {
            availabilityZoneCount: 2
          },
          zoneAwarenessEnabled: true
        },
        domainName: domainName,
        encryptionAtRestOptions: {
          enabled: false,
        },
        nodeToNodeEncryptionOptions: {
          enabled: true,
        },
        vpcOptions: {
          securityGroupIds: ["sg-xxxxxxx"],
          subnetIds: ["subnet-aaaaaa", "subnet-bbbbb"],
        },
        elasticsearchVersion: '7.4',
        logPublishingOptions: {
          "ES_APPLICATION_LOGS": {
            cloudWatchLogsLogGroupArn: ES_APPLICATION_LOGS.logGroupArn,
            enabled: true
          }
        }
      }
    );

    new cdk.CfnOutput(this, `ElasticEndpoint`, { value: elasticsearchDomain.attrDomainEndpoint });
  }  
}

@thomaspoignant
Copy link
Contributor

thomaspoignant commented Jul 21, 2020

@JoHuang I have the same problem than you, did you manage to make it work?

EDIT
You cannot add the logs during the creation, if you create your cluster with

"ES_APPLICATION_LOGS": {
            cloudWatchLogsLogGroupArn: ES_APPLICATION_LOGS.logGroupArn,
            enabled: false
          }

and after the first run you set

"ES_APPLICATION_LOGS": {
            cloudWatchLogsLogGroupArn: ES_APPLICATION_LOGS.logGroupArn,
            enabled: true
          }

it will work

@nom3ad
Copy link
Contributor

nom3ad commented Oct 12, 2021

CloudFormation now supports Cloudwatch Resource policies: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-logs-resourcepolicy.html

@mergify mergify bot closed this as completed in #17015 Nov 1, 2021
mergify bot pushed a commit that referenced this issue Nov 1, 2021
CloudFormation now supports [Cloudwatch logs Resource policies](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-logs-resourcepolicy.html)
This PR adds L2 support for it.

And now its possible to grant access to service principals as follows. Previously this was throwing an error - see #5343

```ts
const eventsTargetLogs = new logs.LogGroup(this, 'EventsTargetLogGroup');
eventsTargetLogs.grantWrite(new iam.ServicePrincipal('events.amazonaws.com')).assertSuccess();
```

In future, following custom resource implementation of `LogGroupResourcePolicy` could be replaced.

https://github.com/aws/aws-cdk/blob/83b8df8c390a27e10bf362f49babfb24ee425506/packages/@aws-cdk/aws-elasticsearch/lib/log-group-resource-policy.ts#L25
https://github.com/aws/aws-cdk/blob/a872e672f8990fc3879413e5d797533d3916e1fd/packages/@aws-cdk/aws-events-targets/lib/log-group-resource-policy.ts#L26
https://github.com/aws/aws-cdk/blob/a872e672f8990fc3879413e5d797533d3916e1fd/packages/@aws-cdk/aws-events-targets/lib/log-group-resource-policy.ts#L26

closes #5343

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

github-actions bot commented Nov 1, 2021

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this issue Feb 21, 2022
CloudFormation now supports [Cloudwatch logs Resource policies](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-logs-resourcepolicy.html)
This PR adds L2 support for it.

And now its possible to grant access to service principals as follows. Previously this was throwing an error - see aws#5343

```ts
const eventsTargetLogs = new logs.LogGroup(this, 'EventsTargetLogGroup');
eventsTargetLogs.grantWrite(new iam.ServicePrincipal('events.amazonaws.com')).assertSuccess();
```

In future, following custom resource implementation of `LogGroupResourcePolicy` could be replaced.

https://github.com/aws/aws-cdk/blob/83b8df8c390a27e10bf362f49babfb24ee425506/packages/@aws-cdk/aws-elasticsearch/lib/log-group-resource-policy.ts#L25
https://github.com/aws/aws-cdk/blob/a872e672f8990fc3879413e5d797533d3916e1fd/packages/@aws-cdk/aws-events-targets/lib/log-group-resource-policy.ts#L26
https://github.com/aws/aws-cdk/blob/a872e672f8990fc3879413e5d797533d3916e1fd/packages/@aws-cdk/aws-events-targets/lib/log-group-resource-policy.ts#L26

closes aws#5343

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@michaelfedell
Copy link
Contributor

@nom3ad

It looks like AWS GovCloud Regions do not yet have support for AWS::Logs::ResourcePolicy resources yet. Is there some way to force cdk to skip the creation of this resource? I am using ECS Patterns ApplicationLoadBalancedFargateService and getting an error back when deploying to GovCloud:

Template format error: Unrecognized resource types: [AWS::Logs::ResourcePolicy]

@nom3ad
Copy link
Contributor

nom3ad commented Aug 7, 2022

@michaelfedell
It could be possible to write a cdk "Aspect" to remove a particular resource from the stack template. Even if you were able to remove resourcePolicy and deploy the stack successfully, things could break at runtime due to permission issues.

@michaelfedell
Copy link
Contributor

@nom3ad - thanks for the suggestion! I have written an aspect to do this, but I have not been able to find any references on pruning a node from the resource tree. Could you provide some guidance here?

@jsii.implements(cdk.IAspect)
class RemoveLogsPolicyAspect:
    def visit(self, node: IConstruct) -> None:
        if isinstance(node, logs.CfnResourcePolicy):
            msg = "AWS::Logs::ResourcePolicy is not yet supported in GovCloud; removing from template."
            cdk.Annotations.of(node).add_warning(msg)
            # TODO: Remove node from stack

@michaelfedell
Copy link
Contributor

Ended up solving this with:

@jsii.implements(cdk.IAspect)
class RemoveLogsPolicyAspect:
    def visit(self, node: IConstruct) -> None:
        if isinstance(node, logs.ResourcePolicy):
            stack = cdk.Stack.of(node)
            # This Node is an L2 Construct for aws-cdk.aws-logs.ResourcePolicy
            # The default_child is of type AWS::Logs::ResourcePolicy, we can remove it by ID
            msg = "AWS::Logs::ResourcePolicy is not yet supported in GovCloud; removing from template."
            cdk.Annotations.of(node).add_warning(msg)
            node.node.try_remove_child(node.node.default_child.node.id)

Unfortunately, I couldn't find a way to determine the current Partition before I ran out of brain power. Would be nice to add a Partition == aws-us-gov to this aspect so that it can be applied partition-agnostic.

@cmckni3
Copy link
Contributor Author

cmckni3 commented Sep 7, 2022

Ended up solving this with:

@jsii.implements(cdk.IAspect)
class RemoveLogsPolicyAspect:
    def visit(self, node: IConstruct) -> None:
        if isinstance(node, logs.ResourcePolicy):
            stack = cdk.Stack.of(node)
            # This Node is an L2 Construct for aws-cdk.aws-logs.ResourcePolicy
            # The default_child is of type AWS::Logs::ResourcePolicy, we can remove it by ID
            msg = "AWS::Logs::ResourcePolicy is not yet supported in GovCloud; removing from template."
            cdk.Annotations.of(node).add_warning(msg)
            node.node.try_remove_child(node.node.default_child.node.id)

Unfortunately, I couldn't find a way to determine the current Partition before I ran out of brain power. Would be nice to add a Partition == aws-us-gov to this aspect so that it can be applied partition-agnostic.

You can create a condition based on Aws.Partition

  new CfnCondition(scope, 'IsGovCloud', {
    expression: Fn.conditionEquals(Aws.PARTITION, 'aws-us-gov'),
  });

@cmckni3
Copy link
Contributor Author

cmckni3 commented Sep 7, 2022

@nom3ad - thanks for the suggestion! I have written an aspect to do this, but I have not been able to find any references on pruning a node from the resource tree. Could you provide some guidance here?

@jsii.implements(cdk.IAspect)
class RemoveLogsPolicyAspect:
    def visit(self, node: IConstruct) -> None:
        if isinstance(node, logs.CfnResourcePolicy):
            msg = "AWS::Logs::ResourcePolicy is not yet supported in GovCloud; removing from template."
            cdk.Annotations.of(node).add_warning(msg)
            # TODO: Remove node from stack

I'm not sure if that's correct. I haven't checked GovCloud in awhile but it should be available in us-gov-west-1.

@michaelfedell
Copy link
Contributor

^ @cmckni3 appreciate the suggestion, but I did try that and unfortunately Aws.PARTITION is a token and not resolvable in the context of this Aspect

@cmckni3
Copy link
Contributor Author

cmckni3 commented Sep 7, 2022

^ @cmckni3 appreciate the suggestion, but I did try that and unfortunately Aws.PARTITION is a token and not resolvable in the context of this Aspect

Yeah make sense depending on how you synth.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-iam Related to AWS Identity and Access Management @aws-cdk/aws-logs Related to Amazon CloudWatch Logs effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. needs-cfn This issue is waiting on changes to CloudFormation before it can be addressed.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants