fix: pass AWS_REGION to etcd-manager #6099

knight42 · 2018-11-19T10:10:32Z

k8s-ci-robot · 2018-11-19T10:10:40Z

Hi @knight42. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

knight42 · 2018-11-19T10:11:05Z

/assign @justinsb

rdrgmnzs · 2018-11-19T18:27:35Z

Thanks @knight42
/ok-to-test

rdrgmnzs · 2018-11-19T18:30:19Z

We actually probably need a better way to pass the region to etcd-manager. The region the s3 bucket is in may not always be the region the cluster is running in.

@justinsb what do you think about setting this so that is defaults to the region the bucket is in but so that the user can overwrite the setting through the spec?

knight42 · 2018-11-21T02:41:22Z

ping @justinsb

Any suggestion?

justinsb · 2018-11-24T19:56:26Z

pkg/model/components/etcdmanager/model.go

@@ -342,6 +342,9 @@ func (b *EtcdManagerBuilder) buildPod(etcdCluster *kops.EtcdClusterSpec) (*v1.Po
 				awsup.TagNameRolePrefix + "master=1",
 			}
 			config.VolumeNameTag = awsup.TagNameEtcdClusterPrefix + etcdCluster.Name
+			container.Env = []v1.EnvVar{
+				{Name: "AWS_REGION", Value: os.Getenv("AWS_REGION")},


Is AWS_REGION set to the state store bucket? I thought we decided it wasn't necessarily the case.

I think we need to explicitly figure out the region of the bucket, and pass that in.

@justinsb If we could infer the region of the bucket that would be best, but the path to the state store bucket is often written in S3Uri, like s3://kops-state-store, one may not figure out the region easily.

justinsb · 2018-11-24T19:57:42Z

Thanks for the fix - I think it's the right idea. What we need to be sure of though is that the AWS_REGION we pass in is indeed the correct region, and I don't think that is necessarily the AWS_REGION that is set for kops. (I think for example you can just set the region in the aws config file, and never set AWS_REGION).

We also shouldn't really set AWS_REGION if AWS_REGION isn't set locally, and in particular not when we're on GCE :-)

knight42 · 2018-11-25T04:58:46Z

What we need to be sure of though is that the AWS_REGION we pass in is indeed the correct region, and I don't think that is necessarily the AWS_REGION that is set for kops.

As mentioned above, the path to the state store bucket is often written in S3Uri, which means the bucket is usually in the same region of the EC2 instances, which means in most of the cases, AWS_REGION is indeed the correct region.

We also shouldn't really set AWS_REGION if AWS_REGION isn't set locally, and in particular not when we're on GCE :-)

Given the context, we could see that only if the cloud provider was AWS, we would set the AWS_REGION.

knight42 · 2019-01-03T06:02:33Z

@rdrgmnzs @justinsb any updates on this?

bksteiny · 2019-04-11T21:15:57Z

I am also running into this problem, and this code fixed my issue while testing the 1.12 beta. If I understand the problem correctly, the current Kops implementation of etcd-manager introduces a breaking change and can't be used if the s3 bucket for a cluster is either in AWS China or (assuming) AWS Gov, right? I can upgrade/revert my test cluster all day with Kops 1.11.x in AWS China, but not with the current implementation in Kops 1.12.

@justinsb - instead of defaulting to the region the cluster is in, are you suggesting the bucket region be explicitly defined in either the cluster spec or some other environment variable? Are you open to @rdrgmnzs's suggestion by defaulting to the same region as the cluster unless otherwise specified somewhere (assuming the cluster spec)?

It sounds like folks are open to resolving this because it is definitely a blocker, but we need to know the preferred way to address it.

k8s-ci-robot · 2019-04-12T02:42:15Z

New changes are detected. LGTM label has been removed.

k8s-ci-robot · 2019-04-12T02:42:26Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: knight42
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: justinsb

If they are not already assigned, you can assign the PR to them by writing /assign @justinsb in a comment when ready.

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:

OWNERS

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

bksteiny · 2019-05-09T15:30:01Z

@justinsb do you have any feedback on this? We talked about this PR during the previous 2 office hour meetings.

mikesplain · 2019-05-10T16:55:30Z

Just a quick update for watchers, we did discuss this again today in office hours. Currently we're going to hold the release for this and I believe @justinsb will chime in shortly. If this requires a bunch of rework, we're going to plan to cut 1.12 and add this in a patch or 1.13, whichever comes first.

bksteiny · 2019-05-10T17:01:01Z

Thanks for the update, @mikesplain!

knight42 · 2019-05-17T03:49:04Z

Close in favor of #6943

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 19, 2018

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 19, 2018

k8s-ci-robot requested review from chrisz100 and robinpercy November 19, 2018 10:10

k8s-ci-robot assigned justinsb Nov 19, 2018

k8s-ci-robot assigned rdrgmnzs Nov 19, 2018

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 19, 2018

justinsb reviewed Nov 24, 2018

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 18, 2019

fix: pass AWS_REGION to etcd-manager

15f08e9

knight42 force-pushed the fix/etcd-manager branch from 803befe to 15f08e9 Compare April 12, 2019 02:42

k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 12, 2019

mikesplain added this to the 1.14 milestone Apr 23, 2019

justinsb added the blocks-next label Apr 26, 2019

Merge branch 'master' into fix/etcd-manager

9a931f7

knight42 closed this May 17, 2019

knight42 deleted the fix/etcd-manager branch May 17, 2019 03:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: pass AWS_REGION to etcd-manager #6099

fix: pass AWS_REGION to etcd-manager #6099

knight42 commented Nov 19, 2018

k8s-ci-robot commented Nov 19, 2018

knight42 commented Nov 19, 2018

rdrgmnzs commented Nov 19, 2018 •

edited

Loading

rdrgmnzs commented Nov 19, 2018 •

edited

Loading

knight42 commented Nov 21, 2018

justinsb Nov 24, 2018

knight42 Nov 25, 2018

justinsb commented Nov 24, 2018

knight42 commented Nov 25, 2018 •

edited

Loading

knight42 commented Jan 3, 2019

bksteiny commented Apr 11, 2019

k8s-ci-robot commented Apr 12, 2019

k8s-ci-robot commented Apr 12, 2019

bksteiny commented May 9, 2019 •

edited

Loading

mikesplain commented May 10, 2019

bksteiny commented May 10, 2019

knight42 commented May 17, 2019

fix: pass AWS_REGION to etcd-manager #6099

fix: pass AWS_REGION to etcd-manager #6099

Conversation

knight42 commented Nov 19, 2018

k8s-ci-robot commented Nov 19, 2018

knight42 commented Nov 19, 2018

rdrgmnzs commented Nov 19, 2018 • edited Loading

rdrgmnzs commented Nov 19, 2018 • edited Loading

knight42 commented Nov 21, 2018

justinsb Nov 24, 2018

Choose a reason for hiding this comment

knight42 Nov 25, 2018

Choose a reason for hiding this comment

justinsb commented Nov 24, 2018

knight42 commented Nov 25, 2018 • edited Loading

knight42 commented Jan 3, 2019

bksteiny commented Apr 11, 2019

k8s-ci-robot commented Apr 12, 2019

k8s-ci-robot commented Apr 12, 2019

bksteiny commented May 9, 2019 • edited Loading

mikesplain commented May 10, 2019

bksteiny commented May 10, 2019

knight42 commented May 17, 2019

rdrgmnzs commented Nov 19, 2018 •

edited

Loading

rdrgmnzs commented Nov 19, 2018 •

edited

Loading

knight42 commented Nov 25, 2018 •

edited

Loading

bksteiny commented May 9, 2019 •

edited

Loading