Implement authentication long polling #9149

ggezer · 2019-05-28T06:51:43Z

AuthenticationServer has a setting for revoking authentication after a period of inactivity. To handle that on the client side, metadata clients, after a period of inactivity, will retry after getting Unauthenticated code. However, due to nature of streaming, data clients can not retry after getting the error because they might have pipelined more data before seeing the error. And since this revocation will not change the connection state, they used to continue getting Unauthenticated. See #9089 for an instance of this problem.

This PR introduces long polling to authentication handshake. Client and server will not complete streams used for authentication and instead will use it for notifying end of an authentication session. With this change, revocation on server will be propagated to client channel via health status, causing a client recreation for later use of the same channel. Also client closing the channel will notify server and it'll clean its state of the recently closed channel.

Periodic cleanup has not been disabled in order to not prolong a duration for a channel to remain authenticated.

AmplabJenkins · 2019-05-28T07:23:17Z

Merged build finished. Test PASSed.

AmplabJenkins · 2019-05-28T07:23:18Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Alluxio-Pull-Request-Builder/3881/
Test PASSed.

gpang

@ggezer Thanks! I left some comments.

gpang · 2019-05-28T16:14:52Z

core/common/src/main/java/alluxio/grpc/GrpcChannel.java

@@ -51,6 +52,11 @@ public GrpcChannel(GrpcManagedChannelPool.ChannelKey channelKey, Channel channel
    mChannelHealthState = channel instanceof AuthenticatedChannel
        ? () -> (((AuthenticatedChannel) channel).isAuthenticated() && mChannelHealthy)
            : () -> mChannelHealthy;
+    if (channel instanceof AuthenticatedChannel) {


we do various things for the instance of AuthenticatedChannel. Can we just have a bool for that?

Also, what other types of channels (not AuthenticatedChannel) are possible? All this checking looks like it can be abstracted away.

Channel is not our class so can't replace AuthenticatedChannel with a boolean. I did some changes for better abstracting the authenticated channel handling.

gpang · 2019-05-28T16:19:07Z

core/common/src/main/java/alluxio/grpc/GrpcServer.java

+   * @return the authentication server associated with this server
+   */
+  @VisibleForTesting
+  public AuthenticationServer getAuthenticationServer() {


Can't the test just pass in the authentication server?

GrpcServer is built by ServerBuilder which dynamically creates the auth server. Removing this method would mean adding a new method to builder with a slightly more complex code.

tests/src/test/java/alluxio/client/fuse/FuseFileSystemIntegrationTest.java

core/common/src/main/java/alluxio/security/authentication/ChannelAuthenticator.java

core/common/src/main/java/alluxio/security/authentication/DefaultAuthenticationServer.java

core/common/src/main/java/alluxio/security/authentication/SaslStreamClientDriver.java

gpang · 2019-05-28T16:32:59Z

core/common/src/main/java/alluxio/security/authentication/SaslStreamClientDriver.java

@@ -41,20 +42,24 @@
  private SaslHandshakeClientHandler mSaslHandshakeClientHandler;
  /** Used to wait until authentication is completed. */
  private SettableFuture<Boolean> mAuthenticated;
+  /** Whether the authentication is active. */


can you add comments on how this is different from mAuthenticated and how they interact? Are both required? Having both is very confusing.

SettableFuture is unfortunately not resettable, so it can't be used to reflect state changes following the initial state. I changed names and comments to try to make it less confusing.

ggezer · 2019-05-28T20:04:27Z

@gpang Updated PR after your feedback. PTAL.

AmplabJenkins · 2019-05-28T20:05:49Z

Merged build finished. Test FAILed.

AmplabJenkins · 2019-05-28T20:05:50Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Alluxio-Pull-Request-Builder/3886/
Test FAILed.

gpang

@ggezer Left some minor comments.

core/common/src/main/java/alluxio/security/authentication/SaslStreamClientDriver.java

AmplabJenkins · 2019-05-28T20:40:00Z

Merged build finished. Test FAILed.

AmplabJenkins · 2019-05-28T20:40:01Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Alluxio-Pull-Request-Builder/3887/
Test FAILed.

AmplabJenkins · 2019-05-28T22:25:39Z

Merged build finished. Test FAILed.

AmplabJenkins · 2019-05-28T22:25:40Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Alluxio-Pull-Request-Builder/3908/
Test FAILed.

gpang · 2019-05-29T15:25:09Z

jenkins, test this please

AmplabJenkins · 2019-05-29T15:53:28Z

Merged build finished. Test FAILed.

AmplabJenkins · 2019-05-29T15:53:29Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Alluxio-Pull-Request-Builder/3950/
Test FAILed.

gpang · 2019-05-29T19:00:27Z

Jenkins, test this please

AmplabJenkins · 2019-05-29T19:24:18Z

Merged build finished. Test FAILed.

AmplabJenkins · 2019-05-29T19:24:19Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Alluxio-Pull-Request-Builder/3960/
Test FAILed.

gpang · 2019-05-29T20:17:37Z

Jenkins, test this please

gpang · 2019-05-29T20:22:13Z

@ggezer I think this looks good, but the tests keep failing

AmplabJenkins · 2019-05-29T20:44:05Z

Merged build finished. Test FAILed.

AmplabJenkins · 2019-05-29T20:44:08Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Alluxio-Pull-Request-Builder/3963/
Test FAILed.

bf8086

Overall LGTM. I left some minor comments inline.

core/common/src/main/java/alluxio/security/authentication/SaslStreamClientDriver.java

core/common/src/test/java/alluxio/security/authentication/GrpcSecurityTest.java

core/common/src/main/java/alluxio/grpc/GrpcChannel.java

core/common/src/main/java/alluxio/security/authentication/DefaultAuthenticationServer.java

AmplabJenkins · 2019-05-29T21:50:15Z

Merged build finished. Test FAILed.

AmplabJenkins · 2019-05-29T21:50:16Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Alluxio-Pull-Request-Builder/3968/
Test FAILed.

core/common/src/main/java/alluxio/security/authentication/DefaultAuthenticationServer.java

AmplabJenkins · 2019-05-30T00:13:40Z

Merged build finished. Test FAILed.

AmplabJenkins · 2019-05-30T00:13:41Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Alluxio-Pull-Request-Builder/3978/
Test FAILed.

AmplabJenkins · 2019-05-30T00:40:06Z

Merged build finished. Test PASSed.

AmplabJenkins · 2019-05-30T00:40:07Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Alluxio-Pull-Request-Builder/3979/
Test PASSed.

bf8086

LGTM. Thanks for the fix!

AmplabJenkins · 2019-05-30T06:31:40Z

Merged build finished. Test FAILed.

AmplabJenkins · 2019-05-30T06:31:42Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Alluxio-Pull-Request-Builder/3994/
Test FAILed.

AmplabJenkins · 2019-05-30T18:58:29Z

Merged build finished. Test PASSed.

AmplabJenkins · 2019-05-30T18:58:31Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Alluxio-Pull-Request-Builder/3999/
Test PASSed.

gpang

LGTM

gpang · 2019-05-30T18:59:40Z

alluxio-bot, merge this please

bf8086 · 2019-06-03T23:04:13Z

@gpang Can you help cherry-pick this PR to 2.0 branch?

calvinjia · 2019-06-03T23:39:56Z

alluxio-bot, cherry-pick this to branch-2.0 please.

alluxio-bot · 2019-06-03T23:40:13Z

Auto cherry-pick successful to branch: branch-2.0

Auto cherry-pick of existing commit. orig-pr: #9149 orig-commit: db93814 orig-commit-author: Göktürk Gezer <gokturk@alluxio.com> pr-link: #9149 change-id: cid-7c847b674046f4836c2d77881d47985a760b8951

alluxio-bot assigned apc999 May 28, 2019

gpang suggested changes May 28, 2019

View reviewed changes

core/common/src/main/java/alluxio/security/authentication/SaslStreamClientDriver.java Outdated Show resolved Hide resolved

core/common/src/main/java/alluxio/security/authentication/SaslStreamClientDriver.java Show resolved Hide resolved

bf8086 mentioned this pull request May 29, 2019

FUSE mount fails after 60mins of inactivity. #9150

Closed

bf8086 suggested changes May 29, 2019

View reviewed changes

bf8086 reviewed May 29, 2019

View reviewed changes

core/common/src/main/java/alluxio/security/authentication/DefaultAuthenticationServer.java Outdated Show resolved Hide resolved

bf8086 reviewed May 29, 2019

View reviewed changes

core/common/src/main/java/alluxio/security/authentication/DefaultAuthenticationServer.java Outdated Show resolved Hide resolved

bf8086 approved these changes May 30, 2019

View reviewed changes

Gokturk Gezer added 9 commits May 30, 2019 11:37

Implement authentication long polling

843cad3

Improve abstraction for authenticated channel

4a07d19

Small fixes

595bb77

Small fixes

6bc2315

Comment fixes

5126e52

Small fixes

cbae490

Update auth revoke timeout

11ed534

Fix tracing statements

a3fb7f4

Break recursive closing of authentication stream

8e6045e

ggezer force-pushed the auth-polling branch from fe08f4e to 8e6045e Compare May 30, 2019 18:37

gpang approved these changes May 30, 2019

View reviewed changes

alluxio-bot merged commit db93814 into Alluxio:master May 30, 2019

Implement authentication long polling #9149

Implement authentication long polling #9149

Conversation

ggezer commented May 28, 2019

AmplabJenkins commented May 28, 2019

AmplabJenkins commented May 28, 2019

gpang left a comment

Choose a reason for hiding this comment

gpang May 28, 2019

Choose a reason for hiding this comment

ggezer May 28, 2019

Choose a reason for hiding this comment

gpang May 28, 2019

Choose a reason for hiding this comment

ggezer May 28, 2019 • edited

Choose a reason for hiding this comment

gpang May 28, 2019

Choose a reason for hiding this comment

ggezer May 28, 2019

Choose a reason for hiding this comment

ggezer commented May 28, 2019

AmplabJenkins commented May 28, 2019

AmplabJenkins commented May 28, 2019

gpang left a comment

Choose a reason for hiding this comment

AmplabJenkins commented May 28, 2019

AmplabJenkins commented May 28, 2019

AmplabJenkins commented May 28, 2019

AmplabJenkins commented May 28, 2019

gpang commented May 29, 2019

AmplabJenkins commented May 29, 2019

AmplabJenkins commented May 29, 2019

gpang commented May 29, 2019

AmplabJenkins commented May 29, 2019

AmplabJenkins commented May 29, 2019

gpang commented May 29, 2019

gpang commented May 29, 2019

AmplabJenkins commented May 29, 2019

AmplabJenkins commented May 29, 2019

bf8086 left a comment

Choose a reason for hiding this comment

AmplabJenkins commented May 29, 2019

AmplabJenkins commented May 29, 2019

AmplabJenkins commented May 30, 2019

AmplabJenkins commented May 30, 2019

AmplabJenkins commented May 30, 2019

AmplabJenkins commented May 30, 2019

bf8086 left a comment

Choose a reason for hiding this comment

AmplabJenkins commented May 30, 2019

AmplabJenkins commented May 30, 2019

AmplabJenkins commented May 30, 2019

AmplabJenkins commented May 30, 2019

gpang left a comment

Choose a reason for hiding this comment

gpang commented May 30, 2019

bf8086 commented Jun 3, 2019

calvinjia commented Jun 3, 2019

alluxio-bot commented Jun 3, 2019

ggezer May 28, 2019 •

edited