From 37d7d9a8df533ce3d35f438cc207a2961109888e Mon Sep 17 00:00:00 2001 From: doujiang24 Date: Mon, 1 Apr 2024 16:12:10 +0800 Subject: [PATCH] =?UTF-8?q?golang=20filter:=20do=20not=20clear=20route=20c?= =?UTF-8?q?ache=20in=20=C2=B7HeaderMap.Set`=20by=20default.=20(#33229)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * golang filter: do not clear route cache in HeaderMap.Set by default. introduce a new API `ClearRouteCache` to clear route cache. fix #33082 Signed-off-by: doujiang24 --- changelogs/current.yaml | 3 ++ contrib/golang/common/go/api/api.h | 1 + contrib/golang/common/go/api/capi.go | 1 + contrib/golang/common/go/api/filter.go | 3 ++ contrib/golang/common/go/api/type.go | 13 +++++++ contrib/golang/filters/http/source/cgo.cc | 5 +++ .../http/source/go/pkg/http/capi_impl.go | 5 +++ .../filters/http/source/go/pkg/http/filter.go | 4 ++ .../filters/http/source/go/pkg/http/type.go | 12 ++++++ .../filters/http/source/golang_filter.cc | 29 +++++++------- .../filters/http/source/golang_filter.h | 3 +- .../http/test/golang_integration_test.cc | 39 +++++++++++++++++++ .../http/test/test_data/basic/filter.go | 11 ++++++ 13 files changed, 114 insertions(+), 15 deletions(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index e7cb2dadf228d..f4067567627e5 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -98,6 +98,9 @@ minor_behavior_changes: change: | Delta SDS removals will no longer result in a "Missing SDS resources" NACK and instead will be ignored. +- area: golang + change: | + Not implicitly clearing route cache in ``HeaderMap.Set``, introduce a new API ``ClearRouteCache`` to do it. bug_fixes: # *Changes expected to improve the state of the world and are unlikely to have negative effects* diff --git a/contrib/golang/common/go/api/api.h b/contrib/golang/common/go/api/api.h index 66d9f5f1cc271..850df3b986253 100644 --- a/contrib/golang/common/go/api/api.h +++ b/contrib/golang/common/go/api/api.h @@ -53,6 +53,7 @@ typedef enum { // NOLINT(modernize-use-using) CAPISerializationFailure = -8, } CAPIStatus; +CAPIStatus envoyGoFilterHttpClearRouteCache(void* r); CAPIStatus envoyGoFilterHttpContinue(void* r, int status); CAPIStatus envoyGoFilterHttpSendLocalReply(void* r, int response_code, void* body_text_data, int body_text_len, void* headers, int headers_num, diff --git a/contrib/golang/common/go/api/capi.go b/contrib/golang/common/go/api/capi.go index 2ff034c7f5108..1733cc6c2e987 100644 --- a/contrib/golang/common/go/api/capi.go +++ b/contrib/golang/common/go/api/capi.go @@ -20,6 +20,7 @@ package api import "unsafe" type HttpCAPI interface { + ClearRouteCache(r unsafe.Pointer) HttpContinue(r unsafe.Pointer, status uint64) HttpSendLocalReply(r unsafe.Pointer, responseCode int, bodyText string, headers map[string][]string, grpcStatus int64, details string) diff --git a/contrib/golang/common/go/api/filter.go b/contrib/golang/common/go/api/filter.go index 8bf79b2d1eb4d..c640bb07df7b8 100644 --- a/contrib/golang/common/go/api/filter.go +++ b/contrib/golang/common/go/api/filter.go @@ -156,6 +156,9 @@ type StreamFilterCallbacks interface { type FilterCallbacks interface { StreamFilterCallbacks + // ClearRouteCache clears the route cache for the current request, and filtermanager will re-fetch the route in the next filter. + // Please be careful to invoke it, since filtermanager will raise an 404 route_not_found response when failed to re-fetch a route. + ClearRouteCache() // Continue or SendLocalReply should be last API invoked, no more code after them. Continue(StatusType) SendLocalReply(responseCode int, bodyText string, headers map[string][]string, grpcStatus int64, details string) diff --git a/contrib/golang/common/go/api/type.go b/contrib/golang/common/go/api/type.go index bcecdff69256d..da02e9b7cffb8 100644 --- a/contrib/golang/common/go/api/type.go +++ b/contrib/golang/common/go/api/type.go @@ -109,16 +109,19 @@ type HeaderMap interface { // Set key-value pair in header map, the previous pair will be replaced if exists. // It may not take affects immediately in the Envoy thread side when it's invoked in a Go thread. + // This won't refresh route cache, please invoke ClearRouteCache if needed. Set(key, value string) // Add value for given key. // Multiple headers with the same key may be added with this function. // Use Set for setting a single header for the given key. // It may not take affects immediately in the Envoy thread side when it's invoked in a Go thread. + // This won't refresh route cache, please invoke ClearRouteCache if needed. Add(key, value string) // Del delete pair of specified key // It may not take affects immediately in the Envoy thread side when it's invoked in a Go thread. + // This won't refresh route cache, please invoke ClearRouteCache if needed. Del(key string) // Range calls f sequentially for each key and value present in the map. @@ -136,6 +139,16 @@ type RequestHeaderMap interface { Method() string Host() string Path() string + // SetMethod set method in header map + // This won't refresh route cache, please invoke ClearRouteCache if needed. + SetMethod(method string) + // SetHost set host in header map + // This won't refresh route cache, please invoke ClearRouteCache if needed. + SetHost(host string) + // SetPath set path in header map + // This won't refresh route cache, please invoke ClearRouteCache if needed. + SetPath(path string) + // Note: Scheme is the downstream protocol, we'd better not override it. } type RequestTrailerMap interface { diff --git a/contrib/golang/filters/http/source/cgo.cc b/contrib/golang/filters/http/source/cgo.cc index 5cd8e047ba437..5117bc7b8b9dc 100644 --- a/contrib/golang/filters/http/source/cgo.cc +++ b/contrib/golang/filters/http/source/cgo.cc @@ -71,6 +71,11 @@ envoyGoConfigHandlerWrapper(void* c, std::function& filter) -> CAPIStatus { return filter->clearRouteCache(); }); +} + CAPIStatus envoyGoFilterHttpContinue(void* r, int status) { return envoyGoFilterHandlerWrapper(r, [status](std::shared_ptr& filter) -> CAPIStatus { return filter->continueStatus(static_cast(status)); diff --git a/contrib/golang/filters/http/source/go/pkg/http/capi_impl.go b/contrib/golang/filters/http/source/go/pkg/http/capi_impl.go index a9a91abd921a3..5f44b7acdfa59 100644 --- a/contrib/golang/filters/http/source/go/pkg/http/capi_impl.go +++ b/contrib/golang/filters/http/source/go/pkg/http/capi_impl.go @@ -110,6 +110,11 @@ func capiStatusToErr(status C.CAPIStatus) error { return errors.New("unknown status") } +func (c *httpCApiImpl) ClearRouteCache(r unsafe.Pointer) { + res := C.envoyGoFilterHttpClearRouteCache(r) + handleCApiStatus(res) +} + func (c *httpCApiImpl) HttpContinue(r unsafe.Pointer, status uint64) { res := C.envoyGoFilterHttpContinue(r, C.int(status)) handleCApiStatus(res) diff --git a/contrib/golang/filters/http/source/go/pkg/http/filter.go b/contrib/golang/filters/http/source/go/pkg/http/filter.go index 029ebf612198a..421fb603e8ce4 100644 --- a/contrib/golang/filters/http/source/go/pkg/http/filter.go +++ b/contrib/golang/filters/http/source/go/pkg/http/filter.go @@ -157,6 +157,10 @@ func (r *httpRequest) RecoverPanic() { } } +func (r *httpRequest) ClearRouteCache() { + cAPI.ClearRouteCache(unsafe.Pointer(r.req)) +} + func (r *httpRequest) Continue(status api.StatusType) { if status == api.LocalReply { fmt.Printf("warning: LocalReply status is useless after sendLocalReply, ignoring") diff --git a/contrib/golang/filters/http/source/go/pkg/http/type.go b/contrib/golang/filters/http/source/go/pkg/http/type.go index 8dd8bad8a366f..1a2b0033b290d 100644 --- a/contrib/golang/filters/http/source/go/pkg/http/type.go +++ b/contrib/golang/filters/http/source/go/pkg/http/type.go @@ -182,6 +182,18 @@ func (h *requestHeaderMapImpl) Host() string { return v } +func (h *requestHeaderMapImpl) SetMethod(method string) { + h.Set(":method", method) +} + +func (h *requestHeaderMapImpl) SetPath(path string) { + h.Set(":path", path) +} + +func (h *requestHeaderMapImpl) SetHost(host string) { + h.Set(":authority", host) +} + // api.ResponseHeaderMap type responseHeaderMapImpl struct { requestOrResponseHeaderMapImpl diff --git a/contrib/golang/filters/http/source/golang_filter.cc b/contrib/golang/filters/http/source/golang_filter.cc index edfab63a4e9c1..e8648ffbaa596 100644 --- a/contrib/golang/filters/http/source/golang_filter.cc +++ b/contrib/golang/filters/http/source/golang_filter.cc @@ -31,13 +31,6 @@ namespace Extensions { namespace HttpFilters { namespace Golang { -void Filter::onHeadersModified() { - // Any changes to request headers can affect how the request is going to be - // routed. If we are changing the headers we also need to clear the route - // cache. - decoding_state_.getFilterCallbacks()->downstreamCallbacks()->clearRouteCache(); -} - Http::LocalErrorStatus Filter::onLocalReply(const LocalReplyData& data) { auto& state = getProcessorState(); ASSERT(state.isThreadSafe()); @@ -402,6 +395,22 @@ bool Filter::doTrailer(ProcessorState& state, Http::HeaderMap& trailers) { /*** APIs for go call C ***/ +CAPIStatus Filter::clearRouteCache() { + Thread::LockGuard lock(mutex_); + if (has_destroyed_) { + ENVOY_LOG(debug, "golang filter has been destroyed"); + return CAPIStatus::CAPIFilterIsDestroy; + } + auto& state = getProcessorState(); + if (!state.isProcessingInGo()) { + ENVOY_LOG(debug, "golang filter is not processing Go"); + return CAPIStatus::CAPINotInGo; + } + ENVOY_LOG(debug, "golang filter clearing route cache"); + decoding_state_.getFilterCallbacks()->downstreamCallbacks()->clearRouteCache(); + return CAPIStatus::CAPIOK; +} + void Filter::continueEncodeLocalReply(ProcessorState& state) { ENVOY_LOG(debug, "golang filter continue encodeHeader(local reply from other filters) after return from " @@ -702,8 +711,6 @@ CAPIStatus Filter::setHeader(absl::string_view key, absl::string_view value, hea default: RELEASE_ASSERT(false, absl::StrCat("unknown header action: ", act)); } - - onHeadersModified(); } else { // should deep copy the string_view before post to dipatcher callback. auto key_str = std::string(key); @@ -728,8 +735,6 @@ CAPIStatus Filter::setHeader(absl::string_view key, absl::string_view value, hea default: RELEASE_ASSERT(false, absl::StrCat("unknown header action: ", act)); } - - onHeadersModified(); } else { ENVOY_LOG(debug, "golang filter has gone or destroyed in setHeader"); } @@ -759,7 +764,6 @@ CAPIStatus Filter::removeHeader(absl::string_view key) { if (state.isThreadSafe()) { // it's safe to write header in the safe thread. headers_->remove(Http::LowerCaseString(key)); - onHeadersModified(); } else { // should deep copy the string_view before post to dipatcher callback. auto key_str = std::string(key); @@ -772,7 +776,6 @@ CAPIStatus Filter::removeHeader(absl::string_view key) { if (!weak_ptr.expired() && !hasDestroyed()) { Thread::LockGuard lock(mutex_); headers_->remove(Http::LowerCaseString(key_str)); - onHeadersModified(); } else { ENVOY_LOG(debug, "golang filter has gone or destroyed in removeHeader"); } diff --git a/contrib/golang/filters/http/source/golang_filter.h b/contrib/golang/filters/http/source/golang_filter.h index cd90766e13369..ec9dcdf41754e 100644 --- a/contrib/golang/filters/http/source/golang_filter.h +++ b/contrib/golang/filters/http/source/golang_filter.h @@ -211,6 +211,7 @@ class Filter : public Http::StreamFilter, void onStreamComplete() override {} + CAPIStatus clearRouteCache(); CAPIStatus continueStatus(GolangStatus status); CAPIStatus sendLocalReply(Http::Code response_code, std::string body_text, @@ -264,8 +265,6 @@ class Filter : public Http::StreamFilter, void continueStatusInternal(GolangStatus status); void continueData(ProcessorState& state); - void onHeadersModified(); - void sendLocalReplyInternal(Http::Code response_code, absl::string_view body_text, std::function modify_headers, Grpc::Status::GrpcStatus grpc_status, absl::string_view details); diff --git a/contrib/golang/filters/http/test/golang_integration_test.cc b/contrib/golang/filters/http/test/golang_integration_test.cc index 56e568caa596a..3362621ebc657 100644 --- a/contrib/golang/filters/http/test/golang_integration_test.cc +++ b/contrib/golang/filters/http/test/golang_integration_test.cc @@ -506,6 +506,35 @@ name: golang cleanup(); } + void testRouteCache(std::string path, bool clear) { + initializeBasicFilter(BASIC); + + codec_client_ = makeHttpConnection(makeClientConnection(lookupPort("http"))); + Http::TestRequestHeaderMapImpl request_headers{ + {":method", "POST"}, {":path", path}, {":scheme", "http"}, {":authority", "test.com"}}; + + auto encoder_decoder = codec_client_->startRequest(request_headers, true); + auto response = std::move(encoder_decoder.second); + + // no route found after clearing + if (!clear) { + waitForNextUpstreamRequest(); + Http::TestResponseHeaderMapImpl response_headers{{":status", "200"}}; + upstream_request_->encodeHeaders(response_headers, true); + } + + ASSERT_TRUE(response->waitForEndStream()); + + // check resp status + if (clear) { + EXPECT_EQ("404", response->headers().getStatusValue()); + } else { + EXPECT_EQ("200", response->headers().getStatusValue()); + } + + cleanup(); + } + void testSendLocalReply(std::string path, std::string phase) { initializeBasicFilter(BASIC); @@ -1107,6 +1136,16 @@ TEST_P(GolangIntegrationTest, RouteConfig_Route) { testRouteConfig("test.com", "/route-config-test", false, "baz"); } +// Set new path without clear route cache, will get 200 response status +TEST_P(GolangIntegrationTest, RouteCache_noClear) { + testRouteCache("/test?newPath=/not-found-path", false); +} + +// Set new path with clear route cache, will get 404 response status +TEST_P(GolangIntegrationTest, RouteCache_Clear) { + testRouteCache("/test?newPath=/not-found-path&clearRoute=1", true); +} + // Out of range in decode header phase TEST_P(GolangIntegrationTest, PanicRecover_DecodeHeader) { testPanicRecover("/test?panic=decode-header", "decode-header"); diff --git a/contrib/golang/filters/http/test/test_data/basic/filter.go b/contrib/golang/filters/http/test/test_data/basic/filter.go index 9990be5b4b771..5a81074bbdf47 100644 --- a/contrib/golang/filters/http/test/test_data/basic/filter.go +++ b/contrib/golang/filters/http/test/test_data/basic/filter.go @@ -32,6 +32,8 @@ type filter struct { databuffer string // return api.Stop panic string // hit panic in which phase badapi bool // bad api call + newPath string // set new path + clearRoute bool // clear route cache } func parseQuery(path string) url.Values { @@ -76,6 +78,8 @@ func (f *filter) initRequest(header api.RequestHeaderMap) { f.localreplay = f.query_params.Get("localreply") f.panic = f.query_params.Get("panic") f.badapi = f.query_params.Get("badapi") != "" + f.newPath = f.query_params.Get("newPath") + f.clearRoute = f.query_params.Get("clearRoute") != "" } func (f *filter) fail(msg string, a ...any) api.StatusType { @@ -227,6 +231,13 @@ func (f *filter) decodeHeaders(header api.RequestHeaderMap, endStream bool) api. if f.panic == "decode-header" { badcode() } + + if f.newPath != "" { + header.SetPath(f.newPath) + } + if f.clearRoute { + f.callbacks.ClearRouteCache() + } return api.Continue }