Fix Unstructured field accessor

Fix the Unstructured GetDeletionGracePeriodSeconds accessor which was
always returning nil regardless of the underlying stored value. The
field value always appearing nil prevents Custom Resource instances
from being deleted when garbage collection is enabled for CRs and
when DeletePropagationOrphan is used. More generally, this fix means that
delete-on-update now works for CR instances.

Add some test coverage for Unstructured metadata deserialization.

The Unstructured DeletionGracePeriodSeconds field marshals as a value
type from JSON and as a pointer type via SetDeletionGracePeriodSeconds.
The GetDeletionGracePeriodSeconds method now supports handling both
int64 and *int64 values so that either underlying value can be returned.

Add a reflection-based unit test which attempts to exercise all the
Object Get/Set methods for nil handling.

pkg/apimachinery/tests/apis_meta_v1_unstructed_unstructure_test.go

@@ -87,13 +87,13 @@ func TestDecode(t *testing.T) {
 			},
 		},
 		{
-			json: []byte(`{"items": [{"metadata": {"name": "object1"}, "apiVersion": "test", "kind": "test_kind"}, {"metadata": {"name": "object2"}, "apiVersion": "test", "kind": "test_kind"}], "apiVersion": "test", "kind": "test_list"}`),
+			json: []byte(`{"items": [{"metadata": {"name": "object1", "deletionGracePeriodSeconds": 10}, "apiVersion": "test", "kind": "test_kind"}, {"metadata": {"name": "object2"}, "apiVersion": "test", "kind": "test_kind"}], "apiVersion": "test", "kind": "test_list"}`),
 			want: &unstructured.UnstructuredList{
 				Object: map[string]interface{}{"apiVersion": "test", "kind": "test_list"},
 				Items: []unstructured.Unstructured{
 					{
 						Object: map[string]interface{}{
-							"metadata":   map[string]interface{}{"name": "object1"},
+							"metadata":   map[string]interface{}{"name": "object1", "deletionGracePeriodSeconds": int64(10)},
 							"apiVersion": "test",
 							"kind":       "test_kind",
 						},
@@ -125,19 +125,22 @@ func TestDecode(t *testing.T) {
 
 func TestUnstructuredGetters(t *testing.T) {
 	trueVar := true
+	ten := int64(10)
 	unstruct := unstructured.Unstructured{
 		Object: map[string]interface{}{
 			"kind":       "test_kind",
 			"apiVersion": "test_version",
 			"metadata": map[string]interface{}{
-				"name":              "test_name",
+				"name":                       "test_name",
-				"namespace":         "test_namespace",
+				"namespace":                  "test_namespace",
-				"generateName":      "test_generateName",
+				"generateName":               "test_generateName",
-				"uid":               "test_uid",
+				"uid":                        "test_uid",
-				"resourceVersion":   "test_resourceVersion",
+				"resourceVersion":            "test_resourceVersion",
-				"selfLink":          "test_selfLink",
+				"generation":                 ten,
-				"creationTimestamp": "2009-11-10T23:00:00Z",
+				"deletionGracePeriodSeconds": ten,
-				"deletionTimestamp": "2010-11-10T23:00:00Z",
+				"selfLink":                   "test_selfLink",
+				"creationTimestamp":          "2009-11-10T23:00:00Z",
+				"deletionTimestamp":          "2010-11-10T23:00:00Z",
 				"labels": map[string]interface{}{
 					"test_label": "test_value",
 				},
@@ -244,25 +247,34 @@ func TestUnstructuredGetters(t *testing.T) {
 	if got, want := unstruct.GetClusterName(), "cluster123"; got != want {
 		t.Errorf("GetClusterName()=%v, want %v", got, want)
 	}
+	if got, want := unstruct.GetDeletionGracePeriodSeconds(), &ten; !reflect.DeepEqual(got, want) {
+		t.Errorf("GetDeletionGracePeriodSeconds()=%v, want %v", got, want)
+	}
+	if got, want := unstruct.GetGeneration(), ten; !reflect.DeepEqual(got, want) {
+		t.Errorf("GetGeneration()=%v, want %v", got, want)
+	}
 }
 
 func TestUnstructuredSetters(t *testing.T) {
 	unstruct := unstructured.Unstructured{}
 	trueVar := true
+	ten := int64(10)
 
 	want := unstructured.Unstructured{
 		Object: map[string]interface{}{
 			"kind":       "test_kind",
 			"apiVersion": "test_version",
 			"metadata": map[string]interface{}{
-				"name":              "test_name",
+				"name":                       "test_name",
-				"namespace":         "test_namespace",
+				"namespace":                  "test_namespace",
-				"generateName":      "test_generateName",
+				"generateName":               "test_generateName",
-				"uid":               "test_uid",
+				"uid":                        "test_uid",
-				"resourceVersion":   "test_resourceVersion",
+				"resourceVersion":            "test_resourceVersion",
-				"selfLink":          "test_selfLink",
+				"selfLink":                   "test_selfLink",
-				"creationTimestamp": "2009-11-10T23:00:00Z",
+				"creationTimestamp":          "2009-11-10T23:00:00Z",
-				"deletionTimestamp": "2010-11-10T23:00:00Z",
+				"deletionTimestamp":          "2010-11-10T23:00:00Z",
+				"deletionGracePeriodSeconds": &ten,
+				"generation":                 ten,
 				"labels": map[string]interface{}{
 					"test_label": "test_value",
 				},
@@ -326,6 +338,8 @@ func TestUnstructuredSetters(t *testing.T) {
 	unstruct.SetOwnerReferences(newOwnerReferences)
 	unstruct.SetFinalizers([]string{"finalizer.1", "finalizer.2"})
 	unstruct.SetClusterName("cluster123")
+	unstruct.SetDeletionGracePeriodSeconds(&ten)
+	unstruct.SetGeneration(ten)
 
 	if !reflect.DeepEqual(unstruct, want) {
 		t.Errorf("Wanted: \n%s\n Got:\n%s", want, unstruct)
@@ -493,3 +507,72 @@ func TestDecodeNumbers(t *testing.T) {
 		t.Fatalf("Expected\n\t%#v, got \n\t%#v", pod, pod2)
 	}
 }
+
+// TestAccessorMethods does opaque roundtrip testing against an Unstructured
+// instance's Object methods to ensure that what is "Set" matches what you
+// subsequently "Get" without any assertions against internal state.
+func TestAccessorMethods(t *testing.T) {
+	int64p := func(i int) *int64 {
+		v := int64(i)
+		return &v
+	}
+	tests := []struct {
+		accessor string
+		val      interface{}
+		nilVal   reflect.Value
+	}{
+		{accessor: "Namespace", val: "foo"},
+		{accessor: "Name", val: "bar"},
+		{accessor: "GenerateName", val: "baz"},
+		{accessor: "UID", val: types.UID("uid")},
+		{accessor: "ResourceVersion", val: "1"},
+		{accessor: "Generation", val: int64(5)},
+		{accessor: "SelfLink", val: "/foo"},
+		// TODO: Handle timestamps, which are being marshalled as UTC and
+		// unmarshalled as Local.
+		// https://github.com/kubernetes/kubernetes/issues/21402
+		// {accessor: "CreationTimestamp", val: someTime},
+		// {accessor: "DeletionTimestamp", val: someTimeP},
+		{accessor: "DeletionTimestamp", nilVal: reflect.ValueOf((*metav1.Time)(nil))},
+		{accessor: "DeletionGracePeriodSeconds", val: int64p(10)},
+		{accessor: "DeletionGracePeriodSeconds", val: int64p(0)},
+		{accessor: "DeletionGracePeriodSeconds", nilVal: reflect.ValueOf((*int64)(nil))},
+		{accessor: "Labels", val: map[string]string{"foo": "bar"}},
+		{accessor: "Annotations", val: map[string]string{"foo": "bar"}},
+		{accessor: "Initializers", val: &metav1.Initializers{Pending: []metav1.Initializer{{Name: "foo"}}}},
+		{accessor: "Initializers", val: &metav1.Initializers{}},
+		{accessor: "Initializers", nilVal: reflect.ValueOf((*metav1.Initializers)(nil))},
+		{accessor: "Finalizers", val: []string{"foo"}},
+		{accessor: "OwnerReferences", val: []metav1.OwnerReference{{Name: "foo"}}},
+		{accessor: "ClusterName", val: "foo"},
+	}
+	for i, test := range tests {
+		t.Logf("evaluating test %d (%s)", i, test.accessor)
+
+		u := &unstructured.Unstructured{}
+		setter := reflect.ValueOf(u).MethodByName("Set" + test.accessor)
+		getter := reflect.ValueOf(u).MethodByName("Get" + test.accessor)
+
+		args := []reflect.Value{}
+		if test.val != nil {
+			args = append(args, reflect.ValueOf(test.val))
+		} else {
+			args = append(args, test.nilVal)
+		}
+		setter.Call(args)
+
+		ret := getter.Call([]reflect.Value{})
+		actual := ret[0].Interface()
+
+		var expected interface{}
+		if test.val != nil {
+			expected = test.val
+		} else {
+			expected = test.nilVal.Interface()
+		}
+
+		if e, a := expected, actual; !reflect.DeepEqual(e, a) {
+			t.Fatalf("%s: expected %v (%T), got %v (%T)", test.accessor, e, e, a, a)
+		}
+	}
+}

staging/src/k8s.io/apiextensions-apiserver/test/integration/finalization_test.go

@@ -69,7 +69,8 @@ func TestFinalization(t *testing.T) {
 	require.NoError(t, err)
 
 	// Removing the finalizers to allow the following delete remove the object.
-	// This step will fail if previous delete wrongly removed the object.
+	// This step will fail if previous delete wrongly removed the object. The
+	// object will be deleted as part of the finalizer update.
 	for {
 		gottenNoxuInstance.SetFinalizers(nil)
 		_, err = noxuResourceClient.Update(gottenNoxuInstance)
@@ -83,14 +84,6 @@ func TestFinalization(t *testing.T) {
 		require.NoError(t, err)
 	}
 
-	// Now when finalizer is not there it should be possible to actually remove the object from the server.
-	err = noxuResourceClient.Delete(name, &metav1.DeleteOptions{
-		Preconditions: &metav1.Preconditions{
-			UID: &uid,
-		},
-	})
-	require.NoError(t, err)
-
 	// Check that the object is actually gone.
 	_, err = noxuResourceClient.Get(name, metav1.GetOptions{})
 	require.Error(t, err)

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

@@ -136,10 +136,15 @@ func getNestedInt64(obj map[string]interface{}, fields ...string) int64 {
 }
 
 func getNestedInt64Pointer(obj map[string]interface{}, fields ...string) *int64 {
-	if str, ok := getNestedField(obj, fields...).(*int64); ok {
+	nested := getNestedField(obj, fields...)
-		return str
+	switch n := nested.(type) {
+	case int64:
+		return &n
+	case *int64:
+		return n
+	default:
+		return nil
 	}
-	return nil
 }
 
 func getNestedSlice(obj map[string]interface{}, fields ...string) []string {
@@ -470,6 +475,9 @@ func (u *Unstructured) GetInitializers() *metav1.Initializers {
 }
 
 func (u *Unstructured) SetInitializers(initializers *metav1.Initializers) {
+	if u.Object == nil {
+		u.Object = make(map[string]interface{})
+	}
 	if initializers == nil {
 		setNestedField(u.Object, nil, "metadata", "initializers")
 		return