-
Notifications
You must be signed in to change notification settings - Fork 4.3k
feat(eks): support AWS Load Balancer Controller 2.8.3 - 2.13.3 #34761
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
Conversation
There was a problem hiding this 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)
|
I tried to run I got the following error during EKS cluster creation |
|
This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. Note that PRs with failing linting check or builds are not reviewed, please ensure your build is passing To prevent automatic closure:
This PR will automatically close in 14 days if no action is taken. |
9743dba to
129082a
Compare
|
This PR adds multiple versions with new features - it might be a good idea to include some guidance for users to help them decide which one to use (or link some documentation to the version documentation). |
|
@pahud Need some guidance here: the PR refers to a previous PR which used a script to download the IAM policy files. Some of the policies grant broad permissions (resource: Do we need a security review to ensure that the default policies are safe? |
129082a to
a1bc09c
Compare
Totally agree. I don't think we should continue import everything from there as the policies is way too wide open. AnalysisAnalyzing the upstream v2.8.2 iam policies with potential CDK scoped implementation 1. Overly Broad Resource Permissions// Multiple statements with Resource: "*"
{
"Action": ["ec2:AuthorizeSecurityGroupIngress", "ec2:RevokeSecurityGroupIngress"],
"Resource": "*" // Can modify ANY security group in the account
}
{
"Action": ["elasticloadbalancing:CreateListener", "elasticloadbalancing:DeleteListener"],
"Resource": "*" // Can modify ANY load balancer listener
}2. Cross-Account/Cross-Region Access// Allows operations across ALL regions and accounts
"Resource": "arn:aws:elasticloadbalancing:*:*:targetgroup/*/*"3. Unnecessary Permissions for Many Use Cases• WAF/Shield permissions (not needed if not using WAF) Benefits of CDK Scoped Approach1. VPC-Scoped Security Groups// Instead of: "Resource": "*" for security group operations
// CDK generates:
{
"Action": ["ec2:AuthorizeSecurityGroupIngress"],
"Resource": [
"arn:aws:ec2:us-east-1:123456789012:security-group/*"
],
"Condition": {
"StringEquals": {
"ec2:vpc": "vpc-12345678" // Only this VPC
}
}
}2. Cluster-Specific Resource Tagging// Enhanced tagging conditions
{
"Condition": {
"StringEquals": {
"aws:RequestTag/kubernetes.io/cluster/my-cluster": "owned",
"aws:RequestTag/kubernetes.io/service-name": "${service-name}"
}
}
}3. Feature-Based Permission Sets// Only grant permissions for features actually used
const albController = new AlbController(this, 'ALB', {
cluster,
features: {
wafIntegration: false, // No WAF permissions
cognitoAuth: false, // No Cognito permissions
shieldProtection: false, // No Shield permissions
crossZoneLoadBalancing: true // Only ELB permissions
}
});4. Subnet-Scoped Load Balancer Creation// Instead of creating load balancers anywhere
{
"Action": ["elasticloadbalancing:CreateLoadBalancer"],
"Resource": "*",
"Condition": {
"StringEquals": {
"elasticloadbalancing:subnet": [
"subnet-12345678", // Only specific subnets
"subnet-87654321"
]
}
}
}Concrete Security BenefitsRisk Reduction Examples:
Migration Path with Immediate BenefitsFor migration path with compatibility, we probably want to introduce something like this: // Phase 1: Drop-in replacement with same broad permissions
albController: {
version: AlbControllerVersion.V2_8_2,
policyMode: 'compatible' // Uses broad permissions like upstream
}
// Phase 2: Opt-in to scoped permissions
albController: {
version: AlbControllerVersion.V2_8_2,
policyMode: 'scoped',
scope: { vpcId: vpc.vpcId }
}
// Phase 3: Feature-specific permissions
albController: {
version: AlbControllerVersion.V2_8_2,
policyMode: 'minimal',
features: ['basic-alb', 'ssl-termination']
}Or just stop supporting the |
I ran this command and it updates the snapshots (takes time though): I have the snapshots updated locally - I tried to push to your branch to make it easier, but I do not have permission. You can run the command above in your own workspace, or give me permission to merge to your branch - whatever works best for you. |
@arlampin take a look at the possible options here. We should scope down the permissions before merging this change. |
a1bc09c to
a25f6d0
Compare
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
c0c596c to
2566e51
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
3 similar comments
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
|
This PR has been in the BUILD FAILING state for 3 weeks, and looks abandoned. Note that PRs with failing linting check or builds are not reviewed, please ensure your build is passing To prevent automatic closure:
This PR will automatically close in 14 days if no action is taken. |
|
Hi @arlampin This PR has been stale for while, are you still working on it? If you need discussion, feel free to reach out to me on cdk.dev |
|
This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error. |
|
Comments on closed issues and PRs are hard for our team to see. |
Issue # (if applicable)
Closes #34724.
Reason for this change
AWS Load Balancer Controller project has released versions up to 2.13.3, CDK supports versions only up to 2.8.2
Description of changes
Changes were based on earlier v2.8.2 update PR #31264:
Description of how you validated changes
packages/aws-cdk-lib/aws-eks/test/alb-controller.test.tsthat tests all version definitions.Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license