feat(kiali): API based filtering for Kiali - #1339
Conversation
17672b7 to
4d6f90c
Compare
|
👋 Heads up — this pull request changes files owned by @aljesusg. You are listed as an owner of one or more of the changed areas in |
cajieh
left a comment
There was a problem hiding this comment.
Could we add an MCP ListTools test similar to kubevirt’s TestKubevirtToolsFilteredWithoutCRDs, so we assert Kiali tools are hidden when the experimental flag is on and the Kiali CRD is missing?
Added comments. Otherwise, it looks good. Thank you!
| var KialiGVK = schema.GroupVersionKind{ | ||
| Group: "kiali.io", | ||
| Version: "v1alpha1", | ||
| Kind: "Kiali", |
There was a problem hiding this comment.
Is filtering on the Kiali Operator CRD (kiali.io/v1alpha1) intentional for OSSM, or should Helm/standalone installs without that CRD still expose tools when [toolset_configs.kiali].url is set?
There was a problem hiding this comment.
@cajieh Thanks for the review. Added an MCP ListTools test similar to TestKubevirtToolsFilteredWithoutCRDs, asserting Kiali tools are hidden when the experimental flag is on and the Kiali Operator CRD is missing.
On the CRD question: filtering on kiali.io/v1alpha1 alone is intentional for OSSM/operator installs, but Helm/standalone installs do not register that CRD and talk to Kiali only via [toolset_configs.kiali].url. Because the tools call the Kiali HTTP API (not the Operator CR), HasKiali now treats Kiali as available if either the Operator GVK is present or a non-empty URL is configured. The MCP server wraps the filtering provider with the live config so that URL check sees current toolset_configs without changing the GetTools API.
Signed-off-by: Josune Cordoba <jcordoba@redhat.com>
Signed-off-by: Josune Cordoba <jcordoba@redhat.com>
Signed-off-by: Josune Cordoba <jcordoba@redhat.com>
4168160 to
ab61eb5
Compare
Signed-off-by: Josune Cordoba <jcordoba@redhat.com>
Cali0707
left a comment
There was a problem hiding this comment.
Thanks for starting on this @josunect !
I left some comments inline, but they may or may not be valid long term as I think there is a larger architectural consideration we need to work through first.
Fundamentally the FilteringProvider was designed to hide cluster-specific details behind a stable abstraction, so that we can swap things to e.g. leverage ACM search instead of iterating over clusters in some setups. However, Kiali is now fundamentally ClusterAware = false after #1224, as it is a mesh control that can span clusters.
This leads to the type assertion from the FilteringProvider to kubernetes.Provider, which IMO we shouldn't be doing. We should either:
- explicitly pass that provider in
- don't assert and expect it to be there
A key reason for this is that doing this and then looking at the default provider only or hand rolling the multi cluster checks by iterating providers blocks us from making optimizations based on the specifics of the multi cluster setup.
I wonder if maybe the correct fix is we make a kube client that internally checks every target rather than exposing a single method like HasGVKs which iterates over the targets? Then you could have a more expressive API and we could still make changes based on the multi cluster setup?
cc @2uasimojo since you have put a lot of thought into this part of the codebase
| // URL is optional: when empty, HasKiali may discover an in-cluster Service URL | ||
| // from a Kiali CR and inject it into this config at runtime. | ||
| if strings.TrimSpace(c.Url) == "" { | ||
| if caValue := strings.TrimSpace(c.CertificateAuthority); caValue != "" { | ||
| if _, err := os.Stat(caValue); err != nil { | ||
| return fmt.Errorf("certificate_authority must be a valid file path: %w", err) | ||
| } | ||
| } | ||
| return nil |
There was a problem hiding this comment.
So HasKiali will only be called if experimental_enable_target_compatibility_tool_filers=true which is not the default. Which means by default this may allow some configs to pass validation that it should not...
Maybe we can at a minimum log a warning in this case?
| func injectDiscoveredURL(cfg *Config, cfgOK bool, url string) { | ||
| setDiscoveredURL(url) | ||
| if cfgOK && cfg != nil { | ||
| cfg.Url = url |
There was a problem hiding this comment.
This write technically isn't synchronized wrt to reads as far as I can tell
| var available bool | ||
| return func() bool { | ||
| once.Do(func() { | ||
| available = evaluateKialiAvailability(context.TODO(), p) |
There was a problem hiding this comment.
If we need to use context.TODO() in places just because the TargetCompatibilityFilters api doesn't pass a ctx parameter in, maybe we should add the ctx paramter to that method?
cc @2uasimojo for input
There was a problem hiding this comment.
maybe we should add the ctx paramter to that method?
(FMR, the method def)
I don't object to adding a context param there, but the prototype is super generic for a reason. And we generally expect the impl to be defined in a closure like we've got here. So if we need a context param, it could be plumbed through HasKiali() itself. WDYT?
| klogutil.FromContext(ctx).V(2).Info("kiali status probe: failed to create HTTP client", "error", err) | ||
| return false | ||
| } | ||
| client.Timeout = statusProbeTimeout |
There was a problem hiding this comment.
I don't think we need the timeout explicitly set since there is the context deadline on probeCtx here
|
|
||
| const ( | ||
| statusPath = "/api/status" | ||
| statusProbeTimeout = 3 * time.Second |
There was a problem hiding this comment.
I'm a little worried about this timeout slowing things down, as this will ultimately be called whenever the WatchTargets cluster-state changes (which can happen a lot in multi cluster envs).
Since this all runs synchronously each time it fires (and at startup) and we potentially run this against 6 urls, that is a 18 second delay.
Maybe we can:
- make these probes in parallel
- Tighten this timeout a decent amount?
2uasimojo
left a comment
There was a problem hiding this comment.
I like the part of this that makes it possible to run without a url.
I think it's fine to hide all the kiali tools if discovery/validation of the endpoint fails.
But I don't think TargetCompatibilityToolFilters (the ServerTool field) is the right mechanism for that. If we decide to condition the expensive discovery/validation behind the experimental_enable_target_compatibility_tool_filters option, it should be more like:
func GetTools(p) []api.ServerTool {
if p.IsTargetCompatibilityToolFiltersEnabled() && !HasKiali() {
return []
}
tools := slices.Concat(
...
)
for i := range tools {
// *No* TargetCompatibilityToolFilters here
tools[i].ClusterAware = ptr.To(false)
}
return tools
}
The ServerTool field is intended for filtering individual tools in a toolset.
| if cr == nil { | ||
| return nil | ||
| } | ||
| instanceName := nestedString(cr.Object, "status", "deployment", "instanceName") |
There was a problem hiding this comment.
I'll again mention my distaste for Unstructured and generic struct walking like this. I hope there's a really good reason we're not importing the schema and unmarshaling into a real typed struct.
| var available bool | ||
| return func() bool { | ||
| once.Do(func() { | ||
| available = evaluateKialiAvailability(context.TODO(), p) |
There was a problem hiding this comment.
maybe we should add the ctx paramter to that method?
(FMR, the method def)
I don't object to adding a context param there, but the prototype is super generic for a reason. And we generally expect the impl to be defined in a closure like we've got here. So if we need a context param, it could be plumbed through HasKiali() itself. WDYT?
Tests:
go test ./pkg/kiali/... ./pkg/toolsets/kiali/...kialis.kiali.io(withexperimental_enable_target_compatibility_tool_filters = true)To test the url auto discovery, the mcp must be deployed inside the cluster:
Prerequisites
Deploy MCP server in-cluster
load into your cluster if using Kind:
where values-kiali-discovery.yaml:
Key config (already in the example values):
Verify
In MCP Inspector (http://localhost:18080/mcp) or via API:
Negative checks
Troubleshooting