You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Dan Burkert (Code Review)" <ge...@cloudera.org> on 2016/11/19 02:55:07 UTC

[kudu-CR] [java-client]: support for Kerberized RPC 

Hello Jean-Daniel Cryans, Todd Lipcon,

I'd like you to do a code review.  Please visit

    http://gerrit.cloudera.org:8080/5150

to review the following change.

Change subject: [java-client]: support for Kerberized RPC
......................................................................

[java-client]: support for Kerberized RPC

This commit adds initial support for connecting to a Kerberized cluster
with the Java client. The application is required to be have an active
login context which contains a subject with Kerberos credentials when
creating the KuduClient to connect to a secured cluster. For example:

```java
// Create a login context ensuring that Kerberos credentials are set, and login.
LoginContext context = new LoginContext(...);
context.login();

// Create a new Kudu client in the privileged context.
KuduClient client = Subject.doAs(context.getSubject(), new PrivilegedAction<KuduClient>() {
    @Override
    public KuduClient run() {
        new KuduClient.KuduClientBuilder(...).build();
    }
});
```

Once the KuduClient is created, the login context is no longer
necessary. This commit does not add a configuration option to ensure the
client rejects connecting to an insecure cluster, that may come in a
follow-up commit.

Testing:

One test is included in TestMiniKuduCluster which explicitly enables
Kerberos authentication and tests that the client can connect. I also
manually switched BaseKuduTest to use a Kerberized cluster, and verified
that tests in TestKuduClient and TestKuduTable which didn't create their
own (uncredentialed) clients passed. I'll leave it to a follow up commit
to figure out how to automate running the full test suite against a
secure cluster.

Change-Id: I5131edb1f2bda443f7980a4aad86362666b3b6f5
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/SecureRpcHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/TabletClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKdc.java
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestMiniKdc.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestMiniKuduCluster.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestUtils.java
M java/kudu-client/src/test/resources/flags
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/rpc/sasl_server.cc
11 files changed, 340 insertions(+), 187 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/50/5150/1
-- 
To view, visit http://gerrit.cloudera.org:8080/5150
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I5131edb1f2bda443f7980a4aad86362666b3b6f5
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] [java-client]: support for Kerberized RPC

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/5150

to look at the new patch set (#2).

Change subject: [java-client]: support for Kerberized RPC
......................................................................

[java-client]: support for Kerberized RPC

This commit adds initial support for connecting to a Kerberized cluster
with the Java client. The application is required to have an active
login context which contains a subject with Kerberos credentials when
creating the KuduClient to connect to a secured cluster. For example:

```java
// Create a login context ensuring that Kerberos credentials are set, and login.
LoginContext context = new LoginContext(...);
context.login();

// Create a new Kudu client in the privileged context.
KuduClient client = Subject.doAs(context.getSubject(), new PrivilegedAction<KuduClient>() {
    @Override
    public KuduClient run() {
        new KuduClient.KuduClientBuilder(...).build();
    }
});
```

Once the KuduClient is created, the login context is no longer
necessary. This commit does not add a configuration option to ensure the
client rejects connecting to an insecure cluster, that may come in a
follow-up commit.

Testing:

One test is included in TestMiniKuduCluster which explicitly enables
Kerberos authentication and tests that the client can connect. I also
manually switched BaseKuduTest to use a Kerberized cluster, and verified
that tests in TestKuduClient and TestKuduTable which didn't create their
own (uncredentialed) clients passed. I'll leave it to a follow up commit
to figure out how to automate running the full test suite against a
secure cluster.

Change-Id: I5131edb1f2bda443f7980a4aad86362666b3b6f5
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/SecureRpcHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/TabletClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKdc.java
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestMiniKdc.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestMiniKuduCluster.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestUtils.java
M java/kudu-client/src/test/resources/flags
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/rpc/sasl_server.cc
12 files changed, 317 insertions(+), 185 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/50/5150/2
-- 
To view, visit http://gerrit.cloudera.org:8080/5150
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5131edb1f2bda443f7980a4aad86362666b3b6f5
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] [java-client]: support for Kerberized RPC

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.

Change subject: [java-client]: support for Kerberized RPC
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5150/2/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
File java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java:

Line 313:       processBuilder.environment().putAll(miniKdc.getEnvVars());
> you think it's worth changing this to not set KRB5CCNAME at all in the subp
I'm inclined to leave it as is, since it matches the C++ external mini cluster behavior.


-- 
To view, visit http://gerrit.cloudera.org:8080/5150
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5131edb1f2bda443f7980a4aad86362666b3b6f5
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] [java-client]: support for Kerberized RPC

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.

Change subject: [java-client]: support for Kerberized RPC
......................................................................


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5150/1/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
File java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java:

Line 495:         LOG.warn(String.format("Could not delete path %s", path), e);
> Missed this question?
Correct, the old call would fail to print the exception.


http://gerrit.cloudera.org:8080/#/c/5150/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestMiniKuduCluster.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestMiniKuduCluster.java:

Line 26:   private static final int DEFAULT_NUM_MASTERS = 3;
> Why?
1 was  the original value.  I had accidentally left it set to 3 when I was testing.


-- 
To view, visit http://gerrit.cloudera.org:8080/5150
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5131edb1f2bda443f7980a4aad86362666b3b6f5
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] [java-client]: support for Kerberized RPC

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: [java-client]: support for Kerberized RPC
......................................................................


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/5150/2/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java:

Line 948:    * Gets the subject who created the Kudu client.
would be worth specifying somewhere (maybe here, maybe in the builder.build() javadoc) at what point the subject-capturing happens. eg is it when the builder is created or when the client is built?


http://gerrit.cloudera.org:8080/#/c/5150/2/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
File java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java:

Line 313:       processBuilder.environment().putAll(miniKdc.getEnvVars());
does this mean that every tserver is sharing the same KRB5CCNAME? doesn't seem quite what we want to avoid cross-process polution


http://gerrit.cloudera.org:8080/#/c/5150/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestMiniKuduCluster.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestMiniKuduCluster.java:

Line 80:         assertTrue(cluster.waitForTabletServers(NUM_TABLET_SERVERS));
does this end up making RPCs underneath?


-- 
To view, visit http://gerrit.cloudera.org:8080/5150
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5131edb1f2bda443f7980a4aad86362666b3b6f5
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] [java-client]: support for Kerberized RPC

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: [java-client]: support for Kerberized RPC
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5150/2/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
File java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java:

Line 313:       processBuilder.environment().putAll(miniKdc.getEnvVars());
> Yes, all servers share the same KRB5CCNAME and keytab.  This is how the C++
ah.. just checked and the server side in security/init.cc sets the KRB5CCNAME variable to 'MEMORY:kudu', so you shouldn't need to be setting KRB5CCNAME at all when starting the daemons


-- 
To view, visit http://gerrit.cloudera.org:8080/5150
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5131edb1f2bda443f7980a4aad86362666b3b6f5
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] [java-client]: support for Kerberized RPC

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.

Change subject: [java-client]: support for Kerberized RPC
......................................................................


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5150/2/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
File java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java:

Line 313:       processBuilder.environment().putAll(miniKdc.getEnvVars());
> does this mean that every tserver is sharing the same KRB5CCNAME? doesn't s
Yes, all servers share the same KRB5CCNAME and keytab.  This is how the C++ external mini cluster works at the moment, from what I can tell (except that this only does one service login, whereas the C++ side inits the same service principal repeatedly).


http://gerrit.cloudera.org:8080/#/c/5150/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestMiniKuduCluster.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestMiniKuduCluster.java:

Line 80:         assertTrue(cluster.waitForTabletServers(NUM_TABLET_SERVERS));
> does this end up making RPCs underneath?
Yes


-- 
To view, visit http://gerrit.cloudera.org:8080/5150
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5131edb1f2bda443f7980a4aad86362666b3b6f5
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] [java-client]: support for Kerberized RPC

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: [java-client]: support for Kerberized RPC
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5150/2/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
File java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java:

Line 313:       processBuilder.environment().putAll(miniKdc.getEnvVars());
> ah.. just checked and the server side in security/init.cc sets the KRB5CCNA
you think it's worth changing this to not set KRB5CCNAME at all in the subprocesses, given they override it? or still good to start with some default?


-- 
To view, visit http://gerrit.cloudera.org:8080/5150
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5131edb1f2bda443f7980a4aad86362666b3b6f5
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] [java-client]: support for Kerberized RPC

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: [java-client]: support for Kerberized RPC
......................................................................


Patch Set 1:

(14 comments)

I only really looked at the test changes, so don't consider this a full review (I'm deferring to the other reviewers for that).

http://gerrit.cloudera.org:8080/#/c/5150/1//COMMIT_MSG
Commit Message:

PS1, Line 10: be
redundant


http://gerrit.cloudera.org:8080/#/c/5150/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java:

Line 950:    * The subject contains credentials necessary to Kerberized Kudu clusters.
Nit: "to connect to Kerberized..." maybe?


http://gerrit.cloudera.org:8080/#/c/5150/1/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKdc.java
File java/kudu-client/src/test/java/org/apache/kudu/client/MiniKdc.java:

PS1, Line 228: createKrb5Conf
Would it be useful to extract this and kdc.conf into template files that are shared by both mini cluster implementations, with substitutions happening at runtime?


Line 333:                       .redirectInput(ProcessBuilder.Redirect.PIPE)
If you've removed redirectOutput() because PIPE is the default value, you can also remove redirectInput().


http://gerrit.cloudera.org:8080/#/c/5150/1/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
File java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java:

Line 107
Technically this is still true...


Line 111
Why remove this precondition?


Line 75:   // Map of ports to process environments. Never removed from. Used to restart processes.
Sorry to pick on you for this (if it helps, I've bugged JD too), but this proliferation of map<pid, ...> is kind of nuts. Can we model a process and all this extra metadata with a class and use a single map?

The new class could have all public fields for all I care; I just want it encapsulated in one place instead of strewn out across a bunch of maps.


Line 220:         String spn = "kudu/" + bindHost;
Nit: can you inline this two lines below?


Line 299:         String spn = "kudu/" + bindHost;
Nit: inline


Line 320:    * Starts a process using the provided command and configures it to be daemon,
Update.


PS1, Line 336:     LOG.info("Starting process: {}, environment: {}",
             :              Joiner.on(" ").join(processBuilder.command()),
             :              Joiner.on(",").withKeyValueSeparator("=").join(processBuilder.environment()));
This is also in MiniKdc.java. Perhaps it can be decomposed into a helper method?


Line 495:         LOG.warn(String.format("Could not delete path %s", path), e);
Did the old call not work properly?


Line 547:   public Subject getLoggedInSubject() {
Nit: Javadoc.


http://gerrit.cloudera.org:8080/#/c/5150/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestMiniKdc.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestMiniKdc.java:

Line 43:       assertFalse(klist.contains("alice@KRBTEST.COM"));
Why did this change?


-- 
To view, visit http://gerrit.cloudera.org:8080/5150
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5131edb1f2bda443f7980a4aad86362666b3b6f5
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] [java-client]: support for Kerberized RPC

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: [java-client]: support for Kerberized RPC
......................................................................


Patch Set 2: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/5150
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5131edb1f2bda443f7980a4aad86362666b3b6f5
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] [java-client]: support for Kerberized RPC

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has submitted this change and it was merged.

Change subject: [java-client]: support for Kerberized RPC
......................................................................


[java-client]: support for Kerberized RPC

This commit adds initial support for connecting to a Kerberized cluster
with the Java client. The application is required to have an active
login context which contains a subject with Kerberos credentials when
creating the KuduClient to connect to a secured cluster. For example:

```java
// Create a login context ensuring that Kerberos credentials are set, and login.
LoginContext context = new LoginContext(...);
context.login();

// Create a new Kudu client in the privileged context.
KuduClient client = Subject.doAs(context.getSubject(), new PrivilegedAction<KuduClient>() {
    @Override
    public KuduClient run() {
        new KuduClient.KuduClientBuilder(...).build();
    }
});
```

Once the KuduClient is created, the login context is no longer
necessary. This commit does not add a configuration option to ensure the
client rejects connecting to an insecure cluster, that may come in a
follow-up commit.

Testing:

One test is included in TestMiniKuduCluster which explicitly enables
Kerberos authentication and tests that the client can connect. I also
manually switched BaseKuduTest to use a Kerberized cluster, and verified
that tests in TestKuduClient and TestKuduTable which didn't create their
own (uncredentialed) clients passed. I'll leave it to a follow up commit
to figure out how to automate running the full test suite against a
secure cluster.

Change-Id: I5131edb1f2bda443f7980a4aad86362666b3b6f5
Reviewed-on: http://gerrit.cloudera.org:8080/5150
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Reviewed-by: Todd Lipcon <to...@apache.org>
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/SecureRpcHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/TabletClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKdc.java
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestMiniKdc.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestMiniKuduCluster.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestUtils.java
M java/kudu-client/src/test/resources/flags
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/rpc/sasl_server.cc
12 files changed, 317 insertions(+), 185 deletions(-)

Approvals:
  Adar Dembo: Looks good to me, but someone else must approve
  Todd Lipcon: Looks good to me, approved
  Kudu Jenkins: Verified



-- 
To view, visit http://gerrit.cloudera.org:8080/5150
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I5131edb1f2bda443f7980a4aad86362666b3b6f5
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] [java-client]: support for Kerberized RPC

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.

Change subject: [java-client]: support for Kerberized RPC
......................................................................


Patch Set 2:

(12 comments)

http://gerrit.cloudera.org:8080/#/c/5150/1//COMMIT_MSG
Commit Message:

PS1, Line 10: ha
> redundant
Done


http://gerrit.cloudera.org:8080/#/c/5150/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java:

Line 950:    * The subject contains credentials necessary to authenticate to Kerberized Kudu clusters.
> Nit: "to connect to Kerberized..." maybe?
Done


http://gerrit.cloudera.org:8080/#/c/5150/1/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKdc.java
File java/kudu-client/src/test/java/org/apache/kudu/client/MiniKdc.java:

PS1, Line 228: createKrb5Conf
> Would it be useful to extract this and kdc.conf into template files that ar
They have already diverged somewhat - see the additional restrictions that are being added on cipher types here due to Java restrictions.  Unless you feel strongly I'm going to skip this, I don't think it would simplify things overall.


Line 333:   }
> If you've removed redirectOutput() because PIPE is the default value, you c
Done


http://gerrit.cloudera.org:8080/#/c/5150/1/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
File java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java:

Line 107
> Technically this is still true...
It wasn't adding any information.


Line 111
> Why remove this precondition?
I briefly had a test that set the number of tservers to 0 for debugging (less log spew).  I kept the removal since I don't see the point in restricting this.  I use 0 tserver tests pretty heavily when testing the RPC layer for the rust client, for instance.


Line 75:   private final List<String> pathsToDelete = new ArrayList<>();
> Sorry to pick on you for this (if it helps, I've bugged JD too), but this p
I actually made this a lot worse than it needed to be, I had replaced 'environments' (map of maps) with just a single environment, but forgot to finish removing all the uses.  Now it's just a single additional map shared by the whole cluster.


Line 220:     }
> Nit: can you inline this two lines below?
Done


Line 299:    * Starts a process using the provided command and configures it to be daemon,
> Nit: inline
Done


Line 320:     thread.setName(Iterables.getLast(Splitter.on(File.separatorChar).split(command.get(0))) + ":" + port);
> Update.
Done


PS1, Line 336:    * Starts a previously killed master process on the specified port.
             :    * @param port which port the master was listening on for RPCs
             :    * @throws Exception
> This is also in MiniKdc.java. Perhaps it can be decomposed into a helper me
I ended up removing this.


http://gerrit.cloudera.org:8080/#/c/5150/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestMiniKdc.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestMiniKdc.java:

Line 43:       assertFalse(klist.contains("alice@KRBTEST.COM"));
> Why did this change?
We no longer allow multiple logged in client principals due to moving from DIR to FILE typed ccache.  A similar change was made on the C++ side in 123b9918c7005f65010c5d431f4e3cac459e7a31


-- 
To view, visit http://gerrit.cloudera.org:8080/5150
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5131edb1f2bda443f7980a4aad86362666b3b6f5
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] [java-client]: support for Kerberized RPC

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: [java-client]: support for Kerberized RPC
......................................................................


Patch Set 2: Code-Review+1

-- 
To view, visit http://gerrit.cloudera.org:8080/5150
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5131edb1f2bda443f7980a4aad86362666b3b6f5
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] [java-client]: support for Kerberized RPC

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: [java-client]: support for Kerberized RPC
......................................................................


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5150/1/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
File java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java:

Line 495:   }
> Did the old call not work properly?
Missed this question?


http://gerrit.cloudera.org:8080/#/c/5150/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestMiniKuduCluster.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestMiniKuduCluster.java:

Line 26:   private static final int DEFAULT_NUM_MASTERS = 1;
Why?


-- 
To view, visit http://gerrit.cloudera.org:8080/5150
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5131edb1f2bda443f7980a4aad86362666b3b6f5
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes