Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql: fix crdb_internal.{leases,node_runtime_info} when accessed by a tenant #55738

Merged
merged 1 commit into from
Oct 20, 2020
Merged

sql: fix crdb_internal.{leases,node_runtime_info} when accessed by a tenant #55738

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 4 additions & 10 deletions pkg/sql/crdb_internal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,10 +176,7 @@ CREATE TABLE crdb_internal.node_runtime_info (

node := p.ExecCfg().NodeInfo

dNodeID := tree.DNull
if nodeID, ok := node.NodeID.OptionalNodeID(); ok {
dNodeID = tree.NewDInt(tree.DInt(nodeID))
}
nodeID, _ := node.NodeID.OptionalNodeID() // zero if not available
dbURL, err := node.PGURL(url.User(security.RootUser))
if err != nil {
return err
Expand Down Expand Up @@ -209,7 +206,7 @@ CREATE TABLE crdb_internal.node_runtime_info (
} {
k, v := kv[0], kv[1]
if err := addRow(
dNodeID,
tree.NewDInt(tree.DInt(nodeID)),
tree.NewDString(item.component),
tree.NewDString(k),
tree.NewDString(v),
Expand Down Expand Up @@ -504,18 +501,15 @@ CREATE TABLE crdb_internal.leases (
populate: func(
ctx context.Context, p *planner, _ *dbdesc.Immutable, addRow func(...tree.Datum) error,
) (err error) {
dNodeID := tree.DNull
if nodeID, ok := p.execCfg.NodeID.OptionalNodeID(); ok {
dNodeID = tree.NewDInt(tree.DInt(nodeID))
}
nodeID, _ := p.execCfg.NodeID.OptionalNodeID() // zero if not available
p.LeaseMgr().VisitLeases(func(desc catalog.Descriptor, dropped bool, _ int, expiration tree.DTimestamp) (wantMore bool) {
if p.CheckAnyPrivilege(ctx, desc) != nil {
// TODO(ajwerner): inspect what type of error got returned.
return true
}

err = addRow(
dNodeID,
tree.NewDInt(tree.DInt(nodeID)),
tree.NewDInt(tree.DInt(int64(desc.GetID()))),
tree.NewDString(desc.GetName()),
tree.NewDInt(tree.DInt(int64(desc.GetParentID()))),
Expand Down
5 changes: 4 additions & 1 deletion pkg/sql/logictest/testdata/logic_test/crdb_internal
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
# LogicTest: !3node-tenant(47895)
# 3node-tenant is blocked from running this file due to heavy reliance on
# unavailable node IDs in this test.
# LogicTest: !3node-tenant

query error database "crdb_internal" does not exist
ALTER DATABASE crdb_internal RENAME TO not_crdb_internal

Expand Down
12 changes: 12 additions & 0 deletions pkg/sql/logictest/testdata/logic_test/crdb_internal_tenant
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
# LogicTest: 3node-tenant

query II
SELECT count(distinct(node_id)), count(*) FROM crdb_internal.node_runtime_info
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be good to have at least one test case for all the internal tables and functions, to ensure they don't regress. I'd copy the entire crdb_internal suite, and then modify it for tenants, so that our testing is at least as good as that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Doing this in #55860 and will backport both these commits to 20.2.1

----
1 12

query IT
SELECT node_id, name FROM crdb_internal.leases ORDER BY name
----
0 role_members
0 test