Merge pull request #129257 from liggitt/coerce-labels-annotations

Coerce null label and annotation values to empty string

staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/helpers.go

@@ -188,7 +188,7 @@ func NestedSlice(obj map[string]interface{}, fields ...string) ([]interface{}, b
 // NestedStringMap returns a copy of map[string]string value of a nested field.
 // Returns false if value is not found and an error if not a map[string]interface{} or contains non-string values in the map.
 func NestedStringMap(obj map[string]interface{}, fields ...string) (map[string]string, bool, error) {
-	m, found, err := nestedMapNoCopy(obj, fields...)
+	m, found, err := nestedMapNoCopy(obj, false, fields...)
 	if !found || err != nil {
 		return nil, found, err
 	}
@@ -203,10 +203,32 @@ func NestedStringMap(obj map[string]interface{}, fields ...string) (map[string]s
 	return strMap, true, nil
 }
 
+// NestedNullCoercingStringMap returns a copy of map[string]string value of a nested field.
+// Returns `nil, true, nil` if the value exists and is explicitly null.
+// Returns `nil, false, err` if the value is not a map or a null value, or is a map and contains non-string non-null values.
+// Null values in the map are coerced to "" to match json decoding behavior.
+func NestedNullCoercingStringMap(obj map[string]interface{}, fields ...string) (map[string]string, bool, error) {
+	m, found, err := nestedMapNoCopy(obj, true, fields...)
+	if !found || err != nil || m == nil {
+		return nil, found, err
+	}
+	strMap := make(map[string]string, len(m))
+	for k, v := range m {
+		if str, ok := v.(string); ok {
+			strMap[k] = str
+		} else if v == nil {
+			strMap[k] = ""
+		} else {
+			return nil, false, fmt.Errorf("%v accessor error: contains non-string value in the map under key %q: %v is of the type %T, expected string", jsonPath(fields), k, v, v)
+		}
+	}
+	return strMap, true, nil
+}
+
 // NestedMap returns a deep copy of map[string]interface{} value of a nested field.
 // Returns false if value is not found and an error if not a map[string]interface{}.
 func NestedMap(obj map[string]interface{}, fields ...string) (map[string]interface{}, bool, error) {
-	m, found, err := nestedMapNoCopy(obj, fields...)
+	m, found, err := nestedMapNoCopy(obj, false, fields...)
 	if !found || err != nil {
 		return nil, found, err
 	}
@@ -215,11 +237,14 @@ func NestedMap(obj map[string]interface{}, fields ...string) (map[string]interfa
 
 // nestedMapNoCopy returns a map[string]interface{} value of a nested field.
 // Returns false if value is not found and an error if not a map[string]interface{}.
-func nestedMapNoCopy(obj map[string]interface{}, fields ...string) (map[string]interface{}, bool, error) {
+func nestedMapNoCopy(obj map[string]interface{}, tolerateNil bool, fields ...string) (map[string]interface{}, bool, error) {
 	val, found, err := NestedFieldNoCopy(obj, fields...)
 	if !found || err != nil {
 		return nil, found, err
 	}
+	if val == nil && tolerateNil {
+		return nil, true, nil
+	}
 	m, ok := val.(map[string]interface{})
 	if !ok {
 		return nil, false, fmt.Errorf("%v accessor error: %v is of the type %T, expected map[string]interface{}", jsonPath(fields), val, val)

staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/helpers_test.go

@@ -19,6 +19,8 @@ package unstructured
 import (
 	"io/ioutil"
 	"math"
+	"reflect"
+	"strings"
 	"sync"
 	"testing"
 
@@ -297,3 +299,90 @@ func TestNestedNumberAsFloat64(t *testing.T) {
 		})
 	}
 }
+
+func TestNestedNullCoercingStringMap(t *testing.T) {
+	for _, tc := range []struct {
+		name           string
+		obj            map[string]interface{}
+		path           []string
+		wantObj        map[string]string
+		wantFound      bool
+		wantErrMessage string
+	}{
+		{
+			name:           "missing map",
+			obj:            nil,
+			path:           []string{"path"},
+			wantObj:        nil,
+			wantFound:      false,
+			wantErrMessage: "",
+		},
+		{
+			name:           "null map",
+			obj:            map[string]interface{}{"path": nil},
+			path:           []string{"path"},
+			wantObj:        nil,
+			wantFound:      true,
+			wantErrMessage: "",
+		},
+		{
+			name:           "non map",
+			obj:            map[string]interface{}{"path": 0},
+			path:           []string{"path"},
+			wantObj:        nil,
+			wantFound:      false,
+			wantErrMessage: "type int",
+		},
+		{
+			name:           "empty map",
+			obj:            map[string]interface{}{"path": map[string]interface{}{}},
+			path:           []string{"path"},
+			wantObj:        map[string]string{},
+			wantFound:      true,
+			wantErrMessage: "",
+		},
+		{
+			name:           "string value",
+			obj:            map[string]interface{}{"path": map[string]interface{}{"a": "1", "b": "2"}},
+			path:           []string{"path"},
+			wantObj:        map[string]string{"a": "1", "b": "2"},
+			wantFound:      true,
+			wantErrMessage: "",
+		},
+		{
+			name:           "null value",
+			obj:            map[string]interface{}{"path": map[string]interface{}{"a": "1", "b": nil}},
+			path:           []string{"path"},
+			wantObj:        map[string]string{"a": "1", "b": ""},
+			wantFound:      true,
+			wantErrMessage: "",
+		},
+		{
+			name:           "invalid value",
+			obj:            map[string]interface{}{"path": map[string]interface{}{"a": "1", "b": nil, "c": 0}},
+			path:           []string{"path"},
+			wantObj:        nil,
+			wantFound:      false,
+			wantErrMessage: `key "c": 0`,
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			gotObj, gotFound, gotErr := NestedNullCoercingStringMap(tc.obj, tc.path...)
+			if !reflect.DeepEqual(gotObj, tc.wantObj) {
+				t.Errorf("got %#v, wanted %#v", gotObj, tc.wantObj)
+			}
+			if gotFound != tc.wantFound {
+				t.Errorf("got %v, wanted %v", gotFound, tc.wantFound)
+			}
+			if tc.wantErrMessage != "" {
+				if gotErr == nil {
+					t.Errorf("got nil error, wanted %s", tc.wantErrMessage)
+				} else if gotErrMessage := gotErr.Error(); !strings.Contains(gotErrMessage, tc.wantErrMessage) {
+					t.Errorf("wanted error %q, got: %v", gotErrMessage, tc.wantErrMessage)
+				}
+			} else if gotErr != nil {
+				t.Errorf("wanted nil error, got %v", gotErr)
+			}
+		})
+	}
+}

staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured.go

@@ -397,7 +397,7 @@ func (u *Unstructured) SetDeletionGracePeriodSeconds(deletionGracePeriodSeconds
 }
 
 func (u *Unstructured) GetLabels() map[string]string {
-	m, _, _ := NestedStringMap(u.Object, "metadata", "labels")
+	m, _, _ := NestedNullCoercingStringMap(u.Object, "metadata", "labels")
 	return m
 }
 
@@ -410,7 +410,7 @@ func (u *Unstructured) SetLabels(labels map[string]string) {
 }
 
 func (u *Unstructured) GetAnnotations() map[string]string {
-	m, _, _ := NestedStringMap(u.Object, "metadata", "annotations")
+	m, _, _ := NestedNullCoercingStringMap(u.Object, "metadata", "annotations")
 	return m
 }