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

Introduce Storage Bucket Resource #5

Merged
merged 1 commit into from
Dec 18, 2023
Merged

Conversation

maveonair
Copy link
Member

@maveonair maveonair commented Dec 17, 2023

Description

This pull request aims to improve the capabilities of this Terraform provider by adding support for managing storage buckets and their access keys.

Proposed resource definitions

Storage Bucket Creation

resource "incus_storage_bucket" "bucket1" {
  name    = "bucket1"
  project = "default"
  pool    = "default"
}

resource "incus_storage_bucket_key" "reader" {
  name           = "reader"
  description    = "Read-Only User"
  pool           = incus_storage_bucket.bucket1.pool
  storage_bucket = incus_storage_bucket.bucket1.name
}

resource "incus_storage_bucket_key" "writer" {
  name           = "writer"
  description    = "Write User"
  role           = "admin"
  pool           = incus_storage_bucket.bucket1.pool
  storage_bucket = incus_storage_bucket.test1.name
}

Import existing Storage Bucket and Key

resource "incus_storage_bucket" "bucket2" {
  name = "bucket2"
  pool = "default"
}

import {
  id = "default/default/bucket2" # project/pool/bucket_name
  to = incus_storage_bucket.bucket2
}

resource "incus_storage_bucket_key" "admin" {
  name           = "admin"
  role           = "admin"
  description    = "Admin User"
  pool           = incus_storage_bucket.bucket2.pool
  storage_bucket = incus_storage_bucket.bucket2.name
}

import {
  id = "default/default/bucket2/admin"  # project/pool/bucket_name/key_name
  to = incus_storage_bucket_key.admin
}

Open Questions

I would like to gather feedback and opinions on how to handle the following aspect:

Admin Key Creation

Incus always creates an admin key when a storage bucket is created. I would like to know how to approach this situation:

  • Option 1: Ignore the fact that an admin key is always created and the user should use the Terraform import function to make the admin key usable within Terraform.

  • Option 2: Expose the admin key on the storage bucket itself as the computed values admin_access_key and admin_secret_key, if the admin key is available.

At the moment I prefer "Option 1" to avoid additional implementation logic in the Terraform provider, as it could be that the admin token was deleted by an operator after the bucket was created. This means that the admin_access_key and admin_secret_key would no longer exist, which could lead to confusion.

Please share your thoughts and preferences regarding this matter.

@maveonair maveonair changed the title WIP: Introduce Storage Bucket management [DRAFT] Introduce Storage Bucket management Dec 17, 2023
@maveonair maveonair changed the title [DRAFT] Introduce Storage Bucket management [Draft] Introduce Storage Bucket management Dec 17, 2023
@maveonair maveonair changed the title [Draft] Introduce Storage Bucket management [Draft] Introduce Storage Bucket Resource Dec 17, 2023
@maveonair
Copy link
Member Author

maveonair commented Dec 17, 2023

@mdavidsen @adamcstephens I would appreciate your feedback 😊

@maveonair maveonair added Documentation Documentation needs updating Feature New feature, not a bug labels Dec 17, 2023
@maveonair maveonair force-pushed the storage-buckets branch 2 times, most recently from 8b870f0 to efd1897 Compare December 18, 2023 08:27
@maveonair maveonair removed the Documentation Documentation needs updating label Dec 18, 2023
@maveonair maveonair marked this pull request as ready for review December 18, 2023 09:03
@maveonair maveonair changed the title [Draft] Introduce Storage Bucket Resource Introduce Storage Bucket Resource Dec 18, 2023
Signed-off-by: Fabian Mettler <dev@maveonair.com>
Copy link
Contributor

@adamcstephens adamcstephens left a comment

Choose a reason for hiding this comment

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

I'm not very experienced with the storage buckets. If the bucket is cluster node targeted, does the key need to be as well?


// Computed.

"location": schema.StringAttribute{
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this needed? This should be retrievable through target, and in fact both are set from the same value below.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have been following the implementation of the resource_storage volume. If there is no added value by exporting this attribute, that I would suggest we remove it?

@maveonair
Copy link
Member Author

I'm not very experienced with the storage buckets. If the bucket is cluster node targeted, does the key need to be as well?

As I can see, the location information is part of the storage bucket:

$ incus storage bucket show default bucket1
config: {}
description: ""
name: bucket1
s3_url: ""
location: none

but not set for a storage bucket key:

$ incus storage bucket key show default bucket1 admin
description: Admin user
role: admin
access-key: .....
secret-key: .....
name: admin

@stgraber could you please provide some details here?

@stgraber
Copy link
Member

The storage bucket key doesn't have a location because it's tied to a specific storage bucket which does have a location.

Storage buckets are location-specific on most storage pool drivers. The exception to that being when they are on a remote storage pool such as ceph.

@mdavidsen mdavidsen self-requested a review December 18, 2023 21:28
Copy link
Member

@mdavidsen mdavidsen left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@maveonair maveonair merged commit 91ee1e6 into lxc:main Dec 18, 2023
10 checks passed
@maveonair maveonair deleted the storage-buckets branch December 18, 2023 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New feature, not a bug
Development

Successfully merging this pull request may close these issues.

4 participants