Skip to content

feat(opensearchservice): create AWS::Logs::ResourcePolicy instead of Custom::CloudwatchLogResourcePolicy #34558

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

wafuwafu13
Copy link
Contributor

@wafuwafu13 wafuwafu13 commented May 26, 2025

Issue

Related #5343

Reason for this change

We don't need to create a custom resource to set the log group resource policy since it is supported by CDK and cfn.

// Use a custom resource to set the log group resource policy since it is not supported by CDK and cfn.
// https://github.com/aws/aws-cdk/issues/5343
logGroupResourcePolicy = new LogGroupResourcePolicy(this, `ESLogGroupPolicy${this.node.addr}`, {
// create a cloudwatch logs resource policy name that is unique to this domain instance
policyName: `ESLogPolicy${this.node.addr}`,
policyStatements: [logPolicyStatement],
});

Description of changes

  • Create feature flag
  • Use new logs.ResourcePolicy instead of new LogGroupResourcePolicy when feature flag is set

Describe any new or updated permissions being added

None

Description of how you validated changes

Pass unit/integ tests

Checklist


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@aws-cdk-automation aws-cdk-automation requested a review from a team May 26, 2025 20:07
@github-actions github-actions bot added p2 admired-contributor [Pilot] contributed between 13-24 PRs to the CDK labels May 26, 2025
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

(This review is outdated)

@wafuwafu13
Copy link
Contributor Author

Exemption Request

integration test is updated

@aws-cdk-automation aws-cdk-automation added pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. labels May 26, 2025
@Abogical Abogical added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Jun 2, 2025
@aws-cdk-automation aws-cdk-automation dismissed their stale review June 2, 2025 12:07

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

@aemada-aws aemada-aws self-assigned this Jun 17, 2025
@aemada-aws aemada-aws self-requested a review June 17, 2025 12:20
// Use a custom resource to set the log group resource policy since it is not supported by CDK and cfn.
// https://github.com/aws/aws-cdk/issues/5343
logGroupResourcePolicy = new LogGroupResourcePolicy(this, `ESLogGroupPolicy${this.node.addr}`, {
logGroupResourcePolicy = new logs.ResourcePolicy(this, `ESLogGroupPolicy${this.node.addr}`, {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the contribution. This change is considered a breaking change because:

  • There is a possible conflict as CFN tries to create the resource policy before deleting the custom resource, resulting in a resource policy with the same name already exists error.
  • Deleting the custom resource first will cause a temporary period where permissions on the logs are lost, as on the delete lifecycle event of the custom resource we remove the policy. Ref:
    onDelete: {
    service: 'CloudWatchLogs',
    action: 'deleteResourcePolicy',
    parameters: {
    policyName: props.policyName,
    },
    ignoreErrorCodesMatching: 'ResourceNotFoundException',
    },

Therefore we should add a feature flag for this feature with recommended migration steps, and we can default it to enabled for new CDK projects, but we can't automatically enable this for existing cdk projects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for reviewing. I added a feature flag.

@mergify mergify bot dismissed aemada-aws’s stale review June 25, 2025 14:30

Pull request has been modified.

@wafuwafu13 wafuwafu13 marked this pull request as draft June 25, 2025 16:06
@wafuwafu13 wafuwafu13 marked this pull request as ready for review June 26, 2025 14:11
@wafuwafu13 wafuwafu13 requested a review from aemada-aws June 26, 2025 14:54

//////////////////////////////////////////////////////////////////////
[OPENSEARCHSERVICE_LOG_GROUP_RESOURCE_POLICY_WITHOUT_CUSTOM_RESOURCE]: {
type: FlagType.BugFix,
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a feature, not a bug fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please also rename the PR to indicate a feature, as this directly in the change log. Technically we are not fixing a bug in the previous implementation, we are improving it by using the L1 resource.

@@ -1660,6 +1661,19 @@ export const FLAGS: Record<string, FlagInfo> = {
recommendedValue: true,
compatibilityWithOldBehaviorMd: 'Disable the feature flag to let lambda service create logGroup or specify logGroup or logRetention',
},

//////////////////////////////////////////////////////////////////////
[OPENSEARCHSERVICE_LOG_GROUP_RESOURCE_POLICY_WITHOUT_CUSTOM_RESOURCE]: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
[OPENSEARCHSERVICE_LOG_GROUP_RESOURCE_POLICY_WITHOUT_CUSTOM_RESOURCE]: {
[OPENSEARCHSERVICE_CREATE_CLOUDFORMATION_RESOURCE_POLICY]: {

//////////////////////////////////////////////////////////////////////
[OPENSEARCHSERVICE_LOG_GROUP_RESOURCE_POLICY_WITHOUT_CUSTOM_RESOURCE]: {
type: FlagType.BugFix,
summary: 'When enabled, create log group resource policy without creating a custom resource.',
Copy link
Contributor

Choose a reason for hiding this comment

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

summary: 'When enabled, adds a log group resource policy using AWS::Logs::ResourcePolicy, without relying on a custom resource.,

@aws-cdk-automation aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jul 3, 2025
@wafuwafu13 wafuwafu13 changed the title fix(opensearchservice): create AWS::Logs::ResourcePolicy instead of Custom::CloudwatchLogResourcePolicy feat(opensearchservice): create AWS::Logs::ResourcePolicy instead of Custom::CloudwatchLogResourcePolicy Jul 5, 2025
@mergify mergify bot dismissed aemada-aws’s stale review July 5, 2025 20:06

Pull request has been modified.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: d82b2e3
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jul 5, 2025
@wafuwafu13 wafuwafu13 requested a review from aemada-aws July 6, 2025 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
admired-contributor [Pilot] contributed between 13-24 PRs to the CDK p2 pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. pr-linter/exempt-integ-test The PR linter will not require integ test changes pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants