Copilot commented on code in PR #105:
URL:
https://github.com/apache/cloudstack-kubernetes-provider/pull/105#discussion_r3900905942
##########
cloudstack_loadbalancer.go:
##########
@@ -480,7 +480,7 @@ func (cs *CSCloud) getNetworkIDFromIPAddress(publicIpId
string) (string, error)
return "", err
}
Review Comment:
When GetPublicIpAddressByID returns count==0 with err==nil, this function
returns ("", nil). Callers then treat it as a valid network ID and proceed with
GetNetworkByID(""), which can produce confusing downstream errors. Treat a
missing public IP as an explicit error (or at least a non-nil error) here.
##########
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:
DeleteServiceAndWait currently returns on *any* delete error. That hides
real failures (RBAC, transient apiserver issues) and can skip the rule-cleanup
wait, potentially leaking CloudStack resources into later tests. Only ignore
NotFound; fail the test on other errors.
##########
.github/workflows/e2e-simulator.yml:
##########
@@ -0,0 +1,191 @@
+# 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.
+
+name: E2E (CloudStack simulator)
+
+on:
+ push:
+ branches:
+ - main
+ pull_request:
+ workflow_dispatch:
+
+permissions:
+ contents: read
+
+concurrency:
+ group: ${{ github.workflow }}-${{ github.event.pull_request.number ||
github.ref }}
+ cancel-in-progress: true
+
+jobs:
+ # Build the CCM image once and share it with every matrix cell. The
+ # distroless image is small enough that passing it as an artifact is much
+ # cheaper than four redundant builds.
+ build:
+ if: github.repository == 'apache/cloudstack-kubernetes-provider' ||
github.event_name == 'workflow_dispatch'
+ runs-on: ubuntu-latest
+ timeout-minutes: 20
+ steps:
+ - uses: actions/checkout@v6
+
+ - uses: docker/setup-buildx-action@v3
+
+ - name: Build CCM image
+ uses: docker/build-push-action@v6
+ with:
+ context: .
+ load: true
+ platforms: linux/amd64
+ tags: apache/cloudstack-kubernetes-provider:e2e
+ cache-from: type=gha
+ cache-to: type=gha,mode=max
+
+ - name: Export image
+ run: docker save apache/cloudstack-kubernetes-provider:e2e | zstd -T0
-o ccm-image.tar.zst
+
+ - uses: actions/upload-artifact@v4
+ with:
+ name: ccm-image
+ path: ccm-image.tar.zst
+ retention-days: 1
+
+ e2e:
+ needs: build
+ runs-on: ubuntu-latest
+ timeout-minutes: 45
+ strategy:
+ fail-fast: false
+ matrix:
+ # Latest two Kubernetes minors and latest two CloudStack releases.
+ # The CloudStack axis is not just version coverage: >= 4.22 updates a
+ # load balancer rule's CIDR list in place, while older versions delete
+ # and recreate the rule, so both branches get exercised.
+ k8s: ['v1.37.0', 'v1.36.4']
+ acs: ['4.22.1.0', '4.20.2.0']
Review Comment:
The PR description says the CI matrix tests CloudStack 4.22.1.0 and
4.21.0.0, but the workflow currently uses 4.20.2.0 for the second axis value.
Either the description or this matrix entry should be updated so they match.
##########
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:
The comment says 50-topology-vpc.sh exports CS_PROJECT_ID, but the harness
scripts only write E2E_PROJECT_ID into hack/e2e/_out/ids.env (CS_PROJECT_ID is
set by the test runner, e.g. from E2E_PROJECT_ID in CI). This can mislead local
users trying to run the VPC phase.
--
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]
