Conversation
|
Welcome @plouton24! |
|
Hi @plouton24. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hey @plouton24, thanks for doing this work! Could you rebase it from image-builder's |
600dcd9 to
aa2a54f
Compare
This has been completed. |
|
/ok-to-test |
drew-viles
left a comment
There was a problem hiding this comment.
So I'm no Windows, Proxmox or Python expert - I want to prefix that. But a few things popped up in my looks through this that I wanted to ask about. See comments below :-)
| def main(): | ||
| network_data_path = find_network_data_path() | ||
| if not network_data_path: | ||
| LOG.info( | ||
| "No Proxmox network data found in candidate paths: %s", | ||
| ", ".join(_iter_candidate_paths()), | ||
| ) | ||
| return 0 | ||
|
|
||
| try: | ||
| network_data = load_network_data(network_data_path) | ||
| except Exception: | ||
| LOG.exception( | ||
| "Failed to load Proxmox network data from %s", network_data_path | ||
| ) | ||
| return 0 | ||
|
|
||
| try: | ||
| LOG.info("Applying Proxmox network data from %s", network_data_path) | ||
| applied = apply_network_data(network_data) | ||
| except Exception: | ||
| LOG.exception( | ||
| "Failed to apply Proxmox network data from %s", network_data_path | ||
| ) | ||
| return 0 | ||
|
|
||
| if not applied: | ||
| LOG.warning( | ||
| "No network interfaces were applied from %s", network_data_path | ||
| ) | ||
|
|
||
| return 0 |
There was a problem hiding this comment.
Would it not be better to fail a little louder rather than return 0 if something fails? As I'm reading this, it looks like failures will happen silently meaning an image could successfully build even if something like applying the proxmox network fails.
Would we be ok with this?
There was a problem hiding this comment.
So this is kind of my hack to read the cloudinit network data that CAPMOX injects via a ISO-9660 cd drive for kubeadm boostrap. So this wont fail the build of the VM, as its a node first boot helper script for cloudbase-init(windows version of cloud-init). Though I should try to write to a file to troubleshoot networking incase CAPMOX changes its kubeadmbootstrap process, and be more verbose on failures.
| <ProductKey> | ||
| <Key>VDYBN-27WPP-V4HQT-9VMD4-VMK7H</Key> | ||
| <WillShowUI>OnError</WillShowUI> | ||
| </ProductKey> |
There was a problem hiding this comment.
Do we mind this being here? Is this a demo key?
There was a problem hiding this comment.
This to my understanding is a demo key. Copied from proxmox/ova/windows/2022/autounattend.xml
| build-proxmox-ubuntu-2204: ## Builds the Proxmox ubuntu-2204 image | ||
| build-proxmox-ubuntu-2404: ## Builds the Proxmox ubuntu-2404 image | ||
| build-proxmox-ubuntu-2404-efi: ## Builds the Proxmox ubuntu-2404-efi image that EFI boots | ||
| build-proxmox-rockylinux-9: ## Builds the Proxmox rockylinux-9 image |
There was a problem hiding this comment.
Look like duplicate lines of 862 - 865
| # This uses a packer file builder to input unattend variables into a JSON file to be consumed by the python script before running the vsphere provisioner | ||
| $(if $(findstring windows,$@),$(PACKER) build $(PACKER_WINDOWS_NODE_FLAGS) -var-file="$(abspath packer/proxmox/$(subst build-proxmox-,,$@).json)" -var-file="$(abspath packer/proxmox/$(subst build-proxmox-,,$@).json)" -only=file $(ABSOLUTE_PACKER_VAR_FILES) packer/proxmox/packer-windows.json,) | ||
| $(if $(findstring windows,$@),hack/windows-unattend.py --unattend-file='./packer/proxmox/windows/$(subst build-proxmox-,,$@)/autounattend.xml',) | ||
| $(PACKER) build $(if $(findstring windows,$@),$(PACKER_WINDOWS_NODE_FLAGS),$(PACKER_NODE_FLAGS)) -var-file="$(abspath packer/proxmox/$(subst build-proxmox-,,$@).json)" $(ABSOLUTE_PACKER_VAR_FILES) packer/proxmox/packer$(if $(findstring windows,$@),-windows,).json | ||
|
|
There was a problem hiding this comment.
This section looks copy-pasted: the same -var-file is passed twice, and the comment references vSphere in the Proxmox path. Is the duplicate var-file intentional?
| cmd.exe /c winrm set "winrm/config/service" '@{AllowUnencrypted="true"}' | ||
| cmd.exe /c winrm set "winrm/config/client" '@{AllowUnencrypted="true"}' |
There was a problem hiding this comment.
Is this secure? Is it standard for Windows?
There was a problem hiding this comment.
I copied the existing winrm config from nutanix, oci and azure. Also the ansible windows workflow also has similar settings. I do agree work is needed on most windows enabled providers to disable winrm after image build if not required, as it is a security risk. Just was unsure if there were capmox/capi specific need for winrm to be enabled after build. I can test disabling this on sysprep/final build step.
There was a problem hiding this comment.
Is this a WIP file and is is needed?
Comments on the most of the for the ones i didnt comment on I will fix via a commit. As need to fix typos and other small issues you mentioned. |
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Change description
This PR adds Windows Server 2022 image support for the Proxmox provider in image-builder. For use with CAPMOX CAPI project.
The main changes are:
proxmox-iso-
META_DATA-
USER_DATA-
NETWORK_CONFIGNETWORK_CONFIGA few implementation notes:
META_DATAandUSER_DATAare handled through Cloudbase-Init config overridesNETWORK_CONFIGis currently handled by a small Proxmox-specific python script staged through the Windows provider roleThis PR adds support for a new provider / OS combination.
Related issues
Testing
Testing performed during development:
make build-proxmox-windows-2022Additional context