NO-JIRA: chore(extended/prometheus): reduce targets-auth skip list and handle mTLS#31372
NO-JIRA: 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 |
openshift-ci
Bot
added
the
approved
| } | ||
| 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: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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: |
machine424
changed the title
openshift-ci
Bot
added
the
ready-for-human-review
|
Scheduling required tests: |
jan--f
left a comment
jan--f
left a comment
There was a problem hiding this comment.
/lgtm
one optional nit, but feel free to ignore.
| } | ||
|
|
||
| // Set $MONITORING_AUTH_TEST_NAMESPACE to focus on the targets from a single namespace | ||
| var namespaceUnderTest = os.Getenv("MONITORING_AUTH_TEST_NAMESPACE") |
There was a problem hiding this comment.
Maybe a short comment why this is no longer needed can be helpful to future readers.
There was a problem hiding this comment.
I can leave the comment here:
this is no longer needed and was making the logic hard to read.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jan--f, 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 |
|
/retitle NO_JIRA: chore(extended/prometheus): reduce targets-auth skip list and handle mTLS |
openshift-ci
Bot
changed the title
|
/verified by existing tests |
openshift-ci-robot
added
the
verified
|
@machine424: This PR has been marked as verified by 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. |
|
/retitle NO-JIRA: chore(extended/prometheus): reduce targets-auth skip list and handle mTLS |
|
/jira refresh |
|
@machine424: No Jira issue is referenced in the title of this pull request. 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. |
openshift-ci
Bot
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@machine424: This pull request explicitly references no jira issue. 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. |
|
@machine424: The following test 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. |
3 participants
from ci/prow/e2e-aws-ovn-serial-1of2 (link).
Summary by CodeRabbit