chore(extended/prometheus): reduce targets-auth skip list and handle mTLS#31372
chore(extended/prometheus): reduce targets-auth skip list and handle mTLS#31372machine424 wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
WalkthroughThe Prometheus unauthenticated-target test now uses a curl helper that returns HTTP status and error text, replaces environment-driven namespace filtering with local slices, and updates target polling, success checks, and failure reporting. ChangesPrometheus unauthorized-access test update
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: machine424 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 |
| } | ||
| networkPolicies := BuildNetworkPolicies(oc.Namespace(), ports) | ||
| for _, networkPolicy := range networkPolicies { | ||
| policies, err := oc.AdminKubeClient().NetworkingV1().NetworkPolicies(networkPolicy.Namespace).List(context.Background(), metav1.ListOptions{}) |
There was a problem hiding this comment.
the goal is to ensure auth is present even if NPs are bypassed.
deploying unneeded allow NP is not a problem.
|
/pipeline required |
|
Scheduling required tests: |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@test/extended/prometheus/prometheus.go`:
- Line 100: The test is still using context.Background() in the NetworkPolicies
Create call, which bypasses Ginkgo cancellation; update the affected It block in
prometheus.go to accept func(ctx g.SpecContext) and thread ctx through both the
oc.AdminKubeClient().NetworkingV1().NetworkPolicies(...).Create call and the
wait.PollUntilContextTimeout usage so the test respects spec cancellation.
In `@test/extended/util/prometheus/helpers.go`:
- Around line 306-309: The RunHostCmd handling in the prometheus helper is
dropping curl’s output as soon as err is non-nil, so the caller never gets the
--write-out text needed for the mTLS fallback. Update the helper around
e2eoutput.RunHostCmd to preserve and return output even when the command exits
non-zero, and let the caller in prometheus.go handle the exec error/retry
decision after parsing the output, using the existing host command/curl flow as
the entry point.
- Line 305: The `RunHostCmd` call in `helpers.go` is still vulnerable because
`fmt.Sprintf` with `%q` does not fully shell-quote `url` before `/bin/sh -c`
executes it. Update the helper around `RunHostCmd` to avoid shell interpretation
by switching to argv-based execution if available, or otherwise properly
shell-quote the URL and insert `--` before it so command substitutions cannot be
evaluated.
🪄 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), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 0f29d88c-5d99-430d-9410-5853afa0b80c
📒 Files selected for processing (2)
test/extended/prometheus/prometheus.gotest/extended/util/prometheus/helpers.go
|
Scheduling required tests: |
|
Scheduling required tests: |
|
@machine424: The following tests failed, say
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. |
Summary by CodeRabbit