refactor: configuration passing and NodeAddress validation (#864)

Refactor preparation for #859.
hetznercloud · Feb 10, 2025 · e777d81 · e777d81
1 parent fa11b67
commit e777d81
Show file tree

Hide file tree

Showing 4 changed files with 87 additions and 43 deletions.
diff --git a/hcloud/cloud.go b/hcloud/cloud.go
@@ -157,7 +157,7 @@ func (c *cloud) Instances() (cloudprovider.Instances, bool) {
 }
 
 func (c *cloud) InstancesV2() (cloudprovider.InstancesV2, bool) {
-	return newInstances(c.client, c.robotClient, c.recorder, c.cfg.Instance.AddressFamily, c.networkID), true
+	return newInstances(c.client, c.robotClient, c.recorder, c.networkID, c.cfg), true
 }
 
 func (c *cloud) Zones() (cloudprovider.Zones, bool) {

diff --git a/hcloud/cloud_test.go b/hcloud/cloud_test.go
@@ -30,6 +30,7 @@ import (
 	"k8s.io/client-go/kubernetes/scheme"
 	"k8s.io/client-go/tools/record"
 
+	"github.com/hetznercloud/hcloud-cloud-controller-manager/internal/config"
 	"github.com/hetznercloud/hcloud-cloud-controller-manager/internal/testsupport"
 	"github.com/hetznercloud/hcloud-go/v2/hcloud"
 	"github.com/hetznercloud/hcloud-go/v2/hcloud/schema"
@@ -41,6 +42,7 @@ type testEnv struct {
 	Client      *hcloud.Client
 	RobotClient hrobot.RobotClient
 	Recorder    record.EventRecorder
+	Cfg         config.HCCMConfiguration
 }
 
 func (env *testEnv) Teardown() {
@@ -65,12 +67,17 @@ func newTestEnv() testEnv {
 	robotClient := hrobot.NewBasicAuthClient("", "")
 	robotClient.SetBaseURL(server.URL + "/robot")
 	recorder := record.NewBroadcaster().NewRecorder(scheme.Scheme, corev1.EventSource{Component: "hcloud-cloud-controller-manager"})
+
+	cfg := config.HCCMConfiguration{}
+	cfg.Instance.AddressFamily = config.AddressFamilyIPv4
+
 	return testEnv{
 		Server:      server,
 		Mux:         mux,
 		Client:      client,
 		RobotClient: robotClient,
 		Recorder:    recorder,
+		Cfg:         cfg,
 	}
 }
 

diff --git a/hcloud/instances.go b/hcloud/instances.go
@@ -38,20 +38,32 @@ const (
 )
 
 type instances struct {
-	client        *hcloud.Client
-	robotClient   robot.Client
-	recorder      record.EventRecorder
-	addressFamily config.AddressFamily
-	networkID     int64
+	client      *hcloud.Client
+	robotClient robot.Client
+	recorder    record.EventRecorder
+	networkID   int64
+	cfg         config.HCCMConfiguration
 }
 
 var (
 	errServerNotFound     = errors.New("server not found")
 	errMissingRobotClient = errors.New("no robot client configured, make sure to enable Robot support in the configuration")
 )
 
-func newInstances(client *hcloud.Client, robotClient robot.Client, recorder record.EventRecorder, addressFamily config.AddressFamily, networkID int64) *instances {
-	return &instances{client, robotClient, recorder, addressFamily, networkID}
+func newInstances(
+	client *hcloud.Client,
+	robotClient robot.Client,
+	recorder record.EventRecorder,
+	networkID int64,
+	cfg config.HCCMConfiguration,
+) *instances {
+	return &instances{
+		client,
+		robotClient,
+		recorder,
+		networkID,
+		cfg,
+	}
 }
 
 // lookupServer attempts to locate the corresponding [*hcloud.Server] or [*hrobotmodels.Server] for a given [*corev1.Node].
@@ -184,41 +196,45 @@ func (i *instances) InstanceMetadata(ctx context.Context, node *corev1.Node) (*c
 			op, node.Name, errServerNotFound)
 	}
 
-	metadata, err := server.Metadata(i.addressFamily, i.networkID)
+	metadata, err := server.Metadata(i.networkID, i.cfg)
 	if err != nil {
 		return nil, fmt.Errorf("%s: %w", op, err)
 	}
 
 	return metadata, nil
 }
 
-func hcloudNodeAddresses(addressFamily config.AddressFamily, networkID int64, server *hcloud.Server) []corev1.NodeAddress {
+func hcloudNodeAddresses(
+	networkID int64,
+	server *hcloud.Server,
+	cfg config.HCCMConfiguration,
+) []corev1.NodeAddress {
 	var addresses []corev1.NodeAddress
 	addresses = append(
 		addresses,
 		corev1.NodeAddress{Type: corev1.NodeHostName, Address: server.Name},
 	)
 
-	if addressFamily == config.AddressFamilyIPv4 || addressFamily == config.AddressFamilyDualStack {
-		if !server.PublicNet.IPv4.IsUnspecified() {
-			addresses = append(
-				addresses,
-				corev1.NodeAddress{Type: corev1.NodeExternalIP, Address: server.PublicNet.IPv4.IP.String()},
-			)
-		}
+	dualStack := cfg.Instance.AddressFamily == config.AddressFamilyDualStack
+	ipv4 := cfg.Instance.AddressFamily == config.AddressFamilyIPv4 || dualStack
+	ipv6 := cfg.Instance.AddressFamily == config.AddressFamilyIPv6 || dualStack
+
+	if ipv4 && !server.PublicNet.IPv4.IsUnspecified() {
+		addresses = append(
+			addresses,
+			corev1.NodeAddress{Type: corev1.NodeExternalIP, Address: server.PublicNet.IPv4.IP.String()},
+		)
 	}
 
-	if addressFamily == config.AddressFamilyIPv6 || addressFamily == config.AddressFamilyDualStack {
-		if !server.PublicNet.IPv6.IsUnspecified() {
-			// For a given IPv6 network of 2001:db8:1234::/64, the instance address is 2001:db8:1234::1
-			hostAddress := server.PublicNet.IPv6.IP
-			hostAddress[len(hostAddress)-1] |= 0x01
+	if ipv6 && !server.PublicNet.IPv6.IsUnspecified() {
+		// For a given IPv6 network of 2001:db8:1234::/64, the instance address is 2001:db8:1234::1
+		hostAddress := server.PublicNet.IPv6.IP
+		hostAddress[len(hostAddress)-1] |= 0x01
 
-			addresses = append(
-				addresses,
-				corev1.NodeAddress{Type: corev1.NodeExternalIP, Address: hostAddress.String()},
-			)
-		}
+		addresses = append(
+			addresses,
+			corev1.NodeAddress{Type: corev1.NodeExternalIP, Address: hostAddress.String()},
+		)
 	}
 
 	// Add private IP from network if network is specified
@@ -232,28 +248,35 @@ func hcloudNodeAddresses(addressFamily config.AddressFamily, networkID int64, se
 			}
 		}
 	}
+
 	return addresses
 }
 
-func robotNodeAddresses(addressFamily config.AddressFamily, server *hrobotmodels.Server) []corev1.NodeAddress {
+func robotNodeAddresses(
+	server *hrobotmodels.Server,
+	cfg config.HCCMConfiguration,
+) []corev1.NodeAddress {
 	var addresses []corev1.NodeAddress
 	addresses = append(
 		addresses,
 		corev1.NodeAddress{Type: corev1.NodeHostName, Address: server.Name},
 	)
 
-	if addressFamily == config.AddressFamilyIPv6 || addressFamily == config.AddressFamilyDualStack {
+	dualStack := cfg.Instance.AddressFamily == config.AddressFamilyDualStack
+	ipv4 := cfg.Instance.AddressFamily == config.AddressFamilyIPv4 || dualStack
+	ipv6 := cfg.Instance.AddressFamily == config.AddressFamilyIPv6 || dualStack
+
+	if ipv6 {
 		// For a given IPv6 network of 2a01:f48:111:4221::, the instance address is 2a01:f48:111:4221::1
-		hostAddress := server.ServerIPv6Net
-		hostAddress += "1"
+		hostAddress := server.ServerIPv6Net + "1"
 
 		addresses = append(
 			addresses,
 			corev1.NodeAddress{Type: corev1.NodeExternalIP, Address: hostAddress},
 		)
 	}
 
-	if addressFamily == config.AddressFamilyIPv4 || addressFamily == config.AddressFamilyDualStack {
+	if ipv4 {
 		addresses = append(
 			addresses,
 			corev1.NodeAddress{Type: corev1.NodeExternalIP, Address: server.ServerIP},
@@ -265,7 +288,10 @@ func robotNodeAddresses(addressFamily config.AddressFamily, server *hrobotmodels
 
 type genericServer interface {
 	IsShutdown() (bool, error)
-	Metadata(addressFamily config.AddressFamily, networkID int64) (*cloudprovider.InstanceMetadata, error)
+	Metadata(
+		networkID int64,
+		cfg config.HCCMConfiguration,
+	) (*cloudprovider.InstanceMetadata, error)
 }
 
 type hcloudServer struct {
@@ -276,11 +302,11 @@ func (s hcloudServer) IsShutdown() (bool, error) {
 	return s.Status == hcloud.ServerStatusOff, nil
 }
 
-func (s hcloudServer) Metadata(addressFamily config.AddressFamily, networkID int64) (*cloudprovider.InstanceMetadata, error) {
+func (s hcloudServer) Metadata(networkID int64, cfg config.HCCMConfiguration) (*cloudprovider.InstanceMetadata, error) {
 	return &cloudprovider.InstanceMetadata{
 		ProviderID:    providerid.FromCloudServerID(s.ID),
 		InstanceType:  s.ServerType.Name,
-		NodeAddresses: hcloudNodeAddresses(addressFamily, networkID, s.Server),
+		NodeAddresses: hcloudNodeAddresses(networkID, s.Server, cfg),
 		Zone:          s.Datacenter.Name,
 		Region:        s.Datacenter.Location.Name,
 		AdditionalLabels: map[string]string{
@@ -305,11 +331,11 @@ func (s robotServer) IsShutdown() (bool, error) {
 	return resetStatus.OperatingStatus == "shut off", nil
 }
 
-func (s robotServer) Metadata(addressFamily config.AddressFamily, _ int64) (*cloudprovider.InstanceMetadata, error) {
+func (s robotServer) Metadata(_ int64, cfg config.HCCMConfiguration) (*cloudprovider.InstanceMetadata, error) {
 	return &cloudprovider.InstanceMetadata{
 		ProviderID:    providerid.FromRobotServerNumber(s.ServerNumber),
 		InstanceType:  getInstanceTypeOfRobotServer(s.Server),
-		NodeAddresses: robotNodeAddresses(addressFamily, s.Server),
+		NodeAddresses: robotNodeAddresses(s.Server, cfg),
 		Zone:          getZoneOfRobotServer(s.Server),
 		Region:        getRegionOfRobotServer(s.Server),
 		AdditionalLabels: map[string]string{

diff --git a/hcloud/instances_test.go b/hcloud/instances_test.go
@@ -21,9 +21,11 @@ import (
 	"encoding/json"
 	"net"
 	"net/http"
+	"os"
 	"reflect"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
 	hrobotmodels "github.com/syself/hrobot-go/models"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -89,7 +91,7 @@ func TestInstances_InstanceExists(t *testing.T) {
 		})
 	})
 
-	instances := newInstances(env.Client, env.RobotClient, env.Recorder, config.AddressFamilyIPv4, 0)
+	instances := newInstances(env.Client, env.RobotClient, env.Recorder, 0, env.Cfg)
 
 	tests := []struct {
 		name     string
@@ -209,7 +211,7 @@ func TestInstances_InstanceShutdown(t *testing.T) {
 		})
 	})
 
-	instances := newInstances(env.Client, env.RobotClient, env.Recorder, config.AddressFamilyIPv4, 0)
+	instances := newInstances(env.Client, env.RobotClient, env.Recorder, 0, env.Cfg)
 	env.Mux.HandleFunc("/robot/server/3", func(w http.ResponseWriter, _ *http.Request) {
 		json.NewEncoder(w).Encode(hrobotmodels.ServerResponse{
 			Server: hrobotmodels.Server{
@@ -343,7 +345,7 @@ func TestInstances_InstanceMetadata(t *testing.T) {
 		})
 	})
 
-	instances := newInstances(env.Client, env.RobotClient, env.Recorder, config.AddressFamilyIPv4, 0)
+	instances := newInstances(env.Client, env.RobotClient, env.Recorder, 0, env.Cfg)
 
 	metadata, err := instances.InstanceMetadata(context.TODO(), &corev1.Node{
 		Spec: corev1.NodeSpec{ProviderID: "hcloud://1"},
@@ -387,7 +389,7 @@ func TestInstances_InstanceMetadataRobotServer(t *testing.T) {
 		})
 	})
 
-	instances := newInstances(env.Client, env.RobotClient, env.Recorder, config.AddressFamilyIPv4, 0)
+	instances := newInstances(env.Client, env.RobotClient, env.Recorder, 0, env.Cfg)
 
 	metadata, err := instances.InstanceMetadata(context.TODO(), &corev1.Node{
 		ObjectMeta: metav1.ObjectMeta{
@@ -581,7 +583,11 @@ func TestNodeAddresses(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			addresses := hcloudNodeAddresses(test.addressFamily, test.privateNetwork, test.server)
+			os.Setenv("HCLOUD_INSTANCES_ADDRESS_FAMILY", string(test.addressFamily))
+			cfg, err := config.Read()
+			assert.NoError(t, err)
+
+			addresses := hcloudNodeAddresses(test.privateNetwork, test.server, cfg)
 
 			if !reflect.DeepEqual(addresses, test.expected) {
 				t.Fatalf("Expected addresses %+v but got %+v", test.expected, addresses)
@@ -642,7 +648,12 @@ func TestNodeAddressesRobotServer(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			addresses := robotNodeAddresses(test.addressFamily, test.server)
+			os.Setenv("ROBOT_ENABLED", "true")
+			os.Setenv("HCLOUD_INSTANCES_ADDRESS_FAMILY", string(test.addressFamily))
+			cfg, err := config.Read()
+			assert.NoError(t, err)
+
+			addresses := robotNodeAddresses(test.server, cfg)
 
 			if !reflect.DeepEqual(addresses, test.expected) {
 				t.Fatalf("%s: expected addresses %+v but got %+v", test.name, test.expected, addresses)