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.

Sign up for GitHub

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

Make config errors more important during init operations #33628

Merged
merged 2 commits into from
Aug 9, 2023
Merged

Make config errors more important during init operations #33628

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4bfad53
make config errors more important during init operations
liamcervante Aug 3, 2023
537c315
address comments
liamcervante Aug 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 16 additions & 18 deletions internal/command/init.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,24 +262,22 @@ func (c *InitCommand) Run(args []string) int {
return 1
}

// If we pass the core version check, we want to show any errors from initializing the backend next,
// which will include syntax errors from loading the configuration. However, there's a special case
// where we are unable to load the backend from configuration or state _and_ the configuration has
// errors. In that case, we want to show a slightly friendlier error message for newcomers.
showBackendDiags := back != nil || rootModEarly.Backend != nil || rootModEarly.CloudConfig != nil
if showBackendDiags {
diags = diags.Append(backDiags)
if backDiags.HasErrors() {
c.showDiagnostics(diags)
return 1
}
} else {
diags = diags.Append(earlyConfDiags)
if earlyConfDiags.HasErrors() {
c.Ui.Error(strings.TrimSpace(errInitConfigError))
c.showDiagnostics(diags)
return 1
}
// We've passed the core version check, now we can show errors from the
// configuration and backend initialisation.

// Now, we can check the diagnostics from the early configuration.
diags = diags.Append(earlyConfDiags)
if earlyConfDiags.HasErrors() {
Copy link
Member

@radditude radditude Aug 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm, earlyConfDiags doesn't include errors from backend initialization, so if I have a config that looks like this:

terraform {
  backend "local" {
    bad_attribute = "is bad"
  }
}

bad_block {

}

I won't see anything about the backend problems until after I fix any other config errors.

That doesn't seem like a terrible outcome, but I wonder if it might be just as well to return all these diags together and either get rid of or adjust errInitConfigError so that it makes sense for both config errors and backend errors - the exact wording of that error message used to be important because terraform-exec relied on it, but that matching was since removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, sounds good! I've made it print both sets if either one fails and tweaked the wording a bit!

Copy link
Member

Choose a reason for hiding this comment

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

Ah, actually, an important bit of context came back to me and I fear I gave you a bad suggestion 😅 backDiags includes config validation errors as well, so appending/returning both together means we'll show errors twice.

We could consider:

  1. making loadBackendConfig not also validate the entire root module so backDiags actually contains what it says on the tin
  2. going back to what you had originally in order to fix the crash and straighten out this initialization logic later

2 seems reasonable to me, unless you have other suggestions - sorry for the false flag!

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm more than happy to turn it back, but I don't think they will actually contain duplicate diagnostics. The HCL library itself caches the files it loads: https://github.com/hashicorp/hcl/blob/main/hclparse/parser.go#L56

So, the first time round it loads the partial file with the diagnostics, but on following reads it doesn't reproduce the diagnostics. It just returns the partial file directly with no diagnostics. The tests also don't show duplicate errors.

Either way, I'm more than happy to go back to the previous approach but I think it should be fine.

c.Ui.Error(strings.TrimSpace(errInitConfigError))
c.showDiagnostics(diags)
return 1
}

// Now, we can show any errors from initializing the backend.
diags = diags.Append(backDiags)
if backDiags.HasErrors() {
c.showDiagnostics(diags)
return 1
}

// If everything is ok with the core version check and backend initialization,
Expand Down
44 changes: 38 additions & 6 deletions internal/command/init_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2700,14 +2700,46 @@ func TestInit_invalidSyntaxInvalidBackend(t *testing.T) {
}

errStr := ui.ErrorWriter.String()
if subStr := "There are some problems with the configuration, described below"; strings.Contains(errStr, subStr) {
t.Errorf("Error output should not include preamble\nwant substr: %s\ngot:\n%s", subStr, errStr)
if subStr := "There are some problems with the configuration, described below"; !strings.Contains(errStr, subStr) {
t.Errorf("Error output should include preamble\nwant substr: %s\ngot:\n%s", subStr, errStr)
}
if subStr := "Error: Unsupported block type"; !strings.Contains(errStr, subStr) {
t.Errorf("Error output should mention syntax errors\nwant substr: %s\ngot:\n%s", subStr, errStr)
}
if subStr := "Error: Unsupported backend type"; strings.Contains(errStr, subStr) {
t.Errorf("Error output should not mention the invalid backend\nwant substr: %s\ngot:\n%s", subStr, errStr)
}
}

func TestInit_invalidSyntaxBackendAttribute(t *testing.T) {
td := t.TempDir()
testCopyDir(t, testFixturePath("init-syntax-invalid-backend-attribute-invalid"), td)
defer testChdir(t, td)()

ui := cli.NewMockUi()
view, _ := testView(t)
m := Meta{
Ui: ui,
View: view,
}

c := &InitCommand{
Meta: m,
}

if code := c.Run(nil); code == 0 {
t.Fatalf("succeeded, but was expecting error\nstdout:\n%s\nstderr:\n%s", ui.OutputWriter, ui.ErrorWriter)
}

errStr := ui.ErrorWriter.String()
if subStr := "There are some problems with the configuration, described below"; !strings.Contains(errStr, subStr) {
t.Errorf("Error output should include preamble\nwant substr: %s\ngot:\n%s", subStr, errStr)
}
if subStr := "Error: Unsupported block type"; strings.Contains(errStr, subStr) {
t.Errorf("Error output should not mention syntax errors\nwant substr: %s\ngot:\n%s", subStr, errStr)
if subStr := "Error: Invalid character"; !strings.Contains(errStr, subStr) {
t.Errorf("Error output should mention the invalid character\nwant substr: %s\ngot:\n%s", subStr, errStr)
}
if subStr := "Error: Unsupported backend type"; !strings.Contains(errStr, subStr) {
t.Errorf("Error output should mention the invalid backend\nwant substr: %s\ngot:\n%s", subStr, errStr)
if subStr := "Error: Invalid expression"; !strings.Contains(errStr, subStr) {
t.Errorf("Error output should mention the invalid expression\nwant substr: %s\ngot:\n%s", subStr, errStr)
}
}

Expand Down
13 changes: 11 additions & 2 deletions internal/command/meta_backend.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ import (

"github.com/hashicorp/hcl/v2"
"github.com/hashicorp/hcl/v2/hcldec"
"github.com/zclconf/go-cty/cty"
ctyjson "github.com/zclconf/go-cty/cty/json"

"github.com/hashicorp/terraform/internal/backend"
"github.com/hashicorp/terraform/internal/cloud"
"github.com/hashicorp/terraform/internal/command/arguments"
Expand All @@ -29,8 +32,6 @@ import (
"github.com/hashicorp/terraform/internal/states/statemgr"
"github.com/hashicorp/terraform/internal/terraform"
"github.com/hashicorp/terraform/internal/tfdiags"
"github.com/zclconf/go-cty/cty"
ctyjson "github.com/zclconf/go-cty/cty/json"

backendInit "github.com/hashicorp/terraform/internal/backend/init"
backendLocal "github.com/hashicorp/terraform/internal/backend/local"
Expand Down Expand Up @@ -1398,6 +1399,14 @@ func (m *Meta) backendInitFromConfig(c *configs.Backend) (backend.Backend, cty.V
return nil, cty.NilVal, diags
}

if !configVal.IsWhollyKnown() {
diags = diags.Append(tfdiags.Sourceless(
tfdiags.Error,
"Unknown values within backend definition",
"The `terraform` configuration block should contain only concrete and static values. Another diagnostic should contain more information about which part of the configuration is problematic."))
return nil, cty.NilVal, diags
}

// TODO: test
if m.Input() {
var err error
Expand Down
10 changes: 10 additions & 0 deletions internal/command/testdata/init-syntax-invalid-backend-attribute-invalid/main.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@

terraform {
backend "local" {
path = $invalid
}
}

variable "input" {
type = string
}