[harshkha3417]

Fixes #7807.

Motivation

When ShenYu Admin runs in cluster mode, follower nodes forward requests to the master node via ClusterForwardFilter. Previously, copyHeaders(responseEntity.getHeaders(), response) blindly copied all response headers from the master directly into the client-facing HttpServletResponse.

Because the master node responds with chunked transfer encoding (Transfer-Encoding: chunked), which RestTemplate decodes into raw bytes:

1. When clients send Connection: close, the follower emits Transfer-Encoding: chunked in the response headers while Tomcat emits an unchunked stream. HTTP clients fail when trying to parse the unchunked body as chunk-length hex, crashing with IncompleteRead or protocol errors.
2. When clients send Connection: keep-alive, Tomcat chunk-encodes the stream and adds its own framing headers, resulting in duplicated Transfer-Encoding: chunked lines and duplicated Connection: keep-alive, keep-alive values.
3. On the request side, client hop-by-hop headers and the follower's Host header were forwarded to the master instead of letting the HTTP client handle connection framing.

Modifications

1. Hop-by-Hop & Framing Header Filtering:

   • Filter standard hop-by-hop headers (Connection, Keep-Alive, Proxy-Authenticate, Proxy-Authorization, TE, Trailer, Transfer-Encoding, Upgrade) in both request and response copying in ClusterForwardFilter.
   • Dynamically inspect and strip any custom tokens listed in Connection header values.
   • On the response side, strip Content-Length from the master and explicitly set response.setContentLength(body.length) based on actual decoded payload bytes (omitted on 204 / 304).
   • On the request side, strip client Host, Content-Length, and hop-by-hop headers to allow RestTemplate to target the master correctly. Support multi-valued request headers.

2. Test Coverage:

   • Added ClusterForwardFilterTest covering:
     • Unit tests for request header filtering (Host, Connection, Content-Length, Transfer-Encoding, custom connection tokens).
     • Unit tests for response header filtering (Transfer-Encoding, Connection, Keep-Alive, Content-Length, custom tokens).
     • Real HTTP integration test with master returning chunked encoding and embedded Tomcat follower under Connection: close verifying complete read without IncompleteRead, absence of Transfer-Encoding: chunked, and valid Content-Length.
     • Real HTTP integration test with embedded Tomcat follower under Connection: keep-alive verifying absence of duplicate Transfer-Encoding and duplicate Connection headers.

Verification

• Passed ./mvnw test -pl shenyu-admin -Dtest=ClusterForwardFilterTest
• Passed ./mvnw checkstyle:check -pl shenyu-admin (0 violations)
• Passed ./mvnw apache-rat:check -pl shenyu-admin (0 unapproved files)

[Aias00]

Approved on content. It needs a rebase before it can merge — the branch is CONFLICTING — but the change itself is correct and fixes a real response-corruption bug.

The fix is right in both directions:

Response side. Stripping the hop-by-hop set and then setting Content-Length from the actual buffered byte count is exactly the correction. The old code copied Transfer-Encoding: chunked from the master even though RestTemplate.exchange(..., byte[].class) had already decoded the body into a byte array, so Tomcat emitted both a chunked framing and its own content length — the client either hangs waiting for a chunk terminator that never comes or sees a doubled body. Recomputing the length from the bytes you are actually writing is the only consistent answer when you have already materialised the body.

Request side. Dropping Host and Content-Length alongside the hop-by-hop headers is the detail that makes forwarding correct rather than "almost correct": Host must describe the master, not the original client request, and the body is re-sent by RestTemplate, which recomputes the length. Copying the original Content-Length would have re-introduced the mismatch in the other direction.

Dynamic tokens. buildHopByHopHeaders also folds in whatever the Connection header names, which is the part of RFC 7230 §6.1 that is usually forgotten and the part that makes the static set insufficient on its own.

Status-code handling. Skipping setContentLength for 204, 304 and 1xx is correct — those must not carry a body length — and the null-body fallback to new byte[0] replaces a requireNonNull that would have thrown on a legitimately empty response.

CORS. Guarding with containsHeader before setHeader avoids the duplicate Access-Control-Allow-Origin the old unconditional addHeader produced whenever the master had already set one.

Two minor notes:

