Merge pull request #24390 from mberhault/marc/fix_privileges_in_validate

sql: fill in required privileges at Validate time.
cockroachdb · Apr 1, 2018 · 0dec51d · 0dec51d
2 parents 156c3a4 + 1c6b558
commit 0dec51d
Show file tree

Hide file tree

Showing 4 changed files with 18 additions and 3 deletions.
diff --git a/pkg/sql/descriptor.go b/pkg/sql/descriptor.go
@@ -202,8 +202,6 @@ func getDescriptorByID(
 			return errors.Errorf("%q is not a database", desc.String())
 		}
 
-		database.Privileges.MaybeFixPrivileges(id)
-
 		if err := database.Validate(); err != nil {
 			return err
 		}

diff --git a/pkg/sql/grant_revoke.go b/pkg/sql/grant_revoke.go
@@ -91,6 +91,12 @@ func (p *planner) changePrivileges(
 			changePrivilege(privileges, string(grantee))
 		}
 
+		// Validate privilege descriptors directly as the db/table level Validate
+		// may fix up the descriptor.
+		if err := privileges.Validate(descriptor.GetID()); err != nil {
+			return nil, err
+		}
+
 		switch d := descriptor.(type) {
 		case *sqlbase.DatabaseDescriptor:
 			if err := d.Validate(); err != nil {

diff --git a/pkg/sql/sqlbase/structured.go b/pkg/sql/sqlbase/structured.go
@@ -1161,6 +1161,11 @@ func (desc *TableDescriptor) ValidateTable(st *cluster.Settings) error {
 		}
 	}
 
+	// Fill in any incorrect privileges that may have been missed due to mixed-versions.
+	// TODO(mberhault): remove this in 2.1 (maybe 2.2) when privilege-fixing migrations have been
+	// run again and mixed-version clusters always write "good" descriptors.
+	desc.Privileges.MaybeFixPrivileges(desc.GetID())
+
 	// Validate the privilege descriptor.
 	return desc.Privileges.Validate(desc.GetID())
 }
@@ -2407,6 +2412,12 @@ func (desc *DatabaseDescriptor) Validate() error {
 	if desc.ID == 0 {
 		return fmt.Errorf("invalid database ID %d", desc.ID)
 	}
+
+	// Fill in any incorrect privileges that may have been missed due to mixed-versions.
+	// TODO(mberhault): remove this in 2.1 (maybe 2.2) when privilege-fixing migrations have been
+	// run again and mixed-version clusters always write "good" descriptors.
+	desc.Privileges.MaybeFixPrivileges(desc.GetID())
+
 	// Validate the privilege descriptor.
 	return desc.Privileges.Validate(desc.GetID())
 }

diff --git a/pkg/sqlmigrations/migrations.go b/pkg/sqlmigrations/migrations.go
@@ -181,7 +181,7 @@ var backwardCompatibleMigrations = []migrationDescriptor{
 	},
 	{
 		// Introduced in v2.0.
-		// TODO(benesch): bake this migration into v2.1, but create a new migration
+		// TODO(mberhault): bake this migration into v2.1, but create a new migration
 		// with the same function to catch any tables written in a mixed-version setting.
 		name:   "ensure admin role privileges in all descriptors",
 		workFn: ensureMaxPrivileges,