Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Implement the ServerDiscovery.WatchServers gRPC endpoint #12819

Merged
merged 5 commits into from
Apr 21, 2022
Merged

Implement the ServerDiscovery.WatchServers gRPC endpoint #12819

merged 5 commits into from
Apr 21, 2022

Conversation

mkeeler
Copy link
Member

@mkeeler mkeeler commented Apr 19, 2022

This PR adds a ServerDiscovery.WatchServers gRPC endpoint.

Additionally I did a bit of refactoring to of gRPC test functionality to make sharing more of it possible.

Base automatically changed from autopilot-events to main April 19, 2022 17:03
@mkeeler mkeeler force-pushed the mkeeler/server-discovery-grpc branch 2 times, most recently from 28590ef to f0f7469 Compare April 19, 2022 18:07
mkeeler
mkeeler commented Apr 19, 2022
View reviewed changes
agent/consul/grpc_integration_test.go
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
t.Cleanup(cancel)

ctx = public.ContextWithToken(ctx, TestDefaultInitialManagementToken)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a token to the context because the new testGRPCIntegrationServer turns ACLs on.

mkeeler
mkeeler commented Apr 19, 2022
View reviewed changes
agent/grpc/public/services/connectca/sign.go
@@ -48,6 +48,7 @@ func (s *Server) Sign(ctx context.Context, req *pbconnectca.SignRequest) (*pbcon
var rsp *pbconnectca.SignResponse
handled, err := s.ForwardRPC(&rpcInfo, func(conn *grpc.ClientConn) error {
logger.Trace("forwarding RPC")
ctx := public.ForwardMetadataContext(ctx)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is critical to have the ACL token (and any other metadata from the incoming request) forwarded. This is due to the gRPC library using different keys to store the incoming and outgoing requests metadata.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, good catch! I wonder if there's a way to make this happen with an interceptor, that wouldn't risk leaking tokens to any other gRPC APIs we integrate with in the future 🤔

mkeeler
mkeeler commented Apr 19, 2022
View reviewed changes
agent/consul/server_test.go
@@ -257,6 +257,26 @@ func testACLServerWithConfig(t *testing.T, cb func(*Config), initReplicationToke
return dir, srv, codec
}

func testGRPCIntegrationServer(t *testing.T, cb func(*Config)) (*Server, *grpc.ClientConn) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kind of self explanatory but the purpose here is to create a test server with a listening gRPC server configured for it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Thanks for pulling this out 🙇🏻‍♂️

mkeeler
mkeeler commented Apr 19, 2022
View reviewed changes
agent/grpc/public/services/connectca/server_test.go
"github.com/hashicorp/consul/proto-public/pbconnectca"
)

func noopForwardRPC(structs.RPCInfo, func(*grpc.ClientConn) error) (bool, error) {
return false, nil
}

func testStateStore(t *testing.T, publisher state.EventPublisher) *state.Store {
func setupFSMAndPublisher(t *testing.T) (*testutils.FakeFSM, state.EventPublisher) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bulk of what used to be here was moved to a shared location to reduce some of the boilerplate.

@mkeeler mkeeler force-pushed the mkeeler/server-discovery-grpc branch from 97be3e4 to fbbc823 Compare April 20, 2022 20:55
@mkeeler mkeeler force-pushed the mkeeler/server-discovery-grpc branch from fbbc823 to 30706b2 Compare April 20, 2022 21:23
boxofrad
boxofrad approved these changes Apr 21, 2022
View reviewed changes
Copy link
Contributor

@boxofrad boxofrad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, nice job!

agent/consul/server_test.go
@@ -257,6 +257,26 @@ func testACLServerWithConfig(t *testing.T, cb func(*Config), initReplicationToke
return dir, srv, codec
}

func testGRPCIntegrationServer(t *testing.T, cb func(*Config)) (*Server, *grpc.ClientConn) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Thanks for pulling this out 🙇🏻‍♂️

agent/grpc/public/services/connectca/sign.go
@@ -48,6 +48,7 @@ func (s *Server) Sign(ctx context.Context, req *pbconnectca.SignRequest) (*pbcon
var rsp *pbconnectca.SignResponse
handled, err := s.ForwardRPC(&rpcInfo, func(conn *grpc.ClientConn) error {
logger.Trace("forwarding RPC")
ctx := public.ForwardMetadataContext(ctx)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, good catch! I wonder if there's a way to make this happen with an interceptor, that wouldn't risk leaking tokens to any other gRPC APIs we integrate with in the future 🤔

agent/grpc/public/services/serverdiscovery/watch_servers.go
// closed due to an ACL change, we'll attempt to re-authorize and resume it to
// prevent unnecessarily terminating the stream.
var idx uint64
for {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not for now (as I think it'd be premature) but if we ended up writing this event-consumer-loop for a third time then it'd probably be worth extracting 😄

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

riddhi89
riddhi89 approved these changes Apr 21, 2022
View reviewed changes
Copy link
Contributor

@riddhi89 riddhi89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥳

@mkeeler mkeeler force-pushed the mkeeler/server-discovery-grpc branch from f858cdc to 4a254d8 Compare April 21, 2022 14:56
@mkeeler
Copy link
Member Author

mkeeler commented Apr 21, 2022

Just waiting for green tests again (had to regen the protobuf files after adding comments to make CI happy)

@mkeeler mkeeler merged commit 7ce2b48 into main Apr 21, 2022
@mkeeler mkeeler deleted the mkeeler/server-discovery-grpc branch April 21, 2022 16:56
@hc-github-team-consul-core
Copy link
Collaborator

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/647772.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@boxofrad boxofrad boxofrad approved these changes

@riddhi89 riddhi89 riddhi89 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mkeeler @hc-github-team-consul-core @boxofrad @riddhi89