vishesh92 commented on code in PR #105:
URL:
https://github.com/apache/cloudstack-kubernetes-provider/pull/105#discussion_r3901453288
##########
cloudstack_loadbalancer.go:
##########
@@ -480,7 +480,7 @@ func (cs *CSCloud) getNetworkIDFromIPAddress(publicIpId
string) (string, error)
return "", err
}
Review Comment:
Right symptom, but this particular path doesn't apply: in cloudstack-go
v2.19.1 `GetPublicIpAddressByID` returns a nil error *only* when
`l.Count == 1` — every `count == 0` return carries a non-nil error, which the
preceding `if err != nil` already caught. So that branch was unreachable.
Digging into it did turn up two real ways the function could return
`("", nil)`, and the first is a genuine bug:
1. When `GetNetworkByID` failed, the handler returned `err` — which is nil at
that point — instead of `netErr`. A network lookup failure was reported as
success.
2. The guard tested `ip.Networkid` but the lookup used
`ip.Associatednetworkid`, so an IP with the first set and the second empty
called `GetNetworkByID("")`.
That matters for the reason you gave: `GetNetworkByID("")` isn't rejected, it
lists networks unfiltered and can resolve to an arbitrary network rather than
failing.
Fixed so every failure path returns a non-nil error, and so
`Associatednetworkid` is both tested and used. Added unit tests for the two
paths, which fail against the previous version.
##########
test/e2e/framework.go:
##########
@@ -0,0 +1,381 @@
+//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"
+ 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 :=
strings.Join(strings.SplitN(resp.ManagementServersMetrics[0].Version, ".",
4)[0:3], ".")
+ v, err := semver.ParseTolerant(raw)
+ if err != nil {
+ f.T.Fatalf("parsing management server version %q: %v", raw, err)
+ }
+ return v
+}
+
+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)
+}
+
+// CreateLBService creates a LoadBalancer service in the test namespace and
+// registers cleanup that both deletes it and waits for the CloudStack rules
+// to disappear, so a leaked rule cannot poison later tests.
+func (f *Framework) CreateLBService(mutate func(*corev1.Service))
*corev1.Service {
+ f.T.Helper()
+ svc := &corev1.Service{
+ ObjectMeta: metav1.ObjectMeta{
+ Name: "e2e",
+ Namespace: f.Namespace,
+ },
+ Spec: corev1.ServiceSpec{
+ Type: corev1.ServiceTypeLoadBalancer,
+ Selector: map[string]string{"app": "e2e"},
+ Ports: []corev1.ServicePort{
+ {Name: "http", Port: 80, Protocol:
corev1.ProtocolTCP},
+ },
+ },
+ }
+ if mutate != nil {
+ mutate(svc)
+ }
+ created, err := f.K8s.CoreV1().Services(f.Namespace).Create(
+ context.Background(), svc, metav1.CreateOptions{})
+ if err != nil {
+ f.T.Fatalf("creating service: %v", err)
+ }
+ f.T.Cleanup(func() { f.DeleteServiceAndWait(created) })
+ return created
+}
+
+// DeleteServiceAndWait deletes the service (if it still exists) and waits for
+// its CloudStack load balancer rules to be cleaned up.
+func (f *Framework) DeleteServiceAndWait(svc *corev1.Service) {
+ f.T.Helper()
+ err := f.K8s.CoreV1().Services(svc.Namespace).Delete(
+ context.Background(), svc.Name, metav1.DeleteOptions{})
+ if err != nil {
+ return // already gone
+ }
Review Comment:
Valid — fixed. Only `apierrors.IsNotFound` returns early now; any other error
fails the test.
Worth spelling out why it mattered: this is registered as `t.Cleanup` for
every LoadBalancer service, so a swallowed error also skipped the
rule-cleanup wait below it, leaking CloudStack load balancer rules into later
tests that share the same simulator and public IP pool.
##########
test/e2e/vpc_test.go:
##########
@@ -0,0 +1,132 @@
+//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 (
+ "os"
+ "strings"
+ "testing"
+)
+
+// vpcFramework skips unless the harness is in the VPC phase
+// (50-topology-vpc.sh exports E2E_ACL_ID / E2E_VPC_ID and CS_PROJECT_ID).
+func vpcFramework(t *testing.T) (*Framework, string, string) {
Review Comment:
Correct, the comment was wrong. `50-topology-vpc.sh` appends
`E2E_PROJECT_ID` to `hack/e2e/_out/ids.env`; `CS_PROJECT_ID` is only ever set
by the workflow, which maps one to the other.
Updated the comment to describe what the script actually writes, and added
the exact command for running the phase by hand, since sourcing `ids.env` and
running `-run TestVPC` would otherwise skip silently.
--
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]
