Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pkg/trace: add support for meta_struct span proto field #10366

Merged
merged 14 commits into from
Feb 18, 2022

Conversation

nizox
Copy link
Contributor

@nizox nizox commented Dec 28, 2021

What does this PR do?

This PR adds support for the meta_struct span proto field in the trace-agent. This field is analogous to meta but allows to attach structured data serialized in msgpack to a span instead of strings.

Motivation

AppSec runs inside the APM tracers and detects security events (also called triggers) thanks to AppSec rules. The security events are attached to the relevant span and are currently reported using the _dd.appsec.json meta tag as a JSON serialized string.

There is currently no compatibility between the legacy _dd.appsec.json meta tag and the new appsec meta struct tag. The tracers need to send one or the other depending on the protocol in use.

Additional Notes

AppSec events are low-volume and it is estimated that 1 trace out of 100 000 have AppSec events for customers where the feature is enabled.

Possible Drawbacks / Trade-offs

Only the v0.7 protocol supports the new meta_struct field. A flag has been introduced to indicate that an agent supports the meta_struct field.

Reviewer's Checklist

  • If known, an appropriate milestone has been selected; otherwise the Triage milestone is set.
  • The appropriate team/.. label has been applied, if known.
  • A release note has been added or the changelog/no-changelog label has been applied.
  • Changed code has automated tests for its functionality.
  • Adequate QA/testing plan information is provided if the qa/skip-qa label is not applied.
  • If applicable, docs team has been notified or an issue has been opened on the documentation repo.
  • If applicable, the need-change/operator and need-change/helm labels have been applied.
  • If applicable, the config template has been updated.

pkg/trace/pb/hook_test.go Outdated Show resolved Hide resolved
pkg/trace/pb/span.proto Outdated Show resolved Hide resolved
pkg/trace/pb/appsec.go Outdated Show resolved Hide resolved
pkg/trace/agent/obfuscate.go Outdated Show resolved Hide resolved
pkg/trace/agent/obfuscate.go Outdated Show resolved Hide resolved
pkg/trace/agent/obfuscate.go Outdated Show resolved Hide resolved
@nizox nizox requested a review from Julio-Guerra December 30, 2021 15:36
@nizox nizox added this to the 7.34.0 milestone Jan 3, 2022
@nizox nizox marked this pull request as ready for review January 3, 2022 10:35
@nizox nizox requested a review from a team as a code owner January 3, 2022 10:35
@nizox nizox changed the title [WIP][APPSEC] pkg/trace: add support for AppSec structs [APPSEC] pkg/trace: add support for AppSec structs Jan 3, 2022
@Julio-Guerra
Copy link
Contributor

You should also put pkg/trace/pb/appsec.proto in the CODEOWNERS file:
https://github.com/DataDog/datadog-agent/blob/main/.github/CODEOWNERS#L204-L205

@gbbr gbbr changed the title [APPSEC] pkg/trace: add support for AppSec structs pkg/trace: add support for AppSec structs Jan 5, 2022
@gbbr gbbr added the team/agent-apm trace-agent label Jan 5, 2022
Copy link
Contributor

@gbbr gbbr left a comment

Choose a reason for hiding this comment

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

Left some nits.

pkg/trace/pb/hook_test.go Outdated Show resolved Hide resolved
pkg/trace/pb/hook_test.go Outdated Show resolved Hide resolved
pkg/trace/pb/hook_test.go Outdated Show resolved Hide resolved
pkg/trace/agent/obfuscate_test.go Outdated Show resolved Hide resolved
pkg/trace/agent/obfuscate_test.go Outdated Show resolved Hide resolved
pkg/trace/agent/obfuscate.go Outdated Show resolved Hide resolved
pkg/trace/agent/obfuscate.go Outdated Show resolved Hide resolved
pkg/trace/agent/obfuscate.go Outdated Show resolved Hide resolved
pkg/trace/agent/obfuscate_test.go Outdated Show resolved Hide resolved
pkg/trace/agent/obfuscate_test.go Outdated Show resolved Hide resolved
pkg/trace/agent/obfuscate.go Outdated Show resolved Hide resolved
pkg/trace/agent/obfuscate.go Outdated Show resolved Hide resolved
pkg/trace/agent/obfuscate.go Outdated Show resolved Hide resolved
pkg/trace/agent/obfuscate.go Outdated Show resolved Hide resolved
pkg/trace/pb/appsec.proto Outdated Show resolved Hide resolved
pkg/trace/agent/obfuscate_test.go Outdated Show resolved Hide resolved
pkg/trace/agent/obfuscate_test.go Outdated Show resolved Hide resolved
pkg/trace/agent/obfuscate_test.go Outdated Show resolved Hide resolved
pkg/trace/agent/obfuscate_test.go Outdated Show resolved Hide resolved
@kacper-murzyn kacper-murzyn modified the milestones: 7.34.0, 7.35.0 Jan 10, 2022
Co-authored-by: Gabriel Aszalos <gabriel.aszalos@gmail.com>
@nizox nizox force-pushed the nicolas.vivet/span-meta-struct branch from cdb1ac7 to d6be19b Compare February 4, 2022 16:27
@nizox nizox requested a review from knusbaum February 8, 2022 15:19
@nizox nizox requested a review from raphaelgavache February 16, 2022 13:25
@nizox nizox changed the title pkg/trace: add support for AppSec structs pkg/trace: add support for meta_struct span proto field Feb 17, 2022
pkg/trace/agent/obfuscate_test.go Outdated Show resolved Hide resolved
pkg/trace/api/api.go Outdated Show resolved Hide resolved
@nizox nizox merged commit c813584 into main Feb 18, 2022
@nizox nizox deleted the nicolas.vivet/span-meta-struct branch February 18, 2022 09:10
@gbbr
Copy link
Contributor

gbbr commented Feb 25, 2022

There is not a single test in this PR which tests that a Msgpack payload containing the new field gets decoded correctly. The new decoding code has 0 coverage. 🙁

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

Successfully merging this pull request may close these issues.

7 participants