Skip to content

Fix two terraform-cache-brownfield regressions in the resource cache - #2319

Open
ksamoray wants to merge 1 commit into
vmware:masterfrom
ksamoray:diagnostics/cache-search-race-logging
Open

Fix two terraform-cache-brownfield regressions in the resource cache#2319
ksamoray wants to merge 1 commit into
vmware:masterfrom
ksamoray:diagnostics/cache-search-race-logging

Conversation

@ksamoray

@ksamoray ksamoray commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

terraform-cache-brownfield started failing after passing consistently,
across two unrelated bugs surfaced by config_scope cache mode:

  1. IpAddressPoolStaticSubnet/IpAddressPoolBlockSubnet permanently
    bypassed the cache on every read. CacheAwareResourceRead has a
    shortIDBypass safety net: for path-indexed resource types, if the
    resourceID passed in doesn't contain "/", the cache is skipped
    entirely in favor of a direct GET, since a short id may not be
    unique across the project-wide search scope these types share.
    Both resources' Read functions unconditionally used the short
    d.Id() as that key instead of preferring d.Get("path") (populated
    since the prior Create) the way every other path-indexed resource
    does, so shortIDBypass fired on every single read, permanently.

    Fix: prefer d.Get("path"), falling back to d.Id() only when path
    isn't set yet (Create-then-Read within the same apply, or import) —
    matching the existing pattern already used by e.g. Service,
    SegmentPort, and PolicyNatRule.

  2. GatewayPolicy, SecurityPolicy, and VpcGroup silently wiped their
    provider-managed cache tracking tag (scope="nsx-tf/tf-run-id") on
    every Update, including a bare description change with no config
    drift on tags. Their Create/Update seeds Terraform state's "tag"
    attribute with the merged (user + provider-managed) tag list
    immediately before building the outgoing NSX PATCH payload from
    that same state — the only way the shared build-and-patch helpers
    pick up the provider-managed tag for the payload. That seed used
    initPolicyTagsSet, which filters the provider-managed tag out — a
    filter added so Read never leaks the tag into state — silently
    defeating the seed before the payload was ever built. Every Update
    sent an empty tag list, and the Read that immediately follows
    caught the tag missing and re-patched it — a spurious extra write
    on every steady-state apply.

    Fix: add initPolicyTagsSetForOutgoingPatch, the same conversion
    without the filter, for this one seed-before-build-and-patch use
    across all 8 call sites (gateway policy, security policy, VPC
    gateway policy, VPC group; Create and Update). The tag actually
    persisted to Terraform state is unaffected — each CRUD function's
    own trailing Read still overwrites "tag" via setPolicyTagsInSchema,
    which filters correctly for that purpose.

Both fixes include a mock-based regression test that fails against
the pre-fix code and passes with the fix.

@ksamoray
ksamoray requested a review from a team September 7, 2026 12:12
@ksamoray
ksamoray force-pushed the diagnostics/cache-search-race-logging branch from 0f7f7dd to c7cbada Compare September 7, 2026 12:50
terraform-cache-brownfield started failing after passing consistently,
across two unrelated bugs surfaced by config_scope cache mode:

1. IpAddressPoolStaticSubnet/IpAddressPoolBlockSubnet permanently
   bypassed the cache on every read. CacheAwareResourceRead has a
   shortIDBypass safety net: for path-indexed resource types, if the
   resourceID passed in doesn't contain "/", the cache is skipped
   entirely in favor of a direct GET, since a short id may not be
   unique across the project-wide search scope these types share.
   Both resources' Read functions unconditionally used the short
   d.Id() as that key instead of preferring d.Get("path") (populated
   since the prior Create) the way every other path-indexed resource
   does, so shortIDBypass fired on every single read, permanently.

   Fix: prefer d.Get("path"), falling back to d.Id() only when path
   isn't set yet (Create-then-Read within the same apply, or import) —
   matching the existing pattern already used by e.g. Service,
   SegmentPort, and PolicyNatRule.

2. GatewayPolicy, SecurityPolicy, and VpcGroup silently wiped their
   provider-managed cache tracking tag (scope="nsx-tf/tf-run-id") on
   every Update, including a bare description change with no config
   drift on tags. Their Create/Update seeds Terraform state's "tag"
   attribute with the merged (user + provider-managed) tag list
   immediately before building the outgoing NSX PATCH payload from
   that same state — the only way the shared build-and-patch helpers
   pick up the provider-managed tag for the payload. That seed used
   initPolicyTagsSet, which filters the provider-managed tag out — a
   filter added so Read never leaks the tag into state — silently
   defeating the seed before the payload was ever built. Every Update
   sent an empty tag list, and the Read that immediately follows
   caught the tag missing and re-patched it — a spurious extra write
   on every steady-state apply.

   Fix: add initPolicyTagsSetForOutgoingPatch, the same conversion
   without the filter, for this one seed-before-build-and-patch use
   across all 8 call sites (gateway policy, security policy, VPC
   gateway policy, VPC group; Create and Update). The tag actually
   persisted to Terraform state is unaffected — each CRUD function's
   own trailing Read still overwrites "tag" via setPolicyTagsInSchema,
   which filters correctly for that purpose.

Both fixes include a mock-based regression test that fails against
the pre-fix code and passes with the fix.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@ksamoray
ksamoray force-pushed the diagnostics/cache-search-race-logging branch from 745974f to ae2679e Compare September 8, 2026 11:14
@ksamoray

ksamoray commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

/test-all

1 similar comment
@ksamoray

ksamoray commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

/test-all

@ksamoray

ksamoray commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

/test-cache CONFIG_SCOPE

@ksamoray ksamoray changed the title Add diagnostics for cache-population race investigation Fix two terraform-cache-brownfield regressions in the resource cache Sep 8, 2026
@ksamoray

ksamoray commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

/test-cache

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.

1 participant