Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

feat: coder-attach: add support for external workspaces#19178

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.

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

Already on GitHub?Sign in to your account

Closed
kacpersaw wants to merge34 commits intomainfromkacpersaw/feat-coder-attach

Conversation

kacpersaw
Copy link
Contributor

Depends on:coder/terraform-provider-coder#424

This pull request introduces support for external workspace management, allowing users to register and manage workspaces that are provisioned and managed outside of the Coder.

CLI changes

  • coder external-workspaces create - Creates a new external workspace (this command extendscoder create)
    • Example:coder external-workspaces create ext-workspace --template=externally-managed-workspace -y
    • Checks if template has coder_external_agent resource before creating a workspace
  • coder external-workspaces list - Lists all external workspaces
  • coder external-workspaces agent-instructions <workspace name> <agent name> - Retrieves agent connection instruction
    • Example:coder external-workspaces agent-instructions ext-workspace main --output=json

coderd changes

  • GET /api/v2/init-script - Gets the agent initialization script
    • By default, it returns a script for Linux (amd64), but with query parameters (os andarch) you can get the init script for different platforms
  • GET /api/v2/workspaces/{workspace}/external-agent/{agent}/credentials - Gets credentials for an external agent

Database changes

  • Addedhas_external_agent field to workspace builds and template versions
  • Updated queries to filter workspaces/templates by thehas_external_agent field

provisioner changes

  • Added support forcoder_external_agent terraform resource
  • Updated provisioner to detect and track external agents in workspace builds

Frontend changes

  • Added a new component AgentExternal which shows instructions for connecting external agents.
  • Added redacted fields toCodeExample so you can now hide specific parts of the code instead of the full line
  • Hides workspace actions if workspace is using external agent.
image

@kacpersawkacpersaw changed the titlefeat(coder-attach): add support for external workspacesfeat: coder-attach: add support for external workspacesAug 5, 2025
@kacpersawkacpersaw marked this pull request as ready for reviewAugust 5, 2025 11:44
Copy link
Member

@johnstcnjohnstcn left a comment•
edited
Loading

Choose a reason for hiding this comment

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

I would highly recommend breaking this PR down by domain. Right now it's extremely large.

Suggested breakpoints:

  • Database
  • API/protobuf
  • CLI
  • UI

kacpersaw and aslilac reacted with thumbs up emoji
Copy link
Member

Choose a reason for hiding this comment

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

When adding a new field to a protobuf, we increment the minor version inprovisionerd/proto/version.go.

kacpersaw reacted with thumbs up emoji
Comment on lines 138 to 142
actions:{
display:"flex",
alignItems:"center",
gap:4,
},
Copy link
Member

Choose a reason for hiding this comment

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

pls use tailwind for new styles

// the secure option, not remember to opt in
secret=true,

// Redact parts of the code if the user doesn't want to obfuscate the whole code
Copy link
Member

Choose a reason for hiding this comment

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

I know there was already a bad pattern here, but these comments will be much more helpful on the type definition in JSDoc format so that the language server will actually recognize them

redactPattern,
redactReplacement="********",

// Show a button to show the redacted parts of the code
Copy link
Member

Choose a reason for hiding this comment

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

same here

redactReplacement="********",

// Show a button to show the redacted parts of the code
redactShowButton=false,
Copy link
Member

Choose a reason for hiding this comment

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

this is a really confusing name. it kind of sounds like by setting it to true you're "redacting the show button" which doesn't really make sense. justshowButton would also be confusing. what button are we showing and why?

I think these are usually called "reveal" buttons. maybeshowRevealButton orenableRevealButton?

Suggested change
redactShowButton=false,
redactShowButton,

also you don't need the default.undefined is treated likefalse when used as a boolean.

Comment on lines 57 to 64
conststyles={
externalAgentSection:(theme)=>({
fontSize:16,
color:theme.palette.text.secondary,
paddingBottom:8,
lineHeight:1.4,
}),
}satisfiesRecord<string,Interpolation<Theme>>;
Copy link
Member

Choose a reason for hiding this comment

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

pls use tailwind for new styles

…attach# Conflicts:#coderd/database/queries/templateversions.sql
Copy link
Member

@deansheatherdeansheather left a comment

Choose a reason for hiding this comment

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

BE review 👍

Comment on lines 135 to 147
cliui.ChangeFormatterData(cliui.TextFormat(),func(dataany) (any,error) {
agent,ok:=data.(externalAgent)
if!ok {
return"",xerrors.Errorf("expected externalAgent, got %T",data)
}

varoutput strings.Builder
_,_=output.WriteString(fmt.Sprintf("Please run the following commands to attach agent %s:\n",cliui.Keyword(agent.AgentName)))
_,_=output.WriteString(fmt.Sprintf("%s\n",pretty.Sprint(cliui.DefaultStyles.Code,fmt.Sprintf("export CODER_AGENT_TOKEN=%s",agent.AuthToken))))
_,_=output.WriteString(pretty.Sprint(cliui.DefaultStyles.Code,fmt.Sprintf("curl -fsSL %s | sh",agent.InitScript)))

returnoutput.String(),nil
}),
Copy link
Member

Choose a reason for hiding this comment

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

This chunk of code seems to be used multiple times, can you not reuseprintExternalAgents()?

Comment on lines 74 to 77
template,err:=client.TemplateByName(inv.Context(),organization.ID,templateName.Value.String())
iferr!=nil {
returnxerrors.Errorf("get template by name: %w",err)
}
Copy link
Member

Choose a reason for hiding this comment

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

A lot of these queries are going to be repeated by the original handler, could we change thecreateHandler function into a model where the queries get run once, then a "validate" function that gets swapped forcoder create orcoder external-workspaces create gets executed (and all the queried objects get passed in), then the workspace gets created.

workspace,workspaceBuild,err:=createHandler(ctx,inv,createOptions{beforeCreate:func(ctx context.Context,template codersdk.Template,...)error {},})

cli/root.go Outdated
r.update(),
r.whoami(),

// External Workspace Commands
Copy link
Member

Choose a reason for hiding this comment

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

I'd just group this with workspaces tbh


cmd:=&serpent.Command{
Use:"external-workspaces [subcommand]",
Short:"External workspace related commands",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Short:"External workspace related commands",
Short:"Create or manage external workspaces",

)

cmd:=&serpent.Command{
Use:"agent-instructions [workspace name] [agent name]",
Copy link
Member

Choose a reason for hiding this comment

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

Other commands use[user/]workspace[.agent] syntax for selecting an agent, defaulting to the only agent in the workspace if the agent is unspecified and there's only one. I believe there's a helper function incli to resolve these for you.

return
}
script=strings.ReplaceAll(script,"${ACCESS_URL}",api.AccessURL.String()+"/")
script=strings.ReplaceAll(script,"${AUTH_TYPE}","token")
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't auth type be configurable e.g. with an query parameter var? Defaulting the value to "token" is fine, though.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Phase 1 assumes token-based auth, so I think it can stay as it is. In the future, we can extend this to include other types of auth.

deansheather reacted with thumbs up emoji
Copy link
Member

Choose a reason for hiding this comment

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

How would that look like on the init script endpoint? A query parameter defaulting totoken perhaps?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Yep, that's the plan 👍

deansheather reacted with thumbs up emoji
returnxerrors.Errorf("update template version external auth providers: %w",err)
}

