-
Notifications
You must be signed in to change notification settings - Fork 390
feat(services): U2C cache logic #4593
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
base: next
Are you sure you want to change the base?
Changes from all commits
944c94c
94a2838
b7fd012
8643dd9
69d8b17
06aa9d1
beb8df4
41e9e11
ea1306a
c67b0c2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,9 @@ const CLUSTER_SERVICE = process.env.WEBEX_CONVERSATION_CLUSTER_SERVICE || DEFAUL | |
| const DEFAULT_CLUSTER_IDENTIFIER = | ||
| process.env.WEBEX_CONVERSATION_DEFAULT_CLUSTER || `${DEFAULT_CLUSTER}:${CLUSTER_SERVICE}`; | ||
|
|
||
| const CATALOG_CACHE_KEY_V2 = 'services.v2.u2cHostMap'; | ||
| const CATALOG_TTL_MS = 24 * 60 * 60 * 1000; // 24 hours | ||
|
|
||
| /* eslint-disable no-underscore-dangle */ | ||
| /** | ||
| * @class | ||
|
|
@@ -110,6 +113,7 @@ const Services = WebexPlugin.extend({ | |
| * @returns {Array<ServiceHost>} - An array of `ServiceHost` objects. | ||
| */ | ||
| getMobiusClusters(): Array<ServiceHost> { | ||
| this.logger.info('services: fetching mobius clusters'); | ||
| const clusters: Array<ServiceHost> = []; | ||
| const services: Array<Service> = this._services || []; | ||
|
|
||
|
|
@@ -249,6 +253,22 @@ const Services = WebexPlugin.extend({ | |
| serviceHostMap?.services, | ||
| serviceHostMap?.timestamp | ||
| ); | ||
| // Build selection metadata for caching discrimination (preauth/signin) | ||
| let selectionMeta: {selectionType: string; selectionValue: string} | undefined; | ||
| if (serviceGroup === 'preauth' || serviceGroup === 'signin') { | ||
| try { | ||
| const key = formattedQuery && Object.keys(formattedQuery || {})[0]; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we expecting only one key at a time ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes at a time we would use either orgId or email or location to fetch the catalog |
||
| if (key) { | ||
| selectionMeta = { | ||
| selectionType: key, | ||
| selectionValue: formattedQuery[key], | ||
| }; | ||
| } | ||
| } catch { | ||
| this.logger.warn('services: error building selection meta'); | ||
| } | ||
|
Comment on lines
+259
to
+269
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. see if we need try-catch for this |
||
| } | ||
| this._cacheCatalog(serviceGroup, serviceHostMap, selectionMeta); | ||
| this.updateCredentialsConfig(); | ||
| catalog.status[serviceGroup].collecting = false; | ||
| }) | ||
|
|
@@ -934,6 +954,216 @@ const Services = WebexPlugin.extend({ | |
| return url.replace(data.defaultUrl, data.priorityUrl); | ||
| }, | ||
|
|
||
| /** | ||
| * @private | ||
| * Cache the catalog in the bounded storage. | ||
| * @param {ServiceGroup} serviceGroup - preauth, signin, postauth | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we allowing this to cache all service groups ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes We are caching all service groups |
||
| * @param {ServiceHostmap} hostMap - The hostmap to cache | ||
| * @param {object} [meta] - Optional selection metadata for cache discrimination | ||
| * @returns {Promise<void>} | ||
| */ | ||
| async _cacheCatalog( | ||
| serviceGroup: ServiceGroup, | ||
| hostMap: ServiceHostmap, | ||
| meta?: {selectionType: string; selectionValue: string} | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can create type for meta, this is repeated |
||
| ): Promise<void> { | ||
| let current: {orgId?: string; env?: {fedramp?: boolean; u2cDiscoveryUrl?: string}} = {}; | ||
| let orgId: string | undefined; | ||
| try { | ||
| // Respect calling.cacheU2C toggle; if disabled, skip writing cache | ||
| if (this.webex.config?.calling && this.webex.config.calling.cacheU2C === false) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we making the config mandatory ? If not, then default value should be false if we are adding check with
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not a mandatory config but by default we would like to keep it false unless passed otherwise
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have updated the condition a bit now |
||
| return; | ||
| } | ||
|
|
||
| try { | ||
| const raw = | ||
| typeof window !== 'undefined' && (window as any).localStorage | ||
| ? (window as any).localStorage.getItem(CATALOG_CACHE_KEY_V2) | ||
| : null; | ||
| current = raw ? JSON.parse(raw) : {}; | ||
| } catch { | ||
| current = {}; | ||
| } | ||
|
|
||
| try { | ||
| const {credentials} = this.webex; | ||
| orgId = credentials.getOrgId(); | ||
| } catch { | ||
| orgId = current.orgId; | ||
| } | ||
|
Comment on lines
+988
to
+993
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is not possible to do so, let's discuss this Monday |
||
|
|
||
| // Capture environment fingerprint to invalidate cache across env changes | ||
| let env: {fedramp?: boolean; u2cDiscoveryUrl?: string} | undefined; | ||
| try { | ||
| const fedramp = !!this.webex.config?.fedramp; | ||
| const u2cDiscoveryUrl = this.webex.config?.services?.discovery?.u2c; | ||
| env = {fedramp, u2cDiscoveryUrl}; | ||
| } catch { | ||
| env = current.env; | ||
| } | ||
|
|
||
| const updated = { | ||
| ...current, | ||
| orgId: orgId || current.orgId, | ||
| env: env || current.env, | ||
| // When selection meta is provided, store as an object; otherwise keep legacy shape | ||
| [serviceGroup]: meta ? {hostMap, meta} : hostMap, | ||
| cachedAt: Date.now(), | ||
| }; | ||
|
|
||
| if (typeof window !== 'undefined' && (window as any).localStorage) { | ||
| (window as any).localStorage.setItem(CATALOG_CACHE_KEY_V2, JSON.stringify(updated)); | ||
| } | ||
| } catch { | ||
| this.logger.warn('services: error caching catalog'); | ||
|
Comment on lines
+1017
to
+1018
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added in all places |
||
| } | ||
| }, | ||
|
|
||
| /** | ||
| * @private | ||
| * Load the catalog from cache and hydrate the in-memory ServiceCatalog. | ||
| * @returns {Promise<boolean>} true if cache was loaded, false otherwise | ||
| */ | ||
| async _loadCatalogFromCache(): Promise<boolean> { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's break the logic into 3 different methods for each service group -
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could ypou please help me understand how that would help here and why is that needed |
||
| let currentOrgId: string | undefined; | ||
| try { | ||
| // Respect calling.cacheU2C toggle; if disabled, skip using cache | ||
| if (this.webex.config?.calling && this.webex.config.calling.cacheU2C === false) { | ||
| return false; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's add log for this too |
||
| } | ||
|
|
||
| if (typeof window === 'undefined' || !(window as any).localStorage) { | ||
| return false; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here, we should add log
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added |
||
| } | ||
| const raw = (window as any).localStorage.getItem(CATALOG_CACHE_KEY_V2); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If possible, let's create the util for the localStorage, there we can add all the browser related checks |
||
| const cached = raw ? JSON.parse(raw) : undefined; | ||
| if (!cached) { | ||
| return false; | ||
| } | ||
|
|
||
| // TTL enforcement | ||
| const cachedAt = Number(cached.cachedAt) || 0; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's add check so that the value is not
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But why would that happen when we are specifically typecasting it using Number() and we have a null check for cahed already right so value cannot be undefined |
||
| if (!cachedAt || Date.now() - cachedAt > CATALOG_TTL_MS) { | ||
| try { | ||
| this.clearCatalogCache(); | ||
| } catch { | ||
| this.logger.warn('services: error clearing catalog cache'); | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| // If authorized, ensure cached org matches | ||
| try { | ||
| if (this.webex.credentials?.canAuthorize) { | ||
| const {credentials} = this.webex; | ||
| currentOrgId = credentials.getOrgId(); | ||
| if (cached.orgId && cached.orgId !== currentOrgId) { | ||
| return false; | ||
| } | ||
| } | ||
| } catch { | ||
| this.logger.warn('services: error checking orgId'); | ||
| } | ||
|
|
||
| // Ensure cached environment matches current environment | ||
| try { | ||
| const fedramp = !!this.webex.config?.fedramp; | ||
| const u2cDiscoveryUrl = this.webex.config?.services?.discovery?.u2c; | ||
| const currentEnv = {fedramp, u2cDiscoveryUrl}; | ||
| if (cached.env) { | ||
| const sameEnv = | ||
| cached.env.fedramp === currentEnv.fedramp && | ||
| cached.env.u2cDiscoveryUrl === currentEnv.u2cDiscoveryUrl; | ||
| if (!sameEnv) { | ||
| return false; | ||
| } | ||
| } | ||
| } catch (e) { | ||
| this.logger.warn('services: error checking environment', e); | ||
| } | ||
|
|
||
| const catalog = this._getCatalog(); | ||
| const groups: Array<ServiceGroup> = ['preauth', 'signin', 'postauth']; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. groups --> serviceGroups |
||
|
|
||
| // Helper: compute intended preauth selection based on current context | ||
| const getIntendedPreauthSelection = () => { | ||
| if (this.webex.credentials?.canAuthorize) { | ||
| if (currentOrgId) { | ||
| return {selectionType: 'orgId', selectionValue: currentOrgId}; | ||
| } | ||
| } | ||
|
|
||
| const emailConfig = this.webex.config && this.webex.config.email; | ||
|
|
||
| if (typeof emailConfig === 'string' && emailConfig.trim()) { | ||
| return { | ||
| selectionType: 'emailhash', | ||
| selectionValue: sha256(emailConfig.toLowerCase()).toString(), | ||
| }; | ||
| } | ||
|
|
||
| // fall back to proximity mode when no orgId or email available | ||
| return {selectionType: 'mode', selectionValue: 'DEFAULT_BY_PROXIMITY'}; | ||
| }; | ||
|
Comment on lines
+1089
to
+1108
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can be moved out of this method |
||
|
|
||
| groups.forEach((g) => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. g --> serviceGroup |
||
| const cachedGroup = cached[g]; | ||
| if (!cachedGroup) { | ||
| return; | ||
| } | ||
|
|
||
| // Support legacy (hostMap) and new ({hostMap, meta}) shapes | ||
| const hostMap: ServiceHostmap = | ||
| cachedGroup && cachedGroup.hostMap ? cachedGroup.hostMap : cachedGroup; | ||
| const meta: {selectionType: string; selectionValue: string} | undefined = | ||
| cachedGroup && cachedGroup.meta ? cachedGroup.meta : undefined; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| if (g === 'preauth' && meta) { | ||
| // For proximity-based selection, always fetch fresh to respect IP/region changes | ||
| if (meta.selectionType === 'mode') { | ||
| return; | ||
| } | ||
|
|
||
| const intended = getIntendedPreauthSelection(); | ||
| const matches = | ||
| intended && | ||
| intended.selectionType === meta.selectionType && | ||
| intended.selectionValue === meta.selectionValue; | ||
| if (!matches) { | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| if (hostMap) { | ||
| catalog.updateServiceGroups(g, hostMap?.services, hostMap?.timestamp); | ||
| } | ||
| }); | ||
|
|
||
| this.updateCredentialsConfig(); | ||
|
|
||
| return true; | ||
| } catch { | ||
| return false; | ||
| } | ||
| }, | ||
|
|
||
| /** | ||
| * Clear the catalog cache from the bounded storage (v2). | ||
| * @returns {Promise<void>} | ||
| */ | ||
| clearCatalogCache(): Promise<void> { | ||
| try { | ||
| if (typeof window !== 'undefined' && (window as any).localStorage) { | ||
| (window as any).localStorage.removeItem(CATALOG_CACHE_KEY_V2); | ||
| } | ||
|
Comment on lines
+1157
to
+1159
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here, having a util will simplify the logic |
||
| } catch { | ||
| this.logger.warn('services: error clearing catalog cache'); | ||
| } | ||
|
|
||
| return Promise.resolve(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we resolve/reject based on try-catch ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How does rejecting it helps ? We are not going to use these methods to change any flow. All it needs to do is cache it and use the cache but any kind of faikure should not block the flows |
||
| }, | ||
|
|
||
| /** | ||
| * @private | ||
| * Simplified method wrapper for sending a request to get | ||
|
|
@@ -1093,7 +1323,14 @@ const Services = WebexPlugin.extend({ | |
|
|
||
| // wait for webex instance to be ready before attempting | ||
| // to update the service catalogs | ||
| this.listenToOnce(this.webex, 'ready', () => { | ||
| this.listenToOnce(this.webex, 'ready', async () => { | ||
| const warmed = await this._loadCatalogFromCache(); | ||
| if (warmed) { | ||
| catalog.isReady = true; | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| const {supertoken} = this.webex.credentials; | ||
| // Validate if the supertoken exists. | ||
| if (supertoken && supertoken.access_token) { | ||
|
|
||
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.
remove the console log
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.
removed