v1alpha2 API #1183

justinsb · 2016-12-17T17:59:28Z

This change is

justinsb · 2016-12-18T22:08:32Z

@kris-nova the TF changes are now manageable I think:

The private subnet cannot have the private- prefix (sadly), because our conversion
logic doesn't have knowledge of the cluster network topology
ELB has health check
Bastions now have IAM profile & role
Renamed bastion_to_master -> all_bastion_to_master
Renamed SecurityGroupRules that have a CIDR include the CIDR in the name

So if you have a look at the TF changes here I think they are more understandable now: dfc2fa7

krisnova · 2016-12-18T22:14:17Z

tests/integration/privateweave/kubernetes.tf

@@ -13,7 +13,7 @@ resource "aws_autoscaling_group" "bastion-privateweave-example-com" {
  launch_configuration = "${aws_launch_configuration.bastion-privateweave-example-com.id}"
  max_size = 1
  min_size = 1
-  vpc_zone_identifier = ["${aws_subnet.private-us-test-1a-privateweave-example-com.id}"]
+  vpc_zone_identifier = ["${aws_subnet.utility-us-test-1a-privateweave-example-com.id}"]


@justinsb

We aren't renaming private subnets to utility subnets are we?

In general the 2 conventions that we were using

Public

These are where the NAT gateways live, and have the synonym utility

Private

These are private, and hold the resources like instances

In general this was a PITA because the keyword private was easily confused with what kops calls a private topology

We are not, but "normal" public & private subnets (where nodes live) are unqualified i.e. us-east-1a.k8s.example.com in both topologies. We have to do this because when we're doing the version conversion we don't have access to the full cluster state. It does have some advantages though - I think it might be possible to move from topology public <-> private, though I haven't tried it..

The utility subnet (ELBs, NAT gateways) is only used in private topologies, and is always public. I did debate renaming it to dmz (as I think that's the common term) but utility is probably at least as clear and less likely to offend.

krisnova · 2016-12-18T22:18:29Z

@justinsb your changes LGTM if you could please follow up on my comment about the private keyword.. just wondering about the context you are using it - otherwise the TF changes look great

chrislovecnm · 2016-12-19T00:10:52Z

Also CI and e2e. Any way we going to get those to pass?

chrislovecnm

Could not resist. I know it is WIP, but I had to take a look. OMG btw. This is amazing!!!!!!

Lots of questions and requests for fmt on errors.

chrislovecnm · 2016-12-19T00:11:53Z

Makefile

+
+apimachinery:
+	#go install ./cmd/libs/go2idl/conversion-gen
+	~/k8s/bin/conversion-gen  --input-dirs k8s.io/kops/pkg/apis/kops/v1alpha1 --v=8  --output-file-base=zz_generated.conversion


Should we have the comments parts in another target and have this documented?

Well... so the challenge is that the fix I had to make it k8s so the conversion generator works only just landed today. So we also have to update k8s to the latest, or pull it out into the shared repo. I'm inclined to do the latter. It's a placeholder for actually installing the code for real

I would encourage us to bump k8s version before this merges.

Just starting review but do we have an API machinery doc floating around this PR?

We don't but we have #1164

Bumping k8s version means going onto the master branch :-(

chrislovecnm · 2016-12-19T00:15:38Z

cmd/kops/create_ig.go

@@ -96,7 +96,12 @@ func RunCreateInstanceGroup(f *util.Factory, cmd *cobra.Command, args []string,
 	// Populate some defaults
 	ig := &api.InstanceGroup{}
 	ig.ObjectMeta.Name = groupName
-	ig.Spec.Role = api.InstanceGroupRole(options.Role)
+
+	role, ok := api.ParseInstanceGroupRole(options.Role, true)


So I got coached on using err, so we should use err ;)

Another case of weird golangisms...

This is actually fine - the function api.ParseInstanceGroupRole() is not returning an err but rather returning a bool than be used in a very similar way as err

Usually you see this if the function doesn't actually do anything wrong, just returns a bool that the user can trigger off of

chrislovecnm · 2016-12-19T00:16:48Z

cmd/kops/rollingupdate_cluster.go

 	cmd.Flags().DurationVar(&rollingupdateCluster.MasterInterval, "master-interval", 5*time.Minute, "Time to wait between restarting masters")
 	cmd.Flags().DurationVar(&rollingupdateCluster.NodeInterval, "node-interval", 2*time.Minute, "Time to wait between restarting nodes")
+	cmd.Flags().DurationVar(&rollingupdateCluster.BastionInterval, "bastion-interval", 5*time.Minute, "Time to wait between restarting bastions")


Oh crap, this will impact my pr ...

Do we need a change here? This PR is huge - to say the least, in a name of keeping the comments clean can you let us know if anything needs to change? If so - what?

This needed

What is needed? We have nothing here that suggests a change to be honest.

Don't understand what we need to do

I will handle on my PR ;) We cannot drain a Bastion ;)

chrislovecnm · 2016-12-19T00:18:22Z

pkg/apis/kops/cluster.go

+)
+
+type ClusterSubnetSpec struct {
+	// TODO: Rename


Is this todo still valid?

Yes, I probably should do this - I changed it from Name -> SubnetName to force compiler errors, because previous Name was actually Zone. This rename will not be visible outside the code though. But I'll do it!

I noticed we had subnet.SubnetName floating around earlier - if we have a usecase for it let's leave it - but if we can use subnet.Name I would prefer that to keep things consistent.

chrislovecnm · 2016-12-19T00:19:11Z

pkg/apis/kops/cluster.go

-	if len(c.Spec.AdminAccess) == 0 {
-		c.Spec.AdminAccess = append(c.Spec.AdminAccess, "0.0.0.0/0")
+	// TODO: Move elsewhere
+	if len(c.Spec.SSHAccess) == 0 {


Think @vendrov can refactor

What needs to be changed here?

chrislovecnm · 2016-12-19T00:39:22Z

upup/pkg/fi/cloudup/apply_cluster.go

 		}

 		nodeUpTags, err := buildNodeupTags(role, tf.cluster, tf.tags)
 		if err != nil {
-			return "", err
+			return nil, err
 		}


SGTM - might I have a fmt please sir

chrislovecnm · 2016-12-19T00:41:02Z

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

+	var r realVPCDHCPOptionsAssociation
+	if err := json.Unmarshal(data, &r); err != nil {
+		return err
+	}


I think I should stop asking

Particularly on autogenerated code :-)

Got me on that one

chrislovecnm · 2016-12-19T00:43:16Z

upup/pkg/fi/cloudup/subnets.go

+	return a[i].Zone < a[j].Zone
+}
+
+func assignCIDRsToSubnets(c *kops.Cluster) error {


This is fun code. How much test coverage do we have?

I see unit tests.. See comment about negative test cases.

chrislovecnm · 2016-12-19T00:44:25Z

upup/pkg/fi/cloudup/subnets_test.go

@@ -0,0 +1,150 @@
+/*
+Copyright 2016 The Kubernetes Authors.


Would love to see some negative test cases. If I am reading this correctly we do not have any, could be wrong

Let's do it in another PR

chrislovecnm · 2016-12-19T00:46:45Z

upup/pkg/kutil/rollingupdate_cluster.go

 		default:
 			return fmt.Errorf("unknown group type for group %q", group.InstanceGroup.ObjectMeta.Name)
 		}
 	}

+	// Upgrade bastions first; if these go down we can't see anything


Ok so we update the bastion here. No drain, but definitely validate.

Actually no validate

Yep - just get these out of the way first IMHO

Great test for this PR, push my PR to after this monster

chrislovecnm · 2016-12-19T02:11:53Z

Think this PR will only address a couple of open bugs... SGTM

We probably shouldn't, but we delete the ASG when you `kops delete ig`. If the ASG doesn't exist though, we can still delete the IG.

Utility subnets are only for type=Bastion

As the subnets will partition the parent NetworkCIDR, so they require it to be set.

By testing with data from various schema versions, we effectively check that they are equivalent. Also this uncovered a few places where we were not strictly ordering things - add some sorts in there.

k8s-ci-robot · 2016-12-19T04:23:47Z

Jenkins Kubernetes AWS e2e failed for commit 91b77ae. Full PR test history.

The magic incantation to run this job again is @k8s-bot aws e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

No schema impact

krisnova

@justinsb This is amazing - great work..

Left a few comments, most of my notes are semantic or questioning new API property names.. Overall this is solid.. very nice

krisnova · 2016-12-19T03:50:44Z

Makefile

+
+apimachinery:
+	#go install ./cmd/libs/go2idl/conversion-gen
+	~/k8s/bin/conversion-gen  --input-dirs k8s.io/kops/pkg/apis/kops/v1alpha1 --v=8  --output-file-base=zz_generated.conversion


Just starting review but do we have an API machinery doc floating around this PR?

krisnova · 2016-12-19T04:00:18Z

cmd/kops/create_cluster.go

-			if existingZones[zone] == nil {
-				cluster.Spec.Zones = append(cluster.Spec.Zones, &api.ClusterZoneSpec{
-					Name: zone,
+		for _, subnetName := range parseZoneList(c.Zones) {


Zone's and subnets are hard

It looks like parseZoneList return a slice of subnet names..? Is that what we want?

Probably not - thanks :-)

krisnova · 2016-12-19T04:03:56Z

cmd/kops/create_cluster.go

 		}
+
+		if c.Bastion {
+			return fmt.Errorf("Bastion supports --topology='private' only.")


Can we get rid of the command line flag connotations in error messages?

We call kops as a library often, and the users typically have no concept of --topology

Wordsmith:

return fmt.Errorf("Attempting to create bastion, in a non-bastion configuration (Topology: private)")

krisnova · 2016-12-19T04:05:52Z

cmd/kops/create_ig.go

@@ -96,7 +96,12 @@ func RunCreateInstanceGroup(f *util.Factory, cmd *cobra.Command, args []string,
 	// Populate some defaults
 	ig := &api.InstanceGroup{}
 	ig.ObjectMeta.Name = groupName
-	ig.Spec.Role = api.InstanceGroupRole(options.Role)
+
+	role, ok := api.ParseInstanceGroupRole(options.Role, true)


Another case of weird golangisms...

This is actually fine - the function api.ParseInstanceGroupRole() is not returning an err but rather returning a bool than be used in a very similar way as err

Usually you see this if the function doesn't actually do anything wrong, just returns a bool that the user can trigger off of

krisnova · 2016-12-19T04:06:52Z

cmd/kops/integration_test.go

 }

-func runTest(t *testing.T, clusterName string, srcDir string) {
+func runTest(t *testing.T, clusterName string, srcDir string, private bool) {


Woo! Private bool! (No change needed)

krisnova · 2016-12-19T04:54:23Z

pkg/model/external_access.go

+	}
+
+	// SSH is open to AdminCIDR set
+	if b.Cluster.IsTopologyPublic() {


What about if !b.Cluster.IsTopologyPublic()? Where are defining the security groups that open the IG's to the ELB security groups?

When we define that ELB, we do it there. So api_loadbalancer

We can refactor if you'd prefer

PS the refactoring will be much easier once we get this in - just normal go refactoring :-)

yes yes indeed - this is far from a deal breaker, and more exciting than anything.. The PRs coming through on this code are going to do just fine - no refactor needed :)

krisnova · 2016-12-19T05:01:13Z

pkg/model/misc.go

@@ -0,0 +1,25 @@
+/*


Convenience functions are good.. and I see when they are useful..

But I have to gripes here..

misc.go

Just naming a file that, opens up the door to contributors dropping in arbitrary logic they don't want to figure out where it should really go

Suggest: convenience.go

s() and i64()

While these are super handy, and I love them in the new models.. they don't really tell the dev (usually me) what the heck they do..

(Yes I mean this) Can we maybe use the convenience functions as wrappers to very specific verbose function names?

func s(v string) *string { return stringToStringPointer(v) } func stringToStringPointer(v string) *string { return &v }

It might seem like super over kill to dig 3 functions deep just to add a & to the beginning of a string variable name, but pointers are a huge deal. Also when we are working on the models, if we need to see what s() does, the code will document itself immediately.

Just in general whenever a shortcut name like s() is used, I have found it useful to have it wrap a more verbose function - that's all I am getting at - simple code doc would work too

Great suggestion.

We actually have aws.String and fi.String so I delegated to those already (and added a comment to fi.String)

I actually debated having a macros package and _ importing them, but I like your convenience.go suggestion a lot better!

krisnova · 2016-12-19T05:09:45Z

tests/integration/create_cluster/ha/expected-v1alpha2.yaml

@@ -0,0 +1,118 @@
+apiVersion: kops/v1alpha2


So this is great - I love seeing these new tests come to life - Good job @justinsb

krisnova · 2016-12-19T05:15:23Z

upup/pkg/fi/cloudup/subnets.go

@@ -0,0 +1,182 @@
+package cloudup


Headers here,

Also GJ on getting the subnets here - you read my mind

krisnova · 2016-12-19T05:17:10Z

upup/pkg/kutil/rollingupdate_cluster.go

 		default:
 			return fmt.Errorf("unknown group type for group %q", group.InstanceGroup.ObjectMeta.Name)
 		}
 	}

+	// Upgrade bastions first; if these go down we can't see anything


Yep - just get these out of the way first IMHO

justinsb · 2016-12-19T06:39:57Z

Removing WIP!

I think we discussed most of these concerns

krisnova · 2016-12-19T06:46:45Z

This is a big PR, with a ton of work thanks to @justinsb - This change LGTM

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 17, 2016

justinsb force-pushed the use_ig_for_bastions_2 branch 2 times, most recently from fba580c to 13f5ef3 Compare December 18, 2016 21:07

krisnova reviewed Dec 18, 2016

View reviewed changes

chrislovecnm previously requested changes Dec 19, 2016

View reviewed changes

justinsb added 12 commits December 18, 2016 21:56

Don't name bastion groups 'bastions.bastion...'

125b9ba

Fix API ELB security group rules

b7522ce

Update tests for new TF output

ef6d1fd

Allow kops delete instancegroup, even if ASG does not exist

0ef9d51

We probably shouldn't, but we delete the ASG when you `kops delete ig`. If the ASG doesn't exist though, we can still delete the IG.

Create stub IAM policy for bastions

51a4adb

Update tags on AutoScalingGroups

5404bb1

Infer subnets of the correct type in kops create ig

ee241b5

Utility subnets are only for type=Bastion

Remove dead code

4475d68

Populate zones when created a new cluster/subnet

031168f

Set default NetworkCIDR before subnets

3ad4e09

As the subnets will partition the parent NetworkCIDR, so they require it to be set.

Set subnet types, create bastion subnets

b7accc4

Multi-version testing; fix few edge cases

91b77ae

By testing with data from various schema versions, we effectively check that they are equivalent. Also this uncovered a few places where we were not strictly ordering things - add some sorts in there.

justinsb force-pushed the use_ig_for_bastions_2 branch from 3043b4c to 91b77ae Compare December 19, 2016 04:17

justinsb added 2 commits December 18, 2016 23:56

A few real-world fixes to create-cluster around initialization

aeef9dc

Rename SubnetName -> Name

a03ea54

No schema impact

krisnova reviewed Dec 19, 2016

View reviewed changes

justinsb added 4 commits December 19, 2016 00:22

Fix example code

3e8858a

Remove overly strict tests

270f429

Fix file headers

50296f1

Fixes per code review

8ce09c6

krisnova self-assigned this Dec 19, 2016

justinsb changed the title ~~WIP: v1alpha2 API~~ v1alpha2 API Dec 19, 2016

krisnova merged commit b3e6dab into kubernetes:master Dec 19, 2016

This was referenced Dec 19, 2016

kops says it is starting the cluster when it isn't #1141

Closed

Fix private CIDR #999

Closed

chrislovecnm mentioned this pull request Dec 19, 2016

Umbrella List of Test Issues and Test Cases #1206

Closed

27 tasks

v1alpha2 API #1183

v1alpha2 API #1183

Conversation

justinsb commented Dec 17, 2016 • edited by k8s-reviewable

justinsb commented Dec 18, 2016

krisnova Dec 18, 2016 • edited

Choose a reason for hiding this comment

Public

Private

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krisnova commented Dec 18, 2016

chrislovecnm commented Dec 19, 2016

chrislovecnm 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

krisnova Dec 19, 2016 • edited

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

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

chrislovecnm commented Dec 19, 2016

k8s-ci-robot commented Dec 19, 2016

krisnova 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

krisnova Dec 19, 2016 • edited

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

misc.go

s() and i64()

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

justinsb commented Dec 19, 2016

krisnova commented Dec 19, 2016

justinsb commented Dec 17, 2016 •

edited by k8s-reviewable

krisnova Dec 18, 2016 •

edited

krisnova Dec 19, 2016 •

edited

krisnova Dec 19, 2016 •

edited