DAOS-13670 control: Add system name validation on startup (#12644)

Explicitly check the configured system name and fail if
it does not meet requirements.

Miscellaneous small improvements of the src/tests/ftest/control/version.py test.

Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@intel.com>
Co-authored-by: Michael MacDonald <mjmac.macdonald@intel.com>

src/control/cmd/daos_agent/config.go

@@ -16,6 +16,7 @@ import (
 
 	"github.com/daos-stack/daos/src/control/build"
 	"github.com/daos-stack/daos/src/control/common"
+	"github.com/daos-stack/daos/src/control/lib/daos"
 	"github.com/daos-stack/daos/src/control/security"
 )
 
@@ -83,6 +84,11 @@ func LoadConfig(cfgPath string) (*Config, error) {
 	if err := yaml.UnmarshalStrict(data, cfg); err != nil {
 		return nil, err
 	}
+
+	if !daos.SystemNameIsValid(cfg.SystemName) {
+		return nil, fmt.Errorf("invalid system name: %q", cfg.SystemName)
+	}
+
 	return cfg, nil
 }
 

src/control/cmd/dmg/main.go

@@ -222,6 +222,12 @@ and access control settings, along with system wide operations.`
 			logCmd.SetLog(log)
 		}
 
+		switch cmd.(type) {
+		case *versionCmd:
+			// this command don't need the rest of the setup
+			return cmd.Execute(args)
+		}
+
 		ctlCfg, err := control.LoadConfig(opts.ConfigPath)
 		if err != nil {
 			if errors.Cause(err) != control.ErrNoConfigFile {

src/control/lib/control/config.go

@@ -15,6 +15,7 @@ import (
 	"gopkg.in/yaml.v2"
 
 	"github.com/daos-stack/daos/src/control/build"
+	"github.com/daos-stack/daos/src/control/lib/daos"
 	"github.com/daos-stack/daos/src/control/security"
 )
 
@@ -93,5 +94,9 @@ func LoadConfig(cfgPath string) (*Config, error) {
 	}
 	cfg.Path = cfgPath
 
+	if !daos.SystemNameIsValid(cfg.SystemName) {
+		return nil, fmt.Errorf("invalid system name: %q", cfg.SystemName)
+	}
+
 	return cfg, nil
 }

src/control/lib/daos/system_prop.go

@@ -18,6 +18,21 @@ import (
 	"github.com/daos-stack/daos/src/control/common"
 )
 
+/*
+#include <daos_types.h>
+*/
+import "C"
+
+// SystemNameIsValid returns true if the given name is valid for a DAOS system.
+func SystemNameIsValid(name string) bool {
+	// NB: So far, this seems to be the only constraint on system names.
+	if name == "" || len(name) > C.DAOS_SYS_NAME_MAX {
+		return false
+	}
+
+	return true
+}
+
 // BoolPropVal is a boolean property value.
 type BoolPropVal bool
 

src/control/server/config/server.go

@@ -22,6 +22,7 @@ import (
 	"github.com/daos-stack/daos/src/control/build"
 	"github.com/daos-stack/daos/src/control/common"
 	"github.com/daos-stack/daos/src/control/fault"
+	"github.com/daos-stack/daos/src/control/lib/daos"
 	"github.com/daos-stack/daos/src/control/logging"
 	"github.com/daos-stack/daos/src/control/security"
 	"github.com/daos-stack/daos/src/control/server/engine"
@@ -354,6 +355,10 @@ func (cfg *Server) Load() error {
 			cfg.Path)
 	}
 
+	if !daos.SystemNameIsValid(cfg.SystemName) {
+		return errors.Errorf("invalid system name: %q", cfg.SystemName)
+	}
+
 	// Update server config based on legacy parameters.
 	if err := updateFromLegacyParams(cfg); err != nil {
 		return errors.Wrap(err, "updating config from legacy parameters")

src/tests/ftest/control/daos_agent_config.py

@@ -35,7 +35,7 @@ def test_daos_agent_config_basic(self):
         :avocado: tags=all,daily_regression
         :avocado: tags=vm
         :avocado: tags=control,basic
-        :avocado: tags=agent_start,daos_agent_config_test,test_daos_agent_config_basic
+        :avocado: tags=DaosAgentConfigTest,test_daos_agent_config_basic
         """
         # Setup the agents
         self.add_agent_manager()

src/tests/ftest/control/daos_agent_config.yaml

@@ -32,8 +32,13 @@ agent_config_val: !mux
   name_special_chars:
     config_val:
       - "name"
-      - "! @#$%^&*()_+{}|:<>?-=[];',./"
+      - "! @#$%^&*()_+{}"
       - "PASS"
+  name_exceeds_max_length:
+    config_val:
+      - "name"
+      - "! @#$%^&*()_+{}|:<>?-=[];',./"
+      - "FAIL"
   name_alphanumeric:
     config_val:
       - "name"

src/tests/ftest/control/daos_control_config.py

@@ -26,7 +26,7 @@ def test_daos_control_config_basic(self):
         :avocado: tags=all,daily_regression
         :avocado: tags=vm
         :avocado: tags=control,basic
-        :avocado: tags=control_start,test_daos_control_config_basic
+        :avocado: tags=DaosControlConfigTest,test_daos_control_config_basic
         """
         # Get the input to verify
         c_val = self.params.get("config_val", "/run/control_config_val/*/")

src/tests/ftest/control/daos_control_config.yaml

@@ -32,8 +32,13 @@ control_config_val: !mux
   name_special_chars:
     config_val:
       - "name"
-      - "! @#$%^&*()_+{}|:<>?-=[];',./"
+      - "! @#$%^&*()_+{}"
       - "PASS"
+  name_exceeds_max_length:
+    config_val:
+      - "name"
+      - "! @#$%^&*()_+{}|:<>?-=[];',./"
+      - "FAIL"
   name_numeric:
     config_val:
       - "name"

src/tests/ftest/control/version.py

@@ -7,7 +7,7 @@
 import json
 
 from apricot import TestWithServers
-from general_utils import run_pcmd, report_errors
+from general_utils import run_pcmd, report_errors, append_error
 from server_utils_base import DaosServerCommandRunner
 
 
@@ -22,6 +22,7 @@ class DAOSVersion(TestWithServers):
     def __init__(self, *args, **kwargs):
         """Initialize a DAOSVersion object."""
         super().__init__(*args, **kwargs)
+
         # Don't waste time starting servers and agents.
         self.setup_start_servers = False
         self.setup_start_agents = False
@@ -32,22 +33,34 @@ def test_version(self):
         :avocado: tags=all,full_regression
         :avocado: tags=vm
         :avocado: tags=control,daos_cmd
-        :avocado: tags=version_number,test_version
+        :avocado: tags=DAOSVersion,test_version
         """
-        errors = []
-
         # Get RPM version.
-        rpm_command = "rpm -qa|grep daos-server"
+        rpm_command = "rpm -qa | grep daos-server"
         output = run_pcmd(hosts=self.hostlist_servers, command=rpm_command)
-        self.log.info("RPM output = %s", output)
-        stdout = output[0]["stdout"][0]
-        self.log.info("RPM stdout = %s", stdout)
-        result = re.findall(r"daos-server-[tests-|tests_openmpi-]*([\d.]+)", stdout)
+        self.log.debug("RPM output = %s", output)
+        rc = output[0]["exit_status"]
+        stdout = output[0]["stdout"]
+        if rc != 0:
+            report_errors(self, ["DAOS RPMs not properly installed: rc={}".format(rc)])
+        rpm_version = None
+        for rpm in stdout:
+            result = re.findall(r"daos-server-[tests-|tests_openmpi-]*([\d.]+)", rpm)
+            if result:
+                rpm_version = result[0]
+                break
         if not result:
-            errors.append("RPM version is not in the output! {}".format(output))
-        else:
-            rpm_version = result[0]
-            self.log.info("RPM version = %s", rpm_version)
+            report_errors(self, ["RPM version could not be defined"])
+        self.log.info("RPM version = %s", rpm_version)
+
+        # Remove configuration files
+        cleanup_cmds = [
+            "sudo find /etc/daos/certs -type f -delete -print",
+            "sudo rm -fv /etc/daos/daos_server.yml /etc/daos/daos_control.yml"
+            "     /etc/daos/daos_agent.yml",
+        ]
+        for cmd in cleanup_cmds:
+            run_pcmd(hosts=self.hostlist_servers, command=cmd)
 
         # Get dmg version.
         dmg_version = self.get_dmg_command().version()["response"]["version"]
@@ -57,11 +70,22 @@ def test_version(self):
         daos_version = self.get_daos_command().version()["response"]["version"]
         self.log.info("daos version = %s", daos_version)
 
+        errors = []
+
         # Get daos_agent version.
+        daos_agent_version = None
         daos_agent_cmd = "daos_agent --json version"
         output = run_pcmd(hosts=self.hostlist_servers, command=daos_agent_cmd)
-        daos_agent_version = json.loads("".join(output[0]["stdout"]))["response"]["version"]
-        self.log.info("daos_agent version = %s", daos_agent_version)
+        self.log.debug("DAOS Agent output = %s", output)
+        rc = output[0]["exit_status"]
+        stdout = output[0]["stdout"]
+        if rc != 0:
+            msg = "DAOS Agent not properly installed: rc={}".format(rc)
+            append_error(errors, msg, stdout)
+        else:
+            self.log.info("DAOS Agent stdout = %s", "".join(stdout))
+            daos_agent_version = json.loads("".join(stdout))["response"]["version"]
+            self.log.info("daos_agent version = %s", daos_agent_version)
 
         # Get daos_server version
         daos_server_cmd = DaosServerCommandRunner(path=self.bin)
@@ -75,15 +99,13 @@ def test_version(self):
             ("daos_agent", daos_agent_version),
             ("daos_server", daos_server_version)
         ]
-
         for tool_version in tool_versions:
             tool = tool_version[0]
             version = tool_version[1]
             if version != rpm_version:
                 msg = "Unexpected version! {} = {}, RPM = {}".format(
                     tool, version, rpm_version)
-                errors.append(msg)
+                append_error(errors, msg)
 
-        self.log.info("###### Test Result ######")
-        report_errors(test=self, errors=errors)
-        self.log.info("#########################")
+        report_errors(self, errors)
+        self.log.info("Test passed")

src/tests/ftest/control/version.yaml

@@ -12,3 +12,13 @@ server_config:
           class: ram
           scm_mount: /mnt/daos
   system_ram_reserved: 1
+  transport_config:
+    allow_insecure: true
+
+agent_config:
+  transport_config:
+    allow_insecure: true
+
+dmg:
+  transport_config:
+    allow_insecure: true

src/tests/ftest/server/daos_server_config.py

@@ -1,5 +1,5 @@
 """
-  (C) Copyright 2020-2022 Intel Corporation.
+  (C) Copyright 2020-2023 Intel Corporation.
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
 """
@@ -46,6 +46,14 @@ def test_daos_server_config_basic(self):
         # Get the input to verify
         c_val = self.params.get("config_val", "/run/server_config_val/*/")
 
+        if c_val[0] == "name":
+            # Set the dmg system name to match the server in order to avoid
+            # mismatch failures that aren't part of this test
+            self.assertTrue(
+                self.server_managers[-1].dmg.set_config_value(c_val[0], c_val[1]),
+                "Error setting the '{}' config file parameter to '{}'".format(
+                    c_val[0], c_val[1]))
+
         # Identify the attribute and modify its value to test value
         self.assertTrue(
             self.server_managers[0].set_config_value(c_val[0], c_val[1]),

src/tests/ftest/util/general_utils.py

@@ -1317,6 +1317,20 @@ def get_display_size(size):
         bytes_to_human(size, binary=False))
 
 
+def append_error(errors, title, details=None):
+    """Helper adding an error to the list of errors
+
+    Args:
+        errors (list): List of error messages
+        title (str): Error message title
+        details (list, optional): List of string of the error details
+    """
+    msg = title
+    if details:
+        msg += "\n\t" + "\n\t".join(details)
+    errors.append(msg)
+
+
 def report_errors(test, errors):
     """Print errors and fail the test if there's any errors.