Skip to content

PSS: Add parsing of .tfmigrate.hcl files to define state migration operations#38526

Open
SarahFrench wants to merge 17 commits into
mainfrom
pss/TF-36919-parse-tfmigrate-hcl-files
Open

PSS: Add parsing of .tfmigrate.hcl files to define state migration operations#38526
SarahFrench wants to merge 17 commits into
mainfrom
pss/TF-36919-parse-tfmigrate-hcl-files

Conversation

@SarahFrench
Copy link
Copy Markdown
Member

@SarahFrench SarahFrench commented May 5, 2026

Closes https://hashicorp.atlassian.net/browse/TF-36919

This PR enables parsing .tfmigrate.hcl files and new blocks:

  • state_store_provider (like required_providers but enforces a single provider and the constraint has to be a specific value)
  • from block - i.e. where the state is being migrated from. The .tf configuration is the desired state location, the 'to'.
    • backend - just like in the terraform block
    • state_store - just like in the terraform block

This PR also implements validation in the parser like:

  • No duplicate blocks allowed - and enforcing this across 1+ files.
    • Applies to: from, state_store_provider
  • Within a from block:
    • No duplicated backend or state_store blocks
    • The state_store and backend blocks are mutually exclusive.
  • If using state_store, state_store_provider must also be present.
  • Providers described in state_store and state_store_provider must match.
  • A state_store_provider block must:
    • Describe a specific version (version constraints not used here!)
    • Describe only a single provider (only one entry).

The state migrate command will eventually use this logic, and this PR hasn't connected those parts of the codebase yet. When that does happen I expect that calling code would populate the ProviderSupplyMode data.

Target Release

1.16.x

Rollback Plan

  • If a change needs to be reverted, we will roll out an update to the code within 7 days.

Changes to Security Controls

Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.

CHANGELOG entry

  • This change is user-facing and I added a changelog entry.
  • This change is not user-facing.

@SarahFrench SarahFrench added the no-changelog-needed Add this to your PR if the change does not require a changelog entry label May 5, 2026
@SarahFrench SarahFrench force-pushed the pss/TF-36919-parse-tfmigrate-hcl-files branch 3 times, most recently from e6b00a6 to 7105ee7 Compare May 6, 2026 10:11
…les including `migrate_from_backend` blocks.
…er` blocks across multiple .tfmigrate.hcl files
…s missing its counterpart `migrate_from_state_store` block
…migrate.hcl files. Add tests for error paths and happy path.
…cross migrate_from_state_store and state_store_provider blocks. When they match, copy provider data across to description of the state store.

Copying that data across is similar to how we use methods called `resolveStateStoreProviderType` when paring config (via NewModule or FinalizeConfig functions).
@SarahFrench SarahFrench force-pushed the pss/TF-36919-parse-tfmigrate-hcl-files branch from 7105ee7 to 657a47f Compare May 6, 2026 10:40
…single provider. Also, add early returns to avoid repeated or incorrect error diagnostics.

Incorrect errors can occur if an error prevents a block being processed, and then downstream code reacts to the lack of that data with another error. That would be misleading (e.g. an error in a state_store_provider block meaning that later an incorrect error is raised reporting a missing state_store_provider block, when it is incorrect not missing).
Copy link
Copy Markdown
Member Author

@SarahFrench SarahFrench left a comment

Choose a reason for hiding this comment

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

Some self-review/contextual comments to help others with review.

Comment thread internal/configs/parser_config_dir.go Outdated
Comment on lines +146 to +149
} else {
// They match, so copy across relevant data.
ss.ProviderAddr = ssp.Type
}
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This serves a similar purpose as this code in NewModule:

if mod.StateStore != nil {
diags = append(diags, mod.resolveStateStoreProviderType()...)
}

return file, diags
}

func decodeStateStoreProviderBlock(block *hcl.Block) (*RequiredProvider, hcl.Diagnostics) {
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The logic in this method was adapted from the decodeRequiredProvidersBlock function:

func decodeRequiredProvidersBlock(block *hcl.Block) (*RequiredProviders, hcl.Diagnostics) {
attrs, diags := block.Body.JustAttributes()
if diags.HasErrors() {
return nil, diags
}
ret := &RequiredProviders{
RequiredProviders: make(map[string]*RequiredProvider),
DeclRange: block.DefRange,
}
for name, attr := range attrs {
rp := &RequiredProvider{
Name: name,
DeclRange: attr.Expr.Range(),
}

In decodeStateStoreProviderBlock we don't have multiple attributes (i.e don't have multiple local names with paired objects). We return early if there isn't 1 entry. After that there's no need for looping but the code looks very similar to that original code.

Comment thread internal/configs/state_migrate_file.go Outdated
Comment on lines +232 to +257
// Assert we have a single constraint, for a specific version
if len(constraints) != 1 {
diags = append(diags, &hcl.Diagnostic{
Severity: hcl.DiagError,
Summary: "Invalid version constraint",
Detail: "The version attribute inside the state_store_provider block must specify a single, specific version (e.g. \"= 1.0.0\").",
Subject: kv.Value.Range().Ptr(),
})
return nil, diags
}

// A constraint to use v1.2.3 could have an = operator or no operator at all.
constraintStr = strings.TrimPrefix(constraintStr, "=") // Remove a preceding `=`, if it exists.
constraintStr = strings.TrimSpace(constraintStr) // There might have been whitespace between the operator and the version.

_, err = versions.ParseVersion(constraintStr)
if err != nil {
// Errors indicate that the constraint wasn't a specific version.
diags = append(diags, &hcl.Diagnostic{
Severity: hcl.DiagError,
Summary: `Non-specific version constraint in "state_store_provider" configuration block`,
Detail: "The version constraint defined in a state_store_provider block must specify a single, specific version (e.g. \"= 1.0.0\", or \"1.0.0\").",
Subject: kv.Value.Range().Ptr(),
})
return nil, diags
}
Copy link
Copy Markdown
Member Author

@SarahFrench SarahFrench May 6, 2026

Choose a reason for hiding this comment

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

Also, I figured it was worth reusing existing types instead of creating a new type that used Version instead of VersionConstraint, so this method returns a RequiredProvider pointer.

Due to this we still use a version constraint, but I opted for validation that asserts the constraint is pinning to an exact value. The hashicorp/go-version representation of the version constraint doesn't allow calling code to inspect data and ask 'is this constraint using an = operator (implied or explicit)?'. Because of that, I landed on this approach where I check for an = operator (implied or explicit) and then attempt to parse the string as a version, and use successful parsing as a sign that the constraint was pinning a single version.

If there are other/better approaches to this I'm keen to get feedback. An alternative is updating hashicorp/go-version but I think that decision shouldn't be taken lightly.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I understand the need to reuse the interface, i.e. pass a constraint downstream but I think that it doesn't mean we need to leak that all the way through to the user?

I mean, if we eventually parse a valid go-version.Version then we could construct an artificial = constraint from that, could we not?

I don't think we need to be allowing an explicit = from the end user?

@SarahFrench SarahFrench marked this pull request as ready for review May 6, 2026 12:17
@SarahFrench SarahFrench requested a review from a team as a code owner May 6, 2026 12:17
@radeksimko radeksimko changed the title PSS: Add parsing of .tfmigrate.hcl files to define state migration operations PSS: Add parsing of .tfmigrate.hcl files to define state migration operations May 7, 2026
Comment thread internal/configs/module.go Outdated
Comment thread internal/configs/module.go Outdated
Comment thread internal/configs/module.go Outdated
Comment thread internal/configs/parser_config_dir.go Outdated
Comment thread internal/configs/parser_config_dir.go
Copy link
Copy Markdown
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

I'm tempted to suggest a few more changes to the schema to keep the new language closer to the existing one in *.tf

required_provider {
  // ...
}

migrate_from {
  state_store {
    // ...
  }

  backend {
    // ...
  }
}

or

required_provider {
  // ...
}

from {
  state_store {
    // ...
  }

  backend {
    // ...
  }
}

I think this could make it a bit easier to understand for users who're already familiar with the language? 🤔 Also it would reflect the reality where we parse both state_store and backend using all the existing decoder logic, i.e. we expect the same structure there.

Comment thread internal/configs/module.go Outdated
Comment thread internal/configs/parser_config_dir.go Outdated
Comment thread internal/configs/state_migrate_file.go Outdated
Comment thread internal/configs/state_migrate_file.go Outdated
Comment thread internal/configs/state_migrate_file.go Outdated
Comment on lines +232 to +257
// Assert we have a single constraint, for a specific version
if len(constraints) != 1 {
diags = append(diags, &hcl.Diagnostic{
Severity: hcl.DiagError,
Summary: "Invalid version constraint",
Detail: "The version attribute inside the state_store_provider block must specify a single, specific version (e.g. \"= 1.0.0\").",
Subject: kv.Value.Range().Ptr(),
})
return nil, diags
}

// A constraint to use v1.2.3 could have an = operator or no operator at all.
constraintStr = strings.TrimPrefix(constraintStr, "=") // Remove a preceding `=`, if it exists.
constraintStr = strings.TrimSpace(constraintStr) // There might have been whitespace between the operator and the version.

_, err = versions.ParseVersion(constraintStr)
if err != nil {
// Errors indicate that the constraint wasn't a specific version.
diags = append(diags, &hcl.Diagnostic{
Severity: hcl.DiagError,
Summary: `Non-specific version constraint in "state_store_provider" configuration block`,
Detail: "The version constraint defined in a state_store_provider block must specify a single, specific version (e.g. \"= 1.0.0\", or \"1.0.0\").",
Subject: kv.Value.Range().Ptr(),
})
return nil, diags
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I understand the need to reuse the interface, i.e. pass a constraint downstream but I think that it doesn't mean we need to leak that all the way through to the user?

I mean, if we eventually parse a valid go-version.Version then we could construct an artificial = constraint from that, could we not?

I don't think we need to be allowing an explicit = from the end user?

Comment thread internal/configs/state_migrate_file.go Outdated
…t, instead of directly parsing as constraint and trying to validate the type of constraint it is
1- Move all cross-block checks to be performed after all file contents are combined.
2- Remove unneeded nil checks after using NewModule function
3- Hoist assigning `mod.SourceDir` value to top of the LoadConfigDir method
4- Remove conditional panic
…, instead of migrate_from_x blocks.

This makes config validation a little easier - identification of duplicates simplified by looking for >1 from block.
…edding to simplify StateMigrationFile struct.
@SarahFrench
Copy link
Copy Markdown
Member Author

SarahFrench commented May 11, 2026

I've addressed feedback, including the proposed language change here. The PR description and associated RFC are also updated.

This change made naming cleaner but also makes validation more general (multiple error diagnostics can be collapsed down to a single 'can't have >1 from block there, mate')

@SarahFrench SarahFrench requested a review from radeksimko May 11, 2026 19:09
@SarahFrench SarahFrench requested a review from austinvalle May 11, 2026 19:09
Copy link
Copy Markdown
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for making all the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog-needed Add this to your PR if the change does not require a changelog entry

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants