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-11910]: added tag headers to traffic logs and tests #6818

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

Conversation

kofoworola
Copy link
Contributor

@kofoworola kofoworola commented Jan 6, 2025

User description

Description

Implemented TagHeaders in Tyk OAS API Definition

Related Issue

TT-11910

Motivation and Context

How This Has Been Tested

Screenshots (if appropriate)

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)

Checklist

  • I ensured that the documentation is up to date
  • 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

PR Type

Enhancement, Tests


Description

  • Added TagHeaders field to TrafficLogs for analytic tags.

  • Updated TrafficLogs methods to handle TagHeaders.

  • Introduced new tests for TrafficLogs functionality.

  • Extended API schema to include TagHeaders in X-Tyk-TrafficLogs.


Changes walkthrough 📝

Relevant files
Enhancement
middleware.go
Add `TagHeaders` field and update methods                               

apidef/oas/middleware.go

  • Added TagHeaders field to TrafficLogs struct.
  • Updated Fill and ExtractTo methods to handle TagHeaders.
  • +5/-0     
    x-tyk-api-gateway.json
    Extend schema to include `tagHeaders`                                       

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

  • Added tagHeaders property to X-Tyk-TrafficLogs schema.
  • Defined tagHeaders as an array of strings.
  • +6/-0     
    Tests
    middleware_test.go
    Add tests for `TrafficLogs` with `TagHeaders`                       

    apidef/oas/middleware_test.go

  • Added tests for TrafficLogs with TagHeaders.
  • Covered scenarios with and without TagHeaders.
  • +49/-0   
    oas_test.go
    Remove redundant `TagHeaders` test case                                   

    apidef/oas/oas_test.go

    • Removed redundant test case for TagHeaders.
    +0/-1     

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

    @kofoworola kofoworola force-pushed the feat/TT-11910/analyticstags branch from 9967353 to 230f56e Compare January 6, 2025 10:25
    @kofoworola kofoworola marked this pull request as ready for review January 6, 2025 10:25
    @kofoworola kofoworola requested a review from a team January 6, 2025 10:26
    Copy link
    Contributor

    github-actions bot commented Jan 6, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Possible Issue

    Ensure that the TagHeaders field is correctly handled when it is empty or contains unexpected values, as this could lead to unexpected behavior in analytics aggregation.

    	// TagHeaders is a string array of HTTP headers that can be extracted
    	// and transformed into analytic tags(statistics aggregated by tag, per hour)
    	TagHeaders []string `bson:"tagHeaders" json:"tagHeaders,omitempty"`
    }
    
    // Fill fills *TrafficLogs from apidef.APIDefinition.
    func (t *TrafficLogs) Fill(api apidef.APIDefinition) {
    	t.Enabled = !api.DoNotTrack
    	t.TagHeaders = api.TagHeaders
    }
    
    // ExtractTo extracts *TrafficLogs into *apidef.APIDefinition.
    func (t *TrafficLogs) ExtractTo(api *apidef.APIDefinition) {
    	api.DoNotTrack = !t.Enabled
    	api.TagHeaders = t.TagHeaders
    Test Coverage

    Validate that the added test cases for TrafficLogs cover all edge cases, including scenarios with invalid or unexpected TagHeaders values.

    func TestTrafficLogs(t *testing.T) {
    	t.Parallel()
    	t.Run("empty", func(t *testing.T) {
    		t.Parallel()
    
    		var emptyTrafficLogs TrafficLogs
    
    		var convertedAPI apidef.APIDefinition
    		convertedAPI.SetDisabledFlags()
    		emptyTrafficLogs.ExtractTo(&convertedAPI)
    
    		var resultTrafficLogs TrafficLogs
    		resultTrafficLogs.Fill(convertedAPI)
    
    		assert.Equal(t, emptyTrafficLogs, resultTrafficLogs)
    	})
    
    	t.Run("enabled with tag header", func(t *testing.T) {
    		t.Parallel()
    		trafficLogs := TrafficLogs{
    			Enabled:    true,
    			TagHeaders: []string{"X-Team-Name"},
    		}
    
    		var convertedAPI apidef.APIDefinition
    		convertedAPI.SetDisabledFlags()
    		trafficLogs.ExtractTo(&convertedAPI)
    		assert.Equal(t, trafficLogs.TagHeaders, convertedAPI.TagHeaders)
    		assert.False(t, convertedAPI.DoNotTrack)
    
    		var resultTrafficLogs TrafficLogs
    		resultTrafficLogs.Fill(convertedAPI)
    		assert.Equal(t, trafficLogs.TagHeaders, resultTrafficLogs.TagHeaders)
    		assert.Equal(t, trafficLogs.Enabled, resultTrafficLogs.Enabled)
    	})
    
    	t.Run("enabled with no tag header", func(t *testing.T) {
    		t.Parallel()
    		trafficLogs := TrafficLogs{
    			Enabled:    true,
    			TagHeaders: []string{},
    		}
    		var convertedAPI apidef.APIDefinition
    		convertedAPI.SetDisabledFlags()
    		trafficLogs.ExtractTo(&convertedAPI)
    		assert.Empty(t, convertedAPI.TagHeaders)
    	})
    }

    Copy link
    Contributor

    github-actions bot commented Jan 6, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Security
    Add a pattern constraint to the schema for tagHeaders to validate header names

    Update the schema to include constraints or patterns for tagHeaders to ensure only
    valid header names are allowed.

    apidef/oas/schema/x-tyk-api-gateway.json [1989-1993]

     "tagHeaders": {
       "type": "array",
       "items": {
    -    "type": "string"
    +    "type": "string",
    +    "pattern": "^[a-zA-Z0-9-]+$"
       }
     }
    Suggestion importance[1-10]: 9

    Why: Adding a pattern constraint to the schema ensures that only valid header names are allowed, which strengthens input validation at the schema level. This is a high-impact improvement for security and data integrity.

    9
    Add validation for TagHeaders to ensure input integrity and security

    Ensure that TagHeaders is properly validated for invalid or malicious input to
    prevent potential security issues or unexpected behavior.

    apidef/oas/middleware.go [1558]

    -TagHeaders []string `bson:"tagHeaders" json:"tagHeaders,omitempty"`
    +TagHeaders []string `bson:"tagHeaders" json:"tagHeaders,omitempty"` // Add validation logic to sanitize and validate input
    Suggestion importance[1-10]: 8

    Why: Adding validation for TagHeaders is crucial to ensure that invalid or malicious input does not lead to security vulnerabilities or unexpected behavior. This suggestion addresses a potential security concern effectively.

    8
    General
    Add a test case to handle invalid or unexpected TagHeaders values

    Add a test case to verify the behavior of TrafficLogs when TagHeaders contains
    invalid or unexpected values.

    apidef/oas/middleware_test.go [195-199]

    -t.Run("enabled with tag header", func(t *testing.T) {
    +t.Run("enabled with invalid tag header", func(t *testing.T) {
         t.Parallel()
         trafficLogs := TrafficLogs{
             Enabled:    true,
    -        TagHeaders: []string{"X-Team-Name"},
    +        TagHeaders: []string{"Invalid-Header-Name"},
         }
    Suggestion importance[1-10]: 7

    Why: Including a test case for invalid TagHeaders values improves the robustness of the code by ensuring that edge cases are handled correctly. This enhances the reliability of the implementation.

    7

    apidef/oas/middleware.go Outdated Show resolved Hide resolved
    @@ -175,6 +175,55 @@ func TestGlobal(t *testing.T) {
    })
    }

    func TestTrafficLogs(t *testing.T) {
    t.Parallel()
    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    t.Parallel is not necessary. We limit parallelization due to StartTest details, also it's only meant for slow tests. Data model conversions don't strike me as slow.

    Please add some whitespace before assertions, consider readability; move var declarations to start of test functions to not intermix with logic that's being tested.

    @@ -266,7 +266,6 @@ func TestOAS_ExtractTo_ResetAPIDefinition(t *testing.T) {
    "APIDefinition.ExpireAnalyticsAfter",
    "APIDefinition.ResponseProcessors[0].Name",
    "APIDefinition.ResponseProcessors[0].Options",
    "APIDefinition.TagHeaders[0]",
    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    Was looking for this explicitly, 🙌 👍

    apidef/oas/middleware.go Outdated Show resolved Hide resolved
    Copy link
    Contributor

    github-actions bot commented Jan 6, 2025

    API Changes

    --- prev.txt	2025-01-07 13:51:15.332042747 +0000
    +++ current.txt	2025-01-07 13:51:10.853017834 +0000
    @@ -3929,6 +3929,9 @@
     	// 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.
     

    Copy link
    Contributor

    @titpetric titpetric left a comment

    Choose a reason for hiding this comment

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

    I've applied the godoc change as the CI didn't let it pass.
    The remaining feedback is cosmetic, please take care if it before merging.

    LGTM.

    @kofoworola kofoworola force-pushed the feat/TT-11910/analyticstags branch from c91180c to 9128a1c Compare January 7, 2025 09:00
    @kofoworola kofoworola enabled auto-merge (squash) January 7, 2025 13:51
    Copy link

    sonarqubecloud bot commented Jan 7, 2025

    Quality Gate Failed Quality Gate failed

    Failed conditions
    1 Security Hotspot

    See analysis details on SonarQube Cloud

    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