Migration UX improvements and bug fixes#254
Merged
tamas-jozsa merged 16 commits intomainfrom Mar 27, 2026
Merged
Conversation
Adds test cases that reproduce 6 migration bugs discovered when the
research team (terraform-cfaccounts MR !7756) migrated their Terraform
config from Cloudflare Provider v4 → v5.
TKT-001: cloudflare_authenticated_origin_pulls — cert_id still references
cloudflare_authenticated_origin_pulls_certificate instead of
cloudflare_authenticated_origin_pulls_hostname_certificate in for_each
resources. The research team had to manually fix this with sed.
TKT-002: cloudflare_zero_trust_access_policy — include/exclude/require
remain as HCL blocks instead of being converted to include = [{ ... }]
attribute list syntax. This causes 'invalid JSON, expected {, got ['
during state upgrade.
TKT-003: cloudflare_zero_trust_access_policy — application_id and
precedence are not removed from policies and not moved to the linked
access application's policies = [...] block. Causes
'unsupported attribute application_id' state decode failure.
TKT-004: service_token = [id] list not converted to
service_token = { token_id = id } object format.
TKT-005: email_domain = ['domain'] list not converted to
email_domain = { domain = 'domain' } object format.
TKT-006: any_valid_service_token = true not converted to
any_valid_service_token = {} empty object format.
The expected output files reflect the CURRENT (buggy) migration output.
Tests pass now to confirm bugs are reproduced. They will need to be
updated to correct v5 syntax once the bugs are fixed.
Tracking tickets created in tickets/ directory.
TKT-001: Fix cloudflare_authenticated_origin_pulls cert_id reference update
The GetResourceRename for authenticated_origin_pulls_certificate now returns
the hostname certificate type as the rename target, so the global cross-file
reference postprocessor correctly renames all cert_id references from
cloudflare_authenticated_origin_pulls_certificate.X to
cloudflare_authenticated_origin_pulls_hostname_certificate.X — including
for_each references like resource.name[each.key].id.
Removed the heuristic name-based Postprocess fallback.
TKT-004: Fix service_token list not converted to {token_id=...} object
Added 'service_token' to the arrayAttrs map in expandArrayAttribute with
inner field name 'token_id', so service_token = [id_ref] is correctly
converted to service_token = { token_id = id_ref }.
Root cause of TKT-004 (and fix): The buildExprTokens serializer only emitted
the root name of ScopeTraversalExpr, dropping .attr, [index] etc. Fixed to
serialize the full traversal chain including attribute access, index access,
and relative traversals (each.key, each.value). Also added handlers for
RelativeTraversalExpr, IndexExpr, and FunctionCallExpr.
TKT-002/005/006 were already working correctly — email_domain, any_valid_service_token,
and include block → list conversion all produce correct v5 output.
TKT-003: application_id is already removed with a warning diagnostic. The
warning guides the user to manually add policies = [...] to the application.
Tests: Updated expected fixtures to reflect correct migration output.
Added new test patterns covering all research team issues.
…al API credentials authenticated_origin_pulls: - Add authenticated_origin_pulls_e2e.tf with only zone-wide AOP - Exclude per-hostname certificate resources — the API rejects fake cert data (error 1408). TKT-001 cert_id fix is validated by integration tests only. zero_trust_access_policy: - Add zero_trust_access_policy_e2e.tf excluding: - service_token_policy/multi/combined (need real service token resources) - app_scoped_policy (needs real domain/application, TKT-003) - Fix no_service_token_policy: change decision to 'allow' (non_identity + email_domain is invalid in the API), use email_domain instead of email - All TKT fixes are validated by both integration tests and the e2e subset
Five additional provider-side bugs discovered in the research team PR
(terraform-cfaccounts MR !7756) that cause UpgradeResourceState failures
during terraform plan.
All follow the same pattern: the v5 provider UpgradeState handler expects
a JSON object {} but the v4 state stored the field as a JSON array [] or
plain string.
Affected resources:
- TKT-007: zero_trust_access_policy.connection_rules ([] → {})
- TKT-008: cloudflare_zone.plan ('enterprise' string → {})
- TKT-009: cloudflare_logpush_job.output_options ([] → {})
- TKT-010: cloudflare_notification_policy.filters ([] → {})
- TKT-011: cloudflare_ruleset.action_parameters ([] → {})
These are provider bugs in cloudflare-terraform-next, not tf-migrate issues.
…pty ID bug The v4 provider had a bug where cloudflare_leaked_credential_check_rule was created successfully but the detection_id was not stored in state (id = ''). When migrating to v5: 1. The v5 provider's Read() detects the empty ID and removes the resource from state (our earlier fix to resource.go) 2. On the next terraform apply, Terraform tries to POST a new rule 3. If the rule was actually created by v4, the API returns error 11003: 'custom detection for given username and password already exists' The migrator now appends a MIGRATION WARNING comment to each cloudflare_leaked_credential_check_rule resource block, guiding users to find the existing detection_id via the API and import the resource before running terraform apply.
…nal output
The MIGRATION WARNING comment was only written to the .tf file as a code
comment — it was invisible in the terminal output during migration.
Now also appends a hcl.Diagnostic warning so it appears in the migration
diagnostics summary alongside other actionable warnings like resource splits
and deprecated field removals.
The terminal output now shows:
⚠ Manual action required for cloudflare_leaked_credential_check_rule.<name>
The v4 provider had a bug where the detection_id was not stored in state.
Steps to fix:
1. Find the existing detection_id: curl ...
2. Import: terraform import cloudflare_leaked_credential_check_rule.<name> ...
…curl and import commands
When the zone_id attribute is a literal string, extract it and use it
directly in the curl and import commands so they are immediately runnable.
Before:
curl -s "...zones/<zone_id>/..." ...
terraform import ...lcc_rule_basic <zone_id>/<detection_id>
After (zone_id is a literal):
curl -s "...zones/63a1772ec2bacdee371d3119f58c3300/..." ...
terraform import ...lcc_rule_basic 63a1772ec2bacdee371d3119f58c3300/<detection_id>
Also improved the jq filter to show {id, username, password} so users
can identify which detection matches their config.
… a literal hex value ExtractStringFromAttribute was returning 'var' when zone_id = var.zone_id, producing a broken curl URL: zones/var/leaked-credential-checks/detections Now only use the extracted value if it is exactly 32 characters (a real Cloudflare zone ID). Variable references like var.zone_id, local.zone_id, etc. fall back cleanly to the <zone_id> placeholder.
…lder IDs
Instead of only emitting a MIGRATION WARNING comment and terminal
diagnostic, the migrator now generates an actual import {} block in the
migrated file with placeholder IDs (<zone_id>/<detection_id>).
This is more actionable: the user just needs to:
1. Find the detection_id via the curl command in the terminal output
2. Replace <detection_id> in the import block
When zone_id is a literal 32-char hex string, it is embedded in the
import block ID automatically (only <detection_id> needs to be replaced).
When zone_id is a variable reference, both placeholders are present.
The fix to use RemoveOriginal: true is required because the pipeline
handler only processes extra blocks (blocksToAdd) when RemoveOriginal
is true - the original block is included in result.Blocks.
Also updates unit tests and integration test fixtures to reflect the
new import block generation.
…teps Diagnostics ----------- Introduce transform.DiagInfo (= hcl.DiagInvalid, the zero value) as a named info severity level. Diagnostics at this level are suppressed by default and only shown with --verbose. Demote four 'Deprecated field(s) removed' diagnostics to DiagInfo since the migrator already handled the change automatically -- no user action required: - certificate_pack: wait_for_active_status removed - workers_script: tags field removed - page_rule: minify / disable_railgun removed - origin_ca_certificate: min_days_for_renewal removed Keep actionable warnings at DiagWarning (resource splits, manual imports, dynamic blocks requiring migration, etc.). printDiagnostics now shows a hint in the summary line when info messages are suppressed: Migration diagnostics: 1 warning(s), 3 info message(s) suppressed (use --verbose to see) Provider version detection -------------------------- Fix targetProviderVersion to include prereleases (prerelease: true). Previously only Draft was checked, causing beta releases to be skipped and falling through to the hardcoded fallback (5.19.0-beta.2). Update fallback to 5.19.0-beta.3. Next steps output ----------------- - Add step 1 'Review warnings above' only when actionable warnings exist - Show explicit git commands: git add . / git commit / git push - Auto-number steps dynamically so numbering is always correct
When cloudflare_argo has both smart_routing and tiered_caching, the
v5 migration splits it into two resources:
- cloudflare_argo_smart_routing (moved block, state carries over)
- cloudflare_argo_tiered_caching (new, no state entry)
Previously the user was told to run 'terraform import' manually.
Now an import {} block is generated alongside the resource block, so
the user only needs to replace <zone_id> with the real value (or
nothing at all if zone_id is a literal 32-char hex string).
The diagnostic message is updated to reference the generated block
and explain what still needs to be done.
Add FilePath (full path) field to transform.Context alongside the
existing Filename (base name only) field. Populate it from the full
file path at both Context creation sites in main.go.
Use ctx.FilePath in the two diagnostics that generate import blocks
so users see exactly which file to edit:
⚠ Action required: fill in detection_id for ...lcc_rule_basic
An import {} block has been generated in:
/path/to/leaked_credential_check_rule.tf
⚠ Action required: import tiered caching resource for cloudflare_argo.xxx
An import {} block has been added in:
/path/to/argo.tf
targetProviderVersion now returns (version, fromAPI bool) so callers
can distinguish between a live GitHub API result and the hardcoded
fallback. updateProviderVersionConstraint logs at debug level:
- 'Provider version fetched from GitHub API' when the API responded
- 'GitHub API unavailable, using built-in fallback version' when
rate-limited, offline, or the request otherwise failed
Use --log-level debug to see which path was taken.
…ailable
Remove the hardcoded fallback version map. When the GitHub releases API
is unreachable (rate-limited, offline, or returns non-200), skip updating
required_providers entirely and instead print an explicit warning with
instructions for the user to update the version manually:
⚠ Could not fetch the latest provider version from GitHub
(network unavailable or rate-limited).
The provider version in required_providers has NOT been updated.
Next steps:
1. Find the latest version: https://github.com/.../releases
2. Update required_providers manually
3. Regenerate the lock file
4. Commit and push
A stale hardcoded version is worse than being explicit — it would silently
pin users to an outdated beta without any indication something went wrong.
If a cloudflare_tiered_cache block has already been migrated (has 'value' but no 'cache_type'), return it unchanged. Previously re-running the migrator would treat the already-migrated value attribute as if it were a variable reference, generate a second cloudflare_tiered_cache block, and (for generic/smart/off) also a second cloudflare_argo_tiered_caching block — causing duplicate resource errors on terraform init.
…n all next-steps paths
--target-provider-version
New migrate flag that accepts an explicit provider version string
(e.g. 5.19.0-beta.3), bypassing the GitHub API lookup entirely.
Useful in CI or air-gapped environments where the API is unreachable.
Example:
tf-migrate migrate --target-provider-version 5.19.0-beta.3
Review warnings step in version-fetch-failure path
printVersionFetchFailure now receives hcl.Diagnostics and prepends
'Review the warnings above...' as step 1 when there are actionable
warnings — consistent with the normal success path.
tamas-jozsa
force-pushed
the
leaked-credential
branch
from
f38c1e0 to
e31a5b9
Compare
vaishakdinesh
approved these changes
Mar 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Migration UX improvements and bug fixes
Actionable import blocks
Two resources that previously required manual
terraform importcommands now auto-generateimport {}blocks with placeholder IDs in the migrated files:cloudflare_argo— when bothsmart_routingandtiered_cachingare present, thecloudflare_argo_tiered_cachingsplit resource gets an import block. Ifzone_idis a literal string it's embedded automatically; variable references fall back to<zone_id>.cloudflare_leaked_credential_check_rule— the v4 provider had a bug wheredetection_idwas never stored in state. The migrator now generates an import block with<zone_id>/<detection_id>placeholders and includes the exactcurlcommand to find the real detection ID.Both warnings include the full file path so users know exactly where to make the edit.
Cleaner diagnostic output
Introduced
transform.DiagInfoas a named info severity level (suppressed by default, shown with--verbose). Deprecation notices that the migrator already handled automatically are demoted from warnings to info:certificate_pack—wait_for_active_statusremovedworkers_script—tagsremovedpage_rule—minify/disable_railgunremovedorigin_ca_certificate—min_days_for_renewalremovedThe summary line now reads:
Migration diagnostics: 2 warning(s), 4 info message(s) suppressed (use --verbose to see)Improved next steps output
git add . / git commit / git pushcommands shown instead of proseProvider version detection fixes
prerelease: true) — previously onlydraftwas checked, so all beta releases were skipped and the hardcoded fallback was always usedrequired_providersmanually with a link to the releases page--target-provider-versionflag to bypass the API lookup entirely (useful in CI or air-gapped environments)Bug fixes
tiered_cachemigrator idempotency — re-running on an already-migrated file previously generated duplicate resource blocks causingterraform initto fail with "Duplicate resource" errors. Fixed by bailing out early whencache_typeis absent.