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

command: Fix init flags silent exit bug #25300

Merged
merged 1 commit into from
Jun 24, 2020

Conversation

alisdair
Copy link
Contributor

When using -flag=value with Powershell, unquoted values are broken into separate arguments. This means that the following command:

terraform init -backend-config=./backend.conf

is interpreted by Terraform as:

terraform init -backend-config= ./backend.conf

This results in an empty backend-config setting (which is semantically valid!) followed by a custom configuration path (pointing at a file).

Due to a bug where we could exit without printing diagnostics, this would result in a silent failure that was very difficult to diagnose. See #25266.

When using `-flag=value` with Powershell, unquoted values are broken
into separate arguments. This means that the following command:

  terraform init -backend-config=./backend.conf

is interpreted by Terraform as:

  terraform init -backend-config= ./backend.conf

This results in an empty backend-config setting (which is semantically
valid!) followed by a custom configuration path (pointing at a file).

Due to a bug where we could exit without printing diagnostics, this
would result in a silent failure that was very difficult to diagnose.
@alisdair alisdair requested a review from a team June 18, 2020 22:04
@alisdair alisdair self-assigned this Jun 18, 2020
@codecov
Copy link

codecov bot commented Jun 18, 2020

Codecov Report

Merging #25300 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted Files Coverage Δ
command/init.go 71.04% <100.00%> (+0.59%) ⬆️
dag/walk.go 93.16% <0.00%> (+0.62%) ⬆️
backend/remote/backend_common.go 55.25% <0.00%> (+0.67%) ⬆️
dag/marshal.go 54.44% <0.00%> (+1.11%) ⬆️
terraform/node_resource_plan.go 93.44% <0.00%> (+1.63%) ⬆️
communicator/communicator.go 83.33% <0.00%> (+3.70%) ⬆️

Copy link
Contributor

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

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

Good catch! Looks like not showing the error messages here was a result of an incorrect adaptation of an old implementation prior to diagnostics.

With that said, you got me thinking about the -backend-config= (zero-length value after =) case a little: isn't it the case that this must either be a filename or a key=value string? If so, I think that an empty string could never be either of those, and so we could potentially have a more tailored error message here noting that the option value is invalid.

This will still, of course, remain confusing under PowerShell: PowerShell's insistence on inserting unrequested spaces in the command line means that the result would still be pretty confusing, and appear as a Terraform bug rather than a PowerShell parsing quirk:

PS C:\> terraform init -backend-config=./foo

Error: Module directory ./foo does not exist

Error: -backend-config= requires an argument

Without being able to reliably detect that the calling program is PowerShell here I don't really see any robust way to do better: what the user entered and what PowerShell passes to us are different in an infuriatingly-subtle yet significant way. 😖

@alisdair
Copy link
Contributor Author

With that said, you got me thinking about the -backend-config= (zero-length value after =) case a little: isn't it the case that this must either be a filename or a key=value string? If so, I think that an empty string could never be either of those, and so we could potentially have a more tailored error message here noting that the option value is invalid.

Sadly I don't think this is possible. While I can't find where this is documented (if it is), the command interprets an empty string value as meaning "remove all previous backend-config overrides":

terraform/command/init.go

Lines 777 to 781 in 69b94ec

if len(items) == 1 && items[0].Value == "" {
// Explicitly remove all -backend-config options.
// We do this by setting an empty but non-nil ConfigOverrides.
return configs.SynthBody("-backend-config=''", synthVals), diags
}

This was introduced in #21439.

I'm also not satisfied with this current solution to the bug, but at least having some error message displayed would give the user a chance of figuring out that there's a quoting issue happening. I'm not targeting this at 0.13.0 anyway (since we're close to an RC), so maybe a better solution will occur to us before merge 🤞

@apparentlymart
Copy link
Contributor

Oh yes, I'd forgotten about that tricky overloading of -backend-config=. That is a good point and I agree.

@alisdair
Copy link
Contributor Author

I'm not targeting this at 0.13.0 anyway (since we're close to an RC)

After thinking about this some more I've changed my mind. Given the tiny scope of this change, I think it's low enough risk that it's better to merge now than leave this silent failure edge case to trip someone else up.

@alisdair alisdair merged commit 550d75f into master Jun 24, 2020
@alisdair alisdair deleted the alisdair/fix-init-flags-silent-exit branch June 24, 2020 14:33
@ghost
Copy link

ghost commented Jul 25, 2020

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, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants