Adds gRPC Server-side TLS Support #204

danehans · 2018-03-14T23:21:26Z

Adds gRPC Server-side TLS support to Fortio grpc client and server. Addresses TODO: option to specify certs. in fgrpc/grpcrunner.go.

ldemailly · 2018-03-15T04:52:20Z

I don't think we should put self signed certs in the release docker image

maybe it can be a volume/mount like the json data dir ?

for testing we can have indeed a script generating a sample

danehans · 2018-03-15T16:44:53Z

Thank you for taking a look at the PR.

I don't think we should put self signed certs in the release docker image

Agreed, that is why I did not update release/Dockerfile.

maybe it can be a volume/mount like the json data dir ?

I'm not familiar with how the json data dir is mounted. Do you have a pointer?

for testing we can have indeed a script generating a sample

Agreed, I wanted to make sure we can perform e2e testing on the feature. This is why I updated the non-release Docker image. Maybe I don't understand how the non-release and release images work with one another.

ldemailly · 2018-03-15T18:33:57Z

Thank you for taking a look at the PR.

I haven't yet, really. will now

I'm not familiar with how the json data dir is mounted. Do you have a pointer?

https://github.com/istio/fortio/blob/master/Dockerfile#L33

Agreed, I wanted to make sure we can perform e2e testing on the feature. This is why I updated the non-release Docker image. Maybe I don't understand how the non-release and release images work with one another.

Dockerfile is the release for istio/fortio
there is additional code in release/ that extract a tgz out of it but it's secondary

ldemailly

tbh in addition to all the technical issues/details below,

I'm really not a fan of adding all this code - it might grow on me though once simplified and working but... not sure.... I'll give it some more thoughts

ldemailly · 2018-03-15T18:34:30Z

Dockerfile

@@ -20,10 +20,17 @@ RUN /usr/local/go/bin/go version
 RUN CGO_ENABLED=0 GOOS=darwin /usr/local/go/bin/go build -a -ldflags -s -o fortio.go18.mac istio.io/fortio
 # Just check it stays compiling on Windows (would need to set the rsrcDir too)
 RUN CGO_ENABLED=0 GOOS=windows go build -a -o fortio.exe istio.io/fortio
+# Generate TLS assets for secure grpc


undo all this

ldemailly · 2018-03-15T18:35:38Z

ftls/ftls.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package ftls


I don't think we need a brand new top level package just for this

it can be a file inside fnet

ldemailly · 2018-03-15T18:40:40Z

ftls/ftls.go

+
+// NewCertPool creates an x509 certPool with the provided CA files.
+func NewCertPool(CAFiles []string) (*x509.CertPool, error) {
+	certPool := x509.NewCertPool()


this seems overly complicated for a testing tool we only need
https://godoc.org/google.golang.org/grpc/credentials#NewClientTLSFromFile
no ?

(though maybe it is worth having this for http tls too)

I thought this would be needed to setup one or more trusted CA for authorizing client/server connections.

ldemailly · 2018-03-15T18:43:21Z

fgrpc/grpcrunner.go

+		}
+		creds := credentials.NewTLS(tlsCfg)
+		opts = grpc.WithTransportCredentials(creds)
+	case strings.HasPrefix(serverAddr, "https://"):


I don't think the switch between standard CA vs custom CA should be based on https:// prefix
it's a config option, to be added to grpcoptions

ldemailly · 2018-03-15T18:46:20Z

fgrpc/pingsrv.go

