nbering / terraform-provider-ansible

"Logical" provider for integrating with an Ansible Dynamic Inventory script.
https://nbering.github.io/terraform-provider-ansible/
Mozilla Public License 2.0
329 stars 64 forks source link

Merge priority property #23

Closed nbering closed 5 years ago

nbering commented 5 years ago

Related to https://github.com/nbering/terraform-inventory/issues/11.

Adds a priority property, which allows for overlapping variable sources that will take the variable from the correct source.

Resource Default
ansible_host 50
ansible_group 50
ansible_host_var 60
ansible_group_var 60

Since *_var resources are more explicit (setting only one variable), I assume most people would prefer they merge on top of the corresponding host or group by default. This behaviour can of course by overriden by setting the priority property of an *_var resource to a value lower than 50.

The defaults of 50 and 60 are completely arbitrary, but I figured most would use values somewhere between 0 and 100, so I put the defaults near the middle.

resource "ansible_host" "www" {
  inventory_hostname = "www.example.com"
  variable_priority  = 50  # Not really necessary, since this is the default.

  vars = {
    foo = "aaa"
    bar = "bbb"
  }
}

resource "ansible_host_var" "override" {
  inventory_hostname = "www.example.com"
  variable_priority  = 60 # Not really necessary, since this is the default.
  key                = "foo"
  value              = "ccc"
}

resource "ansible_host_var" "underride" {
  inventory_hostname = "www.example.com"
  variable_priority  = 40 # Lower than the default for a host resource
  key                = "bar"
  value              = "ddd"
}

resource "ansible_host_var" "extra" {
  inventory_hostname = "www.example.com"
  variable_priority  = 40 # Lower than the default for a host resource
  key                = "baz"
  value              = "eee"
}

The output host variables for www.example.com from the terraform-inventory script would be as follows:

foo: ccc  # from ansible_host_var.override
bar: bbb  # from ansible_host.www
baz: eee  # from ansible_host_var.extra

The ansible_host_var.underride resource has a lower merge priority than the ansible_host.www resource with the same inventory_hostname... and the variable key was set in ansible_host.www, so the value for foo in ansible_host_var.underride was not used in the output.

As a counter-example, he ansible_host_var.extra resource has a lower merge priority than the ansible_host.www resource with the same inventory_hostname... but the variable key was not set in ansible_host.www, so the value for baz in ansible_host_var.extra is used in the output.

jtopjian commented 5 years ago

Hi @nbering!

I have a few thoughts - feel free to ignore :)

How about variable_priority instead of priority? This might help prevent possible issues confusing it with any other kind of possible "priority".

Another possibility is to create a new kind of vars attribute (deprecating the old) that would be modeled something like:

resource "ansible_group" "group_1" {
  variables {
    priority = n

    data = {
      foo = bar
    }
  }
}

One other idea: How about making ansible_*_var and the ansible_group.vars mutually exclusive with the recommendation to begin using ansible_*_var. This would provide a single path of variable declaration.

nbering commented 5 years ago

@jtopjian variable_priority might be a better name for this, thanks.

As for making them mutually exclusive, that's actually counter to my motivation for adding the ansible_*_var resource types.

The idea came from having ansible_host inside a submodule, and wanting to append an extra variable from the root module.

jtopjian commented 5 years ago

The idea came from having ansible_host inside a submodule, and wanting to append an extra variable from the root module.

πŸ€”

Being able to append extra variables is definitely worthwhile.

Couldn't you rewrite the ansible_host resource without vars and then create new ansible_host_var resources in the module?

I think (I haven't tested this) that if you had ansible_host_var declared in a module and you declare ansible_host_var in the configuration consuming the module with the same key, that would be a conflict of IDs... so you wouldn't be able to overwrite variables.

And if that's true, then I see how you've come to needing ansible_group.vars and ansible_group_var. :)

nbering commented 5 years ago

I think (I haven't tested this) that if you had ansible_host_var declared in a module and you declare ansible_host_var in the configuration consuming the module with the same key, that would be a conflict of IDs... so you wouldn't be able to overwrite variables.

I'm actually surprised that I haven't encountered an ID conflict as yet. That's something that's been on my mind to go out of my way to produce and check what the result would be. I know Terraform generally assumes you've re-created the resource if the ID changes, but I'm not positive whether it throws an error if more than one instance has the same id.

nbering commented 5 years ago

@jtopjian I'm curious what your motivation is for suggesting the change to vars on host.

I'm kind of happy with how they are now, aside from #13 (which can be worked around with Jinja expressions). My personal inclination is to keep the system as flexible as possible. Any rules imposed on the system would seem to be almost entirely arbitrary, considering that the integration literally just moves some values around, and there's no real "API" underneath aside from that imposed by Ansible's dynamic inventory format, and Terraform's state file - which are both pretty flexible.

jtopjian commented 5 years ago

I'm actually surprised that I haven't encountered an ID conflict as yet.

I just tried this on a whim and it looks like you can have a resource in a module with the same ID as the resource in the configuration consuming the module.

# tree .
.
β”œβ”€β”€ main.tf
β”œβ”€β”€ module
β”‚Β Β  └── main.tf
└── terraform.tfstate

main.tf:

module "host_vars" {
  source = "./module"
}

resource "ansible_host_var" "foo" {
  inventory_hostname = "foo.example.com"
  key                = "foo"
  value              = "baz"
}

module/main.tf:

resource "ansible_host_var" "foo" {
  inventory_hostname = "foo.example.com"
  key                = "foo"
  value              = "bar"
}

I'm curious what your motivation is for suggesting the change to vars on host.

Which suggestion? The one detailed in "Another possibility is to create a new kind of vars attribute (deprecating the old) that would be modeled something like:"?

If so, that was only so you could have the word priority verbatim nested inside variable - it was just an alternative to calling the top-level attribute variable_priority.

Or the suggestion about ansible_host.vars / ansible_group.vars mutually exclusive to ansible_*_var? This was with the thinking that you could remove the prioritization system altogether - but I didn't consider the module use-case.

nbering commented 5 years ago

you could have the word priority verbatim nested inside variable - it was just an alternative to calling the top-level attribute variable_priority.

I'm going to avoid that because shredding dictionary structures in 0.11.x Terraform state is awful. I'm really glad the structure they were using is going away in 0.12.x, but I'm going to wait a bit before dropping 0.11.x - it took us a while to move from 0.10.x to 0.11.x because the major releases tend to be pretty buggy at first.

Or the suggestion about ansible_host.vars / ansiblegroup.vars mutually exclusive to ansible*_var? This was with the thinking that you could remove the prioritization system altogether - but I didn't consider the module use-case.

Yes, as mentioned, I did this primarily because of submodules. I can imagine some other scenarios, but it just felt like a natural solution. The only alternative I had considered was sorting by Terraform identifier.

nbering commented 5 years ago

By the way, thanks for your feedback, @jtopjian. I really appreciate the input.

jtopjian commented 5 years ago

Likewise - thank you for taking the time to discuss!