[kudu-CR] [c++ client] re-acquire authn token if expired

[Alexey Serbin (Code Review)]

Alexey Serbin has submitted this change and it was merged.

Change subject: [c++ client] re-acquire authn token if expired
..


[c++ client] re-acquire authn token if expired

Updated C++ client to re-acquire authn token if server responds with
FATAL_INVALID_AUTHENTICATION_TOKEN error on connection negotiation.
If that happens while negotiating a connection for a call, the
connection is closed and the client reconnects to the cluster to get
a new authn token.  After successfully re-acquiring authn token the
client retries the RPC call again.

Added corresponding integration test to cover authn token re-acquisition
for various scenarios.

Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
Reviewed-on: http://gerrit.cloudera.org:8080/6648
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert 
Reviewed-by: Todd Lipcon 
---
M docs/known_issues.adoc
M docs/security.adoc
M src/kudu/client/batcher.cc
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/master_rpc.cc
M src/kudu/client/master_rpc.h
M src/kudu/client/meta_cache.cc
M src/kudu/client/scanner-internal.cc
M src/kudu/client/scanner-internal.h
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/authn_token_expire-itest.cc
M src/kudu/integration-tests/cluster_verifier.h
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/retriable_rpc.h
M src/kudu/rpc/rpc.h
M src/kudu/rpc/server_negotiation.cc
24 files changed, 752 insertions(+), 140 deletions(-)

Approvals:
  Dan Burkert: 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/6648
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
Gerrit-PatchSet: 19
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [c++ client] re-acquire authn token if expired

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: [c++ client] re-acquire authn token if expired
..


Patch Set 18: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
Gerrit-PatchSet: 18
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [c++ client] re-acquire authn token if expired

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [c++ client] re-acquire authn token if expired
..


Patch Set 16:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/6648/16/src/kudu/client/client-internal.cc
File src/kudu/client/client-internal.cc:

