Forget modified attribute on config change #8739

Al2Klimov · 2021-04-22T16:56:50Z

Al2Klimov · 2021-04-22T16:57:44Z

Before

➜  icinga2 git:(master) prefix/sbin/icinga2 api setup -x critical
Enabling feature api. Make sure to restart Icinga 2 for these changes to take effect.
Done.

Now restart your Icinga 2 daemon to finish the installation!

➜  icinga2 git:(master) perl -pi -e 's/\bpassword = ".*?"/password = "123456"/' prefix/etc/icinga2/conf.d/api-users.conf
➜  icinga2 git:(master) prefix/sbin/icinga2 daemon -d
[2021-04-22 18:18:18 +0200] information/cli: Icinga application loader (version: v2.12.0-576-g8dc069dc2)
[2021-04-22 18:18:18 +0200] information/cli: Closing console log.
➜  icinga2 git:(master) curl -fksSLu root:123456 -X GET -H 'Accept: application/json' -d '{"attrs":["vars"],"pretty":true}' https://127.0.0.1:5665/v1/objects/hosts/Alexanders-MacBook-Pro.local
{
    "results": [
        {
            "attrs": {
                "vars": {
                    "disks": {
                        "disk": {},
                        "disk /": {
                            "disk_partitions": "/"
                        }
                    },
                    "http_vhosts": {
                        "http": {
                            "http_uri": "/"
                        }
                    },
                    "notification": {
                        "mail": {
                            "groups": [
                                "icingaadmins"
                            ]
                        }
                    },
                    "os": "Linux"
                }
            },
            "joins": {},
            "meta": {},
            "name": "Alexanders-MacBook-Pro.local",
            "type": "Host"
        }
    ]
}
➜  icinga2 git:(master) curl -fksSLu root:123456 -X POST -H 'Accept: application/json' -d '{"attrs":{"vars.disks.disk /.disk_partitions":"/x"},"pretty":true}' https://127.0.0.1:5665/v1/objects/hosts/Alexanders-MacBook-Pro.local
{
    "results": [
        {
            "code": 200,
            "name": "Alexanders-MacBook-Pro.local",
            "status": "Attributes updated.",
            "type": "Host"
        }
    ]
}
➜  icinga2 git:(master) curl -fksSLu root:123456 -X GET -H 'Accept: application/json' -d '{"attrs":["vars"],"pretty":true}' https://127.0.0.1:5665/v1/objects/hosts/Alexanders-MacBook-Pro.local
{
    "results": [
        {
            "attrs": {
                "vars": {
                    "disks": {
                        "disk": {},
                        "disk /": {
                            "disk_partitions": "/x"
                        }
                    },
                    "http_vhosts": {
                        "http": {
                            "http_uri": "/"
                        }
                    },
                    "notification": {
                        "mail": {
                            "groups": [
                                "icingaadmins"
                            ]
                        }
                    },
                    "os": "Linux"
                }
            },
            "joins": {},
            "meta": {},
            "name": "Alexanders-MacBook-Pro.local",
            "type": "Host"
        }
    ]
}
➜  icinga2 git:(master) perl -pi -e 'if (/disk_partitions/) { s~/~/y~ }' prefix/etc/icinga2/conf.d/hosts.conf
➜  icinga2 git:(master) kill -HUP `cat prefix/var/run/icinga2/icinga2.pid`
➜  icinga2 git:(master) curl -fksSLu root:123456 -X GET -H 'Accept: application/json' -d '{"attrs":["vars"],"pretty":true}' https://127.0.0.1:5665/v1/objects/hosts/Alexanders-MacBook-Pro.local
{
    "results": [
        {
            "attrs": {
                "vars": {
                    "disks": {
                        "disk": {},
                        "disk /": {
                            "disk_partitions": "/x"
                        }
                    },
                    "http_vhosts": {
                        "http": {
                            "http_uri": "/"
                        }
                    },
                    "notification": {
                        "mail": {
                            "groups": [
                                "icingaadmins"
                            ]
                        }
                    },
                    "os": "Linux"
                }
            },
            "joins": {},
            "meta": {},
            "name": "Alexanders-MacBook-Pro.local",
            "type": "Host"
        }
    ]
}
➜  icinga2 git:(master)

After

➜  icinga2 git:(bugfix/mod-attr-8717) prefix/sbin/icinga2 api setup -x critical
Enabling feature api. Make sure to restart Icinga 2 for these changes to take effect.
Done.

Now restart your Icinga 2 daemon to finish the installation!

➜  icinga2 git:(bugfix/mod-attr-8717) perl -pi -e 's/\bpassword = ".*?"/password = "123456"/' prefix/etc/icinga2/conf.d/api-users.conf
➜  icinga2 git:(bugfix/mod-attr-8717) prefix/sbin/icinga2 daemon -d
[2021-04-22 18:53:10 +0200] information/cli: Icinga application loader (version: v2.12.0-582-g468807718)
[2021-04-22 18:53:10 +0200] information/cli: Closing console log.
➜  icinga2 git:(bugfix/mod-attr-8717) curl -fksSLu root:123456 -X POST -H 'Accept: application/json' -d '{"attrs":{"vars.disks.disk /.disk_partitions":"/x"},"pretty":true}' https://127.0.0.1:5665/v1/objects/hosts/Alexanders-MacBook-Pro.local
{
    "results": [
        {
            "code": 200,
            "name": "Alexanders-MacBook-Pro.local",
            "status": "Attributes updated.",
            "type": "Host"
        }
    ]
}
➜  icinga2 git:(bugfix/mod-attr-8717) curl -fksSLu root:123456 -X GET -H 'Accept: application/json' -d '{"attrs":["vars"],"pretty":true}' https://127.0.0.1:5665/v1/objects/hosts/Alexanders-MacBook-Pro.local
{
    "results": [
        {
            "attrs": {
                "vars": {
                    "disks": {
                        "disk": {},
                        "disk /": {
                            "disk_partitions": "/x"
                        }
                    },
                    "http_vhosts": {
                        "http": {
                            "http_uri": "/"
                        }
                    },
                    "notification": {
                        "mail": {
                            "groups": [
                                "icingaadmins"
                            ]
                        }
                    },
                    "os": "Linux"
                }
            },
            "joins": {},
            "meta": {},
            "name": "Alexanders-MacBook-Pro.local",
            "type": "Host"
        }
    ]
}
➜  icinga2 git:(bugfix/mod-attr-8717) kill -HUP `cat prefix/var/run/icinga2/icinga2.pid`
➜  icinga2 git:(bugfix/mod-attr-8717) cat prefix/var/lib/icinga2/modified-attributes.conf
var obj = get_object("Host", "Alexanders-MacBook-Pro.local")
if (obj) {
	var result
	if (obj.get_attribute("vars.disks.disk /.disk_partitions", &result) && Json.encode(Internal.serialize(result, 2 /* FAConfig */)) == "\"/\"") {
		obj.modify_attribute("vars.disks.disk /.disk_partitions", "/x")
	}
	obj.version = 1619110395.070914
}
➜  icinga2 git:(bugfix/mod-attr-8717) curl -fksSLu root:123456 -X GET -H 'Accept: application/json' -d '{"attrs":["vars"],"pretty":true}' https://127.0.0.1:5665/v1/objects/hosts/Alexanders-MacBook-Pro.local
{
    "results": [
        {
            "attrs": {
                "vars": {
                    "disks": {
                        "disk": {},
                        "disk /": {
                            "disk_partitions": "/x"
                        }
                    },
                    "http_vhosts": {
                        "http": {
                            "http_uri": "/"
                        }
                    },
                    "notification": {
                        "mail": {
                            "groups": [
                                "icingaadmins"
                            ]
                        }
                    },
                    "os": "Linux"
                }
            },
            "joins": {},
            "meta": {},
            "name": "Alexanders-MacBook-Pro.local",
            "type": "Host"
        }
    ]
}
➜  icinga2 git:(bugfix/mod-attr-8717) perl -pi -e 'if (/disk_partitions/) { s~/~/y~ }' prefix/etc/icinga2/conf.d/hosts.conf
➜  icinga2 git:(bugfix/mod-attr-8717) kill -HUP `cat prefix/var/run/icinga2/icinga2.pid`
➜  icinga2 git:(bugfix/mod-attr-8717) cat prefix/var/lib/icinga2/modified-attributes.conf
var obj = get_object("Host", "Alexanders-MacBook-Pro.local")
if (obj) {
	var result
	if (obj.get_attribute("vars.disks.disk /.disk_partitions", &result) && Json.encode(Internal.serialize(result, 2 /* FAConfig */)) == "\"/\"") {
		obj.modify_attribute("vars.disks.disk /.disk_partitions", "/x")
	}
	obj.version = 1619110395.070914
}
➜  icinga2 git:(bugfix/mod-attr-8717) curl -fksSLu root:123456 -X GET -H 'Accept: application/json' -d '{"attrs":["vars"],"pretty":true}' https://127.0.0.1:5665/v1/objects/hosts/Alexanders-MacBook-Pro.local
{
    "results": [
        {
            "attrs": {
                "vars": {
                    "disks": {
                        "disk": {},
                        "disk /": {
                            "disk_partitions": "/y"
                        }
                    },
                    "http_vhosts": {
                        "http": {
                            "http_uri": "/"
                        }
                    },
                    "notification": {
                        "mail": {
                            "groups": [
                                "icingaadmins"
                            ]
                        }
                    },
                    "os": "Linux"
                }
            },
            "joins": {},
            "meta": {},
            "name": "Alexanders-MacBook-Pro.local",
            "type": "Host"
        }
    ]
}
➜  icinga2 git:(bugfix/mod-attr-8717) kill -HUP `cat prefix/var/run/icinga2/icinga2.pid`
➜  icinga2 git:(bugfix/mod-attr-8717) cat prefix/var/lib/icinga2/modified-attributes.conf
➜  icinga2 git:(bugfix/mod-attr-8717) curl -fksSLu root:123456 -X GET -H 'Accept: application/json' -d '{"attrs":["vars"],"pretty":true}' https://127.0.0.1:5665/v1/objects/hosts/Alexanders-MacBook-Pro.local
{
    "results": [
        {
            "attrs": {
                "vars": {
                    "disks": {
                        "disk": {},
                        "disk /": {
                            "disk_partitions": "/y"
                        }
                    },
                    "http_vhosts": {
                        "http": {
                            "http_uri": "/"
                        }
                    },
                    "notification": {
                        "mail": {
                            "groups": [
                                "icingaadmins"
                            ]
                        }
                    },
                    "os": "Linux"
                }
            },
            "joins": {},
            "meta": {},
            "name": "Alexanders-MacBook-Pro.local",
            "type": "Host"
        }
    ]
}
➜  icinga2 git:(bugfix/mod-attr-8717)

julianbrost

I think before changing this PR, we should have a discussion how we actually expect and want modified attributes to behave.

julianbrost · 2021-06-22T10:18:21Z

lib/icinga/icingaapplication.cpp

+	ConfigWriter::EmitRaw(fp, "\tif (obj.get_attribute(");
+	ConfigWriter::EmitString(fp, attr);
+	ConfigWriter::EmitRaw(fp, ", &result) && Json.encode(Internal.serialize(result, 2 /* FAConfig */)) == ");


Given that obj.get_attribute(...) will return false if the attribute does not exist in the original object (i.e. as read from the regular config files), this will lose modified attributes in this case.

Also, is JSON-encoding things really the best we can do to compare values?

Let’s say: "if the attribute does not exist in the original object anymore" – that’s this PR's intention

Inside config files IMAO yes

That's not what the emitted code checks for. obj at that point corresponds to the object in the config. So if set a modified attribute does_not_exist_in_config_file using the API and then restart Icinga, you would lose that attribute as the get_attribute() call returns false.

But this persists JSON across restarts and then uses string comparison later. Thus we could never again change something in the JSON serialization without risking losing modified attributes. If we would have done JsonEncode(): serialize integers w/o trailing .0 #8697 after this PR, if you had an modified attribute where the original value was an integer, the modified attribute would have been dropped ("1.0" != "1", therefore the config would be considered as changed even though it wasn't).

julianbrost · 2021-06-23T12:51:03Z

This won't make it into 2.13, there are too many remaining issues. Another example: since this persists JSON to later use it in a string comparison, this would effectively prevent any future changes to the JSON encoder. Think of something like #8697 or #8748, with this PR merged, we couldn't do any of that anymore without risking to lose modified attributes.

Al2Klimov · 2021-08-04T12:21:13Z

@cla-bot check

refs #8717

…ck as well refs #8717

…ersistModAttrHelper() as well refs #8717

…dn't change refs #8717

Al2Klimov · 2021-08-11T11:43:25Z

Better?

var obj = get_object("Host", "Alexanders-MacBook-Pro.local")
if (obj) {
	var result
	if (!obj.get_attribute("vars.lol.cat", &result) || Json.encode(Internal.serialize(result, 2.000000)) == "null") {
		obj.modify_attribute("vars.lol.cat", true)
	}
	if (obj.get_attribute("vars.os", &result) && Json.encode(Internal.serialize(result, 2.000000)) == Json.encode("Linux")) {
		obj.modify_attribute("vars.os", "OpenBSD")
	}
	obj.version = 1628682054.726195
}

julianbrost · 2021-08-11T11:55:34Z

I think before changing this PR, we should have a discussion how we actually expect and want modified attributes to behave.

Have we even agreed that this is the behavior we want?

Al2Klimov · 2021-08-11T11:57:45Z

No, I've just asked whether the obvious issues you pointed out are gone.

Al2Klimov · 2023-01-20T09:27:08Z

We don't want this, right?

julianbrost · 2023-01-20T14:14:56Z

This PR is the one trying to remove modified attributes based on detecting changes in config files, isn't it?

Then I'd say we close this, yes.

In general, I'd treat modified attributes as API-based attribute overrides, meaning that if you set an attribute via the API, that takes precedence until you clear it via the API (which we don't provide a proper mechanism for at the moment).

Detecting the intention to change an attribute via config files is hard to impossible, where it is an explicit action in the API. Consider the following example: you have an service that doesn't explicitly set enable_active_checks, so it's enabled by default. Now you disable active checks using the web interface and then add enable_active_checks = true to the config file. I think the intention here would be clear, this should re-enable active checks if we say that changing the config files overrides modified attributes. But actually detecting this would mean we'd have to distinguish default values from the config files from explicitly set ones. But then, if the service wasn't using the default value before, but rather used a template that already set the attribute to the default again, that change wouldn't be detected again.

So the approach of this PR will easily result in a mix of complex rules and strange edge-cases. On the other hand, API-set attribute wins until it's cleared via the API is a pretty simple and clear rule.

icinga-probot bot added this to the 2.13.0 milestone Apr 22, 2021

icinga-probot bot added area/api REST API area/configuration DSL, parser, compiler, error handling bug Something isn't working labels Apr 22, 2021

julianbrost self-requested a review June 21, 2021 15:17

julianbrost requested changes Jun 22, 2021

View reviewed changes

julianbrost removed this from the 2.13.0 milestone Jun 23, 2021

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

Al2Klimov requested a review from julianbrost August 10, 2021 18:45

Al2Klimov added 5 commits August 11, 2021 12:39

Introduce ConfigObject#GetAttribute()

2e944b1

refs #8717

Introduce ConfigObject#get_attribute()

89f782f

refs #8717

Introduce Internal.serialize()

a94efb5

refs #8717

ConfigObject::DumpModifiedAttributes(): pass original value to callba…

95be500

…ck as well refs #8717

IcingaApplication::DumpModifiedAttributes(): pass original value to P…

09244bc

…ersistModAttrHelper() as well refs #8717

Al2Klimov self-assigned this Aug 11, 2021

Al2Klimov removed the request for review from julianbrost August 11, 2021 10:40

Al2Klimov marked this pull request as draft August 11, 2021 10:40

PersistModAttrHelper(): only modify attribute if the object config di…

206a0fa

…dn't change refs #8717

Al2Klimov force-pushed the bugfix/mod-attr-8717 branch from 4688077 to 206a0fa Compare August 11, 2021 11:43

Al2Klimov marked this pull request as ready for review August 11, 2021 11:43

Al2Klimov requested a review from julianbrost August 11, 2021 11:43

Al2Klimov removed their assignment Aug 11, 2021

julianbrost removed their request for review August 11, 2021 12:01

Al2Klimov assigned lippserd Jan 20, 2023

Thomas-Gelf mentioned this pull request Feb 1, 2023

Assign where filter rendered as match("a*",x)||match("b*",x)||match("c*",x)||... Icinga/icingaweb2-module-director#2661

Closed

Al2Klimov mentioned this pull request May 5, 2023

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

Closed

julianbrost closed this May 23, 2023

icinga-probot bot deleted the bugfix/mod-attr-8717 branch May 23, 2023 14:40

julianbrost mentioned this pull request May 23, 2023

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Forget modified attribute on config change #8739

Forget modified attribute on config change #8739

Al2Klimov commented Apr 22, 2021 •

edited

Loading

Al2Klimov commented Apr 22, 2021

julianbrost left a comment

julianbrost Jun 22, 2021

Al2Klimov Aug 10, 2021

julianbrost Aug 11, 2021

julianbrost commented Jun 23, 2021

Al2Klimov commented Aug 4, 2021

Al2Klimov commented Aug 11, 2021

julianbrost commented Aug 11, 2021

Al2Klimov commented Aug 11, 2021

Al2Klimov commented Jan 20, 2023

julianbrost commented Jan 20, 2023

Forget modified attribute on config change #8739

Forget modified attribute on config change #8739

Conversation

Al2Klimov commented Apr 22, 2021 • edited Loading

Al2Klimov commented Apr 22, 2021

Before

After

julianbrost left a comment

Choose a reason for hiding this comment

julianbrost Jun 22, 2021

Choose a reason for hiding this comment

Al2Klimov Aug 10, 2021

Choose a reason for hiding this comment

julianbrost Aug 11, 2021

Choose a reason for hiding this comment

julianbrost commented Jun 23, 2021

Al2Klimov commented Aug 4, 2021

Al2Klimov commented Aug 11, 2021

julianbrost commented Aug 11, 2021

Al2Klimov commented Aug 11, 2021

Al2Klimov commented Jan 20, 2023

julianbrost commented Jan 20, 2023

Al2Klimov commented Apr 22, 2021 •

edited

Loading