From 95a33132ba47005485b192803a505a63f5d020f0 Mon Sep 17 00:00:00 2001 From: Ayush Date: Mon, 25 May 2026 06:57:18 +0000 Subject: [PATCH 1/2] fix: address PR review comments Signed-off-by: Ayush --- .../translator/agent/adk_api_translator.go | 82 +++++++++++++++---- .../translator/agent/cluster_domain_test.go | 36 ++++++++ .../internal/httpserver/handlers/agents.go | 2 +- .../internal/httpserver/handlers/handlers.go | 4 +- go/core/internal/httpserver/server.go | 3 +- go/core/pkg/app/app.go | 4 +- helm/kagent/values.yaml | 2 +- helm/tools/grafana-mcp/values.yaml | 2 +- 8 files changed, 114 insertions(+), 21 deletions(-) create mode 100644 go/core/internal/controller/translator/agent/cluster_domain_test.go diff --git a/go/core/internal/controller/translator/agent/adk_api_translator.go b/go/core/internal/controller/translator/agent/adk_api_translator.go index 0ee9528f8b..b1445642a9 100644 --- a/go/core/internal/controller/translator/agent/adk_api_translator.go +++ b/go/core/internal/controller/translator/agent/adk_api_translator.go @@ -33,6 +33,7 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/validation" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -130,6 +131,7 @@ type AdkApiTranslator interface { inputs *AgentManifestInputs, ) (*AgentOutputs, error) GetOwnedResourceTypes() []client.Object + WithClusterDomain(clusterDomain string) AdkApiTranslator } // probeConfig holds readiness probe timing configuration @@ -177,16 +179,30 @@ func NewAdkApiTranslatorWithWatchedNamespaces(kube client.Client, watchedNamespa defaultModelConfig: defaultModelConfig, plugins: plugins, globalProxyURL: globalProxyURL, + clusterDomain: "cluster.local", sandboxBackend: sandboxBackend, } } +func (a *adkApiTranslator) WithClusterDomain(clusterDomain string) AdkApiTranslator { + if a == nil { + return a + } + clusterDomain = strings.TrimSpace(clusterDomain) + if clusterDomain == "" { + clusterDomain = "cluster.local" + } + a.clusterDomain = clusterDomain + return a +} + type adkApiTranslator struct { kube client.Client watchedNamespaces []string defaultModelConfig types.NamespacedName plugins []TranslatorPlugin globalProxyURL string + clusterDomain string sandboxBackend sandboxbackend.Backend } @@ -953,34 +969,70 @@ func (a *adkApiTranslator) isInternalK8sURL(ctx context.Context, urlStr, namespa return false } - // Check if it ends with .svc.cluster.local (definitely internal) - if strings.HasSuffix(hostname, ".svc.cluster.local") { + clusterDomain := strings.TrimSpace(a.clusterDomain) + if clusterDomain == "" { + clusterDomain = "cluster.local" + } + svcDomain := ".svc." + clusterDomain + + // Only treat fully qualified service hostnames as internal if they match + // a valid Kubernetes service DNS pattern for the current cluster domain. + // Valid forms are: + // service.namespace.svc. + // service.namespace.svc + // service.namespace + if strings.HasSuffix(hostname, svcDomain) { + prefix := strings.TrimSuffix(hostname, svcDomain) + if prefix == "" { + return false + } + parts := strings.Split(prefix, ".") + if len(parts) != 2 { + return false + } + if validation.IsDNS1123Label(parts[0]) != nil || validation.IsDNS1123Label(parts[1]) != nil { + return false + } return true } - // Extract namespace from hostname pattern: {name}.{namespace} - // Examples: test-mcp-server.kagent -> namespace is "kagent" parts := strings.Split(hostname, ".") if len(parts) == 2 { + service := parts[0] potentialNamespace := parts[1] - - // Check if this namespace exists in the cluster - ns := &corev1.Namespace{} - err := a.kube.Get(ctx, types.NamespacedName{Name: potentialNamespace}, ns) - if err == nil { - // Namespace exists, so this is an internal k8s URL - return true + if validation.IsDNS1123Label(service) != nil || validation.IsDNS1123Label(potentialNamespace) != nil { + return false } - // Controller is using namespaced RBAC, so check if the namespace is watched - if (apierrors.IsForbidden(err) || apierrors.IsUnauthorized(err)) && len(a.watchedNamespaces) > 0 { - return slices.Contains(a.watchedNamespaces, potentialNamespace) + return a.namespaceExistsOrWatched(ctx, potentialNamespace) + } + + if len(parts) == 3 && parts[2] == "svc" { + service := parts[0] + potentialNamespace := parts[1] + if validation.IsDNS1123Label(service) != nil || validation.IsDNS1123Label(potentialNamespace) != nil { + return false } - // If namespace doesn't exist, it's likely a TLD or external domain + return a.namespaceExistsOrWatched(ctx, potentialNamespace) } return false } +func (a *adkApiTranslator) namespaceExistsOrWatched(ctx context.Context, potentialNamespace string) bool { + ns := &corev1.Namespace{} + err := a.kube.Get(ctx, types.NamespacedName{Name: potentialNamespace}, ns) + if err == nil { + return true + } + if (apierrors.IsForbidden(err) || apierrors.IsUnauthorized(err)) && len(a.watchedNamespaces) > 0 { + return slices.Contains(a.watchedNamespaces, potentialNamespace) + } + return false +} + + return false +} + func applyProxyURL(originalURL, proxyURL string, headers map[string]string) (targetURL string, updatedHeaders map[string]string, err error) { // Parse original URL to extract path and hostname originalURLParsed, err := url.Parse(originalURL) diff --git a/go/core/internal/controller/translator/agent/cluster_domain_test.go b/go/core/internal/controller/translator/agent/cluster_domain_test.go new file mode 100644 index 0000000000..4a4065b3b4 --- /dev/null +++ b/go/core/internal/controller/translator/agent/cluster_domain_test.go @@ -0,0 +1,36 @@ +package agent + +import ( + "context" + "testing" + + "github.com/stretchr/testify/require" + "github.com/kagent-dev/kagent/go/api/v1alpha2" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + k8sscheme "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +func TestAdkApiTranslator_IsInternalK8sURL(t *testing.T) { + scheme := k8sscheme.Scheme + require.NoError(t, v1alpha2.AddToScheme(scheme)) + + namespace := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{Name: "kagent"}, + } + + kubeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(namespace).Build() + translatorImpl := NewAdkApiTranslatorWithWatchedNamespaces(kubeClient, nil, types.NamespacedName{Name: "default-model"}, nil, "", nil).WithClusterDomain("cluster.local").(*adkApiTranslator) + + require.True(t, translatorImpl.isInternalK8sURL(context.Background(), "http://grafana.kagent.svc.cluster.local:3000/api", "kagent"), "should recognize fully qualified service DNS as internal") + require.True(t, translatorImpl.isInternalK8sURL(context.Background(), "http://grafana.kagent.svc:3000/api", "kagent"), "should recognize service.namespace.svc shorthand as internal") + require.True(t, translatorImpl.isInternalK8sURL(context.Background(), "http://grafana.kagent:3000/api", "kagent"), "should recognize service.namespace shorthand as internal") + + require.False(t, translatorImpl.isInternalK8sURL(context.Background(), "http://grafana.kagent.svc.cluster.local.evil.com:3000/api", "kagent"), "should reject external domains that only contain the cluster suffix") + require.False(t, translatorImpl.isInternalK8sURL(context.Background(), "http://example.com:8080", "kagent"), "should reject normal external URLs") + + translatorImpl.WithClusterDomain("custom.internal") + require.True(t, translatorImpl.isInternalK8sURL(context.Background(), "http://grafana.kagent.svc.custom.internal:3000/api", "kagent"), "should honor custom cluster-domain values") +} diff --git a/go/core/internal/httpserver/handlers/agents.go b/go/core/internal/httpserver/handlers/agents.go index 3233204ba4..776fde0e35 100644 --- a/go/core/internal/httpserver/handlers/agents.go +++ b/go/core/internal/httpserver/handlers/agents.go @@ -296,7 +296,7 @@ func (h *AgentsHandler) buildTranslator(kubeClient client.Client) agent_translat nil, h.ProxyURL, h.SandboxBackend, - ) + ).WithClusterDomain(h.ClusterDomain) } func (h *AgentsHandler) validateAgentObject(ctx context.Context, agent v1alpha2.AgentObject) error { diff --git a/go/core/internal/httpserver/handlers/handlers.go b/go/core/internal/httpserver/handlers/handlers.go index 13a66adeb9..9230caa94d 100644 --- a/go/core/internal/httpserver/handlers/handlers.go +++ b/go/core/internal/httpserver/handlers/handlers.go @@ -38,18 +38,20 @@ type Base struct { DatabaseService database.Client Authorizer auth.Authorizer // Interface for authorization checks ProxyURL string + ClusterDomain string WatchedNamespaces []string SandboxBackend sandboxbackend.Backend } // NewHandlers creates a new Handlers instance with all handler components. -func NewHandlers(kubeClient client.Client, defaultModelConfig types.NamespacedName, dbService database.Client, watchedNamespaces []string, authorizer auth.Authorizer, proxyURL string, rcnclr reconciler.KagentReconciler, sandboxBackend sandboxbackend.Backend) *Handlers { +func NewHandlers(kubeClient client.Client, defaultModelConfig types.NamespacedName, dbService database.Client, watchedNamespaces []string, authorizer auth.Authorizer, proxyURL string, clusterDomain string, rcnclr reconciler.KagentReconciler, sandboxBackend sandboxbackend.Backend) *Handlers { base := &Base{ KubeClient: kubeClient, DefaultModelConfig: defaultModelConfig, DatabaseService: dbService, Authorizer: authorizer, ProxyURL: proxyURL, + ClusterDomain: clusterDomain, WatchedNamespaces: watchedNamespaces, SandboxBackend: sandboxBackend, } diff --git a/go/core/internal/httpserver/server.go b/go/core/internal/httpserver/server.go index aac7e831ab..8915ec565a 100644 --- a/go/core/internal/httpserver/server.go +++ b/go/core/internal/httpserver/server.go @@ -69,6 +69,7 @@ type ServerConfig struct { Authenticator auth.AuthProvider Authorizer auth.Authorizer ProxyURL string + ClusterDomain string Reconciler reconciler.KagentReconciler SandboxBackend sandboxbackend.Backend } @@ -89,7 +90,7 @@ func NewHTTPServer(config ServerConfig) (*HTTPServer, error) { return &HTTPServer{ config: config, router: config.Router, - handlers: handlers.NewHandlers(config.KubeClient, defaultModelConfig, config.DbClient, config.WatchedNamespaces, config.Authorizer, config.ProxyURL, config.Reconciler, config.SandboxBackend), + handlers: handlers.NewHandlers(config.KubeClient, defaultModelConfig, config.DbClient, config.WatchedNamespaces, config.Authorizer, config.ProxyURL, config.ClusterDomain, config.Reconciler, config.SandboxBackend), authenticator: config.Authenticator, }, nil } diff --git a/go/core/pkg/app/app.go b/go/core/pkg/app/app.go index d47ab55adf..3847169841 100644 --- a/go/core/pkg/app/app.go +++ b/go/core/pkg/app/app.go @@ -134,6 +134,7 @@ type Config struct { HttpServerAddr string WatchNamespaces string A2ABaseUrl string + ClusterDomain string Database struct { Url string UrlFile string @@ -174,6 +175,7 @@ func (cfg *Config) SetFlags(commandLine *flag.FlagSet) { commandLine.StringVar(&cfg.DefaultModelConfig.Namespace, "default-model-config-namespace", kagentNamespace, "The namespace of the default model config.") commandLine.StringVar(&cfg.HttpServerAddr, "http-server-address", ":8083", "The address the HTTP server binds to.") commandLine.StringVar(&cfg.A2ABaseUrl, "a2a-base-url", "http://127.0.0.1:8083", "The base URL of the A2A Server endpoint, as advertised to clients.") + commandLine.StringVar(&cfg.ClusterDomain, "cluster-domain", "cluster.local", "The Kubernetes cluster DNS domain used for internal service discovery.") commandLine.StringVar(&cfg.Database.Url, "postgres-database-url", "postgres://postgres:kagent@kagent-postgresql.kagent.svc.cluster.local:5432/postgres", "The URL of the PostgreSQL database.") commandLine.StringVar(&cfg.Database.UrlFile, "postgres-database-url-file", "", "Path to a file containing the PostgreSQL database URL. Takes precedence over --postgres-database-url.") commandLine.BoolVar(&cfg.Database.VectorEnabled, "database-vector-enabled", true, "Enable pgvector extension and memory table. Requires pgvector to be installed on the PostgreSQL server.") @@ -518,7 +520,7 @@ func Start(getExtensionConfig GetExtensionConfig, migrationRunner MigrationRunne extensionCfg.AgentPlugins, cfg.Proxy.URL, extensionCfg.SandboxBackend, - ) + ).WithClusterDomain(cfg.ClusterDomain) rcnclr := reconciler.NewKagentReconciler( apiTranslator, diff --git a/helm/kagent/values.yaml b/helm/kagent/values.yaml index 2327f006ce..46ff30e12c 100644 --- a/helm/kagent/values.yaml +++ b/helm/kagent/values.yaml @@ -527,7 +527,7 @@ cilium-debug-agent: grafana-mcp: enabled: true grafana: - url: "grafana.kagent:3000/api" + url: "http://grafana.kagent.svc.cluster.local:3000/api" serviceAccountToken: "" # apiKey: "" # Deprecated - use serviceAccountToken instead. # secretRef: "" # Name of Secret to reference (contains GRAFANA_SERVICE_ACCOUNT_TOKEN or GRAFANA_API_KEY) diff --git a/helm/tools/grafana-mcp/values.yaml b/helm/tools/grafana-mcp/values.yaml index 01f0f1f50a..34abca93da 100644 --- a/helm/tools/grafana-mcp/values.yaml +++ b/helm/tools/grafana-mcp/values.yaml @@ -1,7 +1,7 @@ replicas: 1 grafana: - url: "grafana.kagent:3000/api" + url: "http://grafana.kagent.svc.cluster.local:3000/api" serviceAccountToken: "" # apiKey: "" # Deprecated - use serviceAccountToken instead. # secretRef: "" # Name of Secret to reference (contains GRAFANA_SERVICE_ACCOUNT_TOKEN or GRAFANA_API_KEY) From 472726e223ca033f0b67867bf173d793b892df34 Mon Sep 17 00:00:00 2001 From: Ayush Date: Mon, 25 May 2026 08:16:36 +0000 Subject: [PATCH 2/2] fix: cluster-domain support; validate svc hostnames; fix test scheme - Remove stray return/brace in adk_api_translator.go - Use len(validation.IsDNS1123Label(...)) > 0 checks - Use fresh scheme in cluster_domain_test.go Signed-off-by: Automated Fix Signed-off-by: Ayush --- .../controller/translator/agent/adk_api_translator.go | 8 +++----- .../controller/translator/agent/cluster_domain_test.go | 5 +++-- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/go/core/internal/controller/translator/agent/adk_api_translator.go b/go/core/internal/controller/translator/agent/adk_api_translator.go index b1445642a9..c929fb5f5f 100644 --- a/go/core/internal/controller/translator/agent/adk_api_translator.go +++ b/go/core/internal/controller/translator/agent/adk_api_translator.go @@ -990,7 +990,7 @@ func (a *adkApiTranslator) isInternalK8sURL(ctx context.Context, urlStr, namespa if len(parts) != 2 { return false } - if validation.IsDNS1123Label(parts[0]) != nil || validation.IsDNS1123Label(parts[1]) != nil { + if len(validation.IsDNS1123Label(parts[0])) > 0 || len(validation.IsDNS1123Label(parts[1])) > 0 { return false } return true @@ -1000,7 +1000,7 @@ func (a *adkApiTranslator) isInternalK8sURL(ctx context.Context, urlStr, namespa if len(parts) == 2 { service := parts[0] potentialNamespace := parts[1] - if validation.IsDNS1123Label(service) != nil || validation.IsDNS1123Label(potentialNamespace) != nil { + if len(validation.IsDNS1123Label(service)) > 0 || len(validation.IsDNS1123Label(potentialNamespace)) > 0 { return false } return a.namespaceExistsOrWatched(ctx, potentialNamespace) @@ -1009,7 +1009,7 @@ func (a *adkApiTranslator) isInternalK8sURL(ctx context.Context, urlStr, namespa if len(parts) == 3 && parts[2] == "svc" { service := parts[0] potentialNamespace := parts[1] - if validation.IsDNS1123Label(service) != nil || validation.IsDNS1123Label(potentialNamespace) != nil { + if len(validation.IsDNS1123Label(service)) > 0 || len(validation.IsDNS1123Label(potentialNamespace)) > 0 { return false } return a.namespaceExistsOrWatched(ctx, potentialNamespace) @@ -1030,8 +1030,6 @@ func (a *adkApiTranslator) namespaceExistsOrWatched(ctx context.Context, potenti return false } - return false -} func applyProxyURL(originalURL, proxyURL string, headers map[string]string) (targetURL string, updatedHeaders map[string]string, err error) { // Parse original URL to extract path and hostname diff --git a/go/core/internal/controller/translator/agent/cluster_domain_test.go b/go/core/internal/controller/translator/agent/cluster_domain_test.go index 4a4065b3b4..80254da0a7 100644 --- a/go/core/internal/controller/translator/agent/cluster_domain_test.go +++ b/go/core/internal/controller/translator/agent/cluster_domain_test.go @@ -9,12 +9,13 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" - k8sscheme "k8s.io/client-go/kubernetes/scheme" + k8sruntime "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/controller-runtime/pkg/client/fake" ) func TestAdkApiTranslator_IsInternalK8sURL(t *testing.T) { - scheme := k8sscheme.Scheme + scheme := k8sruntime.NewScheme() + require.NoError(t, corev1.AddToScheme(scheme)) require.NoError(t, v1alpha2.AddToScheme(scheme)) namespace := &corev1.Namespace{