-
Notifications
You must be signed in to change notification settings - Fork 1
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.
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
Conversation
8158e45
to
8aee608
Compare
39a3e8b
to
e01f3b3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good stuff!
src/client/eppo-client.spec.ts
Outdated
|
||
// original configuration version | ||
storage.setEntries({ [flagKey]: mockFlag }); | ||
setUnobfuscatedFlagEntries({ [flagKey]: mockFlag }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not awaiting
some of these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good catch. These tests have still worked because the underlying Flag Config Storage is a memory-only store, so the actual "writing" of entries is happening sync when the method is called.
Worth it to fix it now before it causes weird problems in the future.
src/client/eppo-client.ts
Outdated
|
||
private isObfuscated() { | ||
const configFormat = getFormatFromString(this.flagConfigurationStore.getFormat()); | ||
const configObfuscated = OBFUSCATED_FORMATS.includes(configFormat); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Thanks for the review, @felipecsl and @aarsilv . I'll leave this up for the day to give you a chance to comment on the caching if you'd like. |
/** | ||
* UFC Configuration formats which are obfuscated. | ||
*/ | ||
export const OBFUSCATED_FORMATS: string[] = [FormatEnum.CLIENT, FormatEnum.PRECOMPUTED]; |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.)
this.overrideStore = overrideStore; | ||
this.configurationRequestParameters = configurationRequestParameters; | ||
this.isObfuscated = isObfuscated; | ||
this.expectObfuscated = isObfuscated; |
There was a problem hiding this comment.
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
this.expectObfuscated = isObfuscated; | ||
} | ||
|
||
private maybeWarnAboutObfuscationMismatch(configObfuscated: boolean) { |
There was a problem hiding this comment.
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?
- The warning mechanism is sufficiently complicated that there's a bug in it (see above comment).
- 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.) - If it's not that useful, I would rather remove it completely. Maybe issue a quick deprecation warning in constructor when
isObfuscated
is supplied
if (this.configObfuscatedCache === undefined) { | ||
this.configObfuscatedCache = OBFUSCATED_FORMATS.includes( | ||
this.flagConfigurationStore.getFormat() ?? FormatEnum.SERVER, | ||
); | ||
} | ||
this.maybeWarnAboutObfuscationMismatch(this.configObfuscatedCache); | ||
return this.configObfuscatedCache; |
There was a problem hiding this comment.
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
/** | ||
* UFC Configuration formats which are obfuscated. | ||
*/ | ||
export const OBFUSCATED_FORMATS: string[] = [FormatEnum.CLIENT, FormatEnum.PRECOMPUTED]; |
There was a problem hiding this comment.
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.)
labels: mergeable
Towards FF-3886
Motivation and Context
The configuration served now contains the
format
field which indicates whether it is an obfuscated configuration payload. This change is made as part of ongoing efforts to clean up the construction and initialization of the EppoClient (and sub-classes) to allow for multiple instance of the EppoClient. .isObfuscated
is a low-hanging fruit.Description
isObfuscated
param inEppoClientParameters
format
field (set onto the flag configuration store when the response is received).setObfuscated
api, but track the variable asexpectObfuscated
andHow has this been tested?