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): Log Group ARN has extra :* #18253

Closed
kylelaker opened this issue Jan 3, 2022 · 21 comments
Closed

(logs): Log Group ARN has extra :* #18253

kylelaker opened this issue Jan 3, 2022 · 21 comments
Labels
@aws-cdk/aws-logs Related to Amazon CloudWatch Logs effort/small Small work item – less than a day of effort feature/enhancement A new API to make things easier or more intuitive. A catch-all for general feature requests. feature-request A feature should be added or improved. p1

Comments

@kylelaker
Copy link
Contributor

What is the problem?

The logGroupArn attribute for a LogGroup L2 construct has an extra :* at the end of the ARN. While this matches the behavior of CloudFormation's Arn attribute for an AWS::Logs::LogGroup, it does not match the documented structure for a Log Group. This breaks integration with other services, such as the creation of an AWS::WAFv2::LoggingConfiguration, which requires the ARN of the Log Group without :*.

Reproduction Steps

Build the following constructs within a Stack. (Sorry for the length; building a Web ACL is noisy)

import * as wafv2 from "aws-cdk-lib/aws-wafv2";
import * as logs from "aws-cdk-lib/aws-logs";

const acl = new waf.CfnWebACL(this, "ACL", {
  scope: "REGIONAL",
  defaultAction: {
    allow: {},
  },
  visibilityConfig: {
    sampledRequestsEnabled: true,
    cloudWatchMetricsEnabled: true,
    metricName: "SampleACLMetric",
  },
  rules: [
    {
      name: "RuleWithAWSManagedRules",
      priority: 0,
      overrideAction: {
        none: {},
      },
      statement: {
        managedRuleGroupStatement: {
          vendorName: "AWS",
          name: "AWSManagedRulesCommonRuleSet",
          excludedRules: [],
        },
      },
      visibilityConfig: {
        sampledRequestsEnabled: true,
        cloudWatchMetricsEnabled: true,
        metricName: "SampleAWSManagedRulesMetric",
      },
    },
  ],
});

const aclLogGroup = new logs.LogGroup(this, "ACLLogs", {
  // WAFv2 Log Groups must begin with `aws-waf-logs`. Including the ACL's
  // ID hopefully prevents name conflicts.
  logGroupName: `aws-waf-logs-${acl.attrId}`,
});

const logConfig = new wafv2.CfnLoggingConfiguration(scope, "WebAclLogging", {
  logDestinationConfigs: [aclLogGroup.logGroupArn],
  resourceArn: acl.attrArn,
});

What did you expect to happen?

The AWS::WAFv2::LoggingConfiguration resource should create successfully, referencing the logGroupArn attribute directly.

The LogGroup construct should provide the ARN in the format specified by the documentation, including

What actually happened?

The AWS::WAFv2::LoggingConfiguration resource failed to create with the error:

Resource handler returned message: "Error reason: The ARN isn't valid. A valid ARN begins with arn: and includes other information separated by colons or slashes., field: LOG_DESTINATION, parameter: arn:aws:logs:us-east-1:1234657890:log-group:aws-waf-logs-foo:*

The LogGroup.logGroupArn had an :* that was not documented as part of the ARN according to most documentation for the ARN of a Log Group.

CDK CLI Version

2.3.0 (build beaa5b2)

Framework Version

No response

Node.js Version

v14.18.2

OS

Linux

Language

Typescript

Language Version

No response

Other information

