Add zones to Hostgroups and Servicegroups

[eurotux-tec]

In the scenario of our company there is an icinga master node and several satellites, each one is responsible for monitoring the infrastructure of a specific customer.

Currently, all hostgroups / servicegroups are replicated to all satellites, so all customers are aware of the other hostgroups / servigroups, which should be avoided.

This PR contains changes to the PHP code and DB schema to allow a zone to be assigned to a hostgroup / servicegroup, so that the hostgroup / servicegroup configuration file is only sent to the corresponding zone.

Do you think that this makes sense or can we do it in a different way? If you agree, please merge this. If you don't, please let me know how can I improve the patch so that is gets merged.

[cla-bot]

Thank you for your pull request. Before we can look at it, you'll need to sign a Contributor License Agreement (CLA).

Please follow instructions at https://icinga.com/company/contributor-agreement to sign the CLA.

After that, please reply here with a comment and we'll verify.

Contributors that have not signed yet: @dgmetux

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Please contact us if you think this is the case.

• If you signed the CLA as a corporation, your GitHub username may not have been submitted to us. Please reach out to the responsible person in your organization.

[cla-bot]

Thank you for your pull request. Before we can look at it, you'll need to sign a Contributor License Agreement (CLA).

Please follow instructions at https://icinga.com/company/contributor-agreement to sign the CLA.

After that, please reply here with a comment and we'll verify.

Contributors that have not signed yet: @araujorm

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Please contact us if you think this is the case.

• If you signed the CLA as a corporation, your GitHub username may not have been submitted to us. Please reach out to the responsible person in your organization.

[cla-bot]

Thank you for your pull request. Before we can look at it, you'll need to sign a Contributor License Agreement (CLA).

Please follow instructions at https://icinga.com/company/contributor-agreement to sign the CLA.

After that, please reply here with a comment and we'll verify.

Contributors that have not signed yet: @araujorm

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Please contact us if you think this is the case.

• If you signed the CLA as a corporation, your GitHub username may not have been submitted to us. Please reach out to the responsible person in your organization.

[eurotux-tec]

Good afternoon.

This patch previously had some extra code that added zones to baskets. That was a mistake and that has been cleaned up. Also it was missing the upgraded full database schemas (it only contained the migration SQL files), now it's fixed. Should be cleaned up.

Since this (being able to associate hostgroups and servicegroups to specific zones) is a useful feature that makes full sense, specially if you have different satellites for different organizations that should not mix, or leak information contained in group names (e.g. costumer names), can you please review this request and tell us if there is any reason for it not to be accepted?

Also, there seems to be some problem with the CLA signature, it seems it's not being recognized even though @araujorm has signed the CLA using both of his emails. Can someone from the Icinga team check what is missing or wrong with that?

[araujorm]

Hi. I've signed the CLA (with both of my emails) but it is not being recognized. Can someone force a re-check, or tell me if I'm doing something wrong? Also, can we have feedback about this patch, and get it merged if it's good?

[bobapple]

@cla-bot check

[Thomas-Gelf]

Hi @eurotux-tec,

thank you for this pull request. I think we definitively should support Zones for those objects. Any objection against unifying Service/Host/UserGroup code for Objects and Forms, at least as far as possible? I kind of dislike the duplicated code. Parts of it already used to be there, but now it get's even more.

In addition to that:

• There is still Basket-related code in the Zone class. Please do not hesitate to open a dedicated pull request, but remove it from this one
• prefersGlobalZone() is true in the base class, you can drop those methods, if "global" is what you want. For Host- and ServiceGroups, "global" is already the default
• What was your motivation for changing the default zone for UserGroups to "global"? I'd reject that part of the patch, as I see no good reason for doing so. Notifications usually take place only in the master zone, this is where your User objects normally reside. I'd try hard to place as few objects as possible into the global zone, especially since I've met a setup with 40 MBytes of User objects 🙈. Still, I'd love to understand your reasoning, as you probably structure your configuration different than I usually would do.

Thanks,
Thomas

[araujorm]

* What was your motivation for changing the default zone for UserGroups to "global"? I'd reject that part of the patch, as I see no good reason for doing so. Notifications usually take place only in the master zone, this is where your User objects normally reside. I'd try hard to place as few objects as possible into the global zone, especially since I've met a setup with 40 MBytes of User objects see_no_evil. Still, I'd love to understand your reasoning, as you probably structure your configuration different than I usually would do.

Some of this code was developed 3 or 4 years ago for internal using on specific use cases .Some of those implementations may not have followed the best possible practices at the time, as we were using director for the first time (although we've been working with Icinga since the times of Icinga 1, and adopted Icinga 2 shortly after it came out some years ago). As years went by, different people maintained those patches internally as a best effort, but some of the features were all mixed up in one big patch. After some growing pains, some nightmares to update every time a new director stable release came out, and realizing how important this module has become for us, we finally got some time to organize and separate the code and filter out some that was never needed in the first place. This is to say that I can't really tell you what the motivation was for the part you mentioned, but I suspect it may have its origin on some early misinterpretation on our part about how zones worked, but I'll still have to dig it up some more.

About the notifications: on our use cases, most of the time, they are sent by the satellites, so they are in other zones than master. Some background info on that follows.

Most of our satellites are on costumer premises, so that we can monitor their internal infrastructure. On each satellite, we usually setup a read-only icingaweb2 with the monitoring module for them to consult their own info, but on our master instance we can see all of our costumers organized by hostgroups and servicegroups so our teams can act from only one centralized place.

Some costumer IT teams require they have full access to each of their machines. We can't leak information on any of the satellites about the other costumers: neither other costumer names (hence our need to have the hostgroups associated to zones), neither the info about costumer user objects that are created to receive their own notifications and their respective email addresses. Relevant to this last part is the fact that each satellite has its own MTA (e.g postfix) configured specifically for that costumer according to their infrastructure (most times, the postfix instance is configured to use an authenticated SMTP relay with credentials provided by the costumer send the messages, but there are cases where the relay is done by other means, this part pretty much depends on each costumer).

Some of the notifications are sent to us, so that our support team can act, but others are specific to the costumer's IT teams and are directly sent to them. So the main reason most of the notifications are sent using the satellites is because if our master fails temporarily for some reason (connectivity or other), the notifications still must be sent, and we can't afford to lose them (on our side, we have a system that processes the incoming email notifications and warns our support team). The only notifications sent by our master server are the ones about itself (disk space, etc) and the connectivity to the satellites - for the satellites template, the host down test is not a ping test but a tcp check on port 5665 (BTW, if there is a way to replace that with a proper check if an endpoint is not connected, we'd love to improve on this part; as that's off-topic here please feel free to PM if you have some better approach).

I hope this info has helped to better understand our needs, and maybe it helps to give fresh ideas.

Thanks @Thomas-Gelf for your suggestions, we'll work on them and update the thread when we have new info.

[araujorm]

Hello.

Sorry for having had this stalled for so long, but I made the corrections that were asked, leaving only what is indeed relevant, adapted to be on par with the current master version (uuid property added, database version and so on), and refreshed the branch used for this pull request.

Can you review it, and if good, ponder about merging it? Anything that might be missing please tell.

Thanks for your work.

[araujorm]

Any objection against unifying Service/Host/UserGroup code for Objects and Forms, at least as far as possible? I kind of dislike the duplicated code. Parts of it already used to be there, but now it get's even more.

@Thomas-Gelf Had missed this part. Meanwhile, took a look a the code and tried to unify it.

We added a new commit to the pull request (in order to have it clear what was made to add support for zones on hostgroups and service groups, and what was made to unify the code afterwards).

For objects, moved some common parts to IcingaObjectGroup, where zone_id is now in defaultProperties and thus those don't need to be overriden in IcingaHostGroup and IcingaServiceGroup anymore. Similar for relations property, which don't need to be specified in descendant classes anymore.

For forms, created a intermediate class IcingaGroupForm (analog to the already existing IcingaObjectGroup for objects) which extends DirectorObjectForm, and made IcingaHostGroupForm, IcingaServiceGroupForm and IcingaUserGroupForm extend this new class instead. Moved the addZoneElements method to this new class (although it's not used by IcingaUserGroupForm), thus removing it from the descendant classes that had it repeated.

Was this what you had in mind, @Thomas-Gelf ? If you think this should be on a separate pull request, we can remove this new commit and make another pull request later after the zone support for hostgroups and servicegroups is merged. Please let us know, will await your feedback.

Edit: correction: the addZoneElements method is used by all the descendant classes, including IcingaUserGroupForm

[araujorm]

Made a correction on the last commit message, because the addZoneElements method is used by all the descendant classes, including IcingaUserGroupForm.

Hope this is what is intended, best regards.

[eurotux-tec]

Hello.

We've refreshed the proposed commits so that they merge with the current master.

Could we have any feedback on whether it's acceptable, or if there is any other approach you'd prefer instead?