From 7962ea54cdf3fe010b923aa53a3a9e25aeb31642 Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Thu, 31 Oct 2024 15:35:42 -0700 Subject: [PATCH 1/2] web: add feature flag PropagateCancels This allow client-initiated cancels to propagate through gRPC. --- features/features.go | 9 +++++++++ test/config-next/wfe2.json | 1 + web/context.go | 13 ++++++++----- web/context_test.go | 36 ++++++++++++++++++++++++++++++++++++ 4 files changed, 54 insertions(+), 5 deletions(-) diff --git a/features/features.go b/features/features.go index ce677a99ed9..33386c615a5 100644 --- a/features/features.go +++ b/features/features.go @@ -103,6 +103,15 @@ type Config struct { // This flag should only be used in conjunction with UseKvLimitsForNewOrder. DisableLegacyLimitWrites bool + // PropagateCancels controls whether the WFE and ocsp-responder allows cancellation + // of an inbound request to cancel downstream gRPC and other queries. In practice, + // cancellation of an inbound request is achieved by Nginx closing the connection + // on which the request was happening. This may help shed load in overcapacity + // situations. However, not that in-progress database queries (for instance, in the + // SA) are not cancelled. Database queries waiting for an available connection may + // be cancelled. + PropagateCancels bool + // InsertAuthzsIndividually causes the SA's NewOrderAndAuthzs method to // create each new authz one at a time, rather than using MultiInserter. // Although this is expected to be a performance penalty, it is necessary to diff --git a/test/config-next/wfe2.json b/test/config-next/wfe2.json index 78977574241..5ad435d1d4b 100644 --- a/test/config-next/wfe2.json +++ b/test/config-next/wfe2.json @@ -127,6 +127,7 @@ "Overrides": "test/config-next/wfe2-ratelimit-overrides.yml" }, "features": { + "PropagateCancels": true, "ServeRenewalInfo": true, "CheckIdentifiersPaused": true, "UseKvLimitsForNewOrder": true diff --git a/web/context.go b/web/context.go index 24943858947..a748137a0aa 100644 --- a/web/context.go +++ b/web/context.go @@ -12,6 +12,7 @@ import ( "strings" "time" + "github.com/letsencrypt/boulder/features" blog "github.com/letsencrypt/boulder/log" ) @@ -127,11 +128,13 @@ func (th *TopHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { Origin: r.Header.Get("Origin"), Extra: make(map[string]interface{}), } - // We specifically override the default r.Context() because we would prefer - // for clients to not be able to cancel our operations in arbitrary places. - // Instead we start a new context, and apply timeouts in our various RPCs. - ctx := context.WithoutCancel(r.Context()) - r = r.WithContext(ctx) + if !features.Get().PropagateCancels { + // We specifically override the default r.Context() because we would prefer + // for clients to not be able to cancel our operations in arbitrary places. + // Instead we start a new context, and apply timeouts in our various RPCs. + ctx := context.WithoutCancel(r.Context()) + r = r.WithContext(ctx) + } // Some clients will send a HTTP Host header that includes the default port // for the scheme that they are using. Previously when we were fronted by diff --git a/web/context_test.go b/web/context_test.go index a5e806c557c..ed98597cdc0 100644 --- a/web/context_test.go +++ b/web/context_test.go @@ -2,13 +2,16 @@ package web import ( "bytes" + "context" "crypto/tls" "fmt" "net/http" "net/http/httptest" "strings" "testing" + "time" + "github.com/letsencrypt/boulder/features" blog "github.com/letsencrypt/boulder/log" "github.com/letsencrypt/boulder/test" ) @@ -117,3 +120,36 @@ func TestHostHeaderRewrite(t *testing.T) { req.Host = "localhost:123" th.ServeHTTP(httptest.NewRecorder(), req) } + +type cancelHandler struct { + res chan string +} + +func (ch cancelHandler) ServeHTTP(e *RequestEvent, w http.ResponseWriter, r *http.Request) { + select { + case <-r.Context().Done(): + ch.res <- r.Context().Err().Error() + case <-time.After(300 * time.Millisecond): + ch.res <- "300 ms passed" + } +} + +func TestPropagateCancel(t *testing.T) { + mockLog := blog.UseMock() + res := make(chan string) + features.Set(features.Config{PropagateCancels: true}) + th := NewTopHandler(mockLog, cancelHandler{res}) + ctx, cancel := context.WithCancel(context.Background()) + go func() { + req, err := http.NewRequestWithContext(ctx, "GET", "/thisisignored", &bytes.Reader{}) + if err != nil { + t.Error(err) + } + th.ServeHTTP(httptest.NewRecorder(), req) + }() + cancel() + result := <-res + if result != "context canceled" { + t.Errorf("expected 'context canceled', got %q", result) + } +} From 02304bbba2905248f736329d4503073aa5d61213 Mon Sep 17 00:00:00 2001 From: Aaron Gable Date: Mon, 4 Nov 2024 14:12:36 -0800 Subject: [PATCH 2/2] Fix typo and formatting --- features/features.go | 14 +++++++------- test/config-next/wfe2.json | 2 +- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/features/features.go b/features/features.go index 33386c615a5..b240b2b68f0 100644 --- a/features/features.go +++ b/features/features.go @@ -103,13 +103,13 @@ type Config struct { // This flag should only be used in conjunction with UseKvLimitsForNewOrder. DisableLegacyLimitWrites bool - // PropagateCancels controls whether the WFE and ocsp-responder allows cancellation - // of an inbound request to cancel downstream gRPC and other queries. In practice, - // cancellation of an inbound request is achieved by Nginx closing the connection - // on which the request was happening. This may help shed load in overcapacity - // situations. However, not that in-progress database queries (for instance, in the - // SA) are not cancelled. Database queries waiting for an available connection may - // be cancelled. + // PropagateCancels controls whether the WFE and ocsp-responder allows + // cancellation of an inbound request to cancel downstream gRPC and other + // queries. In practice, cancellation of an inbound request is achieved by + // Nginx closing the connection on which the request was happening. This may + // help shed load in overcapacity situations. However, note that in-progress + // database queries (for instance, in the SA) are not cancelled. Database + // queries waiting for an available connection may be cancelled. PropagateCancels bool // InsertAuthzsIndividually causes the SA's NewOrderAndAuthzs method to diff --git a/test/config-next/wfe2.json b/test/config-next/wfe2.json index 5ad435d1d4b..d2c50f5a321 100644 --- a/test/config-next/wfe2.json +++ b/test/config-next/wfe2.json @@ -127,7 +127,7 @@ "Overrides": "test/config-next/wfe2-ratelimit-overrides.yml" }, "features": { - "PropagateCancels": true, + "PropagateCancels": true, "ServeRenewalInfo": true, "CheckIdentifiersPaused": true, "UseKvLimitsForNewOrder": true