Fail global required_version check if it contains any prerelease fields

[liamcervante]

Closes #28148

Any required_version that contains a prerelease field will fail the later constraint check anyway. This change ensures the reported error message makes clear that prerelease versions are not supported in the required_version constraint.

[bflad]

I'm wondering if this should also suggest the fix of removing the prerelease information and that the version constraint may still match the prerelease?

[liamcervante]

Yes, that seems reasonable. I've added the extra information to the error message.

[kmoe]

Good stuff. Perhaps this could be tested in internal/terraform/context_test.go?

[liamcervante]

Good stuff. Perhaps this could be tested in internal/terraform/context_test.go?

Done! Added tests that verify it fails when a prerelease field is used in the constraint. I wanted to add a check that ensures when the actual version has a prerelease but the core matches then it still passes the check but the test doesn't mirror the behaviour of the terraform version properly. So I couldn't add that check.

This is because version.go doesn't embed the prerelease info into the actual version internally. So when terraform runs it never thinks it's executing in prerelease mode. This all means a test for case context_test.go wouldn't really make sense.

[alisdair]

Nice fix! I think I spotted an unintended change which shortcuts checking of multiple constraints in a single module, more thoughts inline. Otherwise looks good to merge!

[alisdair]

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?

[liamcervante]

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.

[alisdair]

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!

[liamcervante]

Yes, I see it now! Apologies and fixed!

[github-actions]

Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch.

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.