Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add apply_prefix argument to service object to fix issue #227 #228

Closed
wants to merge 1 commit into from
Closed

Add apply_prefix argument to service object to fix issue #227 #228

wants to merge 1 commit into from

Conversation

fredrik-eriksson
Copy link

Hi,

Although I personally would prefer that the default should be to add the prefix to all apply loops I understand that some may think otherwise. Also, that would be a breaking change.

With this commit I simply add an apply_prefix argument that allows you to optionally add a service prefix to be able to add services that otherwise would result in name collisions. By default apply_prefix is set to undef to preserve the current module behavior.

@bobapple
Copy link
Contributor

The same can be achieved with a syntax like:

apply Service for (interface_name => interface_config in host.vars.interfaces) {
  import "generic-service"
  check_command = "iftraffic"
  display_name = "IF-" + interface_name

  [...]
}

What are the benefits of using the apply_prefix instead?

@fredrik-eriksson
Copy link
Author

No, setting display_name is not the same thing. As I described in issue #227, without the prefix you cannot have multiple services looping over the same dict. Consider a host where the dict vars.network.interfaces look like this:

vars.network.interfaces = {
  lo0 = {
    address = "127.0.0.1"
    addressv6 = "::1"
  }
}

Then try to apply the following services:

apply Service for (interface => opts in host.vars.network.interfaces) to Host {
  vars.ping_address = "opts.address"
  vars.ping_cpl = getvar(host.vars.ping_cpl, 40) 
  check_command = "ping4"
  display_name = "ping4_interface_" + interface
  assign where "opts.address"
}

apply Service for (interface => opts in host.vars.network.interfaces) to Host {
  vars.ping_address = "opts.addressv6"
  vars.ping_cpl = getvar(host.vars.ping_cpl, 40) 
  check_command = "ping6"
  display_name = "ping6_interface_" + interface
  assign where "opts.addressv6"
}

A configtest will then fail with this message:

critical/config: Error: An object with type 'Service' and name 'localhost!lo0' already exists (in /usr/local/etc/icinga2/conf.d/services_ping.conf: 21:1-21:77), new declaration: in /usr/local/etc/icinga2/conf.d/services_ping.conf: 53:1-53:77

It looks to me that icinga2 converts all apply-rules to "real" objects; but those objects still needs to have unique names. If more than one service loops over the interface list on a host (as here with ipv4 and ipv6 services) both services will get the interface name as service name, causing a naming collision. Setting display_name does not solve that issue.

@dnsmichi
Copy link

I think this will add an enhancement to the current apply logic. I'm a heavy user of that functionality when writing apply-for rules. A definitive 👍 from me :)

@lbetz lbetz self-requested a review March 2, 2017 17:34
@lbetz lbetz self-assigned this Mar 2, 2017
@lbetz lbetz added bug and removed bug labels Mar 2, 2017
@lbetz
Copy link
Contributor

lbetz commented Mar 2, 2017

I did it on a different way, maybe you like it too.

Branch: enhancement/Add-apply_prefix-argument-to-service-object-to-fix-issue-228

Use parameter prefix (boolean) to get the object_name as prefix.

@fredrik-eriksson
Copy link
Author

Works for me :)

@geor-g
Copy link

geor-g commented Mar 9, 2017

Could this be merged?

@lbetz lbetz closed this Mar 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants