Support for deploying to existing VPC #346
Conversation
|
Nice work! Making a separate VPC for each k8s cluster would be a real pain :) |
|
Hey thanks! This is great! |
|
\cc @aaronlevy |
|
|
||
| //Find out if stack exists already. This determines whether we should do subnet conflict validatio | ||
| var stackExists bool | ||
| stackNotExistExpr := regexp.MustCompile(fmt.Sprintf("^ValidationError: Stack with id %s does not exist", c.ClusterName)) |
There was a problem hiding this comment.
This is going to be really fragile. There has to be a better way to determine if a stack exist.
What about just len(describeStackOutput.Stacks) > 0 ?
There was a problem hiding this comment.
DescribeStacks returns an Validation error if there are no stacks that have the given name. The regex is the only way I've found to differentiate that error from other stuff (can't contact API, invalid creds, etc)
The other option is to use ListStacks and loop through all the stacks checking for that stack name.
|
@aaronlevy review items addressed. |
|
|
||
| //Find out if stack exists already. This determines whether we should do subnet conflict validation | ||
| var stackExists bool | ||
| stackNotExistExpr := regexp.MustCompile(fmt.Sprintf("^ValidationError: Stack with id %s does not exist", c.ClusterName)) |
There was a problem hiding this comment.
Why do we care at all if the stack exists? Couldn't a VPC exist outside of a cloud-formation stack (and this would skip validation code).
| DescribeSubnets(*ec2.DescribeSubnetsInput) (*ec2.DescribeSubnetsOutput, error) | ||
| } | ||
|
|
||
| func (c *Cluster) validateExistingVPCState(ec2Svc ec2Service) error { |
There was a problem hiding this comment.
The unit test which utilizes this dummy interface is in the next commit
| @@ -5,7 +5,7 @@ import ( | |||
| "testing" | |||
| ) | |||
|
|
|||
| const MinimalConfigYaml = `externalDNSName: test-external-dns-name | |||
| const minimalConfigYaml = `externalDNSName: test-external-dns-name | |||
There was a problem hiding this comment.
it's not useful to expose this, so took the liberty of cleaning it up
|
@aaronlevy ready for review |
| if err != nil { | ||
| return fmt.Errorf("invalid podCIDR: %v", err) | ||
| } | ||
| if vpcNet.Contains(podNetIP) { | ||
| if vpcNet.Contains(podNet.IP) { |
There was a problem hiding this comment.
This seems redundant with https://github.com/coreos/coreos-kubernetes/pull/346/files#diff-e5ad8b828a5de3fe2229bee5c64ab2a0R346
There was a problem hiding this comment.
Yep, will nix this block
|
Couple minor comments, but otherwise lgtm |
In the case of an existing VPC, validates that the new subnet will not conflict with any existing subnets in that VPC
| return fmt.Errorf("could not find vpc %s in region %s", c.VPCID, c.Region) | ||
| } | ||
| if len(vpcOutput.Vpcs) > 1 { | ||
| //Theoretically this should never happen. If it does, we probably want to know. |
There was a problem hiding this comment.
I feel like this an invariant we can trust to amazon.
|
How is this supposed to be used/configured in cluster.yaml? I'm assuming I'll need to build from git but don't see documentation in the repo. |
|
@rothgar |
I've added some validation as well so folks don't waste time creating clusters and tracking down subnet conflicts in the cloudformation events logs when the create fails.