Ensure Platform Prometheus targets are not scraped with insecure_skip_verify#31373
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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a Ginkgo test that retrieves Prometheus scrape configuration, parses the embedded YAML, and fails when non-allowlisted scrape jobs set ChangesPrometheus TLS Verification Test
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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: |
|
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
|
machine424
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
Scheduling required tests: |
danielmellado
left a comment
danielmellado
left a comment
There was a problem hiding this comment.
LGTM, besides what I said about what happens to this when the bugs are fixed. If you intend to do a follow-up PR then it's just fine, thanks!
| g.It("should not skip TLS CA verification", func() { | ||
| // TODO: remove the job when the bug is fixed. | ||
| jobsToSkip := []string{ | ||
| "serviceMonitor/openshift-apiserver/openshift-apiserver-operator-check-endpoints/0", // https://issues.redhat.com/browse/OCPBUGS-98917 | ||
| "serviceMonitor/openshift-network-diagnostics/network-check-source/0", // https://issues.redhat.com/browse/OCPBUGS-98918 | ||
| } |
There was a problem hiding this comment.
Once the bugs are fixed, these entries become silent dead weight. Optional hardening: after the loop, assert each allowlisted job was observed with insecure_skip_verify: true, and fail if an exception is unused so the TODO list self-cleans. Same pattern as “exclude list should be reduced to 0” elsewhere in this file may be good ;)
There was a problem hiding this comment.
I agree, but I don't think TRT and the other teams would appreciate sudden, unannounced test failures. It will create unnecessary noise, require extra triage resources, and potentially block CI for teams in other timezones while we aren't online to fix it.
I made a change to get logs like
job "serviceMonitor/openshift-apiserver/openshift-apiserver-operator-check-endpoints/0" has insecure_skip_verify=false (skip=true)
job "serviceMonitor/openshift-network-diagnostics/network-check-source/0" has insecure_skip_verify=false (skip=true)
once the bugs are fixed.
we can use those to update our tests.
There was a problem hiding this comment.
agree there, thanks for the explanation! lgtm!
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: machine424, simonpasquier 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 |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 5115c57
New tests seen in this PR at sha: 5115c57
|
…tion Scrape configs with insecure_skip_verify set to true do not validate the target's TLS certificate, leaving the connection vulnerable to man-in-the-middle attacks.
|
New changes are detected. LGTM label has been removed. |
|
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 tests seen in this PR at sha: e49f0f7
|
3 participants
Summary by CodeRabbit