diff --git a/plugin/pkg/admission/gc/gc_admission.go b/plugin/pkg/admission/gc/gc_admission.go index 303adc2c180..1bfeaf0a170 100644 --- a/plugin/pkg/admission/gc/gc_admission.go +++ b/plugin/pkg/admission/gc/gc_admission.go @@ -95,21 +95,26 @@ func (a *gcPermissionsEnforcement) Validate(attributes admission.Attributes) (er return nil } - deleteAttributes := authorizer.AttributesRecord{ - User: attributes.GetUserInfo(), - Verb: "delete", - Namespace: attributes.GetNamespace(), - APIGroup: attributes.GetResource().Group, - APIVersion: attributes.GetResource().Version, - Resource: attributes.GetResource().Resource, - Subresource: attributes.GetSubresource(), - Name: attributes.GetName(), - ResourceRequest: true, - 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)) + // if you are creating a thing, you should always be allowed to set an owner ref since you logically had the power + // to never create it. We still need to check block owner deletion below, because the power to delete does not + // imply the power to prevent deletion on other resources. + if attributes.GetOperation() != admission.Create { + deleteAttributes := authorizer.AttributesRecord{ + User: attributes.GetUserInfo(), + Verb: "delete", + Namespace: attributes.GetNamespace(), + APIGroup: attributes.GetResource().Group, + APIVersion: attributes.GetResource().Version, + Resource: attributes.GetResource().Resource, + Subresource: attributes.GetSubresource(), + Name: attributes.GetName(), + ResourceRequest: true, + 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 diff --git a/plugin/pkg/admission/gc/gc_admission_test.go b/plugin/pkg/admission/gc/gc_admission_test.go index 9810fa7ce5d..ccc1dd977fa 100644 --- a/plugin/pkg/admission/gc/gc_admission_test.go +++ b/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 - if tc.oldObj != nil { - 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) + t.Run(tc.name, func(t *testing.T) { + gcAdmit, err := newGCPermissionsEnforcement() + if err != nil { + t.Error(err) + } - err := gcAdmit.Validate(attributes) - if !tc.checkError(err) { - t.Errorf("%v: unexpected err: %v", tc.name, err) - } + operation := admission.Create + if tc.oldObj != nil { + 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) + } + }) } }