Label-Based resource ownership - ALB and Certificates#1092
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 |
d1c0fba to
91b83ab
Compare
c7749a6 to
7af9e33
Compare
91b83ab to
a14b123
Compare
a14b123 to
f7de4f0
Compare
f7de4f0 to
152e114
Compare
Co-authored-by: ske-renovate-ce[bot] <163154779+ske-renovate-ce[bot]@users.noreply.github.com>
…ancer to v1.13.0 (#1104) Co-authored-by: ske-renovate-ce[bot] <163154779+ske-renovate-ce[bot]@users.noreply.github.com>
Co-authored-by: ske-renovate-ce[bot] <163154779+ske-renovate-ce[bot]@users.noreply.github.com>
Co-authored-by: ske-renovate-ce[bot] <163154779+ske-renovate-ce[bot]@users.noreply.github.com>
Co-authored-by: ske-renovate-ce[bot] <163154779+ske-renovate-ce[bot]@users.noreply.github.com>
Co-authored-by: ske-renovate-ce[bot] <163154779+ske-renovate-ce[bot]@users.noreply.github.com>
Co-authored-by: ske-renovate-ce[bot] <163154779+ske-renovate-ce[bot]@users.noreply.github.com>
Co-authored-by: ske-renovate-ce[bot] <163154779+ske-renovate-ce[bot]@users.noreply.github.com>
Co-authored-by: ske-renovate-ce[bot] <163154779+ske-renovate-ce[bot]@users.noreply.github.com>
Write gingko test for ingressclass_controller.go update done with "go mod tidy"
152e114 to
4ef3814
Compare
|
|
||
| // Add user labels, mind the limit | ||
| for k, v := range class.Labels { | ||
| if len(mergedLabels) < 64 { |
There was a problem hiding this comment.
Not sure if this makes sense to check the label count here. If you intention is to validate the input before we submit it to the api then we also should check the label name and value for length and invalid characters
e1f8620 to
cbb57dc
Compare
added tests for label key validation evict one item from the label list when the label item count exceeds 64 to make room for resource ownership label
| // LabelIngressClassUID is the unique key that identifies resources | ||
| // owned by a specific IngressClass. | ||
| LabelIngressClassUID = prefixALBIngressController + "ingress-class-uid" | ||
| maximumLabelCount = 64 |
There was a problem hiding this comment.
This limit applies to both ALB and NLB so maybe consider moving it to the shared labels package
| // prefixStackitInternalLabel is reserved for STACKIT internal labeling of resources that customer are not allowed to use | ||
| prefixStackitInternalLabel = "stackit-" |
There was a problem hiding this comment.
Same here, I think it makes sense moving it to the shared labels package.
| // evict one item to make room for the ownership label | ||
| if len(mergedLabels) >= 64 { | ||
| for k := range mergedLabels { | ||
| delete(mergedLabels, k) | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
We shouldn't silently drop customer labels, customers get confused. We should apply the ownership and global config labels first, then merge the customer labels last. If adding a customer label exceeds the 64-label limit, we return an error instead of silently evicting data.
There was a problem hiding this comment.
I think we should drop the label but also emit an event so the customer has a chance to spot whats going on
There was a problem hiding this comment.
this part could also be a bit more streamlined. Instead of removing a label again when you reach the maximum number you could add this label before you add the customer labels. And also validate the customer labels so that the controller forbids overwriting the LabelIngressClassUID key.
| errorList []errorEvents, | ||
| ) []errorEvents { | ||
| for k, v := range inputLabels { | ||
| if len(mergedLabels) >= 64 { |
There was a problem hiding this comment.
make use of the maximumLabelCount constant
| // evict one item to make room for the ownership label | ||
| if len(mergedLabels) >= 64 { | ||
| for k := range mergedLabels { | ||
| delete(mergedLabels, k) | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
this part could also be a bit more streamlined. Instead of removing a label again when you reach the maximum number you could add this label before you add the customer labels. And also validate the customer labels so that the controller forbids overwriting the LabelIngressClassUID key.
added tests for label key validation evict one item from the label list when the label item count exceeds 64 to make room for resource ownership label
|
@meneksece: 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. |
How to categorize this PR?
What this PR does / why we need it:
This PR introduces structured ownership for Application Load Balancer (ALB) and Certificate resources. By using a consistent labeling system based on the IngressClass UID, the controller can now easily find and clean up the resources it created.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Key Changes:
Ownership Labeling: Introduced LabelIngressClassUID (prefixed with lb.customer.label/) to track resource provenance.
Label Merging Logic: Added logic in getAlbSpecForResources to merge user-defined labels, global config labels, and the internal ownership label (with a safety limit of 64 labels).
Extended Interface: Updated applyCertificates to accept the IngressClass object to ensure certificates are also labeled upon creation.
Breaking changes: