mirror of
https://github.com/coder/coder.git
synced 2026-10-16 14:48:56 +08:00
feat: attach script order dependencies during conversion (#30556)
feat(provisioner): transport resolved script order dependencies Attach finalized `coder_script_order` dependencies to scripts during START imports, covering workspace agents and devcontainer subagents. Dependencies are sorted by prerequisite address and preserve `success` or `completion` requirements. Invalid graphs attach nothing. - Reject Graph responses exceeding the 4 MiB DRPC limit with a readable error reporting the size and limit instead of a transport failure. This check applies to all templates. - Preserve START-only conversion: STOP imports carry addresses but no dependencies. Templates without ordering rules are otherwise unchanged. - Add tests for dependency attachment, requirement preservation, sorting, devcontainer scripts, and oversized responses. Server-side consumption remains a separate follow-up. Refs [PLAT-547](https://linear.app/codercom/issue/PLAT-547/phase-2-transport-resolved-script-dependencies-from-provisioner).
This commit is contained in:
@@ -16,10 +16,12 @@ import (
|
||||
tfjson "github.com/hashicorp/terraform-json"
|
||||
"github.com/spf13/afero"
|
||||
"golang.org/x/xerrors"
|
||||
protobuf "google.golang.org/protobuf/proto"
|
||||
|
||||
"cdr.dev/slog/v3"
|
||||
"github.com/coder/coder/v2/coderd/database"
|
||||
"github.com/coder/coder/v2/coderd/tracing"
|
||||
"github.com/coder/coder/v2/codersdk/drpcsdk"
|
||||
"github.com/coder/coder/v2/provisioner/terraform/scriptorder"
|
||||
"github.com/coder/coder/v2/provisionersdk"
|
||||
"github.com/coder/coder/v2/provisionersdk/proto"
|
||||
@@ -353,7 +355,7 @@ func (s *server) Graph(
|
||||
}
|
||||
}
|
||||
|
||||
return &proto.GraphComplete{
|
||||
response := &proto.GraphComplete{
|
||||
Error: "",
|
||||
Timings: e.timings.aggregate(),
|
||||
Resources: state.Resources,
|
||||
@@ -362,6 +364,35 @@ func (s *server) Graph(
|
||||
Presets: state.Presets,
|
||||
HasExternalAgents: state.HasExternalAgents,
|
||||
}
|
||||
if err := checkGraphCompleteSize(response); err != nil {
|
||||
if convertScriptOrder {
|
||||
return provisionersdk.GraphError(
|
||||
"%s; reduce the number of coder_script_order dependencies", err,
|
||||
)
|
||||
}
|
||||
return provisionersdk.GraphError("%s", err)
|
||||
}
|
||||
return response
|
||||
}
|
||||
|
||||
// checkGraphCompleteSize rejects a Graph response that cannot cross the
|
||||
// DRPC transport in one message, so the import fails with a readable error
|
||||
// instead of a dropped connection. The session wraps the result in a
|
||||
// proto.Response before sending, so the check measures that envelope rather
|
||||
// than the bare GraphComplete. The caller adds the script order hint when
|
||||
// that conversion ran, since it is the only unbounded addition to the
|
||||
// response and the hint would mislead templates without it.
|
||||
func checkGraphCompleteSize(response *proto.GraphComplete) error {
|
||||
size := protobuf.Size(&proto.Response{
|
||||
Type: &proto.Response_Graph{Graph: response},
|
||||
})
|
||||
if size <= drpcsdk.MaxMessageSize {
|
||||
return nil
|
||||
}
|
||||
return xerrors.Errorf(
|
||||
"graph result is %d bytes, above the %d byte message limit",
|
||||
size, drpcsdk.MaxMessageSize,
|
||||
)
|
||||
}
|
||||
|
||||
func logScriptOrderWarnings(
|
||||
|
||||
@@ -0,0 +1,52 @@
|
||||
package terraform
|
||||
|
||||
import (
|
||||
"strconv"
|
||||
"strings"
|
||||
"testing"
|
||||
|
||||
"github.com/stretchr/testify/require"
|
||||
protobuf "google.golang.org/protobuf/proto"
|
||||
|
||||
"github.com/coder/coder/v2/codersdk/drpcsdk"
|
||||
"github.com/coder/coder/v2/provisionersdk/proto"
|
||||
)
|
||||
|
||||
func TestCheckGraphCompleteSize(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
script := &proto.Script{ResourceAddress: "coder_script.build"}
|
||||
response := &proto.GraphComplete{
|
||||
Resources: []*proto.Resource{{
|
||||
Agents: []*proto.Agent{{Scripts: []*proto.Script{script}}},
|
||||
}},
|
||||
}
|
||||
require.NoError(t, checkGraphCompleteSize(response))
|
||||
|
||||
// Grow the response until the bare GraphComplete still fits the limit
|
||||
// but the proto.Response envelope the session sends does not. The
|
||||
// guard must measure the envelope, or this case reaches the transport
|
||||
// and drops the connection.
|
||||
envelope := &proto.Response{Type: &proto.Response_Graph{Graph: response}}
|
||||
dependency := &proto.ScriptDependency{
|
||||
PrerequisiteResourceAddress: "module.setup." + strings.Repeat("x", 90) + ".coder_script.step",
|
||||
Requirement: proto.ScriptDependencyRequirement_SCRIPT_DEPENDENCY_REQUIREMENT_SUCCESS,
|
||||
}
|
||||
for protobuf.Size(envelope) <= drpcsdk.MaxMessageSize {
|
||||
script.Dependencies = append(script.Dependencies, dependency)
|
||||
}
|
||||
// Drop the entry that crossed the limit, then pad the last remaining
|
||||
// one a byte at a time so the envelope crosses by the smallest margin.
|
||||
last := protobuf.Clone(dependency).(*proto.ScriptDependency)
|
||||
script.Dependencies[len(script.Dependencies)-2] = last
|
||||
script.Dependencies = script.Dependencies[:len(script.Dependencies)-1]
|
||||
for protobuf.Size(envelope) <= drpcsdk.MaxMessageSize {
|
||||
last.PrerequisiteResourceAddress += "x"
|
||||
}
|
||||
require.LessOrEqual(t, protobuf.Size(response), drpcsdk.MaxMessageSize)
|
||||
require.Greater(t, protobuf.Size(envelope), drpcsdk.MaxMessageSize)
|
||||
|
||||
err := checkGraphCompleteSize(response)
|
||||
require.ErrorContains(t, err, strconv.Itoa(protobuf.Size(envelope))+" bytes")
|
||||
require.ErrorContains(t, err, strconv.Itoa(drpcsdk.MaxMessageSize)+" byte message limit")
|
||||
}
|
||||
@@ -167,6 +167,10 @@ type State struct {
|
||||
ExternalAuthProviders []*proto.ExternalAuthProviderResource
|
||||
HasExternalAgents bool
|
||||
agentCatalog scriptOrderAgentCatalog
|
||||
// scriptsByAddress holds every script that was bound to a workspace
|
||||
// agent or devcontainer, by resource address, so script order
|
||||
// dependencies can be attached after the order is finalized.
|
||||
scriptsByAddress map[string]*proto.Script
|
||||
}
|
||||
|
||||
var ErrInvalidTerraformAddr = xerrors.New("invalid terraform address")
|
||||
@@ -201,6 +205,7 @@ func ConvertState(ctx context.Context, modules []*tfjson.StateModule, rawGraph s
|
||||
resources := make([]*proto.Resource, 0)
|
||||
resourceAgents := map[string][]*proto.Agent{}
|
||||
agentCatalog := scriptOrderAgentCatalog{}
|
||||
scriptsByAddress := map[string]*proto.Script{}
|
||||
|
||||
// Indexes Terraform resources by their label.
|
||||
// The label is what "terraform graph" uses to reference nodes.
|
||||
@@ -672,12 +677,14 @@ func ConvertState(ctx context.Context, modules []*tfjson.StateModule, rawGraph s
|
||||
// Find agents with the matching ID and associate them!
|
||||
if dependsOnAgent(graph, agent, attrs.AgentID, resource) {
|
||||
agent.Scripts = append(agent.Scripts, script)
|
||||
scriptsByAddress[resource.Address] = script
|
||||
break scriptAgentLoop
|
||||
}
|
||||
|
||||
for _, dc := range agent.GetDevcontainers() {
|
||||
if dependsOnDevcontainer(graph, dc, attrs.AgentID, resource) {
|
||||
dc.Scripts = append(dc.Scripts, script)
|
||||
scriptsByAddress[resource.Address] = script
|
||||
break scriptAgentLoop
|
||||
}
|
||||
}
|
||||
@@ -1054,6 +1061,7 @@ func ConvertState(ctx context.Context, modules []*tfjson.StateModule, rawGraph s
|
||||
ExternalAuthProviders: externalAuthProviders,
|
||||
HasExternalAgents: hasExternalAgentResources(graph),
|
||||
agentCatalog: agentCatalog,
|
||||
scriptsByAddress: scriptsByAddress,
|
||||
}, nil
|
||||
}
|
||||
|
||||
|
||||
@@ -80,6 +80,9 @@ func convertStateWithScriptOrder(
|
||||
if err != nil {
|
||||
return nil, xerrors.Errorf("finalize script order: %w", err)
|
||||
}
|
||||
if err := attachScriptOrderDependencies(order, state.scriptsByAddress); err != nil {
|
||||
return nil, xerrors.Errorf("attach script order dependencies: %w", err)
|
||||
}
|
||||
if len(order.Graphs) > 0 {
|
||||
result.order = &order
|
||||
}
|
||||
|
||||
@@ -65,6 +65,79 @@ func TestConvertStateWithScriptOrderFinalizesAppliedDevcontainerOrder(t *testing
|
||||
require.Equal(t, "coder_devcontainer.repo", conversion.order.Graphs[0].RuntimeAddress)
|
||||
}
|
||||
|
||||
func TestConvertStateWithScriptOrderAttachesDependencies(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
module := scriptOrderTestModule(
|
||||
scriptOrderTestAgent("coder_agent.main", "main", "agent-id"),
|
||||
scriptOrderTestDevcontainer(
|
||||
"coder_devcontainer.repo", "repo", "agent-id", "subagent-id",
|
||||
),
|
||||
scriptOrderTestScript("coder_script.clone_repo", "clone_repo", "agent-id"),
|
||||
scriptOrderTestScript("coder_script.install_tools", "install_tools", "agent-id"),
|
||||
scriptOrderTestScript("coder_script.build", "build", "agent-id"),
|
||||
scriptOrderTestScript("coder_script.dc_setup", "dc_setup", "subagent-id"),
|
||||
scriptOrderTestScript("coder_script.dc_run", "dc_run", "subagent-id"),
|
||||
)
|
||||
module.Resources[len(module.Resources)-1].AttributeValues = map[string]any{
|
||||
"rule": []any{
|
||||
map[string]any{
|
||||
"run": []string{"coder_script.install_tools"},
|
||||
"after": []string{"coder_script.clone_repo"},
|
||||
},
|
||||
map[string]any{
|
||||
// Prerequisites are listed out of order on purpose.
|
||||
"run": []string{"coder_script.build"},
|
||||
"after": []string{"coder_script.install_tools", "coder_script.clone_repo"},
|
||||
"requires": "completion",
|
||||
},
|
||||
map[string]any{
|
||||
"run": []string{"coder_script.dc_run"},
|
||||
"after": []string{"coder_script.dc_setup"},
|
||||
},
|
||||
},
|
||||
}
|
||||
conversion, err := convertStateWithScriptOrder(
|
||||
t.Context(),
|
||||
[]*tfjson.StateModule{module},
|
||||
scriptOrderTestConfig(module),
|
||||
scriptOrderTestGraph("coder_agent.main"),
|
||||
slogtest.Make(t, nil),
|
||||
proto.GraphSource_SOURCE_STATE,
|
||||
)
|
||||
require.NoError(t, err)
|
||||
require.NotNil(t, conversion.order)
|
||||
|
||||
var agents []*proto.Agent
|
||||
for _, resource := range conversion.state.Resources {
|
||||
agents = append(agents, resource.Agents...)
|
||||
}
|
||||
require.Len(t, agents, 1)
|
||||
agent := agents[0]
|
||||
require.Len(t, agent.Devcontainers, 1)
|
||||
agentScripts := scriptOrderTestScriptsByAddress(t, agent.Scripts,
|
||||
"coder_script.build", "coder_script.clone_repo", "coder_script.install_tools",
|
||||
)
|
||||
devcontainerScripts := scriptOrderTestScriptsByAddress(t, agent.Devcontainers[0].Scripts,
|
||||
"coder_script.dc_run", "coder_script.dc_setup",
|
||||
)
|
||||
|
||||
success := proto.ScriptDependencyRequirement_SCRIPT_DEPENDENCY_REQUIREMENT_SUCCESS
|
||||
completion := proto.ScriptDependencyRequirement_SCRIPT_DEPENDENCY_REQUIREMENT_COMPLETION
|
||||
require.Empty(t, agentScripts["coder_script.clone_repo"].Dependencies)
|
||||
require.Equal(t, []*proto.ScriptDependency{
|
||||
{PrerequisiteResourceAddress: "coder_script.clone_repo", Requirement: success},
|
||||
}, agentScripts["coder_script.install_tools"].Dependencies)
|
||||
require.Equal(t, []*proto.ScriptDependency{
|
||||
{PrerequisiteResourceAddress: "coder_script.clone_repo", Requirement: completion},
|
||||
{PrerequisiteResourceAddress: "coder_script.install_tools", Requirement: completion},
|
||||
}, agentScripts["coder_script.build"].Dependencies)
|
||||
require.Empty(t, devcontainerScripts["coder_script.dc_setup"].Dependencies)
|
||||
require.Equal(t, []*proto.ScriptDependency{
|
||||
{PrerequisiteResourceAddress: "coder_script.dc_setup", Requirement: success},
|
||||
}, devcontainerScripts["coder_script.dc_run"].Dependencies)
|
||||
}
|
||||
|
||||
func TestConvertStateWithScriptOrderValidatesPlanWithoutAgentBindings(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
@@ -319,6 +392,25 @@ func scriptOrderTestConfig(module *tfjson.StateModule) *tfjson.Config {
|
||||
return &tfjson.Config{RootModule: &tfjson.ConfigModule{Resources: resources}}
|
||||
}
|
||||
|
||||
// scriptOrderTestScriptsByAddress checks that scripts carry exactly the given
|
||||
// resource addresses and returns them keyed by address.
|
||||
func scriptOrderTestScriptsByAddress(
|
||||
t *testing.T,
|
||||
scripts []*proto.Script,
|
||||
addresses ...string,
|
||||
) map[string]*proto.Script {
|
||||
t.Helper()
|
||||
|
||||
byAddress := make(map[string]*proto.Script, len(scripts))
|
||||
got := make([]string, 0, len(scripts))
|
||||
for _, script := range scripts {
|
||||
byAddress[script.ResourceAddress] = script
|
||||
got = append(got, script.ResourceAddress)
|
||||
}
|
||||
require.ElementsMatch(t, addresses, got)
|
||||
return byAddress
|
||||
}
|
||||
|
||||
func scriptOrderTestGraph(agentAddresses ...string) string {
|
||||
graph := "digraph {\n"
|
||||
graph += `"[root] docker_container.workspace" [label = "docker_container.workspace"]` + "\n"
|
||||
|
||||
Reference in New Issue
Block a user