Merging to release-5.7: 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 CallbackOnPullCertificateFromRPC for consistency. Updated initialization of mdcbStorage with the renamed callback. +1/-3      server.go Update MDCB storage handler initialization                              gateway/server.go Added a nil callback parameter when initializing MDCB storage handler. +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 retrieval logic. +69/-36
Tests	mdcb_storage_test.go Add unit tests for caching and retrieval logic                      storage/mdcb_storage_test.go Added 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 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:33:00.985067333 +0000
+++ current.txt	2024-12-19 16:32:56.493080052 +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 that the first time an OAuth client is pulled from RPC, it is cached in Redis. Refactor MDCB storage code into smaller, testable functions. Create a mock for the storage handler interface and replace DummyStorage with it. Add tests for MDCB storage. Not compliant requirements: Certificates caching should not work the same way as OAuth clients 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 unexpected behavior if new resource types are introduced. Test Coverage While the test cases are comprehensive, edge cases for error handling in processResourceByType and getFromRPCAndCache should be validated further.

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Possible issue	Validate the callback function to prevent nil pointer dereference issues Ensure that the callbackOnPullCertFromRPC function is properly initialized and passed to NewMdcbStorage to avoid potential nil pointer dereference errors when the callback is invoked. 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: This suggestion addresses a potential nil pointer dereference issue by ensuring the callback function is initialized before being passed. This is a critical improvement for preventing runtime errors and ensuring the robustness of the code.	8
General	Handle errors when saving OAuth data locally to prevent silent failures Add error handling for the SetKey method in cacheOAuthClient to ensure failures in saving OAuth data locally are logged or handled appropriately. 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 locally: %v", err) + return err +} +return nil Suggestion importance[1-10]: 7 Why: Adding error handling for the SetKey method in cacheOAuthClient improves reliability by ensuring that failures are logged and not silently ignored. This enhances debugging and error traceability.	7
	Improve error handling in Exists by logging detailed errors for both storage layers Add a fallback mechanism in Exists to handle cases where both local and RPC storage checks fail, ensuring a more robust error message or recovery strategy. storage/mdcb_storage.go [243-244] if errLocal != nil && errRpc != nil { + m.logger.Errorf("Key existence check failed in both storages: local error: %v, RPC error: %v", errLocal, errRpc) return false, errors.New("cannot find key in storages") } Suggestion importance[1-10]: 7 Why: Adding detailed error logging for both local and RPC storage checks in Exists improves error traceability and debugging. This is a meaningful enhancement for understanding and resolving issues in storage operations.	7
	Log errors when processing resources by type fails to improve observability Ensure that getFromRPCAndCache logs an error when the processResourceByType function fails, to aid in debugging and monitoring. storage/mdcb_storage.go [284-285] err = m.processResourceByType(key, val) +if err != nil { + m.logger.Errorf("Failed to process resource by type for key %s: %v", key, err) +} return val, err Suggestion importance[1-10]: 6 Why: Logging errors when processResourceByType fails provides better observability and aids in debugging. While not critical, it is a valuable enhancement for monitoring and troubleshooting.	6

[sonarqubecloud]

Quality Gate failed

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

See analysis details on SonarQube Cloud