refactor: include scheme in base URL for metrics #1243
Conversation
|
Skipping CI for Draft Pull Request. |
WalkthroughUpdated vulnerability silence timestamps and added two new entries; metrics test helpers refactored to use explicit baseURL/HTTPS and propagate baseURL into metric-fetch helpers; tests updated to use test server URLs. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
don't assume it's always `https`, so we can be more flexible when testing against an `http` endpoint Signed-off-by: Xavier Coulon <xcoulon@redhat.com>
xcoulon
force-pushed
the
metrics_getvalue_http
branch
from
a847e48 to
e987764
Compare
Signed-off-by: Xavier Coulon <xcoulon@redhat.com>
rsoaresd
left a comment
rsoaresd
left a comment
There was a problem hiding this comment.
Great work 🚀
Just minor question:
Signed-off-by: Xavier Coulon <xcoulon@redhat.com>
Signed-off-by: Xavier Coulon <xcoulon@redhat.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @testsupport/init.go:
- Line 210: The code hardcodes "https://" when setting initHostAwait.MetricsURL
(using hostMetricsRoute.Status.Ingress[0].Host) which contradicts the PR goal to
derive the scheme from the route TLS config; change the assignment to choose
"https://" if hostMetricsRoute.Spec.TLS is non-nil/has entries and "http://"
otherwise, following the same pattern used for the registration URL (the
conditional logic around route.Spec.TLS in the earlier block), and apply the
same fix for the similar assignment at the other occurrence (line 215) so both
metrics URLs use the correct scheme based on hostMetricsRoute.Spec.TLS.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
testsupport/metrics/metrics.go (1)
145-145: Inconsistent bearer token handling.
GetMetricLabelsunconditionally adds the Authorization header without checking if the bearer token is empty, unlikegetMetricValue(lines 49-51) which includes this check. This creates an inconsistency in how these functions handle authentication.🔎 Proposed fix to add bearer token check
if err != nil { return nil, err } - request.Header.Add("Authorization", fmt.Sprintf("Bearer %s", restConfig.BearerToken)) + if restConfig.BearerToken != "" { + request.Header.Add("Authorization", fmt.Sprintf("Bearer %s", restConfig.BearerToken)) + } resp, err := client.Do(request)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.govulncheck.yamltestsupport/init.gotestsupport/metrics/metrics.gotestsupport/metrics/metrics_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
testsupport/metrics/metrics_test.go (1)
testsupport/metrics/metrics.go (1)
GetMetricValue(16-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build & push operator bundles & dashboard image for e2e tests
- GitHub Check: Unit Tests
- GitHub Check: Build & push operator bundles & dashboard image for e2e tests
🔇 Additional comments (3)
.govulncheck.yaml (1)
7-7: LGTM!The updated silence-until timestamps appropriately extend the deadline for these known vulnerabilities while they remain unpatched in the current Go version.
Also applies to: 13-13, 19-19, 25-25, 31-31, 37-37, 43-43
testsupport/metrics/metrics_test.go (1)
67-67: LGTM!The test changes correctly align with the
baseURLrefactoring. Usingts.URLdirectly (which includes the scheme fromhttptest.NewTLSServer) is cleaner and more accurate than the previous approach.Also applies to: 75-75, 83-83, 91-91, 101-101, 110-110, 119-119
testsupport/metrics/metrics.go (1)
16-17: LGTM!The refactoring from
urltobaseURLwith direct concatenation (baseURL + "/metrics") correctly implements the PR objective of including the scheme in the base URL, rather than assuming "https://".Also applies to: 24-25, 32-32, 36-36
| hostMetricsRoute, err := initHostAwait.SetupRouteForService(t, "host-operator-metrics-service", "/metrics") | ||
| require.NoError(t, err) | ||
| initHostAwait.MetricsURL = hostMetricsRoute.Status.Ingress[0].Host | ||
| initHostAwait.MetricsURL = "https://" + hostMetricsRoute.Status.Ingress[0].Host |
There was a problem hiding this comment.
Hardcoded "https://" scheme contradicts the PR objective.
The PR aims to "include the URL scheme (rather than assuming 'https') when constructing the base URL for metrics, allowing testing against an 'http' endpoint." However, these lines still hardcode "https://" for the metrics URLs.
Follow the pattern established for the registration service URL (lines 100-104), which conditionally checks route.Spec.TLS to determine the appropriate scheme.
🔎 Proposed fix to determine scheme based on TLS configuration
For line 210:
- initHostAwait.MetricsURL = "https://" + hostMetricsRoute.Status.Ingress[0].Host
+ hostMetricsURL := "http://" + hostMetricsRoute.Status.Ingress[0].Host
+ if hostMetricsRoute.Spec.TLS != nil {
+ hostMetricsURL = "https://" + hostMetricsRoute.Status.Ingress[0].Host
+ }
+ initHostAwait.MetricsURL = hostMetricsURLFor line 215:
- initMemberAwait.MetricsURL = "https://" + memberMetricsRoute.Status.Ingress[0].Host
+ memberMetricsURL := "http://" + memberMetricsRoute.Status.Ingress[0].Host
+ if memberMetricsRoute.Spec.TLS != nil {
+ memberMetricsURL = "https://" + memberMetricsRoute.Status.Ingress[0].Host
+ }
+ initMemberAwait.MetricsURL = memberMetricsURLAlso applies to: 215-215
🤖 Prompt for AI Agents
In @testsupport/init.go around line 210, The code hardcodes "https://" when
setting initHostAwait.MetricsURL (using hostMetricsRoute.Status.Ingress[0].Host)
which contradicts the PR goal to derive the scheme from the route TLS config;
change the assignment to choose "https://" if hostMetricsRoute.Spec.TLS is
non-nil/has entries and "http://" otherwise, following the same pattern used for
the registration URL (the conditional logic around route.Spec.TLS in the earlier
block), and apply the same fix for the similar assignment at the other
occurrence (line 215) so both metrics URLs use the correct scheme based on
hostMetricsRoute.Spec.TLS.
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @.govulncheck.yaml:
- Around line 44-55: The vulnerability entries GO-2025-4155 and GO-2025-4175 in
the .govulncheck configuration currently silence until 2026-02-04 without
documented remediation; add a short remediation plan referencing upgrade to
crypto/x509 fixed in go1.24.11, confirm a completed risk assessment entry, and
create an actionable upgrade timeline (owner, target date before 2026-02-04, and
rollback plan) to be linked to these IDs and the silence-until key so the
silence is only allowed if the upgrade plan and risk assessment are recorded.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.govulncheck.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build & push operator bundles & dashboard image for e2e tests
🔇 Additional comments (1)
.govulncheck.yaml (1)
7-7: Verify the relevance of vulnerability silence updates to PR objectives.These vulnerability silence timestamp updates (extending by ~2 months) appear unrelated to the PR's stated objective of refactoring metrics URL scheme handling. If this is intentional maintenance bundled with the metrics refactor, please confirm and ensure there's a plan to upgrade Go or address these standard library vulnerabilities before the new silence deadline.
Also applies to: 13-13, 19-19, 25-25, 31-31, 37-37, 43-43
| # Excessive resource consumption when printing error string for host certificate validation in crypto/x509 | ||
| # Found in: crypto/x509@go1.23.12 | ||
| # Fixed in: crypto/x509@go1.24.11 | ||
| - id: GO-2025-4155 | ||
| info: https://pkg.go.dev/vuln/GO-2025-4155 | ||
| silence-until: 2026-02-04 | ||
| # Improper application of excluded DNS name constraints when verifying wildcard names in crypto/x509 | ||
| # Found in: crypto/x509@go1.23.12 | ||
| # Fixed in: crypto/x509@go1.24.11 | ||
| - id: GO-2025-4175 | ||
| info: https://pkg.go.dev/vuln/GO-2025-4175 | ||
| silence-until: 2026-02-04 |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
GO-2025-4155 crypto/x509 vulnerability
💡 Result:
Summary: GO-2025-4155 (CVE-2025-61729) is a crypto/x509 vulnerability where HostnameError.Error() builds an unbounded, repeatedly-concatenated host list when constructing its error string. A maliciously-crafted certificate can cause excessive CPU/memory (quadratic) usage during hostname verification (Certificate.Verify / VerifyHostname). Fixes released in go1.24.11 and go1.25.5 — upgrade to those or later. [1][2][3]
Sources:
- Go vulnerability entry (GO-2025-4155 / CVE-2025-61729). [1]
- OSV summary and affected ranges (fix: 1.24.11, 1.25.5). [2]
- Vendor/security advisory listing fixes in go1.25.5 (summary of related CVEs). [3]
🌐 Web query:
GO-2025-4175 crypto/x509 vulnerability golang
💡 Result:
Summary — GO-2025-4175 (CVE-2025-61727)
- What it is: an incorrect enforcement of excluded DNS name constraints in crypto/x509 that allows a leaf certificate with a wildcard SAN (e.g. *.example.com) to bypass an excluded subdomain constraint (e.g. exclude test.example.com). [1][2]
- Affected symbols/flows: crypto/x509 Certificate.Verify (certificate verification). [1]
- Affected Go versions: all releases before go1.24.11, and go1.25.0 through before go1.25.5. [1][2]
- Fixed in: go1.24.11 and go1.25.5. Upgrade to one of these or later. [1][3]
- References: Go vulnerability entry and change/issue tracking. [1][2][3]
Sources:
[1] Go vuln DB: GO-2025-4175 / CVE-2025-61727 (pkg.go.dev).
[2] OSV / OSV JSON entry for GO-2025-4175.
[3] Release/security notes referencing go1.25.5 (SUSE advisory).
Confirm remediation plan exists for crypto/x509 vulnerabilities before silence-until deadline.
Both vulnerabilities are documented in the official Go vulnerability database:
- GO-2025-4155: Unbounded string concatenation in HostnameError.Error() causes excessive CPU/memory usage during certificate verification
- GO-2025-4175: Wildcard certificates can bypass excluded DNS name constraints
Since these affect certificate validation (critical security component), ensure:
- Risk assessment has been completed for this codebase
- An upgrade plan to go1.24.11 exists before 2026-02-04
🤖 Prompt for AI Agents
In @.govulncheck.yaml around lines 44-55, The vulnerability entries GO-2025-4155
and GO-2025-4175 in the .govulncheck configuration currently silence until
2026-02-04 without documented remediation; add a short remediation plan
referencing upgrade to crypto/x509 fixed in go1.24.11, confirm a completed risk
assessment entry, and create an actionable upgrade timeline (owner, target date
before 2026-02-04, and rollback plan) to be linked to these IDs and the
silence-until key so the silence is only allowed if the upgrade plan and risk
assessment are recorded.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fbm3307, rajivnathan, xcoulon 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 |
|
4 participants
don't assume it's always
https, so we can be more flexible when testing against anhttpendpointSigned-off-by: Xavier Coulon xcoulon@redhat.com
Summary by CodeRabbit
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.