Skip to content

feat: Instance-Aware Networking (RequestInterceptor) - #677

Draft
RawanMatar89 wants to merge 28 commits into
openedx:developfrom
zeit-labs:infra/04-instance-aware-networking
Draft

RawanMatar89 wants to merge 28 commits into
openedx:developfrom
zeit-labs:infra/04-instance-aware-networking

Conversation

@RawanMatar89

Copy link
Copy Markdown
Contributor

Base branch
infra/03-remote-config-fetch

Description

Routes outgoing requests through the currently selected instance's host and credentials instead of the app-level config, and refreshes tokens against that instance too. This is where the instance model (PR-1) and instance selection (PR-2) actually start affecting network traffic — before this PR, InstanceStore tracked a selection but nothing used it.

What's in this PR

RequestInterceptor

  • Takes a new instanceStore: InstanceProvider dependency (already registered by PR-2 — no new DI wiring needed).
  • adapt(...) now rewrites the host/scheme/port of any request built against the app's base URL to the selected instance's baseURL, via a new rewriteHostIfNeeded(_:). Path and query are left untouched. A request with no selected instance, or one not built against the app base URL in the first place (e.g. an SSO webview, a third-party SDK call), passes through unmodified.
  • refreshToken(...) now prefers the selected instance's baseURL/oAuthClientId over the app-level config, falling back to app config when no instance is selected. token_type stays app-level (unchanged) — no instance-level override for that field exists yet.

DI

  • NetworkAssembly.swift: RequestInterceptor registration now resolves and passes InstanceProvider.

Tests

  • RequestInterceptorTests.swift (new): selected-instance host rewrite (host/path/query preserved correctly), no-instance-selected passthrough, and non-app-base-URL requests left untouched.

Out of scope

  • Instance-scoped local storage / CoreData / downloads.
  • Session lifecycle across instance switches.

volodymyr-chekyrta and others added 26 commits February 12, 2024 09:36
fix: typo in translation.py inline comments
Adds the Sendable Tenant value type and TenantsConfig container that
later multi-tenant layers build on: per-tenant API/SSO/OAuth config,
UI feature flags (UIComponentsConfig, now @unchecked Sendable), and
raw branding fields for a later theming layer to render.

Tenant(dictionary:) parses config.yaml's new TENANTS block.

Not included here: remote catalog fetching, tenant selection/session
state, and anything reading TenantProvider — those land as separate
PRs. TENANTS data in config.yaml is illustrative, not real tenants.
Renames the Tenant domain model and its wire schema to Instance, per
community naming feedback. Tenant -> Instance, TenantsConfig ->
InstancesConfig, TenantProvider -> InstanceProvider,
TenantThemeColors -> InstanceThemeColors; config.yaml's TENANTS block
-> INSTANCES, TENANT_NAME -> INSTANCE_NAME,
IS_SWITCH_TENANT_LOGIN_ENABLED -> IS_SWITCH_INSTANCE_LOGIN_ENABLED.

Pure rename, no behavior change. KEY is left as-is (already generic).
SSO_URL_SUCCESSFUL_LOGIN/successfulSSOLoginURL is untouched here too --
its rename to SSO_FINISHED_URL is separate, upcoming work.
Adds TenantStore: the single source of truth for the selected tenant
and the tenant catalog, persisted to UserDefaults and posting
.tenantDidChange for observers. Auto-selects the sole tenant on a
single-tenant catalog, since there's no picker to choose it through.

Registered in NetworkAssembly (TenantStore/TenantProvider). Not
consumed by anything yet — RequestInterceptor and the rest of the
storage/session layers read it starting in later PRs.

Also fixes a real gap found while extracting this: the original
TenantStoreTests.swift was never added to Core.xcodeproj, so it
silently never ran as part of CoreTests.
Renames TenantStore -> InstanceStore (and its test file), plus the
DI registrations in NetworkAssembly.swift, per the community's
naming feedback. Matches the rename already applied on top of
infra/01-tenant-model.
Adds TenantApiService to fetch the tenant catalog JSON from a remote
endpoint, and TenantConfigLoader to orchestrate a live-fetch-first,
cache-fallback policy: try the network first, fall back to the last
successfully-parsed config on failure.

The remote JSON uses lowercase snake_case keys, so TenantsConfig gains
a normalizeRemoteKeys step that maps them to the existing YAML
upper-snake-case keys and reuses Tenant(dictionary:) for parsing —
no separate remote-specific model.

Registers TenantApiServiceProtocol and TenantConfigLoader in
NetworkAssembly. Nothing calls TenantConfigLoader.load() yet; wiring
it into app launch is a later PR.
Renames TenantApiService -> InstanceApiService, TenantConfigLoader ->
InstanceConfigLoader, TenantsConfigJSONTests -> InstancesConfigJSONTests,
and the remote JSON schema's wire keys (tenant_name -> instance_name,
is_switch_tenant_login_enabled -> is_switch_instance_login_enabled,
the TENANTS/tenants catalog wrapper -> INSTANCES/instances), per the
community's naming feedback.
Adds an optional INSTANCES_CATALOG_URL config key and a config.json
bundled with the app. InstanceConfigLoader now falls back to this
bundled catalog when no URL is configured (or the live fetch and
cache both fail), instead of falling straight to an empty catalog.

- ConfigProtocol.instancesCatalogURL: URL? (nil when unconfigured)
- InstanceApiService.url is now optional; throws .notConfigured when nil
- InstanceConfigLoader.loadBundled() reads config.json from the bundle
- process_config.py copies json_files (config.json) into the built
  bundle alongside config.plist
- default_config/{dev,stage,prod}/file_mappings.yaml gain a json_files
  list; only dev ships an actual config.json for now
- Regenerated ConfigProtocolMock (Mockolo) in every module to add the
  new instancesCatalogURL property

Also removes API_HOST_URL_HIDDEN_LOGIN entirely: the Instance field
(baseURLHiddenLogin), its InstanceKeys/CodingKeys entries, remote
JSON key mapping, and the example value in config.yaml/config.json.
It was unused outside InstancesConfig.swift's own parsing.
Same restructuring as infra/01-tenant-model: LOGO_URL/HEADER_BACKGROUND_URL
now live inside THEME (as logo_url/header_background_url, siblings of
light/dark) instead of their own top-level instance fields. Removed the
now-unused remoteToYAMLKeyMap entries for them -- with no map entry they
pass through normalizeRemoteKeys unchanged (lowercase), landing exactly
where Instance.init?(dictionary:) now looks for them.

- default_config/dev/config.json and config.yaml's example instance
  updated to match: THEME.light/dark now carry only accent_color
  (ThemeColorSet.derived(fromHex:light:dark:) derives the rest), logo_url
  moved under THEME, and the flat color field is dropped.
- Updated InstancesConfigJSONTests to the nested shape.
Same fix as infra/01-tenant-model: NAME/COLOR get explicit uppercase raw
values, and THEME's LIGHT/DARK/LOGO_URL/HEADER_BACKGROUND_URL wrapper
keys go uppercase instead of the lowercase shortcut that let them pass
the remote-key normalizer unmapped. Added matching entries to
remoteToYAMLKeyMap for all six so remote catalog responses (still
naturally lowercase/snake_case) keep normalizing correctly -- without
this, a remote instance missing an explicit uppercase NAME would have
silently been dropped as malformed.

Left untouched: the light/dark palette dicts' own internal field names
(accent_color and siblings) -- Theme's already-shipped contract from the
merged theme/01-theme-engine PR, not this file's to rename.

config.json's own casing isn't touched here -- it's about to be
restructured wholesale (app-level keys + INSTANCES wrapper) in the next
commit, so fixing it twice would be wasted work.
…le replace)

Replaces the old live-fetch-first/cache/bundled/empty fallback chain with
product's actual merge rule:

- Baseline = last-cached successful remote response, else the bundled
  catalog, else empty.
- A live fetch that returns >0 instances wholesale-replaces the baseline
  and becomes the new cache -- so a later offline launch shows the same
  catalog the user had last time, not a reset to the bundled default.
- A fetch that fails, or succeeds with zero instances, leaves the
  baseline (and the cache) untouched -- a reachable-but-empty catalog is
  not the same as an unreachable one and must not wipe out a good cache.
