servicenow.itsm.configuration_item is hard-coded to look in whole 'cmdb_ci' class when not query_sys_id

[EUCTechTopics]

SUMMARY

When trying to update an existing CI by referencing its name, the get_record method in the configuration_item function retrieves records from the entire cmdb_ci class instead of limiting the query to the specific class defined in sys_class_name. This can lead to incorrect or unintended records being retrieved when the user specifies a different class through the sys_class_name parameter.

ISSUE TYPE

• Bug Report

COMPONENT NAME

servicenow.itsm.configuration_item

ANSIBLE VERSION

Ansible: 2.17

COLLECTION VERSION

2.6.3

CONFIGURATION

n/a

OS / ENVIRONMENT

n/a

STEPS TO REPRODUCE

• Use the module with a specified sys_class_name other than cmdb_ci (e.g. 'u_cmdb_pki_certificate').

- name: Create SNOW CI
  servicenow.itsm.configuration_item:
    name: "{{ name }}"
    serial_number: "{{ serial_number }}"
    sys_class_name: "u_cmdb_ci_pki_certificate"

EXPECTED RESULTS

• The get_record method should query the specified class in cmdb_table rather than defaulting to cmdb_ci when sys_class_name is provided.
• The existing CI in the specified class should then be updated

{
  "changed": true,
  "record": {
    "serial_number": "67890"
    },
  "diff": {
    "before": {
      "serial_number": "12345"
      },
    "after": {
     "serial_number": "67890"
     },
  },
  "invocation": {
    "module_args": {
      "name": "NAME",
      "serial_number": "67890",
      "sys_class_name": "u_cmdb_ci_pki_certificate",
      "instance": {
        "host": "https://instance.service-now.com",
        "api_path": "api/now",
        "validate_certs": true,
        "grant_type": null,
        "custom_headers": null,
        "refresh_token": null,
        "access_token": null,
        "timeout": null
      },
      "state": "present",
      "sys_id": null,
      "attachments": null,
      "configuration_item_mapping": null,
      "short_description": null,
      "asset_tag": null,
      "install_status": null,
      "operational_status": null,
      "ip_address": null,
      "mac_address": null,
      "category": null,
      "environment": null,
      "assigned_to": null
    }
  },
  "_ansible_no_log": null
}

ACTUAL RESULTS

• Observe that the get_record function queries the entire cmdb_ci class instead of the specified class.
• When a CI exists with the same name in multiple classes, the module throws an error that multiple records match the query

"msg": "2 api/now/table/cmdb_ci records match the {'name': 'NAME', 'sysparm_exclude_reference_link': 'true', 'sysparm_limit': 1000} query."

[EUCTechTopics]

My suggested solution would be to update the following:

configuration_item.py

if not query_sys_id:
+   cmdb_table = module.params["sys_class_name"] or "cmdb_ci"
-   configuration_item = table_client.get_record("cmdb_ci", query_name)
+   configuration_item = table_client.get_record(cmdb_table, query_name)
    # User did not specify existing CI, so we need to create a new one.
    if not configuration_item:
-       cmdb_table = module.params["sys_class_name"] or "cmdb_ci"
        new = mapper.to_ansible(
            table_client.create_record(
                cmdb_table, mapper.to_snow(payload), module.check_mode
            )
        )

[tupyy]

@EUCTechTopics Hello,
Did you look at servicenow.itsm.api module? You can query any table from that module

[EUCTechTopics]

Hi @tupyy

That does not provide the solution I need because I am not trying to query a table.

The issue is that I am trying to update an existing CI by referencing its name. However, because there are multiple CI's with the same name in different classes, the module fails to find a unique CI to update.

Please take a close look at my suggested code change and you shall see what I mean.

[EUCTechTopics]

I have updated the OP to make this more clear as well.

[tupyy]

From my understanding sysclass_name means table.
Your PR is doing exactly this. It's changing the name of the table passed to table_client.
PTAL:

servicenow.itsm/plugins/module_utils/table.py

Line 27 in 0872e1e

def get_record(self, table, query, must_exist=False):

[EUCTechTopics]