Line 753:   if (leader_master_rpc_primary_creds_ || need_new_rpc_primary_creds) 
{
> I think the version you came up with which is something of a hybrid is the 
I'm glad my micro-updates did not bring confusion this time :)

Thank you for pointing to this piece and providing a clearer alternative.


Line 761: scoped_refptr& rpc(
> Yah I was confused, I thought this was actually calling scoped_refptr const
I agree that this part was not good for a reader.  At least, two readers in a 
row got confused :)  A clear sign that something needs to be changed.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
Gerrit-PatchSet: 16
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [c++ client] re-acquire authn token if expired

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: [c++ client] re-acquire authn token if expired
..


Patch Set 18: Code-Review+1

(2 comments)

http://gerrit.cloudera.org:8080/#/c/6648/16/src/kudu/client/client-internal.cc
File src/kudu/client/client-internal.cc:

Line 753:   } else {
> I asked Mike to look at this as well to have an additional independent revi
I think the version you came up with which is something of a hybrid is the 
best, thanks!


Line 761: std::placeholders::_3,
> What would be that 'unnamed stack-allocated variable'?
Yah I was confused, I thought this was actually calling scoped_refptr 
constructor.  I did a little test, and it doesn't seem to work that way.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
Gerrit-PatchSet: 18
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [c++ client] re-acquire authn token if expired

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [c++ client] re-acquire authn token if expired
..


Patch Set 16:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6648/16/src/kudu/client/client-internal.cc
File src/kudu/client/client-internal.cc:

Line 753:   if (leader_master_rpc_primary_creds_ || need_new_rpc_primary_creds) 
{
> I was having a hard time following this, so I tried to take a crack at redu
I asked Mike to look at this as well to have an additional independent reviewer 
:)

To Mike the reference thing was also confusing.  Also, both variants looked not 
so good to him, so I decided to go with the alternative that you proposed: if 
you trust that code better, let it be the trusted code.  I also found it doing 
the right thing, but I also spent some time to make sure it behaves like I want 
it to behave.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
Gerrit-PatchSet: 16
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [c++ client] re-acquire authn token if expired

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: [c++ client] re-acquire authn token if expired
..

[c++ client] re-acquire authn token if expired

Updated C++ client to re-acquire authn token if server responds with
FATAL_INVALID_AUTHENTICATION_TOKEN error on connection negotiation.
If that happens while negotiating a connection for a call, the
connection is closed and the client reconnects to the cluster to get
a new authn token.  After successfully re-acquiring authn token the
client retries the RPC call again.

Added corresponding integration test to cover authn token re-acquisition
for various scenarios.

Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
---
M docs/known_issues.adoc
M docs/security.adoc
M src/kudu/client/batcher.cc
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/master_rpc.cc
M src/kudu/client/master_rpc.h
M src/kudu/client/meta_cache.cc
M src/kudu/client/scanner-internal.cc
M src/kudu/client/scanner-internal.h
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/authn_token_expire-itest.cc
M src/kudu/integration-tests/cluster_verifier.h
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/retriable_rpc.h
M src/kudu/rpc/rpc.h
M src/kudu/rpc/server_negotiation.cc
24 files changed, 752 insertions(+), 140 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/6648/18
-- 
To view, visit http://gerrit.cloudera.org:8080/6648
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
Gerrit-PatchSet: 18
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [c++ client] re-acquire authn token if expired

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: [c++ client] re-acquire authn token if expired
..

[c++ client] re-acquire authn token if expired

Updated C++ client to re-acquire authn token if server responds with
FATAL_INVALID_AUTHENTICATION_TOKEN error on connection negotiation.
If that happens while negotiating a connection for a call, the
connection is closed and the client reconnects to the cluster to get
a new authn token.  After successfully re-acquiring authn token the
client retries the RPC call again.

Added corresponding integration test to cover authn token re-acquisition
for various scenarios.

Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
---
M docs/known_issues.adoc
M docs/security.adoc
M src/kudu/client/batcher.cc
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/master_rpc.cc
M src/kudu/client/master_rpc.h
M src/kudu/client/meta_cache.cc
M src/kudu/client/scanner-internal.cc
M src/kudu/client/scanner-internal.h
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/authn_token_expire-itest.cc
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/retriable_rpc.h
M src/kudu/rpc/rpc.h
M src/kudu/rpc/server_negotiation.cc
23 files changed, 754 insertions(+), 142 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/6648/17
-- 
To view, visit http://gerrit.cloudera.org:8080/6648
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
Gerrit-PatchSet: 17
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [c++ client] re-acquire authn token if expired

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [c++ client] re-acquire authn token if expired
..


Patch Set 16:

(12 comments)

http://gerrit.cloudera.org:8080/#/c/6648/16/docs/known_issues.adoc
File docs/known_issues.adoc:

PS16, Line 172: one week is now supported only for Kudu C++ client
> nit: I think for the context of limitations, it's better to be explicit abo
Done


http://gerrit.cloudera.org:8080/#/c/6648/16/docs/security.adoc
File docs/security.adoc:

PS16, Line 221: Java
> nit: insert "The"
Done


PS16, Line 223: yet 
> nit: we decided at some point to not use these time-related words in the do
Done


PS16, Line 223:  C++
> nit: the
Done


PS16, Line 224: client
> clients
Done


http://gerrit.cloudera.org:8080/#/c/6648/16/src/kudu/client/batcher.cc
File src/kudu/client/batcher.cc:

PS16, Line 415: existring
> typo
Done


http://gerrit.cloudera.org:8080/#/c/6648/16/src/kudu/client/client-internal.cc
File src/kudu/client/client-internal.cc:

PS16, Line 731: ANY_BUT_AUTHN_OTKEN
> I suggested the new name, so I'll go to bat for it.  Besides being a positi
ok.

ANY_CREDENTIALS and PRIMARY_CREDENTIALS, right?


Line 740:   DCHECK(creds_policy == CredentialsPolicy::ANY_CREDENTIALS ||
> Isn't this statically verified by the compiler?
I don't think it's going to be verified by compiler if somebody adds a new 
credentials policy, like SECONDARY_CREDENTIALS (meaning only secondary 
credentials may be used for connection negotiation).

This debug-only assert will catch the case when this piece of code is not 
updated after that.


Line 753:   if (leader_master_rpc_primary_creds_ || need_new_rpc_primary_creds) 
{
> I was having a hard time following this, so I tried to take a crack at redu
To me -- nope, it's not :)  I also spent some time trying to make sure that 
your code does what I want.  It also has a misleading comment on non-existent 
connections if the final 'else' part.

The two boolean flags store the fact whether it's necessary to create new RPC 
calls with appropriate credentials.


Line 761: scoped_refptr& rpc(
> This statement confusing.  I think it's copying the scoped_refptr to an unn
What would be that 'unnamed stack-allocated variable'?

It just makes a reference either to leader_master_rpc_primary_creds_ or to 
leader_master_rpc_any_creds_

I'm not sure I understand: what unbound variable are you talking about?


http://gerrit.cloudera.org:8080/#/c/6648/16/src/kudu/client/client-internal.h
File src/kudu/client/client-internal.h:

PS16, Line 165:   rpc::CredentialsPolicy creds_policy =
  :   rpc::CredentialsPolicy::ANY_CREDENTIALS);
> maybe it's just me, but I find this wrapping a bit odd. Maybe wrap all the 
I wrapped all parameters at +4 shift -- that looks cleaner (at least no 
additional line just for the default value).


PS16, Line 237:   scoped_refptr 
leader_master_rpc_any_creds_;
  :   std::vector 
leader_master_callbacks_any_creds_;
  :   scoped_refptr 
leader_master_rpc_primary_creds_;
  :   std::vector 
leader_master_callbacks_primary_creds_;
> Yes, the idea was to not wait for that re-connection again in that rare cas
Also, if we don't split the callbacks, how do we know the process of scheduling 
the retries converges at all?  I.e., consider the case when a retries are 
scheduled again and again, and the any-creds call always arrives first all the 
time, then comes the primary-creds call, and that so long?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
Gerrit-PatchSet: 16
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [c++ client] re-acquire authn token if expired

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: [c++ client] re-acquire authn token if expired
..


Patch Set 16:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6648/16/src/kudu/client/client-internal.cc
File src/kudu/client/client-internal.cc:

PS16, Line 731: ANY_BUT_AUTHN_OTKEN
> You are right -- that's the old name (with an additional typo).  Now it's P
I suggested the new name, so I'll go to bat for it.  Besides being a positive 
label, which I think is always better, it leaves the door open for considering 
IPKI certs to be non-primary in the future.  We don't have any reason to do 
this at the moment, but I can imagine we might want to add a tserver->master 
API which is only valid when connecting via primary credentials.  In fact, 
requesting a signed cert should probably already be like this.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
Gerrit-PatchSet: 16
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [c++ client] re-acquire authn token if expired

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: [c++ client] re-acquire authn token if expired
..


Patch Set 16:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/6648/16/src/kudu/client/client-internal.cc
File src/kudu/client/client-internal.cc:

PS16, Line 731: ANY_BUT_AUTHN_OTKEN
PRIMARY


Line 740:   DCHECK(creds_policy == CredentialsPolicy::ANY_CREDENTIALS ||
Isn't this statically verified by the compiler?


Line 753:   if (leader_master_rpc_primary_creds_ || need_new_rpc_primary_creds) 
{
I was having a hard time following this, so I tried to take a crack at reducing 
the number of branches, this is what I came up with (in particular the two 
boolean flags are removed, since I had trouble following those):

if (leader_master_rpc_primary_creds_) {
  // Piggy back on an existing connection with primary credentials.
  leader_master_callbacks_primary_creds_.push_back(cb);
} else if (leader_master_rpc_any_creds_ &&
   cred_policy == CredentialsPolicy::ANY_CREDENTIALS) {
  // Piggy back on an existing connection with any credentials.
  leader_master_callbacks_any_creds_.push_back(cb);
} else {
  // There are no existing connections, create a new one with the required 
policy.

  scoped_refptr rpc(
  new internal::ConnectToClusterRpc(
std::bind(::Data::ConnectedToClusterCb, this,
  std::placeholders::_1,
  std::placeholders::_2,
  std::placeholders::_3,
  creds_policy),
std::move(master_sockaddrs),
deadline,
client->default_rpc_timeout(),
messenger_,
creds_policy));

  switch (creds_policy) {
case PRIMARY_CREDENTIALS:
  leader_master_rpc_primary_creds_.reset(rpc.get());
  leader_master_callbacks_primary_creds_.push_back(cb);
  break;
case ANY_CREDENTIALS:
  leader_master_rpc_any_creds_.reset(rpc.get());
  leader_master_callbacks_any_creds_.push_back(cb);
  break;
  }
  l.unlock();
  rpc->SendRpc();
}

hopefully that's simpler?


Line 761: scoped_refptr& rpc(
This statement confusing.  I think it's copying the scoped_refptr to an unnamed 
stack allocated variable, and then taking a reference to that, but I'm not 
really sure.  I think what you want is 

scoped_refptr& rpc = need_new_rpc_primary_creds 
? leader_master_rpc_primary_creds_ : leader_master_rpc_any_creds_;

I really think we should forbid taking references to unbound variables, the 
lifetime semantics are just too confusing, and there's never a good reason to 
do it.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
Gerrit-PatchSet: 16
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [c++ client] re-acquire authn token if expired

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [c++ client] re-acquire authn token if expired
..


Patch Set 16:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/6648/16/src/kudu/client/client-internal.cc
File src/kudu/client/client-internal.cc:

PS16, Line 731: ANY_BUT_AUTHN_OTKEN
> this isn't the current name (though I actually like something like you have
You are right -- that's the old name (with an additional typo).  Now it's 
PRIMARY_CREDENTIALS.  I'll update this.

The new naming looks more generic.


http://gerrit.cloudera.org:8080/#/c/6648/16/src/kudu/client/client-internal.h
File src/kudu/client/client-internal.h:

PS16, Line 237:   scoped_refptr 
leader_master_rpc_any_creds_;
  :   std::vector 
leader_master_callbacks_any_creds_;
  :   scoped_refptr 
leader_master_rpc_primary_creds_;
  :   std::vector 
leader_master_callbacks_primary_creds_;
> is it really necessary to split the callbacks? eg if you try to request a C
Yes, the idea was to not wait for that re-connection again in that rare case.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
Gerrit-PatchSet: 16
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [c++ client] re-acquire authn token if expired

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: [c++ client] re-acquire authn token if expired
..


Patch Set 16:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/6648/16/docs/known_issues.adoc
File docs/known_issues.adoc:

PS16, Line 172: one week is now supported only for Kudu C++ client
nit: I think for the context of limitations, it's better to be explicit about 
the negative cases. i.e "is not supported by the Java client" or "is only 
supported by the C++ client but not by the Java client" or somesuch.


http://gerrit.cloudera.org:8080/#/c/6648/16/docs/security.adoc
File docs/security.adoc:

PS16, Line 221: Java
nit: insert "The"


PS16, Line 223: yet 
nit: we decided at some point to not use these time-related words in the docs, 
since they imply some future timeline, but we aren't actually talking about 
that timeline.


PS16, Line 223:  C++
nit: the


PS16, Line 224: client
clients


http://gerrit.cloudera.org:8080/#/c/6648/16/src/kudu/client/batcher.cc
File src/kudu/client/batcher.cc:

PS16, Line 415: existring
typo


http://gerrit.cloudera.org:8080/#/c/6648/16/src/kudu/client/client-internal.cc
File src/kudu/client/client-internal.cc:

PS16, Line 731: ANY_BUT_AUTHN_OTKEN
this isn't the current name (though I actually like something like you have 
here better!)


http://gerrit.cloudera.org:8080/#/c/6648/16/src/kudu/client/client-internal.h
File src/kudu/client/client-internal.h:

PS16, Line 165:   rpc::CredentialsPolicy creds_policy =
  :   rpc::CredentialsPolicy::ANY_CREDENTIALS);
maybe it's just me, but I find this wrapping a bit odd. Maybe wrap all the 
params at the '(' instead so that the default param ends up further-indented 
from its param name?


PS16, Line 237:   scoped_refptr 
leader_master_rpc_any_creds_;
  :   std::vector 
leader_master_callbacks_any_creds_;
  :   scoped_refptr 
leader_master_rpc_primary_creds_;
  :   std::vector 
leader_master_callbacks_primary_creds_;
is it really necessary to split the callbacks? eg if you try to request a 
ConnectToCluster with primary-creds-only policy, but one is already running 
without that policy, the worst that happens is that the callback happens and, 
on your retry, you still don't have active tokens, and you have to reconnect 
again, right? i.e this could only be transient and only be transient once in a 
rare case


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
Gerrit-PatchSet: 16
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [c++ client] re-acquire authn token if expired

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: [c++ client] re-acquire authn token if expired
..

[c++ client] re-acquire authn token if expired

Updated C++ client to re-acquire authn token if server responds with
FATAL_INVALID_AUTHENTICATION_TOKEN error on connection negotiation.
If that happens while negotiating a connection for a call, the
connection is closed and the client reconnects to the cluster to get
a new authn token.  After successfully re-acquiring authn token the
client retries the RPC call again.

Added corresponding integration test to cover authn token re-acquisition
for various scenarios.

Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
---
M docs/known_issues.adoc
M docs/security.adoc
M src/kudu/client/batcher.cc
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/master_rpc.cc
M src/kudu/client/master_rpc.h
M src/kudu/client/meta_cache.cc
M src/kudu/client/scanner-internal.cc
M src/kudu/client/scanner-internal.h
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/authn_token_expire-itest.cc
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/retriable_rpc.h
M src/kudu/rpc/rpc.h
M src/kudu/rpc/server_negotiation.cc
23 files changed, 755 insertions(+), 143 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/6648/16
-- 
To view, visit http://gerrit.cloudera.org:8080/6648
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
Gerrit-PatchSet: 16
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [c++ client] re-acquire authn token if expired

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: [c++ client] re-acquire authn token if expired
..

[c++ client] re-acquire authn token if expired

Updated C++ client to re-acquire authn token if server responds with
FATAL_INVALID_AUTHENTICATION_TOKEN error on connection negotiation.
If that happens while negotiating an connection while performing an
RPC call, the connection being negotiated is closed and the client
re-connects to the cluster to get new authn token.  After successfully
re-acquiring authn token the client retries the RPC call again.

Added corresponding integration test to cover authn token re-acquisition
for various scenarios.

Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
---
M docs/known_issues.adoc
M docs/security.adoc
M src/kudu/client/batcher.cc
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/master_rpc.cc
M src/kudu/client/master_rpc.h
M src/kudu/client/meta_cache.cc
M src/kudu/client/scanner-internal.cc
M src/kudu/client/scanner-internal.h
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/authn_token_expire-itest.cc
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/retriable_rpc.h
M src/kudu/rpc/rpc.h
M src/kudu/rpc/server_negotiation.cc
23 files changed, 772 insertions(+), 134 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/6648/14
-- 
To view, visit http://gerrit.cloudera.org:8080/6648
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
Gerrit-PatchSet: 14
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [c++ client] re-acquire authn token if expired

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [c++ client] re-acquire authn token if expired
..


Patch Set 13:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6648/13/src/kudu/client/client-internal.cc
File src/kudu/client/client-internal.cc:

Line 731:   if (creds_policy == CredentialsPolicy::ANY_BUT_AUTHN_TOKEN) {
It seems this part have some room for optimization: e.g., we can re-use 
token-re-acq rpc to piggy back RPCs with 'any_creds' policy.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [c++ client] re-acquire authn token if expired

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: [c++ client] re-acquire authn token if expired
..

[c++ client] re-acquire authn token if expired

Updated C++ client to re-acquire authn token if server responds with
FATAL_INVALID_AUTHENTICATION_TOKEN error on connection negotiation.
If that happens while negotiating an connection while performing an
RPC call, the connection being negotiated is closed and the client
re-connects to the cluster to get new authn token.  After successfully
re-acquiring authn token the client retries the RPC call again.

Added corresponding integration test to cover authn token re-acquisition
for various scenarios.

Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
---
M docs/known_issues.adoc
M docs/security.adoc
M src/kudu/client/batcher.cc
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/master_rpc.cc
M src/kudu/client/master_rpc.h
M src/kudu/client/meta_cache.cc
M src/kudu/client/scanner-internal.cc
M src/kudu/client/scanner-internal.h
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/authn_token_expire-itest.cc
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/retriable_rpc.h
M src/kudu/rpc/rpc.h
M src/kudu/rpc/server_negotiation.cc
23 files changed, 765 insertions(+), 131 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/6648/13
-- 
To view, visit http://gerrit.cloudera.org:8080/6648
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [c++ client] re-acquire authn token if expired

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [c++ client] re-acquire authn token if expired
..


Patch Set 12:

> thanks. I don't think we need anything too formal in terms of a
 > design doc, but just writing down a paragraph for each approach
 > seems like a great place to start

I published a small write-up on this at 
https://docs.google.com/a/cloudera.com/document/d/1hjH9Y7yOJ6VQKPgpbKzjcUqhi7DyhcIgC3U8R_P6Ehs/edit?usp=sharing

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [c++ client] re-acquire authn token if expired

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [c++ client] re-acquire authn token if expired
..


Patch Set 12:

> Haven't followed the full discussion here, but it does seem like
 > this is growing into a pretty large patch spanning KRPC and "kudu
 > itself". Maybe worth taking a step back to write down and discuss
 > the design here, and then we can split out the KRPC work from the
 > specific work on the client?
 > 
 > If I understand correctly, the overview is:
 > - in order to fetch a new token, we need to make sure we're
 > krb5-authenticated to the master(s)
 > -- therefore, we need some way to make an RPC call ignore a
 > pre-established token-authenticated connection and reconnect with
 > krb5
 > -- this necessitates various changes in KRPC rather than just in
 > our own client code
 > 
 > Then it seems like there are a few possible alternatives how to do
 > that -- either something in UserCredentials, something on the
 > RpcController layer, etc, right?

Right.  At least I was looking at 3 different approaches only at KRPC to handle 
establishing and re-using a new connection to master servers which would be 
negotiated using non-token credentials.

I think creating a small document to start discussion on the design is a way to 
do.  I'll create a shared doc and send a link today.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [c++ client] re-acquire authn token if expired

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: [c++ client] re-acquire authn token if expired
..


Patch Set 12:

Haven't followed the full discussion here, but it does seem like this is 
growing into a pretty large patch spanning KRPC and "kudu itself". Maybe worth 
taking a step back to write down and discuss the design here, and then we can 
split out the KRPC work from the specific work on the client?

If I understand correctly, the overview is:
- in order to fetch a new token, we need to make sure we're krb5-authenticated 
to the master(s)
-- therefore, we need some way to make an RPC call ignore a pre-established 
token-authenticated connection and reconnect with krb5
-- this necessitates various changes in KRPC rather than just in our own client 
code

Then it seems like there are a few possible alternatives how to do that -- 
either something in UserCredentials, something on the RpcController layer, etc, 
right?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [c++ client] re-acquire authn token if expired

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [c++ client] re-acquire authn token if expired
..


Patch Set 12:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6648/12/src/kudu/client/client-internal.cc
File src/kudu/client/client-internal.cc:

Line 719:   // flight at a time -- there isn't much sense in requesting this 
information
> Right.  Perhaps then you _do_ want to take into account the token when comp
Agreed: there is some things to think just another time.

Woops, I didn't realize this simple task would have so many nuances and corner 
cases.

And frankly, I don't feel happy about this code.  Probably, that's a sign that 
another pass is necessary.

Thank you for pointing bringing more attention to this.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [c++ client] re-acquire authn token if expired

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [c++ client] re-acquire authn token if expired
..


Patch Set 12:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6648/12/src/kudu/client/client-internal.cc
File src/kudu/client/client-internal.cc:

Line 719:   // flight at a time -- there isn't much sense in requesting this 
information
> This needs to get a little more fine grained, right?  We'd want to start a 
Yep, I thought about that a bit and decided to keep it simple.  The idea is 
that eventually an RPC with 're-acquire authn token' flag should be here, but I 
agree it's not guaranteed.

Also, it seems I also missed another part of the equation here: it's necessary 
to establish a brand new connection to the master to reacquire authn token if 
current connection has been established with a token, otherwise the master will 
not issue a new token.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [c++ client] re-acquire authn token if expired

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: [c++ client] re-acquire authn token if expired
..

[c++ client] re-acquire authn token if expired

Updated C++ client to re-acquire authn token if server responds with
FATAL_INVALID_AUTHENTICATION_TOKEN error on connection negotiation.
If that happens while negotiating an connection while performing an
RPC call, the connection being negotiated is closed and the client
re-connects to the cluster to get new authn token.  After successfully
re-acquiring authn token the client retries the RPC call again.

Added corresponding integration test to cover authn token re-acquisition
for various scenarios.

Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
---
M docs/known_issues.adoc
M docs/security.adoc
M src/kudu/client/batcher.cc
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/master_rpc.cc
M src/kudu/client/master_rpc.h
M src/kudu/client/meta_cache.cc
M src/kudu/client/scanner-internal.cc
M src/kudu/client/scanner-internal.h
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/authn_token_expire-itest.cc
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/proxy.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/retriable_rpc.h
M src/kudu/rpc/rpc.h
M src/kudu/rpc/rpc_controller.cc
M src/kudu/rpc/rpc_controller.h
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/user_credentials.cc
M src/kudu/rpc/user_credentials.h
28 files changed, 680 insertions(+), 135 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/6648/12
-- 
To view, visit http://gerrit.cloudera.org:8080/6648
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [c++ client] re-acquire authn token if expired

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: [c++ client] re-acquire authn token if expired
..

[c++ client] re-acquire authn token if expired

Updated C++ client to re-acquire authn token if server responds with
FATAL_INVALID_AUTHENTICATION_TOKEN error on connection negotiation.
If that happens while negotiating an connection while performing an
RPC call, the connection being negotiated is closed and the client
re-connects to the cluster to get new authn token.  After successfully
re-acquiring authn token the client retries the RPC call again.

Added corresponding integration test to cover authn token re-acquisition
for various scenarios.

Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
---
M docs/known_issues.adoc
M docs/security.adoc
M src/kudu/client/batcher.cc
M src/kudu/client/client-internal.cc
M src/kudu/client/meta_cache.cc
M src/kudu/client/scanner-internal.cc
M src/kudu/client/scanner-internal.h
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/authn_token_expire-itest.cc
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/messenger.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/retriable_rpc.h
M src/kudu/rpc/rpc.h
M src/kudu/rpc/server_negotiation.cc
21 files changed, 553 insertions(+), 82 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/6648/11
-- 
To view, visit http://gerrit.cloudera.org:8080/6648
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [c++ client] re-acquire authn token if expired

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [c++ client] re-acquire authn token if expired
..


Patch Set 9: Verified+1

Unrelated flake: org.apache.kudu.client.ITClient.test

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [c++ client] re-acquire authn token if expired

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: [c++ client] re-acquire authn token if expired
..

[c++ client] re-acquire authn token if expired

Updated C++ client to re-acquire authn token if server responds with
FATAL_INVALID_AUTHENTICATION_TOKEN error on connection negotiation.
If that happens while negotiating an connection while performing an
RPC call, the connection being negotiated is closed and the client
re-connects to the cluster to get new authn token.  After successfully
re-acquiring authn token the client retries the RPC call again.

Added corresponding integration test to cover authn token re-acquisition
for various scenarios.

Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
---
M docs/known_issues.adoc
M docs/security.adoc
M src/kudu/client/batcher.cc
M src/kudu/client/client-internal.cc
M src/kudu/client/meta_cache.cc
M src/kudu/client/scanner-internal.cc
M src/kudu/client/scanner-internal.h
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/authn_token_expire-itest.cc
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/messenger.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/retriable_rpc.h
M src/kudu/rpc/rpc.h
M src/kudu/rpc/server_negotiation.cc
21 files changed, 553 insertions(+), 82 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/6648/9
-- 
To view, visit http://gerrit.cloudera.org:8080/6648
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [c++ client] re-acquire authn token if expired

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [c++ client] re-acquire authn token if expired
..


Patch Set 5:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/6648/8/src/kudu/client/meta_cache.cc
File src/kudu/client/meta_cache.cc:

Line 704:   if (new_status.ok()) {
> This can be kept the same as it was.
Indeed -- it was better that way.


http://gerrit.cloudera.org:8080/#/c/6648/5/src/kudu/client/scanner-internal.cc
File src/kudu/client/scanner-internal.cc:

Line 134: c->data_->messenger_->reset_authn_token();
> I meant a race between two threads with two separate KuduScanner instances 
Ah, I see.  Sure, that could be a race, good point.

I'll add a test for that.  I'm curious why we don't see it now.  There are 
other possible cases with concurrent writes and scans and also with concurrent 
client->master calls.

I think sometime it resolves itself automatically (since server returns 
INVALID_AUTHENTICATION_TOKEN for absent authn token if the negotiation features 
at the client side were determined before token has been reset), and 
RuntimeError() for other cases is very unlikely.

But it's necessary to fix that, I think.


http://gerrit.cloudera.org:8080/#/c/6648/8/src/kudu/client/scanner-internal.h
File src/kudu/client/scanner-internal.h:

PS8, Line 63: athentication
> typo
Done


http://gerrit.cloudera.org:8080/#/c/6648/8/src/kudu/integration-tests/authn_token_expire-itest.cc
File src/kudu/integration-tests/authn_token_expire-itest.cc:

PS8, Line 71: name_("k
> typo
Done


http://gerrit.cloudera.org:8080/#/c/6648/8/src/kudu/rpc/reactor.h
File src/kudu/rpc/reactor.h:

Line 185:  std::unique_ptr 
rpc_error);
> Are these PBs wrapped in a unique_ptr just so they can be moved?  That's fi
Yes, partially.  The main reason, though, is to enforce clear ownership 
semantics and avoid memory leaks.


http://gerrit.cloudera.org:8080/#/c/6648/8/src/kudu/rpc/retriable_rpc.h
File src/kudu/rpc/retriable_rpc.h:

PS8, Line 92: autentication
> typo
Done


PS8, Line 209:   resp_.Clear();
 :   current_ = nullptr;
> Are these two lines necessary for the token retry case as well?
It's a good question.

It seems the current_ does not need reset in that case (if the tablet server 
responded with INVALID_TOKEN it does not mean it's worth re-trying at other 
tserver.

We can clean the prior response, but it seems it's not used anywhere in the 
INVALID_AUTHENTICATION_TOKEN code path.  I'll clear it for uniformity.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [c++ client] re-acquire authn token if expired

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: [c++ client] re-acquire authn token if expired
..


Patch Set 8:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/6648/8/src/kudu/client/meta_cache.cc
File src/kudu/client/meta_cache.cc:

Line 704:   if (new_status.ok()) {
This can be kept the same as it was.


http://gerrit.cloudera.org:8080/#/c/6648/5/src/kudu/client/scanner-internal.cc
File src/kudu/client/scanner-internal.cc:

Line 134: c->data_->messenger_->reset_authn_token();
> There would be a race if any pair of methods like KuduScanner::{OpenTable()
I meant a race between two threads with two separate KuduScanner instances from 
the same client (which I think is allowed).


http://gerrit.cloudera.org:8080/#/c/6648/8/src/kudu/client/scanner-internal.h
File src/kudu/client/scanner-internal.h:

PS8, Line 63: athentication
typo


http://gerrit.cloudera.org:8080/#/c/6648/8/src/kudu/integration-tests/authn_token_expire-itest.cc
File src/kudu/integration-tests/authn_token_expire-itest.cc:

PS8, Line 71: tsrevers
typo


http://gerrit.cloudera.org:8080/#/c/6648/8/src/kudu/rpc/reactor.h
File src/kudu/rpc/reactor.h:

Line 185:  std::unique_ptr 
rpc_error);
Are these PBs wrapped in a unique_ptr just so they can be moved?  That's fine 
if they are, just trying to understand.


http://gerrit.cloudera.org:8080/#/c/6648/8/src/kudu/rpc/retriable_rpc.h
File src/kudu/rpc/retriable_rpc.h:

PS8, Line 92: autentication
typo


PS8, Line 209:   resp_.Clear();
 :   current_ = nullptr;
Are these two lines necessary for the token retry case as well?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [c++ client] re-acquire authn token if expired

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: [c++ client] re-acquire authn token if expired
..

[c++ client] re-acquire authn token if expired

Updated C++ client to re-acquire authn token if server responds with
FATAL_INVALID_AUTHENTICATION_TOKEN error on connection negotiation.
If that happens while negotiating an connection while performing an
RPC call, the connection being negotiated is closed and the client
re-connects to the cluster to get new authn token.  After successfully
re-acquiring authn token the client retries the RPC call again.

Added corresponding integration test to cover authn token re-acquisition
for various scenarios.

Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
---
M docs/known_issues.adoc
M docs/security.adoc
M src/kudu/client/batcher.cc
M src/kudu/client/client-internal.cc
M src/kudu/client/meta_cache.cc
M src/kudu/client/scanner-internal.cc
M src/kudu/client/scanner-internal.h
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/authn_token_expire-itest.cc
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/messenger.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/retriable_rpc.h
M src/kudu/rpc/rpc.h
M src/kudu/rpc/server_negotiation.cc
21 files changed, 541 insertions(+), 81 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/6648/8
-- 
To view, visit http://gerrit.cloudera.org:8080/6648
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [c++ client] re-acquire authn token if expired

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: [c++ client] re-acquire authn token if expired
..

[c++ client] re-acquire authn token if expired

Updated C++ client to re-acquire authn token if server responds with
FATAL_INVALID_AUTHENTICATION_TOKEN error on connection negotiation.
If that happens while negotiating an connection while performing an
RPC call, the connection being negotiated is closed and the client
re-connects to the cluster to get new authn token.  After successfully
re-acquiring authn token the client retries the RPC call again.

Added corresponding integration test to cover authn token re-acquisition
for various scenarios.

Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
---
M docs/known_issues.adoc
M docs/security.adoc
M src/kudu/client/batcher.cc
M src/kudu/client/client-internal.cc
M src/kudu/client/meta_cache.cc
M src/kudu/client/scanner-internal.cc
M src/kudu/client/scanner-internal.h
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/authn_token_expire-itest.cc
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/messenger.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/retriable_rpc.h
M src/kudu/rpc/rpc.h
M src/kudu/rpc/server_negotiation.cc
21 files changed, 557 insertions(+), 81 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/6648/7
-- 
To view, visit http://gerrit.cloudera.org:8080/6648
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [c++ client] re-acquire authn token if expired

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [c++ client] re-acquire authn token if expired
..


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6648/5/src/kudu/rpc/connection.cc
File src/kudu/rpc/connection.cc:

Line 648: rthread->CompleteConnectionNegotiation(conn_, negotiation_status_,
> Done
I decided to do that in a separate changelist -- I think it would be cleaner 
that way.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [c++ client] re-acquire authn token if expired

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: [c++ client] re-acquire authn token if expired
..

[c++ client] re-acquire authn token if expired

Updated C++ client to re-acquire authn token if server responds with
FATAL_INVALID_AUTHENTICATION_TOKEN error on connection negotiation.
If that happens while negotiating an connection while performing an
RPC call, the connection being negotiated is closed and the client
re-connects to the cluster to get new authn token.  After successfully
re-acquiring authn token the client retries the RPC call again.

Added corresponding integration test to cover authn token re-acquisition
for various scenarios.

Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
---
M docs/known_issues.adoc
M docs/security.adoc
M src/kudu/client/batcher.cc
M src/kudu/client/client-internal.cc
M src/kudu/client/meta_cache.cc
M src/kudu/client/scanner-internal.cc
M src/kudu/client/scanner-internal.h
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/authn_token_expire-itest.cc
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/messenger.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/retriable_rpc.h
M src/kudu/rpc/rpc.h
M src/kudu/rpc/server_negotiation.cc
21 files changed, 557 insertions(+), 81 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/6648/6
-- 
To view, visit http://gerrit.cloudera.org:8080/6648
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [c++ client] re-acquire authn token if expired

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: [c++ client] re-acquire authn token if expired
..


Patch Set 5:

(16 comments)

http://gerrit.cloudera.org:8080/#/c/6648/3//COMMIT_MSG
Commit Message:

PS3, Line 11: an
a connection


http://gerrit.cloudera.org:8080/#/c/6648/5/src/kudu/client/client-internal.cc
File src/kudu/client/client-internal.cc:

Line 223: const Status& connect_status = ConnectToCluster(client, 
deadline);
ConnectToCluster returns an owned Status.


http://gerrit.cloudera.org:8080/#/c/6648/5/src/kudu/client/meta_cache.cc
File src/kudu/client/meta_cache.cc:

Line 677:   table_->client()->data_->messenger_->reset_authn_token();
Not every caller of this should be resetting the authn token.  I think it's 
more appropriate to add this line in the block starting on line 713.


Line 706: const rpc::RpcController& controller(retrier().controller());
this should be owned


Line 708: const Status& controller_status = controller.status();
this should be owned


Line 709: if (controller_status.IsRemoteError()) {
Would it be possible to build this logic into the RpcRetrier instead of 
implementing it here?


Line 769:   if (new_status.IsNotAuthorized()) {
I don't think this is necessarily retriable.  For instance it doesn't seem to 
be retried in the scanner (it results in an OTHER_TS_ERROR)


http://gerrit.cloudera.org:8080/#/c/6648/5/src/kudu/client/scanner-internal.cc
File src/kudu/client/scanner-internal.cc:

Line 134: c->data_->messenger_->reset_authn_token();
Is there a race here between calling reset_authn_token() and another thread 
having already executed a successful ConnectToCluster, thus having already 
overwritten the expired token with a fresh one?


http://gerrit.cloudera.org:8080/#/c/6648/5/src/kudu/integration-tests/authn_token_expire-itest.cc
File src/kudu/integration-tests/authn_token_expire-itest.cc:

Line 105: std::copy(common_flags.begin(), common_flags.end(),
This seems more difficult than directly inserting the flags into 
opts.extra_master_flags.


Line 186: TEST_F(AuthnTokenExpireITest, FetchNewAuthnTokenOnInsertAndScan) {
Is this testing something above and beyond InvalidTokenDuringWorkload?  Perhaps 
they can be combined?


Line 214:   // Since the authn token is verified upon connection establishment, 
it's
Should this be handled by FLAGS_rpc_reopen_outbound_connections ?


Line 234: TEST_F(AuthnTokenExpireITest, RestartTabletServers) {
I think this is a good test case, but I think 
FLAGS_rpc_reopen_outbound_connections shouldn't be set for it.


Line 263: TEST_F(AuthnTokenExpireITest, RestartCluster) {
Likewise, I think FLAGS_rpc_reopen_outbound_connections shouldn't be set during 
this.


http://gerrit.cloudera.org:8080/#/c/6648/5/src/kudu/rpc/client_negotiation.cc
File src/kudu/rpc/client_negotiation.cc:

Line 97:   const string& code_name = 
ErrorStatusPB::RpcErrorCodePB_Name(error.code());
RpcErrorCodePB_Name returns an owned string.


http://gerrit.cloudera.org:8080/#/c/6648/5/src/kudu/rpc/connection.cc
File src/kudu/rpc/connection.cc:

Line 148: LOG(WARNING) << "Shutting down connection " << ToString()
the ToString already includes "connection".


Line 648: rthread->CompleteConnectionNegotiation(conn_, negotiation_status_,
while you are here, the negotiation_status_ should probably be moved as well.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [c++ client] re-acquire authn token if expired

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: [c++ client] re-acquire authn token if expired
..

[c++ client] re-acquire authn token if expired

Updated C++ client to re-acquire authn token if server responds with
FATAL_INVALID_AUTHENTICATION_TOKEN error on connection negotiation.
If that happens while negotiating an connection while performing an
RPC call, the connection being negotiated is closed and the client
re-connects to the cluster to get new authn token.  After successfully
re-acquiring authn token the client retries the RPC call again.

Added corresponding integration test to cover authn token re-acquisition
for various scenarios.

Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
---
M src/kudu/client/batcher.cc
M src/kudu/client/client-internal.cc
M src/kudu/client/meta_cache.cc
M src/kudu/client/scanner-internal.cc
M src/kudu/client/scanner-internal.h
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/authn_token_expire-itest.cc
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/messenger.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/retriable_rpc.h
M src/kudu/rpc/rpc.h
M src/kudu/rpc/server_negotiation.cc
19 files changed, 620 insertions(+), 76 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/6648/5
-- 
To view, visit http://gerrit.cloudera.org:8080/6648
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [c++ client] re-acquire authn token if expired

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: [c++ client] re-acquire authn token if expired
..

[c++ client] re-acquire authn token if expired

Updated C++ client to re-acquire authn token if server responds with
FATAL_INVALID_AUTHENTICATION_TOKEN error on connection negotiation.
If that happens while negotiating an connection while performing an
RPC call, the connection being negotiated is closed and the client
re-connects to the cluster to get new authn token.  After successfully
re-acquiring authn token the client retries the RPC call again.

Added corresponding integration test to cover authn token re-acquisition
for various scenarios.

Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
---
M src/kudu/client/batcher.cc
M src/kudu/client/client-internal.cc
M src/kudu/client/meta_cache.cc
M src/kudu/client/scanner-internal.cc
M src/kudu/client/scanner-internal.h
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/authn_token_expire-itest.cc
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/messenger.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/retriable_rpc.h
M src/kudu/rpc/rpc.h
M src/kudu/rpc/server_negotiation.cc
19 files changed, 625 insertions(+), 76 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/6648/4
-- 
To view, visit http://gerrit.cloudera.org:8080/6648
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [c++ client] re-acquire authn token if expired

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [c++ client] re-acquire authn token if expired
..


Patch Set 2:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/6648/2/src/kudu/integration-tests/authn_token_expire-itest.cc
File src/kudu/integration-tests/authn_token_expire-itest.cc:

Line 80: token_validity_seconds_(2),
> is this going to be flaky in TSAN? or is it OK because the new behavior wil
I changed num_tablet_servers_ to 3 to comply with the restrictions in our code 
to use TestWorkload for better test coverage.

As I see, the heartbeat interval as 10 msec is successfully used in majority of 
generic Kudu tests and it does not cause flakiness.

Short token validity interval should not bring problems here because the client 
is supposed to retry, so I don't expect flakiness in SAN builds, given that 2 
seconds is enough to complete Kudu RPC eventually, even on slow VMs.


PS2, Line 78:   : num_tablet_servers_(2),
: heartbeat_interval_ms_(10),
: token_validity_seconds_(2),
: tsk_rotation_seconds_(1),
: key_column_name_("key"),
: key_split_value_(8) {
> why are all of these members instead of just constants?
Because they are re-used in the code below.  Yes, the tsk_rotation_seconds_ 
become obsolete -- I removed it.


Line 88: //FLAGS_scanner_gc_check_interval_us = 50 * 1000;
> ?
Done


Line 98: CHECK_OK(b.Build(_));
> if you don't care about the schema in particular, can you use one of the ex
Done


PS2, Line 166: size_t
> prefer signed type (int)
Done


PS2, Line 177: NO
> typo
Done


PS2, Line 213:   // Since the authn token is verified upon connection 
establishment, it's
 :   // necessary to make sure the client does not keep the 
connections to
 :   // corresponding tablet servers open. So, let's restart the 
tablet servers
 :   // since the RPC layer might keep already established 
connections open.
> we could set the rpc keepalive timer to short, instead?
Good idea -- I added that.


Line 229: //ASSERT_OK(scanner.SetFaultTolerant());
> ?
Done


Line 230: ASSERT_OK(scanner.Open());
> this is only really testing the case of the token being expired on the begi
Good idea -- I added that for connection negotiations which is just cover some 
edge-cases of the authn token re-acq logic.

I'll extend this to other RPCs as well.


PS2, Line 230: ASSERT_OK(scanner.Open());
 : size_t row_count = 0;
 : while (scanner.HasMoreRows()) {
 :   vector rows;
 :   ASSERT_OK(scanner.NextBatch());
 :   row_count += rows.size();
 : }
 : EXPECT_EQ(2, row_count);
> I think client-test-util has some code to CountRowsFromScanner or something
Thanks, done.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [c++ client] re-acquire authn token if expired

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: [c++ client] re-acquire authn token if expired
..

[c++ client] re-acquire authn token if expired

Updated C++ client to re-acquire authn token if server responds with
FATAL_INVALID_AUTHENTICATION_TOKEN error on connection negotiation.
If that happens while negotiating an connection while performing an
RPC call, the connection being negotiated is closed and the client
re-connects to the cluster to get new authn token.  After successfully
re-acquiring authn token the client retries the RPC call again.

Added corresponding integration test to cover authn token re-acquisition
for various scenarios.

Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
---
M src/kudu/client/batcher.cc
M src/kudu/client/client-internal.cc
M src/kudu/client/meta_cache.cc
M src/kudu/client/scanner-internal.cc
M src/kudu/client/scanner-internal.h
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/authn_token_expire-itest.cc
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/messenger.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/retriable_rpc.h
M src/kudu/rpc/rpc.h
M src/kudu/rpc/server_negotiation.cc
19 files changed, 586 insertions(+), 75 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/6648/3
-- 
To view, visit http://gerrit.cloudera.org:8080/6648
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [c++ client] re-acquire authn token if expired

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: [c++ client] re-acquire authn token if expired
..


Patch Set 2:

(13 comments)

http://gerrit.cloudera.org:8080/#/c/6648/2/src/kudu/integration-tests/authn_token_expire-itest.cc
File src/kudu/integration-tests/authn_token_expire-itest.cc:

Line 80: token_validity_seconds_(2),
is this going to be flaky in TSAN? or is it OK because the new behavior will 
get it to retry until it happens to run fast enough?


PS2, Line 78:   : num_tablet_servers_(2),
: heartbeat_interval_ms_(10),
: token_validity_seconds_(2),
: tsk_rotation_seconds_(1),
: key_column_name_("key"),
: key_split_value_(8) {
why are all of these members instead of just constants?


Line 88: //FLAGS_scanner_gc_check_interval_us = 50 * 1000;
?


Line 98: CHECK_OK(b.Build(_));
if you don't care about the schema in particular, can you use one of the 
existing ones like CreateKeyValueTestSchema?


PS2, Line 166: size_t
prefer signed type (int)


PS2, Line 177: NO
typo


PS2, Line 213:   // Since the authn token is verified upon connection 
establishment, it's
 :   // necessary to make sure the client does not keep the 
connections to
 :   // corresponding tablet servers open. So, let's restart the 
tablet servers
 :   // since the RPC layer might keep already established 
connections open.
we could set the rpc keepalive timer to short, instead?


Line 229: //ASSERT_OK(scanner.SetFaultTolerant());
?


Line 230: ASSERT_OK(scanner.Open());
this is only really testing the case of the token being expired on the 
beginning of a Scan and not in the middle of one. How can we get coverage of 
the case where it expires mid-scan (ie when we are transitioning to a new 
tablet or continuing an existing scanner)?

similarly I'm not sure I trust that we've covered all of the various master 
lookup paths (eg expired token when trying to lookup tablet locations). I'm 
wondering whether there is any randomized test we can do that will encourage 
more of these potential overlaps of expiration with various client ops.

One idea would be to explicitly inject false "expired". eg start all servers 
with a flag --inject_token_expiration_errors=0.2 which says that the server 
should respond with a token expiration error on 20% of RPCs. Or perhaps inject 
in the RPC client itself if that's more convenient. Then we can be pretty sure 
that we'll get coverage of lots of different cases without being careful about 
maneuvering timings to expire exactly at certain points.


PS2, Line 230: ASSERT_OK(scanner.Open());
 : size_t row_count = 0;
 : while (scanner.HasMoreRows()) {
 :   vector rows;
 :   ASSERT_OK(scanner.NextBatch());
 :   row_count += rows.size();
 : }
 : EXPECT_EQ(2, row_count);
I think client-test-util has some code to CountRowsFromScanner or something


http://gerrit.cloudera.org:8080/#/c/6648/2/src/kudu/rpc/connection.cc
File src/kudu/rpc/connection.cc:

Line 159:   c->call->SetFailed(status, rpc_error.release());
wouldn't this only send the RPC error to the first of the awaiting calls? It's 
possible with multiple threads in the client that you'd have many pending RPC 
calls on a single connection while it's in the process of negotiation.


http://gerrit.cloudera.org:8080/#/c/6648/2/src/kudu/rpc/retriable_rpc.h
File src/kudu/rpc/retriable_rpc.h:

PS2, Line 71: of
or


PS2, Line 92: autentication
typo


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [c++ client] re-acquire authn token if expired

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: [c++ client] re-acquire authn token if expired
..

[c++ client] re-acquire authn token if expired

Updated C++ client to re-acquire authn token if server responds with
FATAL_INVALID_AUTHENTICATION_TOKEN error on connection negotiation.
If that happens while negotiating an connection while performing an
RPC call, the connection being negotiated is closed and the client
re-connects to the cluster to get new authn token.  After successfully
re-acquiring authn token the client retries the RPC call again.

Added corresponding integration test to cover authn token re-acquisition
for various scenarios.

Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
---
M src/kudu/client/batcher.cc
M src/kudu/client/client-internal.cc
M src/kudu/client/meta_cache.cc
M src/kudu/client/scanner-internal.cc
M src/kudu/client/scanner-internal.h
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/authn_token_expire-itest.cc
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/retriable_rpc.h
M src/kudu/rpc/rpc.h
17 files changed, 524 insertions(+), 75 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [c++ client] re-acquire authn token if expired

[Alexey Serbin (Code Review)]

Alexey Serbin has uploaded a new change for review.

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

Change subject: [c++ client] re-acquire authn token if expired
..

[c++ client] re-acquire authn token if expired

Updated C++ client to re-acquire authn token if server responds with
FATAL_INVALID_AUTHENTICATION_TOKEN error on connection negotiation.
If that happens while negotiating an connection while performing an
RPC call, the connection being negotiated is closed and the client
re-connects to the cluster to get new authn token.  After successfully
re-acquiring authn token the client retries the RPC call again.

Added corresponding integration test to cover authn token re-acquisition
for various scenarios.

Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
---
M src/kudu/client/batcher.cc
M src/kudu/client/client-internal.cc
M src/kudu/client/meta_cache.cc
M src/kudu/client/scanner-internal.cc
M src/kudu/client/scanner-internal.h
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/authn_token_expire-itest.cc
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/retriable_rpc.h
M src/kudu/rpc/rpc.h
17 files changed, 524 insertions(+), 75 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin