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

Add vars to add .ssh/config and /etc/hosts only optionally #74

Closed
wants to merge 2 commits into from

Conversation

Enucatl
Copy link

@Enucatl Enucatl commented Oct 17, 2023

Fix #73

add options to remove tasks as well

Not adding the key to known_hosts would make the cloud-init check fail as it's asking for the key to be verified.

Could disable strict host checking in ssh, but for now I'll leave it as it is.

Fix csmart#73

add options to remove tasks as well

Not adding the key to known_hosts would make the cloud-init check fail
as it's asking for the key to be verified.

Could disable strict host checking in ssh, but for now I'll leave it as
it is.
Copy link
Owner

@csmart csmart left a comment

Choose a reason for hiding this comment

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

@Enucatl thanks for the contribution! I've made a couple of comments for your consideration.

tasks/hosts-add.yml Outdated Show resolved Hide resolved
tasks/hosts-remove.yml Outdated Show resolved Hide resolved
@@ -14,6 +14,7 @@
become: true
delegate_to: "{{ kvmhost }}"
when:
- virt_infra_add_etc_hosts
Copy link
Owner

Choose a reason for hiding this comment

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

@Enucatl similar to ~/.ssh/config, I guess technically it doesn't hurt to have this task always run, otherwise if the entries happened to be in /etc/hosts already and then the user sets virt_infra_add_ssh_config: false they will never get cleaned up on host remove... Or we can just assume users need to go and clean up these files if they remove a VM and have set virt_infra_add_ssh_config: false. It is nice to run less tasks than necessary... What do you think?

Copy link
Author

@Enucatl Enucatl Oct 18, 2023

Choose a reason for hiding this comment

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

In my case I started this because I have ~/.ssh/config mounted in a way the doesn't allow root to edit it (NFS mount with kerberos), so having it run removing a section that doesn't exist (hence doing nothing) would error out.

I thought exactly about the case you mentioned and I considered:

  1. keeping it as in my change would force the user to keep the flag true before the running with undefined to remove that VM. I convinced myself that it's not too bad. After that they can move onto false with no further issues (...for that VM)
  2. We could have separate flags for add and remove tasks, so that there is no ambiguity at all. I felt that this was kind of overcomplicating a simple thing. But maybe it works better, especially in large and complicated environments.

@Enucatl
Copy link
Author

Enucatl commented Oct 20, 2023

I'll close it since I want to make more changes to virt-customize to allow arbitrary flags.
Feel free to cherry-pick these commits in case.

@Enucatl Enucatl closed this Oct 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Do not create entries in .ssh/config and /etc/hosts
2 participants