-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Support optimized security group rules for ALB #2205
Conversation
Skipping CI for Draft Pull Request. |
Codecov Report
@@ Coverage Diff @@
## main #2205 +/- ##
==========================================
+ Coverage 52.39% 53.18% +0.79%
==========================================
Files 133 135 +2
Lines 7257 7446 +189
==========================================
+ Hits 3802 3960 +158
- Misses 3158 3178 +20
- Partials 297 308 +11
Continue to review full report at Codecov.
|
/retest |
0fa8fe0
to
6547504
Compare
} | ||
|
||
func (p *defaultBackendSGProvider) isBackendSGRequired(ctx context.Context) (bool, error) { | ||
ingList := &networking.IngressList{} |
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.
personally i feel this depends on ingress implementation details too much.
also, seems the backendSG is not cleaned if there are some ingress in cluster even the left Ingresses don't need the backendSG.
shall we have a dedicated finalizer so that only ingresses/services needs backSG is added these finalizers?
if we have that, we can
call a dedicated function to test whether ingress/service needs backendSG, and then add this dedicated finalizer and invoke backendSGProvider.Get inside the group controller, and pass obtained backendSG into modelBuilder.
and then call backendSGProvider.Release if any group member have this finalizer, and remove the finalizer if it succeeds.
Or instead of add the finalizer in groupController, we have library some adds the finalizer before we create any cluster/controller-specific resources like backendSG, (e.g. make backendSGProvider.Get accepts a list of k8s resources as well).
i guess we could have a lot cluster/controller-specific resources like this backendSG in the future, e.g. a lambda function used to rewrite http requests for albs to do path rewrite)
6547504
to
ac0efe1
Compare
ac0efe1
to
cf095ae
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kishorj, M00nF1sh The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Issue
Issue: #2118
Description
Add support for using shared backend security groups for ALB. This will result in using a constant number of SG rules per cluster for the purpose of allowing access to ALB traffic. Also enable management of security group rules for manually specified security groups. For details refer to issue #2118.
Tests
alb.ingress.kubernetes.io/security-groups
annotation don't use the backend SG by defaultalb.ingress.kubernetes.io/manage-security-group-rules: "true"
annotation is applied to ingress with manual SG configuration, the backend SG gets attached to the corresponding ALB, and the node SG gets updated to allow traffic--backend-security-groups
in lieu of auto-generated onesChecklist
README.md
, or thedocs
directory)BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