-
Notifications
You must be signed in to change notification settings - Fork 5.5k
[RFC] Terraform roster module to be used with terraform-provider-salt #48873
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, just one thing that I would like to see fixed, and another that is a nitpick.
salt/roster/flat.py
Outdated
@@ -47,19 +47,20 @@ def targets(tgt, tgt_type='glob', **kwargs): | |||
conditioned_raw = {} | |||
for minion in raw: | |||
conditioned_raw[six.text_type(minion)] = salt.config.apply_sdb(raw[minion]) | |||
rmatcher = RosterMatcher(conditioned_raw, tgt, tgt_type, 'ipv4') | |||
rmatcher = RosterMatcher(conditioned_raw, tgt, tgt_type, 'ipv4', opts=__opts__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed, it is just passing opts to a class in the file that should already have access to opts?
I don't see what this gives us.
salt/roster/terraform.py
Outdated
import json | ||
import os.path | ||
import salt.utils | ||
from salt.roster.flat import RosterMatcher |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, ok, so what i think we should do here instead is move RosterMatcher to __utils__
and and write a function that interacts with the RosterMatcher, and just returns rmatcher.targets()
Instead of having to pass __opts__
around. This way, __opts__
will already be available in the __utils__
modules.
Utils is even already packed inside of rosters, for Fluorine.
https://github.com/saltstack/salt/blob/develop/salt/loader.py#L475
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will refactor this in a next PR.
salt/roster/terraform.py
Outdated
''' | ||
ret = {} | ||
with salt.utils.files.fopen(state_file_path, 'r') as fh_: | ||
tfstate = json.load(fh_) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should use salt.utils.json.load
Before moving onto fixing the issues in the roster code, I would focus on the RFC. I would like some feedback on the 2nd alternative. Because if we instead of merging the roster module add a more generic script roster module, then for terraform one would only need the terraform salt provider, and the roster generic script module can be reused for other integrations. In summary, instead of |
@terminalmage @cachedout @rallytime any thoughs on design here? |
@dmacvicar whoa, I find this very nice idea (the main one) and the right step towards resources [re]use! While the idea as it is now is already great and can be left as is, but looking at this from mere user perspective, I find they likely would be even more happy to just call
Maybe we should also teach Salt to read Terraform state to let Salt know where it is. I am really excited where it goes! 👍 👍 |
I think this is true for Salt users. I doubt Terraform users want to change the way they describe resources and that is completely out of the scope of this PR. It belongs to In any case, it is not a simple problem. Terraform graph and Salt states/sls are different. Terraform automatically figures dependencies between resources as you use interpolation between them, unlike Salt For sake of moving the discussion I beg to keep this topic out. We don't want to saltify terraform. We want to let terraform users use Salt. Everything else is an effort that can be done independently of this PR and should be tackled by someone having a) deep knowledge about limitations of both Terraform and Salt b) some say in the future of |
Something that may be a nice addition, but unrelated to this PR would be a companion To give an example: meta-salt. Create a salt cluster with salt. I use terraform to bring up 4 VMs. I use salt-ssh to configure them, 1 as a salt-master and 3 as a salt-minion. I need salt-ssh to know the ip of the VM that was configured as master to change |
Yes, agreed. As I said, current PR perfectly go in as-is and to me current design makes lots of sense. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some small requests. :)
salt/roster/terraform.py
Outdated
''' | ||
from __future__ import absolute_import, unicode_literals | ||
import logging | ||
import salt.utils.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we break these imports up into 2 sections? We usually do something like:
# Import Python libs
from __future__ import absolute_import, unicode_literals
import logging
import os.path
# Import Salt libs
import salt.utils.json
etc.
That makes the import list easier to maintain moving forward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
salt/roster/terraform.py
Outdated
import logging | ||
import salt.utils.json | ||
import os.path | ||
import salt.utils |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than importing salt.utils
can you import the individual files directly? Otherwise you'll see deprecation notices as the big utils init file was broken up in 2018.3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
c9aef2f
to
38df2cd
Compare
Rebased |
salt/roster/terraform.py
Outdated
# Import Salt libs | ||
import salt.utils.files | ||
import salt.utils.json | ||
from salt.roster.flat import RosterMatcher |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move this to __utils__
and then call it from a __utils__
function, like roster.targets()
which calls RosterMatcher, instead of having to pass in __opts__
?
Overall, i really like this approach, i would just rather see the RosterMatcher stuff moved to utils, instead of importing from another roster, and having to pass opts around. |
@gtmanfred I am trying to get this refactored, but after moving it to utils, if I call it using |
if you remove
|
Installing to check this. |
With your patch, i get this
Adding
And it works
|
I fixed the spurious namespace when using I still get undefined
Which confuses me. Other util modules, like __opts__ = salt.config.minion_config('/etc/salt/minion')
__salt__ = salt.loader.minion_mods(__opts__)
global __salt__
if not __salt__:
__salt__ = salt.loader.minion_mods(__opts__)
This error happens both in the testcase and runtime. |
Adding __opts__ = salt.config.minion_config('/etc/salt/minion') in |
switch it so you use Try this diff https://gist.github.com/gtmanfred/854724d07ae240005f326a52bdb62172 |
I think I already mentioned it above, but if I do that, I get:
It is working on runtime (invoking |
Ahh, yeah, it should just be something with Check out the boto module tests, they use that a lot. |
…1: blank line at end of file
…ile_template imported from salt.template
…ed TESTS_DIR imported from tests.support.paths
Done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
So, what do I need to do now? :-) |
Just waiting on reviews, I moved this over to the fluorine branch to make sure it gets in. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather this be reviewed by someone familiar with Terraform, as I know nothing about it.
@dmacvicar I have tested this out using the examples from your GH repo using libvirt. I really like the way you implemented this. I am working on a final few items here and I will be done with my review. One thing that may be kind of cool is using the example code that you have in your GH repo is great, although a working example in a Docker container might also be nice. This would allow people to see how this is all wired up in case they have issues with any of the steps listed. Overall this is a very nice feature to Salt and thank you for making it. I should be ready to give the thumbs up tomorrow. |
Thanks for the feedback @dubb-b . As the roster plugin needs the provider counter-part, it will be quite important to make sure people can use it. I am already providing builds of the code in my repo, and once the Salt part is released I will consider doing stable/tagged releases of it. The installation should be as as easy as copying the builds to The only thing that bothers me is that the official roster module depends on a very specific provider that for now is unreleased and hosted in my home project, however:
I think I could also create some kind of diagram and add it to the documentation later. It would also help understanding all the wiring, as you called it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am finished with the testing, it is working quite well. @rallytime 👍
@dmacvicar There are a few issues identified by CodeClimate. It's not required that they be fixed but could you please take a look and see if any of them seem reasonable to you? |
I have reviewed those. I think all of the issues validate my view on why I don't use or like CodeClimate. They are false alarms and the code is fine. |
awesome 👍 i can't wait for using it 🌞 |
Summary
This is a proposal for a roster module to be used with terraform. It solves the use-case of a self-contained folder in git with infrastructure defined with terraform, where one wants
to provision the infrastructure using salt-ssh. The idea is that the salt-ssh roster is automatically filled with terraform resources.
Motivation
The goal is very simple: self-contained directories with infrastructure created by terraform and configuration using salt-ssh. Example here.
Some time ago I proposed a solution for this, which never got merged (for different reasons).
While I did not know about it at the time, the design was very similar to terraform-inventory module for Ansible, and it suffers from the same problems.
It worked by lookin at terraform state file, and recognizing certain types of resources, like libvirt hosts and AWS instances, then doing things like looking at the public_ip and creating a roster based on that.
However, when I got feedback, the approach drawbacks were made evident:
These drawbacks affected the motivation to get it merged. However it let me thinking on the problem, until I found terraform-ansible-provider and read the inspirational post about it.
I believe this is a much better approach for a terraform/salt-ssh integration roster module.
Design
The design, when compared to the original proposal, is to make the salt side (terraform.py) very agnostic. It does not know about all terraform resource types like AWS or Azure, but only one type of resource:
salt_host
.the
salt_host
resource is provided by another provider: terraform-provider-salt. It is just a logical resource. It does not do much than gluing terraform resources to the roster:So for a libvirt resource that is instantiated two times:
One could create one
salt_host
resource per libvirt domain, and bind the roster host to the first ip address:As you can see, there are no limits on what you can glue into the roster. You have absolute freedom on how many hosts to expose to Salt, and what address to use in there.
This means
terraform.py
is much simpler. It scans thesalt_host
resources from the state, and collects the properties, which are 1:1 mapped to the roster attributes.The roster module would not need to be updated as terraform adds features and resource types.
Alternatives
A roster module that does not require a terraform provider. It was described in the motivation and my previous PR. With the drawbacks described above: need to know about specific terraform resources.
Another idea/alternative is to keep the terraform provider side but alter the
terraform.py
included in this script and make it a more generic "script" roster module. So that if the module finds asalt-ssh.roster
executable, or it is mentioned in the Saltfile, it gets the roster from executing that script. In this case, the terraform-go-provider could implement the script and provide itself the roster (however this may require some knowledge of how to abuse a terraform provider executable to perform another task when called with a different option, and to guess where it is, as providers are not in PATH but in terraform.plugins.d).In any case this is an improvement over this RFC, and could be done in a second iteration/step.
Update: this could be more feasible than I thought: https://twitter.com/dmacvicar/status/1024800245325213697 opinions?
Unresolved questions
terraform-provider-salt
.Drawbacks
The disadvantage is that the roster module needs and depends on a specific salt provider on the terraform side. But this provider (prototype) is very simple, as it is pure logical, and will not need much changes if the roster architecture stays the same.