|
| 1 | +## Upstream Upjet issue draft: file-backed workspace should treat `id: null` as empty state |
| 2 | + |
| 3 | +### Summary |
| 4 | + |
| 5 | +`provider-upjet-logfire` has a local dashboard connector wrapper because Upjet's |
| 6 | +file-backed Terraform workspace flow does not handle a Terraform state whose |
| 7 | +resource attributes contain `id: null`. |
| 8 | + |
| 9 | +For `IdentifierFromProvider` resources, Upjet seeds Terraform with an empty ID |
| 10 | +before the provider has created the external resource. A later |
| 11 | +`terraform apply -refresh-only` can convert that empty-ID state into a persisted |
| 12 | +null-state file. On the next reconcile, Upjet reopens that file and fails with: |
| 13 | + |
| 14 | +```text |
| 15 | +cannot work with a non-string id: <nil> |
| 16 | +``` |
| 17 | + |
| 18 | +The correct upstream behavior is to treat `id: null` the same way it treats an |
| 19 | +empty or missing ID: the resource should be considered absent, and the workspace |
| 20 | +should proceed with a clean create flow. |
| 21 | + |
| 22 | +### Why this is an Upjet bug |
| 23 | + |
| 24 | +- Direct Terraform succeeds against the same provider and backend. |
| 25 | +- Direct Pulumi succeeds against the same provider and backend. |
| 26 | +- The failure only appears in Upjet's file-backed workspace lifecycle. |
| 27 | +- The Terraform provider's dashboard `Read` behavior is valid for normal |
| 28 | + Terraform usage: if the state ID is empty, it removes the resource from state. |
| 29 | + |
| 30 | +Relevant local files: |
| 31 | + |
| 32 | +- `terraform-provider-logfire/internal/provider/dashboard_resource.go` |
| 33 | +- `provider-upjet-logfire/config/external_name.go` |
| 34 | +- `provider-upjet-logfire/internal/controller/dashboardconnector/connector.go` |
| 35 | + |
| 36 | +### Environment used to reproduce |
| 37 | + |
| 38 | +- Upjet: `github.com/crossplane/upjet/v2 v2.2.0` |
| 39 | +- `provider-upjet-logfire`: local controller build with current repo changes |
| 40 | +- Terraform provider framework: `github.com/hashicorp/terraform-plugin-framework v1.17.0` |
| 41 | +- Reproduced with a real Logfire API key and live backend resources |
| 42 | +- Reproduced both in a Crossplane cluster and with a standalone local Terraform |
| 43 | + state file |
| 44 | + |
| 45 | +### Minimal reproduction shape |
| 46 | + |
| 47 | +This is easiest to hit with a resource that uses `config.IdentifierFromProvider` |
| 48 | +and whose Terraform provider removes state when `Read` receives an empty ID. |
| 49 | + |
| 50 | +In `provider-upjet-logfire`, `logfire_dashboard` is one such resource: |
| 51 | + |
| 52 | +```go |
| 53 | +var ExternalNameConfigs = map[string]config.ExternalName{ |
| 54 | + "logfire_dashboard": config.IdentifierFromProvider, |
| 55 | +} |
| 56 | +``` |
| 57 | + |
| 58 | +And the Terraform provider's `Read` path does this: |
| 59 | + |
| 60 | +```go |
| 61 | +if state.ID.IsNull() || state.ID.IsUnknown() || state.ID.ValueString() == "" { |
| 62 | + resp.State.RemoveResource(ctx) |
| 63 | + return |
| 64 | +} |
| 65 | +``` |
| 66 | + |
| 67 | +### Concrete reproduction |
| 68 | + |
| 69 | +1. Seed a Terraform state file for a provider-assigned-ID resource with |
| 70 | + `attributes.id == ""`. |
| 71 | +2. Run `terraform apply -refresh-only`. |
| 72 | +3. Terraform rewrites the state file so the resource still exists structurally, |
| 73 | + but its attributes become null, including `id: null`. |
| 74 | +4. Let Upjet reopen that file-backed workspace. |
| 75 | +5. Upjet fails reopening/reconciling the resource because it expects `id` to be |
| 76 | + a string, not null. |
| 77 | + |
| 78 | +Observed state after `terraform apply -refresh-only`: |
| 79 | + |
| 80 | +```json |
| 81 | +{ |
| 82 | + "version": 4, |
| 83 | + "terraform_version": "1.14.8", |
| 84 | + "resources": [ |
| 85 | + { |
| 86 | + "mode": "managed", |
| 87 | + "type": "logfire_dashboard", |
| 88 | + "name": "test", |
| 89 | + "instances": [ |
| 90 | + { |
| 91 | + "schema_version": 0, |
| 92 | + "attributes": { |
| 93 | + "definition": null, |
| 94 | + "id": null, |
| 95 | + "name": null, |
| 96 | + "project_id": null, |
| 97 | + "slug": null |
| 98 | + } |
| 99 | + } |
| 100 | + ] |
| 101 | + } |
| 102 | + ] |
| 103 | +} |
| 104 | +``` |
| 105 | + |
| 106 | +### Observed runtime behavior |
| 107 | + |
| 108 | +In Crossplane/Upjet, the resource does not cleanly transition to "does not |
| 109 | +exist". Instead, a later reconcile trips over the persisted null-state file and |
| 110 | +fails before the normal create path can run again. |
| 111 | + |
| 112 | +The provider-local workaround in |
| 113 | +`provider-upjet-logfire/internal/controller/dashboardconnector/connector.go` |
| 114 | +does this: |
| 115 | + |
| 116 | +- inspect `/tmp/<uid>/terraform.tfstate` |
| 117 | +- detect `attributes.id == null` |
| 118 | +- delete that state file |
| 119 | +- treat the observe cycle as `ResourceExists=false` |
| 120 | + |
| 121 | +With that wrapper in place, live Crossplane tests succeeded: |
| 122 | + |
| 123 | +- `Project` reached `Ready=True`, `Synced=True` |
| 124 | +- `Dashboard` reached `Ready=True`, `Synced=True` |
| 125 | + |
| 126 | +### Expected upstream behavior |
| 127 | + |
| 128 | +Upjet should treat a file-backed Terraform state with `id: null` as empty state. |
| 129 | + |
| 130 | +That likely means one or both of these behaviors need to change upstream: |
| 131 | + |
| 132 | +1. State-empty detection should consider `id: null` equivalent to no resource. |
| 133 | +2. Workspace refresh/existence logic should not report the resource as present |
| 134 | + when the decoded state only contains null attributes. |
| 135 | + |
| 136 | +Likely upstream inspection points: |
| 137 | + |
| 138 | +- `pkg/terraform/files.go` |
| 139 | +- `pkg/terraform/workspace.go` |
| 140 | + |
| 141 | +The important rule is: |
| 142 | + |
| 143 | +> If the decoded resource state has a missing or null identifier, Upjet should |
| 144 | +> treat it as absent and recover to a clean create/observe path instead of |
| 145 | +> failing on type assertions or state reopening. |
| 146 | +
|
| 147 | +### Local workaround kept in `provider-upjet-logfire` |
| 148 | + |
| 149 | +Until Upjet is fixed upstream, `provider-upjet-logfire` keeps a narrow wrapper |
| 150 | +for dashboards: |
| 151 | + |
| 152 | +- `provider-upjet-logfire/internal/controller/dashboardconnector/connector.go` |
| 153 | +- `provider-upjet-logfire/internal/controller/dashboardconnector/connector_test.go` |
| 154 | + |
| 155 | +This is a containment fix, not the preferred long-term solution. |
| 156 | + |
| 157 | +### Suggested upstream regression test |
| 158 | + |
| 159 | +Add a test around the file-backed workspace/state-empty path that feeds Upjet a |
| 160 | +state whose resource attributes include `id: null`, then assert: |
| 161 | + |
| 162 | +- no error is returned while reopening/refreshing the workspace |
| 163 | +- the resource is treated as absent |
| 164 | +- a subsequent create flow is allowed to proceed normally |
| 165 | + |
| 166 | +### Notes on unrelated generation behavior |
| 167 | + |
| 168 | +`provider-upjet-logfire` will keep `make generate` as the standard generation |
| 169 | +entry point. Other standard Upjet providers also rely on Makefile-exported |
| 170 | +variables for the scraper and docs sync rather than supporting bare |
| 171 | +`go generate` as the primary path. |
0 commit comments