From d9e3770eb1539fb25487d04e9aa98b984d126c69 Mon Sep 17 00:00:00 2001 From: Matt Ellis Date: Thu, 23 May 2019 16:14:17 -0700 Subject: [PATCH 1/2] Fix panics during preview when `metadata` is a computed value During preview, an object's metadata bag may be computed (or be known but contain values which are computed). This could happen, for example, by using `apply` to take an output property from a yet to be created resource and use it to build part of an object's metadata, like we saw in #559. In these cases, we incorrectly panic while attempting to extract out the metadata.lables or metadata.annotations members of the metadata object, when trying to set an annotation or label. To fix this, we now treat requests to set annotations or labels as no-ops if the metadata object is computed (or the label or annotation values inside the metadata object are computed). This allows preview to continue, as expected. During a real update, we will not have computed values and so we will be able to correctly set the labels as we expected. Fixes #559 --- CHANGELOG.md | 6 +++++ pkg/metadata/annotations.go | 14 ++++++++++ pkg/metadata/annotations_test.go | 45 ++++++++++++++++++++++++++++++++ pkg/metadata/labels.go | 8 ++++++ pkg/metadata/labels_test.go | 45 ++++++++++++++++++++++++++++++++ 5 files changed, 118 insertions(+) create mode 100644 pkg/metadata/annotations_test.go create mode 100644 pkg/metadata/labels_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index efde2f4ec6..3fb6fe6705 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ ## 0.23.2 (Unreleased) +### Improvements + +- Fix an issue where the provider would panic during preview if and object's + `metadata` property ended up being computed from one or more unknown + values. (Fixes [#559](https://github.com/pulumi/pulumi-kubernetes/issues/559)). + ## 0.23.1 (May 10, 2019) ### Supported Kubernetes versions diff --git a/pkg/metadata/annotations.go b/pkg/metadata/annotations.go index 8f7b6c4c91..26486acad7 100644 --- a/pkg/metadata/annotations.go +++ b/pkg/metadata/annotations.go @@ -17,6 +17,7 @@ package metadata import ( "strings" + "github.com/pulumi/pulumi/pkg/resource" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" ) @@ -48,9 +49,17 @@ func IsInternalAnnotation(key string) bool { // SetAnnotation sets the specified key, value annotation on the provided Unstructured object. func SetAnnotation(obj *unstructured.Unstructured, key, value string) { // Note: Cannot use obj.GetAnnotations() here because it doesn't properly handle computed values from preview. + // during preview, our strategy for if metdata or annotations end up being computed values, is to just not + // apply an annotiation (since there's no way to insert data into the computed object) metadataRaw := obj.Object["metadata"] + if isComputedValue(metadataRaw) { + return + } metadata := metadataRaw.(map[string]interface{}) annotationsRaw, ok := metadata["annotations"] + if isComputedValue(annotationsRaw) { + return + } var annotations map[string]interface{} if !ok { annotations = make(map[string]interface{}) @@ -79,3 +88,8 @@ func GetAnnotationValue(obj *unstructured.Unstructured, key string) string { annotations := obj.GetAnnotations() return annotations[key] } + +func isComputedValue(v interface{}) bool { + _, isComputed := v.(resource.Computed) + return isComputed +} diff --git a/pkg/metadata/annotations_test.go b/pkg/metadata/annotations_test.go new file mode 100644 index 0000000000..058f588b4e --- /dev/null +++ b/pkg/metadata/annotations_test.go @@ -0,0 +1,45 @@ +// Copyright 2016-2019, Pulumi Corporation. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package metadata + +import ( + "testing" + + "github.com/pulumi/pulumi/pkg/resource" + + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" +) + +func TestSetAnnotationMetadataComputed(t *testing.T) { + obj := &unstructured.Unstructured{Object: map[string]interface{}{ + "metadata": resource.Computed{Element: resource.NewObjectProperty(nil)}, + }} + + // Since metadata is a computed property, we can't really set an annotation of the object, but we should not fail + // as the metadata property of an object could be computed during previews. + SetAnnotation(obj, "foo", "bar") +} + +func TestSetAnnotationMetadataAnnotationsComputed(t *testing.T) { + obj := &unstructured.Unstructured{Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "annotations": resource.Computed{Element: resource.NewObjectProperty(nil)}, + }, + }} + + // Since metadata is a computed property, we can't really set an annotation of the object, but we should not fail + // as the metadata property of an object could be computed during previews. + SetAnnotation(obj, "foo", "bar") +} diff --git a/pkg/metadata/labels.go b/pkg/metadata/labels.go index 46d168c06f..3f7e743012 100644 --- a/pkg/metadata/labels.go +++ b/pkg/metadata/labels.go @@ -21,9 +21,17 @@ import ( // SetLabel sets the specified key/value pair as a label on the provided Unstructured object. func SetLabel(obj *unstructured.Unstructured, key, value string) { // Note: Cannot use obj.GetLabels() here because it doesn't properly handle computed values from preview. + // during preview, our strategy for if metdata or annotations end up being computed values, is to just not + // apply an annotiation (since there's no way to insert data into the computed object) metadataRaw := obj.Object["metadata"] + if isComputedValue(metadataRaw) { + return + } metadata := metadataRaw.(map[string]interface{}) labelsRaw, ok := metadata["labels"] + if isComputedValue(labelsRaw) { + return + } var labels map[string]interface{} if !ok { labels = make(map[string]interface{}) diff --git a/pkg/metadata/labels_test.go b/pkg/metadata/labels_test.go new file mode 100644 index 0000000000..6dfa96712d --- /dev/null +++ b/pkg/metadata/labels_test.go @@ -0,0 +1,45 @@ +// Copyright 2016-2019, Pulumi Corporation. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package metadata + +import ( + "testing" + + "github.com/pulumi/pulumi/pkg/resource" + + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" +) + +func TestSetLabelMetadataComputed(t *testing.T) { + obj := &unstructured.Unstructured{Object: map[string]interface{}{ + "metadata": resource.Computed{Element: resource.NewObjectProperty(nil)}, + }} + + // Since metadata is a computed property, we can't really set an annotation of the object, but we should not fail + // as the metadata property of an object could be computed during previews. + SetLabel(obj, "foo", "bar") +} + +func TestSetLabelMetadataLabelsComputed(t *testing.T) { + obj := &unstructured.Unstructured{Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "labels": resource.Computed{Element: resource.NewObjectProperty(nil)}, + }, + }} + + // Since metadata is a computed property, we can't really set an annotation of the object, but we should not fail + // as the metadata property of an object could be computed during previews. + SetLabel(obj, "foo", "bar") +} From dd492a0f3a43e95243f26e16c343904dfcf40e56 Mon Sep 17 00:00:00 2001 From: Levi Blackstone Date: Wed, 29 May 2019 11:59:01 -0600 Subject: [PATCH 2/2] Add some tests and update changelog --- CHANGELOG.md | 12 +++++-- pkg/metadata/annotations.go | 4 +-- pkg/metadata/annotations_test.go | 62 ++++++++++++++++++++++++-------- pkg/metadata/labels.go | 4 +-- pkg/metadata/labels_test.go | 62 ++++++++++++++++++++++++-------- 5 files changed, 109 insertions(+), 35 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3fb6fe6705..46799ca5f7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,10 +1,16 @@ ## 0.23.2 (Unreleased) +### Major changes + +- None + ### Improvements -- Fix an issue where the provider would panic during preview if and object's - `metadata` property ended up being computed from one or more unknown - values. (Fixes [#559](https://github.com/pulumi/pulumi-kubernetes/issues/559)). +- None + +### Bug fixes + +- Fix panics during preview when `metadata` is a computed value (https://github.com/pulumi/pulumi-kubernetes/pull/572) ## 0.23.1 (May 10, 2019) diff --git a/pkg/metadata/annotations.go b/pkg/metadata/annotations.go index 26486acad7..6691a2066c 100644 --- a/pkg/metadata/annotations.go +++ b/pkg/metadata/annotations.go @@ -49,8 +49,8 @@ func IsInternalAnnotation(key string) bool { // SetAnnotation sets the specified key, value annotation on the provided Unstructured object. func SetAnnotation(obj *unstructured.Unstructured, key, value string) { // Note: Cannot use obj.GetAnnotations() here because it doesn't properly handle computed values from preview. - // during preview, our strategy for if metdata or annotations end up being computed values, is to just not - // apply an annotiation (since there's no way to insert data into the computed object) + // During preview, don't set annotations if the metadata or annotation contains a computed value since there's + // no way to insert data into the computed object. metadataRaw := obj.Object["metadata"] if isComputedValue(metadataRaw) { return diff --git a/pkg/metadata/annotations_test.go b/pkg/metadata/annotations_test.go index 058f588b4e..264768f8c7 100644 --- a/pkg/metadata/annotations_test.go +++ b/pkg/metadata/annotations_test.go @@ -18,28 +18,62 @@ import ( "testing" "github.com/pulumi/pulumi/pkg/resource" - + "github.com/stretchr/testify/assert" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" ) -func TestSetAnnotationMetadataComputed(t *testing.T) { - obj := &unstructured.Unstructured{Object: map[string]interface{}{ +func TestSetAnnotation(t *testing.T) { + noAnnotationObj := &unstructured.Unstructured{Object: map[string]interface{}{ + "metadata": map[string]interface{}{}, + }} + existingAnnotationObj := &unstructured.Unstructured{Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "annotations": map[string]interface{}{ + "pulumi": "rocks", + }, + }, + }} + computedMetadataObj := &unstructured.Unstructured{Object: map[string]interface{}{ "metadata": resource.Computed{Element: resource.NewObjectProperty(nil)}, }} - - // Since metadata is a computed property, we can't really set an annotation of the object, but we should not fail - // as the metadata property of an object could be computed during previews. - SetAnnotation(obj, "foo", "bar") -} - -func TestSetAnnotationMetadataAnnotationsComputed(t *testing.T) { - obj := &unstructured.Unstructured{Object: map[string]interface{}{ + computedAnnotationObj := &unstructured.Unstructured{Object: map[string]interface{}{ "metadata": map[string]interface{}{ "annotations": resource.Computed{Element: resource.NewObjectProperty(nil)}, }, }} - // Since metadata is a computed property, we can't really set an annotation of the object, but we should not fail - // as the metadata property of an object could be computed during previews. - SetAnnotation(obj, "foo", "bar") + type args struct { + obj *unstructured.Unstructured + key string + value string + expectSet bool // True if SetAnnotation is expected to set the annotation. + expectKey string + expectValue string + } + tests := []struct { + name string + args args + }{ + {"set-with-no-annotation", args{ + obj: noAnnotationObj, key: "foo", value: "bar", expectSet: true, expectKey: "foo", expectValue: "bar"}}, + {"set-with-existing-annotations", args{ + obj: existingAnnotationObj, key: "foo", value: "bar", expectSet: true, expectKey: "foo", expectValue: "bar"}}, + + // Computed fields cannot be set, so SetAnnotation is a no-op. + {"set-with-computed-metadata", args{ + obj: computedMetadataObj, key: "foo", value: "bar", expectSet: false}}, + {"set-with-computed-annotation", args{ + obj: computedAnnotationObj, key: "foo", value: "bar", expectSet: false}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + SetAnnotation(tt.args.obj, tt.args.key, tt.args.value) + annotations := tt.args.obj.GetAnnotations() + value, ok := annotations[tt.args.expectKey] + assert.Equal(t, tt.args.expectSet, ok) + if ok { + assert.Equal(t, tt.args.expectValue, value) + } + }) + } } diff --git a/pkg/metadata/labels.go b/pkg/metadata/labels.go index 3f7e743012..4508c557c5 100644 --- a/pkg/metadata/labels.go +++ b/pkg/metadata/labels.go @@ -21,8 +21,8 @@ import ( // SetLabel sets the specified key/value pair as a label on the provided Unstructured object. func SetLabel(obj *unstructured.Unstructured, key, value string) { // Note: Cannot use obj.GetLabels() here because it doesn't properly handle computed values from preview. - // during preview, our strategy for if metdata or annotations end up being computed values, is to just not - // apply an annotiation (since there's no way to insert data into the computed object) + // During preview, don't set labels if the metadata or label contains a computed value since there's + // no way to insert data into the computed object. metadataRaw := obj.Object["metadata"] if isComputedValue(metadataRaw) { return diff --git a/pkg/metadata/labels_test.go b/pkg/metadata/labels_test.go index 6dfa96712d..248885097f 100644 --- a/pkg/metadata/labels_test.go +++ b/pkg/metadata/labels_test.go @@ -18,28 +18,62 @@ import ( "testing" "github.com/pulumi/pulumi/pkg/resource" - + "github.com/stretchr/testify/assert" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" ) -func TestSetLabelMetadataComputed(t *testing.T) { - obj := &unstructured.Unstructured{Object: map[string]interface{}{ +func TestSetLabel(t *testing.T) { + noLabelObj := &unstructured.Unstructured{Object: map[string]interface{}{ + "metadata": map[string]interface{}{}, + }} + existingLabelObj := &unstructured.Unstructured{Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "labels": map[string]interface{}{ + "pulumi": "rocks", + }, + }, + }} + computedMetadataObj := &unstructured.Unstructured{Object: map[string]interface{}{ "metadata": resource.Computed{Element: resource.NewObjectProperty(nil)}, }} - - // Since metadata is a computed property, we can't really set an annotation of the object, but we should not fail - // as the metadata property of an object could be computed during previews. - SetLabel(obj, "foo", "bar") -} - -func TestSetLabelMetadataLabelsComputed(t *testing.T) { - obj := &unstructured.Unstructured{Object: map[string]interface{}{ + computedLabelObj := &unstructured.Unstructured{Object: map[string]interface{}{ "metadata": map[string]interface{}{ "labels": resource.Computed{Element: resource.NewObjectProperty(nil)}, }, }} - // Since metadata is a computed property, we can't really set an annotation of the object, but we should not fail - // as the metadata property of an object could be computed during previews. - SetLabel(obj, "foo", "bar") + type args struct { + obj *unstructured.Unstructured + key string + value string + expectSet bool // True if SetLabel is expected to set the label. + expectKey string + expectValue string + } + tests := []struct { + name string + args args + }{ + {"set-with-no-label", args{ + obj: noLabelObj, key: "foo", value: "bar", expectSet: true, expectKey: "foo", expectValue: "bar"}}, + {"set-with-existing-labels", args{ + obj: existingLabelObj, key: "foo", value: "bar", expectSet: true, expectKey: "foo", expectValue: "bar"}}, + + // Computed fields cannot be set, so SetLabel is a no-op. + {"set-with-computed-metadata", args{ + obj: computedMetadataObj, key: "foo", value: "bar", expectSet: false}}, + {"set-with-computed-label", args{ + obj: computedLabelObj, key: "foo", value: "bar", expectSet: false}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + SetLabel(tt.args.obj, tt.args.key, tt.args.value) + labels := tt.args.obj.GetLabels() + value, ok := labels[tt.args.expectKey] + assert.Equal(t, tt.args.expectSet, ok) + if ok { + assert.Equal(t, tt.args.expectValue, value) + } + }) + } }