Merging to release-5.3.9: 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 and updated its usage in manager.go.
• Added extensive unit tests in mdcb_storage_test.go to validate new caching and retrieval logic, improving test coverage.
• Generated a mock for the Handler interface using GoMock in storage/mock/storage.go to facilitate isolated testing of storage interactions.
• Updated storage.go with a GoMock directive for mock generation.

---

Changes walkthrough 📝

	Relevant files
Enhancement	manager.go Rename callback function for certificate pull consistency certs/manager.go Renamed callback function for certificate pull consistency. Updated initialization of mdcbStorage with the renamed callback. +1/-3      server.go Add callback parameter to MDCB storage handler initialization gateway/server.go Added a new parameter to the getGlobalMDCBStorageHandler function to support callback functionality. +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 caching mechanisms for OAuth clients and certificates. Introduced helper methods for resource-specific caching. Added constants for resource types to improve maintainability. +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 utilities for mocking dependencies. Enhanced test coverage for error handling and resource type processing. +323/-5  storage.go Add GoMock-generated mock for Handler interface                    storage/mock/storage.go Added a GoMock-generated mock for the Handler interface. Enables isolated testing of storage interactions. +502/-0  storage.go Add GoMock directive for Handler interface                              storage/storage.go Added GoMock directive for generating mocks of the Handler interface. +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:24:51.269911049 +0000
+++ current.txt	2024-12-23 15:24:48.352891001 +0000
@@ -11171,11 +11171,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)
 
@@ -11456,6 +11456,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 testable functions. Replace DummyStorage mentions with a mock for the storage handler interface. Add tests for MDCB storage functionality. Not compliant requirements: Address certificate caching, ensuring it works with the certificate manager and secret encoding.

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Callback Initialization The callbackOnPullCertFromRPC is passed as a parameter to NewMdcbStorage, but its usage and initialization should be validated to ensure proper behavior. mdcbStorage := storage.NewMdcbStorage(localStorage, rpcStorage, log, callbackOnPullCertFromRPC) Error Handling The getFromRPCAndCache method does not log errors when the RPC call fails. Consider adding error logging for better debugging. func (m MdcbStorage) getFromRPCAndCache(key string) (string, error) { val, err := m.rpc.GetKey(key) if err != nil { return "", err } err = m.processResourceByType(key, val) return val, err Test Coverage While tests for caching mechanisms are extensive, ensure edge cases for processResourceByType and getFromRPCAndCache are fully covered. func TestProcessResourceByType(t *testing.T) { // Setup errCachingFailed := errors.New("caching failed") // Test cases testCases := []struct { name string key string val string setupMocks func(handler *mock.MockHandler) expectedError error }{ { name: "Successful OAuth client caching", key: "oauth-clientid.client1", val: "clientdata1", setupMocks: func(mockLocal *mock.MockHandler) { mockLocal.EXPECT().SetKey("oauth-clientid.client1", "clientdata1", gomock.Any()).Return(nil) }, expectedError: nil, }, { name: "Failed OAuth client caching", key: "oauth-clientid.failClient2", val: "clientdata2", setupMocks: func(mockLocal *mock.MockHandler) { mockLocal.EXPECT().SetKey("oauth-clientid.failClient2", "clientdata2", gomock.Any()).Return(errCachingFailed) }, expectedError: errCachingFailed, }, { name: "Successful Certificate caching", key: "cert:cert1", val: "certdata1", setupMocks: func(_ *mock.MockHandler) { // Setup expectations for certificate caching if needed }, expectedError: nil, }, { name: "Failed Certificate caching", key: "cert:failCert", val: "certdata2", setupMocks: func(_ *mock.MockHandler) { // Setup expectations for failed certificate caching if needed }, expectedError: errCachingFailed, }, { name: "Unknown resource type", key: "unknown:resource1", val: "data1", setupMocks: func(_ *mock.MockHandler) {}, expectedError: nil, }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { setup := setupTest(t) defer setup.CleanUp() m := setup.MdcbStorage tc.setupMocks(setup.Local) // If testing certificate caching, setup the callback if strings.HasPrefix(tc.key, "cert:") { m.OnRPCCertPull = func(key, _ string) error { if key == "cert:failCert" { return errCachingFailed } return nil } } err := m.processResourceByType(tc.key, tc.val) if tc.expectedError != nil { assert.Error(t, err) assert.ErrorIs(t, err, tc.expectedError) } else { assert.NoError(t, err) } }) } }

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Possible issue	Validate the initialization of callback functions to prevent runtime errors Ensure that the callbackOnPullCertFromRPC function is properly initialized before being passed to storage.NewMdcbStorage to avoid potential nil pointer dereference errors. 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: The suggestion addresses a potential runtime issue by ensuring that the callback function is initialized before being passed to storage.NewMdcbStorage. This is a critical improvement as it prevents nil pointer dereference errors, which could cause the application to crash.	8
General	Add error handling for local storage key retrieval to ensure robustness Add error handling for the m.local.GetKey call in getFromLocal to ensure that unexpected errors are logged or handled appropriately. storage/mdcb_storage.go [290] -return m.local.GetKey(key) +val, err := m.local.GetKey(key) +if err != nil { + m.logger.Errorf("Error retrieving key from local storage: %v", err) + return "", err +} +return val, nil Suggestion importance[1-10]: 7 Why: Adding error handling for the m.local.GetKey call improves robustness by ensuring that unexpected errors are logged and handled appropriately. This is a valuable enhancement for debugging and maintaining the system.	7
	Log errors during caching to improve observability and debugging Ensure that the getFromRPCAndCache function logs errors when caching fails, to aid in debugging and monitoring. storage/mdcb_storage.go [284] +if err != nil { + m.logger.Errorf("Error caching resource for key %s: %v", key, err) +} return val, err Suggestion importance[1-10]: 7 Why: Logging errors during caching in getFromRPCAndCache enhances observability and aids in debugging by providing clear information about failures. This is a practical improvement for monitoring and troubleshooting.	7
	Add a default case to handle unrecognized resource types in switch statements Ensure that the processResourceByType function handles all possible resource types explicitly to avoid unhandled cases. storage/mdcb_storage.go [274] -return nil +default: + m.logger.Warnf("Unhandled resource type for key: %s", key) + return nil Suggestion importance[1-10]: 6 Why: The suggestion improves the processResourceByType function by adding a default case to handle unrecognized resource types. This ensures that all cases are explicitly handled, reducing the risk of silent failures.	6

[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