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

Deprecate name_prefix #1054

Closed
danawillow opened this issue Feb 6, 2018 · 19 comments
Closed

Deprecate name_prefix #1054

danawillow opened this issue Feb 6, 2018 · 19 comments

Comments

@danawillow
Copy link
Contributor

Right now, a few of our resources support specifying the name in multiple ways:

  • A name field, which is for the name explicitly
  • A name_prefix field, which specifies a prefix and generates a random string for the rest
  • Setting neither, which generates a random string

Because of the existence of the random provider, we don't need to have this logic inside the google resources, and getting rid of it will make the entire provider more consistent (since only some resources support name_prefix anyway).

We can do this safely in two steps:

  • Add import capability to the random_id resource to allow setting its output to a user-specified value. This would allow users to transition to the random_id resource without needing to recreate their google resources, allowing them to do that transition on their own time instead of waiting for an opportunity when the resource would need to be recreated to make the change.
  • Deprecate the name_prefix field and make it computed so users can remove it from their configs without recreating the resource

@rosbo @paddycarver to make sure this checks out based on our conversation

@rosbo
Copy link
Contributor

rosbo commented Feb 6, 2018

👍

@danawillow
Copy link
Contributor Author

danawillow commented Feb 8, 2018

@sl1pm4t here's what I'm thinking about for your scenario:

variable "machine_type" {}

resource "google_container_cluster" "is-1035" {
  name               = "is-1035"
  zone               = "us-central1-a"
  initial_node_count = 1
}

resource "random_id" "np" {
  byte_length = 2
  keepers = {
    machine_type = "${var.machine_type}"
  }
}

resource "google_container_node_pool" "is-1035" {
  name               = "is-1035-${random_id.np.hex}"
  zone               = "us-central1-a"
  cluster            = "${google_container_cluster.is-1035.name}"
  node_count         = 1

  node_config {
    machine_type = "${var.machine_type}"
  }

  lifecycle {
    create_before_destroy = true
  }
}

The keepers parameter in random_id gives a map of values that cause the random id to be regenerated, so by tying it to attributes that you think might change, it'll make sure the random id changes too.

@sl1pm4t
Copy link
Contributor

sl1pm4t commented Feb 8, 2018

Yeah, that could be a pretty elegant solution @danawillow.
I'll try it out, thanks!

@paddycarver
Copy link
Contributor

That matches my understanding.

@sl1pm4t
Copy link
Contributor

sl1pm4t commented Feb 9, 2018

@danawillow I tried out the alternative and it works well. Thanks!

@danawillow
Copy link
Contributor Author

I tested this flow out with a build of the random provider with my changes and it works:

resource "google_compute_instance_template" "foobar" {
  # name_prefix = "is-1054-"
  name         = "${random_id.rand.dec}"
  machine_type = "n1-standard-1"

  disk {
    source_image = "debian-8-jessie-v20160803"
    auto_delete  = true
    boot         = true
  }

  network_interface {
    network = "default"
  }
}

resource "random_id" "rand" {
  byte_length = 11
  prefix      = "is-1054-"
}

where originally, name_prefix was filled in and not name.

Imported random_id.rand with the following command:

terraform import random_id.rand is-1054-,ELFZ1rbrAThoeQE

The base64 encoding came from this script: https://play.golang.org/p/9KrkDoxRTOw
The value of "val" in the script came from running terraform show and looking for the value of name:

google_compute_instance_template.foobar:
  ...
  name = is-1054-20180329213336514500000001

@danawillow
Copy link
Contributor Author

Fixed in #1035

@negz
Copy link
Contributor

negz commented Apr 12, 2018

@danawillow I'm in the process of updating my google_compute_instance_templates to account for this new pattern. We use create_before_destroy = true on our templates per this advice.

If I'm not mistaken, given instance templates are immutable, the combination of create_before_destroy = true and the random_id name pattern means we must set every variable that could ever change on an instance template via the random_id's keepers block. That seems to be in the order of ~10 variables for each of our instance templates.

I appreciate the goal of simpler code, and the idea of reusable config blocks rather than bespoke and spotty name_prefix support, but I have to say this new pattern feels quite awkward/unintuitive for what I imagine is a pretty common use case.

Here's an example:

resource "random_id" "template" {
  byte_length = 11
  prefix      = "${local.pool_id}-"

  // This randomly generated ID is stable until one of the below variables
  // changes. This means any variable that should trigger the creation of a new
  // instance template when it changes must be defined here and read 'through'
  // this block from the below google_compute_instance_template.
  keepers = {
    machine_type          = "${var.machine_type}"
    service_account_email = "${google_service_account.account.email}"
    subnetwork            = "${var.gcp_shared_vpc_subnet}"
    subnetwork_project    = "${var.gcp_shared_vpc_project}"
    subnetwork_range_name = "${var.gcp_shared_vpc_secondary_ip_range}"
    source_image          = "${var.image}"
    disk_type             = "${var.disk_type}"
    disk_size_gb          = "${var.disk_size}"
    tag_region            = "${data.google_client_config.current.region}"
    metadata_user-data    = "${data.ignition_config.config.rendered}"
  }
}

