Merging to release-5.3: TT-13513 TT-12767 TT-12768 ensure to save oauth clients locally when pulled from rpc (#6740)

[buger]

User description

TT-13513 TT-12767 TT-12768 ensure to save oauth clients locally when pulled from rpc (#6740)

User description

Description

The Oauth client was not being cached in the local redis when the
gateway was running as an edge in an MDCB setup. This PR then:

• Ensures that the first time that the oauthclient is pulled from RPC
  then we cache it in redis
• Refactor code of the MDCB storage into multiple smaller functions so
  is eaasy to read the code and test
• created mock for the storage handler interface...later we should
  remove all mentions to DummyStorage and use the mock instead
• Created tests for the mdcb storage
• Certificates caching doesnt works in the same way, as they depend on
  the certificate manager and secret set to encode the content

Related Issue

Motivation and Context

How This Has Been Tested

• Run MDCB setup with synchroniser disabled
• Created api and policy via dashboard.
• Protect the api using oauth 2.0
• Created an oauth client via dashboard api
• Create a token in the edge node using the created oauth client
• use the token to consume the api in that edge node
• shut down mdcb
• attempt to generate another token using the edge node
• At this point you should be allowed to create that new token and use
  it against the api

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

Bug fix, Tests, Enhancement

---

Description

• Refactored the GetKey method to separate local and RPC retrieval
  logic, improving maintainability.
• Introduced caching mechanisms for OAuth clients and certificates,
  ensuring resources pulled from RPC are stored locally.
• Added constants for resource types to improve code readability and
  maintainability.
• Renamed callback function for certificate pull consistency.
• Added extensive unit tests for new caching and retrieval logic,
  improving test coverage.
• Generated a mock for the Handler interface using GoMock to
  facilitate isolated testing of storage interactions.

---

Changes walkthrough 📝

	Relevant files
Enhancement	manager.go Rename callback function for certificate pull consistency certs/manager.go Renamed CallbackonPullfromRPC to CallbackOnPullCertificateFromRPC for consistency. Updated the initialization of mdcbStorage with the renamed callback. +1/-1      mdcb_storage.go Refactor key retrieval and add caching mechanisms                storage/mdcb_storage.go Added constants for resource types (resourceOauthClient, resourceCertificate, etc.). Refactored GetKey to separate local and RPC retrieval logic. Introduced caching mechanisms for OAuth clients and certificates. Added helper methods like getFromRPCAndCache, cacheCertificate, and cacheOAuthClient. +74/-32  storage.go Add GoMock directive for Handler interface                              storage/storage.go Added GoMock generation directive for the Handler interface. Prepared the file for mock generation to support testing. +2/-0
Tests	mdcb_storage_test.go Add unit tests for caching and retrieval logic                      storage/mdcb_storage_test.go Added test setup utility for mocking dependencies. Implemented unit tests for new caching and retrieval methods. Enhanced test coverage for resource type processing and error handling. +323/-4  storage.go Add GoMock-generated mock for Handler interface                    storage/mock/storage.go Added a generated mock for the Handler interface using GoMock. Enables testing of storage interactions in isolation. +501/-0

---

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

---

Co-authored-by: sredny buitrago [email protected]
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Co-authored-by: Matias [email protected]
Co-authored-by: Mladen Kolavcic [email protected]

---

PR Type

Bug fix, Tests, Enhancement

---

Description

• Refactored the GetKey method in mdcb_storage.go to separate local and RPC retrieval logic, improving maintainability.
• Introduced caching mechanisms for OAuth clients and certificates, ensuring resources pulled from RPC are stored locally.
• Added constants for resource types to improve code readability and maintainability.
• Renamed callback function for certificate pull consistency in manager.go.
• Added extensive unit tests in mdcb_storage_test.go for new caching and retrieval logic, improving test coverage.
• Generated a GoMock mock for the Handler interface in mock/storage.go to facilitate isolated testing of storage interactions.
• Updated MDCB storage initialization to include callback parameters in server.go.

---

Changes walkthrough 📝

	Relevant files
Enhancement	manager.go Rename callback function and update initialization              certs/manager.go Renamed CallbackonPullfromRPC to CallbackOnPullCertificateFromRPC for consistency. Updated mdcbStorage initialization with the renamed callback. +1/-3      server.go Update MDCB storage initialization with callback parameter gateway/server.go Added a nil callback parameter to getGlobalMDCBStorageHandler for MDCB storage initialization. +1/-0      mdcb_storage.go Refactor key retrieval and add caching mechanisms                storage/mdcb_storage.go Refactored GetKey method to separate local and RPC retrieval logic. Introduced caching mechanisms for OAuth clients and certificates. Added helper methods like getFromRPCAndCache, cacheCertificate, and cacheOAuthClient. Added constants for resource types to improve readability. +69/-36
Tests	mdcb_storage_test.go Add unit tests for caching and retrieval logic                      storage/mdcb_storage_test.go Added extensive unit tests for caching and retrieval logic. Implemented test setup utility for mocking dependencies. Enhanced test coverage for resource type processing and error handling. +323/-5  storage.go Add GoMock-generated mock for Handler interface                    storage/mock/storage.go Added a GoMock-generated mock for the Handler interface. Facilitates isolated testing of storage interactions. +502/-0  storage.go Add GoMock directive for Handler interface                              storage/storage.go Added GoMock generation directive for the Handler interface. Prepared the file for mock generation to support testing. +2/-0

---

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

[github-actions]

API Changes

--- prev.txt	2024-12-19 16:31:06.675313192 +0000
+++ current.txt	2024-12-19 16:31:03.759272092 +0000
@@ -11172,11 +11172,11 @@
     AuthorisationManager to read and write key values to the backend
 
 type MdcbStorage struct {
-	CallbackonPullfromRPC *func(key string, val string) error
+	OnRPCCertPull func(key string, val string) error
 	// Has unexported fields.
 }
 
-func NewMdcbStorage(local, rpc Handler, log *logrus.Entry) *MdcbStorage
+func NewMdcbStorage(local, rpc Handler, log *logrus.Entry, OnRPCCertPull func(key string, val string) error) *MdcbStorage
 
 func (m MdcbStorage) AddToSet(key string, value string)
 
@@ -11457,6 +11457,229 @@
 
 func (v *Vault) Get(key string) (string, error)
 
+# Package: ./storage/mock
+
+package mock // import "github.com/TykTechnologies/tyk/storage/mock"
+
+Package mock is a generated GoMock package.
+
+TYPES
+
+type MockHandler struct {
+	// Has unexported fields.
+}
+    MockHandler is a mock of Handler interface.
+
+func NewMockHandler(ctrl *gomock.Controller) *MockHandler
+    NewMockHandler creates a new mock instance.
+
+func (m *MockHandler) AddToSet(arg0, arg1 string)
+    AddToSet mocks base method.
+
+func (m *MockHandler) AddToSortedSet(arg0, arg1 string, arg2 float64)
+    AddToSortedSet mocks base method.
+
+func (m *MockHandler) AppendToSet(arg0, arg1 string)
+    AppendToSet mocks base method.
+
+func (m *MockHandler) Connect() bool
+    Connect mocks base method.
+
+func (m *MockHandler) Decrement(arg0 string)
+    Decrement mocks base method.
+
+func (m *MockHandler) DeleteAllKeys() bool
+    DeleteAllKeys mocks base method.
+
+func (m *MockHandler) DeleteKey(arg0 string) bool
+    DeleteKey mocks base method.
+
+func (m *MockHandler) DeleteKeys(arg0 []string) bool
+    DeleteKeys mocks base method.
+
+func (m *MockHandler) DeleteRawKey(arg0 string) bool
+    DeleteRawKey mocks base method.
+
+func (m *MockHandler) DeleteRawKeys(arg0 []string) bool
+    DeleteRawKeys mocks base method.
+
+func (m *MockHandler) DeleteScanMatch(arg0 string) bool
+    DeleteScanMatch mocks base method.
+
+func (m *MockHandler) EXPECT() *MockHandlerMockRecorder
+    EXPECT returns an object that allows the caller to indicate expected use.
+
+func (m *MockHandler) Exists(arg0 string) (bool, error)
+    Exists mocks base method.
+
+func (m *MockHandler) GetAndDeleteSet(arg0 string) []any
+    GetAndDeleteSet mocks base method.
+
+func (m *MockHandler) GetExp(arg0 string) (int64, error)
+    GetExp mocks base method.
+
+func (m *MockHandler) GetKey(arg0 string) (string, error)
+    GetKey mocks base method.
+
+func (m *MockHandler) GetKeyPrefix() string
+    GetKeyPrefix mocks base method.
+
+func (m *MockHandler) GetKeys(arg0 string) []string
+    GetKeys mocks base method.
+
+func (m *MockHandler) GetKeysAndValues() map[string]string
+    GetKeysAndValues mocks base method.
+
+func (m *MockHandler) GetKeysAndValuesWithFilter(arg0 string) map[string]string
+    GetKeysAndValuesWithFilter mocks base method.
+
+func (m *MockHandler) GetListRange(arg0 string, arg1, arg2 int64) ([]string, error)
+    GetListRange mocks base method.
+
+func (m *MockHandler) GetMultiKey(arg0 []string) ([]string, error)
+    GetMultiKey mocks base method.
+
+func (m *MockHandler) GetRawKey(arg0 string) (string, error)
+    GetRawKey mocks base method.
+
+func (m *MockHandler) GetRollingWindow(arg0 string, arg1 int64, arg2 bool) (int, []any)
+    GetRollingWindow mocks base method.
+
+func (m *MockHandler) GetSet(arg0 string) (map[string]string, error)
+    GetSet mocks base method.
+
+func (m *MockHandler) GetSortedSetRange(arg0, arg1, arg2 string) ([]string, []float64, error)
+    GetSortedSetRange mocks base method.
+
+func (m *MockHandler) IncrememntWithExpire(arg0 string, arg1 int64) int64
+    IncrememntWithExpire mocks base method.
+
+func (m *MockHandler) RemoveFromList(arg0, arg1 string) error
+    RemoveFromList mocks base method.
+
+func (m *MockHandler) RemoveFromSet(arg0, arg1 string)
+    RemoveFromSet mocks base method.
+
+func (m *MockHandler) RemoveSortedSetRange(arg0, arg1, arg2 string) error
+    RemoveSortedSetRange mocks base method.
+
+func (m *MockHandler) SetExp(arg0 string, arg1 int64) error
+    SetExp mocks base method.
+
+func (m *MockHandler) SetKey(arg0, arg1 string, arg2 int64) error
+    SetKey mocks base method.
+
+func (m *MockHandler) SetRawKey(arg0, arg1 string, arg2 int64) error
+    SetRawKey mocks base method.
+
+func (m *MockHandler) SetRollingWindow(arg0 string, arg1 int64, arg2 string, arg3 bool) (int, []any)
+    SetRollingWindow mocks base method.
+
+type MockHandlerMockRecorder struct {
+	// Has unexported fields.
+}
+    MockHandlerMockRecorder is the mock recorder for MockHandler.
+
+func (mr *MockHandlerMockRecorder) AddToSet(arg0, arg1 any) *gomock.Call
+    AddToSet indicates an expected call of AddToSet.
+
+func (mr *MockHandlerMockRecorder) AddToSortedSet(arg0, arg1, arg2 any) *gomock.Call
+    AddToSortedSet indicates an expected call of AddToSortedSet.
+
+func (mr *MockHandlerMockRecorder) AppendToSet(arg0, arg1 any) *gomock.Call
+    AppendToSet indicates an expected call of AppendToSet.
+
+func (mr *MockHandlerMockRecorder) Connect() *gomock.Call
+    Connect indicates an expected call of Connect.
+
+func (mr *MockHandlerMockRecorder) Decrement(arg0 any) *gomock.Call
+    Decrement indicates an expected call of Decrement.
+
+func (mr *MockHandlerMockRecorder) DeleteAllKeys() *gomock.Call
+    DeleteAllKeys indicates an expected call of DeleteAllKeys.
+
+func (mr *MockHandlerMockRecorder) DeleteKey(arg0 any) *gomock.Call
+    DeleteKey indicates an expected call of DeleteKey.
+
+func (mr *MockHandlerMockRecorder) DeleteKeys(arg0 any) *gomock.Call
+    DeleteKeys indicates an expected call of DeleteKeys.
+
+func (mr *MockHandlerMockRecorder) DeleteRawKey(arg0 any) *gomock.Call
+    DeleteRawKey indicates an expected call of DeleteRawKey.
+
+func (mr *MockHandlerMockRecorder) DeleteRawKeys(arg0 any) *gomock.Call
+    DeleteRawKeys indicates an expected call of DeleteRawKeys.
+
+func (mr *MockHandlerMockRecorder) DeleteScanMatch(arg0 any) *gomock.Call
+    DeleteScanMatch indicates an expected call of DeleteScanMatch.
+
+func (mr *MockHandlerMockRecorder) Exists(arg0 any) *gomock.Call
+    Exists indicates an expected call of Exists.
+
+func (mr *MockHandlerMockRecorder) GetAndDeleteSet(arg0 any) *gomock.Call
+    GetAndDeleteSet indicates an expected call of GetAndDeleteSet.
+
+func (mr *MockHandlerMockRecorder) GetExp(arg0 any) *gomock.Call
+    GetExp indicates an expected call of GetExp.
+
+func (mr *MockHandlerMockRecorder) GetKey(arg0 any) *gomock.Call
+    GetKey indicates an expected call of GetKey.
+
+func (mr *MockHandlerMockRecorder) GetKeyPrefix() *gomock.Call
+    GetKeyPrefix indicates an expected call of GetKeyPrefix.
+
+func (mr *MockHandlerMockRecorder) GetKeys(arg0 any) *gomock.Call
+    GetKeys indicates an expected call of GetKeys.
+
+func (mr *MockHandlerMockRecorder) GetKeysAndValues() *gomock.Call
+    GetKeysAndValues indicates an expected call of GetKeysAndValues.
+
+func (mr *MockHandlerMockRecorder) GetKeysAndValuesWithFilter(arg0 any) *gomock.Call
+    GetKeysAndValuesWithFilter indicates an expected call of
+    GetKeysAndValuesWithFilter.
+
+func (mr *MockHandlerMockRecorder) GetListRange(arg0, arg1, arg2 any) *gomock.Call
+    GetListRange indicates an expected call of GetListRange.
+
+func (mr *MockHandlerMockRecorder) GetMultiKey(arg0 any) *gomock.Call
+    GetMultiKey indicates an expected call of GetMultiKey.
+
+func (mr *MockHandlerMockRecorder) GetRawKey(arg0 any) *gomock.Call
+    GetRawKey indicates an expected call of GetRawKey.
+
+func (mr *MockHandlerMockRecorder) GetRollingWindow(arg0, arg1, arg2 any) *gomock.Call
+    GetRollingWindow indicates an expected call of GetRollingWindow.
+
+func (mr *MockHandlerMockRecorder) GetSet(arg0 any) *gomock.Call
+    GetSet indicates an expected call of GetSet.
+
+func (mr *MockHandlerMockRecorder) GetSortedSetRange(arg0, arg1, arg2 any) *gomock.Call
+    GetSortedSetRange indicates an expected call of GetSortedSetRange.
+
+func (mr *MockHandlerMockRecorder) IncrememntWithExpire(arg0, arg1 any) *gomock.Call
+    IncrememntWithExpire indicates an expected call of IncrememntWithExpire.
+
+func (mr *MockHandlerMockRecorder) RemoveFromList(arg0, arg1 any) *gomock.Call
+    RemoveFromList indicates an expected call of RemoveFromList.
+
+func (mr *MockHandlerMockRecorder) RemoveFromSet(arg0, arg1 any) *gomock.Call
+    RemoveFromSet indicates an expected call of RemoveFromSet.
+
+func (mr *MockHandlerMockRecorder) RemoveSortedSetRange(arg0, arg1, arg2 any) *gomock.Call
+    RemoveSortedSetRange indicates an expected call of RemoveSortedSetRange.
+
+func (mr *MockHandlerMockRecorder) SetExp(arg0, arg1 any) *gomock.Call
+    SetExp indicates an expected call of SetExp.
+
+func (mr *MockHandlerMockRecorder) SetKey(arg0, arg1, arg2 any) *gomock.Call
+    SetKey indicates an expected call of SetKey.
+
+func (mr *MockHandlerMockRecorder) SetRawKey(arg0, arg1, arg2 any) *gomock.Call
+    SetRawKey indicates an expected call of SetRawKey.
+
+func (mr *MockHandlerMockRecorder) SetRollingWindow(arg0, arg1, arg2, arg3 any) *gomock.Call
+    SetRollingWindow indicates an expected call of SetRollingWindow.
+
 # Package: ./tcp
 
 package tcp // import "github.com/TykTechnologies/tyk/tcp"

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 6740 - Partially compliant Fully compliant requirements: Ensure that OAuth clients pulled from RPC are cached locally in Redis. Refactor MDCB storage code into smaller, more maintainable functions. Create a mock for the storage handler interface. Add tests for MDCB storage functionality. Not compliant requirements: Replace mentions of DummyStorage with the new mock. Certificates caching should work differently due to dependency on certificate manager and secret set.

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Code Smell The callbackOnPullCertFromRPC is passed as a parameter to NewMdcbStorage, but its usage and necessity should be validated to ensure it aligns with the intended functionality. Possible Bug The processResourceByType function does not handle unknown resource types explicitly, which might lead to unintended behavior. Consider adding a default case or logging for unsupported types. Code Smell The Exists method combines results from local and RPC storage but does not clarify how discrepancies between the two should be handled. This could lead to confusion or unintended behavior.

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Possible issue	Add nil checks for local and rpc handlers to avoid runtime panics Add error handling for cases where m.local or m.rpc is nil in the Exists method to prevent runtime panics. storage/mdcb_storage.go [240-241] +if m.local == nil || m.rpc == nil { + return false, errors.New("local or rpc handler is not initialized") +} foundLocal, errLocal := m.local.Exists(key) foundRpc, errRpc := m.rpc.Exists(key) Suggestion importance[1-10]: 9 Why: Adding nil checks for m.local and m.rpc in the Exists method prevents runtime panics if these handlers are not initialized, significantly improving the stability of the code.	9
	Validate the callback parameter to prevent nil pointer dereference Ensure that the callbackOnPullCertFromRPC is properly initialized and passed to NewMdcbStorage to avoid potential nil pointer dereference when the callback is invoked. certs/manager.go [104] +if callbackOnPullCertFromRPC == nil { + return errors.New("callbackOnPullCertFromRPC cannot be nil") +} mdcbStorage := storage.NewMdcbStorage(localStorage, rpcStorage, log, callbackOnPullCertFromRPC) Suggestion importance[1-10]: 8 Why: Adding a validation for the callbackOnPullCertFromRPC parameter ensures that the callback is not nil, preventing potential runtime errors due to nil pointer dereference. This is a critical improvement for robustness.	8
	Add a nil check for the callback before invoking it to prevent runtime errors Ensure that the OnRPCCertPull callback is checked for nil before invocation in the cacheCertificate method to avoid unexpected nil pointer dereference. storage/mdcb_storage.go [252-253] +if m.OnRPCCertPull == nil { + return errors.New("OnRPCCertPull callback is not set") +} return m.OnRPCCertPull(key, val) Suggestion importance[1-10]: 7 Why: Adding a nil check for the OnRPCCertPull callback in the cacheCertificate method ensures that the method does not attempt to invoke a nil function, preventing potential runtime errors.	7
General	Add error handling for caching OAuth clients to ensure proper error propagation and logging Handle errors returned by m.local.SetKey in the cacheOAuthClient method to ensure proper error propagation and logging. storage/mdcb_storage.go [260] -return m.local.SetKey(key, val, 0) +err := m.local.SetKey(key, val, 0) +if err != nil { + m.logger.Errorf("Failed to cache OAuth client: %v", err) + return err +} +return nil Suggestion importance[1-10]: 8 Why: Handling errors returned by m.local.SetKey in the cacheOAuthClient method ensures proper error propagation and logs failures, improving the maintainability and debuggability of the code.	8

[sonarqubecloud]

Quality Gate failed

Failed conditions
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE