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

feat: Set ansible_connection to winrm for Windows hosts #75

Merged
merged 2 commits into from
Apr 16, 2024

Conversation

0xFelix
Copy link
Member

@0xFelix 0xFelix commented Apr 5, 2024

What this PR does / why we need it:

This changes the inventory plugin so that it sets the ansible_connection to winrm if it detected a Windows host. If it did not detect a Windows host the ansible_connection is no longer set, so Ansible falls back to its default value of ssh.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #33

Special notes for your reviewer:

Release note:

Set ansible_connection to winrm for Windows hosts

@kubevirt-bot kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Apr 5, 2024
@0xFelix 0xFelix added the bugfixes Fixes that resolve issues. SHOULD not be used for minor enhancements label Apr 5, 2024
Copy link
Contributor

@jcanocan jcanocan left a comment

Choose a reason for hiding this comment

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

Thanks! Nice effort. Just a couple of silly suggestions.

@staticmethod
def is_windows(guest_os_info: Dict, annotations: Dict) -> bool:
if (id := guest_os_info.get("id")) is not None:
return id == "mswindows"
Copy link
Contributor

Choose a reason for hiding this comment

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

wdyt about making the id mswindows a const?

Copy link
Member Author

Choose a reason for hiding this comment

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

Created a constant for it.

ANNOTATION_KUBEVIRT_IO_CLUSTER_PREFERENCE_NAME
)
) is not None:
return cluster_preference.startswith("windows")
Copy link
Contributor

Choose a reason for hiding this comment

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

create the var windows = "windows" to avoid repeating the same string?

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'd like to stick to the way it is. IMO it fits the rest of the code style in the file.

Copy link
Contributor

Choose a reason for hiding this comment

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

All right!

@0xFelix 0xFelix force-pushed the winrm branch 2 times, most recently from 8f16319 to 92811df Compare April 5, 2024 14:25
@jcanocan
Copy link
Contributor

jcanocan commented Apr 5, 2024

Looks great IMHO. Thanks a lot
/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 5, 2024
@0xFelix
Copy link
Member Author

0xFelix commented Apr 9, 2024

/hold Waiting for more feedback from Ansible team

@kubevirt-bot kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 9, 2024
plugins/inventory/kubevirt.py Outdated Show resolved Hide resolved
plugins/inventory/kubevirt.py Show resolved Hide resolved
@kubevirt-bot kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 9, 2024
@0xFelix 0xFelix force-pushed the winrm branch 2 times, most recently from 7720613 to 31f1f8a Compare April 9, 2024 09:23
Copy link
Member

@lyarwood lyarwood left a comment

Choose a reason for hiding this comment

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

/approve

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 9, 2024
@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lyarwood

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 9, 2024
@kubevirt-bot kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 11, 2024
Copy link

@sabre1041 sabre1041 left a comment

Choose a reason for hiding this comment

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

Added small comment related to connection details for Windows machines

# Set up the connection
service = None
if self.is_windows(vmi_guest_os_info, vmi_annotations):
self.inventory.set_variable(vmi_name, "ansible_connection", "winrm")

Choose a reason for hiding this comment

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

Should we want to hardcode winrm as the connection for all Windows instances? Windows does support SSH and it might be ideal to provide a way to override. Maybe based on annotation? What about other connection options that we might want to inject for the specific Virtual Machine?

Copy link
Member Author

Choose a reason for hiding this comment

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

In the next released version with the Constructable feature fixed (see #77), you can do it with the compose key in the inventory plugin's configuration.

Example inventory configuration:

---
plugin: kubevirt.core.kubevirt

results in

ansible-inventory -i inventory.kubevirt.yml --list | jq -r '.["_meta"]["hostvars"]["default-testvm"]["ansible_connection"]'
ssh

Adding compose:

---
plugin: kubevirt.core.kubevirt
compose:
  ansible_connection: "'something'"

results in

ansible-inventory -i inventory.kubevirt.yml --list | jq -r '.["_meta"]["hostvars"]["default-testvm"]["ansible_connection"]'
something

Copy link
Member Author

@0xFelix 0xFelix Apr 16, 2024

Choose a reason for hiding this comment

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

compose takes a Jinja template expression, so you can get quite creative. It should be able to override the value only for specific hosts too.

Choose a reason for hiding this comment

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

That sounds great! Thanks for the update on the set of planned features

@jcanocan
Copy link
Contributor

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 16, 2024
0xFelix added 2 commits April 16, 2024 16:38
Signed-off-by: Felix Matouschek <[email protected]>
This changes the inventory plugin so that it sets the ansible_connection
to winrm if it detected a Windows host. If it did not detect a Windows
host the ansible_connection is no longer set, so Ansible falls back to
its default value of ssh. The detection of SSH services for hosts using
winrm is disabled.

Signed-off-by: Felix Matouschek <[email protected]>
@kubevirt-bot kubevirt-bot added size/L and removed lgtm Indicates that a PR is ready to be merged. size/XL labels Apr 16, 2024
@jcanocan
Copy link
Contributor

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 16, 2024
@0xFelix
Copy link
Member Author

0xFelix commented Apr 16, 2024

/hold cancel

@kubevirt-bot kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 16, 2024
@kubevirt-bot kubevirt-bot merged commit b6c061f into kubevirt:main Apr 16, 2024
79 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bugfixes Fixes that resolve issues. SHOULD not be used for minor enhancements dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The inventory plugin hard codes ansible_connection
5 participants