From 3a938ed7265fc41af21c9d948aa03f3569aea2bb Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Tue, 4 Jun 2024 11:08:22 +0200 Subject: [PATCH 1/7] WIP --- cli/templatepush.go | 9 +++ cli/templatepush_test.go | 105 ++++++++++++++++++++++++++++++++ coderd/coderd.go | 6 +- coderd/coderdtest/coderdtest.go | 9 ++- 4 files changed, 126 insertions(+), 3 deletions(-) diff --git a/cli/templatepush.go b/cli/templatepush.go index e4d776dbaa201..bfcdd7afee0c8 100644 --- a/cli/templatepush.go +++ b/cli/templatepush.go @@ -100,6 +100,15 @@ func (r *RootCmd) templatePush() *serpent.Command { return err } + // If user hasn't provided new provisioner tags, inherit ones from the active template version. + if len(tags) == 0 && template.ActiveVersionID != uuid.Nil { + templateVersion, err := client.TemplateVersion(inv.Context(), template.ActiveVersionID) + if err != nil { + return err + } + tags = templateVersion.Job.Tags + } + userVariableValues, err := ParseUserVariableValues( varsFiles, variablesFile, diff --git a/cli/templatepush_test.go b/cli/templatepush_test.go index 13c9fbc1f35c4..d1d5f98626f8f 100644 --- a/cli/templatepush_test.go +++ b/cli/templatepush_test.go @@ -403,6 +403,111 @@ func TestTemplatePush(t *testing.T) { assert.NotEqual(t, template.ActiveVersionID, templateVersions[1].ID) }) + t.Run("ProvisionerTags", func(t *testing.T) { + t.Parallel() + + t.Run("ChangeTags_SameKeyValues", func(t *testing.T) { + t.Parallel() + + // Start the tagged provisioner + client := coderdtest.New(t, &coderdtest.Options{ + IncludeProvisionerDaemon: true, + ProvisionerDaemonTags: map[string]string{ + "docker": "true", + }, + }) + owner := coderdtest.CreateFirstUser(t, client) + templateAdmin, _ := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID, rbac.RoleTemplateAdmin()) + + // Create the template with initial tagged template version. + templateVersion := coderdtest.CreateTemplateVersion(t, client, owner.OrganizationID, nil, func(ctvr *codersdk.CreateTemplateVersionRequest) { + ctvr.ProvisionerTags = map[string]string{ + "docker": "true", + } + }) + templateVersion = coderdtest.AwaitTemplateVersionJobCompleted(t, client, templateVersion.ID) + template := coderdtest.CreateTemplate(t, client, owner.OrganizationID, templateVersion.ID) + + // Push new template version without provisioner tags. CLI should reuse tags from the previous version. + source := clitest.CreateTemplateVersionSource(t, &echo.Responses{ + Parse: echo.ParseComplete, + ProvisionApply: echo.ApplyComplete, + }) + inv, root := clitest.New(t, "templates", "push", template.Name, "--directory", source, "--test.provisioner", string(database.ProvisionerTypeEcho), "--name", template.Name, + "--provisioner-tag", "docker=true") + clitest.SetupConfig(t, templateAdmin, root) + pty := ptytest.New(t).Attach(inv) + + execDone := make(chan error) + go func() { + execDone <- inv.Run() + }() + + matches := []struct { + match string + write string + }{ + {match: "Upload", write: "yes"}, + } + for _, m := range matches { + pty.ExpectMatch(m.match) + pty.WriteLine(m.write) + } + + require.NoError(t, <-execDone) + }) + + t.Run("DoNotChangeTags", func(t *testing.T) { + t.Parallel() + + // Start the tagged provisioner + client := coderdtest.New(t, &coderdtest.Options{ + IncludeProvisionerDaemon: true, + ProvisionerDaemonTags: map[string]string{ + "docker": "true", + }, + }) + owner := coderdtest.CreateFirstUser(t, client) + templateAdmin, _ := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID, rbac.RoleTemplateAdmin()) + + // Create the template with initial tagged template version. + templateVersion := coderdtest.CreateTemplateVersion(t, client, owner.OrganizationID, nil, func(ctvr *codersdk.CreateTemplateVersionRequest) { + ctvr.ProvisionerTags = map[string]string{ + "docker": "true", + } + }) + templateVersion = coderdtest.AwaitTemplateVersionJobCompleted(t, client, templateVersion.ID) + template := coderdtest.CreateTemplate(t, client, owner.OrganizationID, templateVersion.ID) + + // Push new template version without provisioner tags. CLI should reuse tags from the previous version. + source := clitest.CreateTemplateVersionSource(t, &echo.Responses{ + Parse: echo.ParseComplete, + ProvisionApply: echo.ApplyComplete, + }) + inv, root := clitest.New(t, "templates", "push", template.Name, "--directory", source, "--test.provisioner", string(database.ProvisionerTypeEcho), "--name", template.Name) + clitest.SetupConfig(t, templateAdmin, root) + pty := ptytest.New(t).Attach(inv) + + execDone := make(chan error) + go func() { + execDone <- inv.Run() + }() + + matches := []struct { + match string + write string + }{ + {match: "Upload", write: "yes"}, + } + for _, m := range matches { + pty.ExpectMatch(m.match) + pty.WriteLine(m.write) + } + + require.NoError(t, <-execDone) + }) + }) + t.Run("Variables", func(t *testing.T) { t.Parallel() diff --git a/coderd/coderd.go b/coderd/coderd.go index 98b1686171c07..60bfe9813c559 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -1373,6 +1373,10 @@ 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(dialCtx context.Context, name string, provisionerTypes []codersdk.ProvisionerType) (client proto.DRPCProvisionerDaemonClient, err error) { + return api.CreateInMemoryTaggedProvisionerDaemon(dialCtx, name, provisionerTypes, nil) +} + +func (api *API) CreateInMemoryTaggedProvisionerDaemon(dialCtx context.Context, name string, provisionerTypes []codersdk.ProvisionerType, provisionerTags map[string]string) (client proto.DRPCProvisionerDaemonClient, err error) { tracer := api.TracerProvider.Tracer(tracing.TracerName) clientSession, serverSession := drpc.MemTransportPipe() defer func() { @@ -1400,7 +1404,7 @@ func (api *API) CreateInMemoryProvisionerDaemon(dialCtx context.Context, name st OrganizationID: defaultOrg.ID, CreatedAt: dbtime.Now(), Provisioners: dbTypes, - Tags: provisionersdk.MutateTags(uuid.Nil, nil), + Tags: provisionersdk.MutateTags(uuid.Nil, provisionerTags), LastSeenAt: sql.NullTime{Time: dbtime.Now(), Valid: true}, Version: buildinfo.Version(), APIVersion: proto.CurrentVersion.String(), diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index b379cedd1834a..27866cb620ede 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -125,6 +125,7 @@ type Options struct { // IncludeProvisionerDaemon when true means to start an in-memory provisionerD IncludeProvisionerDaemon bool + ProvisionerDaemonTags map[string]string MetricsCacheRefreshInterval time.Duration AgentStatsRefreshInterval time.Duration DeploymentValues *codersdk.DeploymentValues @@ -512,7 +513,7 @@ func NewWithAPI(t testing.TB, options *Options) (*codersdk.Client, io.Closer, *c setHandler(coderAPI.RootHandler) var provisionerCloser io.Closer = nopcloser{} if options.IncludeProvisionerDaemon { - provisionerCloser = NewProvisionerDaemon(t, coderAPI) + provisionerCloser = NewTaggedProvisionerDaemon(t, coderAPI, options.ProvisionerDaemonTags) } client := codersdk.New(serverURL) t.Cleanup(func() { @@ -552,6 +553,10 @@ func (c *provisionerdCloser) Close() error { // well with coderd testing. It registers the "echo" provisioner for // quick testing. func NewProvisionerDaemon(t testing.TB, coderAPI *coderd.API) io.Closer { + return NewTaggedProvisionerDaemon(t, coderAPI, nil) +} + +func NewTaggedProvisionerDaemon(t testing.TB, coderAPI *coderd.API, provisionerTags map[string]string) io.Closer { t.Helper() // t.Cleanup runs in last added, first called order. t.TempDir() will delete @@ -578,7 +583,7 @@ func NewProvisionerDaemon(t testing.TB, coderAPI *coderd.API) io.Closer { }() daemon := provisionerd.New(func(dialCtx context.Context) (provisionerdproto.DRPCProvisionerDaemonClient, error) { - return coderAPI.CreateInMemoryProvisionerDaemon(dialCtx, "test", []codersdk.ProvisionerType{codersdk.ProvisionerTypeEcho}) + return coderAPI.CreateInMemoryTaggedProvisionerDaemon(dialCtx, "test", []codersdk.ProvisionerType{codersdk.ProvisionerTypeEcho}, provisionerTags) }, &provisionerd.Options{ Logger: coderAPI.Logger.Named("provisionerd").Leveled(slog.LevelDebug), UpdateInterval: 250 * time.Millisecond, From 52e34931e78dd48f88e81071c9caab454e4d36ec Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Tue, 4 Jun 2024 11:16:29 +0200 Subject: [PATCH 2/7] ChangeTags --- cli/templatepush_test.go | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/cli/templatepush_test.go b/cli/templatepush_test.go index d1d5f98626f8f..234206dcf7de9 100644 --- a/cli/templatepush_test.go +++ b/cli/templatepush_test.go @@ -406,16 +406,24 @@ func TestTemplatePush(t *testing.T) { t.Run("ProvisionerTags", func(t *testing.T) { t.Parallel() - t.Run("ChangeTags_SameKeyValues", func(t *testing.T) { + t.Run("ChangeTags", func(t *testing.T) { t.Parallel() - // Start the tagged provisioner - client := coderdtest.New(t, &coderdtest.Options{ + // Start the first provisioner + client, provisionerDocker, api := coderdtest.NewWithAPI(t, &coderdtest.Options{ IncludeProvisionerDaemon: true, ProvisionerDaemonTags: map[string]string{ "docker": "true", }, }) + defer provisionerDocker.Close() + + // Start the second provisioner + provisionerFoobar := coderdtest.NewTaggedProvisionerDaemon(t, api, map[string]string{ + "foobar": "foobaz", + }) + defer provisionerFoobar.Close() + owner := coderdtest.CreateFirstUser(t, client) templateAdmin, _ := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID, rbac.RoleTemplateAdmin()) @@ -434,7 +442,7 @@ func TestTemplatePush(t *testing.T) { ProvisionApply: echo.ApplyComplete, }) inv, root := clitest.New(t, "templates", "push", template.Name, "--directory", source, "--test.provisioner", string(database.ProvisionerTypeEcho), "--name", template.Name, - "--provisioner-tag", "docker=true") + "--provisioner-tag", "foobar=foobaz") clitest.SetupConfig(t, templateAdmin, root) pty := ptytest.New(t).Attach(inv) From 9dcf081d6cc53dd1b01f959da4f7c362f56875f7 Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Tue, 4 Jun 2024 11:31:21 +0200 Subject: [PATCH 3/7] Verify --- cli/templatepush_test.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/cli/templatepush_test.go b/cli/templatepush_test.go index 234206dcf7de9..511f1609530e5 100644 --- a/cli/templatepush_test.go +++ b/cli/templatepush_test.go @@ -463,6 +463,14 @@ func TestTemplatePush(t *testing.T) { } require.NoError(t, <-execDone) + + // Verify template version tags + template, err := client.Template(context.TODO(), template.ID) + require.NoError(t, err) + + templateVersion, err = client.TemplateVersion(context.TODO(), template.ActiveVersionID) + require.NoError(t, err) + require.EqualValues(t, map[string]string{"foobar": "foobaz", "owner": "", "scope": "organization"}, templateVersion.Job.Tags) }) t.Run("DoNotChangeTags", func(t *testing.T) { @@ -513,6 +521,14 @@ func TestTemplatePush(t *testing.T) { } require.NoError(t, <-execDone) + + // Verify template version tags + template, err := client.Template(context.TODO(), template.ID) + require.NoError(t, err) + + templateVersion, err = client.TemplateVersion(context.TODO(), template.ActiveVersionID) + require.NoError(t, err) + require.EqualValues(t, map[string]string{"docker": "true", "owner": "", "scope": "organization"}, templateVersion.Job.Tags) }) }) From 6d40012f698ffdf14b229c827883b394d04037d8 Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Tue, 4 Jun 2024 12:32:22 +0200 Subject: [PATCH 4/7] Fix --- cli/templatepush_test.go | 2 +- coderd/coderd.go | 3 ++- coderd/coderdtest/coderdtest.go | 8 ++++---- coderd/workspacebuilds_test.go | 2 +- 4 files changed, 8 insertions(+), 7 deletions(-) diff --git a/cli/templatepush_test.go b/cli/templatepush_test.go index 511f1609530e5..92d232725ce6d 100644 --- a/cli/templatepush_test.go +++ b/cli/templatepush_test.go @@ -419,7 +419,7 @@ func TestTemplatePush(t *testing.T) { defer provisionerDocker.Close() // Start the second provisioner - provisionerFoobar := coderdtest.NewTaggedProvisionerDaemon(t, api, map[string]string{ + provisionerFoobar := coderdtest.NewTaggedProvisionerDaemon(t, api, "provisioner-foobar", map[string]string{ "foobar": "foobaz", }) defer provisionerFoobar.Close() diff --git a/coderd/coderd.go b/coderd/coderd.go index 60bfe9813c559..ba92bf8d02d9a 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -1399,12 +1399,13 @@ func (api *API) CreateInMemoryTaggedProvisionerDaemon(dialCtx context.Context, n } //nolint:gocritic // in-memory provisioners are owned by system + tags := provisionersdk.MutateTags(uuid.Nil, provisionerTags) daemon, err := api.Database.UpsertProvisionerDaemon(dbauthz.AsSystemRestricted(dialCtx), database.UpsertProvisionerDaemonParams{ Name: name, OrganizationID: defaultOrg.ID, CreatedAt: dbtime.Now(), Provisioners: dbTypes, - Tags: provisionersdk.MutateTags(uuid.Nil, provisionerTags), + Tags: tags, LastSeenAt: sql.NullTime{Time: dbtime.Now(), Valid: true}, Version: buildinfo.Version(), APIVersion: proto.CurrentVersion.String(), diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index 27866cb620ede..7110cc79471fb 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -513,7 +513,7 @@ func NewWithAPI(t testing.TB, options *Options) (*codersdk.Client, io.Closer, *c setHandler(coderAPI.RootHandler) var provisionerCloser io.Closer = nopcloser{} if options.IncludeProvisionerDaemon { - provisionerCloser = NewTaggedProvisionerDaemon(t, coderAPI, options.ProvisionerDaemonTags) + provisionerCloser = NewTaggedProvisionerDaemon(t, coderAPI, "test", options.ProvisionerDaemonTags) } client := codersdk.New(serverURL) t.Cleanup(func() { @@ -553,10 +553,10 @@ func (c *provisionerdCloser) Close() error { // well with coderd testing. It registers the "echo" provisioner for // quick testing. func NewProvisionerDaemon(t testing.TB, coderAPI *coderd.API) io.Closer { - return NewTaggedProvisionerDaemon(t, coderAPI, nil) + return NewTaggedProvisionerDaemon(t, coderAPI, "test", nil) } -func NewTaggedProvisionerDaemon(t testing.TB, coderAPI *coderd.API, provisionerTags map[string]string) io.Closer { +func NewTaggedProvisionerDaemon(t testing.TB, coderAPI *coderd.API, name string, provisionerTags map[string]string) io.Closer { t.Helper() // t.Cleanup runs in last added, first called order. t.TempDir() will delete @@ -583,7 +583,7 @@ func NewTaggedProvisionerDaemon(t testing.TB, coderAPI *coderd.API, provisionerT }() daemon := provisionerd.New(func(dialCtx context.Context) (provisionerdproto.DRPCProvisionerDaemonClient, error) { - return coderAPI.CreateInMemoryTaggedProvisionerDaemon(dialCtx, "test", []codersdk.ProvisionerType{codersdk.ProvisionerTypeEcho}, provisionerTags) + return coderAPI.CreateInMemoryTaggedProvisionerDaemon(dialCtx, name, []codersdk.ProvisionerType{codersdk.ProvisionerTypeEcho}, provisionerTags) }, &provisionerd.Options{ Logger: coderAPI.Logger.Named("provisionerd").Leveled(slog.LevelDebug), UpdateInterval: 250 * time.Millisecond, diff --git a/coderd/workspacebuilds_test.go b/coderd/workspacebuilds_test.go index eb76239b84658..5d99e56820aa1 100644 --- a/coderd/workspacebuilds_test.go +++ b/coderd/workspacebuilds_test.go @@ -728,7 +728,7 @@ func TestWorkspaceDeleteSuspendedUser(t *testing.T) { validateCalls++ if userSuspended { // Simulate the user being suspended from the IDP too. - return "", http.StatusForbidden, fmt.Errorf("user is suspended") + return "", http.StatusForbidden, xerrors.New("user is suspended") } return "OK", 0, nil }, From 4fd057e0e5f1a755cf867c956c057dca52f1e15d Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Tue, 4 Jun 2024 12:41:12 +0200 Subject: [PATCH 5/7] fix lint --- coderd/coderd.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index ba92bf8d02d9a..60bfe9813c559 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -1399,13 +1399,12 @@ func (api *API) CreateInMemoryTaggedProvisionerDaemon(dialCtx context.Context, n } //nolint:gocritic // in-memory provisioners are owned by system - tags := provisionersdk.MutateTags(uuid.Nil, provisionerTags) daemon, err := api.Database.UpsertProvisionerDaemon(dbauthz.AsSystemRestricted(dialCtx), database.UpsertProvisionerDaemonParams{ Name: name, OrganizationID: defaultOrg.ID, CreatedAt: dbtime.Now(), Provisioners: dbTypes, - Tags: tags, + Tags: provisionersdk.MutateTags(uuid.Nil, provisionerTags), LastSeenAt: sql.NullTime{Time: dbtime.Now(), Valid: true}, Version: buildinfo.Version(), APIVersion: proto.CurrentVersion.String(), From ff827351e613c395a9ef88af06ce4de95d83b59e Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Tue, 4 Jun 2024 13:32:43 +0200 Subject: [PATCH 6/7] Address PR comments --- cli/templatepush.go | 1 + cli/templatepush_test.go | 8 ++++---- coderd/workspacebuilds_test.go | 2 +- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/cli/templatepush.go b/cli/templatepush.go index bfcdd7afee0c8..88b52d581002f 100644 --- a/cli/templatepush.go +++ b/cli/templatepush.go @@ -107,6 +107,7 @@ func (r *RootCmd) templatePush() *serpent.Command { return err } tags = templateVersion.Job.Tags + inv.Logger.Info(inv.Context(), "Reusing existing provisioner tags", "tags", tags) } userVariableValues, err := ParseUserVariableValues( diff --git a/cli/templatepush_test.go b/cli/templatepush_test.go index 92d232725ce6d..4e9c8613961e5 100644 --- a/cli/templatepush_test.go +++ b/cli/templatepush_test.go @@ -465,10 +465,10 @@ func TestTemplatePush(t *testing.T) { require.NoError(t, <-execDone) // Verify template version tags - template, err := client.Template(context.TODO(), template.ID) + template, err := client.Template(context.Background(), template.ID) require.NoError(t, err) - templateVersion, err = client.TemplateVersion(context.TODO(), template.ActiveVersionID) + templateVersion, err = client.TemplateVersion(context.Background(), template.ActiveVersionID) require.NoError(t, err) require.EqualValues(t, map[string]string{"foobar": "foobaz", "owner": "", "scope": "organization"}, templateVersion.Job.Tags) }) @@ -523,10 +523,10 @@ func TestTemplatePush(t *testing.T) { require.NoError(t, <-execDone) // Verify template version tags - template, err := client.Template(context.TODO(), template.ID) + template, err := client.Template(context.Background(), template.ID) require.NoError(t, err) - templateVersion, err = client.TemplateVersion(context.TODO(), template.ActiveVersionID) + templateVersion, err = client.TemplateVersion(context.Background(), template.ActiveVersionID) require.NoError(t, err) require.EqualValues(t, map[string]string{"docker": "true", "owner": "", "scope": "organization"}, templateVersion.Job.Tags) }) diff --git a/coderd/workspacebuilds_test.go b/coderd/workspacebuilds_test.go index 5d99e56820aa1..eb76239b84658 100644 --- a/coderd/workspacebuilds_test.go +++ b/coderd/workspacebuilds_test.go @@ -728,7 +728,7 @@ func TestWorkspaceDeleteSuspendedUser(t *testing.T) { validateCalls++ if userSuspended { // Simulate the user being suspended from the IDP too. - return "", http.StatusForbidden, xerrors.New("user is suspended") + return "", http.StatusForbidden, fmt.Errorf("user is suspended") } return "OK", 0, nil }, From 1cc5d0e38571e1ea099ae723aa2c6e775f94bd76 Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Tue, 4 Jun 2024 13:50:23 +0200 Subject: [PATCH 7/7] fix --- cli/templatepush.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/templatepush.go b/cli/templatepush.go index 88b52d581002f..b4ff8e50eb5ed 100644 --- a/cli/templatepush.go +++ b/cli/templatepush.go @@ -107,7 +107,7 @@ func (r *RootCmd) templatePush() *serpent.Command { return err } tags = templateVersion.Job.Tags - inv.Logger.Info(inv.Context(), "Reusing existing provisioner tags", "tags", tags) + inv.Logger.Info(inv.Context(), "reusing existing provisioner tags", "tags", tags) } userVariableValues, err := ParseUserVariableValues(