make gc admission set attribute namespace correctly for owners

plugin/pkg/admission/gc/gc_admission.go

@@ -186,11 +186,9 @@ func (a *gcPermissionsEnforcement) ownerRefToDeleteAttributeRecords(ref metav1.O
 		return ret, err
 	}
 	for _, mapping := range mappings {
-		ret = append(ret, authorizer.AttributesRecord{
+		ar := authorizer.AttributesRecord{
-			User: attributes.GetUserInfo(),
+			User:            attributes.GetUserInfo(),
-			Verb: "update",
+			Verb:            "update",
-			// ownerReference can only refer to an object in the same namespace, so attributes.GetNamespace() equals to the owner's namespace
-			Namespace:       attributes.GetNamespace(),
 			APIGroup:        mapping.Resource.Group,
 			APIVersion:      mapping.Resource.Version,
 			Resource:        mapping.Resource.Resource,
@@ -198,7 +196,12 @@ func (a *gcPermissionsEnforcement) ownerRefToDeleteAttributeRecords(ref metav1.O
 			Name:            ref.Name,
 			ResourceRequest: true,
 			Path:            "",
-		})
+		}
+		if mapping.Scope.Name() == meta.RESTScopeNameNamespace {
+			// if the owner is namespaced, it must be in the same namespace as the dependent is.
+			ar.Namespace = attributes.GetNamespace()
+		}
+		ret = append(ret, ar)
 	}
 	return ret, nil
 }

plugin/pkg/admission/gc/gc_admission_test.go

@@ -68,6 +68,15 @@ func (fakeAuthorizer) Authorize(a authorizer.Attributes) (authorizer.Decision, s
 		return authorizer.DecisionAllow, "", nil
 	}
 
+	if username == "non-node-deleter" {
+		if a.GetVerb() == "delete" && a.GetResource() == "nodes" {
+			return authorizer.DecisionNoOpinion, "", nil
+		}
+		if a.GetVerb() == "update" && a.GetResource() == "nodes" && a.GetSubresource() == "finalizers" {
+			return authorizer.DecisionNoOpinion, "", nil
+		}
+		return authorizer.DecisionAllow, "", nil
+	}
 	return authorizer.DecisionAllow, "", nil
 }
 
@@ -347,6 +356,23 @@ func TestBlockOwnerDeletionAdmission(t *testing.T) {
 		Name:               "ds1",
 		BlockOwnerDeletion: getFalseVar(),
 	}
