diff --git a/pkg/services/orgresolver/linking.go b/pkg/services/orgresolver/linking.go index b5ff09b0de..a4868f0c2b 100644 --- a/pkg/services/orgresolver/linking.go +++ b/pkg/services/orgresolver/linking.go @@ -2,10 +2,12 @@ package orgresolver import ( "context" + "database/sql" "errors" "fmt" "time" + "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/metric" "google.golang.org/grpc" "google.golang.org/grpc/credentials" @@ -32,6 +34,13 @@ type OrgResolver interface { Get(ctx context.Context, owner string) (string, error) } +type CacheStore interface { + // GetOrg returns the cached orgID for owner, or error if absent. + GetOrg(ctx context.Context, owner string) (string, error) + // UpsertOrg stores the owner->orgID mapping. + UpsertOrg(ctx context.Context, owner, orgID string) error +} + type Config struct { URL string TLSEnabled bool @@ -43,6 +52,11 @@ type Config struct { Client linkingclient.LinkingServiceClient // optional Meter metric.Meter // optional + + // CacheEnabled turns on durable caching of owner->orgID mappings via CacheStore. + CacheEnabled bool + // CacheStore is required when CacheEnabled is true. + CacheStore CacheStore } // orgResolver makes direct calls to the linking service to resolve organization IDs from workflow owners. @@ -57,10 +71,26 @@ type orgResolver struct { jwtGenerator JWTGenerator requestTimeout time.Duration - passCount metric.Int64Counter - failCount metric.Int64Counter + cacheEnabled bool + cacheStore CacheStore + + passCount metric.Int64Counter + failCount metric.Int64Counter + cacheLookups metric.Int64Counter // tagged with result=hit|miss|error } +const ( + cacheResultAttrName = "result" + cacheOwnerAttrName = "owner" + cacheResultHit = "hit" + cacheResultMiss = "miss" + cacheResultError = "error" +) + +// ErrCacheMiss is returned by CacheStore.GetOrg when no mapping exists for owner. +// Stores backed by SQL may return sql.ErrNoRows; both are treated as a miss. +var ErrCacheMiss = errors.New("org not found in cache") + // NewOrgResolver creates a new org resolver with the specified configuration // Deprecated: Use Config.New // @@ -84,12 +114,18 @@ func (cfg *Config) New(logger log.Logger) (*orgResolver, error) { requestTimeout = defaultRequestTimeout } + if cfg.CacheEnabled && cfg.CacheStore == nil { + return nil, errors.New("CacheStore is required when CacheEnabled is true") + } + resolver := &orgResolver{ workflowRegistryAddress: cfg.WorkflowRegistryAddress, workflowRegistryChainSelector: cfg.WorkflowRegistryChainSelector, logger: log.Sugared(logger).Named("OrgResolver"), jwtGenerator: cfg.JWTGenerator, requestTimeout: requestTimeout, + cacheEnabled: cfg.CacheEnabled, + cacheStore: cfg.CacheStore, } if cfg.Client != nil { @@ -125,6 +161,12 @@ func (cfg *Config) New(logger log.Logger) (*orgResolver, error) { if err != nil { return nil, fmt.Errorf("failed to create failure count metric: %w", err) } + if resolver.cacheEnabled { + resolver.cacheLookups, err = cfg.Meter.Int64Counter("org_resolver_cache_lookups") + if err != nil { + return nil, fmt.Errorf("failed to create cache lookups metric: %w", err) + } + } } return resolver, nil @@ -148,6 +190,12 @@ func (o *orgResolver) addJWTAuth(ctx context.Context, req any) (context.Context, } func (o *orgResolver) Get(ctx context.Context, owner string) (string, error) { + if o.cacheEnabled { + if orgID, ok := o.checkCache(ctx, owner); ok { + return orgID, nil + } + } + ctx, cancel := context.WithTimeout(ctx, o.requestTimeout) defer cancel() @@ -174,9 +222,45 @@ func (o *orgResolver) Get(ctx context.Context, owner string) (string, error) { if o.passCount != nil { o.passCount.Add(ctx, 1) } + + if o.cacheEnabled { + o.storeInCache(ctx, owner, resp.OrganizationId) + } return resp.OrganizationId, nil } +// checkCache looks up owner in the durable cache. Returns (orgID, true) on hit. +// A cache store error is logged and treated as a miss so lookups remain resilient. +func (o *orgResolver) checkCache(ctx context.Context, owner string) (string, bool) { + orgID, err := o.cacheStore.GetOrg(ctx, owner) + if err != nil { + if errors.Is(err, ErrCacheMiss) || errors.Is(err, sql.ErrNoRows) { + o.recordCacheLookup(ctx, cacheResultMiss, owner) + } else { + o.logger.Warnw("Failed to read org from cache store, falling back to linking service", "owner", owner, "error", err) + o.recordCacheLookup(ctx, cacheResultError, owner) + } + return "", false + } + o.recordCacheLookup(ctx, cacheResultHit, owner) + return orgID, true +} + +func (o *orgResolver) storeInCache(ctx context.Context, owner, orgID string) { + if err := o.cacheStore.UpsertOrg(ctx, owner, orgID); err != nil { + o.logger.Warnw("Failed to persist org to cache store", "owner", owner, "error", err) + } +} + +func (o *orgResolver) recordCacheLookup(ctx context.Context, result, owner string) { + if o.cacheLookups != nil { + o.cacheLookups.Add(ctx, 1, metric.WithAttributes( + attribute.String(cacheResultAttrName, result), + attribute.String(cacheOwnerAttrName, owner), + )) + } +} + func (o *orgResolver) Start(_ context.Context) error { return nil } diff --git a/pkg/services/orgresolver/linking_test.go b/pkg/services/orgresolver/linking_test.go index 9def87e270..0c5ea9dd53 100644 --- a/pkg/services/orgresolver/linking_test.go +++ b/pkg/services/orgresolver/linking_test.go @@ -2,8 +2,10 @@ package orgresolver import ( "context" + "database/sql" "errors" "net" + "sync" "testing" "time" @@ -275,3 +277,279 @@ func TestOrgResolver_Get_WithoutJWTGenerator(t *testing.T) { // Verify that no authorization header was set require.Empty(t, client.receivedAuthHeader) } + +// mockCacheStore implements the CacheStore interface for testing. +type mockCacheStore struct { + data map[string]string + getErr error // error to return from GetOrg (overrides data lookup) + upsertErr error // error to return from UpsertOrg + getCalls int + upsertCalls int + lastUpsertKey string + lastUpsertVal string + mu sync.Mutex +} + +func newMockCacheStore() *mockCacheStore { + return &mockCacheStore{data: make(map[string]string)} +} + +func (m *mockCacheStore) GetOrg(_ context.Context, owner string) (string, error) { + m.mu.Lock() + defer m.mu.Unlock() + m.getCalls++ + if m.getErr != nil { + return "", m.getErr + } + orgID, ok := m.data[owner] + if !ok { + return "", ErrCacheMiss + } + return orgID, nil +} + +func (m *mockCacheStore) UpsertOrg(_ context.Context, owner, orgID string) error { + m.mu.Lock() + defer m.mu.Unlock() + m.upsertCalls++ + m.lastUpsertKey = owner + m.lastUpsertVal = orgID + if m.upsertErr != nil { + return m.upsertErr + } + m.data[owner] = orgID + return nil +} + +func TestOrgResolver_Cache_HitSkipsLinkingService(t *testing.T) { + ctx := context.Background() + client := &mockLinkingClient{} + cache := newMockCacheStore() + + // Pre-populate the cache with a known mapping. + cachedOrgID := "cached-org-123" + workflowOwner := "0xabcdef1234567890" + cache.data[workflowOwner] = cachedOrgID + + cfg := Config{ + URL: "test-url", + WorkflowRegistryAddress: "0x1234567890abcdef", + WorkflowRegistryChainSelector: 1, + CacheEnabled: true, + CacheStore: cache, + Client: client, + } + + resolver, err := cfg.New(logger.Test(t)) + require.NoError(t, err) + + orgID, err := resolver.Get(ctx, workflowOwner) + require.NoError(t, err) + require.Equal(t, cachedOrgID, orgID) + + // The linking service client must not be invoked on a cache hit; verify + // indirectly by checking that no upsert happened (which only occurs after + // a successful remote fetch). + require.Equal(t, 1, cache.getCalls) + require.Equal(t, 0, cache.upsertCalls) +} + +func TestOrgResolver_Cache_MissFallsBackToLinkingServiceAndStores(t *testing.T) { + ctx := context.Background() + client := &mockLinkingClient{} + cache := newMockCacheStore() + + workflowOwner := "0xabcdef1234567890" + + cfg := Config{ + URL: "test-url", + WorkflowRegistryAddress: "0x1234567890abcdef", + WorkflowRegistryChainSelector: 1, + CacheEnabled: true, + CacheStore: cache, + Client: client, + } + + resolver, err := cfg.New(logger.Test(t)) + require.NoError(t, err) + + orgID, err := resolver.Get(ctx, workflowOwner) + require.NoError(t, err) + require.Equal(t, "org-"+workflowOwner, orgID) + + // Cache miss should trigger a linking-service fetch, and the result should + // be persisted to the cache store. + require.Equal(t, 1, cache.getCalls) + require.Equal(t, 1, cache.upsertCalls) + require.Equal(t, workflowOwner, cache.lastUpsertKey) + require.Equal(t, "org-"+workflowOwner, cache.lastUpsertVal) + require.Equal(t, "org-"+workflowOwner, cache.data[workflowOwner]) +} + +func TestOrgResolver_Cache_HitOnSecondCall(t *testing.T) { + ctx := context.Background() + client := &mockLinkingClient{} + cache := newMockCacheStore() + + workflowOwner := "0xabcdef1234567890" + + cfg := Config{ + URL: "test-url", + WorkflowRegistryAddress: "0x1234567890abcdef", + WorkflowRegistryChainSelector: 1, + CacheEnabled: true, + CacheStore: cache, + Client: client, + } + + resolver, err := cfg.New(logger.Test(t)) + require.NoError(t, err) + + // First call: cache miss -> fetches from linking service and stores. + orgID1, err := resolver.Get(ctx, workflowOwner) + require.NoError(t, err) + require.Equal(t, "org-"+workflowOwner, orgID1) + require.Equal(t, 1, cache.upsertCalls) + + // Second call: should hit the cache that was populated on the first call. + orgID2, err := resolver.Get(ctx, workflowOwner) + require.NoError(t, err) + require.Equal(t, "org-"+workflowOwner, orgID2) + + // Only one upsert should have happened (on the first call). + require.Equal(t, 1, cache.upsertCalls) + require.Equal(t, 2, cache.getCalls) +} + +func TestOrgResolver_Cache_ErrorFallsBackToLinkingService(t *testing.T) { + ctx := context.Background() + client := &mockLinkingClient{} + cache := newMockCacheStore() + cache.getErr = errors.New("cache store unavailable") + + workflowOwner := "0xabcdef1234567890" + + cfg := Config{ + URL: "test-url", + WorkflowRegistryAddress: "0x1234567890abcdef", + WorkflowRegistryChainSelector: 1, + CacheEnabled: true, + CacheStore: cache, + Client: client, + } + + resolver, err := cfg.New(logger.Test(t)) + require.NoError(t, err) + + // A cache store error should not prevent resolution; it falls back to the + // linking service. + orgID, err := resolver.Get(ctx, workflowOwner) + require.NoError(t, err) + require.Equal(t, "org-"+workflowOwner, orgID) + + require.Equal(t, 1, cache.getCalls) + require.Equal(t, 1, cache.upsertCalls) +} + +func TestOrgResolver_Cache_SQLErrNoRowsTreatedAsMiss(t *testing.T) { + ctx := context.Background() + client := &mockLinkingClient{} + cache := newMockCacheStore() + cache.getErr = sql.ErrNoRows + + workflowOwner := "0xabcdef1234567890" + + cfg := Config{ + URL: "test-url", + WorkflowRegistryAddress: "0x1234567890abcdef", + WorkflowRegistryChainSelector: 1, + CacheEnabled: true, + CacheStore: cache, + Client: client, + } + + resolver, err := cfg.New(logger.Test(t)) + require.NoError(t, err) + + orgID, err := resolver.Get(ctx, workflowOwner) + require.NoError(t, err) + require.Equal(t, "org-"+workflowOwner, orgID) + + // sql.ErrNoRows is treated as a miss, so the linking service is consulted + // and the result is stored. + require.Equal(t, 1, cache.upsertCalls) +} + +func TestOrgResolver_Cache_UpsertErrorDoesNotFailGet(t *testing.T) { + ctx := context.Background() + client := &mockLinkingClient{} + cache := newMockCacheStore() + cache.upsertErr = errors.New("cache write failed") + + workflowOwner := "0xabcdef1234567890" + + cfg := Config{ + URL: "test-url", + WorkflowRegistryAddress: "0x1234567890abcdef", + WorkflowRegistryChainSelector: 1, + CacheEnabled: true, + CacheStore: cache, + Client: client, + } + + resolver, err := cfg.New(logger.Test(t)) + require.NoError(t, err) + + // A cache write failure should be logged but must not cause Get to fail. + orgID, err := resolver.Get(ctx, workflowOwner) + require.NoError(t, err) + require.Equal(t, "org-"+workflowOwner, orgID) + + require.Equal(t, 1, cache.upsertCalls) + // The data should not have been written due to the upsert error. + _, exists := cache.data[workflowOwner] + require.False(t, exists) +} + +func TestOrgResolver_Cache_RequiresCacheStoreWhenEnabled(t *testing.T) { + cfg := Config{ + URL: "test-url", + WorkflowRegistryAddress: "0x1234567890abcdef", + WorkflowRegistryChainSelector: 1, + CacheEnabled: true, + CacheStore: nil, // missing store + Client: &mockLinkingClient{}, + } + + _, err := cfg.New(logger.Test(t)) + require.Error(t, err) + require.Contains(t, err.Error(), "CacheStore is required when CacheEnabled is true") +} + +func TestOrgResolver_Cache_DisabledDoesNotUseCacheStore(t *testing.T) { + ctx := context.Background() + client := &mockLinkingClient{} + cache := newMockCacheStore() + + workflowOwner := "0xabcdef1234567890" + + cfg := Config{ + URL: "test-url", + WorkflowRegistryAddress: "0x1234567890abcdef", + WorkflowRegistryChainSelector: 1, + CacheEnabled: false, + CacheStore: cache, // provided but disabled + Client: client, + } + + resolver, err := cfg.New(logger.Test(t)) + require.NoError(t, err) + + orgID, err := resolver.Get(ctx, workflowOwner) + require.NoError(t, err) + require.Equal(t, "org-"+workflowOwner, orgID) + + // With caching disabled, neither GetOrg nor UpsertOrg should be called. + require.Equal(t, 0, cache.getCalls) + require.Equal(t, 0, cache.upsertCalls) +}