Fix OpenStack Terraform group_vars_path handling#13202
Conversation
|
|
Welcome @pushpak-23! |
|
Hi @pushpak-23. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: pushpak-23 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
67af2a6 to
9c3d4aa
Compare
There was a problem hiding this comment.
Pull request overview
Fixes OpenStack Terraform inventory group_vars_path handling so no_floating.yml generation is less likely to fail when Terraform is executed with terraform -chdir, improving reliability of deployments involving bastions and/or nodes without floating IPs.
Changes:
- Update
group_vars_pathvariable description to recommend using an absolute path when usingterraform -chdir. - Pre-create
${var.group_vars_path}viamkdir -pbefore writingno_floating.ymlinlocal-execprovisioners.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| contrib/terraform/openstack/variables.tf | Clarifies intended usage of group_vars_path under terraform -chdir. |
| contrib/terraform/openstack/modules/compute/main.tf | Ensures the target directory exists before writing no_floating.yml during instance provisioning. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| provisioner "local-exec" { | ||
| command = "sed -e s/USER/${var.ssh_user}/ -e s/BASTION_ADDRESS/${var.bastion_fips[0]}/ ${path.module}/ansible_bastion_template.txt > ${var.group_vars_path}/no_floating.yml" | ||
| command = "mkdir -p ${var.group_vars_path} && sed -e s/USER/${var.ssh_user}/ -e s/BASTION_ADDRESS/${var.bastion_fips[0]}/ ${path.module}/ansible_bastion_template.txt > ${var.group_vars_path}/no_floating.yml" |
There was a problem hiding this comment.
local-exec is interpolating ${var.group_vars_path} unquoted in both mkdir and the output redirection. If the path contains spaces or shell metacharacters (or starts with -), this can break the command or be unsafe. Quote the path (and consider mkdir -p -- ...) so directory creation and the > .../no_floating.yml target are robust.
| command = "mkdir -p ${var.group_vars_path} && sed -e s/USER/${var.ssh_user}/ -e s/BASTION_ADDRESS/${var.bastion_fips[0]}/ ${path.module}/ansible_bastion_template.txt > ${var.group_vars_path}/no_floating.yml" | |
| command = "mkdir -p -- \"${var.group_vars_path}\" && sed -e s/USER/${var.ssh_user}/ -e s/BASTION_ADDRESS/${var.bastion_fips[0]}/ ${path.module}/ansible_bastion_template.txt > \"${var.group_vars_path}/no_floating.yml\"" |
| provisioner "local-exec" { | ||
| command = "%{if each.value.floating_ip}sed s/USER/${var.ssh_user}/ ${path.module}/ansible_bastion_template.txt | sed s/BASTION_ADDRESS/${element(concat(var.bastion_fips, [for key, value in var.k8s_masters_fips : value.address]), 0)}/ > ${var.group_vars_path}/no_floating.yml%{else}true%{endif}" | ||
| command = "%{if each.value.floating_ip}mkdir -p ${var.group_vars_path} && sed s/USER/${var.ssh_user}/ ${path.module}/ansible_bastion_template.txt | sed s/BASTION_ADDRESS/${element(concat(var.bastion_fips, [for key, value in var.k8s_masters_fips : value.address]), 0)}/ > ${var.group_vars_path}/no_floating.yml%{else}true%{endif}" | ||
| } |
There was a problem hiding this comment.
With mkdir -p ${var.group_vars_path} added, a relative group_vars_path under terraform -chdir will now silently create a new ./group_vars directory under the Terraform config directory and write no_floating.yml there, rather than failing fast. That can mask misconfiguration and later cause Ansible to miss the intended inventory/$CLUSTER/group_vars/no_floating.yml. Consider either (a) enforcing an absolute path via variable validation or (b) making the docs explicitly call out that relative paths are resolved relative to the -chdir working directory and may write files into the Terraform module directory.
|
/ok-to-test |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
/kind bug
What this PR does / why we need it:
Fixes an issue in the OpenStack Terraform provider where
local-execprovisioners writingno_floating.ymlfail when Terraform is executed usingterraform -chdir.The current implementation writes:
> ${var.group_vars_path}/no_floating.ymlwith the default:
group_vars_path = "./group_vars"When Terraform is run from
contrib/terraform/openstackusing-chdir, the relative path resolves from the Terraform working directory instead of the inventory directory, causing:cannot create ./group_vars/no_floating.yml: Directory nonexistentThis PR makes the behavior safer by:
mkdir -p ${var.group_vars_path}before allsedcommands that generateno_floating.ymlgroup_vars_pathvariable description to clarify that absolute paths are recommended because relative paths may fail withterraform -chdirThis prevents failures during bastion and node creation and makes the OpenStack Terraform workflow more reliable.
Which issue(s) this PR fixes:
Fixes #13201
Special notes for your reviewer:
Issue was reproduced while deploying Kubespray on OpenStack with an existing Neutron network and no-floating-IP worker nodes.
Terraform failed during bastion creation because
group_vars/no_floating.ymlcould not be created even though the inventory directory containedgroup_vars/.Root cause was relative path resolution under
terraform -chdir.Does this PR introduce a user-facing change?: