Skip to content
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

backend: add service discovery interface and implement for single host deployments #2600

Merged
merged 5 commits into from Sep 25, 2020

Conversation

Rugvip
Copy link
Member

@Rugvip Rugvip commented Sep 24, 2020

Fixes #1847, #2596

Went with an interface similar to the frontend DiscoveryApi, since it's dead simple but still provides a lot of flexibility in the implementation.

Also ended up with two different methods, one for internal endpoint discovery and one for external. The two use-cases are explained a bit more in the docs, but basically it's service-to-service vs callback URLs.

This did get me thinking about uniqueness and that we're heading towards a global namespace for backend plugin IDs. That's probably fine tbh, but if we're happy with that we should leverage it a bit more to simplify the backend setup. For example we'd have each plugin provide its own ID and not manually mount on paths in the backend.

Draft until we're happy with the implementation, then I can add more docs and changelog entry. Also didn't go on a thorough hunt for places where discovery can be used, but tbh I don't think there are many since it's been pretty awkward to do service-to-service communication.

@Rugvip Rugvip changed the title backend: add service discovery interface and an implement for single host deployments backend: add service discovery interface and implement for single host deployments Sep 24, 2020
listenPort = DEFAULT_PORT,
} = readBaseOptions(config.getConfig('backend'));
const protocol = config.has('backend.https') ? 'https' : 'http';
const internalBaseUrl = `${protocol}://${listenHost}:${listenPort}`;
Copy link
Member

Choose a reason for hiding this comment

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

Can listenHost be :: or 0.0.0.0 for example? Is it always possible to form the url using these components?

Copy link
Member Author

Choose a reason for hiding this comment

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

That should work just fine, changed it to support IPv6 though

Copy link
Member Author

@Rugvip Rugvip Sep 25, 2020

Choose a reason for hiding this comment

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

Probably nicer to translate those to localhost even if it happens to work though, updated that too

packages/backend-common/src/discovery/types.ts Outdated Show resolved Hide resolved
packages/backend-common/src/discovery/types.ts Outdated Show resolved Hide resolved
packages/backend-common/src/discovery/types.ts Outdated Show resolved Hide resolved
plugins/proxy-backend/src/service/router.ts Outdated Show resolved Hide resolved

const catalogRes = await fetch(`${catalogUrl}/entities/by-name/${triple}`);
if (!catalogRes.ok) {
catalogRes.body.pipe(res.status(catalogRes.status));
Copy link
Member

Choose a reason for hiding this comment

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

Fancy. And this works whether there's a body or not?

Copy link
Member Author

Choose a reason for hiding this comment

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

Should, an empty pipe is still a pipe, no? 😁

@Rugvip Rugvip force-pushed the rugvip/disco branch 3 times, most recently from c9bb58c to 880d8b9 Compare September 25, 2020 08:04
@Rugvip Rugvip marked this pull request as ready for review September 25, 2020 10:37
@Rugvip Rugvip requested review from a team as code owners September 25, 2020 10:37
Copy link
Member

@benjdlambert benjdlambert left a comment

Choose a reason for hiding this comment

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

Looks good to me. Any docs we should update too?

@Rugvip
Copy link
Member Author

Rugvip commented Sep 25, 2020

@benjdlambert couldn't find any docs to update, cept for the changelog

@benjdlambert
Copy link
Member

Ok maybe one for an upcoming docs party.

@benjdlambert
Copy link
Member

@spotify/techdocs-core can I get your approval too?

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.

Use DiscoveryApi to find the catalog API route in techdocs-backend router. RFC: Backend Service Discovery
4 participants