certprovider-api: update api to include certificate name #3797

easwars · 2020-08-06T16:12:55Z

No description provided.

easwars · 2020-08-06T17:25:10Z

@ZhenLian : This is the PR I was talking about.

credentials/tls/certprovider/distributor.go

credentials/tls/certprovider/provider.go

credentials/tls/certprovider/store.go

credentials/tls/certprovider/provider.go

ZhenLian · 2020-08-06T21:52:52Z

@ZhenLian : This is the PR I was talking about.

Thanks for letting me know! Also, I checked with @jiangtaoli2016: currently we don't have any plans to expose the certificate name to users in file_reloading case, so I think using a default cert name to hide this information from users would work for our case here.

easwars

PTAL.

credentials/tls/certprovider/distributor.go

credentials/tls/certprovider/provider.go

credentials/tls/certprovider/store.go

And get rid of KeyMaterialReader type.

credentials/tls/certprovider/provider.go

credentials/tls/certprovider/distributor_test.go

credentials/tls/certprovider/store_test.go

dfawley · 2020-08-14T18:52:59Z

credentials/tls/certprovider/store_test.go

+	p, err := fpb1.providerChan.Receive()
 	if err != nil {
-		t.Fatal(err)
+		t.Fatalf("Timeout when expecting certProvider %q to be created", fakeProvider1Name)
 	}


Not related to this PR but.. It would be more straightforward and useful in more scenarios if Receive (and Send) accepted a context, vs. having a hidden internal timeout. Can this be changed?

Also, we have two copies of this testutils.Channel: one in internal and one in internal/xds -- we need to unify them.

Will do in a follow up PR. Definitely makes sense.

credentials/tls/certprovider/store_test.go

credentials/tls/certprovider/distributor_test.go

credentials/tls/certprovider/store_test.go

dfawley · 2020-08-14T22:07:23Z

credentials/tls/certprovider/distributor_test.go

