Skip to content

feat: Add elb_log_delivery_policy_source_organizations variable #330

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 1 commit into
base: master
Choose a base branch
from

Conversation

szubersk
Copy link
Contributor

@szubersk szubersk commented Jun 5, 2025

Description

  • New elb_log_delivery_policy_source_organizations variable allows scoping access to the S3 bucket to AWS organization.

  • Improve security by scoping ALB/NLB/VPC flow logs access to the caller's AWS account.

  • Improve variable description. According to AWS documentation, the same S3 permissions can be used for both NLB access logs and VPC flow logs.

References:

Motivation and Context

  • Reuse existing code to provide VPC flow logs delivery capability.
  • Improve S3 access security.
  • Improve documentation around attachable policies.

Fix #324

Breaking Changes

N/A

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

@szubersk szubersk force-pushed the szubersk-flow-logs branch from e411b02 to adb163d Compare June 5, 2025 11:19
* New `elb_log_delivery_policy_source_organizations` variable allows scoping access to the S3 bucket to AWS organization.

* Improve security by scoping ALB/NLB/VPC flow logs access to the caller's AWS account.

* Improve variable description. According to AWS documentation, the same S3 permissions can be used for both NLB access logs and VPC flow logs.

References:
* https://docs.aws.amazon.com/vpc/latest/userguide/flow-logs-s3-permissions.html

* https://docs.aws.amazon.com/elasticloadbalancing/latest/network/enable-access-logs.html

* https://docs.aws.amazon.com/elasticloadbalancing/latest/application/enable-access-logging.html

Fix terraform-aws-modules#324

Signed-off-by: szubersk <[email protected]>
@szubersk szubersk force-pushed the szubersk-flow-logs branch from adb163d to 4570fd3 Compare June 9, 2025 01:27
@@ -622,7 +622,7 @@ data "aws_iam_policy_document" "elb_log_delivery" {
for_each = { for k, v in local.elb_service_accounts : k => v if k == data.aws_region.current.name }

content {
sid = format("ELBRegion%s", title(statement.key))
Copy link
Member

Choose a reason for hiding this comment

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

this is a breaking change and should not be modified at this time

@@ -5,13 +5,13 @@ variable "create_bucket" {
}

variable "attach_elb_log_delivery_policy" {
description = "Controls if S3 bucket should have ELB log delivery policy attached"
Copy link
Member

Choose a reason for hiding this comment

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

why are there all of these changes? ELB is correct - ALB and NLB are in fact ELB v2 (classic load balancer was ELB v1)

Copy link
Member

@bryantbiggs bryantbiggs left a comment

Choose a reason for hiding this comment

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

it looks like we are trying to do too much in this PR and I'm not quite following, nor do I agree with the name changes (i.e. ELB to ALB, etc.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

attach_lb_log_delivery_policy Does Not Include aws:SourceAccount and aws:SourceArn Checks
2 participants