[
https://issues.apache.org/jira/browse/HADOOP-11567?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Benoy Antony updated HADOOP-11567:
----------------------------------
Attachment: HADOOP-11567-003.patch
Thanks for the detailed review and comments, [~arpitagarwal].
New patch which takes care of some of comments is attached.
Please see my answers to the comments.
# bq. FileSignerSecretProvider.java:140: Close InputStream?
Using try with resources to close the resources before exiting the method -
https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html
# bq. FileSignerSecretProvider.java:162: Close InputStream here also?
same as above
# bq. FileSignerSecretProvider.java:175: updateSecretsIfRequired should be
synchronized right?
Please note that fields involved are volatile. So the changes are visible
across threads and “happens before” guarantee of volatile variable makes sure
that changes are visible in the same order. (see the discussion here :
http://stackoverflow.com/questions/30325841/can-operations-which-assign-values-to-multiple-volatile-fields-consecutively-b
). So we do not need synchronization for achieving visibility of the state.
There is no state corruption even if multiple threads execute the thread at the
same time. This means that we do need synchronization to guarantee exclusive
access. So in summary , we do not need synchronization here.
# bq. FileSignerSecretProvider.java:176: Prefer using the monotonic
System.nanoTime().
Since the usecase does not really care even if the rotation is off by a few
milliseconds, is it neccessary to use System.nanoTime() ? Note that
System.nanoTime() could be expensive depending on the architecture .
http://stackoverflow.com/questions/19052316/why-is-system-nanotime-way-slower-in-performance-than-system-currenttimemill
.
# bq. FileSignerSecretProvider.java:196: Potential perf regression: we should
throttle calls to pollForSecretChange. Every call to getCurrentSecret could
cause a disk operation now. Previously it was just a read from memory.
_pollForSecretChange_ will read the file only if the secret file is changed. I
believe , the file change will be a very rare event. Once events are read, the
events are cleared as per
http://docs.oracle.com/javase/7/docs/api/java/nio/file/WatchKey.html#pollEvents()
. So only a single call after a file change will result in a disk read
operation. This was the main attraction for using this approach versus a thread
which monitors for file change at regular intervals. Please let me know if I am
interpreting this wrong. Also do let me know if I still need to any throttling
measures for additional safety.
# bq. FileSignerSecretProvider.java:203: Same perf regression here
same as above.
# bq. FileSignerSecretProvider.java:63: From the code below it looks like
effectivetimeinmillis must be specified as milliseconds since the epoch. That
should be documented. Also do you think we can eliminate both config settings
to reduce configuration and errors. Instead use a default system-wide
transition period. Does that work for your use case?
I have added the documentation. We really need an ability to update the secrets
in a staged manner since have a large number of nodes and it take some time to
update the file on all nodes. _effectivetimeinmillis_ helps here. We have a
large number of active users. So we need to phase out the secrets properly.
_transitionperiodinmillis_ helps here. Since this is mostly cluster and
situtaion dependent, I cannot think of constant value for these two attributes.
I have also updated this feature in the http authentication.
# bq. FileSignerSecretProvider.java:89: Can we use List\<byte[]\> instead of
byte[][]?
_SignerProvider.getSecrets_ has a return value byte[][]. if we use
_List<byte>_ , then we require conversions.
# bq. FileSignerSecretProvider.java:97: Previously we didn't throw on null.
Throwing is correct but may not be backward compatible.
removed non-null assertions.
*Minor*
# bq. FileSignerSecretProvider.java:104: Space after comma.
fixed
# bq. FileSignerSecretProvider.java:10: Unnecessary change?
fixed
# bq. FileSignerSecretProvider.java:123: Missing code to close the reader?
Using try with resources to close the resources before exiting the method -
https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html
# bq. FileSignerSecretProvider.java:16: Unnecessary change?
fixed
# bq. FileSignerSecretProvider.java:175: Nitpick: Extra space before (
fixed
# bq. FileSignerSecretProvider.java:207: Nitpick: extra newline.
fixed
> Refresh HTTP Authentication secret without restarting the server
> ----------------------------------------------------------------
>
> Key: HADOOP-11567
> URL: https://issues.apache.org/jira/browse/HADOOP-11567
> Project: Hadoop Common
> Issue Type: Improvement
> Affects Versions: 2.6.0
> Reporter: Benoy Antony
> Assignee: Benoy Antony
> Labels: BB2015-05-TBR
> Attachments: HADOOP-11567-001.patch, HADOOP-11567-002.patch,
> HADOOP-11567-003.patch
>
>
> The _AuthenticationFilter_ uses the secret read from a file specified via
> hadoop.http.authentication.signature.secret.file to sign the cookie
> containing user authentication information.
> The secret is read only during initialization and hence needs a restart to
> update the secret.
> ZKSignerSecretProvider can be used to rotate the secrets without restarting
> the servers, but it needs a zookeeper setup.
> The jira is to refresh secret by updating the file.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