-	grpcServer := grpc.NewServer()
+	var opts []grpc.ServerOption
+	if secure {
+		tlsCfg, err := ftls.NewCredentials(false, ftls.DefaultServerCert, ftls.DefaultServerKey,


if they are hard coded what is the point of passing constants.
we need some grpcoptions struct to control tls instead of just 1 boolean

ldemailly · 2018-03-15T18:47:50Z

ftls/info.go

+// ClientConfig returns a tls.Config for client use.
+func (info *TLSInfo) ClientConfig() (*tls.Config, error) {
+	// CA for verifying the server
+	pool, err := NewCertPool([]string{info.CAFile})


how would you put both the standard CA and additional one in 1 config ?

Change CAFile to be of type []string that represent one or more paths to trusted CA certs. Provide the slice to NewCertPool. Create a cmdline flag that allows a user to specify multiple comma-separated cert paths. Is that acceptable?

i'd rather have just the NewClientTLSFromFile for now I think; and less code

codecov · 2018-03-16T03:50:43Z

Codecov Report

Merging #204 into master will increase coverage by 0.2%.
The diff coverage is 87.5%.

@@           Coverage Diff            @@
##           master    #204     +/-   ##
========================================
+ Coverage    90.2%   90.5%   +0.2%     
========================================
  Files          10      10             
  Lines        1906    1899      -7     
========================================
- Hits         1720    1718      -2     
+ Misses        119     116      -3     
+ Partials       67      65      -2

Impacted Files	Coverage Δ
fgrpc/pingsrv.go	`88.1% <84.6%> (+0.9%)`	⬆️
fgrpc/grpcrunner.go	`90.8% <90.9%> (+2.6%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be80813...81b16cd. Read the comment docs.

danehans · 2018-03-16T04:29:38Z

@ldemailly Thanks for your review. I addressed your comments and have updated the PR. The purpose of this PR is to address TODO: option to specify certs. I should have created an issue to discuss my intentions before getting this far. If my approach is unwanted, let me know how you would like to address the TODO.

danehans · 2018-03-16T15:59:06Z

OK. I'll strip most of this stuff out and update the PR.

ldemailly · 2018-03-16T21:45:42Z

Sorry been crazy busy... will look more soon

ldemailly

let's use --key --cert --cacert
like curl
and
https://istio.io/docs/tasks/security/mutual-tls.html#testing-the-authentication-setup

ldemailly · 2018-03-26T22:24:06Z

README.md

@@ -77,8 +77,12 @@ target is a url (http load tests) or host:port (grpc health test).  flags are:
  -grpc-port string
 	grpc server port. Can be in the form of host:port, ip:port or port.
 	(default "8079")
-  -grpc-secure


we still need a way to standard TLS no?

This PR provides backwards compatibility. Instead of passing the -grpc-secure bool, Dial will use the one of the following:

A. The cert from -server-cert if it's provided. I can easily add the flag back in, but IMO it no longer adds value.
B. Us nil creds if dest is an https prefix (this provides backwards compatibility)
C. Insecure Dial if A or C do not apply.

I thought this is a cleaner approach than setting the bool and cert/key flags. IMO having both sets of flags causes user confusion. Using the legacy bool flag made sense, a user flips the secure/insecure switch.

I'm not following - how do I get the previous behavior exactly ?

The previous behavior is accomplished by using an https prefix with the grpc destination.
For example:

grpc client

fortio grpcping https://fortio.istio.io

or any client that uses GRPCRunnerOptions

fortio curl -loglevel debug http://localhost:8080/fortio/fetch/localhost:8080/fortio/?url=https://fortio.istio.io:443&load=Start&qps=-1&json=on&n=100&runner=grpc

ic I missed that - does it work with fortio load -grpc https://... too ? (it should I guess as destination is passed down unchanged if I recall)

we should put tls/no tls in the (json) result object

we should probably document that behavior somewhere (that grpc host:port is insecure unless a cert is given and https://host:port is secure)

does it work with fortio load -grpc https://... too ?

It does. I extensively tested this patch today. I documented the details here.

we should probably document that behavior somewhere (that grpc host:port is insecure unless a cert is given and https://host:port is secure)

I updated the Readme.

ldemailly · 2018-03-26T22:30:36Z

fortio_main.go

+	goMaxProcsFlag  = flag.Int("gomaxprocs", 0, "Setting for runtime.GOMAXPROCS, <1 doesn't change the default")
+	profileFlag     = flag.String("profile", "", "write .cpu and .mem profiles to file")
+	grpcFlag        = flag.Bool("grpc", false, "Use GRPC (health check) for load testing")
+	serverCertFlag  = flag.String("server-cert", "",


add a new line to minimize the diff
(go can be annoying with reformatting to align - also as mentioned I think the grpc-secure flag is still needed, the cert,ca,key are optional)

Will do. For clarity, the ca flag was dropped since refactoring this PR based on your initial feedback.

ldemailly · 2018-03-26T22:32:53Z

fortio_main.go

@@ -162,7 +165,7 @@ func main() {
 		}
 	case "server":
 		isServer = true
-		fgrpc.PingServer(*grpcPortFlag, fgrpc.DefaultHealthServiceName)
+		fgrpc.PingServer(*grpcPortFlag, *serverCertFlag, *serverKeyFlag, fgrpc.DefaultHealthServiceName)


I'm thinking we should add some GRPC(Server)Options instead of 5 string params
(shared between the client and server maybe)
but maybe that can be a followup

I would like to address that in a follow-on PR. This PR has been around for a while. I opened issue
#220 and assigned myself.

ldemailly · 2018-03-26T22:33:04Z

fortio_main.go

@@ -249,7 +252,7 @@ func fortioLoad(justCurl bool, percList []float64) {
 		o := fgrpc.GRPCRunnerOptions{
 			RunnerOptions:      ro,
 			Destination:        url,
-			Secure:             *grpcSecureFlag,
+			Cert:               *serverCertFlag,


the 2 are different

Understood. See my response above and let me know your thoughts. I want to make sure we're on the same page before I rebase this PR (it was a PITA the last time I rebased).

ldemailly · 2018-03-26T22:34:21Z

ui/uihandler.go

@@ -153,6 +153,7 @@ func Handler(w http.ResponseWriter, r *http.Request) {
 	qps, _ := strconv.ParseFloat(r.FormValue("qps"), 64)      // nolint: gas
 	durStr := r.FormValue("t")
 	grpcSecure := (r.FormValue("grpc-secure") == "on")
+	cert := r.FormValue("cert")


for this I think we should just use the flags (if you want a fortio UI using different cert, start a new one with different cert path)

From what I see, the challenge with that approach is:

uihandler.go creates instance o of the fgrpc.GRPCRunnerOptions object .

The flags created in fortio_main.go are not available to uihandler.go or any other pkg for that matter. Therefore, the Cert field in o can not be populated based on any flags.

I believe what you are asking for can be accomplished by putting Fortio flags into a pkg (i.e. cli ) and having fortio_main.go and any other dependent pkg's import cli. If this has your interest, please create an Issue, assign me and I'll work on refactoring the Fortio cli into a separate pkg. However, I don't think that work should block this PR.

we could also pass those global options to ui.Serve()

Yes, assuming the cli options are in a separate pkg. As I mentioned, I am happy to take on this Fortio cli refactoring. Just create an Issue and assign me. Until then, I think providing a field to enter the cert path is a reasonable option.

danehans

Thanks for the follow-up review!

danehans · 2018-03-27T18:36:41Z

README.md

@@ -77,8 +77,12 @@ target is a url (http load tests) or host:port (grpc health test).  flags are:
  -grpc-port string
 	grpc server port. Can be in the form of host:port, ip:port or port.
 	(default "8079")
-  -grpc-secure


This PR provides backwards compatibility. Instead of passing the -grpc-secure bool, Dial will use the one of the following:

A. The cert from -server-cert if it's provided. I can easily add the flag back in, but IMO it no longer adds value.
B. Us nil creds if dest is an https prefix (this provides backwards compatibility)
C. Insecure Dial if A or C do not apply.

I thought this is a cleaner approach than setting the bool and cert/key flags. IMO having both sets of flags causes user confusion. Using the legacy bool flag made sense, a user flips the secure/insecure switch.

danehans · 2018-03-27T18:41:42Z

fortio_main.go

+	goMaxProcsFlag  = flag.Int("gomaxprocs", 0, "Setting for runtime.GOMAXPROCS, <1 doesn't change the default")
+	profileFlag     = flag.String("profile", "", "write .cpu and .mem profiles to file")
+	grpcFlag        = flag.Bool("grpc", false, "Use GRPC (health check) for load testing")
+	serverCertFlag  = flag.String("server-cert", "",


Will do. For clarity, the ca flag was dropped since refactoring this PR based on your initial feedback.

danehans · 2018-03-27T18:48:36Z

fortio_main.go

@@ -162,7 +165,7 @@ func main() {
 		}
 	case "server":
 		isServer = true
-		fgrpc.PingServer(*grpcPortFlag, fgrpc.DefaultHealthServiceName)
+		fgrpc.PingServer(*grpcPortFlag, *serverCertFlag, *serverKeyFlag, fgrpc.DefaultHealthServiceName)


I would like to address that in a follow-on PR. This PR has been around for a while. I opened issue
#220 and assigned myself.

danehans · 2018-03-27T18:49:47Z

fortio_main.go

@@ -249,7 +252,7 @@ func fortioLoad(justCurl bool, percList []float64) {
 		o := fgrpc.GRPCRunnerOptions{
 			RunnerOptions:      ro,
 			Destination:        url,
-			Secure:             *grpcSecureFlag,
+			Cert:               *serverCertFlag,


Understood. See my response above and let me know your thoughts. I want to make sure we're on the same page before I rebase this PR (it was a PITA the last time I rebased).

danehans · 2018-03-27T19:10:06Z

ui/uihandler.go

@@ -153,6 +153,7 @@ func Handler(w http.ResponseWriter, r *http.Request) {
 	qps, _ := strconv.ParseFloat(r.FormValue("qps"), 64)      // nolint: gas
 	durStr := r.FormValue("t")
 	grpcSecure := (r.FormValue("grpc-secure") == "on")
+	cert := r.FormValue("cert")


From what I see, the challenge with that approach is:

uihandler.go creates instance o of the fgrpc.GRPCRunnerOptions object .

The flags created in fortio_main.go are not available to uihandler.go or any other pkg for that matter. Therefore, the Cert field in o can not be populated based on any flags.

I believe what you are asking for can be accomplished by putting Fortio flags into a pkg (i.e. cli ) and having fortio_main.go and any other dependent pkg's import cli. If this has your interest, please create an Issue, assign me and I'll work on refactoring the Fortio cli into a separate pkg. However, I don't think that work should block this PR.

ldemailly · 2018-03-29T04:50:52Z

fgrpc/grpcrunner.go

+		log.Infof("Using server certificate %v to construct TLS credentials", cert)
+		opts = append(opts, grpc.WithTransportCredentials(creds))
+	case strings.HasPrefix(serverAddr, "https://"):
+		creds := credentials.NewTLS(nil)


ic this is where the https/tls behaviour is

Yeah, all the intelligence is in Dial.

ldemailly · 2018-03-29T04:52:36Z

README.md

@@ -77,8 +77,12 @@ target is a url (http load tests) or host:port (grpc health test).  flags are:
  -grpc-port string
 	grpc server port. Can be in the form of host:port, ip:port or port.
 	(default "8079")
-  -grpc-secure


ic I missed that - does it work with fortio load -grpc https://... too ? (it should I guess as destination is passed down unchanged if I recall)

we should put tls/no tls in the (json) result object

we should probably document that behavior somewhere (that grpc host:port is insecure unless a cert is given and https://host:port is secure)

ldemailly · 2018-03-29T04:54:19Z

fortio_main.go

@@ -162,7 +165,7 @@ func main() {
 		}
 	case "server":
 		isServer = true
-		fgrpc.PingServer(*grpcPortFlag, fgrpc.DefaultHealthServiceName)
+		fgrpc.PingServer(*grpcPortFlag, *serverCertFlag, *serverKeyFlag, fgrpc.DefaultHealthServiceName)


ldemailly · 2018-03-29T04:54:43Z

fortio_main.go

@@ -249,7 +252,7 @@ func fortioLoad(justCurl bool, percList []float64) {
 		o := fgrpc.GRPCRunnerOptions{
 			RunnerOptions:      ro,
 			Destination:        url,
-			Secure:             *grpcSecureFlag,
+			Cert:               *serverCertFlag,


ldemailly · 2018-03-29T04:55:20Z

fortio_main.go

@@ -312,20 +315,19 @@ func fortioLoad(justCurl bool, percList []float64) {
 }

 func grpcClient() {
-	if len(flag.Args()) != 1 {
+	if len(flag.Args()) < 1 {


which case is that change for ?

ldemailly · 2018-03-29T04:56:15Z

fgrpc/pingsrv_test.go

+	iPort := PingServer("0", "", "", "foo")
+	iAddr := fmt.Sprintf("localhost:%d", iPort)
+	t.Logf("test insecure grpc ping server running, will connect to %s", iAddr)
+	sPort := PingServer("0", "../testdata/server.crt", "../testdata/server.key",


isn't this relative path making success dependent on where it is ran from ? (somehow doubt it works from both parent or different directories?)

not sure we need a new directory - tbd if go testing has provision for files relative to test files?

I tested by copying the cert/key from the testdata directory to the project's root directory and updating the following in grpcrunner_test.go:

svrCrt = "../server.crt" svrKey = "../server.key"

The tests passed.

not sure we need a new directory - tbd if go testing has provision for files relative to test files?

I like the idea of storing all test data for a project in a specific directory. I think it's clean and makes it very apparent to devs. Let me know where I should move the test cert/key if you want me to remove the directory.

ldemailly · 2018-03-29T20:39:12Z

this is good, do you have time to raise the coverage ? (for instance pass invalid credentials, connect to the wrong port/settings,...)

ldemailly · 2018-03-29T20:43:20Z

where is -cacert ? I see in the example you pass a cert to the client, I assume that implies it expects that cert, but it should be a CA instead ?

ldemailly · 2018-03-29T20:44:34Z

README.md

-	Use secure transport (tls) for GRPC
+  -cert
+	Full path to the server certificate required for secure grpc.
+	Used by grpcping and server. (default "") means secure grpc is disabled.


add:

"unless https:// destination is used, in which case standard TLS is expected (with valid server certificates signed by the internet standard CAs)"

or some such

ldemailly · 2018-03-29T20:45:59Z

fgrpc/grpcrunner.go

+	//var creds credentials.TransportCredentials
+	switch {
+	case cert != "":
+		creds, err := credentials.NewClientTLSFromFile(cert, "")


I wonder here if we're confusing client and server cert

we should be able to do mTLS by providing an optional client cert; and expect a server cert signed by a custom CA

I updated the name of this PR to properly reflect the implemented functionality. This PR originally provided mTLS, but I simplified things based on your initial feedback. This PR will allow clients (i.e. grpcping) to authenticate the Fortio gRPC server and encrypt the data exchanged. In my original PR, the server cert was fetched from the CA (Fortio server). Since refactoring, the server cert must be shipped with the client-side application. This blog provides details of the original and current approach for this PR. Let me know how you would like to proceed.

This section provides a detailed explanation of the basic TLS handshake implemented by this PR.

danehans · 2018-04-24T16:43:58Z

@ldemailly will this PR be ready to merge after I fix the conflicts?

ldemailly

almost, see below

ldemailly · 2018-04-24T18:43:18Z

Makefile

 # go test ./... and others run in vendor/ and cause problems (!)
-PACKAGES:=$(shell find . -type d -print | egrep -v "/(\.|vendor|static|templates|release|docs|json)")
+PACKAGES:=$(shell find . -type d -print | egrep -v "/(\.|vendor|static|templates|release|docs|json|cert-tmp)")


$(CERT_TEMP) ?

ldemailly · 2018-04-24T18:43:38Z

Makefile

@@ -14,25 +14,41 @@ TAG:=$(USER)$(shell date +%y%m%d_%H%M%S)

 DOCKER_TAG = $(DOCKER_PREFIX)$(IMAGE):$(TAG)

+CERT_TEMP := ./cert-tmp/


CERT_TEMP_DIR := ...

ldemailly · 2018-04-24T18:46:44Z

README.md

+UI starting - visit:
+http://localhost:8080/fortio/
+Https redirector running on :8081
+Fortio 0.9.0 grpc ping server listening on port :8079


when merging make sure there is 0.9.1 everywhere

Looks like docs are now using 0.10.0. I have changed to 0.10.0.

it's actually 0.10.1 now :-)

ldemailly · 2018-04-24T18:47:27Z

README.md

+* A `grpcping` using TLS. First, start Fortio server with the `-cert` and `-key` flags.
+`/path/to/fortio/server.crt` and `/path/to/fortio/server.key` are paths to the TLS certificate and key that
+you are responsible for providing. Creating a TLS certificate is outside the scope of this document. This example uses
+`localhost` in the `Subject Alternative Name` (SAN) of the certificate. The SAN should match the domain name that


remove that part (localhost san) as it doesn't matter / is also out of scope ? - or mentioned the makefile can generate test only certs

ldemailly · 2018-04-24T18:51:34Z

README.md

+Next, run the `load` command with the `-cacert` flag:
+```
+$ fortio load -cacert /etc/ssl/certs/ca.crt -grpc localhost:8079
+


missing close 3 ` - please make sure you preview/look over the changes

ldemailly · 2018-04-24T18:56:45Z

Webtest.sh

+CERT=/etc/ssl/certs/ca-certificates.crt
+TEST_CERT_VOL=/etc/ssl/certs/fortio
+BUILD_IMAGE_TAG=v7
+BUILD_IMAGE=istio/fortio.build:$BUILD_IMAGE_TAG


change so it works with https://github.com/istio/fortio/blob/master/Makefile#L112

ie: add the file name to the list and use istio/fortio.build:v7 directly so sed finds it when we go to v8

ldemailly · 2018-04-24T19:02:01Z

fortio_main.go

@@ -78,8 +78,10 @@ var (
 	goMaxProcsFlag    = flag.Int("gomaxprocs", 0, "Setting for runtime.GOMAXPROCS, <1 doesn't change the default")
 	profileFlag       = flag.String("profile", "", "write .cpu and .mem profiles to file")
 	grpcFlag          = flag.Bool("grpc", false, "Use GRPC (health check by default, add -ping for ping) for load testing")
-	grpcSecureFlag    = flag.Bool("grpc-secure", false, "Use secure transport (tls) for GRPC")


that change isn't reflected in the README.md

see release/updateFlags.sh for how to update

we also need to document somewhere that https:// replaces -grpc-secure

I have updated the readme to remove the -secure-grpc flag and ran the updateFlags.sh script.

danehans · 2018-04-25T19:35:00Z

@ldemailly thanks for the feedback. I need a couple days to wrap-up something I'm working on and I'll push a new commit that addresses your concerns.

ldemailly · 2018-04-25T21:50:38Z

thanks for your patience and flexibility !

danehans

I'm getting ready to push my latest commit that addresses your feedback.

danehans · 2018-04-30T20:56:00Z

fortio_main.go

@@ -78,8 +78,10 @@ var (
 	goMaxProcsFlag    = flag.Int("gomaxprocs", 0, "Setting for runtime.GOMAXPROCS, <1 doesn't change the default")
 	profileFlag       = flag.String("profile", "", "write .cpu and .mem profiles to file")
 	grpcFlag          = flag.Bool("grpc", false, "Use GRPC (health check by default, add -ping for ping) for load testing")
-	grpcSecureFlag    = flag.Bool("grpc-secure", false, "Use secure transport (tls) for GRPC")


I have updated the readme to remove the -secure-grpc flag and ran the updateFlags.sh script.

danehans · 2018-04-30T21:04:57Z

README.md

+* A `grpcping` using TLS. First, start Fortio server with the `-cert` and `-key` flags.
+`/path/to/fortio/server.crt` and `/path/to/fortio/server.key` are paths to the TLS certificate and key that
+you are responsible for providing. Creating a TLS certificate is outside the scope of this document. This example uses
+`localhost` in the `Subject Alternative Name` (SAN) of the certificate. The SAN should match the domain name that


danehans · 2018-04-30T21:05:11Z

Makefile

@@ -14,25 +14,41 @@ TAG:=$(USER)$(shell date +%y%m%d_%H%M%S)

 DOCKER_TAG = $(DOCKER_PREFIX)$(IMAGE):$(TAG)

+CERT_TEMP := ./cert-tmp/


danehans · 2018-04-30T21:05:13Z

Makefile

 # go test ./... and others run in vendor/ and cause problems (!)
-PACKAGES:=$(shell find . -type d -print | egrep -v "/(\.|vendor|static|templates|release|docs|json)")
+PACKAGES:=$(shell find . -type d -print | egrep -v "/(\.|vendor|static|templates|release|docs|json|cert-tmp)")


danehans · 2018-04-30T21:08:04Z

README.md

+UI starting - visit:
+http://localhost:8080/fortio/
+Https redirector running on :8081
+Fortio 0.9.0 grpc ping server listening on port :8079


Looks like docs are now using 0.10.0. I have changed to 0.10.0.

danehans · 2018-04-30T21:19:45Z

README.md

+Next, run the `load` command with the `-cacert` flag:
+```
+$ fortio load -cacert /etc/ssl/certs/ca.crt -grpc localhost:8079
+


danehans · 2018-04-30T21:20:56Z

fortio_main.go

@@ -78,8 +78,10 @@ var (
 	goMaxProcsFlag    = flag.Int("gomaxprocs", 0, "Setting for runtime.GOMAXPROCS, <1 doesn't change the default")
 	profileFlag       = flag.String("profile", "", "write .cpu and .mem profiles to file")
 	grpcFlag          = flag.Bool("grpc", false, "Use GRPC (health check by default, add -ping for ping) for load testing")
-	grpcSecureFlag    = flag.Bool("grpc-secure", false, "Use secure transport (tls) for GRPC")


danehans

I'm getting ready to push my latest commit that addresses your feedback.

danehans · 2018-05-02T12:13:04Z

@ldemailly do you mind reviewing when you have a moment?

ldemailly · 2018-05-14T15:25:33Z

looking now

ldemailly · 2018-05-14T16:10:22Z

getting some github (unicorn) errors when I try to open this PR somehow)

qq: why did you remove TestExitedPingServer ?

ldemailly

Really almost there now - Thanks!

A few comment and you only need to update where I prefixed by "NeedFix" (4 places)

ldemailly · 2018-05-14T17:34:49Z

Webtest.sh

 # switch to report mode
 docker stop $DOCKERID
 docker rm $DOCKERNAME
+docker stop $DOCKERSECID
+docker rm $DOCKERSECNAME


this should probably be "trap"ed too but I guess we can clean that up later

I thought the trap is used for the final Fortio instance, the report mode server. I stop/rm the Docker assets for the secure grpc instance of Fortio server before the script starts the report mode Fortio server.

trap means cleanup like a finally, in case of error in any of tbe tests

ldemailly · 2018-05-14T17:37:40Z

fgrpc/grpcrunner.go

 	if err != nil {
-		log.Errf("failed to conect to %s with tls %v: %v", serverAddr, tls, err)
+		log.Errf("failed to connect to %s: %v", serverAddr, err)


NeedFix: when an error occurs it is very important to log all the input, please add cacert and override to the statement

ldemailly · 2018-05-14T17:38:06Z

fgrpc/grpcrunner.go

+// Dial dials grpc using insecure or tls transport security when serverAddr
+// has prefixHTTPS or cert is provided. If override is set to a non empty string,
+// it will override the virtual host name of authority in requests.
+func Dial(serverAddr string, cacert, override string) (conn *grpc.ClientConn, err error) {


NeedFix: either put the type for all 3 or only once but here it's odd to have string 2 out of 3 args

ldemailly · 2018-05-14T17:41:10Z

fgrpc/grpcrunner_test.go

+				},
+				Destination:  sDest,
+				CACert:       caCrt,
+				CertOverride: "invalidName",


can we have a positive test of CertOverride (where it is set yet works ?)

ldemailly · 2018-05-14T17:43:06Z

fgrpc/grpcrunner_test.go

+		{
+			name: "insecure runner with payload",
+			runnerOpts: GRPCRunnerOptions{
+				RunnerOptions: periodic.RunnerOptions{


NeedFix: there is a lot of copy paste of this section, why not have the runner options in a variable ?

I guess I find table tests easier to read when testing multiple test cases. WOuld you rather I follow the existing pattern for the test? Something like this:

func TestGRPCRunner(t *testing.T) { log.SetLogLevel(log.Info) iPort := PingServer("0", "", "", "bar", 0) iDest := fmt.Sprintf("localhost:%d", iPort) sPort := PingServer("0", svrCrt, svrKey, "bar", 0) sDest := fmt.Sprintf("localhost:%d", sPort) ro := periodic.RunnerOptions{ QPS: 100, Resolution: 0.00001, } opts := GRPCRunnerOptions{ RunnerOptions: ro, Destination: iDest, Profiler: "test.profile", } res, err := RunGRPCTest(&opts) if err != nil { t.Error(err) return } totalReq := res.DurationHistogram.Count ok := res.RetCodes[grpc_health_v1.HealthCheckResponse_SERVING] if totalReq != ok { t.Errorf("Mismatch between requests %d and ok %v", totalReq, res.RetCodes) } // Test a grpc runner with TLS enabled. opts.Destination = sDest opts.CACert = caCrt res, err = RunGRPCTest(&opts) if err != nil { t.Error(err) return } totalReq = res.DurationHistogram.Count ok = res.RetCodes[grpc_health_v1.HealthCheckResponse_SERVING] if totalReq != ok { t.Errorf("Mismatch between requests %d and ok %v", totalReq, res.RetCodes) } }

Keep in mind the above example is only exercising 2 test cases.

ldemailly · 2018-05-14T17:43:35Z

fgrpc/grpcrunner_test.go

+		{
+			name: "insecure runner",
+			runnerOpts: GRPCRunnerOptions{
+				RunnerOptions: periodic.RunnerOptions{


NeedFix: same

ldemailly · 2018-05-14T17:45:10Z

fgrpc/pingsrv.go

+			log.Fatalf("Invalid TLS credentials: %v\n", err)
+		}
+		log.Infof("Using server certificate %v to construct TLS credentials", cert)
+		log.Infof("Using server key %v to construct TLS credentials", key)


this could probably be a single log info instead of 2 but it's ok as is too

danehans

Please see my feedback and example refactor of TestGRPCRunner. I would like to make sure we're on the same page before I refactor TestGRPCRunner and TestGRPCRunnerWithError.

danehans · 2018-05-14T22:51:54Z

Webtest.sh

 # switch to report mode
 docker stop $DOCKERID
 docker rm $DOCKERNAME
+docker stop $DOCKERSECID
+docker rm $DOCKERSECNAME


I thought the trap is used for the final Fortio instance, the report mode server. I stop/rm the Docker assets for the secure grpc instance of Fortio server before the script starts the report mode Fortio server.

danehans · 2018-05-14T22:53:07Z

fgrpc/grpcrunner.go

+// Dial dials grpc using insecure or tls transport security when serverAddr
+// has prefixHTTPS or cert is provided. If override is set to a non empty string,
+// it will override the virtual host name of authority in requests.
+func Dial(serverAddr string, cacert, override string) (conn *grpc.ClientConn, err error) {


danehans · 2018-05-14T22:57:51Z

fgrpc/grpcrunner.go

 	if err != nil {
-		log.Errf("failed to conect to %s with tls %v: %v", serverAddr, tls, err)
+		log.Errf("failed to connect to %s: %v", serverAddr, err)


danehans · 2018-05-14T23:51:50Z

fgrpc/grpcrunner_test.go

+		{
+			name: "insecure runner with payload",
+			runnerOpts: GRPCRunnerOptions{
+				RunnerOptions: periodic.RunnerOptions{


I guess I find table tests easier to read when testing multiple test cases. WOuld you rather I follow the existing pattern for the test? Something like this:

func TestGRPCRunner(t *testing.T) { log.SetLogLevel(log.Info) iPort := PingServer("0", "", "", "bar", 0) iDest := fmt.Sprintf("localhost:%d", iPort) sPort := PingServer("0", svrCrt, svrKey, "bar", 0) sDest := fmt.Sprintf("localhost:%d", sPort) ro := periodic.RunnerOptions{ QPS: 100, Resolution: 0.00001, } opts := GRPCRunnerOptions{ RunnerOptions: ro, Destination: iDest, Profiler: "test.profile", } res, err := RunGRPCTest(&opts) if err != nil { t.Error(err) return } totalReq := res.DurationHistogram.Count ok := res.RetCodes[grpc_health_v1.HealthCheckResponse_SERVING] if totalReq != ok { t.Errorf("Mismatch between requests %d and ok %v", totalReq, res.RetCodes) } // Test a grpc runner with TLS enabled. opts.Destination = sDest opts.CACert = caCrt res, err = RunGRPCTest(&opts) if err != nil { t.Error(err) return } totalReq = res.DurationHistogram.Count ok = res.RetCodes[grpc_health_v1.HealthCheckResponse_SERVING] if totalReq != ok { t.Errorf("Mismatch between requests %d and ok %v", totalReq, res.RetCodes) } }

Keep in mind the above example is only exercising 2 test cases.

danehans

Please see my feedback and example refactor of TestGRPCRunner. I would like to make sure we're on the same page before I refactor TestGRPCRunner and TestGRPCRunnerWithError.

ldemailly · 2018-05-16T20:41:39Z

Webtest.sh

@@ -14,6 +14,9 @@ DOCKERID=$(docker run -d --ulimit nofile=$FILE_LIMIT --name $DOCKERNAME istio/fo
 function cleanup {
  docker stop $DOCKERID
  docker rm $DOCKERNAME
+  docker stop $DOCKERSECID


in theory should have an if that it is set/started (as it's not started right away unlike DOCKERID) but it's ok, you didn't have to change it (for that reason as well - I'll try - one test is interrupt or have a test error out and see if it still finishes correctly/cleansup without too many additional errors)

ldemailly · 2018-05-16T22:52:37Z

fgrpc/grpcrunner.go

+			log.Errf("Invalid TLS credentials: %v\n", err)
+			return nil, err
+		}
+		log.Infof("Using CA certificate %v to construct TLS credentials", cacert)


optional: here too (override logging)

ldemailly · 2018-05-16T22:54:27Z

fgrpc/grpcrunner_test.go

-	if totalReq != ok {
-		t.Errorf("Mismatch between requests %d and ok %v", totalReq, res.RetCodes)
+	for _, test := range tests {
+		switch {


I didn't mean to have to put a big switch here :( why can't those be above directly ?

ldemailly · 2018-05-21T23:28:13Z

closing for #249 (this PR makes github choke so redone there)
to see history/comments check with incognito window (and/or when github fixes that bug)

@danehans

Add support for custom CA and certificates and key, and thus mTLS, for fortio's GRPC client/server Work by @danehans in #204 with my support/reviews/tweaks commits: * Adds gRPC mTLS Support * Addresses @ldemailly 3/15/18 feedback * Move cert log msgs and fix typo * Addresses round 2 of @ldemailly comments * Added docs and minor fixups * Updated Dial return error * Updates 2nd ping server fail test with new params * Removes unneeded var declaration in Dial func * Realigns flags in fortio_main * Updates cert/key flags and updates readme * Adds more details to docs * Updates e2e/ut, docs and Dial func to support vhost override * Implements new -ca-cert flag * Generates certs, adds e2e and -cacert flag * Removes openssl dep * Adds logs to certgen and fixes remote docker bind mount * reverts to certgen script and ui * Updates makefile/Webtest/Certgen and ui https func * Add make deps to docker compose * Fixes make command in Dockerfile.test for CI * Added Make dep and cleaned-up addHTTPS func * Adds add'l Dial func test cases * Addresses @ldemailly comments * Adds TestExitedPingServer test * Refactor PingServer test for failed crt/key * use 0.10.1 like rest of readme * Addresses ldemailly feedback * Removed docker stop/rm from trap * Fix make test 2nd make test would error out because of cert-tmp * Fix clean and error handling for test (new docker images) * Switch v to 0.11.0 Except for json which should be 0.9.0 (format as it was when it was added) * Fixing test to not check for test.name to set options (!) * Even more cleanup of duplicate * Sync the readme and the flags from the code

ldemailly self-requested a review March 15, 2018 04:52

ldemailly reviewed Mar 15, 2018

View reviewed changes

danehans force-pushed the grpc_certs branch from cc8dbcb to 573fe79 Compare March 16, 2018 04:36

danehans mentioned this pull request Mar 22, 2018

add option to provide client certs and ca for grpc (done for https runner/curl) #141

Closed

2 tasks

danehans force-pushed the grpc_certs branch from 598e68f to 99d42c7 Compare March 22, 2018 18:34

ldemailly reviewed Mar 26, 2018

View reviewed changes

danehans mentioned this pull request Mar 27, 2018

Reduce the Number of PingServer Input Parameters #220

Open

danehans commented Mar 27, 2018

View reviewed changes

ldemailly reviewed Mar 29, 2018

View reviewed changes

danehans force-pushed the grpc_certs branch from 6687a82 to 568f431 Compare March 29, 2018 19:54

ldemailly reviewed Mar 29, 2018

View reviewed changes

danehans changed the title ~~Adds gRPC mTLS Support~~ Adds gRPC Server-side TLS Support Mar 29, 2018

Daneyon Hansen added 8 commits March 29, 2018 16:21

Adds gRPC mTLS Support

cf67cba

Addresses @ldemailly 3/15/18 feedback

6eb082a

Move cert log msgs and fix typo

37fad1c

Addresses round 2 of @ldemailly comments

07fa5d9

Added docs and minor fixups

ab914fe

Updated Dial return error

b17a110

Updates 2nd ping server fail test with new params

13765b5

Removes unneeded var declaration in Dial func

0c67f37

ldemailly reviewed Apr 24, 2018

View reviewed changes

Merge branch 'master' fortio into grpc_certs

02b4ac5

danehans commented May 1, 2018

View reviewed changes

Daneyon Hansen added 4 commits May 1, 2018 12:01

Addresses @ldemailly comments

d611127

Merge branch 'master' of fortio into grpc_certs

462e470

Adds TestExitedPingServer test

e22361c

Refactor PingServer test for failed crt/key

c425d19

danehans force-pushed the grpc_certs branch from ef95b80 to c425d19 Compare May 2, 2018 10:02

ldemailly and others added 2 commits May 14, 2018 18:13

Merge branch 'master' into grpc_certs

aeacf9f

use 0.10.1 like rest of readme

7d1d2d3

ldemailly reviewed May 14, 2018

View reviewed changes

danehans commented May 14, 2018

View reviewed changes

Daneyon Hansen added 4 commits May 16, 2018 10:26

Addresses ldemailly feedback

84ad79a

Merge branch 'master' of fortio into grpc_certs

9c55004

Merge branch 'grpc_certs' fortio into grpc_certs

75fccef

Removed docker stop/rm from trap

81b16cd

ldemailly reviewed May 16, 2018

View reviewed changes

This was referenced May 21, 2018

Danehans grpc certs #249

Merged

Percentile list tests are added for uihandler #207

Closed

ldemailly closed this May 21, 2018

		@@ -14,25 +14,41 @@ TAG:=$(USER)$(shell date +%y%m%d_%H%M%S)

		DOCKER_TAG = $(DOCKER_PREFIX)$(IMAGE):$(TAG)

		CERT_TEMP := ./cert-tmp/

Adds gRPC Server-side TLS Support #204

Adds gRPC Server-side TLS Support #204

Conversation

danehans commented Mar 14, 2018 • edited Loading

ldemailly commented Mar 15, 2018 • edited Loading

danehans commented Mar 15, 2018

ldemailly commented Mar 15, 2018

ldemailly left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 16, 2018 • edited Loading

Codecov Report

danehans commented Mar 16, 2018

danehans commented Mar 16, 2018

ldemailly commented Mar 16, 2018

ldemailly left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danehans Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danehans Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

ldemailly Mar 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danehans left a comment

Choose a reason for hiding this comment

danehans Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danehans Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldemailly commented Mar 29, 2018

ldemailly commented Mar 29, 2018

ldemailly Mar 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danehans commented Apr 24, 2018

ldemailly left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danehans commented Mar 14, 2018 •

edited

Loading

ldemailly commented Mar 15, 2018 •

edited

Loading

codecov bot commented Mar 16, 2018 •

edited

Loading

danehans Mar 27, 2018 •

edited

Loading

danehans Mar 27, 2018 •

edited

Loading

ldemailly Mar 29, 2018 •

edited

Loading

danehans Mar 27, 2018 •

edited

Loading

danehans Mar 27, 2018 •

edited

Loading

ldemailly Mar 29, 2018 •

edited

Loading