From c5786021f08cd6dfb8a043d5c20adfa14ca1f60d Mon Sep 17 00:00:00 2001 From: Sandhya Dasu Date: Wed, 5 Apr 2023 12:42:59 -0400 Subject: [PATCH] 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. --- pkg/asset/installconfig/aws/metadata.go | 58 +++++++------------ .../installconfig/aws/validation_test.go | 4 +- 2 files changed, 25 insertions(+), 37 deletions(-) diff --git a/pkg/asset/installconfig/aws/metadata.go b/pkg/asset/installconfig/aws/metadata.go index f0828b3008d..296ec49d912 100644 --- a/pkg/asset/installconfig/aws/metadata.go +++ b/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") } } diff --git a/pkg/asset/installconfig/aws/validation_test.go b/pkg/asset/installconfig/aws/validation_test.go index 808598b1d0d..f26a4645538 100644 --- a/pkg/asset/installconfig/aws/validation_test.go +++ b/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)