refactor(kubernetes): streamline provider configuration and in-cluster detection (#378)

* refactor(kubernetes): streamline provider configuration and in-cluster detection

- Removed IsInCluster method from Manager and created function scoped to the runtime environment.
  As a method, the implementation was not correct.
  Removed GetAPIServerHost method from Manager which is no used.
- **Temporarily** added an `inCluster` field to the Manager struct but should be eventually removed since it doesn't really make sense to hava a Manager in-cluster or out-of-cluster in the multi-cluster scenario.
- Provider resolution (resolveStrategy) is now clearer, added complete coverage for all scenarios.
- Added additional coverage for provider and manager.

Signed-off-by: Marc Nuri <[email protected]>

* refactor(kubernetes): update NewManager to accept kubeconfig context and simplify manager creation

- Removes Provider.newForContext(context string) method.

Signed-off-by: Marc Nuri <[email protected]>

---------

Signed-off-by: Marc Nuri <[email protected]>

Author: manusa

internal/test/env.go

@@ -0,0 +1,15 @@
+package test
+
+import (
+	"os"
+	"strings"
+)
+
+func RestoreEnv(originalEnv []string) {
+	os.Clearenv()
+	for _, env := range originalEnv {
+		if key, value, found := strings.Cut(env, "="); found {
+			_ = os.Setenv(key, value)
+		}
+	}
+}

pkg/kubernetes/configuration.go

@@ -1,9 +1,9 @@
 package kubernetes
 
 import (
+	"github.com/containers/kubernetes-mcp-server/pkg/config"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/client-go/rest"
-	"k8s.io/client-go/tools/clientcmd"
 	clientcmdapi "k8s.io/client-go/tools/clientcmd/api"
 	"k8s.io/client-go/tools/clientcmd/api/latest"
 )
@@ -22,29 +22,13 @@ var InClusterConfig = func() (*rest.Config, error) {
 	return inClusterConfig, err
 }
 
-// resolveKubernetesConfigurations resolves the required kubernetes configurations and sets them in the Kubernetes struct
-func resolveKubernetesConfigurations(kubernetes *Manager) error {
-	// Always set clientCmdConfig
-	pathOptions := clientcmd.NewDefaultPathOptions()
-	if kubernetes.staticConfig.KubeConfig != "" {
-		pathOptions.LoadingRules.ExplicitPath = kubernetes.staticConfig.KubeConfig
+func IsInCluster(cfg *config.StaticConfig) bool {
+	// Even if running in-cluster, if a kubeconfig is provided, we consider it as out-of-cluster
+	if cfg != nil && cfg.KubeConfig != "" {
+		return false
 	}
-	kubernetes.clientCmdConfig = clientcmd.NewNonInteractiveDeferredLoadingClientConfig(
-		pathOptions.LoadingRules,
-		&clientcmd.ConfigOverrides{ClusterInfo: clientcmdapi.Cluster{Server: ""}})
-	var err error
-	if kubernetes.IsInCluster() {
-		kubernetes.cfg, err = InClusterConfig()
-		if err == nil && kubernetes.cfg != nil {
-			return nil
-		}
-	}
-	// Out of cluster
-	kubernetes.cfg, err = kubernetes.clientCmdConfig.ClientConfig()
-	if kubernetes.cfg != nil && kubernetes.cfg.UserAgent == "" {
-		kubernetes.cfg.UserAgent = rest.DefaultKubernetesUserAgent()
-	}
-	return err
+	restConfig, err := InClusterConfig()
+	return err == nil && restConfig != nil
 }
 
 func (k *Kubernetes) NamespaceOrDefault(namespace string) string {
@@ -54,7 +38,7 @@ func (k *Kubernetes) NamespaceOrDefault(namespace string) string {
 // ConfigurationContextsDefault returns the current context name
 // TODO: Should be moved to the Provider level ?
 func (k *Kubernetes) ConfigurationContextsDefault() (string, error) {
-	if k.manager.IsInCluster() {
+	if k.manager.inCluster {
 		return inClusterKubeConfigDefaultContext, nil
 	}
 	cfg, err := k.manager.clientCmdConfig.RawConfig()
@@ -67,7 +51,7 @@ func (k *Kubernetes) ConfigurationContextsDefault() (string, error) {
 // ConfigurationContextsList returns the list of available context names
 // TODO: Should be moved to the Provider level ?
 func (k *Kubernetes) ConfigurationContextsList() (map[string]string, error) {
-	if k.manager.IsInCluster() {
+	if k.manager.inCluster {
 		return map[string]string{inClusterKubeConfigDefaultContext: ""}, nil
 	}
 	cfg, err := k.manager.clientCmdConfig.RawConfig()
@@ -93,7 +77,7 @@ func (k *Kubernetes) ConfigurationContextsList() (map[string]string, error) {
 func (k *Kubernetes) ConfigurationView(minify bool) (runtime.Object, error) {
 	var cfg clientcmdapi.Config
 	var err error
-	if k.manager.IsInCluster() {
+	if k.manager.inCluster {
 		cfg = *clientcmdapi.NewConfig()
 		cfg.Clusters["cluster"] = &clientcmdapi.Cluster{
 			Server:                k.manager.cfg.Host,

pkg/kubernetes/configuration_test.go

@@ -47,7 +47,7 @@ users:
 			kubeconfig = "` + strings.ReplaceAll(kubeconfigPath, `\`, `\\`) + `"
 		`)))
 		s.Run("without authorization header returns original manager", func() {
-			testManager, err := NewManager(testStaticConfig)
+			testManager, err := NewManager(testStaticConfig, "")
 			s.Require().NoErrorf(err, "failed to create test manager: %v", err)
 			s.T().Cleanup(testManager.Close)
 
@@ -58,7 +58,7 @@ users:
 		})
 
 		s.Run("with invalid authorization header returns original manager", func() {
-			testManager, err := NewManager(testStaticConfig)
+			testManager, err := NewManager(testStaticConfig, "")
 			s.Require().NoErrorf(err, "failed to create test manager: %v", err)
 			s.T().Cleanup(testManager.Close)
 
@@ -70,7 +70,7 @@ users:
 		})
 
 		s.Run("with valid bearer token creates derived manager with correct configuration", func() {
-			testManager, err := NewManager(testStaticConfig)
+			testManager, err := NewManager(testStaticConfig, "")
 			s.Require().NoErrorf(err, "failed to create test manager: %v", err)
 			s.T().Cleanup(testManager.Close)
 
@@ -138,7 +138,7 @@ users:
 		`)))
 
 		s.Run("with no authorization header returns oauth token required error", func() {
-			testManager, err := NewManager(testStaticConfig)
+			testManager, err := NewManager(testStaticConfig, "")
 			s.Require().NoErrorf(err, "failed to create test manager: %v", err)
 			s.T().Cleanup(testManager.Close)
 
@@ -149,7 +149,7 @@ users:
 		})
 
 		s.Run("with invalid authorization header returns oauth token required error", func() {
-			testManager, err := NewManager(testStaticConfig)
+			testManager, err := NewManager(testStaticConfig, "")
 			s.Require().NoErrorf(err, "failed to create test manager: %v", err)
 			s.T().Cleanup(testManager.Close)
 
@@ -161,7 +161,7 @@ users:
 		})
 
 		s.Run("with valid bearer token creates derived manager", func() {
-			testManager, err := NewManager(testStaticConfig)
+			testManager, err := NewManager(testStaticConfig, "")
 			s.Require().NoErrorf(err, "failed to create test manager: %v", err)
 			s.T().Cleanup(testManager.Close)
 

pkg/kubernetes/kubernetes_derived_test.go

@@ -25,6 +25,7 @@ import (
 type Manager struct {
 	cfg                     *rest.Config
 	clientCmdConfig         clientcmd.ClientConfig
+	inCluster               bool
 	discoveryClient         discovery.CachedDiscoveryInterface
 	accessControlClientSet  *AccessControlClientset
 	accessControlRESTMapper *AccessControlRESTMapper
@@ -37,18 +38,37 @@ type Manager struct {
 var _ helm.Kubernetes = (*Manager)(nil)
 var _ Openshift = (*Manager)(nil)
 
-func NewManager(config *config.StaticConfig) (*Manager, error) {
+func NewManager(config *config.StaticConfig, kubeconfigContext string) (*Manager, error) {
 	k8s := &Manager{
 		staticConfig: config,
 	}
-	if err := resolveKubernetesConfigurations(k8s); err != nil {
-		return nil, err
+	pathOptions := clientcmd.NewDefaultPathOptions()
+	if k8s.staticConfig.KubeConfig != "" {
+		pathOptions.LoadingRules.ExplicitPath = k8s.staticConfig.KubeConfig
+	}
+	k8s.clientCmdConfig = clientcmd.NewNonInteractiveDeferredLoadingClientConfig(
+		pathOptions.LoadingRules,
+		&clientcmd.ConfigOverrides{
+			ClusterInfo:    clientcmdapi.Cluster{Server: ""},
+			CurrentContext: kubeconfigContext,
+		})
+	var err error
+	if IsInCluster(k8s.staticConfig) {
+		k8s.cfg, err = InClusterConfig()
+		k8s.inCluster = true
+	} else {
+		k8s.cfg, err = k8s.clientCmdConfig.ClientConfig()
+	}
+	if err != nil || k8s.cfg == nil {
+		return nil, fmt.Errorf("failed to create kubernetes rest config: %v", err)
+	}
+	if k8s.cfg.UserAgent == "" {
+		k8s.cfg.UserAgent = rest.DefaultKubernetesUserAgent()
 	}
 	// TODO: Won't work because not all client-go clients use the shared context (e.g. discovery client uses context.TODO())
 	//k8s.cfg.Wrap(func(original http.RoundTripper) http.RoundTripper {
 	//	return &impersonateRoundTripper{original}
 	//})
-	var err error
 	k8s.accessControlClientSet, err = NewAccessControlClientset(k8s.cfg, k8s.staticConfig)
 	if err != nil {
 		return nil, err
@@ -107,21 +127,6 @@ func (m *Manager) Close() {
 	}
 }
 
-func (m *Manager) GetAPIServerHost() string {
-	if m.cfg == nil {
-		return ""
-	}
-	return m.cfg.Host
-}
-
-func (m *Manager) IsInCluster() bool {
-	if m.staticConfig.KubeConfig != "" {
-		return false
-	}
-	cfg, err := InClusterConfig()
-	return err == nil && cfg != nil
-}
-
 func (m *Manager) configuredNamespace() string {
 	if ns, _, nsErr := m.clientCmdConfig.Namespace(); nsErr == nil {
 		return ns
@@ -221,11 +226,14 @@ func (m *Manager) Derived(ctx context.Context) (*Kubernetes, error) {
 		return &Kubernetes{manager: m}, nil
 	}
 	clientCmdApiConfig.AuthInfos = make(map[string]*clientcmdapi.AuthInfo)
-	derived := &Kubernetes{manager: &Manager{
-		clientCmdConfig: clientcmd.NewDefaultClientConfig(clientCmdApiConfig, nil),
-		cfg:             derivedCfg,
-		staticConfig:    m.staticConfig,
-	}}
+	derived := &Kubernetes{
+		manager: &Manager{
+			clientCmdConfig: clientcmd.NewDefaultClientConfig(clientCmdApiConfig, nil),
+			inCluster:       m.inCluster,
+			cfg:             derivedCfg,
+			staticConfig:    m.staticConfig,
+		},
+	}
 	derived.manager.accessControlClientSet, err = NewAccessControlClientset(derived.manager.cfg, derived.manager.staticConfig)
 	if err != nil {
 		if m.staticConfig.RequireOAuth {