It may be more viable to fix this in other L2 constructs that need a log group than it is to fix it in the LogGroup construct itself at this point (and it doesn't seem that L2 constructs for the Web ACL logging configuration exist yet).

A workaround is to construct the ARN manually (with adjustments if the Log Group is in another account/region):

// The `.logGroupArn` attribute of `LogGroup` contains a `:*` at the end,
// which causes conflicts with the Web ACL Logging Configuration
const logGroupArn = Stack.of(this).formatArn({
  arnFormat: ArnFormat.COLON_RESOURCE_NAME,
  service: "logs",
  resource: "log-group",
  resourceName: aclLogGroup.logGroupName,
});
@kylelaker kylelaker added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jan 3, 2022
@github-actions github-actions bot added the @aws-cdk/aws-logs Related to Amazon CloudWatch Logs label Jan 3, 2022
@peterwoodworth
Copy link
Contributor

peterwoodworth commented Jan 4, 2022

I believe that this is intentional, since the arn comes directly from CloudFormation.

Here's where the arn is set

const resource = new CfnLogGroup(this, 'Resource', {
kmsKeyId: props.encryptionKey?.keyArn,
logGroupName: this.physicalName,
retentionInDays,
});
resource.applyRemovalPolicy(props.removalPolicy);
this.logGroupArn = this.getResourceArnAttribute(resource.attrArn, {
service: 'logs',
resource: 'log-group',
resourceName: this.physicalName,
arnFormat: ArnFormat.COLON_RESOURCE_NAME,
});
this.logGroupName = this.getResourceNameAttribute(resource.ref);

I'm not sure what we would want to do here, since I'm not sure why CloudFormation includes this in the arn in the first place. @comcalvi what do you think?

@peterwoodworth peterwoodworth added effort/small Small work item – less than a day of effort p2 feature-request A feature should be added or improved. feature/enhancement A new API to make things easier or more intuitive. A catch-all for general feature requests. and removed bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jan 4, 2022
@comcalvi
Copy link
Contributor

comcalvi commented Jan 5, 2022

This is likely a bug in WAFv2. The CFN property that causes the issue, LogDestinationConfigs, is rendered to this CFN when I synthesize the provided template:

          {
            "Fn::GetAtt": [
              "ACLLogs29F73701",
              "Arn"
            ]
          }
        ],

In the console, that log group has the :* at the end of their ARN. Since CloudFormation is retrieving the correct ARN, WAFv2 must not be handling the ARN parameter correctly. I'll contact the WAFv2 team.

Another workaround is to use Fn.split(':*') plus Fn.select(0) to get the ARN in the right format.

@ryanotella
Copy link

ryanotella commented Jan 27, 2022

Workaround example in CDKv2

const logGroup = new LogGroup(this, 'LogGroup', {
  logGroupName: `aws-waf-logs-firewall`,
  retention: RetentionDays.SIX_MONTHS,
  removalPolicy: RemovalPolicy.DESTROY,
});

const logDestination = Fn.select(0, Fn.split('*', logGroup.logGroupArn));

@peterwoodworth peterwoodworth added the needs-cfn This issue is waiting on changes to CloudFormation before it can be addressed. label Jan 27, 2022
@comcalvi comcalvi removed their assignment Jan 27, 2022
@BeatKO
Copy link

BeatKO commented Feb 10, 2022

Someone managed to solve it? i can't use the provided ARN, i get this error
Access log destination must only contain characters a-z, A-Z, 0-9, '_', '-', '/', '.': 'IDIDIDIDIDIDF:*'

@csumpter
Copy link
Contributor

csumpter commented Feb 10, 2022

@BeatKO

import * as cdk from "@aws-cdk/core";
import * as waf from "@aws-cdk/aws-wafv2";
import * as cwLogs from "@aws-cdk/aws-logs";

...

const loggingGroup = new cwLogs.LogGroup(this, "LogGroup", {
  // waf log group names must start with "aws-waf-logs-" or the stack will not deploy
  logGroupName: "aws-waf-logs-myname",
  retention: cwLogs.RetentionDays.ONE_MONTH,
  removalPolicy: cdk.RemovalPolicy.DESTROY,
});
new waf.CfnLoggingConfiguration(this, "LoggingConfiguration", {
  // the arn that comes back natively from the LogGroup has ":*" appended on the end of it, including it on the destination will cause a deploy time error
  logDestinationConfigs: [cdk.Fn.select(0, cdk.Fn.split(":*", loggingGroup.logGroupArn))],
  resourceArn: this.webAcl.attrArn,
});

@lacteolus
Copy link

It seems that workaround doesn't work anymore.
We started getting error for newly created stacks.

Resource handler returned message: "Unable to deliver logs to the configured destination. You might need to grant log delivery permissions for the destination. If you're using S3 as your log destination, you might have exceeded your bucket limit.

Old stacks deployed before are updated without issues.
It's hard to exact date or CDK version when it happened, but I guess it started a few days ago with CDK >2.12.0
Tried to switch back to using ARN:

Resource handler returned message: "Error reason: The ARN isn't valid. A valid ARN begins with arn: and includes other information separated by colons or slashes., field: LOG_DESTINATION, parameter

Can anyone confirm this issue even with workraround applied?

@newair
Copy link

newair commented Apr 19, 2022

It seems that workaround doesn't work anymore. We started getting error for newly created stacks.

Resource handler returned message: "Unable to deliver logs to the configured destination. You might need to grant log delivery permissions for the destination. If you're using S3 as your log destination, you might have exceeded your bucket limit.

Old stacks deployed before are updated without issues. It's hard to exact date or CDK version when it happened, but I guess it started a few days ago with CDK >2.12.0 Tried to switch back to using ARN:

Resource handler returned message: "Error reason: The ARN isn't valid. A valid ARN begins with arn: and includes other information separated by colons or slashes., field: LOG_DESTINATION, parameter

Can anyone confirm this issue even with workaround applied?

We faced the same issue. Started suddenly getting

Resource handler returned message: "Unable to deliver logs to the configured destination. You might need to grant log delivery permissions for the destination. If you're using S3 as your log destination, you might have exceeded your bucket limit

@lacteolus Did you find a solution for this?

Thanks !

@aidbal
Copy link

aidbal commented May 12, 2022

I just had the exact same issue.

We were tyring to deploy stack with StateMachine configured with logging but it kept failing with

Resource handler returned message: "The state machine IAM Role is not authorized to access the Log Destination
(Service: AWSStepFunctions; Status Code: 400; Error Code: AccessDeniedException;
Request ID: 1bcfb19b-43a2-4007-a3d4-9eb4b30e7a96; Proxy: null)"
(RequestToken: 4a54f63f-a4df-7112-c6e6-ae7f9fd45669, HandlerErrorCode: AccessDenied)

For CloudFormation stacks we are using L1 and L2 AWS CDK Constructs.

We create an L1 LogGroup with CfnLogGroup construct.

But we are using L2 StateMachine construct.

Since L2 StateMachine construct only accepts ILogGroup, we have to use either LogGroup.fromLogGroupArn or LogGroup.fromLogGroupName.

We were using it like this:

const l1LogGroup = new CfnLogGroup(this, "LogGroupL1")

const stateMachine = new StateMachine(this, "StateMachine", {
  ...
  logs: {
    destination: LogGroup.fromLogGroupArn(this, "StateMachineLogGroupL2", l1LogGroup.attrArn),
    level: LogLevel.ALL
  }
})

