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/04/05 20:22:45 UTC
[kudu-CR] sentry: generate table privileges
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12941
Change subject: sentry: generate table privileges
......................................................................
sentry: generate table privileges
This patch adds token generation to the authz provider. A follow-up
patch will integrate this functionality with the catalog manager.
Change-Id: Icb6c533a04d2749e0ee68546dac61011432cba2f
---
M src/kudu/master/authz_provider.h
M src/kudu/master/default_authz_provider.h
M src/kudu/master/sentry_authz_provider-test.cc
M src/kudu/master/sentry_authz_provider.cc
M src/kudu/master/sentry_authz_provider.h
5 files changed, 256 insertions(+), 24 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/12941/1
--
To view, visit http://gerrit.cloudera.org:8080/12941
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Icb6c533a04d2749e0ee68546dac61011432cba2f
Gerrit-Change-Number: 12941
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
[kudu-CR] sentry: populate TablePrivilegePBs
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12941 )
Change subject: sentry: populate TablePrivilegePBs
......................................................................
Patch Set 7: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/12941
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icb6c533a04d2749e0ee68546dac61011432cba2f
Gerrit-Change-Number: 12941
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)
Gerrit-Comment-Date: Tue, 09 Apr 2019 23:21:46 +0000
Gerrit-HasComments: No
[kudu-CR] sentry: populate TablePrivilegePBs
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/12941 )
Change subject: sentry: populate TablePrivilegePBs
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/12941
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icb6c533a04d2749e0ee68546dac61011432cba2f
Gerrit-Change-Number: 12941
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-Comment-Date: Tue, 09 Apr 2019 18:50:36 +0000
Gerrit-HasComments: No
[kudu-CR] sentry: populate TablePrivilegePBs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12941 )
Change subject: sentry: populate TablePrivilegePBs
......................................................................
Patch Set 5: -Verified
Rebased
--
To view, visit http://gerrit.cloudera.org:8080/12941
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icb6c533a04d2749e0ee68546dac61011432cba2f
Gerrit-Change-Number: 12941
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-Comment-Date: Tue, 09 Apr 2019 23:16:11 +0000
Gerrit-HasComments: No
[kudu-CR] sentry: generate table privileges
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12941
to look at the new patch set (#2).
Change subject: sentry: generate table privileges
......................................................................
sentry: generate table privileges
This patch adds TablePrivilegePB generation to the authz provider. A
follow-up patch will integrate this functionality with the catalog
manager.
Change-Id: Icb6c533a04d2749e0ee68546dac61011432cba2f
---
M src/kudu/master/authz_provider.h
M src/kudu/master/default_authz_provider.h
M src/kudu/master/sentry_authz_provider-test.cc
M src/kudu/master/sentry_authz_provider.cc
M src/kudu/master/sentry_authz_provider.h
5 files changed, 256 insertions(+), 24 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/12941/2
--
To view, visit http://gerrit.cloudera.org:8080/12941
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icb6c533a04d2749e0ee68546dac61011432cba2f
Gerrit-Change-Number: 12941
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] sentry: populate TablePrivilegePBs
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/12941 )
Change subject: sentry: populate TablePrivilegePBs
......................................................................
Patch Set 7: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/12941
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icb6c533a04d2749e0ee68546dac61011432cba2f
Gerrit-Change-Number: 12941
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)
Gerrit-Comment-Date: Wed, 10 Apr 2019 00:00:56 +0000
Gerrit-HasComments: No
[kudu-CR] sentry: generate TablePrivilegePBs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12941 )
Change subject: sentry: generate TablePrivilegePBs
......................................................................
Patch Set 3:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/12941/2/src/kudu/master/default_authz_provider.h
File src/kudu/master/default_authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/12941/2/src/kudu/master/default_authz_provider.h@62
PS2, Line 62: }
> Don't we need to set pb->table_id also? Given the function signature, I'd e
Done
http://gerrit.cloudera.org:8080/#/c/12941/2/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/12941/2/src/kudu/master/sentry_authz_provider.cc@554
PS2, Line 554: RETURN_NOT_OK(GetSentryPrivileges(SentryAuthorizableScope::TABLE, table_name,
> Should set table_id, right?
Good point, though I don't think the SentryAuthzProvider has any business setting the table ID, given it doesn't operate on table IDs. I'll be enforce that a table ID is set instead.
http://gerrit.cloudera.org:8080/#/c/12941/2/src/kudu/master/sentry_authz_provider.cc@555
PS2, Line 555: user, &privileges));
> Seems like the transformations that happen in the rest of this function cou
I'm not sure that's exactly the case, given we're using a SchemaPB to map column IDs. I.e. tokens filled based on the same authorization metadata may be different upon subsequent calls to this function if the schema has changed.
http://gerrit.cloudera.org:8080/#/c/12941/2/src/kudu/master/sentry_authz_provider.cc@558
PS2, Line 558: for (const auto& privilege : privileges.privileges) {
> This can be constructed once outside the loop.
Done
http://gerrit.cloudera.org:8080/#/c/12941/2/src/kudu/master/sentry_authz_provider.cc@581
PS2, Line 581: }
: } else if (!pb->scan_privilege() &&
: (ContainsKey(privilege.granted_privileges, SentryAction::ALL) ||
: ContainsKey(privilege.granted_privileges, SentryAction::OWNER) ||
: ContainsKey(privilege.granted_privileges, SentryAction::SELECT))) {
: // Pull out any scan privileges at the column scope.
: DCHECK_EQ(SentryAuthorizableScope::COLUMN, privilege.scope);
:
> We can skip this if an earlier privilege set privilege_pb.scan_privilege(),
Done
http://gerrit.cloudera.org:8080/#/c/12941/2/src/kudu/master/sentry_authz_provider.cc@599
PS2, Line 599: }
> Note that this implementation _overwrites_ pb while the default implementat
Done
--
To view, visit http://gerrit.cloudera.org:8080/12941
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icb6c533a04d2749e0ee68546dac61011432cba2f
Gerrit-Change-Number: 12941
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-Comment-Date: Mon, 08 Apr 2019 05:43:38 +0000
Gerrit-HasComments: Yes
[kudu-CR] sentry: generate TablePrivilegePBs
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12941 )
Change subject: sentry: generate TablePrivilegePBs
......................................................................
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/12941/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12941/3//COMMIT_MSG@9
PS3, Line 9: This patch allos the authz provider to generate TablePrivilegePBs, which
allows
http://gerrit.cloudera.org:8080/#/c/12941/3/src/kudu/master/authz_provider.h
File src/kudu/master/authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/12941/3/src/kudu/master/authz_provider.h@87
PS3, Line 87: const SchemaPB& schema_pb,
Not immediately clear why the input is a SchemaPB and not a Schema.
http://gerrit.cloudera.org:8080/#/c/12941/2/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/12941/2/src/kudu/master/sentry_authz_provider.cc@555
PS2, Line 555: user, &privileges));
> I'm not sure that's exactly the case, given we're using a SchemaPB to map c
Fair enough. Maybe add a comment about that "uncacheability" here?
--
To view, visit http://gerrit.cloudera.org:8080/12941
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icb6c533a04d2749e0ee68546dac61011432cba2f
Gerrit-Change-Number: 12941
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-Comment-Date: Mon, 08 Apr 2019 23:41:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] sentry: populate TablePrivilegePBs
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12941 )
Change subject: sentry: populate TablePrivilegePBs
......................................................................
Patch Set 4: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/12941
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icb6c533a04d2749e0ee68546dac61011432cba2f
Gerrit-Change-Number: 12941
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-Comment-Date: Tue, 09 Apr 2019 04:58:20 +0000
Gerrit-HasComments: No
[kudu-CR] sentry: populate TablePrivilegePBs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12941 )
Change subject: sentry: populate TablePrivilegePBs
......................................................................
Patch Set 5: Verified+1
A couple of Java flakies.
--
To view, visit http://gerrit.cloudera.org:8080/12941
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icb6c533a04d2749e0ee68546dac61011432cba2f
Gerrit-Change-Number: 12941
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-Comment-Date: Tue, 09 Apr 2019 17:24:08 +0000
Gerrit-HasComments: No
[kudu-CR] sentry: populate TablePrivilegePBs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12941 )
Change subject: sentry: populate TablePrivilegePBs
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12941/6/src/kudu/master/sentry_authz_provider-test.cc
File src/kudu/master/sentry_authz_provider-test.cc:
http://gerrit.cloudera.org:8080/#/c/12941/6/src/kudu/master/sentry_authz_provider-test.cc@85
PS6, Line 85: using std::unique_ptr;
> warning: using decl 'tuple' is unused [misc-unused-using-decls]
Done
--
To view, visit http://gerrit.cloudera.org:8080/12941
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icb6c533a04d2749e0ee68546dac61011432cba2f
Gerrit-Change-Number: 12941
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)
Gerrit-Comment-Date: Tue, 09 Apr 2019 23:18:10 +0000
Gerrit-HasComments: Yes
[kudu-CR] sentry: populate TablePrivilegePBs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12941 )
Change subject: sentry: populate TablePrivilegePBs
......................................................................
Patch Set 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/12941/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12941/3//COMMIT_MSG@9
PS3, Line 9: This patch allows the authz provider to populate TablePrivilegePBs,
> allows
Done
http://gerrit.cloudera.org:8080/#/c/12941/3/src/kudu/master/authz_provider.h
File src/kudu/master/authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/12941/3/src/kudu/master/authz_provider.h@87
PS3, Line 87: const SchemaPB& schema_pb,
> Not immediately clear why the input is a SchemaPB and not a Schema.
It's a limitation of what's available in the catalog manager. A SchemaPB is available from the TableInfo, not a Schema. We could do some translation, but I don't think it'd be worth it.
I'm hesitant to document this, since it doesn't seem particularly important.
http://gerrit.cloudera.org:8080/#/c/12941/2/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/12941/2/src/kudu/master/sentry_authz_provider.cc@555
PS2, Line 555: // because the column-level privileges depend on the input schema, which may
> Fair enough. Maybe add a comment about that "uncacheability" here?
Done
--
To view, visit http://gerrit.cloudera.org:8080/12941
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icb6c533a04d2749e0ee68546dac61011432cba2f
Gerrit-Change-Number: 12941
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-Comment-Date: Tue, 09 Apr 2019 00:16:33 +0000
Gerrit-HasComments: Yes
[kudu-CR] sentry: generate TablePrivilegePBs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12941
to look at the new patch set (#3).
Change subject: sentry: generate TablePrivilegePBs
......................................................................
sentry: generate TablePrivilegePBs
This patch allos the authz provider to generate TablePrivilegePBs, which
will be used as authorization metadata for authz tokens. A follow-up
patch will integrate this functionality into the GetTableSchema
endpoint.
Change-Id: Icb6c533a04d2749e0ee68546dac61011432cba2f
---
M src/kudu/master/authz_provider.h
M src/kudu/master/default_authz_provider.h
M src/kudu/master/sentry_authz_provider-test.cc
M src/kudu/master/sentry_authz_provider.cc
M src/kudu/master/sentry_authz_provider.h
5 files changed, 268 insertions(+), 24 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/12941/3
--
To view, visit http://gerrit.cloudera.org:8080/12941
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icb6c533a04d2749e0ee68546dac61011432cba2f
Gerrit-Change-Number: 12941
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)
[kudu-CR] sentry: populate TablePrivilegePBs
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12941 )
Change subject: sentry: populate TablePrivilegePBs
......................................................................
Patch Set 5: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12941/4/src/kudu/master/authz_provider.h
File src/kudu/master/authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/12941/4/src/kudu/master/authz_provider.h@88
PS4, Line 88: security::TablePrivilegePB
> I didn't. I guess it's not a bad idea, but I'm kind of against introducing
SGTM
--
To view, visit http://gerrit.cloudera.org:8080/12941
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icb6c533a04d2749e0ee68546dac61011432cba2f
Gerrit-Change-Number: 12941
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-Comment-Date: Tue, 09 Apr 2019 20:55:40 +0000
Gerrit-HasComments: Yes
[kudu-CR] sentry: populate TablePrivilegePBs
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/12941
to look at the new patch set (#7).
Change subject: sentry: populate TablePrivilegePBs
......................................................................
sentry: populate TablePrivilegePBs
This patch allows the authz provider to populate TablePrivilegePBs,
which will be used as authorization metadata for authz tokens. A
follow-up patch will integrate this functionality into the
GetTableSchema endpoint.
Change-Id: Icb6c533a04d2749e0ee68546dac61011432cba2f
---
M src/kudu/master/authz_provider.h
M src/kudu/master/default_authz_provider.h
M src/kudu/master/sentry_authz_provider-test.cc
M src/kudu/master/sentry_authz_provider.cc
M src/kudu/master/sentry_authz_provider.h
5 files changed, 271 insertions(+), 25 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/12941/7
--
To view, visit http://gerrit.cloudera.org:8080/12941
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icb6c533a04d2749e0ee68546dac61011432cba2f
Gerrit-Change-Number: 12941
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] sentry: populate TablePrivilegePBs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12941
to look at the new patch set (#5).
Change subject: sentry: populate TablePrivilegePBs
......................................................................
sentry: populate TablePrivilegePBs
This patch allows the authz provider to populate TablePrivilegePBs,
which will be used as authorization metadata for authz tokens. A
follow-up patch will integrate this functionality into the
GetTableSchema endpoint.
Change-Id: Icb6c533a04d2749e0ee68546dac61011432cba2f
---
M src/kudu/master/authz_provider.h
M src/kudu/master/default_authz_provider.h
M src/kudu/master/sentry_authz_provider-test.cc
M src/kudu/master/sentry_authz_provider.cc
M src/kudu/master/sentry_authz_provider.h
5 files changed, 271 insertions(+), 24 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/12941/5
--
To view, visit http://gerrit.cloudera.org:8080/12941
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icb6c533a04d2749e0ee68546dac61011432cba2f
Gerrit-Change-Number: 12941
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)
[kudu-CR] sentry: populate TablePrivilegePBs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12941
to look at the new patch set (#4).
Change subject: sentry: populate TablePrivilegePBs
......................................................................
sentry: populate TablePrivilegePBs
This patch allows the authz provider to populate TablePrivilegePBs,
which will be used as authorization metadata for authz tokens. A
follow-up patch will integrate this functionality into the
GetTableSchema endpoint.
Change-Id: Icb6c533a04d2749e0ee68546dac61011432cba2f
---
M src/kudu/master/authz_provider.h
M src/kudu/master/default_authz_provider.h
M src/kudu/master/sentry_authz_provider-test.cc
M src/kudu/master/sentry_authz_provider.cc
M src/kudu/master/sentry_authz_provider.h
5 files changed, 272 insertions(+), 24 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/12941/4
--
To view, visit http://gerrit.cloudera.org:8080/12941
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icb6c533a04d2749e0ee68546dac61011432cba2f
Gerrit-Change-Number: 12941
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)
[kudu-CR] sentry: populate TablePrivilegePBs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12941 )
Change subject: sentry: populate TablePrivilegePBs
......................................................................
Patch Set 5:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/12941/4/src/kudu/master/authz_provider.h
File src/kudu/master/authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/12941/4/src/kudu/master/authz_provider.h@88
PS4, Line 88: security::TablePrivilegePB
> Did you consider other ways of passing information about table privileges t
I didn't. I guess it's not a bad idea, but I'm kind of against introducing yet another "privilege" object (eg we already have AuthorizablePrivilege, SentryPrivilegeBranch, TablePrivilegePB), especially when this one fits the bill well enough.
http://gerrit.cloudera.org:8080/#/c/12941/4/src/kudu/master/sentry_authz_provider-test.cc
File src/kudu/master/sentry_authz_provider-test.cc:
http://gerrit.cloudera.org:8080/#/c/12941/4/src/kudu/master/sentry_authz_provider-test.cc@340
PS4, Line 340: SentryAuthzProviderFilterPrivilegesTest
> I start to seeing timeout for /sentry_authz_provider-test.cc, maybe conside
https://gerrit.cloudera.org/c/12966/1
http://gerrit.cloudera.org:8080/#/c/12941/4/src/kudu/master/sentry_authz_provider-test.cc@341
PS4, Line 341: const
> nit: maybe add constexpr for extra sugar?
Done
http://gerrit.cloudera.org:8080/#/c/12941/4/src/kudu/master/sentry_authz_provider-test.cc@386
PS4, Line 386: the column sco
> 'the column scope' ?
Done
http://gerrit.cloudera.org:8080/#/c/12941/4/src/kudu/master/sentry_authz_provider-test.cc@439
PS4, Line 439: te("$0
> If not excluding some fields from comparison, why not to use Equals()/Equiv
Done
http://gerrit.cloudera.org:8080/#/c/12941/4/src/kudu/master/sentry_authz_provider-test.cc@440
PS4, Line 440: }
> nit: should the indent be 4 here?
Done
--
To view, visit http://gerrit.cloudera.org:8080/12941
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icb6c533a04d2749e0ee68546dac61011432cba2f
Gerrit-Change-Number: 12941
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-Comment-Date: Tue, 09 Apr 2019 07:45:04 +0000
Gerrit-HasComments: Yes
[kudu-CR] sentry: populate TablePrivilegePBs
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12941 )
Change subject: sentry: populate TablePrivilegePBs
......................................................................
Patch Set 4:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/12941/4/src/kudu/master/authz_provider.h
File src/kudu/master/authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/12941/4/src/kudu/master/authz_provider.h@88
PS4, Line 88: security::TablePrivilegePB
Did you consider other ways of passing information about table privileges that would not include table_id and other irrelevant to this interface field? I.e., the idea is to use some other structure to pass that information around in this API.
http://gerrit.cloudera.org:8080/#/c/12941/4/src/kudu/master/sentry_authz_provider-test.cc
File src/kudu/master/sentry_authz_provider-test.cc:
http://gerrit.cloudera.org:8080/#/c/12941/4/src/kudu/master/sentry_authz_provider-test.cc@341
PS4, Line 341: const
nit: maybe add constexpr for extra sugar?
http://gerrit.cloudera.org:8080/#/c/12941/4/src/kudu/master/sentry_authz_provider-test.cc@386
PS4, Line 386: a scope column
'the column scope' ?
http://gerrit.cloudera.org:8080/#/c/12941/4/src/kudu/master/sentry_authz_provider-test.cc@439
PS4, Line 439: Compare
If not excluding some fields from comparison, why not to use Equals()/Equivalent() ?
--
To view, visit http://gerrit.cloudera.org:8080/12941
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icb6c533a04d2749e0ee68546dac61011432cba2f
Gerrit-Change-Number: 12941
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-Comment-Date: Tue, 09 Apr 2019 00:45:14 +0000
Gerrit-HasComments: Yes
[kudu-CR] sentry: generate table privileges
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12941 )
Change subject: sentry: generate table privileges
......................................................................
Patch Set 2:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/12941/2/src/kudu/master/default_authz_provider.h
File src/kudu/master/default_authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/12941/2/src/kudu/master/default_authz_provider.h@62
PS2, Line 62: Status GetTablePrivilegePB(const std::string& /*table_name*/,
Don't we need to set pb->table_id also? Given the function signature, I'd expect it to write out 'pb' from scratch; if that's not the case, the contract should be clarified in a comment.
http://gerrit.cloudera.org:8080/#/c/12941/2/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/12941/2/src/kudu/master/sentry_authz_provider.cc@554
PS2, Line 554: TablePrivilegePB privilege_pb;
Should set table_id, right?
http://gerrit.cloudera.org:8080/#/c/12941/2/src/kudu/master/sentry_authz_provider.cc@555
PS2, Line 555: unordered_set<string> scannable_col_names;
Seems like the transformations that happen in the rest of this function could happen in GetSentryPrivileges and thus be cached. What's the rationale for doing them for every token retrieval vs. doing them earlier and benefitting from caching?
http://gerrit.cloudera.org:8080/#/c/12941/2/src/kudu/master/sentry_authz_provider.cc@558
PS2, Line 558: SentryAuthorizableScope(SentryAuthorizableScope::TABLE))) {
This can be constructed once outside the loop.
http://gerrit.cloudera.org:8080/#/c/12941/2/src/kudu/master/sentry_authz_provider.cc@581
PS2, Line 581: else if (ContainsKey(privilege.granted_privileges, SentryAction::ALL) ||
: ContainsKey(privilege.granted_privileges, SentryAction::OWNER) ||
: ContainsKey(privilege.granted_privileges, SentryAction::SELECT)) {
: // Pull out any scan privileges at the column scope.
: DCHECK_EQ(SentryAuthorizableScope::COLUMN, privilege.scope);
: DCHECK(!privilege.column_name.empty());
: EmplaceIfNotPresent(&scannable_col_names, privilege.column_name);
: }
We can skip this if an earlier privilege set privilege_pb.scan_privilege(), right?
http://gerrit.cloudera.org:8080/#/c/12941/2/src/kudu/master/sentry_authz_provider.cc@599
PS2, Line 599: *pb = std::move(privilege_pb);
Note that this implementation _overwrites_ pb while the default implementation modifies it in situ. Could you make them consistent and clarify the intended behavior in the contract?
--
To view, visit http://gerrit.cloudera.org:8080/12941
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icb6c533a04d2749e0ee68546dac61011432cba2f
Gerrit-Change-Number: 12941
Gerrit-PatchSet: 2
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-Comment-Date: Mon, 08 Apr 2019 04:56:38 +0000
Gerrit-HasComments: Yes
[kudu-CR] sentry: populate TablePrivilegePBs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12941 )
Change subject: sentry: populate TablePrivilegePBs
......................................................................
sentry: populate TablePrivilegePBs
This patch allows the authz provider to populate TablePrivilegePBs,
which will be used as authorization metadata for authz tokens. A
follow-up patch will integrate this functionality into the
GetTableSchema endpoint.
Change-Id: Icb6c533a04d2749e0ee68546dac61011432cba2f
Reviewed-on: http://gerrit.cloudera.org:8080/12941
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
Reviewed-by: Hao Hao <ha...@cloudera.com>
---
M src/kudu/master/authz_provider.h
M src/kudu/master/default_authz_provider.h
M src/kudu/master/sentry_authz_provider-test.cc
M src/kudu/master/sentry_authz_provider.cc
M src/kudu/master/sentry_authz_provider.h
5 files changed, 271 insertions(+), 25 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/12941
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Icb6c533a04d2749e0ee68546dac61011432cba2f
Gerrit-Change-Number: 12941
Gerrit-PatchSet: 8
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] sentry: populate TablePrivilegePBs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12941
to look at the new patch set (#6).
Change subject: sentry: populate TablePrivilegePBs
......................................................................
sentry: populate TablePrivilegePBs
This patch allows the authz provider to populate TablePrivilegePBs,
which will be used as authorization metadata for authz tokens. A
follow-up patch will integrate this functionality into the
GetTableSchema endpoint.
Change-Id: Icb6c533a04d2749e0ee68546dac61011432cba2f
---
M src/kudu/master/authz_provider.h
M src/kudu/master/default_authz_provider.h
M src/kudu/master/sentry_authz_provider-test.cc
M src/kudu/master/sentry_authz_provider.cc
M src/kudu/master/sentry_authz_provider.h
5 files changed, 271 insertions(+), 24 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/12941/6
--
To view, visit http://gerrit.cloudera.org:8080/12941
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icb6c533a04d2749e0ee68546dac61011432cba2f
Gerrit-Change-Number: 12941
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)
[kudu-CR] sentry: populate TablePrivilegePBs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has removed a vote on this change.
Change subject: sentry: populate TablePrivilegePBs
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/12941
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: Icb6c533a04d2749e0ee68546dac61011432cba2f
Gerrit-Change-Number: 12941
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)
[kudu-CR] sentry: populate TablePrivilegePBs
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/12941 )
Change subject: sentry: populate TablePrivilegePBs
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/12941/4/src/kudu/master/sentry_authz_provider-test.cc
File src/kudu/master/sentry_authz_provider-test.cc:
http://gerrit.cloudera.org:8080/#/c/12941/4/src/kudu/master/sentry_authz_provider-test.cc@340
PS4, Line 340: SentryAuthzProviderFilterPrivilegesTest
I start to seeing timeout for /sentry_authz_provider-test.cc, maybe consider to shard the tests?
http://gerrit.cloudera.org:8080/#/c/12941/4/src/kudu/master/sentry_authz_provider-test.cc@440
PS4, Line 440: << Substitute("$0 vs $1", SecureDebugString(expected_pb), SecureDebugString(privilege_pb));
nit: should the indent be 4 here?
--
To view, visit http://gerrit.cloudera.org:8080/12941
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icb6c533a04d2749e0ee68546dac61011432cba2f
Gerrit-Change-Number: 12941
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-Comment-Date: Tue, 09 Apr 2019 05:27:28 +0000
Gerrit-HasComments: Yes