diff --git a/cmd/main.go b/cmd/main.go index ae31b335..ae0402f9 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -155,6 +155,9 @@ func main() { AllDatabases: func(namespace string) ([]postgresqlv1alpha1.PostgreSQLDatabase, error) { return kube.PostgreSQLDatabases(mgr.GetClient(), namespace) }, + AllUsers: func() ([]postgresqlv1alpha1.PostgreSQLUser, error) { + return kube.PostgreSQLUsers(mgr.GetClient()) + }, ResourceResolver: func(resource postgresqlv1alpha1.ResourceVar, namespace string) (string, error) { return kube.ResourceValue(mgr.GetClient(), resource, namespace) }, diff --git a/pkg/grants/cross_namespace_revoke_test.go b/pkg/grants/cross_namespace_revoke_test.go new file mode 100644 index 00000000..58a914a6 --- /dev/null +++ b/pkg/grants/cross_namespace_revoke_test.go @@ -0,0 +1,332 @@ +package grants + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + lunarwayv1alpha1 "go.lunarway.com/postgresql-controller/api/v1alpha1" + "go.lunarway.com/postgresql-controller/pkg/postgres" + "go.lunarway.com/postgresql-controller/test" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +// TestCrossNamespaceRevocation_KrviScenario reproduces the real-world issue +// where the same user has PostgreSQLUser resources in multiple namespaces, and +// two of them (team-a and team-b) point to the same PostgreSQL host +// (shared-db.example.local.). +// +// Each namespace's reconciliation independently computes desired roles from +// only its own databases. When rolesDiff runs, it revokes any existing roles +// not in the expected list — including roles granted by the other namespace. +func TestCrossNamespaceRevocation_KrviScenario(t *testing.T) { + sharedHost := "shared-db.example.local." + + // Databases known in the "team-a" namespace on the shared host. + teamADatabases := []lunarwayv1alpha1.PostgreSQLDatabase{ + runningDatabase(sharedHost, "orders"), + runningDatabase(sharedHost, "invoices"), + } + + // Databases known in the "team-b" namespace on the shared host. + teamBDatabases := []lunarwayv1alpha1.PostgreSQLDatabase{ + runningDatabase(sharedHost, "reports"), + runningDatabase(sharedHost, "billing"), + } + + // --- Step 1: groupAccesses for team-a namespace --- + teamAGranter := newTestGranter(teamADatabases) + teamAAccesses, err := teamAGranter.groupAccesses( + test.NewLogger(t), "team-a", + []lunarwayv1alpha1.AccessSpec{ + { + Host: lunarwayv1alpha1.ResourceVar{Value: sharedHost}, + AllDatabases: &trueValue, + Reason: "team-a service account needs read access", + }, + }, + nil, + ) + require.NoError(t, err) + + teamASchemas := databaseSchemas(teamAAccesses[sharedHost]) + teamARoleNames := schemaRoleNames(teamASchemas) + assert.ElementsMatch(t, []string{"orders_read", "invoices_read"}, teamARoleNames) + + // --- Step 2: groupAccesses for team-b namespace --- + teamBGranter := newTestGranter(teamBDatabases) + teamBAccesses, err := teamBGranter.groupAccesses( + test.NewLogger(t), "team-b", + []lunarwayv1alpha1.AccessSpec{ + { + Host: lunarwayv1alpha1.ResourceVar{Value: sharedHost}, + AllDatabases: &trueValue, + Reason: "team-b service account needs read access", + }, + }, + nil, + ) + require.NoError(t, err) + + teamBSchemas := databaseSchemas(teamBAccesses[sharedHost]) + teamBRoleNames := schemaRoleNames(teamBSchemas) + assert.ElementsMatch(t, []string{"reports_read", "billing_read"}, teamBRoleNames) + + // --- Step 3: Demonstrate the conflict --- + // The two namespaces produce completely disjoint role sets for the same + // host and the same user. When reconciled independently, each namespace's + // rolesDiff call will revoke the other's roles because they are not in its + // expected list. + // + // This is the root cause: SyncUser processes one PostgreSQLUser at a time + // and has no awareness of other PostgreSQLUser resources for the same + // username on the same host in different namespaces. + for _, teamARole := range teamARoleNames { + assert.NotContains(t, teamBRoleNames, teamARole, + "BUG CONFIRMED: team-a role %q is not in team-b's expected list and WILL be revoked when team-b reconciles", teamARole) + } + for _, teamBRole := range teamBRoleNames { + assert.NotContains(t, teamARoleNames, teamBRole, + "BUG CONFIRMED: team-b role %q is not in team-a's expected list and WILL be revoked when team-a reconciles", teamBRole) + } +} + +// TestCrossNamespaceRevocation_SameNamespaceSafe verifies that within a single +// namespace, multiple entries on the same host do NOT cause revocations because +// all entries are grouped together into one databaseSchemas slice. +func TestCrossNamespaceRevocation_SameNamespaceSafe(t *testing.T) { + host := "db.host.example.com" + + databases := []lunarwayv1alpha1.PostgreSQLDatabase{ + runningDatabase(host, "users"), + runningDatabase(host, "products"), + runningDatabase(host, "analytics"), + } + + // User has allDatabases AND a specific database entry on the same host. + reads := []lunarwayv1alpha1.AccessSpec{ + { + Host: lunarwayv1alpha1.ResourceVar{Value: host}, + AllDatabases: &trueValue, + Reason: "service account needs read access to all databases", + }, + { + Host: lunarwayv1alpha1.ResourceVar{Value: host}, + Database: lunarwayv1alpha1.ResourceVar{Value: "users"}, + Schema: lunarwayv1alpha1.ResourceVar{Value: "users"}, + Reason: "explicit entry for users database", + }, + } + + granter := newTestGranter(databases) + accesses, err := granter.groupAccesses(test.NewLogger(t), "team-a", reads, nil) + require.NoError(t, err) + + schemas := databaseSchemas(accesses[host]) + roleNames := schemaRoleNames(schemas) + + // All databases should be present. The duplicate from the specific entry + // is harmless — rolesDiff deduplicates via contains(). + assert.Contains(t, roleNames, "users_read") + assert.Contains(t, roleNames, "products_read") + assert.Contains(t, roleNames, "analytics_read") +} + +// TestCrossNamespaceRevocation_ExpiredWriteOnlyRevokesWrite verifies that when +// a time-bound write entry expires, only the write role is removed — read +// access from allDatabases is unaffected. This is correct single-namespace +// behavior. +func TestCrossNamespaceRevocation_ExpiredWriteOnlyRevokesWrite(t *testing.T) { + host := "db.host.example.com" + now := time.Date(2025, 10, 26, 0, 0, 0, 0, time.UTC) + + databases := []lunarwayv1alpha1.PostgreSQLDatabase{ + runningDatabase(host, "orders"), + runningDatabase(host, "products"), + } + + reads := []lunarwayv1alpha1.AccessSpec{ + { + Host: lunarwayv1alpha1.ResourceVar{Value: host}, + AllDatabases: &trueValue, + Reason: "service account needs read access", + }, + } + + // This write entry has expired. + startTime := metav1.NewTime(time.Date(2025, 10, 25, 10, 0, 0, 0, time.UTC)) + stopTime := metav1.NewTime(time.Date(2025, 10, 25, 15, 0, 0, 0, time.UTC)) + writes := []lunarwayv1alpha1.WriteAccessSpec{ + { + AccessSpec: lunarwayv1alpha1.AccessSpec{ + Host: lunarwayv1alpha1.ResourceVar{Value: host}, + Database: lunarwayv1alpha1.ResourceVar{Value: "orders"}, + Schema: lunarwayv1alpha1.ResourceVar{Value: "orders"}, + Reason: "temporary write access", + Start: &startTime, + Stop: &stopTime, + }, + }, + } + + granter := Granter{ + Now: func() time.Time { return now }, + AllDatabasesReadEnabled: true, + AllDatabases: func(namespace string) ([]lunarwayv1alpha1.PostgreSQLDatabase, error) { + return databases, nil + }, + ResourceResolver: func(r lunarwayv1alpha1.ResourceVar, ns string) (string, error) { + return r.Value, nil + }, + } + + accesses, err := granter.groupAccesses(test.NewLogger(t), "team-a", reads, writes) + require.NoError(t, err) + + schemas := databaseSchemas(accesses[host]) + roleNames := schemaRoleNames(schemas) + + // The expired write should NOT be included. + assert.NotContains(t, roleNames, "orders_readwrite", + "expired write should not be in expected roles") + + // Read access should still be present for all databases. + assert.Contains(t, roleNames, "orders_read") + assert.Contains(t, roleNames, "products_read") +} + +// TestCrossNamespaceRevocation_MergeFixPreventsRevocation verifies that +// mergeSiblingAccesses expands the expected role set to include roles from +// sibling namespaces, preventing cross-namespace revocations. +func TestCrossNamespaceRevocation_MergeFixPreventsRevocation(t *testing.T) { + sharedHost := "shared-db.example.local." + + teamADatabases := []lunarwayv1alpha1.PostgreSQLDatabase{ + runningDatabase(sharedHost, "orders"), + runningDatabase(sharedHost, "invoices"), + } + teamBDatabases := []lunarwayv1alpha1.PostgreSQLDatabase{ + runningDatabase(sharedHost, "reports"), + runningDatabase(sharedHost, "billing"), + } + + teamAUser := lunarwayv1alpha1.PostgreSQLUser{ + ObjectMeta: metav1.ObjectMeta{Name: "alice", Namespace: "team-a"}, + Spec: lunarwayv1alpha1.PostgreSQLUserSpec{ + Name: "alice", + Read: &[]lunarwayv1alpha1.AccessSpec{ + { + Host: lunarwayv1alpha1.ResourceVar{Value: sharedHost}, + AllDatabases: &trueValue, + Reason: "team-a service account needs read access", + }, + }, + }, + } + teamBUser := lunarwayv1alpha1.PostgreSQLUser{ + ObjectMeta: metav1.ObjectMeta{Name: "alice", Namespace: "team-b"}, + Spec: lunarwayv1alpha1.PostgreSQLUserSpec{ + Name: "alice", + Read: &[]lunarwayv1alpha1.AccessSpec{ + { + Host: lunarwayv1alpha1.ResourceVar{Value: sharedHost}, + AllDatabases: &trueValue, + Reason: "team-b service account needs read access", + }, + }, + }, + } + + allUsers := []lunarwayv1alpha1.PostgreSQLUser{teamAUser, teamBUser} + + // Build a granter that knows about all databases across namespaces. + granter := Granter{ + Now: time.Now, + AllDatabasesReadEnabled: true, + AllDatabases: func(namespace string) ([]lunarwayv1alpha1.PostgreSQLDatabase, error) { + switch namespace { + case "team-a": + return teamADatabases, nil + case "team-b": + return teamBDatabases, nil + default: + return append(teamADatabases, teamBDatabases...), nil + } + }, + AllUsers: func() ([]lunarwayv1alpha1.PostgreSQLUser, error) { + return allUsers, nil + }, + ResourceResolver: func(r lunarwayv1alpha1.ResourceVar, ns string) (string, error) { + return r.Value, nil + }, + } + + // Compute accesses for the team-a namespace. + teamAAccesses, err := granter.groupAccesses( + test.NewLogger(t), "team-a", + *teamAUser.Spec.Read, + nil, + ) + require.NoError(t, err) + + // Apply the merge fix. + granter.mergeSiblingAccesses(test.NewLogger(t), "team-a", "alice", teamAAccesses) + + mergedSchemas := databaseSchemas(teamAAccesses[sharedHost]) + mergedRoleNames := schemaRoleNames(mergedSchemas) + + // After merging, all 4 databases must be present so rolesDiff won't revoke any of them. + assert.ElementsMatch(t, + []string{"orders_read", "invoices_read", "reports_read", "billing_read"}, + mergedRoleNames, + "merged accesses must contain roles from both namespaces to prevent revocation", + ) +} + +// --- Helpers --- + +func runningDatabase(host, name string) lunarwayv1alpha1.PostgreSQLDatabase { + return lunarwayv1alpha1.PostgreSQLDatabase{ + Spec: lunarwayv1alpha1.PostgreSQLDatabaseSpec{ + Name: name, + Host: lunarwayv1alpha1.ResourceVar{Value: host}, + User: lunarwayv1alpha1.ResourceVar{}, // empty → fallback to database name as schema + }, + Status: lunarwayv1alpha1.PostgreSQLDatabaseStatus{ + Phase: lunarwayv1alpha1.PostgreSQLDatabasePhaseRunning, + }, + } +} + +func newTestGranter(databases []lunarwayv1alpha1.PostgreSQLDatabase) Granter { + return Granter{ + Now: time.Now, + AllDatabasesReadEnabled: true, + AllDatabases: func(namespace string) ([]lunarwayv1alpha1.PostgreSQLDatabase, error) { + return databases, nil + }, + ResourceResolver: func(r lunarwayv1alpha1.ResourceVar, ns string) (string, error) { + return r.Value, nil + }, + } +} + +func schemaRoleNames(schemas []postgres.DatabaseSchema) []string { + var names []string + for _, s := range schemas { + suffix := "read" + switch s.Privileges { + case postgres.PrivilegeWrite: + suffix = "readwrite" + case postgres.PrivilegeOwningWrite: + suffix = "readowningwrite" + } + schema := s.Schema + if schema == "public" { + schema = s.Name + } + names = append(names, schema+"_"+suffix) + } + return names +} diff --git a/pkg/grants/grants.go b/pkg/grants/grants.go index 2b77b5bf..f36214f3 100644 --- a/pkg/grants/grants.go +++ b/pkg/grants/grants.go @@ -17,7 +17,7 @@ type Granter struct { AllDatabasesWriteEnabled bool ExtendedWritesEnabled bool AllDatabases func(namespace string) ([]lunarwayv1alpha1.PostgreSQLDatabase, error) - AllUsers func(namespace string) ([]lunarwayv1alpha1.PostgreSQLUser, error) + AllUsers func() ([]lunarwayv1alpha1.PostgreSQLUser, error) ResourceResolver func(resource lunarwayv1alpha1.ResourceVar, namespace string) (string, error) StaticRoles []string diff --git a/pkg/grants/sync.go b/pkg/grants/sync.go index a7ed9b89..29195e94 100644 --- a/pkg/grants/sync.go +++ b/pkg/grants/sync.go @@ -34,6 +34,9 @@ func (g *Granter) SyncUser(log logr.Logger, namespace, rolePrefix string, user l } log.Error(err, "Some access requests could not be resolved. Continuating with the resolved ones") } + if g.AllUsers != nil { + g.mergeSiblingAccesses(log, namespace, user.Spec.Name, accesses) + } log.Info(fmt.Sprintf("Found access requests for %d hosts", len(accesses))) hosts, err := g.connectToHosts(log, accesses) @@ -113,6 +116,39 @@ func (g *Granter) setRolesOnHosts(log logr.Logger, name string, accesses HostAcc return nil } +func (g *Granter) mergeSiblingAccesses(log logr.Logger, namespace, username string, accesses HostAccess) { + allUsers, err := g.AllUsers() + if err != nil { + log.Error(err, "Failed to list all users for cross-namespace merge; proceeding without") + return + } + for _, sibling := range allUsers { + if sibling.Spec.Name != username || sibling.Namespace == namespace { + continue + } + siblingRead := []lunarwayv1alpha1.AccessSpec{} + if sibling.Spec.Read != nil { + siblingRead = *sibling.Spec.Read + } + siblingWrite := []lunarwayv1alpha1.WriteAccessSpec{} + if sibling.Spec.Write != nil { + siblingWrite = *sibling.Spec.Write + } + siblingAccesses, err := g.groupAccesses(log, sibling.Namespace, siblingRead, siblingWrite) + if err != nil { + log.Error(err, "Failed to compute sibling accesses", "siblingNamespace", sibling.Namespace) + if len(siblingAccesses) == 0 { + continue + } + } + for host, siblingList := range siblingAccesses { + if _, ok := accesses[host]; ok { + accesses[host] = append(accesses[host], siblingList...) + } + } + } +} + func databaseSchemas(accesses []ReadWriteAccess) []postgres.DatabaseSchema { var ds []postgres.DatabaseSchema for _, access := range accesses { diff --git a/pkg/kube/kube.go b/pkg/kube/kube.go index 717847c0..ca258b66 100644 --- a/pkg/kube/kube.go +++ b/pkg/kube/kube.go @@ -95,3 +95,13 @@ func PostgreSQLDatabases(c client.Client, namespace string) ([]lunarwayv1alpha1. } return databases.Items, nil } + +// PostgreSQLUsers returns all PostgreSQLUser resources cluster-wide. +func PostgreSQLUsers(c client.Client) ([]lunarwayv1alpha1.PostgreSQLUser, error) { + var users lunarwayv1alpha1.PostgreSQLUserList + err := c.List(context.TODO(), &users) + if err != nil { + return nil, fmt.Errorf("get users: %w", err) + } + return users.Items, nil +} diff --git a/pkg/postgres/cross_namespace_revoke_test.go b/pkg/postgres/cross_namespace_revoke_test.go new file mode 100644 index 00000000..0780bc7f --- /dev/null +++ b/pkg/postgres/cross_namespace_revoke_test.go @@ -0,0 +1,119 @@ +package postgres + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "go.lunarway.com/postgresql-controller/test" +) + +// TestRolesDiff_CrossNamespaceRevocation demonstrates the bug where two +// PostgreSQLUser resources in different namespaces pointing to the same +// PostgreSQL host cause each reconciliation to revoke roles granted by the +// other namespace. +// +// Scenario: +// - alice in "team-a" namespace: allDatabases on shared-db.example.local. +// - alice in "team-b" namespace: allDatabases on shared-db.example.local. +// +// When "team-a" reconciles, it only knows about team-a databases and revokes +// roles from team-b databases. When "team-b" reconciles next, it revokes +// team-a roles. The user sees intermittent access loss. +func TestRolesDiff_CrossNamespaceRevocation(t *testing.T) { + logger := test.NewLogger(t) + + // Simulate initial state: both namespaces have been reconciled once. + // The PostgreSQL user has roles from both team-a and team-b namespaces. + existingRoles := []string{ + "app_role", + "orders_read", // from team-a namespace + "invoices_read", // from team-a namespace + "reports_read", // from team-b namespace + "billing_read", // from team-b namespace + } + + // --- Reconciliation from "team-a" namespace --- + // team-a only knows about its own databases: orders, invoices. + teamADatabases := []DatabaseSchema{ + {Name: "orders", Schema: "orders", Privileges: PrivilegeRead}, + {Name: "invoices", Schema: "invoices", Privileges: PrivilegeRead}, + } + staticRoles := []string{"app_role"} + + addable, removeable := rolesDiff(logger, existingRoles, staticRoles, teamADatabases) + + // BUG: team-a reconciliation revokes team-b roles because they are not + // in team-a's expected list. + assert.Nil(t, addable, "no roles should be added") + assert.Equal(t, []string{"reports_read", "billing_read"}, removeable, + "BUG REPRODUCED: team-a namespace reconciliation revokes team-b namespace roles") + + // --- After team-a reconciliation, the user only has team-a roles --- + rolesAfterTeamAReconcile := []string{ + "app_role", + "orders_read", + "invoices_read", + } + + // --- Reconciliation from "team-b" namespace --- + // team-b only knows about its own databases: reports, billing. + teamBDatabases := []DatabaseSchema{ + {Name: "reports", Schema: "reports", Privileges: PrivilegeRead}, + {Name: "billing", Schema: "billing", Privileges: PrivilegeRead}, + } + + addable2, removeable2 := rolesDiff(logger, rolesAfterTeamAReconcile, staticRoles, teamBDatabases) + + // BUG: team-b reconciliation now revokes team-a roles! + assert.Equal(t, []string{"reports_read", "billing_read"}, addable2, + "team-b roles should be re-added") + assert.Equal(t, []string{"orders_read", "invoices_read"}, removeable2, + "BUG REPRODUCED: team-b namespace reconciliation revokes team-a namespace roles") +} + +// TestRolesDiff_CrossNamespaceRevocationCycle demonstrates a full +// grant/revoke cycle showing how roles oscillate between namespaces across +// reconciliation loops. +func TestRolesDiff_CrossNamespaceRevocationCycle(t *testing.T) { + logger := test.NewLogger(t) + staticRoles := []string{"app_role"} + + teamADatabases := []DatabaseSchema{ + {Name: "orders", Schema: "orders", Privileges: PrivilegeRead}, + } + teamBDatabases := []DatabaseSchema{ + {Name: "reports", Schema: "reports", Privileges: PrivilegeRead}, + } + + // Start: user has no roles. + existingRoles := []string{} + + // Cycle 1: team-a reconciles first. + addable, removeable := rolesDiff(logger, existingRoles, staticRoles, teamADatabases) + assert.Equal(t, []string{"app_role", "orders_read"}, addable) + assert.Nil(t, removeable) + + // Apply: user now has app_role + orders_read. + existingRoles = []string{"app_role", "orders_read"} + + // Cycle 1: team-b reconciles second. + addable, removeable = rolesDiff(logger, existingRoles, staticRoles, teamBDatabases) + assert.Equal(t, []string{"reports_read"}, addable) + // BUG: orders_read is revoked! + assert.Equal(t, []string{"orders_read"}, removeable, + "BUG: team-b revokes orders_read granted by team-a namespace") + + // Apply: user now has app_role + reports_read (orders_read LOST). + existingRoles = []string{"app_role", "reports_read"} + + // Cycle 2: team-a reconciles again. + addable, removeable = rolesDiff(logger, existingRoles, staticRoles, teamADatabases) + assert.Equal(t, []string{"orders_read"}, addable) + // BUG: reports_read is revoked! + assert.Equal(t, []string{"reports_read"}, removeable, + "BUG: team-a revokes reports_read granted by team-b namespace") + + // The user NEVER has both orders_read AND reports_read at the same time + // after the initial reconciliation. Access oscillates depending on which + // namespace reconciles last. +}