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(acme): implement resources and data sources for ACME accounts #1455

Merged
merged 23 commits into from
Aug 8, 2024

Conversation

ZauberNerd
Copy link
Contributor

@ZauberNerd ZauberNerd commented Jul 24, 2024

This PR implements the API for managing a Proxmox Cluster's ACME accounts and the related data sources and resources.

Contributor's Note

  • I have added / updated documentation in /docs for any user-facing features or additions.
  • I have added / updated acceptance tests in /fwprovider/tests for any new or updated resources / data sources.
  • I have ran make example to verify that the change works as expected.

Proof of Work

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

@ZauberNerd ZauberNerd changed the title WIP: feat(acme): implement resources and data sources for ACME configuration WIP: feat(acme): implement resources and data sources for ACME accounts Jul 26, 2024
// Name is the ACME account config file name.
Name types.String `tfsdk:"name"`
// Account is the ACME account information.
// Account types.Map `tfsdk:"account"` // XXX
Copy link
Contributor Author

@ZauberNerd ZauberNerd Jul 26, 2024

Choose a reason for hiding this comment

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

The API documents account as object but no further properties (https://pve.proxmox.com/pve-docs/api-viewer/#/cluster/acme/account/{name}), but we need to retrieve the email address.

The data returned by the API looks like this:

{
  "data": {
    "account": {
      "initialIp": "x.x.x.x",
      "createdAt": "2024-07-26T10:43:22Z",
      "status": "valid",
      "key": {
        "kty": "RSA",
        "use": "sig",
        "e": "xxxx",
        "n": "xxxx"
      },
      "contact": [
        "mailto:example@email.com"
      ]
    },
    "directory": "https://acme-staging-v02.api.letsencrypt.org/directory",
    "location": "https://acme-staging-v02.api.letsencrypt.org/acme/acct/xxxx",
    "tos": "https://letsencrypt.org/documents/LE-SA-v1.3-September-21-2022.pdf"
  }
}

But I'm not sure if it is always the same structure or if this structure only applies to letsencrypt?
Edit: The Proxmox admin guide mentions that only letsencrypt is implemented (https://pve.proxmox.com/pve-docs/pve-admin-guide.html#sysadmin_certificate_management), but in the UI I can select "custom" and then enter URL, EAB Key ID and EAB Key.

Copy link
Contributor Author

@ZauberNerd ZauberNerd Jul 27, 2024

Choose a reason for hiding this comment

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

I accessed the https://acme-staging-v02.api.letsencrypt.org/acme/acct/xxxx API directly and it looks like the account field is just passed through from the letsencrypt API and at least status and contact are specced: https://datatracker.ietf.org/doc/html/rfc8555/#section-7.1.2
The Proxmox web UI code then also just takes the first element of data.account as the email address, so I assume we could do that as well (https://git.proxmox.com/?p=pve-manager.git;a=blob;f=www/manager6/node/ACME.js;h=7fe49171e3b7249033be070d880cf988bf3e9772;hb=HEAD#l290).

Copy link
Owner

Choose a reason for hiding this comment

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

I think that's reasonable, and we can use the same approach.
So we'd need to add model fields for

  • email
  • createdAt
  • status

to be able to return the same details as UI does:
Screenshot 2024-07-29 at 9 26 02 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the data source I am just passing through the contact: []string array into the state. Or should I orient myself on the UI and take the first element of contact?

fwprovider/acme/datasource_acme_account.go Outdated Show resolved Hide resolved
fwprovider/acme/datasource_acme_account.go Outdated Show resolved Hide resolved
state.Directory = types.StringValue(account.Directory)
state.TOS = types.StringValue(account.TOS)
// XXX account.Location?
// XXX account.Account?
Copy link
Contributor Author

@ZauberNerd ZauberNerd Jul 26, 2024

Choose a reason for hiding this comment

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

See account above about letsencrypt and the data value. GETting a specific ACME account also returns the account object and a location url, which in the case of letsencrypt is a url to the letsencrypt account.
https://pve.proxmox.com/pve-docs/api-viewer/#/cluster/acme/account/{name}

I guess these should go into the state as computed properties?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In contrast to the data source (#1455 (comment)) I think for the resource I need to extract the first email address from data.contact and put that into state.Contact, right?
Because in the POST https://pve.proxmox.com/pve-docs/api-viewer/#/cluster/acme/account I'm providing contact, directory, eab-hmac-key, eab-kid, name and tos_url
but GETting an account (https://pve.proxmox.com/pve-docs/api-viewer/#/cluster/acme/account/{name}) returns account, directory, location and tos so that should be mapped to the state, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bpg I'm a bit unsure how to handle the resource and data source in this case, because the docs say it should closely follow the underlying API (https://developer.hashicorp.com/terraform/plugin/best-practices/hashicorp-provider-design-principles#resource-and-attribute-schema-should-closely-match-the-underlying-api), but the API and UI differ.
Also, should the schema of the data source and resource be the same or different?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the PR to take the first email address from contact and write that into the state.

fwprovider/acme/resource_acme_account.go Outdated Show resolved Hide resolved
fwprovider/acme/resource_acme_account.go Show resolved Hide resolved
fwprovider/acme/resource_acme_account.go Outdated Show resolved Hide resolved
proxmox/cluster/acme/account/client.go Show resolved Hide resolved
@ZauberNerd ZauberNerd changed the title WIP: feat(acme): implement resources and data sources for ACME accounts feat(acme): implement resources and data sources for ACME accounts Jul 26, 2024
@ZauberNerd ZauberNerd marked this pull request as ready for review July 26, 2024 12:32
Copy link
Owner

@bpg bpg left a comment

Choose a reason for hiding this comment

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

Hi @ZauberNerd 👋🏼

Thanks so much for contributing this! I think I've replied to all of your questions, but please ping me if you want to discuss something else.

Could you please also add the new resource/datasources to the /tools/tools.go list, then run make docs and commit the result? This will update the generated provider's docs.

I'd also love to see some acceptance tests for the resource, but I'll probably add them myself after you finalize your code.
That would help me with the review as well :)

proxmox/cluster/acme/account/client.go Show resolved Hide resolved
// Name is the ACME account config file name.
Name types.String `tfsdk:"name"`
// Account is the ACME account information.
// Account types.Map `tfsdk:"account"` // XXX
Copy link
Owner

Choose a reason for hiding this comment

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

I think that's reasonable, and we can use the same approach.
So we'd need to add model fields for

  • email
  • createdAt
  • status

to be able to return the same details as UI does:
Screenshot 2024-07-29 at 9 26 02 PM

fwprovider/acme/resource_acme_account.go Outdated Show resolved Hide resolved
fwprovider/acme/resource_acme_account.go Outdated Show resolved Hide resolved
fwprovider/acme/resource_acme_account.go Show resolved Hide resolved
Signed-off-by: Björn Brauer <zaubernerd@zaubernerd.de>
Signed-off-by: Björn Brauer <zaubernerd@zaubernerd.de>
Signed-off-by: Björn Brauer <zaubernerd@zaubernerd.de>
Signed-off-by: Björn Brauer <zaubernerd@zaubernerd.de>
Signed-off-by: Björn Brauer <zaubernerd@zaubernerd.de>
Signed-off-by: Björn Brauer <zaubernerd@zaubernerd.de>
Signed-off-by: Björn Brauer <zaubernerd@zaubernerd.de>
Signed-off-by: Björn Brauer <zaubernerd@zaubernerd.de>
Signed-off-by: Björn Brauer <zaubernerd@zaubernerd.de>
Signed-off-by: Björn Brauer <zaubernerd@zaubernerd.de>
Signed-off-by: Björn Brauer <zaubernerd@zaubernerd.de>
Signed-off-by: Björn Brauer <zaubernerd@zaubernerd.de>
Signed-off-by: Björn Brauer <zaubernerd@zaubernerd.de>
Signed-off-by: Björn Brauer <zaubernerd@zaubernerd.de>
Signed-off-by: Björn Brauer <zaubernerd@zaubernerd.de>
Signed-off-by: Björn Brauer <zaubernerd@zaubernerd.de>
Signed-off-by: Björn Brauer <zaubernerd@zaubernerd.de>
…anged"

This reverts commit af4a881.

Signed-off-by: Björn Brauer <zaubernerd@zaubernerd.de>
Signed-off-by: Björn Brauer <zaubernerd@zaubernerd.de>
Signed-off-by: Björn Brauer <zaubernerd@zaubernerd.de>
@ZauberNerd
Copy link
Contributor Author

@bpg I've updated the PR and added a few comments.
Could you please take another look?

I'd also love to see some acceptance tests for the resource, but I'll probably add them myself after you finalize your code.
That would help me with the review as well :)

I'm happy with adding acceptance tests myself, let me know which you prefer.

Signed-off-by: Pavel Boldyrev <627562+bpg@users.noreply.github.com>
@bpg
Copy link
Owner

bpg commented Aug 7, 2024

@ZauberNerd I started looking into adding acceptance tests and immediately hit a road bump. For some reason, the POST /api2/json/cluster/acme/account/ API can't work via API Token authentication and requires the user to be authenticated with local root@pam:

error creating ACME account: received an HTTP 403 response - Reason:
        Permission check failed (user != root@pam)

I don't really like switching all acceptance tests to use root account authentication, so I'll need to update the test harness to use different authentication types for different tests.

However, I also don't want to hold this PR up much longer. I think it is in a good state. I'll run a few more manual tests tomorrow and approve it soon.

bpg added 2 commits August 7, 2024 23:11
… & schema attributes alphabetically

Signed-off-by: Pavel Boldyrev <627562+bpg@users.noreply.github.com>
Copy link
Owner

@bpg bpg left a comment

Choose a reason for hiding this comment

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

Awesome work, thanks again for adding this feature!

LGTM! 🚀

@bpg bpg merged commit 9de4037 into bpg:main Aug 8, 2024
8 checks passed
@bpg
Copy link
Owner

bpg commented Aug 8, 2024

@all-contributors please add @ZauberNerd for code, idea

Copy link
Contributor

@bpg

I've put up a pull request to add @ZauberNerd! 🎉

@ZauberNerd ZauberNerd deleted the acme branch August 8, 2024 06:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants