Copilot commented on code in PR #86:
URL:
https://github.com/apache/cloudstack-kubernetes-provider/pull/86#discussion_r2583776575
##########
cloudstack.go:
##########
@@ -95,9 +99,32 @@ func newCSCloud(cfg *CSConfig) (*CSCloud, error) {
return nil, errors.New("no cloud provider config given")
}
+ version, err := cs.getManagementServerVersion()
+ if err != nil {
+ return nil, err
+ }
+ cs.version = version
+
return cs, nil
}
+func (cs *CSCloud) getManagementServerVersion() (semver.Version, error) {
+ msServersResp, err :=
cs.client.Management.ListManagementServersMetrics(cs.client.Management.NewListManagementServersMetricsParams())
+ if err != nil {
+ return semver.Version{}, err
+ }
+ if msServersResp.Count == 0 {
+ return semver.Version{}, errors.New("no management servers
found")
+ }
+ version := msServersResp.ManagementServersMetrics[0].Version
+ v, err := semver.ParseTolerant(strings.Join(strings.Split(version,
".")[0:3], "."))
+ if err != nil {
+ klog.Errorf("failed to parse management server version: %v",
err)
+ return semver.Version{}, err
+ }
+ return v, nil
+}
Review Comment:
Missing test coverage: The new version detection functionality in
`getManagementServerVersion` lacks test coverage. Consider adding tests to
verify:
- Successful version parsing for typical version strings
- Handling of version strings with fewer than 3 components
- Error handling when no management servers are found
- Error handling when the API call fails
##########
cloudstack_loadbalancer.go:
##########
@@ -561,37 +567,110 @@ func (lb *loadBalancer) releaseLoadBalancerIP() error {
return nil
}
+func (lb *loadBalancer) getCIDRList(service *corev1.Service) ([]string, error)
{
+ sourceCIDRs := getStringFromServiceAnnotation(service,
ServiceAnnotationLoadBalancerSourceCidrs, defaultAllowedCIDR)
+ var cidrList []string
+ if sourceCIDRs != "" {
+ cidrList = strings.Split(sourceCIDRs, ",")
+ for i, cidr := range cidrList {
+ cidr = strings.TrimSpace(cidr)
+ if _, _, err := net.ParseCIDR(cidr); err != nil {
+ return nil, fmt.Errorf("invalid CIDR %s in
annotation %s: %w", cidr, ServiceAnnotationLoadBalancerSourceCidrs, err)
+ }
+ cidrList[i] = cidr
+ }
+ }
+ return cidrList, nil
+}
+
// checkLoadBalancerRule checks if the rule already exists and if it does, if
it can be updated. If
// it does exist but cannot be updated, it will delete the existing rule so it
can be created again.
-func (lb *loadBalancer) checkLoadBalancerRule(lbRuleName string, port
corev1.ServicePort, protocol LoadBalancerProtocol)
(*cloudstack.LoadBalancerRule, bool, error) {
+func (lb *loadBalancer) checkLoadBalancerRule(lbRuleName string, port
corev1.ServicePort, protocol LoadBalancerProtocol, service *corev1.Service,
version semver.Version) (*cloudstack.LoadBalancerRule, bool, error) {
lbRule, ok := lb.rules[lbRuleName]
if !ok {
return nil, false, nil
}
- // Check if any of the values we cannot update (those that require a
new load balancer rule) are changed.
- if lbRule.Publicip == lb.ipAddr && lbRule.Privateport ==
strconv.Itoa(int(port.NodePort)) && lbRule.Publicport ==
strconv.Itoa(int(port.Port)) {
- updateAlgo := lbRule.Algorithm != lb.algorithm
- updateProto := lbRule.Protocol != protocol.CSProtocol()
- return lbRule, updateAlgo || updateProto, nil
+ cidrList, err := lb.getCIDRList(service)
+ if err != nil {
+ return nil, false, err
}
- // Delete the load balancer rule so we can create a new one using the
new values.
- if err := lb.deleteLoadBalancerRule(lbRule); err != nil {
- return nil, false, err
+ var lbRuleCidrList []string
+ if lbRule.Cidrlist != "" {
+ lbRuleCidrList = strings.Split(lbRule.Cidrlist, " ")
+ for i, cidr := range lbRuleCidrList {
+ cidr = strings.TrimSpace(cidr)
+ lbRuleCidrList[i] = cidr
+ }
+ }
+
+ // Check if basic properties match (IP and ports). If not, we need to
recreate the rule.
+ basicPropsMatch := lbRule.Publicip == lb.ipAddr &&
+ lbRule.Privateport == strconv.Itoa(int(port.NodePort)) &&
+ lbRule.Publicport == strconv.Itoa(int(port.Port))
+
+ cidrListChanged := len(cidrList) != len(lbRuleCidrList) ||
!setsEqual(cidrList, lbRuleCidrList)
+
+ // Check if CIDR list also changed and version < 4.22, then we must
recreate the rule.
+ if !basicPropsMatch || (cidrListChanged &&
version.LT(semver.Version{Major: 4, Minor: 22, Patch: 0})) {
+ // Delete the load balancer rule so we can create a new one
using the new values.
+ if err := lb.deleteLoadBalancerRule(lbRule); err != nil {
+ return nil, false, err
+ }
+ return nil, false, nil
+ }
+
+ // Rule can be updated. Check what needs updating.
+ updateAlgo := lbRule.Algorithm != lb.algorithm
+ updateProto := lbRule.Protocol != protocol.CSProtocol()
+
+ return lbRule, updateAlgo || updateProto || cidrListChanged, nil
+}
+
+// setsEqual checks if two slices contain the exact same unique elements,
regardless of order.
+func setsEqual(listA, listB []string) bool {
+ createSet := func(list []string) map[string]bool {
+ set := make(map[string]bool)
+ for _, item := range list {
+ set[item] = true
+ }
+ return set
+ }
+
+ setA := createSet(listA)
+ setB := createSet(listB)
+
+ if len(setA) != len(setB) {
+ return false
}
- return nil, false, nil
+ for item := range setA {
+ if _, found := setB[item]; !found {
+ return false
+ }
+ }
+
+ return true
}
Review Comment:
Missing test coverage: The new `setsEqual` helper function lacks test
coverage. Consider adding unit tests to verify:
- Comparing sets with same elements in different order
- Comparing sets with different elements
- Handling of empty sets
- Handling of duplicate elements in input slices
##########
cloudstack_loadbalancer.go:
##########
@@ -561,37 +567,110 @@ func (lb *loadBalancer) releaseLoadBalancerIP() error {
return nil
}
+func (lb *loadBalancer) getCIDRList(service *corev1.Service) ([]string, error)
{
+ sourceCIDRs := getStringFromServiceAnnotation(service,
ServiceAnnotationLoadBalancerSourceCidrs, defaultAllowedCIDR)
+ var cidrList []string
+ if sourceCIDRs != "" {
+ cidrList = strings.Split(sourceCIDRs, ",")
+ for i, cidr := range cidrList {
+ cidr = strings.TrimSpace(cidr)
+ if _, _, err := net.ParseCIDR(cidr); err != nil {
+ return nil, fmt.Errorf("invalid CIDR %s in
annotation %s: %w", cidr, ServiceAnnotationLoadBalancerSourceCidrs, err)
+ }
+ cidrList[i] = cidr
+ }
+ }
+ return cidrList, nil
+}
+
// checkLoadBalancerRule checks if the rule already exists and if it does, if
it can be updated. If
// it does exist but cannot be updated, it will delete the existing rule so it
can be created again.
-func (lb *loadBalancer) checkLoadBalancerRule(lbRuleName string, port
corev1.ServicePort, protocol LoadBalancerProtocol)
(*cloudstack.LoadBalancerRule, bool, error) {
+func (lb *loadBalancer) checkLoadBalancerRule(lbRuleName string, port
corev1.ServicePort, protocol LoadBalancerProtocol, service *corev1.Service,
version semver.Version) (*cloudstack.LoadBalancerRule, bool, error) {
lbRule, ok := lb.rules[lbRuleName]
if !ok {
return nil, false, nil
}
- // Check if any of the values we cannot update (those that require a
new load balancer rule) are changed.
- if lbRule.Publicip == lb.ipAddr && lbRule.Privateport ==
strconv.Itoa(int(port.NodePort)) && lbRule.Publicport ==
strconv.Itoa(int(port.Port)) {
- updateAlgo := lbRule.Algorithm != lb.algorithm
- updateProto := lbRule.Protocol != protocol.CSProtocol()
- return lbRule, updateAlgo || updateProto, nil
+ cidrList, err := lb.getCIDRList(service)
+ if err != nil {
+ return nil, false, err
}
- // Delete the load balancer rule so we can create a new one using the
new values.
- if err := lb.deleteLoadBalancerRule(lbRule); err != nil {
- return nil, false, err
+ var lbRuleCidrList []string
+ if lbRule.Cidrlist != "" {
+ lbRuleCidrList = strings.Split(lbRule.Cidrlist, " ")
Review Comment:
Potential delimiter inconsistency: This code splits the load balancer rule's
CIDR list by space (" "), but firewall rules (line 900) split by comma (",").
If CloudStack returns CIDR lists in a consistent format across different rule
types, this inconsistency could cause the comparison to fail. Please verify
which delimiter CloudStack actually uses for load balancer rule CIDR lists and
ensure consistency.
```suggestion
lbRuleCidrList = strings.Split(lbRule.Cidrlist, ",")
```
##########
cloudstack_loadbalancer.go:
##########
@@ -561,37 +567,110 @@ func (lb *loadBalancer) releaseLoadBalancerIP() error {
return nil
}
+func (lb *loadBalancer) getCIDRList(service *corev1.Service) ([]string, error)
{
+ sourceCIDRs := getStringFromServiceAnnotation(service,
ServiceAnnotationLoadBalancerSourceCidrs, defaultAllowedCIDR)
+ var cidrList []string
+ if sourceCIDRs != "" {
+ cidrList = strings.Split(sourceCIDRs, ",")
+ for i, cidr := range cidrList {
+ cidr = strings.TrimSpace(cidr)
+ if _, _, err := net.ParseCIDR(cidr); err != nil {
+ return nil, fmt.Errorf("invalid CIDR %s in
annotation %s: %w", cidr, ServiceAnnotationLoadBalancerSourceCidrs, err)
+ }
+ cidrList[i] = cidr
+ }
+ }
+ return cidrList, nil
+}
+
// checkLoadBalancerRule checks if the rule already exists and if it does, if
it can be updated. If
// it does exist but cannot be updated, it will delete the existing rule so it
can be created again.
-func (lb *loadBalancer) checkLoadBalancerRule(lbRuleName string, port
corev1.ServicePort, protocol LoadBalancerProtocol)
(*cloudstack.LoadBalancerRule, bool, error) {
+func (lb *loadBalancer) checkLoadBalancerRule(lbRuleName string, port
corev1.ServicePort, protocol LoadBalancerProtocol, service *corev1.Service,
version semver.Version) (*cloudstack.LoadBalancerRule, bool, error) {
lbRule, ok := lb.rules[lbRuleName]
if !ok {
return nil, false, nil
}
- // Check if any of the values we cannot update (those that require a
new load balancer rule) are changed.
- if lbRule.Publicip == lb.ipAddr && lbRule.Privateport ==
strconv.Itoa(int(port.NodePort)) && lbRule.Publicport ==
strconv.Itoa(int(port.Port)) {
- updateAlgo := lbRule.Algorithm != lb.algorithm
- updateProto := lbRule.Protocol != protocol.CSProtocol()
- return lbRule, updateAlgo || updateProto, nil
+ cidrList, err := lb.getCIDRList(service)
+ if err != nil {
+ return nil, false, err
}
- // Delete the load balancer rule so we can create a new one using the
new values.
- if err := lb.deleteLoadBalancerRule(lbRule); err != nil {
- return nil, false, err
+ var lbRuleCidrList []string
+ if lbRule.Cidrlist != "" {
+ lbRuleCidrList = strings.Split(lbRule.Cidrlist, " ")
+ for i, cidr := range lbRuleCidrList {
+ cidr = strings.TrimSpace(cidr)
+ lbRuleCidrList[i] = cidr
+ }
+ }
+
+ // Check if basic properties match (IP and ports). If not, we need to
recreate the rule.
+ basicPropsMatch := lbRule.Publicip == lb.ipAddr &&
+ lbRule.Privateport == strconv.Itoa(int(port.NodePort)) &&
+ lbRule.Publicport == strconv.Itoa(int(port.Port))
+
+ cidrListChanged := len(cidrList) != len(lbRuleCidrList) ||
!setsEqual(cidrList, lbRuleCidrList)
+
+ // Check if CIDR list also changed and version < 4.22, then we must
recreate the rule.
+ if !basicPropsMatch || (cidrListChanged &&
version.LT(semver.Version{Major: 4, Minor: 22, Patch: 0})) {
+ // Delete the load balancer rule so we can create a new one
using the new values.
+ if err := lb.deleteLoadBalancerRule(lbRule); err != nil {
+ return nil, false, err
+ }
+ return nil, false, nil
+ }
+
+ // Rule can be updated. Check what needs updating.
+ updateAlgo := lbRule.Algorithm != lb.algorithm
+ updateProto := lbRule.Protocol != protocol.CSProtocol()
+
+ return lbRule, updateAlgo || updateProto || cidrListChanged, nil
+}
Review Comment:
Missing test coverage: The updated `checkLoadBalancerRule` function lacks
test coverage for the new CIDR list comparison logic. Consider adding tests to
verify:
- CIDR list changes trigger recreation on CloudStack < 4.22
- CIDR list changes trigger updates on CloudStack >= 4.22
- Empty CIDR list handling
- CIDR list comparison with different orderings
##########
cloudstack.go:
##########
@@ -95,9 +99,32 @@ func newCSCloud(cfg *CSConfig) (*CSCloud, error) {
return nil, errors.New("no cloud provider config given")
}
+ version, err := cs.getManagementServerVersion()
+ if err != nil {
+ return nil, err
+ }
+ cs.version = version
+
return cs, nil
}
+func (cs *CSCloud) getManagementServerVersion() (semver.Version, error) {
+ msServersResp, err :=
cs.client.Management.ListManagementServersMetrics(cs.client.Management.NewListManagementServersMetricsParams())
+ if err != nil {
+ return semver.Version{}, err
+ }
+ if msServersResp.Count == 0 {
+ return semver.Version{}, errors.New("no management servers
found")
+ }
+ version := msServersResp.ManagementServersMetrics[0].Version
+ v, err := semver.ParseTolerant(strings.Join(strings.Split(version,
".")[0:3], "."))
Review Comment:
Potential out-of-bounds panic: If the version string has fewer than 3
dot-separated components (e.g., "4.22"), `strings.Split(version, ".")[0:3]`
will panic with an index out of range error. Consider checking the length of
the split result before slicing, or handle this case more gracefully. For
example:
```go
parts := strings.Split(version, ".")
if len(parts) < 3 {
// pad with zeros or handle accordingly
}
versionStr := strings.Join(parts[0:min(len(parts), 3)], ".")
v, err := semver.ParseTolerant(versionStr)
```
```suggestion
parts := strings.Split(version, ".")
for len(parts) < 3 {
parts = append(parts, "0")
}
versionStr := strings.Join(parts[0:3], ".")
v, err := semver.ParseTolerant(versionStr)
```
##########
cloudstack_loadbalancer.go:
##########
@@ -561,37 +567,110 @@ func (lb *loadBalancer) releaseLoadBalancerIP() error {
return nil
}
+func (lb *loadBalancer) getCIDRList(service *corev1.Service) ([]string, error)
{
+ sourceCIDRs := getStringFromServiceAnnotation(service,
ServiceAnnotationLoadBalancerSourceCidrs, defaultAllowedCIDR)
+ var cidrList []string
+ if sourceCIDRs != "" {
+ cidrList = strings.Split(sourceCIDRs, ",")
+ for i, cidr := range cidrList {
+ cidr = strings.TrimSpace(cidr)
+ if _, _, err := net.ParseCIDR(cidr); err != nil {
+ return nil, fmt.Errorf("invalid CIDR %s in
annotation %s: %w", cidr, ServiceAnnotationLoadBalancerSourceCidrs, err)
+ }
+ cidrList[i] = cidr
+ }
+ }
+ return cidrList, nil
+}
Review Comment:
Missing test coverage: The new `getCIDRList` function lacks test coverage.
Consider adding unit tests to verify:
- Parsing of comma-separated CIDR lists
- Validation of invalid CIDR formats
- Handling of empty/missing annotations (should default to "0.0.0.0/0")
- Trimming of whitespace in CIDR entries
--
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]
