Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement api-level request size limit for oas (TT-11459) #6822

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

pvormste
Copy link
Contributor

@pvormste pvormste commented Jan 9, 2025

User description

TT-11459
Summary [OAS] Implement API-level request size limit
Type Story Story
Status In Dev
Points N/A
Labels -

This PR adds support for API-level request size limit for OAS.

Types of changes

  • 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)
  • Refactoring or add test (improvements in base code or adds test coverage to functionality)

PR Type

Enhancement, Tests, Documentation


Description

  • Implemented API-level request size limit for OAS.

  • Added GlobalRequestSizeLimit struct and related methods.

  • Updated OAS schema and documentation for request size limit.

  • Added comprehensive tests for request size limit functionality.


Changes walkthrough 📝

Relevant files
Enhancement
api_definitions.go
Add support for request size limit in API definitions       

apidef/api_definitions.go

  • Added GlobalSizeLimitDisabled field to VersionInfo.
  • Enhanced API definition to support request size limit configuration.
  • +1/-0     
    middleware.go
    Add request size limit handling in middleware                       

    apidef/oas/middleware.go

  • Introduced RequestSizeLimit field in Global struct.
  • Added methods to handle request size limit configuration.
  • Refactored version handling with helper functions.
  • +88/-7   
    mw_request_size_limit.go
    Update middleware to respect request size limit                   

    gateway/mw_request_size_limit.go

    • Updated EnabledForSpec to check GlobalSizeLimitDisabled.
    +1/-1     
    x-tyk-api-gateway.json
    Update OAS schema for request size limit                                 

    apidef/oas/schema/x-tyk-api-gateway.json

  • Added X-Tyk-GlobalRequestSizeLimit definition.
  • Updated schema to include requestSizeLimit field.
  • +18/-0   
    Tests
    middleware_test.go
    Add tests for request size limit functionality                     

    apidef/oas/middleware_test.go

  • Added tests for GlobalRequestSizeLimit functionality.
  • Covered both Fill and ExtractTo methods.
  • +144/-0 
    oas_test.go
    Update OAS tests for request size limit                                   

    apidef/oas/oas_test.go

    • Updated tests to include GlobalSizeLimitDisabled field.
    +1/-1     
    mw_request_size_limit_test.go
    Add middleware tests for request size limit                           

    gateway/mw_request_size_limit_test.go

    • Added tests for EnabledForSpec with various configurations.
    +47/-0   
    Documentation
    apidef-oas.md
    Update documentation for request size limit                           

    docs/dev/apidef-oas.md

  • Documented VersionData handling for request size limit.
  • Added examples for Fill and ExtractTo methods.
  • +19/-0   

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

    @buger
    Copy link
    Member

    buger commented Jan 9, 2025

    I'm a bot and I 👍 this PR title. 🤖

    Copy link
    Contributor

    github-actions bot commented Jan 9, 2025

    API Changes

    --- prev.txt	2025-01-09 12:07:54.268971184 +0000
    +++ current.txt	2025-01-09 12:07:49.515965936 +0000
    @@ -1456,6 +1456,7 @@
     	GlobalResponseHeadersDisabled bool              `bson:"global_response_headers_disabled" json:"global_response_headers_disabled"`
     	IgnoreEndpointCase            bool              `bson:"ignore_endpoint_case" json:"ignore_endpoint_case"`
     	GlobalSizeLimit               int64             `bson:"global_size_limit" json:"global_size_limit"`
    +	GlobalSizeLimitDisabled       bool              `bson:"global_size_limit_disabled" json:"global_size_limit_disabled"`
     	OverrideTarget                string            `bson:"override_target" json:"override_target"`
     }
     
    @@ -2728,6 +2729,9 @@
     
     	// TrafficLogs contains the configurations related to API level log analytics.
     	TrafficLogs *TrafficLogs `bson:"trafficLogs,omitempty" json:"trafficLogs,omitempty"`
    +
    +	// RequestSizeLimit contains the configuration related to limiting the global request size.
    +	RequestSizeLimit *GlobalRequestSizeLimit `bson:"requestSizeLimit,omitempty" json:"requestSizeLimit,omitempty"`
     }
         Global contains configuration that affects the whole API (all endpoints).
     
    @@ -2744,6 +2748,23 @@
         ensures backwards compatibility and proper handling of the deprecated fields
         during the migration process.
     
    +type GlobalRequestSizeLimit struct {
    +	// Enabled activates the Request Size Limit.
    +	// Tyk classic API definition: `version_data.versions..global_size_limit_disabled`.
    +	Enabled bool `bson:"enabled" json:"enabled"`
    +	// Value contains the value of the request size limit.
    +	// Tyk classic API definition: `version_data.versions..global_size_limit`.
    +	Value int64 `bson:"value" json:"value"`
    +}
    +    GlobalRequestSizeLimit holds configuration about the global limits for
    +    request sizes.
    +
    +func (g *GlobalRequestSizeLimit) ExtractTo(api *apidef.APIDefinition)
    +    ExtractTo extracts *GlobalRequestSizeLimit into *apidef.APIDefinition.
    +
    +func (g *GlobalRequestSizeLimit) Fill(api apidef.APIDefinition)
    +    Fill fills *GlobalRequestSizeLimit from apidef.APIDefinition.
    +
     type HMAC struct {
     	// Enabled activates the HMAC authentication mode.
     	// Tyk classic API definition: `enable_signature_checking`
    @@ -3929,9 +3950,6 @@
     	// Enabled enables traffic log analytics for the API.
     	// Tyk classic API definition: `do_not_track`.
     	Enabled bool `bson:"enabled" json:"enabled"`
    -	// TagHeaders is a string array of HTTP headers that can be extracted
    -	// and transformed into analytics tags (statistics aggregated by tag, per hour).
    -	TagHeaders []string `bson:"tagHeaders" json:"tagHeaders,omitempty"`
     }
         TrafficLogs holds configuration about API log analytics.
     
    @@ -12495,8 +12513,6 @@
     
     package regression // import "github.com/TykTechnologies/tyk/tests/regression"
     
    -# Package: ./tests/system
    -
     # Package: ./trace
     
     package trace // import "github.com/TykTechnologies/tyk/trace"

    Copy link
    Contributor

    github-actions bot commented Jan 9, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Possible Issue

    The fillRequestSizeLimit function in the Global struct assumes that the RequestSizeLimit field is always initialized correctly. Ensure that edge cases where api.VersionData.Versions is nil or improperly structured are handled gracefully.

    func (g *Global) fillRequestSizeLimit(api apidef.APIDefinition) {
    	if g.RequestSizeLimit == nil {
    		g.RequestSizeLimit = &GlobalRequestSizeLimit{}
    	}
    
    	g.RequestSizeLimit.Fill(api)
    	if ShouldOmit(g.RequestSizeLimit) {
    		g.RequestSizeLimit = nil
    	}
    }
    Logic Validation

    The EnabledForSpec function now includes a condition for GlobalSizeLimitDisabled. Ensure that this logic correctly handles all scenarios, especially when GlobalSizeLimit is set to 0 or disabled.

    func (t *RequestSizeLimitMiddleware) EnabledForSpec() bool {
    	for _, version := range t.Spec.VersionData.Versions {
    		if len(version.ExtendedPaths.SizeLimit) > 0 ||
    			(!version.GlobalSizeLimitDisabled && version.GlobalSizeLimit > 0) {
    			return true
    		}
    	}
    	return false

    Copy link
    Contributor

    github-actions bot commented Jan 9, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Add test cases for nil VersionData.Versions in TestGlobalRequestSizeLimit

    Include test cases for scenarios where api.VersionData.Versions is nil in the
    TestGlobalRequestSizeLimit test suite to ensure robustness.

    apidef/oas/middleware_test.go [1044]

     testcases := []struct {
         title    string
         input    apidef.APIDefinition
         expected *GlobalRequestSizeLimit
     }{
         {
             title:    "no versions",
             input:    apidef.APIDefinition{},
             expected: nil,
         },
    +    {
    +        title:    "nil versions map",
    +        input:    apidef.APIDefinition{VersionData: apidef.VersionData{Versions: nil}},
    +        expected: nil,
    +    },
         ...
     }
    Suggestion importance[1-10]: 7

    Why: Including a test case for nil VersionData.Versions enhances the robustness of the test suite by covering an edge case that could occur in real-world scenarios.

    7
    Ensure GlobalSizeLimit is validated as non-negative in EnabledForSpec

    Validate that version.GlobalSizeLimit is non-negative in EnabledForSpec to prevent
    potential logical errors or misuse.

    gateway/mw_request_size_limit.go [42]

    -(!version.GlobalSizeLimitDisabled && version.GlobalSizeLimit > 0)
    +(!version.GlobalSizeLimitDisabled && version.GlobalSizeLimit >= 0)
    Suggestion importance[1-10]: 2

    Why: The suggestion to validate GlobalSizeLimit as non-negative is redundant since the condition already ensures it is greater than 0. This adds minimal value to the existing logic.

    2
    Possible issue
    Add a nil check for api.VersionData.Versions before calling fillRequestSizeLimit

    Ensure that the fillRequestSizeLimit function properly handles cases where
    api.VersionData.Versions is nil to avoid potential nil pointer dereferences.

    apidef/oas/middleware.go [234]

    -g.fillRequestSizeLimit(api)
    +if api.VersionData.Versions != nil {
    +    g.fillRequestSizeLimit(api)
    +}
    Suggestion importance[1-10]: 3

    Why: Adding a nil check for api.VersionData.Versions could prevent potential nil pointer dereferences, but the PR already includes helper functions like requireMainVersion to handle such cases. This makes the suggestion less impactful.

    3

    Copy link

    sonarqubecloud bot commented Jan 9, 2025

    Quality Gate Failed Quality Gate failed

    Failed conditions
    1 Security Hotspot

    See analysis details on SonarQube Cloud

    Copy link
    Contributor

    @jeffy-mathew jeffy-mathew left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    lgtm

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    3 participants