- No catalog URL configured -> InstanceApiService throws immediately,
  same fallback-to-baseline path as any other fetch failure.

fatalError guard for "no catalog URL and no valid local instance" is
deliberately not here -- that's launch-sequencing (RouteController), not
loader concern; it belongs with the DI/launch wiring work.
- default_config/{dev,stage,prod}/config.json restructured to
  {app-level keys, INSTANCES}: FIREBASE/FACEBOOK/MICROSOFT/GOOGLE/
  APPLE_SIGNIN/BRANCH/URI_SCHEME/APP_STORE_ID/INSTANCES_CATALOG_URL
  alongside the instance array under INSTANCES. Remote catalog
  responses are unaffected -- still a bare array, only the local file
  gained the wrapper.
- config.yaml deleted in all three environments; nothing read it
  anymore once process_config.py's build-time source moved to JSON
  (dev's INSTANCES block there was already dead code -- confirmed
  nothing but InstanceStoreTests hand-constructs Instance dictionaries
  directly, no runtime path ever parsed it).
- process_config.py: PlistManager now parses json_files via
  json.load() instead of config_files via yaml.safe_load() --
  load_config()/yaml_to_plist() -> load_config()/json_to_plist(),
  otherwise unchanged (same merge_dicts, same plist output shape).
  whitelabel.py's own PlistManager call site (imports the class from
  this file) updated to match -- it was about to silently feed YAML
  paths into a JSON-only loader.
- file_mappings.yaml (all three envs): dropped the now-unused `files:`
  (YAML) list, kept `json_files:`.
- NetworkAssembly.swift: InstanceApiServiceProtocol's URL now comes
  from the bundled config.json directly (new
  InstanceConfigLoader.bundledCatalogURL()), not
  ConfigProtocol.instancesCatalogURL -- that property is left in place
  but unused for now, to be removed together with the rest of
  ConfigProtocol's app-level shrinkage (firebase/facebook/etc. moving
  off YAML-via-Config) rather than regenerating all 9 Mockolo
  ConfigProtocolMocks twice for two separate small removals.

Deliberately NOT touched: config_settings.yaml and file_mappings.yaml
themselves stay YAML -- build-routing metadata, not app config content,
never part of what was agreed to move to JSON. Documentation/
CONFIGURATION_MANAGEMENT.md still describes the old YAML-based flow in
detail and needs a real rewrite, not just the one-line README pointer
fixed here -- flagging rather than attempting that in this commit.
…IENT_ID as a temporary bridge

Config (the sole ConfigProtocol implementation still wired into DI
pre-PR-9) hard-fatalError()s on launch if these 4 top-level plist keys
are missing. Removing them from config.yaml in the YAML-retirement
commit broke app launch entirely in the current (PR-4-without-PR-9)
state, since InstanceAwareConfig doesn't exist yet to take over.

Bridge values mirror the example instance's own values (dev) / the
same placeholder pattern already used elsewhere in this file (stage,
prod). Remove once PR-9's InstanceAwareConfig replaces Config as the
wired-in ConfigProtocol and these top-level keys are no longer read.
…erceptor)

Reuses the tenants branch's already-built host-rewrite approach (renamed
tenant -> instance): API is still constructed once against the app's
default ConfigProtocol.baseURL, and RequestInterceptor.adapt(...)
re-targets any request built against that host at the currently
selected instance's baseURL, preserving path/query/fragment. Requests
that already point elsewhere (SSO webviews, third-party SDKs) are left
untouched. refreshToken(...) prefers the selected instance's baseURL/
oAuthClientId, falling back to the app config for single-instance
deployments or before an instance is picked.

instanceStore comes in via the InstanceProvider protocol, already
registered in DI by PR-2 -- no new DI wiring beyond passing it through
to RequestInterceptor's initializer.
@openedx-webhooks openedx-webhooks added open-source-contribution PR author is not from Axim or 2U core contributor PR author is a Core Contributor (who may or may not have write access to this repo). labels Sep 16, 2026
@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @RawanMatar89!

This repository is currently maintained by @openedx/openedx-mobile-maintainers.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

  • If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
    • This process (including the steps you'll need to take) is documented here.
  • If it doesn't, simply proceed with the next step.
🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

  • Dependencies

    This PR must be merged before / after / at the same time as ...

  • Blockers

    This PR is waiting for OEP-1234 to be accepted.

  • Timeline information

    This PR must be merged by XX date because ...

  • Partner information

    This is for a course on edx.org.

  • Supporting documentation
  • Relevant Open edX discussion forum threads
🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

🔘 Update the status of your PR

Your PR is currently marked as a draft. After completing the steps above, update its status by clicking "Ready for Review", or removing "WIP" from the title, as appropriate.


Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

  • The size and impact of the changes that it introduces
  • The need for product review
  • Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

@github-project-automation github-project-automation Bot moved this to Needs Triage in Contributions Sep 16, 2026
@IvanStepanok

Copy link
Copy Markdown
Contributor

@RawanMatar89 Stop, nooooooo...
All we need is just add a few new values to yaml file. Something like this:
LMS_DIRECTORY:
ENABLED: true
DIRECTORY_URL: "https://providers.openedx-lms.stepanok.com/p/northwind-education-group/directory.json"
DIRECTORY_FILE: ""

And inside a JSON you are setup all the instances. As example something like this:
{
"format": "v1",
"provider": {
"name": "Northwind Education Group",
"tagline": "Five campuses, one app",
"logo": null
},
"include": [
{
"name": "Northwind College",
"description": "Main campus",
"url": "https://learn.northwind.edu",
"logo": "https://cdn.northwind.edu/logo.png",
"accent_color": "#002545",
"api": {
"feedback_email": "support@northwind.edu"
},
"theme": {
"accent_color_dark": "#4989bf",
"login_background": "https://cdn.northwind.edu/signin.png"
},
"ui_components": {
"course_unit_progress_enabled": true,
"course_dropdown_navigation_enabled": true
},
"dashboard": { "type": "list" }
}
]
}

We need to support JSON file by URL or locally by DIRECTORY_FILE value. We need them both. Please, do not remove config.yaml🙌

@mphilbrick211 mphilbrick211 moved this from Needs Triage to Waiting on Author in Contributions Sep 17, 2026
@RawanMatar89

Copy link
Copy Markdown
Contributor Author

@RawanMatar89 Stop, nooooooo... All we need is just add a few new values to yaml file. Something like this: LMS_DIRECTORY: ENABLED: true DIRECTORY_URL: "https://providers.openedx-lms.stepanok.com/p/northwind-education-group/directory.json" DIRECTORY_FILE: ""

And inside a JSON you are setup all the instances. As example something like this: { "format": "v1", "provider": { "name": "Northwind Education Group", "tagline": "Five campuses, one app", "logo": null }, "include": [ { "name": "Northwind College", "description": "Main campus", "url": "https://learn.northwind.edu", "logo": "https://cdn.northwind.edu/logo.png", "accent_color": "#002545", "api": { "feedback_email": "support@northwind.edu" }, "theme": { "accent_color_dark": "#4989bf", "login_background": "https://cdn.northwind.edu/signin.png" }, "ui_components": { "course_unit_progress_enabled": true, "course_dropdown_navigation_enabled": true }, "dashboard": { "type": "list" } } ] }

We need to support JSON file by URL or locally by DIRECTORY_FILE value. We need them both. Please, do not remove config.yaml🙌

@ivan-stepanok 😄 I don't think we're actually disagreeing on the shape here, let me untangle it:

The URL-or-local-file JSON directory you're describing is exactly what's already built on the branch this PR stacks on — InstanceConfigLoader fetches a JSON catalog from a URL, falls back to a bundled JSON catalog if the fetch fails, same schema either way. So that part isn't going anywhere.

The one thing that did change is the outer local app config — instead of keeping that as YAML and adding a separate JSON directory format on top, it's now JSON too (config.json), so the build script, the bundled catalog, and the remote catalog all read the same format instead of two. That's not about disliking YAML 😅 — it's cutting the second parser (and the two-schemas-to-keep-in-sync problem) out of the pipeline. It's also just finishing the direction we were already in — this is basically what I did with the instances block a year ago (only yaml was used), just now split cleanly into local + remote with one shared format instead of mixed in.

If there's something specific relying on config.yaml staying YAML, flag it and we'll account for it — but as a format choice on its own I'd rather keep the one-format pipeline. Let me know if that resolves it or if I'm missing something on your side. 🤝

@IvanStepanok

Copy link
Copy Markdown
Contributor

Thanks, @RawanMatar89 – the remote catalog looks good, and I'm fine with JSON becoming the build-time config source. The plist output is unchanged, so nothing shifts at runtime. A few things I'd like in before this merges, though:

A migration path. Anyone upgrading with an existing YAML config in their config_directory will just get a fatalError on launch with no hint why. process_config.py should detect a config.yaml with no config.json next to it and fail with a clear message – ideally shipping a small converter script alongside it.

Docs in the same PR. Documentation/CONFIGURATION_MANAGEMENT.md describes the YAML flow end to end, and config.yaml's inline comments are currently the only documentation several of these keys have. If the comments go away, that content needs to land in the doc here, not in a follow-up.

Pick one format. config_settings.yaml and file_mappings.yaml staying YAML leaves two formats side by side in the same folder, which is worse for whoever configures this than either option on its own. Either convert them too, or keep config.yaml and just add the catalog pointer key to it.

None of this is a blocker on the approach – just don't want to hand operators a silent break and stale docs.

@RawanMatar89

Copy link
Copy Markdown
Contributor Author

Thank you @IvanStepanok 🙏🏻 all three are fair, and none of them are done yet, so no pushback:

Migration path: confirmed the gap. I'll add an explicit check up front and ship a small converter script alongside it, since the schema didn't reshape anything moving formats.
Docs: you're right, the doc in this PR is still the pre-instance-model version. I have the rewritten one (JSON-only pipeline, remote catalog, schema conventions) but never actually pushed it — that's on me. Landing it in this PR, not a follow-up.
Format consolidation: also confirmed 👍🏻, config_settings.yaml and file_mappings.yaml are still YAML. I'll convert both to JSON rather than growing config.yaml back — both are small (a couple of keys / a one-item list), so it's a quick conversion, not a redesign. 🤝

Will push all three as part of this PR before asking for another look. 🙌

process_config.py previously just fell into the generic "config files not
found" exit when config.json didn't exist yet, with no indication that
config.yaml (retired in 85aa83f) was the reason. fail_if_unmigrated_yaml_config
now detects a leftover config.yaml with no sibling config.json and fails with
a message naming both files and the converter to run.

Add config_script/yaml_to_json_config.py: a small standalone converter
(reuses this project's existing PyYAML/json deps, no new dependency) so
operators don't hand-write the JSON.

See Ivan's review on openedx#677.
Last two YAML files in default_config/ -- everything else there has been
JSON since 85aa83f. Converts config_settings.yaml -> config_settings.json
and each environment's file_mappings.yaml -> file_mappings.json (via
yaml_to_json_config.py), updates process_config.py and whitelabel.py (which
duplicates the same config_settings/file_mappings parsing) to read JSON
instead.

Addresses Ivan's review on openedx#677: migration path, docs, and one format
instead of JSON+YAML side by side in the same folder.
@RawanMatar89

Copy link
Copy Markdown
Contributor Author

Hi @IvanStepanok, the three changes from your last review are in. Pushed and ready for another look. 👀

One thing worth flagging before you dig in: these are stacked PRs, so this one's diff only makes sense on top of the branches below it. Branch names show the order — infra/01-tenant-modelinfra/02-tenant-storeinfra/03-remote-config-fetch → infra/04-instance-aware-networking (this one). Could we review and approve/merge in that order, starting from infra/01-tenant-model? Reviewing this one in isolation will show you everything from the branches underneath it too, which makes it harder to tell what's actually new here.

One naming note: any branch still says "tenant" was created before we agreed on the tenant → instance rename, and its content is already renamed to instance throughout. Just a stale branch name, not a leftover reference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core contributor PR author is a Core Contributor (who may or may not have write access to this repo). open-source-contribution PR author is not from Axim or 2U

Projects

Status: Waiting on Author

Development

Successfully merging this pull request may close these issues.

8 participants