Skip to content

fix(cmd): disable klog in STDIO mode #331

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 17, 2025
Merged

fix(cmd): disable klog in STDIO mode #331

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ require (
github.com/spf13/cobra v1.10.1
github.com/spf13/pflag v1.0.10
github.com/stretchr/testify v1.11.1
golang.org/x/net v0.42.0
golang.org/x/oauth2 v0.31.0
golang.org/x/sync v0.17.0
helm.sh/helm/v3 v3.18.6
Expand Down Expand Up @@ -122,7 +123,6 @@ require (
go.yaml.in/yaml/v2 v2.4.2 // indirect
go.yaml.in/yaml/v3 v3.0.4 // indirect
golang.org/x/crypto v0.40.0 // indirect
golang.org/x/net v0.42.0 // indirect
golang.org/x/sys v0.34.0 // indirect
golang.org/x/term v0.33.0 // indirect
golang.org/x/text v0.28.0 // indirect
Expand Down
6 changes: 6 additions & 0 deletions pkg/kubernetes-mcp-server/cmd/root.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,12 @@ func (m *MCPServerOptions) loadFlags(cmd *cobra.Command) {
func (m *MCPServerOptions) initializeLogging() {
flagSet := flag.NewFlagSet("klog", flag.ContinueOnError)
klog.InitFlags(flagSet)
if m.StaticConfig.Port == "" {
Copy link
Member

@ardaguclu ardaguclu Sep 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we simply by pass klog.InitFlags if m.StaticConfig.Port is empty?

Copy link
Member Author

@manusa manusa Sep 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because we need to set the other flags (logtostderr, alsologtostderr, stderrthreshold) which have defaults that enable logging of some messages

Copy link
Member Author

@manusa manusa Sep 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the fix is not for our logs (which there aren't many at the moment) but rather for the upstream kubernetes logs which aren't controlled easily.

Copy link
Member

@ardaguclu ardaguclu Sep 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does that mean our logs do not break the jsonrpc?

Copy link
Member Author

@manusa manusa Sep 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, they would break the jsonrpc too.
This statement also disables them (as asserted in the provided tests).

Copy link
Member

@ardaguclu ardaguclu Sep 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for clarification; by running klog.InitFlags in SDTIO, which logs that do not break jsonrpc will be logged?

Copy link
Member Author

@manusa manusa Sep 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, let me try to summarize.

If you run kubernetes-mcp-server in STDIO mode (i.e. no --port flag) with an unauthorized cluster you can immediately trigger an error that will be logged to STDERR:

E0917 16:47:01.678632   85672 memcache.go:265] "Unhandled Error" err="couldn't get current server API group list: ...

I understand that there might be other scenarios where this might happen.

I've started to see that some MCP Hosts(->Clients) start failing because they've become ore strict and will mark the MCP server as not working.

The goal here is to disable all logging either to stderr or stdout in order not to break the protocol for these more strict clients.

By running klog.InitFlags and then providing the three log configuration flags we prevent klog from logging to stderr which is what happens by default.

Not sure if it's clearer now.

Copy link
Member

@ardaguclu ardaguclu Sep 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for explanation. It makes sense.

LGTM

// disable klog output for stdio mode
// this is needed to avoid klog writing to stderr and breaking the protocol
_ = flagSet.Parse([]string{"-logtostderr=false", "-alsologtostderr=false", "-stderrthreshold=FATAL"})
return
}
loggerOptions := []textlogger.ConfigOption{textlogger.Output(m.Out)}
if m.StaticConfig.LogLevel >= 0 {
loggerOptions = append(loggerOptions, textlogger.Verbosity(m.StaticConfig.LogLevel))
Expand Down
43 changes: 32 additions & 11 deletions pkg/kubernetes-mcp-server/cmd/root_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ import (
"strings"
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"k8s.io/cli-runtime/pkg/genericiooptions"
)

Expand Down Expand Up @@ -48,7 +50,7 @@ func TestConfig(t *testing.T) {
t.Run("defaults to none", func(t *testing.T) {
ioStreams, out := testStream()
rootCmd := NewMCPServer(ioStreams)
rootCmd.SetArgs([]string{"--version", "--log-level=1"})
rootCmd.SetArgs([]string{"--version", "--port=1337", "--log-level=1"})
expectedConfig := `" - Config: "`
if err := rootCmd.Execute(); !strings.Contains(out.String(), expectedConfig) {
t.Fatalf("Expected config to be %s, got %s %v", expectedConfig, out.String(), err)
Expand All @@ -59,7 +61,7 @@ func TestConfig(t *testing.T) {
rootCmd := NewMCPServer(ioStreams)
_, file, _, _ := runtime.Caller(0)
emptyConfigPath := filepath.Join(filepath.Dir(file), "testdata", "empty-config.toml")
rootCmd.SetArgs([]string{"--version", "--log-level=1", "--config", emptyConfigPath})
rootCmd.SetArgs([]string{"--version", "--port=1337", "--log-level=1", "--config", emptyConfigPath})
_ = rootCmd.Execute()
expected := `(?m)\" - Config\:[^\"]+empty-config\.toml\"`
if m, err := regexp.MatchString(expected, out.String()); !m || err != nil {
Expand All @@ -69,7 +71,7 @@ func TestConfig(t *testing.T) {
t.Run("invalid path throws error", func(t *testing.T) {
ioStreams, _ := testStream()
rootCmd := NewMCPServer(ioStreams)
rootCmd.SetArgs([]string{"--version", "--log-level=1", "--config", "invalid-path-to-config.toml"})
rootCmd.SetArgs([]string{"--version", "--port=1337", "--log-level=1", "--config", "invalid-path-to-config.toml"})
err := rootCmd.Execute()
if err == nil {
t.Fatal("Expected error for invalid config path, got nil")
Expand Down Expand Up @@ -142,15 +144,15 @@ func TestToolsets(t *testing.T) {
t.Run("default", func(t *testing.T) {
ioStreams, out := testStream()
rootCmd := NewMCPServer(ioStreams)
rootCmd.SetArgs([]string{"--version", "--log-level=1"})
rootCmd.SetArgs([]string{"--version", "--port=1337", "--log-level=1"})
if err := rootCmd.Execute(); !strings.Contains(out.String(), "- Toolsets: core, config, helm") {
t.Fatalf("Expected toolsets 'full', got %s %v", out, err)
}
})
t.Run("set with --toolsets", func(t *testing.T) {
ioStreams, out := testStream()
rootCmd := NewMCPServer(ioStreams)
rootCmd.SetArgs([]string{"--version", "--log-level=1", "--toolsets", "helm,config"})
rootCmd.SetArgs([]string{"--version", "--port=1337", "--log-level=1", "--toolsets", "helm,config"})
_ = rootCmd.Execute()
expected := `(?m)\" - Toolsets\: helm, config\"`
if m, err := regexp.MatchString(expected, out.String()); !m || err != nil {
Expand All @@ -172,15 +174,15 @@ func TestListOutput(t *testing.T) {
t.Run("defaults to table", func(t *testing.T) {
ioStreams, out := testStream()
rootCmd := NewMCPServer(ioStreams)
rootCmd.SetArgs([]string{"--version", "--log-level=1"})
rootCmd.SetArgs([]string{"--version", "--port=1337", "--log-level=1"})
if err := rootCmd.Execute(); !strings.Contains(out.String(), "- ListOutput: table") {
t.Fatalf("Expected list-output 'table', got %s %v", out, err)
}
})
t.Run("set with --list-output", func(t *testing.T) {
ioStreams, out := testStream()
rootCmd := NewMCPServer(ioStreams)
rootCmd.SetArgs([]string{"--version", "--log-level=1", "--list-output", "yaml"})
rootCmd.SetArgs([]string{"--version", "--port=1337", "--log-level=1", "--list-output", "yaml"})
_ = rootCmd.Execute()
expected := `(?m)\" - ListOutput\: yaml\"`
if m, err := regexp.MatchString(expected, out.String()); !m || err != nil {
Expand All @@ -193,15 +195,15 @@ func TestReadOnly(t *testing.T) {
t.Run("defaults to false", func(t *testing.T) {
ioStreams, out := testStream()
rootCmd := NewMCPServer(ioStreams)
rootCmd.SetArgs([]string{"--version", "--log-level=1"})
rootCmd.SetArgs([]string{"--version", "--port=1337", "--log-level=1"})
if err := rootCmd.Execute(); !strings.Contains(out.String(), " - Read-only mode: false") {
t.Fatalf("Expected read-only mode false, got %s %v", out, err)
}
})
t.Run("set with --read-only", func(t *testing.T) {
ioStreams, out := testStream()
rootCmd := NewMCPServer(ioStreams)
rootCmd.SetArgs([]string{"--version", "--log-level=1", "--read-only"})
rootCmd.SetArgs([]string{"--version", "--port=1337", "--log-level=1", "--read-only"})
_ = rootCmd.Execute()
expected := `(?m)\" - Read-only mode\: true\"`
if m, err := regexp.MatchString(expected, out.String()); !m || err != nil {
Expand All @@ -214,15 +216,15 @@ func TestDisableDestructive(t *testing.T) {
t.Run("defaults to false", func(t *testing.T) {
ioStreams, out := testStream()
rootCmd := NewMCPServer(ioStreams)
rootCmd.SetArgs([]string{"--version", "--log-level=1"})
rootCmd.SetArgs([]string{"--version", "--port=1337", "--log-level=1"})
if err := rootCmd.Execute(); !strings.Contains(out.String(), " - Disable destructive tools: false") {
t.Fatalf("Expected disable destructive false, got %s %v", out, err)
}
})
t.Run("set with --disable-destructive", func(t *testing.T) {
ioStreams, out := testStream()
rootCmd := NewMCPServer(ioStreams)
rootCmd.SetArgs([]string{"--version", "--log-level=1", "--disable-destructive"})
rootCmd.SetArgs([]string{"--version", "--port=1337", "--log-level=1", "--disable-destructive"})
_ = rootCmd.Execute()
expected := `(?m)\" - Disable destructive tools\: true\"`
if m, err := regexp.MatchString(expected, out.String()); !m || err != nil {
Expand Down Expand Up @@ -255,3 +257,22 @@ func TestAuthorizationURL(t *testing.T) {
}
})
}

func TestStdioLogging(t *testing.T) {
t.Run("stdio disables klog", func(t *testing.T) {
ioStreams, out := testStream()
rootCmd := NewMCPServer(ioStreams)
rootCmd.SetArgs([]string{"--version", "--log-level=1"})
err := rootCmd.Execute()
require.NoErrorf(t, err, "Expected no error executing command, got %v", err)
assert.Equalf(t, "0.0.0\n", out.String(), "Expected only version output, got %s", out.String())
})
t.Run("http mode enables klog", func(t *testing.T) {
ioStreams, out := testStream()
rootCmd := NewMCPServer(ioStreams)
rootCmd.SetArgs([]string{"--version", "--log-level=1", "--port=1337"})
err := rootCmd.Execute()
require.NoErrorf(t, err, "Expected no error executing command, got %v", err)
assert.Containsf(t, out.String(), "Starting kubernetes-mcp-server", "Expected klog output, got %s", out.String())
})
}
Loading