Skip to content

Commit 7c23758

Browse files
hugodutkahugodutka
committed
set module path to sentinel value when parsing failed and log an error
1 parent e266cc7 commit 7c23758

File tree

3 files changed

+44
-19
lines changed

3 files changed

+44
-19
lines changed

‎provisioner/terraform/executor.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -356,7 +356,7 @@ func (e *executor) planResources(ctx, killCtx context.Context, planfilePath stri
356356
}
357357
modules = append(modules, plan.PlannedValues.RootModule)
358358

359-
state, err := ConvertState(modules, rawGraph)
359+
state, err := ConvertState(ctx, modules, rawGraph, e.server.logger)
360360
if err != nil {
361361
return nil, err
362362
}
@@ -484,9 +484,9 @@ func (e *executor) stateResources(ctx, killCtx context.Context) (*State, error)
484484
return converted, nil
485485
}
486486

487-
converted, err = ConvertState([]*tfjson.StateModule{
487+
converted, err = ConvertState(ctx, []*tfjson.StateModule{
488488
state.Values.RootModule,
489-
}, rawGraph)
489+
}, rawGraph, e.server.logger)
490490
if err != nil {
491491
return nil, err
492492
}

‎provisioner/terraform/resources.go

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package terraform
22

33
import (
4+
"context"
45
"fmt"
56
"strings"
67

@@ -9,6 +10,8 @@ import (
910
"github.com/mitchellh/mapstructure"
1011
"golang.org/x/xerrors"
1112

13+
"cdr.dev/slog"
14+
1215
"github.com/coder/terraform-provider-coder/provider"
1316

1417
tfaddr "github.com/hashicorp/go-terraform-address"
@@ -134,7 +137,7 @@ type State struct {
134137
// ConvertState consumes Terraform state and a GraphViz representation
135138
// produced by `terraform graph` to produce resources consumable by Coder.
136139
// nolint:gocognit // This function makes more sense being large for now, until refactored.
137-
func ConvertState(modules []*tfjson.StateModule, rawGraph string) (*State, error) {
140+
func ConvertState(ctx context.Context, modules []*tfjson.StateModule, rawGraph string, logger slog.Logger) (*State, error) {
138141
parsedGraph, err := gographviz.ParseString(rawGraph)
139142
if err != nil {
140143
return nil, xerrors.Errorf("parse graph: %w", err)
@@ -598,9 +601,16 @@ func ConvertState(modules []*tfjson.StateModule, rawGraph string) (*State, error
598601
modulePath, err := convertAddressToModulePath(resource.Address)
599602
if err != nil {
600603
// Module path recording was added primarily to keep track of
601-
// modules in telemetry. We're adding this fallback so we can
602-
// detect if there are any issues with the address parsing.
603-
modulePath = fmt.Sprintf("error parsing address: %s", resource.Address)
604+
// modules in telemetry. We're adding this sentinel value so
605+
// we can detect if there are any issues with the address
606+
// parsing.
607+
//
608+
// We don't want to set modulePath to null here because, in
609+
// the database, a null value in WorkspaceResource's ModulePath
610+
// indicates "this resource was created before module paths
611+
// were tracked."
612+
modulePath = "FAILED_TO_PARSE_TERRAFORM_ADDRESS"
613+
logger.Error(ctx, "failed to parse Terraform address", slog.F("address", resource.Address))
604614
}
605615

606616
agents, exists := resourceAgents[label]

‎provisioner/terraform/resources_test.go

Lines changed: 27 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package terraform_test
22

33
import (
4+
"context"
45
"encoding/json"
56
"fmt"
67
"os"
@@ -14,11 +15,18 @@ import (
1415
"github.com/stretchr/testify/require"
1516
protobuf "google.golang.org/protobuf/proto"
1617

18+
"cdr.dev/slog"
19+
"cdr.dev/slog/sloggers/slogtest"
20+
1721
"github.com/coder/coder/v2/cryptorand"
1822
"github.com/coder/coder/v2/provisioner/terraform"
1923
"github.com/coder/coder/v2/provisionersdk/proto"
2024
)
2125

26+
func ctxAndLogger(t *testing.T) (context.Context, slog.Logger) {
27+
return context.Background(), slogtest.Make(t, nil).Leveled(slog.LevelDebug)
28+
}
29+
2230
func TestConvertResources(t *testing.T) {
2331
t.Parallel()
2432
// nolint:dogsled
@@ -688,6 +696,7 @@ func TestConvertResources(t *testing.T) {
688696
dir := filepath.Join(filepath.Dir(filename), "testdata", folderName)
689697
t.Run("Plan", func(t *testing.T) {
690698
t.Parallel()
699+
ctx, logger := ctxAndLogger(t)
691700

692701
tfPlanRaw, err := os.ReadFile(filepath.Join(dir, folderName+".tfplan.json"))
693702
require.NoError(t, err)
@@ -705,7 +714,7 @@ func TestConvertResources(t *testing.T) {
705714
// and that no errors occur!
706715
modules = append(modules, tfPlan.PlannedValues.RootModule)
707716
}
708-
state, err := terraform.ConvertState(modules, string(tfPlanGraph))
717+
state, err := terraform.ConvertState(ctx, modules, string(tfPlanGraph), logger)
709718
require.NoError(t, err)
710719
sortResources(state.Resources)
711720
sortExternalAuthProviders(state.ExternalAuthProviders)
@@ -763,6 +772,7 @@ func TestConvertResources(t *testing.T) {
763772

764773
t.Run("Provision", func(t *testing.T) {
765774
t.Parallel()
775+
ctx, logger := ctxAndLogger(t)
766776
tfStateRaw, err := os.ReadFile(filepath.Join(dir, folderName+".tfstate.json"))
767777
require.NoError(t, err)
768778
var tfState tfjson.State
@@ -771,7 +781,7 @@ func TestConvertResources(t *testing.T) {
771781
tfStateGraph, err := os.ReadFile(filepath.Join(dir, folderName+".tfstate.dot"))
772782
require.NoError(t, err)
773783

774-
state, err := terraform.ConvertState([]*tfjson.StateModule{tfState.Values.RootModule}, string(tfStateGraph))
784+
state, err := terraform.ConvertState(ctx, []*tfjson.StateModule{tfState.Values.RootModule}, string(tfStateGraph), logger)
775785
require.NoError(t, err)
776786
sortResources(state.Resources)
777787
sortExternalAuthProviders(state.ExternalAuthProviders)
@@ -808,6 +818,7 @@ func TestConvertResources(t *testing.T) {
808818

809819
func TestAppSlugValidation(t *testing.T) {
810820
t.Parallel()
821+
ctx, logger := ctxAndLogger(t)
811822

812823
// nolint:dogsled
813824
_, filename, _, _ := runtime.Caller(0)
@@ -829,7 +840,7 @@ func TestAppSlugValidation(t *testing.T) {
829840
}
830841
}
831842

832-
state, err := terraform.ConvertState([]*tfjson.StateModule{tfPlan.PlannedValues.RootModule}, string(tfPlanGraph))
843+
state, err := terraform.ConvertState(ctx, []*tfjson.StateModule{tfPlan.PlannedValues.RootModule}, string(tfPlanGraph), logger)
833844
require.Nil(t, state)
834845
require.Error(t, err)
835846
require.ErrorContains(t, err, "invalid app slug")
@@ -841,14 +852,15 @@ func TestAppSlugValidation(t *testing.T) {
841852
}
842853
}
843854

844-
state, err = terraform.ConvertState([]*tfjson.StateModule{tfPlan.PlannedValues.RootModule}, string(tfPlanGraph))
855+
state, err = terraform.ConvertState(ctx, []*tfjson.StateModule{tfPlan.PlannedValues.RootModule}, string(tfPlanGraph), logger)
845856
require.Nil(t, state)
846857
require.Error(t, err)
847858
require.ErrorContains(t, err, "duplicate app slug")
848859
}
849860

850861
func TestMetadataResourceDuplicate(t *testing.T) {
851862
t.Parallel()
863+
ctx, logger := ctxAndLogger(t)
852864

853865
// Load the multiple-apps state file and edit it.
854866
dir := filepath.Join("testdata", "resource-metadata-duplicate")
@@ -860,14 +872,15 @@ func TestMetadataResourceDuplicate(t *testing.T) {
860872
tfPlanGraph, err := os.ReadFile(filepath.Join(dir, "resource-metadata-duplicate.tfplan.dot"))
861873
require.NoError(t, err)
862874

863-
state, err := terraform.ConvertState([]*tfjson.StateModule{tfPlan.PlannedValues.RootModule}, string(tfPlanGraph))
875+
state, err := terraform.ConvertState(ctx, []*tfjson.StateModule{tfPlan.PlannedValues.RootModule}, string(tfPlanGraph), logger)
864876
require.Nil(t, state)
865877
require.Error(t, err)
866878
require.ErrorContains(t, err, "duplicate metadata resource: null_resource.about")
867879
}
868880

869881
func TestParameterValidation(t *testing.T) {
870882
t.Parallel()
883+
ctx, logger := ctxAndLogger(t)
871884

872885
// nolint:dogsled
873886
_, filename, _, _ := runtime.Caller(0)
@@ -891,7 +904,7 @@ func TestParameterValidation(t *testing.T) {
891904
}
892905
}
893906

894-
state, err := terraform.ConvertState([]*tfjson.StateModule{tfPlan.PriorState.Values.RootModule}, string(tfPlanGraph))
907+
state, err := terraform.ConvertState(ctx, []*tfjson.StateModule{tfPlan.PriorState.Values.RootModule}, string(tfPlanGraph), logger)
895908
require.Nil(t, state)
896909
require.Error(t, err)
897910
require.ErrorContains(t, err, "coder_parameter names must be unique but \"identical\" appears multiple times")
@@ -907,7 +920,7 @@ func TestParameterValidation(t *testing.T) {
907920
}
908921
}
909922

910-
state, err = terraform.ConvertState([]*tfjson.StateModule{tfPlan.PriorState.Values.RootModule}, string(tfPlanGraph))
923+
state, err = terraform.ConvertState(ctx, []*tfjson.StateModule{tfPlan.PriorState.Values.RootModule}, string(tfPlanGraph), logger)
911924
require.Nil(t, state)
912925
require.Error(t, err)
913926
require.ErrorContains(t, err, "coder_parameter names must be unique but \"identical-0\" and \"identical-1\" appear multiple times")
@@ -923,7 +936,7 @@ func TestParameterValidation(t *testing.T) {
923936
}
924937
}
925938

926-
state, err = terraform.ConvertState([]*tfjson.StateModule{tfPlan.PriorState.Values.RootModule}, string(tfPlanGraph))
939+
state, err = terraform.ConvertState(ctx, []*tfjson.StateModule{tfPlan.PriorState.Values.RootModule}, string(tfPlanGraph), logger)
927940
require.Nil(t, state)
928941
require.Error(t, err)
929942
require.ErrorContains(t, err, "coder_parameter names must be unique but \"identical-0\", \"identical-1\" and \"identical-2\" appear multiple times")
@@ -954,9 +967,10 @@ func TestInstanceTypeAssociation(t *testing.T) {
954967
tc := tc
955968
t.Run(tc.ResourceType, func(t *testing.T) {
956969
t.Parallel()
970+
ctx, logger := ctxAndLogger(t)
957971
instanceType, err := cryptorand.String(12)
958972
require.NoError(t, err)
959-
state, err := terraform.ConvertState([]*tfjson.StateModule{{
973+
state, err := terraform.ConvertState(ctx, []*tfjson.StateModule{{
960974
Resources: []*tfjson.StateResource{{
961975
Address: tc.ResourceType + ".dev",
962976
Type: tc.ResourceType,
@@ -973,7 +987,7 @@ func TestInstanceTypeAssociation(t *testing.T) {
973987
subgraph "root" {
974988
"[root] `+tc.ResourceType+`.dev" [label = "`+tc.ResourceType+`.dev", shape = "box"]
975989
}
976-
}`)
990+
}`, logger)
977991
require.NoError(t, err)
978992
require.Len(t, state.Resources, 1)
979993
require.Equal(t, state.Resources[0].GetInstanceType(), instanceType)
@@ -1012,9 +1026,10 @@ func TestInstanceIDAssociation(t *testing.T) {
10121026
tc := tc
10131027
t.Run(tc.ResourceType, func(t *testing.T) {
10141028
t.Parallel()
1029+
ctx, logger := ctxAndLogger(t)
10151030
instanceID, err := cryptorand.String(12)
10161031
require.NoError(t, err)
1017-
state, err := terraform.ConvertState([]*tfjson.StateModule{{
1032+
state, err := terraform.ConvertState(ctx, []*tfjson.StateModule{{
10181033
Resources: []*tfjson.StateResource{{
10191034
Address: "coder_agent.dev",
10201035
Type: "coder_agent",
@@ -1044,7 +1059,7 @@ func TestInstanceIDAssociation(t *testing.T) {
10441059
"[root] `+tc.ResourceType+`.dev" -> "[root] coder_agent.dev"
10451060
}
10461061
}
1047-
`)
1062+
`, logger)
10481063
require.NoError(t, err)
10491064
require.Len(t, state.Resources, 1)
10501065
require.Len(t, state.Resources[0].Agents, 1)

0 commit comments

Comments
 (0)