[BUG] value_raw set to a variable about ansible-collection-checkmk.general HOT 54 OPEN

First, Happy new year everybody, and sorry for my late reply. I had a very busy December and the had some time to relax.

The "ID" is only key the REST API uses to identify rules.
That doesn't fit very well to the way how idem potency works in Ansible, though.
In Ansible, there are two possible solutions:

1. We only can check if ruleset, folder, conditions, properties, value_raw are equal. Probably, we have to remove all "()[]{}' from value_raw vefore comparison.
2. In most environments, it might also be a solution to only compare description and use this as an ID in your Ansible variables.

What do you think of implementing both solutions by inventing a parameter "keys" that has "[ ruleset, folder, conditions, properties, value_raw]" as a default, but can also be set to "[ description ]"? The function get_existing_rule() then can take care of that choice.

@clagio, @muehlings, @geof77 & @msekania: What's your opinion on this?

from ansible-collection-checkmk.general.

With the next major Checkmk release, we plan to make the API fully JSON-compatible which should rid us of this issue.
For the time being there is no easy way out I think.

from ansible-collection-checkmk.general.

Just my 2 cents (coming from #335): I think without clearly and unambiguously identifying the rule you want to change, no amount of checking title, description, folder, conditions or anything else will work. What if one of these parameters is exactly the one you want to change?

So I'd like to propose to add a mandatory parameter to rule to identify it from Ansible, e.g.

rule:
  id: myrule1
  location: ...
  conditions: ...

This ID could be attached with some boilerplate to the description or title field (actual title - RULE myrule1 GENERATED BY ANSIBLE).

This way, you can change any aspect of this rule, even transfer it to another folder.

from ansible-collection-checkmk.general.

I talked to one of our developers, and it looks like this is an upstream issue, which we cannot solve in the collection.
A lot of programming details in here I do not understand, but it sounds like some workarounds were already discussed.
I will keep this open for some time, maybe I can get some feedback from our API team.

Maybe someone can write up some sort of bottom line, where we are right now?

from ansible-collection-checkmk.general.

from my understanding there is a ID field that uniquely identify a rule, but a new random ID it's created automatically when you call the API, so you can have multiple rules with all the same values/properties but different ID. To achieve idempotency, in the collection has been arbitrarily decided to check against "conditions", "properties" and "value_raw". Personally I think we should have a "name" field for the rule (that's why I suggested to use the description since it's basically a name for the rule), and check against that, but again should be changed in the API.

from ansible-collection-checkmk.general.

Hello and happy new year!
Just an idea: since 1) checkmk has code to format the rule as returned by the API call and 2) nothing stops us from creating identical rules, what about first creating the new rule in disabled state. The API will return the formated rule back to the module. The API representation of the new rule could then be compared with other existing rules. Then if it does not exist, simply recreate it with the enabled state (or leave it alone if the disabled state is desired). Another solution would be to use the cmk code to pre-format the rule before comparison. I don't know if that would work...
Edit: @muehlings looks like we're looking at the same file :)

from ansible-collection-checkmk.general.

@lgetwan @robin-tribe29
I understand that changing the value_raw format won't happen soon. Instead, devs could add an API endpoint to get the API rule representation without actually creating it. Then the module can use this endpoint instead of creating rules for this purpose. This would keep checkmk's logs clean and it would be pretty simple to update the module code.

from ansible-collection-checkmk.general.

Don't know if that solves all constellations for setting the value_raw option for a rule, but this works for me in all my playbooks so far:

setting var for http_test:
rulevalue: "{'name': 'http_test', 'host': {'address': ('direct', '{{ server.hostname }}'), 'port': 80}, 'mode': ('url', {})}"
setting var for mongodb as example:
rulevalue: "{'auth_mechanism': 'SCRAM-SHA-256', 'host': '{{ mongodb.hostname }}', 'auth_source': 'admin', 'username': 'mongoadmin', 'password': '{{ mongodb.password }}'}"

setting var for credentials (example for mariadb/mysql):
rulevalue: "{'credentials':('checkmk', '{{ mariadb_password }}')}"

In the rule use:
value_raw: "{{ rulevalue | string }}"

Maybe someone in this thread can test this with their own rules.
If that works for everyone, we could close it.

KR,
Max

from ansible-collection-checkmk.general.

