Refactor prehog & teleterm protos to match project conventions #20810

ravicious · 2023-01-27T12:27:31Z

Summary of changes:

Moved prehog's and teleterm's proto files from their respective lib/ dir to proto/.
Updated go_package of lib/prehog and lib/teleterm protobufs to match conventions used by api/proto and proto.
- lib/prehog Go protobufs now live in a different location:
  - lib/prehog/gen/prehog/v1alpha before
  - gen/proto/go/prehog/v1alpha now
- lib/teleterm Go protobufs now live in a different location:
  - lib/teleterm/api/protogen/golang/v1 before
  - gen/proto/go/teleport/lib/teleterm/v1/ now
  - proto package name has been updated to match the new location.
lib/prehog no longer uses managed mode to set go_package_prefix.
JS protobufs for both packages now live in gen/proto/js in the root folder.
Connect no longer needs to copy protobufs to its own directory.
- Instead it uses protobufs from gen/proto/js so that any change done through make grpc is automatically picked up by Connect.

This is a followup to #19774 where I promised to further refactor JS protos at a later date.

I requested review from multiple people so that each person can look at the PR from a slightly different angle:

@codingllama for general overview of changes in gRPC tooling
@espadolini for checking lib/prehog changes
@gzdunek for checking lib/teleterm changes

This makes them follow the pattern set out by api/proto and proto.

This also entailed changing the location of lib/teleterm protos and changing the value of their package specifier to match the conventions in other parts of the codebase. This is a breaking change but that is fine for Connect as the protos are used locally only and each build ships with matching protobufs.

We used to copy protobufs over to web/packages/teleterm/src/services/tshd since webapps used to be in a separate repo. This is no longer the case, so we can just make teleterm use protobufs from gen-proto-js.

ravicious · 2023-01-27T12:33:36Z

lib/prehog/buf.gen.yaml

-managed:
-  enabled: true
-  go_package_prefix:
-    default: github.com/gravitational/teleport/lib/prehog/gen


I had a short discussion with Edoardo about why we don't want to use managed mode here. Just copying what I said there:

Alan suggested that lib/prehog protos should use go_package to control the output location, that's why I disabled managed mode. Buf docs actually list some advantages for using managed mode.

In our case though not using managed mode and defining the location with go_package instead means that we can use the same buf.gen.yaml config for multiple Buf modules. That means that we have different buf.gen.yaml for different sets of protoc plugins rather than a separate buf.gen.yaml for each module.

codingllama

This is great, @ravicious. Thanks!

What do you think about unifying the paths even further? Ie, moving all non-api protos under proto/ and all generated code under gen/?

/ <-- teleport root
  proto/
    teleport/
      lib/
        prehog/   <- lib/prehog/proto goes here
        teleterm/ <- lib/teleterm/api/proto goes here
  gen/
    proto/
      go/ <- all Go generated files
      js/ <- all Js generated files

This mirrors api/ and becomes a nice, unified place for all non-public protos (and corresponding generated code).

build.assets/genproto.sh

codingllama · 2023-01-27T14:48:04Z

buf-connect-go.gen.yaml

+version: v1
+plugins:
+  - name: go
+    path: lib/prehog/bin/protoc-gen-go


Not for this PR, but we should consolidate so we don't have tooling versions tracked by go.mod and tooling versions tracked via buildbox.

I really like the idea of just needing go to generate all the RPCs, but as that's no longer a possibility with grpc_node_plugin anyway, I'm 👍

build.assets/genproto.sh

lib/teleterm/api/proto/teleterm/v1/access_request.proto

codingllama · 2023-01-27T15:02:22Z

Could we backport the teleterm changes to v10 too?

ravicious · 2023-01-27T15:17:59Z

What do you think about unifying the paths even further? Ie, moving all non-api protos under proto/ and all generated code under gen/?

Oh yeah, that would work and we wouldn't need to have special handling of gen-proto-js. I just wasn't sure what exactly we're aiming for so I assumed we want to have gen/proto-like folders in every place that uses protos, I should've asked really.

espadolini · 2023-01-27T17:13:40Z

Do we ever rely on the horrible "add methods to a protobuf type" thing outside of api? If we don't, I'm also in favor of having all generated code in one conveniently deletable gen/ directory.

codingllama · 2023-01-27T17:22:03Z

Do we ever rely on the horrible "add methods to a protobuf type" thing outside of api? If we don't, I'm also in favor of having all generated code in one conveniently deletable gen/ directory.

Outside of api/types/ we don't; we even delete/recreate gen/ folders on codegen in order to "incentivize" not doing that. +1 for keeping generated code cleanly separate from manually-written code.

ravicious · 2023-01-30T11:19:25Z

@codingllama Take a look at the last two commits I pushed. Things accomplished in them:

prehog and teleterm protos are moved under proto/teleport/lib/
prehog and teleterm workspaces are gone.
- Now we only have two workspaces api/proto and proto. It's not clear to me if they need to exist as workspaces but I ran into problems when I completely removed buf.work.yaml so for now I just kept it.
genproto.sh now uses --path almost everywhere.
JS protobufs are generated to gen/proto/js.
The linter temporarily ignores prehog and teleterm.
- They used to have their separate configs but now they all use proto/buf.yaml.

Now, the biggest thing I need help with are package names. I didn't touch them at all. If I let the linter run on prehog and teleterm protos, I see these messages:

proto/teleport/lib/prehog/v1alpha/connect.proto:17:1:Files with package "prehog.v1alpha" must be within a directory "prehog/v1alpha" relative to root but were in directory "teleport/lib/prehog/v1alpha".

proto/teleport/lib/teleterm/v1/service.proto:17:1:Files with package "teleterm.v1" must be within a directory "teleterm/v1" relative to root but were in directory "teleport/lib/teleterm/v1".

Does it mean that the proper packages for them would be teleport.lib.teleterm.v1 and teleport.lib.prehog.v1alpha? The lint rule contradicts what you suggested, that is moving protos to proto/teleport/lib/* and naming teleterm's package "teleport.teleterm.v1".

I don't have much experience here so I'd follow whatever Buf recommends but I wonder if you have opinions on this.

codingllama · 2023-01-30T16:10:08Z

@codingllama Take a look at the last two commits I pushed. Things accomplished in them:

prehog and teleterm protos are moved under proto/teleport/lib/

prehog and teleterm workspaces are gone.

👍

Now we only have two workspaces api/proto and proto. It's not clear to me if they need to exist as workspaces but I ran into problems when I completely removed buf.work.yaml so for now I just kept it.

I think only have 1 workspace, but 2 proto roots. Looks good to me.

genproto.sh now uses --path almost everywhere.

JS protobufs are generated to gen/proto/js.

👍

The linter temporarily ignores prehog and teleterm.

They used to have their separate configs but now they all use proto/buf.yaml.

👍

Now, the biggest thing I need help with are package names. I didn't touch them at all. If I let the linter run on prehog and teleterm protos, I see these messages:

proto/teleport/lib/prehog/v1alpha/connect.proto:17:1:Files with package "prehog.v1alpha" must be within a directory "prehog/v1alpha" relative to root but were in directory "teleport/lib/prehog/v1alpha".
proto/teleport/lib/teleterm/v1/service.proto:17:1:Files with package "teleterm.v1" must be within a directory "teleterm/v1" relative to root but were in directory "teleport/lib/teleterm/v1".

Does it mean that the proper packages for them would be teleport.lib.teleterm.v1 and teleport.lib.prehog.v1alpha? The lint rule contradicts what you suggested, that is moving protos to proto/teleport/lib/* and naming teleterm's package "teleport.teleterm.v1".

I don't have much experience here so I'd follow whatever Buf recommends but I wonder if you have opinions on this.

That was my bad - packages names should match the directory structure, starting from the root of proto imports (which, in our case, is now proto/).

This is what I suggest:

Folder proto/teleport/lib/teleterm/v1, package teleport.lib.teleterm.v1
Folder proto/teleport/lib/prehog/v1alpha, package teleport.lib.prehog.v1alpha

All prefixed with teleport, so it's clear what they belong to (and we are nice proto citizens).
All under lib/, so it's easy reason about the proto/Go package relationship. I would likely drop the "lib" for public protos, but these are not public (as they are not declared in api/).

Generated paths follow the same paths as above:

gen/proto/go/teleport/lib/prehog/v1alpha/*.go
gen/proto/go/teleport/lib/teleterm/v1/*.go
gen/proto/js/teleport/lib/prehog/v1alpha/*.{js,ts} (already correct)
gen/proto/js/teleport/lib/teleterm/v1/*.{js,ts} (already correct)

I hope this helps!

ravicious · 2023-01-30T16:59:09Z

Yeah this helps a lot, thanks! I'll hack on it tomorrow, I'll also have to figure out how to selectively block linter rules (or maybe just fix them) but that seems easier than agreeing on package names.

espadolini · 2023-01-31T10:03:27Z

Are we supposed to change the package name for the prehog proto? It's not really a teleport proto, the source of truth lives in gravitational/cloud.

ravicious · 2023-01-31T11:36:55Z

Are we supposed to change the package name for the prehog proto? It's not really a teleport proto, the source of truth lives in gravitational/cloud.

Do you mean whether the reporting team should take care of that or if we should consider renaming it at all? Also what do you consider to be the source of truth in this case? From what I see the proto files live here and gravitational/cloud only has the generated files.

I'll take of renaming in this PR if we agree on the names. From what I gathered, changing the package name should be a backwards-compatible change unless prehog uses JSON encoding somewhere.

ravicious · 2023-02-02T10:21:37Z

Not sure why for the last merge commit GitHub is not showing only the changes that my PR did over what's on master, git show displays that properly. There were conflicts due to changes in prehog protos so all I did to resolve them was to run make grpc again.

api/proto/buf.yaml

espadolini · 2023-02-02T10:55:46Z

buf-connect-go.gen.yaml

+version: v1
+plugins:
+  - name: go
+    path: lib/prehog/bin/protoc-gen-go


I really like the idea of just needing go to generate all the RPCs, but as that's no longer a possibility with grpc_node_plugin anyway, I'm 👍

codingllama

LGTM. Ship it!

gzdunek

Teleterm changes look good too!

ravicious · 2023-02-03T14:46:55Z

🙌 🙌 🙌 🙌 🙌

public-teleport-github-review-bot · 2023-02-03T15:10:00Z

@ravicious See the table below for backport results.

Branch	Result
branch/v10	Failed
branch/v11	Failed
branch/v12	Failed

* Adjust go_package of lib/prehog Go protobufs This makes them follow the pattern set out by api/proto and proto. * Adjust go_package of lib/teleterm Go protobufs * Use single buf.gen.yaml to generate JS protos This also entailed changing the location of lib/teleterm protos and changing the value of their package specifier to match the conventions in other parts of the codebase. This is a breaking change but that is fine for Connect as the protos are used locally only and each build ships with matching protobufs. * Make web/packages/teleterm use protobufs from gen-proto-js We used to copy protobufs over to web/packages/teleterm/src/services/tshd since webapps used to be in a separate repo. This is no longer the case, so we can just make teleterm use protobufs from gen-proto-js. * Move prehog & teleterm protos into proto/teleport/lib * Generate JS protos to gen/proto/js * Move lib/teleterm Go protobufs to gen/proto/go * Move lib/prehog Go protobufs to gen/proto/go * Rename lib/teleterm proto package * Re-enable linter rules for teleterm & prehog * Update prehogv1 path in usagereporter_test.go * Use except instead of ignore_only to allow Google API-style responses * Add UNARY_RPC to api/proto & proto * Ignore gen/ when running addlicense * buf-js.gen.yaml: Remove comment about lack of go_package for JS * Move prehog protos to proto/prehog/v1alpha * Adjust prehog's go_package to match proto package

… (#21298) * Moved prehog's and teleterm's proto files from their respective `lib/` dir to `proto/`. * Updated `go_package` of `lib/prehog` and `lib/teleterm` protobufs to match conventions used by `api/proto` and `proto`. * `lib/prehog` Go protobufs now live in a different location: * `lib/prehog/gen/prehog/v1alpha` before * `gen/proto/go/prehog/v1alpha` now * `lib/teleterm` Go protobufs now live in a different location: * `lib/teleterm/api/protogen/golang/v1` before * `gen/proto/go/teleport/lib/teleterm/v1/` now * proto package name has been updated to match the new location. * `lib/prehog` no longer uses managed mode to set `go_package_prefix`. * JS protobufs for both packages now live in `gen/proto/js` in the root folder. * Connect no longer needs to copy protobufs to its own directory. * Instead it uses protobufs from `gen/proto/js` so that any change done through `make grpc` is automatically picked up by Connect.

ravicious · 2023-02-07T10:57:15Z

v11 and v10 backports should also include the followup fix for Teleport Operator. #21362

* Moved teleterm's proto files from their respective `lib/` dir to `proto/`. * Updated `go_package` of `lib/teleterm` protobufs to match conventions used by `api/proto` and `proto`. * `lib/teleterm/api/protogen/golang/v1` before * `gen/proto/go/teleport/lib/teleterm/v1/` now * proto package name has been updated to match the new location. * JS protobufs for now live in `gen/proto/js` in the root folder. * Connect no longer needs to copy protobufs to its own directory. * Instead it uses protobufs from `gen/proto/js` so that any change done through `make grpc` is automatically picked up by Connect.

ravicious added 4 commits January 27, 2023 13:12

Adjust go_package of lib/prehog Go protobufs

c0c4a81

This makes them follow the pattern set out by api/proto and proto.

Adjust go_package of lib/teleterm Go protobufs

700b0a8

Make web/packages/teleterm use protobufs from gen-proto-js

b2af272

We used to copy protobufs over to web/packages/teleterm/src/services/tshd since webapps used to be in a separate repo. This is no longer the case, so we can just make teleterm use protobufs from gen-proto-js.

ravicious added backport/branch/v11 labels Jan 27, 2023

ravicious commented Jan 27, 2023

View reviewed changes

ravicious marked this pull request as ready for review January 27, 2023 12:36

ravicious requested review from gzdunek, codingllama and espadolini January 27, 2023 12:36

github-actions bot added the size/sm label Jan 27, 2023

github-actions bot requested review from rudream and ryanclark January 27, 2023 12:37

ravicious removed request for ryanclark and rudream January 27, 2023 12:41

codingllama reviewed Jan 27, 2023

View reviewed changes

ravicious added 2 commits January 30, 2023 12:06

Move prehog & teleterm protos into proto/teleport/lib

adc3c18

Generate JS protos to gen/proto/js

daa7a04

ravicious requested a review from codingllama January 30, 2023 11:19

ravicious added the backport/branch/v10 label Jan 30, 2023

espadolini approved these changes Feb 2, 2023

View reviewed changes

ravicious added 3 commits February 3, 2023 09:08

Move prehog protos to proto/prehog/v1alpha

a45c531

Adjust prehog's go_package to match proto package

5eb1265

Merge branch 'master' into ravicious/grpc-js-refactor

147d1b2

ravicious changed the title ~~Use single buf.gen.yaml for JS protos~~ Refactor prehog & teleterm protos to match project conventions Feb 3, 2023

codingllama approved these changes Feb 3, 2023

View reviewed changes

public-teleport-github-review-bot bot removed the request for review from gzdunek February 3, 2023 14:25

gzdunek approved these changes Feb 3, 2023

View reviewed changes

ravicious added this pull request to the merge queue Feb 3, 2023

Merged via the queue into master with commit 3ac5a0a Feb 3, 2023

ravicious deleted the ravicious/grpc-js-refactor branch February 3, 2023 15:34

alexfornuto mentioned this pull request Feb 6, 2023

Add docs for Connect usage reporting #21294

Merged

ravicious mentioned this pull request Feb 6, 2023

[v12] Refactor prehog & teleterm protos to match project conventions #21298

Merged

ravicious mentioned this pull request Feb 7, 2023

Fix operator build #21362

Merged

ravicious mentioned this pull request Feb 7, 2023

[v11] Refactor teleterm protos to match project conventions #21394

Merged

This was referenced Feb 8, 2023

[v10] Refactor teleterm protos to match project conventions #21451

Merged

Update JS grpc-tools to 1.12.4 #21452

Merged

This was referenced Feb 14, 2023

Remove the need to copy protogen files from lib/teleterm to packages/teleterm #16621

Closed

[v11] Refactor prehog protos to match project conventions #21868

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor prehog & teleterm protos to match project conventions #20810

Refactor prehog & teleterm protos to match project conventions #20810

ravicious commented Jan 27, 2023 •

edited

Loading

ravicious Jan 27, 2023

codingllama left a comment

codingllama Jan 27, 2023

espadolini Feb 2, 2023

codingllama commented Jan 27, 2023

ravicious commented Jan 27, 2023

espadolini commented Jan 27, 2023

codingllama commented Jan 27, 2023

ravicious commented Jan 30, 2023

codingllama commented Jan 30, 2023

ravicious commented Jan 30, 2023

espadolini commented Jan 31, 2023

ravicious commented Jan 31, 2023

ravicious commented Feb 2, 2023

espadolini Feb 2, 2023

codingllama left a comment

gzdunek left a comment

ravicious commented Feb 3, 2023

public-teleport-github-review-bot bot commented Feb 3, 2023

ravicious commented Feb 7, 2023

Refactor prehog & teleterm protos to match project conventions #20810

Refactor prehog & teleterm protos to match project conventions #20810

Conversation

ravicious commented Jan 27, 2023 • edited Loading

ravicious Jan 27, 2023

Choose a reason for hiding this comment

codingllama left a comment

Choose a reason for hiding this comment

codingllama Jan 27, 2023

Choose a reason for hiding this comment

espadolini Feb 2, 2023

Choose a reason for hiding this comment

codingllama commented Jan 27, 2023

ravicious commented Jan 27, 2023

espadolini commented Jan 27, 2023

codingllama commented Jan 27, 2023

ravicious commented Jan 30, 2023

codingllama commented Jan 30, 2023

ravicious commented Jan 30, 2023

espadolini commented Jan 31, 2023

ravicious commented Jan 31, 2023

ravicious commented Feb 2, 2023

espadolini Feb 2, 2023

Choose a reason for hiding this comment

codingllama left a comment

Choose a reason for hiding this comment

gzdunek left a comment

Choose a reason for hiding this comment

ravicious commented Feb 3, 2023

public-teleport-github-review-bot bot commented Feb 3, 2023

ravicious commented Feb 7, 2023

ravicious commented Jan 27, 2023 •

edited

Loading