Skip to content

Fail global required_version check if it contains any prerelease fields #31331

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

Merged
liamcervante merged 5 commits into from
Jun 30, 2022
Merged

Fail global required_version check if it contains any prerelease fields #31331

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5220c4c
Fail global required_version check if it contains any prerelease fields
Jun 28, 2022
df90f74
go mod tidy
Jun 28, 2022
6f423c1
Improve required_version prerelease not supported error string
Jun 28, 2022
b5e1794
Add prerelease version constraint unit tests
Jun 28, 2022
2b5fb66
Fix side-effects by populating global diags too soon
Jun 29, 2022
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
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ require (
github.com/hashicorp/go-retryablehttp v0.7.0
github.com/hashicorp/go-tfe v1.0.0
github.com/hashicorp/go-uuid v1.0.2
github.com/hashicorp/go-version v1.3.0
github.com/hashicorp/go-version v1.6.0
github.com/hashicorp/hcl v0.0.0-20170504190234-a4b07c25de5f
github.com/hashicorp/hcl/v2 v2.13.0
github.com/hashicorp/terraform-config-inspect v0.0.0-20210209133302-4fd17a0faac2
Expand Down
3 changes: 2 additions & 1 deletion go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -402,8 +402,9 @@ github.com/hashicorp/go-uuid v1.0.2/go.mod h1:6SBZvOh/SIDV7/2o3Jml5SYk/TvGqwFJ/b
github.com/hashicorp/go-version v1.0.0/go.mod h1:fltr4n8CU8Ke44wwGCBoEymUuxUHl09ZGVZPK5anwXA=
github.com/hashicorp/go-version v1.1.0/go.mod h1:fltr4n8CU8Ke44wwGCBoEymUuxUHl09ZGVZPK5anwXA=
github.com/hashicorp/go-version v1.2.0/go.mod h1:fltr4n8CU8Ke44wwGCBoEymUuxUHl09ZGVZPK5anwXA=
github.com/hashicorp/go-version v1.3.0 h1:McDWVJIU/y+u1BRV06dPaLfLCaT7fUTJLp5r04x7iNw=
github.com/hashicorp/go-version v1.3.0/go.mod h1:fltr4n8CU8Ke44wwGCBoEymUuxUHl09ZGVZPK5anwXA=
github.com/hashicorp/go-version v1.6.0 h1:feTTfFNnjP967rlCxM/I9g701jU+RN74YKx2mOkIeek=
github.com/hashicorp/go-version v1.6.0/go.mod h1:fltr4n8CU8Ke44wwGCBoEymUuxUHl09ZGVZPK5anwXA=
github.com/hashicorp/golang-lru v0.5.0/go.mod h1:/m3WP610KZHVQ1SGc6re/UDhFvYD7pJ4Ao+sR/qLZy8=
github.com/hashicorp/golang-lru v0.5.1 h1:0hERBMJE1eitiLkihrMvRVBYAkpHzc/J3QdDN+dAcgU=
github.com/hashicorp/golang-lru v0.5.1/go.mod h1:/m3WP610KZHVQ1SGc6re/UDhFvYD7pJ4Ao+sR/qLZy8=
Expand Down
16 changes: 16 additions & 0 deletions internal/terraform/context_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,22 @@ func TestNewContextRequiredVersion(t *testing.T) {
false,
},

{
"prerelease doesn't match with inequality",
"",
"0.8.0",
"> 0.7.0-beta",
true,
},

{
"prerelease doesn't match with equality",
"",
"0.7.0",
"0.7.0-beta",
true,
},

{
"module matches",
"context-required-version-module",
Expand Down
21 changes: 21 additions & 0 deletions internal/terraform/version_required.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,27 @@ func CheckCoreVersionRequirements(config *configs.Config) tfdiags.Diagnostics {
module := config.Module

for _, constraint := range module.CoreVersionConstraints {
// Before checking if the constraints are met, check that we are not using any prerelease fields as these
// are not currently supported.
for _, required := range constraint.Required {
if required.Prerelease() {
diags = diags.Append(&hcl.Diagnostic{
Severity: hcl.DiagError,
Summary: "Invalid required_version constraint",
Detail: fmt.Sprintf(
"Prerelease version constraints are not supported: %s. Remove the prerelease information from the constraint. Prerelease versions of terraform will match constraints using their version core only.",
required.String()),
Subject: constraint.DeclRange.Ptr(),
})
}
}

if len(diags) > 0 {
Copy link
Copy Markdown
Contributor

@alisdair alisdair Jun 28, 2022

Choose a reason for hiding this comment

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

This makes me think we'll skip all constraints after the first one fails for any reason, whereas at the moment I think we check all of the constraints that we can. Could we change this to only continue here if the current constraint has a prerelease version in it, allowing other constraints to evaluate separately?

Copy link
Copy Markdown
Contributor Author

@liamcervante liamcervante Jun 29, 2022

Choose a reason for hiding this comment

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

I think to clarify.

At the top level we have the CoreVersionConstraints field. CoreVersionConstraints is a slice of VersionConstraint. VersionConstraint is just a wrapper around the go-version Constraints struct (with some metadata). Constraints is then a slice of Constraint structs.

The existing behavior and my implementation both will only ever print a single diagnostic for each VersionConstraint and both iterate over the entire CoreVersionConstraints slice and analyse all of them even if an early one fails.

I think what you're asking for is this: #31339. But I would argue it represents a greater behavior change than my current implementation in this PR.

More details:

The existing behavior eagerly fails within Constraints. As soon as one of the constraints doesn't match the version it gives up. CheckCoreVersionRequirements will then add a single diagnostic for the entire VersionConstraint that wrapped the current Constraints struct and then move on to the next VersionConstraint in the CoreVersionConstraints field.

My implementation (this PR) matches this, so if a single Constraint contains a prerelease field then it will eagerly fail without checking the rest of the constraints within that specific VersionConstraint and add a single diagnostic for that VersionConstraint. But it will then continue on and still check all the other VersionConstraint structs within the CoreVersionConstraints field and print out a specific diagnostic for each one.

The other PR changes this behaviour so that every Constraint within each VersionConstraint is always evaluated instead of just giving up when the first one fails.

Copy link
Copy Markdown
Contributor

@alisdair alisdair Jun 29, 2022

Choose a reason for hiding this comment

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

Trying this branch locally seems to show a change in behaviour which lines up with my reading of the code. Consider this (nonsense) configuration:

terraform {
  required_version = "> 1.3.0"
}

terraform {
  required_version = "< 1.3.0"
}

On main, running terraform init gives two diagnostics:

$ terraform init
╷
│ Error: Unsupported Terraform Core version
│
│   on main.tf line 2, in terraform:
│    2:   required_version = "> 1.3.0"
│
│ This configuration does not support Terraform version 1.3.0-dev. To proceed,
│ either choose another supported Terraform version or update this version
│ constraint. Version constraints are normally set for good reason, so updating
│ the constraint may lead to other errors or unexpected behavior.
╵
╷
│ Error: Unsupported Terraform Core version
│
│   on main.tf line 6, in terraform:
│    6:   required_version = "< 1.3.0"
│
│ This configuration does not support Terraform version 1.3.0-dev. To proceed,
│ either choose another supported Terraform version or update this version
│ constraint. Version constraints are normally set for good reason, so updating
│ the constraint may lead to other errors or unexpected behavior.
╵

On this branch, only the first constraint diagnostic is shown (because it's added to diags, and we then continue past any others in this module):

$ terraform init
╷
│ Error: Unsupported Terraform Core version
│
│   on main.tf line 2, in terraform:
│    2:   required_version = "> 1.3.0"
│
│ This configuration does not support Terraform version 1.3.0-dev. To proceed,
│ either choose another supported Terraform version or update this version
│ constraint. Version constraints are normally set for good reason, so updating
│ the constraint may lead to other errors or unexpected behavior.
╵

What I was trying to suggest was something like this:

 var prereleaseDiags tfdiags.Diagnostics
 // Before checking if the constraints are met, check that we are not using any prerelease fields as these
 // are not currently supported.
 for _, required := range constraint.Required {
 	if required.Prerelease() {
 		prereleaseDiags = prereleaseDiags.Append(&hcl.Diagnostic{
 			Severity: hcl.DiagError,
 			Summary:  "Invalid required_version constraint",
 			Detail: fmt.Sprintf(
 				"Prerelease version constraints are not supported: %s. Remove the prerelease information from the constraint. Prerelease versions of terraform will match constraints using their version core only.",
 				required.String()),
 			Subject: constraint.DeclRange.Ptr(),
 		})
 	}
 }

 diags = diags.Append(prereleaseDiags)
 if len(prereleaseDiags) > 0 {
  		continue
 }

That is, keeping track of prerelease diags separately from others, and only skipping over constraint checking for this iteration of the loop if there's a prerelease present.

I don't think this is a critical behaviour change, and my conditional approval still stands if this doesn't make sense to you!

Copy link
Copy Markdown
Contributor Author

@liamcervante liamcervante Jun 29, 2022

Choose a reason for hiding this comment

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

Yes, I see it now! Apologies and fixed!

// There were some prerelease fields in the constraints. Don't check the constraints as they will
// fail, and the diagnostics are already populated.
continue
}

if !constraint.Required.Check(tfversion.SemVer) {
switch {
case len(config.Path) == 0:
Expand Down