Skip to content

feat: inspect format field to determine whether config is obfuscated #218

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Feb 19, 2025
Merged

feat: inspect format field to determine whether config is obfuscated #218

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
cb062eb
chore: inspect field to determine whether config is obfuscated
typotter Feb 13, 2025
8aee608
really fix tests
typotter Feb 14, 2025
e01f3b3
o(1) enum conversion
typotter Feb 14, 2025
5ebe36e
cache obfuscated checks and await in tests
typotter Feb 18, 2025
f067c3b
merge main
typotter Feb 19, 2025
04d8a43
chore: drop STR to Enum and lean into string enum shape goodness
typotter Feb 19, 2025
0786d71
comments
typotter Feb 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 40 additions & 28 deletions src/client/eppo-client.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,12 @@ import * as td from 'testdouble';

import {
ASSIGNMENT_TEST_DATA_DIR,
getTestAssignments,
IAssignmentTestCase,
MOCK_UFC_RESPONSE_FILE,
OBFUSCATED_MOCK_UFC_RESPONSE_FILE,
SubjectTestCase,
getTestAssignments,
readMockUFCResponse,
SubjectTestCase,
testCasesByFileName,
validateTestAssignments,
} from '../../test/testHelpers';
Expand All @@ -22,13 +22,13 @@ import {
} from '../configuration';
import { IConfigurationStore } from '../configuration-store/configuration-store';
import { MemoryOnlyConfigurationStore } from '../configuration-store/memory.store';
import { MAX_EVENT_QUEUE_SIZE, DEFAULT_POLL_INTERVAL_MS, POLL_JITTER_PCT } from '../constants';
import { DEFAULT_POLL_INTERVAL_MS, MAX_EVENT_QUEUE_SIZE, POLL_JITTER_PCT } from '../constants';
import { decodePrecomputedFlag } from '../decoding';
import { Flag, ObfuscatedFlag, Variation, VariationType } from '../interfaces';
import { Flag, ObfuscatedFlag, VariationType, FormatEnum, Variation } from '../interfaces';
import { getMD5Hash } from '../obfuscation';
import { AttributeType } from '../types';

import EppoClient, { FlagConfigurationRequestParameters, checkTypeMatch } from './eppo-client';
import EppoClient, { checkTypeMatch, FlagConfigurationRequestParameters } from './eppo-client';
import { initConfiguration } from './test-utils';

// Use a known salt to produce deterministic hashes
Expand All @@ -46,6 +46,18 @@ describe('EppoClient E2E test', () => {
}) as jest.Mock;
const storage = new MemoryOnlyConfigurationStore<Flag | ObfuscatedFlag>();

/**
* Use this helper instead of directly setting entries on the `storage` ConfigurationStore.
* This method ensures the format field is set as it is required for parsing.
* @param entries
*/
function setUnobfuscatedFlagEntries(
entries: Record<string, Flag | ObfuscatedFlag>,
): Promise<boolean> {
storage.setFormat(FormatEnum.SERVER);
return storage.setEntries(entries);
}

