feat: add support for load balancer, listener, and pool tags annotations#3058
feat: add support for load balancer, listener, and pool tags annotations#3058A10ss wants to merge 3 commits into
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 |
|
Welcome @A10ss! |
|
Hi @A10ss. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
b9d7293 to
6da530f
Compare
|
/ok-to-test |
kayrus
left a comment
There was a problem hiding this comment.
Thanks for the PR. See my initial comments.
| var desiredTags []string | ||
| if len(svcConf.lbTags) == 0 { | ||
| klog.V(4).Infof("No load balancer tags found from service annotation key: %s", ServiceAnnotationLoadBalancerTags) | ||
| } else if err := json.Unmarshal([]byte(svcConf.lbTags), &desiredTags); err != nil { |
There was a problem hiding this comment.
I don't like an idea to use json inside the annotation. Can you make it simpler? See an example
There was a problem hiding this comment.
Yep, the json format could support more complex tags, and for LB tags we may don't need very complex tags, so I think your point is right, I'll change it.
| } else { | ||
| if svcConf.supportLBTags { |
There was a problem hiding this comment.
This logic feels a bit hard to follow with the nested if statements. Would it be possible to simplify or refactor it to improve readability? The same applies to other parts of new code.
|
@kayrus please help review this PR again, thanks! |
|
Hi @kayrus, |
|
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
|
Hi @kayrus, Any updates on this? Thx! |
|
Hi. Apologies for the late response. Please update the initial PR message with a new tags format and add a documentation. Thinking out loud: this follows the spaghetti code style I'd like us to move away from in future (see #3076). I know this isn't your decision. The overall OCCM code style encourages contributors to develop patches this way. I believe at some point we'll tackle a code refactoring, and unit tests would help tremendously with that. Therefore, please add unit tests to this PR as well. |
|
@A10ss Can you include the requested changes by @kayrus, or shall @alexanderstephan or me take care of it? If we don't hear back from you until 20. July 2026, we will fork the PR and add the requested changes. |
yahor-kurachkin
left a comment
There was a problem hiding this comment.
I left a few stylistic suggestions. Other than that, it looks good to me. As kayrus mentioned, it also makes sense to add unit tests.
|
|
||
| // mergeTags merges existedTags and desiredTags, returns true if all desiredTags are in existedTags. | ||
| func mergeTags(existedTags []string, desiredTags []string) (bool, []string) { | ||
| if len(existedTags) == 0 || existedTags == nil { |
There was a problem hiding this comment.
existedTags == nil is unreachable because len(existedTags) returns 0 even for nil slice.
| if len(existedTags) == 0 || existedTags == nil { | |
| if len(existedTags) == 0 { |
| } else { | ||
| return false, tagSet.Union(desiredTagsSet).List() | ||
| } |
There was a problem hiding this comment.
| } else { | |
| return false, tagSet.Union(desiredTagsSet).List() | |
| } | |
| } | |
| return false, tagSet.Union(desiredTagsSet).List() |
There was a problem hiding this comment.
The else block in the end of the function is redundant and can be removed.
| var desiredTags []string | ||
| if len(svcConf.lbTags) == 0 { | ||
| klog.V(4).Infof("No load balancer tags found from service annotation key: %s", ServiceAnnotationLoadBalancerTags) | ||
| } else { | ||
| desiredTags = cpoutil.SplitTrim(svcConf.lbTags, ',') | ||
| } |
There was a problem hiding this comment.
Not a big deal, but I think this version is a bit cleaner. SplitTrim works fine with empty strings. And this block of code is repeated several times.
| var desiredTags []string | |
| if len(svcConf.lbTags) == 0 { | |
| klog.V(4).Infof("No load balancer tags found from service annotation key: %s", ServiceAnnotationLoadBalancerTags) | |
| } else { | |
| desiredTags = cpoutil.SplitTrim(svcConf.lbTags, ',') | |
| } | |
| desiredTags := cpoutil.SplitTrim(svcConf.lbTags, ',') | |
| if len(desiredTags) == 0 { | |
| klog.V(4).Infof("No load balancer tags found from service annotation key: %s", ServiceAnnotationLoadBalancerTags) | |
| } |
|
Hi @yahor-kurachkin, I integrated the review in #3149. Please take a look there! |
What this PR does / why we need it:
This PR adds support for the add tags to load balancer, listener, and pool via the service annotations in cloud-provider-openstack (OCCM).
Which issue this PR fixes(if applicable):
fixes #3032 #2327
Special notes for reviewers:
You can add annotations to the service like:
Then check if the tags have been added to the service's load blancer(the service's type should be load balancer)
Release note: