contrib/envoyproxy: envoy external processing support #2895

e-n-0 · 2024-09-27T12:15:12Z

Motivation

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

What does this PR do?

This PR adds a new gRPC Interceptor (StreamServerInterceptor) to support the interception of ext_proc v3 calls to gRPC server. When the interceptor is applied, all messages of the external processing protocol are instrumented without returning an handle to the original server code. The implementation of a server using this instrumentation can be found in the part 2.

The implementation includes:

Analysing synchronously HTTP Requests and Responses data (headers, ip, path, host, status code)
Blocking requests (supporting blocking with content-type and redirect)
Some refacto of existing code to handle more easily crafted requests without an actual valid http.Request object.

Tests

This PR includes unit testing in the envoy_tests.go, simulating scenarios of malicious or benign requests, validating span tags, security events and blocking results.

System-tests have been implemented on this PR. A new external-processing scenario has been added in the golang stage.

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!

pr-commenter · 2024-09-27T14:05:16Z

Benchmarks

Benchmark execution time: 2024-12-11 11:03:14

Comparing candidate commit ee0cd57 in PR branch flavien/service-extensions with baseline commit a48a41e in branch main.

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

eliottness

Thanks for contributing 🙏. Couple of requests:

Normally you should not have to import any listener packages, nor to export functions from it.
Could you link to a manual CI run from the System Tests workflow pointing on your system-tests branch where the workflow run is green ? You probably have to add your scenario in the workflow for it to run
I suspect we need to add tests for the changes you did in the emitter/waf/actions package. Let's see if this would make sense to separate it in another PR
Let's review together the code of envoy.go on fridayso we can structure it better

internal/appsec/emitter/httpsec/http.go

internal/appsec/emitter/waf/actions/http_redirect.go

internal/appsec/listener/httpsec/request.go

contrib/envoyproxy/envoy/envoy.go

eliottness · 2024-11-12T12:57:20Z

Consolidated system-test run: https://github.com/DataDog/dd-trace-go/actions/runs/11797728094

eliottness · 2024-11-12T13:12:22Z

contrib/internal/httptrace/response_writer.go

+// ResetStatusCode resets the status code of the response writer.
+func ResetStatusCode(w http.ResponseWriter) {
+	if rw, ok := w.(*responseWriter); ok {
+		rw.status = 0
+	}
+}
+


This required because of this if statement.

contrib/envoyproxy/envoy/envoy.go

contrib/envoyproxy/envoy/fakehttp.go

contrib/envoyproxy/envoy/envoy.go

contrib/envoyproxy/envoy/envoy_test.go

contrib/envoyproxy/envoy/envoy.go

Signed-off-by: Eliott Bouhana <[email protected]>

* Add support for context propagation * Normalize span tag use Co-authored-by: Flavien Darche <[email protected]> Signed-off-by: Eliott Bouhana <[email protected]>

datadog-datadog-prod-us1 · 2024-12-11T10:27:40Z

Datadog Report

Branch report: flavien/service-extensions
Commit report: d008bfb
Test service: dd-trace-go

✅ 0 Failed, 5110 Passed, 70 Skipped, 2m 52.72s Total Time

rarguelloF · 2024-12-11T10:46:33Z

contrib/envoyproxy/go-control-plane/envoy.go

+	wrappedResponseWriter http.ResponseWriter
+}
+
+func StreamServerInterceptor(opts ...grpctrace.Option) grpc.StreamServerInterceptor {


Please add a godoc to the exported stuff 🙏 Also it would be great to briefly explain what this actually does, as it took me some time to figure out.

Suggested change

func StreamServerInterceptor(opts ...grpctrace.Option) grpc.StreamServerInterceptor {

// StreamServerInterceptor returns a new grpc.StreamServerInterceptor intended to be used with an envoyproxy grpc server.

// It will use the regular grpc tracing package for all methods except from `envoy.service.ext_proc.v3.ExternalProcessor/Process` where it will [...]

func StreamServerInterceptor(opts ...grpctrace.Option) grpc.StreamServerInterceptor {

(please feel free to modify the comment if I wrote anything incorrect, and add more context as you did in the PR description).

rarguelloF · 2024-12-11T10:47:48Z

contrib/envoyproxy/go-control-plane/example_test.go

+	// Create a listener for the server.
+	ln, err := net.Listen("tcp", ":50051")
+	if err != nil {
+		log.Fatal(err)
+	}
+
+	// Create the server interceptor using the envoy go control plane package.
+	si := go_control_plane.StreamServerInterceptor()
+
+	// Initialize the grpc server as normal, using the envoy server interceptor.
+	s := grpc.NewServer(grpc.StreamInterceptor(si))
+
+	// ... register your services
+


Could you add a more "real-world" like example? similar to https://github.com/envoyproxy/go-control-plane/blob/main/examples/dyplomat/main.go#L43-L53

(currently this example is just a generic grpc server without any envoyproxy stuff)

rarguelloF · 2024-12-11T10:48:54Z

contrib/envoyproxy/go-control-plane/envoy.go

+	}
+}
+
+func ProcessRequestHeaders(ctx context.Context, req *envoyextproc.ProcessingRequest_RequestHeaders) (*envoyextproc.ProcessingResponse, *currentRequest, bool, error) {


please add a godoc comment, or consider unexporting this function if this is not intended to be used directly.

rarguelloF · 2024-12-11T11:03:34Z

contrib/envoyproxy/go-control-plane/envoy.go

+			}
+		}()
+
+		for {


I find this pattern a little bit odd for an interceptor / middleware.

Since it seems this is pretty much specifically intended to override the behaviour of ext_procv3.ExternalProcessorServer.Process, have you considered exporting this functionality as an implementation of this interface instead of a middleware? This way, users could just do:

import envoytrace "gopkg.in/DataDog/dd-trace-go.v1/contrib/envoyproxy/go-control-plane" // srv would be the user provided implementation of `ext_procv3.ExternalProcessorServer` appsecBlockSrv := envoytrace.AppsecBlockingProcessorServer(srv) // internally you would call srv.Process() when the request is not blocked ext_procv3.RegisterExternalProcessorServer(grpcServer, appsecBlockSrv)

rarguelloF · 2024-12-11T11:05:26Z

contrib/envoyproxy/go-control-plane/envoy.go

+			if err != nil {
+				// Note: Envoy is inconsistent with the "end_of_stream" value of its headers responses,
+				// so we can't fully rely on it to determine when it will close (cancel) the stream.
+				if err == io.EOF || err.(interface{ GRPCStatus() *status.Status }).GRPCStatus().Code() == codes.Canceled {


you can use s, ok := status.FromError(err) to do the assertion safely.

rarguelloF · 2024-12-11T11:06:50Z

contrib/envoyproxy/go-control-plane/envoy.go

+		}, nil
+
+	case *envoyextproc.ProcessingRequest_ResponseBody:
+		r := req.Request.(*envoyextproc.ProcessingRequest_ResponseBody)


this line is not necessary, the type asserted variable is already available in v

Suggested change

r := req.Request.(*envoyextproc.ProcessingRequest_ResponseBody)

r := v

DataDog deleted a comment from datadog-datadog-prod-us1 bot Sep 27, 2024

e-n-0 force-pushed the flavien/service-extensions branch 7 times, most recently from fcbd354 to a587a09 Compare September 27, 2024 13:33

e-n-0 force-pushed the flavien/service-extensions branch 16 times, most recently from 4134743 to a96cc2a Compare September 30, 2024 15:26

e-n-0 requested review from a team as code owners November 6, 2024 16:50

eliottness requested changes Nov 7, 2024

View reviewed changes

e-n-0 force-pushed the flavien/service-extensions branch from bdd1915 to 1f2b791 Compare November 8, 2024 14:25

eliottness force-pushed the flavien/service-extensions branch 2 times, most recently from 9630788 to 0929de5 Compare November 12, 2024 12:54

eliottness reviewed Nov 12, 2024

View reviewed changes

eliottness changed the title ~~appsec: envoy external processing support~~ contrib/envoyproxy: envoy external processing support Nov 12, 2024

eliottness approved these changes Nov 12, 2024

View reviewed changes

eliottness force-pushed the flavien/service-extensions branch from 0929de5 to e3bb1e5 Compare November 13, 2024 16:14

rarguelloF requested changes Nov 15, 2024

View reviewed changes

e-n-0 force-pushed the flavien/service-extensions branch from c7063d8 to b0844b8 Compare November 28, 2024 14:59

e-n-0 requested a review from rarguelloF December 11, 2024 10:09

e-n-0 and others added 11 commits December 11, 2024 11:12

Service Extension Callout (Envoy external processing)

4327d10

Remove binary to move it to another branch

a2f01c2

Fix CI

f3bcc4d

Apply comments

e3a3dbc

revert all changes expect in contrib/envoyproxy

75b6240

Signed-off-by: Eliott Bouhana <[email protected]>

rework to fake a new http.Request and a http.ResponseWriter

e210f05

* Add support for context propagation * Normalize span tag use Co-authored-by: Flavien Darche <[email protected]> Signed-off-by: Eliott Bouhana <[email protected]>

Apply comments

89ebfe6

Add "t.Helper" to helper methods

24bb334

Downgrade grpc version to v1.64.0 (some proto changes)

46a08b1

Add example_test

c789009

update some comments

961d73d

e-n-0 force-pushed the flavien/service-extensions branch from b0844b8 to 961d73d Compare December 11, 2024 10:14

go mod tidy (rebase fix)

ee0cd57

rarguelloF reviewed Dec 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

contrib/envoyproxy: envoy external processing support #2895

contrib/envoyproxy: envoy external processing support #2895

e-n-0 commented Sep 27, 2024 •

edited

Loading

pr-commenter bot commented Sep 27, 2024 •

edited

Loading

eliottness left a comment

eliottness commented Nov 12, 2024

eliottness Nov 12, 2024

datadog-datadog-prod-us1 bot commented Dec 11, 2024

rarguelloF Dec 11, 2024

rarguelloF Dec 11, 2024

rarguelloF Dec 11, 2024

rarguelloF Dec 11, 2024

rarguelloF Dec 11, 2024

rarguelloF Dec 11, 2024

+              			}
+              		}()
+              		for {

	r := req.Request.(*envoyextproc.ProcessingRequest_ResponseBody)
	r := v

contrib/envoyproxy: envoy external processing support #2895

Are you sure you want to change the base?

contrib/envoyproxy: envoy external processing support #2895

Conversation

e-n-0 commented Sep 27, 2024 • edited Loading

Motivation

What does this PR do?

Tests

Reviewer's Checklist

pr-commenter bot commented Sep 27, 2024 • edited Loading

Benchmarks

eliottness left a comment

Choose a reason for hiding this comment

eliottness commented Nov 12, 2024

eliottness Nov 12, 2024

Choose a reason for hiding this comment

datadog-datadog-prod-us1 bot commented Dec 11, 2024

Datadog Report

rarguelloF Dec 11, 2024

Choose a reason for hiding this comment

rarguelloF Dec 11, 2024

Choose a reason for hiding this comment

rarguelloF Dec 11, 2024

Choose a reason for hiding this comment

rarguelloF Dec 11, 2024

Choose a reason for hiding this comment

rarguelloF Dec 11, 2024

Choose a reason for hiding this comment

rarguelloF Dec 11, 2024

Choose a reason for hiding this comment

e-n-0 commented Sep 27, 2024 •

edited

Loading

pr-commenter bot commented Sep 27, 2024 •

edited

Loading