review(kubernetes): force usage of Derived kubernetes

Addresses comment by ardaguclu

Author: manusa

pkg/kubernetes/configuration.go

@@ -21,7 +21,7 @@ var InClusterConfig = func() (*rest.Config, error) {
 }
 
 // resolveKubernetesConfigurations resolves the required kubernetes configurations and sets them in the Kubernetes struct
-func resolveKubernetesConfigurations(kubernetes *kubernetes) error {
+func resolveKubernetesConfigurations(kubernetes *Manager) error {
 	// Always set clientCmdConfig
 	pathOptions := clientcmd.NewDefaultPathOptions()
 	if kubernetes.Kubeconfig != "" {
@@ -45,56 +45,60 @@ func resolveKubernetesConfigurations(kubernetes *kubernetes) error {
 	return err
 }
 
-func (k *kubernetes) IsInCluster() bool {
-	if k.Kubeconfig != "" {
+func (m *Manager) IsInCluster() bool {
+	if m.Kubeconfig != "" {
 		return false
 	}
 	cfg, err := InClusterConfig()
 	return err == nil && cfg != nil
 }
 
-func (k *kubernetes) configuredNamespace() string {
-	if ns, _, nsErr := k.clientCmdConfig.Namespace(); nsErr == nil {
+func (m *Manager) configuredNamespace() string {
+	if ns, _, nsErr := m.clientCmdConfig.Namespace(); nsErr == nil {
 		return ns
 	}
 	return ""
 }
 
-func (k *kubernetes) NamespaceOrDefault(namespace string) string {
+func (m *Manager) NamespaceOrDefault(namespace string) string {
 	if namespace == "" {
-		return k.configuredNamespace()
+		return m.configuredNamespace()
 	}
 	return namespace
 }
 
+func (k *Kubernetes) NamespaceOrDefault(namespace string) string {
+	return k.manager.NamespaceOrDefault(namespace)
+}
+
 // ToRESTConfig returns the rest.Config object (genericclioptions.RESTClientGetter)
-func (k *kubernetes) ToRESTConfig() (*rest.Config, error) {
-	return k.cfg, nil
+func (m *Manager) ToRESTConfig() (*rest.Config, error) {
+	return m.cfg, nil
 }
 
 // ToRawKubeConfigLoader returns the clientcmd.ClientConfig object (genericclioptions.RESTClientGetter)
-func (k *kubernetes) ToRawKubeConfigLoader() clientcmd.ClientConfig {
-	return k.clientCmdConfig
+func (m *Manager) ToRawKubeConfigLoader() clientcmd.ClientConfig {
+	return m.clientCmdConfig
 }
 
-func (k *kubernetes) ConfigurationView(minify bool) (runtime.Object, error) {
+func (m *Manager) ConfigurationView(minify bool) (runtime.Object, error) {
 	var cfg clientcmdapi.Config
 	var err error
-	if k.IsInCluster() {
+	if m.IsInCluster() {
 		cfg = *clientcmdapi.NewConfig()
 		cfg.Clusters["cluster"] = &clientcmdapi.Cluster{
-			Server:                k.cfg.Host,
-			InsecureSkipTLSVerify: k.cfg.Insecure,
+			Server:                m.cfg.Host,
+			InsecureSkipTLSVerify: m.cfg.Insecure,
 		}
 		cfg.AuthInfos["user"] = &clientcmdapi.AuthInfo{
-			Token: k.cfg.BearerToken,
+			Token: m.cfg.BearerToken,
 		}
 		cfg.Contexts["context"] = &clientcmdapi.Context{
 			Cluster:  "cluster",
 			AuthInfo: "user",
 		}
 		cfg.CurrentContext = "context"
-	} else if cfg, err = k.clientCmdConfig.RawConfig(); err != nil {
+	} else if cfg, err = m.clientCmdConfig.RawConfig(); err != nil {
 		return nil, err
 	}
 	if minify {

pkg/kubernetes/configuration_test.go

@@ -12,10 +12,10 @@ import (
 
 func TestKubernetes_IsInCluster(t *testing.T) {
 	t.Run("with explicit kubeconfig", func(t *testing.T) {
-		k := kubernetes{
+		m := Manager{
 			Kubeconfig: "kubeconfig",
 		}
-		if k.IsInCluster() {
+		if m.IsInCluster() {
 			t.Errorf("expected not in cluster, got in cluster")
 		}
 	})
@@ -27,10 +27,10 @@ func TestKubernetes_IsInCluster(t *testing.T) {
 		defer func() {
 			InClusterConfig = originalFunction
 		}()
-		k := kubernetes{
+		m := Manager{
 			Kubeconfig: "",
 		}
-		if !k.IsInCluster() {
+		if !m.IsInCluster() {
 			t.Errorf("expected in cluster, got not in cluster")
 		}
 	})
@@ -42,10 +42,10 @@ func TestKubernetes_IsInCluster(t *testing.T) {
 		defer func() {
 			InClusterConfig = originalFunction
 		}()
-		k := kubernetes{
+		m := Manager{
 			Kubeconfig: "",
 		}
-		if k.IsInCluster() {
+		if m.IsInCluster() {
 			t.Errorf("expected not in cluster, got in cluster")
 		}
 	})
@@ -57,10 +57,10 @@ func TestKubernetes_IsInCluster(t *testing.T) {
 		defer func() {
 			InClusterConfig = originalFunction
 		}()
-		k := kubernetes{
+		m := Manager{
 			Kubeconfig: "",
 		}
-		if k.IsInCluster() {
+		if m.IsInCluster() {
 			t.Errorf("expected not in cluster, got in cluster")
 		}
 	})
@@ -72,8 +72,8 @@ func TestKubernetes_ResolveKubernetesConfigurations_Explicit(t *testing.T) {
 			t.Skip("Skipping test on non-linux platforms")
 		}
 		tempDir := t.TempDir()
-		k := kubernetes{Kubeconfig: path.Join(tempDir, "config")}
-		err := resolveKubernetesConfigurations(&k)
+		m := Manager{Kubeconfig: path.Join(tempDir, "config")}
+		err := resolveKubernetesConfigurations(&m)
 		if err == nil {
 			t.Errorf("expected error, got nil")
 		}
@@ -90,8 +90,8 @@ func TestKubernetes_ResolveKubernetesConfigurations_Explicit(t *testing.T) {
 		if err := os.WriteFile(kubeconfigPath, []byte(""), 0644); err != nil {
 			t.Fatalf("failed to create kubeconfig file: %v", err)
 		}
-		k := kubernetes{Kubeconfig: kubeconfigPath}
-		err := resolveKubernetesConfigurations(&k)
+		m := Manager{Kubeconfig: kubeconfigPath}
+		err := resolveKubernetesConfigurations(&m)
 		if err == nil {
 			t.Errorf("expected error, got nil")
 		}
@@ -123,16 +123,16 @@ users:
 		if err := os.WriteFile(kubeconfigPath, []byte(kubeconfigContent), 0644); err != nil {
 			t.Fatalf("failed to create kubeconfig file: %v", err)
 		}
-		k := kubernetes{Kubeconfig: kubeconfigPath}
-		err := resolveKubernetesConfigurations(&k)
+		m := Manager{Kubeconfig: kubeconfigPath}
+		err := resolveKubernetesConfigurations(&m)
 		if err != nil {
 			t.Fatalf("expected no error, got %v", err)
 		}
-		if k.cfg == nil {
+		if m.cfg == nil {
 			t.Errorf("expected non-nil config, got nil")
 		}
-		if k.cfg.Host != "https://example.com" {
-			t.Errorf("expected host https://example.com, got %s", k.cfg.Host)
+		if m.cfg.Host != "https://example.com" {
+			t.Errorf("expected host https://example.com, got %s", m.cfg.Host)
 		}
 	})
 }

pkg/kubernetes/events.go

@@ -9,7 +9,7 @@ import (
 	"strings"
 )
 
-func (k *kubernetes) EventsList(ctx context.Context, namespace string) ([]map[string]any, error) {
+func (k *Kubernetes) EventsList(ctx context.Context, namespace string) ([]map[string]any, error) {
 	var eventMap []map[string]any
 	raw, err := k.ResourcesList(ctx, &schema.GroupVersionKind{
 		Group: "", Version: "v1", Kind: "Event",

pkg/kubernetes/kubernetes.go

@@ -6,9 +6,7 @@ import (
 	"github.com/manusa/kubernetes-mcp-server/pkg/helm"
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/meta"
-	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
-	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/client-go/discovery"
 	"k8s.io/client-go/discovery/cached/memory"
 	"k8s.io/client-go/dynamic"
@@ -19,7 +17,6 @@ import (
 	"k8s.io/client-go/tools/clientcmd"
 	clientcmdapi "k8s.io/client-go/tools/clientcmd/api"
 	"k8s.io/klog/v2"
-	"k8s.io/metrics/pkg/apis/metrics"
 	"strings"
 )
 
@@ -29,36 +26,11 @@ const (
 
 type CloseWatchKubeConfig func() error
 
-type Kubernetes interface {
-	WatchKubeConfig(onKubeConfigChange func() error)
-	Close()
-	Derived(ctx context.Context) DerivedKubernetes
-	ConfigurationView(minify bool) (runtime.Object, error)
-	IsOpenShift(ctx context.Context) bool
+type Kubernetes struct {
+	manager *Manager
 }
 
-type DerivedKubernetes interface {
-	IsOpenShift(ctx context.Context) bool
-	CacheInvalidate()
-	NewHelm() *helm.Helm
-	EventsList(ctx context.Context, namespace string) ([]map[string]any, error)
-	NamespacesList(ctx context.Context, options ResourceListOptions) (runtime.Unstructured, error)
-	PodsListInAllNamespaces(ctx context.Context, options ResourceListOptions) (runtime.Unstructured, error)
-	PodsListInNamespace(ctx context.Context, namespace string, options ResourceListOptions) (runtime.Unstructured, error)
-	PodsGet(ctx context.Context, namespace, name string) (*unstructured.Unstructured, error)
-	PodsDelete(ctx context.Context, namespace, name string) (string, error)
-	PodsLog(ctx context.Context, namespace, name, container string) (string, error)
-	PodsRun(ctx context.Context, namespace, name, image string, port int32) ([]*unstructured.Unstructured, error)
-	PodsTop(ctx context.Context, options PodsTopOptions) (*metrics.PodMetricsList, error)
-	PodsExec(ctx context.Context, namespace, name, container string, command []string) (string, error)
-	ProjectsList(ctx context.Context, options ResourceListOptions) (runtime.Unstructured, error)
-	ResourcesList(ctx context.Context, gvk *schema.GroupVersionKind, namespace string, options ResourceListOptions) (runtime.Unstructured, error)
-	ResourcesGet(ctx context.Context, gvk *schema.GroupVersionKind, namespace, name string) (*unstructured.Unstructured, error)
-	ResourcesCreateOrUpdate(ctx context.Context, resource string) ([]*unstructured.Unstructured, error)
-	ResourcesDelete(ctx context.Context, gvk *schema.GroupVersionKind, namespace, name string) error
-}
-
-type kubernetes struct {
+type Manager struct {
 	// Kubeconfig path override
 	Kubeconfig                  string
 	cfg                         *rest.Config
@@ -72,8 +44,8 @@ type kubernetes struct {
 	dynamicClient               *dynamic.DynamicClient
 }
 
-func NewKubernetes(kubeconfig string) (Kubernetes, error) {
-	k8s := &kubernetes{
+func NewKubernetes(kubeconfig string) (*Manager, error) {
+	k8s := &Manager{
 		Kubeconfig: kubeconfig,
 	}
 	if err := resolveKubernetesConfigurations(k8s); err != nil {
@@ -102,11 +74,11 @@ func NewKubernetes(kubeconfig string) (Kubernetes, error) {
 	return k8s, nil
 }
 
-func (k *kubernetes) WatchKubeConfig(onKubeConfigChange func() error) {
-	if k.clientCmdConfig == nil {
+func (m *Manager) WatchKubeConfig(onKubeConfigChange func() error) {
+	if m.clientCmdConfig == nil {
 		return
 	}
-	kubeConfigFiles := k.clientCmdConfig.ConfigAccess().GetLoadingPrecedence()
+	kubeConfigFiles := m.clientCmdConfig.ConfigAccess().GetLoadingPrecedence()
 	if len(kubeConfigFiles) == 0 {
 		return
 	}
@@ -132,33 +104,33 @@ func (k *kubernetes) WatchKubeConfig(onKubeConfigChange func() error) {
 			}
 		}
 	}()
-	if k.CloseWatchKubeConfig != nil {
-		_ = k.CloseWatchKubeConfig()
+	if m.CloseWatchKubeConfig != nil {
+		_ = m.CloseWatchKubeConfig()
 	}
-	k.CloseWatchKubeConfig = watcher.Close
+	m.CloseWatchKubeConfig = watcher.Close
 }
 
-func (k *kubernetes) Close() {
-	if k.CloseWatchKubeConfig != nil {
-		_ = k.CloseWatchKubeConfig()
+func (m *Manager) Close() {
+	if m.CloseWatchKubeConfig != nil {
+		_ = m.CloseWatchKubeConfig()
 	}
 }
 
-func (k *kubernetes) ToDiscoveryClient() (discovery.CachedDiscoveryInterface, error) {
-	return k.discoveryClient, nil
+func (m *Manager) ToDiscoveryClient() (discovery.CachedDiscoveryInterface, error) {
+	return m.discoveryClient, nil
 }
 
-func (k *kubernetes) ToRESTMapper() (meta.RESTMapper, error) {
-	return k.deferredDiscoveryRESTMapper, nil
+func (m *Manager) ToRESTMapper() (meta.RESTMapper, error) {
+	return m.deferredDiscoveryRESTMapper, nil
 }
 
-func (k *kubernetes) Derived(ctx context.Context) DerivedKubernetes {
+func (m *Manager) Derived(ctx context.Context) *Kubernetes {
 	authorization, ok := ctx.Value(AuthorizationHeader).(string)
 	if !ok || !strings.HasPrefix(authorization, "Bearer ") {
-		return k
+		return &Kubernetes{manager: m}
 	}
 	klog.V(5).Infof("%s header found (Bearer), using provided bearer token", AuthorizationHeader)
-	derivedCfg := rest.CopyConfig(k.cfg)
+	derivedCfg := rest.CopyConfig(m.cfg)
 	derivedCfg.BearerToken = strings.TrimPrefix(authorization, "Bearer ")
 	derivedCfg.BearerTokenFile = ""
 	derivedCfg.Username = ""
@@ -167,41 +139,42 @@ func (k *kubernetes) Derived(ctx context.Context) DerivedKubernetes {
 	derivedCfg.AuthConfigPersister = nil
 	derivedCfg.ExecProvider = nil
 	derivedCfg.Impersonate = rest.ImpersonationConfig{}
-	clientCmdApiConfig, err := k.clientCmdConfig.RawConfig()
+	clientCmdApiConfig, err := m.clientCmdConfig.RawConfig()
 	if err != nil {
-		return k
+		return &Kubernetes{manager: m}
 	}
 	clientCmdApiConfig.AuthInfos = make(map[string]*clientcmdapi.AuthInfo)
-	derived := &kubernetes{
-		Kubeconfig:      k.Kubeconfig,
+	derived := &Kubernetes{manager: &Manager{
+		Kubeconfig:      m.Kubeconfig,
 		clientCmdConfig: clientcmd.NewDefaultClientConfig(clientCmdApiConfig, nil),
 		cfg:             derivedCfg,
-		scheme:          k.scheme,
-		parameterCodec:  k.parameterCodec,
-	}
-	derived.clientSet, err = clientgokubernetes.NewForConfig(derived.cfg)
+		scheme:          m.scheme,
+		parameterCodec:  m.parameterCodec,
+	}}
+	derived.manager.clientSet, err = clientgokubernetes.NewForConfig(derived.manager.cfg)
 	if err != nil {
-		return k
+		return &Kubernetes{manager: m}
 	}
-	derived.discoveryClient = memory.NewMemCacheClient(discovery.NewDiscoveryClient(derived.clientSet.CoreV1().RESTClient()))
-	derived.deferredDiscoveryRESTMapper = restmapper.NewDeferredDiscoveryRESTMapper(derived.discoveryClient)
-	derived.dynamicClient, err = dynamic.NewForConfig(derived.cfg)
+	derived.manager.discoveryClient = memory.NewMemCacheClient(discovery.NewDiscoveryClient(derived.manager.clientSet.CoreV1().RESTClient()))
+	derived.manager.deferredDiscoveryRESTMapper = restmapper.NewDeferredDiscoveryRESTMapper(derived.manager.discoveryClient)
+	derived.manager.dynamicClient, err = dynamic.NewForConfig(derived.manager.cfg)
 	if err != nil {
-		return k
+		return &Kubernetes{manager: m}
 	}
 	return derived
 }
 
-func (k *kubernetes) CacheInvalidate() {
-	if k.discoveryClient != nil {
-		k.discoveryClient.Invalidate()
+// TODO: check test to see why cache isn't getting invalidated automatically https://github.com/manusa/kubernetes-mcp-server/pull/125#discussion_r2152194784
+func (k *Kubernetes) CacheInvalidate() {
+	if k.manager.discoveryClient != nil {
+		k.manager.discoveryClient.Invalidate()
 	}
-	if k.deferredDiscoveryRESTMapper != nil {
-		k.deferredDiscoveryRESTMapper.Reset()
+	if k.manager.deferredDiscoveryRESTMapper != nil {
+		k.manager.deferredDiscoveryRESTMapper.Reset()
 	}
 }
 
-func (k *kubernetes) NewHelm() *helm.Helm {
+func (k *Kubernetes) NewHelm() *helm.Helm {
 	// This is a derived Kubernetes, so it already has the Helm initialized
-	return helm.NewHelm(k)
+	return helm.NewHelm(k.manager)
 }