saltstack-formulas / keepalived-formula

http://docs.saltstack.com/en/latest/topics/development/conventions/formulas.html
Apache License 2.0
12 stars 42 forks source link

Tracking scripts doesn't work because pillar data i written alphabetically into config #12

Closed kense2660 closed 5 years ago

kense2660 commented 7 years ago

I've noticed when defining pillar data, the key values are ordered alphabetically into the keepalived config file. This makes an issue when using tracking scripts which must be defined before the instance in the keepalived config.

Example:

Pillar data configuration:

include:
  - keepalived

keepalived:
  vrrp_script check_apache:
    script: '"killall -0 apache"'
    interval: 2
    weight: 10

  vrrp_instance:
    VI_1:
      state: MASTER
      interface: eth0
      virtual_router_id: 250
      priority: 110
      advert_int: 1
      virtual_ipaddress:
        - 10.10.10.10
      unicast_src_ip: 10.10.10.11
      unicast_peer:
        - 10.10.10.12
      track_script:
        - check_apache

... will be written like this into the keepalived config:

vrrp_instance {
  VI_1 {
    advert_int      1
    interface      eth0
    priority      110
    state      MASTER
    track_script {
      check_apache
    }
    unicast_peer {
     10.10.10.10
    }
    unicast_src_ip       10.10.10.11
    virtual_ipaddress {
       10.10.10.12
    }
    virtual_router_id      250
  }
}
vrrp_script check_apache {
  interval  2
  script    "killall -0 apache"
  weight    10
}

Is it possible to sort the key values by order and not alphabetically?

jebas commented 7 years ago

You can’t do that hash, but if you change it to a list, it should work.

Example:

  vrrp_instance:
    VI_1:
      - state: MASTER
      - interface: eth0
      - virtual_router_id: 250
      - priority: 110
      - advert_int: 1

Since python will not retrieve hash keys in a guaranteed order, they are placed alphabetically so that configuration file will remain the same each time the saltstack is run. However, a list will always be in that order it was created.

kense2660 commented 7 years ago

Thank you for the answer but this will only manipulate the sub keys. Isn't it possible to do the same with the main key values vrrp_instance and vrrp_script?

Keepalived needs vrrp_script to be defined before the vrrp_instance

jebas commented 7 years ago

Now I really wish I had written out the unit tests for this macro. Hindsight always being 20/20.

Try changing line 54 of the keepalived.jinja from: {{- config_entries(entry, indents) -}} to {{- config_entries(entry, indents, carryover) -}} and see if that produces the results you want.

The marco is designed to be recursive, so it looks at the type of information handed to it, and determines if it is a string, number, null, map, and then defaults to array. Originally the array wasn't passing the carryover information, so groupings had to be hashes. With the above change you should be able to do the following:

vrrp_instances:

Please let me know if this works.

Question?

Outside of cosmetic, is this creating an issue in keepalived?

kense2660 commented 7 years ago

That didn't help :(

Yes, it's causing an issue in keepalived. Keepalived will simply ignore your tracking scripts if they aren't defined before you refer to them in your configuration.

What I really need, is to get the following output from the pillar data configuration written in the beginning:

vrrp_script check_apache {
  [...]
}

vrrp_instance VI_1 {
  [...]
}

But instead I'll get this though the vrrp_script is defined before the vrrp_instance in the pillar:

vrrp_instance VI_1 {
  [...]
}

vrrp_script check_apache {
  [...]
}
jebas commented 7 years ago

I understand the issue, and I am working on it.

jebas commented 7 years ago

I just submitted pull request #13 which added config testing and does allow vrrp_scripts to come before vrrp_instances if they are placed in a list. Let me know if that works for you.

jebas commented 7 years ago

I've killed pull request #13 and replaced it with pull request #14. Both resolved the issue, however #14 creates a human readable configuration file where #13 did not.

kense2660 commented 7 years ago

Thank you! It's working now :)

kungfoome commented 7 years ago

Is this still an issue? I'm having the same issues myself

jebas commented 7 years ago

They merged my changes back in January, so it should not be an issue now. Just remember that when order is important, change the hash to a list. Hashes are alphabetically arranged; where lists keep their original order of entry.

