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

appsec: Service Extension callout #2965

Open
wants to merge 35 commits into
base: main
Choose a base branch
from

Conversation

e-n-0
Copy link
Member

@e-n-0 e-n-0 commented Nov 6, 2024

Motivation

This is the part 2 PR to support Envoy's External Processing. This part is creating a package to be used for the GCP Services Extensions product.
You can find all related document for this implementation in Confluence ASM - GCP Services Extensions.
You can find the part 1 of this PR.

What does this PR do?

This PR creates a new binary project that implements a gRPC server using the Envoy External Processor protocol of Envoy and an HTTP server for the health check. The gRPC server use the gRPC Interceptor (StreamServerInterceptor) implemented from the part 1.

Release

The PR includes a new GitHub action to publish new docker images to the repo at each new release.
These images are push under the name ghcr.io/datadog/dd-trace-go/service-extensions-callout and exists with the following tags:

  • latest
  • version (e.g. v1.69.1)
  • commit sha

You can find the package released in the GitHub repo registry.

Tests

System tests

System tests have been implemented on this PR. A new external-processing scenario with the docker image given to customers and envoy infrastructure has been added in the golang stage.
APM and ASM test has been imported to be sure that all spans and tags are correctly set to validate the integrity of the created traces.

Macro Benchmarks

Benchmarks of the the whole extension is available on this benchmarking platform branch.
More information with dashboard can be found here.

Reviewer's Checklist

  • Changed code has unit tests for its functionality at or near 100% coverage.
  • System-Tests covering this feature have been added and enabled with the va.b.c-dev version tag.
  • There is a benchmark for any new code, or changes to existing code.
  • If this interacts with the agent in a new way, a system test has been added.
  • Add an appropriate team label so this PR gets put in the right place for the release notes.
  • Non-trivial go.mod changes, e.g. adding new modules, are reviewed by @DataDog/dd-trace-go-guild.

Unsure? Have a question? Request a review!

@github-actions github-actions bot added the apm:ecosystem contrib/* related feature requests or bugs label Nov 6, 2024
.github/workflows/service-extensions-publish.yml Outdated Show resolved Hide resolved
.github/workflows/service-extensions-publish.yml Outdated Show resolved Hide resolved
.github/workflows/service-extensions-publish.yml Outdated Show resolved Hide resolved
.github/workflows/service-extensions-publish.yml Outdated Show resolved Hide resolved
.github/workflows/service-extensions-publish.yml Outdated Show resolved Hide resolved
@e-n-0 e-n-0 force-pushed the flavien/service-extensions-part2 branch from a87535b to 0044a8f Compare November 6, 2024 12:42
@e-n-0 e-n-0 force-pushed the flavien/service-extensions-part2 branch 2 times, most recently from 6c165d2 to 0607577 Compare November 6, 2024 13:41
@pr-commenter
Copy link

pr-commenter bot commented Nov 6, 2024

Benchmarks

Benchmark execution time: 2024-12-19 18:03:04

Comparing candidate commit e3cc085 in PR branch flavien/service-extensions-part2 with baseline commit e8b2b8c in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 59 metrics, 0 unstable metrics.

@e-n-0 e-n-0 force-pushed the flavien/service-extensions-part2 branch from 0607577 to 867a55b Compare November 6, 2024 14:44
@e-n-0 e-n-0 changed the title Service Extension Callout Standalone + Build appsec: Service Extension callout Nov 6, 2024
@e-n-0 e-n-0 force-pushed the flavien/service-extensions-part2 branch 2 times, most recently from 34f396f to 7d3e562 Compare November 6, 2024 16:12
@e-n-0 e-n-0 force-pushed the flavien/service-extensions branch from 2eeb6b1 to 402f7d0 Compare November 6, 2024 16:14
@e-n-0 e-n-0 force-pushed the flavien/service-extensions-part2 branch from 7d3e562 to ca1de07 Compare November 6, 2024 16:29
@e-n-0 e-n-0 self-assigned this Nov 6, 2024
@e-n-0 e-n-0 added the appsec label Nov 6, 2024
@e-n-0 e-n-0 marked this pull request as ready for review November 6, 2024 16:50
@e-n-0 e-n-0 requested review from a team as code owners November 6, 2024 16:50
@e-n-0 e-n-0 force-pushed the flavien/service-extensions-part2 branch from 0614501 to 2b06ea5 Compare November 6, 2024 17:02
.github/workflows/service-extensions-publish.yml Outdated Show resolved Hide resolved
contrib/envoyproxy/envoy/cmd/serviceextensions/Dockerfile Outdated Show resolved Hide resolved
contrib/envoyproxy/envoy/cmd/serviceextensions/Dockerfile Outdated Show resolved Hide resolved
contrib/envoyproxy/envoy/cmd/serviceextensions/main.go Outdated Show resolved Hide resolved
contrib/envoyproxy/envoy/cmd/serviceextensions/main.go Outdated Show resolved Hide resolved
contrib/envoyproxy/envoy/cmd/serviceextensions/main.go Outdated Show resolved Hide resolved
contrib/envoyproxy/envoy/cmd/serviceextensions/main.go Outdated Show resolved Hide resolved
contrib/envoyproxy/envoy/cmd/serviceextensions/main.go Outdated Show resolved Hide resolved
contrib/envoyproxy/envoy/cmd/serviceextensions/main.go Outdated Show resolved Hide resolved
@eliottness eliottness force-pushed the flavien/service-extensions branch from 1f2b791 to 9630788 Compare November 8, 2024 18:05
@eliottness eliottness requested a review from a team as a code owner November 8, 2024 18:05
@eliottness eliottness force-pushed the flavien/service-extensions branch 2 times, most recently from 0929de5 to e3bb1e5 Compare November 13, 2024 16:14
@eliottness eliottness removed the request for review from a team November 20, 2024 08:57
@e-n-0 e-n-0 force-pushed the flavien/service-extensions branch from c7063d8 to b0844b8 Compare November 28, 2024 14:59
@e-n-0 e-n-0 force-pushed the flavien/service-extensions-part2 branch 12 times, most recently from 8ee70e2 to 42d9365 Compare December 16, 2024 19:47
@e-n-0 e-n-0 force-pushed the flavien/service-extensions-part2 branch from 42d9365 to 673ec27 Compare December 16, 2024 19:50
@DataDog DataDog deleted a comment from datadog-datadog-prod-us1 bot Dec 16, 2024
@DataDog DataDog deleted a comment from datadog-datadog-prod-us1 bot Dec 16, 2024
@DataDog DataDog deleted a comment from datadog-datadog-prod-us1 bot Dec 16, 2024
@DataDog DataDog deleted a comment from datadog-datadog-prod-us1 bot Dec 16, 2024
@DataDog DataDog deleted a comment from datadog-datadog-prod-us1 bot Dec 16, 2024
@DataDog DataDog deleted a comment from datadog-datadog-prod-us1 bot Dec 16, 2024
@e-n-0 e-n-0 requested a review from rarguelloF December 18, 2024 16:43
Copy link
Contributor

@rarguelloF rarguelloF left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, just a few extra comments.
Also please remember to run go mod tidy in the project root and commit the changes 🙏

})

if err := g.Wait(); err != nil {
log.Error("service_extension: %v\n", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

You might want to os.Exit(1) in this case to signal the process exit with an error.

However if you do this, please note any defer you might have won't run. If you want to use os.Exit and defer, the best approach would be to move the logic from lines 81-95 to a separate function func startService(cfg serviceExtensionConfig) error, and have these defer's in there.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the comment, I applied the changes in e3cc085 😄
(I also added a Flush to the logs before exiting.)

@e-n-0 e-n-0 requested a review from rarguelloF December 19, 2024 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apm:ecosystem contrib/* related feature requests or bugs appsec
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants