From 79a419e033df6c01ffd62eb20feedb01170e1315 Mon Sep 17 00:00:00 2001
From: Miroslav Lisik <mlisik@redhat.com>
Date: Thu, 25 Jan 2024 15:25:05 +0100
Subject: [PATCH] fix: do not put empty uid/gid options to an uidgid file

* fixed command `pcs cluster uidgid add`

Resolves #772
---
 CHANGELOG.md                          |  10 ++
 pcs/cluster.py                        |   9 +-
 pcs/utils.py                          |  32 +++++-
 pcs_test/tier1/legacy/test_cluster.py | 154 ++++++++++++++++++++++----
 4 files changed, 173 insertions(+), 32 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index ee5aab0b..62e7f230 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,15 @@
 # Change Log
 
+## [Unreleased]
+
+### Fixed
+- Do not put empty uid/gid corosync configuration options to an uidgid file
+  when not specified in `pcs cluster uidgid add` command. Empty options cause
+  corosync start failure. ([ghissue#772])
+
+[ghissue#772]: https://github.com/ClusterLabs/pcs/issues/772
+
+
 ## [0.11.7] - 2024-01-11
 
 ### Security
diff --git a/pcs/cluster.py b/pcs/cluster.py
index 01689684..070287c1 100644
--- a/pcs/cluster.py
+++ b/pcs/cluster.py
@@ -1176,8 +1176,8 @@ def cluster_uidgid(
     del lib
     modifiers.ensure_only_supported()
     if not argv:
-        found = False
         uid_gid_files = os.listdir(settings.corosync_uidgid_dir)
+        uid_gid_lines: list[str] = []
         for ug_file in uid_gid_files:
             uid_gid_dict = utils.read_uid_gid_file(ug_file)
             if "uid" in uid_gid_dict or "gid" in uid_gid_dict:
@@ -1188,9 +1188,10 @@ def cluster_uidgid(
                 if "gid" in uid_gid_dict:
                     line += uid_gid_dict["gid"]
 
-                print(line)
-                found = True
-        if not found and not silent_list:
+                uid_gid_lines.append(line)
+        if uid_gid_lines:
+            print("\n".join(sorted(uid_gid_lines)))
+        elif not silent_list:
             print_to_stderr("No uidgids configured")
         return
 
diff --git a/pcs/utils.py b/pcs/utils.py
index 44987be1..188f75bc 100644
--- a/pcs/utils.py
+++ b/pcs/utils.py
@@ -16,9 +16,11 @@ import xml.dom.minidom
 import xml.etree.ElementTree as ET
 from functools import lru_cache
 from io import BytesIO
+from textwrap import dedent
 from typing import (
     Any,
     Dict,
+    Optional,
     Tuple,
 )
 from urllib.parse import urlencode
@@ -221,6 +223,25 @@ def read_uid_gid_file(uidgid_filename):
     return uidgid
 
 
+def get_uidgid_file_content(
+    uid: Optional[str] = None, gid: Optional[str] = None
+) -> Optional[str]:
+    if not uid and not gid:
+        return None
+    uid_gid_lines = []
+    if uid:
+        uid_gid_lines.append(f"  uid: {uid}")
+    if gid:
+        uid_gid_lines.append(f"  gid: {gid}")
+    return dedent(
+        """\
+        uidgid {{
+        {uid_gid_keys}
+        }}
+        """
+    ).format(uid_gid_keys="\n".join(uid_gid_lines))
+
+
 def write_uid_gid_file(uid, gid):
     """
     Commandline options: no options
@@ -237,11 +258,12 @@ def write_uid_gid_file(uid, gid):
         counter = counter + 1
         uidgid_filename = orig_filename + "-" + str(counter)
 
-    data = "uidgid {\n  uid: %s\ngid: %s\n}\n" % (uid, gid)
-    with open(
-        os.path.join(settings.corosync_uidgid_dir, uidgid_filename), "w"
-    ) as uidgid_file:
-        uidgid_file.write(data)
+    data = get_uidgid_file_content(uid, gid)
+    if data:
+        with open(
+            os.path.join(settings.corosync_uidgid_dir, uidgid_filename), "w"
+        ) as uidgid_file:
+            uidgid_file.write(data)
 
 
 def find_uid_gid_files(uid, gid):
diff --git a/pcs_test/tier1/legacy/test_cluster.py b/pcs_test/tier1/legacy/test_cluster.py
index 6e72fe80..03064000 100644
--- a/pcs_test/tier1/legacy/test_cluster.py
+++ b/pcs_test/tier1/legacy/test_cluster.py
@@ -1,3 +1,4 @@
+import os
 from functools import partial
 from unittest import TestCase
 
@@ -6,6 +7,8 @@ from pcs_test.tools.misc import get_test_resource as rc
 from pcs_test.tools.misc import (
     get_tmp_dir,
     get_tmp_file,
+    outdent,
+    read_test_resource,
     skip_unless_root,
     write_file_to_tmpfile,
 )
@@ -18,28 +21,40 @@ from pcs_test.tools.pcs_runner import (
 class UidGidTest(TestCase):
     def setUp(self):
         self.uid_gid_dir = get_tmp_dir("tier1_cluster_uidgid")
+        self._pcs = partial(
+            pcs,
+            None,
+            mock_settings={"corosync_uidgid_dir": self.uid_gid_dir.name},
+        )
 
     def tearDown(self):
         self.uid_gid_dir.cleanup()
 
+    def assert_uidgid_file_content(self, filename, content):
+        self.assertEqual(
+            read_test_resource(os.path.join(self.uid_gid_dir.name, filename)),
+            content,
+        )
+
+    def assert_uidgid_file_removed(self, filename):
+        self.assertEqual(
+            os.path.isfile(os.path.join(self.uid_gid_dir.name, filename)),
+            False,
+        )
+
     def test_uidgid(self):
         # pylint: disable=too-many-statements
-        _pcs = partial(
-            pcs,
-            None,
-            mock_settings={"corosync_uidgid_dir": self.uid_gid_dir.name},
-        )
-        stdout, stderr, retval = _pcs("cluster uidgid".split())
+        stdout, stderr, retval = self._pcs("cluster uidgid".split())
         self.assertEqual(stdout, "")
         self.assertEqual(stderr, "No uidgids configured\n")
         self.assertEqual(retval, 0)
 
-        stdout, stderr, retval = _pcs("cluster uidgid add".split())
+        stdout, stderr, retval = self._pcs("cluster uidgid add".split())
         self.assertEqual(stdout, "")
         self.assertTrue(stderr.startswith("\nUsage:"))
         self.assertEqual(retval, 1)
 
-        stdout, stderr, retval = _pcs("cluster uidgid rm".split())
+        stdout, stderr, retval = self._pcs("cluster uidgid rm".split())
         self.assertEqual(stdout, "")
         self.assertTrue(
             stderr.startswith(
@@ -49,19 +64,30 @@ class UidGidTest(TestCase):
         )
         self.assertEqual(retval, 1)
 
-        stdout, stderr, retval = _pcs("cluster uidgid xx".split())
+        stdout, stderr, retval = self._pcs("cluster uidgid xx".split())
         self.assertEqual(stdout, "")
         self.assertTrue(stderr.startswith("\nUsage:"))
         self.assertEqual(retval, 1)
 
-        stdout, stderr, retval = _pcs(
+        stdout, stderr, retval = self._pcs(
             "cluster uidgid add uid=testuid gid=testgid".split()
         )
         self.assertEqual(stdout, "")
         self.assertEqual(stderr, "")
         self.assertEqual(retval, 0)
+        self.assert_uidgid_file_content(
+            "pcs-uidgid-testuid-testgid",
+            outdent(
+                """\
+                uidgid {
+                  uid: testuid
+                  gid: testgid
+                }
+                """
+            ),
+        )
 
-        stdout, stderr, retval = _pcs(
+        stdout, stderr, retval = self._pcs(
             "cluster uidgid add uid=testuid gid=testgid".split()
         )
         self.assertEqual(stdout, "")
@@ -72,7 +98,7 @@ class UidGidTest(TestCase):
         )
         self.assertEqual(retval, 1)
 
-        stdout, stderr, retval = _pcs(
+        stdout, stderr, retval = self._pcs(
             "cluster uidgid delete uid=testuid2 gid=testgid2".split()
         )
         self.assertEqual(stdout, "")
@@ -82,7 +108,7 @@ class UidGidTest(TestCase):
         )
         self.assertEqual(retval, 1)
 
-        stdout, stderr, retval = _pcs(
+        stdout, stderr, retval = self._pcs(
             "cluster uidgid remove uid=testuid gid=testgid2".split()
         )
         self.assertEqual(stdout, "")
@@ -92,7 +118,7 @@ class UidGidTest(TestCase):
         )
         self.assertEqual(retval, 1)
 
-        stdout, stderr, retval = _pcs(
+        stdout, stderr, retval = self._pcs(
             "cluster uidgid rm uid=testuid2 gid=testgid".split()
         )
         self.assertEqual(stdout, "")
@@ -104,57 +130,139 @@ class UidGidTest(TestCase):
         )
         self.assertEqual(retval, 1)
 
-        stdout, stderr, retval = _pcs("cluster uidgid".split())
+        stdout, stderr, retval = self._pcs("cluster uidgid".split())
         self.assertEqual(stdout, "UID/GID: uid=testuid gid=testgid\n")
         self.assertEqual(stderr, "")
         self.assertEqual(retval, 0)
 
-        stdout, stderr, retval = _pcs(
+        stdout, stderr, retval = self._pcs(
             "cluster uidgid delete uid=testuid gid=testgid".split()
         )
         self.assertEqual(stdout, "")
         self.assertEqual(stderr, "")
         self.assertEqual(retval, 0)
+        self.assert_uidgid_file_removed("pcs-uidgid-testuid-testgid")
 
-        stdout, stderr, retval = _pcs(
+        stdout, stderr, retval = self._pcs(
             "cluster uidgid add uid=testuid gid=testgid".split()
         )
         self.assertEqual(stdout, "")
         self.assertEqual(stderr, "")
         self.assertEqual(retval, 0)
 
-        stdout, stderr, retval = _pcs("cluster uidgid".split())
+        stdout, stderr, retval = self._pcs("cluster uidgid".split())
         self.assertEqual(stdout, "UID/GID: uid=testuid gid=testgid\n")
         self.assertEqual(stderr, "")
         self.assertEqual(retval, 0)
 
-        stdout, stderr, retval = _pcs(
+        stdout, stderr, retval = self._pcs(
             "cluster uidgid remove uid=testuid gid=testgid".split()
         )
         self.assertEqual(stdout, "")
         self.assertEqual(stderr, "")
         self.assertEqual(retval, 0)
 
-        stdout, stderr, retval = _pcs(
+        stdout, stderr, retval = self._pcs(
             "cluster uidgid add uid=testuid gid=testgid".split()
         )
         self.assertEqual(stdout, "")
         self.assertEqual(stderr, "")
         self.assertEqual(retval, 0)
+        self.assert_uidgid_file_content(
+            "pcs-uidgid-testuid-testgid",
+            outdent(
+                """\
+                uidgid {
+                  uid: testuid
+                  gid: testgid
+                }
+                """
+            ),
+        )
 
-        stdout, stderr, retval = _pcs("cluster uidgid".split())
+        stdout, stderr, retval = self._pcs("cluster uidgid".split())
         self.assertEqual(stdout, "UID/GID: uid=testuid gid=testgid\n")
         self.assertEqual(stderr, "")
         self.assertEqual(retval, 0)
 
-        stdout, stderr, retval = _pcs(
+        stdout, stderr, retval = self._pcs(
             "cluster uidgid delete uid=testuid gid=testgid".split()
         )
         self.assertEqual(stdout, "")
         self.assertEqual(stderr, "")
         self.assertEqual(retval, 0)
+        self.assert_uidgid_file_removed("pcs-uidgid-testuid-testgid")
+
+        stdout, stderr, retval = self._pcs("cluster uidgid".split())
+        self.assertEqual(stdout, "")
+        self.assertEqual(stderr, "No uidgids configured\n")
+        self.assertEqual(retval, 0)
+
+    def test_missing_uid_gid(self):
+        stdout, stderr, retval = self._pcs(
+            "cluster uidgid add uid=1000".split()
+        )
+        self.assertEqual(stdout, "")
+        self.assertEqual(stderr, "")
+        self.assertEqual(retval, 0)
+        self.assert_uidgid_file_content(
+            "pcs-uidgid-1000-",
+            outdent(
+                """\
+                uidgid {
+                  uid: 1000
+                }
+                """
+            ),
+        )
+
+        stdout, stderr, retval = self._pcs(
+            "cluster uidgid add gid=1001".split()
+        )
+        self.assertEqual(stdout, "")
+        self.assertEqual(stderr, "")
+        self.assertEqual(retval, 0)
+        self.assert_uidgid_file_content(
+            "pcs-uidgid--1001",
+            outdent(
+                """\
+                uidgid {
+                  gid: 1001
+                }
+                """
+            ),
+        )
+
+        stdout, stderr, retval = self._pcs("cluster uidgid".split())
+        self.assertEqual(
+            stdout,
+            outdent(
+                """\
+                UID/GID: uid= gid=1001
+                UID/GID: uid=1000 gid=
+                """
+            ),
+        )
+        self.assertEqual(stderr, "")
+        self.assertEqual(retval, 0)
+
+        stdout, stderr, retval = self._pcs(
+            "cluster uidgid delete uid=1000".split()
+        )
+        self.assertEqual(stdout, "")
+        self.assertEqual(stderr, "")
+        self.assertEqual(retval, 0)
+        self.assert_uidgid_file_removed("pcs-uidgid-1000-")
+
+        stdout, stderr, retval = self._pcs(
+            "cluster uidgid delete gid=1001".split()
+        )
+        self.assertEqual(stdout, "")
+        self.assertEqual(stderr, "")
+        self.assertEqual(retval, 0)
+        self.assert_uidgid_file_removed("pcs-uidgid--1001")
 
-        stdout, stderr, retval = _pcs("cluster uidgid".split())
+        stdout, stderr, retval = self._pcs("cluster uidgid".split())
         self.assertEqual(stdout, "")
         self.assertEqual(stderr, "No uidgids configured\n")
         self.assertEqual(retval, 0)
-- 
2.25.1

