Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove hard-coded region list from AWS cloud provider #75990

Merged
merged 1 commit into from May 15, 2019

Conversation

@mcrute
Copy link
Member

commented Apr 1, 2019

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
This consumes the region list from the AWS SDK and accounts for special opt-in regions and removes the hard-coded region list in k8s. This will ensure that the regions are always up-to-date as we update the AWS SDK instead of requiring duplicated accounting.

Adding new regions will requiring updating the AWS SDK dependency and re-building k8s, however this change will always trust the region returned by the EC2 metadata service. Given that built binaries should still work in new regions.

Does this PR introduce a user-facing change?:

consume the AWS region list from the AWS SDK instead of a hard-coded list in the cloud provider

/sig aws
/sig cloud-provider
/assign @justinsb

@mcrute

This comment has been minimized.

Copy link
Member Author

commented Apr 8, 2019

/priority important-soon

@mcrute

This comment has been minimized.

Copy link
Member Author

commented Apr 8, 2019

Ping @justinsb

@mcrute mcrute force-pushed the mcrute:remove-regions branch from 1780e6f to 46c2d7d May 10, 2019

@k8s-ci-robot k8s-ci-robot added approved and removed needs-rebase labels May 10, 2019

@mcrute mcrute force-pushed the mcrute:remove-regions branch 2 times, most recently from 485b7a7 to b8844e2 May 13, 2019

@andrewsykim
Copy link
Member

left a comment

/approve

Nice clean up! One more nit re: old comment, lgtm otherwise

staging/src/k8s.io/legacy-cloud-providers/aws/aws.go Outdated Show resolved Hide resolved
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewsykim, mcrute

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@mcrute mcrute force-pushed the mcrute:remove-regions branch from b8844e2 to 33e9ff9 May 14, 2019

// (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 {

This comment has been minimized.

Copy link
@andrewsykim

andrewsykim May 14, 2019

Member

Wondering if isRegionValid should return an error. If getAvailabilityZone returns "", nil, unlikely but still possible, then IsRegionValid would return true because we ignore the error below. Thoughts?

This comment has been minimized.

Copy link
@mcrute

mcrute May 14, 2019

Author Member

That's a good point. I added a check for the error in that condition. I don't like the idea of returning an error here because it makes the function interface much less intuitive and the handling of the result ambiguous.

@mcrute mcrute force-pushed the mcrute:remove-regions branch from 33e9ff9 to f1ea02e May 14, 2019

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.

@mcrute mcrute force-pushed the mcrute:remove-regions branch from f1ea02e to 3af7a72 May 14, 2019

@andrewsykim

This comment has been minimized.

Copy link
Member

commented May 14, 2019

/lgtm
/retest

@k8s-ci-robot k8s-ci-robot added the lgtm label May 14, 2019

@k8s-ci-robot k8s-ci-robot merged commit 4036097 into kubernetes:master May 15, 2019

20 checks passed

cla/linuxfoundation mcrute authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details

@mcrute mcrute deleted the mcrute:remove-regions branch May 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.