gRPC: migrate conf endpoint#55648
Conversation
6133724 to
f1f6ede
Compare
Very excited! |
ggilmore
left a comment
There was a problem hiding this comment.
A question: What's the behavior of the application if the site configuration can't be accessed for whatever reason?
I believe our binaries refuse to start up if they can't fetch the configuration, but what happens if the site configuration can't be fetched for a while while the binaries are already up and running?
(This could happen with REST too, but we have an additional failure mode here - the message size growing too large (however unlikely)).
|
|
||
| var Client = &internalClient{URL: "http://" + frontendInternal} | ||
| var Client = &internalClient{ | ||
| URL: "http://" + frontendInternal, |
There was a problem hiding this comment.
I think it'd be nice to have a bit more robust url handling here.
Example: What if frontendInternal is http://foo:3090 already? With this logic, URL will be http://http://foo:3090.
Maybe you can adopt the logic that I wrote in https://github.com/sourcegraph/sourcegraph/blob/main/internal/grpc/defaults/cache.go#L97-L117?
|
|
||
| message RawUnified { | ||
| int32 id = 1; | ||
| string site = 2; |
There was a problem hiding this comment.
I believe we've done our due-diligence to ensure that the site configuration must be utf-8 encoded?
It seems like this is implicitly enforced by our postgres settings: https://sourcegraph.com/github.com/sourcegraph/sourcegraph/-/blob/docker-images/postgres-12-alpine/Dockerfile.wolfi?L27
(I also tried setting the site config json override file to an invalid utf-8 sequence and saw that sourcegraph complained as I expected: echo -n -e '\xf0\x28\x8c\xbc' > external-services-config.json )
I don't have a strong preference for this, but do you have thoughts for represnting the site config json as bytes rather than string? We already need to cast it to []byte for json.Unmarshal purposes.
There was a problem hiding this comment.
Yep, I did look into this. JSON is UTF-8 by definition, so we will fail validation far before we ever get to protobuf. It's possible that the JSON encodes non-utf8 strings, but at this level (the raw JSON), we don't need to worry about that. I think string is preferable to bytes in this case mostly because that's what's used in our types now
|
The backport to To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-5.1 5.1
# Navigate to the new working tree
cd .worktrees/backport-5.1
# Create a new branch
git switch --create backport-55648-to-5.1
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 1dd8601839b18d072718a732139355fb3793e784
# Push it to GitHub
git push --set-upstream origin backport-55648-to-5.1
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-5.1Then, create a pull request where the |
(cherry picked from commit 1dd8601)
(cherry picked from commit 1dd8601)
* Backend: remove internalClient.SendEmail (#55459) Now that config can be fetched from all services, there is no need for this internalClient method. One fewer to implement for gRPC (cherry picked from commit f9d67fe) * gRPC: migrate conf endpoint (#55648) (cherry picked from commit 1dd8601) * Backend: remove internalClient.ExternalURL (#55463) This removes the `internalClient.ExternalURL` method, which can be replaced with a simple call to `conf.Get()`
This implements the internal
confendpoint with gRPC. This is protected by a separate environment variable, unlike most of our gRPC endpoints, because we need the site config in order to determine whether to use gRPC. This allows us to enable this on instances we control (and can monitor), and exclude the conf endpoint from the "enabled by default" in the next minor release so we can still safely disable gRPC if necessary.This maintains the current polling method for updating config. After this is stable, we can look at switch to a server-streaming-update model.
Test plan
Added a roundtrip quick test. And it caught a small bug, so it works 🙂 The environment variable is enabled with
sg, so this will get naturally tested during dev. Once this rolls out to all services, we can enable the env variable on sourcegraph.com and on managed instances.