From 2a101a4e62c7ef839b397261d43b61f032bcae4a Mon Sep 17 00:00:00 2001 From: Brandon Croft Date: Thu, 31 Oct 2024 12:29:23 -0600 Subject: [PATCH] Prefer kv tags when tags defined as set This will prevent duplicate workspace tags from being created when set tags are matched using kv bindings, or vice-versa. --- internal/cloud/backend.go | 149 ++++++++++++++++++++---------- internal/cloud/backend_test.go | 32 ++++++- internal/cloud/testing.go | 18 ++++ internal/cloud/tfe_client_mock.go | 40 +++++--- 4 files changed, 175 insertions(+), 64 deletions(-) diff --git a/internal/cloud/backend.go b/internal/cloud/backend.go index 9d20de77553e..57ef672ba312 100644 --- a/internal/cloud/backend.go +++ b/internal/cloud/backend.go @@ -11,6 +11,7 @@ import ( "net/http" "net/url" "os" + "slices" "sort" "strings" "sync" @@ -46,7 +47,7 @@ const ( genericHostname = "localterraform.com" ) -var ErrCloudDoesNotSupportKVTags = errors.New("your version of Terraform Enterprise does not support key-value tags. Please upgrade to a version that supports this feature.") +var ErrCloudDoesNotSupportKVTags = errors.New("your version of Terraform Enterprise does not support key-value tags. Please upgrade Terraform Enterprise to a version that supports this feature or use set type tags instead.") // Cloud is an implementation of EnhancedBackend in service of the HCP Terraform or Terraform Enterprise // integration for Terraform CLI. This backend is not intended to be surfaced at the user level and @@ -626,13 +627,22 @@ func (b *Cloud) Workspaces() ([]string, error) { return names, nil } + tagBindingsSupported := true + _, err := b.client.Workspaces.ListTagBindings(context.Background(), b.Organization) + if errors.Is(err, tfe.ErrResourceNotFound) { + tagBindingsSupported = false + } + // Otherwise, multiple workspaces are being mapped. Query HCP Terraform for all the remote // workspaces by the provided mapping strategy. options := &tfe.WorkspaceListOptions{} if b.WorkspaceMapping.Strategy() == WorkspaceTagsStrategy { options.Tags = strings.Join(b.WorkspaceMapping.TagsAsSet, ",") } else if b.WorkspaceMapping.Strategy() == WorkspaceKVTagsStrategy { - options.TagBindings = b.WorkspaceMapping.tfeTagBindings() + if !tagBindingsSupported { + return nil, ErrCloudDoesNotSupportKVTags + } + options.TagBindings = b.WorkspaceMapping.asTFETagBindings() } log.Printf("[TRACE] cloud: Listing workspaces with tag bindings %q", b.WorkspaceMapping.DescribeTags()) @@ -760,7 +770,7 @@ func (b *Cloud) StateMgr(name string) (statemgr.Full, error) { if b.WorkspaceMapping.Strategy() == WorkspaceTagsStrategy { workspaceCreateOptions.Tags = b.WorkspaceMapping.tfeTags() } else if b.WorkspaceMapping.Strategy() == WorkspaceKVTagsStrategy { - workspaceCreateOptions.TagBindings = b.WorkspaceMapping.tfeTagBindings() + workspaceCreateOptions.TagBindings = b.WorkspaceMapping.asTFETagBindings() } // Create project if not exists, otherwise use it @@ -813,26 +823,24 @@ func (b *Cloud) StateMgr(name string) (statemgr.Full, error) { } } - updateRequired, err := b.workspaceTagsRequireUpdate(context.Background(), workspace, b.WorkspaceMapping) - if updateRequired { - if err != nil { - if errors.Is(err, tfe.ErrResourceNotFound) { - return nil, fmt.Errorf("workspace %s does not exist or access to it is unauthorized", name) - } else if errors.Is(err, ErrCloudDoesNotSupportKVTags) { - return nil, err + tagCheck, errFromTagCheck := b.workspaceTagsRequireUpdate(context.Background(), workspace, b.WorkspaceMapping) + if tagCheck.requiresUpdate { + if errFromTagCheck != nil { + if errors.Is(errFromTagCheck, ErrCloudDoesNotSupportKVTags) { + return nil, fmt.Errorf("backend does not support key/value tags. Try using key-only tags: %w", errFromTagCheck) } - return nil, fmt.Errorf("error checking if workspace %s requires update: %w", name, err) } - log.Printf("[TRACE] cloud: Updating tags for %s workspace %s/%s to %s", b.appName, b.Organization, name, b.WorkspaceMapping.DescribeTags()) - if b.WorkspaceMapping.Strategy() == WorkspaceTagsStrategy { + log.Printf("[TRACE] cloud: Updating tags for %s workspace %s/%s to %q", b.appName, b.Organization, name, b.WorkspaceMapping.DescribeTags()) + // Always update using KV tags if possible + if !tagCheck.supportsKVTags { options := tfe.WorkspaceAddTagsOptions{ Tags: b.WorkspaceMapping.tfeTags(), } err = b.client.Workspaces.AddTags(context.Background(), workspace.ID, options) - } else if b.WorkspaceMapping.Strategy() == WorkspaceKVTagsStrategy { + } else { options := tfe.WorkspaceAddTagBindingsOptions{ - TagBindings: b.WorkspaceMapping.tfeTagBindings(), + TagBindings: b.WorkspaceMapping.asTFETagBindings(), } _, err = b.client.Workspaces.AddTagBindings(context.Background(), workspace.ID, options) } @@ -1160,39 +1168,75 @@ func (b *Cloud) cliColorize() *colorstring.Colorize { } } -func (b *Cloud) workspaceTagsRequireUpdate(ctx context.Context, workspace *tfe.Workspace, workspaceMapping WorkspaceMapping) (bool, error) { - if workspaceMapping.Strategy() == WorkspaceTagsStrategy { - existingTags := map[string]struct{}{} - for _, t := range workspace.TagNames { - existingTags[t] = struct{}{} - } +type tagRequiresUpdateResult struct { + requiresUpdate bool + supportsKVTags bool +} - for _, tag := range workspaceMapping.TagsAsSet { - if _, ok := existingTags[tag]; !ok { - return true, nil - } - } - } else if workspaceMapping.Strategy() == WorkspaceKVTagsStrategy { - existingTags := make(map[string]string) - bindings, err := b.client.Workspaces.ListTagBindings(ctx, workspace.ID) - if err != nil && err == tfe.ErrResourceNotFound { - return true, ErrCloudDoesNotSupportKVTags - } else if err != nil { - return true, err - } +func (b *Cloud) workspaceTagsRequireUpdate(ctx context.Context, workspace *tfe.Workspace, workspaceMapping WorkspaceMapping) (result tagRequiresUpdateResult, err error) { + result = tagRequiresUpdateResult{ + supportsKVTags: true, + } - for _, binding := range bindings { - existingTags[binding.Key] = binding.Value + // First, depending on the strategy, build a map of the tags defined in config + // so we can compare them to the actual tags on the workspace + normalizedTagMap := make(map[string]string) + if workspaceMapping.IsTagsStrategy() { + for _, b := range workspaceMapping.asTFETagBindings() { + normalizedTagMap[b.Key] = b.Value } - - for tag, val := range workspaceMapping.TagsAsMap { - if existingVal, ok := existingTags[tag]; !ok || existingVal != val { - return true, nil + } else { + // Not a tag strategy + return + } + + // Fetch tag bindings and determine if they should be checked + bindings, err := b.client.Workspaces.ListTagBindings(ctx, workspace.ID) + if err != nil && errors.Is(err, tfe.ErrResourceNotFound) { + // By this time, the workspace should have been fetched, proving that the + // authenticated user has access to it. If the tag bindings are not found, + // it would mean that the backened does not support tag bindings. + result.supportsKVTags = false + } else if err != nil { + return + } + + err = nil +check: + // Check desired workspace tags against existing tags + for k, v := range normalizedTagMap { + log.Printf("[TRACE] cloud: Checking tag %q=%q", k, v) + if v == "" { + // Tag can exist in legacy tags or tag bindings + if !slices.Contains(workspace.TagNames, k) || (result.supportsKVTags && !slices.ContainsFunc(bindings, func(b *tfe.TagBinding) bool { + return b.Key == k + })) { + result.requiresUpdate = true + break check + } + } else if !result.supportsKVTags { + // There is a value defined, but the backend does not support tag bindings + result.requiresUpdate = true + err = ErrCloudDoesNotSupportKVTags + break check + } else { + // There is a value, so it must match a tag binding + if !slices.ContainsFunc(bindings, func(b *tfe.TagBinding) bool { + return b.Key == k && b.Value == v + }) { + result.requiresUpdate = true + break check } } } - return false, nil + doesOrDoesnot := "does " + if !result.requiresUpdate { + doesOrDoesnot = "does not " + } + log.Printf("[TRACE] cloud: Workspace %s %srequire tag update", workspace.Name, doesOrDoesnot) + + return } type WorkspaceMapping struct { @@ -1390,21 +1434,24 @@ func (wm WorkspaceMapping) tfeTags() []*tfe.Tag { return tags } -func (wm WorkspaceMapping) tfeTagBindings() []*tfe.TagBinding { +func (wm WorkspaceMapping) asTFETagBindings() []*tfe.TagBinding { var tagBindings []*tfe.TagBinding - if wm.Strategy() != WorkspaceKVTagsStrategy { - return tagBindings - } + if wm.Strategy() == WorkspaceKVTagsStrategy { + tagBindings = make([]*tfe.TagBinding, len(wm.TagsAsMap)) - tagBindings = make([]*tfe.TagBinding, len(wm.TagsAsMap)) + index := 0 + for key, val := range wm.TagsAsMap { + tagBindings[index] = &tfe.TagBinding{Key: key, Value: val} + index += 1 + } + } else if wm.Strategy() == WorkspaceTagsStrategy { + tagBindings = make([]*tfe.TagBinding, len(wm.TagsAsSet)) - index := 0 - for key, val := range wm.TagsAsMap { - tagBindings[index] = &tfe.TagBinding{Key: key, Value: val} - index += 1 + for i, tag := range wm.TagsAsSet { + tagBindings[i] = &tfe.TagBinding{Key: tag} + } } - return tagBindings } diff --git a/internal/cloud/backend_test.go b/internal/cloud/backend_test.go index d5c644ff4096..1b7e37342cce 100644 --- a/internal/cloud/backend_test.go +++ b/internal/cloud/backend_test.go @@ -79,6 +79,36 @@ func TestCloud_backendWithTags(t *testing.T) { } } +func TestCloud_backendWithKVTags(t *testing.T) { + b, bCleanup := testBackendWithKVTags(t) + defer bCleanup() + + _, err := b.client.Workspaces.Create(context.Background(), "hashicorp", tfe.WorkspaceCreateOptions{ + Name: tfe.String("ws-billing-101"), + TagBindings: []*tfe.TagBinding{ + {Key: "dept", Value: "billing"}, + {Key: "costcenter", Value: "101"}, + }, + }) + if err != nil { + t.Fatalf("error creating workspace: %s", err) + } + + workspaces, err := b.Workspaces() + if err != nil { + t.Fatalf("error: %s", err) + } + + actual := len(workspaces) + if actual != 1 { + t.Fatalf("expected 1 workspaces, got %d", actual) + } + + if workspaces[0] != "ws-billing-101" { + t.Fatalf("expected workspace name to be 'ws-billing-101', got %s", workspaces[0]) + } +} + func TestCloud_DescribeTags(t *testing.T) { cases := map[string]struct { expected string @@ -1223,7 +1253,7 @@ func TestCloud_resolveCloudConfig(t *testing.T) { t.Fatalf("%s: expected final config of %#v but instead got %#v", name, tc.expectedResult, result) } - if !reflect.DeepEqual(tc.expectedResult.workspaceMapping.tfeTagBindings(), result.workspaceMapping.tfeTagBindings()) { + if !reflect.DeepEqual(tc.expectedResult.workspaceMapping.asTFETagBindings(), result.workspaceMapping.asTFETagBindings()) { t.Fatalf("%s: expected final config of %#v but instead got %#v", name, tc.expectedResult, result) } } diff --git a/internal/cloud/testing.go b/internal/cloud/testing.go index 009a11961651..9018001841c6 100644 --- a/internal/cloud/testing.go +++ b/internal/cloud/testing.go @@ -120,6 +120,24 @@ func testBackendWithTags(t *testing.T) (*Cloud, func()) { return b, c } +func testBackendWithKVTags(t *testing.T) (*Cloud, func()) { + obj := cty.ObjectVal(map[string]cty.Value{ + "hostname": cty.NullVal(cty.String), + "organization": cty.StringVal("hashicorp"), + "token": cty.NullVal(cty.String), + "workspaces": cty.ObjectVal(map[string]cty.Value{ + "name": cty.NullVal(cty.String), + "tags": cty.MapVal(map[string]cty.Value{ + "dept": cty.StringVal("billing"), + "costcenter": cty.StringVal("101"), + }), + "project": cty.NullVal(cty.String), + }), + }) + b, _, c := testBackend(t, obj, nil) + return b, c +} + func testBackendNoOperations(t *testing.T) (*Cloud, func()) { obj := cty.ObjectVal(map[string]cty.Value{ "hostname": cty.NullVal(cty.String), diff --git a/internal/cloud/tfe_client_mock.go b/internal/cloud/tfe_client_mock.go index f6f7fca8ad98..53bfc374cf45 100644 --- a/internal/cloud/tfe_client_mock.go +++ b/internal/cloud/tfe_client_mock.go @@ -1942,35 +1942,51 @@ func newMockWorkspaces(client *MockClient) *MockWorkspaces { func (m *MockWorkspaces) List(ctx context.Context, organization string, options *tfe.WorkspaceListOptions) (*tfe.WorkspaceList, error) { wl := &tfe.WorkspaceList{} // Get all the workspaces that match the Search value - searchValue := "" var ws []*tfe.Workspace - var tags []string + searchValue := "" + searchTags := make(map[string]string) if options != nil { if len(options.Search) > 0 { searchValue = options.Search } if len(options.Tags) > 0 { - tags = strings.Split(options.Tags, ",") + for _, tag := range strings.Split(options.Tags, ",") { + searchTags[tag] = "" + } + } + if len(options.TagBindings) > 0 { + for _, kvTag := range options.TagBindings { + searchTags[kvTag.Key] = kvTag.Value + } } } for _, w := range m.workspaceIDs { - wTags := make(map[string]struct{}) + wTags := make(map[string]string) for _, wTag := range w.Tags { - wTags[wTag.Name] = struct{}{} + wTags[wTag.Name] = "" } - if strings.Contains(w.Name, searchValue) { - tagsSatisfied := true - for _, tag := range tags { - if _, ok := wTags[tag]; !ok { + for _, kvTag := range w.TagBindings { + wTags[kvTag.Key] = kvTag.Value + } + + tagsSatisfied := true + for k, v := range searchTags { + if value, ok := wTags[k]; ok { + if value != v { tagsSatisfied = false + break } + } else { + tagsSatisfied = false + break } - if tagsSatisfied { - ws = append(ws, w) - } + } + + if strings.Contains(w.Name, searchValue) && tagsSatisfied { + ws = append(ws, w) } }