Autodetect version #479

liztio · 2018-07-11T17:03:40Z

What this PR does / why we need it:
Sonobuoy will now detect the server's version and try to run the appropriate conformance container

Which issue(s) this PR fixes

Fixes RFE: Autodetect kubernetes version and set kube-conformance image accordingly #444

Special notes for your reviewer:

Release note:

sonobuoy run automatically detects server version, gen uses latest

--kube-conformance-image-version has several possible arguments:
auto queries the server version and uses that
latest uses the `latest` tag
a literal version (e.g. v1.10.2) always uses that

Signed-off-by: liz <liz@heptio.com>

--kube-conformance-image-version has several options: auto queries the server version and uses that latest uses the `latest` tag a literal version (e.g. v1.10.2) always uses that Signed-off-by: liz <liz@heptio.com>

chuckha

some feedback comments

chuckha · 2018-07-11T18:50:58Z

cmd/sonobuoy/app/gen.go

@@ -65,14 +69,46 @@ func (g *genFlags) Config() (*client.GenConfig, error) {
 return nil, errors.Wrap(err, "could not retrieve E2E config")
 }

+ kubeclient, kubeError := maybeGetClient(&g.kubecfg)


This flow is an unusual go pattern. The name maybeGetClient should be getClient that returns a client and an error, then the error should be checked and handled accordingly, not depend on rbacMode.Enabled handling the nil case.

I know it's unusual, but there's not much choice. The client is only required in some situtations, so erroring out when the configuration isn't valid is the wrong behaviour. I'm pretty committed to having sonobuoy gen work without access to a cluster, and I don't know any other ways to support this.

+1 re naming getClient.

The logic flow below this can be reduced based on whether there is a client and error below.

chuckha · 2018-07-11T18:51:29Z

pkg/config/config.go

@@ -30,7 +30,7 @@ const (
 // DefaultNamespace is the namespace where the master and plugin workers will run (but not necessarily the pods created by the plugin workers).
 DefaultNamespace = "heptio-sonobuoy"
 // DefaultKubeConformanceImage is the URL of the docker image to run for the kube conformance tests.
- DefaultKubeConformanceImage = "gcr.io/heptio-images/kube-conformance:latest"
+ DefaultKubeConformanceImage = "gcr.io/heptio-images/kube-conformance:%s"


This default should not change. The default value needs to be valid on its own. If you need a format string you could create one instead of changing the default value.

s/const/var

chuckha · 2018-07-11T19:37:02Z

cmd/sonobuoy/app/gen.go

+
+ rbacEnabled, err := genflags.rbacMode.Enabled(kubeclient)
+ if err != nil {
+ if errors.Cause(err) == ErrRBACNoClient {


This type of error checking is a bit fragile and also requires errors to be exported for testing. An alternative approach to consider is defining a behavior on the custom error and then type checking to see if the returned error implements that behavior:

https://golang.org/src/os/error.go#L22

This makes it easy to create an error that implements that behavior without having to do a string comparison.

chuckha · 2018-07-11T19:41:01Z

cmd/sonobuoy/app/gen.go

+ var discoveryClient discovery.ServerVersionInterface
+ var image string
+
+ if g.kubeConformanceImage != "" {


This part could be at the very top, if kubeConformanceImage is defined, return the config. Otherwise, we can do the client song and dance and then return the config. This would drop the indentation level, be easier to read, and remove some of the error checking here I think.

We also need the client song and dance for RBAC detection.

ughhhhh yeah true

chuckha · 2018-07-11T19:41:32Z

cmd/sonobuoy/app/gen.go

+ if err != nil {
+ if errors.Cause(err) == ErrImageVersionNoClient {
+ return nil, errors.Wrap(err, kubeError.Error())
+ } else {


drop the else, it's very rarely used in go

timothysc · 2018-07-11T20:10:37Z

cmd/sonobuoy/app/gen.go

@@ -65,14 +69,46 @@ func (g *genFlags) Config() (*client.GenConfig, error) {
 return nil, errors.Wrap(err, "could not retrieve E2E config")
 }

+ kubeclient, kubeError := maybeGetClient(&g.kubecfg)


+1 re naming getClient.

timothysc · 2018-07-11T20:17:58Z

cmd/sonobuoy/app/gen.go

@@ -65,14 +69,46 @@ func (g *genFlags) Config() (*client.GenConfig, error) {
 return nil, errors.Wrap(err, "could not retrieve E2E config")
 }

+ kubeclient, kubeError := maybeGetClient(&g.kubecfg)


The logic flow below this can be reduced based on whether there is a client and error below.

timothysc · 2018-07-11T20:18:53Z

pkg/config/config.go

@@ -30,7 +30,7 @@ const (
 // DefaultNamespace is the namespace where the master and plugin workers will run (but not necessarily the pods created by the plugin workers).
 DefaultNamespace = "heptio-sonobuoy"
 // DefaultKubeConformanceImage is the URL of the docker image to run for the kube conformance tests.
- DefaultKubeConformanceImage = "gcr.io/heptio-images/kube-conformance:latest"
+ DefaultKubeConformanceImage = "gcr.io/heptio-images/kube-conformance:%s"


s/const/var

Signed-off-by: liz <liz@heptio.com>

timothysc · 2018-07-11T21:12:07Z

cmd/sonobuoy/app/gen.go

@@ -65,14 +69,45 @@ func (g *genFlags) Config() (*client.GenConfig, error) {
 return nil, errors.Wrap(err, "could not retrieve E2E config")
 }

+ kubeclient, kubeError := getClient(&g.kubecfg)
+
+ rbacEnabled, err := genflags.rbacMode.Enabled(kubeclient)


Comment on the semantics here and open an issue on auto-detection then lgtm.

chuckha

changes look good, thanks for opening that cleanup ticket

Signed-off-by: liz <liz@heptio.com>

liztio added 2 commits July 11, 2018 12:55

Autodetect kubernetes server version

b13a01a

Signed-off-by: liz <liz@heptio.com>

sonobuoy run automatically detects server version, gen uses latest

6f64a20

--kube-conformance-image-version has several options: auto queries the server version and uses that latest uses the `latest` tag a literal version (e.g. v1.10.2) always uses that Signed-off-by: liz <liz@heptio.com>

liztio force-pushed the autodetect-version branch from d7cb99d to 6f64a20 Compare July 11, 2018 17:05

chuckha reviewed Jul 11, 2018

View reviewed changes

timothysc suggested changes Jul 11, 2018

View reviewed changes

Code review feedback

65b48e4

Signed-off-by: liz <liz@heptio.com>

liztio force-pushed the autodetect-version branch from f68fd3b to 65b48e4 Compare July 11, 2018 20:44

timothysc approved these changes Jul 11, 2018

View reviewed changes

chuckha approved these changes Jul 11, 2018

View reviewed changes

Add some clarification comments for future use

e0a2326

Signed-off-by: liz <liz@heptio.com>

timothysc merged commit 43b287a into vmware-tanzu:master Jul 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Autodetect version #479

Autodetect version #479

liztio commented Jul 11, 2018

chuckha left a comment

chuckha Jul 11, 2018

liztio Jul 11, 2018

timothysc Jul 11, 2018

timothysc Jul 11, 2018

chuckha Jul 11, 2018

timothysc Jul 11, 2018

chuckha Jul 11, 2018

chuckha Jul 11, 2018

liztio Jul 11, 2018

chuckha Jul 11, 2018

chuckha Jul 11, 2018

timothysc Jul 11, 2018

timothysc Jul 11, 2018

timothysc Jul 11, 2018

timothysc Jul 11, 2018

chuckha left a comment

Autodetect version #479

Autodetect version #479

Conversation

liztio commented Jul 11, 2018

chuckha 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

chuckha left a comment

Choose a reason for hiding this comment