Skip to content

Fix peering establishment bugs on followers #14119

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

Merged
merged 3 commits into from
Aug 11, 2022

Conversation

kisunji
Copy link
Contributor

@kisunji kisunji commented Aug 10, 2022

Description

  1. isFailedPreconditionErr did not properly handle wrapped status errors
  2. PeerStreamService was not registered with the internal gRPC server, making gRPC forwarding to leaders throw rpc error: code = Unimplemented desc = unknown service hashicorp.consul.internal.peerstream.PeerStreamService

Testing & Reproduction steps

  • Added testcase that will break if grpc library ever changes its implementation

PR Checklist

  • updated test coverage
  • external facing docs updated
  • not a security concern

Sorry, something went wrong.

@kisunji kisunji added backport/1.13 theme/cluster-peering Related to Consul's cluster peering feature pr/no-changelog PR does not need a corresponding .changelog entry labels Aug 10, 2022
@@ -816,6 +816,7 @@ func newGRPCHandlerFromConfig(deps Deps, config *Config, s *Server) connHandler

// Note: these external gRPC services are also exposed on the internal server to
// enable RPC forwarding.
s.peerStreamServer.Register(srv)
Copy link
Contributor

@freddygv freddygv Aug 10, 2022

Choose a reason for hiding this comment

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

Can you add a test to ensure that forwarding is working? Here's an example:
https://github.com/hashicorp/consul-enterprise/blob/main/agent/consul/partition_backend_test.go#L84

If you comment out this change the unknown service error should come up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in 5078d6e

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Confirmed unknown service comes up without this change

@@ -1332,3 +1333,13 @@ func TestLeader_Peering_retryLoopBackoffPeering_cancelContext(t *testing.T) {
fmt.Errorf("error 1"),
}, allErrors)
}

func Test_isFailedPreconditionErr(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

@kisunji kisunji changed the title Handle wrapped errors in isFailedPreconditionErr Fix peering establishment bugs on followers Aug 10, 2022
Copy link
Contributor

@freddygv freddygv left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fix! :shipit:

@kisunji kisunji merged commit 3926009 into main Aug 11, 2022
@kisunji kisunji deleted the kisunji/checkwrappedfailedprecondition branch August 11, 2022 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/no-changelog PR does not need a corresponding .changelog entry theme/cluster-peering Related to Consul's cluster peering feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants