really lazy replace env vars to allow for parallel secret loading during plugin init #9
Conversation
kriszyp
left a comment
kriszyp
left a comment
There was a problem hiding this comment.
Ok. But we really should get the race condition addressed. Do you have any idea if it is bug in the secrets/env loader (not returning a promise that resolves after env loaded) or if it is a core component loader bug (not waiting for the promise to resolve)? Might be something for @Ethan-Arrowood to look at?
@kriszyp This really is a fun one. The harper app has the oauth plugin in its own config.yaml, before has a resources config with a resources.js that loads secrets, e.g. await loadsecrets() which downloads secrets from a service and stuffs those into process.env - but that happens after the oauth plugin env var substitution happens (currently). I've reprod this with a local secrets file in resources.js... |
|
Ok, I see what is going on here. I had actually thought we were using a plugin for loading secrets, which I believe would make this a little more straightforward since a secrets plugin could return a promise from |
| for (const [key, value] of Object.entries(rawOptions)) { | ||
| if (typeof value === 'string' && value.startsWith('${') && value.endsWith('}')) { | ||
| const envVar = value.slice(2, -1); |
There was a problem hiding this comment.
You no like regexes?
| for (const [key, value] of Object.entries(rawOptions)) { | |
| if (typeof value === 'string' && value.startsWith('${') && value.endsWith('}')) { | |
| const envVar = value.slice(2, -1); | |
| const re = /^\${(.+)}$/; | |
| for (const [key, value] of Object.entries(rawOptions)) { | |
| const m = typeof value === 'string' && value.match(re); | |
| if (m) { | |
| const envVar = m[1]; |
J/K, yours is more readable, though I do wonder which is faster...
There was a problem hiding this comment.
could be an interesting test...might try something later
| const missingFields = REQUIRED_PROVIDER_FIELDS.filter((key) => { | ||
| const value = config[key as keyof OAuthProviderConfig]; | ||
| // Consider env var placeholders as "missing" if they haven't been loaded yet | ||
| return !value || (typeof value === 'string' && value.startsWith('${') && value.endsWith('}')); |
There was a problem hiding this comment.
!value will cause 0 and '' (empty strings) to be true. Maybe this should check if value !== undefined or maybe !Object.hasOwn(config, key)?
| const missingFields = REQUIRED_PROVIDER_FIELDS.filter((key) => { | ||
| const value = config[key as keyof OAuthProviderConfig]; | ||
| return !value || (typeof value === 'string' && value.startsWith('${') && value.endsWith('}')); | ||
| }); |
There was a problem hiding this comment.
Duplicate code? Could this be handled by a function?
There was a problem hiding this comment.
should totally be pulled out :) -> https://github.com/HarperFast/oauth/pull/11/files
|
going to hold off on this pr to see if resolving this issue is sufficient: HarperFast/harper#29 |
resolves #8