chore: switch auth mode to kubeconfig for kind e2e tests - #183
Conversation
Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>
|
Warning Review limit reached
Next review available in: 54 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe Estimated code review effort: 1 (Trivial) | ~3 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
With this one, I would actually prefer having e2e coverage for header mode, and either find a way to pass the headier in the mcpchecker scenario, or do ad-hoc configuration change before the evals are run. Alternatively, one can run obs-mcp locally with |
Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>
mcpchecker supports custom headers, however they are either static in I added a small
That works if logs+traces toolset are configured with |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@Makefile`:
- Line 270: Update the Makefile target around the mcp-config.yaml authorization
update to capture the kubectl token in a separate variable, verify the command
succeeds and the token is non-empty, and only then invoke yq to write the Bearer
authorization value. Preserve the existing configuration path and fail clearly
without modifying the file when token creation fails or returns no token.
- Line 281: Update the MCP checker recipe around the mcp-config mutation to save
the original configuration, install an exit trap that restores it on success,
failure, or interruption, and execute the mutation, mcpchecker check, and result
summary within the same shell. Restore the saved file rather than deleting the
entire .mcpServers.obs.headers object.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: b90d4304-718d-4fc8-a1d1-245312bfb811
📒 Files selected for processing (1)
Makefile
|
|
||
| .PHONY: run-mcpchecker-eval | ||
| run-mcpchecker-eval: $(MCPCHECKER) ## Run mcpchecker eval (TASK=name, CATEGORY=..., EVAL_CONFIG=eval.yaml, RUNS=3) | ||
| @yq -i '.mcpServers.obs.headers.Authorization = "Bearer '"$$(kubectl -n obs-mcp create token obs-mcp)"'"' $(MCPCHECKER_EVAL_DIR)/mcp-config.yaml |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Makefile around target =="
sed -n '230,295p' Makefile
echo
echo "== Search for mcp-config target and eval scripts =="
rg -n "mcp-config|obs-mcp|create token|Authorization|FAIL|FAILED|ERROR|yq" Makefile evals scripts *.sh 2>/dev/null || trueRepository: rhobs/obs-mcp
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Behavioral probe: demonstrate that an empty subshell return inside yq variable expansion
# lets yq execute successfully, while the shell overall may fail only if the command is run
# separately or yq itself fails.
cmd='kubectl -n obs-mcp create token obs-mcp'
python3 - <<'PY'
import subprocess, shlex
# Simulate "failed kubectl create token" producing no stdout and non-zero status.
result = subprocess.run(["false"], capture_output=True, text=True)
sub_output = result.stdout.strip()
yq_result = subprocess.run(["echo", f'Authorization = "Bearer {sub_output}"'], capture_output=True, text=True)
print({
"subshell_status": result.returncode,
"subshell_stdin_to_outer": repr(sub_output),
"equivalent_inner_command": subprocess.run(["sh", "-c", "kubectl create token obs-mcp || true"], capture_output=True, text=True).returncode,
"outer_yq_like_command_status": yq_result.returncode,
"outer_yq_like_output": yq_result.stdout.rstrip()
})
PYRepository: rhobs/obs-mcp
Length of output: 326
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== git status/stat =="
git diff --stat
echo
echo "== focused Makefile search for mktemp/mcp_config/token cleanup/eval commands =="
rg -n "mcp-check|mcp-checker|mcp_checker|mcp-config|obs-mcp|create token|Bearer|Authorization|mktemp|rm -f|kill|trap|evals/mcpchecker/eval.yaml|evals/mcpchecker/results" Makefile evals/mcpchecker 2>/dev/null | head -n 200Repository: rhobs/obs-mcp
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Makefile token generation lines =="
grep -n -C 5 'kubectl -n obs-mcp create token obs-mcp' Makefile || true
echo
echo "== focused Makefile lines containing mcp-checker related variable assignments =="
grep -n -A 8 -B 8 'MCPCHECKER_EVAL_DIR\|mcp-config\|Bearer\|Authorization' Makefile || true
echo
echo "== line count for eval output files that matched broad search issue =="
wc -l evals/mcpchecker/results/output.json 2>/dev/null || trueRepository: rhobs/obs-mcp
Length of output: 2945
Validate the token before modifying mcp-config.yaml.
If kubectl create token fails, the nested substitution can leave Authorization as Bearer while yq still exits successfully. Capture the token separately, check the command status and result, and only update the config if the token is present.
🤖 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 `@Makefile` at line 270, Update the Makefile target around the mcp-config.yaml
authorization update to capture the kubectl token in a separate variable, verify
the command succeeds and the token is non-empty, and only then invoke yq to
write the Bearer authorization value. Preserve the existing configuration path
and fail clearly without modifying the file when token creation fails or returns
no token.
| endif | ||
| $(MCPCHECKER) result summary $(MCPCHECKER_EVAL_DIR)/mcpchecker-obs-mcp-tools-out.json | ||
|
|
||
| @yq -i 'del(.mcpServers.obs.headers)' $(MCPCHECKER_EVAL_DIR)/mcp-config.yaml |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- Makefile context ---\n'
sed -n '240,305p' Makefile
printf '\n--- references to mcpconfig / bearer / evals ---\n'
rg -n "mcp-config|bearer|MCPCHECKER|evals/mcpchecker|result summary|mcpchecker check|yq" Makefile .github -S 2>/dev/null || true
printf '\n--- diff stat ---\n'
git diff --stat || trueRepository: rhobs/obs-mcp
Length of output: 242
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Makefile context ---'
sed -n '240,305p' Makefile
printf '%s\n' ''
printf '%s\n' '--- references to mcpconfig / bearer / evals ---'
rg -n "mcp-config|bearer|MCPCHECKER|evals/mcpchecker|result summary|mcpchecker check|yq" Makefile .github -S || true
printf '%s\n' ''
printf '%s\n' '--- diff stat ---'
git diff --stat || trueRepository: rhobs/obs-mcp
Length of output: 5129
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Makefile variables/macros before target ---'
sed -n '1,40p' Makefile
printf '%s\n' ''
printf '%s\n' '--- eval yaml/mcp config files ---'
git ls-files 'evals/mcpchecker/*' | while read -r f; do
printf '%s\n' "--- $f ---"
sed -n '1,220p' "$f"
done
printf '%s\n' ''
printf '%s\n' '--- Makefile invocation/reference snippets ---'
rg -n "run-mcpchecker-eval|evals/mcpchecker/mcpc |mcpchecker-obs-mcp-tools-out|publish-mcpchecker-evals" -S . || trueRepository: rhobs/obs-mcp
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
makefile="$(pwd)/Makefile"
python3 - <<'PY' "$makefile"
import sys
from pathlib import Path
p=Path(sys.argv[1])
text=p.read_text()
target="run-mcpchecker-eval:"
idx=text.find(target)
body=text[idx:]
start=body.index('\n')+1
end=body.find('\n\n')
target_body=text[idx:start+end]
print(target_body)
lines=[L.rstrip('\n') for L in target_body.splitlines()]
commands=[L.lstrip('\t\r ') for L in lines if L and L[0] not in '#@']
print("line_count =", len(text[:text.find(idx+end)].splitlines()) if target in open(sys.argv[1]).read() else "?")
print("commands_count =", len(commands))
for i,line in enumerate(lines,1):
stripped=line.lstrip('\t\r ')
print(f"{i}: {stripped}")
PY
printf '%s\n' '--- gitignore results artifacts ignored? ---'
sed -n '1,120p' evals/mcpchecker/.gitignoreRepository: rhobs/obs-mcp
Length of output: 258
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 <<'PY'
from pathlib import Path
makefile = Path('Makefile').read_text()
lines = makefile.splitlines()
for i, line in enumerate(lines, 1):
if line == 'run-mcpchecker-eval: $(MCPCHECKER) ## Run mcpchecker eval (TASK=name, CATEGORY=..., EVAL_CONFIG=eval.yaml, RUNS=3)':
start = i
break
else:
print("target not found")
raise SystemExit(0)
for i in range(start, min(start + 18, len(lines) + 1)):
print(f"{i}: {lines[i-1].replace(chr(9), '<tab>')}")
PY
printf '%s\n' '--- gitignore results artifacts ignored? ---'
cat evals/mcpchecker/.gitignoreRepository: rhobs/obs-mcp
Length of output: 1439
Restore the MCP config for all exits.
This cleanup only runs after result summary. If mcpchecker check fails or the command is interrupted, the bearer token stays in evals/mcpchecker/mcp-config.yaml. Run the mutation, mcpchecker check, and summary in one shell with an exit trap, and restore a saved copy instead of deleting the whole headers object.
🤖 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 `@Makefile` at line 281, Update the MCP checker recipe around the mcp-config
mutation to save the original configuration, install an exit trap that restores
it on success, failure, or interruption, and execute the mutation, mcpchecker
check, and result summary within the same shell. Restore the saved file rather
than deleting the entire .mcpServers.obs.headers object.
|
One thing worth checking publish-mcpchecker-evals: https://github.com/rhobs/obs-mcp/blob/main/Makefile#L283 has been copying the results, because there was nothing sensitive before: would be good to check the json output and sanitize. |
Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>
Good catch! The bearer token was in the output. I modified the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andreasgerstmayr, iNecas The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
2 participants
In #172 I hardened the auth logic to require an auth token in the header if the auth mode is set to
header. The mcpchecker evals don't send an auth header though, so the evals failed when run on kind.This PR changes the auth mode to
kubeconfigfor the Kubernetes overlay. On OpenShift we already use thekubeconfigauth mode.The downside of this change is that the
headerauth mode is not e2e-tested anymore. Thoughts?