Skip to content

change(rc): Update Remote Config condition evaluation hashing #2760

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 10 commits into from
Nov 7, 2024
Merged

change(rc): Update Remote Config condition evaluation hashing #2760

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
c9b42e2
move from farmhash to sha256
Nov 1, 2024
77a8707
lint
Nov 1, 2024
3d04abb
add more special char test cases
Nov 3, 2024
ae2fabe
another special char case
Nov 4, 2024
c3ba59f
further refine test cases
Nov 4, 2024
9cc3820
clean up tests
Nov 4, 2024
9a8570a
Merge branch 'master' into ssrc-hash
kjelko Nov 5, 2024
f8dd7f5
Trigger CIs
Nov 5, 2024
c6c1e6d
fix newline
Nov 7, 2024
83b438c
Merge branch 'master' into ssrc-hash
kjelko Nov 7, 2024
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
24 changes: 7 additions & 17 deletions src/remote-config/condition-evaluator-internal.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ import {
CustomSignalCondition,
CustomSignalOperator,
} from './remote-config-api';
import * as farmhash from 'farmhash-modern';
import { createHash } from 'crypto';


/**
* Encapsulates condition evaluation logic to simplify organization and
Expand Down Expand Up @@ -151,9 +152,8 @@ export class ConditionEvaluator {
const seedPrefix = seed && seed.length > 0 ? `${seed}.` : '';
const stringToHash = `${seedPrefix}${context.randomizationId}`;

const hash64 = ConditionEvaluator.hashSeededRandomizationId(stringToHash)

const instanceMicroPercentile = hash64 % BigInt(100 * 1_000_000);
const hash = ConditionEvaluator.hashSeededRandomizationId(stringToHash)
const instanceMicroPercentile = hash % BigInt(100 * 1_000_000);

switch (percentOperator) {
case PercentConditionOperator.LESS_OR_EQUAL:
Expand All @@ -173,18 +173,8 @@ export class ConditionEvaluator {
}

static hashSeededRandomizationId(seededRandomizationId: string): bigint {
// For consistency with the Remote Config fetch endpoint's percent condition behavior
// we use Farmhash's fingerprint64 algorithm and interpret the resulting unsigned value
// as a signed value.
let hash64 = BigInt.asIntN(64, farmhash.fingerprint64(seededRandomizationId));

// Manually negate the hash if its value is less than 0, since Math.abs doesn't
// support BigInt.
if (hash64 < 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking (no action req'd): we're now directly instantiating the bigint with the hash, rather than coercing it to behave like a 64bit signed Java long, so it will always be positive.

hash64 = -hash64;
}

return hash64;
const hex = createHash('sha256').update(seededRandomizationId).digest('hex');
Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking (no action req'd): we had a 64 bits hash before, so we don't need 256 bits, but sha256 is broadly available and there's no noticeable performance difference, so sha256 is fine.

return BigInt(`0x${hex}`);
}

private evaluateCustomSignalCondition(
Expand Down Expand Up @@ -332,4 +322,4 @@ function compareSemanticVersions(
if (version1[i] > version2[i]) return predicateFn(1);
}
return false;
}
}
189 changes: 77 additions & 112 deletions test/unit/remote-config/condition-evaluator.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import {
} from '../../../src/remote-config/remote-config-api';
import { v4 as uuidv4 } from 'uuid';
import { clone } from 'lodash';
import * as farmhash from 'farmhash-modern';
import * as crypto from 'crypto';

const expect = chai.expect;

Expand Down Expand Up @@ -251,9 +251,8 @@ describe('ConditionEvaluator', () => {

it('should use zero for undefined microPercent', () => {
// Stubs ID hasher to return a number larger than zero.
const stub = sinon
.stub(farmhash, 'fingerprint64')
.returns(1n);
const stub = sinon.stub(crypto.Hash.prototype, 'digest');
stub.withArgs('hex').returns('1');
stubs.push(stub);

const condition = {
Expand Down Expand Up @@ -284,9 +283,8 @@ describe('ConditionEvaluator', () => {

it('should use zeros for undefined microPercentRange', () => {
// Stubs ID hasher to return a number in range.
const stub = sinon
.stub(farmhash, 'fingerprint64')
.returns(1n);
const stub = sinon.stub(crypto.Hash.prototype, 'digest');
stub.withArgs('hex').returns('1');
stubs.push(stub);

const condition = {
Expand Down Expand Up @@ -317,9 +315,8 @@ describe('ConditionEvaluator', () => {

it('should use zero for undefined microPercentUpperBound', () => {
// Stubs ID hasher to return a number outside range.
const stub = sinon
.stub(farmhash, 'fingerprint64')
.returns(1n);
const stub = sinon.stub(crypto.Hash.prototype, 'digest');
stub.withArgs('hex').returns('1');
stubs.push(stub);

const condition = {
Expand Down Expand Up @@ -353,9 +350,8 @@ describe('ConditionEvaluator', () => {

it('should use zero for undefined microPercentLowerBound', () => {
// Stubs ID hasher to return a number in range.
const stub = sinon
.stub(farmhash, 'fingerprint64')
.returns(1n);
const stub = sinon.stub(crypto.Hash.prototype, 'digest');
stub.withArgs('hex').returns('1');
stubs.push(stub);

const condition = {
Expand Down Expand Up @@ -388,9 +384,9 @@ describe('ConditionEvaluator', () => {
});

it('should evaluate 9 as less or equal to 10', () => {
const stub = sinon
.stub(farmhash, 'fingerprint64')
.returns(9n);
const stub = sinon.stub(crypto.Hash.prototype, 'digest');
stub.withArgs('hex').returns('9');
stubs.push(stub);

stubs.push(stub);
const condition = {
Expand Down Expand Up @@ -419,9 +415,9 @@ describe('ConditionEvaluator', () => {
});

it('should evaluate 10 as less or equal to 10', () => {
const stub = sinon
.stub(farmhash, 'fingerprint64')
.returns(10n);
const stub = sinon.stub(crypto.Hash.prototype, 'digest');
stub.withArgs('hex').returns((10).toString(16));
stubs.push(stub);

stubs.push(stub);
const condition = {
Expand Down Expand Up @@ -450,9 +446,9 @@ describe('ConditionEvaluator', () => {
});

it('should evaluate 11 as not less or equal to 10', () => {
const stub = sinon
.stub(farmhash, 'fingerprint64')
.returns(11n);
const stub = sinon.stub(crypto.Hash.prototype, 'digest');
stub.withArgs('hex').returns((11).toString(16));
stubs.push(stub);

stubs.push(stub);
const condition = {
Expand Down Expand Up @@ -481,9 +477,9 @@ describe('ConditionEvaluator', () => {
});

it('should negate -11 to 11 and evaluate as not less or equal to 10', () => {
const stub = sinon
.stub(farmhash, 'fingerprint64')
.returns(-11n);
const stub = sinon.stub(crypto.Hash.prototype, 'digest');
stub.withArgs('hex').returns((11).toString(16));
stubs.push(stub);

stubs.push(stub);
const condition = {
Expand Down Expand Up @@ -540,9 +536,9 @@ describe('ConditionEvaluator', () => {
});

it('should evaluate 11M as greater than 10M', () => {
const stub = sinon
.stub(farmhash, 'fingerprint64')
.returns(11n);
const stub = sinon.stub(crypto.Hash.prototype, 'digest');
stub.withArgs('hex').returns((11).toString(16));
stubs.push(stub);

stubs.push(stub);
const condition = {
Expand Down Expand Up @@ -571,9 +567,8 @@ describe('ConditionEvaluator', () => {
});

it('should evaluate 9 as not greater than 10', () => {
const stub = sinon
.stub(farmhash, 'fingerprint64')
.returns(9n);
const stub = sinon.stub(crypto.Hash.prototype, 'digest');
stub.withArgs('hex').returns('9');
stubs.push(stub);

const condition = {
Expand Down Expand Up @@ -661,9 +656,8 @@ describe('ConditionEvaluator', () => {
});

it('should evaluate 10 as between 9 and 11', () => {
const stub = sinon
.stub(farmhash, 'fingerprint64')
.returns(10n);
const stub = sinon.stub(crypto.Hash.prototype, 'digest');
stub.withArgs('hex').returns((10).toString(16));
stubs.push(stub);

const condition = {
Expand Down Expand Up @@ -726,9 +720,8 @@ describe('ConditionEvaluator', () => {
});

it('should evaluate 12 as not between 9 and 11', () => {
const stub = sinon
.stub(farmhash, 'fingerprint64')
.returns(12n);
const stub = sinon.stub(crypto.Hash.prototype, 'digest');
stub.withArgs('hex').returns((12).toString(16));
stubs.push(stub);

const condition = {
Expand Down Expand Up @@ -759,6 +752,51 @@ describe('ConditionEvaluator', () => {
expect(actual).to.be.false;
});

describe('known percent condition values', () => {
// This test is useful for ensuring consistency across all places we
// evaluate percent conditions. It creates a set of 10 conditions targeting 50%
// with randomizationIds 0-9 and a constant `seed` value.
const conditionEvaluator = new ConditionEvaluator();

const percentCondition = {
percentOperator: PercentConditionOperator.BETWEEN,
microPercentRange: {
microPercentLowerBound: 0,
microPercentUpperBound: 50_000_000 // 50%
},
};

const testCases = [
{ seed: '1', randomizationId: 'one', result: false },
{ seed: '2', randomizationId: 'two', result: false },
{ seed: '3', randomizationId: 'three', result: true },
{ seed: '4', randomizationId: 'four', result: false },
{ seed: '5', randomizationId: 'five', result: true },
{ seed: '', randomizationId: '😊', result: true },
{ seed: '', randomizationId: '😀', result: false },
{ seed: 'hêl£o', randomizationId: 'wørlÐ', result: false },
{ seed: 'řemøťe', randomizationId: 'çōnfįġ', result: true },
{ seed: 'long', randomizationId: '.'.repeat(100), result: true },
{ seed: 'very-long', randomizationId: '.'.repeat(1000), result: false },
];

testCases.map(({ randomizationId, seed, result }) => {

const idSummary = randomizationId.length > 25
? `a ${randomizationId.length} character randomizationID`
: `"${randomizationId}"`;

it(`should evaluate ${idSummary} with seed "${seed}" to ${result}`, () => {
const context = { randomizationId };
const evalResult = conditionEvaluator.evaluateConditions([{
name: 'is_enabled',
condition: { percent: { ...percentCondition, seed } }
}], context);
expect(evalResult.get('is_enabled')).to.equal(result);
});
});
});

// The following tests are probabilistic. They use tolerances based on
// standard deviations to balance accuracy and flakiness. Random IDs will
// hash to the target range + 3 standard deviations 99.7% of the time,
Expand Down Expand Up @@ -958,15 +996,15 @@ describe('ConditionEvaluator', () => {
describe('STRING_CONTAINS', () => {
const testCases: CustomSignalTestCase[] = [
{ targets: ['foo', 'biz'], actual: 'foobar', outcome: true },
{ targets: ['foo', 'biz'],actual: 'bar',outcome: false },
{ targets: ['foo', 'biz'], actual: 'bar', outcome: false },
];

testCases.forEach(runCustomSignalTestCase(CustomSignalOperator.STRING_CONTAINS));
});

describe('STRING_DOES_NOT_CONTAIN', () => {
const testCases: CustomSignalTestCase[] = [
{ targets: ['foo', 'biz'],actual: 'bar',outcome: true },
{ targets: ['foo', 'biz'], actual: 'bar', outcome: true },
{ targets: ['foo', 'biz'], actual: 'foobar', outcome: false },
];

Expand Down Expand Up @@ -1135,77 +1173,4 @@ describe('ConditionEvaluator', () => {
});
});
});

describe('hashSeededRandomizationId', () => {
// The Farmhash algorithm produces a 64 bit unsigned integer,
// which we convert to a signed integer for legacy compatibility.
// This has caused confusion in the past, so we explicitly
// test here.
it('should leave numbers <= 2^63-1 (max signed long) as is', function () {
if (nodeVersion.startsWith('14')) {
this.skip();
}

const stub = sinon
.stub(farmhash, 'fingerprint64')
// 2^63-1 = 9223372036854775807.
.returns(BigInt('9223372036854775807'));
stubs.push(stub);

const actual = ConditionEvaluator.hashSeededRandomizationId('anything');

expect(actual).to.equal(BigInt('9223372036854775807'))
});

it('should convert 2^63 to negative (min signed long) and then find the absolute value', function () {
if (nodeVersion.startsWith('14')) {
this.skip();
}

const stub = sinon
.stub(farmhash, 'fingerprint64')
// 2^63 = 9223372036854775808.
.returns(BigInt('9223372036854775808'));
stubs.push(stub);

const actual = ConditionEvaluator.hashSeededRandomizationId('anything');

// 2^63 is the negation of 2^63-1
expect(actual).to.equal(BigInt('9223372036854775808'))
});

it('should convert 2^63+1 to negative and then find the absolute value', function () {
if (nodeVersion.startsWith('14')) {
this.skip();
}

const stub = sinon
.stub(farmhash, 'fingerprint64')
// 2^63+1 9223372036854775809.
.returns(BigInt('9223372036854775809'));
stubs.push(stub);

const actual = ConditionEvaluator.hashSeededRandomizationId('anything');

// 2^63+1 is larger than 2^63, so the absolute value is smaller
expect(actual).to.equal(BigInt('9223372036854775807'))
});

it('should handle the value that initially caused confusion', function () {
if (nodeVersion.startsWith('14')) {
this.skip();
}

const stub = sinon
.stub(farmhash, 'fingerprint64')
// We were initially confused about the nature of this value ...
.returns(BigInt('16081085603393958147'));
stubs.push(stub);

const actual = ConditionEvaluator.hashSeededRandomizationId('anything');

// ... Now we know it's the unsigned equivalent of this absolute value.
expect(actual).to.equal(BigInt('2365658470315593469'))
});
});
});