Skip to content
This repository has been archived by the owner on Sep 30, 2020. It is now read-only.

Use existing subnets when creating/updating cluster #227

Merged
merged 6 commits into from
Jan 24, 2017
Merged

Use existing subnets when creating/updating cluster #227

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8820e8e
Launch in existing subnets
Jan 10, 2017
dc4cdc2
Validate update and export
Jan 10, 2017
f034cbb
Fix test data
Jan 10, 2017
2c9e398
Move r53 schecks to create, update nodepool stack template
Jan 11, 2017
c7ee3a1
fixing subnet refs
jeremyd Jan 12, 2017
bc7c8a1
Don't try to validate config during update
redbaron Jan 18, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 14 additions & 6 deletions cluster/cluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,12 +151,7 @@ func (c *Cluster) stackProvisioner() *cfnstack.Provisioner {
return cfnstack.NewProvisioner(c.StackName(), c.WorkerDeploymentSettings().StackTags(), stackPolicyBody, c.session)
}

func (c *Cluster) Create(stackBody string, s3URI string) error {
r53Svc := route53.New(c.session)
if err := c.validateDNSConfig(r53Svc); err != nil {
return err
}

func (c *Cluster) Validate(stackBody string, s3URI string) error {
ec2Svc := ec2.New(c.session)
if c.KeyName != "" {
if err := c.validateKeyPair(ec2Svc); err != nil {
Expand All @@ -176,6 +171,19 @@ func (c *Cluster) Create(stackBody string, s3URI string) error {
return err
}

return nil
}

func (c *Cluster) Create(stackBody string, s3URI string) error {
r53Svc := route53.New(c.session)
if err := c.validateDNSConfig(r53Svc); err != nil {
return err
}

if err := c.Validate(stackBody, s3URI); err != nil {
return err
}

cfSvc := cloudformation.New(c.session)
s3Svc := s3.New(c.session)

Expand Down
10 changes: 8 additions & 2 deletions cmd/up.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,11 @@ package cmd
import (
"fmt"

"io/ioutil"

"github.com/coreos/kube-aws/cluster"
"github.com/coreos/kube-aws/config"
"github.com/spf13/cobra"
"io/ioutil"
)

var (
Expand Down Expand Up @@ -47,6 +48,12 @@ func runCmdUp(cmd *cobra.Command, args []string) error {
return fmt.Errorf("Failed to render stack template: %v", err)
}

cluster := cluster.New(conf, upOpts.awsDebug)

if err := cluster.Validate(string(data), upOpts.s3URI); err != nil {
return fmt.Errorf("Error validating cluster: %v", err)
}

if upOpts.export {
templatePath := fmt.Sprintf("%s.stack-template.json", conf.ClusterName)
fmt.Printf("Exporting %s\n", templatePath)
Expand All @@ -59,7 +66,6 @@ func runCmdUp(cmd *cobra.Command, args []string) error {
return nil
}

cluster := cluster.New(conf, upOpts.awsDebug)
fmt.Printf("Creating AWS resources. This should take around 5 minutes.\n")
if err := cluster.Create(string(data), upOpts.s3URI); err != nil {
return fmt.Errorf("Error creating cluster: %v", err)
Expand Down
43 changes: 30 additions & 13 deletions config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,8 @@ type Subnet struct {
AvailabilityZone string `yaml:"availabilityZone,omitempty"`
InstanceCIDR string `yaml:"instanceCIDR,omitempty"`
lastAllocatedAddr *net.IP
SubnetId string `yaml:"subnetId,omitempty"`
SubnetLogicalName string
}

type Experimental struct {
Expand Down Expand Up @@ -812,7 +814,11 @@ func (c DeploymentSettings) Valid() (*DeploymentValidationResult, error) {
}

var instanceCIDRs = make([]*net.IPNet, 0)

for i, subnet := range c.Subnets {
if subnet.SubnetId != "" {
continue
}
if subnet.AvailabilityZone == "" {
return nil, fmt.Errorf("availabilityZone must be set for subnet #%d", i)
}
Expand Down Expand Up @@ -935,7 +941,6 @@ Validates the an existing VPC and it's existing subnets do not conflict with thi
cluster configuration
*/
func (c *Cluster) ValidateExistingVPC(existingVPCCIDR string, existingSubnetCIDRS []string) error {

_, existingVPC, err := net.ParseCIDR(existingVPCCIDR)
if err != nil {
return fmt.Errorf("error parsing existing vpc cidr %s : %v", existingVPCCIDR, err)
Expand Down Expand Up @@ -970,19 +975,23 @@ func (c *Cluster) ValidateExistingVPC(existingVPCCIDR string, existingSubnetCIDR
// Loop through all subnets
// Note: legacy instanceCIDR/availabilityZone stuff has already been marshalled into this format
for _, subnet := range c.Subnets {
_, instanceNet, err := net.ParseCIDR(subnet.InstanceCIDR)
if err != nil {
return fmt.Errorf("error parsing instances cidr %s : %v", c.InstanceCIDR, err)
}
if subnet.SubnetId != "" {
continue
} else {
_, instanceNet, err := net.ParseCIDR(subnet.InstanceCIDR)
if err != nil {
return fmt.Errorf("error parsing instances cidr %s : %v", c.InstanceCIDR, err)
}

//Loop through all existing subnets in the VPC and look for conflicting CIDRS
for _, existingSubnet := range existingSubnets {
if netutil.CidrOverlap(instanceNet, existingSubnet) {
return fmt.Errorf(
"instance cidr (%s) conflicts with existing subnet cidr=%s",
instanceNet,
existingSubnet,
)
//Loop through all existing subnets in the VPC and look for conflicting CIDRS
for _, existingSubnet := range existingSubnets {
if netutil.CidrOverlap(instanceNet, existingSubnet) {
return fmt.Errorf(
"instance cidr (%s) conflicts with existing subnet cidr=%s",
instanceNet,
existingSubnet,
)
}
}
}
}
Expand Down Expand Up @@ -1075,3 +1084,11 @@ func withHostedZoneIDPrefix(id string) string {
}
return id
}

// Ref returns SubnetId or ref to newly created resource
func (s Subnet) Ref() string {
if s.SubnetId != "" {
return fmt.Sprintf("%q", s.SubnetId)
}
return fmt.Sprintf(`{"Ref" : "%s"}`, s.SubnetLogicalName)
}
19 changes: 10 additions & 9 deletions config/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,15 @@ package config
import (
"bytes"
"fmt"
"github.com/coreos/kube-aws/netutil"
"github.com/coreos/kube-aws/test/helper"
"gopkg.in/yaml.v2"
"net"
"reflect"
"strings"
"testing"
"text/template"

"github.com/coreos/kube-aws/netutil"
"github.com/coreos/kube-aws/test/helper"
"gopkg.in/yaml.v2"
)

const minimalConfigYaml = `externalDNSName: test.staging.core-os.net
Expand Down Expand Up @@ -874,8 +875,8 @@ func TestValidateExistingVPC(t *testing.T) {

cluster.VPCCIDR = "10.0.0.0/16"
cluster.Subnets = []*Subnet{
{"ap-northeast-1a", "10.0.1.0/24", nil},
{"ap-northeast-1a", "10.0.2.0/24", nil},
{"ap-northeast-1a", "10.0.1.0/24", nil, "", ""},
{"ap-northeast-1a", "10.0.2.0/24", nil, "", ""},
}

for _, testCase := range validCases {
Expand All @@ -900,8 +901,8 @@ func TestValidateUserData(t *testing.T) {

cluster.Region = "us-west-1"
cluster.Subnets = []*Subnet{
{"us-west-1a", "10.0.1.0/16", nil},
{"us-west-1b", "10.0.2.0/16", nil},
{"us-west-1a", "10.0.1.0/16", nil, "", ""},
{"us-west-1b", "10.0.2.0/16", nil, "", ""},
}

helper.WithDummyCredentials(func(dir string) {
Expand All @@ -924,8 +925,8 @@ func TestRenderStackTemplate(t *testing.T) {

cluster.Region = "us-west-1"
cluster.Subnets = []*Subnet{
{"us-west-1a", "10.0.1.0/16", nil},
{"us-west-1b", "10.0.2.0/16", nil},
{"us-west-1a", "10.0.1.0/16", nil, "", ""},
{"us-west-1b", "10.0.2.0/16", nil, "", ""},
}

helper.WithDummyCredentials(func(dir string) {
Expand Down
2 changes: 2 additions & 0 deletions config/templates/cluster.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,8 +171,10 @@ controller:
# subnets:
# - availabilityZone: us-west-1a
# instanceCIDR: "10.0.0.0/24"
# subnetId: "subnet-xxxxxxxx" #optional
Copy link
Contributor

Choose a reason for hiding this comment

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

ideally it should work with just subnetId specified

subnets:
 - subnetId: subnet-aaaaa
 - subnetId: subnet-zzzzz

does it work like that already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No I did not pull any data from AWS, it uses {{$subnet.AvailabilityZone}} at some points so I used the static value. I'll look into pulling that from existing subnet

Copy link
Contributor

Choose a reason for hiding this comment

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

why AvailabilityZone is even needed if subnets are specified?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I tried getting data from AWS but I couldn't find a nice place to put that checks as i would also need to set availabilityZone and instanceCIDR to not break other stuff. So i ran into other validation failing when removing this out.

I don't mind setting availabilityZone and instanceCIDR and I will leave it as is for now. It would take me to much time to correctly do this. If anyone has an suggestion, easy fix or want to help whit this that would be awesome.

# - availabilityZone: us-west-1b
# instanceCIDR: "10.0.1.0/24"
# subnetId: "subnet-xxxxxxxx" #optional
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: subnetId will be renamed to id in the future according to #195 (comment)
cc @icereval


# CIDR for all service IP addresses
# serviceCIDR: "10.3.0.0/24"
Expand Down
42 changes: 15 additions & 27 deletions config/templates/stack-template.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,12 +51,8 @@
{{end}}
"VPCZoneIdentifier": [
{{range $index, $subnet := .Subnets}}
{{with $subnetLogicalName := printf "Subnet%d" $index}}
{{if gt $index 0}},{{end}}
{
"Ref": "{{$subnetLogicalName}}"
}
{{end}}
{{$subnet.Ref}}
{{end}}
]
},
Expand Down Expand Up @@ -159,12 +155,8 @@
],
"VPCZoneIdentifier": [
{{range $index, $subnet := .Subnets}}
{{with $subnetLogicalName := printf "Subnet%d" $index}}
{{if gt $index 0}},{{end}}
{
"Ref": "{{$subnetLogicalName}}"
}
{{end}}
{{$subnet.Ref}}
{{end}}
],
"LoadBalancerNames" : [
Expand Down Expand Up @@ -660,7 +652,7 @@
"Subnets" : [
{{range $index, $subnet := .Subnets}}
{{if gt $index 0}},{{end}}
{ "Ref" : "Subnet{{$index}}" }
{{$subnet.Ref}}
{{end}}
],
"Listeners" : [
Expand Down Expand Up @@ -1084,17 +1076,17 @@
}
{{end}}
{{range $index, $subnet := .Subnets}}
{{with $subnetLogicalName := printf "Subnet%d" $index}}
{{if not $subnet.SubnetId }}
,
"{{$subnetLogicalName}}": {
"{{$subnet.SubnetLogicalName}}": {
"Properties": {
"AvailabilityZone": "{{$subnet.AvailabilityZone}}",
"CidrBlock": "{{$subnet.InstanceCIDR}}",
"MapPublicIpOnLaunch": {{$.MapPublicIPs}},
"Tags": [
{
"Key": "Name",
"Value": "{{$.ClusterName}}-{{$subnetLogicalName}}"
"Value": "{{$.ClusterName}}-{{$subnet.SubnetLogicalName}}"
},
{
"Key": "KubernetesCluster",
Expand All @@ -1105,19 +1097,19 @@
},
"Type": "AWS::EC2::Subnet"
}
{{end}}
{{if $.ElasticFileSystemID}}
,
"{{$subnetLogicalName}}MountTarget": {
"{{$subnet.SubnetLogicalName}}MountTarget": {
Copy link
Contributor

@mumoshu mumoshu Jan 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Note: When the $subnet is an existing one, we probably need to create a mount target if and only if it doesn't exist for the pair of the subnet and the EFS.
See #208 (comment)

"Properties" : {
"FileSystemId": "{{$.ElasticFileSystemID}}",
"SubnetId": { "Ref": "{{$subnetLogicalName}}" },
"SubnetId": {{$subnet.Ref}},
"SecurityGroups": [ { "Ref": "SecurityGroupMountTarget" } ]
},
"Type" : "AWS::EFS::MountTarget"
}
{{end}}
{{end}}
{{end}}
{{if not .VPCID}}
,
"{{.VPCLogicalName}}": {
Expand Down Expand Up @@ -1190,14 +1182,12 @@
"Type": "AWS::EC2::VPCGatewayAttachment"
}
{{range $index, $subnet := .Subnets}}
{{with $subnetLogicalName := printf "Subnet%d" $index}}
{{if not $subnet.SubnetId }}
,
"{{$subnetLogicalName}}RouteTableAssociation": {
"{{$subnet.SubnetLogicalName}}RouteTableAssociation": {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit but we probably need to create a route table association if and only if the one doesn't exist for the subnet or I guess a cluster creation fails because of a duplicated association in the route table?

"Properties": {
"RouteTableId": { "Ref" : "RouteTable"},
"SubnetId": {
"Ref": "{{$subnetLogicalName}}"
}
"SubnetId": {{$subnet.Ref}}
},
"Type": "AWS::EC2::SubnetRouteTableAssociation"
}
Expand All @@ -1206,14 +1196,12 @@
{{else}}
{{if .RouteTableID}}
{{range $index, $subnet := .Subnets}}
{{with $subnetLogicalName := printf "Subnet%d" $index}}
{{if not $subnet.SubnetId }}
,
"{{$subnetLogicalName}}RouteTableAssociation": {
"{{$subnet.SubnetLogicalName}}RouteTableAssociation": {
"Properties": {
"RouteTableId": "{{$.RouteTableID}}",
"SubnetId": {
"Ref": "{{$subnetLogicalName}}"
}
"SubnetId": {{$subnet.Ref}}
},
"Type": "AWS::EC2::SubnetRouteTableAssociation"
}
Expand Down
3 changes: 2 additions & 1 deletion nodepool/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,15 @@ import (
"fmt"
"io/ioutil"

"path/filepath"

cfg "github.com/coreos/kube-aws/config"
"github.com/coreos/kube-aws/coreos/amiregistry"
"github.com/coreos/kube-aws/coreos/userdatavalidation"
"github.com/coreos/kube-aws/filereader/jsontemplate"
"github.com/coreos/kube-aws/filereader/userdatatemplate"
model "github.com/coreos/kube-aws/model"
"gopkg.in/yaml.v2"
"path/filepath"
)

type Ref struct {
Expand Down
Loading