certprovider-api: update api to include certificate name #3797
Conversation
bae0fb2 to
9444c3a
Compare
9444c3a to
c64344c
Compare
|
@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. |
1ea3040 to
a731625
Compare
And get rid of KeyMaterialReader type.
| p, err := fpb1.providerChan.Receive() | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| t.Fatalf("Timeout when expecting certProvider %q to be created", fakeProvider1Name) | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Will do in a follow up PR. Definitely makes sense.
| @@ -141,13 +131,15 @@ func (s) TestDistributor(t *testing.T) { | |||
| waitAndDo(t, proceedCh, errCh, func() { | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
@dfawley Still working on a test. Will let you know once this is ready to looked at again. Thanks. |
|
Hope I've got the assignment right. |
| return | ||
|
|
||
| } | ||
| if errors.Is(err, context.DeadlineExceeded) { |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
| go waitForKeyMaterial(ctx, dist, km1, nil, errCh) | ||
| dist.Set(km1, nil) | ||
| if err := <-errCh; err != nil { | ||
| t.Fatal(err) | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| waitAndDo(t, proceedCh, errCh, func() { | ||
| dist.Stop() | ||
| }) | ||
| // TestDistributorConcurrency invokes methods on the distributor in parallel. |
There was a problem hiding this comment.
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()| go waitForKeyMaterial(ctx, dist, km1, nil, errCh) | ||
| dist.Set(km1, nil) | ||
| if err := <-errCh; err != nil { | ||
| t.Fatal(err) | ||
| } |
There was a problem hiding this comment.
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.
| waitAndDo(t, proceedCh, errCh, func() { | ||
| dist.Stop() | ||
| }) | ||
| // TestDistributorConcurrency invokes methods on the distributor in parallel. |
No description provided.