Merging to release-5.3: [TT-12318] SSE streaming is broken (#6391)

[buger]

User description

TT-12318 SSE streaming is broken (#6391)

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

---

Co-authored-by: Tit Petric tit@tyk.io

---

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 specific gRPC streaming check with a generic streaming request check. +1/-1      reverse_proxy.go Refactor reverse proxy logic for streaming requests and responses gateway/reverse_proxy.go Refactored reverse proxy logic to handle streaming requests and responses. Simplified upgrade handling and cache evaluation. +15/-26  streaming.go Add utility functions for streaming and upgrade checks      internal/httputil/streaming.go - Added utility functions for streaming and upgrade checks. +48/-0
Tests	reverse_proxy_test.go Refactor and stabilize SSE test                                                    gateway/reverse_proxy_test.go Refactored and stabilized SSE test. Improved test structure and error handling. +32/-20
Configuration changes	Makefile Simplify linting commands                                                                Makefile - Simplified linting commands. +2/-14    docker-compose.yml Modularize service definitions in Docker Compose                  docker-compose.yml Modularized service definitions by including external Redis and HTTPBin services. +6/-7      docker-compose.yml Include external Redis and HTTPBin services                            docker/services/docker-compose.yml - Included external Redis and HTTPBin services. +1/-0      httpbin.yml Add HTTPBin service definition                                                      docker/services/httpbin.yml - Added HTTPBin service definition. +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]]

:boom: CI tests failed :see_no_evil:

git-state

all ok

Please look at the run or in the Checks tab.

[github-actions[bot]]

:boom: CI tests failed :see_no_evil:

git-state

all ok

Please look at the run or in the Checks tab.

[github-actions[bot]]

API Changes

--- prev.txt    2024-07-11 09:41:37.815476924 +0000
+++ current.txt 2024-07-11 09:41:35.055495578 +0000
@@ -7274,9 +7274,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
@@ -9664,7 +9661,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]]

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 handling of streaming requests and responses. Ensure that the new checks (`httputil.IsStreamingRequest(req) || httputil.IsStreamingResponse(res)`) correctly handle all cases previously covered by specific gRPC and WebSocket checks. **Performance Concern:** The new streaming utility functions in `internal/httputil/streaming.go` are called multiple times within the same request cycle. This could introduce unnecessary overhead due to repeated header checks. Consider optimizing by storing results in the request context. **Regression Risk:** The changes to how upgrades and streaming are detected and handled could potentially introduce regressions in edge cases not covered by tests, especially in environments with mixed types of streaming and non-streaming traffic.

[github-actions[bot]]

PR Code Suggestions ✨

Category	Suggestion	Score
Possible issue	Ensure the return values from IsUpgrade are correctly assigned after changes in its definition ___ **The IsUpgrade function has changed its return type order which might affect existing calls expecting the old order. Ensure that all calls to this function have been updated to reflect this change to prevent logical errors.** [gateway/reverse_proxy.go [1097]](https://github.com/TykTechnologies/tyk/pull/6398/files#diff-e6e07722257f7e41691e471185ad6d84fd56dc9e5459526ea32e9a5e8fa1a01bR1097-R1097) ```diff -reqUpType, outReqUpgrade := p.IsUpgrade(req) +outReqUpgrade, reqUpType := p.IsUpgrade(req) ``` Suggestion importance[1-10]: 10 Why: The suggestion correctly identifies the change in the return type order of the `IsUpgrade` function and ensures that the return values are assigned correctly, preventing potential logical errors.	10
Best practice	Replace time.Sleep with a synchronization mechanism to avoid flaky tests ___ **Replace the time.Sleep in the test with a more robust synchronization mechanism. Using time.Sleep can lead to flaky tests depending on timing and system load. Consider using channels or other synchronization primitives to control the timing of events in tests.** [gateway/reverse_proxy_test.go [1762]](https://github.com/TykTechnologies/tyk/pull/6398/files#diff-ce040f6555143f760fba6059744bc600b6954f0966dfb0fa2832b5eabf7a3c3fR1762-R1762) ```diff -time.Sleep(50 * time.Millisecond) +// Example using a channel to signal readiness +readyChan := make(chan struct{}) +go func() { + // simulate work + readyChan <- struct{}{} +}() +<-readyChan // wait for goroutine to signal readiness ``` Suggestion importance[1-10]: 8 Why: Replacing `time.Sleep` with a more robust synchronization mechanism can improve test reliability and prevent flaky tests, which is a best practice in test design.	8
Enhancement	Enhance the IsUpgrade function to return an error for better error handling ___ **Refactor the IsUpgrade function to return an error along with the current return values to provide more detailed error information, which can be useful for debugging and error handling.** [internal/httputil/streaming.go [24-36]](https://github.com/TykTechnologies/tyk/pull/6398/files#diff-3fe81a2ca7149490b32fa630f8b3818880b6f2f4929efa416a4c8c0174d0f43aR24-R36) ```diff -func IsUpgrade(req *http.Request) (string, bool) { +func IsUpgrade(req *http.Request) (string, bool, error) { connection := strings.ToLower(strings.TrimSpace(req.Header.Get(headerConnection))) if connection != "upgrade" { - return "", false + return "", false, fmt.Errorf("connection header is not 'upgrade'") } upgrade := strings.ToLower(strings.TrimSpace(req.Header.Get(headerUpgrade))) if upgrade != "" { - return upgrade, true + return upgrade, true, nil } - return "", false + return "", false, fmt.Errorf("no upgrade type specified") } ``` Suggestion importance[1-10]: 7 Why: Refactoring the `IsUpgrade` function to return an error can provide more detailed error information, which is useful for debugging and error handling. However, it requires changes in all places where this function is called.	7

[github-actions[bot]]

:boom: CI tests failed :see_no_evil:

git-state

all ok

Please look at the run or in the Checks tab.

[sonarcloud[bot]]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
95.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud