Topology Support (AKA Private Networking) #428 #694

krisnova · 2016-10-19T01:37:11Z

Network topologies in kops

Use Case: Private networking in AWS with Kubernetes defined in #428
Example Usage: Proposed Documentation

Summary

Adding new topologies
- public The existing kops creation topology. All masters and nodes launched into public subnets
- private A new topology. All masters and nodes launched into private subnets, a bastion server/ELB and the k8s API with an ELB.
Adding new flags for kops create cluster
- --topology public|private
- -t public|private
Adding new configuration directive

topology:
   masters: public|private
   nodes: public|private

krisnova · 2016-10-21T03:52:19Z

This will close #428

ajohnstone · 2016-10-21T04:41:48Z

@kris-nova Trying to get my head around the code changes, looks good, however am curious what happens with multiple availability zones and instance groups. Might be worth documenting that a little further.

A NAT gateway can only be attached to one subnet.
A subnet is in a single AZ.
An instance group can span multiple zones. E.g.

NAME            ROLE    MACHINETYPE MIN MAX ZONES
master-eu-west-1a   Master  m4.large    1   1   eu-west-1a
master-eu-west-1b   Master  m4.large    1   1   eu-west-1b
master-eu-west-1c   Master  m4.large    1   1   eu-west-1c
nodes           Node    m4.large    5   5   eu-west-1a,eu-west-1b,eu-west-1c

The changes look like they only apply one NAT gateway?

krisnova · 2016-10-21T04:44:26Z

@ajohnstone - It's probably a bit earlier to be having design discussions around the code. Unfortunately, I am still working on putting the larger pieces together. It's a work in progress (WIP)

Your point is valid, and we know that we will be needing more than 1 NGW in the long run..

Cheers

justinsb

Looks good so far. Added some comments with the pitfalls I can see!

justinsb · 2016-10-23T18:53:09Z

cmd/kops/create_cluster.go

+	case api.TopologyHybrid1:
+		cluster.Spec.Topology = &api.TopologySpec{Type: api.TopologyHybrid1}
+	default:
+		glog.Warningf("Unable to detect topology. Defaulting to public topology.")


I think we should differentiate between empty and invalid here (to guard against typos)

justinsb · 2016-10-23T18:53:57Z

docs/topology.md

+| ----------------- |----------- | ----------------------------------------------------------------------------------------------------------- |
+|   Public Cluster  |   public   | All masters/nodes will be launched in a **public subnet** in the VPC                                        |
+|   Private Cluster |   private  | All masters/nodes will be launched in a **private subnet** in the VPC                                       |
+|     Hybrid (1)    |   hybrid1  | All masters will be launched into a **private subnet**, All nodes will be launched into a **public subnet** |


Don't love this name. I realize it is only a shortcut name, but maybe something more self-descriptive like privatemasterspublicnodes. Although that is a terrible name ;-)

I honestly didn't like it either :) Will think on it and see what else we can come up with

justinsb · 2016-10-23T18:56:13Z

cmd/kops/create_cluster.go

+	case api.TopologyPrivate:
+		cluster.Spec.Topology = &api.TopologySpec{Type: api.TopologyPrivate}
+	case api.TopologyHybrid1:
+		cluster.Spec.Topology = &api.TopologySpec{Type: api.TopologyHybrid1}


I wonder if we should expand topology spec to have two fields - one for master & one for nodes. And then I wonder if this is in fact configured per instance-group... (though I don't actually understand the use-case of mixing right now)

justinsb · 2016-10-23T18:56:54Z

pkg/apis/kops/cluster.go

@@ -343,8 +346,6 @@ func (c *Cluster) FillDefaults() error {
 		// OK
 	} else if c.Spec.Networking.External != nil {
 		// OK
-	} else if c.Spec.Networking.CNI != nil {
-		// OK


I don't think you meant to delete this - a rebase thing?

Yep good catch - will fix :)

justinsb · 2016-10-23T19:02:21Z

pkg/apis/kops/cluster.go

+// Network Topology functions for template parsing
+//
+// Each of these functions can be used in the model templates
+// The go template package currently only supports boolean


You can do eq Topology "private" I think.

In general, it might be easier to replace the network.yaml with go code. I have a prototype of it somewhere that I'll paste a link to if I find it. But the whole reliance on templates just seems pretty fragile and seems to have turned out to be harder for people to change, not easier.

But completely up to you if you want to do this for this PR. Just if you're fighting templates you might find it easier to dump them :-)

I think we should continue to use the templates and follow suit here - if we want to move away from them later we can.. But I think that is a larger effort.

They work for now, so lets just keep it consistent

justinsb · 2016-10-23T19:53:13Z

upup/pkg/fi/cloudup/awstasks/natgateway.go

+
+		response, err := t.Cloud.EC2().CreateNatGateway(request)
+		if err != nil {
+			return fmt.Errorf("error creating Nat gateway: %v", err)


What I've tried doing (inconsistently) is referring to them by the class name i.e. NATGateway everywhere.

justinsb · 2016-10-23T19:54:14Z

upup/pkg/fi/cloudup/awstasks/natgateway.go

+}
+
+type terraformNatGateway struct {
+	AllocationId *string           `json:"AllocationID,omitempty"`


You need to match the names here: https://www.terraform.io/docs/providers/aws/r/nat_gateway.html

i.e. json:"allocation_id,omitempty"

justinsb · 2016-10-23T19:54:22Z

upup/pkg/fi/cloudup/awstasks/natgateway.go

+}
+
+func (e *NATGateway) TerraformLink() *terraform.Literal {
+	return terraform.LiteralProperty("aws_natgateway", *e.AllocationID, "id")


aws_nat_gateway

justinsb · 2016-10-23T19:54:26Z

upup/pkg/fi/cloudup/awstasks/natgateway.go

+		//SubnetID:      e.SubnetID,
+	}
+
+	return t.RenderResource("aws_natgateway", *e.AllocationID, tf)


aws_nat_gateway

justinsb · 2016-10-23T19:54:50Z

upup/pkg/fi/cloudup/awstasks/natgateway_fitask.go

+	"k8s.io/kops/upup/pkg/fi"
+)
+
+// VPC


Ugh ... did it generate this? whoops...

krisnova · 2016-10-25T06:22:09Z

Headers in this PR will bring #706 to life..

This will close #706

billyshambrook · 2016-10-27T06:36:23Z

Hey @kris-nova, looking forward to this, I am up for testing this once it's out of WIP. Currently I am rejigging the terraform output to get the desired effect.

chrislovecnm · 2016-10-27T07:03:47Z

@billyshambrook if you can spend a few minutes, once we are complete and work on a test plan. At least document how you are doing the testing. Would be AWESOME!!

- Fixing topology.md (linting after review) - Adding error message for a neglected --networking cni on private topologies - Adding troubleshooting to documentation

…opology

- Fixing build errors - Missed a privatemasters reference - Fixing the nil pointer problem in SG awstask

- Adding populateClusterSpec unit tests - Topology happy/sad paths - Fleshing out topology in the buildMinimalCluster() function

justinsb · 2016-11-08T23:40:23Z

pkg/apis/kops/cluster.go

@@ -40,67 +40,72 @@ type ClusterList struct {

 type ClusterSpec struct {
 	// The Channel we are following
-	Channel string `json:"channel,omitempty"`
+	Channel               string `json:"channel,omitempty"`


Does this need a gofmt?

Yep! Fixed in 07eb92f

chrislovecnm · 2016-11-09T04:05:09Z

LGTM

rbtcollins · 2016-11-09T21:44:46Z

What's the underlying cause for the reliance on weave rather than AWS routing? I'm in particular concerned about communication between two of these clusters federated across AWS regions.

chulkilee · 2016-11-09T22:11:45Z

@rbtcollins don't know much about federation, but probably due to routes limit in AWS route table? see http://docs.aws.amazon.com/AmazonVPC/latest/UserGuide/VPC_Appendix_Limits.html#vpc-limits-route-tables

robinpercy · 2016-11-09T22:57:08Z

upup/pkg/fi/cloudup/template_functions.go

+// This function is replacing existing yaml
+func (tf *TemplateFunctions) GetBastionZone() (string, error) {
+	var name string
+	if len(tf.cluster.Spec.Zones) <= 1 {


@kris-nova I know this has been merged, but just bumped into this 2 zone requirement on a test, non-HA, deploy. Thought I'd bring it up here in case it's an off-by-one error. More likely that I'm missing an obvious reason for it though :)

Hey @robinpercy - can you provide a little more detail on how we could replicate this? Did you actually run into a scenario where this broke and you were unable to successfully deploy a cluster?

I think the logic here should support zone lists of length 1.

Happy to get a patch out for this ASAP if we can just confirm this is the concern.

Thanks for catching this one!

Hey @kris-nova. Yeah, I ran into this when only specifying a single zone during cluster creation:

$ kops create cluster --topology=private --networking=cni --zones=us-west-1c --ssh-public-key=${MYKEY} --cloud=aws "${NAME}" I1109 22:03:08.390730 4441 cluster.go:498] Assigned CIDR 172.20.96.0/19 to zone us-west-1c I1109 22:03:08.392186 4441 cluster.go:509] Assigned Private CIDR 172.20.128.0/19 to zone us-west-1c I1109 22:03:09.759130 4441 populate_cluster_spec.go:237] Defaulting DNS zone to: Z1HBLAHBLAHBLAHDF Previewing changes that will be made: error building tasks: error handling file "cloudup/_aws/topologies/_topology_private/bastion.yaml": error executing template "_aws/topologies/_topology_private/bastion.yaml": template: _aws/topologies/_topology_private/bastion.yaml:117:24: executing "_aws/topologies/_topology_private/bastion.yaml" at <GetBastionZone>: error calling GetBastionZone: Unable to detect zone name for bastion

Let me know if I should create a ticket.

Good find - I created ticket #860 and opened the PR #861 to solve the problem.

Good catch! How critical is it to get this merged soon?

It's not all that critical for me. I'm just spinning up small test clusters to see how the private topology can work for us. And I'm happy to do that from the PR branch for the time being. Thanks for the quick response on this.

erutherford · 2016-11-09T23:19:49Z

@rbtcollins @chulkilee it's because Kubernetes is hard coded to only handle one route table for node routing updates in AWS. Without modifying kubernetes code, this project is forced to utilize an overlay network to provide a functioning private network configuration

rbtcollins · 2016-11-09T23:22:06Z

@erutherford ok so whats the issue # in kubernetes itself - kops is part of the ecosystem, k8s isn't a fixed quantity to consume, its part of the solution.

erutherford · 2016-11-09T23:32:42Z

@rbtcollins I did a quick scan for tickets and didn't see one, so I don't know of an issue that's open yet. I was merely explaining my understanding of why weave was used for this iteration. I believe this is the code in question.

rbtcollins · 2016-11-10T07:13:41Z

Ok; should I file a ticket then?

googlebot added the cla: yes label Oct 19, 2016

krisnova mentioned this pull request Oct 21, 2016

Support for private subnet instance groups with NAT Gateway #428

Closed

7 tasks

krisnova changed the title ~~[WIP] Private Networking #428~~ [WIP] Topology Support (AKA Private Networking) #428 Oct 22, 2016

googlebot added cla: no and removed cla: yes labels Oct 22, 2016

krisnova force-pushed the private-networking branch from 50dc524 to 2043349 Compare October 22, 2016 05:46

googlebot added cla: yes and removed cla: no labels Oct 22, 2016

krisnova force-pushed the private-networking branch 2 times, most recently from 372598d to 42b44e6 Compare October 22, 2016 05:49

googlebot added cla: no and removed cla: yes labels Oct 23, 2016

krisnova closed this Oct 23, 2016

krisnova force-pushed the private-networking branch from e9e4a96 to 18cc365 Compare October 23, 2016 07:02

krisnova reopened this Oct 23, 2016

googlebot added cla: yes and removed cla: no labels Oct 23, 2016

krisnova force-pushed the private-networking branch from 5e220db to 7f4004b Compare October 23, 2016 07:17

justinsb reviewed Oct 23, 2016

View reviewed changes

krisnova added the [WIP] Work In Progress label Oct 27, 2016

krisnova assigned justinsb Oct 27, 2016

googlebot added cla: no and removed cla: yes labels Oct 28, 2016

krisnova force-pushed the private-networking branch from b1ac2b2 to 0fde4cb Compare October 28, 2016 05:02

krisnova added 12 commits November 8, 2016 15:16

Documentation and CNI requirements

5b81b86

- Fixing topology.md (linting after review) - Adding error message for a neglected --networking cni on private topologies - Adding troubleshooting to documentation

Remove refs to privatemasters

c1644cc

Fixing verbage for AWS route awstask

3c92c6a

Header linter fix

b8d2301

Updating error message for invalid topology.

729598a

Fixing NP panic

95a8c59

Note on upgrades being experimental

5f600eb

Flipping associatePublicIP bool for nodes/bastion/master in private t…

6f78e0c

…opology

Tweaking shell script

4610325

Cleaning up dev script

de9baa2

Stubbing out tests

b1febd9

- Fixing build errors - Missed a privatemasters reference - Fixing the nil pointer problem in SG awstask

Unit Tests

8c41dad

- Adding populateClusterSpec unit tests - Topology happy/sad paths - Fleshing out topology in the buildMinimalCluster() function

krisnova force-pushed the private-networking branch from bd8c59e to 8c41dad Compare November 8, 2016 23:19

justinsb reviewed Nov 8, 2016

View reviewed changes

krisnova added 2 commits November 8, 2016 16:59

Remove dev.sh - We have a make target for this :)

