diff --git a/.changeset/cache-consumers-without-secret.md b/.changeset/cache-consumers-without-secret.md new file mode 100644 index 0000000..1d493f0 --- /dev/null +++ b/.changeset/cache-consumers-without-secret.md @@ -0,0 +1,5 @@ +--- +"posthog-ruby": patch +--- + +Allow external flag definition cache consumers to evaluate flags locally and refresh cached definitions without a secret key. diff --git a/examples/redis_flag_cache.rb b/examples/redis_flag_cache.rb index 35cde0b..41da94c 100644 --- a/examples/redis_flag_cache.rb +++ b/examples/redis_flag_cache.rb @@ -19,6 +19,10 @@ # flag_definition_cache_provider: cache # ) # +# For cache-only readers, omit secret_key and use the same provider. The SDK +# reads cached definitions without calling the leader-election method. Keep +# at least one worker with secret_key configured to publish fresh definitions. +# # Requirements: # gem install redis @@ -33,8 +37,8 @@ # from making N redundant API calls. # # Uses leader election: -# - One instance "wins" and becomes responsible for fetching -# - Other instances read from the shared cache +# - One instance with secret_key "wins" and becomes responsible for fetching +# - Other instances read from the shared cache; workers without secret_key do not compete # - If the leader dies, the lock expires (TTL) and another instance takes over # # Uses Lua scripts for atomic operations, following Redis distributed lock @@ -90,7 +94,8 @@ def flag_definitions # Determine if this instance should fetch flag definitions from PostHog. # - # Atomically either acquires the lock (if free) or extends it (if we own it). + # Called by the SDK only for workers with secret_key configured. Atomically + # either acquires the lock (if free) or extends it (if we own it). # # @return [Boolean] true if this instance is the leader and should fetch def should_fetch_flag_definitions? diff --git a/lib/posthog/client.rb b/lib/posthog/client.rb index 17eae40..0707a48 100644 --- a/lib/posthog/client.rb +++ b/lib/posthog/client.rb @@ -85,9 +85,9 @@ def _decrement_instance_count(api_key) # @param opts [Hash] Client configuration. # @option opts [String, nil] :api_key Your project's API key. Missing or blank values disable the client. - # @option opts [String, nil] :secret_key The credential used for local feature flag evaluation and remote - # config. Accepts either a Personal API Key (`phx_...`) or a Project Secret API Key (`phs_...`). Required - # for local feature flag evaluation. + # @option opts [String, nil] :secret_key The credential used to fetch local feature flag definitions and remote + # config. Accepts either a Personal API Key (`phx_...`) or a Project Secret API Key (`phs_...`). Not required + # for local evaluation when definitions are read from +:flag_definition_cache_provider+. # @option opts [String, nil] :personal_api_key # @deprecated Use +:secret_key+ instead. Retained as an alias; when both are supplied, +:secret_key+ wins. # @option opts [String] :host Fully qualified hostname of the PostHog server. Defaults to `https://us.i.posthog.com`. @@ -180,6 +180,7 @@ def initialize(opts = {}) @feature_flags_poller = nil @secret_key = secret_key @personal_api_key = secret_key + @flag_definition_cache_provider = opts[:flag_definition_cache_provider] if @disabled && !opts[:silence_disabled_client_error] logger.error('api_key is missing or empty after trimming whitespace; check your project API key') @@ -214,7 +215,7 @@ def initialize(opts = {}) opts[:host], opts[:feature_flag_request_timeout_seconds] || Defaults::FeatureFlags::FLAG_REQUEST_TIMEOUT_SECONDS, opts[:on_error], - flag_definition_cache_provider: opts[:flag_definition_cache_provider], + flag_definition_cache_provider: @flag_definition_cache_provider, feature_flag_request_max_retries: opts[:feature_flag_request_max_retries], async_load: opts[:feature_flags_async_load] == true, user_agent: @headers['User-Agent'], @@ -883,9 +884,9 @@ def get_all_flags_and_payloads( def reload_feature_flags return if @disabled - unless @secret_key + unless @secret_key || @flag_definition_cache_provider logger.error( - 'You need to specify a secret_key to locally evaluate feature flags' + 'You need to specify a secret_key or flag_definition_cache_provider to locally evaluate feature flags' ) return end diff --git a/lib/posthog/feature_flags.rb b/lib/posthog/feature_flags.rb index 97592cf..25a34ce 100644 --- a/lib/posthog/feature_flags.rb +++ b/lib/posthog/feature_flags.rb @@ -100,9 +100,9 @@ def initialize( run_now: @async_load ) { _load_feature_flags } - # If no secret_key, disable local evaluation & thus polling for definitions - if @secret_key.nil? - logger.info 'No secret_key provided, disabling local evaluation' + # Local evaluation can load definitions from either the API or an external cache. + if @secret_key.nil? && @flag_definition_cache_provider.nil? + logger.info 'No secret_key or flag_definition_cache_provider provided, disabling local evaluation' @loaded_flags_successfully_once.make_true else # load once synchronously before timer, unless @async_load @@ -1260,7 +1260,8 @@ def _load_feature_flags if @flag_definition_cache_provider begin - should_fetch = @flag_definition_cache_provider.should_fetch_flag_definitions? + # Cache-only readers must not acquire or renew fetch leadership. + should_fetch = @secret_key && @flag_definition_cache_provider.should_fetch_flag_definitions? rescue StandardError => e logger.error("[FEATURE FLAGS] Cache provider should_fetch error: #{e}") should_fetch = true @@ -1271,6 +1272,10 @@ def _load_feature_flags begin cached_data = @flag_definition_cache_provider.flag_definitions if cached_data + unless cached_data.is_a?(Hash) && get_by_symbol_or_string_key(cached_data, 'flags').is_a?(Array) + raise ArgumentError, 'Cached flag definitions must contain a flags array' + end + logger.debug '[FEATURE FLAGS] Using cached flag definitions from external cache' _apply_flag_definitions(cached_data) return @@ -1288,6 +1293,11 @@ def _load_feature_flags end def _fetch_and_apply_flag_definitions + unless @secret_key + logger.warn '[FEATURE FLAGS] A secret_key is required to fetch flag definitions from PostHog' + return + end + begin res = _request_feature_flag_definitions(etag: @flags_etag.value) rescue StandardError => e diff --git a/lib/posthog/flag_definition_cache.rb b/lib/posthog/flag_definition_cache.rb index 9e127d1..ca10b27 100644 --- a/lib/posthog/flag_definition_cache.rb +++ b/lib/posthog/flag_definition_cache.rb @@ -5,7 +5,8 @@ module PostHog # # Enables multi-worker environments (Kubernetes, load-balanced servers, # serverless functions) to share flag definitions via an external cache, - # reducing redundant API calls. + # reducing redundant API calls. Cache consumers need only the project API key; + # +:secret_key+ is required only for workers that fetch definitions from PostHog. # # Implement the four required methods on any object and pass it as the # +:flag_definition_cache_provider+ option when creating a {Client}. @@ -16,7 +17,7 @@ module PostHog # Retrieve cached flag definitions. Return a Hash with +:flags+, # +:group_type_mapping+, +:cohorts+, +:minimal_flag_called_events+, and +:property_matching_version+ # keys, or +nil+ if the cache is empty. Returning +nil+ triggers an API - # fetch when no flags are loaded yet (emergency fallback). Providers + # fetch when no flags are loaded yet and +:secret_key+ is configured (emergency fallback). Providers # written before +:minimal_flag_called_events+ existed continue to work; # a missing key is treated as +false+. Preserve +:property_matching_version+ # with the definitions: exactly +2+ selects explicit equality; missing/1 @@ -27,7 +28,9 @@ module PostHog # @!method should_fetch_flag_definitions? # Return +true+ if this instance should fetch new definitions from the # API, +false+ to read from cache instead. Use for distributed lock - # coordination so only one worker fetches at a time. + # coordination so only one worker fetches at a time. Called only when + # +:secret_key+ is configured. Without it, the SDK reads +flag_definitions+ + # directly without acquiring or renewing fetch leadership. # @return [Boolean] # # @!method on_flag_definitions_received(data) @@ -53,12 +56,16 @@ module PostHog # - +on_flag_definitions_received+ errors are logged; flags remain in memory # - +shutdown+ errors are logged; shutdown continues # + # API fallback requires +:secret_key+. Without it, the SDK logs a warning and + # keeps any previously loaded definitions. Cache-only readers skip the fetch + # decision and never publish. Polling and manual reloads continue reading the + # cache so consumers can pick up later updates. + # # == Example # # cache = RedisFlagCache.new(redis, service_key: 'my-service') # client = PostHog::Client.new( # api_key: '', - # secret_key: '', # flag_definition_cache_provider: cache # ) # diff --git a/spec/posthog/feature_flags_property_matching_version_spec.rb b/spec/posthog/feature_flags_property_matching_version_spec.rb index 78c627f..b92885c 100644 --- a/spec/posthog/feature_flags_property_matching_version_spec.rb +++ b/spec/posthog/feature_flags_property_matching_version_spec.rb @@ -200,16 +200,18 @@ def expect_local_flags(poller, expected) { status: 200, body: 'invalid json' }, { status: 200, body: definitions.to_json } ) - poller._load_feature_flags - expect_local_flags(poller, false) - poller._load_feature_flags - expect_local_flags(poller, false) - poller._load_feature_flags - expect_local_flags(poller, false) - poller._load_feature_flags - expect_local_flags(poller, false) - poller._load_feature_flags - expect_local_flags(poller, true) + authenticated_poller = described_class.new(60, API_KEY, API_KEY, 'https://us.i.posthog.com', 3) + expect_local_flags(authenticated_poller, false) + authenticated_poller._load_feature_flags + expect_local_flags(authenticated_poller, false) + authenticated_poller._load_feature_flags + expect_local_flags(authenticated_poller, false) + authenticated_poller._load_feature_flags + expect_local_flags(authenticated_poller, false) + authenticated_poller._load_feature_flags + expect_local_flags(authenticated_poller, true) + ensure + authenticated_poller&.shutdown_poller end it 'round trips version through JSON external caches and defaults older entries to legacy' do @@ -217,7 +219,10 @@ def expect_local_flags(poller, expected) allow(provider).to receive(:flag_definitions) stored = nil allow(provider).to receive(:on_flag_definitions_received) { |data| stored = JSON.parse(JSON.generate(data)) } - poller.instance_variable_set(:@flag_definition_cache_provider, provider) + stub_request(:get, url).to_return(status: 200, body: definitions.to_json) + authenticated_poller = described_class.new( + 60, API_KEY, API_KEY, 'https://us.i.posthog.com', 3, flag_definition_cache_provider: provider + ) cached_poller = described_class.new(60, nil, API_KEY, 'https://us.i.posthog.com', 3) cached_poller.instance_variable_set(:@flag_definition_cache_provider, provider) allow(provider).to receive(:flag_definitions) { stored } @@ -227,7 +232,7 @@ def expect_local_flags(poller, expected) stub_request(:get, url).to_return( status: 200, body: definitions.merge(property_matching_version: version).to_json ) - poller._load_feature_flags + authenticated_poller._load_feature_flags expect(stored['property_matching_version']).to eq(version) allow(provider).to receive(:should_fetch_flag_definitions?).and_return(false) cached_poller._load_feature_flags @@ -238,6 +243,7 @@ def expect_local_flags(poller, expected) cached_poller._load_feature_flags expect_local_flags(cached_poller, true) ensure + authenticated_poller&.shutdown_poller cached_poller&.shutdown_poller end end diff --git a/spec/posthog/flag_definition_cache_spec.rb b/spec/posthog/flag_definition_cache_spec.rb index f88abb3..1bf0a89 100644 --- a/spec/posthog/flag_definition_cache_spec.rb +++ b/spec/posthog/flag_definition_cache_spec.rb @@ -130,6 +130,275 @@ def get_poller(client) client.instance_variable_get(:@feature_flags_poller) end + describe 'cache consumers without a secret key' do + let(:definitions_request) do + stub_request(:get, local_eval_url).to_return(status: 200, body: sample_flags_data.to_json) + end + let(:flags_request) do + stub_request(:post, 'https://us.i.posthog.com/flags/?v=2') + .to_return(status: 200, body: { featureFlags: {} }.to_json) + end + let(:disabled_flags_data) do + sample_flags_data.merge('flags' => sample_flags_data['flags'].map { |flag| flag.merge('active' => false) }) + end + + before do + definitions_request + flags_request + provider.should_fetch_return_value = false + provider.stored_data = sample_flags_data + end + + after do + @client&.shutdown + expect(definitions_request).not_to have_been_requested + expect(flags_request).not_to have_been_requested + expect(provider.should_fetch_call_count).to eq(0) + expect(provider.on_received_call_count).to eq(0) + end + + def build_cache_consumer(**opts) + @client = Client.new( + api_key: API_KEY, + test_mode: true, + flag_definition_cache_provider: provider, + **opts + ) + end + + def cached_flag_value(client, only_evaluate_locally: false) + client.evaluate_flags( + 'some-user', + flag_keys: ['test-flag'], + person_properties: { 'region' => 'USA' }, + only_evaluate_locally: only_evaluate_locally + ).get_flag('test-flag') + end + + it 'loads cached definitions at construction and evaluates through public entry points' do + expect(provider).not_to receive(:should_fetch_flag_definitions?) + expect(provider).to receive(:flag_definitions).and_call_original + client = build_cache_consumer + + expect(client.feature_flags_loaded?).to be(true) + expect(cached_flag_value(client)).to be(true) + expect(client.get_feature_flag('disabled-flag', 'some-user')).to be(false) + expect(client.get_feature_flag('test-flag', 'some-user', person_properties: { region: 'USA' })).to be(true) + expect(client.get_all_flags('some-user', person_properties: { region: 'USA' })) + .to eq('test-flag' => true, 'disabled-flag' => false) + expect(provider.get_call_count).to eq(1) + expect(provider.on_received_call_count).to eq(0) + end + + it 'refreshes definitions synchronously through public reload' do + client = build_cache_consumer + expect(cached_flag_value(client)).to be(true) + provider.stored_data = disabled_flags_data + + client.reload_feature_flags + + expect(provider.get_call_count).to eq(2) + expect(cached_flag_value(client)).to be(false) + end + + it 'refreshes cached definitions through the existing polling task and stops on shutdown' do + client = build_cache_consumer(feature_flags_polling_interval: 0.05) + provider.stored_data = disabled_flags_data + + eventually { expect(cached_flag_value(client)).to be(false) } + expect(provider.get_call_count).to be >= 2 + client.shutdown + expect(provider.shutdown_call_count).to eq(1) + expect(get_poller(client).instance_variable_get(:@task).running?).to be(false) + end + + it 'loads cached definitions on the background thread when async loading is enabled' do + release = Queue.new + started = Queue.new + fetch_thread = nil + allow(provider).to receive(:flag_definitions) do + fetch_thread = Thread.current + started << true + release.pop + sample_flags_data + end + client = build_cache_consumer(feature_flags_async_load: true) + eventually { expect(started).not_to be_empty } + + expect(client.feature_flags_loaded?).to be(false) + expect(cached_flag_value(client, only_evaluate_locally: true)).to be_nil + expect(fetch_thread).not_to eq(Thread.current) + release << true + eventually { expect(client.feature_flags_loaded?).to be(true) } + expect(cached_flag_value(client)).to be(true) + ensure + release&.close + end + + it 'retries loading on first evaluation after an initial cache miss' do + provider.stored_data = nil + client = build_cache_consumer + expect(client.feature_flags_loaded?).to be(false) + provider.stored_data = sample_flags_data + + expect(cached_flag_value(client)).to be(true) + expect(client.feature_flags_loaded?).to be(true) + expect(provider.get_call_count).to eq(2) + end + + { + 'cache miss' => proc { |cache| cache.stored_data = nil }, + 'cache read failure' => proc { |cache| cache.get_error = RuntimeError.new('Redis timeout') }, + 'malformed cache' => proc { |cache| cache.stored_data = { 'flags' => 'invalid' } }, + 'cache without flags' => proc { |cache| cache.stored_data = {} } + }.each do |scenario, configure_provider| + it "skips direct fetches and warns on an initial #{scenario}" do + configure_provider.call(provider) + expect(Logging.logger).to receive(:warn).with(/secret_key.*fetch flag definitions/) + client = build_cache_consumer + + expect(client.feature_flags_loaded?).to be(false) + expect(provider.get_call_count).to eq(1) + expect(provider.on_received_call_count).to eq(0) + end + + it "preserves the last snapshot on reload after a #{scenario}" do + provider.stored_data = sample_flags_data.merge('property_matching_version' => 2) + client = build_cache_consumer + configure_provider.call(provider) + reads_before_reload = provider.get_call_count + + client.reload_feature_flags + + expect(client.feature_flags_loaded?).to be(true) + expect(cached_flag_value(client)).to be(true) + expect(get_poller(client)._evaluation_snapshot[:property_matching_version]).to eq(2) + expect(provider.get_call_count).to eq(reads_before_reload + 1) + expect(provider.on_received_call_count).to eq(0) + end + end + + { + 'positive fetch decision' => proc { |cache| cache.should_fetch_return_value = true }, + 'fetch decision failure' => proc { |cache| cache.should_fetch_error = RuntimeError.new('Redis unavailable') } + }.each do |scenario, configure_provider| + it "reads cached definitions without consulting a provider with a #{scenario}" do + configure_provider.call(provider) + client = build_cache_consumer + + expect(cached_flag_value(client)).to be(true) + provider.stored_data = disabled_flags_data + client.reload_feature_flags + + expect(cached_flag_value(client)).to be(false) + expect(provider.get_call_count).to eq(2) + end + end + + [nil, '', ' '].each do |key| + it "reads the cache when privileged keys normalize to nil from #{key.inspect}" do + provider.should_fetch_return_value = true + client = build_cache_consumer(secret_key: key, personal_api_key: key) + + expect(client.feature_flags_loaded?).to be(true) + expect(cached_flag_value(client)).to be(true) + expect(provider.get_call_count).to eq(1) + end + end + + it 'hydrates matching metadata and resets to legacy when a fresh snapshot omits the version' do + data = sample_flags_data.merge('property_matching_version' => 2) + data['flags'] = [{ + 'key' => 'versioned-flag', 'active' => true, + 'filters' => { 'groups' => [{ 'properties' => [{ 'key' => 'value', 'value' => false }] }] } + }] + provider.stored_data = data + client = build_cache_consumer + expect(client.get_feature_flag('versioned-flag', 'user', person_properties: { value: 'banana' })).to be(false) + + provider.stored_data = data.except('property_matching_version') + client.reload_feature_flags + + expect(client.get_feature_flag('versioned-flag', 'user', person_properties: { value: 'banana' })).to be(true) + end + + it 'does not consult the provider without a project API key' do + client = build_cache_consumer(api_key: nil) + client.reload_feature_flags + + expect(client.feature_flags_loaded?).to be(false) + expect(provider.should_fetch_call_count).to eq(0) + expect(provider.get_call_count).to eq(0) + end + + it 'keeps local evaluation disabled without either a secret key or a provider' do + client = build_cache_consumer(flag_definition_cache_provider: nil) + client.reload_feature_flags + + expect(client.feature_flags_loaded?).to be(false) + expect(cached_flag_value(client, only_evaluate_locally: true)).to be_nil + expect(get_poller(client).instance_variable_get(:@task).running?).to be(false) + end + end + + describe 'shared fetch leadership' do + it 'lets a keyed publisher refresh while a keyless reader uses the same cache' do + reader_provider = provider + publisher_provider = MockCacheProvider.new + shared_data = sample_flags_data + leader = nil + [reader_provider, publisher_provider].each do |cache| + allow(cache).to receive(:flag_definitions).and_wrap_original do |method| + method.call + shared_data + end + allow(cache).to receive(:should_fetch_flag_definitions?).and_wrap_original do |method| + method.call + leader ||= cache + leader == cache + end + end + allow(publisher_provider).to receive(:on_flag_definitions_received).and_wrap_original do |method, data| + method.call(data) + shared_data = JSON.parse(JSON.dump(data)) + end + definition_request = stub_request(:get, local_eval_url) + .to_return(status: 200, body: sample_flags_data.to_json) + reader = Client.new(api_key: API_KEY, test_mode: true, flag_definition_cache_provider: reader_provider) + publisher = Client.new( + api_key: API_KEY, secret_key: API_KEY, test_mode: true, + flag_definition_cache_provider: publisher_provider + ) + expect(leader).to eq(publisher_provider) + expect(definition_request).to have_been_requested.once + + updated_data = sample_flags_data.merge( + 'flags' => sample_flags_data['flags'].map { |flag| flag.merge('active' => false) }, + 'property_matching_version' => 2, 'minimal_flag_called_events' => true + ) + stub_request(:get, local_eval_url).to_return(status: 200, body: updated_data.to_json) + reader.reload_feature_flags + publisher.reload_feature_flags + reader.reload_feature_flags + + expect(reader.get_feature_flag('test-flag', 'user', only_evaluate_locally: true)).to be(false) + expect(publisher.get_feature_flag('test-flag', 'user', only_evaluate_locally: true)).to be(false) + expect(reader_provider.should_fetch_call_count).to eq(0) + expect(reader_provider.on_received_call_count).to eq(0) + expect(reader_provider.get_call_count).to eq(3) + expect(publisher_provider.should_fetch_call_count).to eq(2) + expect(publisher_provider.on_received_call_count).to eq(2) + snapshot = get_poller(reader)._evaluation_snapshot + expect(snapshot[:property_matching_version]).to eq(2) + expect(snapshot[:minimal_flag_called_events]).to be(true) + expect(snapshot[:group_type_mapping][:'0']).to eq('company') + expect(snapshot[:cohorts][:'1'][:type]).to eq('AND') + ensure + reader&.shutdown + publisher&.shutdown + end + end + describe 'cache initialization' do it 'uses cached data when should_fetch? returns false and cache has data' do provider.should_fetch_return_value = false