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

[ansible_runner] Add NetworkCredential for Ansible::Runner lib #19007

Merged

Conversation

NickLaMuro
Copy link
Member

As part of this change, Ansible::Runner:MachineCredential as updated so that it won't overwrite any existing data in that it would have not overwritten in env/password if the file already exist. Data from MachineCredential is still favored over other sources (like NetworkCredential), but this allows ssh unlock passphrases that would be only defined in NetworkCredential to exist along side any other passwords from MachineCredentials.

@NickLaMuro
Copy link
Member Author

@miq-bot add_label changelog/yes, core, embedded ansible, enhancement

cc @carbonin @Fryguy

end

it "clobbers existing ssh key unlock keys" do
existing_data = { ssh_unlock_key => "hunter2" }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ummmm... shouldn't hunter2 be ******* ? Or do I only see it because it's my password, but you see *******? 😆

# Modeled off of awx codebase:
#
# https://github.com/ansible/awx/blob/1242ee2b/awx/main/tasks.py#L1432-L1443
#
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this comment belong in the tests? (expected in the code, but not the tests)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have had it in both in every other credential provider.

@Fryguy
Copy link
Member

Fryguy commented Jul 19, 2019

LGTM... @carbonin can you also review.

Copy link
Member

@carbonin carbonin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some minor style stuff from me. Looks good @NickLaMuro

With network credentials, awx favor's the MachineCredetial info over the
NetworkCredential data in the env/password file, so that file is loaded
if it exists already, and the data is appended to it only if there isn't
data already.
- Adds missing closing paren in comment
- Re-aligns assignments in .params_to_attributes
When creating a env/password file in MachineCredential, only inject new
values into the file if it already exists, but don't overwrite data that
wouldn't otherwise be re-written.

Example:  if an key for unlocking an encrypted SSH key exists, don't
delete that data while adding the other existing data.

A helper method, `.initialize_password_data` was added to the top level
`Ansible::Runner::Credential` class to share loading the existing data
or creating a new hash.
@NickLaMuro NickLaMuro force-pushed the ansible_runner_network_credential branch from 069b64a to bcdd800 Compare July 19, 2019 15:14
@miq-bot
Copy link
Member

miq-bot commented Jul 19, 2019

Some comments on commits NickLaMuro/manageiq@9f6b031~...bcdd800

lib/ansible/runner/credential/machine_credential.rb

  • ⚠️ - 32 - Detected pp. Remove all debugging statements.
  • ⚠️ - 33 - Detected pp. Remove all debugging statements.

spec/lib/ansible/runner/credential/machine_credential_spec.rb

  • ⚠️ - 115 - Detected pp. Remove all debugging statements.
  • ⚠️ - 116 - Detected pp. Remove all debugging statements.
  • ⚠️ - 129 - Detected pp. Remove all debugging statements.
  • ⚠️ - 130 - Detected pp. Remove all debugging statements.

spec/lib/ansible/runner/credential/network_credential_spec.rb

  • ⚠️ - 161 - Detected pp. Remove all debugging statements.
  • ⚠️ - 162 - Detected pp. Remove all debugging statements.
  • ⚠️ - 173 - Detected pp. Remove all debugging statements.
  • ⚠️ - 174 - Detected pp. Remove all debugging statements.

@miq-bot
Copy link
Member

miq-bot commented Jul 19, 2019

Checked commits NickLaMuro/manageiq@9f6b031~...bcdd800 with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
6 files checked, 0 offenses detected
Everything looks fine. 🍰

@NickLaMuro
Copy link
Member Author

@carbonin Changes applied.

@carbonin carbonin self-assigned this Jul 19, 2019
@carbonin carbonin merged commit 837761c into ManageIQ:master Jul 19, 2019
@carbonin carbonin added this to the Sprint 116 Ending Jul 22, 2019 milestone Jul 19, 2019
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.

4 participants