security/advancedtls: FileWatcher CRL provider initialization enhancement

[erm-g]

Part of CRL Provider effort. The functionality in this PR includes enhancement for FileWatcher provider initialization logic. It also adds minor godoc improvements.

RELEASE NOTES: N/A

[codecov]

Codecov Report

Merging #6760 (6101672) into master (70f1a40) will decrease coverage by 0.04%.
Report is 8 commits behind head on master.
The diff coverage is n/a.

Additional details and impacted files

[dfawley]

It seems like a new test (or test case) should be added to cover making sure the initial scan happens synchronously.

[erm-g]

It seems like a new test (or test case) should be added to cover making sure the initial scan happens synchronously.

It's covered by updated TestFileWatcherCRLProvider
If the initial load is async, then CRL check happens before the CRLDirectory content is loaded and the test fails, I double checked it locally.

[dfawley]

It's covered by updated TestFileWatcherCRLProvider

Oh, I see. The old Close was done to block until the scan happened. Very odd to still access p after calling p.Close, which should normally be considered illegal, but that's not happening anymore which is good.

[gtcooke94]

LGTM

[dfawley]

Why change this? This forces the static CRL provider to implement it, when it doesn't need it.

[erm-g]

I was taking another look at gRFC and realized that Close() is specified at the interface level there - grpc/proposal#382
Then I checked the Go repo and realized that having `Close()' at interface level plus no-op impls (if needed) is an existing pattern -

grpc-go/balancer/balancer.go

Line 388 in 70f1a40

Close()

During internal discussion folks suggested to make that change - WDYT?

[dfawley]

We need to close the LB policy from gRPC; hence it's included in that interface. You don't close the CRL provider from advancedTLS. So the same reasoning doesn't apply in this case. I would not add it here, since it isn't necessary.

[gtcooke94]

This was designed to be very close to the Credential Reloading feature which has Close in the interface

grpc-go/credentials/tls/certprovider/provider.go

Lines 75 to 89 in 6bed353

	// Provider makes it possible to keep channel credential implementations up to
	// date with secrets that they rely on to secure communications on the
	// underlying channel.
	//
	// Provider implementations are free to rely on local or remote sources to fetch
	// the latest secrets, and free to share any state between different
	// instantiations as they deem fit.
	type Provider interface {
	// KeyMaterial returns the key material sourced by the Provider.
	// Callers are expected to use the returned value as read-only.
	KeyMaterial(ctx context.Context) (*KeyMaterial, error)
	// Close cleans up resources allocated by the Provider.
	Close()
	}

[dfawley]

If the cert provider doesn't need it (i.e. gRPC never calls it), then we should consider removing it from there. I'm not sure whether that's the case.
But that's not justification to add it here. It's not needed here, so we probably shouldn't add it.

Feel free to disregard my advice if you really want this.

[erm-g]

I agree with Doug here, for example Close() from provider.go is used in xDS case by gRPC for recreation.
Since we don't have such cases for CRL, I'd prefer to drop it from the interface. @gtcooke94 WDYT?