Address reviews

citusdata · Nov 15, 2023 · 58f55e7 · 58f55e7
1 parent e5dd577
commit 58f55e7
Show file tree

Hide file tree

Showing 7 changed files with 75 additions and 77 deletions.
diff --git a/src/backend/distributed/commands/role.c b/src/backend/distributed/commands/role.c
@@ -947,11 +947,8 @@ GenerateSecLabelOnRoleStmts(Oid roleid, char *rolename)
 
 		secLabelStmt->provider = TextDatumGetCString(
 			datumArray[Anum_pg_shseclabel_provider - 1]);
-		if (!isNullArray[Anum_pg_shseclabel_label - 1])
-		{
-			secLabelStmt->label = TextDatumGetCString(
-				datumArray[Anum_pg_shseclabel_label - 1]);
-		}
+		secLabelStmt->label = TextDatumGetCString(
+			datumArray[Anum_pg_shseclabel_label - 1]);
 
 		secLabelStmts = lappend(secLabelStmts, secLabelStmt);
 	}

diff --git a/src/backend/distributed/commands/seclabel.c b/src/backend/distributed/commands/seclabel.c
@@ -112,8 +112,9 @@ void
 citus_test_object_relabel(const ObjectAddress *object, const char *seclabel)
 {
 	if (seclabel == NULL ||
-		strcmp(seclabel, "citus unclassified") == 0 ||
-		strcmp(seclabel, "citus classified") == 0)
+		strcmp(seclabel, "citus_unclassified") == 0 ||
+		strcmp(seclabel, "citus_classified") == 0 ||
+		strcmp(seclabel, "citus '!unclassified") == 0)
 	{
 		return;
 	}

diff --git a/src/backend/distributed/shared_library_init.c b/src/backend/distributed/shared_library_init.c
@@ -584,6 +584,8 @@ _PG_init(void)
 	{
 		register_label_provider("citus_tests_label_provider",
 								citus_test_object_relabel);
+		register_label_provider("citus '!tests_label_provider",
+								citus_test_object_relabel);
 	}
 }
 

diff --git a/src/test/regress/expected/multi_test_helpers.out b/src/test/regress/expected/multi_test_helpers.out
@@ -526,8 +526,8 @@ BEGIN
     RETURN result;
 END;
 $func$ LANGUAGE plpgsql;
--- Returns pg_seclabels entries from all nodes in the cluster
--- for which the provider is citus_tests_label_provider and the object name is the input
+-- Returns pg_seclabels entries from all nodes in the cluster for which
+-- the object name is the input.
 CREATE OR REPLACE FUNCTION get_citus_tests_label_provider_labels(object_name text,
                                                                  master_port INTEGER DEFAULT 57636,
                                                                  worker_1_port INTEGER DEFAULT 57637,
@@ -539,9 +539,8 @@ RETURNS TABLE (
 AS $func$
 DECLARE
     pg_seclabels_cmd TEXT := 'SELECT to_jsonb(q.*) FROM (' ||
-                             'SELECT objtype, label FROM pg_seclabels ' ||
-                             'WHERE provider = ''citus_tests_label_provider'' AND ' ||
-                             'objname = ''' || object_name || ''') q';
+                             'SELECT provider, objtype, label FROM pg_seclabels ' ||
+                             'WHERE objname = ''' || object_name || ''') q';
 BEGIN
     RETURN QUERY
     SELECT

diff --git a/src/test/regress/expected/seclabel.out b/src/test/regress/expected/seclabel.out
@@ -15,15 +15,15 @@ SELECT citus_remove_node('localhost', :worker_2_port);
 CREATE ROLE user1;
 CREATE ROLE "user 2";
 -- check an invalid label for our current dummy hook citus_test_object_relabel
-SECURITY LABEL ON ROLE user1 IS 'invalid_label';
+SECURITY LABEL FOR citus_tests_label_provider ON ROLE user1 IS 'invalid_label';
 ERROR:  'invalid_label' is not a valid security label for Citus tests.
 -- if we disable metadata_sync, the command will not be propagated
 SET citus.enable_metadata_sync TO off;
-SECURITY LABEL ON ROLE user1 IS 'citus unclassified';
+SECURITY LABEL FOR citus_tests_label_provider ON ROLE user1 IS 'citus_unclassified';
 SELECT node_type, result FROM get_citus_tests_label_provider_labels('user1') ORDER BY node_type;
-  node_type  |                       result
+  node_type  |                                            result
 ---------------------------------------------------------------------
- coordinator | {"label": "citus unclassified", "objtype": "role"}
+ coordinator | {"label": "citus_unclassified", "objtype": "role", "provider": "citus_tests_label_provider"}
  worker_1    |
 (2 rows)
 
@@ -43,60 +43,60 @@ CREATE VIEW v_dist AS SELECT * FROM a;
 -- distributed function
 CREATE FUNCTION notice(text) RETURNS void LANGUAGE plpgsql AS $$
     BEGIN RAISE NOTICE '%', $1; END; $$;
-SECURITY LABEL ON TABLE a IS 'citus classified';
+SECURITY LABEL FOR citus_tests_label_provider ON TABLE a IS 'citus_classified';
 NOTICE:  not propagating SECURITY LABEL commands whose object type is not role
 HINT:  Connect to worker nodes directly to manually run the same SECURITY LABEL command.
-SECURITY LABEL ON FUNCTION notice IS 'citus unclassified';
+SECURITY LABEL FOR citus_tests_label_provider ON FUNCTION notice IS 'citus_unclassified';
 NOTICE:  not propagating SECURITY LABEL commands whose object type is not role
 HINT:  Connect to worker nodes directly to manually run the same SECURITY LABEL command.
-SECURITY LABEL ON VIEW v_dist IS 'citus classified';
+SECURITY LABEL FOR citus_tests_label_provider ON VIEW v_dist IS 'citus_classified';
 NOTICE:  not propagating SECURITY LABEL commands whose object type is not role
 HINT:  Connect to worker nodes directly to manually run the same SECURITY LABEL command.
 SELECT node_type, result FROM get_citus_tests_label_provider_labels('a') ORDER BY node_type;
-  node_type  |                      result
+  node_type  |                                           result
 ---------------------------------------------------------------------
- coordinator | {"label": "citus classified", "objtype": "table"}
+ coordinator | {"label": "citus_classified", "objtype": "table", "provider": "citus_tests_label_provider"}
  worker_1    |
 (2 rows)
 
 SELECT node_type, result FROM get_citus_tests_label_provider_labels('notice(text)') ORDER BY node_type;
-  node_type  |                         result
+  node_type  |                                              result
 ---------------------------------------------------------------------
- coordinator | {"label": "citus unclassified", "objtype": "function"}
+ coordinator | {"label": "citus_unclassified", "objtype": "function", "provider": "citus_tests_label_provider"}
  worker_1    |
 (2 rows)
 
 SELECT node_type, result FROM get_citus_tests_label_provider_labels('v_dist') ORDER BY node_type;
-  node_type  |                      result
+  node_type  |                                           result
 ---------------------------------------------------------------------
- coordinator | {"label": "citus classified", "objtype": "view"}
+ coordinator | {"label": "citus_classified", "objtype": "view", "provider": "citus_tests_label_provider"}
  worker_1    |
 (2 rows)
 
 \c - - - :worker_1_port
-SECURITY LABEL ON TABLE a IS 'citus classified';
-SECURITY LABEL ON FUNCTION notice IS 'citus unclassified';
-SECURITY LABEL ON VIEW v_dist IS 'citus classified';
+SECURITY LABEL FOR citus_tests_label_provider ON TABLE a IS 'citus_classified';
+SECURITY LABEL FOR citus_tests_label_provider ON FUNCTION notice IS 'citus_unclassified';
+SECURITY LABEL FOR citus_tests_label_provider ON VIEW v_dist IS 'citus_classified';
 \c - - - :master_port
 SELECT node_type, result FROM get_citus_tests_label_provider_labels('a') ORDER BY node_type;
-  node_type  |                      result
+  node_type  |                                           result
 ---------------------------------------------------------------------
- coordinator | {"label": "citus classified", "objtype": "table"}
- worker_1    | {"label": "citus classified", "objtype": "table"}
+ coordinator | {"label": "citus_classified", "objtype": "table", "provider": "citus_tests_label_provider"}
+ worker_1    | {"label": "citus_classified", "objtype": "table", "provider": "citus_tests_label_provider"}
 (2 rows)
 
 SELECT node_type, result FROM get_citus_tests_label_provider_labels('notice(text)') ORDER BY node_type;
-  node_type  |                         result
+  node_type  |                                              result
 ---------------------------------------------------------------------
- coordinator | {"label": "citus unclassified", "objtype": "function"}
- worker_1    | {"label": "citus unclassified", "objtype": "function"}
+ coordinator | {"label": "citus_unclassified", "objtype": "function", "provider": "citus_tests_label_provider"}
+ worker_1    | {"label": "citus_unclassified", "objtype": "function", "provider": "citus_tests_label_provider"}
 (2 rows)
 
 SELECT node_type, result FROM get_citus_tests_label_provider_labels('v_dist') ORDER BY node_type;
-  node_type  |                      result
+  node_type  |                                           result
 ---------------------------------------------------------------------
- coordinator | {"label": "citus classified", "objtype": "view"}
- worker_1    | {"label": "citus classified", "objtype": "view"}
+ coordinator | {"label": "citus_classified", "objtype": "view", "provider": "citus_tests_label_provider"}
+ worker_1    | {"label": "citus_classified", "objtype": "view", "provider": "citus_tests_label_provider"}
 (2 rows)
 
 DROP TABLE a CASCADE;
@@ -105,66 +105,66 @@ DROP FUNCTION notice;
 -- test that SECURITY LABEL statement is actually propagated for ROLES
 SET citus.log_remote_commands TO on;
 SET citus.grep_remote_commands = '%SECURITY LABEL%';
-SECURITY LABEL for citus_tests_label_provider ON ROLE user1 IS 'citus classified';
-NOTICE:  issuing SECURITY LABEL FOR citus_tests_label_provider ON ROLE user1 IS 'citus classified'
+SECURITY LABEL for citus_tests_label_provider ON ROLE user1 IS 'citus_classified';
+NOTICE:  issuing SECURITY LABEL FOR citus_tests_label_provider ON ROLE user1 IS 'citus_classified'
 DETAIL:  on server postgres@localhost:xxxxx connectionId: xxxxxxx
 SECURITY LABEL for citus_tests_label_provider ON ROLE user1 IS NULL;
 NOTICE:  issuing SECURITY LABEL FOR citus_tests_label_provider ON ROLE user1 IS NULL
 DETAIL:  on server postgres@localhost:xxxxx connectionId: xxxxxxx
-SECURITY LABEL for citus_tests_label_provider ON ROLE user1 IS 'citus unclassified';
-NOTICE:  issuing SECURITY LABEL FOR citus_tests_label_provider ON ROLE user1 IS 'citus unclassified'
+SECURITY LABEL for citus_tests_label_provider ON ROLE user1 IS 'citus_unclassified';
+NOTICE:  issuing SECURITY LABEL FOR citus_tests_label_provider ON ROLE user1 IS 'citus_unclassified'
 DETAIL:  on server postgres@localhost:xxxxx connectionId: xxxxxxx
-SECURITY LABEL for citus_tests_label_provider ON ROLE "user 2" IS 'citus classified';
-NOTICE:  issuing SECURITY LABEL FOR citus_tests_label_provider ON ROLE "user 2" IS 'citus classified'
+SECURITY LABEL for "citus '!tests_label_provider" ON ROLE "user 2" IS 'citus ''!unclassified';
+NOTICE:  issuing SECURITY LABEL FOR "citus '!tests_label_provider" ON ROLE "user 2" IS 'citus ''!unclassified'
 DETAIL:  on server postgres@localhost:xxxxx connectionId: xxxxxxx
 \c - - - :worker_1_port
 -- command not allowed from worker node
-SECURITY LABEL for citus_tests_label_provider ON ROLE user1 IS 'citus unclassified';
+SECURITY LABEL for citus_tests_label_provider ON ROLE user1 IS 'citus ''!unclassified';
 ERROR:  operation is not allowed on this node
 HINT:  Connect to the coordinator and run it again.
 \c - - - :master_port
 RESET citus.log_remote_commands;
 SELECT node_type, result FROM get_citus_tests_label_provider_labels('user1') ORDER BY node_type;
-  node_type  |                       result
+  node_type  |                                            result
 ---------------------------------------------------------------------
- coordinator | {"label": "citus unclassified", "objtype": "role"}
- worker_1    | {"label": "citus unclassified", "objtype": "role"}
+ coordinator | {"label": "citus_unclassified", "objtype": "role", "provider": "citus_tests_label_provider"}
+ worker_1    | {"label": "citus_unclassified", "objtype": "role", "provider": "citus_tests_label_provider"}
 (2 rows)
 
 SELECT node_type, result FROM get_citus_tests_label_provider_labels('"user 2"') ORDER BY node_type;
-  node_type  |                      result
+  node_type  |                                              result
 ---------------------------------------------------------------------
- coordinator | {"label": "citus classified", "objtype": "role"}
- worker_1    | {"label": "citus classified", "objtype": "role"}
+ coordinator | {"label": "citus '!unclassified", "objtype": "role", "provider": "citus '!tests_label_provider"}
+ worker_1    | {"label": "citus '!unclassified", "objtype": "role", "provider": "citus '!tests_label_provider"}
 (2 rows)
 
 -- add a new node and check that it also propagates the SECURITY LABEL statement to the new node
 SET citus.log_remote_commands TO on;
 SET citus.grep_remote_commands = '%SECURITY LABEL%';
 SELECT 1 FROM citus_add_node('localhost', :worker_2_port);
-NOTICE:  issuing SELECT worker_create_or_alter_role('user1', 'CREATE ROLE user1 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT NOLOGIN NOREPLICATION NOBYPASSRLS CONNECTION LIMIT -1 PASSWORD NULL VALID UNTIL ''infinity''', 'ALTER ROLE user1 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT NOLOGIN NOREPLICATION NOBYPASSRLS CONNECTION LIMIT -1 PASSWORD NULL VALID UNTIL ''infinity''');SECURITY LABEL FOR citus_tests_label_provider ON ROLE user1 IS 'citus unclassified'
+NOTICE:  issuing SELECT worker_create_or_alter_role('user1', 'CREATE ROLE user1 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT NOLOGIN NOREPLICATION NOBYPASSRLS CONNECTION LIMIT -1 PASSWORD NULL VALID UNTIL ''infinity''', 'ALTER ROLE user1 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT NOLOGIN NOREPLICATION NOBYPASSRLS CONNECTION LIMIT -1 PASSWORD NULL VALID UNTIL ''infinity''');SECURITY LABEL FOR citus_tests_label_provider ON ROLE user1 IS 'citus_unclassified'
 DETAIL:  on server postgres@localhost:xxxxx connectionId: xxxxxxx
-NOTICE:  issuing SELECT worker_create_or_alter_role('user 2', 'CREATE ROLE "user 2" NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT NOLOGIN NOREPLICATION NOBYPASSRLS CONNECTION LIMIT -1 PASSWORD NULL VALID UNTIL ''infinity''', 'ALTER ROLE "user 2" NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT NOLOGIN NOREPLICATION NOBYPASSRLS CONNECTION LIMIT -1 PASSWORD NULL VALID UNTIL ''infinity''');SECURITY LABEL FOR citus_tests_label_provider ON ROLE "user 2" IS 'citus classified'
+NOTICE:  issuing SELECT worker_create_or_alter_role('user 2', 'CREATE ROLE "user 2" NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT NOLOGIN NOREPLICATION NOBYPASSRLS CONNECTION LIMIT -1 PASSWORD NULL VALID UNTIL ''infinity''', 'ALTER ROLE "user 2" NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT NOLOGIN NOREPLICATION NOBYPASSRLS CONNECTION LIMIT -1 PASSWORD NULL VALID UNTIL ''infinity''');SECURITY LABEL FOR "citus '!tests_label_provider" ON ROLE "user 2" IS 'citus ''!unclassified'
 DETAIL:  on server postgres@localhost:xxxxx connectionId: xxxxxxx
  ?column?
 ---------------------------------------------------------------------
         1
 (1 row)
 
 SELECT node_type, result FROM get_citus_tests_label_provider_labels('user1') ORDER BY node_type;
-  node_type  |                       result
+  node_type  |                                            result
 ---------------------------------------------------------------------
- coordinator | {"label": "citus unclassified", "objtype": "role"}
- worker_1    | {"label": "citus unclassified", "objtype": "role"}
- worker_2    | {"label": "citus unclassified", "objtype": "role"}
+ coordinator | {"label": "citus_unclassified", "objtype": "role", "provider": "citus_tests_label_provider"}
+ worker_1    | {"label": "citus_unclassified", "objtype": "role", "provider": "citus_tests_label_provider"}
+ worker_2    | {"label": "citus_unclassified", "objtype": "role", "provider": "citus_tests_label_provider"}
 (3 rows)
 
 SELECT node_type, result FROM get_citus_tests_label_provider_labels('"user 2"') ORDER BY node_type;
-  node_type  |                      result
+  node_type  |                                              result
 ---------------------------------------------------------------------
- coordinator | {"label": "citus classified", "objtype": "role"}
- worker_1    | {"label": "citus classified", "objtype": "role"}
- worker_2    | {"label": "citus classified", "objtype": "role"}
+ coordinator | {"label": "citus '!unclassified", "objtype": "role", "provider": "citus '!tests_label_provider"}
+ worker_1    | {"label": "citus '!unclassified", "objtype": "role", "provider": "citus '!tests_label_provider"}
+ worker_2    | {"label": "citus '!unclassified", "objtype": "role", "provider": "citus '!tests_label_provider"}
 (3 rows)
 
 -- cleanup

diff --git a/src/test/regress/sql/multi_test_helpers.sql b/src/test/regress/sql/multi_test_helpers.sql
@@ -551,8 +551,8 @@ BEGIN
 END;
 $func$ LANGUAGE plpgsql;
 
--- Returns pg_seclabels entries from all nodes in the cluster
--- for which the provider is citus_tests_label_provider and the object name is the input
+-- Returns pg_seclabels entries from all nodes in the cluster for which
+-- the object name is the input.
 CREATE OR REPLACE FUNCTION get_citus_tests_label_provider_labels(object_name text,
                                                                  master_port INTEGER DEFAULT 57636,
                                                                  worker_1_port INTEGER DEFAULT 57637,
@@ -564,9 +564,8 @@ RETURNS TABLE (
 AS $func$
 DECLARE
     pg_seclabels_cmd TEXT := 'SELECT to_jsonb(q.*) FROM (' ||
-                             'SELECT objtype, label FROM pg_seclabels ' ||
-                             'WHERE provider = ''citus_tests_label_provider'' AND ' ||
-                             'objname = ''' || object_name || ''') q';
+                             'SELECT provider, objtype, label FROM pg_seclabels ' ||
+                             'WHERE objname = ''' || object_name || ''') q';
 BEGIN
     RETURN QUERY
     SELECT

diff --git a/src/test/regress/sql/seclabel.sql b/src/test/regress/sql/seclabel.sql
@@ -13,11 +13,11 @@ CREATE ROLE user1;
 CREATE ROLE "user 2";
 
 -- check an invalid label for our current dummy hook citus_test_object_relabel
-SECURITY LABEL ON ROLE user1 IS 'invalid_label';
+SECURITY LABEL FOR citus_tests_label_provider ON ROLE user1 IS 'invalid_label';
 
 -- if we disable metadata_sync, the command will not be propagated
 SET citus.enable_metadata_sync TO off;
-SECURITY LABEL ON ROLE user1 IS 'citus unclassified';
+SECURITY LABEL FOR citus_tests_label_provider ON ROLE user1 IS 'citus_unclassified';
 SELECT node_type, result FROM get_citus_tests_label_provider_labels('user1') ORDER BY node_type;
 
 RESET citus.enable_metadata_sync;
@@ -33,18 +33,18 @@ CREATE VIEW v_dist AS SELECT * FROM a;
 CREATE FUNCTION notice(text) RETURNS void LANGUAGE plpgsql AS $$
     BEGIN RAISE NOTICE '%', $1; END; $$;
 
-SECURITY LABEL ON TABLE a IS 'citus classified';
-SECURITY LABEL ON FUNCTION notice IS 'citus unclassified';
-SECURITY LABEL ON VIEW v_dist IS 'citus classified';
+SECURITY LABEL FOR citus_tests_label_provider ON TABLE a IS 'citus_classified';
+SECURITY LABEL FOR citus_tests_label_provider ON FUNCTION notice IS 'citus_unclassified';
+SECURITY LABEL FOR citus_tests_label_provider ON VIEW v_dist IS 'citus_classified';
 
 SELECT node_type, result FROM get_citus_tests_label_provider_labels('a') ORDER BY node_type;
 SELECT node_type, result FROM get_citus_tests_label_provider_labels('notice(text)') ORDER BY node_type;
 SELECT node_type, result FROM get_citus_tests_label_provider_labels('v_dist') ORDER BY node_type;
 
 \c - - - :worker_1_port
-SECURITY LABEL ON TABLE a IS 'citus classified';
-SECURITY LABEL ON FUNCTION notice IS 'citus unclassified';
-SECURITY LABEL ON VIEW v_dist IS 'citus classified';
+SECURITY LABEL FOR citus_tests_label_provider ON TABLE a IS 'citus_classified';
+SECURITY LABEL FOR citus_tests_label_provider ON FUNCTION notice IS 'citus_unclassified';
+SECURITY LABEL FOR citus_tests_label_provider ON VIEW v_dist IS 'citus_classified';
 
 \c - - - :master_port
 SELECT node_type, result FROM get_citus_tests_label_provider_labels('a') ORDER BY node_type;
@@ -58,14 +58,14 @@ DROP FUNCTION notice;
 SET citus.log_remote_commands TO on;
 SET citus.grep_remote_commands = '%SECURITY LABEL%';
 
-SECURITY LABEL for citus_tests_label_provider ON ROLE user1 IS 'citus classified';
+SECURITY LABEL for citus_tests_label_provider ON ROLE user1 IS 'citus_classified';
 SECURITY LABEL for citus_tests_label_provider ON ROLE user1 IS NULL;
-SECURITY LABEL for citus_tests_label_provider ON ROLE user1 IS 'citus unclassified';
-SECURITY LABEL for citus_tests_label_provider ON ROLE "user 2" IS 'citus classified';
+SECURITY LABEL for citus_tests_label_provider ON ROLE user1 IS 'citus_unclassified';
+SECURITY LABEL for "citus '!tests_label_provider" ON ROLE "user 2" IS 'citus ''!unclassified';
 
 \c - - - :worker_1_port
 -- command not allowed from worker node
-SECURITY LABEL for citus_tests_label_provider ON ROLE user1 IS 'citus unclassified';
+SECURITY LABEL for citus_tests_label_provider ON ROLE user1 IS 'citus ''!unclassified';
 
 \c - - - :master_port
 RESET citus.log_remote_commands;