Turns out, CfnLogGroup.attrArn returns ARN with :* attached at the end (https://docs.aws.amazon.com/cdk/api/v1/docs/@aws-cdk_aws-logs.CfnLogGroup.html#attrarn ):

attrArn
Type: string
The ARN of the log group, such as arn:aws:logs:us-west-1:123456789012:log-group:/mystack-testgroup-12ABC1AB12A1:*.

But the problem arises when LogGroup.fromLogGroupArn also attaches :* at the end:

declare abstract class LogGroupBase extends Resource implements ILogGroup {
    /**
     * The ARN of this log group, with ':*' appended
     */
    abstract readonly logGroupArn: string;

Which is in my opinion wrong, in the AWS console, the LogGroup ARN has the :* attached, so I don't understand why LogGroup.fromLogGroupArn also appends one more :* at the end. (You should take resource ARN as it is in the console and create a construct, not do more magic and append some trivial values at the end)

So when we made this call:

LogGroup.fromLogGroupArn(this, "StateMachineLogGroupL2", l1LogGroup.attrArn)

The Log Group ARN passed to StateMachine had :*:* at the end. Which was the reason of the failed deployments.

I have fixed this issue by constructing L2 LogGroup construct from name:

LogGroup.fromLogGroupName(this, "StateMachineLogGroupL2", logGroup.logGroupName)

But in my opinion LogGroup.fromLogGroupArn should be fixed.

@comcalvi
Copy link
Contributor

comcalvi commented May 18, 2022

@aidbal the behavior you're describing is working as intended. In the past, fromLogGroupArn() would return the log group ARNs without any :* at the end, which would result in log groups having incorrect permissions. Unfortunately this is one of those cases where the L1 of one service and the L2 of another don't play nice, so the solution you described (creating the L2 from the L1, and passing that instead) is the intended behavior here. See this PR for details.

@aidbal
Copy link

aidbal commented May 20, 2022

Hi @comcalvi, thanks for linking me the PR.

I agree that there was a need to patch the bug which caused incorrect permissions.

I think that your patch created another bug (the one I mentioned) unintentionally.

And it's not about mixing L1 and L2, I can easily break your bug fix by only using L2 constructs (explained at the end)

Why the bug fix is not complete

Here you guys check if the arn has :* attached, if so, you replace it with empty string.

However, you don't put into the consideration that the ARN might be a token and so your regex might not find anything.

I think the better approach there would have been to check if it's a token. If so, don't append any :*. If not, search for :* and replace it with empty string, then append :* at the end.

I think this makes the most sense as the LogGroupArn in AWS Console shows up with :* at the end:

AWS Console LogGroup ARN

Meaning the user expects the ARN of the LogGroup to always be the one with :* attached in the end. Thus, any CloudFormation values (i.e. Tokens), referring to LogGroupArn, will always represent the value with :* at the end (at least to the customer's eyes). No need to append any additional values.

Breaking your bug fix with L2 only

If you don't fix this bug and say that it only affects it when L1 and L2 are mixed together, I can easily create L2 Log group and get the ARN from that:

const logGroupL2 = new LogGroup(this, "LogGroupL2")

const stateMachine = new StateMachine(this, "StateMachine", {
  ...
  logs: {
    destination: LogGroup.fromLogGroupArn(this, "LogGroupFromArn", logGroupL2.logGroupArn)
    level: LogLevel.ALL
  }
})

This results in the following CloudFormation code:

              "CloudWatchLogsLogGroup": {
                "LogGroupArn": {
                  "Fn::Join": [
                    "",
                    [
                      {
                        "Fn::GetAtt": [
                          "LogGroupL255E4F451",
                          "Arn"
                        ]
                      },
                      ":*"
                    ]
                  ]
                }
              }

No L1 construct being mixed in, but still breaks your bug fix.

@alexandervandekleutab
Copy link

What is the status of this bug? Is it still not possible to create an ACL via CDK with a cloudwatch log group?

@stanmarsh2
Copy link

stanmarsh2 commented Aug 23, 2022

Resource handler returned message: "Unable to deliver logs to the configured destination. You might need to grant log delivery permissions for the destination. If you're using S3 as your log destination, you might have exceeded your bucket limit

@lacteolus Did you find a solution for this?

Yes, it's a different issue.
AWS creates resource based policies for us, but it has length limit.
Later after limit is exceeded we need to create it by ourselves or clean it up.

  • Manual clean-up: aws logs delete-resource-policy --policy-name ...

  • Info on how to create Resource policy in CDK:
    Resource policy docs: link
    CDK Api: logGroup.addToResourcePolicy(...)
    Cloudformation resource type: AWS::Logs::ResourcePolicy
    More info and all kudos to @lorelei-rupp-imprivata : link

@richard-cp
Copy link

richard-cp commented Feb 29, 2024

Did anyone find a solution to this?

I get the same error:

Resource handler returned message: "Error reason: The ARN isn't valid. A valid ARN begins with arn: and includes other information separated by colons or slashes., field: RESOURCE_ARN

No matter how I format the ARN.

I have tried the mentioned workaround [cdk.Fn.select(0, cdk.Fn.split(":*", loggingGroup.logGroupArn))] but get the same error.

In my case the log group already exists, and have tried specifying the ARN 6 different ways, hardcoding it to get around the L1 & L2 constructs "not playing nice" - as mentioned in a post above.

Example CDK snippet with the different ARNs attempted:

const loggingConfig = new wafv2.CfnLoggingConfiguration(
this,
`webacl-logging-config`,
  {
	  resourceArn: webAcl.ref,
	  logDestinationConfigs: ['arn:aws:logs:us-east-1:xxxxxxxxx:log-group:app-webacl-logs'],
      // logDestinationConfigs: ['arn:aws:logs:us-east-1:xxxxxxxxx:log-group:app-webacl-logs:'],
      // logDestinationConfigs: ['arn:aws:logs:us-east-1:xxxxxxxxx:log-group:app-webacl-logs:*'],
      // logDestinationConfigs: ['arn:aws:logs:us-east-1:xxxxxxxxx:log-group:/app-webacl-logs'],
      // logDestinationConfigs: ['arn:aws:logs:us-east-1:xxxxxxxxx:log-group:/app-webacl-logs:'],
      // logDestinationConfigs: ['arn:aws:logs:us-east-1:xxxxxxxxx:log-group:/app-webacl-logs:*'],
  },
);

I have also tried another approach of using an s3 bucket as the logDestinationConfigs, for this approach I use a Kinesis stream for log delivery to s3, the stream has the managed policy AWSWAFFullAccess on it role, and the log destination is configured as follows: logDestinationConfigs: [deliveryStream.attrArn] - However, this also throws the same error as the cloudwatch log group as above.

Not sure I understand what the problem is here - has anyone got a workaround that still works?

Update: I have resolved this.
I was referencing the resourceArn: webAcl.ref incorrectly. The ref does not return the arn in CDK. You must instead reference resourceArn: webAcl.attrArn.
For the logDestinationConfigs, I did not need to split or strip the * out. Referencing the cloudwatch log group like this logDestinationConfigs: [logGroup.logGroupArn], worked fine. Even with a mix of L1 & L2 constructs. (LogGroup L2, LoggingConfiguration L1)

@ryanotella
Copy link

Probably related to the Log Group name requirement?

https://docs.aws.amazon.com/waf/latest/developerguide/logging-cw-logs.html#logging-cw-logs-naming

@arash-cid
Copy link

This is so stupid.

The error is the Log group Name must start with aws-waf-logs-

F AWS, F Microsoft

@pahud
Copy link
Contributor

pahud commented Mar 13, 2024

Just bumped this issue to p1 and self-assigned. I will be looking into this issue and escalate to the team when necessary.

@pahud pahud added p1 and removed p2 labels Mar 13, 2024
@pahud pahud self-assigned this Mar 13, 2024
@pahud
Copy link
Contributor

pahud commented Mar 13, 2024

I am able to cdk deploy with the provided sample in the original post.

export class DummyStack extends Stack {
  constructor(scope: Construct, id: string, props: StackProps) {
    super(scope, id, props);


    const acl = new wafv2.CfnWebACL(this, "ACL", {
      scope: "REGIONAL",
      defaultAction: {
        allow: {},
      },
      visibilityConfig: {
        sampledRequestsEnabled: true,
        cloudWatchMetricsEnabled: true,
        metricName: "SampleACLMetric",
      },
      rules: [
        {
          name: "RuleWithAWSManagedRules",
          priority: 0,
          overrideAction: {
            none: {},
          },
          statement: {
            managedRuleGroupStatement: {
              vendorName: "AWS",
              name: "AWSManagedRulesCommonRuleSet",
              excludedRules: [],
            },
          },
          visibilityConfig: {
            sampledRequestsEnabled: true,
            cloudWatchMetricsEnabled: true,
            metricName: "SampleAWSManagedRulesMetric",
          },
        },
      ],
    });
    
    const aclLogGroup = new logs.LogGroup(this, "ACLLogs", {
      // WAFv2 Log Groups must begin with `aws-waf-logs`. Including the ACL's
      // ID hopefully prevents name conflicts.
      logGroupName: `aws-waf-logs-${acl.attrId}`,
    });
    
    const logConfig = new wafv2.CfnLoggingConfiguration(this, "WebAclLogging", {
      logDestinationConfigs: [aclLogGroup.logGroupArn],
      resourceArn: acl.attrArn,
    });


  }
}

The AWS::WAFv2::LoggingConfiguration resource was created with no error.

I assume this issue does not exist anymore.

Please comment if does.

% npx cdk --version
2.132.0 (build 9a51c89)

@pahud pahud added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. and removed needs-cfn This issue is waiting on changes to CloudFormation before it can be addressed. labels Mar 13, 2024
@mo7ty
Copy link

mo7ty commented Mar 13, 2024

I assume this issue does not exist anymore.

Probably CDK might be already handling LogGroup's ARN with extra :*, or AWS::WAFv2::LoggingConfiguration accepting it, because the ARN of a LogGroup still carries it.

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Mar 13, 2024
@emanserav
Copy link

emanserav commented Apr 8, 2024

hi @pahud, I confirm is also working for me

cdk version 
2.133.0 (build dcc1e75)

PS: I had a typo before in the WAFv2 Log Group name, I am mentioning this for others as well even if it was already mentioned in this thread at least a few times, please take note on:
!!! WAFv2 CloudWatch LogGroup must begin with aws-waf-logs- !!!

@pahud
Copy link
Contributor

pahud commented Apr 17, 2024

As this issue doesn't seem to existing anymore. I am closing it now.
Feel free to create a new issue if there's any concern.

@pahud pahud closed this as completed Apr 17, 2024
@pahud pahud removed their assignment Apr 17, 2024
Copy link

⚠️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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-logs Related to Amazon CloudWatch Logs effort/small Small work item – less than a day of effort feature/enhancement A new API to make things easier or more intuitive. A catch-all for general feature requests. feature-request A feature should be added or improved. p1
Projects
None yet
Development

No branches or pull requests