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

Expose GatewayServices endpoint in HTTP API #8099

Merged
merged 7 commits into from
Jun 12, 2020
Merged

Conversation

freddygv
Copy link
Contributor

@freddygv freddygv commented Jun 12, 2020

This PR moves the GatewayServices out of internal and into the Catalog API.

The reason for doing this is to facilitate 3rd party integrations, such as when proxies other than Envoy wish to be configured as a gateway for Consul's service mesh. This is the endpoint we use internally to determine what services an ingress/terminating gateway will route to.

There were a few things that needed to happen, so this is best consumed by commit:

  1. Moved the RPC endpoint to be under the catalog, since that's where the state store operations are. (mostly plumbing)
  2. Exposed an HTTP endpoint.
  3. Refactored usage of structs.ServiceID to use structs.ServiceName (mostly plumbing). This was because the JSON output of ServiceID has the following fields, which could cause confusion when users expect a service name:
{
	"ID": "$id",
	"Namespace: "$ns"
}
  1. Added docs

Remaining TODO:

  • Fix up Enterprise

@freddygv freddygv added this to the 1.8.0 milestone Jun 12, 2020
@freddygv freddygv requested a review from a team June 12, 2020 15:14
Copy link
Contributor

@crhino crhino left a comment

Choose a reason for hiding this comment

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

Few comments from initially looking over the code.

I was slightly concerned about the big rename of ServiceID -> ServiceName potentially causing some changes in other API return values, but from going through the rename it looks like everything that was changed was just internal bookkeeping. No return values of state store methods/endpoints changed. 👍

As for the API endpoint, /v1/catalog/gateway-services/ lines up nicely with /v1/catalog/node-services/. The other potential I thought of was /v1/connect/gateway-services/, since it relates to service mesh, but no strong opinions there.

Will test it out locally next.

agent/structs/structs.go Show resolved Hide resolved
website/pages/api-docs/catalog.mdx Outdated Show resolved Hide resolved
@crhino
Copy link
Contributor

crhino commented Jun 12, 2020

Thinking more, I do wonder if gateway-service is the correct name for an API, since we are not really returning much of the same data that the other *-service endpoints do.

Going by current conventions, I think /v1/catalog/gateway-services/ accurately describes "Return to me a list of services associated to a specific gateway", but the data model we return back is very different than /v1/catalog/node-services/. The data model is more of a mapping than the other catalog endpoints. Thoughts?

@banks
Copy link
Member

banks commented Jun 12, 2020

Great points @chrino.

I think on the whole /v1/catalog/gateway-services/ seems like a logical home for this. I'm OK with it also returning a slightly different format than other catalog endpoints - we already return different result formats for /catalog/services (just a list with tags) and /catalog/node/:node (which despite the name is mostly list of services on a single node. The other /services or /connect endpoints are for generic discovery but at specific to a single target service.

Since the use-case here is different it makes sense to me that the format is different since we are querying for a mapping - it's close to the /catalog/node/:node in the sense that it's a mapping of services exposed through a specific gateway service but the actual data needed is different to that seems OK to me.

So yeah, didn't read all the diff yet but the API decision here LGTM 👍 .

Comment on lines +168 to +169
| `consul.client.api.success.catalog_gateway_services.` | This increments whenever a Consul agent successfully responds to a request to list services associated with a gateway. | requests | counter |
| `consul.client.rpc.error.catalog_gateway_services.` | This increments whenever a Consul agent receives an RPC error for a request to list services associated with a gateway. | errors | counter |
Copy link
Contributor

Choose a reason for hiding this comment

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

We have mismatched docs and implementations here, I think we probably want 3 metrics here, like catalog_node_services:

  1. consul.client.api.catalog_gateway_services.
  2. consul.client.api.error.catalog_gateway_services.
  3. consul.client.api.sucess.catalog_gateway_services.

The docs have success and error, while the implementation has consul.client.api.catalog_gateway_services. and error.

Copy link
Contributor

@crhino crhino left a comment

Choose a reason for hiding this comment

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

Ok, tested it out locally and it looks good. 1 remaining comment around making sure to add the success metric for the endpoint and updated the telemetry docs, but otherwise looks good to me. 👍

@freddygv freddygv merged commit 965c80e into master Jun 12, 2020
@freddygv freddygv deleted the gateway-services-endpoint branch June 12, 2020 21:14
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.

None yet

3 participants