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!: Restricting autokey module to autokey configuration use case #163

Open
wants to merge 27 commits into
base: master
Choose a base branch
from

Conversation

nb-goog
Copy link

@nb-goog nb-goog commented Nov 11, 2024

In this PR,

  1. Restricting autokey module function to only configure and enable autokey on an input folder number. Deleted configuration responsible for creating keyhandle from the input map.
  2. Added two examples
    2.1 for enabling autokey config on a project
    2.2 for creating a bucket using autokey

@nb-goog nb-goog requested a review from a team as a code owner November 11, 2024 12:00
@nb-goog nb-goog changed the title Restricting autokey module to autokey configuration use case feat: Restricting autokey module to autokey configuration use case Nov 11, 2024
@romanini-ciandt
Copy link
Member

/gcbrun

Copy link
Member

@romanini-ciandt romanini-ciandt left a comment

Choose a reason for hiding this comment

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

Just for my own understanding, why is it better to keep google_kms_key_handle resource outside the terraform-google-kms module scope?

examples/autokey/autokey-setup/README.md Outdated Show resolved Hide resolved
examples/autokey/autokey-setup/README.md Outdated Show resolved Hide resolved
examples/autokey/autokey-setup/README.md Outdated Show resolved Hide resolved
examples/autokey/autokey-setup/README.md Outdated Show resolved Hide resolved
@nb-goog nb-goog changed the title feat: Restricting autokey module to autokey configuration use case feat!: Restricting autokey module to autokey configuration use case Nov 12, 2024
@nb-goog
Copy link
Author

nb-goog commented Nov 12, 2024

Just for my own understanding, why is it better to keep google_kms_key_handle resource outside the terraform-google-kms module scope?

KMS has two personas

  1. key admin
  2. Key users (developers, etc

key admin should only focus on key management policies and setups AND shouldnt have access to kms keys created.

The keyhandle creation and access to keys is for second persona (key users) thats why we are separating it out

@romanini-ciandt
Copy link
Member

Just for my own understanding, why is it better to keep google_kms_key_handle resource outside the terraform-google-kms module scope?

KMS has two personas

  1. key admin
  2. Key users (developers, etc

key admin should only focus on key management policies and setups AND shouldnt have access to kms keys created.

The keyhandle creation and access to keys is for second persona (key users) thats why we are separating it out

If the only motivation to cut google_kms_key_handle from KMS module is to handle the different personas, how about we implement a change that would accommodate that capability while still keep the benefits of the module?

Suggestion: we could turn google_kms_autokey_config creation optional when using autokey's submodule. google_kms_key_handle is already optional, based on user inputs. That way, key admins could use autokey's submodule passing the desired inputs to create just the Autokey Configuration, while key users could also use autokey's submodule but passing different inputs in order to just create the Key Handle.

To be more clear, the module's usage would be something like that:

# Key admins usage example
module "autokey" {
  source  = "terraform-google-modules/kms/google//modules/autokey"
  version = "3.1.0"

  project_id            = "project-bar"
  autokey_configuration_folder = "123456789"
  }
}

# Key users usage example
module "autokey" {
  source  = "terraform-google-modules/kms/google//modules/autokey"
  version = "3.1.0"

  autokey_handles = {
    storage_bucket = {
      name                   = "bucket-key-handle",
      project                = "project-foo",
      resource_type_selector = "storage.googleapis.com/Bucket",
      location               = "us-central1"
    },
    compute_disk = {
          name                   = "disk-key-handle",
          project                = "project-foo2",
          resource_type_selector = "compute.googleapis.com/Disk",
          location               = "us-central1"
    }
   # It would be possible to create multiple Key Handles here, if desired.
  }
}

Some benefits of using the KMS module approach:

  • Customers (key users) could create multiple Key Handles using a single terraform module call;
  • By using modules, customers could take leverage from "version", bringing more stability to their environments;
  • We can keep the instructions that would help customers to migrate from the old autokey module to this one;
  • We provide an opinionated way for customers to interact with terraform resources using a module, like most of the Google services have;

Let me know what you think about that!
Also, I'm available to implement this proposed change, if you think it's a good idea.

Thanks!

@romanini-ciandt
Copy link
Member

/gcbrun

@romanini-ciandt
Copy link
Member

/gcbrun

@nb-goog
Copy link
Author

nb-goog commented Nov 19, 2024

Just for my own understanding, why is it better to keep google_kms_key_handle resource outside the terraform-google-kms module scope?

KMS has two personas

  1. key admin
  2. Key users (developers, etc

key admin should only focus on key management policies and setups AND shouldnt have access to kms keys created.
The keyhandle creation and access to keys is for second persona (key users) thats why we are separating it out

If the only motivation to cut google_kms_key_handle from KMS module is to handle the different personas, how about we implement a change that would accommodate that capability while still keep the benefits of the module?

Suggestion: we could turn google_kms_autokey_config creation optional when using autokey's submodule. google_kms_key_handle is already optional, based on user inputs. That way, key admins could use autokey's submodule passing the desired inputs to create just the Autokey Configuration, while key users could also use autokey's submodule but passing different inputs in order to just create the Key Handle.

To be more clear, the module's usage would be something like that:

# Key admins usage example
module "autokey" {
  source  = "terraform-google-modules/kms/google//modules/autokey"
  version = "3.1.0"

  project_id            = "project-bar"
  autokey_configuration_folder = "123456789"
  }
}

# Key users usage example
module "autokey" {
  source  = "terraform-google-modules/kms/google//modules/autokey"
  version = "3.1.0"

  autokey_handles = {
    storage_bucket = {
      name                   = "bucket-key-handle",
      project                = "project-foo",
      resource_type_selector = "storage.googleapis.com/Bucket",
      location               = "us-central1"
    },
    compute_disk = {
          name                   = "disk-key-handle",
          project                = "project-foo2",
          resource_type_selector = "compute.googleapis.com/Disk",
          location               = "us-central1"
    }
   # It would be possible to create multiple Key Handles here, if desired.
  }
}

Some benefits of using the KMS module approach:

  • Customers (key users) could create multiple Key Handles using a single terraform module call;
  • By using modules, customers could take leverage from "version", bringing more stability to their environments;
  • We can keep the instructions that would help customers to migrate from the old autokey module to this one;
  • We provide an opinionated way for customers to interact with terraform resources using a module, like most of the Google services have;

Let me know what you think about that! Also, I'm available to implement this proposed change, if you think it's a good idea.

Thanks!

See this makes a lot more sense if Security admin and Key admin are the same person.
There is clear guidance from the PMs and Autokey customers that configuration and key creation should be clearly separated..

Your suggestion makes sense but the expectation from the perspective of the personas are quite differnt.

@nb-goog nb-goog closed this Nov 19, 2024
@nb-goog nb-goog reopened this Nov 19, 2024
@romanini-ciandt
Copy link
Member

/gcbrun

@romanini-ciandt
Copy link
Member

/gcbrun

@nb-goog
Copy link
Author

nb-goog commented Dec 2, 2024

/gcbrun

@nb-goog
Copy link
Author

nb-goog commented Dec 2, 2024

/gcbrun

could you please re-run

@nb-goog
Copy link
Author

nb-goog commented Dec 2, 2024

/gcbrun

could you please rerun

@romanini-ciandt
Copy link
Member

/gcbrun

@romanini-ciandt
Copy link
Member

/gcbrun

@romanini-ciandt
Copy link
Member

/gcbrun

@nb-goog nb-goog requested a review from apeabody December 6, 2024 14:16
Copy link

@tdbhacks tdbhacks left a comment

Choose a reason for hiding this comment

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

Mostly minor changes, @romanini-ciandt mind taking a quick look as well to check if you see anything else?

.terraform.lock Outdated Show resolved Hide resolved
examples/bucket_setup_using_autokey/variables.tf Outdated Show resolved Hide resolved
examples/bucket_setup_using_autokey/variables.tf Outdated Show resolved Hide resolved
modules/autokey/variables.tf Show resolved Hide resolved
test/setup/outputs.tf Outdated Show resolved Hide resolved
modules/autokey/README.md Outdated Show resolved Hide resolved
examples/bucket_setup_using_autokey/outputs.tf Outdated Show resolved Hide resolved
examples/autokey_example/main.tf Outdated Show resolved Hide resolved
examples/bucket_setup_using_autokey/main.tf Outdated Show resolved Hide resolved
examples/bucket_setup_using_autokey/outputs.tf Outdated Show resolved Hide resolved
examples/bucket_setup_using_autokey/variables.tf Outdated Show resolved Hide resolved
examples/bucket_setup_using_autokey/variables.tf Outdated Show resolved Hide resolved
@romanini-ciandt
Copy link
Member

/gcbrun

@romanini-ciandt
Copy link
Member

/gcbrun

Copy link

@tdbhacks tdbhacks left a comment

Choose a reason for hiding this comment

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

This LGTM, I'll defer to:

  • @romanini-ciandt to do one last pass and approve
  • the CFT team re: breaking change, I believe Leo will be summarizing the changes and looping them in, so they're aware and can give final approval.

@nb-goog nb-goog removed the request for review from erlanderlo December 13, 2024 04:04
Copy link
Member

@romanini-ciandt romanini-ciandt left a comment

Choose a reason for hiding this comment

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

The terraform code looks good, I just can't evaluate the decisions made in order to generate this breaking change.

As @tdbhacks mentioned, I'll summarize the changes and involve CFT team so they can give the final approval.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants