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.

Sign up for GitHub

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

Jump to bottom

Support for "hardware mapping" as dedicated Terraform resource & data source #886

Closed
svengreb opened this issue Jan 12, 2024 · 4 comments · Fixed by #1213
Closed

Support for "hardware mapping" as dedicated Terraform resource & data source #886

svengreb opened this issue Jan 12, 2024 · 4 comments · Fixed by #1213
Assignees
@svengreb
Labels
✨ enhancement New feature or request

Comments

@svengreb
Copy link
Contributor

svengreb commented Jan 12, 2024 •
edited

First of all: Happy new year and thanks for this provider, this is definitely the best to automate everything for a home lab 😄

I recently set up a new OPNsense router device (Protectli VP2420) as an additional Proxmox node. The whole VM is automatically created via Packer which results in a VM template in the end. The actual VM is managed with Terraform, using this provider. Since the NIC that acts as the WAN in OPNSense should only be used by this VM, and also to improve the performance, I pass it through as PCI device.

After revisiting my code after the holidays I noticed that it is also possible to use a mapped resource, but that there is no dedicated proxmox_virtual_environment_cluster_hardware_mapping resource but that this step must still be done manually (or automated through other ways that interact with the Proxmox API). Even though the Proxmox API endpoint for hardware mappings is quite confusing I guess it might be nice if this provider could allow to manage this resource too.

I know that this would add another point in the "Known Issues" documentation since it is only possible to map a PCI/USB device using the root PAM account, but I guess this is still better than having to use my mouse to click it… because IaC everything 😀

An alternative solution I've considered was to add this PCI passthrough mapping to my Packer configuration, but that is not an ideal solution as it would bind it to the template which should rather act as a "one-time VM template" that gets deleted after (fully) cloning the VM with Terraform. This is also only a solution for me and does not allow to manage hardware mappings with Terraform in a general way.

Looking forward to hear your opinion! I'm also open to contribute this feature if you're busy, or support in any way to test it if you'd implement it yourself.
@svengreb svengreb changed the title Support to resource mapping as dedicated Terraform resource Support for "resource mapping" as dedicated Terraform resource Jan 12, 2024
@bpg bpg added the ✨ enhancement New feature or request label Jan 14, 2024
@bpg
Copy link
Owner

bpg commented Jan 14, 2024

Hi @svengreb! 👋🏼

Thank you for the suggestion. I'm totally on board with configuring PCI mapping through a resource. Even though I don't play around with mapped devices in my home lab, I'd love some help getting this feature up and running!

If you (or anyone else! 😄) want to jump in and contribute, we've got two main things to tackle: setting up an API wrapper around the Proxmox REST API in /proxmox, and creating the resource itself in /fwprovider. The most recent resources/datasources were created using the TF Plugin Framework, and you can use the code under /fwprovider as an example.

Feel free to drop any questions here or, better yet, open a pull request. Any contributions and insights will be much appreciated!

@svengreb
Copy link
Contributor Author

Thanks for the tips to get started in this repository! 👍🏼
I've planned to implement it within the next weeks, but of course everyone who reads this ticket can pick it up.

@svengreb
Copy link
Contributor Author

svengreb commented Mar 3, 2024 •
edited

Just a quick update: I started (and almost completed) the implementation while running it against my own homelab, just in case someone wanted to pick this issue up and we end up with duplicated PRs in the end 😄

@svengreb svengreb changed the title Support for "resource mapping" as dedicated Terraform resource Support for "hardware mapping" as dedicated Terraform resource & data source Apr 15, 2024
svengreb added a commit to svengreb/terraform-provider-proxmox that referenced this issue Apr 15, 2024
@svengreb
feat(cluster): Implement initial support for "hardware mappings"
7598a22 
Right now it is alredy possible to use a mapped resource [1], but there
is no dedicated `proxmox_virtual_environment_cluster_hardware_mapping`
resource but this step must still be done manually (or automated through
other ways that interact with the Proxmox API).

This commit implements support for "hardware mapping" resources and data
sources for the, currently, available bus types PCI and USB, based on
the Proxmox VE API documentations [2].

There are some "specialities" in these resources and data sources:

1. The Proxmox VE API attribute, but this implementations names it
   "comment" since this naming is generally across the Proxmox VE web UI
   and API documentations. This still follows the Terraform
   "best practices" [3] as it improves the user experience by matching
   the field name to the naming used in the human-facing interfaces.

2. Like in point 1, the name of the attribute of "node checks
   diagnostics" for USB hardware mappings is "errors" in the Proxmox VE
   API while it is "checks" for hardware mappings of type PCI.
   The second naming pattern is also generally used across the
   Proxmox VE web UI and API documentations, including the "check_node"
   attribute that is also implemented in the
   "proxmox_virtual_environment_hardware_mappings" data source.
   Therefore, this implementation named both attributes "checks" which
   still follows the Terraform "best practices" [3] as it improves the
   user experience by matching the field name to the naming used in the
   human-facing interfaces.
3. This implmenetation comes with the "unique" feature of allowing
   comments (named "descriptions" by the Proxmox VE API) for an entry in
   a device map which is not possible through the web UI at all but only
   adding a comment for the whole mapping entry instead.

