WIP: Ensure Platform Prometheus targets are not scraped with insecure_skip_verify#31373
WIP: Ensure Platform Prometheus targets are not scraped with insecure_skip_verify#31373machine424 wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
/pipeline required |
|
Scheduling required tests: |
openshift-ci
Bot
added
the
approved
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
WalkthroughAdds a new Ginkgo test in the Prometheus extended test suite that fetches scrape configuration from the Prometheus config API endpoint, parses the embedded YAML, and fails if any scrape config disables TLS CA verification via insecure_skip_verify. ChangesPrometheus TLS Verification Test
Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/extended/prometheus/prometheus.go (1)
146-169: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueBrittle substring match on TLS error text.
strings.Contains(curlErrMsg, "certificate required")couples the etcd pass condition to the exact wording emitted by whatever TLS backend curl is built against. If the backend (OpenSSL vs. others) or curl version phrases the mTLS handshake alert differently, this branch silently falls through to the retry/timeout path. Consider matching a broader signal (e.g.statusCode == 0 && err != nilfor the etcd/etcd job) or documenting the assumed curl/TLS backend.🤖 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 `@test/extended/prometheus/prometheus.go` around lines 146 - 169, The etcd mTLS exception in the polling closure is too dependent on the exact curl error text, so update the `wait.PollUntilContextTimeout` callback to avoid `strings.Contains(curlErrMsg, "certificate required")` as the deciding signal. In the `targetNs == "openshift-etcd"` and `targetJob == "etcd"` branch, prefer a broader condition based on the existing `statusCode == 0` and `err != nil` checks (or otherwise make the TLS assumption explicit in the test), so the `scrapeErr` logic in this block remains stable across curl/TLS backend differences.
🤖 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.
Nitpick comments:
In `@test/extended/prometheus/prometheus.go`:
- Around line 146-169: The etcd mTLS exception in the polling closure is too
dependent on the exact curl error text, so update the
`wait.PollUntilContextTimeout` callback to avoid `strings.Contains(curlErrMsg,
"certificate required")` as the deciding signal. In the `targetNs ==
"openshift-etcd"` and `targetJob == "etcd"` branch, prefer a broader condition
based on the existing `statusCode == 0` and `err != nil` checks (or otherwise
make the TLS assumption explicit in the test), so the `scrapeErr` logic in this
block remains stable across curl/TLS backend differences.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: b47b2f75-8acc-4c30-8669-d0dc97e770d5
📒 Files selected for processing (2)
test/extended/prometheus/prometheus.gotest/extended/util/prometheus/helpers.go
openshift-ci
Bot
added
the
ready-for-human-review
|
/pipeline required |
|
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. |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 3e2b3ca
New tests seen in this PR at sha: 3e2b3ca
|
1 participant
Summary by CodeRabbit