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

Add ability to create cluster in existing kops-owned VPC #277

Merged
merged 3 commits into from Oct 24, 2018

Conversation

@errordeveloper
Copy link
Member

commented Oct 19, 2018

Description

This will allow kops users to create an EKS cluster in
the same VPC where their kops cluster is. The cluster must
be in one of EKS regions, and it must have 3 subnets in
different AZs.

This fixes #50 and paves the way for #42 as well.

Checklist

  • Code compiles correctly (i.e make build)
  • All tests passing (i.e. make test)
  • Added/modified documentation as required (such as the README)

@errordeveloper errordeveloper changed the title Add ability to create cluster in existing kops-owned VPC WIP: Add ability to create cluster in existing kops-owned VPC Oct 19, 2018

@errordeveloper

This comment has been minimized.

Copy link
Member Author

commented Oct 19, 2018

TODOs (can be addressed in a separate PR):

  • user docs
  • discuss if we need to check $KOPS_STATE_STORE
  • connect SGs also, so we can send traffic between the clusters
  • discuss if we should do something when less then 3 subnets are available
  • test connectivity (nodeport, ALB ingress, kops pod to EKS pod)
  • add an issue to discuss cluster peering add-ons (e.g. via CoreDNS)
"github.com/kubicorn/kubicorn/pkg/logger"
)

type KopsWrapper struct {

This comment has been minimized.

Copy link
@golangcibot

golangcibot Oct 19, 2018

type name will be used as kops.KopsWrapper by other packages, and that stutters; consider calling this Wrapper

@errordeveloper

This comment has been minimized.

Copy link
Member Author

commented Oct 19, 2018

@StevenACoffman please take a look and let us know if this solves your needs!

@StevenACoffman

This comment has been minimized.

Copy link
Contributor

commented Oct 21, 2018

I'm very excited! It looks like --vpc-from-kops-cluster argument specifies a kopsClusterName to lookup all the vpcs and subnets from an account, find those existing that match the tags that identify that Kops cluster, and re-use that vpc and subnets.

Nice!

@errordeveloper

This comment has been minimized.

Copy link
Member Author

commented Oct 22, 2018

@StevenACoffman yes, that's exactly what we have here. I'm hoping we can review and merge soon enough.

pkg/kops/kops.go Outdated Show resolved Hide resolved

@errordeveloper errordeveloper force-pushed the kops-vpc branch 2 times, most recently from 1e972b8 to 51f8b65 Oct 23, 2018

import (
"fmt"

"github.com/weaveworks/eksctl/pkg/eks/api"

This comment has been minimized.

Copy link
@golangcibot

golangcibot Oct 23, 2018

File is not goimports-ed

This comment has been minimized.

Copy link
@errordeveloper

errordeveloper Oct 23, 2018

Author Member

I am rather very confused.

@errordeveloper errordeveloper changed the title WIP: Add ability to create cluster in existing kops-owned VPC Add ability to create cluster in existing kops-owned VPC Oct 23, 2018

return err
}

if err := kw.UseVPC(); err != nil {

This comment has been minimized.

Copy link
@Lazyshot

Lazyshot Oct 23, 2018

Collaborator

It's not clear that this is altering the cluster config.

I think it would be more clear if we returned the altered version of the ClusterConfig and set it directly to the ctl.Spec field.

This comment has been minimized.

Copy link
@errordeveloper

errordeveloper Oct 23, 2018

Author Member

I agree it's not explicit enough. It just seemed very convenient to use cfg once and create a pointer to it inside the wrapper struct (less code). See my next comment.

type Wrapper struct {
clusterName string
cloud awsup.AWSCloud
spec *api.ClusterConfig

This comment has been minimized.

Copy link
@errordeveloper

errordeveloper Oct 23, 2018

Author Member

remove spec

}

// NewWrapper constructs a kops wrapper for a given EKS cluster config and kops cluster
func NewWrapper(cfg *api.ClusterConfig, kopsClusterName string) (*Wrapper, error) {

This comment has been minimized.

Copy link
@errordeveloper

errordeveloper Oct 23, 2018

Author Member
NewWrapper(region, kopsClusterName string)
}

// UseVPC finds VPC and subnets that give kops cluster uses and add those to EKS cluster config
func (k *Wrapper) UseVPC() error {

This comment has been minimized.

Copy link
@errordeveloper

errordeveloper Oct 23, 2018

Author Member
UseVPC(spec api.ClusterConfig)

This comment has been minimized.

Copy link
@errordeveloper

errordeveloper Oct 23, 2018

Author Member

I guess a nice alternative would be GetVPC() *api.ClusterVPC, which would lead to some refactoring, but that maybe a wise idea anyway, we'd probably want to group some related fields inside of api.ClusterConfig anyway (most notably nodegroup stuff).

@errordeveloper

This comment has been minimized.

Copy link
Member Author

commented Oct 23, 2018

@Lazyshot how do you find proposed API changes?

@Lazyshot

This comment has been minimized.

Copy link
Collaborator

commented Oct 23, 2018

Yeah, I think that's better.

I'm not sure if we want to include the refactoring of the cluster config spec in this PR or not.

Add ability to create cluster in existing kops-owned VPC
This will allow kops users to create an EKS cluster in
the same VPC where their kops cluster is. The cluster must
be in one of EKS regions, and it must have 3 subnets in
different AZs.
@errordeveloper

This comment has been minimized.

Copy link
Member Author

commented Oct 24, 2018

@Lazyshot I've updated the API, would you mind to take another look please?

@errordeveloper

This comment has been minimized.

Copy link
Member Author

commented Oct 24, 2018

I've punted discussion to another issue, this PR seems okay to merge as it doesn't get in the way of current critical code path, we can discuss in #282.

@errordeveloper errordeveloper merged commit dfb9938 into master Oct 24, 2018

2 checks passed

GolangCI No issues found!
Details
ci/circleci: make-eksctl-image Your tests passed on CircleCI!
Details

@errordeveloper errordeveloper deleted the kops-vpc branch Oct 24, 2018

@ozzieba ozzieba referenced this pull request Nov 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.