Note that this implementation also adds another point in the
"Known Issues" documentation since it is only possible to map a
PCI/USB device using the `root` PAM account, but this is still better
than having to manually configure it through the web UI or by
interacting with the Proxmox VE API on other ways.

[1]: bpg#500
[2]: https://pve.proxmox.com/pve-docs/api-viewer/#/cluster/mapping/pci
[3]: https://developer.hashicorp.com/terraform/plugin/best-practices/hashicorp-provider-design-principles#resource-and-attribute-schema-should-closely-match-the-underlying-api

Signed-off-by: Sven Greb <development@svengreb.de>

Closes bpgGH-886

Signed-off-by: Sven Greb <development@svengreb.de>
svengreb added a commit to svengreb/terraform-provider-proxmox that referenced this issue Apr 15, 2024
@svengreb
feat(cluster): Implement initial support for "hardware mappings"
578558a 
Right now it is alredy possible to use a mapped resource [1], but there
is no dedicated `proxmox_virtual_environment_cluster_hardware_mapping`
resource but this step must still be done manually (or automated through
other ways that interact with the Proxmox API).

This commit implements support for "hardware mapping" resources and data
sources for the, currently, available bus types PCI and USB, based on
the Proxmox VE API documentations [2].

There are some "specialities" in these resources and data sources:

1. The Proxmox VE API attribute, but this implementations names it
   "comment" since this naming is generally across the Proxmox VE web UI
   and API documentations. This still follows the Terraform
   "best practices" [3] as it improves the user experience by matching
   the field name to the naming used in the human-facing interfaces.

2. Like in point 1, the name of the attribute of "node checks
   diagnostics" for USB hardware mappings is "errors" in the Proxmox VE
   API while it is "checks" for hardware mappings of type PCI.
   The second naming pattern is also generally used across the
   Proxmox VE web UI and API documentations, including the "check_node"
   attribute that is also implemented in the
   "proxmox_virtual_environment_hardware_mappings" data source.
   Therefore, this implementation named both attributes "checks" which
   still follows the Terraform "best practices" [3] as it improves the
   user experience by matching the field name to the naming used in the
   human-facing interfaces.
3. This implmenetation comes with the "unique" feature of allowing
   comments (named "descriptions" by the Proxmox VE API) for an entry in
   a device map which is not possible through the web UI at all but only
   adding a comment for the whole mapping entry instead.

Note that this implementation also adds another point in the
"Known Issues" documentation since it is only possible to map a
PCI/USB device using the `root` PAM account, but this is still better
than having to manually configure it through the web UI or by
interacting with the Proxmox VE API on other ways.

[1]: bpg#500
[2]: https://pve.proxmox.com/pve-docs/api-viewer/#/cluster/mapping/pci
[3]: https://developer.hashicorp.com/terraform/plugin/best-practices/hashicorp-provider-design-principles#resource-and-attribute-schema-should-closely-match-the-underlying-api

Signed-off-by: Sven Greb <development@svengreb.de>

Closes bpgGH-886

Signed-off-by: Sven Greb <development@svengreb.de>
@svengreb svengreb mentioned this issue Apr 15, 2024
Merged
3 tasks
svengreb added a commit to svengreb/terraform-provider-proxmox that referenced this issue Apr 15, 2024
@svengreb
feat(cluster): Implement initial support for "hardware mappings"
c887500 
Right now it is alredy possible to use a mapped resource [1], but there
is no dedicated `proxmox_virtual_environment_cluster_hardware_mapping`
resource but this step must still be done manually (or automated through
other ways that interact with the Proxmox API).

This commit implements support for "hardware mapping" resources and data
sources for the, currently, available bus types PCI and USB, based on
the Proxmox VE API documentations [2].

There are some "specialities" in these resources and data sources:

1. The Proxmox VE API attribute, but this implementations names it
   "comment" since this naming is generally across the Proxmox VE web UI
   and API documentations. This still follows the Terraform
   "best practices" [3] as it improves the user experience by matching
   the field name to the naming used in the human-facing interfaces.

2. Like in point 1, the name of the attribute of "node checks
   diagnostics" for USB hardware mappings is "errors" in the Proxmox VE
   API while it is "checks" for hardware mappings of type PCI.
   The second naming pattern is also generally used across the
   Proxmox VE web UI and API documentations, including the "check_node"
   attribute that is also implemented in the
   "proxmox_virtual_environment_hardware_mappings" data source.
   Therefore, this implementation named both attributes "checks" which
   still follows the Terraform "best practices" [3] as it improves the
   user experience by matching the field name to the naming used in the
   human-facing interfaces.
3. This implmenetation comes with the "unique" feature of allowing
   comments (named "descriptions" by the Proxmox VE API) for an entry in
   a device map which is not possible through the web UI at all but only
   adding a comment for the whole mapping entry instead.

Note that this implementation also adds another point in the
"Known Issues" documentation since it is only possible to map a
PCI/USB device using the `root` PAM account, but this is still better
than having to manually configure it through the web UI or by
interacting with the Proxmox VE API on other ways.

