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

[TT-11913] Add custom analytics plugins configuration to OAS #6829

Merged

Conversation

buraksezer
Copy link
Contributor

@buraksezer buraksezer commented Jan 15, 2025

User description

TT-11913
Summary [OAS] Custom Analytics plugins
Type Story Story
Status In Dev
Points N/A
Labels -

PR for TT-11913

This PR adds Plugins section to TrafficLogs and related unit tests. UI and integration tests will be implemented separately.

In api_definitions.go file, I added TrafficLogs []MiddlewareDefinition field but In OAS contract, TrafficLogs has also Enabled and TagHeaders fields. Currently, we don't extract those fields to classic API definition. I think we might need to add a new struct for TrafficLogs to api_defitions.go file that covers that fields along with the custom plugins configuration.


PR Type

Enhancement, Tests


Description

  • Added Plugins field to TrafficLogs for custom analytics plugins.

  • Updated TrafficLogs struct in OAS to support plugin configurations.

  • Enhanced unit tests to validate TrafficLogs with custom plugins.

  • Updated OAS schema to include plugins in TrafficLogs.


Changes walkthrough 📝

Relevant files
Enhancement
api_definitions.go
Add `TrafficLogs` field to `MiddlewareSection`                     

apidef/api_definitions.go

  • Added TrafficLogs field to MiddlewareSection.
  • Enabled configuration of custom analytics plugins.
  • +1/-0     
    middleware.go
    Add `Plugins` support to `TrafficLogs` in OAS                       

    apidef/oas/middleware.go

  • Added Plugins field to TrafficLogs struct.
  • Implemented logic to fill and extract Plugins in TrafficLogs.
  • Enhanced TrafficLogs to handle custom plugin configurations.
  • +17/-0   
    x-tyk-api-gateway.json
    Update OAS schema to include `plugins` in `TrafficLogs`   

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

  • Updated schema to include plugins in TrafficLogs.
  • Defined plugins as an array of custom plugin configurations.
  • +6/-0     
    Tests
    middleware_test.go
    Add unit tests for `TrafficLogs` custom plugins                   

    apidef/oas/middleware_test.go

  • Added unit tests for TrafficLogs with custom plugins.
  • Validated Fill and ExtractTo methods for TrafficLogs.
  • Tested custom plugin configurations in TrafficLogs.
  • +30/-0   

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

    @buger
    Copy link
    Member

    buger commented Jan 15, 2025

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

    Copy link
    Contributor

    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 Struct Expansion

    The addition of TrafficLogs as a new field in the MiddlewareSection struct may require validation to ensure it does not introduce unintended side effects or conflicts with existing middleware configurations.

    TrafficLogs []MiddlewareDefinition `bson:"traffic_logs" json:"traffic_logs"`
    Plugin Handling Logic

    The logic for handling Plugins in the TrafficLogs struct, specifically in the Fill and ExtractTo methods, should be reviewed for correctness and edge cases, such as empty or malformed plugin configurations.

    	if len(api.CustomMiddleware.TrafficLogs) == 0 {
    		t.Plugins = nil
    	} else {
    		t.Plugins = make(CustomPlugins, len(api.CustomMiddleware.TrafficLogs))
    		t.Plugins.Fill(api.CustomMiddleware.TrafficLogs)
    	}
    }
    
    // ExtractTo extracts *TrafficLogs into *apidef.APIDefinition.
    func (t *TrafficLogs) ExtractTo(api *apidef.APIDefinition) {
    	api.DoNotTrack = !t.Enabled
    	api.TagHeaders = t.TagHeaders
    
    	if t.RetentionPeriod == nil {
    		t.RetentionPeriod = &RetentionPeriod{}
    		defer func() {
    			t.RetentionPeriod = nil
    		}()
    	}
    	t.RetentionPeriod.ExtractTo(api)
    
    	if len(t.Plugins) == 0 {
    		api.CustomMiddleware.TrafficLogs = nil
    	} else {
    		api.CustomMiddleware.TrafficLogs = make([]apidef.MiddlewareDefinition, len(t.Plugins))
    		t.Plugins.ExtractTo(api.CustomMiddleware.TrafficLogs)
    Schema Update

    The addition of the plugins field in the schema should be validated to ensure it aligns with the intended OAS contract and does not break compatibility with existing configurations.

    "plugins": {
      "type": "array",
      "items": {
        "$ref": "#/definitions/X-Tyk-CustomPluginConfig"
      }

    Copy link
    Contributor

    API Changes

    --- prev.txt	2025-01-15 08:43:41.338705507 +0000
    +++ current.txt	2025-01-15 08:43:36.299641615 +0000
    @@ -889,6 +889,7 @@
     	PostKeyAuth []MiddlewareDefinition `bson:"post_key_auth" json:"post_key_auth"`
     	AuthCheck   MiddlewareDefinition   `bson:"auth_check" json:"auth_check"`
     	Response    []MiddlewareDefinition `bson:"response" json:"response"`
    +	TrafficLogs []MiddlewareDefinition `bson:"traffic_logs" json:"traffic_logs"`
     	Driver      MiddlewareDriver       `bson:"driver" json:"driver"`
     	IdExtractor MiddlewareIdExtractor  `bson:"id_extractor" json:"id_extractor"`
     }
    @@ -4013,6 +4014,9 @@
     	// RetentionPeriod holds the configuration for the analytics retention, it contains configuration
     	// for how long you would like analytics data to last for.
     	RetentionPeriod *RetentionPeriod `bson:"retentionPeriod" json:"retentionPeriod,omitempty"`
    +	// Plugins configures custom plugins to allow for extensive modifications to analytics records
    +	// The plugins would be executed in the order of configuration in the list.
    +	Plugins CustomPlugins `bson:"plugins,omitempty" json:"plugins,omitempty"`
     }
         TrafficLogs holds configuration about API log analytics.
     

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Add validation rules for required fields in the plugins schema to ensure proper plugin configurations

    Ensure that the schema for plugins includes validation rules for required fields
    such as FunctionName and Path to prevent invalid plugin configurations.

    apidef/oas/schema/x-tyk-api-gateway.json [2017-2021]

     "plugins": {
       "type": "array",
       "items": {
    -    "$ref": "#/definitions/X-Tyk-CustomPluginConfig"
    +    "type": "object",
    +    "properties": {
    +      "FunctionName": {
    +        "type": "string",
    +        "minLength": 1
    +      },
    +      "Path": {
    +        "type": "string",
    +        "minLength": 1
    +      }
    +    },
    +    "required": ["FunctionName", "Path"]
       }
     }
    Suggestion importance[1-10]: 9

    Why: Adding validation rules for required fields in the plugins schema ensures that invalid plugin configurations are caught early. This is a critical improvement to the schema's integrity and prevents potential misconfigurations.

    9
    Add test cases for handling invalid or malformed TrafficLogs.Plugins data in Fill and ExtractTo methods

    Include test cases for scenarios where TrafficLogs.Plugins contains invalid or
    malformed data to ensure robust error handling in both Fill and ExtractTo methods.

    apidef/oas/middleware_test.go [271-292]

    -t.Run("with custom analytics plugin", func(t *testing.T) {
    +t.Run("with invalid custom analytics plugin", func(t *testing.T) {
         t.Parallel()
    -    expectedTrafficLogsPlugin := TrafficLogs{
    +    invalidTrafficLogsPlugin := TrafficLogs{
             Enabled:    true,
             TagHeaders: []string{},
             Plugins: CustomPlugins{
                 {
    -                Enabled:      true,
    -                FunctionName: "CustomAnalyticsPlugin",
    -                Path:         "/path/to/plugin",
    +                Enabled:      false,
    +                FunctionName: "",
    +                Path:         "",
                 },
             },
         }
    +    api := apidef.APIDefinition{}
    +    invalidTrafficLogsPlugin.ExtractTo(&api)
    +    actualTrafficLogsPlugin := TrafficLogs{}
    +    actualTrafficLogsPlugin.Fill(api)
    +    assert.NotEqual(t, invalidTrafficLogsPlugin, actualTrafficLogsPlugin)
    +})
    Suggestion importance[1-10]: 7

    Why: Including test cases for invalid or malformed TrafficLogs.Plugins data ensures that the Fill and ExtractTo methods handle such scenarios correctly. This improves the code's test coverage and reliability, though it is less critical than runtime error handling.

    7
    Possible issue
    Add error handling for invalid or unexpected data in api.CustomMiddleware.TrafficLogs during the Fill method execution

    Ensure that the Fill method for TrafficLogs properly handles cases where
    api.CustomMiddleware.TrafficLogs contains invalid or unexpected data to avoid
    potential runtime errors.

    apidef/oas/middleware.go [1605-1610]

     if len(api.CustomMiddleware.TrafficLogs) == 0 {
         t.Plugins = nil
     } else {
         t.Plugins = make(CustomPlugins, len(api.CustomMiddleware.TrafficLogs))
    -    t.Plugins.Fill(api.CustomMiddleware.TrafficLogs)
    +    if err := t.Plugins.Fill(api.CustomMiddleware.TrafficLogs); err != nil {
    +        // Handle error appropriately, e.g., log or return
    +        t.Plugins = nil
    +    }
     }
    Suggestion importance[1-10]: 8

    Why: The suggestion introduces error handling for potential invalid data in api.CustomMiddleware.TrafficLogs, which improves robustness and prevents runtime errors. This is a significant enhancement to the method's reliability.

    8
    Validate t.Plugins data before populating api.CustomMiddleware.TrafficLogs in the ExtractTo method

    Add validation to the ExtractTo method for TrafficLogs to ensure that t.Plugins
    contains valid data before attempting to populate api.CustomMiddleware.TrafficLogs.

    apidef/oas/middleware.go [1626-1630]

     if len(t.Plugins) == 0 {
         api.CustomMiddleware.TrafficLogs = nil
     } else {
         api.CustomMiddleware.TrafficLogs = make([]apidef.MiddlewareDefinition, len(t.Plugins))
    -    t.Plugins.ExtractTo(api.CustomMiddleware.TrafficLogs)
    +    if err := t.Plugins.ExtractTo(api.CustomMiddleware.TrafficLogs); err != nil {
    +        // Handle error appropriately, e.g., log or return
    +        api.CustomMiddleware.TrafficLogs = nil
    +    }
     }
    Suggestion importance[1-10]: 8

    Why: Adding validation to ensure t.Plugins contains valid data before populating api.CustomMiddleware.TrafficLogs enhances the method's robustness and prevents potential issues with invalid data. This is a meaningful improvement to the code's reliability.

    8

    Copy link

    Quality Gate Failed Quality Gate failed

    Failed conditions
    1 Security Hotspot

    See analysis details on SonarQube Cloud

    @buraksezer buraksezer merged commit fcad52f into master Jan 15, 2025
    30 of 41 checks passed
    @buraksezer buraksezer deleted the feat/TT-11913/Custom-Analytics-plugins-without-conflicts branch January 15, 2025 11:07
    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.

    4 participants