beforeAll(async () => {
await initConfiguration(storage);
});
Expand Down Expand Up @@ -88,8 +100,8 @@ describe('EppoClient E2E test', () => {
describe('error encountered', () => {
let client: EppoClient;

beforeAll(() => {
storage.setEntries({ [flagKey]: mockFlag });
beforeAll(async () => {
await setUnobfuscatedFlagEntries({ [flagKey]: mockFlag });
client = new EppoClient({ flagConfigurationStore: storage });

td.replace(EppoClient.prototype, 'getAssignmentDetail', function () {
Expand Down Expand Up @@ -144,8 +156,8 @@ describe('EppoClient E2E test', () => {
});

describe('setLogger', () => {
beforeAll(() => {
storage.setEntries({ [flagKey]: mockFlag });
beforeAll(async () => {
await setUnobfuscatedFlagEntries({ [flagKey]: mockFlag });
});

it('Invokes logger for queued events', () => {
Expand Down Expand Up @@ -192,8 +204,8 @@ describe('EppoClient E2E test', () => {
});

describe('precomputed flags', () => {
beforeAll(() => {
storage.setEntries({
beforeAll(async () => {
await setUnobfuscatedFlagEntries({
[flagKey]: mockFlag,
disabledFlag: { ...mockFlag, enabled: false },
anotherFlag: {
Expand Down Expand Up @@ -425,10 +437,10 @@ describe('EppoClient E2E test', () => {
);
});

it('logs variation assignment and experiment key', () => {
it('logs variation assignment and experiment key', async () => {
const mockLogger = td.object<IAssignmentLogger>();

storage.setEntries({ [flagKey]: mockFlag });
await setUnobfuscatedFlagEntries({ [flagKey]: mockFlag });
const client = new EppoClient({ flagConfigurationStore: storage });
client.setAssignmentLogger(mockLogger);

Expand All @@ -450,11 +462,11 @@ describe('EppoClient E2E test', () => {
expect(loggedAssignmentEvent.allocation).toEqual(mockFlag.allocations[0].key);
});

it('handles logging exception', () => {
it('handles logging exception', async () => {
const mockLogger = td.object<IAssignmentLogger>();
td.when(mockLogger.logAssignment(td.matchers.anything())).thenThrow(new Error('logging error'));

storage.setEntries({ [flagKey]: mockFlag });
await setUnobfuscatedFlagEntries({ [flagKey]: mockFlag });
const client = new EppoClient({ flagConfigurationStore: storage });
client.setAssignmentLogger(mockLogger);

Expand All @@ -469,8 +481,8 @@ describe('EppoClient E2E test', () => {
expect(assignment).toEqual('variation-a');
});

it('exports flag configuration', () => {
storage.setEntries({ [flagKey]: mockFlag });
it('exports flag configuration', async () => {
await setUnobfuscatedFlagEntries({ [flagKey]: mockFlag });
const client = new EppoClient({ flagConfigurationStore: storage });
expect(client.getFlagConfigurations()).toEqual({ [flagKey]: mockFlag });
});
Expand All @@ -479,10 +491,10 @@ describe('EppoClient E2E test', () => {
let client: EppoClient;
let mockLogger: IAssignmentLogger;

beforeEach(() => {
beforeEach(async () => {
mockLogger = td.object<IAssignmentLogger>();

storage.setEntries({ [flagKey]: mockFlag });
await setUnobfuscatedFlagEntries({ [flagKey]: mockFlag });
client = new EppoClient({ flagConfigurationStore: storage });
client.setAssignmentLogger(mockLogger);
});
Expand Down Expand Up @@ -537,7 +549,7 @@ describe('EppoClient E2E test', () => {
});

it('logs for each unique flag', async () => {
await storage.setEntries({
await setUnobfuscatedFlagEntries({
[flagKey]: mockFlag,
'flag-2': {
...mockFlag,
Expand All @@ -564,10 +576,10 @@ describe('EppoClient E2E test', () => {
expect(td.explain(mockLogger.logAssignment).callCount).toEqual(3);
});

it('logs twice for the same flag when allocations change', () => {
it('logs twice for the same flag when allocations change', async () => {
client.useNonExpiringInMemoryAssignmentCache();

storage.setEntries({
await setUnobfuscatedFlagEntries({
[flagKey]: {
...mockFlag,

Expand All @@ -588,7 +600,7 @@ describe('EppoClient E2E test', () => {
});
client.getStringAssignment(flagKey, 'subject-10', {}, 'default');

storage.setEntries({
await setUnobfuscatedFlagEntries({
[flagKey]: {
...mockFlag,
allocations: [
Expand All @@ -610,17 +622,17 @@ describe('EppoClient E2E test', () => {
expect(td.explain(mockLogger.logAssignment).callCount).toEqual(2);
});

it('logs the same subject/flag/variation after two changes', () => {
it('logs the same subject/flag/variation after two changes', async () => {
client.useNonExpiringInMemoryAssignmentCache();

// original configuration version
storage.setEntries({ [flagKey]: mockFlag });
await setUnobfuscatedFlagEntries({ [flagKey]: mockFlag });

client.getStringAssignment(flagKey, 'subject-10', {}, 'default'); // log this assignment
client.getStringAssignment(flagKey, 'subject-10', {}, 'default'); // cache hit, don't log

// change the variation
storage.setEntries({
await setUnobfuscatedFlagEntries({
[flagKey]: {
...mockFlag,
allocations: [
Expand All @@ -643,13 +655,13 @@ describe('EppoClient E2E test', () => {
client.getStringAssignment(flagKey, 'subject-10', {}, 'default'); // cache hit, don't log

// change the flag again, back to the original
storage.setEntries({ [flagKey]: mockFlag });
await setUnobfuscatedFlagEntries({ [flagKey]: mockFlag });

client.getStringAssignment(flagKey, 'subject-10', {}, 'default'); // important: log this assignment
client.getStringAssignment(flagKey, 'subject-10', {}, 'default'); // cache hit, don't log

// change the allocation
storage.setEntries({
await setUnobfuscatedFlagEntries({
[flagKey]: {
...mockFlag,
allocations: [
Expand Down
61 changes: 52 additions & 9 deletions src/client/eppo-client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ import { LRUInMemoryAssignmentCache } from '../cache/lru-in-memory-assignment-ca
import { NonExpiringInMemoryAssignmentCache } from '../cache/non-expiring-in-memory-cache-assignment';
import { TLRUInMemoryAssignmentCache } from '../cache/tlru-in-memory-assignment-cache';
import {
IConfigurationWire,
ConfigurationWireV1,
IConfigurationWire,
IPrecomputedConfiguration,
PrecomputedConfiguration,
} from '../configuration';
Expand All @@ -27,6 +27,7 @@ import {
DEFAULT_POLL_CONFIG_REQUEST_RETRIES,
DEFAULT_POLL_INTERVAL_MS,
DEFAULT_REQUEST_TIMEOUT_MS,
OBFUSCATED_FORMATS,
} from '../constants';
import { decodeFlag } from '../decoding';
import { EppoValue } from '../eppo_value';
Expand All @@ -46,6 +47,7 @@ import {
BanditVariation,
ConfigDetails,
Flag,
FormatEnum,
IPrecomputedBandit,
ObfuscatedFlag,
PrecomputedFlag,
Expand Down Expand Up @@ -101,6 +103,12 @@ export type EppoClientParameters = {
banditVariationConfigurationStore?: IConfigurationStore<BanditVariation[]>;
banditModelConfigurationStore?: IConfigurationStore<BanditParameters>;
configurationRequestParameters?: FlagConfigurationRequestParameters;
/**
* Setting this value will have no side effects other than triggering a warning when the actual
* configuration's obfuscated does not match the value set here.
*
* @deprecated obfuscation is determined by inspecting the `format` field of the UFC response.
*/
isObfuscated?: boolean;
};

Expand All @@ -122,7 +130,9 @@ export default class EppoClient {
private assignmentCache?: AssignmentCache;
// whether to suppress any errors and return default values instead
private isGracefulFailureMode = true;
private isObfuscated: boolean;
private expectObfuscated: boolean;
private obfuscationMismatchWarningIssued = false;
private configObfuscatedCache?: boolean;
private requestPoller?: IPoller;
private readonly evaluator = new Evaluator();

Expand Down Expand Up @@ -151,7 +161,30 @@ export default class EppoClient {
this.banditModelConfigurationStore = banditModelConfigurationStore;
this.overrideStore = overrideStore;
this.configurationRequestParameters = configurationRequestParameters;
this.isObfuscated = isObfuscated;
this.expectObfuscated = isObfuscated;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐛 This looks like it sets expectObfuscation (to either true or false) even if user didn't pass any value, which may cause an unexpected warning

}

private maybeWarnAboutObfuscationMismatch(configObfuscated: boolean) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason we develop elaborate warning for a deprecated flag?

  1. The warning mechanism is sufficiently complicated that there's a bug in it (see above comment).
  2. If we think this kind of warning is useful enough to justify mechanism, it makes sense to keep it and not deprecate the flag (or add expectObfuscation flag). (Although I don't think it's worth it.)
  3. If it's not that useful, I would rather remove it completely. Maybe issue a quick deprecation warning in constructor when isObfuscated is supplied

// Don't warn again if we did on the last check.
if (configObfuscated !== this.expectObfuscated && !this.obfuscationMismatchWarningIssued) {
this.obfuscationMismatchWarningIssued = true;
logger.warn(
`[Eppo SDK] configuration obfuscation [${configObfuscated}] does not match expected [${this.expectObfuscated}]`,
);
} else if (configObfuscated === this.expectObfuscated) {
// Reset the warning to false in case the client configuration (re-)enters a mismatched state.
this.obfuscationMismatchWarningIssued = false;
}
}

private isObfuscated() {
if (this.configObfuscatedCache === undefined) {
this.configObfuscatedCache = OBFUSCATED_FORMATS.includes(
this.flagConfigurationStore.getFormat() ?? FormatEnum.SERVER,
);
}
this.maybeWarnAboutObfuscationMismatch(this.configObfuscatedCache);
return this.configObfuscatedCache;
Comment on lines +181 to +187
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: maintaining this cache is probably as costly as doing two comparisons (we actually only need one as precomputed format is not valid here).

major: cache invalidation is also dubious and there seems to be a bug lurking in it. Cache is reset before issues a configuration fetch. However, as fetch is async, the cache will get re-populated if evaluation is requested, and subsequent configuration fetch success does not invalidate the cache. This may cause an evaluation bug if initial configuration obfuscation does not match remote configuration obfuscation (e.g., client initialized with non-obfuscated server config first and fetches an obfuscated configuration later)

If we want to cache obfuscation status, configuration itself is the right place to do that

}

setConfigurationRequestParameters(
Expand All @@ -163,6 +196,7 @@ export default class EppoClient {
// noinspection JSUnusedGlobalSymbols
setFlagConfigurationStore(flagConfigurationStore: IConfigurationStore<Flag | ObfuscatedFlag>) {
this.flagConfigurationStore = flagConfigurationStore;
this.configObfuscatedCache = undefined;
}

// noinspection JSUnusedGlobalSymbols
Expand Down Expand Up @@ -201,8 +235,15 @@ export default class EppoClient {
}

// noinspection JSUnusedGlobalSymbols
/**
* Setting this value will have no side effects other than triggering a warning when the actual
* configuration's obfuscated does not match the value set here.
*
* @deprecated The client determines whether the configuration is obfuscated by inspection
* @param isObfuscated
*/
setIsObfuscated(isObfuscated: boolean) {
this.isObfuscated = isObfuscated;
this.expectObfuscated = isObfuscated;
}

setOverrideStore(store: ISyncStore<Variation>): void {
Expand Down Expand Up @@ -267,6 +308,7 @@ export default class EppoClient {

const pollingCallback = async () => {
if (await this.flagConfigurationStore.isExpired()) {
this.configObfuscatedCache = undefined;
return configurationRequestor.fetchAndStoreConfigurations();
}
};
Expand Down Expand Up @@ -885,7 +927,7 @@ export default class EppoClient {
configDetails,
subjectKey,
subjectAttributes,
this.isObfuscated,
this.isObfuscated(),
);

// allocationKey is set along with variation when there is a result. this check appeases typescript below
Expand Down Expand Up @@ -1035,15 +1077,16 @@ export default class EppoClient {
);
}

const isObfuscated = this.isObfuscated();
const result = this.evaluator.evaluateFlag(
flag,
configDetails,
subjectKey,
subjectAttributes,
this.isObfuscated,
isObfuscated,
expectedVariationType,
);
if (this.isObfuscated) {
if (isObfuscated) {
// flag.key is obfuscated, replace with requested flag key
result.flagKey = flagKey;
}
Expand Down Expand Up @@ -1094,7 +1137,7 @@ export default class EppoClient {
}

private getFlag(flagKey: string): Flag | null {
return this.isObfuscated
return this.isObfuscated()
? this.getObfuscatedFlag(flagKey)
: this.flagConfigurationStore.get(flagKey);
}
Expand Down Expand Up @@ -1262,7 +1305,7 @@ export default class EppoClient {

private buildLoggerMetadata(): Record<string, unknown> {
return {
obfuscated: this.isObfuscated,
obfuscated: this.isObfuscated(),
sdkLanguage: 'javascript',
sdkLibVersion: LIB_VERSION,
};
Expand Down
10 changes: 10 additions & 0 deletions src/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { FormatEnum } from './interfaces';

export const DEFAULT_REQUEST_TIMEOUT_MS = 5000;
export const REQUEST_TIMEOUT_MILLIS = DEFAULT_REQUEST_TIMEOUT_MS; // for backwards compatibility
export const DEFAULT_POLL_INTERVAL_MS = 30000;
Expand All @@ -15,3 +17,11 @@ export const NULL_SENTINEL = 'EPPO_NULL';
export const MAX_EVENT_QUEUE_SIZE = 100;
export const BANDIT_ASSIGNMENT_SHARDS = 10000;
export const DEFAULT_TLRU_TTL_MS = 600_000;

/**
* UFC Configuration formats which are obfuscated.
*
* We use string[] instead of FormatEnum[] to allow easy interaction with this value in its wire type (string).
* Converting from string to enum requires a map lookup or array iteration and is much more awkward than the inverse.
*/
export const OBFUSCATED_FORMATS: string[] = [FormatEnum.CLIENT, FormatEnum.PRECOMPUTED];
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making this string[] instead of the interpolated FormatEnum[] allows us to check if the config wire value (a string) is one of these formats without having to convert from string to enum (which actually requires a lookup or array iteration).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I don't think this is actually true as FormatEnum is a string enum, so there's no lookup involved when casting string to enum. Check JS tab here — stringToEnum is a noop

(Even though there's no performance penalty for casting string to enum, doing so is type-unsafe as the string could be anything, including values not listed in enum.)

Loading