From 4c4ef62a7b2f6ebd460512fae5e9ee1ee326708a Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Thu, 7 Dec 2023 11:49:18 +0000 Subject: [PATCH 1/8] feat(cli): allow specifying name of provisioner daemon --- cli/cliutil/hostname.go | 25 ++++++++++++++++++ cli/server.go | 9 ++++--- coderd/coderd.go | 6 ++--- coderd/coderdtest/coderdtest.go | 4 ++- codersdk/provisionerdaemons.go | 3 +++ docs/cli/provisionerd_start.md | 10 +++++++ enterprise/cli/provisionerdaemons.go | 18 +++++++++++-- enterprise/cli/provisionerdaemons_test.go | 26 +++++++++++++++++++ .../coder_provisionerd_start_--help.golden | 4 +++ enterprise/coderd/provisionerdaemons.go | 10 ++++++- enterprise/coderd/provisionerdaemons_test.go | 16 ++++++++++++ enterprise/replicasync/replicasync.go | 7 ++--- enterprise/wsproxy/wsproxy.go | 7 ++--- scripts/coder-dev.sh | 3 +++ 14 files changed, 127 insertions(+), 21 deletions(-) create mode 100644 cli/cliutil/hostname.go diff --git a/cli/cliutil/hostname.go b/cli/cliutil/hostname.go new file mode 100644 index 0000000000000..0695be4757964 --- /dev/null +++ b/cli/cliutil/hostname.go @@ -0,0 +1,25 @@ +package cliutil + +import ( + "os" + "sync" + + "golang.org/x/xerrors" +) + +var ( + hostname string + hostnameOnce sync.Once +) + +func Hostname() string { + hostnameOnce.Do(func() { + h, err := os.Hostname() + if err != nil { + // Something must be very wrong if this fails. + panic(xerrors.Errorf("lookup hostname: %w", err)) + } + hostname = h + }) + return hostname +} diff --git a/cli/server.go b/cli/server.go index 99dccc1088646..dd4302e615aea 100644 --- a/cli/server.go +++ b/cli/server.go @@ -62,6 +62,7 @@ import ( "github.com/coder/coder/v2/buildinfo" "github.com/coder/coder/v2/cli/clibase" "github.com/coder/coder/v2/cli/cliui" + "github.com/coder/coder/v2/cli/cliutil" "github.com/coder/coder/v2/cli/config" "github.com/coder/coder/v2/coderd" "github.com/coder/coder/v2/coderd/autobuild" @@ -875,9 +876,10 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd. defer provisionerdWaitGroup.Wait() provisionerdMetrics := provisionerd.NewMetrics(options.PrometheusRegistry) for i := int64(0); i < vals.Provisioner.Daemons.Value(); i++ { + name := fmt.Sprintf("%s-%d", cliutil.Hostname(), i) daemonCacheDir := filepath.Join(cacheDir, fmt.Sprintf("provisioner-%d", i)) daemon, err := newProvisionerDaemon( - ctx, coderAPI, provisionerdMetrics, logger, vals, daemonCacheDir, errCh, &provisionerdWaitGroup, + ctx, coderAPI, provisionerdMetrics, logger, vals, daemonCacheDir, errCh, &provisionerdWaitGroup, name, ) if err != nil { return xerrors.Errorf("create provisioner daemon: %w", err) @@ -1243,6 +1245,7 @@ func newProvisionerDaemon( cacheDir string, errCh chan error, wg *sync.WaitGroup, + name string, ) (srv *provisionerd.Server, err error) { ctx, cancel := context.WithCancel(ctx) defer func() { @@ -1334,9 +1337,9 @@ func newProvisionerDaemon( return provisionerd.New(func(ctx context.Context) (proto.DRPCProvisionerDaemonClient, error) { // This debounces calls to listen every second. Read the comment // in provisionerdserver.go to learn more! - return coderAPI.CreateInMemoryProvisionerDaemon(ctx) + return coderAPI.CreateInMemoryProvisionerDaemon(ctx, name) }, &provisionerd.Options{ - Logger: logger.Named("provisionerd"), + Logger: logger.Named(fmt.Sprintf("provisionerd-%s", name)), UpdateInterval: time.Second, ForceCancelInterval: cfg.Provisioner.ForceCancelInterval.Value(), Connector: connector, diff --git a/coderd/coderd.go b/coderd/coderd.go index 2a6078bf2776c..bacc84de58b1c 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -21,7 +21,6 @@ import ( "github.com/go-chi/chi/v5/middleware" "github.com/google/uuid" "github.com/klauspost/compress/zstd" - "github.com/moby/moby/pkg/namesgenerator" "github.com/prometheus/client_golang/prometheus" httpSwagger "github.com/swaggo/http-swagger/v2" "go.opentelemetry.io/otel/trace" @@ -1150,7 +1149,7 @@ func compressHandler(h http.Handler) http.Handler { // CreateInMemoryProvisionerDaemon is an in-memory connection to a provisionerd. // Useful when starting coderd and provisionerd in the same process. -func (api *API) CreateInMemoryProvisionerDaemon(ctx context.Context) (client proto.DRPCProvisionerDaemonClient, err error) { +func (api *API) CreateInMemoryProvisionerDaemon(ctx context.Context, name string) (client proto.DRPCProvisionerDaemonClient, err error) { tracer := api.TracerProvider.Tracer(tracing.TracerName) clientSession, serverSession := provisionersdk.MemTransportPipe() defer func() { @@ -1165,9 +1164,8 @@ func (api *API) CreateInMemoryProvisionerDaemon(ctx context.Context) (client pro } mux := drpcmux.New() - name := namesgenerator.GetRandomName(1) + api.Logger.Info(ctx, "starting in-memory provisioner daemon", slog.F("name", name)) logger := api.Logger.Named(fmt.Sprintf("inmem-provisionerd-%s", name)) - logger.Info(ctx, "starting in-memory provisioner daemon") srv, err := provisionerdserver.NewServer( api.ctx, api.AccessURL, diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index f5ed26cfe97d6..036a390235529 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -530,7 +530,7 @@ func NewProvisionerDaemon(t testing.TB, coderAPI *coderd.API) io.Closer { }() daemon := provisionerd.New(func(ctx context.Context) (provisionerdproto.DRPCProvisionerDaemonClient, error) { - return coderAPI.CreateInMemoryProvisionerDaemon(ctx) + return coderAPI.CreateInMemoryProvisionerDaemon(ctx, t.Name()) }, &provisionerd.Options{ Logger: coderAPI.Logger.Named("provisionerd").Leveled(slog.LevelDebug), UpdateInterval: 250 * time.Millisecond, @@ -567,6 +567,8 @@ 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{ + ID: uuid.New(), + Name: t.Name(), Organization: org, Provisioners: []codersdk.ProvisionerType{codersdk.ProvisionerTypeEcho}, Tags: tags, diff --git a/codersdk/provisionerdaemons.go b/codersdk/provisionerdaemons.go index abc78236807f9..e277a7aad0e07 100644 --- a/codersdk/provisionerdaemons.go +++ b/codersdk/provisionerdaemons.go @@ -177,6 +177,8 @@ func (c *Client) provisionerJobLogsAfter(ctx context.Context, path string, after type ServeProvisionerDaemonRequest struct { // ID is a unique ID for a provisioner daemon. ID uuid.UUID `json:"id" format:"uuid"` + // Name is the human-readable unique identifier for the daemon. + Name string `json:"name"` // Organization is the organization for the URL. At present provisioner daemons ARE NOT scoped to organizations // and so the organization ID is optional. Organization uuid.UUID `json:"organization" format:"uuid"` @@ -198,6 +200,7 @@ func (c *Client) ServeProvisionerDaemon(ctx context.Context, req ServeProvisione } query := serverURL.Query() query.Add("id", req.ID.String()) + query.Add("name", req.Name) for _, provisioner := range req.Provisioners { query.Add("provisioner", string(provisioner)) } diff --git a/docs/cli/provisionerd_start.md b/docs/cli/provisionerd_start.md index 8f7e72b01207a..64e8e0792cb28 100644 --- a/docs/cli/provisionerd_start.md +++ b/docs/cli/provisionerd_start.md @@ -22,6 +22,16 @@ coder provisionerd start [flags] Directory to store cached data. +### --name + +| | | +| ----------- | ------------------------------------------- | +| Type | string | +| Environment | $CODER_PROVISIONER_DAEMON_NAME | +| Default | ser6 | + +Name of this provisioner daemon. Defaults to the current hostname without FQDN. + ### --poll-interval | | | diff --git a/enterprise/cli/provisionerdaemons.go b/enterprise/cli/provisionerdaemons.go index 6583f1b1a8329..1bfadde542875 100644 --- a/enterprise/cli/provisionerdaemons.go +++ b/enterprise/cli/provisionerdaemons.go @@ -16,6 +16,7 @@ import ( agpl "github.com/coder/coder/v2/cli" "github.com/coder/coder/v2/cli/clibase" "github.com/coder/coder/v2/cli/cliui" + "github.com/coder/coder/v2/cli/cliutil" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/provisionerdserver" "github.com/coder/coder/v2/codersdk" @@ -48,6 +49,7 @@ func (r *RootCmd) provisionerDaemonStart() *clibase.Cmd { pollInterval time.Duration pollJitter time.Duration preSharedKey string + name string ) client := new(codersdk.Client) cmd := &clibase.Cmd{ @@ -68,6 +70,10 @@ func (r *RootCmd) provisionerDaemonStart() *clibase.Cmd { return err } + if len(name) > 64 { + return xerrors.Errorf("name cannot be greater than 64 characters in length") + } + logger := slog.Make(sloghuman.Sink(inv.Stderr)) if ok, _ := inv.ParsedFlags().GetBool("verbose"); ok { logger = logger.Leveled(slog.LevelDebug) @@ -122,7 +128,7 @@ func (r *RootCmd) provisionerDaemonStart() *clibase.Cmd { } }() - logger.Info(ctx, "starting provisioner daemon", slog.F("tags", tags)) + logger.Info(ctx, "starting provisioner daemon", slog.F("tags", tags), slog.F("name", name)) connector := provisionerd.LocalProvisioners{ string(database.ProvisionerTypeTerraform): proto.NewDRPCProvisionerClient(terraformClient), @@ -130,7 +136,8 @@ 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{ - ID: id, + ID: id, + Name: name, Provisioners: []codersdk.ProvisionerType{ codersdk.ProvisionerTypeTerraform, }, @@ -205,6 +212,13 @@ func (r *RootCmd) provisionerDaemonStart() *clibase.Cmd { Description: "Pre-shared key to authenticate with Coder server.", Value: clibase.StringOf(&preSharedKey), }, + { + Flag: "name", + Env: "CODER_PROVISIONER_DAEMON_NAME", + Description: "Name of this provisioner daemon. Defaults to the current hostname without FQDN.", + Value: clibase.StringOf(&name), + Default: cliutil.Hostname(), + }, } return cmd diff --git a/enterprise/cli/provisionerdaemons_test.go b/enterprise/cli/provisionerdaemons_test.go index ff8ca63f572e9..6b17f0c564da6 100644 --- a/enterprise/cli/provisionerdaemons_test.go +++ b/enterprise/cli/provisionerdaemons_test.go @@ -36,6 +36,32 @@ func TestProvisionerDaemon_PSK(t *testing.T) { pty.ExpectMatchContext(ctx, "starting provisioner daemon") } +func TestProvisionerDaemon_Named(t *testing.T) { + t.Parallel() + + client, _ := coderdenttest.New(t, &coderdenttest.Options{ + ProvisionerDaemonPSK: "provisionersftw", + LicenseOptions: &coderdenttest.LicenseOptions{ + Features: license.Features{ + codersdk.FeatureExternalProvisionerDaemons: 1, + }, + }, + }) + + inv, conf := newCLI(t, "provisionerd", "start", + "--psk=provisionersftw", + "--name=matt-daemon", + ) + err := conf.URL().Write(client.URL.String()) + require.NoError(t, err) + pty := ptytest.New(t).Attach(inv) + ctx, cancel := context.WithTimeout(inv.Context(), testutil.WaitLong) + defer cancel() + clitest.Start(t, inv) + pty.ExpectMatchContext(ctx, "starting provisioner daemon") + pty.ExpectMatchContext(ctx, "matt-daemon") +} + func TestProvisionerDaemon_SessionToken(t *testing.T) { t.Parallel() t.Run("ScopeUser", func(t *testing.T) { diff --git a/enterprise/cli/testdata/coder_provisionerd_start_--help.golden b/enterprise/cli/testdata/coder_provisionerd_start_--help.golden index 80d28883a8854..59f01f7f6f3d2 100644 --- a/enterprise/cli/testdata/coder_provisionerd_start_--help.golden +++ b/enterprise/cli/testdata/coder_provisionerd_start_--help.golden @@ -9,6 +9,10 @@ OPTIONS: -c, --cache-dir string, $CODER_CACHE_DIRECTORY (default: [cache dir]) Directory to store cached data. + --name string, $CODER_PROVISIONER_DAEMON_NAME (default: ser6) + Name of this provisioner daemon. Defaults to the current hostname + without FQDN. + --poll-interval duration, $CODER_PROVISIONERD_POLL_INTERVAL (default: 1s) Deprecated and ignored. diff --git a/enterprise/coderd/provisionerdaemons.go b/enterprise/coderd/provisionerdaemons.go index 3b3c3b6f0251c..8778758c37f1c 100644 --- a/enterprise/coderd/provisionerdaemons.go +++ b/enterprise/coderd/provisionerdaemons.go @@ -178,6 +178,14 @@ func (api *API) provisionerDaemonServe(rw http.ResponseWriter, r *http.Request) } } + var name string + if vals, ok := r.URL.Query()["name"]; !ok || len(vals) == 0 { + api.Logger.Warn(ctx, "unnamed provisioner daemon") + name = namesgenerator.GetRandomName(1) + } else { + name = vals[0] + } + tags, authorized := api.provisionerDaemonAuth.authorize(r, tags) if !authorized { api.Logger.Warn(ctx, "unauthorized provisioner daemon serve request", slog.F("tags", tags)) @@ -206,7 +214,7 @@ func (api *API) provisionerDaemonServe(rw http.ResponseWriter, r *http.Request) } } - name := namesgenerator.GetRandomName(1) + // name := namesgenerator.GetRandomName(1) log := api.Logger.With( slog.F("name", name), slog.F("provisioners", provisioners), diff --git a/enterprise/coderd/provisionerdaemons_test.go b/enterprise/coderd/provisionerdaemons_test.go index b1348c2878453..5d21a841111fc 100644 --- a/enterprise/coderd/provisionerdaemons_test.go +++ b/enterprise/coderd/provisionerdaemons_test.go @@ -40,6 +40,8 @@ func TestProvisionerDaemonServe(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() srv, err := templateAdminClient.ServeProvisionerDaemon(ctx, codersdk.ServeProvisionerDaemonRequest{ + ID: uuid.New(), + Name: t.Name(), Organization: user.OrganizationID, Provisioners: []codersdk.ProvisionerType{ codersdk.ProvisionerTypeEcho, @@ -57,6 +59,8 @@ func TestProvisionerDaemonServe(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() _, err := templateAdminClient.ServeProvisionerDaemon(ctx, codersdk.ServeProvisionerDaemonRequest{ + ID: uuid.New(), + Name: t.Name(), Organization: user.OrganizationID, Provisioners: []codersdk.ProvisionerType{ codersdk.ProvisionerTypeEcho, @@ -80,6 +84,8 @@ func TestProvisionerDaemonServe(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() _, err := another.ServeProvisionerDaemon(ctx, codersdk.ServeProvisionerDaemonRequest{ + ID: uuid.New(), + Name: t.Name(), Organization: user.OrganizationID, Provisioners: []codersdk.ProvisionerType{ codersdk.ProvisionerTypeEcho, @@ -105,6 +111,8 @@ func TestProvisionerDaemonServe(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() _, err := another.ServeProvisionerDaemon(ctx, codersdk.ServeProvisionerDaemonRequest{ + ID: uuid.New(), + Name: t.Name(), Organization: user.OrganizationID, Provisioners: []codersdk.ProvisionerType{ codersdk.ProvisionerTypeEcho, @@ -245,6 +253,8 @@ 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{ + ID: uuid.New(), + Name: t.Name(), Organization: user.OrganizationID, Provisioners: []codersdk.ProvisionerType{ codersdk.ProvisionerTypeEcho, @@ -321,6 +331,8 @@ func TestProvisionerDaemonServe(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() _, err := another.ServeProvisionerDaemon(ctx, codersdk.ServeProvisionerDaemonRequest{ + ID: uuid.New(), + Name: t.Name(), Organization: user.OrganizationID, Provisioners: []codersdk.ProvisionerType{ codersdk.ProvisionerTypeEcho, @@ -350,6 +362,8 @@ func TestProvisionerDaemonServe(t *testing.T) { defer cancel() another := codersdk.New(client.URL) _, err := another.ServeProvisionerDaemon(ctx, codersdk.ServeProvisionerDaemonRequest{ + ID: uuid.New(), + Name: t.Name(), Organization: user.OrganizationID, Provisioners: []codersdk.ProvisionerType{ codersdk.ProvisionerTypeEcho, @@ -377,6 +391,8 @@ func TestProvisionerDaemonServe(t *testing.T) { defer cancel() another := codersdk.New(client.URL) _, err := another.ServeProvisionerDaemon(ctx, codersdk.ServeProvisionerDaemonRequest{ + ID: uuid.New(), + Name: t.Name(), Organization: user.OrganizationID, Provisioners: []codersdk.ProvisionerType{ codersdk.ProvisionerTypeEcho, diff --git a/enterprise/replicasync/replicasync.go b/enterprise/replicasync/replicasync.go index 0414de3dc4ce5..b7e46b99f6124 100644 --- a/enterprise/replicasync/replicasync.go +++ b/enterprise/replicasync/replicasync.go @@ -8,7 +8,6 @@ import ( "fmt" "net/http" "net/url" - "os" "strings" "sync" "time" @@ -19,6 +18,7 @@ import ( "cdr.dev/slog" "github.com/coder/coder/v2/buildinfo" + "github.com/coder/coder/v2/cli/cliutil" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/dbauthz" "github.com/coder/coder/v2/coderd/database/dbtime" @@ -57,10 +57,7 @@ func New(ctx context.Context, logger slog.Logger, db database.Store, ps pubsub.P // primary purpose is to clean up dead replicas. options.CleanupInterval = 30 * time.Minute } - hostname, err := os.Hostname() - if err != nil { - return nil, xerrors.Errorf("get hostname: %w", err) - } + hostname := cliutil.Hostname() databaseLatency, err := db.Ping(ctx) if err != nil { return nil, xerrors.Errorf("ping database: %w", err) diff --git a/enterprise/wsproxy/wsproxy.go b/enterprise/wsproxy/wsproxy.go index b0194d69d3f26..d83f760c8fc7c 100644 --- a/enterprise/wsproxy/wsproxy.go +++ b/enterprise/wsproxy/wsproxy.go @@ -7,7 +7,6 @@ import ( "fmt" "net/http" "net/url" - "os" "reflect" "regexp" "strings" @@ -27,6 +26,7 @@ import ( "cdr.dev/slog" "github.com/coder/coder/v2/buildinfo" + "github.com/coder/coder/v2/cli/cliutil" "github.com/coder/coder/v2/coderd" "github.com/coder/coder/v2/coderd/httpapi" "github.com/coder/coder/v2/coderd/httpmw" @@ -206,10 +206,7 @@ func New(ctx context.Context, opts *Options) (*Server, error) { // Register the workspace proxy with the primary coderd instance and start a // goroutine to periodically re-register. replicaID := uuid.New() - osHostname, err := os.Hostname() - if err != nil { - return nil, xerrors.Errorf("get OS hostname: %w", err) - } + osHostname := cliutil.Hostname() regResp, registerDone, err := client.RegisterWorkspaceProxyLoop(ctx, wsproxysdk.RegisterWorkspaceProxyLoopOpts{ Logger: opts.Logger, Request: wsproxysdk.RegisterWorkspaceProxyRequest{ diff --git a/scripts/coder-dev.sh b/scripts/coder-dev.sh index 69696351ea05a..d379e226b6f3b 100755 --- a/scripts/coder-dev.sh +++ b/scripts/coder-dev.sh @@ -20,6 +20,9 @@ fi if [[ ${1:-} == exp ]] && [[ ${2:-} == scaletest ]]; then BINARY_TYPE=coder fi +if [[ ${1:-} == provisionerd ]]; then + BINARY_TYPE=coder +fi RELATIVE_BINARY_PATH="build/${BINARY_TYPE}_${GOOS}_${GOARCH}" # To preserve the CWD when running the binary, we need to use pushd and popd to From 9415b6648bc16f2ca5cd12a517764f89bc64e249 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Thu, 7 Dec 2023 12:10:35 +0000 Subject: [PATCH 2/8] fixup! feat(cli): allow specifying name of provisioner daemon --- docs/cli/provisionerd_start.md | 1 - enterprise/cli/provisionerdaemons.go | 5 ++++- .../cli/testdata/coder_provisionerd_start_--help.golden | 2 +- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/docs/cli/provisionerd_start.md b/docs/cli/provisionerd_start.md index 64e8e0792cb28..e28a8adafeb05 100644 --- a/docs/cli/provisionerd_start.md +++ b/docs/cli/provisionerd_start.md @@ -28,7 +28,6 @@ Directory to store cached data. | ----------- | ------------------------------------------- | | Type | string | | Environment | $CODER_PROVISIONER_DAEMON_NAME | -| Default | ser6 | Name of this provisioner daemon. Defaults to the current hostname without FQDN. diff --git a/enterprise/cli/provisionerdaemons.go b/enterprise/cli/provisionerdaemons.go index 1bfadde542875..6a1efe0eda5dd 100644 --- a/enterprise/cli/provisionerdaemons.go +++ b/enterprise/cli/provisionerdaemons.go @@ -73,6 +73,9 @@ func (r *RootCmd) provisionerDaemonStart() *clibase.Cmd { if len(name) > 64 { return xerrors.Errorf("name cannot be greater than 64 characters in length") } + if name == "" { + name = cliutil.Hostname() + } logger := slog.Make(sloghuman.Sink(inv.Stderr)) if ok, _ := inv.ParsedFlags().GetBool("verbose"); ok { @@ -217,7 +220,7 @@ func (r *RootCmd) provisionerDaemonStart() *clibase.Cmd { Env: "CODER_PROVISIONER_DAEMON_NAME", Description: "Name of this provisioner daemon. Defaults to the current hostname without FQDN.", Value: clibase.StringOf(&name), - Default: cliutil.Hostname(), + Default: "", }, } diff --git a/enterprise/cli/testdata/coder_provisionerd_start_--help.golden b/enterprise/cli/testdata/coder_provisionerd_start_--help.golden index 59f01f7f6f3d2..b497c5ab6231c 100644 --- a/enterprise/cli/testdata/coder_provisionerd_start_--help.golden +++ b/enterprise/cli/testdata/coder_provisionerd_start_--help.golden @@ -9,7 +9,7 @@ OPTIONS: -c, --cache-dir string, $CODER_CACHE_DIRECTORY (default: [cache dir]) Directory to store cached data. - --name string, $CODER_PROVISIONER_DAEMON_NAME (default: ser6) + --name string, $CODER_PROVISIONER_DAEMON_NAME Name of this provisioner daemon. Defaults to the current hostname without FQDN. From 95970f98507db87859fcdce0e62b8140b1043c05 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Thu, 7 Dec 2023 13:35:00 +0000 Subject: [PATCH 3/8] address PR comments --- codersdk/provisionerdaemons.go | 2 +- enterprise/cli/provisionerdaemons.go | 18 ++++++++++++--- enterprise/cli/provisionerdaemons_test.go | 27 +---------------------- enterprise/coderd/provisionerdaemons.go | 1 - 4 files changed, 17 insertions(+), 31 deletions(-) diff --git a/codersdk/provisionerdaemons.go b/codersdk/provisionerdaemons.go index e277a7aad0e07..50440d24aca8e 100644 --- a/codersdk/provisionerdaemons.go +++ b/codersdk/provisionerdaemons.go @@ -178,7 +178,7 @@ type ServeProvisionerDaemonRequest struct { // ID is a unique ID for a provisioner daemon. ID uuid.UUID `json:"id" format:"uuid"` // Name is the human-readable unique identifier for the daemon. - Name string `json:"name"` + Name string `json:"name" example:"my-cool-provisioner-daemon"` // Organization is the organization for the URL. At present provisioner daemons ARE NOT scoped to organizations // and so the organization ID is optional. Organization uuid.UUID `json:"organization" format:"uuid"` diff --git a/enterprise/cli/provisionerdaemons.go b/enterprise/cli/provisionerdaemons.go index 6a1efe0eda5dd..c11dfeecca238 100644 --- a/enterprise/cli/provisionerdaemons.go +++ b/enterprise/cli/provisionerdaemons.go @@ -6,6 +6,7 @@ import ( "context" "fmt" "os" + "regexp" "time" "github.com/google/uuid" @@ -42,6 +43,16 @@ func (r *RootCmd) provisionerDaemons() *clibase.Cmd { return cmd } +func validateProvisionerDaemonName(name string) error { + if len(name) > 64 { + return xerrors.Errorf("name cannot be greater than 64 characters in length") + } + if ok, err := regexp.MatchString(`^[a-zA-Z0-9][a-zA-Z0-9-]{0,61}[a-zA-Z0-9]$`, name); err != nil || !ok { + return xerrors.Errorf("name %q is not a valid hostname", name) + } + return nil +} + func (r *RootCmd) provisionerDaemonStart() *clibase.Cmd { var ( cacheDir string @@ -70,13 +81,14 @@ func (r *RootCmd) provisionerDaemonStart() *clibase.Cmd { return err } - if len(name) > 64 { - return xerrors.Errorf("name cannot be greater than 64 characters in length") - } if name == "" { name = cliutil.Hostname() } + if err := validateProvisionerDaemonName(name); err != nil { + return err + } + logger := slog.Make(sloghuman.Sink(inv.Stderr)) if ok, _ := inv.ParsedFlags().GetBool("verbose"); ok { logger = logger.Leveled(slog.LevelDebug) diff --git a/enterprise/cli/provisionerdaemons_test.go b/enterprise/cli/provisionerdaemons_test.go index 6b17f0c564da6..4f1e09ad43b5a 100644 --- a/enterprise/cli/provisionerdaemons_test.go +++ b/enterprise/cli/provisionerdaemons_test.go @@ -26,32 +26,7 @@ func TestProvisionerDaemon_PSK(t *testing.T) { }, }, }) - inv, conf := newCLI(t, "provisionerd", "start", "--psk=provisionersftw") - err := conf.URL().Write(client.URL.String()) - require.NoError(t, err) - pty := ptytest.New(t).Attach(inv) - ctx, cancel := context.WithTimeout(inv.Context(), testutil.WaitLong) - defer cancel() - clitest.Start(t, inv) - pty.ExpectMatchContext(ctx, "starting provisioner daemon") -} - -func TestProvisionerDaemon_Named(t *testing.T) { - t.Parallel() - - client, _ := coderdenttest.New(t, &coderdenttest.Options{ - ProvisionerDaemonPSK: "provisionersftw", - LicenseOptions: &coderdenttest.LicenseOptions{ - Features: license.Features{ - codersdk.FeatureExternalProvisionerDaemons: 1, - }, - }, - }) - - inv, conf := newCLI(t, "provisionerd", "start", - "--psk=provisionersftw", - "--name=matt-daemon", - ) + inv, conf := newCLI(t, "provisionerd", "start", "--psk=provisionersftw", "--name=matt-daemon") err := conf.URL().Write(client.URL.String()) require.NoError(t, err) pty := ptytest.New(t).Attach(inv) diff --git a/enterprise/coderd/provisionerdaemons.go b/enterprise/coderd/provisionerdaemons.go index 8778758c37f1c..d07a553acca1a 100644 --- a/enterprise/coderd/provisionerdaemons.go +++ b/enterprise/coderd/provisionerdaemons.go @@ -214,7 +214,6 @@ func (api *API) provisionerDaemonServe(rw http.ResponseWriter, r *http.Request) } } - // name := namesgenerator.GetRandomName(1) log := api.Logger.With( slog.F("name", name), slog.F("provisioners", provisioners), From 80e49ffc3c612878494fefc70af4b7b4b920607d Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Thu, 7 Dec 2023 14:06:41 +0000 Subject: [PATCH 4/8] strip domain suffix of host --- cli/cliutil/hostname.go | 31 +++++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/cli/cliutil/hostname.go b/cli/cliutil/hostname.go index 0695be4757964..88452fc812f94 100644 --- a/cli/cliutil/hostname.go +++ b/cli/cliutil/hostname.go @@ -2,6 +2,7 @@ package cliutil import ( "os" + "strings" "sync" "golang.org/x/xerrors" @@ -12,14 +13,28 @@ var ( hostnameOnce sync.Once ) +// Hostname returns the hostname of the machine, lowercased, +// with any trailing domain suffix stripped. +// It is cached after the first call. +// If the hostname cannot be determined, this will panic. func Hostname() string { - hostnameOnce.Do(func() { - h, err := os.Hostname() - if err != nil { - // Something must be very wrong if this fails. - panic(xerrors.Errorf("lookup hostname: %w", err)) - } - hostname = h - }) + hostnameOnce.Do(func() { hostname = getHostname() }) return hostname } + +func getHostname() string { + h, err := os.Hostname() + if err != nil { + // Something must be very wrong if this fails. + panic(xerrors.Errorf("lookup hostname: %w", err)) + } + + // On some platforms, the hostname can be an FQDN. We only want the hostname. + if idx := strings.Index(h, "."); idx != -1 { + h = h[:idx] + } + + // For the sake of consistency, we also want to lowercase the hostname. + // Per RFC 4343, DNS lookups must be case-insensitive. + return strings.ToLower(h) +} From 6fdcec20b22644264d5dcacf5521cc67d4266abf Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Thu, 7 Dec 2023 14:17:46 +0000 Subject: [PATCH 5/8] truncate inmem provisionerd name with a really long hostname --- cli/server.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/cli/server.go b/cli/server.go index dd4302e615aea..95bea2420c8fa 100644 --- a/cli/server.go +++ b/cli/server.go @@ -876,7 +876,14 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd. defer provisionerdWaitGroup.Wait() provisionerdMetrics := provisionerd.NewMetrics(options.PrometheusRegistry) for i := int64(0); i < vals.Provisioner.Daemons.Value(); i++ { - name := fmt.Sprintf("%s-%d", cliutil.Hostname(), i) + suffix := fmt.Sprintf("%d", i) + hostname := cliutil.Hostname() + // The suffix is added to the hostname, so we may need to trim to fit into + // the 64 character limit. + if len(hostname+suffix) > 62 { + hostname = hostname[:62-len(suffix)] + } + name := fmt.Sprintf("%s-%s", hostname, suffix) daemonCacheDir := filepath.Join(cacheDir, fmt.Sprintf("provisioner-%d", i)) daemon, err := newProvisionerDaemon( ctx, coderAPI, provisionerdMetrics, logger, vals, daemonCacheDir, errCh, &provisionerdWaitGroup, name, From 80f71eb6860a41c338f8f16bef612bd9dbb631d8 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Thu, 7 Dec 2023 14:34:48 +0000 Subject: [PATCH 6/8] extract stringutil.Truncate --- cli/server.go | 6 ++---- coderd/util/strings/strings.go | 11 +++++++++++ coderd/util/strings/strings_test.go | 24 ++++++++++++++++++++++++ 3 files changed, 37 insertions(+), 4 deletions(-) diff --git a/cli/server.go b/cli/server.go index 95bea2420c8fa..7161795fd4687 100644 --- a/cli/server.go +++ b/cli/server.go @@ -87,6 +87,7 @@ import ( "github.com/coder/coder/v2/coderd/unhanger" "github.com/coder/coder/v2/coderd/updatecheck" "github.com/coder/coder/v2/coderd/util/slice" + stringutil "github.com/coder/coder/v2/coderd/util/strings" "github.com/coder/coder/v2/coderd/workspaceapps" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/cryptorand" @@ -877,12 +878,9 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd. provisionerdMetrics := provisionerd.NewMetrics(options.PrometheusRegistry) for i := int64(0); i < vals.Provisioner.Daemons.Value(); i++ { suffix := fmt.Sprintf("%d", i) - hostname := cliutil.Hostname() // The suffix is added to the hostname, so we may need to trim to fit into // the 64 character limit. - if len(hostname+suffix) > 62 { - hostname = hostname[:62-len(suffix)] - } + hostname := stringutil.Truncate(cliutil.Hostname(), 63-len(suffix)) name := fmt.Sprintf("%s-%s", hostname, suffix) daemonCacheDir := filepath.Join(cacheDir, fmt.Sprintf("provisioner-%d", i)) daemon, err := newProvisionerDaemon( diff --git a/coderd/util/strings/strings.go b/coderd/util/strings/strings.go index fda9f0e7c6ea6..f416bba463bbf 100644 --- a/coderd/util/strings/strings.go +++ b/coderd/util/strings/strings.go @@ -17,3 +17,14 @@ func JoinWithConjunction(s []string) string { s[last], ) } + +// Truncate returns the first n characters of s. +func Truncate(s string, n int) string { + if n < 1 { + return "" + } + if len(s) <= n { + return s + } + return s[:n] +} diff --git a/coderd/util/strings/strings_test.go b/coderd/util/strings/strings_test.go index a107a7754fc7f..2db9c9e236e43 100644 --- a/coderd/util/strings/strings_test.go +++ b/coderd/util/strings/strings_test.go @@ -14,3 +14,27 @@ func TestJoinWithConjunction(t *testing.T) { require.Equal(t, "foo and bar", strings.JoinWithConjunction([]string{"foo", "bar"})) require.Equal(t, "foo, bar and baz", strings.JoinWithConjunction([]string{"foo", "bar", "baz"})) } + +func TestTruncate(t *testing.T) { + t.Parallel() + + for _, tt := range []struct { + s string + n int + expected string + }{ + {"foo", 4, "foo"}, + {"foo", 3, "foo"}, + {"foo", 2, "fo"}, + {"foo", 1, "f"}, + {"foo", 0, ""}, + {"foo", -1, ""}, + } { + tt := tt + t.Run(tt.expected, func(t *testing.T) { + t.Parallel() + actual := strings.Truncate(tt.s, tt.n) + require.Equal(t, tt.expected, actual) + }) + } +} From b9e7141e6aa737f08e2ad8702310b5d904bb36ec Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Thu, 7 Dec 2023 15:18:34 +0000 Subject: [PATCH 7/8] return localhost if os.Hostname() fails --- cli/cliutil/hostname.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/cli/cliutil/hostname.go b/cli/cliutil/hostname.go index 88452fc812f94..92badcf5e30c6 100644 --- a/cli/cliutil/hostname.go +++ b/cli/cliutil/hostname.go @@ -4,8 +4,6 @@ import ( "os" "strings" "sync" - - "golang.org/x/xerrors" ) var ( @@ -16,7 +14,8 @@ var ( // Hostname returns the hostname of the machine, lowercased, // with any trailing domain suffix stripped. // It is cached after the first call. -// If the hostname cannot be determined, this will panic. +// If the hostname cannot be determined, for any reason, +// localhost will be returned instead. func Hostname() string { hostnameOnce.Do(func() { hostname = getHostname() }) return hostname @@ -26,7 +25,8 @@ func getHostname() string { h, err := os.Hostname() if err != nil { // Something must be very wrong if this fails. - panic(xerrors.Errorf("lookup hostname: %w", err)) + // We'll just return localhost and hope for the best. + return "localhost" } // On some platforms, the hostname can be an FQDN. We only want the hostname. From 30e8e5923647a20ba4f741cfcb518e8e5edb49dd Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Thu, 7 Dec 2023 15:50:31 +0000 Subject: [PATCH 8/8] invert logic --- enterprise/coderd/provisionerdaemons.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/enterprise/coderd/provisionerdaemons.go b/enterprise/coderd/provisionerdaemons.go index d07a553acca1a..38b65b442a452 100644 --- a/enterprise/coderd/provisionerdaemons.go +++ b/enterprise/coderd/provisionerdaemons.go @@ -178,12 +178,11 @@ func (api *API) provisionerDaemonServe(rw http.ResponseWriter, r *http.Request) } } - var name string - if vals, ok := r.URL.Query()["name"]; !ok || len(vals) == 0 { - api.Logger.Warn(ctx, "unnamed provisioner daemon") - name = namesgenerator.GetRandomName(1) - } else { + name := namesgenerator.GetRandomName(10) + if vals, ok := r.URL.Query()["name"]; ok && len(vals) > 0 { name = vals[0] + } else { + api.Logger.Warn(ctx, "unnamed provisioner daemon") } tags, authorized := api.provisionerDaemonAuth.authorize(r, tags)