Merge pull request #75 from marcelmamula/hdbsql

sap_hdbsql: Add error handling and secure flags for hdbsql

Author: marcelmamula

plugins/modules/sap_hdbsql.py

@@ -76,7 +76,10 @@
         type: list
         elements: str
 notes:
-    - Does not support C(check_mode). Always reports that the state has changed even if no changes have been made.
+    - Does not support C(check_mode).
+    - If filepath is used, changed is true. If query is used, changed is true if it is not a SELECT.
+    - Avoid using login shell flags (like 'become_flags' with value of '-i') when become_user is a SAP admin.
+    - If a login shell is required, manually reset the environment in the task using the environment keyword.
 author:
     - Rainer Leber (@rainerleber)
 '''
@@ -156,12 +159,37 @@
 from ansible.module_utils.common.text.converters import to_native
 
 
-def csv_to_list(rawcsv):
-    reader_raw = csv.DictReader(StringIO(rawcsv))
-    reader = [dict((k, v.strip()) for k, v in row.items()) for row in reader_raw]
+def csv_to_list(raw_csv):
+    if not raw_csv.strip():
+        return []
+
+    reader_raw = csv.DictReader(StringIO(raw_csv))
+    # Using to_native and strip to ensure clean dictionary keys/values
+    reader = [dict((to_native(k), to_native(v).strip()) for k, v in row.items()) for row in reader_raw]
     return list(reader)
 
 
+def run_hdb_command(module, full_cmd):
+    rc, out_raw, err = module.run_command(full_cmd)
+
+    if rc != 0:
+        err_msg = to_native(err).lower()
+
+        if "authentication failed" in err_msg or "invalid username or password" in err_msg:
+            module.fail_json(msg="SAP HANA Authentication Failed. Please check credentials/userstore.", rc=rc, stderr=err)
+
+        elif "insufficient privilege" in err_msg or "258:" in err_msg:
+            module.fail_json(msg="SAP HANA Authorization Error: The user has insufficient privileges to perform this action.", rc=rc, stderr=err)
+
+        elif "connect failed" in err_msg:
+            module.fail_json(msg="SAP HANA Connection Failed. Check host, instance, and port.", rc=rc, stderr=err)
+
+        else:
+            module.fail_json(msg="SQL Execution Error", rc=rc, stderr=err, cmd=full_cmd)
+
+    return out_raw
+
+
 def main():
     module = AnsibleModule(
         argument_spec=dict(
@@ -178,68 +206,88 @@ def main():
             filepath=dict(type='list', elements='path', required=False),
             autocommit=dict(type='bool', default=True),
         ),
-        required_one_of=[('query', 'filepath'), ('sid', 'instance')],
+        required_one_of=[('query', 'filepath')],
         required_if=[('userstore', False, ['password'])],
         supports_check_mode=False,
     )
-    rc, out, err, out_raw = [0, [], "", ""]
 
     params = module.params
+    output = []
+    has_changed = False
 
-    sid = params['sid']
-    bin_path = params['bin_path']
-    instance = params['instance']
-    user = params['user']
-    userstore = params['userstore']
-    password = params['password']
-    autocommit = params['autocommit']
-    host = params['host']
-    database = params['database']
-    encrypted = params['encrypted']
-
-    filepath = params['filepath']
-    query = params['query']
+    # Determine if module will show as changed.
+    # If filepaths are provided, we assume changes will be made, as files typically contain DDL or DML statements.
+    # If only queries are provided, we check if any of them are not SELECT statements. If at least one is not a SELECT, we assume changes will be made.
+    if params['filepath']:
+        has_changed = True
 
+    elif params['query']:
+        for q in params['query']:
+            # Strip whitespace and check if it starts with SELECT
+            if not q.strip().upper().startswith("SELECT"):
+                has_changed = True
+                break
+
+    # Construct Binary Path
+    bin_path = params['bin_path']
     if bin_path is None:
-        bin_path = "/usr/sap/{sid}/HDB{instance}/exe/hdbsql".format(sid=sid.upper(), instance=instance)
+        if not params['sid']:
+            module.fail_json(msg="Parameter 'sid' is required to determine default bin_path if 'bin_path' is not provided.")
+
+        bin_path = "/usr/sap/{sid}/HDB{instance}/exe/hdbsql".format(
+            sid=params['sid'].upper(),
+            instance=params['instance']
+        )
 
     try:
         command = [module.get_bin_path(bin_path, required=True)]
     except Exception as e:
-        module.fail_json(msg='Failed to find hdbsql at the expected path "{0}".Please check SID and instance number: "{1}"'.format(bin_path, to_native(e)))
+        module.fail_json(msg='Executable binary hdbsql not found at {0}: {1}'.format(bin_path, to_native(e)))
 
-    if encrypted is True:
-        command.extend(['-attemptencrypt'])
-    if autocommit is False:
+    # Build Base Command
+    if params['encrypted']:
+        # -e: Enforce encryption (don't fall back)
+        # -ssltrustcert: Trust the server certificate
+        # -sslcreatecert: Create a local certificate if necessary
+        command.extend(['-e', '-ssltrustcert', '-sslcreatecert'])
+
+    if not params['autocommit']:
         command.extend(['-z'])
-    if host is not None:
-        command.extend(['-n', host])
-    if database is not None:
-        command.extend(['-d', database])
-    # -x Suppresses additional output, such as the number of selected rows in a result set.
-    if userstore:
-        command.extend(['-x', '-U', user])
+
+    if params['host']:
+        command.extend(['-n', params['host']])
+
+    if params['database']:
+        command.extend(['-d', params['database']])
+
+    if params['userstore']:
+        command.extend(['-x', '-U', params['user']])
+
     else:
-        command.extend(['-x', '-i', instance, '-u', user, '-p', password])
-
-    if filepath is not None:
-        command.extend(['-E 3', '-I'])
-        for p in filepath:
-            # makes a command like hdbsql -i 01 -u SYSTEM -p secret123# -I /tmp/HANA_CPU_UtilizationPerCore_2.00.020+.txt,
-            # iterates through files and append the output to var out.
-            query_command = command + [p]
-            (rc, out_raw, err) = module.run_command(query_command)
-            out.append(csv_to_list(out_raw))
-    if query is not None:
-        for q in query:
-            # makes a command like hdbsql -i 01 -u SYSTEM -p secret123# "select user_name from users",
-            # iterates through multiple commands and append the output to var out.
+        command.extend(['-x', '-i', params['instance'], '-u', params['user'], '-p', params['password']])
+
+    # Process Queries
+    if params['query']:
+        for q in params['query']:
             query_command = command + [q]
-            (rc, out_raw, err) = module.run_command(query_command)
-            out.append(csv_to_list(out_raw))
-    changed = True
+            out_raw = run_hdb_command(module, query_command)
+            try:
+                output.append(csv_to_list(out_raw))
+            except Exception as e:
+                module.fail_json(msg="Failed to parse CSV output: {0}".format(to_native(e)))
+
+    # Process Files
+    # Note: File processing adds extra arguments so it has to execute after query processing.
+    if params['filepath']:
+        for p in params['filepath']:
+            file_query_command = command + ['-E', '3', '-I', p]
+            out_raw = run_hdb_command(module, file_query_command)
+            try:
+                output.append(csv_to_list(out_raw))
+            except Exception as e:
+                module.fail_json(msg="Failed to parse output from file {0}: {1}".format(p, to_native(e)))
 
-    module.exit_json(changed=changed, rc=rc, query_result=out, stderr=err)
+    module.exit_json(changed=has_changed, query_result=output)
 
 
 if __name__ == '__main__':

tests/unit/plugins/modules/test_sap_hdbsql.py

@@ -53,7 +53,7 @@ def test_sap_hdbsql(self):
             'user': "SYSTEM",
             'password': "1234Qwer",
             'database': "HDB",
-            'query': "SELECT * FROM users;"
+            'query': ["SELECT * FROM users;"]
         }
         with patch.object(basic.AnsibleModule, 'run_command') as run_command:
             run_command.return_value = 0, 'username,name\n  testuser,test user  \n myuser, my user   \n', ''
@@ -76,7 +76,7 @@ def test_hana_userstore_query(self):
             'user': "SYSTEM",
             'userstore': True,
             'database': "HDB",
-            'query': "SELECT * FROM users;"
+            'query': ["SELECT * FROM users;"]
         }
         with patch.object(basic.AnsibleModule, 'run_command') as run_command:
             run_command.return_value = 0, 'username,name\n  testuser,test user  \n myuser, my user   \n', ''
@@ -99,7 +99,88 @@ def test_hana_failed_no_passwd(self):
                 'host': "localhost",
                 'user': "SYSTEM",
                 'database': "HDB",
-                'query': "SELECT * FROM users;"
+                'query': ["SELECT * FROM users;"]
             }
             with set_module_args(args):
                 self.module.main()
+
+    def test_changed_status_select(self):
+        """Verify SELECT query returns changed=False"""
+        args = {
+            'sid': "HDB",
+            'instance': "01",
+            'password': "pwd",
+            'query': ["SELECT * FROM users;"]
+        }
+        with patch.object(basic.AnsibleModule, 'run_command') as run_command:
+            run_command.return_value = 0, 'col1\nval1', ''
+            with self.assertRaises(AnsibleExitJson) as result:
+                with set_module_args(args):
+                    self.module.main()
+            self.assertFalse(result.exception.args[0]['changed'])
+
+    def test_changed_status_update(self):
+        """Verify UPDATE query returns changed=True"""
+        args = {
+            'sid': "HDB",
+            'instance': "01",
+            'password': "pwd",
+            'query': ["UPDATE users SET name='test';"]
+        }
+        with patch.object(basic.AnsibleModule, 'run_command') as run_command:
+            run_command.return_value = 0, '', ''  # Updates often return empty string
+            with self.assertRaises(AnsibleExitJson) as result:
+                with set_module_args(args):
+                    self.module.main()
+            self.assertTrue(result.exception.args[0]['changed'])
+
+    def test_authentication_failure(self):
+        """Verify specific error message for Auth Failure"""
+        args = {
+            'sid': "HDB",
+            'instance': "01",
+            'password': "wrong",
+            'query': ["SELECT 1 FROM DUMMY;"]
+        }
+        with patch.object(basic.AnsibleModule, 'run_command') as run_command:
+            run_command.return_value = 10, '', 'Authentication failed'
+            with self.assertRaises(AnsibleFailJson) as result:
+                with set_module_args(args):
+                    self.module.main()
+            self.assertIn("Authentication Failed", result.exception.args[0]['msg'])
+
+    def test_insufficient_privilege(self):
+        """Verify specific error message for Privilege Error (Error 258)"""
+        args = {
+            'sid': "HDB",
+            'instance': "01",
+            'password': "pwd",
+            'query': ["DROP TABLE important_stuff;"]
+        }
+        with patch.object(basic.AnsibleModule, 'run_command') as run_command:
+            run_command.return_value = 1, '', '* 258: insufficient privilege'
+            with self.assertRaises(AnsibleFailJson) as result:
+                with set_module_args(args):
+                    self.module.main()
+            self.assertIn("Authorization Error", result.exception.args[0]['msg'])
+
+    def test_encrypted_connection_flags(self):
+        """Verify that encryption flags are correctly added to the command"""
+        args = {
+            'sid': "HDB",
+            'instance': "01",
+            'password': "pwd",
+            'encrypted': True,
+            'query': ["SELECT 1 FROM DUMMY;"]
+        }
+        with patch.object(basic.AnsibleModule, 'run_command') as run_command:
+            run_command.return_value = 0, '1', ''
+            with self.assertRaises(AnsibleExitJson):
+                with set_module_args(args):
+                    self.module.main()
+
+            # Get the actual command executed
+            executed_cmd = run_command.call_args[0][0]
+            self.assertIn('-e', executed_cmd)
+            self.assertIn('-ssltrustcert', executed_cmd)
+            self.assertIn('-sslcreatecert', executed_cmd)