Merging to release-5.3: [TT-13155] Explicitly copy BaseMiddleware for each middleware that takes it (#6744)

[buger]

User description

TT-13155 Explicitly copy BaseMiddleware for each middleware that takes it (#6744)

TT-13155

Summary	[Regression] Gateway Debug logs starting v5.3 only logs AccessRightsCheck for most of the middlewares
Type	Bug
Status	In Dev
Points	N/A
Labels	'24Bugsmash, 2025lts, SESAP, customer_bug, jira_escalated

---

PR Type

Enhancement

PR:

• implements per-middleware basemiddleware copy behaviour
• reverts the logger+mutex on base middleware
• touches coprocess/ to better handle grpc server startup, shutdown,
  conflicts on static port number - not to swallow errors when
  net.Listener fails

---

Description

• Refactored the BaseMiddleware initialization process by introducing
  a NewBaseMiddleware function, encapsulating the creation logic.
• Added a Copy method to BaseMiddleware to create scoped copies with
  a duplicated logger, ensuring middleware-specific logging.
• Updated all middleware initialization in gateway/api_loader.go to
  use baseMid.Copy() for better isolation and logging scope.
• Enhanced code readability and maintainability by centralizing
  BaseMiddleware creation logic and ensuring proper separation of
  concerns.

---

Co-authored-by: Tit Petric [email protected]

---

PR Type

Bug fix, Enhancement

---

Description

• Refactored BaseMiddleware initialization by introducing NewBaseMiddleware and Copy methods for better middleware isolation and logging scope.
• Updated middleware initialization in gateway/api_loader.go to use baseMid.Copy() for improved isolation and maintainability.
• Enhanced gRPC test setup by introducing startTestServices and stopTestServices helpers, improving test isolation and reliability.
• Added a dedicated test file for dispatcher logic, improving test coverage and organization.
• Skipped slow and unreliable tests in dnscache/storage_test.go and gateway_test.go.
• Updated test tasks in .taskfiles/test.yml and coprocess/Taskfile.yml to improve test reporting and execution.

---

Changes walkthrough 📝

	Relevant files
Tests	5 files coprocess_grpc_test.go Refactor gRPC test setup and improve test isolation            coprocess/grpc/coprocess_grpc_test.go Refactored test setup to use startTestServices for better test isolation. Removed redundant dispatcher and server setup code. Updated test cases to use new helper functions for cleaner code. +33/-198 dispatcher_test.go Add dedicated dispatcher test file and improve coverage    coprocess/grpc/dispatcher_test.go Added a new test file for dispatcher logic. Moved dispatcher-related test cases to a dedicated file. Improved test coverage for dispatcher functionality. +127/-0  services_test.go Add helpers for gRPC service test setup and teardown          coprocess/grpc/services_test.go Introduced startTestServices and stopTestServices helpers for test setup and teardown. Improved test reliability by dynamically assigning gRPC server ports. Enhanced test maintainability by centralizing service setup logic. +69/-0    storage_test.go Skip slow test with bad practices in storage tests              dnscache/storage_test.go Skipped a slow test with bad practices using time.Sleep. Marked the test for future improvement or removal. +2/-0      testutil.go Improve test utility reliability and error handling            gateway/testutil.go Added error handling for temporary directory creation in test utilities. Improved test reliability by ensuring proper cleanup of temporary resources. +5/-1
Enhancement	4 files api_loader.go Refactor middleware initialization for better isolation    gateway/api_loader.go Refactored BaseMiddleware initialization to use NewBaseMiddleware. Updated middleware initialization to use baseMid.Copy() for better isolation. Improved middleware chain creation by ensuring each middleware gets a unique BaseMiddleware instance. +58/-66  coprocess.go Enhance CoProcess middleware creation with better isolation gateway/coprocess.go Updated CreateCoProcessMiddleware to use BaseMiddleware.Copy() for better isolation. Improved error handling and logging in middleware creation. +3/-3      middleware.go Add `NewBaseMiddleware` and `Copy` for better middleware management gateway/middleware.go Introduced NewBaseMiddleware to encapsulate BaseMiddleware creation logic. Added Copy method to BaseMiddleware for creating scoped copies with unique loggers. Improved thread safety for logger updates in BaseMiddleware. +81/-22  mw_go_plugin.go Enhance Go plugin middleware with isolated BaseMiddleware instances gateway/mw_go_plugin.go Updated Go plugin middleware to use BaseMiddleware.Copy() for better isolation. Improved success handler initialization for plugin middleware. +2/-2
Configuration changes	2 files test.yml Improve test task to generate and consolidate JSON coverage reports .taskfiles/test.yml Updated test task to generate JSON coverage files for each package. Enhanced test reporting by consolidating JSON files into a single report. +5/-4      Taskfile.yml Enhance test task with `gotestsum` and sequential execution coprocess/Taskfile.yml Updated test task to use gotestsum for better test output formatting. Improved test execution by running tests sequentially with -p 1. +4/-1

---

💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

[github-actions]

API Changes

--- prev.txt	2024-12-12 13:20:10.010374331 +0000
+++ current.txt	2024-12-12 13:20:07.100380329 +0000
@@ -7591,18 +7591,27 @@
 type BaseMiddleware struct {
 	Spec  *APISpec
 	Proxy ReturningHttpHandler
+	Gw    *Gateway `json:"-"`
 
-	Gw *Gateway `json:"-"`
 	// Has unexported fields.
 }
     BaseMiddleware wraps up the ApiSpec and Proxy objects to be included in a
     middleware handler, this can probably be handled better.
 
+func NewBaseMiddleware(gw *Gateway, spec *APISpec, proxy ReturningHttpHandler, logger *logrus.Entry) *BaseMiddleware
+    NewBaseMiddleware creates a new *BaseMiddleware. The passed logrus.Entry
+    is duplicated. BaseMiddleware keeps the pointer to *Gateway and *APISpec,
+    as well as Proxy. The logger duplication is used so that basemiddleware
+    copies can be created for different middleware.
+
 func (t *BaseMiddleware) ApplyPolicies(session *user.SessionState) error
     ApplyPolicies will check if any policies are loaded. If any are, it will
     overwrite the session state to use the policy values.
 
-func (t BaseMiddleware) Base() *BaseMiddleware
+func (t *BaseMiddleware) Base() *BaseMiddleware
+    Base serves to provide the full BaseMiddleware API. It's part of
+    the TykMiddleware interface. It escapes to a wider API surface than
+    TykMiddleware, used by middlewares, etc.
 
 func (t *BaseMiddleware) CheckSessionAndIdentityForValidKey(originalKey string, r *http.Request) (user.SessionState, bool)
     CheckSessionAndIdentityForValidKey will check first the Session store for a
@@ -7611,6 +7620,10 @@
 
 func (t *BaseMiddleware) Config() (interface{}, error)
 
+func (m *BaseMiddleware) Copy() *BaseMiddleware
+    Copy provides a new BaseMiddleware with it's own logger scope (copy).
+    The Spec, Proxy and Gw values are not copied.
+
 func (t *BaseMiddleware) EnabledForSpec() bool
 
 func (t *BaseMiddleware) FireEvent(name apidef.TykEvent, meta interface{})
@@ -7620,6 +7633,7 @@
 func (t *BaseMiddleware) Init()
 
 func (t *BaseMiddleware) Logger() (logger *logrus.Entry)
+    Logger is used by middleware process functions.
 
 func (t *BaseMiddleware) OrgSession(orgID string) (user.SessionState, bool)
 
@@ -7629,7 +7643,7 @@
 
 func (t *BaseMiddleware) SetOrgExpiry(orgid string, expiry int64)
 
-func (t *BaseMiddleware) SetRequestLogger(r *http.Request)
+func (t *BaseMiddleware) SetRequestLogger(r *http.Request) *logrus.Entry
 
 func (t *BaseMiddleware) UpdateRequestSession(r *http.Request) bool
 
@@ -10106,11 +10120,9 @@
 }
 
 type TykMiddleware interface {
-	Init()
 	Base() *BaseMiddleware
 
-	SetName(string)
-	SetRequestLogger(*http.Request)
+	Init()
 	Logger() *logrus.Entry
 	Config() (interface{}, error)
 	ProcessRequest(w http.ResponseWriter, r *http.Request, conf interface{}) (error, int) // Handles request

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ✅ 6744 - Fully compliant Fully compliant requirements: Explicitly copy BaseMiddleware for each middleware that uses it. Revert logger and mutex on BaseMiddleware. Improve handling of gRPC server startup, shutdown, and static port conflicts in coprocess/. Avoid swallowing errors when net.Listener fails. Not compliant requirements:

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Code Smell The repeated use of baseMid.Copy() in processSpec could be optimized to reduce redundancy and improve maintainability. Code Smell The NewBaseMiddleware function initializes BaseMiddleware but includes logic for setting CircuitBreakerEnabled and EnforcedTimeoutEnabled directly on the Spec. This could lead to unexpected side effects and should be encapsulated better. Test Skipped The test TestStorageRecordExpiration is skipped due to reliance on time.Sleep, which is a bad practice. Consider refactoring the test to avoid skipping and improve reliability. Test Skipped The test TestOldCachePlugin is skipped due to interference with other tests. Consider isolating the test to avoid skipping.

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Possible issue	Add a check to initialize the logger field if it is nil before accessing it Ensure that the logger field in BaseMiddleware is properly initialized before being accessed in the SetRequestLogger method to avoid potential nil pointer dereferences. gateway/middleware.go [317] +if t.logger == nil { + t.logger = logrus.NewEntry(log) +} t.logger = t.Gw.getLogEntryForRequest(t.logger, r, ctxGetAuthToken(r), nil) Suggestion importance[1-10]: 9 Why: This suggestion addresses a potential nil pointer dereference issue in the SetRequestLogger method, which is critical for preventing runtime errors. The improved code ensures the logger field is properly initialized before being accessed, enhancing the robustness of the code.	9
	Fix potential runtime error by ensuring proper initialization of constants used in the dispatcher Ensure that testHeaderName and testHeaderValue are properly initialized or imported in the dispatcher struct to avoid potential runtime errors. coprocess/grpc/dispatcher_test.go [23-24] object.Request.SetHeaders = map[string]string{ - testHeaderName: testHeaderValue, + "Testheader": "testvalue", } Suggestion importance[1-10]: 8 Why: The suggestion addresses a potential runtime error by ensuring that constants used in the dispatcher are properly initialized. This is a critical fix as it directly impacts the functionality of the dispatcher and avoids unexpected crashes.	8
	Ensure the application halts or handles errors when ioutil.TempDir fails to create a temporary directory Handle the error returned by ioutil.TempDir more robustly to ensure the application does not proceed with an invalid AppPath. gateway/testutil.go [1733-1736] gwConf.AppPath, err = ioutil.TempDir("", "apps") if err != nil { log.WithError(err).Errorf("loadapi: failed to create temp dir") + return nil // or handle the error appropriately } Suggestion importance[1-10]: 8 Why: The suggestion improves error handling by ensuring the application does not proceed with an invalid AppPath when ioutil.TempDir fails. This change enhances the reliability and stability of the application.	8
	Add a nil check for the result of BaseMiddleware.Copy() before using it Verify that m.BaseMiddleware.Copy() does not return nil before assigning it to successHandler to prevent runtime errors. gateway/mw_go_plugin.go [192] -successHandler = &SuccessHandler{BaseMiddleware: m.BaseMiddleware.Copy()} +copiedBaseMiddleware := m.BaseMiddleware.Copy() +if copiedBaseMiddleware != nil { + successHandler = &SuccessHandler{BaseMiddleware: copiedBaseMiddleware} +} Suggestion importance[1-10]: 7 Why: Adding a nil check for BaseMiddleware.Copy() prevents potential runtime errors if the method unexpectedly returns nil. While the likelihood of this issue may be low, the suggestion adds an extra layer of safety.	7
General	Refactor the test to eliminate reliance on time.Sleep for better reliability Avoid skipping the test permanently; instead, refactor the test to eliminate reliance on time.Sleep for better test reliability and performance. dnscache/storage_test.go [154] -t.Skip() // Slow test, bad practices with time.Sleep. +// Refactor test to avoid time.Sleep and improve performance Suggestion importance[1-10]: 6 Why: The suggestion is valid as it proposes refactoring the test to improve reliability and performance. However, it does not provide a concrete solution, which slightly reduces its score.	6
	Add a check to handle missing or empty coverage files in gotestsum commands Ensure the gotestsum commands handle cases where the coverage/*.json files are missing or empty to avoid runtime errors. .taskfiles/test.yml [90] -- gotestsum --hide-summary=skipped --junitfile=unit-tests.xml --raw-command cat coverage/*.json +- if ls coverage/*.json 1> /dev/null 2>&1; then gotestsum --hide-summary=skipped --junitfile=unit-tests.xml --raw-command cat coverage/*.json; else echo "No coverage files found"; fi Suggestion importance[1-10]: 6 Why: This suggestion ensures that the gotestsum command gracefully handles cases where coverage files are missing or empty, preventing runtime errors. While not critical, it improves the robustness of the testing process.	6
	Ensure the Copy method of BaseMiddleware duplicates all fields to prevent side effects Verify that the Copy method of BaseMiddleware correctly duplicates all necessary fields to avoid unintended side effects in middleware processing. gateway/api_loader.go [278] baseMid := NewBaseMiddleware(gw, spec, proxy, logger) +// Ensure Copy method duplicates all fields correctly Suggestion importance[1-10]: 5 Why: While the suggestion is valid and ensures the integrity of the Copy method, it is not actionable as it only asks for verification. This reduces its impact on the PR.	5
	Ensure consistency and completeness in middleware copying for successHandler initialization Validate that the successHandler initialization with BaseMiddleware.Copy() does not introduce inconsistencies or missing fields in the copied middleware. gateway/coprocess.go [311] m.successHandler = &SuccessHandler{m.BaseMiddleware.Copy()} +// Validate copied middleware fields Suggestion importance[1-10]: 5 Why: The suggestion is valid in ensuring that the successHandler initialization does not introduce inconsistencies. However, it is not actionable and only asks for validation, which limits its immediate impact.	5

[sonarqubecloud]

Quality Gate failed

Failed conditions
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE