Copilot commented on code in PR #105:
URL:
https://github.com/apache/cloudstack-kubernetes-provider/pull/105#discussion_r3987730173
##########
cloudstack_loadbalancer.go:
##########
@@ -331,6 +336,10 @@ func (cs *CSCloud) EnsureLoadBalancerDeleted(ctx
context.Context, clusterName st
return err
}
+ if err := lb.deleteDuplicateRules(); err != nil {
+ klog.Errorf("Error removing duplicate load balancer rules for
%v/%v: %v", service.Namespace, service.Name, err)
+ }
Review Comment:
If duplicate cleanup fails here, logging and continuing returns nil after
deleting only `lb.rules`; the service finalizer can complete while the
duplicate rule and public IP remain in CloudStack. That reintroduces the
resource leak this change is intended to fix, especially for transient API
errors. Return the error so the service controller retries, or enqueue a
separate cleanup, rather than treating the sweep as best effort.
##########
cloudstack_loadbalancer.go:
##########
@@ -770,12 +799,76 @@ func (lb *loadBalancer) deleteLoadBalancerRule(lbRule
*cloudstack.LoadBalancerRu
return fmt.Errorf("error deleting load balancer rule %v: %v",
lbRule.Name, err)
}
- // Delete the rule from the map as it no longer exists
- delete(lb.rules, lbRule.Name)
+ // A duplicate shares its name with the rule being kept, which owns the
map entry.
+ if kept, ok := lb.rules[lbRule.Name]; ok && kept.Id == lbRule.Id {
+ delete(lb.rules, lbRule.Name)
+ }
+
+ return nil
+}
+
+// deleteDuplicateRules removes rules that collided by name with the one being
+// managed. A duplicate sits on its own public IP, since CloudStack rejects a
+// second rule on the same IP and port, so its firewall rule and IP go with it;
+// network ACL rules are shared per tier and port with the kept rule and stay.
+func (lb *loadBalancer) deleteDuplicateRules() error {
+ for _, lbRule := range lb.duplicateRules {
+ klog.V(4).Infof("Deleting duplicate load balancer rule: %v
(%v)", lbRule.Name, lbRule.Id)
+ if err := lb.deleteDuplicateRule(lbRule); err != nil {
+ return err
+ }
+ }
+ lb.duplicateRules = nil
+
+ return nil
+}
+
+// deleteDuplicateRule tears down one duplicate: its firewall rule, the rule
+// itself, and its public IP once no other rule uses that IP.
+func (lb *loadBalancer) deleteDuplicateRule(lbRule
*cloudstack.LoadBalancerRule) error {
+ port, err := strconv.Atoi(lbRule.Publicport)
+ protocol := ProtocolFromLoadBalancer(lbRule.Protocol)
+ if err != nil || protocol == LoadBalancerProtocolInvalid {
+ klog.Warningf("Leaving duplicate rule %v (%v) in place:
unusable public port %q or protocol %q", lbRule.Name, lbRule.Id,
lbRule.Publicport, lbRule.Protocol)
+ return nil
+ }
+ if _, err := lb.deleteFirewallRule(lbRule.Publicipid, port, protocol);
err != nil {
+ return err
Review Comment:
The duplicate sweep always calls `deleteFirewallRule`, even for a VPC rule.
The VPC topology in this PR deliberately provides only NetworkACL; the normal
deletion path first inspects the network and uses `deleteNetworkACLRule` for
that case. If `listFirewallRules` rejects a VPC-only IP, this returns before
deleting the duplicate rule/IP (and `EnsureLoadBalancer` fails), leaving the
leak the sweep is meant to fix. Branch on the network service as the existing
deletion path does, or otherwise avoid requiring the Firewall API for VPC
duplicates.
##########
hack/e2e/40-ccm-deploy.sh:
##########
@@ -0,0 +1,93 @@
+#!/usr/bin/env bash
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# Generates cloud-config files, loads the CCM image into kind, deploys
+# deployment.yaml and waits until all nodes are initialized.
+
+set -euo pipefail
+source "$(dirname "${BASH_SOURCE[0]}")/env.sh"
+source "${E2E_ROOT}/lib/log.sh"
+
+# shellcheck source=/dev/null
+source "${E2E_OUT}/keys.env" 2>/dev/null || die "missing ${E2E_OUT}/keys.env —
run 10-simulator-up.sh first"
+
+# The CCM pod must reach the simulator via its IP on the shared docker
+# network: pods cannot resolve docker's embedded DNS, and host.docker.internal
+# does not exist on Linux.
+sim_ip="$(docker inspect -f "{{(index .NetworkSettings.Networks
\"${E2E_NET}\").IPAddress}}" "$SIM_NAME")"
+[[ -n "$sim_ip" ]] || die "could not determine simulator IP on ${E2E_NET}"
+
+# PROJECT_ID is optional; 50-topology-vpc.sh re-runs this script with it set.
+project_line=""
+if [[ -n "${E2E_PROJECT_ID:-}" ]]; then
+ project_line="project-id = ${E2E_PROJECT_ID}"
+fi
+
+# In-cluster and host-process configs differ only in api-url.
+cat >"${E2E_OUT}/cloud-config" <<EOF
+[Global]
+api-url = http://${sim_ip}:8080/client/api
+api-key = ${CS_API_KEY}
+secret-key = ${CS_SECRET_KEY}
+zone = ${ZONE_NAME}
+region = ${E2E_REGION}
+${project_line}
+EOF
+sed "s|http://${sim_ip}:8080|http://localhost:${SIM_HOST_PORT}|" \
+ "${E2E_OUT}/cloud-config" >"${E2E_OUT}/cloud-config-host"
+chmod 600 "${E2E_OUT}/cloud-config" "${E2E_OUT}/cloud-config-host"
+
+# Build the image if it is not present (CI loads a prebuilt artifact instead).
+if ! docker image inspect "$CCM_IMAGE" >/dev/null 2>&1; then
+ log "building ${CCM_IMAGE}"
+ docker build -t "$CCM_IMAGE" "$REPO_ROOT"
+fi
Review Comment:
This unconditionally skips the build whenever `CCM_IMAGE` already exists.
After a developer changes the checkout and reruns `make e2e-up`, kind is
reloaded with the old image, so the advertised local e2e run can test stale CCM
code instead of the current checkout. Please distinguish an explicitly prebuilt
CI image from the local path (or otherwise force/recommend a rebuild when the
source changes).
##########
cloudstack_loadbalancer.go:
##########
@@ -470,24 +497,26 @@ func (cs *CSCloud) getLoadBalancer(service
*corev1.Service) (*loadBalancer, erro
}
// Get network ID from Public IP Address
+// Every failure returns an error: GetNetworkByID does not reject an empty ID
but
+// matches an unfiltered network list, so ("", nil) would resolve to any
network.
func (cs *CSCloud) getNetworkIDFromIPAddress(publicIpId string) (string,
error) {
- ip, count, err := cs.client.Address.GetPublicIpAddressByID(publicIpId)
+ ip, count, err := cs.client.Address.GetPublicIpAddressByID(publicIpId,
cloudstack.WithProject(cs.projectID))
if err != nil {
klog.Errorf("Failed to fetch the public IP for id: %v",
publicIpId)
return "", err
}
if count == 0 {
- return "", err
+ return "", fmt.Errorf("no public IP address found with ID %v",
publicIpId)
Review Comment:
The PR description lists the `getPublicIPAddress` count/slice panic as
fixed, but that method is unchanged: it still checks `l.Count` and then indexes
`l.PublicIpAddresses[0]` (cloudstack_loadbalancer.go:598-602). A response with
a nonzero count and an empty slice can still panic; guard the slice length and
add the regression test before claiming this fix.
--
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]
