fix(api): get env by secretKey #1908
Conversation
| @@ -20,11 +20,16 @@ interface EnvironmentAccount { | |||
| type EnvironmentAccountSecrets = Record<string, EnvironmentAccount>; | |||
|
|
|||
| export const defaultEnvironments = ['prod', 'dev']; | |||
| const CACHE_ENABLED = !(process.env['NANGO_CACHE_ENV_KEYS'] === 'false'); | |||
There was a problem hiding this comment.
we have a feature flag util (reading keys from redis)
const useCache = await featureFlags.isEnabled('use-env-secret-cache', 'global', true);
If you want to be able to enable/disable it live you would need to keep adding to the cache and check the flag only when retrieving the data https://github.com/NangoHQ/nango/pull/1908/files#diff-f3d9e285186619d6dfde44bdbbb720119863b18a3a80a41025f4a9c83e13705aL97
There was a problem hiding this comment.
Indeed, I wasn't aware of this feature.
I think, reading this flag from redis at each call would === caching the secrets to Redis, defeating the purpose of this :D I'd leave it like this unless you really find it better
There was a problem hiding this comment.
If you are worried about skewing the telemetry. You can check the flag outside of the tracking and pass the boolean to getAccountIdAndEnvironmentIdBySecretKey. Up to you though
| let str = decipher.update(enc, 'base64', 'utf8'); | ||
| str += decipher.final('utf8'); | ||
| return str; | ||
| } |
There was a problem hiding this comment.
Because those file are .cjs we can't really import our own code. It's either work locally but not in test, or the opposite :/
|
I added the encryption in test because there was a few edge case not possible to test without it and it's better to be closer to production. Can you re-review and test on your side please? |
Describe your changes
Fixes NAN-642
SecretKey is encrypted meaning we can't select an env by secretKey. The problem has been hidden by the fact that we maintain a in-memory cache, but new users could suffer from this.
Add a new column to store the hashed version of the
secretKeyBackfilled everything so we don't have a transition period
Add a flag to enable or disable local cache
Time the function in datadog so we can track the performance of it
Bonus: remove
getByAccountIdAndEnvironmentthat was duplicated bygetByIdBonus: add a way to properly serialize Error (we have a lot of those in the code but they don't actually work)