From cfae668c49ebdc02c275bde12a1290d9edf8222b Mon Sep 17 00:00:00 2001 From: vishesh92 Date: Thu, 20 Aug 2026 14:27:21 +0530 Subject: [PATCH] Add version as a config variable and use listCapabilities to fetch the version --- README.md | 9 +- cloudstack.go | 43 +++++- cloudstack_loadbalancer_test.go | 101 +++++++++++++ cloudstack_test.go | 257 ++++++++++++++++---------------- docs/development.md | 6 +- 5 files changed, 272 insertions(+), 144 deletions(-) diff --git a/README.md b/README.md index 9d2d93a8..316e8e33 100644 --- a/README.md +++ b/README.md @@ -42,6 +42,7 @@ project-id = zone = region = ssl-no-verify = +version = ``` If `zone` is not set, it is auto-detected from the node the controller runs on. @@ -50,12 +51,10 @@ If `zone` is not set, it is auto-detected from the node the controller runs on. name. Some workloads (such as Rook/Ceph) require the zone and region labels to differ. You need to explicitly set `region` in that case. -The access token needs to be able to fetch VM information and deploy load balancers in the project or domain where the nodes reside. +`version` is normally detected automatically using the `listCapabilities` API. Set it to pin the version manually, +for example when the API user is not allowed to call `listCapabilities`. -The account must also be allowed to call `listManagementServersMetrics`, which the controller uses -on startup to determine the management server version. This is a root admin API and is **not** -included in the default `User` role; without it the controller exits immediately with -`no management servers found`. +The access token needs to be able to fetch VM information and deploy load balancers in the project or domain where the nodes reside. To create the secret, use the following command: ```bash diff --git a/cloudstack.go b/cloudstack.go index e7224207..c0dfe338 100644 --- a/cloudstack.go +++ b/cloudstack.go @@ -55,6 +55,9 @@ type CSConfig struct { ProjectID string `gcfg:"project-id"` Zone string `gcfg:"zone"` Region string `gcfg:"region"` + // Version overrides the CloudStack version that is otherwise + // detected via the listCapabilities API. + Version string `gcfg:"version"` } } @@ -110,7 +113,18 @@ func newCSCloud(cfg *CSConfig) (*CSCloud, error) { return nil, errors.New("no cloud provider config given") } - version, err := cs.getManagementServerVersion() + if cfg.Global.Version != "" { + version, err := parseCloudStackVersion(cfg.Global.Version) + if err != nil { + return nil, fmt.Errorf("could not parse the version given in the cloud provider config: %v", err) + } + klog.V(2).Infof("Using CloudStack version %v from the cloud provider config", version) + cs.version = version + + return cs, nil + } + + version, err := cs.getCloudStackVersion() if err != nil { return nil, err } @@ -119,20 +133,33 @@ func newCSCloud(cfg *CSConfig) (*CSCloud, error) { return cs, nil } -func (cs *CSCloud) getManagementServerVersion() (semver.Version, error) { - msServersResp, err := cs.client.Management.ListManagementServersMetrics(cs.client.Management.NewListManagementServersMetricsParams()) +// getCloudStackVersion returns the version of the CloudStack management server, +// as reported by the listCapabilities API. +func (cs *CSCloud) getCloudStackVersion() (semver.Version, error) { + capabilitiesResp, err := cs.client.Configuration.ListCapabilities(cs.client.Configuration.NewListCapabilitiesParams()) if err != nil { return semver.Version{}, err } - if msServersResp.Count == 0 { - return semver.Version{}, errors.New("no management servers found") + if capabilitiesResp.Capabilities == nil || capabilitiesResp.Capabilities.Cloudstackversion == "" { + return semver.Version{}, errors.New("no CloudStack version returned by the management server") } - version := msServersResp.ManagementServersMetrics[0].Version + + v, err := parseCloudStackVersion(capabilitiesResp.Capabilities.Cloudstackversion) + if err != nil { + klog.Error(err) + return semver.Version{}, err + } + return v, nil +} + +// parseCloudStackVersion parses a CloudStack version such as "4.17.1.0" or +// "4.17.1.0-SNAPSHOT" into a semver version, discarding everything after the +// patch level. +func parseCloudStackVersion(version string) (semver.Version, error) { parts := strings.Split(version, ".") v, err := semver.ParseTolerant(strings.Join(parts[:min(len(parts), 3)], ".")) if err != nil { - klog.Errorf("failed to parse management server version: %v", err) - return semver.Version{}, err + return semver.Version{}, fmt.Errorf("failed to parse CloudStack version %q: %v", version, err) } return v, nil } diff --git a/cloudstack_loadbalancer_test.go b/cloudstack_loadbalancer_test.go index 39b229d5..c07890cd 100644 --- a/cloudstack_loadbalancer_test.go +++ b/cloudstack_loadbalancer_test.go @@ -680,6 +680,57 @@ func TestCheckLoadBalancerRule(t *testing.T) { } }) + // CloudStack moves from 4.x to 24.0 after 4.23, so the 4.22 feature gate has + // to keep treating the new numbering as newer rather than older. + t.Run("cidr change triggers update on the 24.0 series", func(t *testing.T) { + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + // No expectations on the mock; any delete call would fail the test. + mockLB := cloudstack.NewMockLoadBalancerServiceIface(ctrl) + + lbRule := &cloudstack.LoadBalancerRule{ + Id: "rule-id", + Name: "rule", + Publicip: "1.1.1.1", + Privateport: "30000", + Publicport: "80", + Cidrlist: "10.0.0.0/8", + Algorithm: "roundrobin", + Protocol: LoadBalancerProtocolTCP.CSProtocol(), + } + + lb := &loadBalancer{ + CloudStackClient: &cloudstack.CloudStackClient{ + LoadBalancer: mockLB, + }, + ipAddr: "1.1.1.1", + algorithm: "roundrobin", + rules: map[string]*cloudstack.LoadBalancerRule{ + "rule": lbRule, + }, + } + port := corev1.ServicePort{Port: 80, NodePort: 30000, Protocol: corev1.ProtocolTCP} + service := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + ServiceAnnotationLoadBalancerSourceCidrs: "10.0.0.0/8,192.168.0.0/16", + }, + }, + } + + rule, needsUpdate, err := lb.checkLoadBalancerRule("rule", port, LoadBalancerProtocolTCP, service, semver.MustParse("24.0.0")) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if rule != lbRule { + t.Fatalf("expected existing rule to be returned") + } + if !needsUpdate { + t.Fatalf("expected needsUpdate to be true due to CIDR change") + } + }) + t.Run("cidr change triggers delete with older version", func(t *testing.T) { ctrl := gomock.NewController(t) t.Cleanup(ctrl.Finish) @@ -2053,6 +2104,56 @@ func TestUpdateLoadBalancerRule(t *testing.T) { } }) + // The release after 4.23 is numbered 24.0, which must still reach the + // in-place CIDR update rather than falling back to delete-and-recreate. + t.Run("update CIDR list on the 24.0 series", func(t *testing.T) { + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + mockLB := cloudstack.NewMockLoadBalancerServiceIface(ctrl) + updateParams := &cloudstack.UpdateLoadBalancerRuleParams{} + + gomock.InOrder( + mockLB.EXPECT().NewUpdateLoadBalancerRuleParams("rule-123").Return(updateParams), + mockLB.EXPECT().UpdateLoadBalancerRule(gomock.Any()).Return(&cloudstack.UpdateLoadBalancerRuleResponse{}, nil), + ) + + lb := &loadBalancer{ + CloudStackClient: &cloudstack.CloudStackClient{ + LoadBalancer: mockLB, + }, + algorithm: "roundrobin", + rules: map[string]*cloudstack.LoadBalancerRule{ + "test-rule-tcp-80": { + Id: "rule-123", + Algorithm: "roundrobin", + Protocol: "tcp", + Cidrlist: defaultAllowedCIDR, + }, + }, + } + + service := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + ServiceAnnotationLoadBalancerSourceCidrs: "10.0.0.0/8", + }, + }, + } + + if err := lb.updateLoadBalancerRule("test-rule-tcp-80", LoadBalancerProtocolTCP, service, semver.MustParse("24.0.0")); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + cidrList, ok := updateParams.GetCidrlist() + if !ok { + t.Fatalf("expected the CIDR list to be set on the update params") + } + if len(cidrList) != 1 || cidrList[0] != "10.0.0.0/8" { + t.Fatalf("cidrlist = %v, want [10.0.0.0/8]", cidrList) + } + }) + t.Run("error updating rule", func(t *testing.T) { ctrl := gomock.NewController(t) t.Cleanup(ctrl.Finish) diff --git a/cloudstack_test.go b/cloudstack_test.go index 8817e043..1bc5cc35 100644 --- a/cloudstack_test.go +++ b/cloudstack_test.go @@ -66,6 +66,45 @@ func TestReadConfig(t *testing.T) { if !cfg.Global.SSLNoVerify { t.Errorf("incorrect ssl-no-verify: %t", cfg.Global.SSLNoVerify) } + if cfg.Global.Version != "" { + t.Errorf("version should be empty when not configured: %s", cfg.Global.Version) + } + + cfg, err = readConfig(strings.NewReader(` + [Global] + api-url = https://cloudstack.url + version = 4.21.0.0 + `)) + if err != nil { + t.Fatalf("Should succeed when a valid config is provided: %v", err) + } + if cfg.Global.Version != "4.21.0.0" { + t.Errorf("incorrect version: %s", cfg.Global.Version) + } +} + +func TestNewCSCloudWithVersionFromConfig(t *testing.T) { + cfg := &CSConfig{} + cfg.Global.APIURL = "https://cloudstack.url/client/api" + cfg.Global.APIKey = "a-valid-api-key" + cfg.Global.SecretKey = "a-valid-secret-key" + cfg.Global.Version = "4.21.0.0" + + // The version from the config is used as-is, so no API call is made. + cs, err := newCSCloud(cfg) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + expected := semver.MustParse("4.21.0") + if !cs.version.Equals(expected) { + t.Fatalf("version = %v, want %v", cs.version, expected) + } + + cfg.Global.Version = "not-a-version" + if _, err := newCSCloud(cfg); err == nil { + t.Fatalf("expected error for an invalid version in the config") + } } // This allows acceptance testing against an existing CloudStack environment. @@ -124,32 +163,36 @@ func TestLoadBalancer(t *testing.T) { } } -func TestGetManagementServerVersion(t *testing.T) { - t.Run("returns parsed version", func(t *testing.T) { - ctrl := gomock.NewController(t) - t.Cleanup(ctrl.Finish) - - mockMgmt := cloudstack.NewMockManagementServiceIface(ctrl) - params := &cloudstack.ListManagementServersMetricsParams{} - resp := &cloudstack.ListManagementServersMetricsResponse{ - Count: 1, - ManagementServersMetrics: []*cloudstack.ManagementServersMetric{ - {Version: "4.17.1.0"}, - }, - } +// newCSCloudWithCapabilities returns a CSCloud whose Configuration service is mocked to +// answer a single listCapabilities call with the given response and error. +func newCSCloudWithCapabilities(t *testing.T, resp *cloudstack.ListCapabilitiesResponse, err error) *CSCloud { + t.Helper() - gomock.InOrder( - mockMgmt.EXPECT().NewListManagementServersMetricsParams().Return(params), - mockMgmt.EXPECT().ListManagementServersMetrics(params).Return(resp, nil), - ) + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) - cs := &CSCloud{ - client: &cloudstack.CloudStackClient{ - Management: mockMgmt, - }, - } + mockConfig := cloudstack.NewMockConfigurationServiceIface(ctrl) + params := &cloudstack.ListCapabilitiesParams{} + + gomock.InOrder( + mockConfig.EXPECT().NewListCapabilitiesParams().Return(params), + mockConfig.EXPECT().ListCapabilities(params).Return(resp, err), + ) + + return &CSCloud{ + client: &cloudstack.CloudStackClient{ + Configuration: mockConfig, + }, + } +} + +func TestGetCloudStackVersion(t *testing.T) { + t.Run("returns parsed version", func(t *testing.T) { + cs := newCSCloudWithCapabilities(t, &cloudstack.ListCapabilitiesResponse{ + Capabilities: &cloudstack.Capability{Cloudstackversion: "4.17.1.0"}, + }, nil) - version, err := cs.getManagementServerVersion() + version, err := cs.getCloudStackVersion() if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -161,30 +204,11 @@ func TestGetManagementServerVersion(t *testing.T) { }) t.Run("returns correct parsed version with development server", func(t *testing.T) { - ctrl := gomock.NewController(t) - t.Cleanup(ctrl.Finish) - - mockMgmt := cloudstack.NewMockManagementServiceIface(ctrl) - params := &cloudstack.ListManagementServersMetricsParams{} - resp := &cloudstack.ListManagementServersMetricsResponse{ - Count: 1, - ManagementServersMetrics: []*cloudstack.ManagementServersMetric{ - {Version: "4.17.1.0-SNAPSHOT"}, - }, - } - - gomock.InOrder( - mockMgmt.EXPECT().NewListManagementServersMetricsParams().Return(params), - mockMgmt.EXPECT().ListManagementServersMetrics(params).Return(resp, nil), - ) - - cs := &CSCloud{ - client: &cloudstack.CloudStackClient{ - Management: mockMgmt, - }, - } + cs := newCSCloudWithCapabilities(t, &cloudstack.ListCapabilitiesResponse{ + Capabilities: &cloudstack.Capability{Cloudstackversion: "4.17.1.0-SNAPSHOT"}, + }, nil) - version, err := cs.getManagementServerVersion() + version, err := cs.getCloudStackVersion() if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -208,28 +232,11 @@ func TestGetManagementServerVersion(t *testing.T) { {"24.0.0.0-SNAPSHOT", semver.MustParse("24.0.0")}, } { t.Run(tc.version, func(t *testing.T) { - ctrl := gomock.NewController(t) - t.Cleanup(ctrl.Finish) - - mockMgmt := cloudstack.NewMockManagementServiceIface(ctrl) - params := &cloudstack.ListManagementServersMetricsParams{} - resp := &cloudstack.ListManagementServersMetricsResponse{ - Count: 1, - ManagementServersMetrics: []*cloudstack.ManagementServersMetric{ - {Version: tc.version}, - }, - } - - gomock.InOrder( - mockMgmt.EXPECT().NewListManagementServersMetricsParams().Return(params), - mockMgmt.EXPECT().ListManagementServersMetrics(params).Return(resp, nil), - ) - - cs := &CSCloud{ - client: &cloudstack.CloudStackClient{Management: mockMgmt}, - } + cs := newCSCloudWithCapabilities(t, &cloudstack.ListCapabilitiesResponse{ + Capabilities: &cloudstack.Capability{Cloudstackversion: tc.version}, + }, nil) - version, err := cs.getManagementServerVersion() + version, err := cs.getCloudStackVersion() if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -241,86 +248,80 @@ func TestGetManagementServerVersion(t *testing.T) { }) t.Run("returns error when api call fails", func(t *testing.T) { - ctrl := gomock.NewController(t) - t.Cleanup(ctrl.Finish) - - mockMgmt := cloudstack.NewMockManagementServiceIface(ctrl) - params := &cloudstack.ListManagementServersMetricsParams{} - apiErr := errors.New("api failure") - - gomock.InOrder( - mockMgmt.EXPECT().NewListManagementServersMetricsParams().Return(params), - mockMgmt.EXPECT().ListManagementServersMetrics(params).Return(nil, apiErr), - ) - - cs := &CSCloud{ - client: &cloudstack.CloudStackClient{ - Management: mockMgmt, - }, - } + cs := newCSCloudWithCapabilities(t, nil, errors.New("api failure")) - if _, err := cs.getManagementServerVersion(); err == nil { + if _, err := cs.getCloudStackVersion(); err == nil { t.Fatalf("expected error, got nil") } }) - t.Run("returns error when no servers found", func(t *testing.T) { - ctrl := gomock.NewController(t) - t.Cleanup(ctrl.Finish) + t.Run("returns error when no capabilities returned", func(t *testing.T) { + cs := newCSCloudWithCapabilities(t, &cloudstack.ListCapabilitiesResponse{}, nil) - mockMgmt := cloudstack.NewMockManagementServiceIface(ctrl) - params := &cloudstack.ListManagementServersMetricsParams{} - resp := &cloudstack.ListManagementServersMetricsResponse{ - Count: 0, - ManagementServersMetrics: []*cloudstack.ManagementServersMetric{}, + if _, err := cs.getCloudStackVersion(); err == nil { + t.Fatalf("expected error for missing capabilities") } + }) - gomock.InOrder( - mockMgmt.EXPECT().NewListManagementServersMetricsParams().Return(params), - mockMgmt.EXPECT().ListManagementServersMetrics(params).Return(resp, nil), - ) - - cs := &CSCloud{ - client: &cloudstack.CloudStackClient{ - Management: mockMgmt, - }, - } + t.Run("returns error when version is empty", func(t *testing.T) { + cs := newCSCloudWithCapabilities(t, &cloudstack.ListCapabilitiesResponse{ + Capabilities: &cloudstack.Capability{}, + }, nil) - if _, err := cs.getManagementServerVersion(); err == nil { - t.Fatalf("expected error for zero management servers") + if _, err := cs.getCloudStackVersion(); err == nil { + t.Fatalf("expected error for empty version") } }) t.Run("returns error when version cannot be parsed", func(t *testing.T) { - ctrl := gomock.NewController(t) - t.Cleanup(ctrl.Finish) - - mockMgmt := cloudstack.NewMockManagementServiceIface(ctrl) - params := &cloudstack.ListManagementServersMetricsParams{} - resp := &cloudstack.ListManagementServersMetricsResponse{ - Count: 1, - ManagementServersMetrics: []*cloudstack.ManagementServersMetric{ - {Version: "invalid.version.string"}, - }, - } - - gomock.InOrder( - mockMgmt.EXPECT().NewListManagementServersMetricsParams().Return(params), - mockMgmt.EXPECT().ListManagementServersMetrics(params).Return(resp, nil), - ) + cs := newCSCloudWithCapabilities(t, &cloudstack.ListCapabilitiesResponse{ + Capabilities: &cloudstack.Capability{Cloudstackversion: "invalid.version.string"}, + }, nil) - cs := &CSCloud{ - client: &cloudstack.CloudStackClient{ - Management: mockMgmt, - }, - } - - if _, err := cs.getManagementServerVersion(); err == nil { + if _, err := cs.getCloudStackVersion(); err == nil { t.Fatalf("expected parse error") } }) } +func TestParseCloudStackVersion(t *testing.T) { + tests := []struct { + version string + want string + wantErr bool + }{ + {version: "4.17.1.0", want: "4.17.1"}, + {version: "4.17.1.0-SNAPSHOT", want: "4.17.1"}, + {version: "4.21.0", want: "4.21.0"}, + {version: "4.21", want: "4.21.0"}, + {version: "4", want: "4.0.0"}, + {version: "4.23.0.0", want: "4.23.0"}, + {version: "24.0.0.0", want: "24.0.0"}, + {version: "24.0.0.0-SNAPSHOT", want: "24.0.0"}, + {version: "24.0", want: "24.0.0"}, + {version: "invalid.version.string", wantErr: true}, + {version: "", wantErr: true}, + } + + for _, tt := range tests { + t.Run(tt.version, func(t *testing.T) { + got, err := parseCloudStackVersion(tt.version) + if tt.wantErr { + if err == nil { + t.Fatalf("expected error for %q, got %v", tt.version, got) + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !got.Equals(semver.MustParse(tt.want)) { + t.Fatalf("version = %v, want %v", got, tt.want) + } + }) + } +} + func TestGetRegionFromZone(t *testing.T) { tests := []struct { name string diff --git a/docs/development.md b/docs/development.md index 73f3d7d5..28757dd1 100644 --- a/docs/development.md +++ b/docs/development.md @@ -146,8 +146,8 @@ The upstream simulator README suggests `-p 8080:5050`, which publishes the Readiness is checked in three stages rather than with a fixed sleep: jetty answering at all, then the API accepting admin credentials, then `listManagementServersMetrics` returning a server. The last one matters -because the CCM makes exactly that call on startup and refuses to run until it -succeeds. +because it proves a management server is registered and running, not just that +the API answers. The zone is then deployed with marvin, which is preinstalled in the image: @@ -414,7 +414,7 @@ so a run is reproducible; avoid floating tags like `latest`. | Symptom | Cause | | --- | --- | | `LB service provider cannot support this rule` on a VPC | The VPC virtual router accepts only a restricted set of public load balancer ports. 80 and 8080 work; an arbitrary high port such as 8081 is rejected. Pick a port the router supports when adding a VPC test. | -| CCM exits with `no management servers found` | The account cannot call `listManagementServersMetrics`. This is a root-admin API; the default `User` role does not include it. | +| CCM exits with `no CloudStack version returned by the management server` | `listCapabilities` answered without a `cloudstackversion`. Set `version` in the `cloud-config` to pin it and skip the lookup. | | Nodes keep the uninitialized taint; CCM logs `provided node ip for node "..." is not valid` | The CloudStack VM's NIC IP does not match the IP kubelet registered with. Recreate the VM with `ipaddress=` set to the kind node's docker IP. | | Services stay ``; CCM logs `none of the hosts matched the list of VMs retrieved from CS API` | No CloudStack VM has a name matching a Kubernetes node name. | | CCM logs `found hosts that belong to different networks` | VMs matching the node names exist on more than one network — typically leftovers from a previous scenario. |