jebas commented 7 years ago

Hey, @whiteinge. Could you have someone do some maintenance on this repo? You have two pull requests to fix the same issue, and two issues that should probably be closed.

whiteinge commented 7 years ago

Thanks for the ping! Merged.

bigbosst commented 7 years ago

Hey, FYI, I'm on the current latest master ( ad8e0f6a285634164f06aae07b37dd3992929a40 ) and they are still creating in alphabetical order. Scripts are still being ignored.

kjkeane commented 5 years ago

I can confirm this issue still exists, and after some testing it seems to be the dictsort on line 17 keepalived/templates/config.jinja

{%- macro keepalived_config(data, carryover='', recurse=-1, indent=0) -%} 
  {%- set recurse = recurse + 1 -%} 
  {%- if data is none -%} 
    {{- '\n' -}} 
  {%- elif data is string or data is number -%} 
    {{- data|string|indent(indent, True) }}{{ '\n' -}} 
  {%- else -%} 
    {%- if recurse > 0 -%} 
      {{- '{\n' -}} 
      {%- set indent = indent + 2 -%} 
    {%- endif -%} 
    {%- if data is mapping -%} 
      {%- for item in data|dictsort -%} 

output

global_defs {
  notification_email root@example.com
  notification_email_from root@example.com
  smtp_connect_timeout 30
  smtp_server mail.example.com
}
vrrp_instance VI_1 {
  interface eth0
  priority 100
  state MASTER
  virtual_ipaddresses {
    10.0.0.2
  }
  virtual_router_id 51
}
vrrp_script chk_haproxy {
  interval 2
  script killall -0 haproxy
  weight 2
}

After changing this to

    {%- if data is mapping -%} 
      {%- for item in data|dictsort(reverse=True) -%}

The output of the configuration was

vrrp_script chk_haproxy {
  weight 2
  script killall -0 haproxy
  interval 2
}
vrrp_instance VI_1 {
  virtual_router_id 51
  virtual_ipaddresses {
    10.0.0.2
  }
  state MASTER
  priority 100
  interface eth0
}
global_defs {
  smtp_server mail.example.com
  smtp_connect_timeout 30
  notification_email_from root@example.com
  notification_email root@example.com
}

Not sure what the best solution would to be here, since vrrp_scripts needs to be created before it can be called within the vrrp_instance

kjkeane commented 5 years ago

So this isn't really an issue if you follow what was mentioned above. Change it to be a list as mentioned above to fix the issue.

keepalived:
  global_defs:
    smtp_server: localhost
  - vrrp_script:
      script: '"/usr/bin/kill -0 haproxy"'
  - vrrp_instance:
      VI_1:
        virtual_ipaddress:
          - 10.0.0.2
myii commented 2 years ago

For future reference.

When using a list in the pillar, such as:

keepalived:
  config: 
    - global_defs:
        - enable_script_security: true
        - vrrp_version: 3
    - vrrp_script:
        chk_haproxy:
          script: '"/usr/bin/killall -0 haproxy"'
          interval: 2
          weight: 2
    - vrrp_sync_group:
        VI_1:
          group:
            - VI_1_IPv4
            - VI_1_IPv6
          track_script:
            - chk_haproxy
        VI_2:
          group:
            - VI_2_IPv4
            - VI_2_IPv6
          track_script:
            - chk_haproxy
    - vrrp_instance:
        ...

You can take advantage of the template override feature provided in this formula. Just make the following template available at salt://keepalived/files/default/keepalived.conf.tmpl:

########################################################################
# File managed by Salt at <{{ source }}>.
# Your changes will be overwritten.
########################################################################

{%- from "keepalived/macro.jinja" import print_config %}
{%- for section in config %}
{{ print_config(section) }}
{%- endfor %}

You can leave the default template provided by the formula at salt://keepalived/files/default/keepalived.conf.tmpl.jinja. The order of the templates is set here: https://github.com/saltstack-formulas/keepalived-formula/blob/c528cdae93edabfd7d9aecc8de7f74563dbd39d7/keepalived/config/file.sls#L21-L22 -- keepalived.conf.tmpl takes precedence over keepalived.conf.tmpl.jinja.