feat(portal): add dynamic service registry with manifest integration (#278) - #296
Conversation
…lth checking, and graceful degradation
DenizAltunkapan
left a comment
There was a problem hiding this comment.
Good direction, this is exactly what #278 asked for and the structure (manifest validation, graceful degradation, specs) is solid. No conflicts and CI is green. A few things need fixing before this can go in, mostly around deployment reality and boot behaviour.
Summary of the requested changes, details inline:
- The manifests hardcode localhost URLs, which makes every service show as offline in any non dev deployment. That defeats the drift problem this PR sets out to solve.
- The APP_INITIALIZER blocks app boot on all health checks with no timeout.
- Health checks go through the token interceptor, so the auth token is sent to whatever URL a manifest declares, and a down service triggers the global backend unavailable banner at startup.
- The navbar reads the registry once in ngOnInit, which only works because of the blocking initializer.
- Smaller items: apiUrl is required by the interface but not validated, and the habits manifest points at a route that does not exist.
One more nit outside the diff: the PR description links to a local file path (file:///d:/...), please swap that for a repo link.
… non-blocking APP_INITIALIZER, and reactive navbar
|
Went through 47efe67. All five points from my review are properly addressed:
One thing to fix before I approve, and it came in with this last commit. In habits-service.json, route changed from /habits to http://{host}:8092. route is the in-app router path, not a service URL, and baseUrl already carries the host. With the new isExternalUrl branch in the navbar, habits now renders as an external anchor pointing at a bare origin instead of a router link. cloud-service.json and password-manager-service.json both kept their /cloud and /passwords paths correctly, so habits is the only one affected. Set it back to /habits and this is good to go. The isExternalUrl handling itself is worth keeping for services that genuinely live on another origin. |
There was a problem hiding this comment.
Update: on re-reading this against the PR's stated goal, I'm walking back my earlier approval. My review verified that the previous round's comments were resolved, but I missed that the manifest filename list is still hardcoded in loadServices() — which means the core config-drift problem this PR set out to fix isn't actually solved, just moved. Deniz's latest comments (host resolution fragility, unused isLoaded, unenforced requiredScopes/tokenForwarding, hardcoded ports, test seam) all look valid too. Agreeing these need to be addressed before merge.
DenizAltunkapan
left a comment
There was a problem hiding this comment.
Thanks for putting this together, the idea of moving away from a hardcoded service list is the right direction. I have a few concerns about whether this implementation actually achieves that goal, plus one thing that looks like it could cause bugs in production. Left inline notes below.
| } | ||
|
|
||
| loadServices(): Observable<ServiceManifest[]> { | ||
| const manifests = [ |
There was a problem hiding this comment.
This list of manifest filenames is still hardcoded in the app itself. The PR description says the goal is to avoid editing Vault Web when a service is added or changed, but with this array in place, adding a new service still requires a code change here. I think this needs to come from something external, like a single root manifest that lists the available services, or a build time config, otherwise we have not actually removed the config drift problem, we have just moved it from one hardcoded list to another.
| const currentHost = window.location.hostname; | ||
| const currentProtocol = window.location.protocol; | ||
| let resolved = url | ||
| .replace('{host}', currentHost) |
There was a problem hiding this comment.
This string replacement approach for resolving the host is fragile. url.replace('localhost', currentHost) will replace the first occurrence of the literal text localhost anywhere in the string, not just the hostname portion, so if a path or query string ever happens to contain that word this will silently produce a broken URL. I would rather see this done with the URL API, something like constructing a URL object and only touching the hostname property, so we are not doing blind text substitution on production URLs.
| private servicesSubject = new BehaviorSubject<ServiceManifest[]>([]); | ||
| public services$ = this.servicesSubject.asObservable(); | ||
|
|
||
| private isLoaded = false; |
There was a problem hiding this comment.
isLoaded is set to true in loadServices but I do not see it read anywhere else in the class. Either it is dead state that should be removed, or there was an intended guard against calling loadServices multiple times that never got wired in.
| apiUrl: string; | ||
| healthEndpoint: string; | ||
| requiredScopes?: string[]; | ||
| tokenForwarding?: { |
There was a problem hiding this comment.
requiredScopes and tokenForwarding are defined on the manifest and documented in the schema doc, but I cannot find anywhere in this PR where they are actually read or enforced. Advertising scopes and token forwarding behavior that is not implemented yet feels risky from a security expectations standpoint, someone reading the schema doc would reasonably assume this is already handled. Could we either wire this up or leave it out of this PR until it is implemented, with a note that it is planned.
| "displayName": "Cloud", | ||
| "icon": "pi-cloud", | ||
| "route": "/cloud", | ||
| "baseUrl": "http://{host}:8090", |
There was a problem hiding this comment.
The port is still hardcoded in this manifest, same for the other two service manifests. That is probably fine for local development, but I want to understand how this is meant to work outside of localhost, for example in staging or production where services will not be sitting on fixed ports like 8090 through 8092. If that is out of scope for this PR it would help to say so explicitly, otherwise this reads as production configuration.
| }, | ||
| ]; | ||
|
|
||
| (service as any).servicesSubject.next(services); |
There was a problem hiding this comment.
Reaching into the private servicesSubject with a cast to any to set up this test is a sign the service could use a small public seam for testing, for example an optional setter or an exported testing helper. Not blocking, but worth cleaning up so the test does not depend on an internal implementation detail that could change without the test noticing.
Problem
Vault Web previously relied on a hardcoded list of services (Cloud, Password Manager) in its frontend configuration. Adding or modifying a service required editing Vault Web itself, making the portal and actual deployment prone to configuration drift.
Solution
Introduced a dynamic service registry manifest system:
vault-service.json) in docs/service-manifest-schema.md specifying how each module describes itself (display details, route, health endpoints, required scopes, token forwarding).cloud,passwords, andhabitsin the web root.ServiceRegistryServicetriggered via AngularAPP_INITIALIZERto dynamically load manifests at boot and perform automated health checks on each service's health endpoint.Verification Details
service-registry.service.spec.tsto assert manifest parsing, fallback handling, and health check integration.TOTAL: 21 SUCCESS).