--validate: Check server version for minimum version number. #167

G-Harmon · 2018-04-02T23:01:28Z

We need a minimum of v1.8.1 for MCI to work.

This is for Issue #53 .

This change is

nikhiljindal

Thanks @G-Harmon
Looks great. Main comment is about adding a unit test for the parsing and validating server version logic.

nikhiljindal · 2018-04-03T18:02:33Z

app/kubemci/cmd/create.go

+		if newEnough, err := validations.ServerVersionsNewEnough(clients); err != nil {
+			return fmt.Errorf("cluster version could not be validated: %s", err)
+		} else if !newEnough {
+			return fmt.Errorf("not all cluster versions new enough for kubemci")


We should be explicit about what "not new enough" means. Say they should be > 1.8.1 and not 1.10.0?

I wanted to keep the knowledge of the exact supported versions in a minimum of locations. I've centralized it all in validations.go. That prints
fmt.Println("Cluster", key, "is not running a supported kubernetes version. Need at least 1.8.1.")
I'll the testing and logging about 1.10.0.

hmm. Seems like you can just remove the bool here. If err !=nil, then return that as is.
ServerVersionsNewEnough already returns meaningful error messages.

sure, done.

nikhiljindal · 2018-04-03T18:09:27Z

app/kubemci/pkg/validations/validations.go

@@ -89,3 +91,69 @@ func nodePortSameInAllClusters(backend v1beta1.IngressBackend, namespace string,
 	}
 	return nil
 }
+
+func ServerVersionsNewEnough(clients map[string]kubeclient.Interface) (bool, error) {


Does this have to be public?
Call this from the existing public Validate method?

I think it needs to be public. validations.Validate is called from loadbalancersyncer.go. ServerVersionsNewEnough is called from create.go. With this setup, we check cluster versions before calling EnsureIngress().

We should do complete validation first. So validations.Validate() should also be called from create.go before calling EnsureIngress()?

It looks like it works okay to move all the validation code into Validate and calling it earlier in create.go. I'm having a little trouble moving the unit tests over cleanly. (will followup on the unit tests...)

nikhiljindal · 2018-04-03T18:10:37Z

app/kubemci/pkg/validations/validations.go

+		glog.Infof("Checking client %s", key)
+		discoveryClient := clients[key].Discovery()
+		if discoveryClient == nil {
+			return false, fmt.Errorf("no discovery client in client: %+v", clients[key])


Also print the cluster name (key).

nikhiljindal · 2018-04-03T18:11:39Z

app/kubemci/pkg/validations/validations.go

+}
+
+func serverVersionNewEnough(version string) (bool, error) {
+	// Example string we're matching: v1.9.3-gke.0


FYI thats for GKE clusters. Non GKE clusters will return "v1.9.3", without the gke suffix. We should test both in unit test

most of the tests are without the gke suffix, but i've now added one with "-gke.0"

nikhiljindal · 2018-04-03T18:13:26Z

app/kubemci/pkg/validations/validations.go

+	if patch >= 1 {
+		return true, nil
+	}
+	return false, nil


Also check !1.10.0? Or you want to do that in a separate PR?

Done. And it was much easier after factoring it out into a "parse version number" function.

nikhiljindal · 2018-04-03T18:13:41Z

app/kubemci/pkg/validations/validations.go

+	} else if major < 1 {
+		return false, nil
+	}
+	//Minor version


nit: add a space after "//"

nikhiljindal · 2018-04-03T18:17:20Z

app/kubemci/pkg/validations/validations.go

+	return true, nil
+}
+
+func serverVersionNewEnough(version string) (bool, error) {


Add a unit test for this.
Recommend splitting this into
parseServerVersion() and validateServerVersion() and then you can test both independently.

oops. I wrote a test in validations_test.go, but somehow it got lost before uploading this PR.

G-Harmon

Thanks for the review. PTAL. Now with validations_test.go included :)

G-Harmon · 2018-04-03T21:19:43Z

app/kubemci/pkg/validations/validations.go

+	return true, nil
+}
+
+func serverVersionNewEnough(version string) (bool, error) {


oops. I wrote a test in validations_test.go, but somehow it got lost before uploading this PR.

G-Harmon · 2018-04-03T21:22:25Z

app/kubemci/pkg/validations/validations.go

+	if patch >= 1 {
+		return true, nil
+	}
+	return false, nil


Done. And it was much easier after factoring it out into a "parse version number" function.

nikhiljindal

Thanks for the great unit tests!

nikhiljindal · 2018-04-03T23:07:51Z

app/kubemci/pkg/validations/validations_test.go

+		{"v1.2.3", false, 1, 2, 3},
+		{"v10.200.3000", false, 10, 200, 3000},
+		{"v0.8.a", true, 0, 0, 0},
+		{"v1.-2.3", true, 1, 2, 3},


Add v1.9.3-gke.0 as a test case. Also one without "v". Like 1.2.3

nikhiljindal · 2018-04-03T23:09:04Z

app/kubemci/pkg/validations/validations_test.go

+		{1, 8, 0, false},
+		{1, 8, 1, true},
+
+		// 1.10.0 was buggy and doesn't work.


optional: You can link to kubernetes/ingress-gce#182 in case reader is curious

nikhiljindal · 2018-04-03T23:10:52Z

app/kubemci/pkg/validations/validations_test.go

+}
+
+func TestVersionsAcrossClusters(t *testing.T) {
+


nit: line break not needed

nikhiljindal · 2018-04-03T23:13:36Z

app/kubemci/pkg/validations/validations.go

+	return true, nil
+}
+
+func serverVersionNewEnough(major, minor, patch uint64) (bool, error) {


This never returns an error so no need to include it in method signatures

nikhiljindal · 2018-04-03T23:15:41Z

app/kubemci/cmd/create.go

+		if newEnough, err := validations.ServerVersionsNewEnough(clients); err != nil {
+			return fmt.Errorf("cluster version could not be validated: %s", err)
+		} else if !newEnough {
+			return fmt.Errorf("not all cluster versions new enough for kubemci")


hmm. Seems like you can just remove the bool here. If err !=nil, then return that as is.
ServerVersionsNewEnough already returns meaningful error messages.

nikhiljindal · 2018-04-03T23:16:57Z

app/kubemci/pkg/validations/validations.go

@@ -89,3 +91,69 @@ func nodePortSameInAllClusters(backend v1beta1.IngressBackend, namespace string,
 	}
 	return nil
 }
+
+func ServerVersionsNewEnough(clients map[string]kubeclient.Interface) (bool, error) {


We should do complete validation first. So validations.Validate() should also be called from create.go before calling EnsureIngress()?

G-Harmon · 2018-04-04T07:19:05Z

Review status: 0 of 5 files reviewed at latest revision, 4 unresolved discussions.

app/kubemci/pkg/validations/validations.go, line 125 at r1 (raw file):

Previously, nikhiljindal (Nikhil Jindal) wrote…

This never returns an error so no need to include it in method signatures

Done.

app/kubemci/pkg/validations/validations_test.go, line 38 at r1 (raw file):

Previously, nikhiljindal (Nikhil Jindal) wrote…

Add v1.9.3-gke.0 as a test case. Also one without "v". Like 1.2.3

Are you sure it's valid to not start with "v"? Right now, I'm including that in my regular expression: ^v([0-9]).([0-9]).([0-9]*)

Added gke version string.

app/kubemci/pkg/validations/validations_test.go, line 79 at r1 (raw file):

Previously, nikhiljindal (Nikhil Jindal) wrote…

optional: You can link to kubernetes/ingress-gce#182 in case reader is curious

Done.

app/kubemci/pkg/validations/validations_test.go, line 94 at r1 (raw file):

Previously, nikhiljindal (Nikhil Jindal) wrote…

nit: line break not needed

Done.

Comments from Reviewable

nikhiljindal · 2018-04-04T07:44:54Z

Review status: 0 of 5 files reviewed at latest revision, 4 unresolved discussions.

app/kubemci/pkg/validations/validations_test.go, line 38 at r1 (raw file):

Previously, G-Harmon wrote…

Are you sure it's valid to not start with "v"? Right now, I'm including that in my regular expression: ^v([0-9]).([0-9]).([0-9]*)

Added gke version string.

invalid is fine. i was pointing out that you dont have any case without the v prefix

Comments from Reviewable

We need a minimum of v1.8.1 for MCI to work.

G-Harmon · 2018-04-04T17:19:14Z

PTAL, I think this is ready to merge. I added a TODO to move the NodePort validation out of loadbalancersyncer.go and into create.go

nikhiljindal · 2018-04-04T17:30:40Z

/lgtm
thanks!

G-Harmon mentioned this pull request Apr 3, 2018

ingress creation hangs on "Waiting for ingress" #170

Closed

nikhiljindal self-assigned this Apr 3, 2018

nikhiljindal reviewed Apr 3, 2018

View reviewed changes

G-Harmon force-pushed the serverVersionRebased branch from 77f68f3 to df669b4 Compare April 3, 2018 21:47

G-Harmon commented Apr 3, 2018

View reviewed changes

nikhiljindal reviewed Apr 3, 2018

View reviewed changes

G-Harmon force-pushed the serverVersionRebased branch from df669b4 to 0aa8194 Compare April 4, 2018 07:26

G-Harmon mentioned this pull request Apr 4, 2018

IngressSyncer: Fix a bug in DeleteIngress and add more unit tests. #174

Merged

G-Harmon force-pushed the serverVersionRebased branch 2 times, most recently from da39181 to e3c94d5 Compare April 4, 2018 17:14

--validate: Check server version for minimum version number.

e27890b

We need a minimum of v1.8.1 for MCI to work.

G-Harmon force-pushed the serverVersionRebased branch from e3c94d5 to e27890b Compare April 4, 2018 17:18

G-Harmon merged commit 226be1a into GoogleCloudPlatform:master Apr 4, 2018

G-Harmon deleted the serverVersionRebased branch April 4, 2018 17:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

--validate: Check server version for minimum version number. #167

--validate: Check server version for minimum version number. #167

G-Harmon commented Apr 2, 2018 •

edited

Loading

nikhiljindal left a comment

nikhiljindal Apr 3, 2018

G-Harmon Apr 3, 2018

nikhiljindal Apr 3, 2018

G-Harmon Apr 4, 2018

nikhiljindal Apr 3, 2018

G-Harmon Apr 3, 2018

nikhiljindal Apr 3, 2018

G-Harmon Apr 4, 2018

nikhiljindal Apr 3, 2018

G-Harmon Apr 3, 2018

nikhiljindal Apr 3, 2018

G-Harmon Apr 3, 2018

nikhiljindal Apr 3, 2018

G-Harmon Apr 3, 2018

nikhiljindal Apr 3, 2018

G-Harmon Apr 3, 2018

nikhiljindal Apr 3, 2018

G-Harmon Apr 3, 2018

G-Harmon left a comment

G-Harmon Apr 3, 2018

G-Harmon Apr 3, 2018

nikhiljindal left a comment

nikhiljindal Apr 3, 2018

nikhiljindal Apr 3, 2018

nikhiljindal Apr 3, 2018

nikhiljindal Apr 3, 2018

nikhiljindal Apr 3, 2018

nikhiljindal Apr 3, 2018

G-Harmon commented Apr 4, 2018

nikhiljindal commented Apr 4, 2018

G-Harmon commented Apr 4, 2018

nikhiljindal commented Apr 4, 2018

--validate: Check server version for minimum version number. #167

--validate: Check server version for minimum version number. #167

Conversation

G-Harmon commented Apr 2, 2018 • edited Loading

nikhiljindal 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

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

G-Harmon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikhiljindal 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

G-Harmon commented Apr 4, 2018

nikhiljindal commented Apr 4, 2018

G-Harmon commented Apr 4, 2018

nikhiljindal commented Apr 4, 2018

G-Harmon commented Apr 2, 2018 •

edited

Loading