To Fix Bug in the

config/kdump.py

@@ -4,6 +4,25 @@
 from utilities_common.cli import AbbreviationGroup, pass_db
 from ipaddress import ip_address, AddressValueError
 import re
+
+
+def is_valid_ssh_string(ssh_string):
+    import ipaddress
+    try:
+        user, ip = ssh_string.split("@")
+        print(f"Checking IP: {ip}")
+        ipaddress.ip_address(ip)
+        return True
+    except Exception as e:
+        print(f"Validation failed: {e}")
+        return False
+
+
+def is_valid_path(path):
+    # You can make this stricter as needed (checking existence or just syntax)
+    return bool(path.strip()) and path.startswith("/")
+
+
 #
 # 'kdump' group ('sudo config kdump ...')
 #
@@ -98,158 +117,141 @@ def kdump_num_dumps(db, kdump_num_dumps):
     db.cfgdb.mod_entry("KDUMP", "config", {"num_dumps": kdump_num_dumps})
     echo_reboot_warning()
 
+#
+# 'remote' command ('sudo config kdump remote enable/disable')
+#
+
+
+@kdump.group(name="remote", help="Enable or disable remote KDUMP configuration")
+def kdump_remote():
+    pass
+
+#
+# 'remote' command ('sudo config kdump remote enable ...')
+#
+
 
-@kdump.command(name="remote", help="Configure the remote enable/disable for KDUMP mechanism")
-@click.argument('action', metavar='<enable/disable>', required=True, type=click.STRING)  # Corrected this line
+@kdump_remote.command(name="enable", help="Enable remote KDUMP configuration")
 @pass_db
-def remote(db, action):
-    """Enable or disable remote kdump feature"""
+def remote_enable(db):
+    """Enable remote KDUMP"""
     kdump_table = db.cfgdb.get_table("KDUMP")
     check_kdump_table_existence(kdump_table)
 
-    # Get the current status of the remote feature as string
-    current_status = kdump_table["config"].get("remote", "false").lower()
-
-    if action.lower() == 'enable':
-        if current_status == "true":
-            click.echo("Remote kdump feature is already enabled.")
-        else:
-            db.cfgdb.mod_entry("KDUMP", "config", {"remote": "true"})
-            click.echo("Remote kdump feature enabled.")
-            echo_reboot_warning()
-    elif action.lower() == 'disable':
-        if current_status == "false":
-            click.echo("Remote kdump feature is already disabled.")
-        else:
-            db.cfgdb.mod_entry("KDUMP", "config", {"remote": "false"})
-            click.echo("Remote kdump feature disabled.")
-            echo_reboot_warning()
-    else:
-        click.echo("Invalid action. Use 'enable' or 'disable'.")
+    current_val = kdump_table.get("config", {}).get("remote", "false").lower()
 
+    if current_val == "true":
+        click.echo("Remote KDUMP is already enabled.")
+    else:
+        db.cfgdb.mod_entry("KDUMP", "config", {"remote": "true"})
+        click.echo("Remote KDUMP has been enabled.")
+        echo_reboot_warning()
 
-@kdump.group(name="add", help="Add configuration items to KDUMP")
-def add():
-    """Group of commands to add configuration items to KDUMP"""
-    pass
+#
+# 'remote' command ('sudo config kdump remote disable ...')
+#
 
 
-@add.command(name="ssh_string", help="Add an SSH string to the KDUMP configuration")
-@click.argument('ssh_string', metavar='<ssh_key>', required=True)
+@kdump_remote.command(name="disable", help="Disable remote KDUMP configuration")
 @pass_db
-def add_ssh_key(db, ssh_string):
-    """Add an SSH string to KDUMP configuration"""
+def remote_disable(db):
+    """Disable remote KDUMP"""
+    kdump_table = db.cfgdb.get_table("KDUMP")
+    check_kdump_table_existence(kdump_table)
 
-    def is_valid_ssh_key(ssh_string):
-        """Validate the SSH key format"""
-        # Check if it contains username and hostname/IP (format: username@host)
-        if "@" not in ssh_string:
-            return "Invalid format. SSH key must be in 'username@host' format."
+    current_val = kdump_table.get("config", {}).get("remote", "false").lower()
 
-        username, host = ssh_string.split("@", 1)
+    if current_val == "false":
+        click.echo("Remote KDUMP is already disabled.")
+    else:
+        db.cfgdb.mod_entry("KDUMP", "config", {"remote": "false"})
+        click.echo("Remote KDUMP has been disabled.")
+        echo_reboot_warning()
 
-        # Validate username
-        if not username or not username.isalnum():
-            return "Invalid username. Ensure it contains only alphanumeric characters."
 
-        # Validate host (IP or hostname)
-        try:
-            # Check if it's a valid IP address
-            ip_address(host)
-        except AddressValueError:
-            # If not an IP, validate hostname
-            hostname_regex = r'^[a-zA-Z0-9.-]+$'
-            if not re.match(hostname_regex, host) or host.startswith('-') or host.endswith('-'):
-                return "Invalid host. Must be a valid IP or hostname."
+# ------------------ Add group ------------------
 
-        return None  # Validation successful
+@kdump.group(name="add", help="Add kdump configuration parameters")
+def kdump_add():
+    pass
 
-    kdump_table = db.cfgdb.get_table("KDUMP")
-    check_kdump_table_existence(kdump_table)
-    current_status = kdump_table["config"].get("remote", "false").lower()
+#
+# 'ssh_string' command ('sudo config kdump add ssh_string ...')
+#
 
-    if current_status == 'false':
-        click.echo("Remote feature is not enabled. Please enable the remote feature first.")
-        return
 
-    # Validate SSH key
-    validation_error = is_valid_ssh_key(ssh_string)
-    if validation_error:
-        click.echo(f"Error: {validation_error}")
+@kdump_add.command(name="ssh_string", help="Add SSH string in the format user@ip")
+@click.argument('ssh_string', metavar='<user@ip>', required=True)
+@pass_db
+def add_ssh_string(db, ssh_string):
+    if not is_valid_ssh_string(ssh_string):
+        click.echo(f"Error: Invalid SSH string '{ssh_string}'. Must be in format user@<valid_ip>")
         return
 
-    # Add or update the 'ssh_key' entry in the KDUMP table
+    kdump_table = db.cfgdb.get_table("KDUMP")
+    check_kdump_table_existence(kdump_table)
     db.cfgdb.mod_entry("KDUMP", "config", {"ssh_string": ssh_string})
-    click.echo(f"SSH string added to KDUMP configuration: {ssh_string}")
+    click.echo(f"SSH string set to '{ssh_string}' successfully.")
+
+#
+# 'ssh_path' command ('sudo config kdump add ssh_path ...')
+#
 
 
-@add.command(name="ssh_path", help="Add an SSH path to the KDUMP configuration")
+@kdump_add.command(name="ssh_path", help="Add SSH path (must be an absolute path)")
 @click.argument('ssh_path', metavar='<ssh_path>', required=True)
 @pass_db
 def add_ssh_path(db, ssh_path):
-    """Add an SSH path to KDUMP configuration"""
-
-    def is_valid_ssh_path(ssh_path):
-        """Validate the SSH path"""
-        # Check if the path is absolute
-        if not os.path.isabs(ssh_path):
-            return "Invalid path. SSH path must be an absolute path."
-
-        # (Optional) Check if the path exists on the system
-        if not os.path.exists(ssh_path):
-            return f"Invalid path. The path '{ssh_path}' does not exist."
-
-        return None  # Validation successful
+    if not is_valid_path(ssh_path):
+        click.echo(f"Error: Invalid path '{ssh_path}'. Must be an absolute path (starts with '/')")
+        return
 
     kdump_table = db.cfgdb.get_table("KDUMP")
     check_kdump_table_existence(kdump_table)
-    current_status = kdump_table["config"].get("remote", "false").lower()
-    if current_status == 'false':
-        click.echo("Remote feature is not enabled. Please enable the remote feature first.")
-        return
-
-    # Validate SSH path
-    validation_error = is_valid_ssh_path(ssh_path)
-    if validation_error:
-        click.echo(f"Error: {validation_error}")
-        return
-
-    # Add or update the 'ssh_path' entry in the KDUMP table
     db.cfgdb.mod_entry("KDUMP", "config", {"ssh_path": ssh_path})
-    click.echo(f"SSH path added to KDUMP configuration: {ssh_path}")
+    click.echo(f"SSH path set to '{ssh_path}' successfully.")
+
+# ------------------ Remove group ------------------
 
 
-@kdump.group(name="remove", help="remove configuration items to KDUMP")
-def remove():
-    """Group of commands to remove configuration items to KDUMP"""
+@kdump.group(name="remove", help="Remove kdump configuration parameters")
+def kdump_remove():
     pass
 
+#
+# 'ssh_string' remove command ('sudo config kdump remove ssh_string ...')
+#
 
-@remove.command(name="ssh_string", help="Remove the SSH string from the KDUMP configuration")
+
+@kdump_remove.command(name="ssh_string", help="Remove SSH string")
 @pass_db
 def remove_ssh_string(db):
-    """Remove the SSH string from KDUMP configuration"""
     kdump_table = db.cfgdb.get_table("KDUMP")
     check_kdump_table_existence(kdump_table)
 
-    # Check if ssh_string exists
-    if "ssh_string" in kdump_table["config"]:
-        db.cfgdb.mod_entry("KDUMP", "config", {"ssh_string": None})
-        click.echo("SSH string removed from KDUMP configuration.")
-    else:
-        click.echo("SSH string not found in KDUMP configuration.")
+    current_val = kdump_table.get("config", {}).get("ssh_string", "")
+    if not current_val:
+        click.echo("No SSH string is currently set. Nothing to remove.")
+        return
 
+    db.cfgdb.mod_entry("KDUMP", "config", {"ssh_string": ""})
+    click.echo("SSH string removed successfully.")
 
-@remove.command(name="ssh_path", help="Remove the SSH path from the KDUMP configuration")
+#
+# 'ssh_path' remove command ('sudo config kdump remove ssh_path ...')
+#
+
+
+@kdump_remove.command(name="ssh_path", help="Remove SSH path")
 @pass_db
 def remove_ssh_path(db):
-    """Remove the SSH path from KDUMP configuration"""
     kdump_table = db.cfgdb.get_table("KDUMP")
     check_kdump_table_existence(kdump_table)
 
-    # Check if ssh_string exists
-    if "ssh_path" in kdump_table["config"]:
-        db.cfgdb.mod_entry("KDUMP", "config", {"ssh_path": None})
-        click.echo("SSH path removed from KDUMP configuration.")
-    else:
-        click.echo("SSH path not found in KDUMP configuration.")
+    current_val = kdump_table.get("config", {}).get("ssh_path", "")
+    if not current_val:
+        click.echo("No SSH path is currently set. Nothing to remove.")
+        return
+
+    db.cfgdb.mod_entry("KDUMP", "config", {"ssh_path": ""})
+    click.echo("SSH path removed successfully.")

scripts/sonic-kdump-config

@@ -87,6 +87,17 @@ def get_current_image():
     print_err("Unable to locate current SONiC image")
     sys.exit(1)
 
+
+def str2bool(v):
+    if isinstance(v, bool):
+        return v
+    if v.lower() in ('yes', 'true', 't', 'y', '1'):
+        return True
+    elif v.lower() in ('no', 'false', 'f', 'n', '0'):
+        return False
+    else:
+        raise argparse.ArgumentTypeError('Boolean value expected.')
+
 ## Read allocated memory size
 def get_crash_kernel_size():
     try:
@@ -430,23 +441,6 @@ def write_num_dumps(num_dumps):
         print_err("Error while writing KDUMP_NUM_DUMPS into %s" % kdump_cfg)
         sys.exit(1)
 
-def write_kdump_remote():
-    # Assuming there's a function that retrieves the remote value from the config database
-    remote = get_kdump_remote()  # This function needs to be implemented
-
-    kdump_cfg = '/etc/default/kdump-tools'
-
-    if remote:
-        # Uncomment SSH and SSH_KEY in the /etc/default/kdump-tools file
-        run_command("/bin/sed -i 's/#SSH/SSH/' %s" % kdump_cfg, use_shell=False)
-        run_command("/bin/sed -i 's/#SSH_KEY/SSH_KEY/' %s" % kdump_cfg, use_shell=False)
-        print("SSH and SSH_KEY uncommented for remote configuration.")
-    else:
-        # Comment out SSH and SSH_KEY in the /etc/default/kdump-tools file
-        run_command("/bin/sed -i 's/SSH/#SSH/' %s" % kdump_cfg, use_shell=False)
-        run_command("/bin/sed -i 's/SSH_KEY/#SSH_KEY/' %s" % kdump_cfg, use_shell=False)
-        print("SSH and SSH_KEY commented out for local configuration.")
-
 def read_ssh_string():
     (rc, lines, err_str) = run_command("grep '#*SSH=.*' %s | cut -d '=' -f 2 | tr -d '\"'" % kdump_cfg, use_shell=False)
     if rc == 0 and type(lines) == list and len(lines) >= 1:
@@ -826,15 +820,15 @@ def main():
     parser.add_argument('--num_dumps', nargs='?', type=int, action='store', default=False,
         help='Maximum number of kernel dump files stored')
 
-    parser.add_argument('--remote', action='store_true', default=False,
-                help='Enable remote kdump via SSH')
-    
+    parser.add_argument('--remote', type=str2bool, nargs='?', const=True, default=False,
+    help='Enable or disable remote kdump via SSH (true/false)')
+
     parser.add_argument('--ssh_string', nargs='?', type=str, action='store', default=False,
             help='ssh_string for remote kdump')
 
     parser.add_argument('--ssh_path', nargs='?', type=str, action='store',default=False,
             help='ssh_path for remote kdump')
-    
+
     # Memory allocated for capture kernel on Current Image
     parser.add_argument('--memory', nargs='?', type=str, action='store', default=False,
         help='Amount of memory reserved for the capture kernel')