-
Notifications
You must be signed in to change notification settings - Fork 5k
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 helloworld tests with gRPC #1845
Conversation
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.
Some comments left. :-)
@@ -0,0 +1,31 @@ | |||
logLevel = "DEBUG" |
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.
Needed?
[frontends.frontend1] | ||
backend = "backend1" | ||
[frontends.frontend1.routes.test_1] | ||
rule = "Host:127.0.0.1" |
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.
Newline missing.
integration/grpc_test.go
Outdated
} | ||
|
||
func startGrpcServer() error { | ||
lis, err := net.Listen("tcp", ":50051") |
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.
Could we possibly pick a random port to avoid potential flakiness in the future?
integration/grpc_test.go
Outdated
return "", err | ||
} | ||
|
||
//defer conn.Close() |
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.
Should either comment in or remove.
integration/grpc_test.go
Outdated
|
||
func (s *myserver) SayHello(ctx context.Context, in *helloworld.HelloRequest) (*helloworld.HelloReply, error) { | ||
return &helloworld.HelloReply{Message: "Hello " + in.Name}, nil | ||
|
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.
Extra blank.
integration/grpc_test.go
Outdated
|
||
type myserver struct{} | ||
|
||
func (s *myserver) SayHello(ctx context.Context, in *helloworld.HelloRequest) (*helloworld.HelloReply, error) { |
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.
ctx
is not used in this function.
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.
Does the method need to be exported?
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.
The method never returns a non-nil error. Can we drop the second return parameter?
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.
Overall, do we need this method? :-)
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 need to implement GreeterServer
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.
👍
integration/grpc_test.go
Outdated
roots := x509.NewCertPool() | ||
roots.AppendCertsFromPEM(LocalhostCert) | ||
credsClient := credentials.NewClientTLSFromCert(roots, "") | ||
conn, err := grpc.Dial("127.0.0.1:4443", grpc.WithTransportCredentials(credsClient)) |
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.
Where does 4443 come from? The server runs on a different port, doesn't it?
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 is the traefik port
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.
👌
serverOption := grpc.Creds(creds) | ||
|
||
var s *grpc.Server = grpc.NewServer(serverOption) | ||
defer s.Stop() |
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.
Does it make sense to defer-stop here? Don't we need the server to continue running past the the return of this method?
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.
s.Serve at the end of this method will block so it make sense
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 gotcha 👍
integration/grpc_test.go
Outdated
err = try.GetRequest("http://127.0.0.1:8080/api/providers", 1000*time.Millisecond, try.BodyContains("Host:127.0.0.1")) | ||
c.Assert(err, check.IsNil) | ||
|
||
response, err := callHelloClientGrpc() |
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.
What happens if the server takes longer to boot and we fire out the request prior to it being ready? Does gRPC has some kind of retry built in?
Possibly even better: can we synchronize the start of the server with the execution of the test?
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.
LGTM
👍 Last question: What's the number suffix in |
@timoreimann |
integration/grpc_test.go
Outdated
"google.golang.org/grpc/credentials" | ||
) | ||
|
||
var LocalhostCert = []byte(`-----BEGIN CERTIFICATE----- |
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 not re-use any of the other integration test certificates, instead of hard coding another here?
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 didn't find any cert on 127.0.0.1. And I really need a local cert for contacting backend on 127.0.0.1 in a secure way.
May be I can put all the certs in a shared directory for futur reuse ? @dtomcej WDYT ?
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 having all certs in a shared directory would be beneficial. This would prevent having to have people generate certs, and we could have wildcard certs/single host certs/ etc all in one easy to use convenient place for testing.
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.
Oops sorry, this has been long fine from my point of view.
LGTM!
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.
Super close!
Does integration/resources/tls/.README.md.swp
need to be committed?
@dtomcej oups, I really need to update my global gitignore |
Sorry for the delay in review! LGTM! |
This PR add an integration test in order to test a simple grpc helloworld with traefik.
Grpc use http2 protocol and as we use an http reverse proxy, and that golang supports only http2 on https, we can't use directly the code from the helloworld of grpc, we need to start a server with tls certificate.