RHOBS-1607: Bump rhobs/obs-mcp to v0.4.0#341
Conversation
Signed-off-by: Jayapriya Pai <janantha@redhat.com>
Signed-off-by: Jayapriya Pai <janantha@redhat.com>
openshift-ci-robot
added
the
jira/valid-reference
|
@slashpai: This pull request references RHOBS-1607 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
📝 WalkthroughWalkthroughThis change adds a Loki logs toolset with discovery, querying, docs, and eval tasks, updates shared observability auth and tracing config, and refreshes vendored openapi, Prometheus, Kubernetes, x/net, and x/sys dependencies with related security and API changes. ChangesLogs toolset support
Vendored dependency refresh
Sequence Diagram(s)sequenceDiagram
participant Agent
participant LogsHandler as logs handler
participant Discovery as Loki discovery
participant Loki as Loki API
Agent->>LogsHandler: call loki_query_range
LogsHandler->>Discovery: resolve LokiStack URL
Discovery-->>LogsHandler: base URL
LogsHandler->>Loki: query labels or query_range
Loki-->>LogsHandler: JSON response
LogsHandler-->>Agent: tool result
Estimated code review effort🎯 5 (Critical) | ⏱️ ~110 minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
| - **[OADP](OADP.md)** - Tools for OpenShift API for Data Protection (Velero backups, restores, schedules) | ||
| - **[Kiali](KIALI.md)** - Tools for Kiali ServiceMesh with Istio | ||
| - **[KubeVirt](kubevirt.md)** - KubeVirt virtual machine management tools | ||
| - **[Observability](OBSERVABILITY.md)** - Tools for Prometheus metrics and Alertmanager alerts |
There was a problem hiding this comment.
This was non existing doc likely from before obs-mcp toolset
slashpai
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
vendor/github.com/rhobs/obs-mcp/pkg/logs/discovery/discovery.go (1)
33-46: ⚡ Quick winAvoid aborting discovery when one LokiStack entry is bad.
Returning immediately inside the
forloop means one malformed/unresolvable stack prevents all other valid instances from being discovered. SinceresolveLokiURL()inhandlers.godepends onListInstances()before selecting a single instance, this can block unrelated requests.♻️ Suggested direction
for _, item := range list.Items { var stack LokiStack if err := runtime.DefaultUnstructuredConverter.FromUnstructured(item.Object, &stack); err != nil { - return nil, fmt.Errorf("failed to parse LokiStack: %w", err) + continue } tenantsMode := "" if stack.Spec.Tenants != nil { tenantsMode = stack.Spec.Tenants.Mode } baseURL, err := resolveBaseURL(ctx, k8sClient, useRoute, stack.Namespace, stack.Name, tenantsMode) if err != nil { - return nil, err + continue } instances = append(instances, LokiInstance{🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vendor/github.com/rhobs/obs-mcp/pkg/logs/discovery/discovery.go` around lines 33 - 46, The for loop iterating through list.Items currently returns immediately on errors when parsing a LokiStack with runtime.DefaultUnstructuredConverter.FromUnstructured or when calling resolveBaseURL, which prevents discovery of subsequent valid stacks. Instead of returning nil on these errors, log the errors and continue to the next iteration using continue statements, allowing the function to accumulate all successfully resolved stacks and return them even if some individual entries fail to parse or resolve.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/observability/logs.md`:
- Line 24: Update the documentation in the loki_list_instances output section to
accurately reflect the actual handler implementation by removing the reference
to tenants and adding url to the list of returned fields. Additionally, review
the auth-mode section that appears around line 151 and update any references to
serviceaccount to instead reference the actual configuration contract which uses
header and kubeconfig parameters for authentication.
In `@vendor/github.com/go-openapi/loads/options.go`:
- Line 54: The `WithDocLoaderMatches` function unconditionally assigns the
result of `buildLoaderChain(l...)` to `opt.loader`, but since `buildLoaderChain`
can return `nil` when no usable loaders are provided, this causes a nil pointer
panic later in `loaderFromOptions` when attempting to call methods on the
loader. Fix this by adding a conditional check: only assign the result of
`buildLoaderChain(l...)` to `opt.loader` if it is not nil, otherwise preserve
the existing loader value. This ensures that `opts.loader` always remains valid
for subsequent operations.
In `@vendor/github.com/go-openapi/swag/loading/options.go`:
- Line 46: In the defer function on line 46, the error returned by r.Close() is
being discarded using the blank identifier. Instead of ignoring the error with _
= r.Close(), capture the error return value and handle it appropriately by
logging it or propagating it, ensuring that cleanup failures are not silently
ignored and that the code complies with the repository's error-handling
standards.
In `@vendor/github.com/prometheus/common/config/http_config.go`:
- Around line 1241-1247: The cloneRequest function performs a shallow copy that
causes r2.Header to reference the same map as r.Header. The maps.Copy call then
copies from the aliased map into itself, which is a no-op. To fix this, create a
new empty header map for r2.Header before calling maps.Copy so that the headers
are truly deep-copied into a separate map. This ensures that modifications to
headers in one request do not affect the cloned request.
In `@vendor/github.com/rhobs/obs-mcp/pkg/auth/auth.go`:
- Around line 75-99: The bearer token authorization wrapping at the end of the
function is unreachable when useTLS is false because the function returns early.
Move the bearer token authorization logic (the check for token != "" and the
NewAuthorizationCredentialsRoundTripper call) to execute regardless of whether
TLS is configured, ensuring that the Authorization header wrapper is applied to
both TLS and non-TLS transports. This can be done by repositioning the token
authorization wrapper logic after the TLS configuration block but ensuring it
applies to the rt variable before returning, or by restructuring the return
statements to apply the wrapper conditionally based on whether a token exists
rather than having it inside a conditional TLS block.
In `@vendor/github.com/rhobs/obs-mcp/pkg/logs/handlers.go`:
- Around line 220-243: The resolveLokiURL() method prevents the newLokiLoader
function from applying its built-in fallback logic by returning an error when no
explicit URL or instance parameters are provided. Instead of returning an error
at the end of the function when neither config.LokiURL nor
lokiNamespace/lokiName are provided, return an empty string to allow
newLokiLoader to proceed with its own fallback resolution path that handles
configured/environment/default URL resolution. Replace the final fmt.Errorf call
with a return of an empty string and nil error.
In `@vendor/github.com/rhobs/obs-mcp/pkg/logs/loki/loader.go`:
- Line 219: The deferred function containing resp.Body.Close() is currently
discarding the error using the blank identifier. Instead of ignoring the
returned error from Close(), capture the error value and handle it appropriately
by checking if it is not nil and logging or returning it as needed. Replace the
`_ = resp.Body.Close()` pattern in the defer statement with proper error
handling that validates and responds to any error returned by the Close()
operation.
---
Nitpick comments:
In `@vendor/github.com/rhobs/obs-mcp/pkg/logs/discovery/discovery.go`:
- Around line 33-46: The for loop iterating through list.Items currently returns
immediately on errors when parsing a LokiStack with
runtime.DefaultUnstructuredConverter.FromUnstructured or when calling
resolveBaseURL, which prevents discovery of subsequent valid stacks. Instead of
returning nil on these errors, log the errors and continue to the next iteration
using continue statements, allowing the function to accumulate all successfully
resolved stacks and return them even if some individual entries fail to parse or
resolve.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3eb92234-94c2-4fb0-b8b0-bfa9ead01bd5
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (96)
AGENTS.mddocs/README.mddocs/observability/logs.mddocs/observability/metrics.mddocs/observability/otelcol.mddocs/observability/tracing.mdevals/tasks/observability/logs/loki-backend-reachability.yamlevals/tasks/observability/logs/loki-label-names.yamlevals/tasks/observability/logs/loki-label-values.yamlevals/tasks/observability/logs/loki-list-instances.yamlevals/tasks/observability/logs/loki-query-network-flows.yamlgo.modvendor/github.com/go-openapi/loads/.gitignorevendor/github.com/go-openapi/loads/.golangci.ymlvendor/github.com/go-openapi/loads/CONTRIBUTORS.mdvendor/github.com/go-openapi/loads/README.mdvendor/github.com/go-openapi/loads/doc.govendor/github.com/go-openapi/loads/errors.govendor/github.com/go-openapi/loads/loaders.govendor/github.com/go-openapi/loads/options.govendor/github.com/go-openapi/loads/restricted.govendor/github.com/go-openapi/loads/spec.govendor/github.com/go-openapi/spec/.golangci.ymlvendor/github.com/go-openapi/spec/CONTRIBUTORS.mdvendor/github.com/go-openapi/spec/README.mdvendor/github.com/go-openapi/spec/header.govendor/github.com/go-openapi/spec/schema_loader.govendor/github.com/go-openapi/swag/CONTRIBUTORS.mdvendor/github.com/go-openapi/swag/README.mdvendor/github.com/go-openapi/swag/loading/doc.govendor/github.com/go-openapi/swag/loading/loading.govendor/github.com/go-openapi/swag/loading/options.govendor/github.com/go-openapi/validate/CONTRIBUTORS.mdvendor/github.com/go-openapi/validate/README.mdvendor/github.com/go-openapi/validate/spec.govendor/github.com/go-openapi/validate/spec_messages.govendor/github.com/go-openapi/validate/spec_ref_warnings.govendor/github.com/prometheus/alertmanager/api/v2/client/alert/get_alerts_parameters.govendor/github.com/prometheus/alertmanager/api/v2/client/alertgroup/get_alert_groups_parameters.govendor/github.com/prometheus/alertmanager/api/v2/client/receiver/get_receivers_parameters.govendor/github.com/prometheus/alertmanager/api/v2/client/receiver/get_receivers_responses.govendor/github.com/prometheus/alertmanager/api/v2/models/alert_group.govendor/github.com/prometheus/alertmanager/api/v2/models/gettable_alert.govendor/github.com/prometheus/alertmanager/api/v2/models/receiver.govendor/github.com/prometheus/alertmanager/api/v2/models/receiver_reference.govendor/github.com/prometheus/common/config/config.govendor/github.com/prometheus/common/config/http_config.govendor/github.com/prometheus/common/config/oauth_assertion.govendor/github.com/prometheus/common/expfmt/expfmt.govendor/github.com/prometheus/common/expfmt/text_create.govendor/github.com/prometheus/common/expfmt/text_parse.govendor/github.com/prometheus/common/model/labels.govendor/github.com/prometheus/common/model/labelset.govendor/github.com/prometheus/common/model/metric.govendor/github.com/prometheus/common/model/time.govendor/github.com/prometheus/common/model/value.govendor/github.com/prometheus/common/model/value_float.govendor/github.com/prometheus/common/model/value_histogram.govendor/github.com/rhobs/obs-mcp/pkg/auth/auth.govendor/github.com/rhobs/obs-mcp/pkg/auth/token.govendor/github.com/rhobs/obs-mcp/pkg/logs/common.govendor/github.com/rhobs/obs-mcp/pkg/logs/config.govendor/github.com/rhobs/obs-mcp/pkg/logs/definitions.govendor/github.com/rhobs/obs-mcp/pkg/logs/discovery/discovery.govendor/github.com/rhobs/obs-mcp/pkg/logs/discovery/types.govendor/github.com/rhobs/obs-mcp/pkg/logs/handlers.govendor/github.com/rhobs/obs-mcp/pkg/logs/loki/loader.govendor/github.com/rhobs/obs-mcp/pkg/logs/prompt.govendor/github.com/rhobs/obs-mcp/pkg/logs/toolset.govendor/github.com/rhobs/obs-mcp/pkg/logs/types.govendor/github.com/rhobs/obs-mcp/pkg/toolset/config/config.govendor/github.com/rhobs/obs-mcp/pkg/toolset/tools/loki_loader.govendor/github.com/rhobs/obs-mcp/pkg/toolset/tools/prometheus_client.govendor/github.com/rhobs/obs-mcp/pkg/toolset/toolset.govendor/github.com/rhobs/obs-mcp/pkg/traces/common.govendor/github.com/rhobs/obs-mcp/pkg/traces/config.govendor/golang.org/x/net/http2/server_wrap.govendor/golang.org/x/net/http2/transport_wrap.govendor/golang.org/x/sys/unix/ztypes_linux.govendor/golang.org/x/sys/unix/ztypes_linux_386.govendor/golang.org/x/sys/unix/ztypes_linux_amd64.govendor/golang.org/x/sys/unix/ztypes_linux_arm.govendor/golang.org/x/sys/unix/ztypes_linux_arm64.govendor/golang.org/x/sys/unix/ztypes_linux_loong64.govendor/golang.org/x/sys/unix/ztypes_linux_mips.govendor/golang.org/x/sys/unix/ztypes_linux_mips64.govendor/golang.org/x/sys/unix/ztypes_linux_mips64le.govendor/golang.org/x/sys/unix/ztypes_linux_mipsle.govendor/golang.org/x/sys/unix/ztypes_linux_ppc.govendor/golang.org/x/sys/unix/ztypes_linux_ppc64.govendor/golang.org/x/sys/unix/ztypes_linux_ppc64le.govendor/golang.org/x/sys/unix/ztypes_linux_riscv64.govendor/golang.org/x/sys/unix/ztypes_linux_s390x.govendor/golang.org/x/sys/unix/ztypes_linux_sparc64.govendor/k8s.io/apimachinery/pkg/api/validation/objectmeta.govendor/modules.txt
|
|
||
| **Parameters:** none. | ||
|
|
||
| **Output:** JSON per instance includes namespace, name, tenants, and status. Use `lokiNamespace`, `lokiName`, and `tenant` as parameters on all other Loki tools. |
There was a problem hiding this comment.
Align the guide text with the implemented logs contract.
Two lines appear inconsistent with current code/config behavior:
- Output for
loki_list_instancesis documented withtenants, but handlers currently return namespace/name/status/url. - The auth-mode section references
serviceaccount, while the logs config contract documentsheaderandkubeconfig.
📝 Suggested wording updates
- **Output:** JSON per instance includes namespace, name, tenants, and status. Use `lokiNamespace`, `lokiName`, and `tenant` as parameters on all other Loki tools.
+ **Output:** JSON per instance includes `lokiNamespace`, `lokiName`, `status`, and resolved `url`. Use `lokiNamespace`, `lokiName`, and `tenant` as parameters on other Loki tools.
- In `header` and `serviceaccount` modes, you can either set `loki_url` **or** use LokiStack discovery (`loki_list_instances` + `lokiNamespace`/`lokiName` arguments on each tool call).
+ In `header` mode, you can either set `loki_url` **or** use LokiStack discovery (`loki_list_instances` + `lokiNamespace`/`lokiName` arguments on each tool call).Also applies to: 151-151
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/observability/logs.md` at line 24, Update the documentation in the
loki_list_instances output section to accurately reflect the actual handler
implementation by removing the reference to tenants and adding url to the list
of returned fields. Additionally, review the auth-mode section that appears
around line 151 and update any references to serviceaccount to instead reference
the actual configuration contract which uses header and kubeconfig parameters
for authentication.
| prev = prev.WithNext(&loader{DocLoaderWithMatch: ldr}) | ||
| } | ||
| opt.loader = final | ||
| opt.loader = buildLoaderChain(l...) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the nil path statically:
rg -n -C3 'func WithDocLoaderMatches|buildLoaderChain|func loaderFromOptions|l\.loadingOptions = opts\.loadingOptions' \
vendor/github.com/go-openapi/loads/options.go \
vendor/github.com/go-openapi/loads/loaders.goRepository: openshift/openshift-mcp-server
Length of output: 3069
🏁 Script executed:
rg -n -A10 'func defaultOptions' vendor/github.com/go-openapi/loads/options.goRepository: openshift/openshift-mcp-server
Length of output: 316
🏁 Script executed:
rg -n -B5 -A5 'var loaders|^loaders =' vendor/github.com/go-openapi/loads/Repository: openshift/openshift-mcp-server
Length of output: 885
🏁 Script executed:
rg -n 'func.*clone' vendor/github.com/go-openapi/loads/loaders.goRepository: openshift/openshift-mcp-server
Length of output: 113
🏁 Script executed:
rg -n -A10 'func \(l \*loader\) clone' vendor/github.com/go-openapi/loads/loaders.goRepository: openshift/openshift-mcp-server
Length of output: 343
Prevent nil loader assignment in WithDocLoaderMatches
At Line 54, opt.loader is set to buildLoaderChain(l...) unconditionally. Since buildLoaderChain returns nil when no usable loaders are provided, this causes loaderFromOptions to panic at line 26 when calling opts.loader.clone() followed by accessing l.loadingOptions on a nil pointer. Keep the existing loader when no usable chain is built.
Proposed fix
func WithDocLoaderMatches(l ...DocLoaderWithMatch) LoaderOption {
return func(opt *options) {
- opt.loader = buildLoaderChain(l...)
+ if chain := buildLoaderChain(l...); chain != nil {
+ opt.loader = chain
+ }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| opt.loader = buildLoaderChain(l...) | |
| func WithDocLoaderMatches(l ...DocLoaderWithMatch) LoaderOption { | |
| return func(opt *options) { | |
| if chain := buildLoaderChain(l...); chain != nil { | |
| opt.loader = chain | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@vendor/github.com/go-openapi/loads/options.go` at line 54, The
`WithDocLoaderMatches` function unconditionally assigns the result of
`buildLoaderChain(l...)` to `opt.loader`, but since `buildLoaderChain` can
return `nil` when no usable loaders are provided, this causes a nil pointer
panic later in `loaderFromOptions` when attempting to call methods on the
loader. Fix this by adding a conditional check: only assign the result of
`buildLoaderChain(l...)` to `opt.loader` if it is not nil, otherwise preserve
the existing loader value. This ensures that `opts.loader` always remains valid
for subsequent operations.
| if err != nil { | ||
| return nil, errors.Join(err, ErrLoader) | ||
| } | ||
| defer func() { _ = r.Close() }() |
There was a problem hiding this comment.
Handle the r.Close() error instead of discarding it.
Line 46 ignores a returned error from Close(), which can hide cleanup failures and violates the Go error-handling rule in this repo.
Suggested fix
- return func(name string) ([]byte, error) {
+ return func(name string) (data []byte, err error) {
r, err := os.OpenRoot(root)
if err != nil {
return nil, errors.Join(err, ErrLoader)
}
- defer func() { _ = r.Close() }()
+ defer func() {
+ if closeErr := r.Close(); closeErr != nil {
+ err = errors.Join(err, closeErr, ErrLoader)
+ }
+ }()
- return r.ReadFile(name)
+ data, err = r.ReadFile(name)
+ if err != nil {
+ return nil, errors.Join(err, ErrLoader)
+ }
+ return data, nil
}As per coding guidelines: "Never ignore errors in production code; always check and handle errors from functions that return them."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@vendor/github.com/go-openapi/swag/loading/options.go` at line 46, In the
defer function on line 46, the error returned by r.Close() is being discarded
using the blank identifier. Instead of ignoring the error with _ = r.Close(),
capture the error return value and handle it appropriately by logging it or
propagating it, ensuring that cleanup failures are not silently ignored and that
the code complies with the repository's error-handling standards.
Source: Coding guidelines
| func cloneRequest(r *http.Request) *http.Request { | ||
| // Shallow copy of the struct. | ||
| r2 := new(http.Request) | ||
| *r2 = *r | ||
| // Deep copy of the Header. | ||
| r2.Header = make(http.Header) | ||
| for k, s := range r.Header { | ||
| r2.Header[k] = s | ||
| } | ||
| maps.Copy(r.Header, r2.Header) | ||
| return r2 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -A8 -B4 'func cloneRequest\(|maps\.Copy\(|r2\.Header = make' vendor/github.com/prometheus/common/config/http_config.goRepository: openshift/openshift-mcp-server
Length of output: 628
🌐 Web query:
Go maps.Copy function signature semantics golang.org
💡 Result:
The maps.Copy function in Go is defined in the standard library maps package [1][2]. Its function signature is: func Copy[M1 ~map[K]V, M2 ~map[K]V, K comparable, V any](dst M1, src M2) Semantics and Behavior: 1. Operation: It copies all key-value pairs from the source map (src) into the destination map (dst) [1][2]. 2. Overwriting: If a key from the source map already exists in the destination map, the value in the destination map is overwritten by the corresponding value from the source map [1][3]. 3. Type Constraints: The function uses generics. M1 and M2 are restricted to map types (using the ~ approximation operator for type inference) that share the same key (K) and value (V) types [1][2]. The key type (K) must be comparable, while the value type (V) can be any type [1][2]. 4. Shallow Copy: Note that maps.Copy performs a shallow copy [2]. If the map values are references (e.g., slices, pointers, or other maps), the copy operation will copy the reference rather than creating a deep copy of the underlying data [1][3]. Changes to the internal state of those referenced objects in the destination map will be reflected in the source map, and vice-versa [1][3].
Citations:
cloneRequest no longer deep-copies headers (Line 1246).
After the shallow struct copy (*r2 = *r), r2.Header aliases the same map as r.Header. The subsequent maps.Copy(r.Header, r2.Header) copies from the aliased map into itself—a no-op that leaves headers shared across requests. This breaks the deep-copy intent and can leak/mutate headers across wrapper layers.
Suggested fix
func cloneRequest(r *http.Request) *http.Request {
// Shallow copy of the struct.
r2 := new(http.Request)
*r2 = *r
// Deep copy of the Header.
- maps.Copy(r.Header, r2.Header)
+ r2.Header = make(http.Header, len(r.Header))
+ for k, v := range r.Header {
+ r2.Header[k] = slices.Clone(v)
+ }
return r2
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func cloneRequest(r *http.Request) *http.Request { | |
| // Shallow copy of the struct. | |
| r2 := new(http.Request) | |
| *r2 = *r | |
| // Deep copy of the Header. | |
| r2.Header = make(http.Header) | |
| for k, s := range r.Header { | |
| r2.Header[k] = s | |
| } | |
| maps.Copy(r.Header, r2.Header) | |
| return r2 | |
| func cloneRequest(r *http.Request) *http.Request { | |
| // Shallow copy of the struct. | |
| r2 := new(http.Request) | |
| *r2 = *r | |
| // Deep copy of the Header. | |
| r2.Header = make(http.Header, len(r.Header)) | |
| for k, v := range r.Header { | |
| r2.Header[k] = slices.Clone(v) | |
| } | |
| return r2 | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@vendor/github.com/prometheus/common/config/http_config.go` around lines 1241
- 1247, The cloneRequest function performs a shallow copy that causes r2.Header
to reference the same map as r.Header. The maps.Copy call then copies from the
aliased map into itself, which is a no-op. To fix this, create a new empty
header map for r2.Header before calling maps.Copy so that the headers are truly
deep-copied into a separate map. This ensures that modifications to headers in
one request do not affect the cloned request.
| if !useTLS { | ||
| slog.Warn("Connecting without TLS") | ||
| return rt, nil | ||
| } | ||
|
|
||
| if insecure { | ||
| rt.TLSClientConfig = &tls.Config{ | ||
| MinVersion: tls.VersionTLS12, | ||
| InsecureSkipVerify: true, | ||
| } | ||
| } else { | ||
| certs, err := createCertPoolFromRESTConfig(restConfig) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| rt.TLSClientConfig = &tls.Config{ | ||
| MinVersion: tls.VersionTLS12, | ||
| RootCAs: certs, | ||
| } | ||
| } | ||
|
|
||
| if token != "" { | ||
| return promcfg.NewAuthorizationCredentialsRoundTripper( | ||
| "Bearer", promcfg.NewInlineSecret(token), rt), nil | ||
| } |
There was a problem hiding this comment.
Apply bearer auth for non-TLS endpoints as well.
Line 75 returns before Line 96, so when useTLS == false the Authorization wrapper is never applied. This drops auth headers for HTTP targets and breaks authenticated non-HTTPS backends.
🔧 Suggested fix
- if !useTLS {
- slog.Warn("Connecting without TLS")
- return rt, nil
- }
-
- if insecure {
- rt.TLSClientConfig = &tls.Config{
- MinVersion: tls.VersionTLS12,
- InsecureSkipVerify: true,
- }
- } else {
- certs, err := createCertPoolFromRESTConfig(restConfig)
- if err != nil {
- return nil, err
- }
- rt.TLSClientConfig = &tls.Config{
- MinVersion: tls.VersionTLS12,
- RootCAs: certs,
- }
- }
+ if !useTLS {
+ slog.Warn("Connecting without TLS")
+ } else if insecure {
+ rt.TLSClientConfig = &tls.Config{
+ MinVersion: tls.VersionTLS12,
+ InsecureSkipVerify: true,
+ }
+ } else {
+ certs, err := createCertPoolFromRESTConfig(restConfig)
+ if err != nil {
+ return nil, err
+ }
+ rt.TLSClientConfig = &tls.Config{
+ MinVersion: tls.VersionTLS12,
+ RootCAs: certs,
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if !useTLS { | |
| slog.Warn("Connecting without TLS") | |
| return rt, nil | |
| } | |
| if insecure { | |
| rt.TLSClientConfig = &tls.Config{ | |
| MinVersion: tls.VersionTLS12, | |
| InsecureSkipVerify: true, | |
| } | |
| } else { | |
| certs, err := createCertPoolFromRESTConfig(restConfig) | |
| if err != nil { | |
| return nil, err | |
| } | |
| rt.TLSClientConfig = &tls.Config{ | |
| MinVersion: tls.VersionTLS12, | |
| RootCAs: certs, | |
| } | |
| } | |
| if token != "" { | |
| return promcfg.NewAuthorizationCredentialsRoundTripper( | |
| "Bearer", promcfg.NewInlineSecret(token), rt), nil | |
| } | |
| if !useTLS { | |
| slog.Warn("Connecting without TLS") | |
| } else if insecure { | |
| rt.TLSClientConfig = &tls.Config{ | |
| MinVersion: tls.VersionTLS12, | |
| InsecureSkipVerify: true, | |
| } | |
| } else { | |
| certs, err := createCertPoolFromRESTConfig(restConfig) | |
| if err != nil { | |
| return nil, err | |
| } | |
| rt.TLSClientConfig = &tls.Config{ | |
| MinVersion: tls.VersionTLS12, | |
| RootCAs: certs, | |
| } | |
| } | |
| if token != "" { | |
| return promcfg.NewAuthorizationCredentialsRoundTripper( | |
| "Bearer", promcfg.NewInlineSecret(token), rt), nil | |
| } |
🧰 Tools
🪛 OpenGrep (1.22.0)
[ERROR] 81-84: TLS certificate verification is disabled via InsecureSkipVerify. This allows man-in-the-middle attacks. Remove InsecureSkipVerify or set it to false.
(coderabbit.tls.go-insecure-skip-verify)
[ERROR] 81-84: TLS certificate verification is disabled via InsecureSkipVerify. This allows man-in-the-middle attacks. Remove InsecureSkipVerify or set it to false.
(coderabbit.tls.go-insecure-skip-verify)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@vendor/github.com/rhobs/obs-mcp/pkg/auth/auth.go` around lines 75 - 99, The
bearer token authorization wrapping at the end of the function is unreachable
when useTLS is false because the function returns early. Move the bearer token
authorization logic (the check for token != "" and the
NewAuthorizationCredentialsRoundTripper call) to execute regardless of whether
TLS is configured, ensuring that the Authorization header wrapper is applied to
both TLS and non-TLS transports. This can be done by repositioning the token
authorization wrapper logic after the TLS configuration block but ensuring it
applies to the rt variable before returning, or by restructuring the return
statements to apply the wrapper conditionally based on whether a token exists
rather than having it inside a conditional TLS block.
| func (params ToolParams) resolveLokiURL() (string, error) { | ||
| if params.config != nil && params.config.LokiURL != "" { | ||
| return params.config.LokiURL, nil | ||
| } | ||
|
|
||
| namespace := tools.GetString(params.arguments, "lokiNamespace", "") | ||
| name := tools.GetString(params.arguments, "lokiName", "") | ||
| if namespace != "" || name != "" { | ||
| if namespace == "" || name == "" { | ||
| return "", fmt.Errorf("both lokiNamespace and lokiName must be provided together") | ||
| } | ||
| instances, err := discovery.ListInstances(params.context, params.dynamicClient, params.useRoute()) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| instance, err := discovery.FindInstanceByName(instances, namespace, name) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| return instance.GetURL(), nil | ||
| } | ||
|
|
||
| return "", fmt.Errorf("loki URL not configured; set loki_url/--loki-url/LOKI_URL or provide lokiNamespace and lokiName") | ||
| } |
There was a problem hiding this comment.
resolveLokiURL() blocks the loader’s built-in fallback path.
This method returns an error when no explicit URL or instance is provided, so newLokiLoader(url, tenant) never receives an empty URL and cannot apply its own fallback logic (configured/env/default resolution in pkg/toolset/tools/loki_loader.go).
🧩 Suggested fix
func (params ToolParams) resolveLokiURL() (string, error) {
if params.config != nil && params.config.LokiURL != "" {
return params.config.LokiURL, nil
}
@@
if namespace != "" || name != "" {
@@
return instance.GetURL(), nil
}
- return "", fmt.Errorf("loki URL not configured; set loki_url/--loki-url/LOKI_URL or provide lokiNamespace and lokiName")
+ // Allow downstream loader/config fallback behavior when no explicit URL is provided.
+ return "", nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (params ToolParams) resolveLokiURL() (string, error) { | |
| if params.config != nil && params.config.LokiURL != "" { | |
| return params.config.LokiURL, nil | |
| } | |
| namespace := tools.GetString(params.arguments, "lokiNamespace", "") | |
| name := tools.GetString(params.arguments, "lokiName", "") | |
| if namespace != "" || name != "" { | |
| if namespace == "" || name == "" { | |
| return "", fmt.Errorf("both lokiNamespace and lokiName must be provided together") | |
| } | |
| instances, err := discovery.ListInstances(params.context, params.dynamicClient, params.useRoute()) | |
| if err != nil { | |
| return "", err | |
| } | |
| instance, err := discovery.FindInstanceByName(instances, namespace, name) | |
| if err != nil { | |
| return "", err | |
| } | |
| return instance.GetURL(), nil | |
| } | |
| return "", fmt.Errorf("loki URL not configured; set loki_url/--loki-url/LOKI_URL or provide lokiNamespace and lokiName") | |
| } | |
| func (params ToolParams) resolveLokiURL() (string, error) { | |
| if params.config != nil && params.config.LokiURL != "" { | |
| return params.config.LokiURL, nil | |
| } | |
| namespace := tools.GetString(params.arguments, "lokiNamespace", "") | |
| name := tools.GetString(params.arguments, "lokiName", "") | |
| if namespace != "" || name != "" { | |
| if namespace == "" || name == "" { | |
| return "", fmt.Errorf("both lokiNamespace and lokiName must be provided together") | |
| } | |
| instances, err := discovery.ListInstances(params.context, params.dynamicClient, params.useRoute()) | |
| if err != nil { | |
| return "", err | |
| } | |
| instance, err := discovery.FindInstanceByName(instances, namespace, name) | |
| if err != nil { | |
| return "", err | |
| } | |
| return instance.GetURL(), nil | |
| } | |
| // Allow downstream loader/config fallback behavior when no explicit URL is provided. | |
| return "", nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@vendor/github.com/rhobs/obs-mcp/pkg/logs/handlers.go` around lines 220 - 243,
The resolveLokiURL() method prevents the newLokiLoader function from applying
its built-in fallback logic by returning an error when no explicit URL or
instance parameters are provided. Instead of returning an error at the end of
the function when neither config.LokiURL nor lokiNamespace/lokiName are
provided, return an empty string to allow newLokiLoader to proceed with its own
fallback resolution path that handles configured/environment/default URL
resolution. Replace the final fmt.Errorf call with a return of an empty string
and nil error.
| if err != nil { | ||
| return fmt.Errorf("loki request failed: %w", err) | ||
| } | ||
| defer func() { _ = resp.Body.Close() }() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and examine the code around line 219
file_path="vendor/github.com/rhobs/obs-mcp/pkg/logs/loki/loader.go"
if [ -f "$file_path" ]; then
echo "=== File found. Checking line count ==="
wc -l "$file_path"
echo ""
echo "=== Code around line 219 ==="
sed -n '210,230p' "$file_path"
else
echo "File not found: $file_path"
fiRepository: openshift/openshift-mcp-server
Length of output: 769
🏁 Script executed:
# Search for other instances of resp.Body.Close() or similar error-ignoring patterns in the file
rg "\.Close\(\)" vendor/github.com/rhobs/obs-mcp/pkg/logs/loki/loader.go -A 2 -B 2Repository: openshift/openshift-mcp-server
Length of output: 210
🏁 Script executed:
# Check if there's a log handler imported and available in this file
head -50 vendor/github.com/rhobs/obs-mcp/pkg/logs/loki/loader.go | grep -E "^import|log|slog"Repository: openshift/openshift-mcp-server
Length of output: 134
Handle resp.Body.Close() error instead of discarding it.
The deferred close currently ignores the returned error, which violates the repo's Go error-handling rule.
🛠️ Minimal fix
- defer func() { _ = resp.Body.Close() }()
+ defer func() {
+ if closeErr := resp.Body.Close(); closeErr != nil {
+ slog.Warn("failed to close Loki response body", "error", closeErr)
+ }
+ }()As per coding guidelines: "Never ignore errors in production code; always check and handle errors from functions that return them."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@vendor/github.com/rhobs/obs-mcp/pkg/logs/loki/loader.go` at line 219, The
deferred function containing resp.Body.Close() is currently discarding the error
using the blank identifier. Instead of ignoring the returned error from Close(),
capture the error value and handle it appropriately by checking if it is not nil
and logging or returning it as needed. Replace the `_ = resp.Body.Close()`
pattern in the defer statement with proper error handling that validates and
responds to any error returned by the Close() operation.
Source: Coding guidelines
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: iNecas, slashpai The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
…/observability Group the four obs-mcp toolset guides (metrics, tracing, logs, otelcol) under docs/observability/ to mirror the evals layout and keep the top-level docs/ directory focused. Adds new docs/observability/logs.md for the Loki/LogQL toolset introduced in obs-mcp v0.4.0.
|
New changes are detected. LGTM label has been removed. |
|
@slashpai: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@iNecas @slashpai I think some of the toolset names coming in here from the rh-obs package are a bit too generic. The tool names themselves seem great, but from a user config perspective Could you make these all namespaced, like |
4 participants
Also brings new toolset
logscc: @iNecas @matzew
Summary by CodeRabbit
Documentation
New Features
Tests
Chores
obs-mcp(v0.4.0), Kubernetes modules, and OpenAPI libraries