-
Notifications
You must be signed in to change notification settings - Fork 4.4k
fix(eks-v2-alpha): use custom security group from kubectlProviderOptions #36709
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
base: main
Are you sure you want to change the base?
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)
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
|
|
||||||||||||||
|
|
||||||||||||||
|
Hi @jasdeepbhalla, thanks for working on this! I'm actually facing this issue in my project right now and need a fix rather urgently. |
|
@letsgomeow Thanks for reaching out. I was just working on this. Pushed a change to fix the test issue. If it doesnt get resolved, feel free to push commits to this branch to fix it :) |
|
I've prepared a fix for the tests. Please check the PR here: jasdeepbhalla#1 |
Fix failing tests in cluster.test.ts
|
@letsgomeow Updated the PR with your suggestions, let me know if this looks good? |
|
Thanks for incorporating the tests!
|
|
@letsgomeow Updated the tests. Good Catch :) |
|
It looks like the CI is failing. As indicated in the error message, could you please run |
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)
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

Issue # (if applicable)
Closes #36653.
Reason for this change
Despite specifying a
securityGroupinkubectlProviderOptions, the EKS Cluster's security group was being applied to the Kubectl Handler (Lambda) instead of the intended custom security group. This prevented users from using custom security groups for the kubectl provider, which is important for network security configurations, especially when using private subnets and proxy configurations.Description of changes
Changes made:
cluster.ts(line 1293): AddedsecurityGroup: this._kubectlProviderOptions?.securityGroupto theKubectlProviderconstructor call. This ensures the security group specified inkubectlProviderOptionsis passed through to theKubectlProviderconstruct.kubectl-provider.ts(lines 155-161): Updated the security groups logic to:props.securityGroupis provided and use it when availableprops.cluster.clusterSecurityGroupwhen no custom security group is specifiedWhy these changes address the issue:
The root cause was that the
securityGroupproperty fromkubectlProviderOptionswas never being passed to theKubectlProviderconstructor, and the provider always defaulted to using the cluster's security group. By explicitly passing the security group and updating the provider's logic to prefer the custom security group, we now respect the user's configuration.Design decisions:
privateSubnetsare specified, which is the expected behavior for VPC-enabled Lambda functionsDescribe any new or updated permissions being added
No new or updated IAM permissions are required. This change only affects which security group is attached to the existing Kubectl Handler Lambda function. The Lambda function's IAM role and permissions remain unchanged.
Description of how you validated changes
Unit tests added:
kubectl provider uses custom security group when provided in kubectlProviderOptions: Verifies that when a custom security group is specified inkubectlProviderOptions.securityGroup, it is correctly applied to the Lambda function's VPC configuration instead of the cluster security group.kubectl provider falls back to cluster security group when custom security group is not provided: Ensures backward compatibility by verifying that when no custom security group is provided butprivateSubnetsare specified, the cluster security group is used as before.Both tests use the existing test infrastructure (
testFixture,Template.fromStack) and follow the same patterns as the existing security group test. The tests verify the Lambda function'sVpcConfig.SecurityGroupIdsproperty contains the expected security group reference.Manual validation:
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license