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

Abstracts storage constructors back to interface, as prep for redis alternatives #6451

Closed
wants to merge 4 commits into from

Conversation

lonelycode
Copy link
Member

@lonelycode lonelycode commented Aug 7, 2024

User description

This PR abstracts the storage handler implementations, which were all literals like x :=. RedisCluster{blah:blah} to instead take the Handler interface, and added a nifty constructor that allows for more dynamic configuration.

Description

  • Refactored the storage package into sub-modules for each store type (redisCluster, MDCB, DUMMY)
  • Moved utility functions into their own package for storage
  • Move the storage interface out into a separate interfaces package (not ideal, but it gets around circular imports)
  • Created a generic constructor storage.NewStorageHandler(type, opts...) to consolidate instantiation
  • Refactored all literal instantiations to use the constructor
  • Added a dynamic storage "type" getter which can be hooked into configuration options at a alter stage for new back-ends

Related Issue

Remove dependency on Redis

Motivation and Context

In order to eventually drop Redis, or be able to replace it with a different beack end that is more lightweight, the code base needs to use the existing Handler interface to interact with the back end, unfortunately over time, concrete implementations have crept in and made it harder to make the back-end pluggable. This PR aims to address that to some degree.

How This Has Been Tested

lol, the code builds ;-)

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

  • Refactored storage handler initialization across multiple files to use a new dynamic constructor NewStorageHandler.
  • Updated imports to use new storage packages, including redisCluster, mdcb, shared, and util.
  • Moved utility functions for hashing and token generation to a new util package.
  • Added options for configuring Redis storage handlers.
  • Updated tests to reflect changes in storage handler initialization and utility functions.

Changes walkthrough 📝

Relevant files
Enhancement
10 files
server.go
Refactor storage handler initialization in server.go         

gateway/server.go

  • Replaced direct RedisCluster instantiations with NewStorageHandler
    calls.
  • Updated imports to use new storage packages.
  • Refactored storage handler initialization to use dynamic constructor.
  • +100/-21
    rpc_storage_handler.go
    Update storage handler and utility usage in rpc_storage_handler.go

    gateway/rpc_storage_handler.go

  • Updated storage handler to use new util package for hashing.
  • Replaced direct RedisCluster references with shared package errors.
  • Moved OAuth client event processing functions.
  • +102/-100
    api.go
    Refactor storage handler usage and imports in api.go         

    gateway/api.go

  • Updated imports to use new storage packages.
  • Replaced direct RedisCluster references with NewStorageHandler calls.
  • Updated hashing functions to use util package.
  • +44/-11 
    storage.go
    Refactor storage handler interface and add constructor     

    storage/storage.go

  • Removed old storage handler interface and related functions.
  • Added new storage handler constructor and module getter.
  • +26/-158
    mdcb_storage.go
    Update MdcbStorage to use new storage handler interface   

    storage/mdcb/mdcb_storage.go

  • Updated MdcbStorage struct to use new interfaces.Handler.
  • Refactored methods to use new storage handler interface.
  • +41/-40 
    oauth_manager.go
    Refactor storage handler usage and imports in oauth_manager.go

    gateway/oauth_manager.go

  • Updated imports to use new storage packages.
  • Replaced direct RedisCluster references with NewStorageHandler calls.
  • Updated hashing functions to use util package.
  • +30/-12 
    util.go
    Add utility functions for hashing and token generation     

    storage/util/util.go

  • Added utility functions for hashing and token generation.
  • Moved from storage.go to a new util package.
  • +127/-0 
    redis_cluster.go
    Update package name and references in redis_cluster.go     

    storage/redis-cluster/redis_cluster.go

  • Changed package name to redisCluster.
  • Updated error references to use shared package.
  • Updated hashing functions to use util package.
  • +11/-6   
    session_manager.go
    Update session manager to use new storage handler interface

    gateway/session_manager.go

  • Updated imports to use new storage packages.
  • Updated session limiter to use new storage handler interface.
  • +5/-4     
    redis_options.go
    Add options for configuring Redis storage handlers             

    storage/redis_options.go

    • Added options for configuring Redis storage handlers.
    +61/-0   
    Tests
    5 files
    redis_cluster_test.go
    Update package name and error references in redis_cluster_test.go

    storage/redis-cluster/redis_cluster_test.go

  • Changed package name to redisCluster.
  • Updated error references to use shared package.
  • +25/-24 
    coprocess_id_extractor_test.go
    Update imports and storage references in
    coprocess_id_extractor_test.go

    gateway/coprocess_id_extractor_test.go

  • Updated imports to use new storage packages.
  • Replaced direct RedisCluster references with redisCluster.
  • Updated hashing functions to use util package.
  • +13/-12 
    api_test.go
    Update imports and storage references in api_test.go         

    gateway/api_test.go

  • Updated imports to use new storage packages.
  • Replaced direct RedisCluster references with redisCluster.
  • Updated hashing functions to use util package.
  • +15/-13 
    host_checker_manager_test.go
    Update imports and storage references in host_checker_manager_test.go

    gateway/host_checker_manager_test.go

  • Updated imports to use new storage packages.
  • Replaced direct RedisCluster references with redisCluster.
  • +7/-7     
    sliding_log_test.go
    Update imports and storage references in sliding_log_test.go

    internal/rate/sliding_log_test.go

  • Updated imports to use new storage packages.
  • Replaced direct RedisCluster references with redisCluster.
  • +8/-8     
    Additional files (token-limit)
    38 files
    health_check.go
    ...                                                                                                           

    gateway/health_check.go

    ...

    +11/-3   
    mw_api_rate_limit.go
    ...                                                                                                           

    gateway/mw_api_rate_limit.go

    ...

    +4/-4     
    oauth_manager_test.go
    ...                                                                                                           

    gateway/oauth_manager_test.go

    ...

    +4/-3     
    mw_basic_auth.go
    ...                                                                                                           

    gateway/mw_basic_auth.go

    ...

    +3/-3     
    event_handler_webhooks.go
    ...                                                                                                           

    gateway/event_handler_webhooks.go

    ...

    +14/-2   
    host_checker_manager.go
    ...                                                                                                           

    gateway/host_checker_manager.go

    ...

    +4/-4     
    redis_signals.go
    ...                                                                                                           

    gateway/redis_signals.go

    ...

    +5/-4     
    cert_test.go
    ...                                                                                                           

    gateway/cert_test.go

    ...

    +2/-2     
    redis_logrus_hook.go
    ...                                                                                                           

    gateway/redis_logrus_hook.go

    ...

    +20/-3   
    mw_external_oauth.go
    ...                                                                                                           

    gateway/mw_external_oauth.go

    ...

    +13/-2   
    coprocess_api.go
    ...                                                                                                           

    gateway/coprocess_api.go

    ...

    +14/-3   
    api_definition.go
    ...                                                                                                           

    gateway/api_definition.go

    ...

    +2/-2     
    mdcb_options.go
    ...                                                                                                           

    storage/mdcb_options.go

    ...

    +35/-0   
    mw_auth_key_test.go
    ...                                                                                                           

    gateway/mw_auth_key_test.go

    ...

    +2/-2     
    mw_jwt.go
    ...                                                                                                           

    gateway/mw_jwt.go

    ...

    +11/-1   
    ctx.go
    ...                                                                                                           

    ctx/ctx.go

    ...

    +2/-2     
    api_healthcheck.go
    ...                                                                                                           

    gateway/api_healthcheck.go

    ...

    +4/-4     
    analytics.go
    ...                                                                                                           

    gateway/analytics.go

    ...

    +2/-1     
    mw_auth_key.go
    ...                                                                                                           

    gateway/mw_auth_key.go

    ...

    +2/-2     
    mw_basic_auth_test.go
    ...                                                                                                           

    gateway/mw_basic_auth_test.go

    ...

    +2/-2     
    mdcb_storage_test.go
    ...                                                                                                           

    storage/mdcb/mdcb_storage_test.go

    ...

    +4/-3     
    mw_redis_cache.go
    ...                                                                                                           

    gateway/mw_redis_cache.go

    ...

    +2/-2     
    synchronization_forcer_test.go
    ...                                                                                                           

    rpc/synchronization_forcer_test.go

    ...

    +3/-3     
    sliding_log.go
    ...                                                                                                           

    internal/rate/sliding_log.go

    ...

    +1/-1     
    rpc_analytics_purger.go
    ...                                                                                                           

    rpc/rpc_analytics_purger.go

    ...

    +2/-3     
    delete_api_cache.go
    ...                                                                                                           

    gateway/delete_api_cache.go

    ...

    +11/-1   
    storage_test.go
    ...                                                                                                           

    storage/storage_test.go

    ...

    +6/-2     
    redis_analytics_purger.go
    ...                                                                                                           

    gateway/redis_analytics_purger.go

    ...

    +2/-2     
    res_cache.go
    ...                                                                                                           

    gateway/res_cache.go

    ...

    +2/-2     
    manager_test.go
    ...                                                                                                           

    certs/manager_test.go

    ...

    +1/-1     
    ldap_auth_handler.go
    ...                                                                                                           

    gateway/ldap_auth_handler.go

    ...

    +1/-1     
    errors.go
    ...                                                                                                           

    storage/shared/errors.go

    ...

    +7/-0     
    redis_shim_test.go
    ...                                                                                                           

    storage/redis-cluster/redis_shim_test.go

    ...

    +1/-1     
    redis_shim.go
    ...                                                                                                           

    storage/redis-cluster/redis_shim.go

    ...

    +1/-1     
    connection_handler_test.go
    ...                                                                                                           

    storage/redis-cluster/connection_handler_test.go

    ...

    +1/-1     
    dummy_test.go
    ...                                                                                                           

    storage/dummy/dummy_test.go

    ...

    +1/-1     
    connection_handler.go
    ...                                                                                                           

    storage/redis-cluster/connection_handler.go

    ...

    +1/-1     
    dummy.go
    ...                                                                                                           

    storage/dummy/dummy.go

    ...

    +1/-1     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link
    Contributor

    github-actions bot commented Aug 7, 2024

    API Changes

    --- prev.txt	2024-08-07 02:48:49.101048655 +0000
    +++ current.txt	2024-08-07 02:48:45.549018543 +0000
    @@ -4853,8 +4853,8 @@
     func GetCertIDAndChainPEM(certData []byte, secret string) (string, []byte, error)
     func ParsePEM(data []byte, secret string) ([]*pem.Block, error)
     func ParsePEMCertificate(data []byte, secret string) (*tls.Certificate, error)
    -func NewCertificateManager(storage storage.Handler, secret string, logger *logrus.Logger, migrateCertList bool) *certificateManager
    -func NewSlaveCertManager(localStorage, rpcStorage storage.Handler, secret string, logger *logrus.Logger, migrateCertList bool) *certificateManager
    +func NewCertificateManager(storage interfaces.Handler, secret string, logger *logrus.Logger, migrateCertList bool) *certificateManager
    +func NewSlaveCertManager(localStorage, rpcStorage interfaces.Handler, secret string, logger *logrus.Logger, migrateCertList bool) *certificateManager
     
     TYPES
     
    @@ -7861,7 +7861,7 @@
         takes the precedence. If the global one is `true`, value of the one in api
         level doesn't matter.
     
    -func (a *APISpec) Init(authStore, sessionStore, healthStore, orgStore storage.Handler)
    +func (a *APISpec) Init(authStore, sessionStore, healthStore, orgStore interfaces.Handler)
     
     func (s *APISpec) Release()
         Release releases all resources associated with API spec
    @@ -8223,7 +8223,7 @@
     
     func (h *DefaultHealthChecker) CreateKeyName(subKey HealthPrefix) string
     
    -func (h *DefaultHealthChecker) Init(storeType storage.Handler)
    +func (h *DefaultHealthChecker) Init(storeType interfaces.Handler)
     
     func (h *DefaultHealthChecker) StoreCounterVal(counterType HealthPrefix, value string)
     
    @@ -8244,7 +8244,7 @@
     	// Has unexported fields.
     }
     
    -func (b *DefaultSessionManager) Init(store storage.Handler)
    +func (b *DefaultSessionManager) Init(store interfaces.Handler)
     
     func (b *DefaultSessionManager) KeyExpired(newSession *user.SessionState) bool
         KeyExpired checks if a key has expired, if the value of
    @@ -8267,7 +8267,7 @@
     
     func (b *DefaultSessionManager) Stop()
     
    -func (b *DefaultSessionManager) Store() storage.Handler
    +func (b *DefaultSessionManager) Store() interfaces.Handler
     
     func (b *DefaultSessionManager) UpdateSession(keyName string, session *user.SessionState,
     	resetTTLTo int64, hashed bool) error
    @@ -8501,7 +8501,7 @@
     	TestBundleMu sync.Mutex
     
     	// RedisController keeps track of redis connection and singleton
    -	StorageConnectionHandler *storage.ConnectionHandler
    +	StorageConnectionHandler *redisCluster.ConnectionHandler
     
     	// Has unexported fields.
     }
    @@ -8544,7 +8544,7 @@
     
     func (gw *Gateway) GetStorageForApi(apiID string) (ExtendedOsinStorageInterface, int, error)
     
    -func (gw *Gateway) InitHostCheckManager(ctx context.Context, store storage.Handler)
    +func (gw *Gateway) InitHostCheckManager(ctx context.Context, store interfaces.Handler)
     
     func (gw *Gateway) InitializeRPCCache()
     
    @@ -8849,7 +8849,7 @@
     }
     
     type HealthChecker interface {
    -	Init(storage.Handler)
    +	Init(interfaces.Handler)
     	ApiHealthValues() (HealthCheckValues, error)
     	StoreCounterVal(HealthPrefix, string)
     }
    @@ -8899,7 +8899,7 @@
     
     func (hc *HostCheckerManager) HostDown(urlStr string) bool
     
    -func (hc *HostCheckerManager) Init(store storage.Handler)
    +func (hc *HostCheckerManager) Init(store interfaces.Handler)
     
     func (hc *HostCheckerManager) ListFromService(apiID string) ([]HostData, error)
     
    @@ -9094,7 +9094,7 @@
     	SearchString         string
     	// Has unexported fields.
     }
    -    LDAPStorageHandler implements storage.Handler, this is a read-only
    +    LDAPStorageHandler implements interfaces.Handler, this is a read-only
         implementation to access keys from an LDAP service
     
     func (l LDAPStorageHandler) AddToSet(keyName, value string)
    @@ -9797,7 +9797,7 @@
     func (r *RedisOsinStorageInterface) SetUser(username string, session *user.SessionState, timeout int64) error
     
     type RedisPurger struct {
    -	Store storage.Handler
    +	Store interfaces.Handler
     	Gw    *Gateway `json:"-"`
     }
     
    @@ -10145,8 +10145,8 @@
     func (s *ServiceDiscovery) Target(serviceURL string) (*apidef.HostList, error)
     
     type SessionHandler interface {
    -	Init(store storage.Handler)
    -	Store() storage.Handler
    +	Init(store interfaces.Handler)
    +	Store() interfaces.Handler
     	UpdateSession(keyName string, session *user.SessionState, resetTTLTo int64, hashed bool) error
     	RemoveSession(orgID string, keyName string, hashed bool) bool
     	SessionDetail(orgID string, keyName string, hashed bool) (user.SessionState, bool)
    @@ -10175,12 +10175,12 @@
     
     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 interfaces.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
     
    -func (l *SessionLimiter) RedisQuotaExceeded(r *http.Request, session *user.SessionState, quotaKey, scope string, limit *user.APILimit, store storage.Handler, hashKeys bool) bool
    +func (l *SessionLimiter) RedisQuotaExceeded(r *http.Request, session *user.SessionState, quotaKey, scope string, limit *user.APILimit, store interfaces.Handler, hashKeys bool) bool
     
     type SlaveDataCenter struct {
     	SlaveOptions config.SlaveOptionsConfig
    @@ -10835,6 +10835,50 @@
     )
         Gateway's custom response headers
     
    +# Package: ./interfaces
    +
    +package interfaces // import "github.com/TykTechnologies/tyk/interfaces"
    +
    +
    +TYPES
    +
    +type Handler interface {
    +	GetKey(string) (string, error) // Returned string is expected to be a JSON object (user.SessionState)
    +	GetMultiKey([]string) ([]string, error)
    +	GetRawKey(string) (string, error)
    +	SetKey(string, string, int64) error // Second input string is expected to be a JSON object (user.SessionState)
    +	SetRawKey(string, string, int64) error
    +	SetExp(string, int64) error   // Set key expiration
    +	GetExp(string) (int64, error) // Returns expiry of a key
    +	GetKeys(string) []string
    +	DeleteKey(string) bool
    +	DeleteAllKeys() bool
    +	DeleteRawKey(string) bool
    +	Connect() bool
    +	GetKeysAndValues() map[string]string
    +	GetKeysAndValuesWithFilter(string) map[string]string
    +	DeleteKeys([]string) bool
    +	Decrement(string)
    +	IncrememntWithExpire(string, int64) int64
    +	SetRollingWindow(key string, per int64, val string, pipeline bool) (int, []interface{})
    +	GetRollingWindow(key string, per int64, pipeline bool) (int, []interface{})
    +	GetSet(string) (map[string]string, error)
    +	AddToSet(string, string)
    +	GetAndDeleteSet(string) []interface{}
    +	RemoveFromSet(string, string)
    +	DeleteScanMatch(string) bool
    +	GetKeyPrefix() string
    +	AddToSortedSet(string, string, float64)
    +	GetSortedSetRange(string, string, string) ([]string, []float64, error)
    +	RemoveSortedSetRange(string, string, string) error
    +	GetListRange(string, int64, int64) ([]string, error)
    +	RemoveFromList(string, string) error
    +	AppendToSet(string, string)
    +	Exists(string) (bool, error)
    +}
    +    Handler is a standard interface to a storage backend, used by
    +    AuthorisationManager to read and write key values to the backend
    +
     # Package: ./log
     
     package log // import "github.com/TykTechnologies/tyk/log"
    @@ -11143,7 +11187,7 @@
     }
     
     type Purger struct {
    -	Store storage.Handler
    +	Store interfaces.Handler
     }
         RPCPurger will purge analytics data into a Mongo database, requires that the
         Mongo DB string is specified in the Config object
    @@ -11163,7 +11207,7 @@
     	// Has unexported fields.
     }
     
    -func NewSyncForcer(controller *storage.ConnectionHandler, getNodeDataFunc func() []byte) *SyncronizerForcer
    +func NewSyncForcer(controller *redisCluster.ConnectionHandler, getNodeDataFunc func() []byte) *SyncronizerForcer
         NewSyncForcer returns a new syncforcer with a connected redis with a key
         prefix synchronizer-group- for group synchronization control.
     
    @@ -11217,54 +11261,34 @@
     CONSTANTS
     
     const (
    -	// DefaultConn is the default connection type. Not analytics and Not cache.
    -	DefaultConn = "default"
    -	// CacheConn is the cache connection type
    -	CacheConn = "cache"
    -	// AnalyticsConn is the analytics connection type
    -	AnalyticsConn = "analytics"
    -)
    -const B64JSONPrefix = "ey"
    -    `{"` in base64
    -
    -const MongoBsonIdLength = 24
    -
    -VARIABLES
    -
    -var (
    -	// ErrRedisIsDown is returned when we can't communicate with redis
    -	ErrRedisIsDown = errors.New("storage: Redis is either down or was not configured")
    -
    -	// ErrStorageConn is returned when we can't get a connection from the ConnectionHandler
    -	ErrStorageConn = fmt.Errorf("Error trying to get singleton instance: %w", ErrRedisIsDown)
    +	REDIS_CLUSTER = "redis"
    +	MDCB          = "mdcb"
    +	DUMMY         = "dummy"
     )
    -var (
    -	HashSha256    = "sha256"
    -	HashMurmur32  = "murmur32"
    -	HashMurmur64  = "murmur64"
    -	HashMurmur128 = "murmur128"
    +const (
    +	DEFAULT_MODULE = "default"
     )
    -var ErrKeyNotFound = errors.New("key not found")
    -    ErrKeyNotFound is a standard error for when a key is not found in the
    -    storage engine
    -
    -var ErrMDCBConnectionLost = errors.New("mdcb connection is lost")
     
     FUNCTIONS
     
    -func GenerateToken(orgID, keyID, hashAlgorithm string) (string, error)
    -    If hashing algorithm is empty, use legacy key generation
    -
    -func HashKey(in string, hashKey bool) string
    -func HashStr(in string, withAlg ...string) string
    -func NewConnector(connType string, conf config.Config) (model.Connector, error)
    -    NewConnector creates a new storage connection.
    -
    -func TokenHashAlgo(token string) string
    -func TokenID(token string) (id string, err error)
    -    TODO: add checks
    -
    -func TokenOrg(token string) string
    +func GetStorageForModule(module string) string
    +    GetStorageForModule returns the storage type for the given module. Defaults
    +    to REDIS_CLUSTER for the initial implementation.
    +
    +func IsAnalytics(analytics bool) func(interfaces.Handler)
    +func IsCache(cache bool) func(interfaces.Handler)
    +func NewStorageHandler(name string, opts ...func(interfaces.Handler)) (interfaces.Handler, error)
    +func WithConnectionHandler(handler *redisCluster.ConnectionHandler) func(interfaces.Handler)
    +func WithHashKeys(hashKeys bool) func(interfaces.Handler)
    +func WithKeyPrefix(prefix string) func(interfaces.Handler)
    +    Redis Cluster Options
    +
    +func WithLocalStorageHandler(handler interfaces.Handler) func(interfaces.Handler)
    +    MDCB Options
    +
    +func WithLogger(logger *logrus.Entry) func(interfaces.Handler)
    +func WithRedisController(controller *redisCluster.RedisController) func(interfaces.Handler)
    +func WithRpcStorageHandler(handler interfaces.Handler) func(interfaces.Handler)
     
     TYPES
     
    @@ -11276,33 +11300,12 @@
     	GetExp(string) (int64, error) // Returns expiry of a key
     }
     
    -type ConnectionHandler struct {
    -	// Has unexported fields.
    -}
    -    ConnectionHandler is a wrapper around the storage connection. It allows to
    -    dynamically enable/disable talking with storage and mantain a connection map
    -    to different storage types.
    +# Package: ./storage/dummy
     
    -func NewConnectionHandler(ctx context.Context) *ConnectionHandler
    -    NewConnectionHandler creates a new connection handler not connected
    +package dummy // import "github.com/TykTechnologies/tyk/storage/dummy"
     
    -func (rc *ConnectionHandler) Connect(ctx context.Context, onConnect func(), conf *config.Config)
    -    Connect starts a go routine that periodically tries to connect to storage.
     
    -    onConnect will be called when we have established a successful storage
    -    reconnection
    -
    -func (rc *ConnectionHandler) Connected() bool
    -    Connected returns true if we are connected to redis
    -
    -func (rc *ConnectionHandler) DisableStorage(setStorageDown bool)
    -    DisableStorage allows to dynamically enable/disable talking with storage
    -
    -func (rc *ConnectionHandler) Disconnect() error
    -    Disconnect closes the connection to the storage
    -
    -func (rc *ConnectionHandler) WaitConnect(ctx context.Context) bool
    -    WaitConnect waits until we are connected to the storage
    +TYPES
     
     type DummyStorage struct {
     	Data      map[string]string
    @@ -11455,49 +11458,65 @@
         SetRollingWindow sets a rolling window for a key with specified parameters;
         implementation pending.
     
    -type Handler interface {
    -	GetKey(string) (string, error) // Returned string is expected to be a JSON object (user.SessionState)
    -	GetMultiKey([]string) ([]string, error)
    -	GetRawKey(string) (string, error)
    -	SetKey(string, string, int64) error // Second input string is expected to be a JSON object (user.SessionState)
    -	SetRawKey(string, string, int64) error
    -	SetExp(string, int64) error   // Set key expiration
    -	GetExp(string) (int64, error) // Returns expiry of a key
    -	GetKeys(string) []string
    -	DeleteKey(string) bool
    -	DeleteAllKeys() bool
    -	DeleteRawKey(string) bool
    -	Connect() bool
    -	GetKeysAndValues() map[string]string
    -	GetKeysAndValuesWithFilter(string) map[string]string
    -	DeleteKeys([]string) bool
    -	Decrement(string)
    -	IncrememntWithExpire(string, int64) int64
    -	SetRollingWindow(key string, per int64, val string, pipeline bool) (int, []interface{})
    -	GetRollingWindow(key string, per int64, pipeline bool) (int, []interface{})
    -	GetSet(string) (map[string]string, error)
    -	AddToSet(string, string)
    -	GetAndDeleteSet(string) []interface{}
    -	RemoveFromSet(string, string)
    -	DeleteScanMatch(string) bool
    -	GetKeyPrefix() string
    -	AddToSortedSet(string, string, float64)
    -	GetSortedSetRange(string, string, string) ([]string, []float64, error)
    -	RemoveSortedSetRange(string, string, string) error
    -	GetListRange(string, int64, int64) ([]string, error)
    -	RemoveFromList(string, string) error
    -	AppendToSet(string, string)
    -	Exists(string) (bool, error)
    +# Package: ./storage/kv
    +
    +package kv // import "github.com/TykTechnologies/tyk/storage/kv"
    +
    +
    +VARIABLES
    +
    +var (
    +	// ErrKeyNotFound is an error meant when a key doesn't exists in the
    +	// storage backend
    +	ErrKeyNotFound = errors.New("key not found")
    +)
    +
    +TYPES
    +
    +type Consul struct {
    +	// Has unexported fields.
     }
    -    Handler is a standard interface to a storage backend, used by
    -    AuthorisationManager to read and write key values to the backend
    +    Consul is an implementation of a KV store which uses Consul as it's backend
    +
    +func (c *Consul) Get(key string) (string, error)
    +
    +func (c *Consul) Store() *consulapi.KV
    +
    +type Store interface {
    +	Get(key string) (string, error)
    +}
    +    Store is a standard interface to a KV backend
    +
    +func NewConsul(conf config.ConsulConfig) (Store, error)
    +    NewConsul returns a configured consul KV store adapter
    +
    +func NewVault(conf config.VaultConfig) (Store, error)
    +    NewVault returns a configured vault KV store adapter
    +
    +type Vault struct {
    +	// Has unexported fields.
    +}
    +    Vault is an implementation of a KV store which uses Consul as it's backend
    +
    +func (v *Vault) Client() *vaultapi.Client
    +
    +func (v *Vault) Get(key string) (string, error)
    +
    +# Package: ./storage/mdcb
    +
    +package mdcb // import "github.com/TykTechnologies/tyk/storage/mdcb"
    +
    +
    +TYPES
     
     type MdcbStorage struct {
    +	Local                 interfaces.Handler
    +	Rpc                   interfaces.Handler
    +	Logger                *logrus.Entry
     	CallbackonPullfromRPC *func(key string, val string) error
    -	// Has unexported fields.
     }
     
    -func NewMdcbStorage(local, rpc Handler, log *logrus.Entry) *MdcbStorage
    +func NewMdcbStorage(local, rpc interfaces.Handler, log *logrus.Entry) *MdcbStorage
     
     func (m MdcbStorage) AddToSet(key string, value string)
     
    @@ -11564,6 +11583,68 @@
     
     func (m MdcbStorage) SetRollingWindow(key string, per int64, val string, pipeline bool) (int, []interface{})
     
    +# Package: ./storage/redis-cluster
    +
    +package redisCluster // import "github.com/TykTechnologies/tyk/storage/redis-cluster"
    +
    +
    +CONSTANTS
    +
    +const (
    +	// DefaultConn is the default connection type. Not analytics and Not cache.
    +	DefaultConn = "default"
    +	// CacheConn is the cache connection type
    +	CacheConn = "cache"
    +	// AnalyticsConn is the analytics connection type
    +	AnalyticsConn = "analytics"
    +)
    +
    +VARIABLES
    +
    +var (
    +	// ErrRedisIsDown is returned when we can't communicate with redis
    +	ErrRedisIsDown = errors.New("storage: Redis is either down or was not configured")
    +
    +	// ErrStorageConn is returned when we can't get a connection from the ConnectionHandler
    +	ErrStorageConn = fmt.Errorf("Error trying to get singleton instance: %w", ErrRedisIsDown)
    +)
    +
    +FUNCTIONS
    +
    +func NewConnector(connType string, conf config.Config) (model.Connector, error)
    +    NewConnector creates a new storage connection.
    +
    +
    +TYPES
    +
    +type ConnectionHandler struct {
    +	// Has unexported fields.
    +}
    +    ConnectionHandler is a wrapper around the storage connection. It allows to
    +    dynamically enable/disable talking with storage and mantain a connection map
    +    to different storage types.
    +
    +func NewConnectionHandler(ctx context.Context) *ConnectionHandler
    +    NewConnectionHandler creates a new connection handler not connected
    +
    +func (rc *ConnectionHandler) Connect(ctx context.Context, onConnect func(), conf *config.Config)
    +    Connect starts a go routine that periodically tries to connect to storage.
    +
    +    onConnect will be called when we have established a successful storage
    +    reconnection
    +
    +func (rc *ConnectionHandler) Connected() bool
    +    Connected returns true if we are connected to redis
    +
    +func (rc *ConnectionHandler) DisableStorage(setStorageDown bool)
    +    DisableStorage allows to dynamically enable/disable talking with storage
    +
    +func (rc *ConnectionHandler) Disconnect() error
    +    Disconnect closes the connection to the storage
    +
    +func (rc *ConnectionHandler) WaitConnect(ctx context.Context) bool
    +    WaitConnect waits until we are connected to the storage
    +
     type RedisCluster struct {
     	KeyPrefix   string
     	HashKeys    bool
    @@ -11730,50 +11811,51 @@
         plugins to wait for connectivity before proceeding with operations that
         require Redis access.
     
    -# Package: ./storage/kv
    +# Package: ./storage/shared
     
    -package kv // import "github.com/TykTechnologies/tyk/storage/kv"
    +package shared // import "github.com/TykTechnologies/tyk/storage/shared"
     
     
     VARIABLES
     
    -var (
    -	// ErrKeyNotFound is an error meant when a key doesn't exists in the
    -	// storage backend
    -	ErrKeyNotFound = errors.New("key not found")
    -)
    +var ErrKeyNotFound = errors.New("key not found")
    +    ErrKeyNotFound is a standard error for when a key is not found in the
    +    storage engine
     
    -TYPES
    +var ErrMDCBConnectionLost = errors.New("mdcb connection is lost")
    +# Package: ./storage/util
     
    -type Consul struct {
    -	// Has unexported fields.
    -}
    -    Consul is an implementation of a KV store which uses Consul as it's backend
    +package util // import "github.com/TykTechnologies/tyk/storage/util"
     
    -func (c *Consul) Get(key string) (string, error)
     
    -func (c *Consul) Store() *consulapi.KV
    +CONSTANTS
     
    -type Store interface {
    -	Get(key string) (string, error)
    -}
    -    Store is a standard interface to a KV backend
    +const B64JSONPrefix = "ey"
    +    `{"` in base64
     
    -func NewConsul(conf config.ConsulConfig) (Store, error)
    -    NewConsul returns a configured consul KV store adapter
    +const MongoBsonIdLength = 24
     
    -func NewVault(conf config.VaultConfig) (Store, error)
    -    NewVault returns a configured vault KV store adapter
    +VARIABLES
     
    -type Vault struct {
    -	// Has unexported fields.
    -}
    -    Vault is an implementation of a KV store which uses Consul as it's backend
    +var (
    +	HashSha256    = "sha256"
    +	HashMurmur32  = "murmur32"
    +	HashMurmur64  = "murmur64"
    +	HashMurmur128 = "murmur128"
    +)
     
    -func (v *Vault) Client() *vaultapi.Client
    +FUNCTIONS
     
    -func (v *Vault) Get(key string) (string, error)
    +func GenerateToken(orgID, keyID, hashAlgorithm string) (string, error)
    +    If hashing algorithm is empty, use legacy key generation
     
    +func HashKey(in string, hashKey bool) string
    +func HashStr(in string, withAlg ...string) string
    +func TokenHashAlgo(token string) string
    +func TokenID(token string) (id string, err error)
    +    TODO: add checks
    +
    +func TokenOrg(token string) string
     # Package: ./tcp
     
     package tcp // import "github.com/TykTechnologies/tyk/tcp"

    Copy link
    Contributor

    github-actions bot commented Aug 7, 2024

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Removed Code
    Significant portions of code related to hashing and token generation have been removed from storage/storage.go and possibly moved to other files like storage/util/util.go. It's crucial to ensure that these functionalities are preserved and correctly implemented in their new locations. Additionally, the removal of error definitions such as ErrKeyNotFound and ErrMDCBConnectionLost should be carefully managed to ensure they are appropriately relocated and handled.

    Interface Changes
    The SessionHandler interface and its implementation DefaultSessionManager have been modified to use the new interfaces.Handler instead of storage.Handler. This change affects several methods and their implementations. It's important to review these changes to ensure that the new interface provides all necessary methods and that the transition from the old interface does not introduce any regressions or bugs.

    Test Adjustments
    Tests in gateway/gateway_test.go have been adjusted to use redisCluster.RedisCluster instead of storage.RedisCluster. This change is part of a broader refactoring that involves modifying how Redis connections are handled. It's important to ensure that these changes do not affect the expected behavior of the tests and that the Redis cluster functionalities are still correctly integrated and utilized.

    Copy link
    Contributor

    github-actions bot commented Aug 7, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Properly handle errors when creating a storage handler to prevent runtime issues

    The function getStorageForPython should handle the error returned by
    storage.NewStorageHandler properly. Currently, it logs the error but still returns
    the uninitialized store which might lead to runtime panics if used. It's safer to
    return nil when an error occurs.

    gateway/coprocess_api.go [27-34]

     store, err := storage.NewStorageHandler(
         storage.GetStorageForModule(storage.DEFAULT_MODULE),
         storage.WithKeyPrefix(CoProcessDefaultKeyPrefix),
         storage.WithConnectionHandler(rc))
     if err != nil {
         log.WithError(err).Error("could not create storage handler")
    +    return nil
     }
     return store
     
    Suggestion importance[1-10]: 10

    Why: Properly handling errors when creating a storage handler is essential to prevent runtime issues and potential crashes, making this a critical improvement.

    10
    Add error handling for storage initialization to enhance reliability

    Consider adding error handling for the NewMdcbStorage function to catch any
    potential initialization errors. This will prevent runtime panics due to unhandled
    errors.

    certs/manager.go [105]

    -mdcbStorage := mdcb.NewMdcbStorage(localStorage, rpcStorage, log)
    +mdcbStorage, err := mdcb.NewMdcbStorage(localStorage, rpcStorage, log)
    +if err != nil {
    +    return nil, err
    +}
     
    Suggestion importance[1-10]: 9

    Why: Adding error handling for the NewMdcbStorage function is crucial as it prevents potential runtime panics and enhances the reliability of the application.

    9
    Handle errors returned by the spec.Init() method

    Consider handling the error returned by spec.Init() to ensure that initialization
    errors are caught and handled properly.

    gateway/coprocess_id_extractor_test.go [40]

    -spec.Init(redisStore, redisStore, healthStore, orgStore)
    +if err := spec.Init(redisStore, redisStore, healthStore, orgStore); err != nil {
    +    log.WithError(err).Error("Failed to initialize spec")
    +    return nil
    +}
     
    Suggestion importance[1-10]: 9

    Why: Handling the error returned by spec.Init() is crucial for ensuring that any initialization issues are caught and logged, preventing potential runtime errors.

    9
    Handle errors returned by the store.DeleteScanMatch() method

    It's recommended to handle the error returned by store.DeleteScanMatch() to ensure
    that deletion errors are caught and handled properly.

    gateway/delete_api_cache.go [21]

    -return store.DeleteScanMatch(fmt.Sprintf("cache-%s*", apiID))
    +if err := store.DeleteScanMatch(fmt.Sprintf("cache-%s*", apiID)); err != nil {
    +    log.WithError(err).Error("Failed to delete API cache")
    +    return false
    +}
    +return true
     
    Suggestion importance[1-10]: 9

    Why: Handling the error returned by store.DeleteScanMatch() is important to ensure that any issues during cache deletion are caught and logged, improving the robustness of the code.

    9
    Handle errors returned by the store.SetRawKey() method

    Consider handling the error returned by store.SetRawKey() to ensure that key setting
    errors are caught and handled properly.

    gateway/health_check.go [92]

    -err := store.SetRawKey(key, key, 10)
    +if err := store.SetRawKey(key, key, 10); err != nil {
    +    mainLog.WithField("liveness-check", true).WithError(err).Error("Failed to set raw key")
    +    return
    +}
     
    Suggestion importance[1-10]: 9

    Why: Handling the error returned by store.SetRawKey() is essential for ensuring that any issues during key setting are caught and logged, preventing potential silent failures.

    9
    Add nil checks before accessing session cache to prevent nil pointer dereferences

    The method clearCacheForKey should check if b.Gw or b.Gw.SessionCache is nil before
    attempting to delete the cache key. This prevents potential nil pointer
    dereferences.

    gateway/auth_manager.go [103-106]

     cacheKey := util.HashKey(keyName, b.Gw.GetConfig().HashKeys)
    -b.Gw.SessionCache.Delete(cacheKey)
    +if b.Gw != nil && b.Gw.SessionCache != nil {
    +    b.Gw.SessionCache.Delete(cacheKey)
    +}
     
    Suggestion importance[1-10]: 8

    Why: Adding nil checks before accessing session cache is important to prevent nil pointer dereferences, which can cause runtime panics.

    8
    Check for nil before calling Connect() on w.store

    Ensure that the w.store.Connect() method is called after error checking to avoid
    potential nil pointer dereference if the store was not created successfully.

    gateway/event_handler_webhooks.go [85]

    -w.store.Connect()
    +if w.store != nil {
    +    w.store.Connect()
    +} else {
    +    return errors.New("storage handler is not initialized")
    +}
     
    Suggestion importance[1-10]: 8

    Why: Ensuring that w.store is not nil before calling Connect() prevents potential nil pointer dereference, which can lead to runtime panics.

    8
    Possible issue
    Add error handling after connecting to Redis cluster to ensure connection stability

    Consider checking for errors after calling Connect() on cacheStore. This ensures
    that the connection to the Redis cluster is established without issues, which is
    critical for the subsequent operations.

    gateway/api_test.go [1726-1727]

     cacheStore := redisCluster.RedisCluster{ConnectionHandler: ts.Gw.StorageConnectionHandler}
    -cacheStore.Connect()
    +if err := cacheStore.Connect(); err != nil {
    +    log.Fatalf("Failed to connect to Redis cluster: %v", err)
    +}
     
    Suggestion importance[1-10]: 9

    Why: Adding error handling after connecting to the Redis cluster is crucial for ensuring the stability and reliability of the connection, preventing potential runtime issues.

    9
    Enhancement
    Improve error messages in tests for better debugging

    Modify the error message to include more context about the failure, such as the
    expected and actual types, to aid in debugging.

    certs/manager_test.go [199]

    -t.Error("cannot make storage.DummyStorage of type interfaces.Handler")
    +t.Errorf("expected storage.DummyStorage to implement interfaces.Handler, got %T", storage)
     
    Suggestion importance[1-10]: 8

    Why: Enhancing error messages in tests with more context aids in debugging and improves the overall quality of the test suite.

    8
    Ensure storage is connected before attempting to delete keys to avoid silent failures

    In the deleteRawKeysWithAllowanceScope method, add a check to ensure that store is
    not only non-nil but also connected before attempting to delete keys. This ensures
    that the operation does not fail silently if the store is not ready.

    gateway/auth_manager.go [86-89]

    -if store == nil || session == nil {
    +if store == nil || session == nil || !store.IsConnected() {
         return
     }
     
    Suggestion importance[1-10]: 7

    Why: Ensuring the storage is connected before attempting to delete keys enhances the robustness of the method, preventing silent failures.

    7
    Best practice
    Ensure consistent logger configuration across the application

    Replace the direct instantiation of logrus.New() with a function that configures the
    logger according to the application's logging standards. This ensures that all
    loggers have consistent configuration, such as formatting and level settings.

    certs/manager.go [64-66]

     if logger == nil {
    -    logger = logrus.New()
    +    logger = createConfiguredLogger()
     }
     
    Suggestion importance[1-10]: 7

    Why: This suggestion promotes best practices by ensuring consistent logger configuration, which is beneficial for maintainability and debugging. However, it is not critical to the functionality of the application.

    7
    Maintainability
    Refactor conditional checks for clarity and maintainability

    Refactor the condition to check the method and token organization separately for
    clarity and to avoid potential bugs in future modifications.

    gateway/api.go [542]

    -if r.Method == http.MethodPost || util.TokenOrg(keyName) != ""
    +isPostMethod := r.Method == http.MethodPost
    +hasTokenOrg := util.TokenOrg(keyName) != ""
    +if isPostMethod || hasTokenOrg
     
    Suggestion importance[1-10]: 6

    Why: The refactoring suggestion improves code readability and maintainability, which is beneficial for future modifications. However, it does not address a critical issue.

    6

    Copy link

    sonarqubecloud bot commented Aug 7, 2024

    Quality Gate Failed Quality Gate failed

    Failed conditions
    0.0% Coverage on New Code (required ≥ 80%)

    See analysis details on SonarCloud

    hashAlgorithm = defaultHashAlgorithm
    }

    jsonToken := fmt.Sprintf(`{"org":"%s","id":"%s","h":"%s"}`, orgID, keyID, hashAlgorithm)

    Check failure

    Code scanning / CodeQL

    Potentially unsafe quoting Critical

    If this
    JSON value
    contains a double quote, it could break out of the enclosing quotes.
    If this
    JSON value
    contains a double quote, it could break out of the enclosing quotes.
    If this
    JSON value
    contains a double quote, it could break out of the enclosing quotes.
    @lonelycode
    Copy link
    Member Author

    Closing this in favor of TykTechnologies/storage#112 and #6457

    @lonelycode lonelycode closed this Aug 12, 2024
    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