File Watcher Authorization Server Interceptor #9775
Conversation
ashithasantosh
force-pushed
the
file_watcher_impl
branch
2 times, most recently
from
defd3b2 to
6f7fef0
Compare
| * @return an object that caller should close when the file refreshes are not needed | ||
| */ | ||
| public Closeable scheduleRefreshes(long period, TimeUnit unit) | ||
| throws IOException { |
There was a problem hiding this comment.
Intent two more spaces, since it is a line continuation. Ditto for create().
(Or don't wrap the line)
| Logger.getLogger(FileWatcherAuthorizationServerInterceptor.class.getName()); | ||
|
|
||
| private volatile AuthorizationServerInterceptor internalAuthzServerInterceptor; | ||
| private final ScheduledExecutorService scheduledExecutorService = |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Sounds good. Updated the code to use FakeClock.
Thank you for the review! I have all the comments. @ejona86 PTAL
| private Thread testCallback; | ||
|
|
||
| private FileWatcherAuthorizationServerInterceptor(File policyFile) | ||
| throws IllegalArgumentException, IOException { |
There was a problem hiding this comment.
IllegalArgumentException is a RuntimeException, so doesn't need to be listed.
authz/src/main/java/io/grpc/authz/FileWatcherAuthorizationServerInterceptor.java
Show resolved
Hide resolved
| try { | ||
| updateInternalInterceptor(); | ||
| } catch (Exception e) { | ||
| logger.log(Level.WARNING, "Authorization Policy file reload failed: " + e); |
There was a problem hiding this comment.
| try (FileOutputStream outputStream = new FileOutputStream(policyFile)) { | ||
| String policy = "{ \"name\": \"abc\",, }"; | ||
| outputStream.write(policy.getBytes(UTF_8)); | ||
| outputStream.close(); |
There was a problem hiding this comment.
The try-with-resources will close the output stream, so this line can be deleted.
| assertThat(ioe).hasMessageThat().isEqualTo( | ||
| "Use JsonReader.setLenient(true) to accept malformed JSON" | ||
| + " at line 1 column 18 path $.name"); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
Delete this catch and let the exception propagate.
| public void invalidPolicyFailsAuthzInterceptorCreation() throws Exception { | ||
| File policyFile = File.createTempFile("temp", "json"); | ||
| policyFile.deleteOnExit(); | ||
| try (FileOutputStream outputStream = new FileOutputStream(policyFile)) { |
There was a problem hiding this comment.
Feel free to use Guava's Files.write(byte[], File) and/or create a helper function to help make the test more terse.
| fail("exception expected"); | ||
| } catch (IOException ioe) { | ||
| assertThat(ioe).hasMessageThat().isEqualTo( | ||
| "Use JsonReader.setLenient(true) to accept malformed JSON" |
There was a problem hiding this comment.
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.
| FileWatcherAuthorizationServerInterceptor interceptor = | ||
| FileWatcherAuthorizationServerInterceptor.create(policyFile); | ||
| assertNotNull(interceptor); | ||
| interceptor.scheduleRefreshes(1, TimeUnit.SECONDS); |
There was a problem hiding this comment.
What is the point of this line? It is leaking the returned Closeable.
ashithasantosh
left a comment
ashithasantosh
left a comment
There was a problem hiding this comment.
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, seeAdvancedTlsX509TrustManager.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…
IllegalArgumentExceptionis aRuntimeException, 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
eas its own argument.
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 bedelete()instead. I'd expect anydeleteOnExit()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, althoughTemporaryFolderis 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
force-pushed
the
file_watcher_impl
branch
from
3bd8770 to
3f4d2c6
Compare
ashithasantosh
added
the
kokoro:force-run
grpc-kokoro
removed
the
kokoro:force-run
|
@ejona86 PTAL |
larry-safran
left a comment
larry-safran
left a comment
There was a problem hiding this comment.
There is one stylistic review comment not addressed, which is about not relying on deleteOnExit for a test to cleanup the temporary file.
ejona86
left a comment
ejona86
left a comment
There was a problem hiding this comment.
We should probably fix the formatting of the one string before merging.
authz/src/main/java/io/grpc/authz/FileWatcherAuthorizationServerInterceptor.java
Outdated
Show resolved
Hide resolved
| .build()); | ||
|
|
||
| private final File policyFile; | ||
| private FileTime lastModifiedTime; |
There was a problem hiding this comment.
FYI, for testing you can do something like:
4 participants
This change is