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

EE and Streaming licensing #6653

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

EE and Streaming licensing #6653

wants to merge 35 commits into from

Conversation

buger
Copy link
Member

@buger buger commented Oct 18, 2024

User description

Description

Add licensing mechanism for the EE gateway, and require "streams" claim for the Streams specifically

Similar to Dashboard, license_key, and TYK_GW_LICENSEKEY should be set.

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 a new licensing mechanism for the EE gateway, including a LicenseKey field in the configuration.
  • Implemented a new package for handling and validating licenses, with support for test mode.
  • Integrated license feature checks in the streaming middleware to ensure the "streams" feature is enabled.
  • Added comprehensive tests for the new license handling functionality, covering various scenarios.
  • Integrated license validation in the server startup process, logging errors if validation fails.

Changes walkthrough 📝

Relevant files
Enhancement
config.go
Add LicenseKey field to Config struct                                       

config/config.go

  • Added LicenseKey field to Config struct.
+1/-0     
license.go
Implement license handling and validation package               

ee/license/license.go

  • Introduced a new package for handling licenses.
  • Implemented functions for loading and validating licenses.
  • Added support for test mode in license handling.
  • +191/-0 
    middleware.go
    Integrate license feature check in streaming middleware   

    ee/middleware/streams/middleware.go

  • Integrated license feature check for streaming.
  • Added logging for license validation in streaming middleware.
  • +6/-0     
    license_check.go
    Add placeholder license check for non-EE builds                   

    gateway/license_check.go

  • Added a placeholder function for license checking in non-EE builds.
  • +9/-0     
    license_check_ee.go
    Implement license validation for EE builds                             

    gateway/license_check_ee.go

  • Implemented license validation for EE builds.
  • Logged errors if license validation fails.
  • +18/-0   
    server.go
    Integrate license check in server startup                               

    gateway/server.go

  • Integrated license check during server startup.
  • Logged fatal error if license check fails.
  • +4/-0     
    Tests
    license_test.go
    Add tests for license handling and validation                       

    ee/license/license_test.go

  • Added tests for license handling and validation.
  • Covered scenarios for test mode, feature checks, and invalid licenses.

  • +266/-0 
    mw_streaming_test.go
    Add license feature setup in streaming tests                         

    gateway/mw_streaming_test.go

  • Added setup and cleanup for streaming tests with license features.
  • Ensured streaming tests check for license features.
  • +39/-0   

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

    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

    Error Handling
    The error messages in the licensing module could be more descriptive to aid in debugging and user guidance. Consider adding more context or suggestions for resolution.

    Feature Check
    The check for the 'streams' feature in the license should be accompanied by a more user-friendly error or warning message, potentially guiding the user on how to enable this feature if it's not present.

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Ensure the expiration timestamp is valid to prevent runtime errors

    Add a check to ensure that the exp claim is a valid timestamp before converting it
    to int64 to prevent potential runtime panics.

    ee/license/license.go [129-135]

    -if exp, ok := claims["exp"].(float64); ok {
    +if exp, ok := claims["exp"].(float64); ok && exp > 0 {
       if int64(exp) < time.Now().Unix() {
         return errors.New("license has expired")
       }
     } else {
       return errors.New("exp claim is missing or invalid")
     }
    Suggestion importance[1-10]: 8

    Why: This suggestion adds a check to ensure the exp claim is a valid timestamp before conversion, which helps prevent potential runtime panics. It addresses a possible bug and improves the robustness of the code.

    8
    Enhancement
    Improve memory management and behavior consistency when enabling test mode

    Consider handling the case where lic is not nil when enabling test mode to avoid
    potential memory leaks or unexpected behavior.

    ee/license/license.go [43-50]

     testMode = true
    -if lic == nil {
    +if lic != nil {
    +  lic.Scopes = make(map[string]bool)
    +} else {
       lic = &License{
         Scopes: make(map[string]bool),
       }
     }
    Suggestion importance[1-10]: 7

    Why: The suggestion addresses a potential issue where the lic variable might not be nil when enabling test mode, which could lead to unexpected behavior. By resetting the scopes if lic is not nil, the suggestion enhances memory management and ensures consistent behavior.

    7
    Maintainability
    Refactor the Load function to improve code organization and maintainability

    Refactor the Load function to separate concerns, improving readability and
    maintainability by breaking down the function into smaller, more focused
    sub-functions.

    ee/license/license.go [82-162]

     func Load(licenseString string) error {
       ...
       if testMode {
    -    ...
    +    return handleTestMode(licenseString)
       }
    +  return handleProductionMode(licenseString)
    +}
    +func handleTestMode(licenseString string) error {
       ...
    -  if PublicKeyPEM == "" {
    -    ...
    -  }
    -  ...
    -  if err != nil {
    -    ...
    -  }
    +}
    +func handleProductionMode(licenseString string) error {
       ...
     }
    Suggestion importance[1-10]: 6

    Why: The suggestion proposes refactoring the Load function to separate concerns, which can enhance readability and maintainability. While it doesn't address a critical issue, it provides a moderate improvement in code organization.

    6
    Best practice
    Add a reset function to ensure clean state management

    Implement a cleanup function to reset the lic and testMode variables to their
    default states to ensure clean state management between tests or mode switches.

    ee/license/license.go [54-59]

    -func DisableTestMode() {
    +func ResetLicenseState() {
       licMutex.Lock()
       defer licMutex.Unlock()
       testMode = false
       lic = nil
     }
    +func DisableTestMode() {
    +  ResetLicenseState()
    +}
    Suggestion importance[1-10]: 5

    Why: Implementing a cleanup function for resetting lic and testMode variables can improve state management between tests or mode switches. This suggestion follows best practices for clean state management, though it is not critical.

    5

    Base automatically changed from refactor/streams to master October 19, 2024 14:08
    @buger buger requested review from a team as code owners October 19, 2024 14:08
    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.

    1 participant