Forget that attribute is modified if it's changed back to its original state via API #8036

Al2Klimov · 2020-05-27T16:19:07Z

The title says it all.

Edit

E.g. if I change the Director-configured enable_active_checks=true via API to false and back to true, Icinga forgets the false instead of replacing it with true. I.e. mod. attrs. neutralise each other. Director config changes cause no such thing and IIRC we don't want that (#8739).

Al2Klimov · 2020-05-27T16:58:55Z

@lippserd This one covers only leaves, e.g. Checkable#enable_active_checks, CustomVarObject#vars.os. Shall I leave the more complicated cases as-is not to touch a running system?

lippserd · 2020-06-09T07:24:51Z

@Al2Klimov Yep, leaves should be should be sufficient.

yhabteab · 2020-06-19T13:40:36Z

Testprotocol:
It is so, if you now modify an object in Icinga2, this object is logically saved in the modified-attributes.conf file. After restarting Icinga2 daemon let's assume we reset the object to the previous value, Icinga2 could not forget this modified object but it is still saved in the file.

Command to modify an object: curl -k -v -u root:icinga -H 'Accept: application/json' -X POST 'https://localhost:5665/v1/objects/hosts/example.localdomain' -d '{"attrs": {"vars.os": "Ubuntu"}, "pretty": true}'
This modified object looks like this in the file..

var obj = get_object("Host", "example.localdomain")
if (obj) {
        obj.modify_attribute("vars.os", "Ubuntu")
        obj.version = 1592573120.186998
}

Now we reset this modified object..
Command: curl -k -v -u root:icinga -H 'Accept: application/json' -X POST 'https://localhost:5665/v1/objects/hosts/example.localdomain' -d '{"attrs": {"vars.os": "Linux"}, "retty": true}'

Although we have reset this modified object to the previous value, it will only update the value in the file instead of completely removing it from the file.

With this branch, Icinga2 is able to meet requirements. Firstly we modified an object just the same as before.

ar obj = get_object("Host", "example.localdomain")
if (obj) {
        obj.modify_attribute("vars.os", "Ubuntu")
        obj.version = 1592572775.155900
}

And now we set this object to the previous value and restart Icinga2 daemon.
After this the object will be removed completely from the modified-attributes.conf i.e Icinga2 is able to forgot this object.

Al2Klimov · 2021-08-04T12:22:18Z

@cla-bot check

Al2Klimov · 2023-01-20T09:35:46Z

ref/IP/43160

julianbrost · 2023-02-06T15:34:27Z

I find the use of JSON encoding in this PR very surprising.

This one covers only leaves, e.g. Checkable#enable_active_checks, CustomVarObject#vars.os. Shall I leave the more complicated cases as-is not to touch a running system?

Given that there seems to be no update to the PR since that comment, I presume that's still up-to-date. Shouldn't this imply that this PR would work for scalar values only anyways and if so, why would they need JSON encoding to compare them?

Al2Klimov · 2023-02-06T16:16:25Z

This workarounds the console's

<1> => "" == null
true

and

<2> => {} == {}
false

lippserd · 2023-02-06T16:18:07Z

This workarounds the console's
<1> => "" == null
true
and
<2> => {} == {}
false

And why does this matter here?

Al2Klimov · 2023-02-06T16:23:41Z

Admittedly the second example seems not to matter. But the first one shows that the DSL by itself doesn't compare values in a way we need here.

julianbrost · 2023-02-10T11:35:21Z

Would additionally comparing the type do the trick here?

Al2Klimov · 2023-02-10T12:15:14Z

Ah, now I've digged it out! I meant dict leaves – which include arrays:

$ prefix/sbin/icinga2 daemon -d
[2023-02-10 13:09:28 +0100] information/cli: Icinga application loader (version: v2.13.0-610-g0e6341859; debug)
[2023-02-10 13:09:28 +0100] information/cli: Closing console log.
$ curl -ksSu root:123456 -H 'Accept: application/json' -X POST -d '{"attrs":{"vars.notification.mail.groups":["icingaadmins II"]}}' https://127.0.0.1:5665/v1/objects/hosts/'alexandersmbp2.int.netways.de';echo
{"results":[{"code":200,"name":"alexandersmbp2.int.netways.de","status":"Attributes updated.","type":"Host"}]}
$ kill -HUP `cat ../icinga2/prefix/var/run/icinga2/icinga2.pid`
$ cat prefix/var/lib/icinga2/modified-attributes.conf
var obj = get_object("Host", "alexandersmbp2.int.netways.de")
if (obj) {
        obj.modify_attribute("vars.notification.mail.groups", [ "icingaadmins II" ])
        obj.version = 1676031045.182756
}
$ curl -ksSu root:123456 -H 'Accept: application/json' -X POST -d '{"attrs":{"vars.notification.mail.groups":["icingaadmins"]}}' https://127.0.0.1:5665/v1/objects/hosts/'alexandersmbp2.int.netways.de';echo
{"results":[{"code":200,"name":"alexandersmbp2.int.netways.de","status":"Attributes updated.","type":"Host"}]}
$ kill -HUP `cat ../icinga2/prefix/var/run/icinga2/icinga2.pid`
$ cat prefix/var/lib/icinga2/modified-attributes.conf
$

😅

These may also include dicts:

$ curl -ksSu root:123456 -H 'Accept: application/json' -X POST -d '{"attrs":{"vars.notification.mail.groups":[{"icingaadmins III":"Revenge of the Sith"}]}}' https://127.0.0.1:5665/v1/objects/hosts/'alexandersmbp2.int.netways.de';echo
{"results":[{"code":200,"name":"alexandersmbp2.int.netways.de","status":"Attributes updated.","type":"Host"}]}
$ kill -HUP `cat ../icinga2/prefix/var/run/icinga2/icinga2.pid`
$ cat prefix/var/lib/icinga2/modified-attributes.conf
var obj = get_object("Host", "alexandersmbp2.int.netways.de")
if (obj) {
        obj.modify_attribute("vars.notification.mail.groups", [ {
"icingaadmins III" = "Revenge of the Sith"
} ])
        obj.version = 1676031207.947003
}
$ curl -ksSu root:123456 -H 'Accept: application/json' -X POST -d '{"attrs":{"vars.notification.mail.groups":["icingaadmins"]}}' https://127.0.0.1:5665/v1/objects/hosts/'alexandersmbp2.int.netways.de';echo
{"results":[{"code":200,"name":"alexandersmbp2.int.netways.de","status":"Attributes updated.","type":"Host"}]}
$ kill -HUP `cat ../icinga2/prefix/var/run/icinga2/icinga2.pid`
$ cat prefix/var/lib/icinga2/modified-attributes.conf
$

And JSON catches them all.

julianbrost · 2023-05-25T13:58:28Z

I'm wondering why you're using JsonEncode(Serialize(...)) instead of just JsonEncode(...)? The values come from JSON, so is there a need for putting them through Serialize(...) first?

Apart from that, while testing I noticed some behavior. For the first one, my intuition is that it shouldn't be that way, for the second one, I'm not sure what it would need to change that as all that nested dict stuff is somewhat special anyways.

Setting the same initial value

When setting a variable to the initial value while this is also the current value, this generates a modified attribute.

Start with a host with vars.x = "initial value" in the config.

Starting with the clean state, not modified attributes:

$ curl -sSku root:icinga https://localhost:5665/v1/objects/hosts/master-1 --json '{"pretty":1, "attrs":["original_attributes", "vars"]}' -X GET  
                "original_attributes": null,
                "vars": {
                    "x": "initial value"
                }

Now set that variable to the very same value:

$ curl -sSku root:icinga https://localhost:5665/v1/objects/hosts/master-1 --json '{"pretty":1, "attrs": {"vars.x": "initial value"}}'
            "status": "Attributes updated.",

Observe how this has created a modified attribute:

