golang filter: do not clear route cache in ·HeaderMap.Set` by default. (

envoyproxy#33229)

* golang filter: do not clear route cache in HeaderMap.Set by default.

introduce a new API `ClearRouteCache` to clear route cache.
fix envoyproxy#33082

Signed-off-by: doujiang24 <[email protected]>

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*

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,

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)
 

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)

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 {

contrib/golang/filters/http/source/cgo.cc

@@ -71,6 +71,11 @@ envoyGoConfigHandlerWrapper(void* c, std::function<CAPIStatus(std::shared_ptr<Fi
   return CAPIStatus::CAPIFilterIsGone;
 }
 
+CAPIStatus envoyGoFilterHttpClearRouteCache(void* r) {
+  return envoyGoFilterHandlerWrapper(
+      r, [](std::shared_ptr<Filter>& filter) -> CAPIStatus { return filter->clearRouteCache(); });
+}
+
 CAPIStatus envoyGoFilterHttpContinue(void* r, int status) {
   return envoyGoFilterHandlerWrapper(r, [status](std::shared_ptr<Filter>& filter) -> CAPIStatus {
     return filter->continueStatus(static_cast<GolangStatus>(status));

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)

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")

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

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");
       }

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<void(Http::ResponseHeaderMap& headers)> modify_headers,
                               Grpc::Status::GrpcStatus grpc_status, absl::string_view details);

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");

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
 }