fix(permission-grant): validate namespace exists during plan

[Hongyi23]

Fixes #209

Motivation

When a service adds a pulsar_permission_grant that references a namespace which doesn't exist yet, terraform plan currently succeeds — a false green. The failure only surfaces at terraform apply as a 404 Namespace not found from the Pulsar API, meaning the misconfiguration isn't caught until after the PR is merged and the CD pipeline runs.

The root cause is that pulsar_permission_grant performs no namespace existence validation during plan; since plan doesn't make live API calls for the resource, the missing dependency is silently ignored until apply.

Modifications

• Added a CustomizeDiff to pulsar_permission_grant that checks, during plan, whether the referenced namespace exists (both for the namespace form and the namespace owning a topic grant), returning a diagnostic error if it is provably absent.
• The check deliberately errs toward not failing the plan: it skips when the input is unparseable, when meta is nil (e.g. terraform validate), or when the tenant's namespace listing fails (covers the tenant not existing yet during a fresh apply, plus transient/network errors). It only fails when the listing succeeds and the target namespace is absent — avoiding false positives when tenant/namespace are created in the same run.

Verifying this change

• Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

• Unit tests (TestVerifyGrantNamespaceExists) covering: namespace exists/missing, topic-namespace exists/missing, listing-error skip, malformed input skip, and neither-set skip.
• Acceptance test (TestPermissionGrantNamespaceDoesNotExist) that provisions a tenant, then adds a grant against a never-declared namespace and asserts the plan fails.
• Ran the full TestPermissionGrant acceptance suite locally against Pulsar 4.0.3 — all pass, confirming no regression to existing grant behavior.

Documentation

• no-need-doc: no schema/field changes; behavior-only addition surfaced as a plan-time diagnostic.

[github-actions]

@Hongyi23:Thanks for your contribution. For this PR, do we need to update docs?
(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

[github-actions]

@Hongyi23:Thanks for providing doc info!

[freeznet]

Thanks for the thorough writeup, the unit table, and for being explicit about the skip conditions in the code comments — that made this easy to reason about. The goal is sound, and #209's exact config (a bare namespace string under an existing tenant) is genuinely caught here.

Requesting changes, though: the check also fails plans for namespaces that are declared in the same config, which breaks configs that apply cleanly today.

Blocking — false positive when the namespace is created in the same apply

The guard described in the PR body ("skips … when the tenant's namespace listing fails … avoiding false positives when tenant/namespace are created in the same run") only holds while the tenant is also absent. Once the tenant already exists — managed in another state/module, or simply created by an earlier apply — GetNamespaces succeeds, and a namespace that is planned-but-not-yet-created is indistinguishable from one that will never exist.

Repro: step 1 creates cluster + tenant; step 2 adds the namespace and the grant together, using the same interpolation form as this repo's existing tests:

resource "pulsar_namespace" "test_namespace" {
  tenant    = pulsar_tenant.test_tenant.tenant
  namespace = "my-ns"
}

resource "pulsar_permission_grant" "test" {
  namespace = "${pulsar_tenant.test_tenant.tenant}/${pulsar_namespace.test_namespace.namespace}"
  role      = "some-role"
  actions   = ["produce", "consume"]
}

On this branch, against hack/pulsar-docker.sh Pulsar:

Step 2/2 error: Error running pre-apply refresh: exit status 1

Error: namespace "3jylrrsucn/0mkwlzi4yr" does not exist; create it before granting permissions (referenced by pulsar_permission_grant)

  with pulsar_permission_grant.test,
  on terraform_plugin_test.tf line 26, in resource "pulsar_permission_grant" "test":

The byte-identical test passes on master.

Both interpolated attributes are set in config, so the grant's namespace is fully known at plan time even though the namespace resource is only planned, not created. CustomizeDiff has no visibility into other resources in the plan, so it cannot distinguish the two cases.

The existing suite doesn't catch this because every current test creates the tenant in the same step as the grant, so the listing errors and the check is skipped. testPulsarPermissionGrantNamespace (~L290) and the configs at ~L629 / ~L678 all use exactly the interpolation form above — the blind spot sits right where the regression is. I did confirm the full TestPermissionGrant* suite passes on this branch, as you reported.

The check fires or not depending on how the reference is spelled

Change one line of the same config to reference the computed id — which for pulsar_namespace is literally tenant/namespace:

namespace = pulsar_namespace.test_namespace.id

…and it passes on this branch, because .id is unknown at plan time, so diff.GetOk returns "" and the check silently no-ops. Two spellings that produce an identical final value get opposite plan outcomes.

Same on the topic path: I verified topic = pulsar_topic.tp.id — the form used in the docs and the existing tests — never reaches the check at all, even with a pre-existing tenant.

The true-positive surface is narrower than it looks

Because any listing error is skipped, a misspelled tenant — arguably the more common typo — still only surfaces at apply. What's actually caught is "correct existing tenant + absent namespace + name known at plan time and not declared elsewhere in the config".

Suggested directions

1. Fix diagnosability without touching plan (my preference): turn the apply-time 404 into an actionable error — namespace %q does not exist; create it first, e.g. reference pulsar_namespace.<name>.id so Terraform orders the operations — plus a docs example using that reference. In #209's config the namespace is a bare string with no dependency edge, so even if it existed the ordering would be luck; referencing the resource fixes the root cause and makes plan-time validation unnecessary.
2. Make the strict gate opt-in if CI-time failure is really wanted: a provider-level flag (e.g. validate_namespace_exists, default false). Strict pipelines get the false-green fix; everyone else keeps working plans.
3. If it must stay on by default, at minimum gate explicitly on diff.NewValueKnown(...) instead of relying on GetOk returning "" — but that alone doesn't address the known-value case above, so I don't think it's sufficient.

Happy to re-review on whichever direction you pick.

[freeznet]

This is where the false positive lands. contains returning false means "not in the tenant's namespace list right now" — but on a create plan the namespace may be declared in the same config and simply not created yet. CustomizeDiff has no view of other resources in the plan, so absence here isn't evidence of a misconfiguration.

See the repro in the review summary: this fires on config that applies cleanly on master.

[freeznet]

GetOk returns "" for values that are unknown at plan time, and that is the only reason references like pulsar_namespace.x.id don't trip the check. That's incidental rather than intentional — worth gating explicitly on diff.NewValueKnown("namespace") / diff.NewValueKnown("topic") and skipping when either is unknown, so the intent survives future edits.

Note the resulting behavior is spelling-dependent: namespace = pulsar_namespace.x.id (unknown, skipped) and namespace = "${...tenant}/${...namespace}" (known, checked) resolve to the same value but get opposite plan outcomes.

[freeznet]

The SplitN on String() isn't needed — the tenant is already in hand in both branches. The namespace branch can split the raw namespace input (or carry a tenant var through the switch), and the topic branch already calls topicName.GetTenant().

The comment is accurate that NameSpaceName exposes no tenant accessor (fields are unexported), but round-tripping through String() just to re-split obscures that the value was already available.

[freeznet]

One GetNamespaces call per grant resource on every plan, including no-op plans, and each response is the tenant's full namespace list. A config with N grants on one tenant does N identical listings.

If this approach survives, memoize per tenant for the life of the diff, or use a targeted lookup (e.g. GetPolicies(ns) and treat 404 as absent) rather than listing everything.

[freeznet]

Nit: terraform validate doesn't reach CustomizeDiff — it calls ValidateResourceConfig, not PlanResourceChange. The nil guard is worth keeping as defensive code, since getClientFromMeta does an unchecked meta.(PulsarClientBundle), but the stated reason is misleading.

[freeznet]

This covers the case where the namespace is never declared. The gap is the mirror case: tenant already exists and the namespace is declared in the same config.

Adding that as a step here would have caught the regression — step 1 cluster + tenant, step 2 the same plus pulsar_namespace and a grant referencing it via "${pulsar_tenant.test_tenant.tenant}/${pulsar_namespace.test_namespace.namespace}", expecting success. It passes on master and fails on this branch.

[sleungtoast]

Thank you @freeznet for the thorough review. My coworker @Hongyi23 is on leave for the next while so I am picking up his work.

The issues you brought up are very valid and I think your point about "referencing the resource" is probably the correct approach. Our particular use case doesn't have access to the resource directly, so we would need a Terraform data source. With that capability, we can do a lookup for the namespace, referencing that instead of a bare string, and shift the check there instead of changing the behavior of the pulsar_permission_grant plan itself.

We could give a go at creating data sources for pulsar_topic and pulsar_namespace - lmk if you have thoughts, thanks!

[sleungtoast]

We've come up with an alternative - see #209 (comment) for details. Please close, otherwise we will have Hongyi close when he comes back from leave in a few months.