Skip to content

feat: add tailnet and agent API definitions #10324

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

Merged
merged 1 commit into from
Oct 30, 2023

Conversation

spikecurtis
Copy link
Contributor

Adds API definitions and packages for Tailnet and Agent APIs (API version 2.0)

Copy link
Contributor Author

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

Copy link
Member

@kylecarbs kylecarbs left a comment

Choose a reason for hiding this comment

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

Customers use the HTTP endpoint of this to manually send logs.

How do we imagine the implementation logic that forks the gRPC and HTTP handler in coderd?

Copy link
Contributor Author

Customers use the HTTP endpoint of this to manually send logs.

How do we imagine the implementation logic that forks the gRPC and HTTP handler in coderd?

I was thinking an Agent API service, with an HTTP wrapper for the "legacy" methods. If logging isn't going to be deprecated we could leave the existing HTTP handler for that on the main API. I haven't looked at the code in detail, so it may or may not make sense for the dRPC to share code with the HTTP; either is fine.

@spikecurtis spikecurtis force-pushed the spike/agent-tailnet-api-defs branch from ef6cead to 5934176 Compare October 19, 2023 05:39
Copy link
Contributor Author

One thing I've just added, for those watching, are messages to the Coordinator RPC for adding and removing subscriptions (now called Tunnels).

I didn't realize when I originally created the API that there is a second wire protocol that Workspace Proxies use to communicate with Coderd, and I'd like to cover that use case as well such that there is a single wire protocol for tailnet.

Please look at the RFC for more details: https://www.notion.so/coderhq/Agent-Tailnet-APIs-386764332c134e0981946cefe657fd83?pvs=4#9f55e1837ecc4c9ea93cf5760f20c007

@spikecurtis spikecurtis requested a review from kylecarbs October 19, 2023 08:46
@mafredri
Copy link
Member

I was thinking an Agent API service, with an HTTP wrapper for the "legacy" methods. If logging isn't going to be deprecated we could leave the existing HTTP handler for that on the main API. I haven't looked at the code in detail, so it may or may not make sense for the dRPC to share code with the HTTP; either is fine.

I think it'd be possible to share a lot of the code paths by accepting JSON requests and unmarshaling them into protobuf messages, e.g. https://pkg.go.dev/google.golang.org/protobuf/encoding/protojson

This way we don't even need to duplicate the representation, generating the API documentation is a question-mark, though.

Copy link
Contributor Author

I don't think protojson encoding/decoding is compatible with standard encoding/json. So, getting our existing message formats to decode into protobuf is a tall order, and would likely gum up the new API with weird legacy stuff

@spikecurtis spikecurtis force-pushed the spike/agent-tailnet-api-defs branch 2 times, most recently from ec609ce to c18c14d Compare October 19, 2023 11:12
@spikecurtis spikecurtis requested a review from mafredri October 19, 2023 11:13
Copy link
Member

@mafredri mafredri left a comment

Choose a reason for hiding this comment

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

One last thing caught my eye, but all in all this looks good to me!


service Agent {
rpc GetManifest(GetManifestRequest) returns (Manifest);
rpc GetServiceBanner(GetServiceBannerRequest) returns (ServiceBanner);
Copy link
Member

Choose a reason for hiding this comment

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

I would be in favor of dropping the Get here (as typical in Go). Possibly also Batch in the updates. Batch doesn't feel like it adds any value if it's the only method. And pluralization can do the same if we want to convey it.

@kylecarbs
Copy link
Member

I'm not sure I'd consider the HTTP routes legacy... many of our customers (whether we like it or not) have cURL'd our agent HTTP endpoints to make some kinda magic happen, and we do it for things like gitauth.

This obviously doesn't prevent that though, so I'm good with it!

@github-actions github-actions bot added the stale This issue is like stale bread. label Oct 27, 2023
@github-actions github-actions bot closed this Oct 30, 2023
@spikecurtis spikecurtis reopened this Oct 30, 2023
@spikecurtis spikecurtis force-pushed the spike/agent-tailnet-api-defs branch from c18c14d to dda5c85 Compare October 30, 2023 07:16
@spikecurtis spikecurtis merged commit 2a6fd90 into main Oct 30, 2023
@spikecurtis spikecurtis deleted the spike/agent-tailnet-api-defs branch October 30, 2023 08:14
@github-actions github-actions bot locked and limited conversation to collaborators Oct 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
stale This issue is like stale bread.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants