Merge #138909 #138979

138909: sql/schemachanger: Add support for storing policy command and type r=spilchen a=spilchen A previous commit introduced basic support for CREATE/DROP POLICY. This commit expands on that functionality by storing additional details in the policy descriptor. Specifically, it adds support for storing the policy type (restrictive or permissive) and the policy command (ALL, SELECT, INSERT, UPDATE, or DELETE). Since neither the policy type nor the policy command will be modifiable via ALTER POLICY, these attributes are included in the Policy element within the DSC, rather than as separate elements. Epic: CRDB-11724 Informs: #136696 Release note: None 138979: roachtest: update activerecord and ruby-pg expected failures r=rafiss a=rafiss - Mark 2 activerecord tests as flaky. - Mark 4 ruby-pg tests as passing after #138709 was merged. fixes #138886 fixes #138881 Release note: None Co-authored-by: Matt Spilchen <[email protected]> Co-authored-by: Rafi Shamim <[email protected]>
cockroachdb · Jan 13, 2025 · dbdd20f · dbdd20f
3 parents 2e25969 + 0d25b69 + 926f5ed
commit dbdd20f
Show file tree

Hide file tree

Showing 21 changed files with 332 additions and 27 deletions.
diff --git a/pkg/cmd/roachtest/tests/activerecord_blocklist.go b/pkg/cmd/roachtest/tests/activerecord_blocklist.go
@@ -27,6 +27,8 @@ var activeRecordIgnoreList = blocklist{
 	`BasicsTest#test_default_values_are_deeply_dupped`:                                                                                                         "flaky",
 	`CockroachDB::FixturesTest#test_create_fixtures`:                                                                                                           "flaky",
 	`FixtureWithSetModelClassPrevailsOverNamingConventionTest#test_model_class_in_fixture_file_is_respected`:                                                   "flaky",
+	`InheritanceTest#test_eager_load_belongs_to_primary_key_quoting`:                                                                                           "flaky",
+	`InheritanceTest#test_eager_load_belongs_to_something_inherited`:                                                                                           "flaky",
 	`TestAutosaveAssociationOnAHasAndBelongsToManyAssociation#test_should_not_save_and_return_false_if_a_callback_cancelled_saving_in_either_create_or_update`: "flaky",
 	`TestAutosaveAssociationOnAHasAndBelongsToManyAssociation#test_should_not_update_children_when_parent_creation_with_no_reason`:                             "flaky",
 	`TestAutosaveAssociationOnAHasAndBelongsToManyAssociation#test_should_update_children_when_autosave_is_true_and_parent_is_new_but_child_is_not`:            "flaky",

diff --git a/pkg/cmd/roachtest/tests/ruby_pg_blocklist.go b/pkg/cmd/roachtest/tests/ruby_pg_blocklist.go
@@ -54,8 +54,6 @@ var rubyPGBlocklist = blocklist{
 	`PG::Connection multinationalization support Ruby 1.9.x default_internal encoding allows users of the async interface to set the client_encoding to the default_internal`:                                                   "unknown",
 	`PG::Connection multinationalization support Ruby 1.9.x default_internal encoding honors the Encoding.default_internal if it's set and the synchronous interface is used`:                                                   "unknown",
 	`PG::Connection multinationalization support encodes exception messages with the connection's encoding (#96)`:                                                                                                               "unknown",
-	`PG::Connection multinationalization support handles clearing result in or after set_notice_receiver`:                                                                                                                       "unknown",
-	`PG::Connection multinationalization support receives properly encoded messages in the notice callbacks`:                                                                                                                    "unknown",
 	`PG::Connection multinationalization support receives properly encoded text from wait_for_notify`:                                                                                                                           "unknown",
 	`PG::Connection multinationalization support respect and convert character encoding of input strings should convert query string and parameters to #exec_params`:                                                            "unknown",
 	`PG::Connection multinationalization support respect and convert character encoding of input strings should convert query string and parameters to #send_query_params`:                                                      "unknown",
@@ -132,8 +130,6 @@ var rubyPGBlocklist = blocklist{
 	`running with sync_* methods PG::Connection multinationalization support Ruby 1.9.x default_internal encoding allows users of the async interface to set the client_encoding to the default_internal`:                       "unknown",
 	`running with sync_* methods PG::Connection multinationalization support Ruby 1.9.x default_internal encoding honors the Encoding.default_internal if it's set and the synchronous interface is used`:                       "unknown",
 	`running with sync_* methods PG::Connection multinationalization support encodes exception messages with the connection's encoding (#96)`:                                                                                   "unknown",
-	`running with sync_* methods PG::Connection multinationalization support handles clearing result in or after set_notice_receiver`:                                                                                           "unknown",
-	`running with sync_* methods PG::Connection multinationalization support receives properly encoded messages in the notice callbacks`:                                                                                        "unknown",
 	`running with sync_* methods PG::Connection multinationalization support receives properly encoded text from wait_for_notify`:                                                                                               "unknown",
 	`running with sync_* methods PG::Connection multinationalization support respect and convert character encoding of input strings should convert query string and parameters to #exec_params`:                                "unknown",
 	`running with sync_* methods PG::Connection multinationalization support respect and convert character encoding of input strings should convert query string and parameters to #send_query_params`:                          "unknown",

diff --git a/pkg/sql/catalog/catpb/BUILD.bazel b/pkg/sql/catalog/catpb/BUILD.bazel
@@ -40,6 +40,7 @@ go_library(
         "job_id.go",
         "multiregion.go",
         "privilege.go",
+        "redact.go",
         ":gen-privilegedescversion-stringer",  # keep
     ],
     embed = [":catpb_go_proto"],

diff --git a/pkg/sql/catalog/catpb/enum.proto b/pkg/sql/catalog/catpb/enum.proto
@@ -11,6 +11,10 @@ syntax = "proto3";
 package cockroach.sql.catalog.catpb;
 option go_package = "github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb";
 
+// NOTE: When adding new enum types to this file, consider implementing the
+// redact.SafeValue interface if the values are safe to display in redacted logs.
+// This implementation can be added to the redact.go file in this package.
+
 // SystemColumnKind is an enum representing the different kind of system
 // columns that can be synthesized by the execution engine.
 enum SystemColumnKind {
@@ -48,3 +52,28 @@ enum InvertedIndexColumnKind {
   // text columns.
   TRIGRAM = 1;
 }
+
+// Type represents the category of policy.
+//
+// NOTE: When adding a new enum value, ensure it is only utilized after
+// verifying that the cluster version has been finalized. Older versions
+// will validate that only recognized enum values are present.
+enum PolicyType {
+  POLICYTYPE_UNUSED = 0;
+  PERMISSIVE = 1;
+  RESTRICTIVE = 2;
+}
+
+// PolicyCommand specifies the SQL commands to which the policy applies.
+//
+// NOTE: When adding a new enum value, ensure it is only utilized after
+// verifying that the cluster version has been finalized. Older versions
+// will validate that only recognized enum values are present.
+enum PolicyCommand {
+  POLICYCOMMAND_UNUSED = 0;
+  ALL = 1;
+  SELECT = 2;
+  INSERT = 3;
+  UPDATE = 4;
+  DELETE = 5;
+}
diff --git a/pkg/sql/catalog/catpb/redact.go b/pkg/sql/catalog/catpb/redact.go
@@ -0,0 +1,12 @@
+// Copyright 2025 The Cockroach Authors.
+//
+// Use of this software is governed by the CockroachDB Software License
+// included in the /LICENSE file.
+
+package catpb
+
+// SafeValue implements the redact.SafeValue interface.
+func (PolicyType) SafeValue() {}
+
+// SafeValue implements the redact.SafeValue interface.
+func (PolicyCommand) SafeValue() {}
diff --git a/pkg/sql/catalog/descpb/structured.proto b/pkg/sql/catalog/descpb/structured.proto
@@ -706,6 +706,12 @@ message PolicyDescriptor {
 
   // The name of the policy. Unique within a table, and cannot be qualified.
   optional string name = 2 [(gogoproto.nullable) = false];
+
+  // Type indicates whether the policy is permissive or restrictive.
+  optional cockroach.sql.catalog.catpb.PolicyType type = 3 [(gogoproto.nullable) = false];
+
+  // Command specifies the SQL commands to which the policy applies.
+  optional cockroach.sql.catalog.catpb.PolicyCommand command = 4 [(gogoproto.nullable) = false];
 }
 
 // A DescriptorMutation represents a column or an index that

diff --git a/pkg/sql/catalog/tabledesc/validate.go b/pkg/sql/catalog/tabledesc/validate.go
@@ -2083,6 +2083,14 @@ func (desc *wrapper) validatePolicies() error {
 				p.ID, p.Name, other)
 		}
 		idToName[p.ID] = p.Name
+		if _, ok := catpb.PolicyType_name[int32(p.Type)]; !ok || p.Type == catpb.PolicyType_POLICYTYPE_UNUSED {
+			return errors.AssertionFailedf(
+				"policy %q has an unknown policy type %v", p.Name, p.Type)
+		}
+		if _, ok := catpb.PolicyCommand_name[int32(p.Command)]; !ok || p.Command == catpb.PolicyCommand_POLICYCOMMAND_UNUSED {
+			return errors.AssertionFailedf(
+				"policy %q has an unknown policy command %v", p.Name, p.Command)
+		}
 	}
 	return nil
 }

diff --git a/pkg/sql/catalog/tabledesc/validate_test.go b/pkg/sql/catalog/tabledesc/validate_test.go
@@ -3107,12 +3107,16 @@ func TestValidateTableDesc(t *testing.T) {
 				desc.NextPolicyID = 3
 				desc.Policies = []descpb.PolicyDescriptor{
 					{
-						ID:   1,
-						Name: "pol",
+						ID:      1,
+						Name:    "pol",
+						Type:    catpb.PolicyType_PERMISSIVE,
+						Command: catpb.PolicyCommand_ALL,
 					},
 					{
-						ID:   2,
-						Name: "pol",
+						ID:      2,
+						Name:    "pol",
+						Type:    catpb.PolicyType_RESTRICTIVE,
+						Command: catpb.PolicyCommand_INSERT,
 					},
 				}
 			}),
@@ -3122,12 +3126,16 @@ func TestValidateTableDesc(t *testing.T) {
 				desc.NextPolicyID = 11
 				desc.Policies = []descpb.PolicyDescriptor{
 					{
-						ID:   10,
-						Name: "pol_old",
+						ID:      10,
+						Name:    "pol_old",
+						Type:    catpb.PolicyType_RESTRICTIVE,
+						Command: catpb.PolicyCommand_UPDATE,
 					},
 					{
-						ID:   10,
-						Name: "pol_new",
+						ID:      10,
+						Name:    "pol_new",
+						Type:    catpb.PolicyType_PERMISSIVE,
+						Command: catpb.PolicyCommand_DELETE,
 					},
 				}
 			}),
@@ -3137,8 +3145,36 @@ func TestValidateTableDesc(t *testing.T) {
 				desc.NextPolicyID = 5
 				desc.Policies = []descpb.PolicyDescriptor{
 					{
-						ID:   20,
-						Name: "pol",
+						ID:      20,
+						Name:    "pol",
+						Type:    catpb.PolicyType_PERMISSIVE,
+						Command: catpb.PolicyCommand_SELECT,
+					},
+				}
+			}),
+		},
+		{err: `policy "pol" has an unknown policy type POLICYTYPE_UNUSED`,
+			desc: ModifyDescriptor(func(desc *descpb.TableDescriptor) {
+				desc.NextPolicyID = 2
+				desc.Policies = []descpb.PolicyDescriptor{
+					{
+						ID:      1,
+						Name:    "pol",
+						Type:    0,
+						Command: catpb.PolicyCommand_ALL,
+					},
+				}
+			}),
+		},
+		{err: `policy "pol" has an unknown policy command POLICYCOMMAND_UNUSED`,
+			desc: ModifyDescriptor(func(desc *descpb.TableDescriptor) {
+				desc.NextPolicyID = 2
+				desc.Policies = []descpb.PolicyDescriptor{
+					{
+						ID:      1,
+						Name:    "pol",
+						Type:    catpb.PolicyType_PERMISSIVE,
+						Command: 0,
 					},
 				}
 			}),

diff --git a/pkg/sql/logictest/testdata/logic_test/row_level_security b/pkg/sql/logictest/testdata/logic_test/row_level_security
@@ -107,4 +107,58 @@ DROP TABLE explicit1;
 statement ok
 SET use_declarative_schema_changer = $use_decl_sc;
 
+subtest policy_type_and_command_ddl
+
+statement ok
+CREATE TABLE multi_pol_tab1 (c1 INT NOT NULL PRIMARY KEY)
+
+statement ok
+CREATE POLICY "policy 1" ON multi_pol_tab1 AS PERMISSIVE
+
+statement ok
+CREATE POLICY "policy 2" ON multi_pol_tab1 AS RESTRICTIVE
+
+statement ok
+CREATE POLICY "policy 3" ON multi_pol_tab1 FOR ALL
+
+statement ok
+CREATE POLICY "policy 4" ON multi_pol_tab1 FOR INSERT
+
+statement ok
+CREATE POLICY "policy 5" ON multi_pol_tab1 FOR UPDATE
+
+statement ok
+CREATE POLICY "policy 6" ON multi_pol_tab1 FOR DELETE
+
+statement ok
+CREATE POLICY "policy 7" ON multi_pol_tab1 FOR SELECT
+
+query TT
+SHOW CREATE TABLE multi_pol_tab1
+----
+multi_pol_tab1  CREATE TABLE public.multi_pol_tab1 (
+                  c1 INT8 NOT NULL,
+                  CONSTRAINT multi_pol_tab1_pkey PRIMARY KEY (c1 ASC)
+                )
+
+statement ok
+DROP POLICY "policy 1" ON multi_pol_tab1
+
+statement ok
+DROP POLICY "policy 3" ON multi_pol_tab1
+
+statement ok
+DROP POLICY "policy 5" ON multi_pol_tab1
+
+query TT
+SHOW CREATE TABLE multi_pol_tab1
+----
+multi_pol_tab1  CREATE TABLE public.multi_pol_tab1 (
+                  c1 INT8 NOT NULL,
+                  CONSTRAINT multi_pol_tab1_pkey PRIMARY KEY (c1 ASC)
+                )
+
+statement ok
+DROP TABLE multi_pol_tab1
+
 subtest end
diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/create_policy.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/create_policy.go
@@ -6,11 +6,13 @@
 package scbuildstmt
 
 import (
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/sql/privilege"
 	"github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
+	"github.com/cockroachdb/errors"
 )
 
 // CreatePolicy implements CREATE POLICY.
@@ -40,10 +42,42 @@ func CreatePolicy(b BuildCtx, n *tree.CreatePolicy) {
 	b.Add(&scpb.Policy{
 		TableID:  tbl.TableID,
 		PolicyID: policyID,
+		Type:     convertPolicyType(n.Type),
+		Command:  convertPolicyCommand(n.Cmd),
 	})
 	b.Add(&scpb.PolicyName{
 		TableID:  tbl.TableID,
 		PolicyID: policyID,
 		Name:     string(n.PolicyName),
 	})
 }
+
+// convertPolicyType will convert from a tree.PolicyType to a catpb.PolicyType
+func convertPolicyType(in tree.PolicyType) catpb.PolicyType {
+	switch in {
+	case tree.PolicyTypeDefault, tree.PolicyTypePermissive:
+		return catpb.PolicyType_PERMISSIVE
+	case tree.PolicyTypeRestrictive:
+		return catpb.PolicyType_RESTRICTIVE
+	default:
+		panic(errors.AssertionFailedf("cannot convert tree.PolicyType: %v", in))
+	}
+}
+
+// convertPolicyCommand will convert from a tree.PolicyCommand to a catpb.PolicyCommand
+func convertPolicyCommand(in tree.PolicyCommand) catpb.PolicyCommand {
+	switch in {
+	case tree.PolicyCommandDefault, tree.PolicyCommandAll:
+		return catpb.PolicyCommand_ALL
+	case tree.PolicyCommandSelect:
+		return catpb.PolicyCommand_SELECT
+	case tree.PolicyCommandInsert:
+		return catpb.PolicyCommand_INSERT
+	case tree.PolicyCommandUpdate:
+		return catpb.PolicyCommand_UPDATE
+	case tree.PolicyCommandDelete:
+		return catpb.PolicyCommand_DELETE
+	default:
+		panic(errors.AssertionFailedf("cannot convert tree.PolicyCommand: %v", in))
+	}
+}
diff --git a/pkg/sql/schemachanger/scbuild/testdata/create_policy b/pkg/sql/schemachanger/scbuild/testdata/create_policy
@@ -0,0 +1,52 @@
+setup
+CREATE TABLE defaultdb.foo (i INT PRIMARY KEY);
+CREATE USER fred;
+----
+
+build
+CREATE POLICY "first policy" on defaultdb.foo AS PERMISSIVE FOR SELECT TO fred USING (i > 0) WITH CHECK (i % 2 = 0);
+----
+- [[IndexData:{DescID: 104, IndexID: 1}, PUBLIC], PUBLIC]
+  {indexId: 1, tableId: 104}
+- [[TableData:{DescID: 104, ReferencedDescID: 100}, PUBLIC], PUBLIC]
+  {databaseId: 100, tableId: 104}
+- [[Policy:{DescID: 104, PolicyID: 1}, PUBLIC], ABSENT]
+  {command: 2, policyId: 1, tableId: 104, type: 1}
+- [[PolicyName:{DescID: 104, Name: first policy, PolicyID: 1}, PUBLIC], ABSENT]
+  {name: first policy, policyId: 1, tableId: 104}
+
+build
+CREATE POLICY "second policy" on defaultdb.foo AS RESTRICTIVE FOR INSERT USING (false);
+----
+- [[IndexData:{DescID: 104, IndexID: 1}, PUBLIC], PUBLIC]
+  {indexId: 1, tableId: 104}
+- [[TableData:{DescID: 104, ReferencedDescID: 100}, PUBLIC], PUBLIC]
+  {databaseId: 100, tableId: 104}
+- [[Policy:{DescID: 104, PolicyID: 1}, PUBLIC], ABSENT]
+  {command: 3, policyId: 1, tableId: 104, type: 2}
+- [[PolicyName:{DescID: 104, Name: second policy, PolicyID: 1}, PUBLIC], ABSENT]
+  {name: second policy, policyId: 1, tableId: 104}
+
+build
+CREATE POLICY "third policy" on defaultdb.foo FOR DELETE TO CURRENT_USER,fred WITH CHECK (i < 0);
+----
+- [[IndexData:{DescID: 104, IndexID: 1}, PUBLIC], PUBLIC]
+  {indexId: 1, tableId: 104}
+- [[TableData:{DescID: 104, ReferencedDescID: 100}, PUBLIC], PUBLIC]
+  {databaseId: 100, tableId: 104}
+- [[Policy:{DescID: 104, PolicyID: 1}, PUBLIC], ABSENT]
+  {command: 5, policyId: 1, tableId: 104, type: 1}
+- [[PolicyName:{DescID: 104, Name: third policy, PolicyID: 1}, PUBLIC], ABSENT]
+  {name: third policy, policyId: 1, tableId: 104}
+
+build
+CREATE POLICY "fourth policy" on defaultdb.foo AS PERMISSIVE TO PUBLIC,CURRENT_SESSION;
+----
+- [[IndexData:{DescID: 104, IndexID: 1}, PUBLIC], PUBLIC]
+  {indexId: 1, tableId: 104}
+- [[TableData:{DescID: 104, ReferencedDescID: 100}, PUBLIC], PUBLIC]
+  {databaseId: 100, tableId: 104}
+- [[Policy:{DescID: 104, PolicyID: 1}, PUBLIC], ABSENT]
+  {command: 1, policyId: 1, tableId: 104, type: 1}
+- [[PolicyName:{DescID: 104, Name: fourth policy, PolicyID: 1}, PUBLIC], ABSENT]
+  {name: fourth policy, policyId: 1, tableId: 104}