Copilot commented on code in PR #105: URL: https://github.com/apache/cloudstack-kubernetes-provider/pull/105#discussion_r3903106226
########## test/e2e/annotations_test.go: ########## @@ -0,0 +1,167 @@ +//go:build e2e + +/* + * 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. + */ + +package e2e + +import ( + "context" + "strings" + "testing" + + "github.com/blang/semver/v4" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +const ( + annotationSourceCidrs = "service.beta.kubernetes.io/cloudstack-load-balancer-source-cidrs" + annotationHostname = "service.beta.kubernetes.io/cloudstack-load-balancer-hostname" + annotationIPAssociated = "service.beta.kubernetes.io/cloudstack-load-balancer-ip-associated-by-controller" //nolint:gosec +) + +func TestAnnot_SourceCIDRs(t *testing.T) { + f := NewFramework(t) + svc := f.CreateLBService(func(s *corev1.Service) { + s.Annotations = map[string]string{ + annotationSourceCidrs: "10.0.0.0/8,192.168.100.0/24", + } + }) + lbName := defaultLoadBalancerName(svc) + + f.WaitForIngressIP(svc) + rules := f.WaitForLBRules(lbName, 1) + for _, cidr := range []string{"10.0.0.0/8", "192.168.100.0/24"} { + if !strings.Contains(rules[0].Cidrlist, cidr) { + t.Errorf("rule cidrlist = %q, want it to contain %s", rules[0].Cidrlist, cidr) + } + } + originalRuleID := rules[0].Id + + // Change the CIDR list. On >= 4.22 the rule is updated in place (same + // ID); on older versions it is deleted and recreated (new ID). + f.UpdateService(svc, func(s *corev1.Service) { + s.Annotations[annotationSourceCidrs] = "172.16.0.0/12" + }) + inPlace := f.Version.GTE(semver.Version{Major: 4, Minor: 22, Patch: 0}) + f.Eventually(lbSyncTimeout, lbSyncInterval, "cidr list update to propagate", + func() (bool, error) { + current, err := f.LBRules(lbName) + if err != nil || len(current) != 1 { + return false, err + } + if !strings.Contains(current[0].Cidrlist, "172.16.0.0/12") { + return false, nil + } + if inPlace && current[0].Id != originalRuleID { + t.Errorf("expected in-place cidr update on %s (rule ID changed %s -> %s)", + f.Version, originalRuleID, current[0].Id) + } + if !inPlace && current[0].Id == originalRuleID { + t.Errorf("expected rule recreation on %s (rule ID unchanged)", f.Version) + } + return true, nil + }) Review Comment: Calling `t.Errorf` inside the `Eventually` polling function can produce repeated failures across multiple poll iterations (noisy logs) and mixes 'wait until propagated' with 'assert invariants' logic. A cleaner approach is to keep the poll focused on the propagation condition (CIDR present), then perform the in-place vs recreate assertion once afterward using the final rule state (or return a hard error from the condition so the poll stops immediately on a definitive mismatch). ########## cloudstack_loadbalancer.go: ########## @@ -469,25 +469,33 @@ func (cs *CSCloud) getLoadBalancer(service *corev1.Service) (*loadBalancer, erro return lb, nil } -// Get network ID from Public IP Address +// getNetworkIDFromIPAddress returns the ID of the network that a public IP +// address is associated with. +// +// Every failure path returns a non-nil error. Callers pass the result straight +// to GetNetworkByID, where an empty ID is not rejected but looked up as an +// unfiltered network list, so returning ("", nil) would silently resolve to an +// arbitrary network instead of reporting the failure. 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 } Review Comment: These log lines drop the underlying error value, which makes controller logs much harder to debug (you only see the ID, not the reason). Include the error in the log message (e.g., format it as `...: %v`), or switch to structured logging if that’s the convention in this repo for `klog` usage. ########## cloudstack_loadbalancer.go: ########## @@ -469,25 +469,33 @@ func (cs *CSCloud) getLoadBalancer(service *corev1.Service) (*loadBalancer, erro return lb, nil } -// Get network ID from Public IP Address +// getNetworkIDFromIPAddress returns the ID of the network that a public IP +// address is associated with. +// +// Every failure path returns a non-nil error. Callers pass the result straight +// to GetNetworkByID, where an empty ID is not rejected but looked up as an +// unfiltered network list, so returning ("", nil) would silently resolve to an +// arbitrary network instead of reporting the failure. 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("could not find public IP address %v", publicIpId) } - if ip.Networkid != "" { - network, _, netErr := cs.client.Network.GetNetworkByID(ip.Associatednetworkid) - if netErr != nil { - klog.Errorf("Failed to fetch the network for id: %v", ip.Associatednetworkid) - return "", err - } - return network.Id, nil + if ip.Associatednetworkid == "" { + return "", fmt.Errorf("public IP address %v is not associated with a network", publicIpId) + } + + network, _, err := cs.client.Network.GetNetworkByID(ip.Associatednetworkid, cloudstack.WithProject(cs.projectID)) + if err != nil { + klog.Errorf("Failed to fetch the network for id: %v", ip.Associatednetworkid) + return "", err } Review Comment: These log lines drop the underlying error value, which makes controller logs much harder to debug (you only see the ID, not the reason). Include the error in the log message (e.g., format it as `...: %v`), or switch to structured logging if that’s the convention in this repo for `klog` usage. ########## Makefile: ########## @@ -53,6 +59,22 @@ ifneq (${GIT_IS_TAG},NOT_A_TAG) docker tag apache/cloudstack-kubernetes-provider:${GIT_COMMIT_SHORT} apache/cloudstack-kubernetes-provider:${GIT_TAG} endif +# Simulator-based e2e environment; see docs/development.md +e2e-up: + hack/e2e/up.sh Review Comment: As written, `SIM_HOST_PORT` / `CS_API_URL` are Make variables but are not exported to the environment for the `e2e-up` recipe. That means `SIM_HOST_PORT=8081 make e2e-up` likely won’t affect `hack/e2e/env.sh` (which reads environment variables), so users can end up with `make e2e-up` and `make test-e2e` disagreeing about the published port. Consider exporting these variables (or prefixing them on the `e2e-up` command invocation) so Make overrides reliably propagate into the harness scripts. ########## test/e2e/framework.go: ########## @@ -0,0 +1,452 @@ +//go:build e2e + +/* + * 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. + */ + +// Package e2e contains end-to-end tests that run against a live Kubernetes +// cluster whose cloud-controller-manager talks to a CloudStack management +// server (normally the simulator brought up by hack/e2e/up.sh). +// +// Configuration comes from the environment: +// +// KUBECONFIG kubeconfig of the cluster under test +// CS_API_URL CloudStack API endpoint (as reachable from the test process) +// CS_API_KEY CloudStack API key +// CS_SECRET_KEY CloudStack secret key +// CS_PROJECT_ID optional project scoping (set for the VPC phase) +// +// When any required variable is missing, the tests skip. +package e2e + +import ( + "context" + "crypto/rand" + "encoding/hex" + "fmt" + "os" + "strconv" + "strings" + "testing" + "time" + + "github.com/apache/cloudstack-go/v2/cloudstack" + "github.com/blang/semver/v4" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes" + "k8s.io/client-go/tools/clientcmd" +) + +const ( + lbSyncTimeout = 3 * time.Minute + lbSyncInterval = 3 * time.Second +) + +// Framework bundles the clients and helpers shared by all e2e tests. +type Framework struct { + T *testing.T + K8s kubernetes.Interface + CS *cloudstack.CloudStackClient + Namespace string + ProjectID string + Version semver.Version +} + +// NewFramework builds clients from the environment, skipping the test when +// the environment is not configured. It creates a per-test namespace that is +// deleted on cleanup. +func NewFramework(t *testing.T) *Framework { + t.Helper() + + apiURL := os.Getenv("CS_API_URL") + apiKey := os.Getenv("CS_API_KEY") + secretKey := os.Getenv("CS_SECRET_KEY") + if apiURL == "" || apiKey == "" || secretKey == "" { + t.Skip("CS_API_URL/CS_API_KEY/CS_SECRET_KEY not set; skipping e2e test") + } + + kubeconfig := os.Getenv("KUBECONFIG") + if kubeconfig == "" { + t.Skip("KUBECONFIG not set; skipping e2e test") + } + restCfg, err := clientcmd.BuildConfigFromFlags("", kubeconfig) + if err != nil { + t.Fatalf("building kubeconfig: %v", err) + } + k8s, err := kubernetes.NewForConfig(restCfg) + if err != nil { + t.Fatalf("building kubernetes client: %v", err) + } + + verifySSL := true + if noVerify, err := strconv.ParseBool(os.Getenv("CS_SSL_NO_VERIFY")); err == nil { + verifySSL = !noVerify + } + cs := cloudstack.NewAsyncClient(apiURL, apiKey, secretKey, verifySSL) + + f := &Framework{ + T: t, + K8s: k8s, + CS: cs, + ProjectID: os.Getenv("CS_PROJECT_ID"), + } + f.Version = f.managementServerVersion() + f.Namespace = f.createNamespace() + return f +} + +func (f *Framework) managementServerVersion() semver.Version { + f.T.Helper() + resp, err := f.CS.Management.ListManagementServersMetrics( + f.CS.Management.NewListManagementServersMetricsParams()) + if err != nil { + f.T.Fatalf("listing management servers: %v", err) + } + if resp.Count == 0 { + f.T.Fatal("no management servers found") + } + raw := majorMinorPatch(resp.ManagementServersMetrics[0].Version) + v, err := semver.ParseTolerant(raw) + if err != nil { + f.T.Fatalf("parsing management server version %q: %v", raw, err) + } + return v +} + +// majorMinorPatch trims a CloudStack version such as "4.22.1.0" down to the +// first three components. Slicing blindly would panic on a version string with +// fewer than three, so the length is checked first. +func majorMinorPatch(version string) string { + parts := strings.Split(version, ".") + if len(parts) > 3 { + parts = parts[:3] + } + return strings.Join(parts, ".") +} + +func (f *Framework) createNamespace() string { + f.T.Helper() + buf := make([]byte, 4) + if _, err := rand.Read(buf); err != nil { + f.T.Fatalf("generating namespace suffix: %v", err) + } + name := "ccm-e2e-" + hex.EncodeToString(buf) + _, err := f.K8s.CoreV1().Namespaces().Create(context.Background(), + &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: name}}, metav1.CreateOptions{}) + if err != nil { + f.T.Fatalf("creating namespace %s: %v", name, err) + } + f.T.Cleanup(func() { + _ = f.K8s.CoreV1().Namespaces().Delete(context.Background(), name, metav1.DeleteOptions{}) + }) + return name +} + +// Eventually polls cond until it returns true or the timeout elapses. +func (f *Framework) Eventually(timeout, interval time.Duration, desc string, cond func() (bool, error)) { + f.T.Helper() + deadline := time.Now().Add(timeout) + var lastErr error + for time.Now().Before(deadline) { + ok, err := cond() + lastErr = err + if ok { + return + } + time.Sleep(interval) + } + f.T.Fatalf("timed out after %s waiting for %s (last error: %v)", timeout, desc, lastErr) Review Comment: When `cond()` returns `(false, nil)` (common for polling), `lastErr` ends up `nil`, and the failure message reports `last error: <nil>`, which is not very actionable. Consider tracking whether the condition was ever evaluated successfully but false (e.g., separate `lastErr` from `lastState`), and if `lastErr == nil` at timeout, print a clearer message like 'condition never became true' (optionally include the last observed state/error string). ########## Makefile: ########## @@ -29,10 +29,16 @@ LDFLAGS="-X k8s.io/kubernetes/pkg/version.gitVersion=${GIT_VERSION} -X k8s.io/ku export CGO_ENABLED=0 export GO111MODULE=on +# Keep these in step with hack/e2e/env.sh: both are overridable from the +# environment, so `make test-e2e` reaches the same simulator `make e2e-up` +# published rather than assuming the default port. +SIM_HOST_PORT ?= 8080 +CS_API_URL ?= http://localhost:$(SIM_HOST_PORT)/client/api Review Comment: As written, `SIM_HOST_PORT` / `CS_API_URL` are Make variables but are not exported to the environment for the `e2e-up` recipe. That means `SIM_HOST_PORT=8081 make e2e-up` likely won’t affect `hack/e2e/env.sh` (which reads environment variables), so users can end up with `make e2e-up` and `make test-e2e` disagreeing about the published port. Consider exporting these variables (or prefixing them on the `e2e-up` command invocation) so Make overrides reliably propagate into the harness scripts. -- 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]
