[TT-13155] Ensure BaseMiddleware is a middleware scoped copy

[titpetric]

This PR reverts #5914 for the most part.

It covers BaseMiddleware decisions/requirements in docs/arch/middleware.md

https://tyktech.atlassian.net/browse/TT-13155

[github-actions]

PR Description updated to latest commit (fe5c542)

[github-actions]

API Changes

--- prev.txt	2024-10-10 19:26:33.517998358 +0000
+++ current.txt	2024-10-10 19:26:27.340986306 +0000
@@ -7835,7 +7835,7 @@
 func CoProcessLog(CMessage, CLogLevel *C.char)
     CoProcessLog is a bridge for using Tyk log from CP.
 
-func CreateCoProcessMiddleware(hookName string, hookType coprocess.HookType, mwDriver apidef.MiddlewareDriver, baseMid *BaseMiddleware) func(http.Handler) http.Handler
+func CreateCoProcessMiddleware(hookName string, hookType coprocess.HookType, mwDriver apidef.MiddlewareDriver, baseMid BaseMiddleware) func(http.Handler) http.Handler
     CreateCoProcessMiddleware initializes a new CP middleware, takes hook type
     (pre, post, etc.), hook name ("my_hook") and driver ("python").
 
@@ -8079,7 +8079,7 @@
     where it is stored in the request (currently only "header" is supported)
 
 type AccessRightsCheck struct {
-	*BaseMiddleware
+	BaseMiddleware
 }
     AccessRightsCheck is a middleware that will check if the key bing used
     to access the API has permission to access the specific version. If no
@@ -8093,7 +8093,7 @@
     system, return an error to have the chain fail
 
 type AuthKey struct {
-	*BaseMiddleware
+	BaseMiddleware
 }
     KeyExists will check if the key being used to access the API is in the
     request data, and then if the key is in the storage engine
@@ -8105,7 +8105,7 @@
 type BaseExtractor struct {
 	Config            *apidef.MiddlewareIdExtractor
 	IDExtractorConfig apidef.IDExtractorConfig
-	*BaseMiddleware
+	BaseMiddleware
 
 	Spec *APISpec
 }
@@ -8200,7 +8200,7 @@
 func (b *BaseTykResponseHandler) Name() string
 
 type BasicAuthKeyIsValid struct {
-	*BaseMiddleware
+	BaseMiddleware
 
 	// Has unexported fields.
 }
@@ -8279,7 +8279,7 @@
     BundleSaver is an interface used by bundle saver structures.
 
 type CertificateCheckMW struct {
-	*BaseMiddleware
+	BaseMiddleware
 }
     CertificateCheckMW is used if domain was not detected or multiple APIs bind
     on the same domain. In this case authentification check happens not on TLS
@@ -8315,7 +8315,7 @@
 }
 
 type CoProcessMiddleware struct {
-	*BaseMiddleware
+	BaseMiddleware
 
 	HookType         coprocess.HookType
 	HookName         string
@@ -8481,7 +8481,7 @@
 func (d *DummyProxyHandler) ServeHTTP(w http.ResponseWriter, r *http.Request)
 
 type DynamicMiddleware struct {
-	*BaseMiddleware
+	BaseMiddleware
 
 	MiddlewareClassName string
 	Pre                 bool
@@ -8608,7 +8608,7 @@
 }
 
 type ExternalOAuthMiddleware struct {
-	*BaseMiddleware
+	BaseMiddleware
 }
 
 func (k *ExternalOAuthMiddleware) EnabledForSpec() bool
@@ -8839,7 +8839,7 @@
 }
 
 type GoPluginMiddleware struct {
-	*BaseMiddleware
+	BaseMiddleware
 
 	Path       string // path to .so file
 	SymbolName string // function symbol to look up
@@ -8865,7 +8865,7 @@
 	GranularAccessFailReasonValidationError
 )
 type GranularAccessMiddleware struct {
-	*BaseMiddleware
+	BaseMiddleware
 }
     GranularAccessMiddleware will check if a URL is specifically enabled for the
     key
@@ -8877,7 +8877,7 @@
     system, return an error to have the chain fail
 
 type GraphQLComplexityMiddleware struct {
-	*BaseMiddleware
+	BaseMiddleware
 }
 
 func (m *GraphQLComplexityMiddleware) EnabledForSpec() bool
@@ -8889,7 +8889,7 @@
     system, return an error to have the chain fail
 
 type GraphQLGranularAccessMiddleware struct {
-	*BaseMiddleware
+	BaseMiddleware
 }
 
 func (m *GraphQLGranularAccessMiddleware) EnabledForSpec() bool
@@ -8901,7 +8901,7 @@
     system, return an error to have the chain fail
 
 type GraphQLMiddleware struct {
-	*BaseMiddleware
+	BaseMiddleware
 }
 
 func (m *GraphQLMiddleware) EnabledForSpec() bool
@@ -8988,7 +8988,7 @@
 func (h *HTTPDashboardHandler) StopBeating()
 
 type HTTPSignatureValidationMiddleware struct {
-	*BaseMiddleware
+	BaseMiddleware
 
 	// Has unexported fields.
 }
@@ -9181,7 +9181,7 @@
 func (h *HostUptimeChecker) Stop()
 
 type IPBlackListMiddleware struct {
-	*BaseMiddleware
+	BaseMiddleware
 }
     IPBlackListMiddleware lets you define a list of IPs to block from upstream
 
@@ -9194,7 +9194,7 @@
     system, return an error to have the chain fail
 
 type IPWhiteListMiddleware struct {
-	*BaseMiddleware
+	BaseMiddleware
 }
     IPWhiteListMiddleware lets you define a list of IPs to allow upstream
 
@@ -9275,7 +9275,7 @@
 }
 
 type JWTMiddleware struct {
-	*BaseMiddleware
+	BaseMiddleware
 }
 
 func (k *JWTMiddleware) EnabledForSpec() bool
@@ -9285,7 +9285,7 @@
 func (k *JWTMiddleware) ProcessRequest(w http.ResponseWriter, r *http.Request, _ interface{}) (error, int)
 
 type KeyExpired struct {
-	*BaseMiddleware
+	BaseMiddleware
 }
     KeyExpired middleware will check if the requesting key is expired or not.
     It makes use of the authManager to do so.
@@ -9395,7 +9395,7 @@
 func (m MethodNotAllowedHandler) ServeHTTP(w http.ResponseWriter, r *http.Request)
 
 type MiddlewareContextVars struct {
-	*BaseMiddleware
+	BaseMiddleware
 }
 
 func (m *MiddlewareContextVars) EnabledForSpec() bool
@@ -9590,7 +9590,7 @@
     OAuthNotificationType const to reduce risk of collisions
 
 type Oauth2KeyExists struct {
-	*BaseMiddleware
+	BaseMiddleware
 }
     Oauth2KeyExists will check if the key being used to access the API is in the
     request data, and then if the key is in the storage engine
@@ -9604,7 +9604,7 @@
     system, return an error to have the chain fail
 
 type OpenIDMW struct {
-	*BaseMiddleware
+	BaseMiddleware
 
 	// Has unexported fields.
 }
@@ -9624,7 +9624,7 @@
 }
 
 type OrganizationMonitor struct {
-	*BaseMiddleware
+	BaseMiddleware
 
 	// Has unexported fields.
 }
@@ -9654,7 +9654,7 @@
 func (k *OrganizationMonitor) SetOrgSentinel(orgChan chan bool, orgId string)
 
 type PersistGraphQLOperationMiddleware struct {
-	*BaseMiddleware
+	BaseMiddleware
 }
     PersistGraphQLOperationMiddleware lets you convert any HTTP request into a
     GraphQL Operation
@@ -9846,7 +9846,7 @@
 func (r *RPCStorageHandler) StartRPCLoopCheck(orgId string)
 
 type RateCheckMW struct {
-	*BaseMiddleware
+	BaseMiddleware
 }
 
 func (m *RateCheckMW) Name() string
@@ -9854,7 +9854,7 @@
 func (m *RateCheckMW) ProcessRequest(w http.ResponseWriter, r *http.Request, _ interface{}) (error, int)
 
 type RateLimitAndQuotaCheck struct {
-	*BaseMiddleware
+	BaseMiddleware
 }
     RateLimitAndQuotaCheck will check the incomming request and key whether
     it is within it's quota and within it's rate limit, it makes use of the
@@ -9869,7 +9869,7 @@
     system, return an error to have the chain fail
 
 type RateLimitForAPI struct {
-	*BaseMiddleware
+	BaseMiddleware
 
 	// Has unexported fields.
 }
@@ -9913,7 +9913,7 @@
     Stop stops the analytics processing
 
 type RedisCacheMiddleware struct {
-	*BaseMiddleware
+	BaseMiddleware
 
 	// Has unexported fields.
 }
@@ -10103,7 +10103,7 @@
     RequestObject is marshalled to JSON string and passed into JSON middleware
 
 type RequestSigning struct {
-	*BaseMiddleware
+	BaseMiddleware
 }
 
 func (s *RequestSigning) EnabledForSpec() bool
@@ -10113,7 +10113,7 @@
 func (s *RequestSigning) ProcessRequest(w http.ResponseWriter, r *http.Request, _ interface{}) (error, int)
 
 type RequestSizeLimitMiddleware struct {
-	*BaseMiddleware
+	BaseMiddleware
 }
     RequestSizeLimitMiddleware is a middleware that will enforce a limit on the
     request body size. The request has already been copied to memory when this
@@ -10453,7 +10453,7 @@
     StreamManager is responsible for creating a single stream
 
 type StreamingMiddleware struct {
-	*BaseMiddleware
+	BaseMiddleware
 
 	// Has unexported fields.
 }
@@ -10483,7 +10483,7 @@
     StreamsConfig represents a stream configuration
 
 type StripAuth struct {
-	*BaseMiddleware
+	BaseMiddleware
 }
 
 func (sa *StripAuth) EnabledForSpec() bool
@@ -10600,7 +10600,7 @@
 func (tr TraceMiddleware) ProcessRequest(w http.ResponseWriter, r *http.Request, conf interface{}) (error, int)
 
 type TrackEndpointMiddleware struct {
-	*BaseMiddleware
+	BaseMiddleware
 }
     TrackEndpointMiddleware sets context variables to enable or disable whether
     Tyk should record analytitcs for a specific path.
@@ -10614,7 +10614,7 @@
     system, return an error to have the chain fail
 
 type TransformHeaders struct {
-	*BaseMiddleware
+	BaseMiddleware
 }
     TransformMiddleware is a middleware that will apply a template to a request
     body to transform it's contents ready for an upstream API
@@ -10628,7 +10628,7 @@
     system, return an error to have the chain fail
 
 type TransformJQMiddleware struct {
-	*BaseMiddleware
+	BaseMiddleware
 }
 
 func (t *TransformJQMiddleware) EnabledForSpec() bool
@@ -10644,7 +10644,7 @@
 }
 
 type TransformMethod struct {
-	*BaseMiddleware
+	BaseMiddleware
 }
     TransformMiddleware is a middleware that will apply a template to a request
     body to transform it's contents ready for an upstream API
@@ -10658,7 +10658,7 @@
     system, return an error to have the chain fail
 
 type TransformMiddleware struct {
-	*BaseMiddleware
+	BaseMiddleware
 }
     TransformMiddleware is a middleware that will apply a template to a request
     body to transform it's contents ready for an upstream API
@@ -10745,7 +10745,7 @@
 func (rt *TykRoundTripper) RoundTrip(r *http.Request) (*http.Response, error)
 
 type URLRewriteMiddleware struct {
-	*BaseMiddleware
+	BaseMiddleware
 }
     URLRewriteMiddleware Will rewrite an inbund URL to a matching outbound one,
     it can also handle dynamic variable substitution
@@ -10833,7 +10833,7 @@
     a proxy request
 
 type UpstreamBasicAuth struct {
-	*BaseMiddleware
+	BaseMiddleware
 }
     UpstreamBasicAuth is a middleware that will do basic authentication for
     upstream connections. UpstreamBasicAuth middleware is only supported in Tyk
