refactor: move providerID to the package

Parsing and generating magic providerID moved to a separate package.

Signed-off-by: Serge Logvinov <serge.logvinov@sinextra.dev>

cmd/proxmox-cloud-controller-manager/main.go

@@ -26,7 +26,7 @@ import (
 
 	"github.com/spf13/pflag"
 
-	"github.com/sergelogvinov/proxmox-cloud-controller-manager/pkg/proxmox"
+	"github.com/sergelogvinov/proxmox-cloud-controller-manager/pkg/provider"
 
 	"k8s.io/apimachinery/pkg/util/wait"
 	cloudprovider "k8s.io/cloud-provider"
@@ -52,7 +52,7 @@ func main() {
 
 	command.Flags().VisitAll(func(flag *pflag.Flag) {
 		if flag.Name == "cloud-provider" {
-			if err := flag.Value.Set(proxmox.ProviderName); err != nil {
+			if err := flag.Value.Set(provider.ProviderName); err != nil {
 				klog.Fatalf("unable to set cloud-provider flag value: %s", err)
 			}
 		}

pkg/provider/provider.go

@@ -0,0 +1,77 @@
+/*
+Copyright 2023 The Kubernetes Authors.
+
+Licensed 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 provider implements the providerID interface for Proxmox.
+package provider
+
+import (
+	"fmt"
+	"regexp"
+	"strconv"
+	"strings"
+
+	pxapi "github.com/Telmate/proxmox-api-go/proxmox"
+)
+
+const (
+	// ProviderName is the name of the Proxmox provider.
+	ProviderName = "proxmox"
+)
+
+var providerIDRegexp = regexp.MustCompile(`^` + ProviderName + `://([^/]*)/([^/]+)$`)
+
+// GetProviderID returns the magic providerID for kubernetes node.
+func GetProviderID(region string, vmr *pxapi.VmRef) string {
+	return fmt.Sprintf("%s://%s/%d", ProviderName, region, vmr.VmId())
+}
+
+// GetVMID returns the VM ID from the providerID.
+func GetVMID(providerID string) (int, error) {
+	if !strings.HasPrefix(providerID, ProviderName) {
+		return 0, fmt.Errorf("foreign providerID or empty \"%s\"", providerID)
+	}
+
+	matches := providerIDRegexp.FindStringSubmatch(providerID)
+	if len(matches) != 3 {
+		return 0, fmt.Errorf("providerID \"%s\" didn't match expected format \"%s://region/InstanceID\"", providerID, ProviderName)
+	}
+
+	vmID, err := strconv.Atoi(matches[2])
+	if err != nil {
+		return 0, fmt.Errorf("InstanceID have to be a number, but got \"%s\"", matches[2])
+	}
+
+	return vmID, nil
+}
+
+// ParseProviderID returns the VmRef and region from the providerID.
+func ParseProviderID(providerID string) (*pxapi.VmRef, string, error) {
+	if !strings.HasPrefix(providerID, ProviderName) {
+		return nil, "", fmt.Errorf("foreign providerID or empty \"%s\"", providerID)
+	}
+
+	matches := providerIDRegexp.FindStringSubmatch(providerID)
+	if len(matches) != 3 {
+		return nil, "", fmt.Errorf("providerID \"%s\" didn't match expected format \"%s://region/InstanceID\"", providerID, ProviderName)
+	}
+
+	vmID, err := strconv.Atoi(matches[2])
+	if err != nil {
+		return nil, "", fmt.Errorf("InstanceID have to be a number, but got \"%s\"", matches[2])
+	}
+
+	return pxapi.NewVmRef(vmID), matches[1], nil
+}

pkg/provider/provider_test.go

@@ -0,0 +1,193 @@
+/*
+Copyright 2023 The Kubernetes Authors.
+
+Licensed 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 provider_test
+
+import (
+	"fmt"
+	"testing"
+
+	pxapi "github.com/Telmate/proxmox-api-go/proxmox"
+	"github.com/stretchr/testify/assert"
+
+	provider "github.com/sergelogvinov/proxmox-cloud-controller-manager/pkg/provider"
+)
+
+func TestGetProviderID(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		msg                string
+		region             string
+		vmID               int
+		expectedProviderID string
+	}{
+		{
+			msg:                "Valid providerID",
+			region:             "region",
+			vmID:               123,
+			expectedProviderID: "proxmox://region/123",
+		},
+		{
+			msg:                "No region",
+			region:             "",
+			vmID:               123,
+			expectedProviderID: "proxmox:///123",
+		},
+	}
+
+	for _, testCase := range tests {
+		testCase := testCase
+
+		t.Run(fmt.Sprint(testCase.msg), func(t *testing.T) {
+			t.Parallel()
+
+			providerID := provider.GetProviderID(testCase.region, pxapi.NewVmRef(testCase.vmID))
+
+			assert.Equal(t, testCase.expectedProviderID, providerID)
+		})
+	}
+}
+
+func TestGetVmID(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		msg           string
+		providerID    string
+		expectedError error
+		expectedvmID  int
+	}{
+		{
+			msg:           "Valid VMID",
+			providerID:    "proxmox://region/123",
+			expectedError: nil,
+			expectedvmID:  123,
+		},
+		{
+			msg:           "Valid VMID with empty region",
+			providerID:    "proxmox:///123",
+			expectedError: nil,
+			expectedvmID:  123,
+		},
+		{
+			msg:           "Invalid providerID format",
+			providerID:    "proxmox://123",
+			expectedError: fmt.Errorf("providerID \"proxmox://123\" didn't match expected format \"proxmox://region/InstanceID\""),
+		},
+		{
+			msg:           "Non proxmox providerID",
+			providerID:    "cloud:///123",
+			expectedError: fmt.Errorf("foreign providerID or empty \"cloud:///123\""),
+			expectedvmID:  123,
+		},
+		{
+			msg:           "Non proxmox providerID",
+			providerID:    "cloud://123",
+			expectedError: fmt.Errorf("foreign providerID or empty \"cloud://123\""),
+			expectedvmID:  123,
+		},
+		{
+			msg:           "InValid VMID",
+			providerID:    "proxmox://region/abc",
+			expectedError: fmt.Errorf("InstanceID have to be a number, but got \"abc\""),
+			expectedvmID:  0,
+		},
+	}
+
+	for _, testCase := range tests {
+		testCase := testCase
+
+		t.Run(fmt.Sprint(testCase.msg), func(t *testing.T) {
+			t.Parallel()
+
+			VMID, err := provider.GetVMID(testCase.providerID)
+
+			if testCase.expectedError != nil {
+				assert.NotNil(t, err)
+				assert.Equal(t, err.Error(), testCase.expectedError.Error())
+			} else {
+				assert.Equal(t, testCase.expectedvmID, VMID)
+			}
+		})
+	}
+}
+
+func TestParseProviderID(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		msg            string
+		providerID     string
+		expectedError  error
+		expectedvmID   int
+		expectedRegion string
+	}{
+		{
+			msg:            "Valid VMID",
+			providerID:     "proxmox://region/123",
+			expectedError:  nil,
+			expectedvmID:   123,
+			expectedRegion: "region",
+		},
+		{
+			msg:            "Valid VMID with empty region",
+			providerID:     "proxmox:///123",
+			expectedError:  nil,
+			expectedvmID:   123,
+			expectedRegion: "",
+		},
+		{
+			msg:           "Invalid providerID format",
+			providerID:    "proxmox://123",
+			expectedError: fmt.Errorf("providerID \"proxmox://123\" didn't match expected format \"proxmox://region/InstanceID\""),
+		},
+		{
+			msg:           "Non proxmox providerID",
+			providerID:    "cloud:///123",
+			expectedError: fmt.Errorf("foreign providerID or empty \"cloud:///123\""),
+		},
+		{
+			msg:           "Non proxmox providerID",
+			providerID:    "cloud://123",
+			expectedError: fmt.Errorf("foreign providerID or empty \"cloud://123\""),
+		},
+		{
+			msg:           "InValid VMID",
+			providerID:    "proxmox://region/abc",
+			expectedError: fmt.Errorf("InstanceID have to be a number, but got \"abc\""),
+		},
+	}
+
+	for _, testCase := range tests {
+		testCase := testCase
+
+		t.Run(fmt.Sprint(testCase.msg), func(t *testing.T) {
+			t.Parallel()
+
+			vmr, region, err := provider.ParseProviderID(testCase.providerID)
+
+			if testCase.expectedError != nil {
+				assert.NotNil(t, err)
+				assert.Equal(t, err.Error(), testCase.expectedError.Error())
+			} else {
+				assert.NotNil(t, vmr)
+				assert.Equal(t, testCase.expectedvmID, vmr.VmId())
+				assert.Equal(t, testCase.expectedRegion, region)
+			}
+		})
+	}
+}

pkg/proxmox/cloud.go

@@ -22,6 +22,7 @@ import (
 	"io"
 
 	"github.com/sergelogvinov/proxmox-cloud-controller-manager/pkg/cluster"
+	provider "github.com/sergelogvinov/proxmox-cloud-controller-manager/pkg/provider"
 
 	clientkubernetes "k8s.io/client-go/kubernetes"
 	cloudprovider "k8s.io/cloud-provider"
@@ -29,10 +30,8 @@ import (
 )
 
 const (
-	// ProviderName is the name of the Proxmox provider.
-	ProviderName = "proxmox"
 	// ServiceAccountName is the service account name used in kube-system namespace.
-	ServiceAccountName = "proxmox-cloud-controller-manager"
+	ServiceAccountName = provider.ProviderName + "-cloud-controller-manager"
 )
 
 type cloud struct {
@@ -45,7 +44,7 @@ type cloud struct {
 }
 
 func init() {
-	cloudprovider.RegisterCloudProvider(ProviderName, func(config io.Reader) (cloudprovider.Interface, error) {
+	cloudprovider.RegisterCloudProvider(provider.ProviderName, func(config io.Reader) (cloudprovider.Interface, error) {
 		cfg, err := cluster.ReadCloudConfig(config)
 		if err != nil {
 			klog.Errorf("failed to read config: %v", err)
@@ -137,7 +136,7 @@ func (c *cloud) Routes() (cloudprovider.Routes, bool) {
 
 // ProviderName returns the cloud provider ID.
 func (c *cloud) ProviderName() string {
-	return ProviderName
+	return provider.ProviderName
 }
 
 // HasClusterID is not implemented.

pkg/proxmox/cloud_test.go

@@ -23,6 +23,7 @@ import (
 	"github.com/stretchr/testify/assert"
 
 	"github.com/sergelogvinov/proxmox-cloud-controller-manager/pkg/cluster"
+	provider "github.com/sergelogvinov/proxmox-cloud-controller-manager/pkg/provider"
 )
 
 func TestNewCloudError(t *testing.T) {
@@ -73,7 +74,7 @@ clusters:
 	assert.Equal(t, res, false)
 
 	pName := cloud.ProviderName()
-	assert.Equal(t, pName, ProviderName)
+	assert.Equal(t, pName, provider.ProviderName)
 
 	clID := cloud.HasClusterID()
 	assert.Equal(t, clID, true)

pkg/proxmox/instances.go

@@ -19,13 +19,12 @@ package proxmox
 import (
 	"context"
 	"fmt"
-	"regexp"
-	"strconv"
 	"strings"
 
 	pxapi "github.com/Telmate/proxmox-api-go/proxmox"
 
 	"github.com/sergelogvinov/proxmox-cloud-controller-manager/pkg/cluster"
+	provider "github.com/sergelogvinov/proxmox-cloud-controller-manager/pkg/provider"
 
 	v1 "k8s.io/api/core/v1"
 	cloudprovider "k8s.io/cloud-provider"
@@ -48,7 +47,7 @@ func newInstances(client *cluster.Cluster) *instances {
 func (i *instances) InstanceExists(_ context.Context, node *v1.Node) (bool, error) {
 	klog.V(4).Info("instances.InstanceExists() called node: ", node.Name)
 
-	if !strings.HasPrefix(node.Spec.ProviderID, ProviderName) {
+	if !strings.HasPrefix(node.Spec.ProviderID, provider.ProviderName) {
 		klog.V(4).Infof("instances.InstanceExists() node %s has foreign providerID: %s, skipped", node.Name, node.Spec.ProviderID)
 
 		return true, nil
@@ -73,13 +72,13 @@ func (i *instances) InstanceExists(_ context.Context, node *v1.Node) (bool, erro
 func (i *instances) InstanceShutdown(_ context.Context, node *v1.Node) (bool, error) {
 	klog.V(4).Info("instances.InstanceShutdown() called, node: ", node.Name)
 
-	if !strings.HasPrefix(node.Spec.ProviderID, ProviderName) {
+	if !strings.HasPrefix(node.Spec.ProviderID, provider.ProviderName) {
 		klog.V(4).Infof("instances.InstanceShutdown() node %s has foreign providerID: %s, skipped", node.Name, node.Spec.ProviderID)
 
 		return false, nil
 	}
 
-	vmr, region, err := i.parseProviderID(node.Spec.ProviderID)
+	vmr, region, err := provider.ParseProviderID(node.Spec.ProviderID)
 	if err != nil {
 		klog.Errorf("instances.InstanceShutdown() failed to parse providerID %s: %v", node.Spec.ProviderID, err)
 
@@ -126,7 +125,7 @@ func (i *instances) InstanceMetadata(_ context.Context, node *v1.Node) (*cloudpr
 			if err != nil {
 				return nil, fmt.Errorf("instances.InstanceMetadata() - failed to find instance by name %s: %v, skipped", node.Name, err)
 			}
-		} else if !strings.HasPrefix(node.Spec.ProviderID, ProviderName) {
+		} else if !strings.HasPrefix(node.Spec.ProviderID, provider.ProviderName) {
 			klog.V(4).Infof("instances.InstanceMetadata() node %s has foreign providerID: %s, skipped", node.Name, node.Spec.ProviderID)
 
 			return &cloudprovider.InstanceMetadata{}, nil
@@ -148,7 +147,7 @@ func (i *instances) InstanceMetadata(_ context.Context, node *v1.Node) (*cloudpr
 		}
 
 		return &cloudprovider.InstanceMetadata{
-			ProviderID:    i.getProviderID(region, vmRef),
+			ProviderID:    provider.GetProviderID(region, vmRef),
 			NodeAddresses: addresses,
 			InstanceType:  instanceType,
 			Zone:          vmRef.Node(),
@@ -162,7 +161,7 @@ func (i *instances) InstanceMetadata(_ context.Context, node *v1.Node) (*cloudpr
 }
 
 func (i *instances) getInstance(node *v1.Node) (*pxapi.VmRef, string, error) {
-	vm, region, err := i.parseProviderID(node.Spec.ProviderID)
+	vm, region, err := provider.ParseProviderID(node.Spec.ProviderID)
 	if err != nil {
 		return nil, "", fmt.Errorf("instances.getInstance() error: %v", err)
 	}
@@ -205,27 +204,3 @@ func (i *instances) getInstanceType(vmRef *pxapi.VmRef, region string) (string,
 		vmInfo["maxcpu"].(float64),
 		vmInfo["maxmem"].(float64)/1024/1024/1024), nil
 }
-
-var providerIDRegexp = regexp.MustCompile(`^` + ProviderName + `://([^/]*)/([^/]+)$`)
-
-func (i *instances) getProviderID(region string, vmr *pxapi.VmRef) string {
-	return fmt.Sprintf("%s://%s/%d", ProviderName, region, vmr.VmId())
-}
-
-func (i *instances) parseProviderID(providerID string) (*pxapi.VmRef, string, error) {
-	if !strings.HasPrefix(providerID, ProviderName) {
-		return nil, "", fmt.Errorf("foreign providerID or empty \"%s\"", providerID)
-	}
-
-	matches := providerIDRegexp.FindStringSubmatch(providerID)
-	if len(matches) != 3 {
-		return nil, "", fmt.Errorf("providerID \"%s\" didn't match expected format \"%s://region/InstanceID\"", providerID, ProviderName)
-	}
-
-	vmID, err := strconv.Atoi(matches[2])
-	if err != nil {
-		return nil, "", fmt.Errorf("providerID \"%s\" didn't match expected format \"%s://region/InstanceID\"", providerID, ProviderName)
-	}
-
-	return pxapi.NewVmRef(vmID), matches[1], nil
-}

pkg/proxmox/instances_test.go

@@ -29,6 +29,7 @@ import (
 	"github.com/stretchr/testify/suite"
 
 	"github.com/sergelogvinov/proxmox-cloud-controller-manager/pkg/cluster"
+	provider "github.com/sergelogvinov/proxmox-cloud-controller-manager/pkg/provider"
 
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -464,8 +465,6 @@ func (ts *ccmTestSuite) TestInstanceMetadata() {
 func TestGetProviderID(t *testing.T) {
 	t.Parallel()
 
-	i := newInstances(nil)
-
 	tests := []struct {
 		msg      string
 		region   string
@@ -492,7 +491,7 @@ func TestGetProviderID(t *testing.T) {
 		t.Run(fmt.Sprint(testCase.msg), func(t *testing.T) {
 			t.Parallel()
 
-			expected := i.getProviderID(testCase.region, testCase.vmr)
+			expected := provider.GetProviderID(testCase.region, testCase.vmr)
 			assert.Equal(t, expected, testCase.expected)
 		})
 	}
@@ -501,8 +500,6 @@ func TestGetProviderID(t *testing.T) {
 func TestParseProviderID(t *testing.T) {
 	t.Parallel()
 
-	i := newInstances(nil)
-
 	tests := []struct {
 		msg             string
 		magic           string
@@ -540,7 +537,7 @@ func TestParseProviderID(t *testing.T) {
 		{
 			msg:           "Cluster and wrong InstanceID",
 			magic:         "proxmox://cluster/name",
-			expectedError: fmt.Errorf("providerID \"proxmox://cluster/name\" didn't match expected format \"proxmox://region/InstanceID\""),
+			expectedError: fmt.Errorf("InstanceID have to be a number, but got \"name\""),
 		},
 	}
 
@@ -550,7 +547,7 @@ func TestParseProviderID(t *testing.T) {
 		t.Run(fmt.Sprint(testCase.msg), func(t *testing.T) {
 			t.Parallel()
 
-			vmr, cluster, err := i.parseProviderID(testCase.magic)
+			vmr, cluster, err := provider.ParseProviderID(testCase.magic)
 
 			if testCase.expectedError != nil {
 				assert.Equal(t, testCase.expectedError, err)