protobuf changes in preparation for `buf breaking` #26792

espadolini · 2023-05-23T18:46:26Z

This PR, together with the PRs on v11, v12 and v13:

sets the buf breaking configuration to WIRE_JSON (which, sadly, won't prevent us from breaking encoding/json encoding as specified by gogoproto.jsontag, but that doesn't seem to be happening anyway)
adds some missing field reservations for field numbers and names that were missed, from fields that have been deleted
removes some "forwards" field reservations that were added in lieu of backports or to make us aware of actual compatibility breaks
backports part of Proto changes for partial cache #24229 to v12 and v11 (just the unification of proto.WatchKind into types.WatchKind)

Contributes to #26688

With this PR it should be possible to get a clean buf breaking between master and the release branches, with the exception of errors pertaining to field 17 of UserCertsRequest in v11's authservice.proto (as described in #24817) and to errors pertaining to the renaming of RoleV5 to RoleV6 between v11 and later versions.

api/proto/teleport/legacy/client/proto/authservice.proto:329:3:Field "17" with name "RequesterName" on message "UserCertsRequest" changed option "json_name" from "MFAResponse" to "RequesterName".
api/proto/teleport/legacy/client/proto/authservice.proto:329:3:Field "17" on message "UserCertsRequest" changed type from "message" to "enum". See https://developers.google.com/protocol-buffers/docs/proto3#updating for wire compatibility rules and https://developers.google.com/protocol-buffers/docs/proto3#json for JSON compatibility rules.
api/proto/teleport/legacy/client/proto/authservice.proto:329:13:Field "17" on message "UserCertsRequest" changed name from "MFAResponse" to "RequesterName".

api/proto/teleport/legacy/client/proto/authservice.proto:69:5:Field "9" on message "Event" changed type from "types.RoleV5" to "types.RoleV6".
api/proto/teleport/legacy/client/proto/authservice.proto:1056:12:Field "1" on message "GetRolesResponse" changed type from "types.RoleV5" to "types.RoleV6".
api/proto/teleport/legacy/client/proto/authservice.proto:2440:66:RPC "GetCurrentUserRoles" on service "AuthService" changed response type from "types.RoleV5" to "types.RoleV6".
api/proto/teleport/legacy/client/proto/authservice.proto:2574:40:RPC "GetRole" on service "AuthService" changed response type from "types.RoleV5" to "types.RoleV6".
api/proto/teleport/legacy/client/proto/authservice.proto:2578:18:RPC "UpsertRole" on service "AuthService" changed request type from "types.RoleV5" to "types.RoleV6".

How to address backwards compatibility checking in v11 (maybe ignoring those specific errors, and accepting the lack of backwards compatibility check for the RoleV5 message?), and how to handle future resource versioning in general is out of scope for these PRs.

espadolini · 2023-05-25T09:38:21Z

This touches every test that refers to RoleV6 so it probably needs an explicit skip for the flaky test detector. The change in name was purely mechanical (gopls rename api/types/types.pb.go:X:Y RoleImpl, the same for RoleImplSpec, then message renaming in types.proto to match the renaming and then make grpc).

codingllama

Thanks for tackling this, your effort is very much appreciated.

codingllama · 2023-05-25T13:43:19Z

api/proto/teleport/legacy/types/types.proto

-// RoleV6 represents role resource specification
-message RoleV6 {
+// RoleImpl represents role resource specification
+message RoleImpl {


We talked about this before, but I'm not a fan of this rename. There's nothing "Impl" about the protos, they are the definition of the role. This introduces a poor name to our API surface and preserves less-relevant impl details.

I would rather:

Leave names as-is (RoleVx, RoleSpecVx). It's not great but at least it's in keeping with other types.

Rename Role(interface)->RoleI, RoleVx->Role, RoleSpecVx->RoleSpec. Better naming overall, and a better pattern for other types to follow.

The issue with the name with the V in it is that we should pick a name that works across teleport versions even if newer versions support a higher resource version; case in point, if we left RoleV6 in place we'd be looking at bumping the struct name to RoleV6 even in teleport 11, which only supports role resources between v3 and v5.

Calling it Role with an additional rename of the Role to RoleI works and I like it, but what are we going to call the next incompatible protobuf message for RoleI? Role2? And if we're never going to have another implementation of RoleI (and I suspect we might not), why even have an interface for it?

Should we be versioning types instead of versioning the resources within it, i.e. typesv1.Role vs typesv2.Role?

I don't think that works, we actively want to support multiple resource versions with a single protobuf type. Also, versioning "all the types" is kind of counterproductive, you'd need to copy everything when you need a new version of a single resource.

Maybe each group of related messages should be in its own package, probably together with a dedicated service, but that doesn't help the fact that we also need to be able to refer and unmarshal any of those from storage as necessary.

@espadolini

The issue with the name with the V in it is that we should pick a name that works across teleport versions even if newer versions support a higher resource version; case in point, if we left RoleV6 in place we'd be looking at bumping the struct name to RoleV6 even in teleport 11, which only supports role resources between v3 and v5.

Keeping the resource "version numbers" stable, starting from v13, wouldn't be a bad way to "fix" our chronic habit of breaking name changes. We'd have to live with multiple role versions while v12 and v11 are active, but over time it stabilizes.

Calling it Role with an additional rename of the Role to RoleI works and I like it, but what are we going to call the next incompatible protobuf message for RoleI? Role2? And if we're never going to have another implementation of RoleI (and I suspect we might not), why even have an interface for it?

The idea here is to stop with the needless renames, so we wouldn't have new Role/RoleSpec/RoleI unless it is a true breaking change.

As for the purpose of the interfaces, let's not get on that in this thread. :)

@rosstimothy

Should we be versioning types instead of versioning the resources within it, i.e. typesv1.Role vs typesv2.Role?

My 2c is no - bumping the package version is akin to saying everything changed, or enough changed in the API that we are calling it a new major entirely. The "ideal" proto API versioning scheme is that you release v1, do incremental, backwards-compatible changes and never ever release v2 (think of how much trouble we get with breaking changes in our dependencies, mostly for no benefit to us).

The proper way to evolve protos is to keep the name do backwards-compatible changes. Fields like api_version exist to do what we do by renaming the protos (that would be a better way to go about this, btw). A message name change should only happen if the representations are truly incompatible; eg, Role and Role2 are radically different, and that's why the name change would be necessary.

Either way, I was really hoping for this PR to only deal with naming changes that are guaranteed to not affect anything functionally, so maybe that's for a future step we can deal with later, especially since we're gonna have a few more safeguards on not breaking the protobuf wire compat in place.

As a side note, I'd say that this is one of the main benefits of the change detector: making it painfully clear what our bad habits are.

codingllama

LGTM.

Sidestepping the RoleVx question is a good call for now, better to have this land as-is. Don't forget to update the PR description.

api/proto/teleport/legacy/client/proto/authservice.proto

This was referenced May 23, 2023

[v13] protobuf changes in preparation for buf breaking #26793

Merged

[v12] protobuf changes in preparation for buf breaking #26794

Merged

[v11] protobuf changes in preparation for buf breaking #26795

Merged

espadolini force-pushed the espadolini/proto-unbreak/master branch 16 times, most recently from 138b92c to 97df3f1 Compare May 25, 2023 09:34

espadolini marked this pull request as ready for review May 25, 2023 09:34

github-actions bot requested review from alexfornuto, ptgott, r0mant, rosstimothy, tigrato, timothyb89, xinding33 and zmb3 May 25, 2023 09:34

github-actions bot added application-access documentation labels May 25, 2023

github-actions bot added machine-id size/sm tctl tctl - Teleport admin tool tsh tsh - Teleport's command line tool for logging into nodes running Teleport. labels May 25, 2023

espadolini removed documentation kubernetes-access tsh tsh - Teleport's command line tool for logging into nodes running Teleport. tctl tctl - Teleport admin tool application-access machine-id labels May 25, 2023

espadolini requested review from codingllama and removed request for alexfornuto, xinding33 and ptgott May 25, 2023 09:36

codingllama reviewed May 25, 2023

View reviewed changes

ptgott approved these changes May 25, 2023

View reviewed changes

espadolini added 2 commits May 26, 2023 11:00

Use WIRE_JSON in buf breaking

4610a68

Add some field reservations for backwards compat

2ebab3d

espadolini force-pushed the espadolini/proto-unbreak/master branch from 97df3f1 to 2ebab3d Compare May 26, 2023 11:07

rosstimothy approved these changes May 26, 2023

View reviewed changes

codingllama approved these changes May 26, 2023

View reviewed changes

api/proto/teleport/legacy/client/proto/authservice.proto Show resolved Hide resolved

public-teleport-github-review-bot bot removed request for r0mant, tigrato, timothyb89 and zmb3 May 26, 2023 13:36

espadolini added this pull request to the merge queue May 26, 2023

Merged via the queue into master with commit 571477f May 26, 2023
28 checks passed

espadolini deleted the espadolini/proto-unbreak/master branch May 26, 2023 14:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

protobuf changes in preparation for `buf breaking` #26792

protobuf changes in preparation for `buf breaking` #26792

espadolini commented May 23, 2023 •

edited

Loading

espadolini commented May 25, 2023

codingllama left a comment

codingllama May 25, 2023

espadolini May 25, 2023

rosstimothy May 25, 2023

espadolini May 25, 2023

codingllama May 25, 2023

espadolini May 25, 2023

codingllama May 25, 2023

codingllama left a comment •

edited

Loading

protobuf changes in preparation for buf breaking #26792

protobuf changes in preparation for buf breaking #26792

Conversation

espadolini commented May 23, 2023 • edited Loading

espadolini commented May 25, 2023

codingllama left a comment

Choose a reason for hiding this comment

codingllama May 25, 2023

Choose a reason for hiding this comment

espadolini May 25, 2023

Choose a reason for hiding this comment

rosstimothy May 25, 2023

Choose a reason for hiding this comment

espadolini May 25, 2023

Choose a reason for hiding this comment

codingllama May 25, 2023

Choose a reason for hiding this comment

espadolini May 25, 2023

Choose a reason for hiding this comment

codingllama May 25, 2023

Choose a reason for hiding this comment

codingllama left a comment • edited Loading

Choose a reason for hiding this comment

protobuf changes in preparation for `buf breaking` #26792

protobuf changes in preparation for `buf breaking` #26792

espadolini commented May 23, 2023 •

edited

Loading

codingllama left a comment •

edited

Loading