From f8d6d5148205986960004f3f955dd961e23a1115 Mon Sep 17 00:00:00 2001 From: Ian Wahbe Date: Mon, 16 Sep 2024 17:02:35 +0200 Subject: [PATCH] Fix empty labels perma-diff This PR takes advantage of pulumi/pulumi-terraform-bridge#2417 to fixup the incorrect planned state caused by empty labels. Fixes #2372 --- examples/examples_go_test.go | 12 +- provider/go.mod | 11 +- provider/go.sum | 15 +- provider/labels.go | 80 +++++++ provider/labels_impl_test.go | 89 +++++++ provider/provider_yaml_test.go | 221 ++++++++++++++++++ provider/resources.go | 6 +- .../empty-alone-label/Pulumi.yaml | 19 ++ .../empty-default-label/Pulumi.yaml | 22 ++ .../test-programs/empty-label/Pulumi.yaml | 20 ++ .../empty-unmanaged-label/Pulumi.yaml | 21 ++ 11 files changed, 500 insertions(+), 16 deletions(-) create mode 100644 provider/test-programs/empty-alone-label/Pulumi.yaml create mode 100644 provider/test-programs/empty-default-label/Pulumi.yaml create mode 100644 provider/test-programs/empty-label/Pulumi.yaml create mode 100644 provider/test-programs/empty-unmanaged-label/Pulumi.yaml diff --git a/examples/examples_go_test.go b/examples/examples_go_test.go index f9a8a8b9b5..d9bd58663f 100644 --- a/examples/examples_go_test.go +++ b/examples/examples_go_test.go @@ -479,7 +479,7 @@ func expectedLabelValue(arg expectedLabelValueArg) string { func (st labelsState) expectedLabels(prev labelsState) map[string]string { // Note that the upstream provider actually takes a "" value for a label to mean "keep the previous value". // This behaviour is exposed under PlanResourceChange - r := map[string]string{} + expected := map[string]string{} keys := map[string]struct{}{} for k := range st.DefaultLabels { @@ -495,7 +495,7 @@ func (st labelsState) expectedLabels(prev labelsState) map[string]string { prevLabelsVal, inPrevLabels := prev.Labels[k] prevDefaultsVal, inPrevDefaults := prev.DefaultLabels[k] - expectedVal := expectedLabelValue( + expected[k] = expectedLabelValue( expectedLabelValueArg{ labels: valuePresentTuple{value: labelsVal, present: inLabels}, defaults: valuePresentTuple{value: defaultsVal, present: inDefaults}, @@ -503,16 +503,12 @@ func (st labelsState) expectedLabels(prev labelsState) map[string]string { prevDefaults: valuePresentTuple{value: prevDefaultsVal, present: inPrevDefaults}, }, ) - - if expectedVal != "" { - r[k] = expectedVal - } } // Because we're reading pulumiLabels, we need to expect the default provisioning label. - r["goog-pulumi-provisioned"] = "true" + expected["goog-pulumi-provisioned"] = "true" - return r + return expected } func validateStateResult(phase int, st1, st2 labelsState) func( diff --git a/provider/go.mod b/provider/go.mod index 15f0a37066..90e24fc8b6 100644 --- a/provider/go.mod +++ b/provider/go.mod @@ -5,11 +5,13 @@ go 1.22 toolchain go1.22.7 require ( + github.com/hashicorp/go-cty v1.4.1-0.20200414143053-d3edf31b6320 github.com/hashicorp/terraform-plugin-sdk/v2 v2.33.0 github.com/hashicorp/terraform-provider-google-beta v0.0.0 + github.com/hexops/autogold/v2 v2.2.1 github.com/pulumi/providertest v0.0.14 - github.com/pulumi/pulumi-terraform-bridge/pf v0.43.0 - github.com/pulumi/pulumi-terraform-bridge/v3 v3.90.0 + github.com/pulumi/pulumi-terraform-bridge/pf v0.43.1-0.20240918114950-4954882afe70 + github.com/pulumi/pulumi-terraform-bridge/v3 v3.90.1-0.20240918114950-4954882afe70 github.com/pulumi/pulumi/pkg/v3 v3.130.0 github.com/pulumi/pulumi/sdk/v3 v3.130.0 github.com/stretchr/testify v1.9.0 @@ -129,7 +131,6 @@ require ( github.com/hashicorp/errwrap v1.1.0 // indirect github.com/hashicorp/go-checkpoint v0.5.0 // indirect github.com/hashicorp/go-cleanhttp v0.5.2 // indirect - github.com/hashicorp/go-cty v1.4.1-0.20200414143053-d3edf31b6320 // indirect github.com/hashicorp/go-getter v1.7.5 // indirect github.com/hashicorp/go-hclog v1.6.3 // indirect github.com/hashicorp/go-multierror v1.1.1 // indirect @@ -158,6 +159,8 @@ require ( github.com/hashicorp/terraform-svchost v0.1.1 // indirect github.com/hashicorp/vault/api v1.12.0 // indirect github.com/hashicorp/yamux v0.1.1 // indirect + github.com/hexops/gotextdiff v1.0.3 // indirect + github.com/hexops/valast v1.4.4 // indirect github.com/huandu/xstrings v1.4.0 // indirect github.com/iancoleman/strcase v0.3.0 // indirect github.com/imdario/mergo v0.3.15 // indirect @@ -189,6 +192,7 @@ require ( github.com/muesli/reflow v0.3.0 // indirect github.com/muesli/termenv v0.15.2 // indirect github.com/natefinch/atomic v1.0.1 // indirect + github.com/nightlyone/lockfile v1.0.0 // indirect github.com/nxadm/tail v1.4.11 // indirect github.com/oklog/run v1.1.0 // indirect github.com/opentracing/basictracer-go v1.1.0 // indirect @@ -274,4 +278,5 @@ require ( gopkg.in/warnings.v0 v0.1.2 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect lukechampine.com/frand v1.4.2 // indirect + mvdan.cc/gofumpt v0.5.0 // indirect ) diff --git a/provider/go.sum b/provider/go.sum index d846454485..355bd61d18 100644 --- a/provider/go.sum +++ b/provider/go.sum @@ -1401,12 +1401,14 @@ github.com/ettle/strcase v0.1.1/go.mod h1:hzDLsPC7/lwKyBOywSHEP89nt2pDgdy+No1NBA github.com/fatih/color v1.7.0/go.mod h1:Zm6kSWBoL9eyXnKyktHP6abPY2pDugNf5KwzbycvMj4= github.com/fatih/color v1.9.0/go.mod h1:eQcE1qtQxscV5RaZvpXrrb8Drkc3/DdQ+uUYCNjL+zU= github.com/fatih/color v1.13.0/go.mod h1:kLAiJbzzSOZDVNGyDpeOxJ47H46qBXwg5ILebYFFOfk= +github.com/fatih/color v1.15.0/go.mod h1:0h5ZqXfHYED7Bhv2ZJamyIOUej9KtShiJESRwBDUSsw= github.com/fatih/color v1.16.0 h1:zmkK9Ngbjj+K0yRhTVONQh1p/HknKYSlNT+vZCzyokM= github.com/fatih/color v1.16.0/go.mod h1:fL2Sau1YI5c0pdGEVCbKQbLXB6edEj1ZgiY4NijnWvE= github.com/felixge/httpsnoop v1.0.4 h1:NFTV2Zj1bL4mc9sqWACXbQFVBBg2W3GPvqp8/ESS2Wg= github.com/felixge/httpsnoop v1.0.4/go.mod h1:m8KPJKqk1gH5J9DgRY2ASl2lWCfGKXixSwevea8zH2U= github.com/fogleman/gg v1.2.1-0.20190220221249-0403632d5b90/go.mod h1:R/bRT+9gY/C5z7JzPU0zXsXHKM4/ayA+zqcVNZzPa1k= github.com/fogleman/gg v1.3.0/go.mod h1:R/bRT+9gY/C5z7JzPU0zXsXHKM4/ayA+zqcVNZzPa1k= +github.com/frankban/quicktest v1.14.3/go.mod h1:mgiwOwqx65TmIk1wJ6Q7wvnVMocbUorkibMOrVTHZps= github.com/frankban/quicktest v1.14.4 h1:g2rn0vABPOOXmZUj+vbmUp0lPoXEMuhTpIluN0XL9UY= github.com/frankban/quicktest v1.14.4/go.mod h1:4ptaffx2x8+WTWXmUCuVU6aPUX1/Mz7zb5vbUoiM6w0= github.com/fsnotify/fsnotify v1.4.7/go.mod h1:jwhsz4b93w/PPRr/qN1Yymfu8t87LnFCMoQvtojpjFo= @@ -1733,6 +1735,7 @@ github.com/hashicorp/yamux v0.0.0-20180604194846-3520598351bb/go.mod h1:+NfK9FKe github.com/hashicorp/yamux v0.0.0-20181012175058-2f1d1f20f75d/go.mod h1:+NfK9FKeTrX5uv1uIXGdwYDTeHna2qgaIlx54MXqjAM= github.com/hashicorp/yamux v0.1.1 h1:yrQxtgseBDrq9Y652vSRDvsKCJKOUD+GzTS4Y0Y8pvE= github.com/hashicorp/yamux v0.1.1/go.mod h1:CtWFDAQgb7dxtzFs4tWbplKIe2jSi3+5vKbgIO0SLnQ= +github.com/hexops/autogold v0.8.1/go.mod h1:97HLDXyG23akzAoRYJh/2OBs3kd80eHyKPvZw0S5ZBY= github.com/hexops/autogold v1.3.0 h1:IEtGNPxBeBu8RMn8eKWh/Ll9dVNgSnJ7bp/qHgMQ14o= github.com/hexops/autogold v1.3.0/go.mod h1:d4hwi2rid66Sag+BVuHgwakW/EmaFr8vdTSbWDbrDRI= github.com/hexops/autogold/v2 v2.2.1 h1:JPUXuZQGkcQMv7eeDXuNMovjfoRYaa0yVcm+F3voaGY= @@ -1827,6 +1830,7 @@ github.com/mattn/go-isatty v0.0.12/go.mod h1:cbi8OIDigv2wuxKPP5vlRcQ1OAZbq2CE4Ky github.com/mattn/go-isatty v0.0.14/go.mod h1:7GGIvUiUoEMVVmxf/4nioHXj79iQHKdU27kJ6hsGG94= github.com/mattn/go-isatty v0.0.16/go.mod h1:kYGgaQfpe5nmfYZH+SKPsOc2e4SrIfOl2e/yFXSvRLM= github.com/mattn/go-isatty v0.0.17/go.mod h1:kYGgaQfpe5nmfYZH+SKPsOc2e4SrIfOl2e/yFXSvRLM= +github.com/mattn/go-isatty v0.0.19/go.mod h1:W+V8PltTTMOvKvAeJH7IuucS94S2C6jfK/D7dTCTo3Y= github.com/mattn/go-isatty v0.0.20 h1:xfD0iDuEKnDkl03q4limB+vH+GxLEtL/jb4xVJSWWEY= github.com/mattn/go-isatty v0.0.20/go.mod h1:W+V8PltTTMOvKvAeJH7IuucS94S2C6jfK/D7dTCTo3Y= github.com/mattn/go-localereader v0.0.1 h1:ygSAOl7ZXTx4RdPYinUpg6W99U8jWvWi9Ye2JC/oIi4= @@ -1976,10 +1980,10 @@ github.com/pulumi/providertest v0.0.14 h1:5QlAPAAs82jkQraHsJvq1xgVfC7xtW8sFJwv2p github.com/pulumi/providertest v0.0.14/go.mod h1:GcsqEGgSngwaNOD+kICJPIUQlnA911fGBU8HDlJvVL0= github.com/pulumi/pulumi-java/pkg v0.14.0 h1:CKL7lLF81Fq6VRhA5TNFsSMnHraTNCUzIhqCzYX8Wzk= github.com/pulumi/pulumi-java/pkg v0.14.0/go.mod h1:VybuJMWJtJc9ZNbt1kcYH4TbpocMx9mEi7YWL2Co99c= -github.com/pulumi/pulumi-terraform-bridge/pf v0.43.0 h1:g15WgVKJBhFtzhLqOky6R77QIU3x4KkunrLHDSkK6CM= -github.com/pulumi/pulumi-terraform-bridge/pf v0.43.0/go.mod h1:xdU2rcUBjPX/alXMiywUK1GvN4goUHZxos8ZfT6sVXM= -github.com/pulumi/pulumi-terraform-bridge/v3 v3.90.0 h1:e7xfYAiXCE8LCwfKvbGeNAjdPmKwPM3kavEXECt3wvs= -github.com/pulumi/pulumi-terraform-bridge/v3 v3.90.0/go.mod h1:dIVp4qG+GsUwmpz40L7Z+PZnzHf3cQq1CAFwhz++ris= +github.com/pulumi/pulumi-terraform-bridge/pf v0.43.1-0.20240918114950-4954882afe70 h1:NhS1OHDyvpRyBUaajYIgVUtdjbP8ZY740huQDi2YjHs= +github.com/pulumi/pulumi-terraform-bridge/pf v0.43.1-0.20240918114950-4954882afe70/go.mod h1:xdU2rcUBjPX/alXMiywUK1GvN4goUHZxos8ZfT6sVXM= +github.com/pulumi/pulumi-terraform-bridge/v3 v3.90.1-0.20240918114950-4954882afe70 h1:DWZMy2RK0qc53kyvJMu2Gnn5aEBW1oz6Qjz9JPuSuy4= +github.com/pulumi/pulumi-terraform-bridge/v3 v3.90.1-0.20240918114950-4954882afe70/go.mod h1:dIVp4qG+GsUwmpz40L7Z+PZnzHf3cQq1CAFwhz++ris= github.com/pulumi/pulumi-terraform-bridge/x/muxer v0.0.9-0.20240227144008-2da15b3d6f6e h1:yON1jwN6gg4cjnOQF607I3fTiFyIDr9WSsQNXHD6wbM= github.com/pulumi/pulumi-terraform-bridge/x/muxer v0.0.9-0.20240227144008-2da15b3d6f6e/go.mod h1:AvlZujvfRiEu+60frCGEhaLeGttjHwM/g+47+IdPZXw= github.com/pulumi/pulumi-yaml v1.9.2 h1:BCUuRPA1USmFXrExiHRU8yJ+OiphLYnroPxKRgGCJrs= @@ -2007,6 +2011,7 @@ github.com/rogpeppe/go-charset v0.0.0-20180617210344-2471d30d28b4/go.mod h1:qgYe github.com/rogpeppe/go-internal v1.3.0/go.mod h1:M8bDsm7K2OlrFYOpmOWEs/qY81heoFRclV5y23lUDJ4= github.com/rogpeppe/go-internal v1.6.1/go.mod h1:xXDCJY+GAPziupqXw64V24skbSoqbTEfhy4qGm1nDQc= github.com/rogpeppe/go-internal v1.9.0/go.mod h1:WtVeX8xhTBvf0smdhujwtBcq4Qrzq/fJaraNFVN+nFs= +github.com/rogpeppe/go-internal v1.10.0/go.mod h1:UQnix2H7Ngw/k4C5ijL5+65zddjncjaFoBhdsK/akog= github.com/rogpeppe/go-internal v1.11.0/go.mod h1:ddIwULY96R17DhadqLgMfk9H9tvdUzkipdSkR5nkCZA= github.com/rogpeppe/go-internal v1.12.0 h1:exVL4IDcn6na9z1rAb56Vxr+CgyK3nn3O+epU5NdKM8= github.com/rogpeppe/go-internal v1.12.0/go.mod h1:E+RYuTGaKKdloAfM02xzb0FW3Paa99yedzYV+kq4uf4= @@ -2658,6 +2663,7 @@ golang.org/x/tools v0.8.0/go.mod h1:JxBZ99ISMI5ViVkT1tr6tdNmXeTrcpVSD3vZ1RsRdN4= golang.org/x/tools v0.9.1/go.mod h1:owI94Op576fPu3cIGQeHs3joujW/2Oc6MtlxbF5dfNc= golang.org/x/tools v0.9.3/go.mod h1:owI94Op576fPu3cIGQeHs3joujW/2Oc6MtlxbF5dfNc= golang.org/x/tools v0.10.0/go.mod h1:UJwyiVBsOA2uwvK/e5OY3GTpDUJriEd+/YlqAwLPmyM= +golang.org/x/tools v0.12.0/go.mod h1:Sc0INKfu04TlqNoRA1hgpFZbhYXHPr4V5DzpSBTPqQM= golang.org/x/tools v0.13.0/go.mod h1:HvlwmtVNQAhOuCjW7xxvovg8wbNq7LwfXh/k7wXUl58= golang.org/x/tools v0.17.0/go.mod h1:xsh6VxdV005rRVaS6SSAf9oiAqljS7UZUacMZ8Bnsps= golang.org/x/tools v0.22.0 h1:gqSGLZqv+AI9lIQzniJ0nZDRG5GBPsSi+DRNHWNz6yA= @@ -3112,6 +3118,7 @@ modernc.org/token v1.0.0/go.mod h1:UGzOrNV1mAFSEB63lOFHIpNRUVMvYTc6yu1SMY/XTDM= modernc.org/token v1.0.1/go.mod h1:UGzOrNV1mAFSEB63lOFHIpNRUVMvYTc6yu1SMY/XTDM= modernc.org/token v1.1.0/go.mod h1:UGzOrNV1mAFSEB63lOFHIpNRUVMvYTc6yu1SMY/XTDM= modernc.org/z v1.5.1/go.mod h1:eWFB510QWW5Th9YGZT81s+LwvaAs3Q2yr4sP0rmLkv8= +mvdan.cc/gofumpt v0.4.0/go.mod h1:PljLOHDeZqgS8opHRKLzp2It2VBuSdteAgqUfzMTxlQ= mvdan.cc/gofumpt v0.5.0 h1:0EQ+Z56k8tXjj/6TQD25BFNKQXpCvT0rnansIc7Ug5E= mvdan.cc/gofumpt v0.5.0/go.mod h1:HBeVDtMKRZpXyxFciAirzdKklDlGu8aAy1wEbH5Y9js= pgregory.net/rapid v0.6.1 h1:4eyrDxyht86tT4Ztm+kvlyNBLIk071gR+ZQdhphc9dQ= diff --git a/provider/labels.go b/provider/labels.go index c17397b165..debe79eee2 100644 --- a/provider/labels.go +++ b/provider/labels.go @@ -5,11 +5,91 @@ package gcp import ( "context" + "github.com/hashicorp/go-cty/cty" + "github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfbridge" + shimv2 "github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfshim/sdk-v2" "github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfshim/walk" "github.com/pulumi/pulumi/sdk/v3/go/common/resource" + "github.com/pulumi/pulumi/sdk/v3/go/property" ) +// fixEmptyLabels applies a state edit to fix +// https://github.com/pulumi/pulumi-gcp/issues/2372. +func fixEmptyLabels(_ context.Context, req shimv2.PlanStateEditRequest) (cty.Value, error) { + // programLabels holds the labels that we expect to be applied. It is the union of + // labels (from the resource) and defaultLabels (from the provider). + // + // programLabels may not be the full set of labels on the resource, since + // effective_labels can include labels read from the cloud provider. + programLabels := property.Map{} + + // Apply default labels first. + if pConfig := resource.FromResourcePropertyValue(resource.NewProperty(req.ProviderConfig)); pConfig.IsMap() { + l := pConfig.AsMap()["defaultLabels"] + if l.IsMap() { + programLabels = l.AsMap() + } + } + + // Apply labels next, allowing labels to override defaultLabels. + if inputs, ok := (resource.PropertyPath{"labels"}.Get(resource.NewProperty(req.NewInputs))); ok { + if labels := resource.FromResourcePropertyValue(inputs); labels.IsMap() { + for k, v := range labels.AsMap() { + programLabels[k] = v + } + } + } + + fixMap := func(f func(map[string]cty.Value) map[string]cty.Value) func(cty.Value) cty.Value { + return func(v cty.Value) cty.Value { + if !v.Type().IsMapType() || !v.IsKnown() || v.IsNull() { + return v + } + + result := f(v.AsValueMap()) + if len(result) == 0 { + return cty.MapValEmpty(v.Type().ElementType()) + } + return cty.MapVal(result) + } + } + + // fixOutputLabels fixes falsely unknown values label values. + fixOutputLabels := fixMap(func(m map[string]cty.Value) map[string]cty.Value { + for k, v := range m { + if v.IsKnown() { + // If v is known, so no correction is needed. + continue + } + label, ok := programLabels[k] // labels is in the Pulumi namespace + if !ok || label.IsComputed() || !label.IsString() { + // If we didn't inherit label from the program or the + // label is actually unknown, then just continue. + continue + } + + // v is incorrectly unknown, so set it to the known value from the + // program. + m[k] = cty.StringVal(label.AsString()) + } + return m + }) + + // Apply f to m[k] if k in m. + mapIfExists := func(m map[string]cty.Value, k string, f func(cty.Value) cty.Value) { + v, ok := m[k] + if ok { + m[k] = f(v) + } + } + + planState := req.PlanState.AsValueMap() + mapIfExists(planState, "effective_labels", fixOutputLabels) + mapIfExists(planState, "terraform_labels", fixOutputLabels) + return cty.ObjectVal(planState), nil +} + // GCP has many resources with labels called "terraform_labels". We want to change those // to "pulumiLabels". // diff --git a/provider/labels_impl_test.go b/provider/labels_impl_test.go index 155b56ca05..991d36fef6 100644 --- a/provider/labels_impl_test.go +++ b/provider/labels_impl_test.go @@ -6,9 +6,11 @@ import ( "context" "testing" + "github.com/hashicorp/go-cty/cty" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + shimv2 "github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfshim/sdk-v2" "github.com/pulumi/pulumi/sdk/v3/go/common/resource" ) @@ -225,3 +227,90 @@ func TestEnsureLabelPathsExist(t *testing.T) { }) } } + +func TestFixEmptyLabels(t *testing.T) { + tests := []struct { + name string + inputs shimv2.PlanStateEditRequest + expected cty.Value + }{ + { + name: "adjust incorrectly unknown labels", + inputs: shimv2.PlanStateEditRequest{ + PlanState: cty.ObjectVal(map[string]cty.Value{ + "effective_labels": cty.MapVal(map[string]cty.Value{ + "empty": cty.UnknownVal(cty.String), + }), + "terraform_labels": cty.MapVal(map[string]cty.Value{ + "empty": cty.UnknownVal(cty.String), + }), + }), + NewInputs: resource.PropertyMap{ + "labels": resource.NewProperty(resource.PropertyMap{ + "empty": resource.NewProperty(""), + }), + }, + }, + expected: cty.ObjectVal(map[string]cty.Value{ + "effective_labels": cty.MapVal(map[string]cty.Value{ + "empty": cty.StringVal(""), + }), + "terraform_labels": cty.MapVal(map[string]cty.Value{ + "empty": cty.StringVal(""), + }), + }), + }, + { + name: "adjust incorrectly unknown default labels", + inputs: shimv2.PlanStateEditRequest{ + PlanState: cty.ObjectVal(map[string]cty.Value{ + "effective_labels": cty.MapVal(map[string]cty.Value{ + "empty": cty.UnknownVal(cty.String), + }), + }), + ProviderConfig: resource.PropertyMap{ + "defaultLabels": resource.NewProperty(resource.PropertyMap{ + "empty": resource.NewProperty(""), + }), + }, + }, + expected: cty.ObjectVal(map[string]cty.Value{ + "effective_labels": cty.MapVal(map[string]cty.Value{ + "empty": cty.StringVal(""), + }), + }), + }, + { + name: "ignore valid labels", + inputs: shimv2.PlanStateEditRequest{ + PlanState: cty.ObjectVal(map[string]cty.Value{ + "effective_labels": cty.MapVal(map[string]cty.Value{ + "non-empty": cty.StringVal("val"), + "actually-unknown": cty.UnknownVal(cty.String), + }), + }), + NewInputs: resource.PropertyMap{ + "labels": resource.NewProperty(resource.PropertyMap{ + "non-empty": resource.NewProperty("val"), + "actually-unknown": resource.MakeComputed(resource.NewProperty("")), + }), + }, + }, + expected: cty.ObjectVal(map[string]cty.Value{ + "effective_labels": cty.MapVal(map[string]cty.Value{ + "non-empty": cty.StringVal("val"), + "actually-unknown": cty.UnknownVal(cty.String), + }), + }), + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + result, err := fixEmptyLabels(context.Background(), tt.inputs) + require.NoError(t, err) + assert.Equal(t, tt.expected, result) + }) + } +} diff --git a/provider/provider_yaml_test.go b/provider/provider_yaml_test.go index fa87a8d60c..0167163ecd 100644 --- a/provider/provider_yaml_test.go +++ b/provider/provider_yaml_test.go @@ -28,6 +28,7 @@ import ( "strings" "testing" + "github.com/hexops/autogold/v2" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -36,7 +37,10 @@ import ( "github.com/pulumi/providertest/pulumitest/optnewstack" "github.com/pulumi/providertest/pulumitest/opttest" "github.com/pulumi/providertest/replay" + "github.com/pulumi/pulumi/sdk/v3/go/auto" "github.com/pulumi/pulumi/sdk/v3/go/auto/optpreview" + "github.com/pulumi/pulumi/sdk/v3/go/auto/optrefresh" + "github.com/pulumi/pulumi/sdk/v3/go/auto/optup" "github.com/pulumi/pulumi/sdk/v3/go/common/apitype" ) @@ -1051,6 +1055,7 @@ func TestImport(t *testing.T) { programPath string resourceType string explicitProvider bool + skip string // If skip is non-empty, the the test will be skipped with skip's value as the reason }{ { testName: "bucket", @@ -1062,6 +1067,7 @@ func TestImport(t *testing.T) { programPath: filepath.Join("test-programs", "labeled-bucket-with-defaults"), resourceType: "gcp:storage/bucket:Bucket", explicitProvider: true, + skip: "Skipping due to https://github.com/pulumi/pulumi/issues/17290", }, { testName: "bucket-iam-binding", @@ -1070,6 +1076,9 @@ func TestImport(t *testing.T) { }, } { t.Run(tc.testName, func(t *testing.T) { + if tc.skip != "" { + t.Skip(tc.skip) + } test := pulumiTest(t, tc.programPath) res := test.Up() @@ -1120,3 +1129,215 @@ func TestFirestoreDatabaseAutoname(t *testing.T) { pt.SetConfig("gcpProj", proj) pt.Up() } + +func TestEmptyLabels(t *testing.T) { + tests := []struct { + program string + previewStdout autogold.Value + upOutputs autogold.Value + }{ + {"empty-label", autogold.Expect(`Previewing update (test): + + + pulumi:pulumi:Stack empty-label-test create + + gcp:kms:KeyRing ring create + + gcp:kms:CryptoKey key create + + pulumi:pulumi:Stack empty-label-test create +Outputs: + effectiveLabels: [secret] + labels : { + empty : "" + static: "value" + } + pulumiLabels : [secret] + +Resources: + + 3 to create + +`), autogold.Expect(auto.OutputMap{ + "effectiveLabels": auto.OutputValue{ + Value: map[string]interface{}{ + "empty": "", + "goog-pulumi-provisioned": "true", + "static": "value", + }, + Secret: true, + }, + "labels": auto.OutputValue{Value: map[string]interface{}{ + "empty": "", + "static": "value", + }}, + "pulumiLabels": auto.OutputValue{ + Value: map[string]interface{}{ + "empty": "", + "goog-pulumi-provisioned": "true", + "static": "value", + }, + Secret: true, + }, + })}, + {"empty-alone-label", autogold.Expect(`Previewing update (test): + + + pulumi:pulumi:Stack empty-alone-label-test create + + gcp:kms:KeyRing ring create + + gcp:kms:CryptoKey key create + + pulumi:pulumi:Stack empty-alone-label-test create +Outputs: + effectiveLabels: [secret] + labels : { + empty: "" + } + pulumiLabels : [secret] + +Resources: + + 3 to create + +`), autogold.Expect(auto.OutputMap{ + "effectiveLabels": auto.OutputValue{ + Value: map[string]interface{}{ + "empty": "", + "goog-pulumi-provisioned": "true", + }, + Secret: true, + }, + "labels": auto.OutputValue{Value: map[string]interface{}{"empty": ""}}, + "pulumiLabels": auto.OutputValue{ + Value: map[string]interface{}{ + "empty": "", + "goog-pulumi-provisioned": "true", + }, + Secret: true, + }, + })}, + {"empty-default-label", autogold.Expect(`Previewing update (test): + + + pulumi:pulumi:Stack empty-default-label-test create + + gcp:kms:KeyRing ring create + + gcp:kms:CryptoKey key create + + pulumi:pulumi:Stack empty-default-label-test create +Outputs: + effectiveLabels: [secret] + labels : { + static: "value" + } + pulumiLabels : [secret] + +Resources: + + 3 to create + +`), autogold.Expect(auto.OutputMap{ + "effectiveLabels": auto.OutputValue{ + Value: map[string]interface{}{ + "empty-default": "", + "goog-pulumi-provisioned": "true", + "static": "value", + }, + Secret: true, + }, + "labels": auto.OutputValue{Value: map[string]interface{}{"static": "value"}}, + "pulumiLabels": auto.OutputValue{ + Value: map[string]interface{}{ + "empty-default": "", + "goog-pulumi-provisioned": "true", + "static": "value", + }, + Secret: true, + }, + })}, + } + + for _, tt := range tests { + tt := tt + + t.Run(tt.program, func(t *testing.T) { + pt := pulumiTest(t, "test-programs/"+tt.program) + proj := getProject() + pt.SetConfig("gcpProj", proj) + + previewResult := pt.Preview(optpreview.SuppressProgress()) + tt.previewStdout.Equal(t, previewResult.StdOut) + + upResult := pt.Up(optup.SuppressProgress()) + tt.upOutputs.Equal(t, upResult.Outputs) + + pt.Preview(optpreview.ExpectNoChanges()) + + // Refresh against upstream labels + pt.Refresh() + + pt.Refresh(optrefresh.ExpectNoChanges()) + + // We should expect that we refresh cleanly + pt.Preview(optpreview.ExpectNoChanges()) + }) + } +} + +// TestUnmanagedEmptyLabels currently acts as a regression test. It ensures that behavior +// doesn't get worse. +// +// We would prefer the obvious behavior patter: +// +// $ pulumi up --yes +// $ pulumi refresh --yes # Retrieves labels from GCP +// $ pulumi preview --expect-no-changes +// +// Improvement tracked in . +func TestUnmanagedEmptyLabels(t *testing.T) { + pt := pulumiTest(t, "test-programs/empty-unmanaged-label") + proj := getProject() + pt.SetConfig("gcpProj", proj) + + previewResult := pt.Preview(optpreview.SuppressProgress()) + autogold.Expect(`Previewing update (test): + + + pulumi:pulumi:Stack dev-yaml-test create + + gcp:storage:Bucket b create + + command:local:Command set-empty-label create + + pulumi:pulumi:Stack dev-yaml-test create +Outputs: + effectiveLabels: [secret] + pulumiLabels : [secret] + +Resources: + + 3 to create + +`).Equal(t, previewResult.StdOut) + + upResult := pt.Up() + autogold.Expect(auto.OutputMap{ + "effectiveLabels": auto.OutputValue{ + Value: map[string]interface{}{"goog-pulumi-provisioned": "true"}, + Secret: true, + }, + "labels": auto.OutputValue{}, + "pulumiLabels": auto.OutputValue{ + Value: map[string]interface{}{"goog-pulumi-provisioned": "true"}, + Secret: true, + }, + }).Equal(t, upResult.Outputs) + + pt.Preview(optpreview.ExpectNoChanges()) + + // Refresh against upstream labels + pt.Refresh() // This doesn't actually include a diff, but it does change state. + + upResult = pt.Up() // This changes state, even though it should refresh cleanly. + autogold.Expect(auto.OutputMap{ + "effectiveLabels": auto.OutputValue{ + Value: map[string]interface{}{ + "goog-pulumi-provisioned": "true", + "unmanaged": "value", + "unmanaged_empty": "", + }, + Secret: true, + }, + "labels": auto.OutputValue{Value: map[string]interface{}{}}, + "pulumiLabels": auto.OutputValue{ + Value: map[string]interface{}{"goog-pulumi-provisioned": "true"}, + Secret: true, + }, + }).Equal(t, upResult.Outputs) + + // We should expect that we refresh cleanly + pt.Preview(optpreview.ExpectNoChanges()) +} diff --git a/provider/resources.go b/provider/resources.go index a988ec59ad..7072ffb25d 100644 --- a/provider/resources.go +++ b/provider/resources.go @@ -454,6 +454,9 @@ func preConfigureCallbackWithLogger(credentialsValidationRun *atomic.Bool, gcpCl //go:embed cmd/pulumi-resource-gcp/bridge-metadata.json var metadata []byte +// A predicate function that always returns true. +func always[T any](T) bool { return true } + // Provider returns additional overlaid schema and metadata associated with the gcp package. // //nolint:lll @@ -461,7 +464,8 @@ func Provider() tfbridge.ProviderInfo { p := pf.MuxShimWithDisjointgPF( context.Background(), shimv2.NewProvider(gcpProvider.Provider(), - shimv2.WithPlanResourceChange(func(_ string) bool { return true }), + shimv2.WithPlanResourceChange(always), + shimv2.WithPlanStateEdit(fixEmptyLabels), ), gcpPFProvider.New()) diff --git a/provider/test-programs/empty-alone-label/Pulumi.yaml b/provider/test-programs/empty-alone-label/Pulumi.yaml new file mode 100644 index 0000000000..cee009d383 --- /dev/null +++ b/provider/test-programs/empty-alone-label/Pulumi.yaml @@ -0,0 +1,19 @@ +name: empty-alone-label +runtime: yaml +resources: + ring: + type: gcp:kms:KeyRing + properties: + location: us-central1 + key: + type: gcp:kms:CryptoKey + properties: + keyRing: ${ring.id} + labels: + empty: "" + options: + retainOnDelete: true +outputs: + labels: ${key.labels} + effectiveLabels: ${key.effectiveLabels} + pulumiLabels: ${key.pulumiLabels} diff --git a/provider/test-programs/empty-default-label/Pulumi.yaml b/provider/test-programs/empty-default-label/Pulumi.yaml new file mode 100644 index 0000000000..7fe51309eb --- /dev/null +++ b/provider/test-programs/empty-default-label/Pulumi.yaml @@ -0,0 +1,22 @@ +name: empty-default-label +runtime: yaml +config: + gcp:defaultLabels: + value: { empty-default: "" } +resources: + ring: + type: gcp:kms:KeyRing + properties: + location: us-central1 + key: + type: gcp:kms:CryptoKey + properties: + keyRing: ${ring.id} + labels: + static: value + options: + retainOnDelete: true +outputs: + labels: ${key.labels} + effectiveLabels: ${key.effectiveLabels} + pulumiLabels: ${key.pulumiLabels} diff --git a/provider/test-programs/empty-label/Pulumi.yaml b/provider/test-programs/empty-label/Pulumi.yaml new file mode 100644 index 0000000000..a94a548864 --- /dev/null +++ b/provider/test-programs/empty-label/Pulumi.yaml @@ -0,0 +1,20 @@ +name: empty-label +runtime: yaml +resources: + ring: + type: gcp:kms:KeyRing + properties: + location: us-central1 + key: + type: gcp:kms:CryptoKey + properties: + keyRing: ${ring.id} + labels: + static: value + empty: "" + options: + retainOnDelete: true +outputs: + labels: ${key.labels} + effectiveLabels: ${key.effectiveLabels} + pulumiLabels: ${key.pulumiLabels} diff --git a/provider/test-programs/empty-unmanaged-label/Pulumi.yaml b/provider/test-programs/empty-unmanaged-label/Pulumi.yaml new file mode 100644 index 0000000000..a591790248 --- /dev/null +++ b/provider/test-programs/empty-unmanaged-label/Pulumi.yaml @@ -0,0 +1,21 @@ +name: dev-yaml +runtime: yaml + +resources: + b: + type: gcp:storage:Bucket + properties: + location: us-east1 + + set-empty-label: + type: command:local:Command + properties: + create: > + gcloud storage buckets update ${b.url} --update-labels=unmanaged=value,unmanaged_empty= + options: + retainOnDelete: true + +outputs: + labels: ${b.labels} + effectiveLabels: ${b.effectiveLabels} + pulumiLabels: ${b.pulumiLabels}