Fix and improve locking session and AWS Metadata access

This fix eliminates the need for mutexSubnets to update subnet
information within AWS metadata. It also updates populateSubnets
to take care of getting VPC and subnets once for the installation
eliminating duplicate code that could be prone to errors.

Co-authored-by: Rafael F. <[email protected]>

Author: sadasu

pkg/asset/installconfig/aws/metadata.go

@@ -26,8 +26,7 @@ type Metadata struct {
 	Subnets  []string                   `json:"subnets,omitempty"`
 	Services []typesaws.ServiceEndpoint `json:"services,omitempty"`
 
-	mutex        sync.Mutex
-	mutexSubnets sync.Mutex
+	mutex sync.Mutex
 }
 
 // NewMetadata initializes a new Metadata object.
@@ -66,10 +65,9 @@ func (m *Metadata) AvailabilityZones(ctx context.Context) ([]string, error) {
 		if err != nil {
 			return nil, err
 		}
-
 		m.availabilityZones, err = availabilityZones(ctx, session, m.Region)
 		if err != nil {
-			return nil, errors.Wrap(err, "creating AWS session")
+			return nil, errors.Wrap(err, "error retrieving Availability Zones")
 		}
 	}
 
@@ -82,9 +80,8 @@ func (m *Metadata) AvailabilityZones(ctx context.Context) ([]string, error) {
 func (m *Metadata) EdgeSubnets(ctx context.Context) (map[string]Subnet, error) {
 	err := m.populateSubnets(ctx)
 	if err != nil {
-		return nil, err
+		return nil, errors.Wrap(err, "error retrieving Edge Subnets")
 	}
-
 	return m.edgeSubnets, nil
 }
 
@@ -94,9 +91,8 @@ func (m *Metadata) EdgeSubnets(ctx context.Context) (map[string]Subnet, error) {
 func (m *Metadata) PrivateSubnets(ctx context.Context) (map[string]Subnet, error) {
 	err := m.populateSubnets(ctx)
 	if err != nil {
-		return nil, err
+		return nil, errors.Wrap(err, "error retrieving Private Subnets")
 	}
-
 	return m.privateSubnets, nil
 }
 
@@ -106,25 +102,34 @@ func (m *Metadata) PrivateSubnets(ctx context.Context) (map[string]Subnet, error
 func (m *Metadata) PublicSubnets(ctx context.Context) (map[string]Subnet, error) {
 	err := m.populateSubnets(ctx)
 	if err != nil {
-		return nil, err
+		return nil, errors.Wrap(err, "error retrieving Public Subnets")
 	}
-
 	return m.publicSubnets, nil
 }
 
-func (m *Metadata) populateSubnets(ctx context.Context) error {
-	if len(m.publicSubnets) > 0 || len(m.privateSubnets) > 0 {
-		return nil
+// VPC retrieves the VPC ID containing PublicSubnets and PrivateSubnets.
+func (m *Metadata) VPC(ctx context.Context) (string, error) {
+	err := m.populateSubnets(ctx)
+	if err != nil {
+		return "", errors.Wrap(err, "error retrieving VPC")
 	}
+	return m.vpc, nil
+}
+
+func (m *Metadata) populateSubnets(ctx context.Context) error {
+	m.mutex.Lock()
+	defer m.mutex.Unlock()
 
 	if len(m.Subnets) == 0 {
 		return errors.New("no subnets configured")
 	}
 
-	m.mutexSubnets.Lock()
-	defer m.mutexSubnets.Unlock()
+	if m.vpc != "" || len(m.privateSubnets) > 0 || len(m.publicSubnets) > 0 || len(m.edgeSubnets) > 0 {
+		// Call to populate subnets has already happened
+		return nil
+	}
 
-	session, err := m.Session(ctx)
+	session, err := m.unlockedSession(ctx)
 	if err != nil {
 		return err
 	}
@@ -137,25 +142,6 @@ func (m *Metadata) populateSubnets(ctx context.Context) error {
 	return err
 }
 
-// VPC retrieves the VPC ID containing PublicSubnets and PrivateSubnets.
-func (m *Metadata) VPC(ctx context.Context) (string, error) {
-	m.mutex.Lock()
-	defer m.mutex.Unlock()
-
-	if m.vpc == "" {
-		if len(m.Subnets) == 0 {
-			return "", errors.New("cannot calculate VPC without configured subnets")
-		}
-
-		err := m.populateSubnets(ctx)
-		if err != nil {
-			return "", err
-		}
-	}
-
-	return m.vpc, nil
-}
-
 // InstanceTypes retrieves instance type metadata indexed by InstanceType for the configured region.
 func (m *Metadata) InstanceTypes(ctx context.Context) (map[string]InstanceType, error) {
 	m.mutex.Lock()
@@ -169,7 +155,7 @@ func (m *Metadata) InstanceTypes(ctx context.Context) (map[string]InstanceType,
 
 		m.instanceTypes, err = instanceTypes(ctx, session, m.Region)
 		if err != nil {
-			return nil, errors.Wrap(err, "listing instance types")
+			return nil, errors.Wrap(err, "error listing instance types")
 		}
 	}
 

pkg/asset/installconfig/aws/validation_test.go

@@ -540,7 +540,7 @@ func TestValidate(t *testing.T) {
 		privateSubnets: map[string]Subnet{},
 		publicSubnets:  map[string]Subnet{},
 		edgeSubnets:    validEdgeSubnets(),
-		expectErr:      `^platform\.aws\.subnets: Invalid value: \[\]string{\"valid-public-subnet-edge-a\", \"valid-public-subnet-edge-b\", \"valid-public-subnet-edge-c\"}: edge pool. no subnets configured$`,
+		expectErr:      `^\[platform\.aws\.subnets: Invalid value: \[\]string{\"valid-public-subnet-edge-a\", \"valid-public-subnet-edge-b\", \"valid-public-subnet-edge-c\"}: No private subnets found, controlPlane\.platform\.aws\.zones: Invalid value: \[\]string{\"a\", \"b\", \"c\"}: No subnets provided for zones \[a b c\], compute\[0\]\.platform\.aws\.zones: Invalid value: \[\]string{\"a\", \"b\", \"c\"}: No subnets provided for zones \[a b c\]]$`,
 	}, {
 		name: "invalid no subnet for control plane zones",
 		installConfig: func() *types.InstallConfig {
@@ -749,6 +749,7 @@ func TestValidate(t *testing.T) {
 				publicSubnets:     test.publicSubnets,
 				edgeSubnets:       test.edgeSubnets,
 				instanceTypes:     test.instanceTypes,
+				Subnets:           test.installConfig.Platform.AWS.Subnets,
 			}
 			if test.proxy != "" {
 				os.Setenv("HTTP_PROXY", test.proxy)
@@ -881,6 +882,7 @@ func TestValidateForProvisioning(t *testing.T) {
 				instanceTypes:     validInstanceTypes(),
 				Region:            editedInstallConfig.AWS.Region,
 				vpc:               "valid-private-subnet-a",
+				Subnets:           editedInstallConfig.Platform.AWS.Subnets,
 			}
 
 			err := ValidateForProvisioning(route53Client, editedInstallConfig, meta)