Yeah that is correct.

This way when you want to update an existing record by referencing its name, it searches for that record only in the given table (sys_class_name).

[tupyy]

Ok, then why is this not working for you:

- servicenow.itsm.api_info:
    resource: u_cmdb_ci_pki_certificate
    sysparm_query: serial_numberEQUALsomething

The problem with your PR is that is making configuration_item generic. You can pass incident to the sysclass and it will work.

[EUCTechTopics]

Hi @tupyy

Thanks for your follow-up. I understand your point about the flexibility of the sys_class_name parameter and that it essentially represents a table. However, the issue isn't about querying a generic table, but rather about how the existing module behavior handles updating Configuration Items (CIs) based on their names.

The Problem

In scenarios where multiple CIs exist with the same name but belong to different classes, the current module implementation fails to correctly identify the CI to update. This happens because the module defaults to querying the entire cmdb_ci class rather than the specific class provided by sys_class_name. This is problematic because it can lead to either:

• Multiple records being found, or
• Incorrect records being retrieved, leading to potential errors when attempting to update

My Proposed Change

The purpose of my proposed change is to ensure that the search for the CI is limited to the specific class (or table) provided by sys_class_name. By passing cmdb_table (which represents the sys_class_name) to the get_record method, the module will correctly narrow down the search to the intended class, preventing it from querying the entire cmdb_ci class.

Why the Current API Call Suggestion Won’t Work

The example you provided with servicenow.itsm.api_info does allow querying a specific table using sysparm_query. However, this approach is unrelated to the context of my issue, which specifically deals with the behavior of updating existing CIs by name in the current module. My PR doesn't just change the name of the table passed to table_client — it fixes a logic error that could lead to incorrect records being selected when multiple CIs share the same name across different classes.

Configuration Item Specificity

Regarding your concern about making configuration_item generic: the proposed change actually improves specificity. It ensures that when updating or referencing a CI by name, the module is operating within the correct context (i.e., the correct class or table). The risk of incorrectly fetching or updating a CI from a different class is mitigated by this change.
I hope this clarifies the intent and necessity of the change. I'm happy to discuss this further if there are any more concerns.

Thanks again for your attention to this matter!

[EUCTechTopics]

Hi @tupyy

I’ve identified another critical issue related to this behavior:

Additional Problem

When a Configuration Item (CI) with the specified name does not exist in the provided sys_class_name (table), but it does exist in another class (or table), the current implementation mistakenly updates the CI found in the wrong class. This is not the expected behavior.

Expected Behavior

If no CI with the given name exists in the specified sys_class_name, the module should create a new record in the provided class, not update an existing CI from a different class.

Current Behavior

• The module searches for the CI in the cmdb_ci table.
• If it finds a CI with the given name in any class (even if it’s not the specified sys_class_name), it proceeds to update that CI.
• This can lead to unintended updates in the wrong class, which may have significant repercussions, especially in environments where CIs with the same name exist in different classes.

Proposed Fix

The change I proposed earlier will also address this issue. By ensuring that the module only searches within the specified sys_class_name, it will correctly:

1. Find and update the CI if it exists in the specified class.
2. Create a new CI in the specified class if it doesn’t already exist, without mistakenly updating a CI in another class.

This will align the module’s behavior with user expectations and prevent potentially harmful updates to CIs in unintended classes.

Thank you for considering this additional concern. I’m confident that the proposed changes will significantly improve the module’s reliability and accuracy.

[EUCTechTopics]

Hi @tupyy

Following our discussion and your feedback, I reviewed your concern regarding the potential for setting sys_class_name as for example "incident", which could make the query less specific to the CMDB.

To address this, I've updated the query to include sys_class_name. This change ensures that we continue to use the table_client specifically on the cmdb_ci table while extending the filter to incorporate sys_class_name.

Please review the updated PR #405 and let me know if these adjustments better align with your expectations.
Thank you!

[tupyy]

Hello,

Thank you for the PR. After some discussion, we would like to keep the changes very simple. It is something like your first suggestion. If the sysclass_name is defined use that otherwise use cmbd_ci.
I've played a bit with this idea and this is what I have right now (just an idea):

diff --git a/plugins/modules/configuration_item.py b/plugins/modules/configuration_item.py
index a283142..9492b54 100644
--- a/plugins/modules/configuration_item.py
+++ b/plugins/modules/configuration_item.py
@@ -289,12 +289,11 @@ DIRECT_PAYLOAD_FIELDS = (
 def ensure_absent(module, table_client, attachment_client):
     mapper = get_mapper(module, "configuration_item_mapping", PAYLOAD_FIELDS_MAPPING)
     query = utils.filter_dict(module.params, "sys_id", "name")
-    configuration_item = table_client.get_record("cmdb_ci", query)
+    cmdb_table = module.params["sys_class_name"] or "cmdb_ci"
+    configuration_item = table_client.get_record(cmdb_table, query)
 
     if configuration_item:
-        cmdb_table = configuration_item["sys_class_name"]
-        if cmdb_table != "cmdb_ci":
-            configuration_item = table_client.get_record(cmdb_table, query)
+        configuration_item = table_client.get_record(cmdb_table, query)
 
         attachment_client.delete_attached_records(
             cmdb_table,
@@ -324,6 +323,7 @@ def build_payload(module, table_client):
 
 
 def ensure_present(module, table_client, attachment_client):
+    cmdb_table = module.params["sys_class_name"] or "cmdb_ci"
     mapper = get_mapper(module, "configuration_item_mapping", PAYLOAD_FIELDS_MAPPING)
     query_sys_id = utils.filter_dict(module.params, "sys_id")
     query_name = utils.filter_dict(module.params, "name")
@@ -333,10 +333,9 @@ def ensure_present(module, table_client, attachment_client):
     )
 
     if not query_sys_id:
-        configuration_item = table_client.get_record("cmdb_ci", query_name)
+        configuration_item = table_client.get_record(cmdb_table, query_name)
         # User did not specify existing CI, so we need to create a new one.
         if not configuration_item:
-            cmdb_table = module.params["sys_class_name"] or "cmdb_ci"
             new = mapper.to_ansible(
                 table_client.create_record(
                     cmdb_table, mapper.to_snow(payload), module.check_mode
@@ -360,19 +359,9 @@ def ensure_present(module, table_client, attachment_client):
     else:
         # Get existing record using provided sys_id
         old = mapper.to_ansible(
-            table_client.get_record("cmdb_ci", query_sys_id, must_exist=True)
+            table_client.get_record(cmdb_table, query_sys_id, must_exist=True)
         )
-        # Check if provided name already exists
-        if query_name:
-            configuration_item = table_client.get_record("cmdb_ci", query_name)
-            if configuration_item:
-                old2 = mapper.to_ansible(configuration_item)
-                if old["sys_id"] != old2["sys_id"]:
-                    raise errors.ServiceNowError(
-                        "Record with the name {0} already exists.".format(
-                            module.params["name"]
-                        )
-                    )
+
     # Update existing record
     cmdb_table = old["sys_class_name"]
     # If necessary, fetch the record from the table for the extended CI class
diff --git a/tests/integration/targets/itsm_configuration_item/tasks/main.yml b/tests/integration/targets/itsm_configuration_item/tasks/main.yml
index 51c799e..1eb18a6 100644
--- a/tests/integration/targets/itsm_configuration_item/tasks/main.yml
+++ b/tests/integration/targets/itsm_configuration_item/tasks/main.yml
@@ -12,6 +12,7 @@
         environment: development
         install_status: on_order
         operational_status: non_operational
+        sys_class_name: cmdb_ci_computer
         attachments:
           - path: "{{ collection_base_dir }}/tests/integration/targets/itsm_configuration_item/res/sample_file.txt"
       register: base_ci

As you can see, we need the same fix for ensure_absent and there's a small fix for the current integration tests to pass.
Also, you need to add some specific integration tests for this fix.

WDYT

[EUCTechTopics]

sorry for the long radio silence, i will take a look at this and come back with an updated pull request :)

[EUCTechTopics]

Created PR #413