perf: improve authorino caching with coalescing concurrent requests #668
+272
−44
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,193 @@ | ||
| package evaluators | ||
|
|
||
| import ( | ||
| "context" | ||
| "fmt" | ||
| "sync" | ||
| "sync/atomic" | ||
| "testing" | ||
|
|
||
| "github.com/kuadrant/authorino/pkg/auth" | ||
| mock_auth "github.com/kuadrant/authorino/pkg/auth/mocks" | ||
| "github.com/kuadrant/authorino/pkg/evaluators/metadata" | ||
| "github.com/kuadrant/authorino/pkg/httptest" | ||
| "github.com/kuadrant/authorino/pkg/json" | ||
|
|
||
| "go.uber.org/mock/gomock" | ||
| "gotest.tools/assert" | ||
| ) | ||
|
|
||
| const singleflightTestHost = "127.0.0.1:9018" | ||
|
|
||
| func TestMetadataSingleflight_ConcurrentMissesCoalesce(t *testing.T) { | ||
| var callCount atomic.Int32 | ||
|
|
||
| server := httptest.NewHttpServerMock(singleflightTestHost, map[string]httptest.HttpServerMockResponseFunc{ | ||
| "/metadata": func() httptest.HttpServerMockResponse { | ||
| callCount.Add(1) | ||
| return httptest.HttpServerMockResponse{ | ||
| Status: 200, | ||
| Headers: map[string]string{"Content-Type": "application/json"}, | ||
| Body: `{"valid":true}`, | ||
| } | ||
| }, | ||
| }) | ||
| defer server.Close() | ||
|
|
||
| ctrl := gomock.NewController(t) | ||
| defer ctrl.Finish() | ||
|
|
||
| evaluator := &metadata.GenericHttp{ | ||
| Endpoint: fmt.Sprintf("http://%s/metadata", singleflightTestHost), | ||
| Method: "GET", | ||
| AuthCredentials: auth.NewAuthCredential("", "authorization_header"), | ||
| } | ||
|
|
||
| cache := NewEvaluatorCache(&json.JSONValue{Static: "test-key"}, 60) | ||
|
|
||
| config := MetadataConfig{ | ||
| Name: "test-metadata", | ||
| Cache: cache, | ||
| GenericHTTP: evaluator, | ||
| } | ||
| defer config.Clean(context.Background()) | ||
|
There was a problem hiding this comment. 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win Handle each returned error.
Use Also applies to: 123-124, 136-140, 181-181 🧰 Tools🪛 golangci-lint (2.12.2)[error] 53-53: Error return value of (errcheck) 🤖 Prompt for AI AgentsSource: Linters/SAST tools |
||
|
|
||
| const concurrency = 50 | ||
| var wg sync.WaitGroup | ||
| wg.Add(concurrency) | ||
| errors := make([]error, concurrency) | ||
| results := make([]interface{}, concurrency) | ||
|
|
||
| pipelineMock := mock_auth.NewMockAuthPipeline(ctrl) | ||
| pipelineMock.EXPECT().GetAuthorizationJSON().AnyTimes().Return(`{}`) | ||
|
|
||
| for i := 0; i < concurrency; i++ { | ||
| go func(idx int) { | ||
| defer wg.Done() | ||
| results[idx], errors[idx] = config.Call(pipelineMock, context.Background()) | ||
| }(i) | ||
| } | ||
|
|
||
| wg.Wait() | ||
|
|
||
| for i, err := range errors { | ||
| assert.NilError(t, err, "goroutine %d returned error", i) | ||
| } | ||
|
|
||
| for i, res := range results { | ||
| assert.Assert(t, res != nil, "goroutine %d returned nil result", i) | ||
| } | ||
|
|
||
| actual := callCount.Load() | ||
| assert.Assert(t, actual == 1, "expected 1 HTTP call, got %d (singleflight did not coalesce)", actual) | ||
|
Comment on lines
+55
to
+82
There was a problem hiding this comment. 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift Make the tests force overlapping cache misses. These tests can pass if Use a barrier-controlled cache miss. Test two keys through one Also applies to: 110-146, 186-192 🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
| func TestMetadataSingleflight_DifferentKeysDoNotCoalesce(t *testing.T) { | ||
| var callCount atomic.Int32 | ||
|
|
||
| const testHost2 = "127.0.0.1:9019" | ||
| server := httptest.NewHttpServerMock(testHost2, map[string]httptest.HttpServerMockResponseFunc{ | ||
| "/metadata": func() httptest.HttpServerMockResponse { | ||
| callCount.Add(1) | ||
| return httptest.HttpServerMockResponse{ | ||
| Status: 200, | ||
| Headers: map[string]string{"Content-Type": "application/json"}, | ||
| Body: `{"valid":true}`, | ||
| } | ||
| }, | ||
| }) | ||
| defer server.Close() | ||
|
|
||
| ctrl := gomock.NewController(t) | ||
| defer ctrl.Finish() | ||
|
|
||
| evaluator := &metadata.GenericHttp{ | ||
| Endpoint: fmt.Sprintf("http://%s/metadata", testHost2), | ||
| Method: "GET", | ||
| AuthCredentials: auth.NewAuthCredential("", "authorization_header"), | ||
| } | ||
|
|
||
| cacheA := NewEvaluatorCache(&json.JSONValue{Static: "key-A"}, 60) | ||
| cacheB := NewEvaluatorCache(&json.JSONValue{Static: "key-B"}, 60) | ||
|
|
||
| configA := MetadataConfig{ | ||
| Name: "test-metadata", | ||
| Cache: cacheA, | ||
| GenericHTTP: evaluator, | ||
| } | ||
| configB := MetadataConfig{ | ||
| Name: "test-metadata", | ||
| Cache: cacheB, | ||
| GenericHTTP: evaluator, | ||
| } | ||
| defer configA.Clean(context.Background()) | ||
| defer configB.Clean(context.Background()) | ||
|
|
||
| pipelineMockA := mock_auth.NewMockAuthPipeline(ctrl) | ||
| pipelineMockA.EXPECT().GetAuthorizationJSON().AnyTimes().Return(`{}`) | ||
| pipelineMockB := mock_auth.NewMockAuthPipeline(ctrl) | ||
| pipelineMockB.EXPECT().GetAuthorizationJSON().AnyTimes().Return(`{}`) | ||
|
|
||
| var wg sync.WaitGroup | ||
| wg.Add(2) | ||
|
|
||
| go func() { | ||
| defer wg.Done() | ||
| configA.Call(pipelineMockA, context.Background()) | ||
| }() | ||
| go func() { | ||
| defer wg.Done() | ||
| configB.Call(pipelineMockB, context.Background()) | ||
| }() | ||
|
|
||
| wg.Wait() | ||
|
|
||
| actual := callCount.Load() | ||
| assert.Assert(t, actual == 2, "expected 2 HTTP calls (one per distinct cache), got %d", actual) | ||
| } | ||
|
|
||
| func TestMetadataSingleflight_CachePopulatedAfterCoalescedCall(t *testing.T) { | ||
| var callCount atomic.Int32 | ||
|
|
||
| const testHost3 = "127.0.0.1:9020" | ||
| server := httptest.NewHttpServerMock(testHost3, map[string]httptest.HttpServerMockResponseFunc{ | ||
| "/metadata": func() httptest.HttpServerMockResponse { | ||
| callCount.Add(1) | ||
| return httptest.HttpServerMockResponse{ | ||
| Status: 200, | ||
| Headers: map[string]string{"Content-Type": "application/json"}, | ||
| Body: `{"cached":"value"}`, | ||
| } | ||
| }, | ||
| }) | ||
| defer server.Close() | ||
|
|
||
| ctrl := gomock.NewController(t) | ||
| defer ctrl.Finish() | ||
|
|
||
| evaluator := &metadata.GenericHttp{ | ||
| Endpoint: fmt.Sprintf("http://%s/metadata", testHost3), | ||
| Method: "GET", | ||
| AuthCredentials: auth.NewAuthCredential("", "authorization_header"), | ||
| } | ||
|
|
||
| cache := NewEvaluatorCache(&json.JSONValue{Static: "populate-key"}, 60) | ||
|
|
||
| config := MetadataConfig{ | ||
| Name: "test-metadata-populate", | ||
| Cache: cache, | ||
| GenericHTTP: evaluator, | ||
| } | ||
| defer config.Clean(context.Background()) | ||
|
|
||
| pipelineMock := mock_auth.NewMockAuthPipeline(ctrl) | ||
| pipelineMock.EXPECT().GetAuthorizationJSON().AnyTimes().Return(`{}`) | ||
|
|
||
| _, err := config.Call(pipelineMock, context.Background()) | ||
| assert.NilError(t, err) | ||
| assert.Assert(t, callCount.Load() == 1, "first call should invoke evaluator") | ||
|
|
||
| _, err = config.Call(pipelineMock, context.Background()) | ||
| assert.NilError(t, err) | ||
| assert.Assert(t, callCount.Load() == 1, "second call should hit cache, not invoke evaluator again") | ||
| } | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
Repository: Kuadrant/authorino
Length of output: 2395
🏁 Script executed:
Repository: Kuadrant/authorino
Length of output: 20298
🏁 Script executed:
Repository: Kuadrant/authorino
Length of output: 7291
🌐 Web query:
golang.org/x/sync/singleflight Group Do Wait duplicate calls context cancellation💡 Result:
The standard golang.org/x/sync/singleflight package does not provide native, integrated support for context cancellation within its Group.Do method [1][2]. Key behavior regarding duplicate calls and context: 1. Blocking Nature: The Do method is synchronous and blocks until the primary execution finishes [3][4]. If a caller's context is cancelled while it is waiting for a result, the Do method itself does not observe or react to that cancellation; the caller will remain blocked until the underlying function completes or panics [1][5]. 2. Risk of Propagation: If the function passed to Do uses a context derived from one of the callers, the cancellation of that specific caller can cause the entire shared operation to fail for all other waiting callers [1][5]. It is a common best practice to use context.WithoutCancel(ctx) or a background context to detach the shared work from the lifetime of any individual request [1][5]. 3. Alternatives for Cancellation: - DoChan: If you need to respect context cancellation for the waiting period, you can use DoChan [1][6]. This returns a channel, allowing you to use a select statement to wait for either the result or the cancellation of your context [1]: ch:= group.DoChan(key, fn) select { case <-ctx.Done: return nil, ctx.Err case res:= <-ch: return res.Val, res.Err } - External Libraries: If your application requires more sophisticated, context-aware duplicate suppression (e.g., where the underlying work's context is tied to the collective state of waiting callers), consider third-party implementations like resenje.org/singleflight, which are designed to handle context propagation and cancellation more gracefully [7][8][9].
Citations:
Preserve each caller context while joining a flight.
singleflight.Group.Dodoes not accept a context, so callers wait unconditionally while an in-flightevaluator.Callcan be tied to the first caller’s request context. Use a context-aware flight path and a shared evaluation context with an explicit timeout policy in both:pkg/evaluators/authorization.go:77-92pkg/evaluators/metadata.go:73-88📍 Affects 2 files
pkg/evaluators/authorization.go#L77-L92(this comment)pkg/evaluators/metadata.go#L73-L88🤖 Prompt for AI Agents