[1]: bpg#500
[2]: https://pve.proxmox.com/pve-docs/api-viewer/#/cluster/mapping/pci
[3]: https://developer.hashicorp.com/terraform/plugin/best-practices/hashicorp-provider-design-principles#resource-and-attribute-schema-should-closely-match-the-underlying-api

Signed-off-by: Sven Greb <development@svengreb.de>

Closes bpgGH-886

Signed-off-by: Sven Greb <development@svengreb.de>
@svengreb
Copy link
Contributor Author

@bpg The PR is finally submitted: #1213 🎉

Took some time again because I refactored my first revision that used some generic code to "prevent" to create dedicated resources & data sources for each bus type (PCI and USB), but the Terraform Plugin Framework is designed to enforce explicit and non-abstract implementations so I restored by previous implementation agin, which is the state that is now available in the PR.

svengreb added a commit to svengreb/terraform-provider-proxmox that referenced this issue Apr 16, 2024
@svengreb
feat(cluster): Remove unnecessary golangci-lint ignore directive (file)
e33a7ca 
bpgGH-886
svengreb added a commit to svengreb/terraform-provider-proxmox that referenced this issue Apr 17, 2024
@svengreb
feat(cluster): Implement initial support for "hardware mappings"
b0cb569 
Right now it is alredy possible to use a mapped resource [1], but there
is no dedicated `proxmox_virtual_environment_cluster_hardware_mapping`
resource but this step must still be done manually (or automated through
other ways that interact with the Proxmox API).

This commit implements support for "hardware mapping" resources and data
sources for the, currently, available bus types PCI and USB, based on
the Proxmox VE API documentations [2].

There are some "specialities" in these resources and data sources:

1. The Proxmox VE API attribute, but this implementations names it
   "comment" since this naming is generally across the Proxmox VE web UI
   and API documentations. This still follows the Terraform
   "best practices" [3] as it improves the user experience by matching
   the field name to the naming used in the human-facing interfaces.

2. Like in point 1, the name of the attribute of "node checks
   diagnostics" for USB hardware mappings is "errors" in the Proxmox VE
   API while it is "checks" for hardware mappings of type PCI.
   The second naming pattern is also generally used across the
   Proxmox VE web UI and API documentations, including the "check_node"
   attribute that is also implemented in the
   "proxmox_virtual_environment_hardware_mappings" data source.
   Therefore, this implementation named both attributes "checks" which
   still follows the Terraform "best practices" [3] as it improves the
   user experience by matching the field name to the naming used in the
   human-facing interfaces.
3. This implmenetation comes with the "unique" feature of allowing
   comments (named "descriptions" by the Proxmox VE API) for an entry in
   a device map which is not possible through the web UI at all but only
   adding a comment for the whole mapping entry instead.

Note that this implementation also adds another point in the
"Known Issues" documentation since it is only possible to map a
PCI/USB device using the `root` PAM account, but this is still better
than having to manually configure it through the web UI or by
interacting with the Proxmox VE API on other ways.

[1]: bpg#500
[2]: https://pve.proxmox.com/pve-docs/api-viewer/#/cluster/mapping/pci
[3]: https://developer.hashicorp.com/terraform/plugin/best-practices/hashicorp-provider-design-principles#resource-and-attribute-schema-should-closely-match-the-underlying-api

Signed-off-by: Sven Greb <development@svengreb.de>

Closes bpgGH-886

Signed-off-by: Sven Greb <development@svengreb.de>
svengreb added a commit to svengreb/terraform-provider-proxmox that referenced this issue Apr 17, 2024
@svengreb
feat(cluster): Remove unnecessary golangci-lint ignore directive (file)
04cb665 
bpgGH-886
svengreb added a commit to svengreb/terraform-provider-proxmox that referenced this issue Apr 17, 2024
@svengreb
feat(cluster): Refactor package layout to align with other packages
b65c5a2 
This is a result of the pull request review.

bpgGH-886
svengreb added a commit to svengreb/terraform-provider-proxmox that referenced this issue Apr 18, 2024
@svengreb
feat(cluster): Remove unnecessary golangci-lint ignore directive (file)
e036652 
bpgGH-886

Signed-off-by: Sven Greb <development@svengreb.de>
svengreb added a commit to svengreb/terraform-provider-proxmox that referenced this issue Apr 18, 2024
@svengreb
feat(cluster): Refactor package layout to align with other packages
9fc2a08 
This is a result of the pull request review.

bpgGH-886

Signed-off-by: Sven Greb <development@svengreb.de>
svengreb added a commit to svengreb/terraform-provider-proxmox that referenced this issue Apr 18, 2024
@svengreb
feat(cluster): Fix golangci-lint formatting matches
4be4e46 
bpgGH-886

Signed-off-by: Sven Greb <development@svengreb.de>
@bpg bpg closed this as completed in #1213 Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@svengreb svengreb

Labels
✨ enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat(cluster): Implement initial support for "hardware mappings" svengreb/terraform-provider-proxmox
2 participants
@bpg @svengreb