Skip to content

Commit

Permalink
change(rc): Update Remote Config condition evaluation hashing (#2760)
Browse files Browse the repository at this point in the history 
* move from farmhash to sha256

* lint

* add more special char test cases

* another special char case

* further refine test cases

* clean up tests

* Trigger CIs

* fix newline

---------

Co-authored-by: Kevin Elko <[email protected]>
  • Loading branch information
@kjelko
kjelko and Kevin Elko authored Nov 7, 2024
1 parent dcef2ae commit 4babb45
Show file tree
Hide file tree
Showing 2 changed files with 83 additions and 128 deletions.
22 changes: 6 additions & 16 deletions src/remote-config/condition-evaluator-internal.ts
View file
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) {
hash64 = -hash64;
}

return hash64;
const hex = createHash('sha256').update(seededRandomizationId).digest('hex');
return BigInt(`0x${hex}`);
}

private evaluateCustomSignalCondition(
Expand Down
189 changes: 77 additions & 112 deletions test/unit/remote-config/condition-evaluator.spec.ts
View file
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'))
});
});
});

0 comments on commit 4babb45

Please sign in to comment.