Skip to content

feat(traces): ingest SkyWalking traces through edge receivers - #391

Draft
youzi-1122 wants to merge 8 commits into
mainfrom
feat/edge-skywalking-traces
Draft

youzi-1122 wants to merge 8 commits into
mainfrom
feat/edge-skywalking-traces

Conversation

@youzi-1122

Copy link
Copy Markdown
Member

Summary

Accept SkyWalking v3 traces through the existing Edge Collector alongside OTLP. Add independent gRPC/HTTP controls (11800/12800), Kubernetes Service ports, original SkyWalking trace-ID lookup/copy, and shared status-message rendering that marks only errors red. Let the device table grow to fit long Edge versions and scroll horizontally.

Existing configurations default to both SkyWalking protocols enabled when the traces plugin is enabled; explicit protocol opt-outs are retained. No database migration or additional SDK dependency.

Review blocker — do not merge

P1: the bundled Collector does not honor the SkyWalking gRPC bind host. With endpoint: "127.0.0.1:11800", Collector 0.157.0 binds *:11800, and a TCP connection from outside the Edge VM succeeds. Enabling this receiver by default therefore exposes an unauthenticated ingest listener beyond the intended localhost scope. HTTP honors its configured host.

Upstream collectorGRPCAddr() returns only :<port> and startCollector() passes it to net.Listen: https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/v0.157.0/receiver/skywalkingreceiver/skywalking_receiver.go#L91-L97 . The current upstream main branch still has this implementation. A dependency fix or equivalent verified isolation is needed before releasing the default-on gRPC behavior. Updating the endpoint YAML alone does not fix it.

Validation

  • go test -race ./internal/pkg/tracequery ./internal/edgeagent/plugins/... with the real bundled Collector enabled: passed, including SkyWalking gRPC + HTTP conversion and OTLP coexistence.
  • Targeted frontend tests: 15 passed; production frontend build passed.
  • make test-k8s-chart: passed.
  • Local end-to-end: 10 SkyWalking traces / 30 spans, including two error traces; all original IDs, parent links and durations verified through Manager/Tempo.
  • Browser verification: horizontal table scrolling exposes the full version/status; SUCCESS uses neutral styling with no warning icon.
  • These checks do not establish that the receiver respects loopback binding; the live network check above demonstrates the blocking failure.

Author confirmation

@youzi-1122 youzi-1122 left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Review result: 1 blocking finding; not approved for merge.

[P1] Preserve the configured network boundary before enabling SkyWalking gRPC by default (internal/edgeagent/plugins/traces/render.go:50-54). The generated localhost endpoint is not enforced by Collector 0.157.0: its receiver extracts the port, constructs :<port>, and calls net.Listen. The deployed configuration contains 127.0.0.1:11800, but the socket binds *:11800; an external-to-VM TCP connection succeeds. This allows reachable peers to submit traces through the Edge's trusted forwarding credentials and contradicts the advertised local-only default. The upstream main implementation has the same behavior.

Required before merge: use a receiver build that honors the configured host, or provide equivalent isolation, and add a real-listener regression check that accepts loopback traffic while refusing the same port on another interface. HTTP/gRPC conversion tests and YAML assertions do not cover this boundary. The requested default-on behavior should not be released until this is resolved.

The targeted Go race tests, real Collector gRPC/HTTP conversion, 15 frontend tests, frontend build, Helm checks, and local trace-ID/parent/duration checks passed. No additional blocking findings identified in the reviewed patch. GitHub checks are reported separately.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant