-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add tonic
boilerplate
#16
base: mvp
Are you sure you want to change the base?
Conversation
This PR builds upon #13 and adds some more boilerplate, this time for the gRPC service handlers. Much of this code follows the same structure as the PoC's, except this is more modularized and, imo, easier to grok. Some comments:
|
e5c0fb4
to
fbf06e2
Compare
f4b0d4f
to
9d389ab
Compare
fbf06e2
to
6c5d5d7
Compare
Drafting until #13 is ready. Looks like I'll have a lot of rebasing to do :) EDIT: Fixed and reopened. |
dad0270
to
0d47ce7
Compare
9d389ab
to
94e11d4
Compare
94e11d4
to
0fee3ee
Compare
8ff3829
to
7b50113
Compare
One more thing: I'm currently building this without a database connection pool to make things easier to get it off the ground. In the future, it's likely that we'll want to prevent the gRPC service handlers from having to make a unique database connection every time they have a request to fulfill, but the consequences of this aren't benchmarked for our use case yet so I'm trying to avoid prematurely optimizing. |
7b50113
to
d66227d
Compare
d66227d
to
41a4390
Compare
Drafting for a second as I have decided that I shouldn't expose the generated models directly when working on the followup PR. I want to have the ability to extend the functionality of the models so we can define filters inside |
ece0ebe
to
b404ed3
Compare
b404ed3
to
b883aac
Compare
b0eb594
to
25e7793
Compare
@@ -8,3 +8,6 @@ allow-licenses: | |||
- MPL-2.0 | |||
- Unicode-3.0 | |||
- Zlib | |||
allow-dependencies-licenses: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is due dependency-review-action
requiring SPDX-compliant license names. matchit
and ring
both have their reasons for being AND
instead of OR
dual-licensed, and the former of those isn't recognized by SPDX. This removes the license check on these two.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is tuxtape_database_bridge/grpc.rs
intentionally empty?
let v1_protos = [ | ||
proto_tuxtape_dir.join("server/database/v1/database.proto"), | ||
proto_tuxtape_dir.join("server/registrar/v1/registrar.proto"), | ||
proto_tuxtape_dir.join("server/fleet_client/v1/fleet_client.proto"), | ||
proto_tuxtape_dir.join("kernel_builder/v1/kernel_builder.proto"), | ||
proto_tuxtape_dir.join("patch_builder/v1/patch_builder.proto"), | ||
]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just walk the protos dir instead? That way any thing added is automatically picked up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bah, forgot about this one. Will get that up soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think maybe we should avoid walking the directory for now for a few reasons:
There's a reason they didn't give you the ability to just include anything in the .proto folder:
-
you only want to include ones that define service functions (any .proto that gets included by those protos also gets included in the build process, as seen with tuxtape.common.v1.rs).
-
we need to add a dependency to do it, and it doesn't use an SPDX-compliant license title which is more cruft for our license check config
-
the solution is going to get ugly, and will get uglier if we need to bump the proto version
This doesn't exactly work, but it's similar to what would as far as i can tell:
let v1_protos = WalkDir::new(&proto_tuxtape_dir)
.into_iter()
.filter_entry(|entry| {
entry.path().extension().is_some_and(|ext| ext == ".proto")
&& entry
.path()
.components()
.any(|comp| comp == Component::Normal(OsStr::new("v1")))
})
.collect::<Result<Vec<_>, _>>()?;
let v1_protos = v1_protos
.iter()
.map(|entry| entry.path())
.collect::<Vec<&Path>>();
(btw, that's split up bc the Rust compiler says that first .collect vector needs a binding, and i'm not sure how to early return with an error when mapping as you can't return upwards from a closure)
I feel like doing this is going to cause more issues than it solves, and makes the code a lot harder to read. I'd only expect we need to add a handful of protos in the lifetime of TuxTape.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you only want to include ones that define service functions (any .proto that gets included by those protos also gets included in the build process, as seen with tuxtape.common.v1.rs).
We should be able to structure our protos in a way that still allows us to walk the directory.
i.e service protos in a service
directory.
we need to add a dependency to do it, and it doesn't use an SPDX-compliant license title which is more cruft for our license check config
I don't think this is a valid reason not to do it.
the solution is going to get ugly, and will get uglier if we need to bump the proto version
Can we restructure the protos? What exactly would cause issues if proto versions needed to be bumped?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've got the protos structured as suggested by buf. I can restructure them if we really need to, but at that point we're going off script from how I've ever seen Protobuf files structured, so I'd rather keep the structure the same if possible.
What exactly would cause issues if proto versions needed to be bumped?
It would just complicate the filter rules for what gets included and excluded.
I can try to work on a cleaner solution than the one I posted tomorrow. I'm just trying to get this PR wrapped up as this has been open for almost two weeks and I keep rebasing in-development branches on these changes which gets cumbersome when we're frequently restructuring things.
pub async fn start_server( | ||
grpc_addr: SocketAddr, | ||
tls_config: Option<ServerTlsConfig>, | ||
db_conn_details: Arc<DatabaseConnectionDetails>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like something that could take ownership instead of adding complexity with Arc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may be off base here, but there are a few reasons I think we shouldn't do this:
-
We call
start_server()
in a loop inmain()
in case it crashes and we need to log it and bring it back up. If it does crash, then that ownership goes away. Leaving it asArc<DatabaseConnectionDetails>
allows us to have one persistent instance of it across the lifetime of the program. -
Further, there are multiple services spun up within
start_server()
that need an instance of aDatbaseConnectionDetails
as well.
let fleet_client_service_state = FleetClientServiceState::new(db_conn_details.clone()); | ||
let registrar_service_state = RegistrarServiceState::new(db_conn_details.clone()); | ||
|
||
let file_descriptor_set = std::fs::read(proto::FILE_DESCRIPTOR_SET_PATH)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the recommended way to do this? I'd worry about reading a file from the fs everytime we need to start a server.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I have no memory why I did it this way. I'll get on reworking this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember now: we need to do this if we want to put the prost
-generated Rust Protobuf bindings in a non-default location (which, at compile time, is in target/build/...
. If we want the ability to commit our generated code, we need to do something like this. https://docs.rs/tonic/latest/tonic/macro.include_file_descriptor_set.html
(Also, this explains the include!()
above this: https://docs.rs/tonic/latest/tonic/macro.include_proto.html)
Yes, that's getting fleshed out in the following PR. |
6ea110a
to
c95e884
Compare
Issue Number:
Reference the issue this PR fixes.
N/A
Description of Changes:
Provide a clear and concise explanation of what changes you made and why.
tonic
gRPC boilerplate.Testing Done:
How did you test your changes? Share details like steps, tools used, or results.
N/A (business logic incoming).
Terms of contribution:
By submitting this pull request, I agree that this contribution is licensed under the terms of the Apache License, Version 2.0.
Thanks for submitting your pull request! We will review it as soon as possible.