From 894a610e41a170473a9b811d011792f353d70990 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 16 Feb 2024 13:14:58 +0000 Subject: [PATCH 1/8] fix(enterprise/coderd): check provisionerd API version on connection --- codersdk/provisionerdaemons.go | 2 ++ enterprise/coderd/provisionerdaemons.go | 10 ++++++++++ 2 files changed, 12 insertions(+) diff --git a/codersdk/provisionerdaemons.go b/codersdk/provisionerdaemons.go index e8f8ed8eb6c8b..ffeb2c4084921 100644 --- a/codersdk/provisionerdaemons.go +++ b/codersdk/provisionerdaemons.go @@ -18,6 +18,7 @@ import ( "github.com/coder/coder/v2/codersdk/drpc" "github.com/coder/coder/v2/provisionerd/proto" "github.com/coder/coder/v2/provisionerd/runner" + "github.com/coder/coder/v2/provisionersdk" ) type LogSource string @@ -201,6 +202,7 @@ func (c *Client) ServeProvisionerDaemon(ctx context.Context, req ServeProvisione query := serverURL.Query() query.Add("id", req.ID.String()) query.Add("name", req.Name) + query.Add("version", provisionersdk.VersionCurrent.String()) for _, provisioner := range req.Provisioners { query.Add("provisioner", string(provisioner)) } diff --git a/enterprise/coderd/provisionerdaemons.go b/enterprise/coderd/provisionerdaemons.go index f81f17befdb8d..a3435429140e8 100644 --- a/enterprise/coderd/provisionerdaemons.go +++ b/enterprise/coderd/provisionerdaemons.go @@ -239,6 +239,16 @@ func (api *API) provisionerDaemonServe(rw http.ResponseWriter, r *http.Request) apiVersion = qv } + if err := provisionersdk.VersionCurrent.Validate(apiVersion); err != nil { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "Incompatible or unparsable version", + Validations: []codersdk.ValidationError{ + {Field: "version", Detail: err.Error()}, + }, + }) + return + } + // Create the daemon in the database. now := dbtime.Now() daemon, err := api.Database.UpsertProvisionerDaemon(authCtx, database.UpsertProvisionerDaemonParams{ From c1cae0673f854559bb844cfacd23b5cf12e0a147 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 16 Feb 2024 13:44:16 +0000 Subject: [PATCH 2/8] rename package coderd/util/apiversion -> /apiversion --- {coderd/util/apiversion => apiversion}/apiversion.go | 0 {coderd/util/apiversion => apiversion}/apiversion_test.go | 2 +- coderd/healthcheck/provisioner.go | 2 +- codersdk/provisionerdaemons.go | 1 + enterprise/coderd/workspaceproxycoordinate.go | 2 +- enterprise/tailnet/workspaceproxy.go | 2 +- provisionersdk/serve.go | 2 +- tailnet/proto/version.go | 2 +- tailnet/service.go | 2 +- 9 files changed, 8 insertions(+), 7 deletions(-) rename {coderd/util/apiversion => apiversion}/apiversion.go (100%) rename {coderd/util/apiversion => apiversion}/apiversion_test.go (96%) diff --git a/coderd/util/apiversion/apiversion.go b/apiversion/apiversion.go similarity index 100% rename from coderd/util/apiversion/apiversion.go rename to apiversion/apiversion.go diff --git a/coderd/util/apiversion/apiversion_test.go b/apiversion/apiversion_test.go similarity index 96% rename from coderd/util/apiversion/apiversion_test.go rename to apiversion/apiversion_test.go index 0bd6fe0f6b52f..8a18a0bd5ca8e 100644 --- a/coderd/util/apiversion/apiversion_test.go +++ b/apiversion/apiversion_test.go @@ -5,7 +5,7 @@ import ( "github.com/stretchr/testify/require" - "github.com/coder/coder/v2/coderd/util/apiversion" + "github.com/coder/coder/v2/apiversion" ) func TestAPIVersionValidate(t *testing.T) { diff --git a/coderd/healthcheck/provisioner.go b/coderd/healthcheck/provisioner.go index 4ff961454b73a..a61836a3d468c 100644 --- a/coderd/healthcheck/provisioner.go +++ b/coderd/healthcheck/provisioner.go @@ -7,6 +7,7 @@ import ( "golang.org/x/mod/semver" + "github.com/coder/coder/v2/apiversion" "github.com/coder/coder/v2/buildinfo" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/db2sdk" @@ -14,7 +15,6 @@ import ( "github.com/coder/coder/v2/coderd/database/dbtime" "github.com/coder/coder/v2/coderd/healthcheck/health" "github.com/coder/coder/v2/coderd/provisionerdserver" - "github.com/coder/coder/v2/coderd/util/apiversion" "github.com/coder/coder/v2/coderd/util/ptr" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/provisionersdk" diff --git a/codersdk/provisionerdaemons.go b/codersdk/provisionerdaemons.go index ffeb2c4084921..d07eccbaf6724 100644 --- a/codersdk/provisionerdaemons.go +++ b/codersdk/provisionerdaemons.go @@ -14,6 +14,7 @@ import ( "golang.org/x/xerrors" "nhooyr.io/websocket" + "github.com/coder/coder/v2/apiversion" "github.com/coder/coder/v2/buildinfo" "github.com/coder/coder/v2/codersdk/drpc" "github.com/coder/coder/v2/provisionerd/proto" diff --git a/enterprise/coderd/workspaceproxycoordinate.go b/enterprise/coderd/workspaceproxycoordinate.go index a85cc0488e7b6..58522e59acead 100644 --- a/enterprise/coderd/workspaceproxycoordinate.go +++ b/enterprise/coderd/workspaceproxycoordinate.go @@ -6,8 +6,8 @@ import ( "github.com/google/uuid" "nhooyr.io/websocket" + "github.com/coder/coder/v2/apiversion" "github.com/coder/coder/v2/coderd/httpapi" - "github.com/coder/coder/v2/coderd/util/apiversion" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/tailnet/proto" ) diff --git a/enterprise/tailnet/workspaceproxy.go b/enterprise/tailnet/workspaceproxy.go index d8f64aa3985a7..b7daabd89155f 100644 --- a/enterprise/tailnet/workspaceproxy.go +++ b/enterprise/tailnet/workspaceproxy.go @@ -14,7 +14,7 @@ import ( "tailscale.com/tailcfg" "cdr.dev/slog" - "github.com/coder/coder/v2/coderd/util/apiversion" + "github.com/coder/coder/v2/apiversion" "github.com/coder/coder/v2/enterprise/wsproxy/wsproxysdk" agpl "github.com/coder/coder/v2/tailnet" ) diff --git a/provisionersdk/serve.go b/provisionersdk/serve.go index 0b2e10234f017..1f19ca6c83063 100644 --- a/provisionersdk/serve.go +++ b/provisionersdk/serve.go @@ -16,8 +16,8 @@ import ( "cdr.dev/slog" + "github.com/coder/coder/v2/apiversion" "github.com/coder/coder/v2/coderd/tracing" - "github.com/coder/coder/v2/coderd/util/apiversion" "github.com/coder/coder/v2/provisionersdk/proto" ) diff --git a/tailnet/proto/version.go b/tailnet/proto/version.go index 449595feb4e9b..a6040a9feae47 100644 --- a/tailnet/proto/version.go +++ b/tailnet/proto/version.go @@ -1,7 +1,7 @@ package proto import ( - "github.com/coder/coder/v2/coderd/util/apiversion" + "github.com/coder/coder/v2/apiversion" ) const ( diff --git a/tailnet/service.go b/tailnet/service.go index 3be0abcab6ded..4af8d6913c071 100644 --- a/tailnet/service.go +++ b/tailnet/service.go @@ -14,7 +14,7 @@ import ( "tailscale.com/tailcfg" "cdr.dev/slog" - "github.com/coder/coder/v2/coderd/util/apiversion" + "github.com/coder/coder/v2/apiversion" "github.com/coder/coder/v2/tailnet/proto" "golang.org/x/xerrors" From da7e0487f8264db79d8c46947f46901275f5e9cb Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 16 Feb 2024 13:45:01 +0000 Subject: [PATCH 3/8] add version parameter to client.ServeProvisionerDaemon --- coderd/coderdtest/coderdtest.go | 2 +- codersdk/provisionerdaemons.go | 5 ++--- enterprise/cli/provisionerdaemons.go | 2 +- enterprise/coderd/provisionerdaemons_test.go | 18 +++++++++--------- 4 files changed, 13 insertions(+), 14 deletions(-) diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index b1c496e4ba295..3c825fa82f9cd 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -571,7 +571,7 @@ func NewExternalProvisionerDaemon(t testing.TB, client *codersdk.Client, org uui }() daemon := provisionerd.New(func(ctx context.Context) (provisionerdproto.DRPCProvisionerDaemonClient, error) { - return client.ServeProvisionerDaemon(ctx, codersdk.ServeProvisionerDaemonRequest{ + return client.ServeProvisionerDaemon(ctx, provisionersdk.VersionCurrent, codersdk.ServeProvisionerDaemonRequest{ ID: uuid.New(), Name: t.Name(), Organization: org, diff --git a/codersdk/provisionerdaemons.go b/codersdk/provisionerdaemons.go index d07eccbaf6724..bc9b495ee720e 100644 --- a/codersdk/provisionerdaemons.go +++ b/codersdk/provisionerdaemons.go @@ -19,7 +19,6 @@ import ( "github.com/coder/coder/v2/codersdk/drpc" "github.com/coder/coder/v2/provisionerd/proto" "github.com/coder/coder/v2/provisionerd/runner" - "github.com/coder/coder/v2/provisionersdk" ) type LogSource string @@ -195,7 +194,7 @@ type ServeProvisionerDaemonRequest struct { // ServeProvisionerDaemon returns the gRPC service for a provisioner daemon // implementation. The context is during dial, not during the lifetime of the // client. Client should be closed after use. -func (c *Client) ServeProvisionerDaemon(ctx context.Context, req ServeProvisionerDaemonRequest) (proto.DRPCProvisionerDaemonClient, error) { +func (c *Client) ServeProvisionerDaemon(ctx context.Context, v *apiversion.APIVersion, req ServeProvisionerDaemonRequest) (proto.DRPCProvisionerDaemonClient, error) { serverURL, err := c.URL.Parse(fmt.Sprintf("/api/v2/organizations/%s/provisionerdaemons/serve", req.Organization)) if err != nil { return nil, xerrors.Errorf("parse url: %w", err) @@ -203,7 +202,7 @@ func (c *Client) ServeProvisionerDaemon(ctx context.Context, req ServeProvisione query := serverURL.Query() query.Add("id", req.ID.String()) query.Add("name", req.Name) - query.Add("version", provisionersdk.VersionCurrent.String()) + query.Add("version", v.String()) for _, provisioner := range req.Provisioners { query.Add("provisioner", string(provisioner)) } diff --git a/enterprise/cli/provisionerdaemons.go b/enterprise/cli/provisionerdaemons.go index b41ec75197aa9..08fa057f7bb44 100644 --- a/enterprise/cli/provisionerdaemons.go +++ b/enterprise/cli/provisionerdaemons.go @@ -173,7 +173,7 @@ func (r *RootCmd) provisionerDaemonStart() *clibase.Cmd { } id := uuid.New() srv := provisionerd.New(func(ctx context.Context) (provisionerdproto.DRPCProvisionerDaemonClient, error) { - return client.ServeProvisionerDaemon(ctx, codersdk.ServeProvisionerDaemonRequest{ + return client.ServeProvisionerDaemon(ctx, provisionersdk.VersionCurrent, codersdk.ServeProvisionerDaemonRequest{ ID: id, Name: name, Provisioners: []codersdk.ProvisionerType{ diff --git a/enterprise/coderd/provisionerdaemons_test.go b/enterprise/coderd/provisionerdaemons_test.go index ac48e21cdd14f..919876c17878c 100644 --- a/enterprise/coderd/provisionerdaemons_test.go +++ b/enterprise/coderd/provisionerdaemons_test.go @@ -42,7 +42,7 @@ func TestProvisionerDaemonServe(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() daemonName := testutil.MustRandString(t, 63) - srv, err := templateAdminClient.ServeProvisionerDaemon(ctx, codersdk.ServeProvisionerDaemonRequest{ + srv, err := templateAdminClient.ServeProvisionerDaemon(ctx, provisionersdk.VersionCurrent, codersdk.ServeProvisionerDaemonRequest{ ID: uuid.New(), Name: daemonName, Organization: user.OrganizationID, @@ -70,7 +70,7 @@ func TestProvisionerDaemonServe(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() daemonName := testutil.MustRandString(t, 63) - _, err := templateAdminClient.ServeProvisionerDaemon(ctx, codersdk.ServeProvisionerDaemonRequest{ + _, err := templateAdminClient.ServeProvisionerDaemon(ctx, provisionersdk.VersionCurrent, codersdk.ServeProvisionerDaemonRequest{ ID: uuid.New(), Name: daemonName, Organization: user.OrganizationID, @@ -95,7 +95,7 @@ func TestProvisionerDaemonServe(t *testing.T) { another, _ := coderdtest.CreateAnotherUser(t, client, user.OrganizationID, rbac.RoleOrgAdmin(user.OrganizationID)) ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() - _, err := another.ServeProvisionerDaemon(ctx, codersdk.ServeProvisionerDaemonRequest{ + _, err := another.ServeProvisionerDaemon(ctx, provisionersdk.VersionCurrent, codersdk.ServeProvisionerDaemonRequest{ ID: uuid.New(), Name: testutil.MustRandString(t, 63), Organization: user.OrganizationID, @@ -122,7 +122,7 @@ func TestProvisionerDaemonServe(t *testing.T) { another, _ := coderdtest.CreateAnotherUser(t, client, user.OrganizationID) ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() - _, err := another.ServeProvisionerDaemon(ctx, codersdk.ServeProvisionerDaemonRequest{ + _, err := another.ServeProvisionerDaemon(ctx, provisionersdk.VersionCurrent, codersdk.ServeProvisionerDaemonRequest{ ID: uuid.New(), Name: testutil.MustRandString(t, 63), Organization: user.OrganizationID, @@ -220,7 +220,7 @@ func TestProvisionerDaemonServe(t *testing.T) { defer cancel() another := codersdk.New(client.URL) daemonName := testutil.MustRandString(t, 63) - srv, err := another.ServeProvisionerDaemon(ctx, codersdk.ServeProvisionerDaemonRequest{ + srv, err := another.ServeProvisionerDaemon(ctx, provisionersdk.VersionCurrent, codersdk.ServeProvisionerDaemonRequest{ Name: daemonName, Organization: user.OrganizationID, Provisioners: []codersdk.ProvisionerType{ @@ -282,7 +282,7 @@ func TestProvisionerDaemonServe(t *testing.T) { } another := codersdk.New(client.URL) pd := provisionerd.New(func(ctx context.Context) (provisionerdproto.DRPCProvisionerDaemonClient, error) { - return another.ServeProvisionerDaemon(ctx, codersdk.ServeProvisionerDaemonRequest{ + return another.ServeProvisionerDaemon(ctx, provisionersdk.VersionCurrent, codersdk.ServeProvisionerDaemonRequest{ ID: uuid.New(), Name: testutil.MustRandString(t, 63), Organization: user.OrganizationID, @@ -360,7 +360,7 @@ func TestProvisionerDaemonServe(t *testing.T) { another := codersdk.New(client.URL) ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() - _, err := another.ServeProvisionerDaemon(ctx, codersdk.ServeProvisionerDaemonRequest{ + _, err := another.ServeProvisionerDaemon(ctx, provisionersdk.VersionCurrent, codersdk.ServeProvisionerDaemonRequest{ ID: uuid.New(), Name: testutil.MustRandString(t, 32), Organization: user.OrganizationID, @@ -395,7 +395,7 @@ func TestProvisionerDaemonServe(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() another := codersdk.New(client.URL) - _, err := another.ServeProvisionerDaemon(ctx, codersdk.ServeProvisionerDaemonRequest{ + _, err := another.ServeProvisionerDaemon(ctx, provisionersdk.VersionCurrent, codersdk.ServeProvisionerDaemonRequest{ ID: uuid.New(), Name: testutil.MustRandString(t, 63), Organization: user.OrganizationID, @@ -428,7 +428,7 @@ func TestProvisionerDaemonServe(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() another := codersdk.New(client.URL) - _, err := another.ServeProvisionerDaemon(ctx, codersdk.ServeProvisionerDaemonRequest{ + _, err := another.ServeProvisionerDaemon(ctx, provisionersdk.VersionCurrent, codersdk.ServeProvisionerDaemonRequest{ ID: uuid.New(), Name: testutil.MustRandString(t, 63), Organization: user.OrganizationID, From 866430bb175d0da9f4f2ffabcd3e382679ca5863 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 16 Feb 2024 14:05:29 +0000 Subject: [PATCH 4/8] add test for missing provisionerd api_version --- codersdk/provisionerdaemons.go | 4 +- enterprise/coderd/provisionerdaemons_test.go | 60 ++++++++++++++++++++ 2 files changed, 63 insertions(+), 1 deletion(-) diff --git a/codersdk/provisionerdaemons.go b/codersdk/provisionerdaemons.go index bc9b495ee720e..71bf1cf6430c8 100644 --- a/codersdk/provisionerdaemons.go +++ b/codersdk/provisionerdaemons.go @@ -202,7 +202,9 @@ func (c *Client) ServeProvisionerDaemon(ctx context.Context, v *apiversion.APIVe query := serverURL.Query() query.Add("id", req.ID.String()) query.Add("name", req.Name) - query.Add("version", v.String()) + if v != nil { // This is only done in tests + query.Add("version", v.String()) + } for _, provisioner := range req.Provisioners { query.Add("provisioner", string(provisioner)) } diff --git a/enterprise/coderd/provisionerdaemons_test.go b/enterprise/coderd/provisionerdaemons_test.go index 919876c17878c..c0f45a1c4c780 100644 --- a/enterprise/coderd/provisionerdaemons_test.go +++ b/enterprise/coderd/provisionerdaemons_test.go @@ -12,6 +12,7 @@ import ( "cdr.dev/slog" "cdr.dev/slog/sloggers/slogtest" + "github.com/coder/coder/v2/apiversion" "github.com/coder/coder/v2/buildinfo" "github.com/coder/coder/v2/coderd/coderdtest" "github.com/coder/coder/v2/coderd/database" @@ -63,6 +64,65 @@ func TestProvisionerDaemonServe(t *testing.T) { } }) + t.Run("NoVersion", func(t *testing.T) { + t.Parallel() + client, user := coderdenttest.New(t, &coderdenttest.Options{LicenseOptions: &coderdenttest.LicenseOptions{ + Features: license.Features{ + codersdk.FeatureExternalProvisionerDaemons: 1, + }, + }}) + templateAdminClient, _ := coderdtest.CreateAnotherUser(t, client, user.OrganizationID, rbac.RoleTemplateAdmin()) + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + daemonName := testutil.MustRandString(t, 63) + srv, err := templateAdminClient.ServeProvisionerDaemon(ctx, nil, codersdk.ServeProvisionerDaemonRequest{ + ID: uuid.New(), + Name: daemonName, + Organization: user.OrganizationID, + Provisioners: []codersdk.ProvisionerType{ + codersdk.ProvisionerTypeEcho, + }, + Tags: map[string]string{}, + }) + require.NoError(t, err) + srv.DRPCConn().Close() + + daemons, err := client.ProvisionerDaemons(ctx) //nolint:gocritic // Test assertion. + require.NoError(t, err) + if assert.Len(t, daemons, 1) { + assert.Equal(t, daemonName, daemons[0].Name) + assert.Equal(t, buildinfo.Version(), daemons[0].Version) + assert.Equal(t, "1.0", daemons[0].APIVersion) + } + }) + + t.Run("OldVersion", func(t *testing.T) { + t.Parallel() + client, user := coderdenttest.New(t, &coderdenttest.Options{LicenseOptions: &coderdenttest.LicenseOptions{ + Features: license.Features{ + codersdk.FeatureExternalProvisionerDaemons: 1, + }, + }}) + templateAdminClient, _ := coderdtest.CreateAnotherUser(t, client, user.OrganizationID, rbac.RoleTemplateAdmin()) + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + daemonName := testutil.MustRandString(t, 63) + _, err := templateAdminClient.ServeProvisionerDaemon(ctx, apiversion.New(provisionersdk.CurrentMajor+1, provisionersdk.CurrentMinor+1), codersdk.ServeProvisionerDaemonRequest{ + ID: uuid.New(), + Name: daemonName, + Organization: user.OrganizationID, + Provisioners: []codersdk.ProvisionerType{ + codersdk.ProvisionerTypeEcho, + }, + Tags: map[string]string{}, + }) + require.Error(t, err) + var apiError *codersdk.Error + require.ErrorAs(t, err, &apiError) + require.Equal(t, http.StatusBadRequest, apiError.StatusCode()) + require.Contains(t, apiError.Error(), "server is at version 1.0, behind requested major version 2.1") + }) + t.Run("NoLicense", func(t *testing.T) { t.Parallel() client, user := coderdenttest.New(t, &coderdenttest.Options{DontAddLicense: true}) From 334ac5d017cd07bf7f9ef9c930a4e1d78fd13540 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 16 Feb 2024 18:09:23 +0000 Subject: [PATCH 5/8] rework new enterprise/coderd tests to not require version argument to ServeProvisionerDaemon --- enterprise/coderd/provisionerdaemons_test.go | 123 +++++++++++++------ 1 file changed, 84 insertions(+), 39 deletions(-) diff --git a/enterprise/coderd/provisionerdaemons_test.go b/enterprise/coderd/provisionerdaemons_test.go index c0f45a1c4c780..bc5035ce484ac 100644 --- a/enterprise/coderd/provisionerdaemons_test.go +++ b/enterprise/coderd/provisionerdaemons_test.go @@ -3,6 +3,8 @@ package coderd_test import ( "bytes" "context" + "fmt" + "io" "net/http" "testing" @@ -66,61 +68,104 @@ func TestProvisionerDaemonServe(t *testing.T) { t.Run("NoVersion", func(t *testing.T) { t.Parallel() - client, user := coderdenttest.New(t, &coderdenttest.Options{LicenseOptions: &coderdenttest.LicenseOptions{ - Features: license.Features{ - codersdk.FeatureExternalProvisionerDaemons: 1, + // In this test, we just send a HTTP request with minimal parameters to the provisionerdaemons + // endpoint. We do not pass the required machinery to start a websocket connection, so we expect a + // WebSocket protocol violation. This just means the pre-flight checks have passed though. + + // Sending a HTTP request triggers an error log, which would otherwise fail the test. + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}) + client, user := coderdenttest.New(t, &coderdenttest.Options{ + LicenseOptions: &coderdenttest.LicenseOptions{ + Features: license.Features{ + codersdk.FeatureExternalProvisionerDaemons: 1, + }, }, - }}) - templateAdminClient, _ := coderdtest.CreateAnotherUser(t, client, user.OrganizationID, rbac.RoleTemplateAdmin()) - ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) - defer cancel() - daemonName := testutil.MustRandString(t, 63) - srv, err := templateAdminClient.ServeProvisionerDaemon(ctx, nil, codersdk.ServeProvisionerDaemonRequest{ - ID: uuid.New(), - Name: daemonName, - Organization: user.OrganizationID, - Provisioners: []codersdk.ProvisionerType{ - codersdk.ProvisionerTypeEcho, + ProvisionerDaemonPSK: "provisionersftw", + Options: &coderdtest.Options{ + Logger: &logger, }, - Tags: map[string]string{}, }) + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + // Formulate the correct URL for provisionerd server. + srvURL, err := client.URL.Parse(fmt.Sprintf("/api/v2/organizations/%s/provisionerdaemons/serve", user.OrganizationID)) require.NoError(t, err) - srv.DRPCConn().Close() + q := srvURL.Query() + // Set required query parameters. + q.Add("provisioner", "echo") + // Note: Explicitly not setting API version. + q.Add("version", "") + srvURL.RawQuery = q.Encode() + + // Set PSK header for auth. + req, err := http.NewRequestWithContext(ctx, http.MethodGet, srvURL.String(), nil) + require.NoError(t, err) + req.Header.Set(codersdk.ProvisionerDaemonPSK, "provisionersftw") + + // Do the request! + resp, err := client.HTTPClient.Do(req) + require.NoError(t, err) + defer resp.Body.Close() + b, err := io.ReadAll(resp.Body) + require.NoError(t, err) + // The below means that provisionerd tried to serve us! + require.Contains(t, string(b), "Internal error accepting websocket connection.") daemons, err := client.ProvisionerDaemons(ctx) //nolint:gocritic // Test assertion. require.NoError(t, err) if assert.Len(t, daemons, 1) { - assert.Equal(t, daemonName, daemons[0].Name) - assert.Equal(t, buildinfo.Version(), daemons[0].Version) - assert.Equal(t, "1.0", daemons[0].APIVersion) + assert.Equal(t, "1.0", daemons[0].APIVersion) // The whole point of this test is here. } }) t.Run("OldVersion", func(t *testing.T) { t.Parallel() - client, user := coderdenttest.New(t, &coderdenttest.Options{LicenseOptions: &coderdenttest.LicenseOptions{ - Features: license.Features{ - codersdk.FeatureExternalProvisionerDaemons: 1, + // In this test, we just send a HTTP request with minimal parameters to the provisionerdaemons + // endpoint. We do not pass the required machinery to start a websocket connection, so we expect a + // WebSocket protocol violation. This just means the pre-flight checks have passed though. + + // Sending a HTTP request triggers an error log, which would otherwise fail the test. + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}) + client, user := coderdenttest.New(t, &coderdenttest.Options{ + LicenseOptions: &coderdenttest.LicenseOptions{ + Features: license.Features{ + codersdk.FeatureExternalProvisionerDaemons: 1, + }, }, - }}) - templateAdminClient, _ := coderdtest.CreateAnotherUser(t, client, user.OrganizationID, rbac.RoleTemplateAdmin()) - ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) - defer cancel() - daemonName := testutil.MustRandString(t, 63) - _, err := templateAdminClient.ServeProvisionerDaemon(ctx, apiversion.New(provisionersdk.CurrentMajor+1, provisionersdk.CurrentMinor+1), codersdk.ServeProvisionerDaemonRequest{ - ID: uuid.New(), - Name: daemonName, - Organization: user.OrganizationID, - Provisioners: []codersdk.ProvisionerType{ - codersdk.ProvisionerTypeEcho, + ProvisionerDaemonPSK: "provisionersftw", + Options: &coderdtest.Options{ + Logger: &logger, }, - Tags: map[string]string{}, }) - require.Error(t, err) - var apiError *codersdk.Error - require.ErrorAs(t, err, &apiError) - require.Equal(t, http.StatusBadRequest, apiError.StatusCode()) - require.Contains(t, apiError.Error(), "server is at version 1.0, behind requested major version 2.1") + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + // Formulate the correct URL for provisionerd server. + srvURL, err := client.URL.Parse(fmt.Sprintf("/api/v2/organizations/%s/provisionerdaemons/serve", user.OrganizationID)) + require.NoError(t, err) + q := srvURL.Query() + // Set required query parameters. + q.Add("provisioner", "echo") + + // Set a different (newer) version than the current. + v := apiversion.New(provisionersdk.CurrentMajor+1, provisionersdk.CurrentMinor+1) + q.Add("version", v.String()) + srvURL.RawQuery = q.Encode() + + // Set PSK header for auth. + req, err := http.NewRequestWithContext(ctx, http.MethodGet, srvURL.String(), nil) + require.NoError(t, err) + req.Header.Set(codersdk.ProvisionerDaemonPSK, "provisionersftw") + + // Do the request! + resp, err := client.HTTPClient.Do(req) + require.NoError(t, err) + defer resp.Body.Close() + b, err := io.ReadAll(resp.Body) + require.NoError(t, err) + // The below means that provisionerd tried to serve us, checked our api version, and said nope. + require.Contains(t, string(b), "server is at version 1.0, behind requested major version 2.1") }) t.Run("NoLicense", func(t *testing.T) { From a0afb65ec5470362507052c66b65f68d40c34258 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 16 Feb 2024 18:15:58 +0000 Subject: [PATCH 6/8] restore original signature of codersdk.ServeProvisionerDaemon --- coderd/coderdtest/coderdtest.go | 2 +- codersdk/provisionerdaemons.go | 9 ++++----- enterprise/cli/provisionerdaemons.go | 2 +- enterprise/coderd/provisionerdaemons_test.go | 18 +++++++++--------- 4 files changed, 15 insertions(+), 16 deletions(-) diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index 3c825fa82f9cd..b1c496e4ba295 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -571,7 +571,7 @@ func NewExternalProvisionerDaemon(t testing.TB, client *codersdk.Client, org uui }() daemon := provisionerd.New(func(ctx context.Context) (provisionerdproto.DRPCProvisionerDaemonClient, error) { - return client.ServeProvisionerDaemon(ctx, provisionersdk.VersionCurrent, codersdk.ServeProvisionerDaemonRequest{ + return client.ServeProvisionerDaemon(ctx, codersdk.ServeProvisionerDaemonRequest{ ID: uuid.New(), Name: t.Name(), Organization: org, diff --git a/codersdk/provisionerdaemons.go b/codersdk/provisionerdaemons.go index 71bf1cf6430c8..ba5fd64b1ac5e 100644 --- a/codersdk/provisionerdaemons.go +++ b/codersdk/provisionerdaemons.go @@ -14,11 +14,11 @@ import ( "golang.org/x/xerrors" "nhooyr.io/websocket" - "github.com/coder/coder/v2/apiversion" "github.com/coder/coder/v2/buildinfo" "github.com/coder/coder/v2/codersdk/drpc" "github.com/coder/coder/v2/provisionerd/proto" "github.com/coder/coder/v2/provisionerd/runner" + "github.com/coder/coder/v2/provisionersdk" ) type LogSource string @@ -194,7 +194,7 @@ type ServeProvisionerDaemonRequest struct { // ServeProvisionerDaemon returns the gRPC service for a provisioner daemon // implementation. The context is during dial, not during the lifetime of the // client. Client should be closed after use. -func (c *Client) ServeProvisionerDaemon(ctx context.Context, v *apiversion.APIVersion, req ServeProvisionerDaemonRequest) (proto.DRPCProvisionerDaemonClient, error) { +func (c *Client) ServeProvisionerDaemon(ctx context.Context, req ServeProvisionerDaemonRequest) (proto.DRPCProvisionerDaemonClient, error) { serverURL, err := c.URL.Parse(fmt.Sprintf("/api/v2/organizations/%s/provisionerdaemons/serve", req.Organization)) if err != nil { return nil, xerrors.Errorf("parse url: %w", err) @@ -202,9 +202,8 @@ func (c *Client) ServeProvisionerDaemon(ctx context.Context, v *apiversion.APIVe query := serverURL.Query() query.Add("id", req.ID.String()) query.Add("name", req.Name) - if v != nil { // This is only done in tests - query.Add("version", v.String()) - } + query.Add("version", provisionersdk.VersionCurrent.String()) + for _, provisioner := range req.Provisioners { query.Add("provisioner", string(provisioner)) } diff --git a/enterprise/cli/provisionerdaemons.go b/enterprise/cli/provisionerdaemons.go index 08fa057f7bb44..b41ec75197aa9 100644 --- a/enterprise/cli/provisionerdaemons.go +++ b/enterprise/cli/provisionerdaemons.go @@ -173,7 +173,7 @@ func (r *RootCmd) provisionerDaemonStart() *clibase.Cmd { } id := uuid.New() srv := provisionerd.New(func(ctx context.Context) (provisionerdproto.DRPCProvisionerDaemonClient, error) { - return client.ServeProvisionerDaemon(ctx, provisionersdk.VersionCurrent, codersdk.ServeProvisionerDaemonRequest{ + return client.ServeProvisionerDaemon(ctx, codersdk.ServeProvisionerDaemonRequest{ ID: id, Name: name, Provisioners: []codersdk.ProvisionerType{ diff --git a/enterprise/coderd/provisionerdaemons_test.go b/enterprise/coderd/provisionerdaemons_test.go index bc5035ce484ac..3c02256465596 100644 --- a/enterprise/coderd/provisionerdaemons_test.go +++ b/enterprise/coderd/provisionerdaemons_test.go @@ -45,7 +45,7 @@ func TestProvisionerDaemonServe(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() daemonName := testutil.MustRandString(t, 63) - srv, err := templateAdminClient.ServeProvisionerDaemon(ctx, provisionersdk.VersionCurrent, codersdk.ServeProvisionerDaemonRequest{ + srv, err := templateAdminClient.ServeProvisionerDaemon(ctx, codersdk.ServeProvisionerDaemonRequest{ ID: uuid.New(), Name: daemonName, Organization: user.OrganizationID, @@ -175,7 +175,7 @@ func TestProvisionerDaemonServe(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() daemonName := testutil.MustRandString(t, 63) - _, err := templateAdminClient.ServeProvisionerDaemon(ctx, provisionersdk.VersionCurrent, codersdk.ServeProvisionerDaemonRequest{ + _, err := templateAdminClient.ServeProvisionerDaemon(ctx, codersdk.ServeProvisionerDaemonRequest{ ID: uuid.New(), Name: daemonName, Organization: user.OrganizationID, @@ -200,7 +200,7 @@ func TestProvisionerDaemonServe(t *testing.T) { another, _ := coderdtest.CreateAnotherUser(t, client, user.OrganizationID, rbac.RoleOrgAdmin(user.OrganizationID)) ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() - _, err := another.ServeProvisionerDaemon(ctx, provisionersdk.VersionCurrent, codersdk.ServeProvisionerDaemonRequest{ + _, err := another.ServeProvisionerDaemon(ctx, codersdk.ServeProvisionerDaemonRequest{ ID: uuid.New(), Name: testutil.MustRandString(t, 63), Organization: user.OrganizationID, @@ -227,7 +227,7 @@ func TestProvisionerDaemonServe(t *testing.T) { another, _ := coderdtest.CreateAnotherUser(t, client, user.OrganizationID) ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() - _, err := another.ServeProvisionerDaemon(ctx, provisionersdk.VersionCurrent, codersdk.ServeProvisionerDaemonRequest{ + _, err := another.ServeProvisionerDaemon(ctx, codersdk.ServeProvisionerDaemonRequest{ ID: uuid.New(), Name: testutil.MustRandString(t, 63), Organization: user.OrganizationID, @@ -325,7 +325,7 @@ func TestProvisionerDaemonServe(t *testing.T) { defer cancel() another := codersdk.New(client.URL) daemonName := testutil.MustRandString(t, 63) - srv, err := another.ServeProvisionerDaemon(ctx, provisionersdk.VersionCurrent, codersdk.ServeProvisionerDaemonRequest{ + srv, err := another.ServeProvisionerDaemon(ctx, codersdk.ServeProvisionerDaemonRequest{ Name: daemonName, Organization: user.OrganizationID, Provisioners: []codersdk.ProvisionerType{ @@ -387,7 +387,7 @@ func TestProvisionerDaemonServe(t *testing.T) { } another := codersdk.New(client.URL) pd := provisionerd.New(func(ctx context.Context) (provisionerdproto.DRPCProvisionerDaemonClient, error) { - return another.ServeProvisionerDaemon(ctx, provisionersdk.VersionCurrent, codersdk.ServeProvisionerDaemonRequest{ + return another.ServeProvisionerDaemon(ctx, codersdk.ServeProvisionerDaemonRequest{ ID: uuid.New(), Name: testutil.MustRandString(t, 63), Organization: user.OrganizationID, @@ -465,7 +465,7 @@ func TestProvisionerDaemonServe(t *testing.T) { another := codersdk.New(client.URL) ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() - _, err := another.ServeProvisionerDaemon(ctx, provisionersdk.VersionCurrent, codersdk.ServeProvisionerDaemonRequest{ + _, err := another.ServeProvisionerDaemon(ctx, codersdk.ServeProvisionerDaemonRequest{ ID: uuid.New(), Name: testutil.MustRandString(t, 32), Organization: user.OrganizationID, @@ -500,7 +500,7 @@ func TestProvisionerDaemonServe(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() another := codersdk.New(client.URL) - _, err := another.ServeProvisionerDaemon(ctx, provisionersdk.VersionCurrent, codersdk.ServeProvisionerDaemonRequest{ + _, err := another.ServeProvisionerDaemon(ctx, codersdk.ServeProvisionerDaemonRequest{ ID: uuid.New(), Name: testutil.MustRandString(t, 63), Organization: user.OrganizationID, @@ -533,7 +533,7 @@ func TestProvisionerDaemonServe(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() another := codersdk.New(client.URL) - _, err := another.ServeProvisionerDaemon(ctx, provisionersdk.VersionCurrent, codersdk.ServeProvisionerDaemonRequest{ + _, err := another.ServeProvisionerDaemon(ctx, codersdk.ServeProvisionerDaemonRequest{ ID: uuid.New(), Name: testutil.MustRandString(t, 63), Organization: user.OrganizationID, From 1d7516a9bf4f44ce5ec4944504b8b000afdf4ccd Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 16 Feb 2024 18:18:30 +0000 Subject: [PATCH 7/8] revert unrelated change from over-zealous linters --- docs/templates/resource-ordering.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/templates/resource-ordering.md b/docs/templates/resource-ordering.md index 5626dcdf4073d..00bf778b8b232 100644 --- a/docs/templates/resource-ordering.md +++ b/docs/templates/resource-ordering.md @@ -8,7 +8,7 @@ The resource with the lower `order` is presented before the one with greater value. A missing `order` property defaults to 0. If two resources have the same `order` property, the resources will be ordered by property `name` (or `key`). -## Using `order` property +## Using "order" property ### Coder parameters From fb69b8083d11cf1db1ee9317f26410bc997f4405 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 16 Feb 2024 18:21:41 +0000 Subject: [PATCH 8/8] update comment in test --- enterprise/coderd/provisionerdaemons_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/enterprise/coderd/provisionerdaemons_test.go b/enterprise/coderd/provisionerdaemons_test.go index 3c02256465596..d326974c96732 100644 --- a/enterprise/coderd/provisionerdaemons_test.go +++ b/enterprise/coderd/provisionerdaemons_test.go @@ -122,8 +122,8 @@ func TestProvisionerDaemonServe(t *testing.T) { t.Run("OldVersion", func(t *testing.T) { t.Parallel() // In this test, we just send a HTTP request with minimal parameters to the provisionerdaemons - // endpoint. We do not pass the required machinery to start a websocket connection, so we expect a - // WebSocket protocol violation. This just means the pre-flight checks have passed though. + // endpoint. We do not pass the required machinery to start a websocket connection, but we pass a + // version header that should cause provisionerd to refuse to serve us, so no websocket for you! // Sending a HTTP request triggers an error log, which would otherwise fail the test. logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true})