@@ -10892,7 +10892,7 @@
 }
 
 type ValidateJSON struct {
-	*BaseMiddleware
+	BaseMiddleware
 }
 
 func (k *ValidateJSON) EnabledForSpec() bool
@@ -10904,7 +10904,7 @@
     system, return an error to have the chain fail
 
 type ValidateRequest struct {
-	*BaseMiddleware
+	BaseMiddleware
 }
 
 func (k *ValidateRequest) EnabledForSpec() bool
@@ -10924,7 +10924,7 @@
 func (e *ValueExtractor) ExtractAndCheck(r *http.Request) (sessionID string, returnOverrides ReturnOverrides)
 
 type VersionCheck struct {
-	*BaseMiddleware
+	BaseMiddleware
 
 	// Has unexported fields.
 }
@@ -10964,7 +10964,7 @@
 func (h *VersionsHandler) ServeHTTP(w http.ResponseWriter, r *http.Request)
 
 type VirtualEndpoint struct {
-	*BaseMiddleware
+	BaseMiddleware
 
 	// Has unexported fields.
 }

[github-actions]

PR Review

⏱️ Estimated effort to review [1-5]	2, because the changes are mostly structural and involve refactoring for better performance and maintainability. The logic seems straightforward, focusing on improving the logging mechanism within middleware.
🧪 Relevant tests	No
🔍 Possible issues	Possible Issue: The removal of mutex might introduce race conditions if the logger is accessed concurrently in a way that was previously protected by the mutex. It's important to ensure that the new request-scoped logger does not introduce any concurrency issues.
🔒 Security concerns	No

Code feedback:

---

relevant file	gateway/middleware.go
suggestion	Consider implementing a mechanism to ensure that GetRequestLogger is thread-safe if concurrent access to the logger's fields can occur. This could prevent potential data races in a highly concurrent environment. [important]
relevant line	func (t *BaseMiddleware) GetRequestLogger(r *http.Request, name string) *logrus.Entry {

---

relevant file	gateway/middleware.go
suggestion	Ensure that Logger() method's dynamic creation of a new logger entry (if t.logger is nil) does not lead to unexpected behavior or excessive resource allocation during high request volumes. Consider initializing the logger explicitly during middleware initialization. [medium]
relevant line	if t.logger == nil {

---

relevant file	gateway/middleware.go
suggestion	Validate the removal of mutex (loggerMu) does not compromise the thread safety of any shared resources now accessed differently. If shared resources are accessed elsewhere, consider implementing alternative synchronization mechanisms. [important]
relevant line	- loggerMu sync.Mutex

---

relevant file	gateway/middleware.go
suggestion	Review the removal of SetName and SetRequestLogger methods to ensure that all use cases for setting or modifying the logger dynamically are still adequately covered by the new GetRequestLogger method. [medium]
relevant line	- SetName(string)

---

---

✨ Review tool usage guide:

---

Overview:
The review tool scans the PR code changes, and generates a PR review. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.
When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:

/review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...

With a configuration file, use the following template:

[pr_reviewer]
some_config1=...
some_config2=...

Utilizing extra instructions The review tool can be configured with extra instructions, which can be used to guide the model to a feedback tailored to the needs of your project. Be specific, clear, and concise in the instructions. With extra instructions, you are the prompter. Specify the relevant sub-tool, and the relevant aspects of the PR that you want to emphasize. Examples for extra instructions: [pr_reviewer] # /review # extra_instructions=""" In the 'possible issues' section, emphasize the following: - Does the code logic cover relevant edge cases? - Is the code logic clear and easy to understand? - Is the code logic efficient? ... """ Use triple quotes to write multi-line instructions. Use bullet points to make the instructions more readable.

How to enable\disable automation When you first install PR-Agent app, the default mode for the review tool is: pr_commands = ["/review", ...] meaning the review tool will run automatically on every PR, with the default configuration. Edit this field to enable/disable the tool, or to change the used configurations

Auto-labels The review tool can auto-generate two specific types of labels for a PR: a possible security issue label, that detects possible security issues (enable_review_labels_security flag) a Review effort [1-5]: x label, where x is the estimated effort to review the PR (enable_review_labels_effort flag)

Extra sub-tools The review tool provides a collection of possible feedbacks about a PR. It is recommended to review the possible options, and choose the ones relevant for your use case. Some of the feature that are disabled by default are quite useful, and should be considered for enabling. For example: require_score_review, require_soc2_ticket, and more.

Auto-approve PRs By invoking: /review auto_approve The tool will automatically approve the PR, and add a comment with the approval. To ensure safety, the auto-approval feature is disabled by default. To enable auto-approval, you need to actively set in a pre-defined configuration file the following: [pr_reviewer] enable_auto_approval = true (this specific flag cannot be set with a command line argument, only in the configuration file, committed to the repository) You can also enable auto-approval only if the PR meets certain requirements, such as that the estimated_review_effort is equal or below a certain threshold, by adjusting the flag: [pr_reviewer] maximal_review_effort = 5

More PR-Agent commands To invoke the PR-Agent, add a comment using one of the following commands: /review: Request a review of your Pull Request. /describe: Update the PR title and description based on the contents of the PR. /improve [--extended]: Suggest code improvements. Extended mode provides a higher quality feedback. /ask <QUESTION>: Ask a question about the PR. /update_changelog: Update the changelog based on the PR's contents. /add_docs 💎: Generate docstring for new components introduced in the PR. /generate_labels 💎: Generate labels for the PR based on the PR's contents. /analyze 💎: Automatically analyzes the PR, and presents changes walkthrough for each component. See the tools guide for more details. To list the possible configuration parameters, add a /config comment.

See the review usage page for a comprehensive guide on using this tool.

[github-actions]

PR Code Suggestions

Category	Suggestions
Enhancement	Initialize the logger in the Init method to ensure it's always available. Consider initializing logger in the Init method of BaseMiddleware to ensure it's always non-nil when accessed. This avoids the need to check if logger is nil in the Logger method. gateway/middleware.go [250-253] +func (t *BaseMiddleware) Init() { + t.logger = logrus.NewEntry(log) +} func (t *BaseMiddleware) Logger() *logrus.Entry { - if t.logger == nil { - return logrus.NewEntry(log) - } -return t.logger + return t.logger }
Performance	Add a log level check before constructing log entries to improve performance. To improve the performance and reduce the overhead of logging, consider adding a check for the log level before constructing log entries with fields. This is particularly useful if the log level is set to a higher level, and the debug logs are not needed. gateway/middleware.go [157] -mwLog.WithField("ts", startTime.UnixNano()).Debug("Started") +if mwLog.Logger.IsLevelEnabled(logrus.DebugLevel) { + mwLog.WithField("ts", startTime.UnixNano()).Debug("Started") +}
Possible issue	Check if mw.Gw is not nil to avoid nil pointer dereferences. For better error handling, consider checking if mw.Gw is not nil before using it in GetRequestLogger. This prevents potential nil pointer dereferences. gateway/middleware.go [260] +if t.Gw == nil { + return logrus.NewEntry(log).WithField("error", "Gateway instance is nil") +} return t.Gw.getLogEntryForRequest(t.Logger().WithField("mw", name), r, ctxGetAuthToken(r), nil)
Best practice	Ensure thread safety by reintroducing mutex for the logger in BaseMiddleware. To ensure thread safety when accessing the logger field in BaseMiddleware, consider reintroducing a mutex lock mechanism or using atomic operations for setting and getting the logger. gateway/middleware.go [241] -logger *logrus.Entry +loggerMu sync.Mutex +logger *logrus.Entry
	Make GetRequestLogger private if not used outside the package. To adhere to the principle of least privilege, consider making GetRequestLogger a private method if it's not intended to be used outside the package. This helps in encapsulating the functionality within the package. gateway/middleware.go [259] -func (t *BaseMiddleware) GetRequestLogger(r *http.Request, name string) *logrus.Entry { +func (t *BaseMiddleware) getRequestLogger(r *http.Request, name string) *logrus.Entry {

---

✨ Improve tool usage guide:

---

Overview:
The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.
When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:

/improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...

With a configuration file, use the following template:

[pr_code_suggestions]
some_config1=...
some_config2=...

Enabling\disabling automation When you first install the app, the default mode for the improve tool is: pr_commands = ["/improve --pr_code_suggestions.summarize=true", ...] meaning the improve tool will run automatically on every PR, with summarization enabled. Delete this line to disable the tool from running automatically.

Utilizing extra instructions Extra instructions are very important for the improve tool, since they enable to guide the model to suggestions that are more relevant to the specific needs of the project. Be specific, clear, and concise in the instructions. With extra instructions, you are the prompter. Specify relevant aspects that you want the model to focus on. Examples for extra instructions: [pr_code_suggestions] # /improve # extra_instructions=""" Emphasize the following aspects: - Does the code logic cover relevant edge cases? - Is the code logic clear and easy to understand? - Is the code logic efficient? ... """ Use triple quotes to write multi-line instructions. Use bullet points to make the instructions more readable.

A note on code suggestions quality While the current AI for code is getting better and better (GPT-4), it's not flawless. Not all the suggestions will be perfect, and a user should not accept all of them automatically. Suggestions are not meant to be simplistic. Instead, they aim to give deep feedback and raise questions, ideas and thoughts to the user, who can then use his judgment, experience, and understanding of the code base. Recommended to use the 'extra_instructions' field to guide the model to suggestions that are more relevant to the specific needs of the project, or use the custom suggestions 💎 tool With large PRs, best quality will be obtained by using 'improve --extended' mode.

More PR-Agent commands To invoke the PR-Agent, add a comment using one of the following commands: /review: Request a review of your Pull Request. /describe: Update the PR title and description based on the contents of the PR. /improve [--extended]: Suggest code improvements. Extended mode provides a higher quality feedback. /ask <QUESTION>: Ask a question about the PR. /update_changelog: Update the changelog based on the PR's contents. /add_docs 💎: Generate docstring for new components introduced in the PR. /generate_labels 💎: Generate labels for the PR based on the PR's contents. /analyze 💎: Automatically analyzes the PR, and presents changes walkthrough for each component. See the tools guide for more details. To list the possible configuration parameters, add a /config comment.

See the improve usage page for a more comprehensive guide on using this tool.

[github-actions]

💥 CI tests failed 🙈

git-state

all ok

Please look at the run or in the Checks tab.

[sonarcloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarCloud

[buger]

API tests result - postgres15-sha256 env: success ✅
Branch used: refs/pull/6094/merge
Commit: 422271d
Triggered by: pull_request (@titpetric)
Execution page

[buger]

API tests result - mongo44-sha256 env: success ✅
Branch used: refs/pull/6094/merge
Commit: 422271d
Triggered by: pull_request (@titpetric)
Execution page

[buger]

With this change we do not modify t.logger, which means that when logger used outside of BaseMiddleware call, e.g. inside middleware like MWRewrite or similar, that logger will not contain MW name in its context.
In other words, this change works only for the log records directly inside BaseMiddleware functions.

[titpetric]

@buger yep, there are a few options:

• attach it to the context (meaning, we'd need to pass a context everywhere if we wanted a logger/entry),
• generate an uuid and key the value to the uuid from a sync.Map or similar (same as context but different value key)
• note: how we log is questionable/fragmented, there are several global logger vars, Logger() interfaces, etc.

I'd have a preference to fix for correctness and safety, meaning pass along the values explicitly or in a way so they can remain request scoped (meaning, context value, idealy on stack). Would you consider a context logger (less explicit)?

Rationale:

• http.Request already has a request lived context and we should be passing context along anyway,
• we already have a ctx package (SetLogger(ctx *context.Context), GetLogger(context.Context) *logrus.Entry)

[buger]

💔 The detected issue is not in one of the allowed statuses 💔

Detected Status	In Refinement	❌
Allowed Statuses	In Dev,In Code Review,Ready for Testing,In Test,In Progress,In Review	✔️

Please ensure your jira story is in one of the allowed statuses

[buger]

💔 The detected issue is not in one of the allowed statuses 💔

Detected Status	In Refinement	❌
Allowed Statuses	In Dev,In Code Review,Ready for Testing,In Test,In Progress,In Review	✔️

Please ensure your jira story is in one of the allowed statuses