Merging to release-5.7.1: 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, Enhancement, Tests

---

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 callbackOnPullCertFromRPC for consistency. Updated the initialization of mdcbStorage with the renamed callback. +1/-3      server.go Update MDCB storage handler initialization                              gateway/server.go Added a nil callback parameter to the getGlobalMDCBStorageHandler function. +1/-0      mdcb_storage.go Refactor key retrieval and add caching mechanisms                storage/mdcb_storage.go Refactored GetKey to separate local and RPC retrieval logic. Added constants for resource types to improve readability. Introduced caching mechanisms for OAuth clients and certificates. Added helper methods for caching and resource processing. +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 new 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 generated mock for the Handler interface using GoMock. Enables testing of storage interactions in isolation. +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-23 15:27:09.437240972 +0000
+++ current.txt	2024-12-23 15:27:04.586263093 +0000
@@ -11635,11 +11635,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)
 
@@ -11920,6 +11920,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 OAuth clients are cached in local Redis when pulled from RPC. Refactor MDCB storage code into smaller, testable functions. Create a mock for the storage handler interface. Add tests for MDCB storage functionality. Not compliant requirements: Certificates caching should depend on the certificate manager and secret set.

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Callback Initialization The callbackOnPullCertFromRPC is passed to NewMdcbStorage, but its usage and initialization should be validated to ensure it aligns with the intended behavior. mdcbStorage := storage.NewMdcbStorage(localStorage, rpcStorage, log, callbackOnPullCertFromRPC) Error Handling The processResourceByType function does not handle unknown resource types explicitly, which could lead to silent failures. Consider adding a default error case. // processResourceByType based on the type of key it will trigger the proper // caching mechanism func (m MdcbStorage) processResourceByType(key, val string) error { resourceType := getResourceType(key) switch resourceType { case resourceOauthClient: return m.cacheOAuthClient(key, val) case resourceCertificate: return m.cacheCertificate(key, val) } return nil Caching Logic The cacheCertificate and cacheOAuthClient methods rely on external callbacks and local storage. Ensure these dependencies are robust and handle edge cases like nil callbacks or storage failures. // cacheCertificate saves locally resourceCertificate after pull from rpc func (m MdcbStorage) cacheCertificate(key, val string) error { if m.OnRPCCertPull == nil { return nil } return m.OnRPCCertPull(key, val) } // cacheOAuthClient saved oauth data in local storage after pull from rpc func (m MdcbStorage) cacheOAuthClient(key, val string) error { return m.local.SetKey(key, val, 0)

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Possible issue	Add a nil check for storage handlers in the Exists method to prevent runtime errors Handle the case where m.local or m.rpc is nil in the Exists method to prevent potential nil pointer dereference errors. storage/mdcb_storage.go [240-241] +if m.local == nil || m.rpc == nil { + return false, errors.New("local or rpc storage handler is nil") +} foundLocal, errLocal := m.local.Exists(key) foundRpc, errRpc := m.rpc.Exists(key) Suggestion importance[1-10]: 9 Why: Adding a nil check for m.local and m.rpc in the Exists method is crucial to prevent nil pointer dereference errors, which could cause the application to crash. This significantly improves the method's reliability.	9
	Add a check to ensure the callback function is initialized before being passed to avoid runtime errors Ensure that the callbackOnPullCertFromRPC function is properly initialized and passed to NewMdcbStorage to avoid potential nil pointer dereference errors during runtime. certs/manager.go [104] +if callbackOnPullCertFromRPC == nil { + return errors.New("callbackOnPullCertFromRPC is not initialized") +} mdcbStorage := storage.NewMdcbStorage(localStorage, rpcStorage, log, callbackOnPullCertFromRPC) Suggestion importance[1-10]: 8 Why: Adding a check for the initialization of callbackOnPullCertFromRPC ensures that the function is not nil, preventing potential runtime errors. This is a critical improvement for robustness and error handling.	8
	Add error handling for missing callback in cacheCertificate to ensure proper behavior Ensure that the OnRPCCertPull callback is properly checked and invoked in cacheCertificate to avoid unexpected behavior when the callback is nil. storage/mdcb_storage.go [252-255] if m.OnRPCCertPull == nil { - return nil + return errors.New("OnRPCCertPull callback is not set") } return m.OnRPCCertPull(key, val) Suggestion importance[1-10]: 7 Why: While the current implementation safely returns nil when the callback is missing, explicitly returning an error provides better feedback and ensures the caller is aware of the missing callback, improving code clarity and debugging.	7
General	Replace or document the use of nil in callback initialization to ensure clarity and prevent potential issues Replace the nil argument in the NewMdcbStorage call with a meaningful callback or explicitly document why it is safe to pass nil to avoid confusion or potential issues. gateway/server.go [1590] -nil, +callback := func(key string, val string) error { + // Add meaningful implementation or explanation here + return nil +} +callback, Suggestion importance[1-10]: 6 Why: Replacing nil with a meaningful callback or documenting its usage improves code clarity and maintainability. However, the suggestion's impact is moderate as it primarily addresses readability rather than functionality.	6

[sonarqubecloud]

Quality Gate failed

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

See analysis details on SonarQube Cloud