WIP: Add Application Load Balancer Controller Manager#879
WIP: Add Application Load Balancer Controller Manager#879kamilprzybyl wants to merge 26 commits intomainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ske-prow
bot
added
do-not-merge/needs-kind
kamilprzybyl
changed the title
ske-prow
bot
added
the
do-not-merge/work-in-progress
kamilprzybyl
force-pushed
the
feat/kp/add-alb-ingress-controller
branch
from
7ecc416 to
86bdf1d
Compare
Added ExactMatch -Path- to change detection
| // generate self-signed certificates for the metrics server. While convenient for development and testing, | ||
| // this setup is not recommended for production. | ||
| // | ||
| // TODO(user): If you enable certManager, uncomment the following lines: |
There was a problem hiding this comment.
might be good to create separate examples for that
| sort.SliceStable(ruleMetadataList, func(i, j int) bool { | ||
| a, b := ruleMetadataList[i], ruleMetadataList[j] | ||
| // 1. Host name (lexicographically) | ||
| if a.host != b.host { |
pkg/alb/ingress/alb_spec.go
Outdated
| } | ||
|
|
||
| // cleanupCerts deletes the certificates from the Certificates API that are no longer associated with any Ingress in the IngressClass | ||
| func (r *IngressClassReconciler) cleanupCerts(ctx context.Context, ingressClass *networkingv1.IngressClass, ingresses []*networkingv1.Ingress) error { |
There was a problem hiding this comment.
do we really want to delete certificates? That could be very dangerous as users might just not associate the certificate anymore and want to keep it.
There was a problem hiding this comment.
Each Ingress TLS secret reference is mapped to a unique ALB Certificate resource. Even if multiple Ingresses reference the same k8s Secret, they will each trigger the creation of a separate, independent certificate on the ALB. That means, deleting one Ingress (and its associated ALB Certificate) does not impact the certificates still being used by other Ingresses.
pkg/alb/ingress/alb_spec.go
Outdated
|
|
||
| // isCertValid checks if the certificate chain is complete. It is used for checking if | ||
| // the cert-manager's ACME challenge is completed, or if it's sill ongoing. | ||
| func isCertValid(secret *corev1.Secret) (bool, error) { |
There was a problem hiding this comment.
isn't that already handled by the certificate api?
There was a problem hiding this comment.
The Cert API only checks if the public and private keys are matching + if the certificate is a valid X.509 format but it does not reject incomplete chains.
Renamed the function from isCertValid to isCertReady and clarified the function in the comment above.
ske-prow
bot
added
the
needs-rebase
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
| } | ||
|
|
||
| if len(albIngressList) < 1 { | ||
| err := r.handleIngressClassWithoutIngresses(ctx, albIngressList, ingressClass) |
There was a problem hiding this comment.
It doesn't make sense to pass albIngressList if it is always empty.
There was a problem hiding this comment.
You're right, passing an empty list is useless. And with that, the controller does not clean up certificates properly as the list is always empty.
Removed the redundant ingresses parameter from handleIngressClassWithoutIngresses since it was always empty. Updated cleanupCerts to identify and delete certificates using the IngressClass UID prefix, ensuring orphaned certificates are properly removed even when no Ingress resources exist.
kamilprzybyl
force-pushed
the
feat/kp/add-alb-ingress-controller
branch
from
4a5c760 to
d8de28a
Compare
kamilprzybyl
force-pushed
the
feat/kp/add-alb-ingress-controller
branch
from
d8de28a to
f903b25
Compare
… references; adjusted requeuing logic for certificates
|
We currently only delete certificates when the IngressClass is deleted or no Ingress references the IngressClass. |
|
@kamilprzybyl: The following test failed, say
Full PR test history. Your PR dashboard. Command help for this repository. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
4 participants
How to categorize this PR?
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Breaking changes: