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

feat: Added variable to control the create log group permission #514

Conversation

jake-naughton
Copy link
Contributor

@jake-naughton jake-naughton commented Nov 15, 2023

Description

Adding a variable to control the create log group permission on the lambda role. Default for the new variable is to add the permission to the role.

Motivation and Context

Described in the following feature request:
Fixes #503

Breaking Changes

None, variable default will keep current behaviour the same.

How Has This Been Tested?

  • I have updated at least one of the examples/* to demonstrate and validate my change(s)
  • I have tested and validated these changes using one or more of the provided examples/* projects
  • I have executed pre-commit run -a on my pull request

Tested running the example/complete module with init, plan, apply, destroy and inspected the iam roles to confirm that existing lambda roles still had the create log group permission and the new lambda role does not have it.

@@ -122,7 +122,7 @@ data "aws_iam_policy_document" "logs" {
effect = "Allow"

actions = compact([
!var.use_existing_cloudwatch_log_group ? "logs:CreateLogGroup" : "",
!var.use_existing_cloudwatch_log_group && var.attach_create_log_group_permission ? "logs:CreateLogGroup" : "",
Copy link
Member

Choose a reason for hiding this comment

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

If you want to reuse the existing cloudwatch log group (from previous runs), set use_existing_cloudwatch_log_group = true.

I am not sure that we need to have an identical variable attach_create_log_group_permission.

Right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The scenario I am dealing with is ephemeral environments. When I first create a new environment the cloudwatch log group does not exist. So I need use_existing_cloudwatch_log_group set to false. When I destroy the environment, the log group may or may not persist. I then want to recreate that environment at a later date which, if the group exists, will fail.

use_existing_cloudwatch_log_group only works if you know the CloudWatch group will be there, I do not.

As stated in the feature request, a work around would be to create the CloudWatch group separately. However, I would rather contain the logic to a single module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest, since we provision the CloudWatch group anyway, having CreateLogGroup on the Lambda seems redundant to me unless there is something I'm missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@antonbabenko thoughts?

Copy link
Member

@antonbabenko antonbabenko left a comment

Choose a reason for hiding this comment

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

Looks good to be merged. Thanks!

@antonbabenko antonbabenko merged commit c173c27 into terraform-aws-modules:master Nov 22, 2023
31 checks passed
antonbabenko pushed a commit that referenced this pull request Nov 22, 2023
## [6.5.0](v6.4.0...v6.5.0) (2023-11-22)

### Features

* Added variable to control the create log group permission ([#514](#514)) ([c173c27](c173c27))
@antonbabenko
Copy link
Member

This PR is included in version 6.5.0 🎉

Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add variable to manage the CreateLogGroup permission
2 participants