$ curl -sSku root:icinga https://localhost:5665/v1/objects/hosts/master-1 --json '{"pretty":1, "attrs":["original_attributes", "vars"]}' -X GET
            "attrs": {
                "original_attributes": {
                    "vars.x": "initial value"
                },
                "vars": {
                    "x": "initial value"
                }

When reloading the daemon (or waiting long enough for the file to be rewritten by itself), this is also persisted to modified-attributes.conf:

var obj = get_object("Host", "master-1")
if (obj) {
	obj.modify_attribute("vars.x", "initial value")
	obj.version = 1685021240.857364
}

Perform the very same update with the same value again:

$ curl -sSku root:icinga https://localhost:5665/v1/objects/hosts/master-1 --json '{"pretty":1, "attrs": {"vars.x": "initial value"}}'
            "status": "Attributes updated.",

Observe how the modified attribute is gone again:

$ curl -sSku root:icinga https://localhost:5665/v1/objects/hosts/master-1 --json '{"pretty":1, "attrs":["original_attributes", "vars"]}' -X GET
            "attrs": {
                "original_attributes": {},
                "vars": {
                    "x": "initial value"
                }

Inconsistent behavior with nested dictionaries

Start with an object with two non-empty dicts as custom variables:

vars.y = { a = 42 }
vars.z = { a = 42 }

Starting with a clean state, original values are the current values, no modified attributes:

$ curl -sSku root:icinga https://localhost:5665/v1/objects/hosts/master-1 --json '{"pretty":1, "attrs":["original_attributes", "vars"]}' -X GET
                "original_attributes": null,
                "vars": {
                    "y": {
                        "a": 42
                    },
                    "z": {
                        "a": 42
                    }
                }

Modify both numbers, one using the flattened key, one using a nested dict:

$ curl -sSku root:icinga https://localhost:5665/v1/objects/hosts/master-1 --json '{"pretty":1, "attrs": {"vars.y.a": 23, "vars.z": {"a": 23}}}' 
            "status": "Attributes updated.",

The effect on both variables was exactly the same:

$ curl -sSku root:icinga https://localhost:5665/v1/objects/hosts/master-1 --json '{"pretty":1, "attrs":["original_attributes", "vars"]}' -X GET 
                "original_attributes": {
                    "vars.y.a": 42,
                    "vars.z.a": 42
                },
                "vars": {
                    "y": {
                        "a": 23
                    },
                    "z": {
                        "a": 23
                    }
                }

Reset both values back to the original values using the same method:

$ curl -sSku root:icinga https://localhost:5665/v1/objects/hosts/master-1 --json '{"pretty":1, "attrs": {"vars.y.a": 42, "vars.z": {"a": 42}}}'
            "status": "Attributes updated.",

This only removed the modified attribute for the flattened one, the override with the nested dict remained:

$ curl -sSku root:icinga https://localhost:5665/v1/objects/hosts/master-1 --json '{"pretty":1, "attrs":["original_attributes", "vars"]}' -X GET 
            "attrs": {
                "original_attributes": {
                    "vars.z.a": 42
                },
                "vars": {
                    "y": {
                        "a": 42
                    },
                    "z": {
                        "a": 42
                    }
                }

All tests were done with the PR rebased onto the current master at d871c5c, responses are simplified to the relevant part.

…l state refs #7986

Al2Klimov · 2023-05-25T14:29:41Z

I'm wondering why you're using JsonEncode(Serialize(...)) instead of just JsonEncode(...)?

You're right. JsonEncode() already handles alien objects. 🛸🐘

When setting a variable to the initial value while this is also the current value, this generates a modified attribute.

Yes, admittedly this is weird. But it's what the user wants. Also you can construct the same case by changing the config e.g. true -> false, modifying the attribute false -> true and changing the config back to true. I.e. true in config and in a modified attribute. No #8739 to the rescue as we've disposed it. So I'm not worried.

Perform the very same update with the same value again:

$ curl -sSku root:icinga https://localhost:5665/v1/objects/hosts/master-1 --json '{"pretty":1, "attrs": {"vars.x": "initial value"}}'
            "status": "Attributes updated.",

Observe how the modified attribute is gone again:

$ curl -sSku root:icinga https://localhost:5665/v1/objects/hosts/master-1 --json '{"pretty":1, "attrs":["original_attributes", "vars"]}' -X GET
            "attrs": {
                "original_attributes": {},
                "vars": {
                    "x": "initial value"
                }

Nice! (Would be bad if you couldn’t remove that thing.)

Inconsistent behavior with nested dictionaries

Start with an object with two non-empty dicts as custom variables:

vars.y = { a = 42 }
vars.z = { a = 42 }

Starting with a clean state, original values are the current values, no modified attributes:

$ curl -sSku root:icinga https://localhost:5665/v1/objects/hosts/master-1 --json '{"pretty":1, "attrs":["original_attributes", "vars"]}' -X GET
                "original_attributes": null,
                "vars": {
                    "y": {
                        "a": 42
                    },
                    "z": {
                        "a": 42
                    }
                }

Modify both numbers, one using the flattened key, one using a nested dict:

$ curl -sSku root:icinga https://localhost:5665/v1/objects/hosts/master-1 --json '{"pretty":1, "attrs": {"vars.y.a": 23, "vars.z": {"a": 23}}}' 
            "status": "Attributes updated.",

The effect on both variables was exactly the same:

$ curl -sSku root:icinga https://localhost:5665/v1/objects/hosts/master-1 --json '{"pretty":1, "attrs":["original_attributes", "vars"]}' -X GET 
                "original_attributes": {
                    "vars.y.a": 42,
                    "vars.z.a": 42
                },
                "vars": {
                    "y": {
                        "a": 23
                    },
                    "z": {
                        "a": 23
                    }
                }

Reset both values back to the original values using the same method:

$ curl -sSku root:icinga https://localhost:5665/v1/objects/hosts/master-1 --json '{"pretty":1, "attrs": {"vars.y.a": 42, "vars.z": {"a": 42}}}'
            "status": "Attributes updated.",

This only removed the modified attribute for the flattened one, the override with the nested dict remained:

$ curl -sSku root:icinga https://localhost:5665/v1/objects/hosts/master-1 --json '{"pretty":1, "attrs":["original_attributes", "vars"]}' -X GET 
            "attrs": {
                "original_attributes": {
                    "vars.z.a": 42
                },
                "vars": {
                    "y": {
                        "a": 42
                    },
                    "z": {
                        "a": 42
                    }
                }

I know. I've explicitly asked permission not to over-engineer this PR: #8036 (comment)

julianbrost · 2023-05-26T12:53:29Z

When setting a variable to the initial value while this is also the current value, this generates a modified attribute.

Yes, admittedly this is weird. But it's what the user wants.

Well, maybe. The code is using a value to guess what the use wants, as there's no explicit create/drop override information in the request. So the question is, which of these two statements better describes the desired behavior:

If the attribute is already modified and it's set to the value from the config file, the override is dropped, otherwise one is created.
If the attribute is set to the value from the file, there will be no override afterwards, otherwise one is created.

Let me quote the suggestion from #7986:

I think we should just simply drop the modified attribute entry if it matches the current configuration.

The current state of the PR adds a "but create it if there's nothing to drop" to that statement, which sounds kind of weird. And given that you also called the behavior weird yourself, I'm not sure that (1) is the better option.

Also keep in mind that you can modify the attributes of multiple objects at once, so in case you do a POST request with {"attrs":{"enable_active_checks":true}} to /v1/objects/services (possibly with some filter, or without because you want to just re-enable checks everywhere), do we want to end up with an override for all objects that currently have enable_active_checks = true without an active override?

Also you can construct the same case by changing the config e.g. true -> false, modifying the attribute false -> true and changing the config back to true. I.e. true in config and in a modified attribute. No #8739 to the rescue as we've disposed it. So I'm not worried.

Not exactly the same. Yes, you end up in a state where there's an override that has the same value as the config, but there never was an API request updating the field to the value that was present in the config file at that time.

Al2Klimov · 2023-05-26T13:06:43Z

This PR is about forgetting existing ones, not (not?) creating new ones. So your argument is valid, but not blocking anything.

…s original state via API. #8036

Al2Klimov added the enhancement New feature or request label May 27, 2020

Al2Klimov added this to the 2.13.0 milestone May 27, 2020

Al2Klimov self-assigned this May 27, 2020

Al2Klimov assigned lippserd and unassigned Al2Klimov May 27, 2020

Al2Klimov added the needs feedback We'll only proceed once we hear from you again label May 27, 2020

lippserd removed their assignment Jun 9, 2020

lippserd removed the needs feedback We'll only proceed once we hear from you again label Jun 9, 2020

Al2Klimov marked this pull request as ready for review June 9, 2020 08:35

Al2Klimov requested a review from yhabteab June 19, 2020 12:04

yhabteab previously approved these changes Jun 19, 2020

View reviewed changes

Al2Klimov requested a review from julianbrost December 23, 2020 16:02

julianbrost mentioned this pull request Apr 6, 2021

API Call to revert modified attributes vs. editing modified-attributes.conf #8709

Closed

abaumgaertner mentioned this pull request Apr 6, 2021

Forget that attribute is modified if it's changed back to its original state via API #7986

Closed

Al2Klimov mentioned this pull request Apr 19, 2021

More recent changes in the configuration are overwritten by the modified-attributes.conf #8717

Closed

N-o-X self-requested a review July 7, 2021 09:15

julianbrost modified the milestones: 2.13.0, 2.14.0 Jul 26, 2021

cla-bot bot added the cla/signed label Aug 4, 2021

julianbrost removed their request for review November 30, 2021 15:46

julianbrost removed the request for review from N-o-X January 17, 2023 15:05

Al2Klimov added the ref/IP label Jan 20, 2023

Al2Klimov requested a review from julianbrost January 20, 2023 09:38

lippserd changed the title ~~Forget that attribute is modified if it's changed back to its original state~~ Forget that attribute is modified if it's changed back to its original state via API Mar 30, 2023

This was referenced May 15, 2023

Icinga 2.14 #9760

Merged

GHA: Linux: add Fedora 38, Ubuntu 23.04, Debian 12, openSUSE 15.5 #9743

Merged

Forget that attribute is modified if it's changed back to its origina…

700a216

…l state refs #7986

Al2Klimov dismissed yhabteab’s stale review via 700a216 May 25, 2023 14:29

Al2Klimov force-pushed the feature/forget-attribute-modified-original-7986 branch from 885a19f to 700a216 Compare May 25, 2023 14:29

Al2Klimov mentioned this pull request Jun 6, 2023

Doc: POST /v1/objects: insist on "vars.os":"Linux" format #9782

Closed

Al2Klimov closed this Jun 12, 2023

icinga-probot bot removed this from the 2.14.0 milestone Jun 12, 2023

icinga-probot bot deleted the feature/forget-attribute-modified-original-7986 branch June 12, 2023 08:40

Al2Klimov added a commit that referenced this pull request Jul 3, 2023

Forget that attribute was modified via API if it's changed back to it…

eb99df3

…s original state via API. #8036

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Forget that attribute is modified if it's changed back to its original state via API #8036

Forget that attribute is modified if it's changed back to its original state via API #8036

Al2Klimov commented May 27, 2020 •

edited

Loading

Al2Klimov commented May 27, 2020

lippserd commented Jun 9, 2020

yhabteab commented Jun 19, 2020

Al2Klimov commented Aug 4, 2021

Al2Klimov commented Jan 20, 2023

julianbrost commented Feb 6, 2023 •

edited

Loading

Al2Klimov commented Feb 6, 2023

lippserd commented Feb 6, 2023

Al2Klimov commented Feb 6, 2023

julianbrost commented Feb 10, 2023

Al2Klimov commented Feb 10, 2023

julianbrost commented May 25, 2023

Al2Klimov commented May 25, 2023

Inconsistent behavior with nested dictionaries

julianbrost commented May 26, 2023

Al2Klimov commented May 26, 2023

Forget that attribute is modified if it's changed back to its original state via API #8036

Forget that attribute is modified if it's changed back to its original state via API #8036

Conversation

Al2Klimov commented May 27, 2020 • edited Loading

Edit

Al2Klimov commented May 27, 2020

lippserd commented Jun 9, 2020

yhabteab commented Jun 19, 2020

Al2Klimov commented Aug 4, 2021

Al2Klimov commented Jan 20, 2023

julianbrost commented Feb 6, 2023 • edited Loading

Al2Klimov commented Feb 6, 2023

lippserd commented Feb 6, 2023

Al2Klimov commented Feb 6, 2023

julianbrost commented Feb 10, 2023

Al2Klimov commented Feb 10, 2023

julianbrost commented May 25, 2023

Setting the same initial value

Inconsistent behavior with nested dictionaries

Al2Klimov commented May 25, 2023

Inconsistent behavior with nested dictionaries

julianbrost commented May 26, 2023

Al2Klimov commented May 26, 2023

Al2Klimov commented May 27, 2020 •

edited

Loading

julianbrost commented Feb 6, 2023 •

edited

Loading