resource "google_compute_instance_template" "template" {
  name = "${random_id.template.dec}"

  // Instance templates cannot be updated. They must instead be destroyed and
  // replaced. This block instructs Terraform that it must successfully create
  // the updated template before it destroys the old one.
  lifecycle = {
    create_before_destroy = true
  }

  machine_type         = "${random_id.template.keepers.machine_type}"
  instance_description = "Kubernetes control instance for cluster ${var.cluster_id}"

  service_account {
    email  = "${random_id.template.keepers.service_account_email}"
    scopes = ["https://www.googleapis.com/auth/cloud-platform"]
  }

  // Ensure we can pull from GCR at boot time.
  depends_on = ["google_project_iam_member.allow_gcr_pull"]

  // Our primary interface is in the shared VPC, and can access the internet via
  // its ephemeral external IP.
  network_interface {
    subnetwork         = "${random_id.template.keepers.subnetwork}"
    subnetwork_project = "${random_id.template.keepers.subnetwork_project}"

    // This is Terraform for 'give this NIC an external IP too please'.
    access_config {}

    alias_ip_range {
      subnetwork_range_name = "${random_id.template.keepers.subnetwork_range_name}"
      ip_cidr_range         = "/24"
    }
  }

  // Required to accept packets for Alias IP ranges.
  can_ip_forward = true

  disk {
    boot         = true
    source_image = "${random_id.template.keepers.source_image}"
    disk_type    = "${random_id.template.keepers.disk_type}"
    disk_size_gb = "${random_id.template.keepers.disk_size_gb}"
  }

  // We use a local SSD for /var/lib/docker, i.e. where Docker stores images,
  // containers, etc. Local SSDs are faster and cheaper than pd-ssds, but
  // prevent us from being able to restart the instance they are attached to.
  // Local SSDs are always 375GB.
  // https://cloud.google.com/compute/docs/disks/local-ssd
  disk {
    device_name = "varlibdocker" // Will be exposed at /dev/varlibdocker
    disk_type   = "local-ssd"
    type        = "SCRATCH"
  }

  tags = [
    "${var.cluster_id}",
    "${local.pool_id}",
    "terraflop",
    "kubernetes",

    // Required to route traffic to instances outside us-central1
    "${random_id.template.keepers.tag_region}",
  ]

  labels = {
    component = "kubernetes"
    cluster   = "${var.cluster_id}"
  }

  metadata = {
    component = "kubernetes"
    cluster   = "${var.cluster_id}"

    block-project-ssh-keys = "TRUE"

    user-data = "${random_id.template.keepers.metadata_user-data}"
  }
}

@negz
Copy link
Contributor

negz commented Apr 12, 2018

If other folks find themselves here, you might want to be aware of hashicorp/terraform-provider-random#21. It seems that reading variables 'through' the random_id's keepers can fail sporadically.

@matschaffer
Copy link

matschaffer commented Apr 20, 2018

@negz great writeup. I've run into hashicorp/terraform-provider-random#21 as well. And also noticed that it doesn't seem random_id knows what to do with keepers of type list which I tried when assigning service_account.scopes to an instance template.

So far it seems like the best option is to read the same values I pass into the random_id keepers.

@nyurik
Copy link
Contributor

nyurik commented Jul 3, 2018

Hi, thx for the good write up. The main HashiCorp docs on google_compute_ssl_certificate still uses name_prefix in all of its examples. What would be the best text to replace it with? Has it been written for any other related providers? Thanks!

I used this code:

resource "random_id" "certificate" {
  byte_length = 4
  prefix      = "my-certificate-"
}

resource "google_compute_ssl_certificate" "default" {
  name        = "${random_id.certificate.hex}"
  private_key = "${var.domain_key}"
  certificate = "${var.domain_cert}"

  lifecycle {
    create_before_destroy = true
  }
}

@Chupaka
Copy link
Contributor

Chupaka commented Jul 3, 2018

@nyurik, what if random_id.hex begins with a number (0-9), not letter (a-f)? Names can't start with a number, AFAIR.

@nyurik
Copy link
Contributor

nyurik commented Jul 3, 2018

@Chupaka there is a prefix, so the name would be "my-certificate-0A9C3B2F" - which i think is perfectly legit

@Chupaka
Copy link
Contributor

Chupaka commented Jul 3, 2018

@nyurik ah, sorry, misread :)

@danawillow
Copy link
Contributor Author

Hey @nyurik, we actually undeprecated name_prefix for ssl certificate: #1622

@nyurik
Copy link
Contributor

nyurik commented Jul 3, 2018

@danawillow thx! We must have the older version. Would it still make sense to mention both approaches on the ssl docs page? I wasn't even aware of the random id generation until I saw the deprecation warning, and also when i tried name_prefix, I ran into the ID length issue

@danawillow
Copy link
Contributor Author

Totally! If you open a PR with the proposed changes, I'd be happy to review it :)

@nyurik
Copy link
Contributor

nyurik commented Jul 4, 2018

@danawillow thx, I created a PR at #1731

@ghost
Copy link

ghost commented Nov 17, 2018

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

@ghost ghost locked and limited conversation to collaborators Nov 17, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

8 participants