Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from
Sep 2, 2020

Conversation

ZhenLian
Copy link
Contributor

@ZhenLian ZhenLian commented Aug 18, 2020

This is fork of work #3785.
This PR added implementation and unit tests of PemFileProvider. It will fetch identity certificate updates and root certificate updates from the identity PEM file and the root PEM file specified by users via PemFileProviderOptions periodically.

@ZhenLian ZhenLian force-pushed the zhen_reload_provider branch from b9ee1b9 to d7d2584 Compare August 18, 2020 06:00
@ZhenLian ZhenLian changed the title Zhen reload provider advancedtls: add PEMFileProvider implementation Aug 18, 2020
@ZhenLian ZhenLian force-pushed the zhen_reload_provider branch from d7d2584 to 9dff771 Compare August 18, 2020 17:52
@ZhenLian ZhenLian changed the title advancedtls: add PEMFileProvider implementation advancedtls: add PEMFileProvider implementation for on-file-change credential reloading Aug 19, 2020
@ZhenLian ZhenLian requested a review from easwars August 19, 2020 19:06
@ZhenLian
Copy link
Contributor Author

I've modified the tests as we discussed. @easwars Can you please take a look when you get a chance please? Thank you so much!

@easwars
Copy link
Contributor

easwars commented Aug 21, 2020

The certificate provider API update just landed: #3797
Could you please make sure that your changes here are up-to-date and reassign this PR back to me.
Thanks.

@easwars easwars assigned ZhenLian and unassigned easwars Aug 21, 2020
@ZhenLian
Copy link
Contributor Author

@easwars Yes, I think the changes are up-to-date with your newest implementations.
Basically, we are only using the Distributor and Provider. I think most of the changes from your PR are around Builder/store, so I think it's fine. Of course, there might be some minor changes once we upgrade the version, but we probably can do that after the new implementation is released.

security/advancedtls/pemfile_provider.go Outdated Show resolved Hide resolved
security/advancedtls/pemfile_provider.go Show resolved Hide resolved
security/advancedtls/pemfile_provider.go Outdated Show resolved Hide resolved
security/advancedtls/pemfile_provider.go Outdated Show resolved Hide resolved
security/advancedtls/pemfile_provider.go Outdated Show resolved Hide resolved
security/advancedtls/pemfile_provider_test.go Show resolved Hide resolved
security/advancedtls/pemfile_provider_test.go Show resolved Hide resolved
security/advancedtls/pemfile_provider_test.go Outdated Show resolved Hide resolved
security/advancedtls/pemfile_provider_test.go Show resolved Hide resolved
security/advancedtls/pemfile_provider_test.go Show resolved Hide resolved
@ZhenLian ZhenLian requested a review from easwars August 31, 2020 17:05
security/advancedtls/pemfile_provider_test.go Outdated Show resolved Hide resolved
security/advancedtls/pemfile_provider_test.go Show resolved Hide resolved
t.Run(test.desc, func(t *testing.T) {
stage := &stageInfo{}
oldReadKeyCertPairFunc := readKeyCertPairFunc
readKeyCertPairFunc = func(certFile, keyFile string) (tls.Certificate, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option here is to use https://golang.org/pkg/io/ioutil/#TempFile, and populate them with actual contents, so that you dont have to override the readKeyCertPairFunc and readTrustCertFunc. That way you will also be exercising the functionality in readTrustCertFunc. This need not be done as part of this PR. But if you feel this is a good idea, then you can add a TODO and do it in a later PR.
With this approach, you would not need any of this stage business. You would write actual contents into the temp files from your test, and pass the tempFile path to the code which will read directly from there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, actually we originally used this TempFile in #3785, but later changed to this implementation because we think making changes to local file system(even though it's in tmp folder) might be too much for unit tests.
I might use this when writing the integration tests, or when creating examples for the whole dynamic loading feature. Anyway thanks for the suggestions!

@ZhenLian ZhenLian force-pushed the zhen_reload_provider branch from 6da7a7c to 9998493 Compare September 1, 2020 06:25
@ZhenLian
Copy link
Contributor Author

ZhenLian commented Sep 2, 2020

@easwars Thank you again for the review!

@ZhenLian ZhenLian merged commit 9a132e4 into grpc:master Sep 2, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants