Protect CurrentRequestByDatacenter with RWMutex

api/v1alpha1/ionoscloudcluster_types.go

@@ -17,6 +17,8 @@ limitations under the License.
 package v1alpha1
 
 import (
+	"sync"
+
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
@@ -34,6 +36,10 @@ const (
 	IonosCloudClusterKind = "IonosCloudCluster"
 )
 
+// currentRequestByDatacenterMutex is used to synchronize access to the
+// IonosCloudCluster.Status.CurrentRequestByDatacenter map.
+var currentRequestByDatacenterMutex sync.RWMutex
+
 // IonosCloudClusterSpec defines the desired state of IonosCloudCluster.
 type IonosCloudClusterSpec struct {
 	// ControlPlaneEndpoint represents the endpoint used to communicate with the control plane.
@@ -66,6 +72,7 @@ type IonosCloudClusterStatus struct {
 	Conditions clusterv1.Conditions `json:"conditions,omitempty"`
 
 	// CurrentRequestByDatacenter maps data center IDs to a pending provisioning request made during reconciliation.
+	// Use the provided getters and setters to access and modify this map to ensure thread safety.
 	//+optional
 	CurrentRequestByDatacenter map[string]ProvisioningRequest `json:"currentRequest,omitempty"`
 
@@ -117,9 +124,20 @@ func (i *IonosCloudCluster) SetConditions(conditions clusterv1.Conditions) {
 	i.Status.Conditions = conditions
 }
 
+// GetCurrentRequestByDatacenter returns the current provisioning request for the given data center and a boolean
+// indicating if it exists.
+func (i *IonosCloudCluster) GetCurrentRequestByDatacenter(datacenterID string) (ProvisioningRequest, bool) {
+	currentRequestByDatacenterMutex.RLock()
+	defer currentRequestByDatacenterMutex.RUnlock()
+	req, ok := i.Status.CurrentRequestByDatacenter[datacenterID]
+	return req, ok
+}
+
 // SetCurrentRequestByDatacenter sets the current provisioning request for the given data center.
 // This function makes sure that the map is initialized before setting the request.
 func (i *IonosCloudCluster) SetCurrentRequestByDatacenter(datacenterID, method, status, requestPath string) {
+	currentRequestByDatacenterMutex.Lock()
+	defer currentRequestByDatacenterMutex.Unlock()
 	if i.Status.CurrentRequestByDatacenter == nil {
 		i.Status.CurrentRequestByDatacenter = map[string]ProvisioningRequest{}
 	}
@@ -132,6 +150,8 @@ func (i *IonosCloudCluster) SetCurrentRequestByDatacenter(datacenterID, method,
 
 // DeleteCurrentRequestByDatacenter deletes the current provisioning request for the given data center.
 func (i *IonosCloudCluster) DeleteCurrentRequestByDatacenter(datacenterID string) {
+	currentRequestByDatacenterMutex.Lock()
+	defer currentRequestByDatacenterMutex.Unlock()
 	delete(i.Status.CurrentRequestByDatacenter, datacenterID)
 }
 

api/v1alpha1/ionoscloudcluster_types_test.go

@@ -138,7 +138,9 @@ var _ = Describe("IonosCloudCluster", func() {
 			fetched := &IonosCloudCluster{}
 			Expect(k8sClient.Get(context.Background(), key, fetched)).To(Succeed())
 			Expect(fetched.Status.Ready).To(BeFalse())
+			currentRequestByDatacenterMutex.RLock()
 			Expect(fetched.Status.CurrentRequestByDatacenter).To(BeEmpty())
+			currentRequestByDatacenterMutex.RUnlock()
 			Expect(fetched.Status.Conditions).To(BeEmpty())
 
 			By("retrieving the cluster and setting the status")
@@ -157,8 +159,12 @@ var _ = Describe("IonosCloudCluster", func() {
 
 			Expect(k8sClient.Get(context.Background(), key, fetched)).To(Succeed())
 			Expect(fetched.Status.Ready).To(BeTrue())
+			currentRequestByDatacenterMutex.RLock()
 			Expect(fetched.Status.CurrentRequestByDatacenter).To(HaveLen(1))
-			Expect(fetched.Status.CurrentRequestByDatacenter["123"]).To(Equal(wantProvisionRequest))
+			currentRequestByDatacenterMutex.RUnlock()
+			gotProvisionRequest, exists := fetched.GetCurrentRequestByDatacenter("123")
+			Expect(exists).To(BeTrue())
+			Expect(gotProvisionRequest).To(Equal(wantProvisionRequest))
 			Expect(fetched.Status.Conditions).To(HaveLen(1))
 			Expect(conditions.IsTrue(fetched, clusterv1.ReadyCondition)).To(BeTrue())
 
@@ -167,7 +173,9 @@ var _ = Describe("IonosCloudCluster", func() {
 			Expect(k8sClient.Status().Update(context.Background(), fetched)).To(Succeed())
 
 			Expect(k8sClient.Get(context.Background(), key, fetched)).To(Succeed())
+			currentRequestByDatacenterMutex.RLock()
 			Expect(fetched.Status.CurrentRequestByDatacenter).To(BeEmpty())
+			currentRequestByDatacenterMutex.RUnlock()
 		})
 	})
 })

internal/controller/ionoscloudmachine_controller.go

@@ -259,9 +259,9 @@ func (*ionosCloudMachineReconciler) checkRequestStates(
 	cloudService *cloud.Service,
 ) (requeue bool, retErr error) {
 	log := ctrl.LoggerFrom(ctx)
-	// check cluster wide request
+	// check cluster-wide request
 	ionosCluster := machineScope.ClusterScope.IonosCluster
-	if req, exists := ionosCluster.Status.CurrentRequestByDatacenter[machineScope.DatacenterID()]; exists {
+	if req, exists := ionosCluster.GetCurrentRequestByDatacenter(machineScope.DatacenterID()); exists {
 		status, message, err := cloudService.GetRequestStatus(ctx, req.RequestPath)
 		if err != nil {
 			retErr = fmt.Errorf("could not get request status: %w", err)

internal/service/cloud/network_test.go

@@ -62,9 +62,8 @@ func (s *lanSuite) TestLANURLs() {
 func (s *lanSuite) TestNetworkCreateLANSuccessful() {
 	s.mockCreateLANCall().Return(exampleRequestPath, nil).Once()
 	s.NoError(s.service.createLAN(s.ctx, s.machineScope))
-	s.Contains(s.infraCluster.Status.CurrentRequestByDatacenter, s.machineScope.DatacenterID(),
-		"request should be stored in status")
-	req := s.infraCluster.Status.CurrentRequestByDatacenter[s.machineScope.DatacenterID()]
+	req, exists := s.infraCluster.GetCurrentRequestByDatacenter(s.machineScope.DatacenterID())
+	s.True(exists, "request should be stored in status")
 	s.Equal(exampleRequestPath, req.RequestPath, "request path should be stored in status")
 	s.Equal(http.MethodPost, req.Method, "request method should be stored in status")
 	s.Equal(sdk.RequestStatusQueued, req.State, "request status should be stored in status")
@@ -73,9 +72,8 @@ func (s *lanSuite) TestNetworkCreateLANSuccessful() {
 func (s *lanSuite) TestNetworkDeleteLANSuccessful() {
 	s.mockDeleteLANCall(exampleLANID).Return(exampleRequestPath, nil).Once()
 	s.NoError(s.service.deleteLAN(s.ctx, s.machineScope, exampleLANID))
-	s.Contains(s.infraCluster.Status.CurrentRequestByDatacenter, s.machineScope.DatacenterID(),
-		"request should be stored in status")
-	req := s.infraCluster.Status.CurrentRequestByDatacenter[s.machineScope.DatacenterID()]
+	req, exists := s.infraCluster.GetCurrentRequestByDatacenter(s.machineScope.DatacenterID())
+	s.True(exists, "request should be stored in status")
 	s.Equal(exampleRequestPath, req.RequestPath, "request path should be stored in status")
 	s.Equal(http.MethodDelete, req.Method, "request method should be stored in status")
 	s.Equal(sdk.RequestStatusQueued, req.State, "request status should be stored in status")
@@ -105,16 +103,11 @@ func (s *lanSuite) TestNetworkGetLANErrorNotUnique() {
 }
 
 func (s *lanSuite) TestNetworkRemoveLANPendingRequestFromClusterSuccessful() {
-	s.infraCluster.Status.CurrentRequestByDatacenter = map[string]infrav1.ProvisioningRequest{
-		s.machineScope.DatacenterID(): {
-			RequestPath: exampleRequestPath,
-			Method:      http.MethodDelete,
-			State:       sdk.RequestStatusQueued,
-		},
-	}
+	s.infraCluster.SetCurrentRequestByDatacenter(s.machineScope.DatacenterID(), http.MethodDelete, sdk.RequestStatusQueued,
+		exampleRequestPath)
 	s.NoError(s.service.removeLANPendingRequestFromCluster(s.machineScope))
-	s.NotContains(s.infraCluster.Status.CurrentRequestByDatacenter,
-		s.machineScope.DatacenterID(), "request should be removed from status")
+	_, exists := s.infraCluster.GetCurrentRequestByDatacenter(s.machineScope.DatacenterID())
+	s.False(exists, "request should be removed from status")
 }
 
 func (s *lanSuite) TestNetworkRemoveLANPendingRequestFromClusterNoRequest() {
@@ -171,7 +164,8 @@ func (s *lanSuite) TestNetworkReconcileLANDeleteLANExistsNoPendingRequestsHasOth
 	requeue, err := s.service.ReconcileLANDeletion(s.ctx, s.machineScope)
 	s.NoError(err)
 	s.False(requeue)
-	s.NotContains(s.infraCluster.Status.CurrentRequestByDatacenter, s.machineScope.DatacenterID())
+	_, exists := s.infraCluster.GetCurrentRequestByDatacenter(s.machineScope.DatacenterID())
+	s.False(exists)
 }
 
 func (s *lanSuite) TestNetworkReconcileLANDeleteNoExistingLANExistingRequestPending() {
@@ -197,7 +191,8 @@ func (s *lanSuite) TestNetworkReconcileLANDeleteLANDoesNotExist() {
 	requeue, err := s.service.ReconcileLANDeletion(s.ctx, s.machineScope)
 	s.NoError(err)
 	s.False(requeue)
-	s.NotContains(s.infraCluster.Status.CurrentRequestByDatacenter, s.machineScope.DatacenterID())
+	_, exists := s.infraCluster.GetCurrentRequestByDatacenter(s.machineScope.DatacenterID())
+	s.False(exists)
 }
 
 func (s *lanSuite) TestReconcileIPFailoverNICNotInFailoverGroup() {