Skip to content

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.)

Copy link

This PR has been automatically marked as stale because it has been open 30 days
with no activity. Remove stale label or comment or this PR will be closed in 10 days

@github-actions github-actions bot added the stale label Jul 27, 2025
Copy link

github-actions bot commented Aug 7, 2025

This PR was automatically closed because of stale in 10 days

@github-actions github-actions bot closed this Aug 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
3 participants