From b23aed034f78691980c219a4f1e274a6c659e642 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 24 Mar 2026 13:02:45 +0200 Subject: [PATCH] fix: make terraform ConvertState fully deterministic (#23459) All map iterations in ConvertState now use sorted helpers instead of ranging over Go maps directly. Previously only coder_env and coder_script were sorted (via sortedResourcesByType). This extends the pattern to coder_agent, coder_devcontainer, coder_agent_instance, coder_app, coder_metadata, coder_external_auth, and the main resource output list. Also fixes generate.sh writing version.txt to the wrong directory (resources/ instead of testdata/), which caused the Makefile version check to silently desync and trigger unnecessary regeneration. Adds TestConvertStateDeterministic that calls ConvertState 10 times per fixture and asserts byte-identical JSON output without any post-hoc sorting. --- Makefile | 2 +- provisioner/terraform/convertstate_test.go | 98 ++ provisioner/terraform/resources.go | 956 +++++++++--------- provisioner/terraform/testdata/generate.sh | 2 +- .../terraform/testdata/resources/version.txt | 1 - 5 files changed, 583 insertions(+), 476 deletions(-) delete mode 100644 provisioner/terraform/testdata/resources/version.txt diff --git a/Makefile b/Makefile index a96f918f49..11e30edb64 100644 --- a/Makefile +++ b/Makefile @@ -1255,7 +1255,7 @@ coderd/notifications/.gen-golden: $(wildcard coderd/notifications/testdata/*/*.g TZ=UTC go test ./coderd/notifications -run="Test.*Golden$$" -update touch "$@" -provisioner/terraform/testdata/.gen-golden: $(wildcard provisioner/terraform/testdata/*/*.golden) $(GO_SRC_FILES) $(wildcard provisioner/terraform/*_test.go) +provisioner/terraform/testdata/.gen-golden: $(wildcard provisioner/terraform/testdata/*/*.golden) $(wildcard provisioner/terraform/testdata/*/*/*.golden) $(GO_SRC_FILES) $(wildcard provisioner/terraform/*_test.go) TZ=UTC go test ./provisioner/terraform -run="Test.*Golden$$" -update touch "$@" diff --git a/provisioner/terraform/convertstate_test.go b/provisioner/terraform/convertstate_test.go index 3e5cdbc7fb..d2e8aa2dcc 100644 --- a/provisioner/terraform/convertstate_test.go +++ b/provisioner/terraform/convertstate_test.go @@ -127,3 +127,101 @@ func TestConvertStateGolden(t *testing.T) { } } } + +// TestConvertStateDeterministic verifies that ConvertState produces +// identical output across multiple runs. This catches non-deterministic +// map iteration in the implementation. Unlike TestConvertStateGolden, +// this test does NOT sort the output — it relies on ConvertState itself +// being deterministic. +func TestConvertStateDeterministic(t *testing.T) { + t.Parallel() + + testResourceDirectories := filepath.Join("testdata", "resources") + entries, err := os.ReadDir(testResourceDirectories) + require.NoError(t, err) + + for _, testDirectory := range entries { + if !testDirectory.IsDir() { + continue + } + + testFiles, err := os.ReadDir(filepath.Join(testResourceDirectories, testDirectory.Name())) + require.NoError(t, err) + + for _, step := range []string{"plan", "state"} { + srcIdx := slices.IndexFunc(testFiles, func(entry os.DirEntry) bool { + return strings.HasSuffix(entry.Name(), fmt.Sprintf(".tf%s.json", step)) + }) + dotIdx := slices.IndexFunc(testFiles, func(entry os.DirEntry) bool { + return strings.HasSuffix(entry.Name(), fmt.Sprintf(".tf%s.dot", step)) + }) + + if srcIdx == -1 || dotIdx == -1 { + continue + } + + t.Run(step+"_"+testDirectory.Name(), func(t *testing.T) { + t.Parallel() + testDirectoryPath := filepath.Join(testResourceDirectories, testDirectory.Name()) + planFile := filepath.Join(testDirectoryPath, testFiles[srcIdx].Name()) + dotFile := filepath.Join(testDirectoryPath, testFiles[dotIdx].Name()) + + ctx := testutil.Context(t, testutil.WaitMedium) + logger := slogtest.Make(t, nil) + + tfStepRaw, err := os.ReadFile(planFile) + require.NoError(t, err) + + var modules []*tfjson.StateModule + switch step { + case "plan": + var tfPlan tfjson.Plan + err = json.Unmarshal(tfStepRaw, &tfPlan) + require.NoError(t, err) + modules = []*tfjson.StateModule{tfPlan.PlannedValues.RootModule} + if tfPlan.PriorState != nil { + modules = append(modules, tfPlan.PriorState.Values.RootModule) + } + case "state": + var tfState tfjson.State + err = json.Unmarshal(tfStepRaw, &tfState) + require.NoError(t, err) + modules = []*tfjson.StateModule{tfState.Values.RootModule} + default: + t.Fatalf("unknown step: %s", step) + } + + dotFileRaw, err := os.ReadFile(dotFile) + require.NoError(t, err) + + // Run ConvertState 10 times and verify all runs + // produce byte-identical JSON without any sorting. + // We apply deterministicAppIDs because plan files + // lack provider-assigned IDs, causing ConvertState + // to generate random UUIDs as a fallback. + // + // Note: json.Marshal sorts map keys, so this test + // cannot catch non-determinism in map-valued fields + // like Agent.Env. Those are populated from static + // testdata today, so this is not a practical gap. + const runs = 10 + outputs := make([][]byte, runs) + for i := range runs { + state, err := terraform.ConvertState(ctx, modules, string(dotFileRaw), logger) + if err != nil { + // Error strings are deterministic. + outputs[i] = []byte(err.Error()) + continue + } + deterministicAppIDs(state.Resources) + outputs[i], err = json.Marshal(state) + require.NoError(t, err, "run %d: marshal state", i) + } + for i := 1; i < runs; i++ { + require.Equal(t, string(outputs[0]), string(outputs[i]), + "ConvertState produced different output on run %d vs run 0", i) + } + }) + } + } +} diff --git a/provisioner/terraform/resources.go b/provisioner/terraform/resources.go index 8c7c28a436..d4c19038af 100644 --- a/provisioner/terraform/resources.go +++ b/provisioner/terraform/resources.go @@ -257,381 +257,364 @@ func ConvertState(ctx context.Context, modules []*tfjson.StateModule, rawGraph s findTerraformResources(module) } + // Group all resources by type in a single pass so that + // subsequent lookups are O(1) instead of scanning the + // full map each time. + sortedResources := sortResourcesByType(tfResourcesByLabel) + // Find all agents! agentNames := map[string]struct{}{} - for _, tfResources := range tfResourcesByLabel { - for _, tfResource := range tfResources { - if tfResource.Type != "coder_agent" { - continue - } - var attrs agentAttributes - err = mapstructure.Decode(tfResource.AttributeValues, &attrs) - if err != nil { - return nil, xerrors.Errorf("decode agent attributes: %w", err) - } - - // Similar logic is duplicated in terraform/resources.go. - if tfResource.Name == "" { - return nil, xerrors.Errorf("agent name cannot be empty") - } - // In 2025-02 we removed support for underscores in agent names. To - // provide a nicer error message, we check the regex first and check - // for underscores if it fails. - if !provisioner.AgentNameRegex.MatchString(tfResource.Name) { - if strings.Contains(tfResource.Name, "_") { - return nil, xerrors.Errorf("agent name %q contains underscores which are no longer supported, please use hyphens instead (regex: %q)", tfResource.Name, provisioner.AgentNameRegex.String()) - } - return nil, xerrors.Errorf("agent name %q does not match regex %q", tfResource.Name, provisioner.AgentNameRegex.String()) - } - // Agent names must be case-insensitive-unique, to be unambiguous in - // `coder_app`s and CoderVPN DNS names. - if _, ok := agentNames[strings.ToLower(tfResource.Name)]; ok { - return nil, xerrors.Errorf("duplicate agent name: %s", tfResource.Name) - } - agentNames[strings.ToLower(tfResource.Name)] = struct{}{} - - // Handling for deprecated attributes. login_before_ready was replaced - // by startup_script_behavior, but we still need to support it for - // backwards compatibility. - startupScriptBehavior := string(codersdk.WorkspaceAgentStartupScriptBehaviorNonBlocking) - if attrs.StartupScriptBehavior != "" { - startupScriptBehavior = attrs.StartupScriptBehavior - } else { - // Handling for provider pre-v0.6.10 (because login_before_ready - // defaulted to true, we must check for its presence). - if _, ok := tfResource.AttributeValues["login_before_ready"]; ok && !attrs.LoginBeforeReady { - startupScriptBehavior = string(codersdk.WorkspaceAgentStartupScriptBehaviorBlocking) - } - } - - var metadata []*proto.Agent_Metadata - for _, item := range attrs.Metadata { - metadata = append(metadata, &proto.Agent_Metadata{ - Key: item.Key, - DisplayName: item.DisplayName, - Script: item.Script, - Interval: item.Interval, - Timeout: item.Timeout, - Order: item.Order, - }) - } - - // If a user doesn't specify 'display_apps' then they default - // into all apps except VSCode Insiders. - displayApps := provisionersdk.DefaultDisplayApps() - - if len(attrs.DisplayApps) != 0 { - displayApps = &proto.DisplayApps{ - Vscode: attrs.DisplayApps[0].VSCode, - VscodeInsiders: attrs.DisplayApps[0].VSCodeInsiders, - WebTerminal: attrs.DisplayApps[0].WebTerminal, - PortForwardingHelper: attrs.DisplayApps[0].PortForwardingHelper, - SshHelper: attrs.DisplayApps[0].SSHHelper, - } - } - - resourcesMonitoring := &proto.ResourcesMonitoring{ - Volumes: make([]*proto.VolumeResourceMonitor, 0), - } - - for _, resource := range attrs.ResourcesMonitoring { - for _, memoryResource := range resource.Memory { - resourcesMonitoring.Memory = &proto.MemoryResourceMonitor{ - Enabled: memoryResource.Enabled, - Threshold: memoryResource.Threshold, - } - } - } - - for _, resource := range attrs.ResourcesMonitoring { - for _, volume := range resource.Volumes { - resourcesMonitoring.Volumes = append(resourcesMonitoring.Volumes, &proto.VolumeResourceMonitor{ - Path: volume.Path, - Enabled: volume.Enabled, - Threshold: volume.Threshold, - }) - } - } - - agent := &proto.Agent{ - Name: tfResource.Name, - Id: attrs.ID, - Env: attrs.Env, - OperatingSystem: attrs.OperatingSystem, - Architecture: attrs.Architecture, - Directory: attrs.Directory, - ConnectionTimeoutSeconds: attrs.ConnectionTimeoutSeconds, - TroubleshootingUrl: attrs.TroubleshootingURL, - MotdFile: attrs.MOTDFile, - ResourcesMonitoring: resourcesMonitoring, - Metadata: metadata, - DisplayApps: displayApps, - Order: attrs.Order, - ApiKeyScope: attrs.APIKeyScope, - } - // Support the legacy script attributes in the agent! - if attrs.StartupScript != "" { - agent.Scripts = append(agent.Scripts, &proto.Script{ - // This is ▶️ - Icon: "/emojis/25b6-fe0f.png", - LogPath: "coder-startup-script.log", - DisplayName: "Startup Script", - Script: attrs.StartupScript, - StartBlocksLogin: startupScriptBehavior == string(codersdk.WorkspaceAgentStartupScriptBehaviorBlocking), - RunOnStart: true, - }) - } - if attrs.ShutdownScript != "" { - agent.Scripts = append(agent.Scripts, &proto.Script{ - // This is ◀️ - Icon: "/emojis/25c0.png", - LogPath: "coder-shutdown-script.log", - DisplayName: "Shutdown Script", - Script: attrs.ShutdownScript, - RunOnStop: true, - }) - } - switch attrs.Auth { - case "token": - agent.Auth = &proto.Agent_Token{ - Token: attrs.Token, - } - default: - // If token authentication isn't specified, - // assume instance auth. It's our only other - // authentication type! - agent.Auth = &proto.Agent_InstanceId{} - } - - // The label is used to find the graph node! - agentLabel := convertAddressToLabel(tfResource.Address) - - var agentNode *gographviz.Node - for _, node := range graph.Nodes.Lookup { - // The node attributes surround the label with quotes. - if strings.Trim(node.Attrs["label"], `"`) != agentLabel { - continue - } - agentNode = node - break - } - if agentNode == nil { - return nil, xerrors.Errorf("couldn't find node on graph: %q", agentLabel) - } - - var agentResource *graphResource - for _, resource := range findResourcesInGraph(graph, tfResourcesByLabel, agentNode.Name, 0, true) { - if agentResource == nil { - // Default to the first resource because we have nothing to compare! - agentResource = resource - continue - } - if resource.Depth < agentResource.Depth { - // There's a closer resource! - agentResource = resource - continue - } - if resource.Depth == agentResource.Depth && resource.Label < agentResource.Label { - agentResource = resource - continue - } - } - - if agentResource == nil { - continue - } - - agents, exists := resourceAgents[agentResource.Label] - if !exists { - agents = make([]*proto.Agent, 0, 1) - } - agents = append(agents, agent) - resourceAgents[agentResource.Label] = agents + for _, tfResource := range sortedResources["coder_agent"] { + var attrs agentAttributes + err = mapstructure.Decode(tfResource.AttributeValues, &attrs) + if err != nil { + return nil, xerrors.Errorf("decode agent attributes: %w", err) } + + // Similar logic is duplicated in terraform/resources.go. + if tfResource.Name == "" { + return nil, xerrors.Errorf("agent name cannot be empty") + } + // In 2025-02 we removed support for underscores in agent names. To + // provide a nicer error message, we check the regex first and check + // for underscores if it fails. + if !provisioner.AgentNameRegex.MatchString(tfResource.Name) { + if strings.Contains(tfResource.Name, "_") { + return nil, xerrors.Errorf("agent name %q contains underscores which are no longer supported, please use hyphens instead (regex: %q)", tfResource.Name, provisioner.AgentNameRegex.String()) + } + return nil, xerrors.Errorf("agent name %q does not match regex %q", tfResource.Name, provisioner.AgentNameRegex.String()) + } + // Agent names must be case-insensitive-unique, to be unambiguous in + // `coder_app`s and CoderVPN DNS names. + if _, ok := agentNames[strings.ToLower(tfResource.Name)]; ok { + return nil, xerrors.Errorf("duplicate agent name: %s", tfResource.Name) + } + agentNames[strings.ToLower(tfResource.Name)] = struct{}{} + + // Handling for deprecated attributes. login_before_ready was replaced + // by startup_script_behavior, but we still need to support it for + // backwards compatibility. + startupScriptBehavior := string(codersdk.WorkspaceAgentStartupScriptBehaviorNonBlocking) + if attrs.StartupScriptBehavior != "" { + startupScriptBehavior = attrs.StartupScriptBehavior + } else { + // Handling for provider pre-v0.6.10 (because login_before_ready + // defaulted to true, we must check for its presence). + if _, ok := tfResource.AttributeValues["login_before_ready"]; ok && !attrs.LoginBeforeReady { + startupScriptBehavior = string(codersdk.WorkspaceAgentStartupScriptBehaviorBlocking) + } + } + + var metadata []*proto.Agent_Metadata + for _, item := range attrs.Metadata { + metadata = append(metadata, &proto.Agent_Metadata{ + Key: item.Key, + DisplayName: item.DisplayName, + Script: item.Script, + Interval: item.Interval, + Timeout: item.Timeout, + Order: item.Order, + }) + } + + // If a user doesn't specify 'display_apps' then they default + // into all apps except VSCode Insiders. + displayApps := provisionersdk.DefaultDisplayApps() + + if len(attrs.DisplayApps) != 0 { + displayApps = &proto.DisplayApps{ + Vscode: attrs.DisplayApps[0].VSCode, + VscodeInsiders: attrs.DisplayApps[0].VSCodeInsiders, + WebTerminal: attrs.DisplayApps[0].WebTerminal, + PortForwardingHelper: attrs.DisplayApps[0].PortForwardingHelper, + SshHelper: attrs.DisplayApps[0].SSHHelper, + } + } + + resourcesMonitoring := &proto.ResourcesMonitoring{ + Volumes: make([]*proto.VolumeResourceMonitor, 0), + } + + for _, resource := range attrs.ResourcesMonitoring { + for _, memoryResource := range resource.Memory { + resourcesMonitoring.Memory = &proto.MemoryResourceMonitor{ + Enabled: memoryResource.Enabled, + Threshold: memoryResource.Threshold, + } + } + } + + for _, resource := range attrs.ResourcesMonitoring { + for _, volume := range resource.Volumes { + resourcesMonitoring.Volumes = append(resourcesMonitoring.Volumes, &proto.VolumeResourceMonitor{ + Path: volume.Path, + Enabled: volume.Enabled, + Threshold: volume.Threshold, + }) + } + } + + agent := &proto.Agent{ + Name: tfResource.Name, + Id: attrs.ID, + Env: attrs.Env, + OperatingSystem: attrs.OperatingSystem, + Architecture: attrs.Architecture, + Directory: attrs.Directory, + ConnectionTimeoutSeconds: attrs.ConnectionTimeoutSeconds, + TroubleshootingUrl: attrs.TroubleshootingURL, + MotdFile: attrs.MOTDFile, + ResourcesMonitoring: resourcesMonitoring, + Metadata: metadata, + DisplayApps: displayApps, + Order: attrs.Order, + ApiKeyScope: attrs.APIKeyScope, + } + // Support the legacy script attributes in the agent! + if attrs.StartupScript != "" { + agent.Scripts = append(agent.Scripts, &proto.Script{ + // This is ▶️ + Icon: "/emojis/25b6-fe0f.png", + LogPath: "coder-startup-script.log", + DisplayName: "Startup Script", + Script: attrs.StartupScript, + StartBlocksLogin: startupScriptBehavior == string(codersdk.WorkspaceAgentStartupScriptBehaviorBlocking), + RunOnStart: true, + }) + } + if attrs.ShutdownScript != "" { + agent.Scripts = append(agent.Scripts, &proto.Script{ + // This is ◀️ + Icon: "/emojis/25c0.png", + LogPath: "coder-shutdown-script.log", + DisplayName: "Shutdown Script", + Script: attrs.ShutdownScript, + RunOnStop: true, + }) + } + switch attrs.Auth { + case "token": + agent.Auth = &proto.Agent_Token{ + Token: attrs.Token, + } + default: + // If token authentication isn't specified, + // assume instance auth. It's our only other + // authentication type! + agent.Auth = &proto.Agent_InstanceId{} + } + + // The label is used to find the graph node! + agentLabel := convertAddressToLabel(tfResource.Address) + + var agentNode *gographviz.Node + for _, node := range graph.Nodes.Lookup { + // The node attributes surround the label with quotes. + if strings.Trim(node.Attrs["label"], `"`) != agentLabel { + continue + } + agentNode = node + break + } + if agentNode == nil { + return nil, xerrors.Errorf("couldn't find node on graph: %q", agentLabel) + } + + var agentResource *graphResource + for _, resource := range findResourcesInGraph(graph, tfResourcesByLabel, agentNode.Name, 0, true) { + if agentResource == nil { + // Default to the first resource because we have nothing to compare! + agentResource = resource + continue + } + if resource.Depth < agentResource.Depth { + // There's a closer resource! + agentResource = resource + continue + } + if resource.Depth == agentResource.Depth && resource.Label < agentResource.Label { + agentResource = resource + continue + } + } + + if agentResource == nil { + continue + } + + agents, exists := resourceAgents[agentResource.Label] + if !exists { + agents = make([]*proto.Agent, 0, 1) + } + agents = append(agents, agent) + resourceAgents[agentResource.Label] = agents } // Associate Dev Containers with agents. - for _, resources := range tfResourcesByLabel { - for _, resource := range resources { - if resource.Type != "coder_devcontainer" { - continue - } - var attrs agentDevcontainerAttributes - err = mapstructure.Decode(resource.AttributeValues, &attrs) - if err != nil { - return nil, xerrors.Errorf("decode devcontainer attributes: %w", err) - } - for _, agents := range resourceAgents { - for _, agent := range agents { - // Find agents with the matching ID and associate them! - if !dependsOnAgent(graph, agent, attrs.AgentID, resource) { - continue - } - - agent.Devcontainers = append(agent.Devcontainers, &proto.Devcontainer{ - Id: attrs.ID, - Name: resource.Name, - WorkspaceFolder: attrs.WorkspaceFolder, - ConfigPath: attrs.ConfigPath, - SubagentId: attrs.SubAgentID, - }) + for _, resource := range sortedResources["coder_devcontainer"] { + var attrs agentDevcontainerAttributes + err = mapstructure.Decode(resource.AttributeValues, &attrs) + if err != nil { + return nil, xerrors.Errorf("decode devcontainer attributes: %w", err) + } + for _, agents := range resourceAgents { + for _, agent := range agents { + // Find agents with the matching ID and associate them! + if !dependsOnAgent(graph, agent, attrs.AgentID, resource) { + continue } + + agent.Devcontainers = append(agent.Devcontainers, &proto.Devcontainer{ + Id: attrs.ID, + Name: resource.Name, + WorkspaceFolder: attrs.WorkspaceFolder, + ConfigPath: attrs.ConfigPath, + SubagentId: attrs.SubAgentID, + }) } } } // Manually associate agents with instance IDs. - for _, resources := range tfResourcesByLabel { - for _, resource := range resources { - if resource.Type != "coder_agent_instance" { - continue - } - agentIDRaw, valid := resource.AttributeValues["agent_id"] - if !valid { - continue - } - agentID, valid := agentIDRaw.(string) - if !valid { - continue - } - instanceIDRaw, valid := resource.AttributeValues["instance_id"] - if !valid { - continue - } - instanceID, valid := instanceIDRaw.(string) - if !valid { - continue - } + for _, resource := range sortedResources["coder_agent_instance"] { + agentIDRaw, valid := resource.AttributeValues["agent_id"] + if !valid { + continue + } + agentID, valid := agentIDRaw.(string) + if !valid { + continue + } + instanceIDRaw, valid := resource.AttributeValues["instance_id"] + if !valid { + continue + } + instanceID, valid := instanceIDRaw.(string) + if !valid { + continue + } - for _, agents := range resourceAgents { - for _, agent := range agents { - if agent.Id != agentID { - continue - } - // Only apply the instance ID if the agent authentication - // type is set to do so. A user ran into a bug where they - // had the instance ID block, but auth was set to "token". See: - // https://github.com/coder/coder/issues/4551#issuecomment-1336293468 - switch t := agent.Auth.(type) { - case *proto.Agent_Token: - continue - case *proto.Agent_InstanceId: - t.InstanceId = instanceID - } - break + for _, agents := range resourceAgents { + for _, agent := range agents { + if agent.Id != agentID { + continue } + // Only apply the instance ID if the agent authentication + // type is set to do so. A user ran into a bug where they + // had the instance ID block, but auth was set to "token". See: + // https://github.com/coder/coder/issues/4551#issuecomment-1336293468 + switch t := agent.Auth.(type) { + case *proto.Agent_Token: + continue + case *proto.Agent_InstanceId: + t.InstanceId = instanceID + } + break } } } // Associate Apps with agents. appSlugs := make(map[string]struct{}) - for _, resources := range tfResourcesByLabel { - for _, resource := range resources { - if resource.Type != "coder_app" { - continue - } + for _, resource := range sortedResources["coder_app"] { + var attrs agentAppAttributes + err = mapstructure.Decode(resource.AttributeValues, &attrs) + if err != nil { + return nil, xerrors.Errorf("decode app attributes: %w", err) + } - var attrs agentAppAttributes - err = mapstructure.Decode(resource.AttributeValues, &attrs) - if err != nil { - return nil, xerrors.Errorf("decode app attributes: %w", err) + // Default to the resource name if none is set! + if attrs.Slug == "" { + attrs.Slug = resource.Name + } + // Similar logic is duplicated in terraform/resources.go. + if attrs.DisplayName == "" { + if attrs.Name != "" { + // Name is deprecated but still accepted. + attrs.DisplayName = attrs.Name + } else { + attrs.DisplayName = attrs.Slug } + } - // Default to the resource name if none is set! - if attrs.Slug == "" { - attrs.Slug = resource.Name + // Contrary to agent names above, app slugs were never permitted to + // contain uppercase letters or underscores. + if !provisioner.AppSlugRegex.MatchString(attrs.Slug) { + return nil, xerrors.Errorf("app slug %q does not match regex %q", attrs.Slug, provisioner.AppSlugRegex.String()) + } + + if _, exists := appSlugs[attrs.Slug]; exists { + return nil, xerrors.Errorf("duplicate app slug, they must be unique per template: %q", attrs.Slug) + } + appSlugs[attrs.Slug] = struct{}{} + + var healthcheck *proto.Healthcheck + if len(attrs.Healthcheck) != 0 { + healthcheck = &proto.Healthcheck{ + Url: attrs.Healthcheck[0].URL, + Interval: attrs.Healthcheck[0].Interval, + Threshold: attrs.Healthcheck[0].Threshold, } - // Similar logic is duplicated in terraform/resources.go. - if attrs.DisplayName == "" { - if attrs.Name != "" { - // Name is deprecated but still accepted. - attrs.DisplayName = attrs.Name - } else { - attrs.DisplayName = attrs.Slug + } + + sharingLevel := proto.AppSharingLevel_OWNER + switch strings.ToLower(attrs.Share) { + case "owner": + sharingLevel = proto.AppSharingLevel_OWNER + case "authenticated": + sharingLevel = proto.AppSharingLevel_AUTHENTICATED + case "public": + sharingLevel = proto.AppSharingLevel_PUBLIC + } + + openIn := proto.AppOpenIn_SLIM_WINDOW + switch strings.ToLower(attrs.OpenIn) { + case "slim-window": + openIn = proto.AppOpenIn_SLIM_WINDOW + case "tab": + openIn = proto.AppOpenIn_TAB + } + + appID := attrs.ID + if appID == "" { + // This should never happen since the "id" attribute is set on creation: + // https://github.com/coder/terraform-provider-coder/blob/cfa101df4635e405e66094fa7779f9a89d92f400/provider/app.go#L37 + logger.Warn(ctx, "coder_app's id was unexpectedly empty", slog.F("name", attrs.Name)) + + appID = uuid.NewString() + } + app := &proto.App{ + Id: appID, + Slug: attrs.Slug, + DisplayName: attrs.DisplayName, + Command: attrs.Command, + External: attrs.External, + Url: attrs.URL, + Icon: attrs.Icon, + Subdomain: attrs.Subdomain, + SharingLevel: sharingLevel, + Healthcheck: healthcheck, + Order: attrs.Order, + Group: attrs.Group, + Hidden: attrs.Hidden, + OpenIn: openIn, + Tooltip: attrs.Tooltip, + } + + appAgentLoop: + for _, agents := range resourceAgents { + for _, agent := range agents { + // Find agents with the matching ID and associate them! + if dependsOnAgent(graph, agent, attrs.AgentID, resource) { + agent.Apps = append(agent.Apps, app) + break appAgentLoop } - } - // Contrary to agent names above, app slugs were never permitted to - // contain uppercase letters or underscores. - if !provisioner.AppSlugRegex.MatchString(attrs.Slug) { - return nil, xerrors.Errorf("app slug %q does not match regex %q", attrs.Slug, provisioner.AppSlugRegex.String()) - } - - if _, exists := appSlugs[attrs.Slug]; exists { - return nil, xerrors.Errorf("duplicate app slug, they must be unique per template: %q", attrs.Slug) - } - appSlugs[attrs.Slug] = struct{}{} - - var healthcheck *proto.Healthcheck - if len(attrs.Healthcheck) != 0 { - healthcheck = &proto.Healthcheck{ - Url: attrs.Healthcheck[0].URL, - Interval: attrs.Healthcheck[0].Interval, - Threshold: attrs.Healthcheck[0].Threshold, - } - } - - sharingLevel := proto.AppSharingLevel_OWNER - switch strings.ToLower(attrs.Share) { - case "owner": - sharingLevel = proto.AppSharingLevel_OWNER - case "authenticated": - sharingLevel = proto.AppSharingLevel_AUTHENTICATED - case "public": - sharingLevel = proto.AppSharingLevel_PUBLIC - } - - openIn := proto.AppOpenIn_SLIM_WINDOW - switch strings.ToLower(attrs.OpenIn) { - case "slim-window": - openIn = proto.AppOpenIn_SLIM_WINDOW - case "tab": - openIn = proto.AppOpenIn_TAB - } - - appID := attrs.ID - if appID == "" { - // This should never happen since the "id" attribute is set on creation: - // https://github.com/coder/terraform-provider-coder/blob/cfa101df4635e405e66094fa7779f9a89d92f400/provider/app.go#L37 - logger.Warn(ctx, "coder_app's id was unexpectedly empty", slog.F("name", attrs.Name)) - - appID = uuid.NewString() - } - - app := &proto.App{ - Id: appID, - Slug: attrs.Slug, - DisplayName: attrs.DisplayName, - Command: attrs.Command, - External: attrs.External, - Url: attrs.URL, - Icon: attrs.Icon, - Subdomain: attrs.Subdomain, - SharingLevel: sharingLevel, - Healthcheck: healthcheck, - Order: attrs.Order, - Group: attrs.Group, - Hidden: attrs.Hidden, - OpenIn: openIn, - Tooltip: attrs.Tooltip, - } - - appAgentLoop: - for _, agents := range resourceAgents { - for _, agent := range agents { - // Find agents with the matching ID and associate them! - if dependsOnAgent(graph, agent, attrs.AgentID, resource) { - agent.Apps = append(agent.Apps, app) + for _, dc := range agent.GetDevcontainers() { + if dependsOnDevcontainer(graph, dc, attrs.AgentID, resource) { + dc.Apps = append(dc.Apps, app) break appAgentLoop } - - for _, dc := range agent.GetDevcontainers() { - if dependsOnDevcontainer(graph, dc, attrs.AgentID, resource) { - dc.Apps = append(dc.Apps, app) - break appAgentLoop - } - } } } } @@ -641,7 +624,7 @@ func ConvertState(ctx context.Context, modules []*tfjson.StateModule, rawGraph s // Collect and sort env resources by address for deterministic ordering. // When multiple coder_env resources define the same key, the last one // by sorted address wins, ensuring stable behavior across builds. - sortedEnvResources := sortedResourcesByType(tfResourcesByLabel, "coder_env") + sortedEnvResources := sortedResources["coder_env"] for _, resource := range sortedEnvResources { var attrs agentEnvAttributes err = mapstructure.Decode(resource.AttributeValues, &attrs) @@ -676,7 +659,7 @@ func ConvertState(ctx context.Context, modules []*tfjson.StateModule, rawGraph s // Associate scripts with agents. // Sort for deterministic ordering, same as envs above. - sortedScriptResources := sortedResourcesByType(tfResourcesByLabel, "coder_script") + sortedScriptResources := sortedResources["coder_script"] for _, resource := range sortedScriptResources { var attrs agentScriptAttributes err = mapstructure.Decode(resource.AttributeValues, &attrs) @@ -721,114 +704,100 @@ func ConvertState(ctx context.Context, modules []*tfjson.StateModule, rawGraph s resourceCost := map[string]int32{} metadataTargetLabels := map[string]bool{} - for _, resources := range tfResourcesByLabel { - for _, resource := range resources { - if resource.Type != "coder_metadata" { + for _, resource := range sortedResources["coder_metadata"] { + var attrs resourceMetadataAttributes + err = mapstructure.Decode(resource.AttributeValues, &attrs) + if err != nil { + return nil, xerrors.Errorf("decode metadata attributes: %w", err) + } + resourceLabel := convertAddressToLabel(resource.Address) + + var attachedNode *gographviz.Node + for _, node := range graph.Nodes.Lookup { + // The node attributes surround the label with quotes. + if strings.Trim(node.Attrs["label"], `"`) != resourceLabel { continue } - - var attrs resourceMetadataAttributes - err = mapstructure.Decode(resource.AttributeValues, &attrs) - if err != nil { - return nil, xerrors.Errorf("decode metadata attributes: %w", err) - } - resourceLabel := convertAddressToLabel(resource.Address) - - var attachedNode *gographviz.Node - for _, node := range graph.Nodes.Lookup { - // The node attributes surround the label with quotes. - if strings.Trim(node.Attrs["label"], `"`) != resourceLabel { - continue - } - attachedNode = node - break - } - if attachedNode == nil { - continue - } - var attachedResource *graphResource - for _, resource := range findResourcesInGraph(graph, tfResourcesByLabel, attachedNode.Name, 0, false) { - if attachedResource == nil { - // Default to the first resource because we have nothing to compare! - attachedResource = resource - continue - } - if resource.Depth < attachedResource.Depth { - // There's a closer resource! - attachedResource = resource - continue - } - if resource.Depth == attachedResource.Depth && resource.Label < attachedResource.Label { - attachedResource = resource - continue - } - } + attachedNode = node + break + } + if attachedNode == nil { + continue + } + var attachedResource *graphResource + for _, resource := range findResourcesInGraph(graph, tfResourcesByLabel, attachedNode.Name, 0, false) { if attachedResource == nil { + // Default to the first resource because we have nothing to compare! + attachedResource = resource continue } - targetLabel := attachedResource.Label - - if metadataTargetLabels[targetLabel] { - return nil, xerrors.Errorf("duplicate metadata resource: %s", targetLabel) + if resource.Depth < attachedResource.Depth { + // There's a closer resource! + attachedResource = resource + continue } - metadataTargetLabels[targetLabel] = true - - resourceHidden[targetLabel] = attrs.Hide - resourceIcon[targetLabel] = attrs.Icon - resourceCost[targetLabel] = attrs.DailyCost - for _, item := range attrs.Items { - resourceMetadata[targetLabel] = append(resourceMetadata[targetLabel], - &proto.Resource_Metadata{ - Key: item.Key, - Value: item.Value, - Sensitive: item.Sensitive, - IsNull: item.IsNull, - }) + if resource.Depth == attachedResource.Depth && resource.Label < attachedResource.Label { + attachedResource = resource + continue } } + if attachedResource == nil { + continue + } + targetLabel := attachedResource.Label + + if metadataTargetLabels[targetLabel] { + return nil, xerrors.Errorf("duplicate metadata resource: %s", targetLabel) + } + metadataTargetLabels[targetLabel] = true + + resourceHidden[targetLabel] = attrs.Hide + resourceIcon[targetLabel] = attrs.Icon + resourceCost[targetLabel] = attrs.DailyCost + for _, item := range attrs.Items { + resourceMetadata[targetLabel] = append(resourceMetadata[targetLabel], + &proto.Resource_Metadata{ + Key: item.Key, + Value: item.Value, + Sensitive: item.Sensitive, + IsNull: item.IsNull, + }) + } } - for _, tfResources := range tfResourcesByLabel { - for _, resource := range tfResources { - if resource.Mode == tfjson.DataResourceMode { - continue - } - if resource.Type == "coder_script" || resource.Type == "coder_agent" || resource.Type == "coder_agent_instance" || resource.Type == "coder_app" || resource.Type == "coder_metadata" { - continue - } - label := convertAddressToLabel(resource.Address) - modulePath, err := convertAddressToModulePath(resource.Address) - if err != nil { - // Module path recording was added primarily to keep track of - // modules in telemetry. We're adding this sentinel value so - // we can detect if there are any issues with the address - // parsing. - // - // We don't want to set modulePath to null here because, in - // the database, a null value in WorkspaceResource's ModulePath - // indicates "this resource was created before module paths - // were tracked." - modulePath = fmt.Sprintf("%s", ErrInvalidTerraformAddr) - logger.Error(ctx, "failed to parse Terraform address", slog.F("address", resource.Address)) - } - - agents, exists := resourceAgents[label] - if exists { - applyAutomaticInstanceID(resource, agents) - } - - resources = append(resources, &proto.Resource{ - Name: resource.Name, - Type: resource.Type, - Agents: agents, - Metadata: resourceMetadata[label], - Hide: resourceHidden[label], - Icon: resourceIcon[label], - DailyCost: resourceCost[label], - InstanceType: applyInstanceType(resource), - ModulePath: modulePath, - }) + for _, resource := range managedNonCoderResources(sortedResources) { + label := convertAddressToLabel(resource.Address) + modulePath, err := convertAddressToModulePath(resource.Address) + if err != nil { + // Module path recording was added primarily to keep track of + // modules in telemetry. We're adding this sentinel value so + // we can detect if there are any issues with the address + // parsing. + // + // We don't want to set modulePath to null here because, in + // the database, a null value in WorkspaceResource's ModulePath + // indicates "this resource was created before module paths + // were tracked." + modulePath = fmt.Sprintf("%s", ErrInvalidTerraformAddr) + logger.Error(ctx, "failed to parse Terraform address", slog.F("address", resource.Address)) } + + agents, exists := resourceAgents[label] + if exists { + applyAutomaticInstanceID(resource, agents) + } + + resources = append(resources, &proto.Resource{ + Name: resource.Name, + Type: resource.Type, + Agents: agents, + Metadata: resourceMetadata[label], + Hide: resourceHidden[label], + Icon: resourceIcon[label], + DailyCost: resourceCost[label], + InstanceType: applyInstanceType(resource), + ModulePath: modulePath, + }) } var duplicatedParamNames []string @@ -1072,34 +1041,48 @@ func ConvertState(ctx context.Context, modules []*tfjson.StateModule, rawGraph s // A map is used to ensure we don't have duplicates! externalAuthProvidersMap := map[string]*proto.ExternalAuthProviderResource{} - for _, tfResources := range tfResourcesByLabel { - for _, resource := range tfResources { - // Checking for `coder_git_auth` is legacy! - if resource.Type != "coder_external_auth" && resource.Type != "coder_git_auth" { - continue - } + // Process the legacy coder_git_auth type first so that + // coder_external_auth takes precedence when both exist + // with the same provider ID. + for _, resource := range sortedResources["coder_git_auth"] { + id, ok := resource.AttributeValues["id"].(string) + if !ok { + return nil, xerrors.Errorf("external auth id is not a string") + } + optional := false + optionalAttribute, ok := resource.AttributeValues["optional"].(bool) + if ok { + optional = optionalAttribute + } - id, ok := resource.AttributeValues["id"].(string) - if !ok { - return nil, xerrors.Errorf("external auth id is not a string") - } - optional := false - optionalAttribute, ok := resource.AttributeValues["optional"].(bool) - if ok { - optional = optionalAttribute - } + externalAuthProvidersMap[id] = &proto.ExternalAuthProviderResource{ + Id: id, + Optional: optional, + } + } + for _, resource := range sortedResources["coder_external_auth"] { + id, ok := resource.AttributeValues["id"].(string) + if !ok { + return nil, xerrors.Errorf("external auth id is not a string") + } + optional := false + optionalAttribute, ok := resource.AttributeValues["optional"].(bool) + if ok { + optional = optionalAttribute + } - externalAuthProvidersMap[id] = &proto.ExternalAuthProviderResource{ - Id: id, - Optional: optional, - } + externalAuthProvidersMap[id] = &proto.ExternalAuthProviderResource{ + Id: id, + Optional: optional, } } externalAuthProviders := make([]*proto.ExternalAuthProviderResource, 0, len(externalAuthProvidersMap)) for _, it := range externalAuthProvidersMap { externalAuthProviders = append(externalAuthProviders, it) } - + slices.SortFunc(externalAuthProviders, func(a, b *proto.ExternalAuthProviderResource) int { + return cmp.Compare(a.Id, b.Id) + }) hasAITasks := hasAITaskResources(graph) return &State{ @@ -1144,16 +1127,43 @@ func safeInt32Conversion(n int) int32 { return int32(n) } -// sortedResourcesByType collects all resources of the given type from the -// label map and returns them sorted by address. This ensures deterministic -// iteration order when processing resources that are stored in Go maps. -func sortedResourcesByType(tfResourcesByLabel map[string]map[string]*tfjson.StateResource, resourceType string) []*tfjson.StateResource { - var result []*tfjson.StateResource +// sortResourcesByType performs a single pass over the label map and +// returns all resources grouped by type, each group sorted by address. +// Callers index the result by type to get a deterministic slice. +func sortResourcesByType(tfResourcesByLabel map[string]map[string]*tfjson.StateResource) map[string][]*tfjson.StateResource { + byType := map[string][]*tfjson.StateResource{} for _, resources := range tfResourcesByLabel { for _, resource := range resources { - if resource.Type == resourceType { - result = append(result, resource) + byType[resource.Type] = append(byType[resource.Type], resource) + } + } + for _, resources := range byType { + slices.SortFunc(resources, func(a, b *tfjson.StateResource) int { + return cmp.Compare(a.Address, b.Address) + }) + } + return byType +} + +// managedNonCoderResources returns all managed resources that are not +// internal Coder types, sorted by address. It uses the pre-grouped +// map from sortResourcesByType. +func managedNonCoderResources(byType map[string][]*tfjson.StateResource) []*tfjson.StateResource { + skip := map[string]bool{ + "coder_script": true, "coder_agent": true, + "coder_agent_instance": true, "coder_app": true, + "coder_metadata": true, + } + var result []*tfjson.StateResource + for resourceType, resources := range byType { + if skip[resourceType] { + continue + } + for _, resource := range resources { + if resource.Mode == tfjson.DataResourceMode { + continue } + result = append(result, resource) } } slices.SortFunc(result, func(a, b *tfjson.StateResource) int { diff --git a/provisioner/terraform/testdata/generate.sh b/provisioner/terraform/testdata/generate.sh index 03e2e0507a..c98f558385 100755 --- a/provisioner/terraform/testdata/generate.sh +++ b/provisioner/terraform/testdata/generate.sh @@ -138,4 +138,4 @@ if [[ $err -ne 0 ]]; then exit 1 fi -terraform version -json | jq -r '.terraform_version' >version.txt +terraform version -json | jq -r '.terraform_version' >../version.txt diff --git a/provisioner/terraform/testdata/resources/version.txt b/provisioner/terraform/testdata/resources/version.txt deleted file mode 100644 index 24a57f28a4..0000000000 --- a/provisioner/terraform/testdata/resources/version.txt +++ /dev/null @@ -1 +0,0 @@ -1.14.5