review(toolsets): align kiali implementation

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

pkg/kiali/kiali.go

@@ -5,32 +5,41 @@ import (
 	"crypto/tls"
 	"fmt"
 	"io"
-	"k8s.io/klog/v2"
 	"net/http"
 	"net/url"
 	"strings"
+
+	"github.com/containers/kubernetes-mcp-server/pkg/config"
+	"k8s.io/client-go/rest"
+	"k8s.io/klog/v2"
 )
 
 type Kiali struct {
-	manager *Manager
+	bearerToken   string
+	kialiURL      string
+	kialiInsecure bool
 }
 
-func (m *Manager) GetKiali() *Kiali {
-	return &Kiali{manager: m}
-}
-
-func (k *Kiali) GetKiali() *Kiali {
-	return k
+// NewKiali creates a new Kiali instance
+func NewKiali(config *config.StaticConfig, kubernetes *rest.Config) *Kiali {
+	kiali := &Kiali{bearerToken: kubernetes.BearerToken}
+	if cfg, ok := config.GetToolsetConfig("kiali"); ok {
+		if kc, ok := cfg.(*Config); ok && kc != nil {
+			kiali.kialiURL = kc.Url
+			kiali.kialiInsecure = kc.Insecure
+		}
+	}
+	return kiali
 }
 
 // validateAndGetURL validates the Kiali client configuration and returns the full URL
 // by safely concatenating the base URL with the provided endpoint, avoiding duplicate
 // or missing slashes regardless of trailing/leading slashes.
 func (k *Kiali) validateAndGetURL(endpoint string) (string, error) {
-	if k == nil || k.manager == nil || k.manager.KialiURL == "" {
+	if k == nil || k.kialiURL == "" {
 		return "", fmt.Errorf("kiali client not initialized")
 	}
-	baseStr := strings.TrimSpace(k.manager.KialiURL)
+	baseStr := strings.TrimSpace(k.kialiURL)
 	if baseStr == "" {
 		return "", fmt.Errorf("kiali server URL not configured")
 	}
@@ -52,7 +61,7 @@ func (k *Kiali) createHTTPClient() *http.Client {
 	return &http.Client{
 		Transport: &http.Transport{
 			TLSClientConfig: &tls.Config{
-				InsecureSkipVerify: k.manager.KialiInsecure,
+				InsecureSkipVerify: k.kialiInsecure,
 			},
 		},
 	}
@@ -62,10 +71,10 @@ func (k *Kiali) createHTTPClient() *http.Client {
 // Kiali client is currently configured to use (Bearer <token>), or empty
 // if no bearer token is configured.
 func (k *Kiali) authorizationHeader() string {
-	if k == nil || k.manager == nil {
+	if k == nil {
 		return ""
 	}
-	token := strings.TrimSpace(k.manager.BearerToken)
+	token := strings.TrimSpace(k.bearerToken)
 	if token == "" {
 		return ""
 	}

pkg/kiali/kiali_test.go

@@ -1,79 +1,126 @@
 package kiali
 
 import (
-	"context"
+	"fmt"
 	"net/http"
-	"net/http/httptest"
 	"net/url"
 	"testing"
 
+	"github.com/containers/kubernetes-mcp-server/internal/test"
 	"github.com/containers/kubernetes-mcp-server/pkg/config"
+	"github.com/stretchr/testify/suite"
 )
 
-func TestValidateAndGetURL_JoinsProperly(t *testing.T) {
-	cfg := config.Default()
-	cfg.SetToolsetConfig("kiali", &Config{Url: "https://kiali.example/"})
-	m := NewManager(cfg)
-	k := m.GetKiali()
-
-	full, err := k.validateAndGetURL("/api/path")
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-	if full != "https://kiali.example/api/path" {
-		t.Fatalf("unexpected url: %s", full)
-	}
-
-	m.KialiURL = "https://kiali.example"
-	full, err = k.validateAndGetURL("api/path")
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-	if full != "https://kiali.example/api/path" {
-		t.Fatalf("unexpected url: %s", full)
-	}
-
-	// preserve query
-	m.KialiURL = "https://kiali.example"
-	full, err = k.validateAndGetURL("/api/path?x=1&y=2")
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-	u, _ := url.Parse(full)
-	if u.Path != "/api/path" || u.Query().Get("x") != "1" || u.Query().Get("y") != "2" {
-		t.Fatalf("unexpected parsed url: %s", full)
-	}
+type KialiSuite struct {
+	suite.Suite
+	MockServer *test.MockServer
+	Config     *config.StaticConfig
+}
+
+func (s *KialiSuite) SetupTest() {
+	s.MockServer = test.NewMockServer()
+	s.MockServer.Config().BearerToken = ""
+	s.Config = config.Default()
+}
+
+func (s *KialiSuite) TearDownTest() {
+	s.MockServer.Close()
+}
+
+func (s *KialiSuite) TestNewKiali_SetsFields() {
+	s.Config = test.Must(config.ReadToml([]byte(`
+		[toolset_configs.kiali]
+		url = "https://kiali.example/"
+		insecure = true
+	`)))
+	s.MockServer.Config().BearerToken = "bearer-token"
+	k := NewKiali(s.Config, s.MockServer.Config())
+
+	s.Run("URL is set", func() {
+		s.Equal("https://kiali.example/", k.kialiURL, "Unexpected Kiali URL")
+	})
+	s.Run("Insecure is set", func() {
+		s.True(k.kialiInsecure, "Expected Kiali Insecure to be true")
+	})
+	s.Run("BearerToken is set", func() {
+		s.Equal("bearer-token", k.bearerToken, "Unexpected Kiali BearerToken")
+	})
+}
+
+func (s *KialiSuite) TestNewKiali_InvalidConfig() {
+	cfg, err := config.ReadToml([]byte(`
+		[toolset_configs.kiali]
+		url = "://invalid-url"
+	`))
+	s.Error(err, "Expected error reading invalid config")
+	s.ErrorContains(err, "kiali-url must be a valid URL", "Unexpected error message")
+	s.Nil(cfg, "Unexpected Kiali config")
+}
+
+func (s *KialiSuite) TestValidateAndGetURL() {
+	s.Config = test.Must(config.ReadToml([]byte(`
+		[toolset_configs.kiali]
+		url = "https://kiali.example/"
+	`)))
+	k := NewKiali(s.Config, s.MockServer.Config())
+
+	s.Run("Computes full URL", func() {
+		s.Run("with leading slash", func() {
+			full, err := k.validateAndGetURL("/api/path")
+			s.Require().NoError(err, "Expected no error validating URL")
+			s.Equal("https://kiali.example/api/path", full, "Unexpected full URL")
+		})
+
+		s.Run("without leading slash", func() {
+			full, err := k.validateAndGetURL("api/path")
+			s.Require().NoError(err, "Expected no error validating URL")
+			s.Equal("https://kiali.example/api/path", full, "Unexpected full URL")
+		})
+
+		s.Run("with query parameters, preserves query", func() {
+			full, err := k.validateAndGetURL("/api/path?x=1&y=2")
+			s.Require().NoError(err, "Expected no error validating URL")
+			u, err := url.Parse(full)
+			s.Require().NoError(err, "Expected to parse full URL")
+			s.Equal("/api/path", u.Path, "Unexpected path in parsed URL")
+			s.Equal("1", u.Query().Get("x"), "Unexpected query parameter x")
+			s.Equal("2", u.Query().Get("y"), "Unexpected query parameter y")
+		})
+	})
 }
 
 // CurrentAuthorizationHeader behavior is now implicit via executeRequest using Manager.BearerToken
 
-func TestExecuteRequest_SetsAuthAndCallsServer(t *testing.T) {
+func (s *KialiSuite) TestExecuteRequest() {
 	// setup test server to assert path and auth header
 	var seenAuth string
 	var seenPath string
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+	s.MockServer.Config().BearerToken = "token-xyz"
+	s.MockServer.Handle(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		seenAuth = r.Header.Get("Authorization")
 		seenPath = r.URL.String()
 		_, _ = w.Write([]byte("ok"))
 	}))
-	defer srv.Close()
-
-	cfg := config.Default()
-	cfg.SetToolsetConfig("kiali", &Config{Url: srv.URL})
-	m := NewManager(cfg)
-	m.BearerToken = "token-xyz"
-	k := m.GetKiali()
-	out, err := k.executeRequest(context.Background(), "/api/ping?q=1")
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-	if out != "ok" {
-		t.Fatalf("unexpected body: %s", out)
-	}
-	if seenAuth != "Bearer token-xyz" {
-		t.Fatalf("expected auth header to be set, got '%s'", seenAuth)
-	}
-	if seenPath != "/api/ping?q=1" {
-		t.Fatalf("unexpected path: %s", seenPath)
-	}
+
+	s.Config = test.Must(config.ReadToml([]byte(fmt.Sprintf(`
+		[toolset_configs.kiali]
+		url = "%s"
+	`, s.MockServer.Config().Host))))
+	k := NewKiali(s.Config, s.MockServer.Config())
+
+	out, err := k.executeRequest(s.T().Context(), "/api/ping?q=1")
+	s.Require().NoError(err, "Expected no error executing request")
+	s.Run("auth header set", func() {
+		s.Equal("Bearer token-xyz", seenAuth, "Unexpected Authorization header")
+	})
+	s.Run("path is correct", func() {
+		s.Equal("/api/ping?q=1", seenPath, "Unexpected path")
+	})
+	s.Run("response body is correct", func() {
+		s.Equal("ok", out, "Unexpected response body")
+	})
+}
+
+func TestKiali(t *testing.T) {
+	suite.Run(t, new(KialiSuite))
 }