@@ -141,13 +131,15 @@ func (s) TestDistributor(t *testing.T) {
 	waitAndDo(t, proceedCh, errCh, func() {


I don't understand this test very well. Why do you need a separate goroutine vs. doing these operations on the distributor when you would push to proceedCh?

I wanted to test the case where the calls to Set and the calls to KeyMaterial are happening concurrently. But totally agree. This test wasn't very easy to read. Switched it a straight forward flow.

easwars · 2020-08-17T19:41:22Z

@dfawley Still working on a test. Will let you know once this is ready to looked at again. Thanks.

easwars · 2020-08-17T20:05:25Z

Hope I've got the assignment right.
@dfawley : This is ready to be looked at.

credentials/tls/certprovider/distributor_test.go

dfawley · 2020-08-17T20:26:51Z

credentials/tls/certprovider/distributor_test.go

+			return
+
+		}
+		if errors.Is(err, context.DeadlineExceeded) {


if err != nil ?

No, I only want to exit on context deadline errors here. Basically, distributor could have km1 when this function is called. So, the first call to readAndVerifyKeyMaterial will fail. Checking only for context deadline errors here will make sure that we retry till we eventually get what key material we are looking for (or the context expires, at which point, we are done).

For this case, there's not really any reason to do it in parallel then. KeyMaterial will unblock immediately before and after the call to Set the second key material. You could do them sequentially and simplify (i.e. you'd no longer need this function, then).

Makes sense.
Got rid of the function, and simplified this test to only exercise the case where the distributor is blocked waiting for key material, and the Set() happens from another goroutine.

dfawley · 2020-08-20T20:24:55Z

credentials/tls/certprovider/distributor_test.go

+	go waitForKeyMaterial(ctx, dist, km1, nil, errCh)
+	dist.Set(km1, nil)
+	if err := <-errCh; err != nil {
+		t.Fatal(err)
+	}


I think this could be simplified if you reverse what you do:

go dist.Set(km, nil) if km, err := readAndVerifyKeyMaterial()....

With this you could delete errCh and wouldn't need waitForKeyMaterial.

Reversed the order, and got rid of errCh. I still need the waitForKeyMaterial because when I switch the keyMaterial in case #2, I could end up reading the old keyMaterial the first time that I read it.

dfawley · 2020-08-20T20:29:14Z

credentials/tls/certprovider/distributor_test.go

-	waitAndDo(t, proceedCh, errCh, func() {
-		dist.Stop()
-	})
+// TestDistributorConcurrency invokes methods on the distributor in parallel.


You might want to do something to synchronize the timing better.

go fmt.Println("hi") fmt.Println("bye")

Is probably very likely to be ordered one way 99% of the time. Maybe instead:

start := timer.NewTimer(time.Millisecond) go func() { <-start.C fmt.Println("hi") }() <-start.C fmt.Println("bye")

would be more likely to show up races. In this case, if you just want Set to happen while Get is blocked, then:

go func() { time.Sleep(50*time.Microsecond) Set() }() Get()

Done. Thanks.

easwars

Done. PTAL.

easwars · 2020-08-20T22:42:19Z

credentials/tls/certprovider/distributor_test.go

+	go waitForKeyMaterial(ctx, dist, km1, nil, errCh)
+	dist.Set(km1, nil)
+	if err := <-errCh; err != nil {
+		t.Fatal(err)
+	}


Reversed the order, and got rid of errCh. I still need the waitForKeyMaterial because when I switch the keyMaterial in case #2, I could end up reading the old keyMaterial the first time that I read it.

easwars · 2020-08-20T22:42:25Z

credentials/tls/certprovider/distributor_test.go

-	waitAndDo(t, proceedCh, errCh, func() {
-		dist.Stop()
-	})
+// TestDistributorConcurrency invokes methods on the distributor in parallel.


Done. Thanks.

easwars force-pushed the provider_certname branch from bae0fb2 to 9444c3a Compare August 6, 2020 16:48

easwars added no release notes Type: Feature New features or improvements in behavior labels Aug 6, 2020

easwars force-pushed the provider_certname branch from 9444c3a to c64344c Compare August 6, 2020 17:05

easwars requested a review from dfawley August 6, 2020 17:20

easwars assigned dfawley Aug 6, 2020

dfawley reviewed Aug 6, 2020

View reviewed changes

dfawley assigned easwars and unassigned dfawley Aug 6, 2020

easwars added 3 commits August 6, 2020 16:57

certprovider: API update to include certificate name.

5db4170

Revert to using reflect.DeepEqual instead of cmp.Equal in tests.

7a81015

Review comments #1.

a731625

easwars force-pushed the provider_certname branch from 1ea3040 to a731625 Compare August 7, 2020 00:23

easwars commented Aug 7, 2020

View reviewed changes

easwars assigned dfawley and unassigned easwars Aug 7, 2020

menghanl assigned easwars and unassigned dfawley Aug 13, 2020

Pass KeyMaterialOptions to Builder.Build().

a82e4fd

And get rid of KeyMaterialReader type.

easwars assigned dfawley and unassigned easwars Aug 14, 2020

dfawley reviewed Aug 14, 2020

View reviewed changes

dfawley assigned easwars and unassigned dfawley Aug 14, 2020

Review comments #3.

0e704b2

easwars assigned dfawley and unassigned easwars Aug 14, 2020

dfawley reviewed Aug 14, 2020

View reviewed changes

easwars self-assigned this Aug 17, 2020

Make test more readable.

f4ef4f5

easwars assigned dfawley and unassigned easwars and dfawley Aug 17, 2020

dfawley reviewed Aug 17, 2020

View reviewed changes

More test cleanup.

fc80774

dfawley reviewed Aug 20, 2020

View reviewed changes

dfawley assigned easwars and unassigned dfawley Aug 20, 2020

Sych timing better in tests.

bb2dbb0

easwars commented Aug 20, 2020

View reviewed changes

easwars assigned dfawley and unassigned easwars Aug 20, 2020

Simplify the test.

ce951ac

dfawley approved these changes Aug 21, 2020

View reviewed changes

easwars merged commit e14f1c2 into grpc:master Aug 21, 2020

easwars deleted the provider_certname branch August 21, 2020 20:59

easwars mentioned this pull request Aug 21, 2020

advancedtls: add PEMFileProvider implementation for on-file-change credential reloading #3826

Merged

easwars changed the title ~~certprovider: API update to include certificate name.~~ certprovider-api: API update to include certificate name Mar 4, 2021

easwars changed the title ~~certprovider-api: API update to include certificate name~~ certprovider-api: api update to include certificate name Mar 4, 2021

easwars changed the title ~~certprovider-api: api update to include certificate name~~ certprovider-api: update api to include certificate name Mar 4, 2021

github-actions bot locked as resolved and limited conversation to collaborators Sep 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

certprovider-api: update api to include certificate name #3797

certprovider-api: update api to include certificate name #3797

easwars commented Aug 6, 2020

easwars commented Aug 6, 2020

ZhenLian commented Aug 6, 2020

easwars left a comment

dfawley Aug 14, 2020

easwars Aug 14, 2020

dfawley Aug 14, 2020

easwars Aug 15, 2020

easwars commented Aug 17, 2020

easwars commented Aug 17, 2020

dfawley Aug 17, 2020

easwars Aug 17, 2020

dfawley Aug 21, 2020

easwars Aug 21, 2020

dfawley Aug 20, 2020

easwars Aug 20, 2020

dfawley Aug 20, 2020

easwars Aug 20, 2020

easwars left a comment

easwars Aug 20, 2020

easwars Aug 20, 2020

		@@ -141,13 +131,15 @@ func (s) TestDistributor(t *testing.T) {
		waitAndDo(t, proceedCh, errCh, func() {

certprovider-api: update api to include certificate name #3797

certprovider-api: update api to include certificate name #3797

Conversation

easwars commented Aug 6, 2020

easwars commented Aug 6, 2020

ZhenLian commented Aug 6, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

easwars commented Aug 17, 2020

easwars commented Aug 17, 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

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