wip:feat(kubernetes)!: simplified Kubernetes client access for toolsets

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

Author: manusa

internal/test/mock_server.go

@@ -3,10 +3,12 @@ package test
 import (
 	"encoding/json"
 	"errors"
+	"fmt"
 	"io"
 	"net/http"
 	"net/http/httptest"
 	"path/filepath"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -186,7 +188,10 @@ WaitForStreams:
 	return ctx, nil
 }
 
-type DiscoveryClientHandler struct{}
+type DiscoveryClientHandler struct {
+	V1Resources []string
+	Groups      []string
+}
 
 var _ http.Handler = (*DiscoveryClientHandler)(nil)
 
@@ -200,17 +205,18 @@ func (h *DiscoveryClientHandler) ServeHTTP(w http.ResponseWriter, req *http.Requ
 	// Request Performed by DiscoveryClient to Kube API (Get API Groups)
 	if req.URL.Path == "/apis" {
 		w.Header().Set("Content-Type", "application/json")
-		_, _ = w.Write([]byte(`{"kind":"APIGroupList","apiVersion":"v1","groups":[
-			{"name":"apps","versions":[{"groupVersion":"apps/v1","version":"v1"}],"preferredVersion":{"groupVersion":"apps/v1","version":"v1"}}
-		]}`))
+		_, _ = fmt.Fprintf(w, `{"kind":"APIGroupList","apiVersion":"v1","groups":[%s]}`, strings.Join(append(h.Groups,
+			`{"name":"apps","versions":[{"groupVersion":"apps/v1","version":"v1"}],"preferredVersion":{"groupVersion":"apps/v1","version":"v1"}}`,
+		), ","))
 		return
 	}
 	// Request Performed by DiscoveryClient to Kube API (Get API Resources)
 	if req.URL.Path == "/api/v1" {
 		w.Header().Set("Content-Type", "application/json")
-		_, _ = w.Write([]byte(`{"kind":"APIResourceList","apiVersion":"v1","resources":[
-			{"name":"pods","singularName":"","namespaced":true,"kind":"Pod","verbs":["get","list","watch","create","update","patch","delete"]}
-		]}`))
+		_, _ = fmt.Fprintf(w, `{"kind":"APIResourceList","apiVersion":"v1","resources":[%s]}`, strings.Join(append(h.V1Resources,
+			`{"name":"nodes","singularName":"","namespaced":false,"kind":"Node","verbs":["get","list","watch"]}`,
+			`{"name":"pods","singularName":"","namespaced":true,"kind":"Pod","verbs":["get","list","watch","create","update","patch","delete"]}`,
+		), ","))
 		return
 	}
 	if req.URL.Path == "/apis/apps/v1" {
@@ -273,12 +279,27 @@ const tokenReviewSuccessful = `
 	}`
 
 type TokenReviewHandler struct {
+	DiscoveryClientHandler
 	TokenReviewed bool
 }
 
 var _ http.Handler = (*TokenReviewHandler)(nil)
 
+func NewTokenReviewHandler() *TokenReviewHandler {
+	trh := &TokenReviewHandler{}
+	trh.DiscoveryClientHandler.Groups = []string{
+		`{"name":"authentication.k8s.io","versions":[{"groupVersion":"authentication.k8s.io/v1","version":"v1"}],"preferredVersion":{"groupVersion":"authentication.k8s.io/v1","version":"v1"}}`,
+	}
+	return trh
+}
+
 func (h *TokenReviewHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) {
+	h.DiscoveryClientHandler.ServeHTTP(w, req)
+	if req.URL.EscapedPath() == "/apis/authentication.k8s.io/v1" {
+		w.Header().Set("Content-Type", "application/json")
+		_, _ = w.Write([]byte(`{"kind":"APIResourceList","apiVersion":"v1","groupVersion":"authentication.k8s.io/v1","resources":[{"name":"tokenreviews","singularName":"","namespaced":false,"kind":"TokenReview","verbs":["create"]}]}`))
+		return
+	}
 	if req.URL.EscapedPath() == "/apis/authentication.k8s.io/v1/tokenreviews" {
 		w.Header().Set("Content-Type", "application/json")
 		_, _ = w.Write([]byte(tokenReviewSuccessful))

pkg/http/http_authorization_test.go

@@ -324,7 +324,7 @@ func (s *AuthorizationSuite) TestAuthorizationRequireOAuthFalse() {
 }
 
 func (s *AuthorizationSuite) TestAuthorizationRawToken() {
-	tokenReviewHandler := &test.TokenReviewHandler{}
+	tokenReviewHandler := test.NewTokenReviewHandler()
 	s.MockServer.Handle(tokenReviewHandler)
 
 	cases := []struct {
@@ -367,7 +367,7 @@ func (s *AuthorizationSuite) TestAuthorizationRawToken() {
 }
 
 func (s *AuthorizationSuite) TestAuthorizationOidcToken() {
-	tokenReviewHandler := &test.TokenReviewHandler{}
+	tokenReviewHandler := test.NewTokenReviewHandler()
 	s.MockServer.Handle(tokenReviewHandler)
 
 	oidcTestServer := NewOidcTestServer(s.T())
@@ -412,7 +412,7 @@ func (s *AuthorizationSuite) TestAuthorizationOidcToken() {
 }
 
 func (s *AuthorizationSuite) TestAuthorizationOidcTokenExchange() {
-	tokenReviewHandler := &test.TokenReviewHandler{}
+	tokenReviewHandler := test.NewTokenReviewHandler()
 	s.MockServer.Handle(tokenReviewHandler)
 
 	oidcTestServer := NewOidcTestServer(s.T())

pkg/kubernetes/accesscontrol_clientset.go

@@ -3,71 +3,103 @@ package kubernetes
 import (
 	"context"
 	"fmt"
+	"net/http"
 
-	authenticationv1api "k8s.io/api/authentication/v1"
-	authorizationv1api "k8s.io/api/authorization/v1"
+	"github.com/containers/kubernetes-mcp-server/pkg/config"
 	v1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/util/httpstream"
 	"k8s.io/client-go/discovery"
+	"k8s.io/client-go/discovery/cached/memory"
+	"k8s.io/client-go/dynamic"
 	"k8s.io/client-go/kubernetes"
 	authenticationv1 "k8s.io/client-go/kubernetes/typed/authentication/v1"
 	authorizationv1 "k8s.io/client-go/kubernetes/typed/authorization/v1"
 	corev1 "k8s.io/client-go/kubernetes/typed/core/v1"
 	"k8s.io/client-go/rest"
+	"k8s.io/client-go/restmapper"
 	"k8s.io/client-go/tools/remotecommand"
 	"k8s.io/metrics/pkg/apis/metrics"
 	metricsv1beta1api "k8s.io/metrics/pkg/apis/metrics/v1beta1"
 	metricsv1beta1 "k8s.io/metrics/pkg/client/clientset/versioned/typed/metrics/v1beta1"
-
-	"github.com/containers/kubernetes-mcp-server/pkg/config"
 )
 
 // AccessControlClientset is a limited clientset delegating interface to the standard kubernetes.Clientset
 // Only a limited set of functions are implemented with a single point of access to the kubernetes API where
 // apiVersion and kinds are checked for allowed access
 type AccessControlClientset struct {
-	cfg             *rest.Config
-	delegate        kubernetes.Interface
-	discoveryClient discovery.DiscoveryInterface
+	cfg *rest.Config
+	kubernetes.Interface
+	discoveryClient discovery.CachedDiscoveryInterface
+	dynamicClient   *dynamic.DynamicClient
+	restMapper      meta.ResettableRESTMapper
 	metricsV1beta1  *metricsv1beta1.MetricsV1beta1Client
-	staticConfig    *config.StaticConfig // TODO: maybe just store the denied resource slice
 }
 
-func (a *AccessControlClientset) DiscoveryClient() discovery.DiscoveryInterface {
+func NewAccessControlClientset(staticConfig *config.StaticConfig, restConfig *rest.Config) (*AccessControlClientset, error) {
+	configShallowCopy := &(*restConfig)
+	acc := &AccessControlClientset{
+		cfg: configShallowCopy,
+	}
+	discoveryClient, err := discovery.NewDiscoveryClientForConfig(acc.cfg)
+	if err != nil {
+		return nil, fmt.Errorf("failed to create discovery client: %v", err)
+	}
+	acc.discoveryClient = memory.NewMemCacheClient(discoveryClient)
+	acc.restMapper = restmapper.NewDeferredDiscoveryRESTMapper(acc.discoveryClient)
+	acc.cfg.Wrap(func(original http.RoundTripper) http.RoundTripper {
+		return &AccessControlRoundTripper{
+			delegate:     original,
+			staticConfig: staticConfig,
+			restMapper:   acc.restMapper,
+		}
+	})
+	acc.Interface, err = kubernetes.NewForConfig(acc.cfg)
+	if err != nil {
+		return nil, err
+	}
+	acc.dynamicClient, err = dynamic.NewForConfig(acc.cfg)
+	if err != nil {
+		return nil, err
+	}
+	acc.metricsV1beta1, err = metricsv1beta1.NewForConfig(acc.cfg)
+	if err != nil {
+		return nil, err
+	}
+	return acc, nil
+}
+
+func (a *AccessControlClientset) DiscoveryClient() discovery.CachedDiscoveryInterface {
 	return a.discoveryClient
 }
 
+func (a *AccessControlClientset) DynamicClient() dynamic.Interface {
+	return a.dynamicClient
+}
+
+func (a *AccessControlClientset) RESTMapper() meta.ResettableRESTMapper {
+	return a.restMapper
+}
+
+// Nodes returns NodeInterface
+// Deprecated: use CoreV1().Nodes() directly
 func (a *AccessControlClientset) Nodes() (corev1.NodeInterface, error) {
-	gvk := &schema.GroupVersionKind{Group: "", Version: "v1", Kind: "Node"}
-	if !isAllowed(a.staticConfig, gvk) {
-		return nil, isNotAllowedError(gvk)
-	}
-	return a.delegate.CoreV1().Nodes(), nil
+	return a.CoreV1().Nodes(), nil
 }
 
 func (a *AccessControlClientset) NodesLogs(ctx context.Context, name string) (*rest.Request, error) {
-	gvk := &schema.GroupVersionKind{Group: "", Version: "v1", Kind: "Node"}
-	if !isAllowed(a.staticConfig, gvk) {
-		return nil, isNotAllowedError(gvk)
-	}
-
-	if _, err := a.delegate.CoreV1().Nodes().Get(ctx, name, metav1.GetOptions{}); err != nil {
+	if _, err := a.CoreV1().Nodes().Get(ctx, name, metav1.GetOptions{}); err != nil {
 		return nil, fmt.Errorf("failed to get node %s: %w", name, err)
 	}
 
 	url := []string{"api", "v1", "nodes", name, "proxy", "logs"}
-	return a.delegate.CoreV1().RESTClient().
+	return a.CoreV1().RESTClient().
 		Get().
 		AbsPath(url...), nil
 }
 
 func (a *AccessControlClientset) NodesMetricses(ctx context.Context, name string, listOptions metav1.ListOptions) (*metrics.NodeMetricsList, error) {
-	gvk := &schema.GroupVersionKind{Group: metrics.GroupName, Version: metricsv1beta1api.SchemeGroupVersion.Version, Kind: "NodeMetrics"}
-	if !isAllowed(a.staticConfig, gvk) {
-		return nil, isNotAllowedError(gvk)
-	}
 	versionedMetrics := &metricsv1beta1api.NodeMetricsList{}
 	var err error
 	if name != "" {
@@ -87,37 +119,26 @@ func (a *AccessControlClientset) NodesMetricses(ctx context.Context, name string
 }
 
 func (a *AccessControlClientset) NodesStatsSummary(ctx context.Context, name string) (*rest.Request, error) {
-	gvk := &schema.GroupVersionKind{Group: "", Version: "v1", Kind: "Node"}
-	if !isAllowed(a.staticConfig, gvk) {
-		return nil, isNotAllowedError(gvk)
-	}
-
-	if _, err := a.delegate.CoreV1().Nodes().Get(ctx, name, metav1.GetOptions{}); err != nil {
+	if _, err := a.CoreV1().Nodes().Get(ctx, name, metav1.GetOptions{}); err != nil {
 		return nil, fmt.Errorf("failed to get node %s: %w", name, err)
 	}
 
 	url := []string{"api", "v1", "nodes", name, "proxy", "stats", "summary"}
-	return a.delegate.CoreV1().RESTClient().
+	return a.CoreV1().RESTClient().
 		Get().
 		AbsPath(url...), nil
 }
 
+// Pods returns PodInterface
+// Deprecated: use CoreV1().Pods(namespace) directly
 func (a *AccessControlClientset) Pods(namespace string) (corev1.PodInterface, error) {
-	gvk := &schema.GroupVersionKind{Group: "", Version: "v1", Kind: "Pod"}
-	if !isAllowed(a.staticConfig, gvk) {
-		return nil, isNotAllowedError(gvk)
-	}
-	return a.delegate.CoreV1().Pods(namespace), nil
+	return a.CoreV1().Pods(namespace), nil
 }
 
 func (a *AccessControlClientset) PodsExec(namespace, name string, podExecOptions *v1.PodExecOptions) (remotecommand.Executor, error) {
-	gvk := &schema.GroupVersionKind{Group: "", Version: "v1", Kind: "Pod"}
-	if !isAllowed(a.staticConfig, gvk) {
-		return nil, isNotAllowedError(gvk)
-	}
 	// Compute URL
 	// https://github.com/kubernetes/kubectl/blob/5366de04e168bcbc11f5e340d131a9ca8b7d0df4/pkg/cmd/exec/exec.go#L382-L397
-	execRequest := a.delegate.CoreV1().RESTClient().
+	execRequest := a.CoreV1().RESTClient().
 		Post().
 		Resource("pods").
 		Namespace(namespace).
@@ -138,10 +159,6 @@ func (a *AccessControlClientset) PodsExec(namespace, name string, podExecOptions
 }
 
 func (a *AccessControlClientset) PodsMetricses(ctx context.Context, namespace, name string, listOptions metav1.ListOptions) (*metrics.PodMetricsList, error) {
-	gvk := &schema.GroupVersionKind{Group: metrics.GroupName, Version: metricsv1beta1api.SchemeGroupVersion.Version, Kind: "PodMetrics"}
-	if !isAllowed(a.staticConfig, gvk) {
-		return nil, isNotAllowedError(gvk)
-	}
 	versionedMetrics := &metricsv1beta1api.PodMetricsList{}
 	var err error
 	if name != "" {
@@ -160,45 +177,20 @@ func (a *AccessControlClientset) PodsMetricses(ctx context.Context, namespace, n
 	return convertedMetrics, metricsv1beta1api.Convert_v1beta1_PodMetricsList_To_metrics_PodMetricsList(versionedMetrics, convertedMetrics, nil)
 }
 
+// Services returns ServiceInterface
+// Deprecated: use CoreV1().Services(namespace) directly
 func (a *AccessControlClientset) Services(namespace string) (corev1.ServiceInterface, error) {
-	gvk := &schema.GroupVersionKind{Group: "", Version: "v1", Kind: "Service"}
-	if !isAllowed(a.staticConfig, gvk) {
-		return nil, isNotAllowedError(gvk)
-	}
-	return a.delegate.CoreV1().Services(namespace), nil
+	return a.CoreV1().Services(namespace), nil
 }
 
+// SelfSubjectAccessReviews returns SelfSubjectAccessReviewInterface
+// Deprecated: use AuthorizationV1().SelfSubjectAccessReviews() directly
 func (a *AccessControlClientset) SelfSubjectAccessReviews() (authorizationv1.SelfSubjectAccessReviewInterface, error) {
-	gvk := &schema.GroupVersionKind{Group: authorizationv1api.GroupName, Version: authorizationv1api.SchemeGroupVersion.Version, Kind: "SelfSubjectAccessReview"}
-	if !isAllowed(a.staticConfig, gvk) {
-		return nil, isNotAllowedError(gvk)
-	}
-	return a.delegate.AuthorizationV1().SelfSubjectAccessReviews(), nil
+	return a.AuthorizationV1().SelfSubjectAccessReviews(), nil
 }
 
 // TokenReview returns TokenReviewInterface
+// Deprecated: use AuthenticationV1().TokenReviews() directly
 func (a *AccessControlClientset) TokenReview() (authenticationv1.TokenReviewInterface, error) {
-	gvk := &schema.GroupVersionKind{Group: authenticationv1api.GroupName, Version: authorizationv1api.SchemeGroupVersion.Version, Kind: "TokenReview"}
-	if !isAllowed(a.staticConfig, gvk) {
-		return nil, isNotAllowedError(gvk)
-	}
-	return a.delegate.AuthenticationV1().TokenReviews(), nil
-}
-
-func NewAccessControlClientset(cfg *rest.Config, staticConfig *config.StaticConfig) (*AccessControlClientset, error) {
-	clientSet, err := kubernetes.NewForConfig(cfg)
-	if err != nil {
-		return nil, err
-	}
-	metricsClient, err := metricsv1beta1.NewForConfig(cfg)
-	if err != nil {
-		return nil, err
-	}
-	return &AccessControlClientset{
-		cfg:             cfg,
-		delegate:        clientSet,
-		discoveryClient: clientSet.DiscoveryClient,
-		metricsV1beta1:  metricsClient,
-		staticConfig:    staticConfig,
-	}, nil
+	return a.AuthenticationV1().TokenReviews(), nil
 }

pkg/kubernetes/kubernetes_derived_test.go

@@ -122,11 +122,7 @@ users:
 			s.Run("derived manager has initialized clients", func() {
 				// Verify that the derived manager has proper clients initialized
 				s.NotNilf(derived.manager.accessControlClientSet, "expected accessControlClientSet to be initialized")
-				s.Equalf(testStaticConfig, derived.manager.accessControlClientSet.staticConfig, "staticConfig not properly wired to derived manager")
-				s.NotNilf(derived.manager.discoveryClient, "expected discoveryClient to be initialized")
-				s.NotNilf(derived.manager.restMapper, "expected accessControlRESTMapper to be initialized")
 				//s.Equalf(testStaticConfig, derived.manager.re.staticConfig, "staticConfig not properly wired to derived manager")
-				s.NotNilf(derived.manager.dynamicClient, "expected dynamicClient to be initialized")
 			})
 		})
 	})