apigee / terraform-modules

Terraform modules and ready to use end-to-end examples for Apigee.
Apache License 2.0
54 stars 61 forks source link

Sample "x-l7xlb" causes an invalid key lookup #137

Open V0idC0de opened 11 months ago

V0idC0de commented 11 months ago

Using the samples/x-l7xlb template, I tried to setup an Apigee environment in my GCP organization. I used the sample settings in https://github.com/apigee/terraform-modules/blob/main/samples/x-l7xlb/x-demo.tfvars, only filling the necessary project variables to use my existing project.

Running terraform apply showed an error after creating the Apigee instance, saying:

Error: Invalid index

  on main.tf line 75, in module "apigee-x-bridge-mig":
  75:   endpoint_ip = module.apigee-x-core.instance_endpoints[each.key]
    ├────────────────
    │ each.key is "euw1-instance"
    │ module.apigee-x-core.instance_endpoints is map of string with 1 element

The given key does not identify an element in this collection value.

Looking how the values flow and that the phrase "euw1-instance" is only mentioned as key in the apigee_instances map variable, this key is apparently lost in the process of creating the instance and is not a valid key in the map of instances eventually returned by apigee-x-core module.

Checking my GCP Console, I noticed that the instance is called instance-europe-west1. which is weird, since obviously, the instance is expected to be names like the key here. Digging into the code, I think I found a bug in where the name is retrieved from, after creating the instance.

  1. x-l7xlb#main.tf#L64 passes the apigee_instances object right into the module, so the map is name => instance object
  2. apigee-x-core's main.tf#L19 transforms this into a map of region => instance object. Do note, that this instance object does NOT transfer the key of apigee_instances into a name field, which becomes important, once the data is passed into the apigee module.
  3. local.instances is passed into the apigee module here in line 83
  4. Module apigee main.tf#L78 is a loop to create one Apigee instance per object in the var.instances retrieved from outside. Using coalesce() is checks whether the each.value object has a name attribute and if not, assigns the default name instance-${each.key}, which causes the default name instance-europe-west1 observed earlier.
  5. Instance creation works fine nonetheless, since the module accounts for the lack of an explicit instance name - however, at this point, the instances all retrieved a standard name instead of the key value "euw1-instance" previously set in the configuration file, so these pieces of information diverged.
  6. output.tf of apigee-x-core now returns the instance objects (correctly containing the standard names in the instance objects) as output.

Bug happens now:x-l7xlb's main.tf looks like this:

module "apigee-x-bridge-mig" {
  for_each    = var.apigee_instances
  source      = "../../modules/apigee-x-bridge-mig"
  project_id  = module.project.project_id
  network     = module.vpc.network.id
  subnet      = module.vpc.subnet_self_links[local.subnet_region_name[each.value.region]]
  region      = each.value.region
  endpoint_ip = module.apigee-x-core.instance_endpoints[each.key]
}

It utilizes var.apigee_instances to perform a loop. The desired instance names (like "euw1-instance") are the keys and the instance object is the value. However, the endpoint_ip line now tries to retrieve the endpoint_ip of the actual instances, which were just created, by their name in the return value of module.apigee-x-core whose keys are filled with the instances' names - the standard names.

Since the desired instance names from the configuration file were omitted between the apigee-x-core and apigee modules, causing the names to be set by the apigee module itself, the keys in var.apigee_instances cannot be used to retrieve the actual instance objects from the map.

There are multiple ways to fix this.

  1. for_each uses module.apigee-x-core.instance_map (region =>instance object) as iterator. This changes the subnet and region lines, as each.value.region is now retrievable as each.key (may also be contained in the instance object; didn't check). Also, endpoint_ip uses module.apigee-x-core.instance_endpoints[each.value.name] as index.
  2. Make apigee-x-core pass the keys of the apigee_instances object into the instance objects as name field. This should cause apigee to properly recognize the explicit assignment of an instance name and prevent it from falling back to the default instance name. In addition, this prevents the desired instance name from apigee_instances' keys and the actual instance names from diverging.

I prefer approach 1, since it sources the instance names from the module that creates the instances itself and returns their final names, instead of relying on the instances having a specific name from a config file key. It's just receiving the data directly from the source. Edit: After testing this, I realized that terraform plan will fail, since the for_each argument is dynamic and cannot be determined at plan-time. So I prefer approach 2 of adding name = key in apigee-x-core#main.tf @ line 20 to pass the keys of apigee_instances as name field in the local.instances values.

Edit2: I also noticed that the output value instance_endpoints of apigee-x-core is documented to be a map of instance name => instance endpoint IP (see here). However, after applying solution 2 and using the hostname as a key, I still received the error saying that "the key doesn't identify an object in the map". Printing the object revealed that instance_endpoints is actually mapping region => instance endpoint IP, contrary to what the documentation says.

I stumbled upon this when testing for a project of mine, ~so I'll add a pull request, when I have the time and can properly test it. As I'm quite new to Terraform with GCP and this module family, I couldn't provide one along with this report.~ If I overlooked something and caused the bug myself, I'm happy to be corrected :)

Edit3: Pull request added.