don't block creation on lack of delete powers

plugin/pkg/admission/gc/gc_admission.go

@@ -95,21 +95,26 @@ func (a *gcPermissionsEnforcement) Validate(attributes admission.Attributes) (er
 		return nil
 	}
 
-	deleteAttributes := authorizer.AttributesRecord{
+	// if you are creating a thing, you should always be allowed to set an owner ref since you logically had the power
-		User:            attributes.GetUserInfo(),
+	// to never create it.  We still need to check block owner deletion below, because the power to delete does not
-		Verb:            "delete",
+	// imply the power to prevent deletion on other resources.
-		Namespace:       attributes.GetNamespace(),
+	if attributes.GetOperation() != admission.Create {
-		APIGroup:        attributes.GetResource().Group,
+		deleteAttributes := authorizer.AttributesRecord{
-		APIVersion:      attributes.GetResource().Version,
+			User:            attributes.GetUserInfo(),
-		Resource:        attributes.GetResource().Resource,
+			Verb:            "delete",
-		Subresource:     attributes.GetSubresource(),
+			Namespace:       attributes.GetNamespace(),
-		Name:            attributes.GetName(),
+			APIGroup:        attributes.GetResource().Group,
-		ResourceRequest: true,
+			APIVersion:      attributes.GetResource().Version,
-		Path:            "",
+			Resource:        attributes.GetResource().Resource,
-	}
+			Subresource:     attributes.GetSubresource(),
-	decision, reason, err := a.authorizer.Authorize(deleteAttributes)
+			Name:            attributes.GetName(),
-	if decision != authorizer.DecisionAllow {
+			ResourceRequest: true,
-		return admission.NewForbidden(attributes, fmt.Errorf("cannot set an ownerRef on a resource you can't delete: %v, %v", reason, err))
+			Path:            "",
+		}
+		decision, reason, err := a.authorizer.Authorize(deleteAttributes)
+		if decision != authorizer.DecisionAllow {
+			return admission.NewForbidden(attributes, fmt.Errorf("cannot set an ownerRef on a resource you can't delete: %v, %v", reason, err))
+		}
 	}
 
 	// Further check if the user is setting ownerReference.blockOwnerDeletion to
@@ -119,7 +124,7 @@ func (a *gcPermissionsEnforcement) Validate(attributes admission.Attributes) (er
 	for _, ref := range newBlockingRefs {
 		records, err := a.ownerRefToDeleteAttributeRecords(ref, attributes)
 		if err != nil {
-			return admission.NewForbidden(attributes, fmt.Errorf("cannot set blockOwnerDeletion in this case because cannot find RESTMapping for APIVersion %s Kind %s: %v, %v", ref.APIVersion, ref.Kind, reason, err))
+			return admission.NewForbidden(attributes, fmt.Errorf("cannot set blockOwnerDeletion in this case because cannot find RESTMapping for APIVersion %s Kind %s: %v", ref.APIVersion, ref.Kind, err))
 		}
 		// Multiple records are returned if ref.Kind could map to multiple
 		// resources. User needs to have delete permission on all the

plugin/pkg/admission/gc/gc_admission_test.go

@@ -102,6 +102,9 @@ func TestGCAdmission(t *testing.T) {
 		return err == nil
 	}
 	expectCantSetOwnerRefError := func(err error) bool {
+		if err == nil {
+			return false
+		}
 		return strings.Contains(err.Error(), "cannot set an ownerRef on a resource you can't delete")
 	}
 	tests := []struct {
@@ -140,7 +143,7 @@ func TestGCAdmission(t *testing.T) {
 			username:   "non-deleter",
 			resource:   api.SchemeGroupVersion.WithResource("pods"),
 			newObj:     &api.Pod{ObjectMeta: metav1.ObjectMeta{OwnerReferences: []metav1.OwnerReference{{Name: "first"}}}},
-			checkError: expectCantSetOwnerRefError,
+			checkError: expectNoError,
 		},
 		{
 			name:       "non-pod-deleter, create, no objectref change",
@@ -154,7 +157,7 @@ func TestGCAdmission(t *testing.T) {
 			username:   "non-pod-deleter",
 			resource:   api.SchemeGroupVersion.WithResource("pods"),
 			newObj:     &api.Pod{ObjectMeta: metav1.ObjectMeta{OwnerReferences: []metav1.OwnerReference{{Name: "first"}}}},
-			checkError: expectCantSetOwnerRefError,
+			checkError: expectNoError,
 		},
 		{
 			name:       "non-pod-deleter, create, objectref change, but not a pod",
@@ -254,23 +257,26 @@ func TestGCAdmission(t *testing.T) {
 			checkError: expectNoError,
 		},
 	}
-	gcAdmit, err := newGCPermissionsEnforcement()
-	if err != nil {
-		t.Error(err)
-	}
 
 	for _, tc := range tests {
-		operation := admission.Create
+		t.Run(tc.name, func(t *testing.T) {
-		if tc.oldObj != nil {
+			gcAdmit, err := newGCPermissionsEnforcement()
-			operation = admission.Update
+			if err != nil {
-		}
+				t.Error(err)
-		user := &user.DefaultInfo{Name: tc.username}
+			}
-		attributes := admission.NewAttributesRecord(tc.newObj, tc.oldObj, schema.GroupVersionKind{}, metav1.NamespaceDefault, "foo", tc.resource, tc.subresource, operation, user)
 
-		err := gcAdmit.Validate(attributes)
+			operation := admission.Create
-		if !tc.checkError(err) {
+			if tc.oldObj != nil {
-			t.Errorf("%v: unexpected err: %v", tc.name, err)
+				operation = admission.Update
-		}
+			}
+			user := &user.DefaultInfo{Name: tc.username}
+			attributes := admission.NewAttributesRecord(tc.newObj, tc.oldObj, schema.GroupVersionKind{}, metav1.NamespaceDefault, "foo", tc.resource, tc.subresource, operation, user)
+
+			err = gcAdmit.Validate(attributes)
+			if !tc.checkError(err) {
+				t.Errorf("unexpected err: %v", err)
+			}
+		})
 	}
 }