fix(elasticloadbalancingv2): logaccesslogs throws error if region is not set using environment #27938
Conversation
github-actions
bot
added
beginning-contributor
There was a problem hiding this comment.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.
jakob-lj
force-pushed
the
jakob-lj/fix-aws_base_loadbalancer_access_logs_region_bug
branch
from
b952bf0
to
9973189
Compare
jakob-lj
changed the title
aws-cdk-automation
dismissed
their stale review
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
aws-cdk-automation
added
the
pr/needs-maintainer-review
| @@ -307,7 +307,7 @@ export abstract class BaseLoadBalancer extends Resource { | |||
| protected resourcePolicyPrincipal(): iam.IPrincipal { | |||
| const region = Stack.of(this).region; | |||
| if (Token.isUnresolved(region)) { | |||
| throw new Error('Region is required to enable ELBv2 access logging'); | |||
| return new iam.ServicePrincipal('logdelivery.elasticloadbalancing.amazonaws.com'); | |||
There was a problem hiding this comment.
Only newer regions use this principal for elb logging (doc), and other (older) regions still require the elb-account-id for each region.
So with this change, you can synthesize an environment-agnostic template using elb access logging, but it won't work in those older regions. I guess that's not the desired result. Correct me if I misunderstand something 🙏
There was a problem hiding this comment.
I see.
Then the proposed solution in the issue wont work. I should have done some more research here before opening PR. Thank you for review @tmokmss . Would you like to inform in the issue as well?
There was a problem hiding this comment.
Sure I commented to the original issue, and thank you for working on the cdk issues!
When calling logAccessLogs on an Application Load balancer (v2), there is a check getting the region of the stack. This is later used for getting the account where the stack is deployed. However, if the account is undefined, the service principal (logdelivery.elasticloadbalancing.amazonaws.com) is used. So the region should not be required, as it is only used for finding the account, which is not required.
Therefore, it should make sense to return the service principal also if the region is undefined, which is described in the issue.
Closes #27432 .
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license