xds: implement fault injection HTTP filter (A33) #4236

dfawley · 2021-03-02T22:19:53Z

also contains all of #4235

menghanl · 2021-03-05T00:17:52Z

xds/internal/httpfilter/fault/fault.go

+
+func (builder) ParseFilterConfigOverride(override proto.Message) (httpfilter.FilterConfig, error) {
+	// Parsing is the same for the override config.
+	return builder{}.ParseFilterConfig(override)


Name the receiver and use it, instead of create a new one.
Or just make a local function, and call it in both Parse() methods.

Done (latter).

menghanl · 2021-03-05T00:25:29Z

xds/internal/httpfilter/fault/fault.go

+		md, _ := metadata.FromOutgoingContext(ctx)
+		if v := md[headerDelayDuration]; v != nil {
+			if ms, ok := parseIntFromMD(v); ok {
+				delay = time.Duration(time.Duration(ms) * time.Millisecond)


The outer time.Duration conversion is redundant.

menghanl · 2021-03-05T00:28:52Z

xds/internal/httpfilter/fault/fault.go

+			} else {
+				return nil
+			}
+		} else {


How about

v := md[headerDelayDuration] if v == nil { return nil } ms, ok := parseIntFromMD(v) if! ok { return nil } delay = time.Duration(time.Duration(ms) * time.Millisecond)

Good call, done. All the elses can be confusing.

menghanl · 2021-03-05T00:32:24Z

xds/internal/httpfilter/fault/fault.go

+		code, okCode = sanitizeGRPCCode(codes.Code(errType.GrpcStatus)), true
+	case *fpb.FaultAbort_HeaderAbort_:
+		md, _ := metadata.FromOutgoingContext(ctx)
+		if v := md[headerAbortHTTPStatus]; v != nil {


Is it by design that http status has higher priority than gRPC status?

Yes. https://www.envoyproxy.io/docs/envoy/latest/configuration/http/http_filters/fault_filter#controlling-fault-injection-via-http-headers:

If both x-envoy-fault-abort-request and x-envoy-fault-abort-grpc-request headers are set then x-envoy-fault-abort-grpc-request header will be ignored and fault response http status code will be set to x-envoy-fault-abort-request header value.

menghanl · 2021-03-05T00:33:26Z

xds/internal/httpfilter/fault/fault.go

+}
+
+// For overriding in tests
+var intn = grpcrand.Intn


Rename this to randIntn? I at first thought this is a type conversion.

menghanl · 2021-03-05T00:35:55Z

xds/internal/httpfilter/fault/fault.go

+
+func (*okStream) Header() (metadata.MD, error) { return nil, nil }
+func (*okStream) Trailer() metadata.MD         { return nil }
+func (*okStream) CloseSend() error             { return nil }


CloseSend should also return io.EOF, it's a special send.

I don't think that's right. We never return an error from CloseSend:

grpc-go/stream.go

Lines 831 to 834 in 930c791

// Always return nil; io.EOF is the only error that might make sense

// instead, but there is no need to signal the client to call RecvMsg

// as the only use left for the stream after CloseSend is to call

// RecvMsg. This also matches historical behavior.

dfawley

There was a test failure that was/is concerning, but I've re-run locally for hundreds of runs without encountering them. Also the failing test run on GA included a non-FI test failure that I've never seen before, so maybe solar flares are to blame?

dfawley · 2021-03-05T18:09:43Z

xds/internal/httpfilter/fault/fault.go

+
+func (builder) ParseFilterConfigOverride(override proto.Message) (httpfilter.FilterConfig, error) {
+	// Parsing is the same for the override config.
+	return builder{}.ParseFilterConfig(override)


Done (latter).

dfawley · 2021-03-05T18:10:16Z

xds/internal/httpfilter/fault/fault.go

+}
+
+// For overriding in tests
+var intn = grpcrand.Intn


dfawley · 2021-03-05T18:11:07Z

xds/internal/httpfilter/fault/fault.go

+		md, _ := metadata.FromOutgoingContext(ctx)
+		if v := md[headerDelayDuration]; v != nil {
+			if ms, ok := parseIntFromMD(v); ok {
+				delay = time.Duration(time.Duration(ms) * time.Millisecond)


dfawley · 2021-03-05T18:12:16Z

xds/internal/httpfilter/fault/fault.go

+			} else {
+				return nil
+			}
+		} else {


Good call, done. All the elses can be confusing.

dfawley · 2021-03-05T18:19:22Z

xds/internal/httpfilter/fault/fault.go

+		code, okCode = sanitizeGRPCCode(codes.Code(errType.GrpcStatus)), true
+	case *fpb.FaultAbort_HeaderAbort_:
+		md, _ := metadata.FromOutgoingContext(ctx)
+		if v := md[headerAbortHTTPStatus]; v != nil {


Yes. https://www.envoyproxy.io/docs/envoy/latest/configuration/http/http_filters/fault_filter#controlling-fault-injection-via-http-headers:

If both x-envoy-fault-abort-request and x-envoy-fault-abort-grpc-request headers are set then x-envoy-fault-abort-grpc-request header will be ignored and fault response http status code will be set to x-envoy-fault-abort-request header value.

dfawley · 2021-03-05T18:21:28Z

xds/internal/httpfilter/fault/fault.go

+
+func (*okStream) Header() (metadata.MD, error) { return nil, nil }
+func (*okStream) Trailer() metadata.MD         { return nil }
+func (*okStream) CloseSend() error             { return nil }


I don't think that's right. We never return an error from CloseSend:

grpc-go/stream.go

Lines 831 to 834 in 930c791

// Always return nil; io.EOF is the only error that might make sense

// instead, but there is no need to signal the client to call RecvMsg

// as the only use left for the stream after CloseSend is to call

// RecvMsg. This also matches historical behavior.

menghanl · 2021-03-08T22:24:28Z

Can you rebase this onto master? To fix the conflicts, and also then this PR will show only the new diffs.

dfawley · 2021-03-09T17:09:41Z

Can you rebase this onto master? To fix the conflicts, and also then this PR will show only the new diffs.

Done!

menghanl · 2021-03-11T19:24:05Z

xds/internal/httpfilter/fault/fault_test.go

@@ -0,0 +1,646 @@
+// +build !386


Why is this?

dfawley added the Type: Feature New features or improvements in behavior label Mar 2, 2021

dfawley added this to the 1.37 Release milestone Mar 2, 2021

dfawley requested a review from menghanl March 2, 2021 22:19

dfawley assigned menghanl Mar 2, 2021

dfawley force-pushed the fault_injection branch from 0f67327 to 7bdecdb Compare March 2, 2021 22:38

menghanl suggested changes Mar 5, 2021

View reviewed changes

dfawley commented Mar 5, 2021

View reviewed changes

menghanl assigned dfawley and unassigned menghanl Mar 8, 2021

dfawley force-pushed the fault_injection branch from f82ddc9 to ccecbd2 Compare March 9, 2021 17:01

dfawley assigned menghanl and unassigned dfawley Mar 9, 2021

dfawley force-pushed the fault_injection branch from ccecbd2 to 37dc149 Compare March 11, 2021 00:35

menghanl approved these changes Mar 11, 2021

View reviewed changes

xds/internal/httpfilter/fault/fault_test.go Outdated

@@ -0,0 +1,646 @@

// +build !386

Copy link

Contributor

menghanl Mar 11, 2021

Choose a reason for hiding this comment

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

Why is this?

menghanl assigned dfawley and unassigned menghanl Mar 11, 2021

dfawley force-pushed the fault_injection branch from 73beb7f to 48590fd Compare March 11, 2021 21:20

dfawley added 4 commits March 11, 2021 15:01

xds: implement fault injection HTTP filter (A33)

caf1bbe

convert errors more smartly

a22b84e

review

72e33be

fix fault_test

559b7aa

dfawley force-pushed the fault_injection branch from 48590fd to 559b7aa Compare March 11, 2021 23:01

dfawley merged commit d773737 into grpc:master Mar 12, 2021

dfawley deleted the fault_injection branch March 12, 2021 16:38

github-actions bot locked as resolved and limited conversation to collaborators Sep 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

xds: implement fault injection HTTP filter (A33) #4236

xds: implement fault injection HTTP filter (A33) #4236

dfawley commented Mar 2, 2021

menghanl Mar 5, 2021

dfawley Mar 5, 2021

menghanl Mar 5, 2021

dfawley Mar 5, 2021

menghanl Mar 5, 2021

dfawley Mar 5, 2021

menghanl Mar 5, 2021

dfawley Mar 5, 2021

menghanl Mar 5, 2021

dfawley Mar 5, 2021

menghanl Mar 5, 2021

dfawley Mar 5, 2021

dfawley left a comment

dfawley Mar 5, 2021

dfawley Mar 5, 2021

dfawley Mar 5, 2021

dfawley Mar 5, 2021

dfawley Mar 5, 2021

dfawley Mar 5, 2021

menghanl commented Mar 8, 2021

dfawley commented Mar 9, 2021

menghanl Mar 11, 2021

	// Always return nil; io.EOF is the only error that might make sense
	// instead, but there is no need to signal the client to call RecvMsg
	// as the only use left for the stream after CloseSend is to call
	// RecvMsg. This also matches historical behavior.

xds: implement fault injection HTTP filter (A33) #4236

xds: implement fault injection HTTP filter (A33) #4236

Conversation

dfawley commented Mar 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfawley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

menghanl commented Mar 8, 2021

dfawley commented Mar 9, 2021

Choose a reason for hiding this comment