• A trailing comma in Connection: keep-alive, produces an empty token that gets added to the set. Harmless because no header is named "", but StringUtils.isNotBlank on the token would keep the set clean.
• Making copyHeaders package-private for tests, plus a second constructor and two setters, adds a bit of surface. Acceptable for testability, but the setters in particular are worth a @VisibleForTesting-style comment so nobody wires them in production by accident.

Once rebased, the CI on this will still show the inherited InstanceCheckServiceTest failure until #7846 lands — that is unrelated.

[Aias00]

Re-approving after I rebased this branch onto current master (the force-push dismissed the previous approval).

What I did

• Rebased the single commit onto master at 8af03fc as the maintainer, because the branch had gone CONFLICTING.
• The main source auto-merged. ClusterForwardFilter.java now carries both constructors — the original one-arg one and your three-arg one — and both copyHeaders overloads; nothing of yours was dropped.
• The only conflict was ClusterForwardFilterTest.java, and it was an add/add conflict: master gained its own copy of that file from #7797 (the /instance/beat forward-list coverage) while this PR added the hop-by-hop coverage, so git could not merge them and emitted one whole-file conflict.

How I resolved it

I merged the two test classes into one rather than picking a side, because they cover disjoint behaviour and both are worth keeping:

• Kept master's four tests as-is: testMasterHandlesRequestsLocally, testInstanceBeatForwardedToMaster, testUnlistedPathHandledLocally, testApplicationYmlForwardsInstanceBeat, together with the @ExtendWith(MockitoExtension.class) / @MockitoSettings(LENIENT) setup, the restTemplate and clusterSelectMasterService mocks, and the @BeforeEach setUp() that builds the one-arg filter via ReflectionTestUtils.
• Kept all four of yours: testCopyRequestHeadersExcludesHopByHopAndHostAndContentLength, testCopyResponseHeadersExcludesHopByHopAndContentLength, testRealHttpFollowerForwardingWithConnectionClose, testRealHttpFollowerForwardingWithConnectionKeepAlive, together with the static HttpServer + embedded Tomcat fixture.
• One rename was unavoidable: master's @BeforeEach setUp() and your static @BeforeAll setUp() have the same signature, so Java would not accept both. Your static methods are now setUpServers() / tearDownServers(); the bodies are unchanged.
• Imports are the union of both versions, deduplicated and ordered; assertFalse (yours only) and assertNotNull (master's only) are both present, and every import is used.
• I verified the result: no conflict markers, braces and parens balanced, no duplicate method names, no duplicate imports.

The functional review below still stands unchanged — the rebase touched only the test file, and only by juxtaposing two test sets that were already passing independently.

---

Original review:

The fix is right in both directions.

Response side. Stripping the hop-by-hop set and then setting Content-Length from the actual buffered byte count is exactly the correction. The old code copied Transfer-Encoding: chunked from the master even though RestTemplate.exchange(..., byte[].class) had already decoded the body into a byte array, so Tomcat emitted both a chunked framing and its own content length — the client either hangs waiting for a chunk terminator that never comes or sees a doubled body. Recomputing the length from the bytes you are actually writing is the only consistent answer when you have already materialised the body.

Request side. Dropping Host and Content-Length alongside the hop-by-hop headers is the detail that makes forwarding correct rather than "almost correct": Host must describe the master, not the original client request, and the body is re-sent by RestTemplate, which recomputes the length. Copying the original Content-Length would have re-introduced the mismatch in the other direction.

Dynamic tokens. buildHopByHopHeaders also folds in whatever the Connection header names, which is the part of RFC 7230 section 6.1 that is usually forgotten and the part that makes the static set insufficient on its own.

Status-code handling. Skipping setContentLength for 204, 304 and 1xx is correct — those must not carry a body length — and the null-body fallback to new byte[0] replaces a requireNonNull that would have thrown on a legitimately empty response.

CORS. Guarding with containsHeader before setHeader avoids the duplicate Access-Control-Allow-Origin the old unconditional addHeader produced whenever the master had already set one.

Two minor notes:

• A trailing comma in Connection: keep-alive, produces an empty token that gets added to the set. Harmless because no header is named "", but StringUtils.isNotBlank on the token would keep the set clean.
• Making copyHeaders package-private for tests, plus a second constructor and two setters, adds a bit of surface. Acceptable for testability, but the setters in particular are worth a @VisibleForTesting-style comment so nobody wires them in production by accident.

[harshkha3417]

@Aias00 CI approved.Ready to merge!!