🌱 Refactor statusFromHCloudServer (#1649)

Author: guettli

go.mod

@@ -9,6 +9,7 @@ require (
 	github.com/go-logr/zapr v1.3.0
 	github.com/guettli/check-conditions v0.0.9
 	github.com/hetznercloud/hcloud-go/v2 v2.19.1
+	github.com/mitchellh/copystructure v1.2.0
 	github.com/onsi/ginkgo/v2 v2.23.0
 	github.com/onsi/gomega v1.36.2
 	github.com/prometheus/common v0.63.0
@@ -89,7 +90,6 @@ require (
 	github.com/magiconair/properties v1.8.7 // indirect
 	github.com/mailru/easyjson v0.7.7 // indirect
 	github.com/mattn/go-isatty v0.0.20 // indirect
-	github.com/mitchellh/copystructure v1.2.0 // indirect
 	github.com/mitchellh/mapstructure v1.5.0 // indirect
 	github.com/mitchellh/reflectwalk v1.0.2 // indirect
 	github.com/moby/docker-image-spec v1.3.1 // indirect

pkg/services/hcloud/server/server.go

@@ -21,10 +21,12 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"net"
 	"time"
 
 	"github.com/hetznercloud/hcloud-go/v2/hcloud"
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/utils/ptr"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	controlplanev1 "sigs.k8s.io/cluster-api/controlplane/kubeadm/api/v1beta1"
 	capierrors "sigs.k8s.io/cluster-api/errors"
@@ -110,12 +112,10 @@ func (s *Service) Reconcile(ctx context.Context) (res reconcile.Result, err erro
 	s.scope.SetProviderID(server.ID)
 
 	// update HCloudMachineStatus
-	c := s.scope.HCloudMachine.Status.Conditions.DeepCopy()
-	sshKeys := s.scope.HCloudMachine.Status.SSHKeys
-	s.scope.HCloudMachine.Status = statusFromHCloudServer(server)
-	s.scope.SetRegion(failureDomain)
-	s.scope.HCloudMachine.Status.Conditions = c
-	s.scope.HCloudMachine.Status.SSHKeys = sshKeys
+	s.scope.HCloudMachine.Status.Addresses = statusAddresses(server)
+
+	// Copy value
+	s.scope.HCloudMachine.Status.InstanceState = ptr.To(server.Status)
 
 	// validate labels
 	if err := validateLabels(server, s.createLabels()); err != nil {
@@ -738,10 +738,7 @@ func validateLabels(server *hcloud.Server, labels map[string]string) error {
 	return nil
 }
 
-func statusFromHCloudServer(server *hcloud.Server) infrav1.HCloudMachineStatus {
-	// set instance state
-	instanceState := server.Status
-
+func statusAddresses(server *hcloud.Server) []clusterv1.MachineAddress {
 	// populate addresses
 	addresses := []clusterv1.MachineAddress{}
 
@@ -755,8 +752,15 @@ func statusFromHCloudServer(server *hcloud.Server) infrav1.HCloudMachineStatus {
 		)
 	}
 
-	if ip := server.PublicNet.IPv6.IP; ip.IsGlobalUnicast() {
+	if unicastIP := server.PublicNet.IPv6.IP; unicastIP.IsGlobalUnicast() {
+		// Create a copy. This is important, otherwise we modify the IP of `server`. This could lead
+		// to unexpected behaviour.
+		ip := append(net.IP(nil), unicastIP...)
+
+		// Hetzner returns the routed /64 base, increment last byte to obtain first usable address
+		// The local value gets changed, not the IP of `server`.
 		ip[15]++
+
 		addresses = append(
 			addresses,
 			clusterv1.MachineAddress{
@@ -776,10 +780,7 @@ func statusFromHCloudServer(server *hcloud.Server) infrav1.HCloudMachineStatus {
 		)
 	}
 
-	return infrav1.HCloudMachineStatus{
-		InstanceState: &instanceState,
-		Addresses:     addresses,
-	}
+	return addresses
 }
 
 func (s *Service) createLabels() map[string]string {

pkg/services/hcloud/server/server_suite_test.go

@@ -25,7 +25,6 @@ import (
 	"github.com/hetznercloud/hcloud-go/v2/hcloud/schema"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
-	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 
 	infrav1 "github.com/syself/cluster-api-provider-hetzner/api/v1beta1"
 	"github.com/syself/cluster-api-provider-hetzner/pkg/scope"
@@ -188,27 +187,25 @@ const serverJSON = `
 	"volumes": []
 }`
 
-var server *hcloud.Server
-
-const instanceState = hcloud.ServerStatusRunning
-
-var ips = []string{"1.2.3.4", "2001:db8::3", "10.0.0.2"}
-var addressTypes = []clusterv1.MachineAddressType{clusterv1.MachineExternalIP, clusterv1.MachineExternalIP, clusterv1.MachineInternalIP}
-
 func TestServer(t *testing.T) {
 	RegisterFailHandler(Fail)
 	RunSpecs(t, "Server Suite")
 }
 
-var _ = BeforeSuite(func() {
+func newTestServer() *hcloud.Server {
 	var serverSchema schema.Server
 	b := []byte(serverJSON)
 	var buffer bytes.Buffer
-	Expect(json.Compact(&buffer, b))
-	Expect(json.Unmarshal(buffer.Bytes(), &serverSchema)).To(Succeed())
-
-	server = hcloud.ServerFromSchema(serverSchema)
-})
+	err := json.Compact(&buffer, b)
+	if err != nil {
+		panic(err)
+	}
+	err = json.Unmarshal(buffer.Bytes(), &serverSchema)
+	if err != nil {
+		panic(err)
+	}
+	return hcloud.ServerFromSchema(serverSchema)
+}
 
 func newTestService(hcloudMachine *infrav1.HCloudMachine, hcloudClient hcloudclient.Client) *Service {
 	return &Service{

pkg/services/hcloud/server/server_test.go

@@ -19,11 +19,14 @@ package server
 import (
 	"context"
 	"fmt"
+	"testing"
 	"time"
 
 	"github.com/hetznercloud/hcloud-go/v2/hcloud"
+	"github.com/mitchellh/copystructure"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
+	"github.com/stretchr/testify/require"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/utils/ptr"
@@ -36,28 +39,33 @@ import (
 	fakeclient "github.com/syself/cluster-api-provider-hetzner/pkg/services/hcloud/client/fake"
 )
 
-var _ = Describe("statusFromHCloudServer", func() {
-	var sts infrav1.HCloudMachineStatus
-	BeforeEach(func() {
-		sts = statusFromHCloudServer(server)
-	})
-	It("should have the right instance state", func() {
-		Expect(*sts.InstanceState).To(Equal(instanceState))
-	})
-	It("should have three addresses", func() {
-		Expect(len(sts.Addresses)).To(Equal(3))
-	})
-	It("should have the right address IPs", func() {
-		for i, addr := range sts.Addresses {
-			Expect(addr.Address).To(Equal(ips[i]))
-		}
-	})
-	It("should have the right address types", func() {
-		for i, addr := range sts.Addresses {
-			Expect(addr.Type).To(Equal(addressTypes[i]))
-		}
-	})
-})
+func Test_statusAddresses(t *testing.T) {
+	server := newTestServer()
+
+	// Create deep copy.
+	saved, err := copystructure.Copy(server)
+	require.NoError(t, err)
+
+	addresses := statusAddresses(server)
+
+	// should have three addresses
+	require.Equal(t, 3, len(addresses))
+
+	// should have the right address IPs
+	ips := []string{"1.2.3.4", "2001:db8::1", "10.0.0.2"}
+	for i, addr := range addresses {
+		require.Equal(t, ips[i], addr.Address)
+	}
+
+	// Check that input was not altered.
+	require.Equal(t, saved, server)
+
+	// should have the right address types
+	addressTypes := []clusterv1.MachineAddressType{clusterv1.MachineExternalIP, clusterv1.MachineExternalIP, clusterv1.MachineInternalIP}
+	for i, addr := range addresses {
+		require.Equal(t, addressTypes[i], addr.Type)
+	}
+}
 
 type testCaseStatusFromHCloudServer struct {
 	isControlPlane bool