Make regionsCache function argument instead of global variable

pkg/actuators/machine/actuator.go

@@ -43,6 +43,7 @@ type Actuator struct {
 	eventRecorder       record.EventRecorder
 	awsClientBuilder    awsclient.AwsClientBuilderFuncType
 	configManagedClient runtimeclient.Client
+	regionsCache        map[string]awsclient.DescribeRegionsData
 }
 
 // ActuatorParams holds parameter information for Actuator.
@@ -60,6 +61,7 @@ func NewActuator(params ActuatorParams) *Actuator {
 		eventRecorder:       params.EventRecorder,
 		awsClientBuilder:    params.AwsClientBuilder,
 		configManagedClient: params.ConfigManagedClient,
+		regionsCache:        map[string]awsclient.DescribeRegionsData{},
 	}
 }
 
@@ -82,6 +84,7 @@ func (a *Actuator) Create(ctx context.Context, machine *machinev1beta1.Machine)
 		machine:             machine,
 		awsClientBuilder:    a.awsClientBuilder,
 		configManagedClient: a.configManagedClient,
+		regionsCache:        a.regionsCache,
 	})
 	if err != nil {
 		fmtErr := fmt.Errorf(scopeFailFmt, machine.GetName(), err)
@@ -108,6 +111,7 @@ func (a *Actuator) Exists(ctx context.Context, machine *machinev1beta1.Machine)
 		machine:             machine,
 		awsClientBuilder:    a.awsClientBuilder,
 		configManagedClient: a.configManagedClient,
+		regionsCache:        a.regionsCache,
 	})
 	if err != nil {
 		return false, fmt.Errorf(scopeFailFmt, machine.GetName(), err)
@@ -124,6 +128,7 @@ func (a *Actuator) Update(ctx context.Context, machine *machinev1beta1.Machine)
 		machine:             machine,
 		awsClientBuilder:    a.awsClientBuilder,
 		configManagedClient: a.configManagedClient,
+		regionsCache:        a.regionsCache,
 	})
 	if err != nil {
 		fmtErr := fmt.Errorf(scopeFailFmt, machine.GetName(), err)
@@ -163,6 +168,7 @@ func (a *Actuator) Delete(ctx context.Context, machine *machinev1beta1.Machine)
 		machine:             machine,
 		awsClientBuilder:    a.awsClientBuilder,
 		configManagedClient: a.configManagedClient,
+		regionsCache:        a.regionsCache,
 	})
 	if err != nil {
 		fmtErr := fmt.Errorf(scopeFailFmt, machine.GetName(), err)

pkg/actuators/machine/actuator_test.go

@@ -170,11 +170,11 @@ func TestMachineEvents(t *testing.T) {
 
 			mockCtrl := gomock.NewController(t)
 			mockAWSClient := mockaws.NewMockClient(mockCtrl)
-			awsClientBuilder := func(client runtimeclient.Client, secretName, namespace, region string, configManagedClient runtimeclient.Client) (awsclient.Client, error) {
+			awsClientBuilder := func(client runtimeclient.Client, secretName, namespace, region string, configManagedClient runtimeclient.Client, regionsCache map[string]awsclient.DescribeRegionsData) (awsclient.Client, error) {
 				return mockAWSClient, nil
 			}
 			if tc.invalidMachineScope {
-				awsClientBuilder = func(client runtimeclient.Client, secretName, namespace, region string, configManagedClient runtimeclient.Client) (awsclient.Client, error) {
+				awsClientBuilder = func(client runtimeclient.Client, secretName, namespace, region string, configManagedClient runtimeclient.Client, regionsCache map[string]awsclient.DescribeRegionsData) (awsclient.Client, error) {
 					return nil, errors.New("AWS client error")
 				}
 			}

pkg/actuators/machine/controller_test.go

@@ -26,7 +26,7 @@ func TestMachineControllerWithDelayedExistSuccess(t *testing.T) {
 
 	mockCtrl := gomock.NewController(t)
 	mockAWSClient := mockaws.NewMockClient(mockCtrl)
-	awsClientBuilder := func(client runtimeclient.Client, secretName, namespace, region string, configManagedClient runtimeclient.Client) (awsclient.Client, error) {
+	awsClientBuilder := func(client runtimeclient.Client, secretName, namespace, region string, configManagedClient runtimeclient.Client, regionsCache map[string]awsclient.DescribeRegionsData) (awsclient.Client, error) {
 		return mockAWSClient, nil
 	}
 

pkg/actuators/machine/machine_scope.go

@@ -33,6 +33,8 @@ type machineScopeParams struct {
 	machine *machinev1beta1.Machine
 	// api server controller runtime client for the openshift-config-managed namespace
 	configManagedClient runtimeclient.Client
+	// accessKeyID (string) to *DescribeRegionsData map
+	regionsCache map[string]awsclient.DescribeRegionsData
 }
 
 type machineScope struct {
@@ -66,7 +68,7 @@ func newMachineScope(params machineScopeParams) (*machineScope, error) {
 		credentialsSecretName = providerSpec.CredentialsSecret.Name
 	}
 
-	awsClient, err := params.awsClientBuilder(params.client, credentialsSecretName, params.machine.Namespace, providerSpec.Placement.Region, params.configManagedClient)
+	awsClient, err := params.awsClientBuilder(params.client, credentialsSecretName, params.machine.Namespace, providerSpec.Placement.Region, params.configManagedClient, params.regionsCache)
 	if err != nil {
 		return nil, machineapierros.InvalidMachineConfiguration("failed to create aws client: %v", err.Error())
 	}

pkg/actuators/machine/machine_scope_test.go

@@ -260,7 +260,7 @@ func TestPatchMachine(t *testing.T) {
 			machineScope, err := newMachineScope(machineScopeParams{
 				client:  k8sClient,
 				machine: machine,
-				awsClientBuilder: func(client runtimeclient.Client, secretName, namespace, region string, configManagedClient runtimeclient.Client) (awsclient.Client, error) {
+				awsClientBuilder: func(client runtimeclient.Client, secretName, namespace, region string, configManagedClient runtimeclient.Client, regionsCache map[string]awsclient.DescribeRegionsData) (awsclient.Client, error) {
 					return nil, nil
 				},
 			})

pkg/actuators/machine/reconciler_test.go

@@ -99,7 +99,7 @@ func TestAvailabilityZone(t *testing.T) {
 			machineScope, err := newMachineScope(machineScopeParams{
 				client:  fakeClient,
 				machine: machine,
-				awsClientBuilder: func(client runtimeclient.Client, secretName, namespace, region string, configManagedClient runtimeclient.Client) (awsclient.Client, error) {
+				awsClientBuilder: func(client runtimeclient.Client, secretName, namespace, region string, configManagedClient runtimeclient.Client, regionsCache map[string]awsclient.DescribeRegionsData) (awsclient.Client, error) {
 					return mockAWSClient, nil
 				},
 			})
@@ -490,7 +490,7 @@ func TestCreate(t *testing.T) {
 		machineScope, err := newMachineScope(machineScopeParams{
 			client:  fakeClient,
 			machine: machine,
-			awsClientBuilder: func(client runtimeclient.Client, secretName, namespace, region string, configManagedClient runtimeclient.Client) (awsclient.Client, error) {
+			awsClientBuilder: func(client runtimeclient.Client, secretName, namespace, region string, configManagedClient runtimeclient.Client, regionsCache map[string]awsclient.DescribeRegionsData) (awsclient.Client, error) {
 				return mockAWSClient, nil
 			},
 		})
@@ -624,7 +624,7 @@ func TestExists(t *testing.T) {
 			machineScope, err := newMachineScope(machineScopeParams{
 				client:  fakeClient,
 				machine: tc.machine(),
-				awsClientBuilder: func(client runtimeclient.Client, secretName, namespace, region string, configManagedClient runtimeclient.Client) (awsclient.Client, error) {
+				awsClientBuilder: func(client runtimeclient.Client, secretName, namespace, region string, configManagedClient runtimeclient.Client, regionsCache map[string]awsclient.DescribeRegionsData) (awsclient.Client, error) {
 					return tc.awsClient(ctrl), nil
 				},
 			})
@@ -733,7 +733,7 @@ func TestUpdate(t *testing.T) {
 			machineScope, err := newMachineScope(machineScopeParams{
 				client:  fakeClient,
 				machine: tc.machine(),
-				awsClientBuilder: func(client runtimeclient.Client, secretName, namespace, region string, configManagedClient runtimeclient.Client) (awsclient.Client, error) {
+				awsClientBuilder: func(client runtimeclient.Client, secretName, namespace, region string, configManagedClient runtimeclient.Client, regionsCache map[string]awsclient.DescribeRegionsData) (awsclient.Client, error) {
 					return tc.awsClient(ctrl), nil
 				},
 			})
@@ -890,7 +890,7 @@ func TestDelete(t *testing.T) {
 			machineScope, err := newMachineScope(machineScopeParams{
 				client:  fakeClient,
 				machine: tc.machine(),
-				awsClientBuilder: func(client runtimeclient.Client, secretName, namespace, region string, configManagedClient runtimeclient.Client) (awsclient.Client, error) {
+				awsClientBuilder: func(client runtimeclient.Client, secretName, namespace, region string, configManagedClient runtimeclient.Client, regionsCache map[string]awsclient.DescribeRegionsData) (awsclient.Client, error) {
 					return tc.awsClient(ctrl), nil
 				},
 			})
@@ -1028,7 +1028,7 @@ func TestGetMachineInstances(t *testing.T) {
 			machineScope, err := newMachineScope(machineScopeParams{
 				client:  fakeClient,
 				machine: machineCopy,
-				awsClientBuilder: func(client runtimeclient.Client, secretName, namespace, region string, configManagedClient runtimeclient.Client) (awsclient.Client, error) {
+				awsClientBuilder: func(client runtimeclient.Client, secretName, namespace, region string, configManagedClient runtimeclient.Client, regionsCache map[string]awsclient.DescribeRegionsData) (awsclient.Client, error) {
 					return mockAWSClient, nil
 				},
 			})

pkg/client/client.go

@@ -23,7 +23,6 @@ import (
 	"io/ioutil"
 	"os"
 	"strings"
-	"sync"
 	"time"
 
 	"github.com/openshift/machine-api-provider-aws/pkg/version"
@@ -69,7 +68,7 @@ const (
 )
 
 // AwsClientBuilderFuncType is function type for building aws client
-type AwsClientBuilderFuncType func(client client.Client, secretName, namespace, region string, configManagedClient client.Client) (Client, error)
+type AwsClientBuilderFuncType func(client client.Client, secretName, namespace, region string, configManagedClient client.Client, regionsCache map[string]DescribeRegionsData) (Client, error)
 
 // Client is a wrapper object for actual AWS SDK clients to allow for easier testing.
 type Client interface {
@@ -209,35 +208,24 @@ func NewClientFromKeys(accessKey, secretAccessKey, region string) (Client, error
 	}, nil
 }
 
-type describeRegionsData struct {
+// DescribeRegionsData holds output of DescribeRegions API call and the time when it was last updated.
+type DescribeRegionsData struct {
 	err                   error
 	describeRegionsOutput *ec2.DescribeRegionsOutput
 	lastUpdated           time.Time
 }
 
-// Do not access directly, use cachedAWSDescribeRegions() instead.
-var (
-	regionsCacheMap  map[string]*describeRegionsData
-	regionsCacheLock sync.Mutex
-)
-
 // cachedDescribeRegions returns full list of regions from AWS.
 // DescribeRegionsOutput is cached to avoid AWS API calls on each reconcile loop.
-func cachedAWSDescribeRegions(awsSession *session.Session) (*ec2.DescribeRegionsOutput, error) {
-	regionsCacheLock.Lock()
-	defer regionsCacheLock.Unlock()
-	if regionsCacheMap == nil {
-		regionsCacheMap = make(map[string]*describeRegionsData)
-	}
+func cachedAWSDescribeRegions(awsSession *session.Session, regionsCacheMap map[string]DescribeRegionsData) (*ec2.DescribeRegionsOutput, error) {
 	creds, err := awsSession.Config.Credentials.Get()
 	if err != nil {
 		return nil, err
 	}
 	accessKeyID := creds.AccessKeyID
 	regionsCache, found := regionsCacheMap[accessKeyID]
 	if !found {
-		regionsCache = &describeRegionsData{}
-		regionsCacheMap[accessKeyID] = regionsCache
+		regionsCache = DescribeRegionsData{}
 	}
 
 	if regionsCache.describeRegionsOutput != nil && regionsCache.err == nil &&
@@ -262,6 +250,7 @@ func cachedAWSDescribeRegions(awsSession *session.Session) (*ec2.DescribeRegions
 
 	regionsCache.describeRegionsOutput = describeRegionsOutput
 	regionsCache.lastUpdated = time.Now()
+	regionsCacheMap[accessKeyID] = regionsCache
 	return describeRegionsOutput, nil
 }
 
@@ -288,7 +277,7 @@ func validateRegion(describeRegionsOutput *ec2.DescribeRegionsOutput, region str
 // NewValidatedClient creates our client wrapper object for the actual AWS clients we use.
 // This should behave the same as NewClient except it will validate the client configuration
 // (eg the region) before returning the client.
-func NewValidatedClient(ctrlRuntimeClient client.Client, secretName, namespace, region string, configManagedClient client.Client) (Client, error) {
+func NewValidatedClient(ctrlRuntimeClient client.Client, secretName, namespace, region string, configManagedClient client.Client, regionsCache map[string]DescribeRegionsData) (Client, error) {
 	s, err := newAWSSession(ctrlRuntimeClient, secretName, namespace, region, configManagedClient)
 	if err != nil {
 		return nil, err
@@ -305,7 +294,8 @@ func NewValidatedClient(ctrlRuntimeClient client.Client, secretName, namespace,
 		switch err.(type) {
 		case endpoints.UnknownEndpointError:
 			klog.Infof("Region %s is not recognized by aws-sdk, trying to validate using API", region)
-			describeRegionsOutput, err := cachedAWSDescribeRegions(s)
+			var describeRegionsOutput *ec2.DescribeRegionsOutput
+			describeRegionsOutput, err = cachedAWSDescribeRegions(s, regionsCache)
 			if err != nil {
 				return nil, fmt.Errorf("could not retrieve region data: %v", err)
 			}