Skip to content

File Watcher Authorization Server Interceptor #9775

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Apr 19, 2023
Merged

File Watcher Authorization Server Interceptor #9775

merged 3 commits into from
Apr 19, 2023
+669 −54

Conversation

ashithasantosh
Copy link
Contributor

@ashithasantosh ashithasantosh commented Dec 27, 2022 •
edited
Loading

This change is Reviewable

@ashithasantosh ashithasantosh force-pushed the file_watcher_impl branch 2 times, most recently from defd3b2 to 6f7fef0 Compare December 28, 2022 00:36
ejona86
ejona86 reviewed Jan 3, 2023
View reviewed changes
authz/src/main/java/io/grpc/authz/FileWatcherAuthorizationServerInterceptor.java Outdated
* @return an object that caller should close when the file refreshes are not needed
*/
public Closeable scheduleRefreshes(long period, TimeUnit unit)
throws IOException {
Copy link
Member

Choose a reason for hiding this comment

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

Intent two more spaces, since it is a line continuation. Ditto for create().

(Or don't wrap the line)

authz/src/main/java/io/grpc/authz/FileWatcherAuthorizationServerInterceptor.java Outdated
Logger.getLogger(FileWatcherAuthorizationServerInterceptor.class.getName());

private volatile AuthorizationServerInterceptor internalAuthzServerInterceptor;
private final ScheduledExecutorService scheduledExecutorService =
Copy link
Member

Choose a reason for hiding this comment

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

Have the user pass this in to scheduleRefreshes(). For reference, see AdvancedTlsX509TrustManager.updateTrustCredentialsFromFile().

Then, in the tests pass FakeClock. That will then allow you to modify the policy file, forwardTime() (commonly we do this in two phases, forward time too little, check the behavior is still the old behavior, then forward time the small remaining amount), and then check the interceptor behavior changed. You wouldn't need testCallback.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. Updated the code to use FakeClock.

Thank you for the review! I have all the comments. @ejona86 PTAL

authz/src/main/java/io/grpc/authz/FileWatcherAuthorizationServerInterceptor.java Outdated
private Thread testCallback;

private FileWatcherAuthorizationServerInterceptor(File policyFile)
throws IllegalArgumentException, IOException {
Copy link
Member

Choose a reason for hiding this comment

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

IllegalArgumentException is a RuntimeException, so doesn't need to be listed.

authz/src/main/java/io/grpc/authz/FileWatcherAuthorizationServerInterceptor.java Outdated
try {
updateInternalInterceptor();
} catch (Exception e) {
logger.log(Level.WARNING, "Authorization Policy file reload failed: " + e);
Copy link
Member

Choose a reason for hiding this comment

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

Pass e as its own argument.

https://docs.oracle.com/javase/8/docs/api/java/util/logging/Logger.html#log-java.util.logging.Level-java.lang.String-java.lang.Throwable-

authz/src/test/java/io/grpc/authz/FileWatcherAuthorizationServerInterceptorTest.java Outdated
try (FileOutputStream outputStream = new FileOutputStream(policyFile)) {
String policy = "{ \"name\": \"abc\",, }";
outputStream.write(policy.getBytes(UTF_8));
outputStream.close();
Copy link
Member

Choose a reason for hiding this comment

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

The try-with-resources will close the output stream, so this line can be deleted.

authz/src/test/java/io/grpc/authz/FileWatcherAuthorizationServerInterceptorTest.java Outdated
assertThat(ioe).hasMessageThat().isEqualTo(
"Use JsonReader.setLenient(true) to accept malformed JSON"
+ " at line 1 column 18 path $.name");
} catch (Exception e) {
Copy link
Member

Choose a reason for hiding this comment

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

Delete this catch and let the exception propagate.

authz/src/test/java/io/grpc/authz/FileWatcherAuthorizationServerInterceptorTest.java Outdated
public void invalidPolicyFailsAuthzInterceptorCreation() throws Exception {
File policyFile = File.createTempFile("temp", "json");
policyFile.deleteOnExit();
try (FileOutputStream outputStream = new FileOutputStream(policyFile)) {
Copy link
Member

Choose a reason for hiding this comment

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

Feel free to use Guava's Files.write(byte[], File) and/or create a helper function to help make the test more terse.

authz/src/test/java/io/grpc/authz/FileWatcherAuthorizationServerInterceptorTest.java Outdated
fail("exception expected");
} catch (IOException ioe) {
assertThat(ioe).hasMessageThat().isEqualTo(
"Use JsonReader.setLenient(true) to accept malformed JSON"
Copy link
Member

Choose a reason for hiding this comment

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

Do not assert messages that you don't generate. You could maybe check for "malformed," but this full message is far too precise and would trivially break on an upgrade to GSON.

authz/src/test/java/io/grpc/authz/FileWatcherAuthorizationServerInterceptorTest.java Outdated
FileWatcherAuthorizationServerInterceptor interceptor =
FileWatcherAuthorizationServerInterceptor.create(policyFile);
assertNotNull(interceptor);
interceptor.scheduleRefreshes(1, TimeUnit.SECONDS);
Copy link
Member

Choose a reason for hiding this comment

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

What is the point of this line? It is leaking the returned Closeable.

ashithasantosh
ashithasantosh commented Mar 3, 2023
View reviewed changes
Copy link
Contributor Author

@ashithasantosh ashithasantosh left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 5 files reviewed, 13 unresolved discussions (waiting on @ejona86)

a discussion (no related file):
Thank you for the review:) Please review the updated code.

authz/src/main/java/io/grpc/authz/FileWatcherAuthorizationServerInterceptor.java line 50 at r1 (raw file):

Previously, ejona86 (Eric Anderson) wrote…

Have the user pass this in to scheduleRefreshes(). For reference, see AdvancedTlsX509TrustManager.updateTrustCredentialsFromFile().

Then, in the tests pass FakeClock. That will then allow you to modify the policy file, forwardTime() (commonly we do this in two phases, forward time too little, check the behavior is still the old behavior, then forward time the small remaining amount), and then check the interceptor behavior changed. You wouldn't need testCallback.

Done.

authz/src/main/java/io/grpc/authz/FileWatcherAuthorizationServerInterceptor.java line 63 at r1 (raw file):

Previously, ejona86 (Eric Anderson) wrote…

IllegalArgumentException is a RuntimeException, so doesn't need to be listed.

Done.

authz/src/main/java/io/grpc/authz/FileWatcherAuthorizationServerInterceptor.java line 99 at r1 (raw file):

Previously, ejona86 (Eric Anderson) wrote…

Intent two more spaces, since it is a line continuation. Ditto for create().

(Or don't wrap the line)

Done.

authz/src/main/java/io/grpc/authz/FileWatcherAuthorizationServerInterceptor.java line 109 at r1 (raw file):

Previously, ejona86 (Eric Anderson) wrote…

Pass e as its own argument.

https://docs.oracle.com/javase/8/docs/api/java/util/logging/Logger.html#log-java.util.logging.Level-java.lang.String-java.lang.Throwable-

Done.

authz/src/main/java/io/grpc/authz/FileWatcherAuthorizationServerInterceptor.java line 111 at r1 (raw file):

Previously, ejona86 (Eric Anderson) wrote…

Unindent.

Done.

authz/src/test/java/io/grpc/authz/AuthorizationEnd2EndTest.java line 447 at r1 (raw file):

Previously, ejona86 (Eric Anderson) wrote…

This is a strange use of deleteOnExit(), because it seems it could just be delete() instead. I'd expect any deleteOnExit() to be just after the file is created, so that if an exception is thrown later in the test the file would still get deleted.

Moved the logic to delete policy file to tearDown()

authz/src/test/java/io/grpc/authz/FileWatcherAuthorizationServerInterceptorTest.java line 36 at r1 (raw file):

Previously, ejona86 (Eric Anderson) wrote…

Sometimes it is convenient to use createTempFile()+deleteOnExit() in tests, although TemporaryFolder is generally preferred because it will be cleaned up at the end of the test instead of if/when the JVM exits cleanly.

Because this is an easy case, you could also do:

private File policyFile = File.createTempFile("temp", "json");

@After public void cleanUp() {
  policyFile.delete(); // does not throw if file not found
}

Done.

authz/src/test/java/io/grpc/authz/FileWatcherAuthorizationServerInterceptorTest.java line 38 at r1 (raw file):

Previously, ejona86 (Eric Anderson) wrote…

Feel free to use Guava's Files.write(byte[], File) and/or create a helper function to help make the test more terse.

Thank you for the suggestion. Updated the tests to use Guava API.

authz/src/test/java/io/grpc/authz/FileWatcherAuthorizationServerInterceptorTest.java line 41 at r1 (raw file):

Previously, ejona86 (Eric Anderson) wrote…

The try-with-resources will close the output stream, so this line can be deleted.

Done.

authz/src/test/java/io/grpc/authz/FileWatcherAuthorizationServerInterceptorTest.java line 48 at r1 (raw file):

Previously, ejona86 (Eric Anderson) wrote…

Do not assert messages that you don't generate. You could maybe check for "malformed," but this full message is far too precise and would trivially break on an upgrade to GSON.

Done.

authz/src/test/java/io/grpc/authz/FileWatcherAuthorizationServerInterceptorTest.java line 50 at r1 (raw file):

Previously, ejona86 (Eric Anderson) wrote…

Delete this catch and let the exception propagate.

Done.

authz/src/test/java/io/grpc/authz/FileWatcherAuthorizationServerInterceptorTest.java line 84 at r1 (raw file):

Previously, ejona86 (Eric Anderson) wrote…

What is the point of this line? It is leaking the returned Closeable.

Deleted the line. I agree it does not make sense here.

@ashithasantosh ashithasantosh added the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Mar 6, 2023
@grpc-kokoro grpc-kokoro removed the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Mar 6, 2023
@larry-safran larry-safran requested a review from ejona86 March 9, 2023 00:12
@ashithasantosh
Copy link
Contributor Author

@ejona86 PTAL

larry-safran
larry-safran approved these changes Apr 17, 2023
View reviewed changes
Copy link
Contributor

@larry-safran larry-safran left a comment

Choose a reason for hiding this comment

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

There is one stylistic review comment not addressed, which is about not relying on deleteOnExit for a test to cleanup the temporary file.

ejona86
ejona86 approved these changes Apr 17, 2023
View reviewed changes
Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

We should probably fix the formatting of the one string before merging.

authz/src/main/java/io/grpc/authz/FileWatcherAuthorizationServerInterceptor.java Outdated

private final File policyFile;
private FileTime lastModifiedTime;
Copy link
Member

Choose a reason for hiding this comment

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

FYI, for testing you can do something like:

grpc-java/xds/src/test/java/io/grpc/xds/internal/security/certprovider/FileWatcherCertificateProviderTest.java

Lines 117 to 118 in 1551cc7

Files.setLastModifiedTime(
Paths.get(certFile), FileTime.fromMillis(timeProvider.currentTimeMillis()));

@ejona86 ejona86 merged commit b43ddc2 into grpc:master Apr 19, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ejona86 ejona86

@larry-safran larry-safran

Assignees

@ejona86 ejona86

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ashithasantosh @ejona86 @larry-safran @grpc-kokoro