search

Icinga / icingaweb2-module-director

The Director aims to be your new favourite Icinga config deployment tool. Director is designed for those who want to automate their configuration deployment and those who want to grant their “point & click” users easy access to the configuration.
https://icinga.com/docs/director/latest
GNU General Public License v2.0
413 stars 203 forks source link

The 'Merge Policy' option on Sync rules has no/unexpected effects #2122

Open Linuxfabrik opened 4 years ago

Linuxfabrik commented 4 years ago

Expected Behavior

Automation > Sync rule > Properties: The 'Merge Policy' should do what the help text suggests: 'Whether you want to merge or replace the destination field. Makes no difference for strings'

Current Behavior

When using a Custom Variable as the Destination Field the destination is always replaced, no matter what the 'Merge Policy' is set to, even if both the Source Column and the Custom Variable are arrays.

Possible Solution

Either adjust the documentation and help texts to explain the current behavior, or preferably adjust the behavior so that one can actually choose the Merge Policy for every Property.

From looking at the source code, there seem to be three problems. The first one being that this line in the source code suggests, that the Merge Policy is only considered if the Destination Field is set to 'All custom variables (vars)', and defaults to the 'merge' Merge Policy. This is not represented in the GUI.

Secondly, this line means that the replaceVars (aka the Merge Policy) is set for each object, not for each property on that object (eg merge the custom variable A, but replace the custom variable B would result in replaceVars = true aka 'replace' for all variables on that object).

Lastly, there does not seem to be any merging of the Custom Variables when IcingaObject.merge() is called here.

Part of a fix for the last point can be found here, but it does not completely solve the issue. With the fix applied, all arrays in the Custom Variables are always merged - which still is not the behavior that the GUI suggests.

Steps to Reproduce (for bugs)

Your Environment

Thomas-Gelf commented 4 years ago

Hi @Linuxfabrik,

the problem is the Array data type. This has been implemented to merge nested dictionaries, and there it SHOULD work. At least at level 1, there is no deep merge and no deep deep merge :laughing: If it doesn't please let me know.

It will NOT work with Arrays, at least I cannot remember that such thing has ever been implemented. And this will become tricky. Arrays have a specific order, and sometimes order is very important for them. Imagine the following Hosts:

object Host "a" {
  vars.data = [
    "prio1",
    "prio2",
  ]
}

object Host "b" {
  vars.data = [
    "prio4",
  ]
}

Now the Sync provides [ "prio3" ] for both of them. How should the result look like? Should we always append? Should we re-sort them? As you mentioned Puppet, what bothers me is that this is not "idempotent". There is no way to get a deterministic behaviour when trying to accomplish this. Sure, this is something we could live with, I just wanted to raise my concerns.

Cheers, Thomas

NB: eventually, just to get a better understanding of your use-case: could you please share some details of the bigger picture? What are you trying to achieve? Are you filling something like a "monitored applications" Array from multiple source systems?

Linuxfabrik commented 4 years ago

Hi @Thomas-Gelf thanks for the fast response 🙂 Let me explain our situation first. We have a data field called tags on every host object. It is of the data type datalist, with the target data type array. Using these tags we apply our service sets to the hosts, for example our CentOS 7 Service Set to all hosts with the tag centos7.

Now we want certain tags to be set on top from the PuppetDB (for example the operating system, but also others), while still being able to manually add more tags, for example a tag for a certain application service set.

In our case the order of the elements does not matter, we just would like to merge the tags coming from PuppetDB and the tags added by the icinga admins. I see that there are different ways of handling this merge depending on ones requirements, but I'd argue to just allow the user to decide which merge strategy they require for that property.

Considering idempotency: I do not see the problem when merging (assuming duplicate entries are forbidden / removed during the merge). The only issue would be that elements which were previously added through the sync will not be removed when removed in the source. If that is a requirement, one still has the 'replace' option.

Can you explain in which scenario it should work using arrays? I did not quite get that part.

Have a nice day

dgoetz commented 3 years ago

Having the same problem at one customer that we would need merge of an array, I can give you our example as use case.

There are two tables, a generic one and an application specific (or in reality multiple but only one for each case) we get a list of applications from. The list should be in the end one custom variable of type array, so it can be used with rules like "pacemaker" in host.vars.applications. So for this use case order would not matter.

And for idempotence it would make sense if the values coming from the "main" sync are the first and from the update only sync are simply added in order of creation (so the id of the sync rule). Or if it would make implementation easier, it could also be changed to sync attributes from all imports in one sync rule, but having them separate would be preferred as it gives the better overview in our case.

Thomas-Gelf commented 2 years ago

I played around with this, my conclusion:

I pinned it to v1.10.1, but it might be moved to v1.11 later on - as this could be considered a breaking change. Will now be implemented, then we'll see.