OCPBUGS-99935, OCPBUGS-100030: fix: remove opentelemetry to eliminate thrift dependency - #1076
Conversation
Addresses CVE-2026-55969 and CVE-2026-43871, both Apache Thrift vulnerabilities fixed in Thrift >= 0.24.0, tracked in: OCPBUGS-99935 (integer overflow/wraparound, CVE-2026-55969) OCPBUGS-100030 (infinite loop, CVE-2026-43871) Root cause ---------- commons/Cargo.toml had two paths pulling in old thrift versions: - direct: thrift = "0.17" (resolves to thrift 0.17.0) - indirect: opentelemetry-jaeger = "0.13.0" -> thrift 0.13.0 Fix --- Remove opentelemetry entirely. Distributed tracing was an optional, lightly-used feature (disabled unless --service.tracing_endpoint is explicitly set, which no current deployment manifests or the operator do). Removing it eliminates the thrift dependency with no change to any default behavior. The --service.tracing_endpoint CLI flag is preserved for backward compatibility with existing configuration files but is now a no-op. Code changes ------------ commons/Cargo.toml: - Remove opentelemetry, opentelemetry-jaeger, thrift commons/src/tracing.rs: - Replace with a single no-op init_tracer() stub cincinnati/src/plugins/mod.rs: - Remove opentelemetry imports - Remove span instrumentation from process(); plugins now run without context propagation overhead cincinnati/src/plugins/internal/cincinnati_graph_fetch.rs: - Remove opentelemetry imports and span/context injection into upstream request headers graph-builder/src/{main,graph}.rs: - Remove opentelemetry imports, wrap_fn tracing middleware, and per-request span creation metadata-helper/src/{main,signatures}.rs: - Remove opentelemetry imports, wrap_fn tracing middleware, and per-request span creation policy-engine/src/{main,graph}.rs: - Remove opentelemetry imports, wrap_fn tracing middleware, and per-request/per-handler span creation {cincinnati,graph-builder,metadata-helper,policy-engine}/Cargo.toml: - Remove opentelemetry dependency
WalkthroughThe change removes OpenTelemetry dependencies and tracing behavior from shared tracing code, HTTP services, request handlers, and Cincinnati plugins. ChangesOpenTelemetry tracing removal
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
tmshort
changed the title
openshift-ci-robot
added
jira/severity-important
|
@tmshort: This pull request references Jira Issue OCPBUGS-99935, which is valid. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. This pull request references Jira Issue OCPBUGS-100030, which is valid. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
Alternative to #1075 |
wking
left a comment
wking
left a comment
There was a problem hiding this comment.
Reverts #266, which I'm fine with. We don't have time for that kind of detailed performance review at this point. Customers were unlikely to have metrics access (openshift/cincinnati-operator#268 is working on some of that, but isn't merged yet). Reducing our vendor dependencies makes it easier to stay current and safe.
/lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tmshort, wking 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 |
openshift-ci
Bot
added
the
approved
|
@tmshort: This pull request references Jira Issue OCPBUGS-99935, which is valid. 3 validation(s) were run on this bug
This pull request references Jira Issue OCPBUGS-100030, which is valid. 3 validation(s) were run on this bug
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
commons/src/tracing.rs (2)
9-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider warning when an endpoint is still configured.
init_tracersilently discards_maybe_endpoint. An operator who still sets--service.tracing_endpointgets no signal that tracing is disabled. This can cause confusion when nothing that traces appear.Log a warning once when the endpoint is
Some, so deployments that pass this flag learn tracing is now a no-op.Proposed fix
pub fn init_tracer(_name: &'static str, _maybe_endpoint: Option<String>) -> Fallible<()> { + if _maybe_endpoint.is_some() { + log::warn!("tracing_endpoint is set but distributed tracing has been removed; the value is ignored"); + } Ok(()) }🤖 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 `@commons/src/tracing.rs` around lines 9 - 11, Update init_tracer to inspect maybe_endpoint and emit a warning once when it is Some, clearly indicating that tracing is disabled and the configured endpoint is ignored; preserve the existing successful no-op return behavior.
1-11: 🩺 Stability & Availability | 🔵 TrivialConfirm alternate observability remains after removing distributed tracing.
This PR removes distributed tracing entirely across graph-builder, metadata-helper, policy-engine, and the Cincinnati plugins. Confirm that request-scoped logging (for example, correlation identifiers per request) still provides enough context for debugging cross-service issues, since spans previously provided that correlation.
🤖 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 `@commons/src/tracing.rs` around lines 1 - 11, Verify the observability paths affected by removing distributed tracing, starting at init_tracer and its callers, to ensure each request retains a correlation identifier in structured request-scoped logs across service boundaries. Preserve or restore propagation and logging of that identifier in graph-builder, metadata-helper, policy-engine, and Cincinnati plugin request flows; do not rely on the no-op tracer for correlation.
🤖 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 `@commons/src/tracing.rs`:
- Around line 9-11: Update init_tracer to inspect maybe_endpoint and emit a
warning once when it is Some, clearly indicating that tracing is disabled and
the configured endpoint is ignored; preserve the existing successful no-op
return behavior.
- Around line 1-11: Verify the observability paths affected by removing
distributed tracing, starting at init_tracer and its callers, to ensure each
request retains a correlation identifier in structured request-scoped logs
across service boundaries. Preserve or restore propagation and logging of that
identifier in graph-builder, metadata-helper, policy-engine, and Cincinnati
plugin request flows; do not rely on the no-op tracer for correlation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c04f1cb4-c553-4b67-9457-feb027024919
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
cincinnati/Cargo.tomlcincinnati/src/plugins/internal/cincinnati_graph_fetch.rscincinnati/src/plugins/mod.rscommons/Cargo.tomlcommons/src/tracing.rsgraph-builder/Cargo.tomlgraph-builder/src/graph.rsgraph-builder/src/main.rsmetadata-helper/Cargo.tomlmetadata-helper/src/main.rsmetadata-helper/src/signatures.rspolicy-engine/Cargo.tomlpolicy-engine/src/graph.rspolicy-engine/src/main.rs
💤 Files with no reviewable changes (8)
- cincinnati/Cargo.toml
- graph-builder/src/graph.rs
- policy-engine/Cargo.toml
- graph-builder/Cargo.toml
- metadata-helper/Cargo.toml
- cincinnati/src/plugins/internal/cincinnati_graph_fetch.rs
- metadata-helper/src/signatures.rs
- commons/Cargo.toml
|
@tmshort: all tests passed! 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. |
|
@tmshort: Jira Issue OCPBUGS-99935: Some pull requests linked via external trackers have merged: The following pull request, linked via external tracker, has not merged:
All associated pull requests must be merged or unlinked from the Jira bug in order for it to move to the next state. Once unlinked, request a bug refresh with Jira Issue OCPBUGS-99935 has not been moved to the MODIFIED state. Jira Issue OCPBUGS-100030: Some pull requests linked via external trackers have merged: The following pull request, linked via external tracker, has not merged:
All associated pull requests must be merged or unlinked from the Jira bug in order for it to move to the next state. Once unlinked, request a bug refresh with Jira Issue OCPBUGS-100030 has not been moved to the MODIFIED state. 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. |
|
/jira refresh |
|
@tmshort: Jira Issue OCPBUGS-99935: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-99935 has been moved to the MODIFIED state. Jira Issue OCPBUGS-100030: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-100030 has been moved to the MODIFIED state. 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. |
3 participants
Summary
Fixes OCPBUGS-99935 (CVE-2026-55969) and OCPBUGS-100030 (CVE-2026-43871) — both Apache Thrift vulnerabilities fixed in Thrift ≥ 0.24.0.
commons/Cargo.tomlhad two paths pulling in oldthriftversions:thrift = "0.17"opentelemetry-jaeger = "0.13.0"→thrift 0.13.0Fix: remove opentelemetry entirely. Distributed tracing was an optional, lightly-used feature — disabled by default unless
--service.tracing_endpointis explicitly set, which no current deployment manifests or thecincinnati-operatordo. Removing it eliminates thethriftdependency with no change to any default behavior.An alternative approach using
opentelemetry-otlpto replaceopentelemetry-jaegeris available in PR #1075 for comparison.Changes
commons/Cargo.toml: removeopentelemetry,opentelemetry-jaeger,thriftcommons/src/tracing.rs: replace with a single no-opinit_tracer()stubcincinnati/src/plugins/mod.rs: remove span instrumentation fromprocess()cincinnati/src/plugins/internal/cincinnati_graph_fetch.rs: remove span/context injection into upstream request headersgraph-builder/src/{main,graph}.rs: remove tracing middleware and per-request span creationmetadata-helper/src/{main,signatures}.rs: remove tracing middleware and per-request span creationpolicy-engine/src/{main,graph}.rs: remove tracing middleware and per-handler span creation{cincinnati,graph-builder,metadata-helper,policy-engine}/Cargo.toml: removeopentelemetryBackward compatibility
The
--service.tracing_endpointCLI flag is preserved and accepted but now silently ignored, so existing configuration files do not need updating.Test plan
cargo build— clean, no errorscargo test— 2 failures incincinnati_graph_fetchare a subset of the 5 pre-existing failures onmaster(confirmed by running against unmodified tree)thriftandopentelemetryabsent fromCargo.lockafter changeSummary by CodeRabbit
Refactor
Bug Fixes