feat: added acme certificate ordering #2292
Conversation
Summary of ChangesHello @lonelysadness, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Proxmox Terraform provider by adding a new resource for automated ACME SSL certificate management. This new functionality allows users to declare and manage their Proxmox node certificates, including ordering new ones and handling renewals, directly within their Terraform configurations. It integrates with Proxmox's ACME client, supporting various challenge types and providing a streamlined approach to maintaining secure Proxmox environments. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new resource proxmox_virtual_environment_acme_certificate for managing ACME certificates. The implementation is a good starting point and covers creation, update, deletion, and import, along with documentation and examples.
My review focuses on improving robustness and correctness. Key points include:
- Replacing
time.Sleepwith a more reliable polling mechanism to avoid race conditions. - Ensuring the correct certificate is identified after creation/update.
- Using the appropriate API endpoint for updates to guarantee configuration changes are applied.
- Minor improvements in documentation and context propagation.
The changes to proxmox/nodes/config.go and proxmox/nodes/config_types.go are solid improvements. Overall, this is a great feature addition.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable new resource for managing ACME certificates in Proxmox. The implementation is well-structured, particularly the use of a retry mechanism with exponential backoff for polling certificate status. However, there are several critical issues that need to be addressed, including a syntax error, a missing context parameter causing a build failure, and a decoding bug that would lead to a runtime panic. The documentation examples also require corrections to be functional. I've provided detailed comments and suggestions to resolve these issues.
| resource "proxmox_virtual_environment_acme_certificate" "example" { | ||
| node_name = "pve" | ||
| # Depends on the ACME account being created first | ||
| depends_on = [ | ||
| proxmox_virtual_environment_acme_account.example | ||
| ] | ||
| } |
There was a problem hiding this comment.
This example for a basic HTTP-01 challenge is missing the required account and domains attributes, which will cause it to fail. A similar issue exists in the 'Force Certificate Renewal' example starting on line 80. Please update both examples to include these required attributes to make them functional for users.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new resource for managing ACME certificates, which is a great addition. The implementation is mostly solid, with good use of retries for asynchronous operations and a thoughtful approach to finding the correct certificate. I've found a critical issue in the certificate matching logic that needs to be addressed, as it will cause a panic. I've also suggested improvements to the documentation examples to make them complete and functional. Additionally, there's some dead code in the client library that could be cleaned up. Overall, this is a good starting point and with these fixes, it will be a valuable feature.
lonelysadness
force-pushed
the
feat/acme-certificate-ordering
branch
from
30d4168 to
8257ed7
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new resource for managing ACME certificates, which is a valuable addition. The implementation is well-structured, particularly the use of a retry mechanism for handling asynchronous certificate ordering. My review focuses on improving the resource's lifecycle management to prevent stale configurations on Proxmox nodes. Specifically, I've suggested changes to ensure that ACME configurations are properly cleaned up during delete and update operations. These adjustments will enhance the resource's reliability and idempotency.
| func (r *acmeCertificateResource) Delete( | ||
| ctx context.Context, | ||
| req resource.DeleteRequest, | ||
| resp *resource.DeleteResponse, | ||
| ) { | ||
| var state acmeCertificateModel | ||
|
|
||
| resp.Diagnostics.Append(req.State.Get(ctx, &state)...) | ||
|
|
||
| if resp.Diagnostics.HasError() { | ||
| return | ||
| } | ||
|
|
||
| // Note: We don't actually delete the certificate from the node | ||
| // as it's needed for the node's services. We just remove it from Terraform state. | ||
| // If users want to remove the certificate from the node, they should use | ||
| // the Proxmox UI or manually delete it via the API. | ||
| } |
There was a problem hiding this comment.
The Delete function currently only removes the resource from Terraform state, leaving the ACME configuration on the Proxmox node. This can lead to unexpected behavior and stale configurations. While the certificate files should remain, the node's configuration related to ACME (acme and acmedomain0-4 properties) should be cleaned up. This can be achieved by calling nodeClient.UpdateConfig with the delete parameter.
func (r *acmeCertificateResource) Delete(
ctx context.Context,
req resource.DeleteRequest,
resp *resource.DeleteResponse,
) {
var state acmeCertificateModel
resp.Diagnostics.Append(req.State.Get(ctx, &state)...)
if resp.Diagnostics.HasError() {
return
}
nodeName := state.NodeName.ValueString()
nodeClient := r.client.Node(nodeName)
// Clean up the ACME configuration from the node. The certificate files will remain.
toDelete := "acme,acmedomain0,acmedomain1,acmedomain2,acmedomain3,acmedomain4"
configUpdate := &nodes.ConfigUpdateRequestBody{
Delete: &toDelete,
}
if err := nodeClient.UpdateConfig(ctx, configUpdate); err != nil {
// Log a warning as the resource is being deleted anyway, but the user should be notified.
resp.Diagnostics.AddWarning(
"Failed to clean up node ACME configuration",
fmt.Sprintf("An error occurred while cleaning up ACME settings for node %s on delete: %s. Manual cleanup of /etc/pve/nodes/%s/config may be required.", nodeName, err.Error(), nodeName),
)
}
}| if len(dnsDomains) > 0 { | ||
| if len(dnsDomains) > 5 { | ||
| return fmt.Errorf("Proxmox supports a maximum of 5 DNS challenge domains, got %d", len(dnsDomains)) | ||
| } | ||
|
|
||
| for i, domain := range dnsDomains { | ||
| domainConfig := &nodes.ACMEDomainConfig{ | ||
| Domain: domain.Domain.ValueString(), | ||
| } | ||
|
|
||
| if !domain.Plugin.IsNull() { | ||
| plugin := domain.Plugin.ValueString() | ||
| domainConfig.Plugin = &plugin | ||
| } | ||
|
|
||
| if !domain.Alias.IsNull() { | ||
| alias := domain.Alias.ValueString() | ||
| domainConfig.Alias = &alias | ||
| } | ||
|
|
||
| // Map to the appropriate acmedomain field | ||
| switch i { | ||
| case 0: | ||
| configUpdate.ACMEDomain0 = domainConfig | ||
| case 1: | ||
| configUpdate.ACMEDomain1 = domainConfig | ||
| case 2: | ||
| configUpdate.ACMEDomain2 = domainConfig | ||
| case 3: | ||
| configUpdate.ACMEDomain3 = domainConfig | ||
| case 4: | ||
| configUpdate.ACMEDomain4 = domainConfig | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation for configuring DNS challenge domains does not clean up acmedomainX slots that are no longer in use. For instance, if a user changes from 3 DNS domains to 1, the configurations for acmedomain1 and acmedomain2 will persist on the Proxmox node. To ensure the resource is idempotent, you should explicitly clear any unused acmedomainX slots by using the delete parameter in the update request.
if len(dnsDomains) > 5 {
return fmt.Errorf("Proxmox supports a maximum of 5 DNS challenge domains, got %d", len(dnsDomains))
}
for i, domain := range dnsDomains {
domainConfig := &nodes.ACMEDomainConfig{
Domain: domain.Domain.ValueString(),
}
if !domain.Plugin.IsNull() {
plugin := domain.Plugin.ValueString()
domainConfig.Plugin = &plugin
}
if !domain.Alias.IsNull() {
alias := domain.Alias.ValueString()
domainConfig.Alias = &alias
}
// Map to the appropriate acmedomain field
switch i {
case 0:
configUpdate.ACMEDomain0 = domainConfig
case 1:
configUpdate.ACMEDomain1 = domainConfig
case 2:
configUpdate.ACMEDomain2 = domainConfig
case 3:
configUpdate.ACMEDomain3 = domainConfig
case 4:
configUpdate.ACMEDomain4 = domainConfig
}
}
// Clean up unused acmedomain slots
var toDelete []string
for i := len(dnsDomains); i < 5; i++ {
toDelete = append(toDelete, fmt.Sprintf("acmedomain%d", i))
}
if len(toDelete) > 0 {
deleteValue := strings.Join(toDelete, ",")
configUpdate.Delete = &deleteValue
}
Signed-off-by: lonelysadness <[email protected]>
Signed-off-by: lonelysadness <[email protected]>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: lonelysadness <[email protected]> Signed-off-by: lonelysadness <[email protected]>
Signed-off-by: lonelysadness <[email protected]>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: lonelysadness <[email protected]> Signed-off-by: lonelysadness <[email protected]>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: lonelysadness <[email protected]> Signed-off-by: lonelysadness <[email protected]>
Signed-off-by: lonelysadness <[email protected]>
Signed-off-by: lonelysadness <[email protected]>
Signed-off-by: lonelysadness <[email protected]>
Signed-off-by: lonelysadness <[email protected]>
Signed-off-by: lonelysadness <[email protected]>
Signed-off-by: lonelysadness <[email protected]>
Signed-off-by: lonelysadness <[email protected]>
Signed-off-by: lonelysadness <[email protected]>
lonelysadness
force-pushed
the
feat/acme-certificate-ordering
branch
from
fd54e2b to
fd0130c
Compare
|
I think I have a good functional starting point, I'll wait for @bpg opinion or someone else's. Thank you. PS: Sorry for the spam with Gemini, I didn't see the mess it made in the PR. |
All good, thanks for contributing this @lonelysadness! ❤️ It will take some time to go over the PR tho... |
Signed-off-by: Pavel Boldyrev <[email protected]>
| // - Set PROXMOX_VE_ACC_ACME_DOMAIN environment variable | ||
| // - Set PROXMOX_VE_ACC_ACME_DNS_PLUGIN (optional, for DNS-01 challenge) | ||
| // The test will be skipped if these are not set. | ||
| func TestAccResourceACMECertificate(t *testing.T) { |
There was a problem hiding this comment.
Could you add more details here or to the PR description how to set up a test environment to run the tests, what acme server to use, how to set it up, etc.?
Just links to the docs and brief description, enough to anyone else to set that up from scratch.
| if nodeName == "" { | ||
| nodeName = "pve" | ||
| } |
There was a problem hiding this comment.
this is redundant, te.NodeName defaults to pve
| }]` | ||
|
|
||
| te.AddTemplateVars(map[string]interface{}{ | ||
| "NodeName": nodeName, |
| }, | ||
| retry.Attempts(30), // Maximum 30 attempts | ||
| retry.Delay(1*time.Second), // Start with 1 second delay | ||
| retry.DelayType(retry.BackOffDelay), // Use exponential backoff |
There was a problem hiding this comment.
exponential backoff with 30 attempts is a way too long IMO,
prob. just a fixed delay (5 sec? 10 sec?) backoff?
| retry.Attempts(30), // Maximum 30 attempts | ||
| retry.Delay(1*time.Second), // Start with 1 second delay | ||
| retry.DelayType(retry.BackOffDelay), // Use exponential backoff | ||
| retry.MaxJitter(500*time.Millisecond), // Add jitter to prevent thundering herd |
There was a problem hiding this comment.
not sure if that will prevent anything in this scenario 😅
| Force: &force, | ||
| } | ||
|
|
||
| taskID, err := nodeClient.OrderCertificate(ctx, orderReq) |
There was a problem hiding this comment.
looks like we'll send the order call regardless if there were changes or not, or value of the force flag.
should we add a condition here like if plan.Force.ValueBool() || !plan.Account.Equal(state.Account) || !plan.Domains.Equal(state.Domains) {....
| // It prioritizes ACME certificates (issued by certificate authorities like Let's Encrypt) | ||
| // over Proxmox-generated certificates. When multiple certificates match the configured domains, | ||
| // it returns the one with the most matching domains. | ||
| func (r *acmeCertificateResource) findMatchingCertificate( |
There was a problem hiding this comment.
this logic seems pretty complex
i'd like to see unit test covering matching scenarios with some edge cases
bpg
added
the
⌛ pending author's response
|
Thanks for all your feedback. I was busy, I'll take a look in the week ! |
2 participants
Hello,
I've started work on ACME certificate ordering, and it is functioning in my tests using the Desec provider with the DNS-01 challenge.
While the implementation could likely use further optimization, I wanted to share it here as a starting point. Apologies if any parts of the code are rough. Feedback and suggestions are welcome!
Contributor's Note
/docsfor any user-facing features or additions./fwprovider/testsfor any new or updated resources / data sources.make exampleto verify that the change works as expected.Proof of Work
Community Note
Relates #2157