You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Andrew Wong (Code Review)" <ge...@cloudera.org> on 2019/06/08 08:30:17 UTC
[kudu-CR] authz: refactor authorization for ListTables
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13566
Change subject: authz: refactor authorization for ListTables
......................................................................
authz: refactor authorization for ListTables
Authorization for ListTables isn't very performant because it requires
authorizing each table while holding its table lock. This invariant is
held to guarantee that the authorization applies to the correct table in
Kudu (e.g. even amidst a concurrent rename).
If we authorize multiple tables at once, we can improve performance, but
keeping the guarantee is less straightforward. This patch takes a stab
at this by refactoring ListTables to prepare for the following
authorization sequence:
1. With table locks held, put together maps from
{ table_name => TableInfo } and { table_name => table_id }.
2. Authorize the tables (potentially in bulk in the future), keeping
track of the authorized table names.
3. Iterate through the authorized table names and, using the maps from
Step 1, with table locks held, check that the table name hasn't
changed. If it has, this implies there was a concurrent rename (or
equivalent) and the authorization call was not actually made for this
table, and it shouldn't be returned.
4. With confirmation that the tables that were authorized have the IDs
we expect, return the authorized tables.
If the catalog manager isn't set up to authorize tables (e.g.
authorization isn't enabled), steps 2 and 3 are skipped. This patch
refactors ListTables to match this sequence of events without bulk
authorization.
While this is mainly a refactor, the user-facing change is that if there
is a concurrent rename during a ListTables operation, there is a chance
that neither the old nor new table will show up in the listed tables.
This isn't ideal UX, but is conservative w.r.t security. A test is added
exercising this scenario.
Change-Id: I568e1be7b909768a99f0f4f13775e34a01ccd160
---
M src/kudu/integration-tests/master_sentry-itest.cc
M src/kudu/master/authz_provider.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/default_authz_provider.h
M src/kudu/master/sentry_authz_provider.cc
M src/kudu/master/sentry_authz_provider.h
7 files changed, 137 insertions(+), 19 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/13566/1
--
To view, visit http://gerrit.cloudera.org:8080/13566
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I568e1be7b909768a99f0f4f13775e34a01ccd160
Gerrit-Change-Number: 13566
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
[kudu-CR] authz: refactor authorization for ListTables
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13566 )
Change subject: authz: refactor authorization for ListTables
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13566/4/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/13566/4/src/kudu/master/catalog_manager.cc@2843
PS4, Line 2843: // Finally, fill the response with the table IDs and names that were
: // authorized.
How about just doing this on L2833/L2838 instead of intermediating through yet another collection?
http://gerrit.cloudera.org:8080/#/c/13566/4/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/13566/4/src/kudu/master/sentry_authz_provider.cc@197
PS4, Line 197: *checked_table_names = true;
If table_names was empty, should this still be true?
--
To view, visit http://gerrit.cloudera.org:8080/13566
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I568e1be7b909768a99f0f4f13775e34a01ccd160
Gerrit-Change-Number: 13566
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 10 Jun 2019 04:18:15 +0000
Gerrit-HasComments: Yes
[kudu-CR] authz: refactor authorization for ListTables
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13566 )
Change subject: authz: refactor authorization for ListTables
......................................................................
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13566/4/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/13566/4/src/kudu/master/catalog_manager.cc@2843
PS4, Line 2843: }
: return Status:
> How about just doing this on L2833/L2838 instead of intermediating through
Done
http://gerrit.cloudera.org:8080/#/c/13566/4/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/13566/4/src/kudu/master/sentry_authz_provider.cc@197
PS4, Line 197: *checked_table_names = true;
> If table_names was empty, should this still be true?
It doesn't affect the logic either way, but conceptually yes because the (albeit empty list of) tables that were passed in were checked.
--
To view, visit http://gerrit.cloudera.org:8080/13566
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I568e1be7b909768a99f0f4f13775e34a01ccd160
Gerrit-Change-Number: 13566
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 10 Jun 2019 05:48:41 +0000
Gerrit-HasComments: Yes
[kudu-CR] authz: refactor authorization for ListTables
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/13566 )
Change subject: authz: refactor authorization for ListTables
......................................................................
Patch Set 6: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/13566
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I568e1be7b909768a99f0f4f13775e34a01ccd160
Gerrit-Change-Number: 13566
Gerrit-PatchSet: 6
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 10 Jun 2019 19:15:43 +0000
Gerrit-HasComments: No
[kudu-CR] authz: refactor authorization for ListTables
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13566 )
Change subject: authz: refactor authorization for ListTables
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13566/5/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/13566/5/src/kudu/master/catalog_manager.cc@2840
PS5, Line 2840: table->set_id(FindOrDie(table_info_by_name, table_name)->id());
Maybe just iterate over table_info_by_name instead of table_names? The map's keys won't reflect any concurrent rename so you should be safe there.
--
To view, visit http://gerrit.cloudera.org:8080/13566
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I568e1be7b909768a99f0f4f13775e34a01ccd160
Gerrit-Change-Number: 13566
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 10 Jun 2019 16:20:08 +0000
Gerrit-HasComments: Yes
[kudu-CR] authz: refactor authorization for ListTables
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13566
to look at the new patch set (#5).
Change subject: authz: refactor authorization for ListTables
......................................................................
authz: refactor authorization for ListTables
Authorization for ListTables isn't very performant because it requires
authorizing each table while holding its table lock. This invariant is
held to guarantee that the authorization applies to the correct table in
Kudu (e.g. even amidst a concurrent rename).
If we authorize multiple tables at once, we can improve performance, but
keeping the guarantee is less straightforward. This patch takes a stab
at this by refactoring ListTables to prepare for the following
authorization sequence:
1. With table locks held, put together a map from
{ table_name => TableInfo }.
2. Authorize the tables (potentially in bulk in the future), keeping
track of the authorized table names.
3. Iterate through the authorized table names and, using the maps from
Step 1, with table locks held, check that the table name hasn't
changed. If it has, this implies there was a concurrent rename (or
equivalent) and the authorization call was not actually made for this
table, and it shouldn't be returned.
4. With confirmation that the tables that were authorized have the IDs
we expect, return the authorized tables.
If the catalog manager isn't set up to authorize tables (e.g.
authorization isn't enabled), steps 2 and 3 are skipped. This patch
refactors ListTables to match this sequence of events without bulk
authorization.
While this is mainly a refactor, the user-facing change is that if there
is a concurrent rename during a ListTables operation, there is a chance
that neither the old nor new table will show up in the listed tables.
This isn't ideal UX, but is conservative w.r.t security. A test is added
exercising this scenario.
Change-Id: I568e1be7b909768a99f0f4f13775e34a01ccd160
---
M src/kudu/integration-tests/master_sentry-itest.cc
M src/kudu/master/authz_provider.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/default_authz_provider.h
M src/kudu/master/sentry_authz_provider.cc
M src/kudu/master/sentry_authz_provider.h
7 files changed, 111 insertions(+), 19 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/13566/5
--
To view, visit http://gerrit.cloudera.org:8080/13566
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I568e1be7b909768a99f0f4f13775e34a01ccd160
Gerrit-Change-Number: 13566
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] authz: refactor authorization for ListTables
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13566
to look at the new patch set (#3).
Change subject: authz: refactor authorization for ListTables
......................................................................
authz: refactor authorization for ListTables
Authorization for ListTables isn't very performant because it requires
authorizing each table while holding its table lock. This invariant is
held to guarantee that the authorization applies to the correct table in
Kudu (e.g. even amidst a concurrent rename).
If we authorize multiple tables at once, we can improve performance, but
keeping the guarantee is less straightforward. This patch takes a stab
at this by refactoring ListTables to prepare for the following
authorization sequence:
1. With table locks held, put together maps from
{ table_name => TableInfo } and { table_name => table_id }.
2. Authorize the tables (potentially in bulk in the future), keeping
track of the authorized table names.
3. Iterate through the authorized table names and, using the maps from
Step 1, with table locks held, check that the table name hasn't
changed. If it has, this implies there was a concurrent rename (or
equivalent) and the authorization call was not actually made for this
table, and it shouldn't be returned.
4. With confirmation that the tables that were authorized have the IDs
we expect, return the authorized tables.
If the catalog manager isn't set up to authorize tables (e.g.
authorization isn't enabled), steps 2 and 3 are skipped. This patch
refactors ListTables to match this sequence of events without bulk
authorization.
While this is mainly a refactor, the user-facing change is that if there
is a concurrent rename during a ListTables operation, there is a chance
that neither the old nor new table will show up in the listed tables.
This isn't ideal UX, but is conservative w.r.t security. A test is added
exercising this scenario.
Change-Id: I568e1be7b909768a99f0f4f13775e34a01ccd160
---
M src/kudu/integration-tests/master_sentry-itest.cc
M src/kudu/master/authz_provider.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/default_authz_provider.h
M src/kudu/master/sentry_authz_provider.cc
M src/kudu/master/sentry_authz_provider.h
7 files changed, 117 insertions(+), 19 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/13566/3
--
To view, visit http://gerrit.cloudera.org:8080/13566
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I568e1be7b909768a99f0f4f13775e34a01ccd160
Gerrit-Change-Number: 13566
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] authz: refactor authorization for ListTables
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13566 )
Change subject: authz: refactor authorization for ListTables
......................................................................
Patch Set 6: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/13566
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I568e1be7b909768a99f0f4f13775e34a01ccd160
Gerrit-Change-Number: 13566
Gerrit-PatchSet: 6
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 10 Jun 2019 18:17:31 +0000
Gerrit-HasComments: No
[kudu-CR] authz: refactor authorization for ListTables
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13566
to look at the new patch set (#2).
Change subject: authz: refactor authorization for ListTables
......................................................................
authz: refactor authorization for ListTables
Authorization for ListTables isn't very performant because it requires
authorizing each table while holding its table lock. This invariant is
held to guarantee that the authorization applies to the correct table in
Kudu (e.g. even amidst a concurrent rename).
If we authorize multiple tables at once, we can improve performance, but
keeping the guarantee is less straightforward. This patch takes a stab
at this by refactoring ListTables to prepare for the following
authorization sequence:
1. With table locks held, put together maps from
{ table_name => TableInfo } and { table_name => table_id }.
2. Authorize the tables (potentially in bulk in the future), keeping
track of the authorized table names.
3. Iterate through the authorized table names and, using the maps from
Step 1, with table locks held, check that the table name hasn't
changed. If it has, this implies there was a concurrent rename (or
equivalent) and the authorization call was not actually made for this
table, and it shouldn't be returned.
4. With confirmation that the tables that were authorized have the IDs
we expect, return the authorized tables.
If the catalog manager isn't set up to authorize tables (e.g.
authorization isn't enabled), steps 2 and 3 are skipped. This patch
refactors ListTables to match this sequence of events without bulk
authorization.
While this is mainly a refactor, the user-facing change is that if there
is a concurrent rename during a ListTables operation, there is a chance
that neither the old nor new table will show up in the listed tables.
This isn't ideal UX, but is conservative w.r.t security. A test is added
exercising this scenario.
Change-Id: I568e1be7b909768a99f0f4f13775e34a01ccd160
---
M src/kudu/integration-tests/master_sentry-itest.cc
M src/kudu/master/authz_provider.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/default_authz_provider.h
M src/kudu/master/sentry_authz_provider.cc
M src/kudu/master/sentry_authz_provider.h
7 files changed, 118 insertions(+), 19 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/13566/2
--
To view, visit http://gerrit.cloudera.org:8080/13566
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I568e1be7b909768a99f0f4f13775e34a01ccd160
Gerrit-Change-Number: 13566
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] authz: refactor authorization for ListTables
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13566 )
Change subject: authz: refactor authorization for ListTables
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/13566/1/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/13566/1/src/kudu/master/sentry_authz_provider.cc@52
PS1, Line 52: // Whether the given privileges 'privileges_branch' allows for the specified
> warning: using decl 'map' is unused [misc-unused-using-decls]
Done
http://gerrit.cloudera.org:8080/#/c/13566/1/src/kudu/master/sentry_authz_provider.cc@54
PS1, Line 54: // with GRANT ALL option, if any required ('requires_all_with_grant').
> warning: using decl 'unordered_map' is unused [misc-unused-using-decls]
Done
http://gerrit.cloudera.org:8080/#/c/13566/1/src/kudu/master/sentry_authz_provider.cc@56
PS1, Line 56: SentryAuthorizableScope::Scope required_scope,
> warning: using decl 'vector' is unused [misc-unused-using-decls]
Done
--
To view, visit http://gerrit.cloudera.org:8080/13566
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I568e1be7b909768a99f0f4f13775e34a01ccd160
Gerrit-Change-Number: 13566
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Sat, 08 Jun 2019 08:43:33 +0000
Gerrit-HasComments: Yes
[kudu-CR] authz: refactor authorization for ListTables
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/13566 )
Change subject: authz: refactor authorization for ListTables
......................................................................
Patch Set 5: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13566/5/src/kudu/integration-tests/master_sentry-itest.cc
File src/kudu/integration-tests/master_sentry-itest.cc:
http://gerrit.cloudera.org:8080/#/c/13566/5/src/kudu/integration-tests/master_sentry-itest.cc@470
PS5, Line 470: Substitute("$0.$1", kDatabaseName, kTableName)
nit: you can use 'table_name' here.
--
To view, visit http://gerrit.cloudera.org:8080/13566
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I568e1be7b909768a99f0f4f13775e34a01ccd160
Gerrit-Change-Number: 13566
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 10 Jun 2019 17:19:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] authz: refactor authorization for ListTables
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13566
to look at the new patch set (#6).
Change subject: authz: refactor authorization for ListTables
......................................................................
authz: refactor authorization for ListTables
Authorization for ListTables isn't very performant because it requires
authorizing each table while holding its table lock. This invariant is
held to guarantee that the authorization applies to the correct table in
Kudu (e.g. even amidst a concurrent rename).
If we authorize multiple tables at once, we can improve performance, but
keeping the guarantee is less straightforward. This patch takes a stab
at this by refactoring ListTables to prepare for the following
authorization sequence:
1. With table locks held, put together a map from
{ table_name => TableInfo }.
2. Authorize the tables (potentially in bulk in the future), keeping
track of the authorized table names.
3. Iterate through the authorized table names and, using the maps from
Step 1, with table locks held, check that the table name hasn't
changed. If it has, this implies there was a concurrent rename (or
equivalent) and the authorization call was not actually made for this
table, and it shouldn't be returned.
4. With confirmation that the tables that were authorized have the IDs
we expect, return the authorized tables.
If the catalog manager isn't set up to authorize tables (e.g.
authorization isn't enabled), steps 2 and 3 are skipped. This patch
refactors ListTables to match this sequence of events without bulk
authorization.
While this is mainly a refactor, the user-facing change is that if there
is a concurrent rename during a ListTables operation, there is a chance
that neither the old nor new table will show up in the listed tables.
This isn't ideal UX, but is conservative w.r.t security. A test is added
exercising this scenario.
Change-Id: I568e1be7b909768a99f0f4f13775e34a01ccd160
---
M src/kudu/integration-tests/master_sentry-itest.cc
M src/kudu/master/authz_provider.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/default_authz_provider.h
M src/kudu/master/sentry_authz_provider.cc
M src/kudu/master/sentry_authz_provider.h
7 files changed, 112 insertions(+), 19 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/13566/6
--
To view, visit http://gerrit.cloudera.org:8080/13566
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I568e1be7b909768a99f0f4f13775e34a01ccd160
Gerrit-Change-Number: 13566
Gerrit-PatchSet: 6
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] authz: refactor authorization for ListTables
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13566 )
Change subject: authz: refactor authorization for ListTables
......................................................................
authz: refactor authorization for ListTables
Authorization for ListTables isn't very performant because it requires
authorizing each table while holding its table lock. This invariant is
held to guarantee that the authorization applies to the correct table in
Kudu (e.g. even amidst a concurrent rename).
If we authorize multiple tables at once, we can improve performance, but
keeping the guarantee is less straightforward. This patch takes a stab
at this by refactoring ListTables to prepare for the following
authorization sequence:
1. With table locks held, put together a map from
{ table_name => TableInfo }.
2. Authorize the tables (potentially in bulk in the future), keeping
track of the authorized table names.
3. Iterate through the authorized table names and, using the maps from
Step 1, with table locks held, check that the table name hasn't
changed. If it has, this implies there was a concurrent rename (or
equivalent) and the authorization call was not actually made for this
table, and it shouldn't be returned.
4. With confirmation that the tables that were authorized have the IDs
we expect, return the authorized tables.
If the catalog manager isn't set up to authorize tables (e.g.
authorization isn't enabled), steps 2 and 3 are skipped. This patch
refactors ListTables to match this sequence of events without bulk
authorization.
While this is mainly a refactor, the user-facing change is that if there
is a concurrent rename during a ListTables operation, there is a chance
that neither the old nor new table will show up in the listed tables.
This isn't ideal UX, but is conservative w.r.t security. A test is added
exercising this scenario.
Change-Id: I568e1be7b909768a99f0f4f13775e34a01ccd160
Reviewed-on: http://gerrit.cloudera.org:8080/13566
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
Reviewed-by: Hao Hao <ha...@cloudera.com>
---
M src/kudu/integration-tests/master_sentry-itest.cc
M src/kudu/master/authz_provider.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/default_authz_provider.h
M src/kudu/master/sentry_authz_provider.cc
M src/kudu/master/sentry_authz_provider.h
7 files changed, 112 insertions(+), 19 deletions(-)
Approvals:
Adar Dembo: Looks good to me, but someone else must approve
Kudu Jenkins: Verified
Hao Hao: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/13566
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I568e1be7b909768a99f0f4f13775e34a01ccd160
Gerrit-Change-Number: 13566
Gerrit-PatchSet: 7
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] authz: refactor authorization for ListTables
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13566 )
Change subject: authz: refactor authorization for ListTables
......................................................................
Patch Set 6:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13566/5/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/13566/5/src/kudu/master/catalog_manager.cc@2840
PS5, Line 2840: const auto& table_info = name_and_table_info.second;
> Maybe just iterate over table_info_by_name instead of table_names? The map'
Done
http://gerrit.cloudera.org:8080/#/c/13566/5/src/kudu/master/catalog_manager.cc@2840
PS5, Line 2840: const auto& table_info = name_and_table_info.second;
> Maybe just iterate over table_info_by_name instead of table_names? The map'
Good point, done.
--
To view, visit http://gerrit.cloudera.org:8080/13566
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I568e1be7b909768a99f0f4f13775e34a01ccd160
Gerrit-Change-Number: 13566
Gerrit-PatchSet: 6
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 10 Jun 2019 17:51:34 +0000
Gerrit-HasComments: Yes
[kudu-CR] authz: refactor authorization for ListTables
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13566
to look at the new patch set (#4).
Change subject: authz: refactor authorization for ListTables
......................................................................
authz: refactor authorization for ListTables
Authorization for ListTables isn't very performant because it requires
authorizing each table while holding its table lock. This invariant is
held to guarantee that the authorization applies to the correct table in
Kudu (e.g. even amidst a concurrent rename).
If we authorize multiple tables at once, we can improve performance, but
keeping the guarantee is less straightforward. This patch takes a stab
at this by refactoring ListTables to prepare for the following
authorization sequence:
1. With table locks held, put together a map from
{ table_name => TableInfo }.
2. Authorize the tables (potentially in bulk in the future), keeping
track of the authorized table names.
3. Iterate through the authorized table names and, using the maps from
Step 1, with table locks held, check that the table name hasn't
changed. If it has, this implies there was a concurrent rename (or
equivalent) and the authorization call was not actually made for this
table, and it shouldn't be returned.
4. With confirmation that the tables that were authorized have the IDs
we expect, return the authorized tables.
If the catalog manager isn't set up to authorize tables (e.g.
authorization isn't enabled), steps 2 and 3 are skipped. This patch
refactors ListTables to match this sequence of events without bulk
authorization.
While this is mainly a refactor, the user-facing change is that if there
is a concurrent rename during a ListTables operation, there is a chance
that neither the old nor new table will show up in the listed tables.
This isn't ideal UX, but is conservative w.r.t security. A test is added
exercising this scenario.
Change-Id: I568e1be7b909768a99f0f4f13775e34a01ccd160
---
M src/kudu/integration-tests/master_sentry-itest.cc
M src/kudu/master/authz_provider.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/default_authz_provider.h
M src/kudu/master/sentry_authz_provider.cc
M src/kudu/master/sentry_authz_provider.h
7 files changed, 117 insertions(+), 19 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/13566/4
--
To view, visit http://gerrit.cloudera.org:8080/13566
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I568e1be7b909768a99f0f4f13775e34a01ccd160
Gerrit-Change-Number: 13566
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)