Repository: doitintl/kube-no-trouble
Pull request #139 (2021-04-29 11:42:38)

Feat: Detect K8s version
This PR adds support for detecting version of K8s server. It's a foundation for being able to filter relevant findings later on.

From #130:
- [x] Only detect version if one of the relevant collectors (cluster, Helm v2/3) is enabled (I'm thinking about extending the collector interface here)
- [x] Allow version override via flag - for cases when this is used with files in CI

WIP, apart from above it's missing:
- [x] Update docs once flag to override version is added
- [x] Tests
- [x] Maybe reduce code duplication - in particular `GetServerVersion` and discovery K8s client creation in `New*Collector` functions (without introducing to much complexity via embedding).
- [x] Needs squash/commits cleanup before merge (please!)


This PR addresses first half of #130.

stepanstipl: It's WIP, still missing bits as listed above, but would love your feedback @david-doit-intl 
stepanstipl: The latest iteration introduces the `collector.VersionCollector`, and then uses
```go
versionCol, ok := c.(collector.VersionCollector)
```
to check if the interface is implemented. The code is ready for another look _(pending missing tests and readme update)._
david-doit-intl: @stepanstipl looks good squash some commits and consider renaming fake.go
david-doit-intl: btw after this pr lands a new release is propably in order

Patch:
diff --git a/README.md b/README.md
index 6d3fe0d..ddab691 100644
--- a/README.md
+++ b/README.md
@@ -83,14 +83,21 @@ Usage of ./kubent:
       --helm2                     enable Helm v2 collector (default true)
       --helm3                     enable Helm v3 collector (default true)
   -k, --kubeconfig string         path to the kubeconfig file (default "/Users/stepan/.kube/config")
-  -l, --loglevel string           set log level (trace, debug, info, warn, error, fatal, panic, disabled)
+  -l, --log-level string          set log level (trace, debug, info, warn, error, fatal, panic, disabled) (default "info")
   -o, --output string             output format - [text|json] (default "text")
+  -t, --target-version string     target K8s version in major.minor format (autodetected by default)
 ```
 
 - *`-a, --additional-kind`*
   Tells `kubent` to flag additional custom resources when found in the specified version. The flag can be used multiple
   times. The expected format is full *Kind.version.group.com* form - e.g. `-a ManagedCertificate.v1.networking.gke.io`.
 
+- *`-t, --target-version`*
+  `Kubent` will try to detect K8S cluster version and display only relevant findings. This flag allows to override this
+  version for scenarios like use in CI with the file collector only, when detection from an actual cluster is not possible.
+  Expected format is `major.minor[.patch]`, e.g. `1.16` or `1.16.3`.
+
+
 ### Use in CI
 
 `kubent` will by default return `0` exit code if the program succeeds, even if
diff --git a/cmd/kubent/main.go b/cmd/kubent/main.go
index c710191..e7d47b3 100644
--- a/cmd/kubent/main.go
+++ b/cmd/kubent/main.go
@@ -13,7 +13,6 @@ import (
 	"github.com/rs/zerolog"
 	"github.com/rs/zerolog/log"
 	"k8s.io/apimachinery/pkg/runtime/schema"
-
 	_ "k8s.io/client-go/plugin/pkg/client/auth/azure"
 	_ "k8s.io/client-go/plugin/pkg/client/auth/gcp"
 	_ "k8s.io/client-go/plugin/pkg/client/auth/oidc"
@@ -77,6 +76,16 @@ func initCollectors(config *config.Config) []collector.Collector {
 	return collectors
 }
 
+func getServerVersion(collectors []collector.Collector) (string, error) {
+	for _, c := range collectors {
+		versionCol, ok := c.(collector.VersionCollector)
+		if ok {
+			return versionCol.GetServerVersion()
+		}
+	}
+	return "", nil
+}
+
 func main() {
 	exitCode := EXIT_CODE_FAIL_GENERIC
 
@@ -91,9 +100,18 @@ func main() {
 
 	log.Info().Msg(">>> Kube No Trouble `kubent` <<<")
 	log.Info().Msgf("version %s (git sha %s)", version, gitSha)
-	log.Info().Msg("Initializing collectors and retrieving data")
 
+	log.Info().Msg("Initializing collectors and retrieving data")
 	initCollectors := initCollectors(config)
+
+	if config.TargetVersion == "" {
+		config.TargetVersion, err = getServerVersion(initCollectors)
+		if err != nil {
+			log.Fatal().Err(err).Msg("Failed to detect K8s version")
+		}
+	}
+	log.Info().Msgf("Target K8s version is %s", config.TargetVersion)
+
 	collectors := getCollectors(initCollectors)
 
 	// this could probably use some error checking in future, but
diff --git a/pkg/collector/cluster.go b/pkg/collector/cluster.go
index 4a885fc..d06590c 100644
--- a/pkg/collector/cluster.go
+++ b/pkg/collector/cluster.go
@@ -3,6 +3,7 @@ package collector
 import (
 	"encoding/json"
 	"fmt"
+	"k8s.io/client-go/tools/clientcmd"
 
 	"github.com/rs/zerolog/log"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -11,13 +12,12 @@ import (
 	"k8s.io/client-go/discovery/cached/memory"
 	"k8s.io/client-go/dynamic"
 	"k8s.io/client-go/restmapper"
-	"k8s.io/client-go/tools/clientcmd"
 )
 
 type ClusterCollector struct {
 	*commonCollector
+	*kubeCollector
 	clientSet           dynamic.Interface
-	discoveryClient     discovery.DiscoveryInterface
 	additionalResources []schema.GroupVersionResource
 }
 
@@ -28,7 +28,15 @@ type ClusterOpts struct {
 }
 
 func NewClusterCollector(opts *ClusterOpts, additionalKinds []string) (*ClusterCollector, error) {
-	collector := &ClusterCollector{commonCollector: &commonCollector{name: "Cluster"}}
+	kubeCollector, err := newKubeCollector(opts.Kubeconfig, opts.DiscoveryClient)
+	if err != nil {
+		return nil, err
+	}
+
+	collector := &ClusterCollector{
+		kubeCollector:   kubeCollector,
+		commonCollector: newCommonCollector("Cluster"),
+	}
 
 	if opts.ClientSet == nil {
 		config, err := clientcmd.BuildConfigFromFlags("", opts.Kubeconfig)
@@ -45,20 +53,6 @@ func NewClusterCollector(opts *ClusterOpts, additionalKinds []string) (*ClusterC
 		collector.clientSet = opts.ClientSet
 	}
 
-	if opts.DiscoveryClient == nil {
-		config, err := clientcmd.BuildConfigFromFlags("", opts.Kubeconfig)
-		if err != nil {
-			return nil, err
-		}
-
-		collector.discoveryClient, err = discovery.NewDiscoveryClientForConfig(config)
-		if err != nil {
-			return nil, err
-		}
-	} else {
-		collector.discoveryClient = opts.DiscoveryClient
-	}
-
 	mapper := restmapper.NewDeferredDiscoveryRESTMapper(memory.NewMemCacheClient(collector.discoveryClient))
 	for _, ar := range additionalKinds {
 		gvk, _ := schema.ParseKindArg(ar)
diff --git a/pkg/collector/collector.go b/pkg/collector/collector.go
index e708323..61f9417 100644
--- a/pkg/collector/collector.go
+++ b/pkg/collector/collector.go
@@ -5,10 +5,20 @@ type Collector interface {
 	Name() string
 }
 
+type VersionCollector interface {
+	GetServerVersion() (string, error)
+}
+
 type commonCollector struct {
 	name string
 }
 
+func newCommonCollector(name string) *commonCollector {
+	return &commonCollector{
+		name: name,
+	}
+}
+
 func (c *commonCollector) Name() string {
 	return c.name
 }
diff --git a/pkg/collector/fake.go b/pkg/collector/fake.go
new file mode 100644
index 0000000..4ad4103
--- /dev/null
+++ b/pkg/collector/fake.go
@@ -0,0 +1,21 @@
+package collector
+
+const FAKE_VERSION = "1.2.3"
+
+type fakeCollector struct {
+	*commonCollector
+}
+
+func (c *fakeCollector) Get() ([]map[string]interface{}, error) {
+	return []map[string]interface{}{}, nil
+}
+
+func NewFakeCollector() *fakeCollector {
+	return &fakeCollector{
+		commonCollector: newCommonCollector("Fake"),
+	}
+}
+
+func (c *fakeCollector) GetServerVersion() (string, error) {
+	return FAKE_VERSION, nil
+}
diff --git a/pkg/collector/file.go b/pkg/collector/file.go
index 07544b0..aeec71c 100644
--- a/pkg/collector/file.go
+++ b/pkg/collector/file.go
@@ -4,11 +4,12 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
-	"github.com/ghodss/yaml"
-	"helm.sh/helm/v3/pkg/releaseutil"
 	"io/ioutil"
 	"os"
 	"sort"
+
+	"github.com/ghodss/yaml"
+	"helm.sh/helm/v3/pkg/releaseutil"
 )
 
 type FileCollector struct {
@@ -27,7 +28,7 @@ func NewFileCollector(opts *FileOpts) (*FileCollector, error) {
 	}
 
 	collector := &FileCollector{
-		commonCollector: &commonCollector{name: "File"},
+		commonCollector: newCommonCollector("File"),
 		filenames:       opts.Filenames,
 	}
 
diff --git a/pkg/collector/helm2.go b/pkg/collector/helm2.go
index 4d64245..92ce680 100644
--- a/pkg/collector/helm2.go
+++ b/pkg/collector/helm2.go
@@ -2,30 +2,40 @@ package collector
 
 import (
 	"fmt"
-
-	corev1 "k8s.io/client-go/kubernetes/typed/core/v1"
 	"k8s.io/client-go/tools/clientcmd"
 
+	"github.com/ghodss/yaml"
 	"helm.sh/helm/pkg/storage"
 	"helm.sh/helm/pkg/storage/driver"
 	"helm.sh/helm/v3/pkg/releaseutil"
-
-	"github.com/ghodss/yaml"
+	"k8s.io/client-go/discovery"
+	corev1 "k8s.io/client-go/kubernetes/typed/core/v1"
 )
 
 type HelmV2Collector struct {
 	*commonCollector
+	*kubeCollector
 	client       *corev1.CoreV1Client
 	secretsStore *storage.Storage
 	configStore  *storage.Storage
 }
 
 type HelmV2Opts struct {
-	Kubeconfig string
+	Kubeconfig      string
+	DiscoveryClient discovery.DiscoveryInterface
 }
 
 func NewHelmV2Collector(opts *HelmV2Opts) (*HelmV2Collector, error) {
-	collector := &HelmV2Collector{commonCollector: &commonCollector{name: "Helm v2"}}
+
+	kubeCollector, err := newKubeCollector(opts.Kubeconfig, opts.DiscoveryClient)
+	if err != nil {
+		return nil, err
+	}
+
+	collector := &HelmV2Collector{
+		commonCollector: newCommonCollector("Helm v2"),
+		kubeCollector:   kubeCollector,
+	}
 
 	config, err := clientcmd.BuildConfigFromFlags("", opts.Kubeconfig)
 	if err != nil {
@@ -47,7 +57,6 @@ func NewHelmV2Collector(opts *HelmV2Opts) (*HelmV2Collector, error) {
 }
 
 func (c *HelmV2Collector) Get() ([]map[string]interface{}, error) {
-
 	releases, err := c.secretsStore.ListDeployed()
 	if err != nil {
 		return nil, err
diff --git a/pkg/collector/helm3.go b/pkg/collector/helm3.go
index 165d198..6083d9d 100644
--- a/pkg/collector/helm3.go
+++ b/pkg/collector/helm3.go
@@ -2,30 +2,39 @@ package collector
 
 import (
 	"fmt"
-
-	corev1 "k8s.io/client-go/kubernetes/typed/core/v1"
 	"k8s.io/client-go/tools/clientcmd"
 
+	"github.com/ghodss/yaml"
 	"helm.sh/helm/v3/pkg/releaseutil"
 	"helm.sh/helm/v3/pkg/storage"
 	"helm.sh/helm/v3/pkg/storage/driver"
-
-	"github.com/ghodss/yaml"
+	"k8s.io/client-go/discovery"
+	corev1 "k8s.io/client-go/kubernetes/typed/core/v1"
 )
 
 type HelmV3Collector struct {
 	*commonCollector
+	*kubeCollector
 	client       *corev1.CoreV1Client
 	secretsStore *storage.Storage
 	configStore  *storage.Storage
 }
 
 type HelmV3Opts struct {
-	Kubeconfig string
+	Kubeconfig      string
+	DiscoveryClient discovery.DiscoveryInterface
 }
 
 func NewHelmV3Collector(opts *HelmV3Opts) (*HelmV3Collector, error) {
-	collector := &HelmV3Collector{commonCollector: &commonCollector{name: "Helm v3"}}
+	kubeCollector, err := newKubeCollector(opts.Kubeconfig, opts.DiscoveryClient)
+	if err != nil {
+		return nil, err
+	}
+
+	collector := &HelmV3Collector{
+		commonCollector: newCommonCollector("Helm v2"),
+		kubeCollector:   kubeCollector,
+	}
 
 	config, err := clientcmd.BuildConfigFromFlags("", opts.Kubeconfig)
 	if err != nil {
@@ -47,7 +56,6 @@ func NewHelmV3Collector(opts *HelmV3Opts) (*HelmV3Collector, error) {
 }
 
 func (c *HelmV3Collector) Get() ([]map[string]interface{}, error) {
-
 	releases, err := c.secretsStore.ListDeployed()
 	if err != nil {
 		return nil, err
diff --git a/pkg/collector/kube.go b/pkg/collector/kube.go
new file mode 100644
index 0000000..251de2b
--- /dev/null
+++ b/pkg/collector/kube.go
@@ -0,0 +1,39 @@
+package collector
+
+import (
+	"fmt"
+	"k8s.io/client-go/discovery"
+	"k8s.io/client-go/tools/clientcmd"
+)
+
+type kubeCollector struct {
+	discoveryClient discovery.DiscoveryInterface
+}
+
+func newKubeCollector(kubeconfig string, discoveryClient discovery.DiscoveryInterface) (*kubeCollector, error) {
+	col := &kubeCollector{}
+
+	if discoveryClient == nil {
+		config, err := clientcmd.BuildConfigFromFlags("", kubeconfig)
+		if err != nil {
+			return nil, err
+		}
+
+		col.discoveryClient, err = discovery.NewDiscoveryClientForConfig(config)
+		if err != nil {
+			return nil, err
+		}
+	} else {
+		col.discoveryClient = discoveryClient
+	}
+
+	return col, nil
+}
+
+func (c *kubeCollector) GetServerVersion() (string, error) {
+	version, err := c.discoveryClient.ServerVersion()
+	if err != nil {
+		return "", fmt.Errorf("failed to get server version %w", err)
+	}
+	return version.Major + "." + version.Minor, nil
+}
diff --git a/pkg/config/config.go b/pkg/config/config.go
index f145cee..b2996d9 100644
--- a/pkg/config/config.go
+++ b/pkg/config/config.go
@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"strconv"
 	"strings"
 	"unicode"
 
@@ -23,6 +24,7 @@ type Config struct {
 	Kubeconfig      string
 	LogLevel        ZeroLogLevel
 	Output          string
+	TargetVersion   string
 }
 
 func NewFromFlags() (*Config, error) {
@@ -40,6 +42,7 @@ func NewFromFlags() (*Config, error) {
 	flag.StringVarP(&config.Kubeconfig, "kubeconfig", "k", envOrString("KUBECONFIG", filepath.Join(home, ".kube", "config")), "path to the kubeconfig file")
 	flag.StringVarP(&config.Output, "output", "o", "text", "output format - [text|json]")
 	flag.VarP(&config.LogLevel, "log-level", "l", "set log level (trace, debug, info, warn, error, fatal, panic, disabled)")
+	flag.StringVarP(&config.TargetVersion, "target-version", "t", "", "target K8s version in major.minor format (autodetected by default)")
 
 	flag.Parse()
 
@@ -51,6 +54,10 @@ func NewFromFlags() (*Config, error) {
 		return nil, fmt.Errorf("failed to validate arguments: %w", err)
 	}
 
+	if err := validateTargetVersion(config.TargetVersion); err != nil {
+		return nil, fmt.Errorf("failed to validate arguments: %w", err)
+	}
+
 	return &config, nil
 }
 
@@ -77,3 +84,19 @@ func validateAdditionalResources(resources []string) error {
 	}
 	return nil
 }
+
+func validateTargetVersion(version string) error {
+	if version != "" {
+		parts := strings.Split(version, ".")
+		if len(parts) < 2 || len(parts) > 3 {
+			return fmt.Errorf("failed to parse target version, expected format is major.minor, got: %s", version)
+		}
+
+		for _, p := range parts {
+			if _, err := strconv.ParseUint(p, 10, 8); err != nil {
+				return fmt.Errorf("failed to parse target version, all parts are expected to be a number, got: %s", p)
+			}
+		}
+	}
+	return nil
+}


Test patch:
diff --git a/cmd/kubent/main_test.go b/cmd/kubent/main_test.go
index b4bcb4d..c977229 100644
--- a/cmd/kubent/main_test.go
+++ b/cmd/kubent/main_test.go
@@ -9,6 +9,7 @@ import (
 
 	"github.com/doitintl/kube-no-trouble/pkg/collector"
 	"github.com/doitintl/kube-no-trouble/pkg/config"
+
 	"github.com/rs/zerolog"
 )
 
@@ -141,3 +142,52 @@ func TestMainExitCodes(t *testing.T) {
 		})
 	}
 }
+
+func TestGetServerVersionNone(t *testing.T) {
+	collectors := []collector.Collector{}
+
+	version, err := getServerVersion(collectors)
+	if err != nil {
+		t.Errorf("Failed to get version with error: %s", err)
+	}
+
+	if version != "" {
+		t.Errorf("Expected no version to be detected, instead got: %s", version)
+	}
+}
+
+func TestGetServerVersionNotSupported(t *testing.T) {
+	collectors := []collector.Collector{}
+
+	fileCollector, err := collector.NewFileCollector(&collector.FileOpts{Filenames: []string{"../../fixtures/deployment-v1beta1.yaml"}})
+	if err != nil {
+		t.Errorf("Failed to create File collector with error: %s", err)
+	}
+
+	collectors = storeCollector(fileCollector, err, collectors)
+
+	version, err := getServerVersion(collectors)
+	if err != nil {
+		t.Errorf("Failed to get version with error: %s", err)
+	}
+
+	if version != "" {
+		t.Errorf("Expected no version to be detected, instead got: %s", version)
+	}
+}
+
+func TestGetServerVersion(t *testing.T) {
+	collectors := []collector.Collector{}
+
+	fc := collector.NewFakeCollector()
+	collectors = storeCollector(fc, nil, collectors)
+
+	version, err := getServerVersion(collectors)
+	if err != nil {
+		t.Errorf("Failed to get version with error: %s", err)
+	}
+
+	if version != collector.FAKE_VERSION {
+		t.Errorf("Expected no version to be detected, instead got: %s", version)
+	}
+}
diff --git a/pkg/collector/cluster_test.go b/pkg/collector/cluster_test.go
index 4e8f60b..cb02e85 100644
--- a/pkg/collector/cluster_test.go
+++ b/pkg/collector/cluster_test.go
@@ -50,7 +50,7 @@ func TestNewClusterCollectorFakeClient(t *testing.T) {
 
 	collector, err := NewClusterCollector(&testOpts, []string{})
 	if err != nil {
-		t.Errorf("failed to create cluster collector from fake client: %s", err)
+		t.Fatalf("failed to create cluster collector from fake client: %s", err)
 	}
 
 	result, err := collector.Get()
diff --git a/pkg/collector/kube_test.go b/pkg/collector/kube_test.go
new file mode 100644
index 0000000..2243e1c
--- /dev/null
+++ b/pkg/collector/kube_test.go
@@ -0,0 +1,56 @@
+package collector
+
+import (
+	"testing"
+
+	"k8s.io/apimachinery/pkg/version"
+	discoveryFake "k8s.io/client-go/discovery/fake"
+	"k8s.io/client-go/kubernetes/fake"
+)
+
+func TestNewKubeCollector(t *testing.T) {
+	clientSet := fake.NewSimpleClientset()
+	col, err := newKubeCollector("", clientSet.Discovery())
+
+	if err != nil {
+		t.Errorf("Failed to create kubeCollector form fake discovery client")
+	}
+	if col == nil {
+		t.Errorf("Should return collector, instrad got nil")
+	}
+
+}
+
+func TestNewKubeCollectorError(t *testing.T) {
+	_, err := newKubeCollector("does-not-exist", nil)
+
+	if err == nil {
+		t.Errorf("Expected to fail with non-existent kubeconfig")
+	}
+}
+
+func TestGetServerVersion(t *testing.T) {
+	expectedMajor := "1"
+	expectedMinor := "2"
+	expectedVersion := expectedMajor + "." + expectedMinor
+
+	clientSet := fake.NewSimpleClientset()
+	clientSet.Discovery().(*discoveryFake.FakeDiscovery).FakedServerVersion = &version.Info{
+		Major: expectedMajor,
+		Minor: expectedMinor,
+	}
+
+	collector, err := newKubeCollector("", clientSet.Discovery())
+	if err != nil {
+		t.Fatalf("failed to create kubeCollector from fake client: %s", err)
+	}
+
+	version, err := collector.GetServerVersion()
+	if err != nil {
+		t.Errorf("Failed to get version with error: %s", err)
+	}
+
+	if version != expectedVersion {
+		t.Errorf("Expected no version to be detected, instead got: %s", version)
+	}
+}
diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go
index 1b366db..e5bbe5c 100644
--- a/pkg/config/config_test.go
+++ b/pkg/config/config_test.go
@@ -152,3 +152,28 @@ func TestValidateAdditionalResourcesFail(t *testing.T) {
 		}
 	}
 }
+
+func TestValidateTargetVersion(t *testing.T) {
+	validVersions := []string{
+		"", "1.16", "1.16.3", "1.2.3",
+	}
+	invalidVersions := []string{
+		"1", "v12.3.4", "v1.2", "1.blah", "nope",
+	}
+
+	for _, v := range validVersions {
+		err := validateTargetVersion(v)
+
+		if err != nil {
+			t.Errorf("Expected %s to fail validation, it failed instead with: %s", v, err)
+		}
+	}
+
+	for _, v := range invalidVersions {
+		err := validateTargetVersion(v)
+
+		if err == nil {
+			t.Errorf("Expected %s to fail validation, it succeeded instead", v)
+		}
+	}
+}