Fix some race conditions

controllers/etcdcluster_controller_test.go

@@ -2,9 +2,11 @@ package controllers
 
 import (
 	"context"
+	"encoding/json"
 	"errors"
 	"fmt"
 	"net/url"
+	"sync"
 	"testing"
 	"time"
 
@@ -36,20 +38,44 @@ func (_ *AlwaysFailEtcdAPI) MembershipAPI(_ etcdclient.Config) (etcdclient.Membe
 // testing the interactions with the Etcd client API.
 // The `parent` field is a pointer so that a test and the controller-under-test see the
 // same shared Members list.
-// TODO(wallrj) Add locking if we ever want to have the test mutate the Members list.
 type StaticResponseMembersAPI struct {
+	sync.RWMutex
 	parent *StaticResponseEtcdAPI
 }
 
+// deepCopyEtcdClientMember makes a copy of the supplied Member
+func deepCopyEtcdClientMember(in etcdclient.Member) (out etcdclient.Member, err error) {
+	encoded, err := json.Marshal(in)
+	if err != nil {
+		return out, err
+	}
+	err = json.Unmarshal(encoded, &out)
+	return out, err
+}
+
 func (s *StaticResponseMembersAPI) List(ctx context.Context) ([]etcdclient.Member, error) {
-	return s.parent.Members, nil
+	s.RLock()
+	defer s.RUnlock()
+	out := make([]etcdclient.Member, len(s.parent.Members))
+	for i := 0; i < len(out); i++ {
+		new, err := deepCopyEtcdClientMember(s.parent.Members[i])
+		if err != nil {
+			return nil, err
+		}
+		out[i] = new
+	}
+	return out, nil
 }
 
 func (s *StaticResponseMembersAPI) Add(ctx context.Context, peerURL string) (*etcdclient.Member, error) {
+	s.Lock()
+	defer s.Unlock()
 	panic("implement me")
 }
 
 func (s *StaticResponseMembersAPI) Remove(ctx context.Context, mID string) error {
+	s.Lock()
+	defer s.Unlock()
 	for i := range s.parent.Members {
 		if s.parent.Members[i].ID == mID {
 			s.parent.Members = append(s.parent.Members[:i], s.parent.Members[i+1:]...)
@@ -60,10 +86,14 @@ func (s *StaticResponseMembersAPI) Remove(ctx context.Context, mID string) error
 }
 
 func (s *StaticResponseMembersAPI) Update(ctx context.Context, mID string, peerURLs []string) error {
+	s.Lock()
+	defer s.Unlock()
 	panic("implement me")
 }
 
 func (s *StaticResponseMembersAPI) Leader(ctx context.Context) (*etcdclient.Member, error) {
+	s.RLock()
+	defer s.RUnlock()
 	panic("implement me")
 }
 
@@ -341,7 +371,7 @@ func (s *controllerSuite) testClusterController(t *testing.T) {
 					ClientURLs: []string{clientURL.String()},
 				}
 			}
-			s.etcd = &StaticResponseEtcdAPI{Members: members}
+			s.setEtcd(&StaticResponseEtcdAPI{Members: members})
 
 			err = try.Eventually(func() error {
 				fetchedCluster := &etcdv1alpha1.EtcdCluster{}

controllers/suite_test.go

@@ -3,6 +3,7 @@ package controllers
 import (
 	"context"
 	"path/filepath"
+	"sync"
 	"testing"
 	"time"
 
@@ -24,6 +25,7 @@ import (
 )
 
 type controllerSuite struct {
+	sync.RWMutex
 	ctx       context.Context
 	cfg       *rest.Config
 	k8sClient client.Client
@@ -75,9 +77,17 @@ func setupSuite(t *testing.T) (suite *controllerSuite, teardownFunc func()) {
 // can pass ourselves in as an implementation of an EtcdAPI at test assembly time, but a test can switch out it's
 // internal implementation in the middle of a test by setting `controllerSuite.etcd`
 func (s *controllerSuite) MembershipAPI(config etcdclient.Config) (etcdclient.MembersAPI, error) {
+	s.RLock()
+	defer s.RUnlock()
 	return s.etcd.MembershipAPI(config)
 }
 
+func (s *controllerSuite) setEtcd(api etcd.EtcdAPI) {
+	s.Lock()
+	defer s.Unlock()
+	s.etcd = api
+}
+
 func (s *controllerSuite) setupTest(t *testing.T) (teardownFunc func(), namespaceName string) {
 	stopCh := make(chan struct{})
 	logger := test.TestLogger{
@@ -125,16 +135,21 @@ func (s *controllerSuite) setupTest(t *testing.T) (teardownFunc func(), namespac
 	}
 	err = backupController.SetupWithManager(mgr)
 	require.NoError(t, err, "failed to setup EtcdBackupSchedule controller")
-
+	var wg sync.WaitGroup
+	wg.Add(1)
 	go func() {
+		defer wg.Done()
 		err := mgr.Start(stopCh)
 		require.NoError(t, err, "failed to start manager")
 	}()
 
 	return func() {
+		defer func() {
+			close(stopCh)
+			wg.Wait()
+		}()
 		err := s.k8sClient.Delete(s.ctx, namespace)
 		require.NoErrorf(t, err, "Failed to delete test namespace: %#v", namespace)
-		close(stopCh)
 	}, namespace.Name
 }