feat!: extract provisioner tags from coder_workspace_tags data source (#15578)

Relates to https://github.com/coder/coder/issues/15087 and
https://github.com/coder/coder/issues/15427

- Extracts provisioner job tags from `coder_workspace_tags` on template
version creation using `provisioner/terraform/tfparse` added in
https://github.com/coder/coder/pull/15236
- Drops a WARN log in coderd if no matching provisioners found.
- Also drops a warning message in the CLI if no provisioners are found.
- To support both CLI and UI warnings, added a
`codersdk.MatchedProvisioners` struct to the `TemplateVersion` response
containing details of how many provisioners were around at the time of
the insert.

Co-authored-by: Mathias Fredriksson <mafredri@gmail.com>
This commit is contained in:
Cian Johnston
2024-11-25 11:19:14 +00:00
committed by GitHub
co-authored by Mathias Fredriksson
parent 648cdd006c
commit 1cdc3e8921
11 changed files with 694 additions and 95 deletions
+21
View File
@@ -11389,6 +11389,24 @@ const docTemplate = `{
}
}
},
"codersdk.MatchedProvisioners": {
"type": "object",
"properties": {
"available": {
"description": "Available is the number of provisioner daemons that are available to\ntake jobs. This may be less than the count if some provisioners are\nbusy or have been stopped.",
"type": "integer"
},
"count": {
"description": "Count is the number of provisioner daemons that matched the given\ntags. If the count is 0, it means no provisioner daemons matched the\nrequested tags.",
"type": "integer"
},
"most_recently_seen": {
"description": "MostRecentlySeen is the most recently seen time of the set of matched\nprovisioners. If no provisioners matched, this field will be null.",
"type": "string",
"format": "date-time"
}
}
},
"codersdk.MinimalOrganization": {
"type": "object",
"required": [
@@ -13582,6 +13600,9 @@ const docTemplate = `{
"job": {
"$ref": "#/definitions/codersdk.ProvisionerJob"
},
"matched_provisioners": {
"$ref": "#/definitions/codersdk.MatchedProvisioners"
},
"message": {
"type": "string"
},
+21
View File
@@ -10207,6 +10207,24 @@
}
}
},
"codersdk.MatchedProvisioners": {
"type": "object",
"properties": {
"available": {
"description": "Available is the number of provisioner daemons that are available to\ntake jobs. This may be less than the count if some provisioners are\nbusy or have been stopped.",
"type": "integer"
},
"count": {
"description": "Count is the number of provisioner daemons that matched the given\ntags. If the count is 0, it means no provisioner daemons matched the\nrequested tags.",
"type": "integer"
},
"most_recently_seen": {
"description": "MostRecentlySeen is the most recently seen time of the set of matched\nprovisioners. If no provisioners matched, this field will be null.",
"type": "string",
"format": "date-time"
}
}
},
"codersdk.MinimalOrganization": {
"type": "object",
"required": ["id"],
@@ -12323,6 +12341,9 @@
"job": {
"$ref": "#/definitions/codersdk.ProvisionerJob"
},
"matched_provisioners": {
"$ref": "#/definitions/codersdk.MatchedProvisioners"
},
"message": {
"type": "string"
},
+123 -16
View File
@@ -9,6 +9,8 @@ import (
"errors"
"fmt"
"net/http"
"os"
"time"
"github.com/go-chi/chi/v5"
"github.com/google/uuid"
@@ -32,6 +34,7 @@ import (
"github.com/coder/coder/v2/coderd/tracing"
"github.com/coder/coder/v2/codersdk"
"github.com/coder/coder/v2/examples"
"github.com/coder/coder/v2/provisioner/terraform/tfparse"
"github.com/coder/coder/v2/provisionersdk"
sdkproto "github.com/coder/coder/v2/provisionersdk/proto"
)
@@ -74,7 +77,7 @@ func (api *API) templateVersion(rw http.ResponseWriter, r *http.Request) {
warnings = append(warnings, codersdk.TemplateVersionWarningUnsupportedWorkspaces)
}
httpapi.Write(ctx, rw, http.StatusOK, convertTemplateVersion(templateVersion, convertProvisionerJob(jobs[0]), warnings))
httpapi.Write(ctx, rw, http.StatusOK, convertTemplateVersion(templateVersion, convertProvisionerJob(jobs[0]), codersdk.MatchedProvisioners{}, warnings))
}
// @Summary Patch template version by ID
@@ -170,7 +173,7 @@ func (api *API) patchTemplateVersion(rw http.ResponseWriter, r *http.Request) {
return
}
httpapi.Write(ctx, rw, http.StatusOK, convertTemplateVersion(updatedTemplateVersion, convertProvisionerJob(jobs[0]), nil))
httpapi.Write(ctx, rw, http.StatusOK, convertTemplateVersion(updatedTemplateVersion, convertProvisionerJob(jobs[0]), codersdk.MatchedProvisioners{}, nil))
}
// @Summary Cancel template version by ID
@@ -811,7 +814,7 @@ func (api *API) templateVersionsByTemplate(rw http.ResponseWriter, r *http.Reque
return err
}
apiVersions = append(apiVersions, convertTemplateVersion(version, convertProvisionerJob(job), nil))
apiVersions = append(apiVersions, convertTemplateVersion(version, convertProvisionerJob(job), codersdk.MatchedProvisioners{}, nil))
}
return nil
@@ -866,7 +869,7 @@ func (api *API) templateVersionByName(rw http.ResponseWriter, r *http.Request) {
return
}
httpapi.Write(ctx, rw, http.StatusOK, convertTemplateVersion(templateVersion, convertProvisionerJob(jobs[0]), nil))
httpapi.Write(ctx, rw, http.StatusOK, convertTemplateVersion(templateVersion, convertProvisionerJob(jobs[0]), codersdk.MatchedProvisioners{}, nil))
}
// @Summary Get template version by organization, template, and name
@@ -931,7 +934,7 @@ func (api *API) templateVersionByOrganizationTemplateAndName(rw http.ResponseWri
return
}
httpapi.Write(ctx, rw, http.StatusOK, convertTemplateVersion(templateVersion, convertProvisionerJob(jobs[0]), nil))
httpapi.Write(ctx, rw, http.StatusOK, convertTemplateVersion(templateVersion, convertProvisionerJob(jobs[0]), codersdk.MatchedProvisioners{}, nil))
}
// @Summary Get previous template version by organization, template, and name
@@ -1017,7 +1020,7 @@ func (api *API) previousTemplateVersionByOrganizationTemplateAndName(rw http.Res
return
}
httpapi.Write(ctx, rw, http.StatusOK, convertTemplateVersion(previousTemplateVersion, convertProvisionerJob(jobs[0]), nil))
httpapi.Write(ctx, rw, http.StatusOK, convertTemplateVersion(previousTemplateVersion, convertProvisionerJob(jobs[0]), codersdk.MatchedProvisioners{}, nil))
}
// @Summary Archive template unused versions by template id
@@ -1341,9 +1344,6 @@ func (api *API) postTemplateVersionsByOrganization(rw http.ResponseWriter, r *ht
}
}
// Ensures the "owner" is properly applied.
tags := provisionersdk.MutateTags(apiKey.UserID, req.ProvisionerTags)
if req.ExampleID != "" && req.FileID != uuid.Nil {
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
Message: "You cannot specify both an example_id and a file_id.",
@@ -1437,8 +1437,58 @@ func (api *API) postTemplateVersionsByOrganization(rw http.ResponseWriter, r *ht
}
}
// Try to parse template tags from the given file.
tempDir, err := os.MkdirTemp(api.Options.CacheDir, "tfparse-*")
if err != nil {
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
Message: "Internal error checking workspace tags",
Detail: "create tempdir: " + err.Error(),
})
return
}
defer func() {
if err := os.RemoveAll(tempDir); err != nil {
api.Logger.Error(ctx, "failed to remove temporary tfparse dir", slog.Error(err))
}
}()
if err := tfparse.WriteArchive(file.Data, file.Mimetype, tempDir); err != nil {
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
Message: "Internal error checking workspace tags",
Detail: "extract archive to tempdir: " + err.Error(),
})
return
}
parser, diags := tfparse.New(tempDir, tfparse.WithLogger(api.Logger.Named("tfparse")))
if diags.HasErrors() {
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
Message: "Internal error checking workspace tags",
Detail: "parse module: " + diags.Error(),
})
return
}
parsedTags, err := parser.WorkspaceTagDefaults(ctx)
if err != nil {
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
Message: "Internal error checking workspace tags",
Detail: "evaluate default values of workspace tags: " + err.Error(),
})
return
}
// Ensure the "owner" tag is properly applied in addition to request tags and coder_workspace_tags.
// Tag order precedence:
// 1) User-specified tags in the request
// 2) Tags parsed from coder_workspace_tags data source in template file
// 2 may clobber 1.
tags := provisionersdk.MutateTags(apiKey.UserID, req.ProvisionerTags, parsedTags)
var templateVersion database.TemplateVersion
var provisionerJob database.ProvisionerJob
var warnings []codersdk.TemplateVersionWarning
var matchedProvisioners codersdk.MatchedProvisioners
err = api.Database.InTx(func(tx database.Store) error {
jobID := uuid.New()
@@ -1463,6 +1513,27 @@ func (api *API) postTemplateVersionsByOrganization(rw http.ResponseWriter, r *ht
return err
}
// Check for eligible provisioners. This allows us to log a message warning deployment administrators
// of users submitting jobs for which no provisioners are available.
matchedProvisioners, err = checkProvisioners(ctx, tx, organization.ID, tags, api.DeploymentValues.Provisioner.DaemonPollInterval.Value())
if err != nil {
api.Logger.Error(ctx, "failed to check eligible provisioner daemons for job", slog.Error(err))
} else if matchedProvisioners.Count == 0 {
api.Logger.Warn(ctx, "no matching provisioners found for job",
slog.F("user_id", apiKey.UserID),
slog.F("job_id", jobID),
slog.F("job_type", database.ProvisionerJobTypeTemplateVersionImport),
slog.F("tags", tags),
)
} else if matchedProvisioners.Available == 0 {
api.Logger.Warn(ctx, "no active provisioners found for job",
slog.F("user_id", apiKey.UserID),
slog.F("job_id", jobID),
slog.F("job_type", database.ProvisionerJobTypeTemplateVersionImport),
slog.F("tags", tags),
)
}
provisionerJob, err = tx.InsertProvisionerJob(ctx, database.InsertProvisionerJobParams{
ID: jobID,
CreatedAt: dbtime.Now(),
@@ -1552,10 +1623,14 @@ func (api *API) postTemplateVersionsByOrganization(rw http.ResponseWriter, r *ht
api.Logger.Error(ctx, "failed to post provisioner job to pubsub", slog.Error(err))
}
httpapi.Write(ctx, rw, http.StatusCreated, convertTemplateVersion(templateVersion, convertProvisionerJob(database.GetProvisionerJobsByIDsWithQueuePositionRow{
ProvisionerJob: provisionerJob,
QueuePosition: 0,
}), nil))
httpapi.Write(ctx, rw, http.StatusCreated, convertTemplateVersion(
templateVersion,
convertProvisionerJob(database.GetProvisionerJobsByIDsWithQueuePositionRow{
ProvisionerJob: provisionerJob,
QueuePosition: 0,
}),
matchedProvisioners,
warnings))
}
// templateVersionResources returns the workspace agent resources associated
@@ -1622,7 +1697,7 @@ func (api *API) templateVersionLogs(rw http.ResponseWriter, r *http.Request) {
api.provisionerJobLogs(rw, r, job)
}
func convertTemplateVersion(version database.TemplateVersion, job codersdk.ProvisionerJob, warnings []codersdk.TemplateVersionWarning) codersdk.TemplateVersion {
func convertTemplateVersion(version database.TemplateVersion, job codersdk.ProvisionerJob, matchedProvisioners codersdk.MatchedProvisioners, warnings []codersdk.TemplateVersionWarning) codersdk.TemplateVersion {
return codersdk.TemplateVersion{
ID: version.ID,
TemplateID: &version.TemplateID.UUID,
@@ -1638,8 +1713,9 @@ func convertTemplateVersion(version database.TemplateVersion, job codersdk.Provi
Username: version.CreatedByUsername,
AvatarURL: version.CreatedByAvatarURL,
},
Archived: version.Archived,
Warnings: warnings,
Archived: version.Archived,
Warnings: warnings,
MatchedProvisioners: matchedProvisioners,
}
}
@@ -1742,3 +1818,34 @@ func (api *API) publishTemplateUpdate(ctx context.Context, templateID uuid.UUID)
slog.F("template_id", templateID), slog.Error(err))
}
}
func checkProvisioners(ctx context.Context, store database.Store, orgID uuid.UUID, wantTags map[string]string, pollInterval time.Duration) (codersdk.MatchedProvisioners, error) {
// Check for eligible provisioners. This allows us to return a warning to the user if they
// submit a job for which no provisioner is available.
eligibleProvisioners, err := store.GetProvisionerDaemonsByOrganization(ctx, database.GetProvisionerDaemonsByOrganizationParams{
OrganizationID: orgID,
WantTags: wantTags,
})
if err != nil {
// Log the error but do not return any warnings. This is purely advisory and we should not block.
return codersdk.MatchedProvisioners{}, xerrors.Errorf("provisioner daemons by organization: %w", err)
}
threePollsAgo := time.Now().Add(-3 * pollInterval)
mostRecentlySeen := codersdk.NullTime{}
var matched codersdk.MatchedProvisioners
for _, provisioner := range eligibleProvisioners {
if !provisioner.LastSeenAt.Valid {
continue
}
matched.Count++
if provisioner.LastSeenAt.Time.After(threePollsAgo) {
matched.Available++
}
if provisioner.LastSeenAt.Time.After(mostRecentlySeen.Time) {
matched.MostRecentlySeen.Valid = true
matched.MostRecentlySeen.Time = provisioner.LastSeenAt.Time
}
}
return matched, nil
}
+248
View File
@@ -16,6 +16,7 @@ import (
"github.com/coder/coder/v2/coderd/audit"
"github.com/coder/coder/v2/coderd/coderdtest"
"github.com/coder/coder/v2/coderd/database"
"github.com/coder/coder/v2/coderd/database/dbtestutil"
"github.com/coder/coder/v2/coderd/externalauth"
"github.com/coder/coder/v2/coderd/rbac"
"github.com/coder/coder/v2/coderd/rbac/policy"
@@ -221,6 +222,253 @@ func TestPostTemplateVersionsByOrganization(t *testing.T) {
})
require.NoError(t, err)
})
t.Run("WorkspaceTags", func(t *testing.T) {
t.Parallel()
// This test ensures that when creating a template version from an archive continaining a coder_workspace_tags
// data source, we automatically assign some "reasonable" provisioner tag values to the resulting template
// import job.
// TODO(Cian): I'd also like to assert that the correct raw tag values are stored in the database,
// but in order to do this, we need to actually run the job! This isn't straightforward right now.
store, ps := dbtestutil.NewDB(t)
client := coderdtest.New(t, &coderdtest.Options{
Database: store,
Pubsub: ps,
})
owner := coderdtest.CreateFirstUser(t, client)
templateAdmin, templateAdminUser := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID, rbac.RoleTemplateAdmin())
for _, tt := range []struct {
name string
files map[string]string
reqTags map[string]string
wantTags map[string]string
expectError string
}{
{
name: "empty",
wantTags: map[string]string{"owner": "", "scope": "organization"},
},
{
name: "main.tf with no tags",
files: map[string]string{
`main.tf`: `
variable "a" {
type = string
default = "1"
}
data "coder_parameter" "b" {
type = string
default = "2"
}
resource "null_resource" "test" {}`,
},
wantTags: map[string]string{"owner": "", "scope": "organization"},
},
{
name: "main.tf with empty workspace tags",
files: map[string]string{
`main.tf`: `
variable "a" {
type = string
default = "1"
}
data "coder_parameter" "b" {
type = string
default = "2"
}
resource "null_resource" "test" {}
data "coder_workspace_tags" "tags" {
tags = {}
}`,
},
wantTags: map[string]string{"owner": "", "scope": "organization"},
},
{
name: "main.tf with workspace tags",
files: map[string]string{
`main.tf`: `
variable "a" {
type = string
default = "1"
}
data "coder_parameter" "b" {
type = string
default = "2"
}
resource "null_resource" "test" {}
data "coder_workspace_tags" "tags" {
tags = {
"foo": "bar",
"a": var.a,
"b": data.coder_parameter.b.value,
}
}`,
},
wantTags: map[string]string{"owner": "", "scope": "organization", "foo": "bar", "a": "1", "b": "2"},
},
{
name: "main.tf with workspace tags and request tags",
files: map[string]string{
`main.tf`: `
variable "a" {
type = string
default = "1"
}
data "coder_parameter" "b" {
type = string
default = "2"
}
resource "null_resource" "test" {}
data "coder_workspace_tags" "tags" {
tags = {
"foo": "bar",
"a": var.a,
"b": data.coder_parameter.b.value,
}
}`,
},
reqTags: map[string]string{"baz": "zap", "foo": "noclobber"},
wantTags: map[string]string{"owner": "", "scope": "organization", "foo": "bar", "baz": "zap", "a": "1", "b": "2"},
},
{
name: "main.tf with disallowed workspace tag value",
files: map[string]string{
`main.tf`: `
variable "a" {
type = string
default = "1"
}
data "coder_parameter" "b" {
type = string
default = "2"
}
resource "null_resource" "test" {
name = "foo"
}
data "coder_workspace_tags" "tags" {
tags = {
"foo": "bar",
"a": var.a,
"b": data.coder_parameter.b.value,
"test": null_resource.test.name,
}
}`,
},
expectError: `Unknown variable; There is no variable named "null_resource".`,
},
{
name: "main.tf with disallowed function in tag value",
files: map[string]string{
`main.tf`: `
variable "a" {
type = string
default = "1"
}
data "coder_parameter" "b" {
type = string
default = "2"
}
resource "null_resource" "test" {
name = "foo"
}
data "coder_workspace_tags" "tags" {
tags = {
"foo": "bar",
"a": var.a,
"b": data.coder_parameter.b.value,
"test": try(null_resource.test.name, "whatever"),
}
}`,
},
expectError: `Function calls not allowed; Functions may not be called here.`,
},
// We will allow coder_workspace_tags to set the scope on a template version import job
// BUT the user ID will be ultimately determined by the API key in the scope.
// TODO(Cian): Is this what we want? Or should we just ignore these provisioner
// tags entirely?
{
name: "main.tf with workspace tags that attempts to set user scope",
files: map[string]string{
`main.tf`: `
resource "null_resource" "test" {}
data "coder_workspace_tags" "tags" {
tags = {
"scope": "user",
"owner": "12345678-1234-1234-1234-1234567890ab",
}
}`,
},
wantTags: map[string]string{"owner": templateAdminUser.ID.String(), "scope": "user"},
},
{
name: "main.tf with workspace tags that attempt to clobber org ID",
files: map[string]string{
`main.tf`: `
resource "null_resource" "test" {}
data "coder_workspace_tags" "tags" {
tags = {
"scope": "organization",
"owner": "12345678-1234-1234-1234-1234567890ab",
}
}`,
},
wantTags: map[string]string{"owner": "", "scope": "organization"},
},
{
name: "main.tf with workspace tags that set scope=user",
files: map[string]string{
`main.tf`: `
resource "null_resource" "test" {}
data "coder_workspace_tags" "tags" {
tags = {
"scope": "user",
}
}`,
},
wantTags: map[string]string{"owner": templateAdminUser.ID.String(), "scope": "user"},
},
} {
tt := tt
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
ctx := testutil.Context(t, testutil.WaitShort)
// Create an archive from the files provided in the test case.
tarFile := testutil.CreateTar(t, tt.files)
// Post the archive file
fi, err := templateAdmin.Upload(ctx, "application/x-tar", bytes.NewReader(tarFile))
require.NoError(t, err)
// Create a template version from the archive
tvName := strings.ReplaceAll(testutil.GetRandomName(t), "_", "-")
tv, err := templateAdmin.CreateTemplateVersion(ctx, owner.OrganizationID, codersdk.CreateTemplateVersionRequest{
Name: tvName,
StorageMethod: codersdk.ProvisionerStorageMethodFile,
Provisioner: codersdk.ProvisionerTypeTerraform,
FileID: fi.ID,
ProvisionerTags: tt.reqTags,
})
if tt.expectError == "" {
require.NoError(t, err)
// Assert the expected provisioner job is created from the template version import
pj, err := store.GetProvisionerJobByID(ctx, tv.Job.ID)
require.NoError(t, err)
require.EqualValues(t, tt.wantTags, pj.Tags)
} else {
require.ErrorContains(t, err, tt.expectError)
}
// Also assert that we get the expected information back from the API endpoint
require.Zero(t, tv.MatchedProvisioners.Count)
require.Zero(t, tv.MatchedProvisioners.Available)
require.Zero(t, tv.MatchedProvisioners.MostRecentlySeen.Time)
})
}
})
}
func TestPatchCancelTemplateVersion(t *testing.T) {