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

Optional Automatic Pass-Through of Module Variables #15818

Closed
chriswhelix opened this issue Aug 15, 2017 · 20 comments
Closed

Optional Automatic Pass-Through of Module Variables #15818

chriswhelix opened this issue Aug 15, 2017 · 20 comments

Comments

@chriswhelix
Copy link

Description

This issue proposes adding a boolean "pass_through_vars" option to module{} blocks. When present and set to true, all variables defined in the module but not explicitly passed to the module would have the special behavior that:

  • The variable value of the same name from the caller would be automatically passed to the module
  • If no variable definition of the same name exists in the caller, the definition would be implicitly copied from the module

Use Case 1: Variables Shared by Many Modules

Here's an abbreviated, but real, example of one of our configs:

// stacks/go_service/main.tf
module "ecs_service" {
  source = "../../modules/ecs_service"
  environment                        = "${var.environment}"
  region                             = "${var.region}"
}
variable "environment" {
  description = "Helix environment name"
}
variable "region" {
  description = "AWS region"
}

// modules/ecs_service/main.tf
module "region_environment" {
  source              = "../region_environment"
  region              = "${var.region}"
  environment         = "${var.environment}"
}
variable "environment" {
  description = "Helix environment name"
}
variable "region" {
  description = "AWS region"
}

// modules/region_environment/main.tf
module "region" {
  source = "../region"
  region = "${var.region}"
}
variable "environment" {
  description = "Helix environment name"
}
variable "region" {
  description = "AWS region"
}

// modules/region/main.tf
variable "region" {
  description = "AWS region"
}

What I'd like to write instead is:

// stacks/go_service/main.tf
module "ecs_service" {
  source = "../../modules/ecs_service"
  pass_through_vars = true
}

// modules/ecs_service/main.tf
module "region_environment" {
  source              = "../region_environment"
  pass_through_vars = true
}

// modules/region_environment/main.tf
module "region" {
  source = "../region"
  pass_through_vars = true
}
variable "environment" {
  description = "Helix environment name"
}

// modules/region/main.tf
variable "region" {
  description = "AWS region"
}

Use Case 2: Many Variables That Exist Only for Passing to a Module

The above illustrates the vertical dimension of the problem; here's another real-world example, taken from the same config, of the horizontal dimension of the problem:

// Full module entry from stacks/go_service/main.tf as mentioned above
module "ecs_service" {
  source = "../../modules/ecs_service"

  cluster_arn             = "${module.ecs_cluster.arn}"
  launch_configuration    = "${aws_launch_configuration.default.name}"
  instance_role_name      = "${module.launch_configuration_defaults.instance_role_name}"
  instance_security_group = "${module.launch_configuration_defaults.instance_security_group}"

  account                            = "${var.account}"
  adjust_task_count                  = "${var.adjust_task_count}"
  alb_idle_timeout                   = "${var.alb_idle_timeout}"
  alb_visibility                     = "${var.alb_visibility}"
  authorized_users                   = "${var.authorized_users}"
  availability_zones                 = "${var.availability_zones}"
  build_artifact                     = "${var.build_artifact}"
  container_name                     = "${var.name}"
  container_port                     = "${var.container_port}"
  deploy_task_arn                    = "${coalesce(var.deploy_task_arn, aws_ecs_task_definition.default.arn)}"
  deployment_maximum_percent         = "${var.deployment_maximum_percent}"
  deployment_minimum_healthy_percent = "${var.deployment_minimum_healthy_percent}"
  desired_server_count               = "${var.desired_server_count}"
  elb_listener_on_port_80_flag       = "${var.elb_listener_on_port_80_flag}"
  environment                        = "${var.environment}"
  external_target                    = "${var.external_target}"
  health_check_path                  = "${var.health_check_path}"
  max_server_count                   = "${var.max_server_count}"
  memory                             = "${var.memory}"
  min_server_count                   = "${var.min_server_count}"
  name                               = "${var.name}"
  override_route53_name              = "${var.override_route53_name}"
  region                             = "${var.region}"
  remote_state_bucket                = "${var.remote_state_bucket}"
}

This could turn into simply:

module "ecs_service" {
  source = "../../modules/ecs_service"
  pass_through_vars = true

  cluster_arn             = "${module.ecs_cluster.arn}"
  launch_configuration    = "${aws_launch_configuration.default.name}"
  instance_role_name      = "${module.launch_configuration_defaults.instance_role_name}"
  instance_security_group = "${module.launch_configuration_defaults.instance_security_group}"

  container_name                     = "${var.name}"
  deploy_task_arn                    = "${coalesce(var.deploy_task_arn, aws_ecs_task_definition.default.arn)}"
}

Related Issues

#5480 and many older tickets attempt to solve the same problem by adding global variables; that approach has (IMO rightly) been consistently rejected.

#1478 requests "include" functionality, which would address the variable-definition-DRYness aspect of this problem, albeit in a less maintainable way. We are getting similar behavior now by using symlinks to carefully arranged variable definition files, and it's not awesome.

#15449 adds local variables, allowing composite configuration to be passing into a module in a single line. That is a valuable feature that would complement this proposal well, but does not provide an elegant solution to the exact challenges shown in the above use cases.

Advantages of This Approach

  • Modules remain properly encapsulated; no new opportunity is created for modules to produce unexpected behavior.
  • No changes are needed to module definitions for a module caller to take advantage of the feature; nor can modules be constructed in a way that would require callers to use the feature if they don't want to.
  • Modules retain their existing clearly-defined input interfaces, with the ability of the interpreter to immediately detect missing inputs (and, in some situations, interactively prompt for them).
  • Maximal DRYness is achieved for both variable definition and module invocation in the envisioned use cases.

Avoiding Variable Definition Ambiguity

There is a possibility for ambiguity where two modules are invoked using pass_through_vars and both modules provide a definition for variables with the same name. This is particularly significant in relation to default values. I propose the following rules for avoiding unexpected behavior due to such ambiguity:

  1. If a definition exists for a variable in the caller, that is always used.
  2. If N modules all supply identical definitions, that definition is used (once).
  3. If two or more modules supply differing definitions, that is a configuration error that must be corrected by the user.

Development Status

I haven't begun writing any code to this effect, but am prepared to embark upon implementation if there is support for this proposal. Any suggestions with regard to implementation approach would be welcomed.

@apparentlymart
Copy link
Contributor

Hi @chriswhelix! Thanks for writing this up.

This is an interesting idea, and you're right that it avoids the main drawbacks of some other proposals.

We're actually about to embark on some pretty significant changes to the configuration language processing that would, I expect, end up in conflict with this work were you to try to do it concurrently. We're still prototyping to figure out what shape this new approach would take in the code, so if it's okay with you I'd prefer to hold off on digging into the design discussion here until this broader work is feeling more concrete, and thus we can reason better about how this new feature would fit in.

@nodesocket
Copy link

nodesocket commented Aug 15, 2017

Seems like an inherits keyword might be useful.

What if modules could inherit variables:

module "ecs_service" inherits ${var.region}, ${var.environment} {
    ...
}

@chriswhelix
Copy link
Author

@apparentlymart sounds good, thanks!

@ka4eli
Copy link

ka4eli commented Aug 19, 2017

+1 !!!
I have about 70 variables in my main source terraform dir. There are all resources and providers. Also have several modules dirs each with different number of almost the same modules (1-4 modules per dir). Each module references my main terraform dir overriding only 2 variables. All other should be passed at runtime via '-var'. But using modules, I also have to redeclare and reassign all these 70 variables in each module. So if my file has 4 modules I need to copypaste about 70*4 lines of var assignments with 70 lines of var declaration which is additional 350 lines (!!!). And I have 9 modules...
It's easy to copypaste all this stuff but not to support it after.
It would work for me if can just run module and all '-var' and 'var-file' params are passed to source terraform files.

@dmikalova
Copy link

@ka4eli Why can't maps be used for this? That way you can still explicitly call out what components a module depends on in one line per component, but not have to call out every single variable. I think adding implicit values ruins a lot of what makes Terraform's declarative nature so predictable.

@ka4eli
Copy link

ka4eli commented Oct 3, 2017

@dmikalova sorry, but don't understand what maps you've mentioned. Can you give an example?

@dmikalova
Copy link

A variable of type map - documentation.

So you could write the following and just pass 1 var per module around instead of 70 vars total:

variable "config" {
  type = "map"

  default = {
    port = "8000"
    ip = "127.0.0.1"
    otherConfig = "something"
    moreConfig = "etc"
  }
}

@ka4eli
Copy link

ka4eli commented Oct 5, 2017

With my approach I have some clumsy files where modules are defined, but everything else is pretty tidy. With your approach in every place where I use terraform variable I have to get it from map - which is additional code, efforts and readability degradation, but my files with modules will be slim and tidy. I prefer mine. Or may be I haven't understood your approach?

@chriswhelix
Copy link
Author

@apparentlymart how's the related/prerequisite work you mentioned coming along? Is there an issue I should follow on that?

@apparentlymart
Copy link
Contributor

Hi @chriswhelix,

There isn't a single issue for it since it's a stream of lots of different bits of work, but it is in progress and parts of it are already merged, but disabled. It's tough to say when it will be done since it's quite a chain of work, but I want to be honest with you that there's quite a lot there so we're probably going to be busy on this through the rest of the year at least, with initial parts of it coming sooner (in an experimental capacity) to gather feedback.

One of the things we're planning is to make it easier to pass full objects (whole resources, modules, etc) through variables and outputs, with the hope of allowing a different style where the root module instantiates a set of child modules and "wires together" the results (dependency-injection style). This is the main thing I want to have in place before we look at solutions to the "passing loads of variables around" problem; that represents a bit of a shift in usual Terraform design patterns and so I want to get a stronger idea of how Terraform usage changes with that capability to see if the use-cases change slightly as a result.

@chriswhelix
Copy link
Author

@apparentlymart OK, thanks for the update, look forward to seeing the results of that!

@lanmalkieri
Copy link

Hey @apparentlymart, just curious if you guys have an update/timeline for this feature?

Thanks!

@nirfeldman
Copy link

Is this still considered?

@ocervell
Copy link

ocervell commented Jun 4, 2018

+1 any updates ?

@davehewy
Copy link

+1 inheritance of some description would be nice. Even some notion of scope maybe?

@AsenZahariev
Copy link

+1 for method for passing variables to multiple files from one main file.

@mwarkentin
Copy link
Contributor

Is this handled by the improvements in 0.12?

https://www.hashicorp.com/blog/terraform-0-12-rich-value-types

@slayer
Copy link

slayer commented Jun 3, 2019

Any news?

@apparentlymart
Copy link
Contributor

Hi all,

As of Terraform v0.12.0, Terraform supports passing whole objects and lists of objects through variables and outputs, thus allowing the same information to be expressed as fewer variables carrying more information each.

That is our intended solution to both of these use-cases, allowing values to be passed around more concisely while retaining the explicit relationships that help the reader understand how data flows between modules.

For the first use-case, I would define an object containing all of these "global" settings and then pass it to each module that needs it as a single argument. Using the settings shown in the original example above, that might look like this:

variable "environment" {
  type = string
}
variable "region" {
  type = string
}

locals {
  context = {
    environment = var.environment
    region      = var.region
  }
}

module "region" {
  source = "../region"

  context = local.context
  # ...and any other arguments that are specific to this module
}

module "ecs-cluster" {
  source = "../../modules/ecs-cluster"

  context = local.context
  # ...and any other arguments that are specific to this module
}

For use-case two, we can potentially gather together all of the settings for a particular child module into a single object to pass, and unpack it only at the final call to produce the flat argument set.

Along with this, we recommend following a module composition style to construct configurations out of small modules that each represent a single concept, with the root module then being responsible for defining the connections and flow between its child modules. This also encourages each module to have a relatively small interface by raising the level of abstraction; if you have modules that have a large number of variables, that may be an indication that they are too broad and would be better replaced either with multiple small modules (if the problem is decomposable) or with direct use of a particular resource type (if the module is essentially just exposing the full functionality of a particular resource type anyway).

Note that you can use a module itself as an object value in Terraform 0.12, which results in an object whose attribute names are the module output names. Therefore if another module is defined as accepting a variable of a compatible object type (one which has a subset of the same argument names and types) then you can pass the whole module over as an argument, as in this real example where the "nodes" module has a cluster argument that can accept the cluster module's result in full:

module "cluster" {
  source = "../../"

  name               = local.cluster_name
  kubernetes_version = ""
  subnet_ids         = aws_subnet.test.*.id

  tags = local.tags
}

module "nodes_t2_micro" {
  source = "../../modules/node-pool"

  cluster = module.cluster # This is an object constructed from the module outputs
  name    = "${module.cluster.name}-t2micro"

  instance_type = "t2.micro"
  desired_count = 1
  scaling = {
    min = 1
    max = 1
  }

  tags = local.tags
}

The cluster variable in the "node-pool" module is defined as an object type with a set of attributes that is a subset of the set of outputs of the cluster module:

variable "cluster" {
  type = object({
    name                            = string
    vpc_id                          = string
    subnet_ids                      = set(string)
    control_plane_security_group_id = string
    node_iam_role_name              = string
    kubernetes_version              = string
  })
}

(If you want to see the full context here, see my experimental EKS cluster module, which uses module composition to declare node pools separately from the cluster itself.)

With that said, I know the features described above are not exactly what was proposed here, but we designed these features with the original use-case in mind in order to get a similar result while remaining true to the language design principle that being explicit is better than being implicit, because it allows future readers to better understand the intended behavior of a configuration and to maintain it without causing surprising side-effects in unrelated modules.

With that said, I'm going to close this out. If you run into any bugs with these new features, please open new separate issues for them so we can capture all of the necessary debugging information via the issue template.

@ghost
Copy link

ghost commented Jul 25, 2019

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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Jul 25, 2019
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