Copilot commented on code in PR #88:
URL:
https://github.com/apache/cloudstack-kubernetes-provider/pull/88#discussion_r3820293245
##########
cloudstack_loadbalancer.go:
##########
@@ -649,6 +688,60 @@ func (lb *loadBalancer) getCIDRList(service
*corev1.Service) ([]string, error) {
return cidrList, nil
}
+func (lb *loadBalancer) checkStickynessPolicy(lbRule
*cloudstack.LoadBalancerRule, service *corev1.Service)
(*cloudstack.LBStickinessPolicyStickinesspolicy, bool, error) {
+ stickynessPolicy := lb.stickynessPolicies[lbRule.Id]
+ stickynessMethodName := getStringFromServiceAnnotation(service,
ServiceAnnotationLoadBalancerStickynessMethodName, "")
+ stickynessMethodParam := getStringFromServiceAnnotation(service,
ServiceAnnotationLoadBalancerStickynessParam, "")
+ stickynessMethodParams := parseStickynessParams(stickynessMethodParam)
+
+ // If no policy exists and no method name is specified, no action needed
+ if stickynessPolicy == nil {
+ if stickynessMethodName == "" {
+ return nil, false, nil
+ }
+ klog.V(4).Infof("sticky policy not found for rule: %v",
lbRule.Name)
+ return nil, true, nil
+ }
+
+ // If policy exists but method name is not specified, policy should be
deleted
+ if stickynessMethodName == "" {
+ klog.V(4).Infof("sticky policy exists but annotation removed
for rule: %v", lbRule.Name)
+ return stickynessPolicy, true, nil
+ }
+
+ // Policy exists and method name is specified - check if it matches
+ klog.V(4).Infof("sticky policy found for rule: %v", lbRule.Name)
+ if stickynessPolicy.Methodname != stickynessMethodName {
+ klog.V(4).Infof("sticky policy method name does not match: %v",
lbRule.Name)
+ return stickynessPolicy, true, nil
+ }
+
+ // Check if params match
+ if len(stickynessPolicy.Params) != len(stickynessMethodParams) {
+ klog.V(4).Infof("sticky policy params length does not match:
%v", lbRule.Name)
+ return stickynessPolicy, true, nil
+ }
+
+ // Check if all keys in stickynessPolicy.Params match
stickynessMethodParams
+ for key, value := range stickynessPolicy.Params {
+ if stickynessMethodParams[key] != value {
+ klog.V(4).Infof("sticky policy param %v does not match:
%v", key, value)
+ return stickynessPolicy, true, nil
+ }
+ }
Review Comment:
`stickynessMethodParams[key]` returns the zero value ("") when the key is
missing, which can incorrectly treat a missing key as matching when the policy
value is also empty. Check key existence explicitly when comparing param maps.
This issue also appears on line 1280 of the same file.
##########
cloudstack_loadbalancer.go:
##########
@@ -56,6 +56,9 @@ const (
// associated the IP address. This annotation is set by the controller
when it associates
// an unallocated IP, and is used to determine if the IP should be
disassociated on deletion.
ServiceAnnotationLoadBalancerIPAssociatedByController =
"service.beta.kubernetes.io/cloudstack-load-balancer-ip-associated-by-controller"
//nolint:gosec
+
+ ServiceAnnotationLoadBalancerStickynessMethodName =
"service.beta.kubernetes.io/cloudstack-load-balancer-stickyness-method-name"
+ ServiceAnnotationLoadBalancerStickynessParam =
"service.beta.kubernetes.io/cloudstack-load-balancer-stickyness-method-param"
Review Comment:
The annotation keys and identifiers use the misspelling "stickyness"; for a
user-facing Kubernetes annotation this should be "stickiness" to match the
CloudStack terminology and avoid surprising users (see issue #75). Renaming
before release will prevent a long-lived typo in the public API surface.
##########
cloudstack_loadbalancer.go:
##########
@@ -462,6 +491,16 @@ func (cs *CSCloud) getLoadBalancer(service
*corev1.Service) (*loadBalancer, erro
lb.ipAddr = lbRule.Publicip
lb.ipAddrID = lbRule.Publicipid
+
+ lbStickinessPoliciesParams :=
cs.client.LoadBalancer.NewListLBStickinessPoliciesParams()
+ lbStickinessPoliciesParams.SetLbruleid(lbRule.Id)
+ lbStickinessPolicies, err :=
cs.client.LoadBalancer.ListLBStickinessPolicies(lbStickinessPoliciesParams)
+ if err != nil {
+ return nil, fmt.Errorf("error retrieving stickyness
policies: %v", err)
+ }
+ if len(lbStickinessPolicies.LBStickinessPolicies) > 0 {
+ lb.stickynessPolicies[lbRule.Id] =
&lbStickinessPolicies.LBStickinessPolicies[0].Stickinesspolicy[0]
+ }
Review Comment:
Indexing `LBStickinessPolicies[0].Stickinesspolicy[0]` can panic when the
outer slice is non-empty but the inner `Stickinesspolicy` slice is empty. Guard
both lengths before taking `[0]`.
This issue also appears on line 829 of the same file.
##########
cloudstack_loadbalancer.go:
##########
@@ -649,6 +688,60 @@ func (lb *loadBalancer) getCIDRList(service
*corev1.Service) ([]string, error) {
return cidrList, nil
}
+func (lb *loadBalancer) checkStickynessPolicy(lbRule
*cloudstack.LoadBalancerRule, service *corev1.Service)
(*cloudstack.LBStickinessPolicyStickinesspolicy, bool, error) {
+ stickynessPolicy := lb.stickynessPolicies[lbRule.Id]
+ stickynessMethodName := getStringFromServiceAnnotation(service,
ServiceAnnotationLoadBalancerStickynessMethodName, "")
+ stickynessMethodParam := getStringFromServiceAnnotation(service,
ServiceAnnotationLoadBalancerStickynessParam, "")
+ stickynessMethodParams := parseStickynessParams(stickynessMethodParam)
Review Comment:
New stickiness-policy behavior (parsing annotation params, deciding when to
create/delete/recreate policies, and calling the CloudStack API) is introduced
here, but there are no unit tests covering it. Since
`cloudstack_loadbalancer_test.go` exists, please add tests for
`parseStickynessParams` and `checkStickynessPolicy` (e.g., create, delete when
annotation removed, and update on param mismatch).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
