Add credential handling to Ansible::Runner #18968
Conversation
|
Also this doesn't handle the password protected ssh key case (the |
There was a problem hiding this comment.
Here are some initial comments I have. Probably still have a few bits I haven't looked over, but @Fryguy has already approved it and I don't know that these comments are worth holding up a merge.
Can take a look again later today.
|
|
||
| command_line.merge!(cred.command_line) | ||
| env_vars.merge!(cred.env_vars) | ||
| extra_vars.merge!(cred.extra_vars) |
There was a problem hiding this comment.
So seems like there could be some clobbering of values and is not obvious to the user here. Don't have a better solution in mind or think it should hold up the review, but just bringing it up as a topic for discussion for another time.
There was a problem hiding this comment.
Yeah, but I think it would be pretty rare. Plus I would thing the values from the credential should be the highest priority.
There was a problem hiding this comment.
But maybe we should check for conflicts and log?
There was a problem hiding this comment.
Yeah, but I think it would be pretty rare.
I agree with this, for what it is worth. Having multiple credential types being used all at once would be rare I think, but...
Plus I would thing the values from the credential should be the highest priority.
The credentials.each do |id| above this implies that there could be multiple credentials of the same type, and when one overrides the other, it would not be clear who would win out to the user (in practice, it is the last one in the list, of course, but that depends on how they get passed in).
But maybe we should check for conflicts and log?
Yeah, I think this is probably the most pragmatic approach for now. I would rather comment and warn about it when we see conflicts, but assume it won't usually be a problem for now instead of trying to over-engineer a solution for a problem that might not be there.
| end | ||
|
|
||
| it "doesn't send :user if userid is not set" do | ||
| auth.update!(:userid => nil) |
There was a problem hiding this comment.
This will only work if we don't reference cred before we make the authentication change because of the lazy evaluation of let.
I wasn't sure if this made the test too brittle for a slightly more DRY example. I can change this so we initialize the credential in every spec if this is too much magic.
There was a problem hiding this comment.
@NickLaMuro thoughts here?
There was a problem hiding this comment.
I can change this so we initialize the credential in every spec if this is too much magic.
If you are going to be making other changes, I would say go ahead and do that (I think it would just be changing it to a let!(:cred) instead. Possibly add a comment right above it saying "this is done on purpose since some specs rely on this being mutated.
Alternatively, for that spec specifically, you could just call cred right above it with a comment saying "this is done here intentionally". Then all of the other specs can take advantage of the lazy eval, and the one case where it needs to be preloaded, we just do it in the spec manually.
carbonin
force-pushed
the
add_credentials_to_runner
branch
from
4227190 to
37c48dd
Compare
This creates a null-object-like class which will return a subclass based on the type of credential id passed to it. Each subclass will implement methods which will augment the environment that ansible-runner executes in. Specifically they may return a hash representing environment variables, extra vars, command line arguments (to ansible-playbook) and they may write out the password and ssh key files. This commit implements a MachineCredential subclass as a start
carbonin
force-pushed
the
add_credentials_to_runner
branch
from
37c48dd to
22686d2
Compare
|
Some comments on commit carbonin@22686d2 lib/ansible/runner/credential/machine_credential.rb
spec/lib/ansible/runner/credential/machine_credential_spec.rb
|
|
Checked commit carbonin@22686d2 with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0 |
| @auth = Authentication.find(authentication_id) | ||
| @base_dir = base_dir | ||
|
|
||
| FileUtils.mkdir_p(env_dir) |
There was a problem hiding this comment.
Make sure this isn't world readable.
There was a problem hiding this comment.
I think the base dir is created as 0700 based on https://ruby-doc.org/stdlib-2.5.3/libdoc/tmpdir/rdoc/Dir.html#method-c-mktmpdir so I think we're good there.
There was a problem hiding this comment.
Yay!
(as if I didn't know this already because we were just chatting about this in person 😉 )
This creates a null-object-like class which will return a subclass
based on the type of credential id passed to it.
Each subclass will implement methods which will augment the environment
that ansible-runner executes in. Specifically they may return a hash
representing environment variables, extra vars, command line arguments
(to ansible-playbook) and they may write out the password and ssh key
files.
This commit implements a MachineCredential subclass as a start
@NickLaMuro this should allow us to move forward on new credential types in parallel. I'm also working on a followup to push the credential ids down from the service to runner.