Closed Jonpez2 closed 2 years ago
Any thoughts on this please? Would you prefer me to propose a code change?
Hey! Sorry for letting you wait.
I'm fine with supporting use cases like these. I think the cleanest way to support this is to factor out the keypair string fields we have in our config files to a separate Protobuf message. We can then use a oneof
in there to choose between providing the key material inline, vs. using other frameworks.
Instead of using stuff like spiffe-helper, I'm also fine with integrating https://github.com/spiffe/go-spiffe/tree/main/v2. That way you don't need to run all sorts of sidecars just to get TLS working.
My turn to apologize to you!
So actually an even better approach (for me anyway!) would be to upgrade to grpc 1.38.0 and then allow use of xds, like this: https://cloud.google.com/traffic-director/docs/security-proxyless-setup. That still requires a bit of changes inside buildbarn - specifically, all the servers will have to do this (https://github.com/grpc/grpc-go/blob/master/examples/features/xds/server/main.go#L82) - but it means that all the certificate specification and discovery and so forth becomes someone else's problem. Wdyt?
Oh, that would be pretty sweet. Feel free to submit a PR to add a gRPC configuration for enabling xDS!
On it
On Fri, Jul 16, 2021 at 1:29 PM Ed Schouten @.***> wrote:
Oh, that would be pretty sweet. Feel free to submit a PR to add a gRPC configuration for enabling xDS!
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/buildbarn/bb-storage/issues/120#issuecomment-881411314, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABN425PBELZED6KGFYSTMP3TYAQ3XANCNFSM45RSVGDQ .
Nothing is ever simple... https://github.com/grpc/grpc-go/issues/4601
Hey @Jonpez2!
Would a change like this be of any use to you?
https://github.com/buildbarn/bb-storage/compare/eschouten/20210719-service-registrar
This would allow you to write something like this in pkg/grpc/server.go:
var s interface {
grpc.ServiceRegistrar
GetServiceInfo() map[string]grpc.ServiceInfo
Serve(net.Listener) error
}
if useXDS {
s = xds.NewGRPCServer(...)
} else {
s = grpc.NewServer(...)
}
// The rest of the code that registers services and calls .Serve() can go here.
Just let me know and I'll merge this.
Yes that looks perfect!
On Mon, 19 Jul 2021 at 09:25, Ed Schouten @.***> wrote:
Hey @Jonpez2 https://github.com/Jonpez2!
Would a change like this be of any use to you?
https://github.com/buildbarn/bb-storage/compare/eschouten/20210719-service-registrar
This would allow you to write something like this in pkg/grpc/server.go:
var s interface { grpc.ServiceRegistrar Serve(net.Listener) error }if useXDS { s = xds.NewGRPCServer(...) } else { s = grpc.NewServer(...) } // The rest of the code that registers services and calls .Serve() can go here.
Just let me know and I'll merge this.
— You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub https://github.com/buildbarn/bb-storage/issues/120#issuecomment-882351594, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABN425LMJEJJZQWKCTRQZYTTYPOP5ANCNFSM45RSVGDQ .
Great! Merged!
It looks like the construct is sufficient for you to achieve what you want, as long as you take the following into account:
So the change you suggested in your codebase seems to trivially work, which is very nice. However grpc-prometheus appears to be rehousing itself in a different repo, and hasn't released since march. Do we need to port across to the new upstream?
On Mon, Jul 19, 2021 at 9:38 AM Ed Schouten @.***> wrote:
Great! Merged!
It looks like the construct is sufficient for you to achieve what you want, as long as you take the following into account:
- You need to upgrade gRPC to a version that includes grpc/grpc-go@ 145f12a https://github.com/grpc/grpc-go/commit/145f12a95b19d2a2f926176cd63fe5645b376186, as reflection.Register() won't work otherwise.
- grpc_prometheus.Register() needs to be adjusted similarly.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/buildbarn/bb-storage/issues/120#issuecomment-882360582, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABN425JJMRE22VLISWS552TTYPP75ANCNFSM45RSVGDQ .
It looks like v2 hasn't been released yet, so it wouldn't make a lot of sense to invest in that right now. I'd say, just put that PR that I linked above into the already existing patches/com_github_grpc_ecosystem_go_grpc_prometheus/
directory.
ok!
Considering that this issue hasn't received any updates for >1y, I'm going to close it. It should be easier nowadays to get xDS support added, especially with the preparations discussed above. Happy to receive contributions going forward!
I'm currently trying to plug together SPIFFE (https://github.com/spiffe) and buildbarn in a deployment on GKE, to give me well-managed mTLS. My current approach is to use the spiffe-helper (https://github.com/spiffe/spiffe-helper) to keep pem files up to date on the in one container, with spiffe-helper just invoking 'sleep infinity', and then having the buildbarn container share a filesystem so that it can read the resultant certs. Note that SPIFFE rotates the leaf certificates very frequently - on the order of once per hour. This is pretty painful however, as buildbarn uses jsonnet to pipe creds through to the process, which means that a hot reload of the rotated certs is impossible without a full restart.
I'm thinking that the best option here would be for buildbarn to allow specification of a set of files for certs, and then just use grpc's hot reloading code as demonstrated here - https://github.com/grpc/grpc-go/blob/master/security/advancedtls/examples/credential_reloading_from_files/server/main.go
Does that seem right? Or am I missing something obvious?
Thank you!