diff --git a/pkg/terraform/files.go b/pkg/terraform/files.go index e84409fe..4cd1953a 100644 --- a/pkg/terraform/files.go +++ b/pkg/terraform/files.go @@ -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 { diff --git a/pkg/terraform/files_test.go b/pkg/terraform/files_test.go index e86d30d6..9a1f32c6 100644 --- a/pkg/terraform/files_test.go +++ b/pkg/terraform/files_test.go @@ -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()} diff --git a/pkg/terraform/state.go b/pkg/terraform/state.go new file mode 100644 index 00000000..f9097039 --- /dev/null +++ b/pkg/terraform/state.go @@ -0,0 +1,42 @@ +// SPDX-FileCopyrightText: 2023 The Crossplane Authors +// +// 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 +} diff --git a/pkg/terraform/store.go b/pkg/terraform/store.go index 0dd1546a..9c8d8f89 100644 --- a/pkg/terraform/store.go +++ b/pkg/terraform/store.go @@ -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() @@ -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") diff --git a/pkg/terraform/workspace.go b/pkg/terraform/workspace.go index c58025b3..72bdffb4 100644 --- a/pkg/terraform/workspace.go +++ b/pkg/terraform/workspace.go @@ -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 { @@ -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{}, } @@ -141,6 +150,7 @@ type Workspace struct { filterFn func(string) string terraformID string + hasTFID bool } // UseProvider shares a native provider with the receiver Workspace. @@ -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 } @@ -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") + } return ImportResult{ - Exists: s.GetAttributes() != nil, + Exists: exists, State: s, }, nil } diff --git a/pkg/terraform/workspace_test.go b/pkg/terraform/workspace_test.go index d3485ea4..82c364ff 100644 --- a/pkg/terraform/workspace_test.go +++ b/pkg/terraform/workspace_test.go @@ -16,6 +16,7 @@ import ( k8sExec "k8s.io/utils/exec" testingexec "k8s.io/utils/exec/testing" + "github.com/crossplane/upjet/v2/pkg/resource/fake" "github.com/crossplane/upjet/v2/pkg/resource/json" tferrors "github.com/crossplane/upjet/v2/pkg/terraform/errors" ) @@ -72,6 +73,25 @@ func newFakeExec(stdOut string, err error) *testingexec.FakeExec { } } +func newFakeImportExec(fs afero.Afero, tfstate string, err error) *testingexec.FakeExec { + return &testingexec.FakeExec{ + CommandScript: []testingexec.FakeCommandAction{ + func(_ string, _ ...string) k8sExec.Cmd { + return &testingexec.FakeCmd{ + CombinedOutputScript: []testingexec.FakeAction{ + func() ([]byte, []byte, error) { + if err == nil { + _ = fs.WriteFile(directory+"terraform.tfstate", []byte(tfstate), 0o777) + } + return nil, nil, err + }, + }, + } + }, + }, + } +} + func TestWorkspaceApply(t *testing.T) { type args struct { w *Workspace @@ -252,11 +272,64 @@ func TestWorkspaceRefresh(t *testing.T) { err: tferrors.NewRefreshFailed([]byte(filter)), }, }, + "NullID": { + args: args{ + w: NewWorkspace( + directory, WithExecutor(&testingexec.FakeExec{DisableScripts: true}), WithAferoFs(fs), + WithFilterFn(filterFn)), + }, + want: want{ + r: RefreshResult{ + Exists: false, + State: &json.StateV4{ + Resources: []json.ResourceStateV4{ + { + Instances: []json.InstanceObjectStateV4{ + { + AttributesRaw: []byte(`{"id":null}`), + }, + }, + }, + }, + }, + }, + }, + }, + "IDlessSchema": { + args: args{ + w: NewWorkspace( + directory, WithExecutor(&testingexec.FakeExec{DisableScripts: true}), WithAferoFs(fs), + WithFilterFn(filterFn), WithWorkspaceHasIDAttribute(false)), + }, + want: want{ + r: RefreshResult{ + Exists: true, + State: &json.StateV4{ + Resources: []json.ResourceStateV4{ + { + Instances: []json.InstanceObjectStateV4{ + { + AttributesRaw: []byte(`{"name":"example"}`), + }, + }, + }, + }, + }, + }, + }, + }, } for name, tc := range cases { t.Run(name, func(t *testing.T) { - if err := tc.w.fs.WriteFile(directory+"terraform.tfstate", []byte(tfstate), 0777); err != nil { + statePayload := tfstate + switch name { + case "NullID": + statePayload = `{"resources":[{"instances":[{"attributes":{"id":null}}]}]}` + case "IDlessSchema": + statePayload = `{"resources":[{"instances":[{"attributes":{"name":"example"}}]}]}` + } + if err := tc.w.fs.WriteFile(directory+"terraform.tfstate", []byte(statePayload), 0777); err != nil { panic(err) } r, err := tc.w.Refresh(context.TODO()) @@ -270,6 +343,88 @@ func TestWorkspaceRefresh(t *testing.T) { } } +func TestWorkspaceImport(t *testing.T) { + type args struct { + w *Workspace + tr *fake.LegacyTerraformed + tfstate string + } + type want struct { + r ImportResult + err error + } + + newTerraformed := func() *fake.LegacyTerraformed { + return &fake.LegacyTerraformed{ + MetadataProvider: fake.MetadataProvider{ + Type: "upjet_resource", + }, + } + } + + cases := map[string]struct { + args + want + }{ + "NullID": { + args: args{ + w: NewWorkspace(directory, WithAferoFs(afero.NewMemMapFs()), WithFilterFn(filterFn)), + tr: newTerraformed(), + tfstate: `{"resources":[{"instances":[{"attributes":{"id":null}}]}]}`, + }, + want: want{ + r: ImportResult{ + Exists: false, + State: &json.StateV4{ + Resources: []json.ResourceStateV4{ + { + Instances: []json.InstanceObjectStateV4{ + {AttributesRaw: []byte(`{"id":null}`)}, + }, + }, + }, + }, + }, + }, + }, + "IDlessSchema": { + args: args{ + w: NewWorkspace(directory, WithAferoFs(afero.NewMemMapFs()), WithFilterFn(filterFn), WithWorkspaceHasIDAttribute(false)), + tr: newTerraformed(), + tfstate: `{"resources":[{"instances":[{"attributes":{"name":"example"}}]}]}`, + }, + want: want{ + r: ImportResult{ + Exists: true, + State: &json.StateV4{ + Resources: []json.ResourceStateV4{ + { + Instances: []json.InstanceObjectStateV4{ + {AttributesRaw: []byte(`{"name":"example"}`)}, + }, + }, + }, + }, + }, + }, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + tc.w.terraformID = "import-id" + tc.w.executor = newFakeImportExec(tc.w.fs, tc.args.tfstate, nil) + r, err := tc.w.Import(context.TODO(), tc.tr) + if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { + t.Errorf("\n%s\nImport(...): -want error, +got error:\n%s", name, diff) + } + if diff := cmp.Diff(tc.want.r, r, test.EquateErrors()); diff != "" { + t.Errorf("\n%s\nImport(...): -want result, +got result:\n%s", name, diff) + } + }) + } +} + func TestWorkspacePlan(t *testing.T) { type args struct { w *Workspace