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

[QT-601] Nest scenario step variables in eval context #107

Merged
merged 1 commit into from
Sep 15, 2023
Merged

Conversation

ryancragun
Copy link
Collaborator

@ryancragun ryancragun commented Sep 14, 2023

Previously we'd expose any know scenario step variables at the top-level
of a steps entry in the eval context. This leads to problems if a
Terraform module has both an input and output with the same name.
Consider the following scenario:

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

module "worker" {
  source = "./modules/worker"
}

variable "addr" {
  type = string
  default = "http://192.168.0.1/"
}

scenario "boundary" {
  step "cluster" {
    module = module.cluster
    variables {
      addr = var.addr
    }
  }

  step "worker" {
    module = module.worker
    variables {
      upstream_addr = step.cluster.addr
    }
  }

  step "worker_downstream" {
    module = module.worker
    variables {
      upstream_addr = step.worker.upstream_addr
    }
  }
}

We expect expect step.cluster.addr to be the known value of of
var.addr. But what happens with step worker here is the problem.
step.worker.upstream_addr should be module call to step.cluster.addr
but because we've exposed step.cluster.addr in the eval context we
don't do a module call, and instead inherit the value from var.addr.
The same happens with step.worker_downstream.upstream_addr. Instead of
being a module call to step.worker.upstream_addr we inherit the know
value of var.addr.

With this change that behavior is now default. If you wish to use the
prior behavior (getting a steps known variables) you can access them
with the variables key, e.g. step.cluster.variables.addr.

NOTE: this is technically a breaking change in behavior. We'll want to test all of Vault
before we release this. If we do run into issues we can modify scenarios to use the
variables option if they want old behavior.

  • The commit message includes an explanation of the changes
  • Manual validation of the changes have been performed (if possible)
  • New or modified code has requisite test coverage (if possible)
  • I have performed a self-review of the changes
  • I have made necessary changes and/or pull requests for documentation
  • I have written useful comments in the code

@ryancragun ryancragun added the changelog/bug Fix for something that wasn't working. Will be included in "Bug Fixes" category in release notes. label Sep 14, 2023
@ryancragun ryancragun requested a review from a team as a code owner September 14, 2023 23:34
@@ -63,12 +63,12 @@ scenario "step_vars" {

output "module_default" {
description = "a known value inherited through module defaults"
value = step.infra_default.az
value = step.infra_default.variables.az
Copy link

@moduli moduli Sep 15, 2023

Choose a reason for hiding this comment

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

Oh neat. I didn't know you could do this. To clarify, this is just taking the value that you're inputting into step infra_default? I have been doing something like...

  locals {
    network_cluster  = "e2e_cluster"
  }

  step "create_docker_network_cluster" {
    module = module.docker_network
    variables {
      network_name = local.network_cluster
    }
  }

  step "create_boundary" {
    module = module.docker_boundary
    depends_on = [
      step.create_docker_network_cluster,
    ]
    variables {
      network_name     = [local.network_cluster]
    }
  }

but I could change that to...

  step "create_docker_network_cluster" {
    module = module.docker_network
    variables {
      network_name = "e2e_cluster"
    }
  }

  step "create_boundary" {
    module = module.docker_boundary
    depends_on = [
      step.create_docker_network_cluster,
    ]
    variables {
      network_name     = [step.create_docker_network_cluster.variables.network_name]
    }
  }

Is that correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

you can after this ships!

Previously we'd expose any know scenario step variables at the top-level
of a steps entry in the eval context. This leads to problems if a
Terraform module has both an input and output with the same name.
Consider the following scenario:

```hcl
module "cluster" {
  source = "./modules/cluster"
}

module "worker" {
  source = "./modules/worker"
}

variable "addr" {
  type = string
  default = "http://192.168.0.1"
}

scenario "boundary" {
  step "cluster" {
    module = module.cluster
    variables {
      addr = var.addr
    }
  }

  step "worker" {
    module = module.worker
    variables {
      upstream_addr = step.cluster.addr
    }
  }

  step "worker_downstream" {
    module = module.worker
    variables {
      upstream_addr = step.worker.upstream_addr
    }
  }
}
```

We expect expect `step.cluster.addr` to be the known value of of
`var.addr`. But what happens with step worker here is the problem.
`step.worker.upstream_addr` should be module call to `step.cluster.addr`
but because we've exposed `step.cluster.addr` in the eval context we
don't do a module call, and instead inherit the value from `var.addr`.
The same happens with `step.worker_downstream.upstream_addr`. Instead of
being a module call to `step.worker.upstream_addr` we inherit the know
value of `var.addr`.

With this change that behavior is now default. If you wish to use the
prior behavior (getting a steps known variables) you can access them
with the `variables` key, e.g. `step.cluster.variables.addr`.

- [x] The commit message includes an explanation of the changes
- [x] Manual validation of the changes have been performed (if possible)
- [x] New or modified code has requisite test coverage (if possible)
- [x] I have performed a self-review of the changes
- [x] I have made necessary changes and/or pull requests for documentation
- [x] I have written useful comments in the code

Signed-off-by: Ryan Cragun <[email protected]>
@ryancragun ryancragun merged commit 547ee1a into main Sep 15, 2023
5 checks passed
@ryancragun ryancragun deleted the ryan/qt-601 branch September 15, 2023 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog/bug Fix for something that wasn't working. Will be included in "Bug Fixes" category in release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants