Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
EUCTechTopics opened this issue Sep 2, 2024 · 12 comments · May be fixed by #405

Comments

@EUCTechTopics
Copy link

EUCTechTopics commented Sep 2, 2024

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 EUCTechTopics changed the title servicenow.itsm.configuration_item is hard-coded to look in whole 'cmdb_ci servicenow.itsm.configuration_item is hard-coded to look in whole 'cmdb_ci' class when not query_sys_id Sep 2, 2024
@EUCTechTopics
Copy link
Author

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
Copy link
Contributor

tupyy commented Sep 2, 2024

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

@EUCTechTopics
Copy link
Author

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
Copy link
Author

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

@tupyy
Copy link
Contributor

tupyy commented Sep 2, 2024

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:

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

@EUCTechTopics
Copy link
Author

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
Copy link
Contributor

tupyy commented Sep 2, 2024

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
Copy link
Author

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
Copy link
Author

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
Copy link
Author

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
Copy link
Contributor

tupyy commented Sep 9, 2024

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
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants