Merging to release-5.8: [Debugger MVP] Some middleware doesn't work for OAS debugger (#7093) #7102
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
<details open> <summary><a href="https://tyktech.atlassian.net/browse/TT-14156" title="TT-14156" target="_blank">TT-14156</a></summary> <br /> <table> <tr> <th>Summary</th> <td>[Debugger MVP] Some middleware doesn't work for OAS debugger</td> </tr> <tr> <th>Type</th> <td> <img alt="Bug" src="https://tyktech.atlassian.net/rest/api/2/universal_avatar/view/type/issuetype/avatar/10303?size=medium" /> Bug </td> </tr> <tr> <th>Status</th> <td>In Code Review</td> </tr> <tr> <th>Points</th> <td>N/A</td> </tr> <tr> <th>Labels</th> <td><a href="https://tyktech.atlassian.net/issues?jql=project%20%3D%20TT%20AND%20labels%20%3D%20codilime_refined%20ORDER%20BY%20created%20DESC" title="codilime_refined">codilime_refined</a></td> </tr> </table> </details> <!-- do not remove this marker as it will break jira-lint's functionality. added_by_jira_lint --> --- <!-- Provide a general summary of your changes in the Title above --> <!-- Describe your changes in detail --> <!-- This project only accepts pull requests related to open issues. --> <!-- If suggesting a new feature or change, please discuss it in an issue first. --> <!-- If fixing a bug, there should be an issue describing it with steps to reproduce. --> <!-- OSS: Please link to the issue here. Tyk: please create/link the JIRA ticket. --> <!-- Why is this change required? What problem does it solve? --> <!-- Please describe in detail how you tested your changes --> <!-- Include details of your testing environment, and the tests --> <!-- you ran to see how your change affects other areas of the code, etc. --> <!-- This information is helpful for reviewers and QA. --> <!-- What types of changes does your code introduce? Put an `x` in all the boxes that apply: --> - [x] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [x] Refactoring or add test (improvements in base code or adds test coverage to functionality) <!-- Go over all the following points, and put an `x` in all the boxes that apply --> <!-- If there are no documentation updates required, mark the item as checked. --> <!-- Raise up any additional concerns not covered by the checklist. --> - [ ] I ensured that the documentation is up to date - [x] I explained why this PR updates go.mod in detail with reasoning why it's required - [ ] I would like a code coverage CI quality gate exception and have explained why 1. **Mock Middleware Bug Fix** The main issue with the Mock middleware was caused by an incorrect implementation of the (*traceRequest).toRequest method in gateway/tracing.go. This method was joining the API entity's ListenPath with the request path, which created incorrect URLs for mocked endpoints. This explains why it was working incorrectly before this changes. ```go // Previous problematic code in traceRequest.toRequest path, err := url.JoinPath( tr.Spec.Proxy.ListenPath, tr.Request.Path, ) ``` 2. **Rate Limit Middleware Bug Fix** The RateLimitForAPI middleware was using the LastUpdated field from the user.SessionState object to generate a unique key for rate limiting. When testing with the /trace/debug endpoint, this approach caused the rate limiter to use an inconsistent key, resulting in incorrect rate limiting behavior. Fix involves manually providing a stable key to the rate limiter factory by using ApiSpec.Checksum through the WithQuotaKey function: ```go chainObj := gw.processSpec( spec, nil, &gs, logrus.NewEntry(logger), WithQuotaKey(spec.Checksum), // Using Checksum as a stable key ) ``` This ensures consistent rate limiting behavior even for trace/debug endpoints. 3. Test Improvements Written three test cases with multiple assertions: - Tests for endpoints with global rate limiters - Tests for endpoints with per-endpoint rate limiters - Tests for the mock middleware functionality Additionally, I've created a builder/factory pattern to simplify the creation of valid OAS (OpenAPI Specification) objects for testing, which will make future test development easier and more maintainable. These changes collectively improve the reliability of the Mock middleware and Rate Limit middleware, especially when used with the trace/debug functionality. (cherry picked from commit d9954f9)
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
|
API Changes --- prev.txt 2025-06-06 13:39:25.702284296 +0000
+++ current.txt 2025-06-06 13:39:20.386325371 +0000
@@ -1980,6 +1980,11 @@
CONSTANTS
const (
+
+ // UpstreamUrlDefault default upstream url for OAS
+ UpstreamUrlDefault = "http://localhost:3478/"
+)
+const (
WebhookKind = event.WebhookKind
JSVMKind = event.JSVMKind
LogKind = event.LogKind
@@ -2016,6 +2021,10 @@
VARIABLES
var (
+ ErrMinRateLimitExceeded = errors.New("minimum rate limit exceeded")
+ ErrZeroAmountInRateLimit = errors.New("zero amount in rate limit")
+)
+var (
// URLRewriteConditions contains all valid URL rewrite condition values.
URLRewriteConditions = []URLRewriteCondition{
ConditionAll,
@@ -2276,6 +2285,43 @@
Fill updates the BatchProcessing configuration based on the
EnableBatchRequestSupport value from the given APIDefinition.
+type Builder struct {
+ // Has unexported fields.
+}
+ Builder OAS builder is responsible for providing methods for building valid
+ OAS object.
+
+func NewBuilder() Builder
+
+func (b *Builder) Build() (*OAS, error)
+
+type BuilderOption = option.Option[Builder]
+ BuilderOption optional parameter should be used to define builder options in
+ declarative way.
+
+func WithDelete(path string, fn EndpointFactory) BuilderOption
+ WithDelete add delete endpoint/path to OAS
+
+func WithGet(path string, fn EndpointFactory) BuilderOption
+ WithGet add get endpoint/path to OAS
+
+func WithGlobalRateLimit(rate uint, duration time.Duration, enabled ...bool) BuilderOption
+ WithGlobalRateLimit defines global rate limit
+
+func WithListenPath(path string, strip bool) BuilderOption
+
+func WithPost(path string, fn EndpointFactory) BuilderOption
+ WithPost add post endpoint/path to OAS
+
+func WithPut(path string, fn EndpointFactory) BuilderOption
+ WithPut add put endpoint/path to OAS
+
+func WithTestDefaults() BuilderOption
+ WithTestDefaults sets defaults options to be sued for testing
+
+func WithUpstreamUrl(upstreamUrl string) BuilderOption
+ WithUpstreamUrl defines upstream url
+
type CORS struct {
// Enabled is a boolean flag, if set to `true`, this option enables CORS processing.
//
@@ -2708,6 +2754,19 @@
DomainToCertificate holds a single mapping of domain name into a
certificate.
+type EndpointBuilder struct {
+ // Has unexported fields.
+}
+ EndpointBuilder OAS endpoint builder should be used for building endpoint
+ and binding middlewares to it.
+
+func (eb *EndpointBuilder) Mock(fn func(mock *MockResponse)) *EndpointBuilder
+
+func (eb *EndpointBuilder) RateLimit(amount uint, duration time.Duration, enabled ...bool) *EndpointBuilder
+
+type EndpointFactory func(b *EndpointBuilder)
+ EndpointFactory factory method in which endpoint builder should be used.
+
type EndpointPostPlugin struct {
// Enabled activates post plugin.
//
@@ -3547,11 +3606,11 @@
func FillOASFromClassicAPIDefinition(api *apidef.APIDefinition, oas *OAS) (*OAS, error)
-func NewOAS() *OAS
- NewOAS returns an allocated *OAS.
-
func NewOASFromClassicAPIDefinition(api *apidef.APIDefinition) (*OAS, error)
+func NewOas(opts ...BuilderOption) (*OAS, error)
+ NewOas returns an allocated *OAS due to provided options
+
func (s *OAS) AddServers(apiURLs ...string)
AddServers adds a server into the servers definition if not already present.
@@ -5460,6 +5519,23 @@
func AddTo(app *kingpin.Application)
AddTo initializes a version info object.
+# Package: ./common/option
+
+package option // import "github.com/TykTechnologies/tyk/common/option"
+
+
+TYPES
+
+type FailableOption[O any] func(*O) error
+
+type Option[O any] func(*O)
+
+type Options[O any] []Option[O]
+
+func New[O any](opts []Option[O]) Options[O]
+
+func (o Options[O]) Build(baseVal O) *O
+
# Package: ./config
package config // import "github.com/TykTechnologies/tyk/config"
@@ -8649,6 +8725,9 @@
events.
func UpdateAPIVersion(spec *APISpec, name string, verGen func(version *apidef.VersionInfo))
+func WithQuotaKey(key string) option.Option[ProcessSpecOptions]
+ WithQuotaKey overrides quota key manually
+
func WrappedCharsetReader(s string, i io.Reader) (io.Reader, error)
TYPES
@@ -8735,9 +8814,7 @@
GraphEngine graphengine.Engine
- HasMock bool
- HasValidateRequest bool
- OASRouter routers.Router
+ OASRouter routers.Router
// Has unexported fields.
}
APISpec represents a path specification for an API, to avoid enumerating
@@ -8780,6 +8857,8 @@
func (a *APISpec) SanitizeProxyPaths(r *http.Request)
+func (a *APISpec) SetupOperation(r *http.Request)
+
func (a *APISpec) StopSessionManagerPool()
func (a *APISpec) StripListenPath(reqPath string) string
@@ -8891,7 +8970,7 @@
func (t *BaseMiddleware) Config() (interface{}, error)
-func (m *BaseMiddleware) Copy() *BaseMiddleware
+func (t *BaseMiddleware) Copy() *BaseMiddleware
Copy provides a new BaseMiddleware with it's own logger scope (copy).
The Spec, Proxy and Gw values are not copied.
@@ -10451,6 +10530,11 @@
ApplyPolicies []string `json:"apply_policies"`
}
+type ProcessSpecOptions struct {
+ // Has unexported fields.
+}
+ ProcessSpecOptions represents options for processSpec method
+
type ProxyResponse struct {
Response *http.Response
// UpstreamLatency the time it takes to do roundtrip to upstream. Total time
@@ -10652,15 +10736,15 @@
// Has unexported fields.
}
- RateLimitAndQuotaCheck will check the incoming request and key whether
- it is within it's quota and within it's rate limit, it makes use of the
- SessionLimiter object to do this
+ RateLimitForAPI will check the incoming request and key whether it is within
+ it's quota and within it's rate limit, it makes use of the SessionLimiter
+ object to do this
func (k *RateLimitForAPI) EnabledForSpec() bool
func (k *RateLimitForAPI) Name() string
-func (k *RateLimitForAPI) ProcessRequest(w http.ResponseWriter, r *http.Request, _ interface{}) (error, int)
+func (k *RateLimitForAPI) ProcessRequest(_ http.ResponseWriter, r *http.Request, _ interface{}) (error, int)
ProcessRequest will run any checks on the request on the way through the
system, return an error to have the chain fail
@@ -11164,7 +11248,16 @@
func (l *SessionLimiter) Context() context.Context
-func (l *SessionLimiter) ForwardMessage(r *http.Request, session *user.SessionState, rateLimitKey string, quotaKey string, store storage.Handler, enableRL, enableQ bool, api *APISpec, dryRun bool) sessionFailReason
+func (l *SessionLimiter) ForwardMessage(
+ r *http.Request,
+ session *user.SessionState,
+ rateLimitKey string,
+ quotaKey string,
+ store storage.Handler,
+ enableRL, enableQ bool,
+ api *APISpec,
+ dryRun bool,
+) sessionFailReason
ForwardMessage will enforce rate limiting, returning a non-zero
sessionFailReason if session limits have been exceeded. Key values to manage
rate are Rate and Per, e.g. Rate of 10 messages Per 10 seconds
@@ -11618,7 +11711,7 @@
func (k *ValidateJSON) Name() string
-func (k *ValidateJSON) ProcessRequest(w http.ResponseWriter, r *http.Request, _ interface{}) (error, int)
+func (k *ValidateJSON) ProcessRequest(_ http.ResponseWriter, r *http.Request, _ interface{}) (error, int)
ProcessRequest will run any checks on the request on the way through the
system, return an error to have the chain fail
@@ -11805,21 +11898,23 @@
TextXML = "text/xml"
)
const (
- XRealIP = "X-Real-IP"
- XForwardFor = "X-Forwarded-For"
- XAuthResult = "X-Auth-Result"
- XSessionAlias = "X-Session-Alias"
- XInitialURI = "X-Initial-URI"
- XForwardProto = "X-Forwarded-Proto"
- XContentTypeOptions = "X-Content-Type-Options"
- XXSSProtection = "X-XSS-Protection"
- XFrameOptions = "X-Frame-Options"
- XTykNodeID = "x-tyk-nodeid"
- XTykSessionID = "x-tyk-session-id"
- XTykNonce = "x-tyk-nonce"
- XTykHostname = "x-tyk-hostname"
- XGenerator = "X-Generator"
- XTykAuthorization = "X-Tyk-Authorization"
+ XRealIP = "X-Real-IP"
+ XForwardFor = "X-Forwarded-For"
+ XAuthResult = "X-Auth-Result"
+ XSessionAlias = "X-Session-Alias"
+ XInitialURI = "X-Initial-URI"
+ XForwardProto = "X-Forwarded-Proto"
+ XContentTypeOptions = "X-Content-Type-Options"
+ XXSSProtection = "X-XSS-Protection"
+ XFrameOptions = "X-Frame-Options"
+ XTykNodeID = "x-tyk-nodeid"
+ XTykSessionID = "x-tyk-session-id"
+ XTykNonce = "x-tyk-nonce"
+ XTykHostname = "x-tyk-hostname"
+ XGenerator = "X-Generator"
+ XTykAuthorization = "X-Tyk-Authorization"
+ XTykAcceptExampleName = "X-Tyk-Accept-Example-Name"
+ XTykAcceptExampleCode = "X-Tyk-Accept-Example-Code"
)
const (
Upgrade = "Upgrade" |
|
shults
deleted the
merge/release-5.8/d9954f921bdaa346fc3ebd276c048dd09a090deb
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
User description
[Debugger MVP] Some middleware doesn't work for OAS debugger (#7093)
TT-14156
Description
Related Issue
Motivation and Context
How This Has Been Tested
Screenshots (if appropriate)
Types of changes
functionality to change)
coverage to functionality)
Checklist
why it's required
explained why
Description of provided changes
The main issue with the Mock middleware was caused by an incorrect
implementation of the (*traceRequest).toRequest method in
gateway/tracing.go. This method was joining the API entity's ListenPath
with the request path, which created incorrect URLs for mocked
endpoints. This explains why it was working incorrectly before this
changes.
The RateLimitForAPI middleware was using the LastUpdated field from the
user.SessionState object to generate a unique key for rate limiting.
When testing with the /trace/debug endpoint, this approach caused the
rate limiter to use an inconsistent key, resulting in incorrect rate
limiting behavior.
Fix involves manually providing a stable key to the rate limiter factory
by using ApiSpec.Checksum through the WithQuotaKey function:
This ensures consistent rate limiting behavior even for trace/debug
endpoints.
Written three test cases with multiple assertions:
Additionally, I've created a builder/factory pattern to simplify the
creation of valid OAS (OpenAPI Specification) objects for testing, which
will make future test development easier and more maintainable.
These changes collectively improve the reliability of the Mock
middleware and Rate Limit middleware, especially when used with the
trace/debug functionality.
PR Type
Bug fix, Enhancement, Tests
Description
Fix Mock middleware path joining in OAS debugger
Ensure stable rate limit key for OAS trace/debug
Add comprehensive tests for mock and rate limit middleware
Refactor OAS builder and context operation setup for testability
Changes walkthrough 📝
6 files
Fix trace request path handling and quota key usageUse stable quota key for rate limiting in OAS traceUse hasMock() instead of HasMock fieldUse header constants for mock example selectionRemove unused customQuotaKey in org monitorMinor fixes for GetTykMiddleware and validation10 files
Add ProcessSpecOptions and WithQuotaKey for rate limiterAdd OAS builder for testable OAS object creationAdd findOperations and SetupOperation for OAS contextRefactor hasMock and hasVirtualEndpoint logicRemove HasValidateRequest field and improve checksUse SetupOperation for OAS operation contextRefactor ForwardMessage signature for quota keyRemove unused writer param in ProcessRequestAdd generic Option/Options utility for buildersAdd constants for OAS mock example headers4 files
Add tests for OAS mock and rate limit in debuggerUpdate tests to use header constants for mockMinor test formatting improvementsUpdate hasMock tests to new logic1 files
Add generated mock reflection program2 files
Update/add dependencies for testing and OAS builderUpdate/add checksums for new dependencies1 files