feature/private networking

[icereval]

Hey! hope this doesn't step on any toes as I know you had stated you were working on this, but I had some free time and was very interested in getting the private networking working for (and learning) kube-aws.

Code is brought up to speed w/ master and based on your PR #169 and the direction from @mumoshu #152 (comment).

---

Probably the biggest change to existing spec is it breaks away from the index based resource logical naming to a more predictable naming convention (e.g. Subnet0 -> SubnetUsEast1a). Doing so allows for one to allocate a NAT GW per AZ avoiding a single point of failure while downstream referencing the name by Region/AZ from the node pool workers by Stack/LogicalName (e.g. cluster1-SubnetUsEast1aNatGateway).

• An added benefit of moving away from index naming, changes to cluster.yaml subnet/privateSubnet list item order will not cause resources to be destroyed/created/reattached unnecessarily.

---

I'd be happy to collaborate more directly to see this make it in.

TODO:

• Evaluate option for a migration path for index based naming to AZ based naming (might be slim).
• Node Pool yaml configuration for private worker subnets needs to be complete, should be mostly a copy of the etcd/controller configuration.
• Tests

[mumoshu]

I like this way of automatically recognizing which topology an user meant to use 👍

[mumoshu]

nit: I'd like this even more if this is achieved by a function like the below so that we can eliminate the mutable field/state InternetGatewayRef:

func (s DeploymentSettings) InternetGatewayRef() string {
     if config.InternetGatewayId != "" {
         return fmt.Sprintf("%q", config.InternetGatewayID);
     } else {
         return fmt.Sprintf(`{ "Ref" : %q }`, config.InternetGatewayLogicalName)
     }
}

[mumoshu]

FYI, this is how I tend to add minimum set of tests for error cases for each new feature.

• All the entries in the parseErrorCases slice are sorted in alphabetically ascending order of context.
• configYaml is a complete cluster.yaml which is expected to fail loading

[mumoshu]

nit: Naming this PrivateSubnet would better differentiate this and the existing Subnet(which is meant to be "public subnet").

[mumoshu]

nit: Intended eipAllocationId rather than eipAllocationIDs? For historical reason(😝), it is Id rather than ID. You'd assign only one EIP to a NAT gateway hence singular Id instead of plural IDs.

[mumoshu]

Oh, I've noticed that the corresponding model is already made so 👍 https://github.com/neoandroid/kube-aws/pull/1/files#diff-c76375ae6854b059e3f028487955882dR14

[icereval]

@mumoshu, I realized I might have gone a bit far w/ expected backwards compatibility when changing resource names in order to avoid redeploy issues (based on list index order). Is this something you see as a concern? interested in keeping this change? or would you prefer to stick with the more list order naming approach e.g. Subnet0 $index for now and potentially fix this issue later w/ a semantic major version bump?

[mumoshu]

@icereval Thanks for thinking about that!

Assuming you're mentioning on https://github.com/neoandroid/kube-aws/pull/1/files#diff-c76375ae6854b059e3f028487955882dR18, I like the change! You don't need to stick to the Subnet{{$index}} naming now.

Backward compatibility should be kept as much as possible. However, IMHO, doing so while implementing this would unnecessary complicate things for you. I can revisit you and @neoandroid's change after merging to consider adding backward compatibility of that level or doing a big version bump if necessary, as myself being a maintainer.

A bit more context; that's true I don't like unnecessarily breaking compatibility but we had not yet guaranteed kube-aws update to work among different versions of kube-aws.

So, please keep your great improvement on resource naming 👍

[neoandroid]

I finally merged the work from @icereval , I liked the approach and made a couple of modifications to have the same capabilities that I had with the previous PoC.

• Add a parameter on cluster.yml called "controllerLoadBalancerPrivate" to have the option to put the ELB on the public subnet although I have my controllers on a private subnet. This way I can access them via Internet without the need of setting up a bastion host (a work that could be done later, although it could be considered out of the scope of this project too).
• Add support to create workers on private subnets that are not part of a node pool since it was possible on the PoC and I still need it.

The rest of the changes I've added are small fixes: a typo in cluster.yml and a wrong reference for the Internet Gateway on the cfn template.