Skip to content

feat(portal): add dynamic service registry with manifest integration (#278) - #296

Open
priyanshuvishwakarma273403 wants to merge 3 commits into
Vault-Web:mainfrom
priyanshuvishwakarma273403:feat/service-registry-dynamic
Open

feat(portal): add dynamic service registry with manifest integration (#278)#296
priyanshuvishwakarma273403 wants to merge 3 commits into
Vault-Web:mainfrom
priyanshuvishwakarma273403:feat/service-registry-dynamic

Conversation

@priyanshuvishwakarma273403

Copy link
Copy Markdown
Contributor

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:

  1. Manifest Schema Definition: Documented a standard JSON schema (vault-service.json) in docs/service-manifest-schema.md specifying how each module describes itself (display details, route, health endpoints, required scopes, token forwarding).
  2. Mock Manifests shipped: Shipped conformant manifests for cloud, passwords, and habits in the web root.
  3. Registry Startup & Health Verification: Added ServiceRegistryService triggered via Angular APP_INITIALIZER to dynamically load manifests at boot and perform automated health checks on each service's health endpoint.
  4. Graceful UI Degradation: Replaced hardcoded navigation links in the navbar (both desktop and mobile) with dynamically rendered items. Unreachable or missing services (such as Habits) degrade gracefully, displaying as disabled/greyed-out in the menu with an "offline" badge and descriptive tooltip.

Verification Details

  • Created unit tests in service-registry.service.spec.ts to assert manifest parsing, fallback handling, and health check integration.
  • Verified that all unit tests pass successfully (TOTAL: 21 SUCCESS).
  • Validated that Prettier formatting and ESLint are 100% clean (0 errors/warnings).

@DenizAltunkapan DenizAltunkapan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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.
  2. The APP_INITIALIZER blocks app boot on all health checks with no timeout.
  3. 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.
  4. The navbar reads the registry once in ngOnInit, which only works because of the blocking initializer.
  5. 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.

Comment thread frontend/public/cloud-service.json Outdated
Comment thread frontend/src/app/app.config.ts Outdated
Comment thread frontend/src/app/services/service-registry.service.ts Outdated
Comment thread frontend/src/app/navbar/navbar.component.ts Outdated
Comment thread frontend/src/app/services/service-registry.service.ts
Comment thread frontend/public/habits-service.json
… non-blocking APP_INITIALIZER, and reactive navbar
@DenizAltunkapan

Copy link
Copy Markdown
Member

Went through 47efe67. All five points from my review are properly addressed:

  • Host resolution via {host} templating and resolveUrl, including the https upgrade when the page is served over TLS
  • APP_INITIALIZER no longer awaits the health checks, and both the manifest loads and the checks carry a 3s timeout
  • Health checks run through a separate HttpClient built on HttpBackend, so the token interceptor and the error banners are bypassed
  • Navbar is reactive now via BehaviorSubject and services$
  • apiUrl is validated in isValidManifest and lines up with the schema doc

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.

@prashantpiyush1111 prashantpiyush1111 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 DenizAltunkapan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 = [

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?: {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants