Use existing subnets when creating/updating cluster #227

iamsaso · 2017-01-10T09:07:09Z

If you define subnetId you can reuse existing subnets.

subnets:
  - availabilityZone: us-east-1a
    instanceCIDR: "10.0.1.0/24"
    subnetId: "subnet-xxxxxxxx"
  - availabilityZone: us-east-1b
    instanceCIDR: "10.0.2.0/24"
    subnetId: "subnet-xxxxxxxx"
...

there is still some work needed to support node-pools, but I would love to hear opinions or any suggestions on this approach.

It's backward compatible and it uses CF Parameters to overload Subnets with existing subnet id's or references to new resources.

linked to #52

redbaron · 2017-01-10T10:32:39Z

I am not terribly familiar with CF, but I can't see how would it work:
subnets must be specified by id subnet-xxxxx or via reference to a logical resource name created within same CF: { "Ref": "Subnet0" }. You solution references using logical resource name but not using "Ref" function, therefore it would be treated as subnet id and likely will be failing

mumoshu · 2017-01-10T12:58:13Z

Thanks for the PR @Sasso!

In addition to what @redbaron described, I'd recommend you to take a look into https://github.com/coreos/kube-aws/blob/37c242126fd56db938c3f170e51fddd7a6757207/nodepool/config/config.go#L228 to see how it could be achieved; not tested but maybe what you'd need is something like:

func (s Subnet) LogicalName() string {
  logicalNameOfTheSubnet := // something
  return logicalNameOfTheSubnet
}

func (s Subnet) Ref() string {
	if s.SubnetId != "" {
		return fmt.Sprintf("%q", c.SubnetId)
	}
	return fmt.Sprintf(`{"Ref" : "%s"}}`, s.LogicalName())
}

With calling the Ref() func from the template like:

"Properties": {
          "RouteTableId": "{{$.RouteTableID}}",
          "SubnetId": {{$subnet.Ref}}
        },

iamsaso · 2017-01-10T18:38:18Z

@redbaron, @mumoshu thanks for pointing me to the right direction! I pushed an update and I'm testing it now

redbaron · 2017-01-10T18:55:17Z

config/config.go

@@ -299,6 +299,9 @@ type Cluster struct {
 type Subnet struct {
 	AvailabilityZone  string `yaml:"availabilityZone,omitempty"`
 	InstanceCIDR      string `yaml:"instanceCIDR,omitempty"`
+	SubnetId          string `yaml:"subnetId,omitempty"`
+	Create            bool


this looks redundant, it is always == (subnet.SubnetId != "") so might as well remove it

redbaron · 2017-01-10T18:56:10Z

config/config.go

@@ -1032,6 +1044,9 @@ 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 {
+		if !subnet.Create {


validation is still needed, instead of checking for IP ranges, check that subnet actually exist

is there a reason validation is only done in create https://github.com/coreos/kube-aws/blob/master/cluster/cluster.go#L126

shouldn't we validate on update and render too?

redbaron · 2017-01-10T18:57:31Z

config/templates/cluster.yaml

@@ -169,8 +169,10 @@ controller:
 # subnets:
 #   - availabilityZone: us-west-1a
 #     instanceCIDR: "10.0.0.0/24"
+#     subnetId: "subnet-xxxxxxxx" #optional


ideally it should work with just subnetId specified

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

does it work like that already?

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

why AvailabilityZone is even needed if subnets are specified?

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.

redbaron · 2017-01-10T19:01:24Z

config/templates/stack-template.json

@@ -1037,10 +1027,10 @@
    }
    {{if $.ElasticFileSystemID}}
    ,
-    "{{$subnetLogicalName}}MountTarget": {
+    "{{$subnet.SubnetLogicalName}}MountTarget": {


~~this makes a confusing resource name if we use existing subnets. Not sure it is worth fixing though :)~~
this wont work as subnetId has '-' symbol in it which is not alphanumeric and therefore can't be used in CF resource names

I also need to move this out of the subnet create block

codecov-io · 2017-01-10T22:55:03Z

Current coverage is 67.49% (diff: 40.74%)

No coverage report found for master at c538e61.

Powered by Codecov. Last update c538e61...bc7c8a1

iamsaso · 2017-01-10T23:00:31Z

I also added validation for update and export. It seems bad not validating data there.

jeremyd · 2017-01-12T23:39:12Z

This works now. Tested without subnets (regular mode) and with subnetIDs (pre-existing). All tests pass! Anything else we should do to get this accepted? Thanks!

redbaron · 2017-01-12T23:43:05Z

Tested it too today, works as expected. I didn't test nodepools yet though

mumoshu · 2017-01-13T00:52:47Z

@Sasso Thanks for your effort!
I'd greatly appreciate it if you could add some test cases

for main clusters to https://github.com/coreos/kube-aws/blob/master/config/config_test.go#L980
for node pools to https://github.com/coreos/kube-aws/blob/master/nodepool/config/config_test.go#L37

jeremyd · 2017-01-13T01:04:06Z

We use node pools but with the static IDs and it is working. However, I went to test the scenario where users do not have pre-existing subnets and I'm having some problems. We will keep hacking on this.. Might be a few days.. fyi. Thanks!

redbaron · 2017-01-13T08:29:11Z

@jeremyd users NOT having pre-existing subnets is the exact scenario kube-aws originally supports, what kind of problem you encounter? does this PR break it?

jeremyd · 2017-01-13T16:16:31Z

It's not supposed to. Right now it seems to break node pools in that scenario. We are working on it. Unfortunately it's time consuming to test as the only integration test appears to be hard coded to coreos AWS account.

…

On Fri, Jan 13, 2017, 12:29 AM Maxim Ivanov ***@***.***> wrote: @jeremyd <https://github.com/jeremyd> users NOT having pre-existing subnets is the exact scenario kube-aws originally supports, what kind of problem you encounter? does this PR break it? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#227 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAP6PVdmteSMX_HiuUpLtsT_5lz4lKNks5rRzXZgaJpZM4LfLN5> .

mumoshu · 2017-01-14T12:43:43Z

@jeremyd If you meant about the E2E test suite, it isn't hard-coded with a CoreOS AWS account(I don't even have access to one 😝) but you bring your own one for to run it. FYI the usage is documented in https://github.com/coreos/kube-aws/tree/master/e2e

jmcarp · 2017-01-15T01:08:38Z

It looks like this PR adds a subset of the functionality in #195. @Sasso: could you take a look at #195 and send a sub-PR there if you see anything missing? I think we'll get this done faster if we join forces.

redbaron · 2017-01-15T10:24:15Z

@jmcarp this is much simpler change than #195 , it is always easier to merge big changes in small pieces. If this one merged, then #195 can make use of it

redbaron · 2017-01-18T10:57:05Z

@Sasso , branch with conflicts resolved https://github.com/HotelsDotCom/kube-aws/tree/pull-227-conflicts-1

redbaron · 2017-01-18T17:49:57Z

@Sasso , also added commit to remove validation during cluster.Update() as it breaks kube-aws update command if subnets were created (== don't have subnetId specified)

redbaron · 2017-01-18T18:59:53Z

@jeremyd, can you check if latest version of this PR still breaks your case?

mumoshu · 2017-01-23T23:25:23Z

@Sasso @jeremyd @redbaron It seems like my cluster fails in kube-aws validate when there's no subnetLogicalName: mysubnetname defined in cluster.yaml. Is my understanding correct?

$ kube-aws validate --s3-uri s3://mybucket/clusters/kubeawstest4
Validating UserData...
UserData is valid.

Validating stack template...
Error: invalid cloudformation stack: ValidationError: Template format error: Resource name '' is non alphanumeric.

The stack template exported via kube-aws --export is containing this snippet:

"": {
      "Properties": {
        "AvailabilityZone": "ap-northeast-1a",
        "CidrBlock": "10.0.0.0/24",
        "MapPublicIpOnLaunch": true,
        "Tags": [
          {
            "Key": "Name",
            "Value": "kubeawstest4-"
          },
          {
            "Key": "KubernetesCluster",
            "Value": "kubeawstest4"
          }
        ],
        "VpcId": {
          "Ref": "VPC"
        }
      },
      "Type": "AWS::EC2::Subnet"
    },

which indeed seem like the source of the error.

mumoshu · 2017-01-23T23:29:20Z

Would you mind if I add some code to default subnetLogicalName to something like subnet${index} which we had before?

mumoshu · 2017-01-24T01:40:52Z

config/templates/stack-template.json

    ,
-    "{{$subnetLogicalName}}RouteTableAssociation": {
+    "{{$subnet.SubnetLogicalName}}RouteTableAssociation": {


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?

mumoshu · 2017-01-24T01:44:05Z

config/templates/stack-template.json

    {{if $.ElasticFileSystemID}}
    ,
-    "{{$subnetLogicalName}}MountTarget": {
+    "{{$subnet.SubnetLogicalName}}MountTarget": {


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)

mumoshu · 2017-01-24T01:46:24Z

config/templates/cluster.yaml

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


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

mumoshu · 2017-01-24T02:14:40Z

Defaulting of subnetLogicalName is addressed in b7adebb

Update: The branch has been rebased and the commit is now af7fad7

mumoshu · 2017-01-24T02:28:33Z

Thanks to all for your efforts on this 🙇
I've merged this to get the ball rolling.
Please send another pull request(s) for further fixes and improvements if necessary.

jeremyd · 2017-01-27T19:49:11Z

Thanks so much! We were/are so busy going live in production using kube from kube-aws and appreciate the maintainers for working with us on this!! On Mon, Jan 23, 2017, 6:28 PM KUOKA Yusuke <notifications@github.com> wrote: Thanks to all for your efforts on this 🙇 I've merged this to get the ball rolling. Please send another pull request(s) for further fixes and improvements if necessary. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#227 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAP6HTo_q94dYX8Hnn-gonm30nxEO8oks5rVWHSgaJpZM4LfLN5> .

…ed#227) This commit introduces new keys named `subnetId` and `subnetLogiaclName` into items under the top-level `subnets` in `cluster.yaml` to allow deployments to existing subnets while customizing logical names of subnets. Notes on the current implementation: * `subnetId` will be changed to just `id` in the future. See kubernetes-retired#227 (comment) * Route table associations and EFS mount targets may fail while being created in in some cases. See kubernetes-retired#227 (comment) and kubernetes-retired#227 (comment) Notes from original commits before squashing: * Launch in existing subnets * Validate update and export * Don't try to validate config during update * Move r53 schecks to create, update nodepool stack template

mumoshu added the waiting for another code change label Jan 10, 2017

iamsaso force-pushed the master branch from 3844161 to 4fb89ae Compare January 10, 2017 18:35

redbaron reviewed Jan 10, 2017

View reviewed changes

iamsaso force-pushed the master branch from 4fb89ae to de786ba Compare January 10, 2017 22:50

iamsaso force-pushed the master branch from 3828578 to 81316bd Compare January 11, 2017 17:37

mumoshu mentioned this pull request Jan 12, 2017

CREATE_FAILED AWS::EFS::MountTarget Subnet0MountTarget mount target already exists in this AZ #208

Closed

icereval mentioned this pull request Jan 12, 2017

[WIP] Direct & Indirect CloudFormation Stack Parameters #195

Closed

7 tasks

jeremyd force-pushed the master branch from 81316bd to d2378a8 Compare January 12, 2017 19:40

cknowles mentioned this pull request Jan 15, 2017

Create etcd and workers in private subnets, controllers in public subnet #169

Merged

mumoshu mentioned this pull request Jan 18, 2017

Bypass 16kb userdata limit by uploading to s3 bucket #255

Closed

Saso Matejina and others added 6 commits January 19, 2017 09:54

Launch in existing subnets

8820e8e

Validate update and export

dc4cdc2

Fix test data

f034cbb

Move r53 schecks to create, update nodepool stack template

2c9e398

fixing subnet refs

c7ee3a1

Don't try to validate config during update

bc7c8a1

iamsaso force-pushed the master branch from 535cb13 to bc7c8a1 Compare January 19, 2017 17:56

mumoshu mentioned this pull request Jan 20, 2017

Bypass userdata limit with s3 plus follow ups #268

Merged

mumoshu reviewed Jan 24, 2017

View reviewed changes

mumoshu mentioned this pull request Jan 24, 2017

Follow up: Use existing subnets when creating/updating cluster #278

Merged

mumoshu merged commit 68d88b9 into kubernetes-retired:master Jan 24, 2017

mumoshu removed the waiting for another code change label Jan 24, 2017

mumoshu added this to the v0.9.4-rc.1 milestone Jan 24, 2017

mumoshu mentioned this pull request Jan 27, 2017

Add support for customization of network topologies #284

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use existing subnets when creating/updating cluster #227

Use existing subnets when creating/updating cluster #227

iamsaso commented Jan 10, 2017

redbaron commented Jan 10, 2017 •

edited

mumoshu commented Jan 10, 2017

iamsaso commented Jan 10, 2017

redbaron Jan 10, 2017

redbaron Jan 10, 2017

iamsaso Jan 10, 2017

redbaron Jan 10, 2017

iamsaso Jan 10, 2017

redbaron Jan 10, 2017

iamsaso Jan 10, 2017

redbaron Jan 10, 2017 •

edited

iamsaso Jan 10, 2017

codecov-io commented Jan 10, 2017 •

edited

iamsaso commented Jan 10, 2017

jeremyd commented Jan 12, 2017

redbaron commented Jan 12, 2017

mumoshu commented Jan 13, 2017

jeremyd commented Jan 13, 2017

redbaron commented Jan 13, 2017

jeremyd commented Jan 13, 2017 via email

mumoshu commented Jan 14, 2017

jmcarp commented Jan 15, 2017

redbaron commented Jan 15, 2017

redbaron commented Jan 18, 2017

redbaron commented Jan 18, 2017

redbaron commented Jan 18, 2017

mumoshu commented Jan 23, 2017

mumoshu commented Jan 23, 2017

mumoshu Jan 24, 2017

mumoshu Jan 24, 2017 •

edited

mumoshu Jan 24, 2017

mumoshu commented Jan 24, 2017 •

edited

mumoshu commented Jan 24, 2017

jeremyd commented Jan 27, 2017 via email

Use existing subnets when creating/updating cluster #227

Use existing subnets when creating/updating cluster #227

Conversation

iamsaso commented Jan 10, 2017

redbaron commented Jan 10, 2017 • edited

mumoshu commented Jan 10, 2017

iamsaso commented Jan 10, 2017

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

redbaron Jan 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jan 10, 2017 • edited

Current coverage is 67.49% (diff: 40.74%)

iamsaso commented Jan 10, 2017

jeremyd commented Jan 12, 2017

redbaron commented Jan 12, 2017

mumoshu commented Jan 13, 2017

jeremyd commented Jan 13, 2017

redbaron commented Jan 13, 2017

jeremyd commented Jan 13, 2017 via email

mumoshu commented Jan 14, 2017

jmcarp commented Jan 15, 2017

redbaron commented Jan 15, 2017

redbaron commented Jan 18, 2017

redbaron commented Jan 18, 2017

redbaron commented Jan 18, 2017

mumoshu commented Jan 23, 2017

mumoshu commented Jan 23, 2017

Choose a reason for hiding this comment

mumoshu Jan 24, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mumoshu commented Jan 24, 2017 • edited

mumoshu commented Jan 24, 2017

jeremyd commented Jan 27, 2017 via email

redbaron commented Jan 10, 2017 •

edited

redbaron Jan 10, 2017 •

edited

codecov-io commented Jan 10, 2017 •

edited

mumoshu Jan 24, 2017 •

edited

mumoshu commented Jan 24, 2017 •

edited