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

AWS: run k8s master in different account or different provider #39996

Merged
merged 1 commit into from
Mar 1, 2017
Merged

Conversation

scheeles
Copy link
Contributor

@scheeles scheeles commented Jan 17, 2017

Currently the master and the nodes must run in the same account. With this change the master can run in a different AWS account, on a different cloud provider or on premise.

Release Notes

AWS cloud provider: allow to run the master with a different AWS account or even on a different cloud provider than the nodes.

@k8s-ci-robot
Copy link
Contributor

Hi @scheeles. 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 @k8s-bot 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.

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. I understand the commands that are listed here.

@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

Once you've signed, please reply here (e.g. "I signed it!") and we'll verify. Thanks.


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. I understand the commands that are listed here.

1 similar comment
@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

Once you've signed, please reply here (e.g. "I signed it!") and we'll verify. Thanks.


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. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jan 17, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@scheeles
Copy link
Contributor Author

scheeles commented Jan 17, 2017 via email

@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Jan 17, 2017
// Maybe if we're not running on AWS, e.g. bootstrap; for now it is not very useful
// This flag enables the possibility to run the master components on a different
// aws account, on a different cloud provider or on premise.
// If the flag is set also Zone, VPCID and KubernetesClusterTag must be provided
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: If the flag is set

} else if cfg.Global.ExternalMaster == true && (cfg.Global.VPCID == "" || cfg.Global.Zone == "" || cfg.Global.KubernetesClusterTag == "") {
// For the external master the Zone, VPCID and KubernetesClusterTag must be set.
// It is not possible to detect it
return nil, fmt.Errorf("Run with eternal Master but Zone: %s or VPCID: %s or KubernetesClusterTag: %s is not set", cfg.Global.Zone, cfg.Global.VPCID, cfg.Global.KubernetesClusterTag)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

external

Copy link
Member

@justinsb justinsb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we want to remove the AZ specification

metadata EC2Metadata
cfg *CloudConfig
region string
availabilityZone string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is odd, because we can be multi-AZ

@@ -1090,7 +1112,7 @@ func (c *Cloud) getAllZones() (sets.String, error) {
// GetZone implements Zones.GetZone
func (c *Cloud) GetZone() (cloudprovider.Zone, error) {
return cloudprovider.Zone{
FailureDomain: c.selfAWSInstance.availabilityZone,
FailureDomain: c.availabilityZone,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is what we want. It appears to be used in two places:

pkg/controller/service/servicecontroller.go:    zone, err := zones.GetZone()
pkg/kubelet/kubelet_node_status.go:                     zone, err := zones.GetZone()

For kubelet node status, we want to continue to label with the node's zone, not the master's zone.

For servicecontroller, zone is actually unused since be9ce30 I believe. I have proposed it for removal in #40060

Hence I think we want to keep it returning selfAWSInstance.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jan 22, 2017
@@ -389,6 +390,11 @@ type CloudConfig struct {
// Maybe if we're not running on AWS, e.g. bootstrap; for now it is not very useful
Zone string

// The aws VPC flag enables the possibility to run the master components
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: AWS

@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 22, 2017
@zmerlynn zmerlynn assigned justinsb and unassigned zmerlynn Jan 30, 2017
@zmerlynn zmerlynn assigned justinsb and unassigned zmerlynn Jan 30, 2017
@zmerlynn zmerlynn removed their assignment Jan 30, 2017
@k8s-ci-robot k8s-ci-robot removed the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 1, 2017
@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

Once you've signed, please reply here (e.g. "I signed it!") and we'll verify. Thanks.


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. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Mar 1, 2017
@scheeles
Copy link
Contributor Author

scheeles commented Mar 1, 2017

@zmerlynn @justinsb Sorry fixed it now

@justinsb
Copy link
Member

justinsb commented Mar 1, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 1, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

The following people have approved this PR: justinsb, k8s-merge-robot, scheeles, sttts

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/new-api size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 1, 2017
@sttts
Copy link
Contributor

sttts commented Mar 1, 2017

@k8s-bot gci gce e2e test this

@justinsb
Copy link
Member

justinsb commented Mar 1, 2017

@k8s-bot gci gce e2e test this
@k8s-bot cvm gce e2e test this

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 5c168e2 into kubernetes:master Mar 1, 2017
@k8s-ci-robot
Copy link
Contributor

@scheeles: The following test(s) failed:

Test name Commit Details Rerun command
Jenkins GCI GCE e2e 0be5e60 link @k8s-bot gci gce e2e test this

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@scheeles scheeles deleted the aws branch March 2, 2017 05:32
k8s-github-robot pushed a commit that referenced this pull request Apr 3, 2017
Automatic merge from submit-queue (batch tested with PRs 43925, 42512)

AWS: add KubernetesClusterID as additional option when VPC is set

This is a small enhancement after the PRs #41695 and  #39996
## Release Notes
```release-note
AWS cloud provider: allow to set KubernetesClusterID or KubernetesClusterTag in combination with VPC.
```
k8s-github-robot pushed a commit that referenced this pull request Apr 8, 2017
…bnetid-routetableid

Automatic merge from submit-queue

Specify subnetid and routetableid via cloud provider config

**What this PR does / why we need it**:
This is a fix for #39996 which is needed since 1.6

Changes introduced from 1.6 broke partially(LoadBalancer) the support for running the master components in a different environment (different aws account/on premise). This PR will add support for specifying the Subnet & RouteTable to use via the cloud provider config.

**Release note**:

```release-note
AWS cloud provider: fix support running the master with a different AWS account or even on a different cloud provider than the nodes.
```
k8s-github-robot pushed a commit that referenced this pull request Apr 25, 2017
Automatic merge from submit-queue (batch tested with PRs 40060, 44860, 44865, 44825, 44162)

servicecontroller: remove unused zone field

The zone field was unused, and this complicated e.g. #39996

```release-note
NONE
```
jayunit100 pushed a commit to jayunit100/kubernetes that referenced this pull request Apr 25, 2017
The zone field was unused, and this complicated e.g. kubernetes#39996
nckturner pushed a commit to nckturner/aws-cloud-controller-manager that referenced this pull request Feb 28, 2018
Automatic merge from submit-queue (batch tested with PRs 43925, 42512)

AWS: add KubernetesClusterID as additional option when VPC is set

This is a small enhancement after the PRs kubernetes/kubernetes#41695 and  kubernetes/kubernetes#39996
## Release Notes
```release-note
AWS cloud provider: allow to set KubernetesClusterID or KubernetesClusterTag in combination with VPC.
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants