Skip to content

Commit

Permalink
fix error messages for clusterrolebinding
Browse files Browse the repository at this point in the history 
when accessing clusterrolebindings, on an error the system currently
displays the type erroneously as rolebinding by adding the type to the
struct makes it easy to return the correct type

fixes bug 1323997

changelog
- pass unversioned.GroupResource to create instead of a string

- fix error messages for role/clusterrole as well

- fix gofmt error

- changed some types in the testing functions to the correct types
  • Loading branch information
@JacobTanenbaum
JacobTanenbaum committed Oct 3, 2016
1 parent a326aae commit a1a5cdd
Show file tree
Hide file tree
Showing 8 changed files with 37 additions and 31 deletions.
3 changes: 2 additions & 1 deletion pkg/authorization/registry/clusterrole/proxy/proxy.go
View file
Expand Up @@ -34,7 +34,8 @@ func NewClusterRoleStorage(clusterPolicyRegistry clusterpolicyregistry.Registry,

RuleResolver: ruleResolver,
CreateStrategy: roleregistry.ClusterStrategy,
UpdateStrategy: roleregistry.ClusterStrategy},
UpdateStrategy: roleregistry.ClusterStrategy,
Resource: authorizationapi.Resource("clusterrole")},
}
}

Expand Down
1 change: 1 addition & 0 deletions pkg/authorization/registry/clusterrolebinding/proxy/proxy.go
View file
Expand Up @@ -35,6 +35,7 @@ func NewClusterRoleBindingStorage(clusterPolicyRegistry clusterpolicyregistry.Re
RuleResolver: ruleResolver,
CreateStrategy: rolebindingregistry.ClusterStrategy,
UpdateStrategy: rolebindingregistry.ClusterStrategy,
Resource: authorizationapi.Resource("clusterrolebinding"),
},
}
}
Expand Down
23 changes: 12 additions & 11 deletions pkg/authorization/registry/role/policybased/virtual_storage.go
View file
Expand Up @@ -29,11 +29,12 @@ type VirtualStorage struct {
RuleResolver rulevalidation.AuthorizationRuleResolver
CreateStrategy rest.RESTCreateStrategy
UpdateStrategy rest.RESTUpdateStrategy
Resource unversioned.GroupResource
}

// NewVirtualStorage creates a new REST for policies.
func NewVirtualStorage(policyStorage policyregistry.Registry, ruleResolver rulevalidation.AuthorizationRuleResolver) roleregistry.Storage {
return &VirtualStorage{policyStorage, ruleResolver, roleregistry.LocalStrategy, roleregistry.LocalStrategy}
func NewVirtualStorage(policyStorage policyregistry.Registry, ruleResolver rulevalidation.AuthorizationRuleResolver, resource unversioned.GroupResource) roleregistry.Storage {
return &VirtualStorage{policyStorage, ruleResolver, roleregistry.LocalStrategy, roleregistry.LocalStrategy, resource}
}

func (m *VirtualStorage) New() runtime.Object {
Expand Down Expand Up @@ -67,15 +68,15 @@ func (m *VirtualStorage) List(ctx kapi.Context, options *kapi.ListOptions) (runt
func (m *VirtualStorage) Get(ctx kapi.Context, name string) (runtime.Object, error) {
policy, err := m.PolicyStorage.GetPolicy(ctx, authorizationapi.PolicyName)
if kapierrors.IsNotFound(err) {
return nil, kapierrors.NewNotFound(authorizationapi.Resource("role"), name)
return nil, kapierrors.NewNotFound(m.Resource, name)
}
if err != nil {
return nil, err
}

role, exists := policy.Roles[name]
if !exists {
return nil, kapierrors.NewNotFound(authorizationapi.Resource("role"), name)
return nil, kapierrors.NewNotFound(m.Resource, name)
}

return role, nil
Expand All @@ -86,14 +87,14 @@ func (m *VirtualStorage) Delete(ctx kapi.Context, name string, options *kapi.Del
if err := kclient.RetryOnConflict(kclient.DefaultRetry, func() error {
policy, err := m.PolicyStorage.GetPolicy(ctx, authorizationapi.PolicyName)
if kapierrors.IsNotFound(err) {
return kapierrors.NewNotFound(authorizationapi.Resource("role"), name)
return kapierrors.NewNotFound(m.Resource, name)
}
if err != nil {
return err
}

if _, exists := policy.Roles[name]; !exists {
return kapierrors.NewNotFound(authorizationapi.Resource("role"), name)
return kapierrors.NewNotFound(m.Resource, name)
}

delete(policy.Roles, name)
Expand Down Expand Up @@ -129,7 +130,7 @@ func (m *VirtualStorage) createRole(ctx kapi.Context, obj runtime.Object, allowE

role := obj.(*authorizationapi.Role)
if !allowEscalation {
if err := rulevalidation.ConfirmNoEscalation(ctx, authorizationapi.Resource("role"), role.Name, m.RuleResolver, authorizationinterfaces.NewLocalRoleAdapter(role)); err != nil {
if err := rulevalidation.ConfirmNoEscalation(ctx, m.Resource, role.Name, m.RuleResolver, authorizationinterfaces.NewLocalRoleAdapter(role)); err != nil {
return nil, err
}
}
Expand All @@ -140,7 +141,7 @@ func (m *VirtualStorage) createRole(ctx kapi.Context, obj runtime.Object, allowE
return err
}
if _, exists := policy.Roles[role.Name]; exists {
return kapierrors.NewAlreadyExists(authorizationapi.Resource("role"), role.Name)
return kapierrors.NewAlreadyExists(m.Resource, role.Name)
}

role.ResourceVersion = policy.ResourceVersion
Expand Down Expand Up @@ -170,15 +171,15 @@ func (m *VirtualStorage) updateRole(ctx kapi.Context, name string, objInfo rest.
if err := kclient.RetryOnConflict(kclient.DefaultRetry, func() error {
policy, err := m.PolicyStorage.GetPolicy(ctx, authorizationapi.PolicyName)
if kapierrors.IsNotFound(err) {
return kapierrors.NewNotFound(authorizationapi.Resource("role"), name)
return kapierrors.NewNotFound(m.Resource, name)
}
if err != nil {
return err
}

oldRole, exists := policy.Roles[name]
if !exists {
return kapierrors.NewNotFound(authorizationapi.Resource("role"), name)
return kapierrors.NewNotFound(m.Resource, name)
}

obj, err := objInfo.UpdatedObject(ctx, oldRole)
Expand All @@ -200,7 +201,7 @@ func (m *VirtualStorage) updateRole(ctx kapi.Context, name string, objInfo rest.
}

if !allowEscalation {
if err := rulevalidation.ConfirmNoEscalation(ctx, authorizationapi.Resource("role"), role.Name, m.RuleResolver, authorizationinterfaces.NewLocalRoleAdapter(role)); err != nil {
if err := rulevalidation.ConfirmNoEscalation(ctx, m.Resource, role.Name, m.RuleResolver, authorizationinterfaces.NewLocalRoleAdapter(role)); err != nil {
return err
}
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/authorization/registry/role/policybased/virtual_storage_test.go
View file
Expand Up @@ -49,14 +49,14 @@ func testNewLocalPolicies() []authorizationapi.Policy {
func makeLocalTestStorage() roleregistry.Storage {
policyRegistry := test.NewPolicyRegistry(testNewLocalPolicies(), nil)

return NewVirtualStorage(policyRegistry, rulevalidation.NewDefaultRuleResolver(policyRegistry, &test.PolicyBindingRegistry{}, &test.ClusterPolicyRegistry{}, &test.ClusterPolicyBindingRegistry{}))
return NewVirtualStorage(policyRegistry, rulevalidation.NewDefaultRuleResolver(policyRegistry, &test.PolicyBindingRegistry{}, &test.ClusterPolicyRegistry{}, &test.ClusterPolicyBindingRegistry{}), authorizationapi.Resource("role"))
}

func makeClusterTestStorage() roleregistry.Storage {
clusterPolicyRegistry := test.NewClusterPolicyRegistry(testNewClusterPolicies(), nil)
policyRegistry := clusterpolicyregistry.NewSimulatedRegistry(clusterPolicyRegistry)

return NewVirtualStorage(policyRegistry, rulevalidation.NewDefaultRuleResolver(nil, &test.PolicyBindingRegistry{}, clusterPolicyRegistry, &test.ClusterPolicyBindingRegistry{}))
return NewVirtualStorage(policyRegistry, rulevalidation.NewDefaultRuleResolver(nil, &test.PolicyBindingRegistry{}, clusterPolicyRegistry, &test.ClusterPolicyBindingRegistry{}), authorizationapi.Resource("clusterrole"))
}

func TestCreateValidationError(t *testing.T) {
Expand Down
22 changes: 12 additions & 10 deletions pkg/authorization/registry/rolebinding/policybased/virtual_storage.go
View file
Expand Up @@ -27,16 +27,18 @@ type VirtualStorage struct {
RuleResolver rulevalidation.AuthorizationRuleResolver
CreateStrategy rest.RESTCreateStrategy
UpdateStrategy rest.RESTUpdateStrategy
Resource unversioned.GroupResource
}

// NewVirtualStorage creates a new REST for policies.
func NewVirtualStorage(bindingRegistry policybindingregistry.Registry, ruleResolver rulevalidation.AuthorizationRuleResolver) rolebindingregistry.Storage {
func NewVirtualStorage(bindingRegistry policybindingregistry.Registry, ruleResolver rulevalidation.AuthorizationRuleResolver, resource unversioned.GroupResource) rolebindingregistry.Storage {
return &VirtualStorage{
BindingRegistry: bindingRegistry,

RuleResolver: ruleResolver,
CreateStrategy: rolebindingregistry.LocalStrategy,
UpdateStrategy: rolebindingregistry.LocalStrategy,
Resource: resource,
}
}

Expand Down Expand Up @@ -71,15 +73,15 @@ func (m *VirtualStorage) List(ctx kapi.Context, options *kapi.ListOptions) (runt
func (m *VirtualStorage) Get(ctx kapi.Context, name string) (runtime.Object, error) {
policyBinding, err := m.getPolicyBindingOwningRoleBinding(ctx, name)
if kapierrors.IsNotFound(err) {
return nil, kapierrors.NewNotFound(authorizationapi.Resource("rolebinding"), name)
return nil, kapierrors.NewNotFound(m.Resource, name)
}
if err != nil {
return nil, err
}

binding, exists := policyBinding.RoleBindings[name]
if !exists {
return nil, kapierrors.NewNotFound(authorizationapi.Resource("rolebinding"), name)
return nil, kapierrors.NewNotFound(m.Resource, name)
}
return binding, nil
}
Expand All @@ -88,14 +90,14 @@ func (m *VirtualStorage) Delete(ctx kapi.Context, name string, options *kapi.Del
if err := kclient.RetryOnConflict(kclient.DefaultRetry, func() error {
owningPolicyBinding, err := m.getPolicyBindingOwningRoleBinding(ctx, name)
if kapierrors.IsNotFound(err) {
return kapierrors.NewNotFound(authorizationapi.Resource("rolebinding"), name)
return kapierrors.NewNotFound(m.Resource, name)
}
if err != nil {
return err
}

if _, exists := owningPolicyBinding.RoleBindings[name]; !exists {
return kapierrors.NewNotFound(authorizationapi.Resource("rolebinding"), name)
return kapierrors.NewNotFound(m.Resource, name)
}

delete(owningPolicyBinding.RoleBindings, name)
Expand Down Expand Up @@ -146,7 +148,7 @@ func (m *VirtualStorage) createRoleBinding(ctx kapi.Context, obj runtime.Object,

_, exists := policyBinding.RoleBindings[roleBinding.Name]
if exists {
return kapierrors.NewAlreadyExists(authorizationapi.Resource("rolebinding"), roleBinding.Name)
return kapierrors.NewAlreadyExists(m.Resource, roleBinding.Name)
}

roleBinding.ResourceVersion = policyBinding.ResourceVersion
Expand Down Expand Up @@ -200,7 +202,7 @@ func (m *VirtualStorage) updateRoleBinding(ctx kapi.Context, name string, objInf
}
oldRoleBinding, exists = policyBinding.RoleBindings[roleBinding.Name]
if !exists {
return kapierrors.NewNotFound(authorizationapi.Resource("rolebinding"), roleBinding.Name)
return kapierrors.NewNotFound(m.Resource, roleBinding.Name)
}

if len(roleBinding.ResourceVersion) == 0 && m.UpdateStrategy.AllowUnconditionalUpdate() {
Expand Down Expand Up @@ -241,7 +243,7 @@ func (m *VirtualStorage) updateRoleBinding(ctx kapi.Context, name string, objInf
}); err != nil {
if roleBindingConflicted {
// construct the typed conflict error
return nil, false, kapierrors.NewConflict(authorizationapi.Resource("rolebinding"), name, err)
return nil, false, kapierrors.NewConflict(m.Resource, name, err)
}
return nil, false, err
}
Expand All @@ -254,7 +256,7 @@ func (m *VirtualStorage) confirmNoEscalation(ctx kapi.Context, roleBinding *auth
return err
}

return rulevalidation.ConfirmNoEscalation(ctx, authorizationapi.Resource("rolebinding"), roleBinding.Name, m.RuleResolver, modifyingRole)
return rulevalidation.ConfirmNoEscalation(ctx, m.Resource, roleBinding.Name, m.RuleResolver, modifyingRole)
}

// ensurePolicyBindingToMaster returns a PolicyBinding object that has a PolicyRef pointing to the Policy in the passed namespace.
Expand Down Expand Up @@ -320,5 +322,5 @@ func (m *VirtualStorage) getPolicyBindingOwningRoleBinding(ctx kapi.Context, bin
}
}

return nil, kapierrors.NewNotFound(authorizationapi.Resource("rolebinding"), bindingName)
return nil, kapierrors.NewNotFound(m.Resource, bindingName)
}
6 changes: 3 additions & 3 deletions pkg/authorization/registry/rolebinding/policybased/virtual_storage_test.go
View file
Expand Up @@ -69,15 +69,15 @@ func makeTestStorage() rolebindingregistry.Storage {
clusterPolicyRegistry := test.NewClusterPolicyRegistry(testNewClusterPolicies(), nil)
policyRegistry := test.NewPolicyRegistry([]authorizationapi.Policy{}, nil)

return NewVirtualStorage(bindingRegistry, rulevalidation.NewDefaultRuleResolver(policyRegistry, bindingRegistry, clusterPolicyRegistry, clusterBindingRegistry))
return NewVirtualStorage(bindingRegistry, rulevalidation.NewDefaultRuleResolver(policyRegistry, bindingRegistry, clusterPolicyRegistry, clusterBindingRegistry), authorizationapi.Resource("rolebinding"))
}

func makeClusterTestStorage() rolebindingregistry.Storage {
clusterBindingRegistry := test.NewClusterPolicyBindingRegistry(testNewClusterBindings(), nil)
clusterPolicyRegistry := test.NewClusterPolicyRegistry(testNewClusterPolicies(), nil)
bindingRegistry := clusterpolicybindingregistry.NewSimulatedRegistry(clusterBindingRegistry)

return NewVirtualStorage(bindingRegistry, rulevalidation.NewDefaultRuleResolver(nil, nil, clusterPolicyRegistry, clusterBindingRegistry))
return NewVirtualStorage(bindingRegistry, rulevalidation.NewDefaultRuleResolver(nil, nil, clusterPolicyRegistry, clusterBindingRegistry), authorizationapi.Resource("clusterrolebinding"))
}

func TestCreateValidationError(t *testing.T) {
Expand Down Expand Up @@ -354,7 +354,7 @@ func TestDeleteError(t *testing.T) {
bindingRegistry := &test.PolicyBindingRegistry{}
bindingRegistry.Err = errors.New("Sample Error")

storage := NewVirtualStorage(bindingRegistry, rulevalidation.NewDefaultRuleResolver(&test.PolicyRegistry{}, bindingRegistry, &test.ClusterPolicyRegistry{}, &test.ClusterPolicyBindingRegistry{}))
storage := NewVirtualStorage(bindingRegistry, rulevalidation.NewDefaultRuleResolver(&test.PolicyRegistry{}, bindingRegistry, &test.ClusterPolicyRegistry{}, &test.ClusterPolicyBindingRegistry{}), authorizationapi.Resource("rolebinding"))
ctx := kapi.WithUser(kapi.WithNamespace(kapi.NewContext(), "unittest"), &user.DefaultInfo{Name: "system:admin"})
_, err := storage.Delete(ctx, "foo", nil)
if err != bindingRegistry.Err {
Expand Down
4 changes: 2 additions & 2 deletions pkg/cmd/server/admin/overwrite_bootstrappolicy.go
View file
Expand Up @@ -162,8 +162,8 @@ func OverwriteBootstrapPolicy(optsGetter restoptions.Getter, policyFile, createB
clusterpolicybindingregistry.ReadOnlyClusterPolicyBinding{Registry: clusterPolicyBindingRegistry},
)

roleStorage := rolestorage.NewVirtualStorage(policyRegistry, ruleResolver)
roleBindingStorage := rolebindingstorage.NewVirtualStorage(policyBindingRegistry, ruleResolver)
roleStorage := rolestorage.NewVirtualStorage(policyRegistry, ruleResolver, authorizationapi.Resource("role"))
roleBindingStorage := rolebindingstorage.NewVirtualStorage(policyBindingRegistry, ruleResolver, authorizationapi.Resource("rolebinding"))
clusterRoleStorage := clusterrolestorage.NewClusterRoleStorage(clusterPolicyRegistry, clusterPolicyBindingRegistry)
clusterRoleBindingStorage := clusterrolebindingstorage.NewClusterRoleBindingStorage(clusterPolicyRegistry, clusterPolicyBindingRegistry)

Expand Down
5 changes: 3 additions & 2 deletions pkg/cmd/server/origin/master.go
View file
Expand Up @@ -98,6 +98,7 @@ import (
appliedclusterresourcequotaregistry "github.com/openshift/origin/pkg/quota/registry/appliedclusterresourcequota"
clusterresourcequotaregistry "github.com/openshift/origin/pkg/quota/registry/clusterresourcequota"

authorizationapi "github.com/openshift/origin/pkg/authorization/api"
clusterpolicyregistry "github.com/openshift/origin/pkg/authorization/registry/clusterpolicy"
clusterpolicystorage "github.com/openshift/origin/pkg/authorization/registry/clusterpolicy/etcd"
clusterpolicybindingregistry "github.com/openshift/origin/pkg/authorization/registry/clusterpolicybinding"
Expand Down Expand Up @@ -542,8 +543,8 @@ func (c *MasterConfig) GetRestStorage() map[string]rest.Storage {

selfSubjectRulesReviewStorage := selfsubjectrulesreview.NewREST(c.RuleResolver, c.Informers.ClusterPolicies().Lister().ClusterPolicies())

roleStorage := rolestorage.NewVirtualStorage(policyRegistry, c.RuleResolver)
roleBindingStorage := rolebindingstorage.NewVirtualStorage(policyBindingRegistry, c.RuleResolver)
roleStorage := rolestorage.NewVirtualStorage(policyRegistry, c.RuleResolver, authorizationapi.Resource("role"))
roleBindingStorage := rolebindingstorage.NewVirtualStorage(policyBindingRegistry, c.RuleResolver, authorizationapi.Resource("rolebinding"))
clusterRoleStorage := clusterrolestorage.NewClusterRoleStorage(clusterPolicyRegistry, clusterPolicyBindingRegistry)
clusterRoleBindingStorage := clusterrolebindingstorage.NewClusterRoleBindingStorage(clusterPolicyRegistry, clusterPolicyBindingRegistry)

Expand Down

0 comments on commit a1a5cdd

Please sign in to comment.