@pandvan the second issue you are mentioning is out of the scope of this issue. But you can control, where rules are created and if you want to be 100% certain of their position, you can look up the existing rules with the new lookup plugins, which we just released. Anyway, regarding this specific aspect, please open a dedicated issue, if you feel like this is something we should discuss.

Regarding the main discussion, I have no update yet.

from ansible-collection-checkmk.general.

So i have fixed the conversion issue by just passing the value_raw as a string to the ansible module:

  checkmk.general.rule:
    automation_secret: "{{ automation_secret }}"
    automation_user: "{{ automation_user }}"
    rule:
      conditions: "{{ item.rule.conditions }}"
      properties:
        description: "{{ item.rule.properties.description }}"
        comment: "{{ item.rule.properties.comment | default() }}"
        disabled: "{{ item.rule.properties.disabled | default(False) }}"
        documentation_url: "{{ documentation_url }}"
      value_raw: "{{ item.rule.value_raw|string }}"
      location: "{{ item.rule.location }}"
    ruleset: "{{ item.ruleset }}"
    server_url: "{{ master_site_baseurl }}"
    site: "{{ master_site }}"
  loop: "{{ rulesets | flatten(levels=1) }}"
  loop_control:
    label: "{{ item.ruleset }} [{{ item.rule.properties.description | default() }}]"

reading through all the comments above i see this has been provided as a workaround more often, it just disappeared in the amount of comments in this bug.

from ansible-collection-checkmk.general.

Hello clagio,

can you please try to write the rule as dict(key:list)?
vars:
- myvariable: {'levels': [80,90]}

Tuples and ansible are kind of impossible and I suspect that the API call will be JSON-encoded anyways. JSON doesn't know tuples.

The debug output in ansible is the output of json.dumps() and thus the original type of the object isn't preserved. At the moment I have no 2.1 instance available for testing and 2.0-API does not provide the rules.

from ansible-collection-checkmk.general.

Hi, I tried removing the double quotes s you asked, but I get the same error "Unsupported type. Field must be string" but in this case I see from the debug info that all the output is mixed up.
there is a way to see the raw output in ansible before any transformation?

I get the same error also with a different rule (ruleset special_agents:aws) if I try to move the access_key_id in a variable`. As before, if I hardcode the value, it works fine:

value_raw: "{
              'access_key_id': '{{ hostvars['aws-rds'].aws_access_key }}',
              'assume_role': {},
              'global_services': {},
              'regions': ['eu-west-1'],
              'secret_access_key': ('password', 'xxxxxxxxxxxxxxxxxxxx'), 
              'services': {
                'rds': {'limits': True, 'selection': 'all'}
              }"

from ansible-collection-checkmk.general.

Have you removed the "()" and replaced them with "[]", too? Ansible doesn't know the tuple-notation (tup1,tup2) of python. The line in YAML is then interpreted as string and mixed up.

Edit: Does not work. See next comment for solution.

from ansible-collection-checkmk.general.

I set up a 2.1 instance, modified the source and achieved this:

  - name: "Create a rule."
    tribe29.checkmk.rule:
      server_url: "http://127.0.0.1/"
      site: "test"
      automation_user: "automation"
      automation_secret: "$SECRET"
      ruleset: "checkgroup_parameters:memory_percentage_used"
      rule:
          properties: {
              "comment": "Warning at 81%\nCritical at 91%\n",
              "description": "Allow higher memory usage",
              "disabled": false
          }
          value_raw: "{{ myvariable }}" #"{'levels': (80.0, 90.0)}"  #"{{ myvariable }}"
      state: "present"
    vars:
      - myvariable: {'levels': [81,91]}

=>

changed: [debian] => {
    "changed": true,
    "invocation": {
        "module_args": {
            "automation_secret": "$SECRET",
            "automation_user": "automation",
            "rule": {
                "conditions": {
                    "host_labels": [],
                    "host_tags": [],
                    "service_labels": []
                },
                "folder": "/",
                "properties": {
                    "comment": "Warning at 81%\nCritical at 91%\n",
                    "description": "Allow higher memory usage",
                    "disabled": false
                },
                "value_raw": {
                    "levels": [
                        81,
                        91
                    ]
                }
            },
            "ruleset": "checkgroup_parameters:memory_percentage_used",
            "server_url": "http://127.0.0.1/",
            "site": "test",
            "state": "present",
            "validate_certs": true
        }
    },
    "msg": "Created rule"

Source modification:

def get_existing_rule(module, base_url, headers, ruleset, rule):
    # Get rules in ruleset
    rules = get_rules_in_ruleset(module, base_url, headers, ruleset)
    if rules is not None:
        # Loop through all rules
        for r in rules.get("value"):
            # Check if conditions, properties and values are the same
            if type(rule["value_raw"]) == str:
                pass
            else:
                # Input is YAML variable. Need to convert to string...
                rule["value_raw"] = str(rule["value_raw"]).replace('[','(').replace(']',')')
            if (
                sorted(r["extensions"]["conditions"]) == sorted(rule["conditions"])
                and sorted(r["extensions"]["properties"]) == sorted(rule["properties"])
                and sorted(r["extensions"]["value_raw"]) == sorted(rule["value_raw"])
            ):
                # If they are the same, return the ID
                return r["id"]
    return None

and

def create_rule(module, base_url, headers, ruleset, rule):
    api_endpoint = "/domain-types/rule/collections/all"

    params = {
        "ruleset": ruleset,
        "folder": rule["folder"],
        "properties": rule["properties"],
        "conditions": rule["conditions"],
    }

    if type(rule["value_raw"]) == str:
        params["value_raw"] = rule["value_raw"]
    else:
        # Input is YAML variable. Need to convert to string...
        params["value_raw"] = str(rule["value_raw"]).replace('[','(').replace(']',')')

    url = base_url + api_endpoint

    response, info = fetch_url(
        module, url, module.jsonify(params), headers=headers, method="POST"
    )

    if info["status"] != 200:
        exit_failed(
            module,
            "Error calling API. HTTP code %d. Details: %s, %s"
            % (info["status"], info["body"], module.jsonify(params)),
        )

Now I have two options:

1. Write value_raw directly as string => value_raw: "{'levels': (80.0, 90.0)}"
2. Define a YAML(!) variable and reference it in my playbook => value_raw: "{{ myvariable }}" with

    vars:
      - myvariable: {'levels': [81,91]}

My modifications construct the proper string for the API including replacement of the square brackets (=python/YAML list) with the round brackets (=python tuple).

from ansible-collection-checkmk.general.

thanks, I modified the source as you did, and indeed now the first task( myvariable: {'levels': [81,91]}) works, but I get an error with the following :

value_raw: "{
              'access_key_id': '{{ hostvars['aws-rds'].aws_access_key }}',
              'assume_role': {},
              'global_services': {},
              'regions': ['eu-west-1'],
              'secret_access_key': ('password', 'xxxxxxxxxxxxxxxxxxxx'), 
              'services': {
                'rds': {'limits': True, 'selection': 'all'}
              }"

I will have a look later at the sources to see if I can find why

from ansible-collection-checkmk.general.

I had a better look, your change replace all squared brackets with rounded brackets, which means this:
'regions': ['eu-west-1'], becomes 'regions': ('eu-west-1'), and it fails.
It's tricky because depending on the rule we may need a replace or not..

I have also an issue where it keep creating the rule even if already exists, but I haven't narrowed down the problem yet. Would make sense to use the description field to detect if the rule already exists instead of matching conditions/properties/value_raw?

from ansible-collection-checkmk.general.

Yes, you are right.

The only clean way to get this resolved in my opinion is that the API of Checkmk should accept and deliver JSON and not a string which is "evaluated" into a python object. There was some discussion about that in #153
I don't know the reason for the decision to not modifying the API.

The second possibility will be to get rid of all tuples inside the rules. But this is a lot more work to do I think.

I have problems to update the rule without my modifications, too. If only a single character or value differs in "conditions", "properties" or "value_raw" a new rule is created.

from ansible-collection-checkmk.general.

I agree the cleanest way is to fix it at the API level..
I think the different parameters in the value_raw are handled differently by the API, for example:
'regions': ['eu-west-1', 'eu-west-2'], need to stay with the squared brackets (will fails with the rounded brackets )
'secret_access_key': ['password', 'xxxxxxxxxxxxxxxxxxxx'], need to be with rounded brackets or will fail.
But from my understanding there is no difference between the two. I can only suppose secret_access_key is handled different in the API.

Anyway I did some changes and now I have a rule.py that works in all my cases. It's not super clean, but I'm not sure there is a better way without modifying the API. I also changed so that the rule matching (if already exists) is done only comparing the description.

There are several problem trying to compare conditions, properties and value_raw. Value raw has all the problems with escaping chars, and since there are different possible parameters I suspect there is an high chance of mismatch.
Conditions seems to be problematic as well.. below the conditions returned by an existing rule and the one passed by the new rule. This mismatch doesn't happen for all the rules, not sure why, maybe depends on the ruleset type?

existing_rule - compare11:['host_labels', 'host_tags', 'service_labels']
new_rule - compare12:['host_labels']

It's not impossible to match against all the properties, but I think requires a lot of work to make it work for all the possible cases. I believe matching by description it's simple and effective.

I created a new function transform_value_raw, where I take the value_raw and change the brackets only for secret_access_key. Eventually new keys can be added if needed,

def transform_value_raw(value_raw): 
    if type(value_raw) == dict:
        concat_value_raw= "{"
        for key, value in value_raw.items():
            if key == "secret_access_key":
                concat_value_raw = concat_value_raw + "'" +str(key) + "': " +str(value).replace('[','(').replace(']',')')+ ", "
            else:
                if str(value).startswith('[') or str(value).startswith('{'):
                    concat_value_raw = concat_value_raw + "'" +str(key) + "': " +str(value)+ ", "
                else:
                    concat_value_raw = concat_value_raw + "'" +str(key) + "': '" +str(value)+ "', "
        concat_value_raw= concat_value_raw + "}"
        return concat_value_raw
    return value_raw

Modified the create_rule:

def create_rule(module, base_url, headers, ruleset, rule):
    api_endpoint = "/domain-types/rule/collections/all"
    
    value_raw = transform_value_raw(rule["value_raw"])

    params = {
        "ruleset": ruleset,
        "folder": rule["folder"],
        "properties": rule["properties"],
        "conditions": rule["conditions"],
        "value_raw": value_raw
    }
    params_json = json.dumps(params)

    url = base_url + api_endpoint

    response, info = fetch_url(
        module, url, params_json, headers=headers, method="POST"
    )

    if info["status"] != 200:
        exit_failed(
            module,
            "Error calling API. HTTP code %d. Details: %s, %s"
            % (info["status"], info["body"], module.jsonify(params)),
        )

Modified the get_existing_rule:

def get_existing_rule(module, base_url, headers, ruleset, rule):
    # Get rules in ruleset
    rules = get_rules_in_ruleset(module, base_url, headers, ruleset)
    if rules.get("extensions").get("found_rules") != 0 :
        # Loop through all rules
        for r in rules.get("value"):
            if ( r["extensions"]["properties"]["description"] == rule["properties"]["description"]):
                # If they have same description, return the ID
                return r["id"]
    return None

I'm happy to submit the change if is ok

from ansible-collection-checkmk.general.

For all password fields I found out the following: You can just write the password as string without the ('password', 'YOURPASSWORD') notation:
/omd/sites/TEST/lib/python3/cmk/gui/plugins/wato/utils/init.py has a function to transform the string into the required tuple:
forth=lambda v: ("password", v) if not isinstance(v, tuple) else v

This is done automatically and removes at least one type of tuples you have to care about, if the API won't be changed in the future.

I try to find the relevant part in the API of https://github.com/tribe29/checkmk to make a PR there.

from ansible-collection-checkmk.general.

that's right! thanks, it's helpful at least for the passwords

from ansible-collection-checkmk.general.

Another tuple issue with the overall_tags:

'overall_tags': [('{{aws_tag_key}}', ['{{aws_tag_value}}'])],

The playbook runs fine and the rule is created but when I check the rule in checkmk the option "Restrict monitoring services by one of these AWS tags" is not flagged and I get the following message:

Unable to read current options of this rule. Falling back to default values. When saving this rule now, your previous settings will be overwritten. Problem was: Restrict monitoring services by one of these AWS tags: The datatype must be a tuple, but is list.

from ansible-collection-checkmk.general.

@muehlings,

Unfortunately string substitution does not solve the problem and creates other ones, because there are arrays (lists) too, not only tuples!
for example : expect_response: ['302', '403'] it should be an array exactly with [ , ] and not tuple with ( , ), otherwise it's again an error!

With this substitution for example

I agree though that API should deliver correctly formatted JSON.

Otherwise:

• one can also use [[ and ]] as an escape for tuple ( and ).
• additionally one can also solve the YAML to python string conversion for value_raw with jinja2 macro

from ansible-collection-checkmk.general.

Probably this is also helpful,

jinja2 template file with macros value_raw_gen.j2

{#- Generate value_raw for rule from yaml nested dictionary                                                                     -#}
{#- ########################################################################################################################### -#}
{%- macro generateline(elem)                                                                                                    -%}
{%-   if elem is mapping                                                                                                        -%}
{{-     '{' -}}
{%-      for el in elem | dict2items                                                                                            -%}
{%-        if el.value | lower != 'none'                                                                                        -%}
'{{          el.key }}': {{ generateline(el.value) }}
{%-        endif                                                                                                                -%}
{%-      endfor                                                                                                                 -%}
{{-     '}' -}},
{%-   elif ( elem is iterable ) and ( elem is not string )                                                                      -%}
{#-     check if it is real array, escaped, '[[]]' or just a substitute for tuple '[]'                                          -#}
{%-     if ( elem | count == 1 ) and ( elem[0] is not mapping ) and ( elem[0] is iterable ) and ( elem[0] is not string )       -%}
{{-       '(' -}}
{%-       for el in elem[0]                                                                                                     -%}
{{          generateline(el)[:(-1 if loop.last else none):] }}
{%-       endfor                                                                                                                -%}
{{-       ')' -}},
{%-     else                                                                                                                    -%}
{{-       '[' -}}
{%-       for el in elem                                                                                                        -%}
{{          generateline(el)[:(-1 if loop.last else none):] }}
{%-       endfor                                                                                                                -%}
{{-       ']' -}},
{%-     endif                                                                                                                   -%}
{%-   else                                                                                                                      -%}
{%-     if elem |int(-1) == -1                                                                                                  -%}
{%        if '"' in elem or "'" in elem                                                                                         -%}
{{-         elem }},
{%-       else                                                                                                                  -%}
{{          elem | to_json }},
{%-       endif                                                                                                                 -%}
{%-     else                                                                                                                    -%}
{{        elem }},
{%-     endif                                                                                                                   -%}
{%-   endif                                                                                                                     -%}
{%- endmacro                                                                                                                    -%}
{#- ########################################################################################################################### -#}
{#-                                                                                                                             -#}
{#- ########################################################################################################################### -#}
{%- if value_raw is defined                                                                                                     -%}
{{    generateline(value_raw)[:-1:] }}
{%- endif                                                                                                                       -%}

in ansible then

value_raw: "{{ lookup('template', 'value_raw_gen.j2') | string }}"

from ansible-collection-checkmk.general.

If one will not get REST API fixed, so that value_raw format is either JSON (or any ansible compatible format) or can be unambiguously converted to JSON (or any ansible compatible format), then:

• either one writes converter/parser (inside Collection), with "well" defined escape symbols
• employs jinja2 template macros (not necessarily the above listed example), that can be shipped together with tribe29 Collection

Before, it might be also useful to get some obvious bug fixes, I'll be glad to help.

from ansible-collection-checkmk.general.

Thanks, Michael, we are on the same page.
I would like to avoid building workarounds in the collection. I am open to point users to possible approaches though.
What bug fixes are you talking about, and how would you help?

from ansible-collection-checkmk.general.

1. One is already in "pipeline", issue #195, I have some straightforward solution there, bit it brings me to the following question/issue,
2. which are the fields/properties that uniquely identify the rule. Those, which one requires in order to modify the rest of the properties, or, even simpler, to entirely delete this rule?

Both are closely tied to the idempotency of Ansible and the underlying REST API in modules.

If there is a clear answer to the second question, or alternatively a decision what the primary and secondary properties of the rule are, I think the rest can be addressed more systematically and hence relatively easy.

from ansible-collection-checkmk.general.

@clagio, there is one key problem in your proposal,

how can one then apply the same rule to different folders?

possible solutions:

• either splitting rest API in two sets one for rule management and other for rule applications --- I think one should convince lots of people for it (although this is a better solution)
• or (and I think it's easier to implement) include the folder in checks.

Therefore I still think that there should be a reasonable set of primary fields/properties that are used to Identify the rule, like rule-set, folder, conditions, properties, and may be some subset of value_raw (although latter is causing most of the problems)

ID-s are for internal management and you do not know it apriori from Ansible or API side!

from ansible-collection-checkmk.general.

Hi! Two problems related to this issue I came across while developping custom installation playbooks:

1. Maybe remove escaped characters (i.e. \n,\r,\', \",...) from the value_raw before the comparison because it's very difficult (or impossible) to have this right for some rules. For example, I want to create a rule in "ruleset: agent_config:cmk_update_agent". There we must pass the entire certificate in a specific format, with each line surrounded by single-quotes. This is different from what is returned by the API, which does not have the single quotes. So a new rule is created each time the playbook is run! I don't know why we have to send the entire certificate instead of an ID, but that's another story.

2. If there are no conditions in our rule, we still have to specify empty conditions, i.e.:

conditions: { "host_tags": [], "host_labels": [], "service_labels": [] }

If we do not, the comparison fails. It would be great to add these (empty conditions) by default.

I have edited the code to check only the description like suggested above, even if it's not a universal solution, it works for me.

from ansible-collection-checkmk.general.

@geof77,

there is even more fundamental problem in the following comparison in rule module:

sorted(r["extensions"]["value_raw"]) == sorted(rule["value_raw"])

it cannot distinguish between the following variants (just an example, one can also make some others):

• expect_response: ['302', '403']
• expect_response: ['402', '303']
• or any possible permutation of parameters, expect_response: ['432', '300']

To avoid all related complication, even for such a simple case, one should parse value_raw in any reasonable format and not as sorted string.

As for now, as I understand, each of us has it's own workaround, which are not suitable to cover general cases and hnece to be integrated in the rule module, IMHO.

Therefor, I will still repeat my question @robin-tribe29:
which parameters are primary and which secondary on the REST API level?
Implementation in ansible should repeat the same logic as REST API!
As an example, if field host_tag is used in primary group in REST API it should be also in primary group in Ansible module -- used as a member of the parameter set that uniquely identifies the rule.

folder for example, is in the primary group in REST API but not in current implementation of the rule module #195.

from ansible-collection-checkmk.general.

Hello @lgetwan,

Adding flags or mechanisms to circumvent the problem are not my preferred way...

I'm going to analyze the logic of the API endpoints (e.g. cmk/gui/plugins/openapi/endpoints/rule/init.py) soon. It should even be possible to programmatically "write" all ansible modules ("parse('endpoints/rule/init.py') -> endpoint_module.py").

But I think I need a newer development vm. I try to get this snippet

from cmk.gui.watolib.rulesets import (
    AllRulesets,
    FolderRulesets,
    Rule,
    RuleConditions,
    RuleOptions,
    Ruleset,
)

all_rulesets = AllRulesets.load_all_rulesets()
print(all_rulesets)

to work, but "match" in [https://raw.githubusercontent.com/tribe29/checkmk/627515db4ad05457276325806f1948841382c112/cmk/gui/watolib/rulesets.py] is python3.10 and my Debian 10 has python3.9. I'm interested in the structure of the rulesets and how the API itself is checking against "AllRulesets".

@lgetwan Perhaps we should define a "target version" we want the ansible modules to work against (2.1.0pXY.*, python3.10+)?

from ansible-collection-checkmk.general.

Well, Ok... that idea would work only for "create", not for "delete". Creating a temporary rule just to get the format correct (and delete both if it exists)... I don't like it...

from ansible-collection-checkmk.general.

@geof77:
Even if it feels odd to create an additional rule to delete itself and its clone afterwards, it is still a valid workaround. And unfortunately, it's the only possible workaround, right now.

But as we're planning to release a new version of the collection tomorrow, we have to postpone this workaround.
For the planned release, I will reduce the function get_existing_rule() to only compare "properties" and "conditions". Also, I'll write a comment into the docs, saying that the rule module currently doesn't behave as idempotent as it should be.

After the release, we can implement your workaround.

The developers are currently still discussing how to change the Rest API to reply "raw_data" formatted in JSON. If they can do that, that'll make a long-term solution possible.

from ansible-collection-checkmk.general.

@lgetwan : what about the code I proposed in #224 ? The current implementation does not work at all, unless we provide a very precise value_raw string that's exactly identical as the representation returned by the API, and empty values (see #231). My version works all the time (except corner cases I'm try to adress now), although I'm not sure that using eval() is secure...

from ansible-collection-checkmk.general.

@geof77: using eval() is probably not a good idea.
@robin-tribe29, what do you think of that?

But besides that:
Unfortunately, we do have use cases where we have rules with

• identical conditions
• identical folder
• different settings.
  In all rule sets that do a "merge", this might occur.

from ansible-collection-checkmk.general.

@lgetwan indeed it's probably a bad idea, even "safer" alternatives have big warnings attached. I have code implementing the workaround for the "create" operation and it works perfectly. If you think it's ok to do the same trick with "delete", I will give it a try.

from ansible-collection-checkmk.general.

@geof77 You mean the "create new rule and delete both if equal"? Yes, you can give it a try. Can you do that today?

from ansible-collection-checkmk.general.

Unfortunately no

from ansible-collection-checkmk.general.

I mean... yes but it's not fully tested, this is my first try at it:
https://github.com/geof77/ansible-collection-tribe29.checkmk/tree/fix-rules-compare

from ansible-collection-checkmk.general.

@lgetwan I did some more tests and it works quite well, but I have to go now for a few hours. If you can have a look at it, I can make a PR later this evening. I have not tested delete.

from ansible-collection-checkmk.general.

@lgetwan @robin-tribe29
The workaround is done and all my tests succeed.
See #233

from ansible-collection-checkmk.general.

@lgetwan @muehlings @robin-tribe29

When I was looking at the source code of ansible's module_utils to see how argument validation works, I discovered Ansible has a 'safe_eval' function to parse string and create python dictionaries. It's a wrapper around AST's literal_eval. It can be found here:

https://github.com/ansible/ansible/blob/devel/lib/ansible/module_utils/common/validation.py

Unless there is strong advice against it, and since it is already used everywhere in ansible, I propose to modify the rule module to use ansible's safe_eval instead of creating a temp rule.

In fact, I already have the code in https://github.com/geof77/ansible-collection-tribe29.checkmk/tree/rule-safe-eval

But not tested at all yet... myabe later this evening.

What do you think?

from ansible-collection-checkmk.general.

@lgetwan Perhaps we should define a "target version" we want the ansible modules to work against (2.1.0pXY.*, python3.10+)?

@muehlings we are actually working towards proper support communication. Currently, we state the "tested scenario" for each release in SUPPORT.md. Additionally, the automated tests check compatibility for all current Ansible releases. That means, if you can run one of those Ansible versions, the collection should work too. Of course the next layer of complexity is Checkmk itself, and I do not know how good the current state of affairs is towards transparency regarding compatibility.

That being said, I am kind of out of the loop regarding the programming details in here, so I will leave this in the very capable hands of @lgetwan.

from ansible-collection-checkmk.general.

With #241, the need for

an API endpoint to get the API rule representation without actually creating it

is no longer there, I guess. What's your opinion about that, @geof77 and @muehlings?

from ansible-collection-checkmk.general.

@lgetwan : Good question. Indeed #241 does not need it. Maybe it could be useful to do some validation, or if it would accept an incomplete value_raw and return it completed with the default values,... I really don't know...

from ansible-collection-checkmk.general.

@geof77

if it would accept an incomplete value_raw and return it completed with the default values

this sounds indeed interesting!

and thanks for considerably improved rule module!

from ansible-collection-checkmk.general.

@robin-tribe29 @msekania @lgetwan :

I was just thinking... is there any use case where we would have several rules

• of the same ruleset
• in the same folder
• with the same conditions
• and a different value? (i.e. value_raw)

Does it make sense? My understanding is that the first rule matching the conditions would apply and the others would be ignored. If I'm correct, we do not have to compare the value_raw at all and in fact it's not the ideal behavior. So my patches to create a temp rule or use safe_eval to compare value_raw are not necessary... I just stick to the original idea and tried to make it work, but I think it's a bad idea.

I was reading about the host module and the way it moves hosts (issue #175). The rule module should behave similarly.
That is, if we want to create a rule with the same, folder, ruleset and conditions, but different value, the module should not create it but "modify" or "update" the existing rule, simply changing the value_raw.

Otherwise it would be very difficult to modify rules, we would have to look for the existing rule with the correct value_raw and I see no way to do it easily.

So I propose to modify the behavior of the rule module to compare only the ruleset, folder and conditions, and if there is already a rule matching this, we simply delete it and recreate it with the new value_raw.

That would be a huge improvement IMHO.

from ansible-collection-checkmk.general.

@geof77

That is, if we want to create a rule with the same, folder, ruleset and conditions, but different value, the module should not create it but "modify" or "update" the existing rule, simply changing the value_raw.

Yes, I would say that it should behave almost like that, but with an additional check.
It should also take into account the keys in value_raw. So the keys of key-value pairs should also match, but not the values.
But I'm not quite sure how difficult that would be to implement. However, I am willing to help where I can.

"Surprisingly", there is no modify or update function for rule in the REST API.

Otherwise it would be very difficult to modify rules, we would have to look for the existing rule with the correct value_raw and I see no way to do it easily.

How about introducing a flag that explicitly states that ignoring the contents of "value_raw" is explicitly intended in rule comparison?

So I propose to modify the behavior of the rule module to compare only the ruleset, folder and conditions, and if there is already a rule matching this, we simply delete it and recreate it with the new value_raw.

I see the problem here:

delete it and recreate it
namely, if rule_id is changed, it is neither a modify nor an update.

from ansible-collection-checkmk.general.

Hi @geof77 & @msekania,

there are use cases for two rules with identical conditions but different value_raw. E.g.:

Cheers
Lars

from ansible-collection-checkmk.general.

@msekania @lgetwan Sorry for the late response. I will start a new thread to discuss rule updates.

from ansible-collection-checkmk.general.

@Max-checkmk,

Unfortunately it's not so easy, there are several threads where the issues have been discussed extensively, see e.g. #186.

One of the main problems for example: how do you enter the python tuple-s, neither JSON nor YAML have this structure.

With string conversions one also should take care that one does not have extra \n inside.

One should either in the REST API value_raw stricter or write a convertor module.

from ansible-collection-checkmk.general.

Today I tried to convert my existing reference sites to ansible playbooks (read everything with api, construct the yaml).
And then I stumbled over a change in 2.2 (they started to get rid of tuples, yeah!) and some rules who broke my ansible playbook. I did a lot of debugging and tried to get behind the logic why some rules where failing. They looked identical to the byte unless I looked at the comparison of the "disabled" state:

                and r["extensions"]["properties"].get("disabled", False)
                == rule["properties"].get("disabled", False) # StefanM: changed from "" to False

There were indeed rules where the get() on "disabled" gave a result (probably rules where the rule was enabled and disabled and the information remains in the rule) and then on the other side the default hit and set "" for disabled. Before that change ansible almost always generated the rules again and again. Now unchanged rules stay "ok", changed rules and new rules get "changed" as module status. Can anybody please cross check that?

from ansible-collection-checkmk.general.

Just hit this (long) discussion and some others related as I'm trying to setup a Checkmk instance a completely declarative manner (or better, this is wat I'm asked to).

For my basic understanding, this is quite impossible as of today.
Any value change for a rule will cause the creation of a new rule, instead of the update it (or delete and recreate).

Another big issue I'm seeing here is that is not possible to define with certainty where a rule will be add on the ruleset. On the first creation, I can add all my rules (hypothetically defined in an inventory list) on the bottom of the ruleset. But after, from day2, how does this going? As the order of appearance matters, how can I define a position that will not change?

Other than a some sort of rule "id", I think that also a numeral position inside the ruleset is needed to achieve the goal of idempotency and declarative definition of rules.

from ansible-collection-checkmk.general.

in issue #523 I described the issue we are facing, which is in fact the same issue as described here. Characters get replaced in value_raw when using ansible.

While using the exact same value_raw via Swagger UI, it works as expected. I did read this entire issue, however not really sure what the exact issue is now, or what a workaround for the issue is.

in regards of the main discussion, i am not into API/ansible enough to provide any valuable input there.

from ansible-collection-checkmk.general.

I have given up on this. The API is not consistent and the module logic is not consistent.

3 examples:

• Service discovery rules->Disabled services
  3 rules work ("changed" when new or changed, "ok" when nothing to do):

  • 2 with complex regexp,
  • 1 with "Service name begins with".
  • "Service name is" does always produce new rules.
  • 14 rules do not work (always "changed", even if the use the same logic ("begins with", "regexp") as one of the working rules. Always creates new rules.

• Check DNS service:
  Always says "changed". Always creates a new rule.

• Memory levels for Windows/Linux/ESX:
  Always says "changed", even if the content is the same and the rule is not actively updated. Rules are created only once. Cosmetic problem: "changed" is wrong from logic view, rest works as expected.

I work around this with deleting rules.mk and writing the rulesets with ansible again.

Without a full compare (ansible module task) and a reliable source of the real rule content (API task) there will be no solution to this problem.

from ansible-collection-checkmk.general.