ENG-2884 Add multi-host support #132
Conversation
Adds multi-host support to the CLI by optionally looking up the CLI configuration, including the backend host from Firebase. This allows us to specify separate backend hosts for each tenant.
- Adds an optional config to the /orgs/{orgId} document
- Upon login, If the config is found, the config in stored locally
- When executing a command, the local config is used, if found
src/drivers/firestore.ts
Outdated
| let firestore: Firestore; | ||
| let auth: Auth; |
There was a problem hiding this comment.
I think we should avoid mutable global variables like these. Esp if they are about authentication. We may make a mistake in updating them and then the user is attempting to log in to the wrong host (= P0 environment).
An idea here is to keep the module-level const app, firestore, auth objects and only initialize another firestore when we access the public orgs document. That would be a different method, not getDoc, but e.g. getPublicDoc and it creates its own firestore using the bootstrap config.
There was a problem hiding this comment.
I was able to move auth to a local variable. But it's a bit tricky to make everything const. Is this a concern even if these are not exported?
There was a problem hiding this comment.
imo not exported is good enough
src/drivers/config.ts
Outdated
|
|
||
| export const CONFIG_FILE_PATH = path.join(P0_PATH, "config.json"); | ||
|
|
||
| export let tenantConfig = bootstrapConfig; |
There was a problem hiding this comment.
A mutable variable at the module-level will lead to problems imo. We will have the wrong config in it, e.g. someone makes a change that overwrites its value directly.
Can this be accessed via a getter?
let tenantConfig = bootstrapConfig;
...
export const getTenantConfig = () => tenantConfig
src/commands/login.ts
Outdated
| if (orgData.config) { | ||
| await saveTenantConfig(orgData.config); | ||
| } |
There was a problem hiding this comment.
Doesn't this have to move up, right after if (!orgData) throw "Could not find organization";?
Afaict currently we will log the user in to the bootstrap project and whatever the org is configured for login there. I think for orgs that actually use a different host, the login information is going to be in the Firestore of that other host (= P0 environment), and the p0-prod document for their org will only contain a config that points to the right environment.
So we don't want to log these users in to our p0-prod. We want them to log in to their environment. When loginFn is called tenantConfig should already be updated with the right config.
There was a problem hiding this comment.
A second thought: we could always save tenantConfig and use the bootstrapConfig as a fallback right here. Then we don't have to pre-assign in drivers/config.ts, tenantConfig can be undefined until we save here.
await saveTenantConfig(orgData.config ?? bootstrapConfig);
Or, we could also read tenantConfig lazily in a getTenantConfig function that checks the variable, then checks the file, then loads. Since then you can check if tenantConfig === undefined.
src/commands/login.ts
Outdated
| @@ -32,6 +38,8 @@ export const login = async ( | |||
| args: { org: string }, | |||
| options?: { skipAuthenticate?: boolean } | |||
| ) => { | |||
| initializeFirebase({ useBootstrapConfig: true }); | |||
|
|
|||
There was a problem hiding this comment.
Seems the line below is the only getDoc call where we read a public collection.
Could we create a special getPublicDoc function that always initializes and uses the bootstrap config to retrieve the tenant config?
There was a problem hiding this comment.
This is a great suggestion. Done.
* Add publicDoc() function to read from bootstrap firestore * Ensure updated config is set before authentication * Move auth from global to local scope * Rename guard function to fsShutdownGuard for clarity
| [ | ||
| [ | ||
| "/Users/fabianjoya/.p0/config.json", |
There was a problem hiding this comment.
This is won't pass on other computers because it has the home folder in it. I think jest has some way of asserting on a pattern instead.
src/commands/login.ts
Outdated
| const identity = await loadCredentials({ noRefresh: true }); | ||
| await authenticateToFirebase(identity); |
There was a problem hiding this comment.
It used to call authenticate, so that function was reused in the login and all the other commands. I tried to understand why we can't reuse anymore. Can you explain please?
There was a problem hiding this comment.
It's because authenticate now saves the config & initializes firebase. I would like to rename the authenticate method now, but couldn't think of a better name.
And there used to be a circular call chain, which I am trying to break:
authenticate -> loadCredentials -> login -> authenticate
Adds multi-host support to the CLI by optionally looking up the CLI configuration, including the backend host, from Firestore. This allows us to specify separate backend hosts for each tenant.