From a735a0fb48303fd0b70a0601c15e82fd2d03882b Mon Sep 17 00:00:00 2001 From: Ben Date: Sun, 6 Sep 2026 14:28:09 +0300 Subject: [PATCH] fix(kubescape): recover from a failed client init without a pod restart The provider built its Kubernetes clients once, in RegisterTools. If that failed, initError was set and all ten handlers short-circuited on it for the lifetime of the process: tool := NewKubescapeTool(kubeconfig) // sets initError, never retried ... if k.initError != nil { return toolErr.ToMCPResult(), nil } Nothing cleared or retried it. In-cluster this is reachable on ordinary start ordering -- the pod comes up before the API server is reachable, or before its RBAC has been applied -- and the result is that every Kubescape tool stays dead until someone restarts the pod. The cluster recovers; the provider does not. Clients are now built on first use and cached. A failed attempt is reported without retrying for 30s and retried after that, so the provider heals itself once the cluster is reachable, while a loop of failing tool calls cannot hammer the API server -- each attempt can block for a full dial timeout. NewKubescapeTool no longer fails, so a broken config now surfaces on the first tool call instead of at registration. That is the point of the change, but it does move the signal: kubescape_check_health remains the way to ask whether the installation is reachable. Also fixes the ordering it exposed: the client check ran before argument validation, so with no cluster a call missing a required argument reported **Error**: failed to create kubernetes config: ... no configuration has been provided instead of "manifest_name parameter is required". Argument validation needs no cluster, so it now runs first in the five handlers that have required arguments. Construction and the clock are injected so the tests cover what matters without a cluster and without sleeping: a build that fails once and then succeeds proves recovery, and a second call inside the interval proves the failure is not retried. Both fail against the previous code. Docs-exempt: bug fix; no existing doc describes provider client lifecycle Signed-off-by: Ben Hirschberg Signed-off-by: Ben --- pkg/kubescape/client_init_test.go | 192 ++++++++++++++++++++++++++++++ pkg/kubescape/kubescape.go | 174 ++++++++++++++++++--------- pkg/kubescape/testing.go | 13 +- 3 files changed, 322 insertions(+), 57 deletions(-) create mode 100644 pkg/kubescape/client_init_test.go diff --git a/pkg/kubescape/client_init_test.go b/pkg/kubescape/client_init_test.go new file mode 100644 index 0000000..498d70b --- /dev/null +++ b/pkg/kubescape/client_init_test.go @@ -0,0 +1,192 @@ +package kubescape + +import ( + "context" + "errors" + "testing" + "time" + + kubescapefake "github.com/kubescape/storage/pkg/generated/clientset/versioned/fake" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + kubefake "k8s.io/client-go/kubernetes/fake" +) + +// workingClients returns a client set backed by fakes, standing in for a +// successful construction against a reachable API server. +func workingClients() *kubescapeClients { + return &kubescapeClients{ + k8s: kubefake.NewClientset(), + spdx: kubescapefake.NewClientset().SpdxV1beta1(), + } +} + +// toolWithBuilder builds a tool whose client construction and clock are both +// under the test's control, so recovery and rate limiting can be exercised +// without a cluster and without sleeping. +func toolWithBuilder(build func() (*kubescapeClients, error), clock *time.Time) *KubescapeTool { + return &KubescapeTool{ + buildClients: build, + now: func() time.Time { return *clock }, + } +} + +// A client construction that fails once -- because the API server was not yet +// reachable when the pod started -- must not disable the provider for the +// lifetime of the process. This is P5: previously initError was set once and +// every handler short-circuited on it forever. +func TestEnsureClients_RecoversAfterTransientFailure(t *testing.T) { + attempts := 0 + clock := time.Now() + tool := toolWithBuilder(func() (*kubescapeClients, error) { + attempts++ + if attempts == 1 { + return nil, errors.New("dial tcp 10.96.0.1:443: connect: connection refused") + } + return workingClients(), nil + }, &clock) + + result, err := tool.HandleListVulnerabilityManifests(context.Background(), makeRequest(nil)) + require.NoError(t, err) + assert.True(t, result.IsError, "first call should report the construction failure") + + clock = clock.Add(clientRetryInterval + time.Second) + + result, err = tool.HandleListVulnerabilityManifests(context.Background(), makeRequest(nil)) + require.NoError(t, err) + assert.False(t, result.IsError, "provider should recover once the API server is reachable") + assert.Equal(t, 2, attempts) +} + +// Once clients are built they are reused; a healthy provider must not rebuild +// its clients on every call. +func TestEnsureClients_BuildsOnce(t *testing.T) { + attempts := 0 + clock := time.Now() + tool := toolWithBuilder(func() (*kubescapeClients, error) { + attempts++ + return workingClients(), nil + }, &clock) + + for i := 0; i < 3; i++ { + _, err := tool.HandleListVulnerabilityManifests(context.Background(), makeRequest(nil)) + require.NoError(t, err) + } + + assert.Equal(t, 1, attempts) +} + +// A failing construction is cached for clientRetryInterval, so a hot loop of +// tool calls during an outage cannot hammer the API server -- each attempt can +// block for a full dial timeout. +func TestEnsureClients_DoesNotRetryWithinInterval(t *testing.T) { + attempts := 0 + clock := time.Now() + tool := toolWithBuilder(func() (*kubescapeClients, error) { + attempts++ + return nil, errors.New("connection refused") + }, &clock) + + _, err := tool.HandleListVulnerabilityManifests(context.Background(), makeRequest(nil)) + require.NoError(t, err) + + clock = clock.Add(clientRetryInterval / 2) + + result, err := tool.HandleListVulnerabilityManifests(context.Background(), makeRequest(nil)) + require.NoError(t, err) + assert.True(t, result.IsError, "the cached failure is still reported") + assert.Equal(t, 1, attempts, "no second construction attempt within the interval") +} + +// P7: a call missing a required argument must report the missing argument, not +// a kubeconfig error. Argument validation needs no cluster, so it runs first -- +// asserted by the builder never being called. +func TestHandlers_ValidateArgumentsBeforeBuildingClients(t *testing.T) { + tests := []struct { + name string + handler func(*KubescapeTool, context.Context) (string, error) + want string + }{ + { + name: "list_vulnerabilities without manifest_name", + handler: func(k *KubescapeTool, ctx context.Context) (string, error) { + r, err := k.HandleListVulnerabilitiesInManifest(ctx, makeRequest(nil)) + return getResultText(r), err + }, + want: "manifest_name", + }, + { + name: "get_vulnerability_details without cve_id", + handler: func(k *KubescapeTool, ctx context.Context) (string, error) { + r, err := k.HandleGetVulnerabilityDetails(ctx, makeRequest(map[string]interface{}{ + "manifest_name": "some-manifest", + })) + return getResultText(r), err + }, + want: "cve_id", + }, + { + name: "get_configuration_scan without manifest_name", + handler: func(k *KubescapeTool, ctx context.Context) (string, error) { + r, err := k.HandleGetConfigurationScan(ctx, makeRequest(nil)) + return getResultText(r), err + }, + want: "manifest_name", + }, + { + name: "get_application_profile without name", + handler: func(k *KubescapeTool, ctx context.Context) (string, error) { + r, err := k.HandleGetApplicationProfile(ctx, makeRequest(map[string]interface{}{ + "namespace": "default", + })) + return getResultText(r), err + }, + want: "name", + }, + { + name: "get_network_neighborhood without namespace", + handler: func(k *KubescapeTool, ctx context.Context) (string, error) { + r, err := k.HandleGetNetworkNeighborhood(ctx, makeRequest(map[string]interface{}{ + "name": "some-nn", + })) + return getResultText(r), err + }, + want: "namespace", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + attempts := 0 + clock := time.Now() + tool := toolWithBuilder(func() (*kubescapeClients, error) { + attempts++ + return nil, errors.New("failed to create kubernetes config: no configuration has been provided") + }, &clock) + + text, err := tt.handler(tool, context.Background()) + require.NoError(t, err) + + assert.Contains(t, text, tt.want) + assert.NotContains(t, text, "no configuration has been provided") + assert.Equal(t, 0, attempts, "argument validation must not require a cluster") + }) + } +} + +// The exported test helpers keep behaving as the existing suite expects: +// pre-built clients are used as-is, and a tool built with an error stays in +// that error state rather than falling back to an ambient kubeconfig. +func TestNewKubescapeToolWithError_StaysFailed(t *testing.T) { + tool := NewKubescapeToolWithError(errors.New("failed to connect")) + + result, err := tool.HandleListVulnerabilityManifests(context.Background(), makeRequest(nil)) + require.NoError(t, err) + require.True(t, result.IsError) + + tool.lastAttempt = time.Time{} // any retry window has long expired + + result, err = tool.HandleListVulnerabilityManifests(context.Background(), makeRequest(nil)) + require.NoError(t, err) + assert.True(t, result.IsError, "must not fall through to an ambient kubeconfig") +} diff --git a/pkg/kubescape/kubescape.go b/pkg/kubescape/kubescape.go index e2227fa..89394c9 100644 --- a/pkg/kubescape/kubescape.go +++ b/pkg/kubescape/kubescape.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" "strings" + "sync" "time" "github.com/kagent-dev/tools/internal/errors" @@ -38,49 +39,114 @@ const ( storagePodLabel = "app.kubernetes.io/name=storage" ) -// KubescapeTool holds the clients for Kubescape and Kubernetes APIs +// clientRetryInterval is how long a failed client construction is cached +// before another attempt is made. Retrying on every call would let a loop of +// failing tool calls hammer the API server -- and each attempt can block for a +// full dial timeout -- while waiting much longer would leave the provider dead +// well after the cluster recovered. +const clientRetryInterval = 30 * time.Second + +// kubescapeClients is the set of API clients the handlers need. They are built +// together because they share one rest.Config: if one can be built they all +// can, so there is no partially-usable state to represent. +type kubescapeClients struct { + spdx spdxv1beta1.SpdxV1beta1Interface + k8s kubernetes.Interface + apiExt apiextensionsclientset.Interface +} + +// KubescapeTool holds the clients for Kubescape and Kubernetes APIs. +// +// The clients are built on first use rather than at registration. In-cluster +// the provider can start before the API server is reachable or before its RBAC +// has been applied; building once at startup meant such a transient failure +// disabled every Kubescape tool until the pod was restarted. +// +// Every field below is read and written only while holding mu, so a failed +// start recovers safely even with concurrent tool calls. type KubescapeTool struct { + mu sync.Mutex spdxClient spdxv1beta1.SpdxV1beta1Interface k8sClient kubernetes.Interface apiExtClient apiextensionsclientset.Interface - initError error + lastErr error + lastAttempt time.Time + + // buildClients and now are injected so tests can exercise recovery and the + // retry interval without a cluster and without sleeping. + buildClients func() (*kubescapeClients, error) + now func() time.Time } -// NewKubescapeTool creates a new KubescapeTool with Kubernetes clients +// NewKubescapeTool creates a new KubescapeTool. It cannot fail: connection +// problems surface on the first tool call, where they are retried, instead of +// permanently at registration time. Use kubescape_check_health to verify the +// installation. func NewKubescapeTool(kubeconfig string) *KubescapeTool { - tool := &KubescapeTool{} + return &KubescapeTool{ + buildClients: func() (*kubescapeClients, error) { return newClients(kubeconfig) }, + now: time.Now, + } +} +// newClients builds all three API clients from a single rest.Config. +func newClients(kubeconfig string) (*kubescapeClients, error) { config, err := getKubeConfig(kubeconfig) if err != nil { - tool.initError = fmt.Errorf("failed to create kubernetes config: %w", err) - return tool + return nil, fmt.Errorf("failed to create kubernetes config: %w", err) } - // Create standard Kubernetes client + // Standard Kubernetes client k8sClient, err := kubernetes.NewForConfig(config) if err != nil { - tool.initError = fmt.Errorf("failed to create kubernetes client: %w", err) - return tool + return nil, fmt.Errorf("failed to create kubernetes client: %w", err) } - tool.k8sClient = k8sClient - // Create API extensions client for CRD checks + // API extensions client for CRD checks apiExtClient, err := apiextensionsclientset.NewForConfig(config) if err != nil { - tool.initError = fmt.Errorf("failed to create apiextensions client: %w", err) - return tool + return nil, fmt.Errorf("failed to create apiextensions client: %w", err) } - tool.apiExtClient = apiExtClient - // Create Kubescape storage client + // Kubescape storage client spdxClient, err := spdxv1beta1.NewForConfig(config) if err != nil { - tool.initError = fmt.Errorf("failed to create kubescape client: %w", err) - return tool + return nil, fmt.Errorf("failed to create kubescape client: %w", err) + } + + return &kubescapeClients{spdx: spdxClient, k8s: k8sClient, apiExt: apiExtClient}, nil +} + +// ensureClients returns nil once the clients are usable. A previous failure is +// reported without a retry for clientRetryInterval and retried after that, so +// the provider recovers on its own once the cluster becomes reachable. +// +// Handlers must call this before touching any client, and must call it after +// validating their arguments -- argument errors need no cluster, and reporting +// a connection error for a malformed call misdirects the caller. +func (k *KubescapeTool) ensureClients() error { + k.mu.Lock() + defer k.mu.Unlock() + + if k.spdxClient != nil { + return nil + } + if k.lastErr != nil && k.now().Sub(k.lastAttempt) < clientRetryInterval { + return k.lastErr + } + + k.lastAttempt = k.now() + clients, err := k.buildClients() + if err != nil { + k.lastErr = err + return err } - tool.spdxClient = spdxClient - return tool + k.spdxClient = clients.spdx + k.k8sClient = clients.k8s + k.apiExtClient = clients.apiExt + k.lastErr = nil + return nil } func getKubeConfig(kubeconfig string) (*rest.Config, error) { @@ -116,8 +182,8 @@ type CheckStatus struct { // handleCheckHealth verifies Kubescape operator installation and readiness func (k *KubescapeTool) handleCheckHealth(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { - if k.initError != nil { - toolErr := errors.NewKubescapeError("check_health", k.initError) + if err := k.ensureClients(); err != nil { + toolErr := errors.NewKubescapeError("check_health", err) return toolErr.ToMCPResult(), nil } @@ -464,8 +530,8 @@ func (k *KubescapeTool) handleCheckHealth(ctx context.Context, request mcp.CallT // handleListVulnerabilityManifests lists vulnerability manifests at image and workload levels func (k *KubescapeTool) handleListVulnerabilityManifests(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { - if k.initError != nil { - toolErr := errors.NewKubescapeError("list_vulnerability_manifests", k.initError) + if err := k.ensureClients(); err != nil { + toolErr := errors.NewKubescapeError("list_vulnerability_manifests", err) return toolErr.ToMCPResult(), nil } @@ -534,11 +600,6 @@ func (k *KubescapeTool) handleListVulnerabilityManifests(ctx context.Context, re // handleListVulnerabilitiesInManifest lists all CVEs in a specific manifest func (k *KubescapeTool) handleListVulnerabilitiesInManifest(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { - if k.initError != nil { - toolErr := errors.NewKubescapeError("list_vulnerabilities", k.initError) - return toolErr.ToMCPResult(), nil - } - namespace := mcp.ParseString(request, "namespace", defaultKubescapeNamespace) manifestName := mcp.ParseString(request, "manifest_name", "") @@ -546,6 +607,11 @@ func (k *KubescapeTool) handleListVulnerabilitiesInManifest(ctx context.Context, return mcp.NewToolResultError("manifest_name parameter is required"), nil } + if err := k.ensureClients(); err != nil { + toolErr := errors.NewKubescapeError("list_vulnerabilities", err) + return toolErr.ToMCPResult(), nil + } + manifest, err := k.spdxClient.VulnerabilityManifests(namespace).Get(ctx, manifestName, metav1.GetOptions{}) if err != nil { toolErr := errors.NewKubescapeError("get_vulnerability_manifest", err). @@ -606,11 +672,6 @@ func (k *KubescapeTool) handleListVulnerabilitiesInManifest(ctx context.Context, // handleGetVulnerabilityDetails gets detailed info about a specific CVE in a manifest func (k *KubescapeTool) handleGetVulnerabilityDetails(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { - if k.initError != nil { - toolErr := errors.NewKubescapeError("get_vulnerability_details", k.initError) - return toolErr.ToMCPResult(), nil - } - namespace := mcp.ParseString(request, "namespace", defaultKubescapeNamespace) manifestName := mcp.ParseString(request, "manifest_name", "") cveID := mcp.ParseString(request, "cve_id", "") @@ -622,6 +683,11 @@ func (k *KubescapeTool) handleGetVulnerabilityDetails(ctx context.Context, reque return mcp.NewToolResultError("cve_id parameter is required"), nil } + if err := k.ensureClients(); err != nil { + toolErr := errors.NewKubescapeError("get_vulnerability_details", err) + return toolErr.ToMCPResult(), nil + } + manifest, err := k.spdxClient.VulnerabilityManifests(namespace).Get(ctx, manifestName, metav1.GetOptions{}) if err != nil { toolErr := errors.NewKubescapeError("get_vulnerability_manifest", err). @@ -652,8 +718,8 @@ func (k *KubescapeTool) handleGetVulnerabilityDetails(ctx context.Context, reque // handleListConfigurationScans lists configuration security scan results func (k *KubescapeTool) handleListConfigurationScans(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { - if k.initError != nil { - toolErr := errors.NewKubescapeError("list_configuration_scans", k.initError) + if err := k.ensureClients(); err != nil { + toolErr := errors.NewKubescapeError("list_configuration_scans", err) return toolErr.ToMCPResult(), nil } @@ -696,11 +762,6 @@ func (k *KubescapeTool) handleListConfigurationScans(ctx context.Context, reques // handleGetConfigurationScan gets details of a specific configuration scan func (k *KubescapeTool) handleGetConfigurationScan(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { - if k.initError != nil { - toolErr := errors.NewKubescapeError("get_configuration_scan", k.initError) - return toolErr.ToMCPResult(), nil - } - namespace := mcp.ParseString(request, "namespace", defaultKubescapeNamespace) manifestName := mcp.ParseString(request, "manifest_name", "") @@ -708,6 +769,11 @@ func (k *KubescapeTool) handleGetConfigurationScan(ctx context.Context, request return mcp.NewToolResultError("manifest_name parameter is required"), nil } + if err := k.ensureClients(); err != nil { + toolErr := errors.NewKubescapeError("get_configuration_scan", err) + return toolErr.ToMCPResult(), nil + } + manifest, err := k.spdxClient.WorkloadConfigurationScans(namespace).Get(ctx, manifestName, metav1.GetOptions{}) if err != nil { toolErr := errors.NewKubescapeError("get_configuration_scan", err). @@ -726,8 +792,8 @@ func (k *KubescapeTool) handleGetConfigurationScan(ctx context.Context, request // handleListApplicationProfiles lists application profiles showing runtime behavior data func (k *KubescapeTool) handleListApplicationProfiles(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { - if k.initError != nil { - toolErr := errors.NewKubescapeError("list_application_profiles", k.initError) + if err := k.ensureClients(); err != nil { + toolErr := errors.NewKubescapeError("list_application_profiles", err) return toolErr.ToMCPResult(), nil } @@ -801,11 +867,6 @@ func (k *KubescapeTool) handleListApplicationProfiles(ctx context.Context, reque // handleGetApplicationProfile gets detailed runtime behavior for a specific workload func (k *KubescapeTool) handleGetApplicationProfile(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { - if k.initError != nil { - toolErr := errors.NewKubescapeError("get_application_profile", k.initError) - return toolErr.ToMCPResult(), nil - } - namespace := mcp.ParseString(request, "namespace", "") name := mcp.ParseString(request, "name", "") @@ -816,6 +877,11 @@ func (k *KubescapeTool) handleGetApplicationProfile(ctx context.Context, request return mcp.NewToolResultError("namespace parameter is required"), nil } + if err := k.ensureClients(); err != nil { + toolErr := errors.NewKubescapeError("get_application_profile", err) + return toolErr.ToMCPResult(), nil + } + profile, err := k.spdxClient.ApplicationProfiles(namespace).Get(ctx, name, metav1.GetOptions{}) if err != nil { toolErr := errors.NewKubescapeError("get_application_profile", err). @@ -877,8 +943,8 @@ func (k *KubescapeTool) handleGetApplicationProfile(ctx context.Context, request // handleListNetworkNeighborhoods lists network communication patterns for workloads func (k *KubescapeTool) handleListNetworkNeighborhoods(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { - if k.initError != nil { - toolErr := errors.NewKubescapeError("list_network_neighborhoods", k.initError) + if err := k.ensureClients(); err != nil { + toolErr := errors.NewKubescapeError("list_network_neighborhoods", err) return toolErr.ToMCPResult(), nil } @@ -935,11 +1001,6 @@ func (k *KubescapeTool) handleListNetworkNeighborhoods(ctx context.Context, requ // handleGetNetworkNeighborhood gets detailed network connections for a specific workload func (k *KubescapeTool) handleGetNetworkNeighborhood(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { - if k.initError != nil { - toolErr := errors.NewKubescapeError("get_network_neighborhood", k.initError) - return toolErr.ToMCPResult(), nil - } - namespace := mcp.ParseString(request, "namespace", "") name := mcp.ParseString(request, "name", "") @@ -950,6 +1011,11 @@ func (k *KubescapeTool) handleGetNetworkNeighborhood(ctx context.Context, reques return mcp.NewToolResultError("namespace parameter is required"), nil } + if err := k.ensureClients(); err != nil { + toolErr := errors.NewKubescapeError("get_network_neighborhood", err) + return toolErr.ToMCPResult(), nil + } + nn, err := k.spdxClient.NetworkNeighborhoods(namespace).Get(ctx, name, metav1.GetOptions{}) if err != nil { toolErr := errors.NewKubescapeError("get_network_neighborhood", err). diff --git a/pkg/kubescape/testing.go b/pkg/kubescape/testing.go index 093e371..3ee42a6 100644 --- a/pkg/kubescape/testing.go +++ b/pkg/kubescape/testing.go @@ -1,6 +1,8 @@ package kubescape import ( + "time" + spdxv1beta1 "github.com/kubescape/storage/pkg/generated/clientset/versioned/typed/softwarecomposition/v1beta1" apiextensionsclientset "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset" "k8s.io/client-go/kubernetes" @@ -16,13 +18,18 @@ func NewKubescapeToolWithClients( k8sClient: k8sClient, apiExtClient: apiExtClient, spdxClient: spdxClient, - initError: nil, + now: time.Now, } } -// NewKubescapeToolWithError creates a KubescapeTool with an initialization error for testing error paths +// NewKubescapeToolWithError creates a KubescapeTool with an initialization error for testing error paths. +// +// The failure is permanent: the injected builder always returns err, so the +// tool never falls back to whatever kubeconfig the test machine happens to +// have once the retry interval elapses. func NewKubescapeToolWithError(err error) *KubescapeTool { return &KubescapeTool{ - initError: err, + buildClients: func() (*kubescapeClients, error) { return nil, err }, + now: time.Now, } }