err=db.UpdateTemplateVersionExternalAgentByJobID(ctx, database.UpdateTemplateVersionExternalAgentByJobIDParams{
Copy link
Member

Choose a reason for hiding this comment

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

Could probably be merged with the above query.

break
}
}
err=db.UpdateWorkspaceBuildExternalAgentByID(ctx, database.UpdateWorkspaceBuildExternalAgentByIDParams{
Copy link
Member

Choose a reason for hiding this comment

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

Same here


for_,agent:=rangeagents {
ifagent.Name==agentName {
httpapi.Write(ctx,rw,http.StatusOK, codersdk.ExternalAgentCredentials{
Copy link
Member

Choose a reason for hiding this comment

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

If the external agent uses a different authentication strategy than "token" we should probably return a 404 or something else that the frontend can detect.

{workspace.name} workspace:
</p>
<CodeExample
code={`CODER_AGENT_TOKEN="${externalAgentToken}" curl -fsSL "${initScriptURL}" | sh`}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe the generation of the command should be moved to the backend and returned from the API, so we can keep it all in one place. This will need to eventually vary by OS/arch (curl | sh doesn't work on windows).

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

We can extend the credentials endpoint and add another field for the ready-to-go command. Unless you have a different idea?

Copy link
Member

Choose a reason for hiding this comment

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

That sounds good to me!

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Done 👍

Copy link
Member

@deansheatherdeansheather left a comment

Choose a reason for hiding this comment

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

Backend looks good, would like to see a few quick changes then I'll stamp

template_versions.idIN (archived_versions.id)
RETURNINGtemplate_versions.id;

-- name: UpdateTemplateVersionAITaskAndExternalAgentByJobID :exec
Copy link
Member

Choose a reason for hiding this comment

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

MaybeUpdateTemplateVersionFlagsByJobID? The name might get even crazier if we add more of these post-build flag columns.

ORDER BY
tv.nameASC,wb.build_numberDESC;

-- name: UpdateWorkspaceBuildAITaskAndExternalAgentByID :exec
Copy link
Member

Choose a reason for hiding this comment

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

Same here


script,exists:=provisionersdk.AgentScriptEnv()[fmt.Sprintf("CODER_AGENT_SCRIPT_%s_%s",os,arch)]
if!exists {
rw.WriteHeader(http.StatusBadRequest)
Copy link
Member

Choose a reason for hiding this comment

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

You should probably write a message here too, usinghttpapi.Write(..., codersdk.Response{}) is probably fine.

Copy link
Member

Choose a reason for hiding this comment

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

Unknown os/arch: %s/%s

client:=coderdtest.New(t,nil)
script,err:=client.InitScript(context.Background(),"windows","amd64")
require.NoError(t,err)
require.NotEmpty(t,script)
Copy link
Member

Choose a reason for hiding this comment

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

Should test the replacements as well, probably search for substrings like$env:CODER_AGENT_AUTH_TYPE="token" (or whatever it's supposed to look like) etc.

client,db:=coderdtest.NewWithDatabase(t,nil)
user:=coderdtest.CreateFirstUser(t,client)

t.Run("Success",func(t*testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

Should check the returned command matches what you expect for both non-windows and windows in separate tests.

@@ -0,0 +1,28 @@
package codersdk
Copy link
Member

Choose a reason for hiding this comment

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

nit: This file should probably be renamed justinitscript.go to match other files in the dir likeprovisionerdaemons.go. Files ending with_xyz.go are usually for showing stuff similar build tags in the filename

@@ -1,6 +1,7 @@
package cli
Copy link
Member

Choose a reason for hiding this comment

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

nit: Maybe this should beexternalworkspaces.go too

kacpersawand others added2 commitsAugust 8, 2025 16:20
Co-authored-by: Dean Sheather <dean@deansheather.com>
@kacpersawkacpersaw requested a review fromaslilacAugust 8, 2025 14:44
@aslilac
Copy link
Member

I would highly recommend breaking this PR down by domain. Right now it's extremely large.

quoting@johnstcn, I think this still needs to be addressed. I did a cursory review but it's really hard to actually feel comfortable approving a diff this large. I think this should probably be broken into at least like three chunks before it gets merged.

Copy link
Member

@johnstcnjohnstcn left a comment

Choose a reason for hiding this comment

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

Some more comments below, but I would still much prefer for this to be broken down into smaller changes.

Comment on lines +25 to +43
func (r*RootCmd)externalWorkspaces()*serpent.Command {
orgContext:=NewOrganizationContext()

cmd:=&serpent.Command{
Use:"external-workspaces [subcommand]",
Short:"Create or manage external workspaces",
Handler:func(inv*serpent.Invocation)error {
returninv.Command.HelpHandler(inv)
},
Children: []*serpent.Command{
r.externalWorkspaceCreate(),
r.externalWorkspaceAgentInstructions(),
r.externalWorkspaceList(),
},
}

orgContext.AttachOptions(cmd)
returncmd
}
Copy link
Member

Choose a reason for hiding this comment

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

Is this functionality available in OSS as well?

Copy link
Member

Choose a reason for hiding this comment

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

Heads-up: we're starting to update these tests to use a mock DB instead.
See:#19257

Copy link
Member

Choose a reason for hiding this comment

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

Obligatory reminder: double-check migration number before merging!

os:=strings.ToLower(chi.URLParam(r,"os"))
arch:=strings.ToLower(chi.URLParam(r,"arch"))

script,exists:=provisionersdk.AgentScriptEnv()[fmt.Sprintf("CODER_AGENT_SCRIPT_%s_%s",os,arch)]
Copy link
Member

@johnstcnjohnstcnAug 11, 2025•
edited
Loading

Choose a reason for hiding this comment

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

(Out of scope of this PR) Our bootstrap scripts are currently written with the assumption that they are going to be templated into a Terraform resource or similar. We are now providing users with the option to fetch them over HTTP.

What happens if the request gets cut off prematurely? See theget.docker.com script for examples on how they handle this, specifically the last few lines:

#!/bin/shset -e[... the rest of the file]# wrapped up in a function so that we have some protection against only getting# half the file during "curl | sh"do_installEOF

script=strings.ReplaceAll(script,"${ACCESS_URL}",api.AccessURL.String()+"/")
script=strings.ReplaceAll(script,"${AUTH_TYPE}","token")

rw.Header().Set("Content-Type","text/plain; charset=utf-8")
Copy link
Member

Choose a reason for hiding this comment

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

Comment on lines +32 to +35
typecreateOptionsstruct {
beforeCreatefunc(ctx context.Context,client*codersdk.Client,template codersdk.Template,templateVersionID uuid.UUID)error
afterCreatefunc(ctx context.Context,inv*serpent.Invocation,client*codersdk.Client,workspace codersdk.Workspace)error
}
Copy link
Member

Choose a reason for hiding this comment

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

👍 nice abstraction!

…attach# Conflicts:#coderd/provisionerdserver/provisionerdserver.go
@kacpersaw
Copy link
ContributorAuthor

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@johnstcnjohnstcnjohnstcn left review comments

@deansheatherdeansheatherdeansheather approved these changes

@spikecurtisspikecurtisAwaiting requested review from spikecurtisspikecurtis is a code owner

@aslilacaslilacAwaiting requested review from aslilacaslilac is a code owner

Assignees

@kacpersawkacpersaw

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@kacpersaw@aslilac@johnstcn@deansheather
[8]ページ先頭
©2009-2025 Movatter.jp