From 49ff49b9151eb286ed370efdc33a84a41fa08f73 Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Sat, 12 Oct 2019 06:09:48 -0700 Subject: [PATCH 1/5] pkg/asset/installconfig/aws/platform: Put Platform in its own file "aws.go" doesn't add any information that is not covered in the package name. Putting Platform in its own platform.go file will make it easier to find for folks tab-completing into the directory in search of Platform. --- pkg/asset/installconfig/aws/aws.go | 70 ---------------------- pkg/asset/installconfig/aws/platform.go | 78 +++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 70 deletions(-) create mode 100644 pkg/asset/installconfig/aws/platform.go diff --git a/pkg/asset/installconfig/aws/aws.go b/pkg/asset/installconfig/aws/aws.go index 21191627577..824e662149e 100644 --- a/pkg/asset/installconfig/aws/aws.go +++ b/pkg/asset/installconfig/aws/aws.go @@ -2,18 +2,13 @@ package aws import ( - "fmt" "os" "path/filepath" - "sort" - "strings" "github.com/aws/aws-sdk-go/aws/credentials" "github.com/aws/aws-sdk-go/aws/defaults" "github.com/aws/aws-sdk-go/aws/request" "github.com/aws/aws-sdk-go/aws/session" - "github.com/openshift/installer/pkg/types/aws" - "github.com/openshift/installer/pkg/types/aws/validation" "github.com/openshift/installer/pkg/version" "github.com/pkg/errors" "github.com/sirupsen/logrus" @@ -21,71 +16,6 @@ import ( ini "gopkg.in/ini.v1" ) -// Platform collects AWS-specific configuration. -func Platform() (*aws.Platform, error) { - longRegions := make([]string, 0, len(validation.Regions)) - shortRegions := make([]string, 0, len(validation.Regions)) - for id, location := range validation.Regions { - longRegions = append(longRegions, fmt.Sprintf("%s (%s)", id, location)) - shortRegions = append(shortRegions, id) - } - regionTransform := survey.TransformString(func(s string) string { - return strings.SplitN(s, " ", 2)[0] - }) - - defaultRegion := "us-east-1" - _, ok := validation.Regions[defaultRegion] - if !ok { - panic(fmt.Sprintf("installer bug: invalid default AWS region %q", defaultRegion)) - } - - ssn, err := GetSession() - if err != nil { - return nil, err - } - - defaultRegionPointer := ssn.Config.Region - if defaultRegionPointer != nil && *defaultRegionPointer != "" { - _, ok := validation.Regions[*defaultRegionPointer] - if ok { - defaultRegion = *defaultRegionPointer - } else { - logrus.Warnf("Unrecognized AWS region %q, defaulting to %s", *defaultRegionPointer, defaultRegion) - } - } - - sort.Strings(longRegions) - sort.Strings(shortRegions) - - var region string - err = survey.Ask([]*survey.Question{ - { - Prompt: &survey.Select{ - Message: "Region", - Help: "The AWS region to be used for installation.", - Default: fmt.Sprintf("%s (%s)", defaultRegion, validation.Regions[defaultRegion]), - Options: longRegions, - }, - Validate: survey.ComposeValidators(survey.Required, func(ans interface{}) error { - choice := regionTransform(ans).(string) - i := sort.SearchStrings(shortRegions, choice) - if i == len(shortRegions) || shortRegions[i] != choice { - return errors.Errorf("invalid region %q", choice) - } - return nil - }), - Transform: regionTransform, - }, - }, ®ion) - if err != nil { - return nil, err - } - - return &aws.Platform{ - Region: region, - }, nil -} - // GetSession returns an AWS session by checking credentials // and, if no creds are found, asks for them and stores them on disk in a config file func GetSession() (*session.Session, error) { diff --git a/pkg/asset/installconfig/aws/platform.go b/pkg/asset/installconfig/aws/platform.go new file mode 100644 index 00000000000..adde4835137 --- /dev/null +++ b/pkg/asset/installconfig/aws/platform.go @@ -0,0 +1,78 @@ +package aws + +import ( + "fmt" + "sort" + "strings" + + "github.com/openshift/installer/pkg/types/aws" + "github.com/openshift/installer/pkg/types/aws/validation" + "github.com/pkg/errors" + "github.com/sirupsen/logrus" + survey "gopkg.in/AlecAivazis/survey.v1" +) + +// Platform collects AWS-specific configuration. +func Platform() (*aws.Platform, error) { + longRegions := make([]string, 0, len(validation.Regions)) + shortRegions := make([]string, 0, len(validation.Regions)) + for id, location := range validation.Regions { + longRegions = append(longRegions, fmt.Sprintf("%s (%s)", id, location)) + shortRegions = append(shortRegions, id) + } + regionTransform := survey.TransformString(func(s string) string { + return strings.SplitN(s, " ", 2)[0] + }) + + defaultRegion := "us-east-1" + _, ok := validation.Regions[defaultRegion] + if !ok { + panic(fmt.Sprintf("installer bug: invalid default AWS region %q", defaultRegion)) + } + + ssn, err := GetSession() + if err != nil { + return nil, err + } + + defaultRegionPointer := ssn.Config.Region + if defaultRegionPointer != nil && *defaultRegionPointer != "" { + _, ok := validation.Regions[*defaultRegionPointer] + if ok { + defaultRegion = *defaultRegionPointer + } else { + logrus.Warnf("Unrecognized AWS region %q, defaulting to %s", *defaultRegionPointer, defaultRegion) + } + } + + sort.Strings(longRegions) + sort.Strings(shortRegions) + + var region string + err = survey.Ask([]*survey.Question{ + { + Prompt: &survey.Select{ + Message: "Region", + Help: "The AWS region to be used for installation.", + Default: fmt.Sprintf("%s (%s)", defaultRegion, validation.Regions[defaultRegion]), + Options: longRegions, + }, + Validate: survey.ComposeValidators(survey.Required, func(ans interface{}) error { + choice := regionTransform(ans).(string) + i := sort.SearchStrings(shortRegions, choice) + if i == len(shortRegions) || shortRegions[i] != choice { + return errors.Errorf("invalid region %q", choice) + } + return nil + }), + Transform: regionTransform, + }, + }, ®ion) + if err != nil { + return nil, err + } + + return &aws.Platform{ + Region: region, + }, nil +} From d9b87db23cdddf5eb5fcadb3ef6da7fde8ba3708 Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Sat, 12 Oct 2019 06:12:49 -0700 Subject: [PATCH 2/5] pkg/asset/installconfig/aws/session: Put GetSession in its own file "aws.go" doesn't add any information that is not covered in the package name. Putting session handling in its own session.go file will make it easier to find for folks tab-completing into the directory in search of GetSession and friends. --- pkg/asset/installconfig/aws/doc.go | 2 ++ pkg/asset/installconfig/aws/{aws.go => session.go} | 1 - 2 files changed, 2 insertions(+), 1 deletion(-) create mode 100644 pkg/asset/installconfig/aws/doc.go rename pkg/asset/installconfig/aws/{aws.go => session.go} (98%) diff --git a/pkg/asset/installconfig/aws/doc.go b/pkg/asset/installconfig/aws/doc.go new file mode 100644 index 00000000000..b0a2e796292 --- /dev/null +++ b/pkg/asset/installconfig/aws/doc.go @@ -0,0 +1,2 @@ +// Package aws collects AWS-specific configuration. +package aws diff --git a/pkg/asset/installconfig/aws/aws.go b/pkg/asset/installconfig/aws/session.go similarity index 98% rename from pkg/asset/installconfig/aws/aws.go rename to pkg/asset/installconfig/aws/session.go index 824e662149e..349370aaa0d 100644 --- a/pkg/asset/installconfig/aws/aws.go +++ b/pkg/asset/installconfig/aws/session.go @@ -1,4 +1,3 @@ -// Package aws collects AWS-specific configuration. package aws import ( From e08f50d12f2ffc80e6689b98c2ae5675b3f7df9a Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Sat, 12 Oct 2019 06:46:39 -0700 Subject: [PATCH 3/5] pkg/asset/installconfig/installconfig: Add InstallConfig.finish Consolidating finalizing logic which is shared between Generate and Load. This will make it easier to add additional shared logic going forward, because you'll only have to add it to one place. Also inline setDefaults and convert instead of using separate methods for those one-liners. --- pkg/asset/installconfig/installconfig.go | 49 ++++++++---------------- 1 file changed, 15 insertions(+), 34 deletions(-) diff --git a/pkg/asset/installconfig/installconfig.go b/pkg/asset/installconfig/installconfig.go index 2c8f3cfc25a..b9e5f98425d 100644 --- a/pkg/asset/installconfig/installconfig.go +++ b/pkg/asset/installconfig/installconfig.go @@ -75,23 +75,7 @@ func (a *InstallConfig) Generate(parents asset.Parents) error { a.Config.GCP = platform.GCP a.Config.BareMetal = platform.BareMetal - if err := a.setDefaults(); err != nil { - return errors.Wrap(err, "failed to set defaults for install config") - } - - if err := validation.ValidateInstallConfig(a.Config, openstackvalidation.NewValidValuesFetcher()).ToAggregate(); err != nil { - return errors.Wrap(err, "invalid install config") - } - - data, err := yaml.Marshal(a.Config) - if err != nil { - return errors.Wrap(err, "failed to Marshal InstallConfig") - } - a.File = &asset.File{ - Filename: installConfigFilename, - Data: data, - } - return nil + return a.finish("") } // Name returns the human-friendly name of the asset. @@ -124,37 +108,34 @@ func (a *InstallConfig) Load(f asset.FileFetcher) (found bool, err error) { a.Config = config // Upconvert any deprecated fields - if err := a.convert(); err != nil { + if err := conversion.ConvertInstallConfig(a.Config); err != nil { return false, errors.Wrap(err, "failed to upconvert install config") } - if err := a.setDefaults(); err != nil { - return false, errors.Wrap(err, "failed to set defaults for install config") + err = a.finish(installConfigFilename) + if err != nil { + return false, err } + return true, nil +} + +func (a *InstallConfig) finish(filename string) error { + defaults.SetInstallConfigDefaults(a.Config) if err := validation.ValidateInstallConfig(a.Config, openstackvalidation.NewValidValuesFetcher()).ToAggregate(); err != nil { - return false, errors.Wrapf(err, "invalid %q file", installConfigFilename) + if filename == "" { + return errors.Wrap(err, "invalid install config") + } + return errors.Wrapf(err, "invalid %q file", filename) } data, err := yaml.Marshal(a.Config) if err != nil { - return false, errors.Wrap(err, "failed to Marshal InstallConfig") + return errors.Wrap(err, "failed to Marshal InstallConfig") } a.File = &asset.File{ Filename: installConfigFilename, Data: data, } - - return true, nil -} - -func (a *InstallConfig) setDefaults() error { - defaults.SetInstallConfigDefaults(a.Config) return nil } - -// convert converts possibly older versions of the install config to -// the current version, relocating deprecated fields. -func (a *InstallConfig) convert() error { - return conversion.ConvertInstallConfig(a.Config) -} From 68994b7b7c9541c25b394ac825593cf4f91fd288 Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Sat, 12 Oct 2019 06:34:18 -0700 Subject: [PATCH 4/5] pkg/asset/installconfig/aws/metadata: Store AWS metadata This allows us to hold the AWS session (and in the future, other metadata) on the InstallConfig object, where we can load it once in InstallConfig.Generate or InstallConfig.Load, instead of having to call GetSession or other loaders in all consumers. At the moment, I'm just stubbing in the framework; I'll port our other GetSession consumers in future commits. I'm using private properties with getters so we don't have to pay the price of initializing properties we don't need to use. Consumers can call the getter, which will pull fresh data into the caching property the first time, and return the previously cached data on subsequent calls. --- pkg/asset/installconfig/aws/metadata.go | 35 +++++++++++++++++++ pkg/asset/installconfig/installconfig.go | 4 +++ pkg/asset/installconfig/platformcredscheck.go | 7 ++-- 3 files changed, 43 insertions(+), 3 deletions(-) create mode 100644 pkg/asset/installconfig/aws/metadata.go diff --git a/pkg/asset/installconfig/aws/metadata.go b/pkg/asset/installconfig/aws/metadata.go new file mode 100644 index 00000000000..bd14ffedc8a --- /dev/null +++ b/pkg/asset/installconfig/aws/metadata.go @@ -0,0 +1,35 @@ +package aws + +import ( + "context" + "sync" + + "github.com/aws/aws-sdk-go/aws/session" + "github.com/pkg/errors" +) + +// Metadata holds additional metadata for InstallConfig resources that +// does not need to be user-supplied (e.g. because it can be retrieved +// from external APIs). +type Metadata struct { + session *session.Session + mutex sync.Mutex +} + +// Session holds an AWS session which can be used for AWS API calls +// during asset generation. +// GetMetadata loads metadata. +func (m *Metadata) Session(ctx context.Context) (*session.Session, error) { + m.mutex.Lock() + defer m.mutex.Unlock() + + if m.session == nil { + var err error + m.session, err = GetSession() + if err != nil { + return nil, errors.Wrap(err, "creating AWS session") + } + } + + return m.session, nil +} diff --git a/pkg/asset/installconfig/installconfig.go b/pkg/asset/installconfig/installconfig.go index b9e5f98425d..04541969d80 100644 --- a/pkg/asset/installconfig/installconfig.go +++ b/pkg/asset/installconfig/installconfig.go @@ -8,6 +8,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "github.com/openshift/installer/pkg/asset" + "github.com/openshift/installer/pkg/asset/installconfig/aws" "github.com/openshift/installer/pkg/types" "github.com/openshift/installer/pkg/types/conversion" "github.com/openshift/installer/pkg/types/defaults" @@ -23,6 +24,7 @@ const ( type InstallConfig struct { Config *types.InstallConfig `json:"config"` File *asset.File `json:"file"` + AWS *aws.Metadata } var _ asset.WritableAsset = (*InstallConfig)(nil) @@ -122,6 +124,8 @@ func (a *InstallConfig) Load(f asset.FileFetcher) (found bool, err error) { func (a *InstallConfig) finish(filename string) error { defaults.SetInstallConfigDefaults(a.Config) + a.AWS = &aws.Metadata{} + if err := validation.ValidateInstallConfig(a.Config, openstackvalidation.NewValidValuesFetcher()).ToAggregate(); err != nil { if filename == "" { return errors.Wrap(err, "invalid install config") diff --git a/pkg/asset/installconfig/platformcredscheck.go b/pkg/asset/installconfig/platformcredscheck.go index 62b3e58d772..25808e95c25 100644 --- a/pkg/asset/installconfig/platformcredscheck.go +++ b/pkg/asset/installconfig/platformcredscheck.go @@ -36,6 +36,7 @@ func (a *PlatformCredsCheck) Dependencies() []asset.Asset { // Generate queries for input from the user. func (a *PlatformCredsCheck) Generate(dependencies asset.Parents) error { + ctx := context.TODO() ic := &InstallConfig{} dependencies.Get(ic) @@ -43,16 +44,16 @@ func (a *PlatformCredsCheck) Generate(dependencies asset.Parents) error { platform := ic.Config.Platform.Name() switch platform { case aws.Name: - ssn, err := awsconfig.GetSession() + ssn, err := ic.AWS.Session(ctx) if err != nil { - return errors.Wrap(err, "creating AWS session") + return err } err = awsconfig.ValidateCreds(ssn) if err != nil { return errors.Wrap(err, "validate AWS credentials") } case gcp.Name: - _, err = gcpconfig.GetSession(context.TODO()) + _, err = gcpconfig.GetSession(ctx) if err != nil { return errors.Wrap(err, "creating GCP session") } From 1b4c1bfa2b6a488e99a17face6ba8e7aaaa134d7 Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Sat, 12 Oct 2019 07:13:14 -0700 Subject: [PATCH 5/5] pkg/asset/installconfig/aws/availabilityzones: Add zones to Metadata This gives us caching so we no longer pull zones multiple times. Previously, we'd pull zones for each pool that did not configure zones. It also moves us from a per-zone-request GetSession calls to the new Metadata.Session() cache, which saves repetition there as well. --- .../installconfig/aws/availabilityzones.go | 41 ++++++++++++++ pkg/asset/installconfig/aws/metadata.go | 36 +++++++++++- pkg/asset/installconfig/installconfig.go | 4 +- pkg/asset/machines/aws/zones.go | 56 ------------------- pkg/asset/machines/master.go | 6 +- pkg/asset/machines/worker.go | 6 +- 6 files changed, 83 insertions(+), 66 deletions(-) create mode 100644 pkg/asset/installconfig/aws/availabilityzones.go delete mode 100644 pkg/asset/machines/aws/zones.go diff --git a/pkg/asset/installconfig/aws/availabilityzones.go b/pkg/asset/installconfig/aws/availabilityzones.go new file mode 100644 index 00000000000..31cbfdb9b57 --- /dev/null +++ b/pkg/asset/installconfig/aws/availabilityzones.go @@ -0,0 +1,41 @@ +package aws + +import ( + "context" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/aws/session" + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/pkg/errors" +) + +// availabilityZones retrieves a list of availability zones for the given region. +func availabilityZones(ctx context.Context, session *session.Session, region string) ([]string, error) { + client := ec2.New(session, aws.NewConfig().WithRegion(region)) + resp, err := client.DescribeAvailabilityZonesWithContext(ctx, &ec2.DescribeAvailabilityZonesInput{ + Filters: []*ec2.Filter{ + { + Name: aws.String("region-name"), + Values: []*string{aws.String(region)}, + }, + { + Name: aws.String("state"), + Values: []*string{aws.String("available")}, + }, + }, + }) + if err != nil { + return nil, errors.Wrap(err, "fetching availability zones") + } + + zones := []string{} + for _, zone := range resp.AvailabilityZones { + zones = append(zones, *zone.ZoneName) + } + + if len(zones) == 0 { + return nil, errors.Errorf("no available zones in %s", region) + } + + return zones, nil +} diff --git a/pkg/asset/installconfig/aws/metadata.go b/pkg/asset/installconfig/aws/metadata.go index bd14ffedc8a..1e273e144e2 100644 --- a/pkg/asset/installconfig/aws/metadata.go +++ b/pkg/asset/installconfig/aws/metadata.go @@ -12,17 +12,27 @@ import ( // does not need to be user-supplied (e.g. because it can be retrieved // from external APIs). type Metadata struct { - session *session.Session - mutex sync.Mutex + session *session.Session + availabilityZones []string + region string + mutex sync.Mutex +} + +// NewMetadata initializes a new Metadata object. +func NewMetadata(region string) *Metadata { + return &Metadata{region: region} } // Session holds an AWS session which can be used for AWS API calls // during asset generation. -// GetMetadata loads metadata. func (m *Metadata) Session(ctx context.Context) (*session.Session, error) { m.mutex.Lock() defer m.mutex.Unlock() + return m.unlockedSession(ctx) +} + +func (m *Metadata) unlockedSession(ctx context.Context) (*session.Session, error) { if m.session == nil { var err error m.session, err = GetSession() @@ -33,3 +43,23 @@ func (m *Metadata) Session(ctx context.Context) (*session.Session, error) { return m.session, nil } + +// AvailabilityZones retrieves a list of availability zones for the configured region. +func (m *Metadata) AvailabilityZones(ctx context.Context) ([]string, error) { + m.mutex.Lock() + defer m.mutex.Unlock() + + if len(m.availabilityZones) == 0 { + session, err := m.unlockedSession(ctx) + 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 m.availabilityZones, nil +} diff --git a/pkg/asset/installconfig/installconfig.go b/pkg/asset/installconfig/installconfig.go index 04541969d80..e5167602095 100644 --- a/pkg/asset/installconfig/installconfig.go +++ b/pkg/asset/installconfig/installconfig.go @@ -124,7 +124,9 @@ func (a *InstallConfig) Load(f asset.FileFetcher) (found bool, err error) { func (a *InstallConfig) finish(filename string) error { defaults.SetInstallConfigDefaults(a.Config) - a.AWS = &aws.Metadata{} + if a.Config.AWS != nil { + a.AWS = aws.NewMetadata(a.Config.Platform.AWS.Region) + } if err := validation.ValidateInstallConfig(a.Config, openstackvalidation.NewValidValuesFetcher()).ToAggregate(); err != nil { if filename == "" { diff --git a/pkg/asset/machines/aws/zones.go b/pkg/asset/machines/aws/zones.go deleted file mode 100644 index a520ce6bde4..00000000000 --- a/pkg/asset/machines/aws/zones.go +++ /dev/null @@ -1,56 +0,0 @@ -package aws - -import ( - "fmt" - - "github.com/aws/aws-sdk-go/aws" - "github.com/aws/aws-sdk-go/service/ec2" - awsutil "github.com/openshift/installer/pkg/asset/installconfig/aws" -) - -// AvailabilityZones retrieves a list of availability zones for the given region. -func AvailabilityZones(region string) ([]string, error) { - ec2Client, err := ec2Client(region) - if err != nil { - return nil, err - } - zones, err := fetchAvailabilityZones(ec2Client, region) - if err != nil { - return nil, fmt.Errorf("cannot fetch availability zones: %v", err) - } - return zones, nil -} - -func ec2Client(region string) (*ec2.EC2, error) { - ssn, err := awsutil.GetSession() - if err != nil { - return nil, err - } - - client := ec2.New(ssn, aws.NewConfig().WithRegion(region)) - return client, nil -} - -func fetchAvailabilityZones(client *ec2.EC2, region string) ([]string, error) { - req := &ec2.DescribeAvailabilityZonesInput{ - Filters: []*ec2.Filter{ - { - Name: aws.String("region-name"), - Values: []*string{aws.String(region)}, - }, - { - Name: aws.String("state"), - Values: []*string{aws.String("available")}, - }, - }, - } - resp, err := client.DescribeAvailabilityZones(req) - if err != nil { - return nil, err - } - zones := []string{} - for _, zone := range resp.AvailabilityZones { - zones = append(zones, *zone.ZoneName) - } - return zones, nil -} diff --git a/pkg/asset/machines/master.go b/pkg/asset/machines/master.go index 53d3b731551..14ac2723547 100644 --- a/pkg/asset/machines/master.go +++ b/pkg/asset/machines/master.go @@ -1,6 +1,7 @@ package machines import ( + "context" "fmt" "os" "path/filepath" @@ -138,11 +139,10 @@ func (m *Master) Generate(dependencies asset.Parents) error { mpool.Set(ic.Platform.AWS.DefaultMachinePlatform) mpool.Set(pool.Platform.AWS) if len(mpool.Zones) == 0 { - azs, err := aws.AvailabilityZones(ic.Platform.AWS.Region) + mpool.Zones, err = installconfig.AWS.AvailabilityZones(context.TODO()) if err != nil { - return errors.Wrap(err, "failed to fetch availability zones") + return err } - mpool.Zones = azs } pool.Platform.AWS = &mpool machines, err = aws.Machines(clusterID.InfraID, ic, pool, string(*rhcosImage), "master", "master-user-data") diff --git a/pkg/asset/machines/worker.go b/pkg/asset/machines/worker.go index 6344f791397..0ecd941e7dd 100644 --- a/pkg/asset/machines/worker.go +++ b/pkg/asset/machines/worker.go @@ -1,6 +1,7 @@ package machines import ( + "context" "fmt" "os" "path/filepath" @@ -158,11 +159,10 @@ func (w *Worker) Generate(dependencies asset.Parents) error { mpool.Set(ic.Platform.AWS.DefaultMachinePlatform) mpool.Set(pool.Platform.AWS) if len(mpool.Zones) == 0 { - azs, err := aws.AvailabilityZones(ic.Platform.AWS.Region) + mpool.Zones, err = installconfig.AWS.AvailabilityZones(context.TODO()) if err != nil { - return errors.Wrap(err, "failed to fetch availability zones") + return err } - mpool.Zones = azs } pool.Platform.AWS = &mpool sets, err := aws.MachineSets(clusterID.InfraID, ic, &pool, string(*rhcosImage), "worker", "worker-user-data")