fix: use explicit api versions for agent and tailnet #15508
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
22526ba to
2803ed7
Compare
59c864e to
0dc33fb
Compare
There was a problem hiding this comment.
Does this PR cover the case where proto changes are introduced without bumping the version? If it is I didn't quite catch how that results in a build failure. (I.e. is anything stopping me from appending a new method to DRPCAgentClient23?)
Other than that, looks good. If we can automate more of this process in the future that'd be good. 👍🏻
| return nil, nil, err | ||
| } | ||
| return proto.NewDRPCAgentClient(conn), tailnetproto.NewDRPCTailnetClient(conn), nil | ||
| } |
There was a problem hiding this comment.
Could some or most of these changes be automated via code-gen? How easy is it for someone unfamiliar with this domain to add a new API version without forgetting something in the process?
There was a problem hiding this comment.
Hard to automate directly from the .proto because it requires knowing which methods were added in which version.
We could annotate the RPCs somehow, in comments, but I'm hesitant to do that.
- As a matter of automation, I don't think the time spent to implement will save us time overall, considering this is something we change a few times per year.
- What I'm doing here only covers API changes that add new methods. You could also imagine adding new fields to existing messages in a back-compatible way.
- I don't want to entrench us too much into this particular paradigm by building a bunch of infrastructure around it. I'm not totally sure this is the right long-term way to manage the versions, so I want to keep it lightweight. We might change it depending on future changes to the API.
My assumption is that we just edited |
700a82f to
0d7aa15
Compare
0dc33fb to
50f124f
Compare
0d7aa15 to
1a3816c
Compare
50f124f to
7b37b40
Compare
|
@spikecurtis ok, thanks for the explanation. This got me thinking though, could we instead make the protocol version a type alias so you don't need to change all the code every time you're switching? I.e. type DRPCAgentClientLatest = DRPCAgentClient23
func (c *Client) ConnectRPCLatest(...) { return c.ConnectRPC23(...) }Edit: Thinking about this some more, this would essentially automatically bump the protocol in the common case for all consumers when you change the alias. Maybe that's not what we want. I was hoping to avoid the yank renaming across the code-base but may be unavoidable without some rethinking of the API. |
@mafredri I think if you did this, maybe we could do something with golden files and dump a JSON representation of the interface somewhere. If it changes, via a argument field change, new method, etc. The test would fail until you Just an idea to "snapshot" what the API is from a JSON POV and dump it into a test that can detect changes. @spikecurtis I know you mentioned it might not be worth spending a lot of time on this infrastructure. I'll defer to ya'll on it. |
Look, if we wanted to snapshot the API definition, we could just make a copy of the This isn't like a "golden file" where you're testing that the code gives some complex output that's easier to just compare wholesale rather than asserting individual facts about it. We could stash a snapshot of the current version in testdata and directly compare, so that if you edited the proto without creating a new version, tests would fail. Personally, I think CODEOWNERS should help here, but if y'all really want another robot to complain to you about stuff, we can do it. |
Oh yea 🤦. Nah, this is all fine with me 👍 |
|
Let's go with this for now, we can always revisit if the process is too cumbersome or has other shortcomings 👍🏻. |
7b37b40 to
7d0d79d
Compare
1a3816c to
916df4d
Compare
7d0d79d to
46718e6
Compare
Merge activity
|
Bumps the Tailnet and Agent API version 2.3, and creates some extra controls and machinery around these versions.
What happened is that we accidentally shipped two new API features without bumping the version.
ScriptCompletedon the Agent API in Coder v2.16 andRefreshResumeTokenon the Tailnet API in Coder v2.15.Since we can't easily retroactively bump the versions, we'll roll these changes into API version 2.3 along with the new WorkspaceUpdates RPC, which hasn't been released yet. That means there is some ambiguity in Coder v2.15-v2.17 about exactly what methods are supported on the Tailnet and Agent APIs. This isn't great, but hasn't caused us major issues because
Still it's good to get things squared away in terms of versions for SDK users and possible edge cases around client and server versions.
To mitigate against this thing happening again, this PR also:
With the protocol controllers stuff, we've sort of already abstracted the Tailnet API such that the interface type strategy won't work, but I'll work on getting the Controller to be version aware, such that it can check the API version it's getting against the controllers it has -- in a later PR.