Fixes from review on 2016-12-13

Author: Miciah

pkg/authorization/admission/restrictusers/restrictusers.go

@@ -85,10 +85,6 @@ func objectReferenceDelta(elementsToIgnore, elements []kapi.ObjectReference) []k
 // each subject in the binding must be matched by some rolebinding restriction
 // in the namespace.
 func (q *restrictUsersAdmission) Admit(a admission.Attributes) (err error) {
-	if !q.groupCache.Running() {
-		return admission.NewForbidden(a, errors.New("groupCache not running"))
-	}
-
 	// Ignore all operations that correspond to subresource actions.
 	if len(a.GetSubresource()) != 0 {
 		return nil
@@ -157,6 +153,9 @@ func (q *restrictUsersAdmission) Admit(a admission.Attributes) (err error) {
 
 		glog.V(4).Infof("Handling policybinding %s/%s",
 			policybinding.Namespace, policybinding.Name)
+
+	default:
+		return nil
 	}
 
 	newSubjects := objectReferenceDelta(oldSubjects, subjects)
@@ -176,6 +175,10 @@ func (q *restrictUsersAdmission) Admit(a admission.Attributes) (err error) {
 		return nil
 	}
 
+	if !q.groupCache.Running() {
+		return admission.NewForbidden(a, errors.New("groupCache not running"))
+	}
+
 	checkers := []SubjectChecker{}
 	for _, rbr := range roleBindingRestrictionList.Items {
 		checker, err := NewSubjectChecker(&rbr.Spec)
@@ -185,17 +188,14 @@ func (q *restrictUsersAdmission) Admit(a admission.Attributes) (err error) {
 		checkers = append(checkers, checker)
 	}
 
-	checker, err := NewUnionSubjectChecker(checkers)
-	if err != nil {
-		return admission.NewForbidden(a, err)
-	}
-
 	roleBindingRestrictionContext, err := NewRoleBindingRestrictionContext(ns,
 		q.kclient, q.oclient, q.groupCache)
 	if err != nil {
 		return admission.NewForbidden(a, err)
 	}
 
+	checker := NewUnionSubjectChecker(checkers)
+
 	errs := []error{}
 	for _, subject := range newSubjects {
 		allowed, err := checker.Allowed(subject, roleBindingRestrictionContext)

pkg/authorization/admission/restrictusers/restrictusers_test.go

@@ -30,19 +30,17 @@ func TestAdmission(t *testing.T) {
 			},
 		}
 		userAliceRef = kapi.ObjectReference{
-			Kind:      authorizationapi.UserKind,
-			Namespace: "namespace",
-			Name:      "Alice",
+			Kind: authorizationapi.UserKind,
+			Name: "Alice",
 		}
 
 		userBob = userapi.User{
 			ObjectMeta: kapi.ObjectMeta{Name: "Bob"},
 			Groups:     []string{"group"},
 		}
 		userBobRef = kapi.ObjectReference{
-			Kind:      authorizationapi.UserKind,
-			Namespace: "namespace",
-			Name:      "Bob",
+			Kind: authorizationapi.UserKind,
+			Name: "Bob",
 		}
 
 		group = userapi.Group{
@@ -53,9 +51,8 @@ func TestAdmission(t *testing.T) {
 			Users: []string{userBobRef.Name},
 		}
 		groupRef = kapi.ObjectReference{
-			Kind:      authorizationapi.GroupKind,
-			Namespace: "namespace",
-			Name:      "group",
+			Kind: authorizationapi.GroupKind,
+			Name: "group",
 		}
 
 		serviceaccount = kapi.ServiceAccount{
@@ -72,14 +69,12 @@ func TestAdmission(t *testing.T) {
 		}
 
 		systemuserRef = kapi.ObjectReference{
-			Kind:      authorizationapi.SystemUserKind,
-			Namespace: "namespace",
-			Name:      "system user",
+			Kind: authorizationapi.SystemUserKind,
+			Name: "system user",
 		}
 		systemgroupRef = kapi.ObjectReference{
-			Kind:      authorizationapi.SystemGroupKind,
-			Namespace: "namespace",
-			Name:      "system group",
+			Kind: authorizationapi.SystemGroupKind,
+			Name: "system group",
 		}
 	)
 

pkg/authorization/admission/restrictusers/subjectchecker.go

@@ -8,6 +8,7 @@ import (
 	kapi "k8s.io/kubernetes/pkg/api"
 	"k8s.io/kubernetes/pkg/api/unversioned"
 	"k8s.io/kubernetes/pkg/labels"
+	kerrors "k8s.io/kubernetes/pkg/util/errors"
 
 	authorizationapi "github.com/openshift/origin/pkg/authorization/api"
 	oclient "github.com/openshift/origin/pkg/client"
@@ -25,25 +26,24 @@ type SubjectChecker interface {
 type UnionSubjectChecker []SubjectChecker
 
 // NewUnionSubjectChecker returns a new UnionSubjectChecker.
-func NewUnionSubjectChecker(checkers []SubjectChecker) (UnionSubjectChecker, error) {
-	return UnionSubjectChecker(checkers), nil
+func NewUnionSubjectChecker(checkers []SubjectChecker) UnionSubjectChecker {
+	return UnionSubjectChecker(checkers)
 }
 
 // Allowed determines whether the given subject is allowed in rolebindings in
 // the project.
-func (checkers UnionSubjectChecker) Allowed(subject kapi.ObjectReference,
-	ctx *RoleBindingRestrictionContext) (bool, error) {
+func (checkers UnionSubjectChecker) Allowed(subject kapi.ObjectReference, ctx *RoleBindingRestrictionContext) (bool, error) {
+	errs := []error{}
 	for _, checker := range []SubjectChecker(checkers) {
 		allowed, err := checker.Allowed(subject, ctx)
 		if err != nil {
-			return false, err
-		}
-		if allowed {
-			return true, nil
+			errs = append(errs, err)
+		} else if allowed {
+			return true, kerrors.NewAggregate(errs)
 		}
 	}
 
-	return false, nil
+	return false, kerrors.NewAggregate(errs)
 }
 
 // RoleBindingRestrictionContext holds context that is used when determining
@@ -68,9 +68,7 @@ type RoleBindingRestrictionContext struct {
 
 // NewRoleBindingRestrictionContext returns a new RoleBindingRestrictionContext
 // object.
-func NewRoleBindingRestrictionContext(ns string, kc kclientset.Interface,
-	oc oclient.Interface,
-	groupCache *usercache.GroupCache) (*RoleBindingRestrictionContext, error) {
+func NewRoleBindingRestrictionContext(ns string, kc kclientset.Interface, oc oclient.Interface, groupCache *usercache.GroupCache) (*RoleBindingRestrictionContext, error) {
 	return &RoleBindingRestrictionContext{
 		namespace:       ns,
 		kclient:         kc,
@@ -151,8 +149,8 @@ type UserSubjectChecker struct {
 }
 
 // NewUserSubjectChecker returns a new UserSubjectChecker.
-func NewUserSubjectChecker(userRestriction *authorizationapi.UserRestriction) (UserSubjectChecker, error) {
-	return UserSubjectChecker{userRestriction: userRestriction}, nil
+func NewUserSubjectChecker(userRestriction *authorizationapi.UserRestriction) UserSubjectChecker {
+	return UserSubjectChecker{userRestriction: userRestriction}
 }
 
 // Allowed determines whether the given user subject is allowed in rolebindings
@@ -217,8 +215,8 @@ type GroupSubjectChecker struct {
 }
 
 // NewGroupSubjectChecker returns a new GroupSubjectChecker.
-func NewGroupSubjectChecker(groupRestriction *authorizationapi.GroupRestriction) (GroupSubjectChecker, error) {
-	return GroupSubjectChecker{groupRestriction: groupRestriction}, nil
+func NewGroupSubjectChecker(groupRestriction *authorizationapi.GroupRestriction) GroupSubjectChecker {
+	return GroupSubjectChecker{groupRestriction: groupRestriction}
 }
 
 // Allowed determines whether the given group subject is allowed in rolebindings
@@ -268,10 +266,10 @@ type ServiceAccountSubjectChecker struct {
 }
 
 // NewServiceAccountSubjectChecker returns a new ServiceAccountSubjectChecker.
-func NewServiceAccountSubjectChecker(serviceAccountRestriction *authorizationapi.ServiceAccountRestriction) (ServiceAccountSubjectChecker, error) {
+func NewServiceAccountSubjectChecker(serviceAccountRestriction *authorizationapi.ServiceAccountRestriction) ServiceAccountSubjectChecker {
 	return ServiceAccountSubjectChecker{
 		serviceAccountRestriction: serviceAccountRestriction,
-	}, nil
+	}
 }
 
 // Allowed determines whether the given serviceaccount subject is allowed in
@@ -306,13 +304,13 @@ func (checker ServiceAccountSubjectChecker) Allowed(subject kapi.ObjectReference
 func NewSubjectChecker(spec *authorizationapi.RoleBindingRestrictionSpec) (SubjectChecker, error) {
 	switch {
 	case spec.UserRestriction != nil:
-		return NewUserSubjectChecker(spec.UserRestriction)
+		return NewUserSubjectChecker(spec.UserRestriction), nil
 
 	case spec.GroupRestriction != nil:
-		return NewGroupSubjectChecker(spec.GroupRestriction)
+		return NewGroupSubjectChecker(spec.GroupRestriction), nil
 
 	case spec.ServiceAccountRestriction != nil:
-		return NewServiceAccountSubjectChecker(spec.ServiceAccountRestriction)
+		return NewServiceAccountSubjectChecker(spec.ServiceAccountRestriction), nil
 	}
 
 	return nil, fmt.Errorf("invalid RoleBindingRestrictionSpec: %v", spec)

pkg/authorization/admission/restrictusers/subjectchecker_test.go

@@ -27,34 +27,29 @@ func mustNewSubjectChecker(t *testing.T, spec *authorizationapi.RoleBindingRestr
 func TestSubjectCheckers(t *testing.T) {
 	var (
 		userBobRef = kapi.ObjectReference{
-			Kind:      authorizationapi.UserKind,
-			Namespace: "namespace",
-			Name:      "Bob",
+			Kind: authorizationapi.UserKind,
+			Name: "Bob",
 		}
 		userAliceRef = kapi.ObjectReference{
-			Kind:      authorizationapi.UserKind,
-			Namespace: "namespace",
-			Name:      "Alice",
+			Kind: authorizationapi.UserKind,
+			Name: "Alice",
 		}
 		groupRef = kapi.ObjectReference{
-			Kind:      authorizationapi.GroupKind,
-			Namespace: "namespace",
-			Name:      "group",
+			Kind: authorizationapi.GroupKind,
+			Name: "group",
 		}
 		serviceaccountRef = kapi.ObjectReference{
 			Kind:      authorizationapi.ServiceAccountKind,
 			Namespace: "namespace",
 			Name:      "serviceaccount",
 		}
 		systemuserRef = kapi.ObjectReference{
-			Kind:      authorizationapi.SystemUserKind,
-			Namespace: "namespace",
-			Name:      "system user",
+			Kind: authorizationapi.SystemUserKind,
+			Name: "system user",
 		}
 		systemgroupRef = kapi.ObjectReference{
-			Kind:      authorizationapi.SystemGroupKind,
-			Namespace: "namespace",
-			Name:      "system group",
+			Kind: authorizationapi.SystemGroupKind,
+			Name: "system group",
 		}
 		group = userapi.Group{
 			ObjectMeta: kapi.ObjectMeta{
@@ -249,7 +244,7 @@ func TestSubjectCheckers(t *testing.T) {
 		if groups, _ := groupCache.GroupsFor(group.Users[0]); len(groups) == 1 {
 			break
 		}
-		time.Sleep(time.Millisecond)
+		time.Sleep(10 * time.Millisecond)
 	}
 
 	ctx, err := NewRoleBindingRestrictionContext("namespace",

pkg/authorization/api/register.go

@@ -62,6 +62,3 @@ func addKnownTypes(scheme *runtime.Scheme) error {
 	)
 	return nil
 }
-
-func (obj *RoleBindingRestrictionList) GetObjectKind() unversioned.ObjectKind { return &obj.TypeMeta }
-func (obj *RoleBindingRestriction) GetObjectKind() unversioned.ObjectKind     { return &obj.TypeMeta }

pkg/authorization/api/v1/register.go

@@ -1,7 +1,6 @@
 package v1
 
 import (
-	"k8s.io/kubernetes/pkg/api/meta"
 	"k8s.io/kubernetes/pkg/api/unversioned"
 	"k8s.io/kubernetes/pkg/runtime"
 )
@@ -52,7 +51,3 @@ func addKnownTypes(scheme *runtime.Scheme) error {
 	)
 	return nil
 }
-
-func (obj *RoleBindingRestrictionList) GetObjectKind() unversioned.ObjectKind { return &obj.TypeMeta }
-func (obj *RoleBindingRestriction) GetObjectKind() unversioned.ObjectKind     { return &obj.TypeMeta }
-func (obj *RoleBindingRestriction) GetObjectMeta() meta.Object                { return &obj.ObjectMeta }

pkg/cmd/server/bootstrappolicy/policy.go

@@ -137,7 +137,7 @@ func GetBootstrapClusterRoles() []authorizationapi.ClusterRole {
 				authorizationapi.NewRule(read...).Groups(certificatesGroup).Resources("certificatesigningrequests", "certificatesigningrequests/approval", "certificatesigningrequests/status").RuleOrDie(),
 
 				authorizationapi.NewRule(read...).Groups(authzGroup).Resources("clusterpolicies", "clusterpolicybindings", "clusterroles", "clusterrolebindings",
-					"policies", "policybindings", "roles", "rolebindings", "rolebindingrestrictions").RuleOrDie(),
+					"policies", "policybindings", "roles", "rolebindings").RuleOrDie(),
 
 				authorizationapi.NewRule(read...).Groups(buildGroup).Resources("builds", "builds/details", "buildconfigs", "buildconfigs/webhooks", "builds/log").RuleOrDie(),
 
@@ -253,7 +253,7 @@ func GetBootstrapClusterRoles() []authorizationapi.ClusterRole {
 				authorizationapi.NewRule("create").Groups(authzGroup).Resources("localresourceaccessreviews", "localsubjectaccessreviews", "subjectrulesreviews").RuleOrDie(),
 				authorizationapi.NewRule("create").Groups(securityGroup).Resources("podsecuritypolicysubjectreviews", "podsecuritypolicyselfsubjectreviews", "podsecuritypolicyreviews").RuleOrDie(),
 
-				authorizationapi.NewRule(read...).Groups(authzGroup).Resources("policies", "policybindings").RuleOrDie(),
+				authorizationapi.NewRule(read...).Groups(authzGroup).Resources("policies", "policybindings", "rolebindingrestrictions").RuleOrDie(),
 
 				authorizationapi.NewRule(readWrite...).Groups(buildGroup).Resources("builds", "buildconfigs", "buildconfigs/webhooks").RuleOrDie(),
 				authorizationapi.NewRule(read...).Groups(buildGroup).Resources("builds/log").RuleOrDie(),

pkg/cmd/server/origin/master_config.go

@@ -331,8 +331,8 @@ var (
 	// openshiftAdmissionControlPlugins gives the in-order default admission chain for openshift resources.
 	openshiftAdmissionControlPlugins = []string{
 		"ProjectRequestLimit",
-		"openshift.io/RestrictSubjectBindings",
 		"OriginNamespaceLifecycle",
+		"openshift.io/RestrictSubjectBindings",
 		"PodNodeConstraints",
 		"openshift.io/JenkinsBootstrapper",
 		"BuildByStrategy",
@@ -373,8 +373,8 @@ var (
 	CombinedAdmissionControlPlugins = []string{
 		lifecycle.PluginName,
 		"ProjectRequestLimit",
-		"openshift.io/RestrictSubjectBindings",
 		"OriginNamespaceLifecycle",
+		"openshift.io/RestrictSubjectBindings",
 		"PodNodeConstraints",
 		"openshift.io/JenkinsBootstrapper",
 		"BuildByStrategy",