Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Autodetect version #479

Merged
merged 4 commits into from
Jul 11, 2018
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
22 changes: 19 additions & 3 deletions cmd/sonobuoy/app/args.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,11 +58,27 @@ func AddSonobuoyImage(image *string, flags *pflag.FlagSet) {
// AddKubeConformanceImage initialises an image url flag.
func AddKubeConformanceImage(image *string, flags *pflag.FlagSet) {
flags.StringVar(
image, "kube-conformance-image", config.DefaultKubeConformanceImage,
"Container image override for the kube conformance image.",
image, "kube-conformance-image", "",
"Container image override for the kube conformance image. Overrides --kube-conformance-image-version.",
)
}

// AddKubeConformanceImageVersion initialises an image version flag.
func AddKubeConformanceImageVersion(imageVersion *ConformanceImageVersion, flags *pflag.FlagSet, defaultVersion ConformanceImageVersion) {
help := "Use Heptio's KubeConformance image, but override the version. "
switch defaultVersion {
case ConformanceImageVersionAuto:
help += "Default is 'auto', which will be set to your cluster's version."
case ConformanceImageVersionLatest:
help += "Default is 'latest', which will run the tests for the most recently released Sonobuoy conformance image."
default:
help += fmt.Sprintf("Default is '%s'", defaultVersion)
}

*imageVersion = defaultVersion // default
flags.Var(imageVersion, "kube-conformance-image-version", help)
}

// AddKubeconfigFlag adds a kubeconfig flag to the provided command.
func AddKubeconfigFlag(cfg *Kubeconfig, flags *pflag.FlagSet) {
// The default is the empty string (look in the environment)
Expand Down Expand Up @@ -147,7 +163,7 @@ func AddRBACModeFlags(mode *RBACMode, flags *pflag.FlagSet, defaultMode RBACMode
flags.Var(
mode, "rbac",
// Doesn't use the map in app.rbacModeMap to preserve order so we can add an explanation for detect.
"Whether to enable rbac on Sonobuoy. Valid modes are Enable, Disable, and Detect (query the server to see whether to enable Sonobuoy).",
"Whether to enable rbac on Sonobuoy. Valid modes are Enable, Disable, and Detect (query the server to see whether to enable RBAC).",
)
}

Expand Down
79 changes: 52 additions & 27 deletions cmd/sonobuoy/app/gen.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,27 +23,30 @@ import (
"github.com/pkg/errors"
"github.com/spf13/cobra"
"github.com/spf13/pflag"
"k8s.io/client-go/discovery"
"k8s.io/client-go/kubernetes"

"github.com/heptio/sonobuoy/pkg/client"
"github.com/heptio/sonobuoy/pkg/config"
"github.com/heptio/sonobuoy/pkg/errlog"
)

type genFlags struct {
sonobuoyConfig SonobuoyConfig
mode client.Mode
rbacMode RBACMode
kubecfg Kubeconfig
e2eflags *pflag.FlagSet
namespace string
sonobuoyImage string
kubeConformanceImage string
imagePullPolicy ImagePullPolicy
sonobuoyConfig SonobuoyConfig
mode client.Mode
rbacMode RBACMode
kubecfg Kubeconfig
e2eflags *pflag.FlagSet
namespace string
sonobuoyImage string
kubeConformanceImage string
kubeConformanceImageVersion ConformanceImageVersion
imagePullPolicy ImagePullPolicy
}

var genflags genFlags

func GenFlagSet(cfg *genFlags, rbac RBACMode) *pflag.FlagSet {
func GenFlagSet(cfg *genFlags, rbac RBACMode, version ConformanceImageVersion) *pflag.FlagSet {
genset := pflag.NewFlagSet("generate", pflag.ExitOnError)
AddModeFlag(&cfg.mode, genset)
AddSonobuoyConfigFlag(&cfg.sonobuoyConfig, genset)
Expand All @@ -55,6 +58,7 @@ func GenFlagSet(cfg *genFlags, rbac RBACMode) *pflag.FlagSet {
AddNamespaceFlag(&cfg.namespace, genset)
AddSonobuoyImage(&cfg.sonobuoyImage, genset)
AddKubeConformanceImage(&cfg.kubeConformanceImage, genset)
AddKubeConformanceImageVersion(&cfg.kubeConformanceImageVersion, genset, version)

return genset
}
Expand All @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 re naming getClient.

Copy link
Contributor

Choose a reason for hiding this comment

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

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


rbacEnabled, err := genflags.rbacMode.Enabled(kubeclient)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

if err != nil {
if errors.Cause(err) == ErrRBACNoClient {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

return nil, errors.Wrap(err, kubeError.Error())
}
return nil, err
}

var discoveryClient discovery.ServerVersionInterface
var image string

if g.kubeConformanceImage != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

ughhhhh yeah true

image = g.kubeConformanceImage
} else {
if kubeclient != nil {
discoveryClient = kubeclient.DiscoveryClient
}

imageVersion, err := g.kubeConformanceImageVersion.Get(discoveryClient)
if err != nil {
if errors.Cause(err) == ErrImageVersionNoClient {
return nil, errors.Wrap(err, kubeError.Error())
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

return nil, err
}
}

image = fmt.Sprintf(config.DefaultKubeConformanceImage, imageVersion)
}

return &client.GenConfig{
E2EConfig: e2ecfg,
Config: GetConfigWithMode(&g.sonobuoyConfig, g.mode),
Image: g.sonobuoyImage,
Namespace: g.namespace,
EnableRBAC: getRBACOrExit(&g.rbacMode, &g.kubecfg),
EnableRBAC: rbacEnabled,
ImagePullPolicy: g.imagePullPolicy.String(),
KubeConformanceImage: g.kubeConformanceImage,
KubeConformanceImage: image,
}, nil
}

Expand All @@ -85,7 +121,7 @@ var GenCommand = &cobra.Command{
}

func init() {
GenCommand.Flags().AddFlagSet(GenFlagSet(&genflags, EnabledRBACMode))
GenCommand.Flags().AddFlagSet(GenFlagSet(&genflags, EnabledRBACMode, ConformanceImageVersionLatest))
RootCmd.AddCommand(GenCommand)
}

Expand All @@ -112,11 +148,8 @@ func genManifest(cmd *cobra.Command, args []string) {
os.Exit(1)
}

// getRBACOrExit is a helper function for working with RBACMode. RBACMode is a bit of a special case
// because it only needs a kubeconfig for detect, otherwise errors from kubeconfig can be ignored.
// This function returns a bool because it os.Exit()s in error cases.
func getRBACOrExit(mode *RBACMode, kubeconfig *Kubeconfig) bool {

// maybeGetClient returns a client if one can be found, and the error attempting to retrieve that client if not.
func maybeGetClient(kubeconfig *Kubeconfig) (*kubernetes.Clientset, error) {
// Usually we don't need a client. But in this case, we _might_ if we're using detect.
// So pass in nil if we get an error, then display the errors from trying to get a client
// if it turns out we needed it.
Expand All @@ -133,13 +166,5 @@ func getRBACOrExit(mode *RBACMode, kubeconfig *Kubeconfig) bool {
kubeError = err
}

rbacEnabled, err := genflags.rbacMode.Enabled(client)
if err != nil {
errlog.LogError(errors.Wrap(err, "couldn't detect RBAC mode."))
if errors.Cause(err) == ErrRBACNoClient {
errlog.LogError(kubeError)
}
os.Exit(1)
}
return rbacEnabled
return client, kubeError
}
96 changes: 96 additions & 0 deletions cmd/sonobuoy/app/imageversion.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
/*
Copyright 2018 Heptio Inc.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package app

import (
"strings"

version "github.com/hashicorp/go-version"
"github.com/pkg/errors"
"k8s.io/client-go/discovery"
)

// ConformanceImageVersion represents the version of a conformance image, or "auto" to detect the version
type ConformanceImageVersion string

var (
//ErrImageVersionNoClient is the error returned when we need a client but didn't get on
ErrImageVersionNoClient = errors.New(`can't use nil client with "auto" image version`)
)

const (
// ConformanceImageVersionAuto represents detecting the server's kubernetes version.
ConformanceImageVersionAuto = "auto"
// ConformanceImageVersionLatest represents always using the server's latest version.
ConformanceImageVersionLatest = "latest"
)

// String needed for pflag.Value.
func (c *ConformanceImageVersion) String() string { return string(*c) }

// Type needed for pflag.Value.
func (c *ConformanceImageVersion) Type() string { return "ConformanceImageVersion" }

// Set the ImageVersion to either the string "auto" or a version string
func (c *ConformanceImageVersion) Set(str string) error {
switch str {
case ConformanceImageVersionAuto:
*c = ConformanceImageVersionAuto
case ConformanceImageVersionLatest:
*c = ConformanceImageVersionLatest
default:
if err := validateVersion(str); err != nil {
return err
}
*c = ConformanceImageVersion(str)
}

return nil
}

// Get retrieves the preset version if there is one, or queries client if the ConformanceImageVersion is set to `auto`.
// kubernetes.Interface.Discovery() provides ServerVersionInterface.
func (c *ConformanceImageVersion) Get(client discovery.ServerVersionInterface) (string, error) {
if *c == ConformanceImageVersionAuto {
if client == nil {
return "", ErrImageVersionNoClient
}
version, err := client.ServerVersion()
if err != nil {
return "", errors.Wrap(err, "couldn't retrieve server version")
}

if err := validateVersion(version.GitVersion); err != nil {
return "", err
}

return version.GitVersion, nil
}
return string(*c), nil
}

func validateVersion(v string) error {
version, err := version.NewVersion(v)
if err == nil {
if version.Metadata() != "" || version.Prerelease() != "" {
err = errors.New("version cannot have prelease or metadata, please use a stable version")
} else if !strings.HasPrefix(v, "v") {
err = errors.New("version must start with v")
}
}
return errors.Wrapf(err, "version %q is invalid", v)
}
Loading