Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12122 )
Change subject: KUDU-2543 pt 2: pass around default authz tokens ...................................................................... Patch Set 6: (13 comments) http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/client/authz_token_cache.h File src/kudu/client/authz_token_cache.h: http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/client/authz_token_cache.h@134 PS5, Line 134: authz_rpcs_; > nit: why not name it 'authz_rpcs_' to match 'authz_tokens_'? Done http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/client/client-internal.cc File src/kudu/client/client-internal.cc: http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/client/client-internal.cc@466 PS5, Line 466: Parse the server part > nit: use StoreAuthzToken() instead? Done http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/client/client-test.cc@5853 PS5, Line 5853: client_->data_->RetrieveAuthzTokenAsync(client_table_.get(), s.AsStatusCallback(), > Is it possible to change the logic to create a new client and only retrieve That's possible, we could also not run RetrieveAuthzTokenAsync if there is a token in the cache. I'm going to hold off on it though since I don't think that adds to the test coverage and would make this test more complicated. http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/integration-tests/auth_token_expire-itest.cc File src/kudu/integration-tests/auth_token_expire-itest.cc: http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/integration-tests/auth_token_expire-itest.cc@143 PS5, Line 143: i > nit: extra space. Done http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/integration-tests/auth_token_expire-itest.cc@254 PS5, Line 254: p > nit: space. Done http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/integration-tests/authz_token-itest.cc File src/kudu/integration-tests/authz_token-itest.cc: http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/integration-tests/authz_token-itest.cc@184 PS5, Line 184: Insert > nit: 'Inserts' to aligned with other comments style. Done http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/integration-tests/authz_token-itest.cc@205 PS5, Line 205: Get > nit: 'Gets' Done http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/integration-tests/authz_token-itest.cc@281 PS5, Line 281: > Can you add a comment to explain what is the expected behavior in this case Done http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/integration-tests/authz_token-itest.cc@352 PS5, Line 352: auto& e : er > Nit: got two spaces here. Done http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/integration-tests/authz_token-itest.cc@362 PS5, Line 362: > Why not parameterized the functor here as well? Done http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/integration-tests/authz_token-itest.cc@369 PS5, Line 369: yPB tsk; > Should a similar test be exist for authn token as well? A similar test exists for authn tokens in security-unknown-tsk-itest. I chose to put this test here because the logic differed enough that parameterizing security-unknown-tsk-itest would have been a lot of work and I think would've made the test harder to follow. http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/integration-tests/authz_token-itest.cc@477 PS5, Line 477: token_ratio = 1.0; > Will a similar also apply for authn tokens? Also, wondering how you consid auth_token_expire-itest has some tests that explicitly test for expired tokens / token reacquisition during master leader changes (e.g. MultiMasterIdleConnectionsITest), which is similar to this (invalid tokens during master election storms). I opted to not reuse some of the existing tests because parameterizing them would be non-trivial and make them harder to understand (and I found some tests a bit confusing to start with, given a few of them are targeting very specific error scenarios). auth_token_expire-itest tests generally target expiration of tokens. authz_token-itest targets other error scenarios. http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/integration-tests/authz_token-itest.cc@523 PS5, Line 523: // Client should have no problems connecting to an old cluster. : ASSERT_OK(Inse > What happens if a old client try to communicate with servers that require a That'd be the same as sending requests with no authz tokens, so the client would get an error. That's harder to test, since I'm not maintaining the old behavior of the client through some config flag, but https://gerrit.cloudera.org/c/11751/10/src/kudu/tserver/tablet_server_authorization-test.cc#259 shows the behavior of using the proxy with no token. -- To view, visit http://gerrit.cloudera.org:8080/12122 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7971d652d6adc822167cf959bffd5f994a7ca565 Gerrit-Change-Number: 12122 Gerrit-PatchSet: 6 Gerrit-Owner: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 23 Jan 2019 01:29:47 +0000 Gerrit-HasComments: Yes
