Skip to content

Handle null Terraform state ids #628

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
bruno-espino wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Handle null Terraform state ids #628

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 7 additions & 22 deletions pkg/terraform/files.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,30 +321,15 @@ func (fp *FileProducer) isStateEmpty() (bool, error) {
if err := json.JSParser.Unmarshal(data, s); err != nil {
return false, errors.Wrap(err, errUnmarshalTFState)
}
attrData := s.GetAttributes()
if attrData == nil {
return true, nil
}
attr := map[string]any{}
if err := json.JSParser.Unmarshal(attrData, &attr); err != nil {
return false, errors.Wrap(err, errUnmarshalAttr)
}

// for ID-less resource schemas, don't check for
// ID and assume empty when there is no attribute
if !fp.hasTFID {
return len(attr) == 0, nil
}

id, ok := attr["id"]
if !ok {
return true, nil
attr, err := stateAttributes(s.GetAttributes())
if err != nil {
return false, err
}
sid, ok := id.(string)
if !ok {
return false, errors.Errorf(errFmtNonString, fmt.Sprint(id))
exists, err := stateExists(attr, fp.hasTFID)
if err != nil {
return false, err
}
return sid == "", nil
return !exists, nil
}

type MainConfiguration struct {
Expand Down
26 changes: 25 additions & 1 deletion pkg/terraform/files_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,8 +276,32 @@ func TestIsStateEmpty(t *testing.T) {
empty: false,
},
},
"NullID": {
reason: "If the ID is null, treat the state as empty.",
args: args{
fs: func() afero.Afero {
f := afero.Afero{Fs: afero.NewMemMapFs()}
s := json.NewStateV4()
s.Resources = []json.ResourceStateV4{
{
Instances: []json.InstanceObjectStateV4{
{
AttributesRaw: []byte(`{"id": null}`),
},
},
},
}
d, _ := json.JSParser.Marshal(s)
_ = f.WriteFile(filepath.Join(dir, "terraform.tfstate"), d, 0600)
return f
},
},
want: want{
empty: true,
},
},
"NonStringID": {
reason: "If the ID is there but not string, return true.",
reason: "If the ID is there but not string, return an error.",
args: args{
fs: func() afero.Afero {
f := afero.Afero{Fs: afero.NewMemMapFs()}
Expand Down
42 changes: 42 additions & 0 deletions pkg/terraform/state.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
// SPDX-FileCopyrightText: 2023 The Crossplane Authors <https://crossplane.io>
//
// SPDX-License-Identifier: Apache-2.0

package terraform

import (
"fmt"

"github.com/pkg/errors"

"github.com/crossplane/upjet/v2/pkg/resource/json"
)

func stateAttributes(raw []byte) (map[string]any, error) {
if raw == nil {
return nil, nil
}
attr := map[string]any{}
if err := json.JSParser.Unmarshal(raw, &attr); err != nil {
return nil, errors.Wrap(err, errUnmarshalAttr)
}
return attr, nil
}

func stateExists(attr map[string]any, hasTFID bool) (bool, error) {
if attr == nil {
return false, nil
}
if !hasTFID {
return len(attr) != 0, nil
}
id, ok := attr["id"]
if !ok || id == nil {
return false, nil
}
sid, ok := id.(string)
if !ok {
return false, errors.Errorf(errFmtNonString, fmt.Sprint(id))
}
return sid != "", nil
}
19 changes: 12 additions & 7 deletions pkg/terraform/store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,11 +230,22 @@ func (ws *WorkspaceStore) Workspace(ctx context.Context, c resource.SecretClient
if err := ws.fs.MkdirAll(dir, os.ModePerm); err != nil {
return nil, errors.Wrap(err, "cannot create directory for workspace")
}
// these are guaranteed to be never nil, defensively check just in case
if cfg.TerraformResource == nil || cfg.TerraformResource.Schema == nil {
return nil, errors.New("no Terraform schema found for resource")
}
_, hasIDInSchema := cfg.TerraformResource.Schema["id"]
ws.mu.Lock()
w, ok := ws.store[tr.GetUID()]
if !ok {
l := ws.logger.WithValues("workspace", dir)
ws.store[tr.GetUID()] = NewWorkspace(dir, WithLogger(l), WithExecutor(ws.executor), WithFilterFn(ts.filterSensitiveInformation))
ws.store[tr.GetUID()] = NewWorkspace(
dir,
WithLogger(l),
WithExecutor(ws.executor),
WithFilterFn(ts.filterSensitiveInformation),
WithWorkspaceHasIDAttribute(hasIDInSchema),
)
w = ws.store[tr.GetUID()]
}
ws.mu.Unlock()
Expand All @@ -243,12 +254,6 @@ func (ws *WorkspaceStore) Workspace(ctx context.Context, c resource.SecretClient
if w.LastOperation.IsRunning() {
return w, nil
}

// these are guaranteed to be never nil, defensively check just in case
if cfg.TerraformResource == nil || cfg.TerraformResource.Schema == nil {
return nil, errors.New("no Terraform schema found for resource")
}
_, hasIDInSchema := cfg.TerraformResource.Schema["id"]
fp, err := NewFileProducer(ctx, c, dir, tr, ts, cfg, WithFileProducerFeatures(ws.features), WithHasIDAttribute(hasIDInSchema))
if err != nil {
return nil, errors.Wrap(err, "cannot create a new file producer")
Expand Down
30 changes: 28 additions & 2 deletions pkg/terraform/workspace.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,14 @@ func WithProviderInUse(providerInUse InUse) WorkspaceOption {
}
}

// WithWorkspaceHasIDAttribute configures whether the Terraform resource
// has an ID attribute in its schema.
func WithWorkspaceHasIDAttribute(hasTerraformID bool) WorkspaceOption {
return func(w *Workspace) {
w.hasTFID = hasTerraformID
}
}

// NewWorkspace returns a new Workspace object that operates in the given
// directory.
func NewWorkspace(dir string, opts ...WorkspaceOption) *Workspace {
Expand All @@ -106,6 +114,7 @@ func NewWorkspace(dir string, opts ...WorkspaceOption) *Workspace {
dir: dir,
logger: logging.NewNopLogger(),
fs: afero.Afero{Fs: afero.NewOsFs()},
hasTFID: true,
providerInUse: noopInUse{},
mu: &sync.Mutex{},
}
Expand Down Expand Up @@ -141,6 +150,7 @@ type Workspace struct {
filterFn func(string) string

terraformID string
hasTFID bool
}

// UseProvider shares a native provider with the receiver Workspace.
Expand Down Expand Up @@ -288,8 +298,16 @@ func (w *Workspace) Refresh(ctx context.Context) (RefreshResult, error) {
if err := json.JSParser.Unmarshal(raw, s); err != nil {
return RefreshResult{}, errors.Wrap(err, "cannot unmarshal tfstate file")
}
attr, err := stateAttributes(s.GetAttributes())
if err != nil {
return RefreshResult{}, errors.Wrap(err, "cannot unmarshal state attributes")
}
exists, err := stateExists(attr, w.hasTFID)
if err != nil {
return RefreshResult{}, errors.Wrap(err, "cannot determine whether refreshed state exists")
}
return RefreshResult{
Exists: s.GetAttributes() != nil,
Exists: exists,
State: s,
}, nil
}
Expand Down Expand Up @@ -393,8 +411,16 @@ func (w *Workspace) Import(ctx context.Context, tr resource.Terraformed) (Import
if err := json.JSParser.Unmarshal(raw, s); err != nil {
return ImportResult{}, errors.Wrap(err, "cannot unmarshal tfstate file")
}
attr, err := stateAttributes(s.GetAttributes())
if err != nil {
return ImportResult{}, errors.Wrap(err, "cannot unmarshal state attributes")
}
exists, err := stateExists(attr, w.hasTFID)
if err != nil {
return ImportResult{}, errors.Wrap(err, "cannot determine whether imported state exists")
}
Comment on lines +414 to +421
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify whether Import behavior is covered by tests in pkg/terraform.
rg -n --type=go -C3 'func TestWorkspaceImport|\\.Import\\(' pkg/terraform
rg -n --type=go -C3 'id":null|IDlessSchema|WithWorkspaceHasIDAttribute\\(false\\)' pkg/terraform

Repository: crossplane/upjet

Length of output: 2651

🏁 Script executed:

#!/bin/bash
# Find test function containing the id:null and IDlessSchema test cases
cd pkg/terraform
grep -n "func Test" workspace_test.go | head -20

Repository: crossplane/upjet

Length of output: 337

🏁 Script executed:

#!/bin/bash
# Check which test function contains the "NullID" and "IDlessSchema" cases
sed -n '260,320p' pkg/terraform/workspace_test.go

Repository: crossplane/upjet

Length of output: 1712

🏁 Script executed:

#!/bin/bash
# Verify if lines 414-421 are in Import or Refresh function
sed -n '400,430p' pkg/terraform/workspace.go | cat -n

Repository: crossplane/upjet

Length of output: 1297

🏁 Script executed:

#!/bin/bash
# Search comprehensively for any Import-related tests
rg -n "func.*Import|\.Import\(" pkg/terraform/workspace_test.go

Repository: crossplane/upjet

Length of output: 42

Add test cases for the Import path with null IDs and ID-less schemas

The new stateAttributes and stateExists logic is now used in both Refresh and Import (lines 414–421), but Import lacks test coverage. You added "NullID" and "IDlessSchema" test cases in TestWorkspaceRefresh—can you add equivalent cases to a TestWorkspaceImport function to ensure the Import path handles these scenarios correctly?

Per coding guidelines, "**/*.go: All Upjet code must be covered by tests."

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@pkg/terraform/workspace.go` around lines 414 - 421, Add a new
TestWorkspaceImport that mirrors TestWorkspaceRefresh and includes the "NullID"
and "IDlessSchema" cases so the Import path exercises the new stateAttributes
and stateExists logic; for each case construct the same resource/state fixtures
used in TestWorkspaceRefresh, call the workspace Import path (the same method
that hits the code invoking stateAttributes and stateExists), and assert the
import result/behavior matches expectations (no panics and correct existence
handling). Ensure the test names are "NullID" and "IDlessSchema", reuse the same
setup/fixtures/assertions from TestWorkspaceRefresh, and place the test
alongside other workspace tests so stateAttributes/stateExists are covered by
unit tests.

return ImportResult{
Exists: s.GetAttributes() != nil,
Exists: exists,
State: s,
}, nil
}
Expand Down
Loading