Skip to content

chore(extended/prometheus): reduce targets-auth skip list and handle mTLS#31372

Open
machine424 wants to merge 1 commit into
openshift:mainfrom
machine424:promauth
Open

chore(extended/prometheus): reduce targets-auth skip list and handle mTLS#31372
machine424 wants to merge 1 commit into
openshift:mainfrom
machine424:promauth

Conversation

@machine424

@machine424 machine424 commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Summary by CodeRabbit

  • Tests
    • Improved Prometheus auth-related test coverage and reliability for unauthenticated target access checks.
    • Unauthorized target validations now report both HTTP status and curl error details for clearer diagnosis.
    • Reduced flaky behavior in namespace skipping by using deterministic in-test skip lists and consistent membership checks.
    • Simplified network policy setup for target ports and added explicit handling for certificate-required (mTLS) responses for the etcd target.

@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Pipeline controller notification
This repo is configured to use the pipeline controller. Second-stage tests will be triggered either automatically or after lgtm label is added, depending on the repository configuration. The pipeline controller will automatically detect which contexts are required and will utilize /test Prow commands to trigger the second stage.

For optional jobs, comment /test ? to see a list of all defined jobs. To trigger manually all jobs from second stage use /pipeline required command.

This repository is configured in: automatic mode

@coderabbitai

coderabbitai Bot commented Jul 9, 2026

Copy link
Copy Markdown

Walkthrough

The 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.

Changes

Prometheus unauthorized-access test update

Layer / File(s) Summary
Curl helper output parsing
test/extended/util/prometheus/helpers.go
Replaces URLStatusCodeExecViaPod with CurlExecViaPod, which parses combined curl status and error text and returns both values plus the exec error.
Namespace and policy setup
test/extended/prometheus/prometheus.go
Removes the env-driven namespace selection and set-based skip logic, then simplifies network-policy creation to always install the needed policies.
Unauthenticated scrape polling
test/extended/prometheus/prometheus.go
Switches polling to helper.CurlExecViaPod, adds the etcd certificate-required success path, and updates failure messages and refresh handling to use the new curl error text.

Estimated code review effort: 3 (Moderate) | ~25 minutes

🚥 Pre-merge checks | ✅ 15
✅ Passed checks (15 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names ✅ Passed The touched Ginkgo titles are static string literals; no runtime-derived names or interpolations were added.
Test Structure And Quality ✅ Passed The updated It block stays focused on unauthenticated scraping, uses deferred cleanup for created resources, and all cluster waits use PollUntilContextTimeout.
Microshift Test Compatibility ✅ Passed No new Ginkgo tests were added; this patch only refactors an existing serial spec/helper and adds no new MicroShift-unsupported API/resource use.
Single Node Openshift (Sno) Test Compatibility ✅ Passed The PR only edits an existing auth-scrape test/helper; it adds no new Ginkgo specs and introduces no multi-node or HA assumptions.
Topology-Aware Scheduling Compatibility ✅ Passed Only extended Prometheus test/helper code changed; no deployment manifests, operators, or controllers, and no new scheduling constraints were introduced.
Ote Binary Stdout Contract ✅ Passed No new stdout writes appear in main/init/TestMain/BeforeSuite/AfterSuite/RunSpecs setup; the only fmt.Printf is inside a regular helper called from a test body.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed The changed Ginkgo test uses cluster-internal routes/services and curl; no hardcoded IPv4, IP-family assumptions, or public internet access were introduced.
No-Weak-Crypto ✅ Passed The diff only changes Prometheus test helpers; no MD5/SHA1/DES/RC4/3DES/Blowfish/ECB, custom crypto, or secret/token comparisons were added.
Container-Privileges ✅ Passed Only Go test/helper files changed; no K8s manifests or container privilege fields were added or modified.
No-Sensitive-Data-In-Logs ✅ Passed The new logs only add curl error text and existing target URLs; no passwords, tokens, PII, or other secret-bearing values are logged.
Title check ✅ Passed The title accurately summarizes the main changes: reducing the Prometheus targets-auth skip list and adding mTLS handling.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@openshift-ci

openshift-ci Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

[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

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 9, 2026
@openshift-ci openshift-ci Bot requested review from jan--f and rexagod July 9, 2026 10:30
}
networkPolicies := BuildNetworkPolicies(oc.Namespace(), ports)
for _, networkPolicy := range networkPolicies {
policies, err := oc.AdminKubeClient().NetworkingV1().NetworkPolicies(networkPolicy.Namespace).List(context.Background(), metav1.ListOptions{})

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the goal is to ensure auth is present even if NPs are bypassed.
deploying unneeded allow NP is not a problem.

@machine424

Copy link
Copy Markdown
Contributor Author

/pipeline required

@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Scheduling required tests:
/test e2e-aws-csi
/test e2e-aws-ovn-fips
/test e2e-aws-ovn-microshift
/test e2e-aws-ovn-microshift-serial
/test e2e-aws-ovn-serial-1of2
/test e2e-aws-ovn-serial-2of2
/test e2e-gcp-csi
/test e2e-gcp-ovn
/test e2e-gcp-ovn-upgrade
/test e2e-metal-ipi-ovn-ipv6
/test e2e-vsphere-ovn
/test e2e-vsphere-ovn-upi

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 965e8a0 and 4701ad1.

📒 Files selected for processing (2)
  • test/extended/prometheus/prometheus.go
  • test/extended/util/prometheus/helpers.go

Comment thread test/extended/prometheus/prometheus.go
Comment thread test/extended/util/prometheus/helpers.go
Comment thread test/extended/util/prometheus/helpers.go Outdated
@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Scheduling required tests:
/test e2e-aws-csi
/test e2e-aws-ovn-fips
/test e2e-aws-ovn-microshift
/test e2e-aws-ovn-microshift-serial
/test e2e-aws-ovn-serial-1of2
/test e2e-aws-ovn-serial-2of2
/test e2e-gcp-csi
/test e2e-gcp-ovn
/test e2e-gcp-ovn-upgrade
/test e2e-metal-ipi-ovn-ipv6
/test e2e-vsphere-ovn
/test e2e-vsphere-ovn-upi

@machine424 machine424 changed the title chore(extended/prometheus): reduce targets-auth skip list and handle etcd mTLS chore(extended/prometheus): reduce targets-auth skip list and handle mTLS Jul 9, 2026
@openshift-ci openshift-ci Bot added the ready-for-human-review Indicates a PR has been reviewed by automated tools and is ready for human review label Jul 9, 2026
@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Scheduling required tests:
/test e2e-aws-csi
/test e2e-aws-ovn-fips
/test e2e-aws-ovn-microshift
/test e2e-aws-ovn-microshift-serial
/test e2e-aws-ovn-serial-1of2
/test e2e-aws-ovn-serial-2of2
/test e2e-gcp-csi
/test e2e-gcp-ovn
/test e2e-gcp-ovn-upgrade
/test e2e-metal-ipi-ovn-ipv6
/test e2e-vsphere-ovn
/test e2e-vsphere-ovn-upi

@openshift-ci

openshift-ci Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

@machine424: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-aws-ovn-fips 7e3002e link true /test e2e-aws-ovn-fips
ci/prow/e2e-vsphere-ovn-upi 7e3002e link true /test e2e-vsphere-ovn-upi

Full PR test history. Your PR dashboard.

Details

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. ready-for-human-review Indicates a PR has been reviewed by automated tools and is ready for human review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant