From 3af7a72719373d97c3846b0f5897f6e2ef785849 Mon Sep 17 00:00:00 2001 From: Mike Crute Date: Mon, 1 Apr 2019 15:49:57 -0700 Subject: [PATCH] Remove hardcoded region list from AWS provider This extracts the region list from the AWS SDK and accounts for special opt-in regions. This will ensure that the regions are always up-to-date as we update the AWS SDK instead of requiring duplicated accounting. --- .../k8s.io/legacy-cloud-providers/aws/BUILD | 2 - .../k8s.io/legacy-cloud-providers/aws/aws.go | 50 +++++++--- .../legacy-cloud-providers/aws/aws_test.go | 41 ++++++++ .../legacy-cloud-providers/aws/regions.go | 96 ------------------- .../aws/regions_test.go | 85 ---------------- 5 files changed, 76 insertions(+), 198 deletions(-) delete mode 100644 staging/src/k8s.io/legacy-cloud-providers/aws/regions.go delete mode 100644 staging/src/k8s.io/legacy-cloud-providers/aws/regions_test.go diff --git a/staging/src/k8s.io/legacy-cloud-providers/aws/BUILD b/staging/src/k8s.io/legacy-cloud-providers/aws/BUILD index 7805a85b49..127b74bc54 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/aws/BUILD +++ b/staging/src/k8s.io/legacy-cloud-providers/aws/BUILD @@ -19,7 +19,6 @@ go_library( "device_allocator.go", "instances.go", "log_handler.go", - "regions.go", "retry_handler.go", "sets_ippermissions.go", "tags.go", @@ -74,7 +73,6 @@ go_test( "aws_test.go", "device_allocator_test.go", "instances_test.go", - "regions_test.go", "retry_handler_test.go", "tags_test.go", ], diff --git a/staging/src/k8s.io/legacy-cloud-providers/aws/aws.go b/staging/src/k8s.io/legacy-cloud-providers/aws/aws.go index 8417e8e5bb..f7c0554589 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/aws/aws.go +++ b/staging/src/k8s.io/legacy-cloud-providers/aws/aws.go @@ -270,9 +270,6 @@ const MaxReadThenCreateRetries = 30 // need hardcoded defaults. const DefaultVolumeType = "gp2" -// Used to call recognizeWellKnownRegions just once -var once sync.Once - // Services is an abstraction over AWS, to allow mocking/other implementations type Services interface { Compute(region string) (EC2, error) @@ -1214,14 +1211,8 @@ func newAWSCloud(cfg CloudConfig, awsServices Services) (*Cloud, error) { return nil, err } - // Trust that if we get a region from configuration or AWS metadata that it is valid, - // and register ECR providers - recognizeRegion(regionName) - if !cfg.Global.DisableStrictZoneCheck { - valid := isRegionValid(regionName) - if !valid { - // This _should_ now be unreachable, given we call RecognizeRegion + if !isRegionValid(regionName, metadata) { return nil, fmt.Errorf("not a valid AWS zone (unknown region): %s", zone) } } else { @@ -1303,14 +1294,43 @@ func newAWSCloud(cfg CloudConfig, awsServices Services) (*Cloud, error) { } } - // Register regions, in particular for ECR credentials - once.Do(func() { - recognizeWellKnownRegions() - }) - return awsCloud, nil } +// isRegionValid accepts an AWS region name and returns if the region is a +// valid region known to the AWS SDK. Considers the region returned from the +// EC2 metadata service to be a valid region as it's only available on a host +// running in a valid AWS region. +func isRegionValid(region string, metadata EC2Metadata) bool { + // Does the AWS SDK know about the region? + for _, p := range endpoints.DefaultPartitions() { + for r := range p.Regions() { + if r == region { + return true + } + } + } + + // ap-northeast-3 is purposely excluded from the SDK because it + // requires an access request (for more details see): + // https://github.com/aws/aws-sdk-go/issues/1863 + if region == "ap-northeast-3" { + return true + } + + // Fallback to checking if the region matches the instance metadata region + // (ignoring any user overrides). This just accounts for running an old + // build of Kubernetes in a new region that wasn't compiled into the SDK + // when Kubernetes was built. + if az, err := getAvailabilityZone(metadata); err == nil { + if r, err := azToRegion(az); err == nil && region == r { + return true + } + } + + return false +} + // Initialize passes a Kubernetes clientBuilder interface to the cloud provider func (c *Cloud) Initialize(clientBuilder cloudprovider.ControllerClientBuilder, stop <-chan struct{}) { c.clientBuilder = clientBuilder diff --git a/staging/src/k8s.io/legacy-cloud-providers/aws/aws_test.go b/staging/src/k8s.io/legacy-cloud-providers/aws/aws_test.go index 7ad3180a31..8f9216241e 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/aws/aws_test.go +++ b/staging/src/k8s.io/legacy-cloud-providers/aws/aws_test.go @@ -1835,6 +1835,47 @@ func TestCreateDisk(t *testing.T) { awsServices.ec2.(*MockedFakeEC2).AssertExpectations(t) } +func TestRegionIsValid(t *testing.T) { + fake := newMockedFakeAWSServices("fakeCluster") + fake.selfInstance.Placement = &ec2.Placement{ + AvailabilityZone: aws.String("pl-fake-999a"), + } + + // This is the legacy list that was removed, using this to ensure we avoid + // region regressions if something goes wrong in the SDK + regions := []string{ + "ap-northeast-1", + "ap-northeast-2", + "ap-northeast-3", + "ap-south-1", + "ap-southeast-1", + "ap-southeast-2", + "ca-central-1", + "eu-central-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", + "cn-north-1", + "cn-northwest-1", + "us-gov-west-1", + "ap-northeast-3", + + // Ensures that we always trust what the metadata service returns + "pl-fake-999", + } + + for _, region := range regions { + assert.True(t, isRegionValid(region, fake.metadata), "expected region '%s' to be valid but it was not", region) + } + + assert.False(t, isRegionValid("pl-fake-991a", fake.metadata), "expected region 'pl-fake-991' to be invalid but it was not") +} + func TestGetCandidateZonesForDynamicVolume(t *testing.T) { tests := []struct { name string diff --git a/staging/src/k8s.io/legacy-cloud-providers/aws/regions.go b/staging/src/k8s.io/legacy-cloud-providers/aws/regions.go deleted file mode 100644 index f834ee1aa7..0000000000 --- a/staging/src/k8s.io/legacy-cloud-providers/aws/regions.go +++ /dev/null @@ -1,96 +0,0 @@ -/* -Copyright 2016 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package aws - -import ( - "sync" - - "k8s.io/klog" - - "k8s.io/apimachinery/pkg/util/sets" -) - -// wellKnownRegions is the complete list of regions known to the AWS cloudprovider -// and credentialprovider. -var wellKnownRegions = [...]string{ - // from `aws ec2 describe-regions --region us-east-1 --query Regions[].RegionName | sort` - "ap-northeast-1", - "ap-northeast-2", - "ap-northeast-3", - "ap-south-1", - "ap-southeast-1", - "ap-southeast-2", - "ca-central-1", - "eu-central-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", - - // these are not registered in many / most accounts - "cn-north-1", - "cn-northwest-1", - "us-gov-west-1", -} - -// awsRegionsMutex protects awsRegions -var awsRegionsMutex sync.Mutex - -// awsRegions is a set of recognized regions -var awsRegions sets.String - -// recognizeRegion is called for each AWS region we know about. -// It currently registers a credential provider for that region. -// There are two paths to discovering a region: -// * we hard-code some well-known regions -// * if a region is discovered from instance metadata, we add that -func recognizeRegion(region string) { - awsRegionsMutex.Lock() - defer awsRegionsMutex.Unlock() - - if awsRegions == nil { - awsRegions = sets.NewString() - } - - if awsRegions.Has(region) { - klog.V(6).Infof("found AWS region %q again - ignoring", region) - return - } - - klog.V(4).Infof("found AWS region %q", region) - - awsRegions.Insert(region) -} - -// recognizeWellKnownRegions calls RecognizeRegion on each WellKnownRegion -func recognizeWellKnownRegions() { - for _, region := range wellKnownRegions { - recognizeRegion(region) - } -} - -// isRegionValid checks if the region is in the set of known regions -func isRegionValid(region string) bool { - awsRegionsMutex.Lock() - defer awsRegionsMutex.Unlock() - - return awsRegions.Has(region) -} diff --git a/staging/src/k8s.io/legacy-cloud-providers/aws/regions_test.go b/staging/src/k8s.io/legacy-cloud-providers/aws/regions_test.go deleted file mode 100644 index c48bc70586..0000000000 --- a/staging/src/k8s.io/legacy-cloud-providers/aws/regions_test.go +++ /dev/null @@ -1,85 +0,0 @@ -/* -Copyright 2016 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package aws - -import ( - "testing" -) - -// TestRegions does basic checking of region verification / addition -func TestRegions(t *testing.T) { - recognizeWellKnownRegions() - - tests := []struct { - Add string - Lookup string - ExpectIsRegion bool - }{ - { - Lookup: "us-east-1", - ExpectIsRegion: true, - }, - { - Lookup: "us-east-1a", - ExpectIsRegion: false, - }, - { - Add: "us-test-1", - Lookup: "us-east-1", - ExpectIsRegion: true, - }, - { - Lookup: "us-test-1", - ExpectIsRegion: true, - }, - { - Add: "us-test-1", - Lookup: "us-test-1", - ExpectIsRegion: true, - }, - } - - for _, test := range tests { - if test.Add != "" { - recognizeRegion(test.Add) - } - - if test.Lookup != "" { - if isRegionValid(test.Lookup) != test.ExpectIsRegion { - t.Fatalf("region valid mismatch: %q", test.Lookup) - } - } - } -} - -// TestRecognizesNewRegion verifies that we see a region from metadata, we recognize it as valid -func TestRecognizesNewRegion(t *testing.T) { - region := "us-testrecognizesnewregion-1" - if isRegionValid(region) { - t.Fatalf("region already valid: %q", region) - } - - awsServices := NewFakeAWSServices(TestClusterID).WithAz(region + "a") - _, err := newAWSCloud(CloudConfig{}, awsServices) - if err != nil { - t.Errorf("error building AWS cloud: %v", err) - } - - if !isRegionValid(region) { - t.Fatalf("newly discovered region not valid: %q", region) - } -}