+	blockNode := metav1.OwnerReference{
+		APIVersion:         "v1",
+		Kind:               "Node",
+		Name:               "node1",
+		BlockOwnerDeletion: getTrueVar(),
+	}
+	notBlockNode := metav1.OwnerReference{
+		APIVersion:         "v1",
+		Kind:               "Node",
+		Name:               "node",
+		BlockOwnerDeletion: getFalseVar(),
+	}
+	nilBlockNode := metav1.OwnerReference{
+		APIVersion: "v1",
+		Kind:       "Node",
+		Name:       "node",
+	}
 
 	expectNoError := func(err error) bool {
 		return err == nil
@@ -386,7 +412,7 @@ func TestBlockOwnerDeletionAdmission(t *testing.T) {
 			name:       "super-user, create, some ownerReferences have blockOwnerDeletion=true",
 			username:   "super",
 			resource:   api.SchemeGroupVersion.WithResource("pods"),
-			newObj:     podWithOwnerRefs(blockRC1, blockRC2),
+			newObj:     podWithOwnerRefs(blockRC1, blockRC2, blockNode),
 			checkError: expectNoError,
 		},
 		{
@@ -403,6 +429,13 @@ func TestBlockOwnerDeletionAdmission(t *testing.T) {
 			newObj:     podWithOwnerRefs(notBlockRC1, nilBlockRC2),
 			checkError: expectNoError,
 		},
+		{
+			name:       "non-node-deleter, create, all ownerReferences have blockOwnerDeletion=false",
+			username:   "non-node-deleter",
+			resource:   api.SchemeGroupVersion.WithResource("pods"),
+			newObj:     podWithOwnerRefs(notBlockNode),
+			checkError: expectNoError,
+		},
 		{
 			name:       "non-rc-deleter, create, some ownerReferences have blockOwnerDeletion=true",
 			username:   "non-rc-deleter",
@@ -417,21 +450,28 @@ func TestBlockOwnerDeletionAdmission(t *testing.T) {
 			newObj:     podWithOwnerRefs(blockDS1),
 			checkError: expectNoError,
 		},
+		{
+			name:       "non-node-deleter, create, some ownerReferences have blockOwnerDeletion=true",
+			username:   "non-node-deleter",
+			resource:   api.SchemeGroupVersion.WithResource("pods"),
+			newObj:     podWithOwnerRefs(blockNode),
+			checkError: expectCantSetBlockOwnerDeletionError,
+		},
 		// cases are for update
 		{
 			name:       "super-user, update, no ownerReferences change blockOwnerDeletion",
 			username:   "super",
 			resource:   api.SchemeGroupVersion.WithResource("pods"),
-			oldObj:     podWithOwnerRefs(nilBlockRC1),
+			oldObj:     podWithOwnerRefs(nilBlockRC1, nilBlockNode),
-			newObj:     podWithOwnerRefs(notBlockRC1),
+			newObj:     podWithOwnerRefs(notBlockRC1, notBlockNode),
 			checkError: expectNoError,
 		},
 		{
 			name:       "super-user, update, some ownerReferences change to blockOwnerDeletion=true",
 			username:   "super",
 			resource:   api.SchemeGroupVersion.WithResource("pods"),
-			oldObj:     podWithOwnerRefs(notBlockRC1),
+			oldObj:     podWithOwnerRefs(notBlockRC1, notBlockNode),
-			newObj:     podWithOwnerRefs(blockRC1),
+			newObj:     podWithOwnerRefs(blockRC1, blockNode),
 			checkError: expectNoError,
 		},
 		{
@@ -439,7 +479,7 @@ func TestBlockOwnerDeletionAdmission(t *testing.T) {
 			username:   "super",
 			resource:   api.SchemeGroupVersion.WithResource("pods"),
 			oldObj:     podWithOwnerRefs(),
-			newObj:     podWithOwnerRefs(blockRC1),
+			newObj:     podWithOwnerRefs(blockRC1, blockNode),
 			checkError: expectNoError,
 		},
 		{
@@ -466,6 +506,14 @@ func TestBlockOwnerDeletionAdmission(t *testing.T) {
 			newObj:     podWithOwnerRefs(blockRC1),
 			checkError: expectCantSetBlockOwnerDeletionError,
 		},
+		{
+			name:       "non-node-deleter, update, some ownerReferences change from blockOwnerDeletion=nil to true",
+			username:   "non-node-deleter",
+			resource:   api.SchemeGroupVersion.WithResource("pods"),
+			oldObj:     podWithOwnerRefs(nilBlockNode),
+			newObj:     podWithOwnerRefs(blockNode),
+			checkError: expectCantSetBlockOwnerDeletionError,
+		},
 		{
 			name:       "non-rc-deleter, update, some ownerReferences change from blockOwnerDeletion=true to false",
 			username:   "non-rc-deleter",
@@ -474,6 +522,14 @@ func TestBlockOwnerDeletionAdmission(t *testing.T) {
 			newObj:     podWithOwnerRefs(notBlockRC1),
 			checkError: expectNoError,
 		},
+		{
+			name:       "non-node-deleter, update, some ownerReferences change from blockOwnerDeletion=true to false",
+			username:   "non-node-deleter",
+			resource:   api.SchemeGroupVersion.WithResource("pods"),
+			oldObj:     podWithOwnerRefs(blockNode),
+			newObj:     podWithOwnerRefs(notBlockNode),
+			checkError: expectNoError,
+		},
 		{
 			name:       "non-rc-deleter, update, some ownerReferences change blockOwnerDeletion, but all such references are to daemonset",
 			username:   "non-rc-deleter",
@@ -506,6 +562,14 @@ func TestBlockOwnerDeletionAdmission(t *testing.T) {
 			newObj:     podWithOwnerRefs(blockDS1),
 			checkError: expectNoError,
 		},
+		{
+			name:       "non-node-deleter, update, add ownerReferences with blockOwnerDeletion=true",
+			username:   "non-node-deleter",
+			resource:   api.SchemeGroupVersion.WithResource("pods"),
+			oldObj:     podWithOwnerRefs(),
+			newObj:     podWithOwnerRefs(blockNode),
+			checkError: expectCantSetBlockOwnerDeletionError,
+		},
 	}
 	gcAdmit, err := newGCPermissionsEnforcement()
 	if err != nil {