Skip to content

Commit 5d71bf5

Browse files
craig[bot]Dedej-Berginkyle-a-wongtuansydauspilchen
committed
144269: roachtest: add RLS policies to the DRT cluster r=Dedej-Bergin a=Dedej-Bergin This adds a new DRT operation to test row-level security (RLS) features in live clusters. The operation: 1. Selects a random table from a populated database 2. Checks if RLS is already enabled and records the current state 3. Enables RLS on the table (with random FORCE RLS toggle) 4. Creates 5-20 random policies with varying operations (ALL/SELECT/INSERT/UPDATE/DELETE) 5. Waits for a random time before cleanup 6. Properly restores the table to its original state This will help test both DDL and DML related to RLS policies in a live system, stressing the system when applying/removing policies. The policies use simple expressions (true/false) to focus on testing RLS infrastructure rather than complex policy logic. Fixes: #138831 Epic: CRDB-11724 Release note: none 144393: workload: add Row Level Security operations to Random Schema Changer r=Dedej-Bergin a=Dedej-Bergin Added four new operations to the Random Schema Changer workload to test ROW LEVEL SECURITY commands: ``` ALTER TABLE <table> ENABLE ROW LEVEL SECURITY ALTER TABLE <table> DISABLE ROW LEVEL SECURITY ALTER TABLE <table> FORCE ROW LEVEL SECURITY ALTER TABLE <table> NO FORCE ROW LEVEL SECURITY ``` These operations are available in the declarative schema changer for cluster version 25.2 and above. Informs: #137120 Epic: CRDB-11724 Release note: none 144558: ui: fix duplicate drop unused index tags r=kyle-a-wong a=kyle-a-wong "Drop unused index" tags were being duplicated in the table index details page for every drop index recommendation that existed for said table. Now, only 1 tag is shown per row Epic: CC-30965 Fixes: CC-31183 Release note (bug fix): Fixed ui bug where "Drop unused index" tag was being shown more than once in the table index details page 144559: authors: add Tuan Dau to authors r=tuansydau a=tuansydau Epic: None Release note: None Thanks for the review in advance 144637: sql/schemachanger: gate CREATE POLICY in DSC prior to 25.2 r=spilchen a=spilchen In v25.1, CREATE POLICY was partially implemented but guarded by a feature gate, which blocked usage of the DDL. That gate was removed for 25.2 since RLS is now GA. However, CREATE POLICY still depends on DSC elements not present in 25.1. To prevent failures in mixed-version clusters, a version gate was added to the DSC logic to ensure CREATE POLICY cannot be planned unless the cluster has finalized to 25.2. A new mixed-version test was added instead of modifying the existing `row_level_security` test, since the latter doesn't fully account for mixed-version behavior. This test is expected to be short-lived, relevant only while 25.1 remains supported. Fixes #144625 Epic: CRDB-11724 Release note: none Co-authored-by: Bergin Dedej <[email protected]> Co-authored-by: Kyle Wong <[email protected]> Co-authored-by: Tuan Dau <[email protected]> Co-authored-by: Matt Spilchen <[email protected]>
6 parents 2c6916a + d916f2e + 3a55e7c + 3cc847f + 4825627 + fc8a3f7 commit 5d71bf5

File tree

11 files changed

+341
-22
lines changed

11 files changed

+341
-22
lines changed

‎AUTHORS‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -509,6 +509,7 @@ Tommy Truongchau <[email protected]> <[email protected]>
509509
Toshi Noguchi <[email protected]> <[email protected]>
510510
Tristan Ohlson <[email protected]> <@cockroachlabs.com>
511511
Tristan Rice <[email protected]> <[email protected]>
512+
Tuan Dau <[email protected]>
512513
Tushar Jain <[email protected]>
513514
Txiaozhe <[email protected]>
514515
Tyler Neely <[email protected]>

‎pkg/cmd/roachtest/operations/BUILD.bazel‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ go_library(
77
"add_column.go",
88
"add_database.go",
99
"add_index.go",
10+
"add_rls_policy.go",
1011
"backup_restore.go",
1112
"cluster_settings.go",
1213
"debug_zip.go",

‎pkg/cmd/roachtest/operations/add_rls_policy.go‎

Lines changed: 231 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,231 @@
1+
// Copyright 2024 The Cockroach Authors.
2+
//
3+
// Use of this software is governed by the CockroachDB Software License
4+
// included in the /LICENSE file.
5+
6+
package operations
7+
8+
import (
9+
"context"
10+
"fmt"
11+
"strings"
12+
"time"
13+
14+
"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/cluster"
15+
"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/operation"
16+
"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/operations/helpers"
17+
"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/option"
18+
"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/registry"
19+
"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/roachtestflags"
20+
"github.com/cockroachdb/cockroach/pkg/util/randutil"
21+
)
22+
23+
type cleanupRLSPolicy struct {
24+
db, table string
25+
policies []string
26+
originalRLSStmt string
27+
locked bool
28+
waitDuration time.Duration
29+
}
30+
31+
func (cl *cleanupRLSPolicy) Cleanup(ctx context.Context, o operation.Operation, c cluster.Cluster) {
32+
o.Status(fmt.Sprintf("Scheduling cleanup to happen after %s", cl.waitDuration))
33+
34+
// Start a goroutine to handle the wait and cleanup. Since this runs in the
35+
// background, we also create a new context so that the background goroutine
36+
// isn't aborted by the parent context.
37+
go func() {
38+
newCtx := context.Background()
39+
40+
if deadline, ok := ctx.Deadline(); ok {
41+
var cancel context.CancelFunc
42+
newCtx, cancel = context.WithDeadline(newCtx, deadline.Add(cl.waitDuration))
43+
defer cancel()
44+
}
45+
ctx = newCtx
46+
47+
// Wait for the specified duration before performing cleanup.
48+
time.Sleep(cl.waitDuration)
49+
50+
conn := c.Conn(ctx, o.L(), 1, option.VirtualClusterName(roachtestflags.VirtualCluster))
51+
defer conn.Close()
52+
53+
// Switch to the database where the table is located
54+
o.Status(fmt.Sprintf("switching to database %s for cleanup", cl.db))
55+
if _, err := conn.ExecContext(ctx, fmt.Sprintf("USE %s", cl.db)); err != nil {
56+
o.Fatal(err)
57+
}
58+
59+
if cl.locked {
60+
helpers.SetSchemaLocked(ctx, o, conn, cl.db, cl.table, false /* lock */)
61+
defer helpers.SetSchemaLocked(ctx, o, conn, cl.db, cl.table, true /* lock */)
62+
}
63+
64+
// Drop all policies that were created
65+
for _, policy := range cl.policies {
66+
o.Status(fmt.Sprintf("dropping policy %s on table %s.%s", policy, cl.db, cl.table))
67+
_, err := conn.ExecContext(ctx, fmt.Sprintf("DROP POLICY %s ON %s.%s", policy, cl.db, cl.table))
68+
if err != nil {
69+
o.Fatal(err)
70+
}
71+
}
72+
73+
// Restore original RLS state or disable it if it wasn't enabled before
74+
if cl.originalRLSStmt != "" {
75+
// Restore the original RLS state
76+
o.Status(fmt.Sprintf("restoring original row level security state for %s.%s", cl.db, cl.table))
77+
if _, err := conn.ExecContext(ctx, cl.originalRLSStmt); err != nil {
78+
o.Fatal(err)
79+
}
80+
} else {
81+
// If the table didn't have RLS before, disable it
82+
o.Status(fmt.Sprintf("disabling row level security for %s.%s", cl.db, cl.table))
83+
_, err := conn.ExecContext(ctx, fmt.Sprintf("ALTER TABLE %s.%s DISABLE ROW LEVEL SECURITY, NO FORCE ROW LEVEL SECURITY", cl.db, cl.table))
84+
if err != nil {
85+
o.Fatal(err)
86+
}
87+
}
88+
}()
89+
}
90+
91+
func runAddRLSPolicy(
92+
ctx context.Context, o operation.Operation, c cluster.Cluster,
93+
) registry.OperationCleanup {
94+
conn := c.Conn(ctx, o.L(), 1, option.VirtualClusterName(roachtestflags.VirtualCluster))
95+
defer func() { _ = conn.Close() }()
96+
97+
rng, _ := randutil.NewPseudoRand()
98+
99+
// Pick a random table
100+
dbName := helpers.PickRandomDB(ctx, o, conn, helpers.SystemDBs)
101+
tableName := helpers.PickRandomTable(ctx, o, conn, dbName)
102+
103+
// Check if the table already has RLS enabled and store the original statement if needed
104+
var tblName, createStmt string
105+
err := conn.QueryRowContext(ctx, fmt.Sprintf("SHOW CREATE TABLE %s.%s", dbName, tableName)).Scan(&tblName, &createStmt)
106+
if err != nil {
107+
o.Fatal(err)
108+
}
109+
110+
// Look for any RLS statements in the CREATE TABLE
111+
originalRLSStmt := ""
112+
113+
// Check if RLS is enabled and capture the original statement
114+
if strings.Contains(createStmt, "ROW LEVEL SECURITY") {
115+
// Extract the ALTER TABLE statement for RLS
116+
lines := strings.Split(createStmt, "\n")
117+
for _, line := range lines {
118+
if strings.Contains(line, "ROW LEVEL SECURITY") {
119+
addMissingSemicolon := ""
120+
if !strings.Contains(line, ";") {
121+
addMissingSemicolon = ";"
122+
}
123+
// Store the line as the original RLS statement and semicolon if missing
124+
originalRLSStmt = strings.TrimSpace(line + addMissingSemicolon)
125+
break
126+
}
127+
}
128+
}
129+
130+
// If the table's schema is locked, then unlock the table and make sure it will
131+
// be re-locked during cleanup.
132+
locked := helpers.IsSchemaLocked(o, conn, dbName, tableName)
133+
if locked {
134+
helpers.SetSchemaLocked(ctx, o, conn, dbName, tableName, false /* lock */)
135+
defer helpers.SetSchemaLocked(ctx, o, conn, dbName, tableName, true /* lock */)
136+
}
137+
138+
// Enable RLS on the table with random FORCE option
139+
shouldForceRLS := rng.Intn(2) == 0 // 50% chance of using FORCE
140+
forceClause := ""
141+
if shouldForceRLS {
142+
forceClause = ", FORCE ROW LEVEL SECURITY"
143+
}
144+
145+
o.Status(fmt.Sprintf("enabling row level security on table %s.%s%s", dbName, tableName, forceClause))
146+
_, err = conn.ExecContext(ctx, fmt.Sprintf("ALTER TABLE %s.%s ENABLE ROW LEVEL SECURITY%s", dbName, tableName, forceClause))
147+
if err != nil {
148+
o.Fatal(err)
149+
}
150+
151+
// Create between 0-5 policies
152+
numPolicies := rng.Intn(6)
153+
policies := make([]string, 0, numPolicies)
154+
155+
operations := []string{"ALL", "SELECT", "INSERT", "UPDATE", "DELETE"}
156+
users := []string{"public", "current_user", "session_user"}
157+
158+
for i := 0; i < numPolicies; i++ {
159+
// Pick a random operation
160+
operation := operations[rng.Intn(len(operations))]
161+
162+
// Pick a random user
163+
user := users[rng.Intn(len(users))]
164+
165+
// Create unique policy name
166+
policyName := fmt.Sprintf("rls_policy_%s_%d", operation, rng.Uint32())
167+
policies = append(policies, policyName)
168+
169+
o.Status(fmt.Sprintf("creating policy %s on table %s.%s for %s to %s",
170+
policyName, dbName, tableName, user, operation))
171+
172+
withCheck := ""
173+
using := ""
174+
175+
// WITH CHECK does is not supported for INSERT and DELETE
176+
if operation != "SELECT" && operation != "DELETE" {
177+
// Randomly choose between true or false
178+
checkExpr := "true"
179+
if rng.Intn(2) == 0 {
180+
checkExpr = "false"
181+
}
182+
withCheck = fmt.Sprintf("WITH CHECK (%s)", checkExpr)
183+
}
184+
185+
// USING is not supported for INSERT
186+
if operation != "INSERT" {
187+
// Randomly choose between true or false
188+
usingExpr := "true"
189+
if rng.Intn(2) == 0 {
190+
usingExpr = "false"
191+
}
192+
using = fmt.Sprintf("USING (%s)", usingExpr)
193+
}
194+
195+
_, err = conn.ExecContext(ctx, fmt.Sprintf(`
196+
CREATE POLICY %s ON %s.%s
197+
FOR %s
198+
TO %s
199+
%s
200+
%s
201+
`, policyName, dbName, tableName, operation, user, using, withCheck))
202+
if err != nil {
203+
o.Fatal(err)
204+
}
205+
}
206+
207+
o.Status(fmt.Sprintf("created %d RLS policies on table %s.%s", numPolicies, dbName, tableName))
208+
209+
// Return the cleanup struct with wait duration.
210+
waitTime := time.Hour
211+
return &cleanupRLSPolicy{
212+
db: dbName,
213+
table: tableName,
214+
policies: policies,
215+
originalRLSStmt: originalRLSStmt,
216+
locked: locked,
217+
waitDuration: waitTime,
218+
}
219+
}
220+
221+
func registerAddRLSPolicy(r registry.Registry) {
222+
r.AddOperation(registry.OperationSpec{
223+
Name: "add-rls-policy",
224+
Owner: registry.OwnerSQLFoundations,
225+
Timeout: 30 * time.Minute,
226+
CompatibleClouds: registry.AllClouds,
227+
CanRunConcurrently: registry.OperationCanRunConcurrently,
228+
Dependencies: []registry.OperationDependency{registry.OperationRequiresPopulatedDatabase},
229+
Run: runAddRLSPolicy,
230+
})
231+
}

‎pkg/cmd/roachtest/operations/register.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ func RegisterOperations(r registry.Registry) {
1515
registerAddColumn(r)
1616
registerAddDatabase(r)
1717
registerAddIndex(r)
18+
registerAddRLSPolicy(r)
1819
registerGrantRevoke(r)
1920
registerNetworkPartition(r)
2021
registerDiskStall(r)

‎pkg/sql/logictest/testdata/logic_test/mixed_version_row_level_security‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
# LogicTest: local-mixed-25.1
2+
3+
subtest create_drop_sanity
4+
5+
statement ok
6+
CREATE TABLE sanity1();
7+
8+
statement error pq: unimplemented: CREATE POLICY is not yet implemented
9+
CREATE POLICY p1 on sanity1 USING (true);
10+
11+
# No block for DROP POLICY because policies shouldn't exist in versions prior to 25.2
12+
statement error pq: policy "nonexist1" for table "sanity1" does not exist
13+
DROP POLICY nonexist1 on sanity1;
14+
15+
# Attempting to alter RLS returns this error because the declarative schema
16+
# changer fails with an unimplemented error, causing a fallback to the legacy
17+
# schema changer—which also doesn't support it.
18+
statement error pq: ALTER TABLE ... ROW LEVEL SECURITY is only implemented in the declarative schema changer
19+
ALTER TABLE sanity1 ENABLE ROW LEVEL SECURITY, FORCE ROW LEVEL SECURITY;

‎pkg/sql/logictest/tests/local-mixed-25.1/generated_test.go‎

Lines changed: 7 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎pkg/sql/schemachanger/scbuild/internal/scbuildstmt/create_policy.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
package scbuildstmt
77

88
import (
9+
"github.com/cockroachdb/cockroach/pkg/clusterversion"
910
"github.com/cockroachdb/cockroach/pkg/security/username"
1011
"github.com/cockroachdb/cockroach/pkg/sql/catalog"
1112
"github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb"
@@ -23,11 +24,15 @@ import (
2324
"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
2425
"github.com/cockroachdb/cockroach/pkg/sql/sem/volatility"
2526
"github.com/cockroachdb/cockroach/pkg/sql/types"
27+
"github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented"
2628
"github.com/cockroachdb/errors"
2729
)
2830

2931
// CreatePolicy implements CREATE POLICY.
3032
func CreatePolicy(b BuildCtx, n *tree.CreatePolicy) {
33+
if !b.EvalCtx().Settings.Version.ActiveVersion(b).IsActive(clusterversion.V25_2) {
34+
panic(unimplemented.NewWithIssue(136696, "CREATE POLICY is not yet implemented"))
35+
}
3136
b.IncrementSchemaChangeCreateCounter("policy")
3237

3338
tableElems := b.ResolveTable(n.TableName, ResolveParams{

‎pkg/ui/workspaces/cluster-ui/src/api/databases/tableIndexesApi.ts‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,11 @@ export const useTableIndexStats = ({
8888
stat?.statistics?.stats?.total_rows_read?.toNumber() ?? 0,
8989
indexRecs:
9090
data?.index_recommendations
91-
?.filter(rec => rec?.type != null)
91+
?.filter(
92+
rec =>
93+
rec?.type != null &&
94+
rec.index_id === stat.statistics?.key?.index_id,
95+
)
9296
.map(formatIndexRecsProto) ?? [],
9397
};
9498
});

‎pkg/workload/schemachange/operation_generator.go‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3048,6 +3048,7 @@ func (og *operationGenerator) insertRow(ctx context.Context, tx pgx.Tx) (stmt *o
30483048
{code: pgcode.ForeignKeyViolation, condition: fkViolation},
30493049
{code: pgcode.NotNullViolation, condition: true},
30503050
{code: pgcode.CheckViolation, condition: true},
3051+
{code: pgcode.InsufficientPrivilege, condition: true}, // For RLS violations
30513052
})
30523053
og.expectedCommitErrors.addAll(codesWithConditions{
30533054
{code: pgcode.ForeignKeyViolation, condition: fkViolation},
@@ -4862,3 +4863,45 @@ func (og *operationGenerator) setSeedInDB(ctx context.Context, tx pgx.Tx) error
48624863
}
48634864
return nil
48644865
}
4866+
4867+
func (og *operationGenerator) alterTableRLS(ctx context.Context, tx pgx.Tx) (*opStmt, error) {
4868+
tableName, err := og.randTable(ctx, tx, og.pctExisting(true), "")
4869+
if err != nil {
4870+
return nil, err
4871+
}
4872+
4873+
tableExists, err := og.tableExists(ctx, tx, tableName)
4874+
if err != nil {
4875+
return nil, err
4876+
}
4877+
4878+
// Define the available RLS options
4879+
rlsOptions := []string{
4880+
"ENABLE ROW LEVEL SECURITY",
4881+
"DISABLE ROW LEVEL SECURITY",
4882+
"FORCE ROW LEVEL SECURITY",
4883+
"NO FORCE ROW LEVEL SECURITY",
4884+
}
4885+
4886+
// Randomly decide between 1 or 2 options
4887+
numOptions := og.randIntn(2) + 1
4888+
4889+
// Randomly select a unique permutation of the options
4890+
perm := og.params.rng.Perm(len(rlsOptions))
4891+
selectedOptions := make([]string, numOptions)
4892+
for i := 0; i < numOptions; i++ {
4893+
selectedOptions[i] = rlsOptions[perm[i]]
4894+
}
4895+
4896+
// Build the SQL statement
4897+
sqlStatement := fmt.Sprintf(`ALTER TABLE %s %s`, tableName, strings.Join(selectedOptions, ", "))
4898+
4899+
opStmt := makeOpStmt(OpStmtDDL)
4900+
opStmt.expectedExecErrors.addAll(codesWithConditions{
4901+
{code: pgcode.FeatureNotSupported, condition: !og.useDeclarativeSchemaChanger},
4902+
{code: pgcode.UndefinedTable, condition: !tableExists},
4903+
})
4904+
4905+
opStmt.sql = sqlStatement
4906+
return opStmt, nil
4907+
}

0 commit comments

Comments
 (0)