advancedtls: add examples for reloading from file system #3976

ZhenLian · 2020-10-22T06:41:16Z

This PR adds an example in advancedtls on how to reload credentials from file system.

easwars · 2020-10-22T21:54:56Z

Could you please hold off on this until #3981 gets in. Thanks.

security/advancedtls/examples/credential_reloading_from_files/client/main.go

security/advancedtls/examples/credential_reloading_from_files/server/main.go

easwars · 2020-10-23T00:42:55Z

security/advancedtls/examples/credential_reloading_from_files/README.md

+A very common way to achieve this is to reload from files.
+
+In this example, users will need to provide a set of locations on the file system holding the credential data.
+Once the credential data needs to be updated, users just need to change the credential data in the file system, and gRPC will pick up the changes automatically.


The client is making a single connection to the server. Given that established connections are not affected by change in the files being watched, how does this example demonstrate the credential reloading feature?

Gotcha. I understand your concern. I think the purpose of this example is to demonstrate how to set the API. The credential reloading feature is covered in advancedtls_integration_test, which reloads credentials in temp files.
This example is needed by that internal project, which will have a link pointed to this example in their user guide. The original sample code they gave me also didn't include the reloading feature.
I think the current wordings in README is a bit confusing. I've updated some text. Could you please take a look? Thank you so much!

easwars · 2020-10-23T00:43:22Z

security/advancedtls/examples/examples_test.sh

@@ -0,0 +1,129 @@
+#!/bin/bash


I can barely read bash. Please ask someone else to take a look at this.

I don't, either :) @dfawley Could you please take a look at this please?
This is just a simple copy from https://github.com/grpc/grpc-go/blob/master/examples/examples_test.sh, with the following minor modifications:

put the right test name and output in EXAMPLES, EXPECTED_SERVER_OUTPUT, and EXPECTED_CLIENT_OUTPUT

replace cd ./examples with cd ./security/advancedtls/examples

add a sleep 2s at the time after we start the server and before we initiate the client request(I added this mainly because otherwise, the test could sometimes fail locally with a server unavailable error. I didn't try on Travis, but when I ran other tests in main grpc it would sometimes fail as well. I am guessing my machine is a bit old that sometimes the server code needs more time to boot up)

add a sleep 2s at the time after we start the server and before we initiate the client request(I added this mainly because otherwise, the test could sometimes fail locally with a server unavailable error. I didn't try on Travis, but when I ran other tests in main grpc it would sometimes fail as well. I am guessing my machine is a bit old that sometimes the server code needs more time to boot up)

That seems strange. Do you see these errors when running the other examples_test.sh? If the client sends wait-for-ready RPCs then you shouldn't need to worry about this at all.

Ah, it turned out that the tests failed in other examples_test.sh was due to a port is already used on my machine, instead of the unavailable error. So I think other scripts are good.
Just to confirm, the way to send wait-for-ready RPCs is through grpc.WithBlock(), right? I've updated that, removed the 2s sleep time, and ran the script for 100 times. No errors anymore.
Could you please check if the script make sense now?

Just to confirm, the way to send wait-for-ready RPCs is through grpc.WithBlock(), right? I've updated that, removed the 2s sleep time, and ran the script for 100 times. No errors anymore.

No, WithBlock makes Dial wait until there is a ready connection. WaitForReady means the RPC will wait until the channel is ready (however long and after however many failures that takes) before sending the RPC. It shouldn't be necessary, but if you are seeing weird failures due to connectivity, it can help. WithBlock also shouldn't be necessary; I'd remove it unless you can explain why it fixes things.

OK, sure. I've changed to WaitForReady and it also works. Please take a look, thank you so much!

OH, I thought you wouldn't need WFR either, as both of these options are actually unusual. But I see there is a race at startup here: if the client Dials before the server is Listening, then it will get an error. In this case, either WithBlock or WFR RPCs should be fine.

Great! Thanks for the confirmation!

easwars

The example client and server look good to me. Please also get approval for the bash changes. Thanks.

Also do you think you can add a TODO somewhere (either in the code or in the readme) that this example needs to be updated once #3981 gets in. Again, I'm a little concerned about asking someone to use this example which is going to change very quickly. At least it needs to be clearly communicated to them.

easwars · 2020-10-23T18:53:28Z

security/advancedtls/examples/credential_reloading_from_files/client/main.go

+	// Default timeout for normal connections.
+	defaultConnTimeout = 5 * time.Second
+	// Intervals that set to monitor the credential updates.
+	credRefreshingInterval = 200 * time.Millisecond


Does this have to be this low? Or at least add a comment saying this is set to an impractically low value just for this example, and that in real use cases, this should be in the order of minutes.

Sounds good! It doesn't need to be this low. I've updated it to 1 minute.

ZhenLian · 2020-10-23T22:30:58Z

@easwars Sounds good. I've added some TODOs here. The other team said they would need the example in a "preview" state, and they've made it clear that things in their user guide could change. So I think it should be fine here.

ZhenLian · 2020-10-27T16:58:15Z

@easwars @dfawley Thanks a lot for the review! Merging now...

* add examples for reloading from file system

add examples for reloading from file system

0e21dbd

ZhenLian force-pushed the zhen_aTLS_examples branch from c0b46b9 to 0e21dbd Compare October 22, 2020 06:42

fix fmt issue

bd48f0b

menghanl assigned easwars Oct 22, 2020

menghanl requested a review from easwars October 22, 2020 20:45

easwars requested changes Oct 23, 2020

View reviewed changes

fix comments

c70a9e4

ZhenLian force-pushed the zhen_aTLS_examples branch from 5d975c7 to c70a9e4 Compare October 23, 2020 05:34

ZhenLian requested a review from easwars October 23, 2020 16:59

easwars approved these changes Oct 23, 2020

View reviewed changes

easwars assigned ZhenLian and unassigned easwars Oct 23, 2020

fix new comments

23eb281

ZhenLian requested a review from dfawley October 23, 2020 22:17

dfawley approved these changes Oct 24, 2020

View reviewed changes

ZhenLian added no release notes and removed no release notes labels Oct 26, 2020

ZhenLian modified the milestone: 1.34 Release Oct 26, 2020

ZhenLian force-pushed the zhen_aTLS_examples branch from 19b994d to bee76a3 Compare October 26, 2020 17:24

use grpc.WaitForReady(true); retrigger presubit checks

3d8d3b8

ZhenLian force-pushed the zhen_aTLS_examples branch from bee76a3 to 3d8d3b8 Compare October 26, 2020 19:50

ZhenLian merged commit 829af01 into grpc:master Oct 27, 2020

davidkhala pushed a commit to Hyperledger-TWGC/grpc that referenced this pull request Dec 7, 2020

advancedtls: add examples for reloading from file system (grpc#3976)

18a4d41

* add examples for reloading from file system

github-actions bot locked as resolved and limited conversation to collaborators May 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

advancedtls: add examples for reloading from file system #3976

advancedtls: add examples for reloading from file system #3976

ZhenLian commented Oct 22, 2020 •

edited

easwars commented Oct 22, 2020

easwars Oct 23, 2020

ZhenLian Oct 23, 2020

easwars Oct 23, 2020

ZhenLian Oct 23, 2020

dfawley Oct 23, 2020

ZhenLian Oct 23, 2020

dfawley Oct 23, 2020

ZhenLian Oct 23, 2020

dfawley Oct 24, 2020

ZhenLian Oct 26, 2020

easwars left a comment

easwars Oct 23, 2020

ZhenLian Oct 23, 2020

ZhenLian commented Oct 23, 2020

ZhenLian commented Oct 27, 2020

advancedtls: add examples for reloading from file system #3976

advancedtls: add examples for reloading from file system #3976

Conversation

ZhenLian commented Oct 22, 2020 • edited

easwars commented Oct 22, 2020

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

easwars left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZhenLian commented Oct 23, 2020

ZhenLian commented Oct 27, 2020

ZhenLian commented Oct 22, 2020 •

edited