Skip to content

fix: resolve access policy and AOP certificate migration bugs#248

Merged
vaishakdinesh merged 9 commits intomainfrom
migration-fixes
Mar 26, 2026
Merged

fix: resolve access policy and AOP certificate migration bugs#248
vaishakdinesh merged 9 commits intomainfrom
migration-fixes

Conversation

@tamas-jozsa
Copy link
Collaborator

@tamas-jozsa tamas-jozsa commented Mar 26, 2026

Problem 1: authenticated_origin_pullscert_id still references old resource type

When cloudflare_authenticated_origin_pulls_certificate resources with type = "per-hostname" are renamed to cloudflare_authenticated_origin_pulls_hostname_certificate, the cert_id attribute in cloudflare_authenticated_origin_pulls resources using for_each was not updated — leaving references like cloudflare_authenticated_origin_pulls_certificate.cert[each.key].id pointing at a resource type that no longer exists in the v5 provider.

Fix: GetResourceRename for authenticated_origin_pulls_certificate now returns cloudflare_authenticated_origin_pulls_hostname_certificate as the rename target, so the global cross-file postprocessor correctly renames all cert_id references including for_each indexed ones. The heuristic name-based Postprocess fallback was removed.


Problem 2: zero_trust_access_policyservice_token list not converted to {token_id=...} object

service_token = [resource.name.id] inside include blocks was not being converted to service_token = { token_id = resource.name.id }. The migrator was silently producing broken output.

Root cause: Two issues:

  1. service_token was not in the arrayAttrs map in expandArrayAttribute
  2. buildExprTokens for ScopeTraversalExpr only serialized the root identifier, silently dropping .id, [each.key], and all other traversal steps

Fix: Added service_token → token_id to arrayAttrs. Fixed buildExprTokens to serialize the full traversal chain, and added handlers for RelativeTraversalExpr, IndexExpr, and FunctionCallExpr so references like resource.name[each.key].id are correctly preserved in all include/exclude/require conditions.


Already working (test coverage added)

The following transformations were already producing correct output — test cases added to prevent regression:

  • include {} block → include = [{ ... }] attribute list conversion
  • email_domain = ["domain"] → { domain = "domain" } object conversion (single and multiple domains)
  • any_valid_service_token = true → {} empty object conversion

application_id / precedence removal

application_id and precedence are correctly removed from policies with a diagnostic warning explaining that application-scoped policies must be manually converted to inline policies = [...] blocks on the linked cloudflare_zero_trust_access_application. Full automation is not possible since it requires cross-resource modification.


E2E test improvements

  • Added scoped _e2e.tf files for authenticated_origin_pulls (per-hostname cert resources require valid real certificates) and zero_trust_access_policy (service token and application-scoped resources require real credentials)
  • Added drift exemptions:
    • zero_trust_access_policy: session_duration is removed from v5 policies; the API normalizes it to "24h" — one-time drift that resolves after first apply
    • zero_trust_dlp_predefined_profile: name and open_access are Computed-only in v5 and show as (known after apply) on the first plan — one-time drift

========================================
✓ E2E Test Complete!
========================================

Summary:

  Step 1: v4 terraform apply
    Status: ✓ SUCCESS

  Step 2: Migration (v4 → v5)
    Status: ✓ SUCCESS

  Step 3: v5 plan (before apply)
    Status: ✓ SUCCESS - Drift matched exemptions
    Result: 17 material changes (17 matched exemptions, 0 unmatched)
    Terraform: Plan: 16 to import, 6 to add, 7 to change, 0 to destroy.

  Step 4: v5 terraform apply
    Status: ✓ SUCCESS

  Step 5: v5 plan (after apply)
    Status: ✓ SUCCESS - Stable state achieved
    Result: 7 material changes matched exemptions (0 unmatched)

Logs saved to:
  - /Users/tjozsa/cf-repos/sdks/migration-work/agent-a/tf-migrate/e2e/tmp

@tamas-jozsa tamas-jozsa force-pushed the migration-fixes branch 2 times, most recently from 7724bcc to 5e9b460 Compare March 26, 2026 16:59
@tamas-jozsa tamas-jozsa changed the title Migration fixes fix: resolve access policy and AOP certificate migration bugs Mar 26, 2026
tamas-jozsa and others added 9 commits March 26, 2026 14:41
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.
…ected output

The integration test runner was rebuilding the tf-migrate binary on every
sub-test invocation (~86 resources × ~3s build = ~4min), causing the test
suite to time out with 'panic: test timed out after 1m30s'.

Added a sync.Once cache so the binary is compiled only once per test run,
reducing total integration test time from >90s to ~35s.

Also updated the expected output for authenticated_origin_pulls_certificate:
the GetResourceRename change now correctly renames all cross-file references
from cloudflare_authenticated_origin_pulls_certificate to
cloudflare_authenticated_origin_pulls_hostname_certificate, which affected
the expected output for per-hostname resources in the existing test.
The provider test harness (acctest.go RunMigrationV2Command) passes --yes
to tf-migrate. After the rename to --skip-phase-check the provider tests
started failing with 'unknown flag: --yes'.

Add --yes back as a hidden (non-documented) alias that sets skipPhaseCheck=true,
preserving backward compatibility with provider test harnesses and any CI
scripts that predate the rename.
@vaishakdinesh vaishakdinesh merged commit e6a6d8f into main Mar 26, 2026
9 checks passed
@vaishakdinesh vaishakdinesh deleted the migration-fixes branch March 26, 2026 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants