From 1ff7b079e489c15c72632b8c217bee6e8a8590bd Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Wed, 3 Apr 2019 15:16:41 -0700 Subject: [PATCH 1/3] pkg/asset/installconfig/aws: Extract regions from RHCOS metadata Uncomment all of the regions in pkg/types/aws/validation (unwinding that part of bd88157ab0, hack/build: Pin to RHCOS 400.7.20190306.0, 2019-03-12, #1407). Instead, perform the "can we find an AMI for that region?" check directly in pkg/asset/installconfig/aws when we're building a list of regions for the wizard prompt. With this change, bumping the rhcos.json asset (via hack/update-rhcos-bootimage.py) will automatically keep the wizard prompt's choices in sync with the published AMIs. --- pkg/asset/installconfig/aws/aws.go | 9 ++++++++ pkg/rhcos/ami.go | 17 ++++++++++++++ pkg/types/aws/validation/platform.go | 34 ++++++++++++++-------------- 3 files changed, 43 insertions(+), 17 deletions(-) diff --git a/pkg/asset/installconfig/aws/aws.go b/pkg/asset/installconfig/aws/aws.go index 21191627577..64ea7f3feb0 100644 --- a/pkg/asset/installconfig/aws/aws.go +++ b/pkg/asset/installconfig/aws/aws.go @@ -2,6 +2,7 @@ package aws import ( + "context" "fmt" "os" "path/filepath" @@ -12,6 +13,7 @@ import ( "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/rhcos" "github.com/openshift/installer/pkg/types/aws" "github.com/openshift/installer/pkg/types/aws/validation" "github.com/openshift/installer/pkg/version" @@ -25,7 +27,14 @@ import ( func Platform() (*aws.Platform, error) { longRegions := make([]string, 0, len(validation.Regions)) shortRegions := make([]string, 0, len(validation.Regions)) + rhcosRegions, err := rhcos.AMIRegions(context.TODO()) + if err != nil { + return nil, err + } for id, location := range validation.Regions { + if _, ok := rhcosRegions[id]; !ok { + continue + } longRegions = append(longRegions, fmt.Sprintf("%s (%s)", id, location)) shortRegions = append(shortRegions, id) } diff --git a/pkg/rhcos/ami.go b/pkg/rhcos/ami.go index f6d76767b33..f41f7628183 100644 --- a/pkg/rhcos/ami.go +++ b/pkg/rhcos/ami.go @@ -20,3 +20,20 @@ func AMI(ctx context.Context, region string) (string, error) { return ami.HVM, nil } + +// AMIRegions returns a set of AWS regions with HVM AMIs of the Red +// Hat Enterprise Linux CoreOS release. +func AMIRegions(ctx context.Context) (map[string]struct{}, error) { + meta, err := fetchRHCOSBuild(ctx) + if err != nil { + return nil, errors.Wrap(err, "failed to fetch RHCOS metadata") + } + + exists := struct{}{} + regions := make(map[string]struct{}, len(meta.AMIs)) + for region := range meta.AMIs { + regions[region] = exists + } + + return regions, nil +} diff --git a/pkg/types/aws/validation/platform.go b/pkg/types/aws/validation/platform.go index 271263eaba3..318b3349ef1 100644 --- a/pkg/types/aws/validation/platform.go +++ b/pkg/types/aws/validation/platform.go @@ -13,29 +13,29 @@ var ( // the short name of the region. The value of the map is the long // name of the region. Regions = map[string]string{ - //"ap-east-1": "Hong Kong", + "ap-east-1": "Hong Kong", "ap-northeast-1": "Tokyo", "ap-northeast-2": "Seoul", - //"ap-northeast-3": "Osaka-Local", + "ap-northeast-3": "Osaka-Local", "ap-south-1": "Mumbai", "ap-southeast-1": "Singapore", "ap-southeast-2": "Sydney", "ca-central-1": "Central", - //"cn-north-1": "Beijing", - //"cn-northwest-1": "Ningxia", - "eu-central-1": "Frankfurt", - "eu-north-1": "Stockholm", - "eu-west-1": "Ireland", - "eu-west-2": "London", - "eu-west-3": "Paris", - //"me-south-1": "Bahrain", - "sa-east-1": "São Paulo", - "us-east-1": "N. Virginia", - "us-east-2": "Ohio", - //"us-gov-east-1": "AWS GovCloud (US-East)", - //"us-gov-west-1": "AWS GovCloud (US-West)", - "us-west-1": "N. California", - "us-west-2": "Oregon", + "cn-north-1": "Beijing", + "cn-northwest-1": "Ningxia", + "eu-central-1": "Frankfurt", + "eu-north-1": "Stockholm", + "eu-west-1": "Ireland", + "eu-west-2": "London", + "eu-west-3": "Paris", + "me-south-1": "Bahrain", + "sa-east-1": "São Paulo", + "us-east-1": "N. Virginia", + "us-east-2": "Ohio", + "us-gov-east-1": "AWS GovCloud (US-East)", + "us-gov-west-1": "AWS GovCloud (US-West)", + "us-west-1": "N. California", + "us-west-2": "Oregon", } validRegionValues = func() []string { From 6f1a6eef2706ea7c5d7ba2deb6d551c3e8e64058 Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Fri, 11 Oct 2019 15:03:18 -0700 Subject: [PATCH 2/3] pkg/types/aws/validation/platform_test: Convert to regexps So we can ensure we get the error we expect, instead of some other error. --- pkg/types/aws/validation/platform_test.go | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/pkg/types/aws/validation/platform_test.go b/pkg/types/aws/validation/platform_test.go index 957cc38f81e..1de3aac1125 100644 --- a/pkg/types/aws/validation/platform_test.go +++ b/pkg/types/aws/validation/platform_test.go @@ -13,21 +13,20 @@ func TestValidatePlatform(t *testing.T) { cases := []struct { name string platform *aws.Platform - valid bool + expected string }{ { name: "minimal", platform: &aws.Platform{ Region: "us-east-1", }, - valid: true, }, { name: "invalid region", platform: &aws.Platform{ Region: "bad-region", }, - valid: false, + expected: "^test-path.region: Unsupported value: \"bad-region\": supported values: .*\"us-east-1\"", }, { name: "valid machine pool", @@ -35,7 +34,6 @@ func TestValidatePlatform(t *testing.T) { Region: "us-east-1", DefaultMachinePlatform: &aws.MachinePool{}, }, - valid: true, }, { name: "invalid machine pool", @@ -47,16 +45,16 @@ func TestValidatePlatform(t *testing.T) { }, }, }, - valid: false, + expected: "^test-path.defaultMachinePlatform.iops: Invalid value: -10: Storage IOPS must be positive$", }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { err := ValidatePlatform(tc.platform, field.NewPath("test-path")).ToAggregate() - if tc.valid { + if tc.expected == "" { assert.NoError(t, err) } else { - assert.Error(t, err) + assert.Regexp(t, tc.expected, err) } }) } From b8a02dceb96e4fa50cb31de430a5bc6c5ffed971 Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Wed, 3 Apr 2019 16:52:30 -0700 Subject: [PATCH 3/3] pkg/rhcos/ami: List regions on match failure So users can easily see what the alternatives are. Also soften the expected regexp so we don't have to bump it when we get a new RHCOS JSON file with a new region. --- pkg/rhcos/ami.go | 10 +++++++++- pkg/types/aws/validation/platform.go | 7 +++++++ pkg/types/aws/validation/platform_test.go | 14 ++++++++++++++ pkg/types/validation/installconfig_test.go | 2 +- 4 files changed, 31 insertions(+), 2 deletions(-) diff --git a/pkg/rhcos/ami.go b/pkg/rhcos/ami.go index f41f7628183..44e1f399155 100644 --- a/pkg/rhcos/ami.go +++ b/pkg/rhcos/ami.go @@ -2,6 +2,8 @@ package rhcos import ( "context" + "sort" + "strings" "github.com/pkg/errors" ) @@ -15,7 +17,13 @@ func AMI(ctx context.Context, region string) (string, error) { ami, ok := meta.AMIs[region] if !ok { - return "", errors.Errorf("no RHCOS AMIs found in %s", region) + regions := make([]string, 0, len(meta.AMIs)) + for rgn := range meta.AMIs { + regions = append(regions, rgn) + } + sort.Strings(regions) + + return "", errors.Errorf("no RHCOS AMIs found in %q (%s)", region, strings.Join(regions, ", ")) } return ami.HVM, nil diff --git a/pkg/types/aws/validation/platform.go b/pkg/types/aws/validation/platform.go index 318b3349ef1..dba0ed16978 100644 --- a/pkg/types/aws/validation/platform.go +++ b/pkg/types/aws/validation/platform.go @@ -1,10 +1,12 @@ package validation import ( + "context" "sort" "k8s.io/apimachinery/pkg/util/validation/field" + "github.com/openshift/installer/pkg/rhcos" "github.com/openshift/installer/pkg/types/aws" ) @@ -55,6 +57,11 @@ func ValidatePlatform(p *aws.Platform, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} if _, ok := Regions[p.Region]; !ok { allErrs = append(allErrs, field.NotSupported(fldPath.Child("region"), p.Region, validRegionValues)) + } else if p.AMIID == "" { + _, err := rhcos.AMI(context.TODO(), p.Region) + if err != nil { + allErrs = append(allErrs, field.Invalid(fldPath.Child("region"), p.Region, err.Error())) + } } if p.DefaultMachinePlatform != nil { allErrs = append(allErrs, ValidateMachinePool(p, p.DefaultMachinePlatform, fldPath.Child("defaultMachinePlatform"))...) diff --git a/pkg/types/aws/validation/platform_test.go b/pkg/types/aws/validation/platform_test.go index 1de3aac1125..3d4019d4792 100644 --- a/pkg/types/aws/validation/platform_test.go +++ b/pkg/types/aws/validation/platform_test.go @@ -28,6 +28,20 @@ func TestValidatePlatform(t *testing.T) { }, expected: "^test-path.region: Unsupported value: \"bad-region\": supported values: .*\"us-east-1\"", }, + { + name: "valid region without RHCOS", + platform: &aws.Platform{ + Region: "us-gov-west-1", + }, + expected: "^test-path.region: Invalid value: \"us-gov-west-1\": no RHCOS AMIs found in \"us-gov-west-1\" (.*us-east-1.*)$", + }, + { + name: "valid region without RHCOS, but with an explicit AMI", + platform: &aws.Platform{ + AMIID: "ami-123", + Region: "us-gov-west-1", + }, + }, { name: "valid machine pool", platform: &aws.Platform{ diff --git a/pkg/types/validation/installconfig_test.go b/pkg/types/validation/installconfig_test.go index 09d1170fab9..f3f7523073f 100644 --- a/pkg/types/validation/installconfig_test.go +++ b/pkg/types/validation/installconfig_test.go @@ -407,7 +407,7 @@ func TestValidateInstallConfig(t *testing.T) { } return c }(), - expectedError: `^platform\.aws\.region: Unsupported value: "": supported values: "ap-northeast-1", "ap-northeast-2", "ap-south-1", "ap-southeast-1", "ap-southeast-2", "ca-central-1", "eu-central-1", "eu-north-1", "eu-west-1", "eu-west-2", "eu-west-3", "sa-east-1", "us-east-1", "us-east-2", "us-west-1", "us-west-2"$`, + expectedError: `^platform\.aws\.region: Unsupported value: "": supported values: .*"us-east-1"`, }, { name: "valid libvirt platform",