bcbf3df

gofmt on pkg/apis/kops/cluster.go

07eb92f

chrislovecnm merged commit 3eb5948 into kubernetes:master Nov 9, 2016

robinpercy reviewed Nov 9, 2016

View reviewed changes

This was referenced Nov 9, 2016

Error running single node clusters with topology #860

Closed

Fixing conditional for our single node friends #861

Merged

alok87 mentioned this pull request Nov 10, 2016

Error parsing bastion yaml #862

Closed

krisnova mentioned this pull request Nov 21, 2016

Currently only '--networking cni' is supported for private topologies #878

Closed

bcorijn mentioned this pull request Dec 22, 2016

Add topology with private masters and public nodes #1241

Closed

Topology Support (AKA Private Networking) #428 #694

Topology Support (AKA Private Networking) #428 #694

Conversation

krisnova commented Oct 19, 2016 • edited Loading

Network topologies in kops

Summary

krisnova commented Oct 21, 2016

ajohnstone commented Oct 21, 2016

krisnova commented Oct 21, 2016

justinsb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krisnova commented Oct 25, 2016

billyshambrook commented Oct 27, 2016

chrislovecnm commented Oct 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrislovecnm commented Nov 9, 2016

rbtcollins commented Nov 9, 2016

chulkilee commented Nov 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robinpercy Nov 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erutherford commented Nov 9, 2016

rbtcollins commented Nov 9, 2016

erutherford commented Nov 9, 2016

rbtcollins commented Nov 10, 2016

krisnova commented Oct 19, 2016 •

edited

Loading

robinpercy Nov 9, 2016 •

edited

Loading