From a286a060ad190462a51b09290ef1656d37a3bbb8 Mon Sep 17 00:00:00 2001 From: George MacRorie Date: Tue, 11 Jul 2023 12:15:55 +0100 Subject: [PATCH 1/2] feat(middleware): support caching evaluation.VariantEvaluationResponse --- internal/server/middleware/grpc/middleware.go | 54 ++++++- .../server/middleware/grpc/middleware_test.go | 151 ++++++++++++++++++ 2 files changed, 204 insertions(+), 1 deletion(-) diff --git a/internal/server/middleware/grpc/middleware.go b/internal/server/middleware/grpc/middleware.go index 624548e472..82a9cc0c0b 100644 --- a/internal/server/middleware/grpc/middleware.go +++ b/internal/server/middleware/grpc/middleware.go @@ -16,6 +16,7 @@ import ( "go.flipt.io/flipt/internal/server/metrics" flipt "go.flipt.io/flipt/rpc/flipt" fauth "go.flipt.io/flipt/rpc/flipt/auth" + "go.flipt.io/flipt/rpc/flipt/evaluation" "go.opentelemetry.io/otel/trace" "go.uber.org/zap" "google.golang.org/grpc" @@ -227,6 +228,50 @@ func CacheUnaryInterceptor(cache cache.Cacher, logger *zap.Logger) grpc.UnarySer if err := cache.Delete(ctx, flagCacheKey(keyer.GetNamespaceKey(), keyer.GetFlagKey())); err != nil { logger.Error("deleting from cache", zap.Error(err)) } + case *evaluation.EvaluationRequest: + key, err := evaluationCacheKey(r) + if err != nil { + logger.Error("getting cache key", zap.Error(err)) + return handler(ctx, req) + } + + cached, ok, err := cache.Get(ctx, key) + if err != nil { + // if error, log and without cache + logger.Error("getting from cache", zap.Error(err)) + return handler(ctx, req) + } + + if ok { + resp := &evaluation.VariantEvaluationResponse{} + if err := proto.Unmarshal(cached, resp); err != nil { + logger.Error("unmarshalling from cache", zap.Error(err)) + return handler(ctx, req) + } + + logger.Debug("evaluate cache hit", zap.Stringer("response", resp)) + return resp, nil + } + + logger.Debug("evaluate cache miss") + resp, err := handler(ctx, req) + if err != nil { + return resp, err + } + + // marshal response + data, merr := proto.Marshal(resp.(*evaluation.VariantEvaluationResponse)) + if merr != nil { + logger.Error("marshalling for cache", zap.Error(err)) + return resp, err + } + + // set in cache + if cerr := cache.Set(ctx, key, data); cerr != nil { + logger.Error("setting in cache", zap.Error(err)) + } + + return resp, err } return handler(ctx, req) @@ -348,7 +393,14 @@ func flagCacheKey(namespaceKey, key string) string { return fmt.Sprintf("flipt:%x", md5.Sum([]byte(k))) } -func evaluationCacheKey(r *flipt.EvaluationRequest) (string, error) { +type evaluationRequest interface { + GetNamespaceKey() string + GetFlagKey() string + GetEntityId() string + GetContext() map[string]string +} + +func evaluationCacheKey(r evaluationRequest) (string, error) { out, err := json.Marshal(r.GetContext()) if err != nil { return "", fmt.Errorf("marshalling req to json: %w", err) diff --git a/internal/server/middleware/grpc/middleware_test.go b/internal/server/middleware/grpc/middleware_test.go index 1356497c37..8ff1832778 100644 --- a/internal/server/middleware/grpc/middleware_test.go +++ b/internal/server/middleware/grpc/middleware_test.go @@ -12,6 +12,7 @@ import ( "go.flipt.io/flipt/internal/server/auth" "go.flipt.io/flipt/internal/server/auth/method/token" "go.flipt.io/flipt/internal/server/cache/memory" + servereval "go.flipt.io/flipt/internal/server/evaluation" "go.flipt.io/flipt/internal/storage" flipt "go.flipt.io/flipt/rpc/flipt" sdktrace "go.opentelemetry.io/otel/sdk/trace" @@ -770,6 +771,156 @@ func TestCacheUnaryInterceptor_Evaluate(t *testing.T) { } } +func TestCacheUnaryInterceptor_Evaluation_Variant(t *testing.T) { + var ( + store = &storeMock{} + cache = memory.NewCache(config.CacheConfig{ + TTL: time.Second, + Enabled: true, + Backend: config.CacheMemory, + }) + cacheSpy = newCacheSpy(cache) + logger = zaptest.NewLogger(t) + s = servereval.New(logger, store) + ) + + store.On("GetFlag", mock.Anything, mock.Anything, "foo").Return(&flipt.Flag{ + Key: "foo", + Enabled: true, + }, nil) + + store.On("GetEvaluationRules", mock.Anything, mock.Anything, "foo").Return( + []*storage.EvaluationRule{ + { + ID: "1", + FlagKey: "foo", + SegmentKey: "bar", + SegmentMatchType: flipt.MatchType_ALL_MATCH_TYPE, + Rank: 0, + Constraints: []storage.EvaluationConstraint{ + // constraint: bar (string) == baz + { + ID: "2", + Type: flipt.ComparisonType_STRING_COMPARISON_TYPE, + Property: "bar", + Operator: flipt.OpEQ, + Value: "baz", + }, + // constraint: admin (bool) == true + { + ID: "3", + Type: flipt.ComparisonType_BOOLEAN_COMPARISON_TYPE, + Property: "admin", + Operator: flipt.OpTrue, + }, + }, + }, + }, nil) + + store.On("GetEvaluationDistributions", mock.Anything, "1").Return( + []*storage.EvaluationDistribution{ + { + ID: "4", + RuleID: "1", + VariantID: "5", + Rollout: 100, + VariantKey: "boz", + VariantAttachment: `{"key":"value"}`, + }, + }, nil) + + tests := []struct { + name string + req *evaluation.EvaluationRequest + wantMatch bool + }{ + { + name: "matches all", + req: &evaluation.EvaluationRequest{ + FlagKey: "foo", + EntityId: "1", + Context: map[string]string{ + "bar": "baz", + "admin": "true", + }, + }, + wantMatch: true, + }, + { + name: "no match all", + req: &evaluation.EvaluationRequest{ + FlagKey: "foo", + EntityId: "1", + Context: map[string]string{ + "bar": "boz", + "admin": "true", + }, + }, + }, + { + name: "no match just bool value", + req: &evaluation.EvaluationRequest{ + FlagKey: "foo", + EntityId: "1", + Context: map[string]string{ + "admin": "true", + }, + }, + }, + { + name: "no match just string value", + req: &evaluation.EvaluationRequest{ + FlagKey: "foo", + EntityId: "1", + Context: map[string]string{ + "bar": "baz", + }, + }, + }, + } + + unaryInterceptor := CacheUnaryInterceptor(cacheSpy, logger) + + handler := func(ctx context.Context, r interface{}) (interface{}, error) { + return s.Variant(ctx, r.(*evaluation.EvaluationRequest)) + } + + info := &grpc.UnaryServerInfo{ + FullMethod: "FakeMethod", + } + + for i, tt := range tests { + var ( + i = i + 1 + req = tt.req + wantMatch = tt.wantMatch + ) + + t.Run(tt.name, func(t *testing.T) { + got, err := unaryInterceptor(context.Background(), req, info, handler) + require.NoError(t, err) + assert.NotNil(t, got) + + resp := got.(*evaluation.VariantEvaluationResponse) + assert.NotNil(t, resp) + + assert.Equal(t, i, cacheSpy.getCalled, "cache get wasn't called as expected") + assert.NotEmpty(t, cacheSpy.getKeys, "cache keys should not be empty") + + if !wantMatch { + assert.False(t, resp.Match) + assert.Empty(t, resp.SegmentKey) + return + } + + assert.True(t, resp.Match) + assert.Equal(t, "bar", resp.SegmentKey) + assert.Equal(t, "boz", resp.VariantKey) + assert.Equal(t, `{"key":"value"}`, resp.VariantAttachment) + }) + } +} + func TestAuditUnaryInterceptor_CreateFlag(t *testing.T) { var ( store = &storeMock{} From a6f5d7db15000a0a398c1c4d4a59c7d21b12626c Mon Sep 17 00:00:00 2001 From: George MacRorie Date: Tue, 11 Jul 2023 12:34:04 +0100 Subject: [PATCH 2/2] feat(middleware): support caching evaluation.BooleanEvaluationResponse --- internal/server/middleware/grpc/middleware.go | 29 +++- .../server/middleware/grpc/middleware_test.go | 139 ++++++++++++++++++ 2 files changed, 165 insertions(+), 3 deletions(-) diff --git a/internal/server/middleware/grpc/middleware.go b/internal/server/middleware/grpc/middleware.go index 82a9cc0c0b..f66cf10c60 100644 --- a/internal/server/middleware/grpc/middleware.go +++ b/internal/server/middleware/grpc/middleware.go @@ -243,14 +243,23 @@ func CacheUnaryInterceptor(cache cache.Cacher, logger *zap.Logger) grpc.UnarySer } if ok { - resp := &evaluation.VariantEvaluationResponse{} + resp := &evaluation.EvaluationResponse{} if err := proto.Unmarshal(cached, resp); err != nil { logger.Error("unmarshalling from cache", zap.Error(err)) return handler(ctx, req) } logger.Debug("evaluate cache hit", zap.Stringer("response", resp)) - return resp, nil + switch r := resp.Response.(type) { + case *evaluation.EvaluationResponse_VariantResponse: + return r.VariantResponse, nil + case *evaluation.EvaluationResponse_BooleanResponse: + return r.BooleanResponse, nil + default: + logger.Error("unexpected eval cache response type", zap.String("type", fmt.Sprintf("%T", resp.Response))) + } + + return handler(ctx, req) } logger.Debug("evaluate cache miss") @@ -259,8 +268,22 @@ func CacheUnaryInterceptor(cache cache.Cacher, logger *zap.Logger) grpc.UnarySer return resp, err } + evalResponse := &evaluation.EvaluationResponse{} + switch r := resp.(type) { + case *evaluation.VariantEvaluationResponse: + evalResponse.Type = evaluation.EvaluationResponseType_VARIANT_EVALUATION_RESPONSE_TYPE + evalResponse.Response = &evaluation.EvaluationResponse_VariantResponse{ + VariantResponse: r, + } + case *evaluation.BooleanEvaluationResponse: + evalResponse.Type = evaluation.EvaluationResponseType_BOOLEAN_EVALUATION_RESPONSE_TYPE + evalResponse.Response = &evaluation.EvaluationResponse_BooleanResponse{ + BooleanResponse: r, + } + } + // marshal response - data, merr := proto.Marshal(resp.(*evaluation.VariantEvaluationResponse)) + data, merr := proto.Marshal(evalResponse) if merr != nil { logger.Error("marshalling for cache", zap.Error(err)) return resp, err diff --git a/internal/server/middleware/grpc/middleware_test.go b/internal/server/middleware/grpc/middleware_test.go index 8ff1832778..62a1bb7742 100644 --- a/internal/server/middleware/grpc/middleware_test.go +++ b/internal/server/middleware/grpc/middleware_test.go @@ -921,6 +921,145 @@ func TestCacheUnaryInterceptor_Evaluation_Variant(t *testing.T) { } } +func TestCacheUnaryInterceptor_Evaluation_Boolean(t *testing.T) { + var ( + store = &storeMock{} + cache = memory.NewCache(config.CacheConfig{ + TTL: time.Second, + Enabled: true, + Backend: config.CacheMemory, + }) + cacheSpy = newCacheSpy(cache) + logger = zaptest.NewLogger(t) + s = servereval.New(logger, store) + ) + + store.On("GetFlag", mock.Anything, mock.Anything, "foo").Return(&flipt.Flag{ + Key: "foo", + Enabled: false, + Type: flipt.FlagType_BOOLEAN_FLAG_TYPE, + }, nil) + + store.On("GetEvaluationRollouts", mock.Anything, mock.Anything, "foo").Return( + []*storage.EvaluationRollout{ + { + RolloutType: flipt.RolloutType_SEGMENT_ROLLOUT_TYPE, + Segment: &storage.RolloutSegment{ + Key: "bar", + MatchType: flipt.MatchType_ALL_MATCH_TYPE, + Constraints: []storage.EvaluationConstraint{ + // constraint: bar (string) == baz + { + ID: "2", + Type: flipt.ComparisonType_STRING_COMPARISON_TYPE, + Property: "bar", + Operator: flipt.OpEQ, + Value: "baz", + }, + // constraint: admin (bool) == true + { + ID: "3", + Type: flipt.ComparisonType_BOOLEAN_COMPARISON_TYPE, + Property: "admin", + Operator: flipt.OpTrue, + }, + }, + Value: true, + }, + Rank: 1, + }, + }, nil) + + tests := []struct { + name string + req *evaluation.EvaluationRequest + wantMatch bool + }{ + { + name: "matches all", + req: &evaluation.EvaluationRequest{ + FlagKey: "foo", + EntityId: "1", + Context: map[string]string{ + "bar": "baz", + "admin": "true", + }, + }, + wantMatch: true, + }, + { + name: "no match all", + req: &evaluation.EvaluationRequest{ + FlagKey: "foo", + EntityId: "1", + Context: map[string]string{ + "bar": "boz", + "admin": "true", + }, + }, + }, + { + name: "no match just bool value", + req: &evaluation.EvaluationRequest{ + FlagKey: "foo", + EntityId: "1", + Context: map[string]string{ + "admin": "true", + }, + }, + }, + { + name: "no match just string value", + req: &evaluation.EvaluationRequest{ + FlagKey: "foo", + EntityId: "1", + Context: map[string]string{ + "bar": "baz", + }, + }, + }, + } + + unaryInterceptor := CacheUnaryInterceptor(cacheSpy, logger) + + handler := func(ctx context.Context, r interface{}) (interface{}, error) { + return s.Boolean(ctx, r.(*evaluation.EvaluationRequest)) + } + + info := &grpc.UnaryServerInfo{ + FullMethod: "FakeMethod", + } + + for i, tt := range tests { + var ( + i = i + 1 + req = tt.req + wantMatch = tt.wantMatch + ) + + t.Run(tt.name, func(t *testing.T) { + got, err := unaryInterceptor(context.Background(), req, info, handler) + require.NoError(t, err) + assert.NotNil(t, got) + + resp := got.(*evaluation.BooleanEvaluationResponse) + assert.NotNil(t, resp) + + assert.Equal(t, i, cacheSpy.getCalled, "cache get wasn't called as expected") + assert.NotEmpty(t, cacheSpy.getKeys, "cache keys should not be empty") + + if !wantMatch { + assert.False(t, resp.Value) + assert.Equal(t, evaluation.EvaluationReason_DEFAULT_EVALUATION_REASON, resp.Reason) + return + } + + assert.True(t, resp.Value) + assert.Equal(t, evaluation.EvaluationReason_MATCH_EVALUATION_REASON, resp.Reason) + }) + } +} + func TestAuditUnaryInterceptor_CreateFlag(t *testing.T) { var ( store = &storeMock{}