NotificationsYou must be signed in to change notification settings
Fork1k
Star11k

feat: Add workspace application support#1773

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

Jump to bottom

Merged

kylecarbs merged 42 commits intomainfromapps

Jun 4, 2022

Merged

feat: Add workspace application support#1773

kylecarbs merged 42 commits intomainfromapps

Jun 4, 2022

Conversation

Copy link

Member

kylecarbs commentedMay 26, 2022•
edited
Loading

This adds applications as a property to a workspace agent.

The resource is added to the Terraform provider here:
coder/terraform-provider-coder#17

Apps will be opened in the dashboard or via the CLI
withcoder open <name>. Ifcommand is specified, a
terminal will appear locally and on the web. Iftarget
is specified, the browser will open to an exposed instance
of that target.

Fix odd context cancel bug on HTTP transport.

kylecarbs added9 commits

May 6, 2022 18:31

feat: Add app support

bdfa61a

This adds apps as a property to a workspace agent.The resource is added to the Terraform provider here:coder/terraform-provider-coder#17Apps will be opened in the dashboard or via the CLIwith `coder open <name>`. If `command` is specified, aterminal will appear locally and in the web. If `target`is specified, the browser will open to an exposed instanceof that target.

Merge branch 'main' into devurls

8b2f6c4

Merge branch 'main' into devurls

e3cf488

Compare fields in apps test

e3ff8ad

Update Terraform provider to use relative path

b6e1ea6

Add some basic structure for routing

430cfe7

chore: Remove interface from coderd and lift API surface

6ef781c

Abstracting coderd into an interface added misdirection becausethe interface was never intended to be fulfilled outside of a singleimplementation.This lifts the abstraction, and attaches all handlers to a root structnamed `*coderd.API`.

Merge branch 'routeclean' into devurls

f70dd17

Merge branch 'main' into devurls

0805250

kylecarbs self-assigned this

May 26, 2022

Add basic proxy logic

934b1ff

f0ssel reviewed

May 26, 2022

View reviewed changes

coderd/database/migrations/000014_workspace_apps.up.sql Outdated

		CREATE TABLE workspace_apps (
		id uuid NOT NULL,
		created_at timestamp with time zone NOT NULL,
		agent_id uuid NOT NULL REFERENCES workspace_agents (id) ON DELETE CASCADE,

Copy link

Contributor

f0sselMay 26, 2022

Choose a reason for hiding this comment

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

I am unfamiliar with the schemas around agents, builds, templates, etc. I would expect this to be in lifecycle with the template version I think. Why is it related to the agent?

Copy link

MemberAuthor

kylecarbsMay 27, 2022

Choose a reason for hiding this comment

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

Workspace agents are attached to a template version too.

A workspace build can contain different agents than a template version based on variables. This introduces complexity for us, but is something we unfortunately need to account for.

kylecarbs added3 commits

May 27, 2022 13:29

Add proxying based on path

866eeed

Merge branch 'main' into apps

4b73034

Add app proxying for wildcards

b4f9615

kylecarbs linked an issue

May 31, 2022

that may beclosed by this pull request

Workspace apps support#257

Closed

3 tasks

kylecarbs added7 commits

May 31, 2022 14:33

Add wsconncache

c88df46

fix: Race when writing to a closed pipe

d327df7

This is such an intermittent race it's difficult to track,but regardless this is an improvement to the code.

Merge branch 'readclose' into apps

f84f5ea

fix: Race when writing to a closed pipe

cec2de3

This is such an intermittent race it's difficult to track,but regardless this is an improvement to the code.

Merge branch 'readclose' into apps

c57f8dd

fix: Race when writing to a closed pipe

8e61cac

This is such an intermittent race it's difficult to track,but regardless this is an improvement to the code.

Merge branch 'readclose' into apps

b6e6d7b

kylecarbs force-pushed theapps branch from602e725 tob6e6d7bCompare

May 31, 2022 15:53

kylecarbs added2 commits

June 1, 2022 02:50

fix: Race when writing to a closed pipe

46b24f7

This is such an intermittent race it's difficult to track,but regardless this is an improvement to the code.

Merge branch 'readclose' into apps

4d8b257

kylecarbs force-pushed theapps branch fromc0f7b32 to4d8b257Compare

June 1, 2022 03:07

spikecurtis reviewed

Jun 1, 2022

View reviewed changes

provisionersdk/proto/provisioner.proto

		oneof auth {
		string token = 8;
		string instance_id = 9;
		string token = 9;

Copy link

Contributor

spikecurtisJun 1, 2022

Choose a reason for hiding this comment

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

generally a no-no to change field indexes. Any reason it's particularly important here?

Copy link

MemberAuthor

kylecarbsJun 3, 2022

Choose a reason for hiding this comment

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

It's nicer structurally from my perspective.

provisionersdk/proto/provisioner.proto

		message App {
		string name = 1;
		string command = 2;
		string url = 3;

Copy link

Contributor

spikecurtisJun 1, 2022

Choose a reason for hiding this comment

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

in the terraform provider docs you say command or url but not both can be set. Should these be in aoneof?

Copy link

MemberAuthor

kylecarbsJun 2, 2022

Choose a reason for hiding this comment

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

Good catch.

coderd/coderd.go OutdatedShow resolvedHide resolved

coderd/wsconncache/wsconncache.go OutdatedShow resolvedHide resolved

coderd/wsconncache/wsconncache.goShow resolvedHide resolved

coderd/wsconncache/wsconncache.go OutdatedShow resolvedHide resolved

Copy link

Contributor

spikecurtis commentedJun 1, 2022

At a high level, I also find it a bit strange that we're having coderd negotiate a WebRTC connection to the agent, when the agent isalready connected to coderd via the channel that the ICE negotiation is happening over.

Copy link

Contributor

spikecurtis commentedJun 1, 2022

Also missing from this (Ok if being tracked in other issues)

Authorization checks
Support forcommand apps (I'm only seeing URLs supported)

kylecarbs force-pushed theapps branch from30289cb to0a63becCompare

June 3, 2022 04:28

kylecarbs added3 commits

June 3, 2022 04:42

Remove unused access url

d3b9ab5

Add RBAC tests

7a1ae15

Merge branch 'main' into apps

5b9194f

kylecarbs marked this pull request as ready for review

June 3, 2022 05:11

kylecarbs requested a review froma team as acode owner

June 3, 2022 05:11

Merge branch 'main' into apps

cd2d12e

greyscaled reviewed

Jun 3, 2022

View reviewed changes

coderd/coderd.go

		// %40 is the encoded character of the @ symbol. VS Code Web does
		// not handle character encoding properly, so it's safe to assume
		// other applications might not as well.
		r.Route("/%40{user}/{workspacename}/apps/{workspaceapp}", apps)

Copy link

Contributor

greyscaledJun 3, 2022•
edited
Loading

Choose a reason for hiding this comment

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

Does this mean the FE should useencodeURIComponent ?

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/encodeURIComponent

so like

async (username: string, workspaceName: string, app: string) => {  encoderURIComponent(`/api/v0/@{username}/{workspaceName}/apps/{app}`)}

Copy link

MemberAuthor

kylecarbsJun 3, 2022

Choose a reason for hiding this comment

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

Nah, we should always use@ if possible. This is an edge-case for where certain client-side applications don't handle the@ properly.

Copy link

Contributor

greyscaledJun 3, 2022

Choose a reason for hiding this comment

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

TY!!

greyscaled reviewed

Jun 3, 2022

View reviewed changes

coderd/coderd.goShow resolvedHide resolved

presleyp reviewed

Jun 3, 2022

View reviewed changes

site/site.go

		// metadata in error pages.
		func WithAPIResponse(ctx context.Context, apiResponse APIResponse) context.Context {
		return context.WithValue(ctx, apiResponseContextKey{}, apiResponse)
		}

Copy link

Contributor

presleypJun 3, 2022

Choose a reason for hiding this comment

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

Is this dev url specific or is it related to the error message discussion from yesterday?

Copy link

MemberAuthor

kylecarbsJun 3, 2022

Choose a reason for hiding this comment

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

Specific to this. The content can be changed trivially too, I intentionally kept it pretty loose while we hash that out.

presleyp reviewed

Jun 3, 2022

View reviewed changes

site/src/api/typesGenerated.ts

		export interface WorkspaceApp {
		readonly id: string
		readonly name: string
		readonly command?: string

Copy link

Contributor

presleypJun 3, 2022

Choose a reason for hiding this comment

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

what iscommand?

Copy link

MemberAuthor

kylecarbsJun 3, 2022

Choose a reason for hiding this comment

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

It will open a command inside of a terminal. I'll be opening a subsequent PR to add this in. An issue to track it is here:#2023

greyscaled reviewed

Jun 3, 2022

View reviewed changes

Copy link

Contributor

greyscaled left a comment

Choose a reason for hiding this comment

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

FE = ✔️

kylecarbs added3 commits

June 3, 2022 15:58

Fix dial agent syntax

b056400

Merge branch 'main' into apps

fe3aecc

Fix linting errors

2018cdc

f0ssel approved these changes

Jun 3, 2022

View reviewed changes

Copy link

Contributor

f0ssel left a comment

Choose a reason for hiding this comment

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

Generally everything looks good to me. A little long so I didn't dive in super hard but the tests seem to cover enough bases to give me confidence.

coderd/rbac/README.md

		@@ -51,7 +51,7 @@ This can be represented by the following truth table, where Y represents *positi

		## Example Permissions

		- `+site...read`: allowed to perform the `read` action against all objects of type `devurl` in a given Coder deployment.

Copy link

Contributor

f0sselJun 3, 2022

Choose a reason for hiding this comment

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

kill it with 🔥

site/src/api/typesGenerated.ts Outdated

		readonly name: string
		readonly command?: string
		readonly access_url?: string
		readonly icon: string

Copy link

Contributor

f0sselJun 3, 2022

Choose a reason for hiding this comment

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

Is icon required? I think we should have some fallback on the frontend and make it optional in terraform if it isn't already.

Copy link

MemberAuthor

kylecarbsJun 3, 2022

Choose a reason for hiding this comment

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

True. I shall fix

kylecarbs added5 commits

June 3, 2022 16:55

Fix gen

2d5261f

Fix icon required

856f17d

Merge branch 'main' into apps

1a21f94

Adjust migration number

ad90bcb

Fix proxy error status code

38abbb5

kylecarbs force-pushed theapps branch fromca2d449 to38abbb5Compare

June 4, 2022 17:55

kylecarbs added2 commits

June 4, 2022 18:06

Fix empty db lookup

4f89642

Merge branch 'main' into apps

637be3e

kylecarbs force-pushed theapps branch from0d2989e to637be3eCompare

June 4, 2022 18:08

Merge branch 'main' into apps

50da4fb

kylecarbs merged commit013f028 intomain

Jun 4, 2022

kylecarbs deleted the apps branch

June 4, 2022 20:13

kylecarbs added a commit that referenced this pull request

Jun 10, 2022

feat: Add workspace application support (#1773)

69c9133

* feat: Add app supportThis adds apps as a property to a workspace agent.The resource is added to the Terraform provider here:coder/terraform-provider-coder#17Apps will be opened in the dashboard or via the CLIwith `coder open <name>`. If `command` is specified, aterminal will appear locally and in the web. If `target`is specified, the browser will open to an exposed instanceof that target.* Compare fields in apps test* Update Terraform provider to use relative path* Add some basic structure for routing* chore: Remove interface from coderd and lift API surfaceAbstracting coderd into an interface added misdirection becausethe interface was never intended to be fulfilled outside of a singleimplementation.This lifts the abstraction, and attaches all handlers to a root structnamed `*coderd.API`.* Add basic proxy logic* Add proxying based on path* Add app proxying for wildcards* Add wsconncache* fix: Race when writing to a closed pipeThis is such an intermittent race it's difficult to track,but regardless this is an improvement to the code.* fix: Race when writing to a closed pipeThis is such an intermittent race it's difficult to track,but regardless this is an improvement to the code.* fix: Race when writing to a closed pipeThis is such an intermittent race it's difficult to track,but regardless this is an improvement to the code.* fix: Race when writing to a closed pipeThis is such an intermittent race it's difficult to track,but regardless this is an improvement to the code.* Add workspace route proxying endpoint- Makes the workspace conn cache concurrency-safe- Reduces unnecessary open checks in `peer.Channel`- Fixes the use of a temporary context when dialing a workspace agent* Add embed errors* chore: Refactor site to improve testingIt was difficult to develop this package due to theembed build tag being mandatory on the tests. The logicto test doesn't require any embedded files.* Add test for error handler* Remove unused access url* Add RBAC tests* Fix dial agent syntax* Fix linting errors* Fix gen* Fix icon required* Adjust migration number* Fix proxy error status code* Fix empty db lookup

kylecarbs mentioned this pull request

Jun 16, 2022

feat: port forwarding dropdown#1824

Closed

4 tasks

Labels

None yet

5 participants

Movatterモバイル変換

feat: Add workspace application support#1773

feat: Add workspace application support#1773

Uh oh!

Conversation

kylecarbs commentedMay 26, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

spikecurtis commentedJun 1, 2022

Uh oh!

spikecurtis commentedJun 1, 2022

Uh oh!

greyscaledJun 3, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greyscaled left a comment

Choose a reason for hiding this comment

Uh oh!

f0ssel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kylecarbs commentedMay 26, 2022•
edited
Loading

greyscaledJun 3, 2022•
edited
Loading