Repository: doitintl/kube-no-trouble (go)

Issue:
Helm - namespace incorrectly assigned for Helm cluster-level resources
We incorrectly assign Chart namespace even for Cluster-level resources. These should not have a namespace:

```
__________________________________________________________________________________________
>>> Deprecated APIs removed in 1.22 <<<
------------------------------------------------------------------------------------------
KIND                       NAMESPACE     NAME                                             API_VERSION                         REPLACE_WITH (SINCE)
...
...
StorageClass               default       slow                                             storage.k8s.io/v1beta1              storage.k8s.io/v1 (1.6+)
```

Pull request:
fix: Helm - only default to release namespace for namespaced resources
This uses discovery client to check if given type of resource is namespaced. Discovery client should be cached, so it's fine to call ServerResourcesForGroupVersion() multiple times.

Fixes #195

Patch:
diff --git a/pkg/collector/helm.go b/pkg/collector/helm.go
index db4cdfc..beaab21 100644
--- a/pkg/collector/helm.go
+++ b/pkg/collector/helm.go
@@ -2,12 +2,16 @@ package collector
 
 import (
 	"fmt"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/client-go/discovery"
 
 	"github.com/ghodss/yaml"
+	"github.com/rs/zerolog/log"
 	"helm.sh/helm/v3/pkg/releaseutil"
 )
 
-func parseManifests(manifest string, defaultNamespace string) ([]map[string]interface{}, error) {
+func parseManifests(manifest string, defaultNamespace string, discoveryClient discovery.DiscoveryInterface) ([]map[string]interface{}, error) {
 	var results []map[string]interface{}
 
 	manifests := releaseutil.SplitManifests(manifest)
@@ -20,7 +24,7 @@ func parseManifests(manifest string, defaultNamespace string) ([]map[string]inte
 			return nil, err
 		}
 
-		fixNamespace(&manifest, defaultNamespace)
+		fixNamespace(&unstructured.Unstructured{Object: manifest}, defaultNamespace, discoveryClient)
 
 		results = append(results, manifest)
 	}
@@ -28,14 +32,27 @@ func parseManifests(manifest string, defaultNamespace string) ([]map[string]inte
 	return results, nil
 }
 
-func fixNamespace(manifest *map[string]interface{}, defaultNamespace string) {
+func fixNamespace(resource *unstructured.Unstructured, defaultNamespace string, discoveryClient discovery.DiscoveryInterface) {
 	// Default to the release namespace if the manifest doesn't have the namespace set
-	if meta, ok := (*manifest)["metadata"]; ok {
-		switch v := meta.(type) {
-		case map[string]interface{}:
-			if val, ok := v["namespace"]; !ok || val == nil {
-				v["namespace"] = defaultNamespace
-			}
+	if resource.GetNamespace() == "" && isResourceNamespaced(discoveryClient, resource.GroupVersionKind()) {
+		resource.SetNamespace(defaultNamespace)
+	}
+}
+
+func isResourceNamespaced(discoveryClient discovery.DiscoveryInterface, gvk schema.GroupVersionKind) bool {
+	rs, err := discoveryClient.ServerResourcesForGroupVersion(gvk.GroupVersion().String())
+	// It seems discovery client fails with error if resource is not found
+	// this can happen, but is not fatal, we should notify user but continue
+	if err != nil {
+		log.Warn().Msgf("failed to discover supported resources for %s: %v", gvk.GroupVersion(), err)
+		return false
+	}
+
+	for _, r := range rs.APIResources {
+		if r.Kind == gvk.Kind {
+			return r.Namespaced
 		}
 	}
+
+	return false
 }
diff --git a/pkg/collector/helm2.go b/pkg/collector/helm2.go
index 6ee383c..aa2c7e1 100644
--- a/pkg/collector/helm2.go
+++ b/pkg/collector/helm2.go
@@ -68,7 +68,7 @@ func (c *HelmV2Collector) Get() ([]map[string]interface{}, error) {
 	var results []map[string]interface{}
 
 	for _, r := range releases {
-		if manifests, err := parseManifests(r.Manifest, r.Namespace); err != nil {
+		if manifests, err := parseManifests(r.Manifest, r.Namespace, c.discoveryClient); err != nil {
 			log.Warn().Msgf("failed to parse release %s/%s: %v", r.Namespace, r.Name, err)
 		} else {
 			results = append(results, manifests...)
diff --git a/pkg/collector/helm3.go b/pkg/collector/helm3.go
index 44df134..17f4aee 100644
--- a/pkg/collector/helm3.go
+++ b/pkg/collector/helm3.go
@@ -67,7 +67,7 @@ func (c *HelmV3Collector) Get() ([]map[string]interface{}, error) {
 	var results []map[string]interface{}
 
 	for _, r := range releases {
-		if manifests, err := parseManifests(r.Manifest, r.Namespace); err != nil {
+		if manifests, err := parseManifests(r.Manifest, r.Namespace, c.discoveryClient); err != nil {
 			log.Warn().Msgf("failed to parse release %s/%s: %v", r.Namespace, r.Name, err)
 		} else {
 			results = append(results, manifests...)


Test patch:
diff --git a/pkg/collector/helm_test.go b/pkg/collector/helm_test.go
index ddb4ba0..315828f 100644
--- a/pkg/collector/helm_test.go
+++ b/pkg/collector/helm_test.go
@@ -1,10 +1,24 @@
 package collector
 
 import (
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"reflect"
 	"testing"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/client-go/kubernetes/fake"
 )
 
+var FAKE_API_RESOURCES = &metav1.APIResourceList{
+	GroupVersion: "v1",
+	APIResources: []metav1.APIResource{
+		{Name: "pods", Namespaced: true, Kind: "Pod"},
+		{Name: "services", Namespaced: true, Kind: "Service"},
+		{Name: "namespaces", Namespaced: false, Kind: "Namespace"},
+	},
+}
+
 func TestSplitManifests(t *testing.T) {
 	testCases := []struct {
 		name     string
@@ -24,7 +38,8 @@ func TestSplitManifests(t *testing.T) {
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
 
-			manifests, err := parseManifests(tc.input, "default")
+			fcs := fake.NewSimpleClientset()
+			manifests, err := parseManifests(tc.input, "default", fcs.Discovery())
 
 			if err != nil {
 				t.Fatalf("Expected to successfully parse manifests: %v", err)
@@ -45,15 +60,15 @@ func TestFixNamespace(t *testing.T) {
 		expected string                 // kinds of objects
 	}{
 		{"present",
-			map[string]interface{}{"metadata": map[string]interface{}{"namespace": "some-namespace"}},
+			map[string]interface{}{"apiVersion": "v1", "kind": "Pod", "metadata": map[string]interface{}{"namespace": "some-namespace"}},
 			"some-namespace",
 		},
 		{"missing",
-			map[string]interface{}{"metadata": map[string]interface{}{}},
+			map[string]interface{}{"apiVersion": "v1", "kind": "Pod", "metadata": map[string]interface{}{}},
 			"default-namespace",
 		},
 		{"nil",
-			map[string]interface{}{"metadata": map[string]interface{}{"namespace": nil}},
+			map[string]interface{}{"apiVersion": "v1", "kind": "Pod", "metadata": map[string]interface{}{"namespace": nil}},
 			"default-namespace",
 		},
 	}
@@ -61,7 +76,9 @@ func TestFixNamespace(t *testing.T) {
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
 
-			fixNamespace(&tc.input, tc.expected)
+			fcs := fake.NewSimpleClientset()
+			fcs.Resources = []*metav1.APIResourceList{FAKE_API_RESOURCES}
+			fixNamespace(&unstructured.Unstructured{tc.input}, tc.expected, fcs.Discovery())
 
 			meta, ok := tc.input["metadata"]
 			if !ok {
@@ -85,3 +102,29 @@ func TestFixNamespace(t *testing.T) {
 		})
 	}
 }
+
+func Test_isResourceNamespaced(t *testing.T) {
+	tests := []struct {
+		name string
+		gvk  string
+		want bool
+	}{
+		{"true-pod", "Pod.v1.", true},
+		{"false-namespace", "Namespace.v1.", false},
+		{"false-non-existent", "XXX.v1.", false},
+	}
+
+	fcs := fake.NewSimpleClientset()
+	fcs.Resources = []*metav1.APIResourceList{FAKE_API_RESOURCES}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			gvk, _ := schema.ParseKindArg(tt.gvk)
+
+			got := isResourceNamespaced(fcs.Discovery(), *gvk)
+			if got != tt.want {
+				t.Errorf("isResourceNamespaced() got = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}