Skip to content
This repository has been archived by the owner on Sep 6, 2023. It is now read-only.

feat: add project API key resource #147

Merged
merged 5 commits into from
Jul 20, 2021
Merged

feat: add project API key resource #147

merged 5 commits into from
Jul 20, 2021

Conversation

rawkode
Copy link
Contributor

@rawkode rawkode commented Jul 9, 2021

This PR aims to add support for creating project level API Keys.

@rawkode rawkode marked this pull request as ready for review July 9, 2021 17:29
"project_id": {
Type: schema.TypeString,
ForceNew: true,
Required: true,
Copy link
Member

@displague displague Jul 9, 2021

Choose a reason for hiding this comment

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

If this field were not required, this resource could be metal_api_key with optional project scoping.

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, I recognize that we have both metal_project_ssh_key and metal_ssh_key.

I'm not sure that I see a reason for the distinction, then or now. project_ssh_keys came a few years after ssh_keys.
https://github.com/equinix/terraform-provider-metal/blob/main/metal/resource_metal_project_ssh_key.go

When I was first introduced to the API, I found some justification for this because some resources are created behind /projects/id/foo, while others are not. After a bit more exposure to the API, I find that resources (project or not) are all typically accessible behind /foo/id. Project scoped or not, these optionally scoped resources have the same response body, other than the project id field.

When a resource is project scoped, the API path contains the (optional) project_id rather than the POST body. If a listing endpoint is present in /projects/id/foo, it is offered as a convenience to /foo?project_id=id.

They are otherwise the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

@rawkode @displague I think we can do both metal_user_api_key and metal_project_api_key in a similar way as the ssh_keys.

Just make the project_id Optional: true. Create and Delete methods can be shared between metal_user_api_key and metal_project_api_key. You will need to have separate Read methods.

@displague I dived to the API key methods and created equinixmetal-archive/packngo#297

Copy link
Contributor Author

@rawkode rawkode Jul 11, 2021

Choose a reason for hiding this comment

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

For what it's worth, I did consider using a single resource for both and I do prefer that API, but unfortunately the backing API at EM doesn't provide a single read endpoint. I did consider working around that, but importing a resource wouldn't work and I thought that was more important. Perhaps we can work with engineering to clean up this API. 

Copy link
Contributor

Choose a reason for hiding this comment

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

@rawkode you're right that the read endpoint for user API key is different. You could create a resource for the API key, reusing some of the project API key code. If you'd change the project_id to Optional: true, you could just add resource_metal_user_api_key.go as

func resourceMetalUserAPIKey() *schema.Resource {
	return &schema.Resource{
		Create: resourceMetalProjectAPIKeyCreate,
		Read:   resourceMetalUserAPIKeyRead,
		Delete: resourceMetalProjectAPIKeyDelete,
		Importer: &schema.ResourceImporter{
			State: schema.ImportStatePassthrough,
		},

		Schema: metalProjectAPIKey(),
	}
}

func resourceMetalUserAPIKeyRead(d *schema.ResourceData, meta interface{}) error {
	client := meta.(*packngo.Client)
	apiKey, err := client.APIKeys.UserGet(d.Id(), nil)
	if err != nil {
		err = friendlyError(err)
		if isNotFound(err) {
			log.Printf("[WARN] User APIKey (%s) not found, removing from state", d.Id())
			d.SetId("")
			return nil
		}
		return err
	}
	d.SetId(apiKey.ID)
	d.Set("description", apiKey.Description)
	d.Set("read_only", apiKey.ReadOnly)
	d.Set("created", apiKey.Created)
	d.Set("updated", apiKey.Updated)
	return nil
}

.. the user API key resource would actually be importable.

If you don't want to do it in this PR, it's OK we can add metal_user_api_key later. Thsis is good to go after you remove the Import and the Update method.

"project_id": {
Type: schema.TypeString,
ForceNew: true,
Required: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

@rawkode @displague I think we can do both metal_user_api_key and metal_project_api_key in a similar way as the ssh_keys.

Just make the project_id Optional: true. Create and Delete methods can be shared between metal_user_api_key and metal_project_api_key. You will need to have separate Read methods.

@displague I dived to the API key methods and created equinixmetal-archive/packngo#297

Read: resourceMetalProjectAPIKeyRead,
Update: resourceMetalProjectAPIKeyUpdate,
Delete: resourceMetalProjectAPIKeyDelete,
Importer: &schema.ResourceImporter{
Copy link
Contributor

Choose a reason for hiding this comment

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

terraform import only allows to use one parameter, usually it's resource UUID. In case of resources like this, which need 2 params - project ID and key ID, you can't really do import out-of the box like you did.

It can be solved by parsing both IDs from a single string "<project_id>:<key_id>" (https://www.terraform.io/docs/extend/resources/import.html#importer-state-function), and implementing custom Resource Importer, but I'm not sure if we need import of this resource right here right now. Maybe it's better if you just remove the importer (for the project_api_key).

return nil
}

func resourceMetalProjectAPIKeyUpdate(d *schema.ResourceData, meta interface{}) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Update of API key is impossible. Remove the Update method.

@t0mk
Copy link
Contributor

t0mk commented Jul 16, 2021

@rawkode I refactored your code a bit and added metal_user_api_key and tests for both resources. I will later write docs and ask you and @displague to review.

@t0mk
Copy link
Contributor

t0mk commented Jul 20, 2021

@displague this is ready for review

@displague displague merged commit 2ede959 into equinix:main Jul 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants