[TT-12318] SSE streaming is broken

titpetric commented 1 month ago

User description

This PR mends the evaluation if the cache should be used, considering:

request: websockets (not cached), upgrade header
request: a grpc streaming request (not cached)
response: a SSE (server side streaming events) header

Previously the checks would handle only the request side to set a caching flag to true; the change sets the flag to true absolutely, and then evaluates the request and response disabling the cache before copying the request into a buffer can occur.

The underlying cause/issue of the regression was a skipped flaky test for SSE. The test case has now been restored to health.

Fixes: https://tyktech.atlassian.net/browse/TT-12318 https://tyktech.atlassian.net/browse/TT-5250

Closes: #6322

PR Type

Enhancement, Tests, Configuration changes

Description

Replaced specific gRPC streaming checks with generic streaming request checks.
Simplified and refactored reverse proxy logic to handle streaming requests and responses.
Added utility functions for streaming and upgrade checks.
Refactored and stabilized SSE test.
Simplified linting commands in Makefile.
Modularized service definitions in Docker Compose by including external Redis and HTTPBin services.

Changes walkthrough 📝

Relevant files

Enhancement

handler_success.go `Replace gRPC streaming check with generic streaming request check` gateway/handler_success.go - Replaced `IsGrpcStreaming` with `httputil.IsStreamingRequest`.	+1/-1
reverse_proxy.go `Refactor reverse proxy to handle streaming requests and responses` gateway/reverse_proxy.go Replaced `IsGrpcStreaming` with `httputil.IsStreamingRequest`. Simplified `ServeHTTP` method by removing conditional streaming check. Refactored `IsUpgrade` method to return upgrade type and boolean. Added streaming request and response checks to disable caching.	+13/-26
streaming.go `Add utility functions for streaming and upgrade checks` internal/httputil/streaming.go Added utility functions to check for streaming requests and responses. Added utility function to check for upgrade requests.	+48/-0

Tests

reverse_proxy_test.go

Refactor and stabilize SSE test

gateway/reverse_proxy_test.go

Removed flaky test annotation.

Refactored SSE test to use context for timeout and synchronization.

+28/-20

Configuration changes

Makefile `Simplify linting commands in Makefile` Makefile - Simplified linting commands by using `task lint`.	+2/-14
docker-compose.yml `Modularize service definitions in Docker Compose` docker-compose.yml Included external service definitions for Redis and HTTPBin. Removed inline Redis service definition.	+4/-7
docker-compose.yml `Include HTTPBin service in Docker Compose` docker/services/docker-compose.yml - Included HTTPBin service definition.	+1/-0
httpbin.yml `Add HTTPBin service definition` docker/services/httpbin.yml - Added HTTPBin service definition for testing.	+15/-0

💡 PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

github-actions[bot] commented 1 month ago

API Changes

--- prev.txt    2024-07-09 09:47:54.064194546 +0000
+++ current.txt 2024-07-09 09:47:50.800196110 +0000
@@ -7636,9 +7636,6 @@
     InstrumentationMW will set basic instrumentation events, variables and
     timers on API jobs

-func IsGrpcStreaming(r *http.Request) bool
-    IsGrpcStreaming determines wether a request represents a grpc streaming req
-
 func JSONToFormValues(r *http.Request) error
     JSONToFormValues if r has header Content-Type set to application/json this
     will decode request body as json to map[string]string and adds the key/value
@@ -10036,7 +10033,10 @@

 func (p *ReverseProxy) HandleResponse(rw http.ResponseWriter, res *http.Response, ses *user.SessionState) error

-func (p *ReverseProxy) IsUpgrade(req *http.Request) (bool, string)
+func (p *ReverseProxy) IsUpgrade(req *http.Request) (string, bool)
+    IsUpgrade will return the upgrade header value and true if present for the
+    request. It requires EnableWebSockets to be enabled in the gateway HTTP
+    server config.

 func (p *ReverseProxy) ServeHTTP(rw http.ResponseWriter, req *http.Request) ProxyResponse

github-actions[bot] commented 1 month ago

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Key issues to review

**Possible Bug:** The refactoring in `gateway/reverse_proxy.go` changes the logic for handling streaming requests and responses. The new implementation checks for streaming requests and responses and sets `withCache` to false accordingly. This is a significant change in behavior and should be thoroughly tested to ensure it does not introduce new bugs or regressions, especially in edge cases where streaming might not be properly detected. **Refactoring Impact:** The changes in `gateway/reverse_proxy.go` and `internal/httputil/streaming.go` introduce new utility functions and alter the flow of request handling. It's crucial to ensure that these changes do not impact the performance or functionality of the proxy, particularly under high load or with complex request scenarios.

github-actions[bot] commented 1 month ago

PR Code Suggestions ✨

Category	Suggestion	Score
Possible bug	Ensure dynamic handling of streaming requests in the proxy service ___ Replace the hardcoded boolean parameter with a dynamic check for streaming requests to ensure that the proxy handles different types of streaming correctly. [gateway/reverse_proxy.go [518]](https://github.com/TykTechnologies/tyk/pull/6391/files#diff-e6e07722257f7e41691e471185ad6d84fd56dc9e5459526ea32e9a5e8fa1a01bR518-R518) ```diff -resp := p.WrappedServeHTTP(rw, req, true) +resp := p.WrappedServeHTTP(rw, req, !httputil.IsStreamingRequest(req)) ``` Suggestion importance[1-10]: 9 Why: This suggestion addresses a potential bug by dynamically checking for streaming requests instead of using a hardcoded boolean. It enhances the robustness and correctness of the proxy handling logic.	9
Enhancement	Improve test reliability by replacing static sleep with dynamic synchronization ___ Replace the static sleep in the test with a dynamic wait or synchronization mechanism to avoid flakiness and ensure that the test behavior closely mimics the real-world scenario. [gateway/reverse_proxy_test.go [1761]](https://github.com/TykTechnologies/tyk/pull/6391/files#diff-ce040f6555143f760fba6059744bc600b6954f0966dfb0fa2832b5eabf7a3c3fR1761-R1761) ```diff -time.Sleep(50 * time.Millisecond) +// Implement a dynamic wait or synchronization mechanism here +// Example: wait for a condition or signal ``` Suggestion importance[1-10]: 8 Why: Replacing static sleep with dynamic synchronization mechanisms can significantly improve test reliability and reduce flakiness, making the tests more robust and reflective of real-world scenarios.	8
Maintainability	Enhance clarity and future-proofing by explicitly checking for all types of streaming requests ___ Consider checking for both gRPC and HTTP streaming requests explicitly to ensure that the function handles all streaming types correctly. This change makes the intent clearer and ensures that any future modifications in streaming detection are localized to the utility functions without altering the business logic. [gateway/handler_success.go [332-334]](https://github.com/TykTechnologies/tyk/pull/6391/files#diff-45135957493eca37f2e3e9a81079577777133c53b27cf95ea2ff0329c05bd006R332-R334) ```diff -if httputil.IsStreamingRequest(r) { +if httputil.IsGrpcStreaming(r) \|\| httputil.IsStreamingRequest(r) { return false } ``` Suggestion importance[1-10]: 7 Why: The suggestion improves code clarity and maintainability by explicitly checking for both gRPC and HTTP streaming requests. However, the existing utility function `IsStreamingRequest` already encapsulates this logic, making the change somewhat redundant.	7
Maintainability	Improve the structure and clarity of the upgrade checking logic in HTTP utilities ___ Refactor the `IsUpgrade` function to separate concerns by creating distinct functions for checking the connection header and the upgrade header. This improves readability and maintainability. [internal/httputil/streaming.go [26-34]](https://github.com/TykTechnologies/tyk/pull/6391/files#diff-3fe81a2ca7149490b32fa630f8b3818880b6f2f4929efa416a4c8c0174d0f43aR26-R34) ```diff -connection := strings.ToLower(strings.TrimSpace(req.Header.Get(headerConnection))) -if connection != "upgrade" { +if !hasUpgradeConnection(req) { return "", false } -upgrade := strings.ToLower(strings.TrimSpace(req.Header.Get(headerUpgrade))) -if upgrade != "" { - return upgrade, true -} -return "", false +return getUpgradeType(req) ``` Suggestion importance[1-10]: 6 Why: The refactoring suggestion improves readability and maintainability by separating concerns within the `IsUpgrade` function. However, the existing implementation is already clear and functional, so the improvement is minor.	6