From d938fe36e3c13e2bca58f90a5103f9c1a225c013 Mon Sep 17 00:00:00 2001 From: Levi Blackstone Date: Thu, 1 Aug 2019 11:27:16 -0600 Subject: [PATCH 1/5] Handle Output values in diffs The server-side diff based on a dryRun doesn't understand Pulumi Outputs, so it would cause an error for any update that depended a calculated value. As a workaround, fallback to a client-side diff if a dryRun diff returns an error. --- CHANGELOG.md | 1 + pkg/provider/provider.go | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c12209a2b3..026493f032 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ ### Bug fixes - Properly reference override values in Python Helm SDK (https://github.com/pulumi/pulumi-kubernetes/pull/676). +- Handle Output values in diffs. (https://github.com/pulumi/pulumi-kubernetes/pull/682). ## 0.25.3 (July 29, 2019) diff --git a/pkg/provider/provider.go b/pkg/provider/provider.go index 1999701018..625b12d3bc 100644 --- a/pkg/provider/provider.go +++ b/pkg/provider/provider.go @@ -574,7 +574,8 @@ func (k *kubeProvider) Diff( var isInputPatch bool var patchBase *unstructured.Unstructured - tryDryRun := supportsDryRun && oldInputs.GroupVersionKind().String() == gvk.String() + tryDryRun := supportsDryRun && oldInputs.GroupVersionKind().String() == gvk.String() && + !hasComputedValue(newInputs) && !hasComputedValue(oldInputs) if tryDryRun { patch, patchBase, err = k.dryRunPatch(oldInputs, newInputs) From f9ed212c400385f1349bcb3750de037eda30b1a9 Mon Sep 17 00:00:00 2001 From: Levi Blackstone Date: Thu, 1 Aug 2019 12:38:27 -0600 Subject: [PATCH 2/5] Fix hasComputedValue to handle nested slices --- pkg/provider/util.go | 13 +++++++++++++ pkg/provider/util_test.go | 20 ++++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/pkg/provider/util.go b/pkg/provider/util.go index 8ecc6682b1..96e3111eb0 100644 --- a/pkg/provider/util.go +++ b/pkg/provider/util.go @@ -31,6 +31,19 @@ func hasComputedValue(obj *unstructured.Unstructured) bool { if field, isMap := v.(map[string]interface{}); isMap { objects = append(objects, field) } + if field, isSlice := v.([]interface{}); isSlice { + for _, v := range field { + if _, isComputed := v.(resource.Computed); isComputed { + return true + } + if field, isMap := v.(map[string]interface{}); isMap { + objects = append(objects, field) + } + } + } + if field, isSlice := v.([]map[string]interface{}); isSlice { + objects = append(objects, field...) + } } } diff --git a/pkg/provider/util_test.go b/pkg/provider/util_test.go index 8fc716594e..8f3890d0ab 100644 --- a/pkg/provider/util_test.go +++ b/pkg/provider/util_test.go @@ -92,6 +92,26 @@ func TestHasComputedValue(t *testing.T) { }}, hasComputedValue: true, }, + { + name: "Object with nested slice of map[string]interface{} has a computed value", + obj: &unstructured.Unstructured{Object: map[string]interface{}{ + "field1": 1, + "field2": []map[string]interface{}{ + {"field3": resource.Computed{}}, + }, + }}, + hasComputedValue: true, + }, + { + name: "Object with nested slice of interface{} has a computed value", + obj: &unstructured.Unstructured{Object: map[string]interface{}{ + "field1": 1, + "field2": []interface{}{ + resource.Computed{}, + }, + }}, + hasComputedValue: true, + }, } for _, test := range tests { From 1c9737230e882cdda707864b420ee98d430c6820 Mon Sep 17 00:00:00 2001 From: Levi Blackstone Date: Thu, 1 Aug 2019 13:47:28 -0600 Subject: [PATCH 3/5] Address feedback --- pkg/provider/provider.go | 3 +++ pkg/provider/util.go | 15 +++++++++------ pkg/provider/util_test.go | 12 ++++++++++++ 3 files changed, 24 insertions(+), 6 deletions(-) diff --git a/pkg/provider/provider.go b/pkg/provider/provider.go index 625b12d3bc..4f30468c90 100644 --- a/pkg/provider/provider.go +++ b/pkg/provider/provider.go @@ -574,6 +574,9 @@ func (k *kubeProvider) Diff( var isInputPatch bool var patchBase *unstructured.Unstructured + // TODO: Skipping dry run entirely for resources with computed values is a hack. We will want to address this + // more granularly so that previews are as accurate as possible, but this is an easy workaround for a critical + // bug. tryDryRun := supportsDryRun && oldInputs.GroupVersionKind().String() == gvk.String() && !hasComputedValue(newInputs) && !hasComputedValue(oldInputs) if tryDryRun { diff --git a/pkg/provider/util.go b/pkg/provider/util.go index 96e3111eb0..4deba08838 100644 --- a/pkg/provider/util.go +++ b/pkg/provider/util.go @@ -25,11 +25,17 @@ func hasComputedValue(obj *unstructured.Unstructured) bool { } curr, objects = objects[0], objects[1:] for _, v := range curr { - if _, isComputed := v.(resource.Computed); isComputed { + switch field := v.(type) { + case resource.Computed: return true - } - if field, isMap := v.(map[string]interface{}); isMap { + case map[string]interface{}: objects = append(objects, field) + case []interface{}: + for _, v := range field { + objects = append(objects, map[string]interface{}{"": v}) + } + case []map[string]interface{}: + objects = append(objects, field...) } if field, isSlice := v.([]interface{}); isSlice { for _, v := range field { @@ -41,9 +47,6 @@ func hasComputedValue(obj *unstructured.Unstructured) bool { } } } - if field, isSlice := v.([]map[string]interface{}); isSlice { - objects = append(objects, field...) - } } } diff --git a/pkg/provider/util_test.go b/pkg/provider/util_test.go index 8f3890d0ab..bab73e548f 100644 --- a/pkg/provider/util_test.go +++ b/pkg/provider/util_test.go @@ -112,6 +112,18 @@ func TestHasComputedValue(t *testing.T) { }}, hasComputedValue: true, }, + { + name: "Object with nested slice of map[string]interface{} with nested slice of interface{} has a computed value", + obj: &unstructured.Unstructured{Object: map[string]interface{}{ + "field1": 1, + "field2": []map[string]interface{}{ + {"field3": []interface{}{ + resource.Computed{}, + }}, + }, + }}, + hasComputedValue: true, + }, } for _, test := range tests { From 3b6c82b57cb59b0a6306b37286e961cc8ec03ede Mon Sep 17 00:00:00 2001 From: Levi Blackstone Date: Thu, 1 Aug 2019 14:19:05 -0600 Subject: [PATCH 4/5] Forgot to unused logic --- pkg/provider/util.go | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/pkg/provider/util.go b/pkg/provider/util.go index 4deba08838..08e50566ad 100644 --- a/pkg/provider/util.go +++ b/pkg/provider/util.go @@ -37,16 +37,6 @@ func hasComputedValue(obj *unstructured.Unstructured) bool { case []map[string]interface{}: objects = append(objects, field...) } - if field, isSlice := v.([]interface{}); isSlice { - for _, v := range field { - if _, isComputed := v.(resource.Computed); isComputed { - return true - } - if field, isMap := v.(map[string]interface{}); isMap { - objects = append(objects, field) - } - } - } } } From 2d9e4c6d977b0bd71c88d3b286b7667ceb9f980b Mon Sep 17 00:00:00 2001 From: Levi Blackstone Date: Thu, 1 Aug 2019 14:42:17 -0600 Subject: [PATCH 5/5] Add a couple more tests --- pkg/provider/util_test.go | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/pkg/provider/util_test.go b/pkg/provider/util_test.go index bab73e548f..01cb550cbc 100644 --- a/pkg/provider/util_test.go +++ b/pkg/provider/util_test.go @@ -124,6 +124,38 @@ func TestHasComputedValue(t *testing.T) { }}, hasComputedValue: true, }, + { + name: "Complex nested object with computed value", + obj: &unstructured.Unstructured{Object: map[string]interface{}{ + "field1": 1, + "field2": []map[string]interface{}{ + {"field3": []interface{}{ + []map[string]interface{}{ + {"field4": []interface{}{ + resource.Computed{}, + }}, + }, + }}, + }, + }}, + hasComputedValue: true, + }, + { + name: "Complex nested object with no computed value", + obj: &unstructured.Unstructured{Object: map[string]interface{}{ + "field1": 1, + "field2": []map[string]interface{}{ + {"field3": []interface{}{ + []map[string]interface{}{ + {"field4": []interface{}{ + "field5", + }}, + }, + }}, + }, + }}, + hasComputedValue: false, + }, } for _, test := range tests {