From bc92f800bd16e8fee021e12030bf8313af7acbd3 Mon Sep 17 00:00:00 2001 From: xilabao Date: Tue, 6 Jun 2017 10:24:24 +0800 Subject: [PATCH] compact rules which has the same ResourceName --- pkg/registry/rbac/validation/BUILD | 2 - .../rbac/validation/policy_compact.go | 30 +++++++++----- .../rbac/validation/policy_compact_test.go | 40 +++++++++++++------ 3 files changed, 48 insertions(+), 24 deletions(-) diff --git a/pkg/registry/rbac/validation/BUILD b/pkg/registry/rbac/validation/BUILD index 2e97ba5a5c2..dac4ac52465 100644 --- a/pkg/registry/rbac/validation/BUILD +++ b/pkg/registry/rbac/validation/BUILD @@ -21,7 +21,6 @@ go_test( "//pkg/api:go_default_library", "//pkg/apis/rbac:go_default_library", "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", - "//vendor/k8s.io/apimachinery/pkg/runtime/schema:go_default_library", "//vendor/k8s.io/apimachinery/pkg/util/diff:go_default_library", "//vendor/k8s.io/apiserver/pkg/authentication/user:go_default_library", ], @@ -40,7 +39,6 @@ go_library( "//pkg/apis/rbac:go_default_library", "//vendor/github.com/golang/glog:go_default_library", "//vendor/k8s.io/apimachinery/pkg/api/errors:go_default_library", - "//vendor/k8s.io/apimachinery/pkg/runtime/schema:go_default_library", "//vendor/k8s.io/apimachinery/pkg/util/errors:go_default_library", "//vendor/k8s.io/apiserver/pkg/authentication/serviceaccount:go_default_library", "//vendor/k8s.io/apiserver/pkg/authentication/user:go_default_library", diff --git a/pkg/registry/rbac/validation/policy_compact.go b/pkg/registry/rbac/validation/policy_compact.go index 03e596b2038..223d81613cc 100644 --- a/pkg/registry/rbac/validation/policy_compact.go +++ b/pkg/registry/rbac/validation/policy_compact.go @@ -20,17 +20,23 @@ import ( "fmt" "reflect" - "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/kubernetes/pkg/api" "k8s.io/kubernetes/pkg/apis/rbac" ) +type simpleResource struct { + Group string + Resource string + ResourceNameExist bool + ResourceName string +} + // CompactRules combines rules that contain a single APIGroup/Resource, differ only by verb, and contain no other attributes. // this is a fast check, and works well with the decomposed "missing rules" list from a Covers check. func CompactRules(rules []rbac.PolicyRule) ([]rbac.PolicyRule, error) { compacted := make([]rbac.PolicyRule, 0, len(rules)) - simpleRules := map[schema.GroupResource]*rbac.PolicyRule{} + simpleRules := map[simpleResource]*rbac.PolicyRule{} for _, rule := range rules { if resource, isSimple := isSimpleResourceRule(&rule); isSimple { if existingRule, ok := simpleRules[resource]; ok { @@ -66,12 +72,12 @@ func CompactRules(rules []rbac.PolicyRule) ([]rbac.PolicyRule, error) { return compacted, nil } -// isSimpleResourceRule returns true if the given rule contains verbs, a single resource, a single API group, and no other values -func isSimpleResourceRule(rule *rbac.PolicyRule) (schema.GroupResource, bool) { - resource := schema.GroupResource{} +// isSimpleResourceRule returns true if the given rule contains verbs, a single resource, a single API group, at most one Resource Name, and no other values +func isSimpleResourceRule(rule *rbac.PolicyRule) (simpleResource, bool) { + resource := simpleResource{} // If we have "complex" rule attributes, return early without allocations or expensive comparisons - if len(rule.ResourceNames) > 0 || len(rule.NonResourceURLs) > 0 { + if len(rule.ResourceNames) > 1 || len(rule.NonResourceURLs) > 0 { return resource, false } // If we have multiple api groups or resources, return early @@ -79,11 +85,17 @@ func isSimpleResourceRule(rule *rbac.PolicyRule) (schema.GroupResource, bool) { return resource, false } - // Test if this rule only contains APIGroups/Resources/Verbs - simpleRule := &rbac.PolicyRule{APIGroups: rule.APIGroups, Resources: rule.Resources, Verbs: rule.Verbs} + // Test if this rule only contains APIGroups/Resources/Verbs/ResourceNames + simpleRule := &rbac.PolicyRule{APIGroups: rule.APIGroups, Resources: rule.Resources, Verbs: rule.Verbs, ResourceNames: rule.ResourceNames} if !reflect.DeepEqual(simpleRule, rule) { return resource, false } - resource = schema.GroupResource{Group: rule.APIGroups[0], Resource: rule.Resources[0]} + + if len(rule.ResourceNames) == 0 { + resource = simpleResource{Group: rule.APIGroups[0], Resource: rule.Resources[0], ResourceNameExist: false} + } else { + resource = simpleResource{Group: rule.APIGroups[0], Resource: rule.Resources[0], ResourceNameExist: true, ResourceName: rule.ResourceNames[0]} + } + return resource, true } diff --git a/pkg/registry/rbac/validation/policy_compact_test.go b/pkg/registry/rbac/validation/policy_compact_test.go index cebb749ced7..bcfd2e2f77e 100644 --- a/pkg/registry/rbac/validation/policy_compact_test.go +++ b/pkg/registry/rbac/validation/policy_compact_test.go @@ -21,7 +21,6 @@ import ( "sort" "testing" - "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/kubernetes/pkg/api" "k8s.io/kubernetes/pkg/apis/rbac" ) @@ -43,6 +42,9 @@ func TestCompactRules(t *testing.T) { {Verbs: []string{"create"}, APIGroups: []string{"extensions"}, Resources: []string{"daemonsets"}}, {Verbs: []string{"delete"}, APIGroups: []string{"extensions"}, Resources: []string{"daemonsets"}}, + {Verbs: []string{"patch"}, APIGroups: []string{"extensions"}, Resources: []string{"daemonsets"}, ResourceNames: []string{""}}, + {Verbs: []string{"get"}, APIGroups: []string{"extensions"}, Resources: []string{"daemonsets"}, ResourceNames: []string{"foo"}}, + {Verbs: []string{"list"}, APIGroups: []string{"extensions"}, Resources: []string{"daemonsets"}, ResourceNames: []string{"foo"}}, {Verbs: []string{"educate"}, APIGroups: []string{""}, Resources: []string{"dolphins"}}, @@ -56,6 +58,8 @@ func TestCompactRules(t *testing.T) { }, Expected: []rbac.PolicyRule{ {Verbs: []string{"create", "delete"}, APIGroups: []string{"extensions"}, Resources: []string{"daemonsets"}}, + {Verbs: []string{"patch"}, APIGroups: []string{"extensions"}, Resources: []string{"daemonsets"}, ResourceNames: []string{""}}, + {Verbs: []string{"get", "list"}, APIGroups: []string{"extensions"}, Resources: []string{"daemonsets"}, ResourceNames: []string{"foo"}}, {Verbs: []string{"get", "list", "update", "patch"}, APIGroups: []string{""}, Resources: []string{"builds"}}, {Verbs: []string{"educate"}, APIGroups: []string{""}, Resources: []string{"dolphins"}}, {Verbs: nil, APIGroups: []string{""}, Resources: []string{"pirates"}}, @@ -145,58 +149,68 @@ func TestIsSimpleResourceRule(t *testing.T) { testcases := map[string]struct { Rule rbac.PolicyRule Simple bool - Resource schema.GroupResource + Resource simpleResource }{ "simple, no verbs": { Rule: rbac.PolicyRule{Verbs: []string{}, APIGroups: []string{""}, Resources: []string{"builds"}}, Simple: true, - Resource: schema.GroupResource{Group: "", Resource: "builds"}, + Resource: simpleResource{Group: "", Resource: "builds"}, }, "simple, one verb": { Rule: rbac.PolicyRule{Verbs: []string{"get"}, APIGroups: []string{""}, Resources: []string{"builds"}}, Simple: true, - Resource: schema.GroupResource{Group: "", Resource: "builds"}, + Resource: simpleResource{Group: "", Resource: "builds"}, + }, + "simple, one empty resource name": { + Rule: rbac.PolicyRule{Verbs: []string{"get"}, APIGroups: []string{""}, Resources: []string{"builds"}, ResourceNames: []string{""}}, + Simple: true, + Resource: simpleResource{Group: "", Resource: "builds", ResourceNameExist: true, ResourceName: ""}, + }, + "simple, one resource name": { + Rule: rbac.PolicyRule{Verbs: []string{"get"}, APIGroups: []string{""}, Resources: []string{"builds"}, ResourceNames: []string{"foo"}}, + Simple: true, + Resource: simpleResource{Group: "", Resource: "builds", ResourceNameExist: true, ResourceName: "foo"}, }, "simple, multi verb": { Rule: rbac.PolicyRule{Verbs: []string{"get", "list"}, APIGroups: []string{""}, Resources: []string{"builds"}}, Simple: true, - Resource: schema.GroupResource{Group: "", Resource: "builds"}, + Resource: simpleResource{Group: "", Resource: "builds"}, }, "complex, empty": { Rule: rbac.PolicyRule{}, Simple: false, - Resource: schema.GroupResource{}, + Resource: simpleResource{}, }, "complex, no group": { Rule: rbac.PolicyRule{Verbs: []string{"get"}, APIGroups: []string{}, Resources: []string{"builds"}}, Simple: false, - Resource: schema.GroupResource{}, + Resource: simpleResource{}, }, "complex, multi group": { Rule: rbac.PolicyRule{Verbs: []string{"get"}, APIGroups: []string{"a", "b"}, Resources: []string{"builds"}}, Simple: false, - Resource: schema.GroupResource{}, + Resource: simpleResource{}, }, "complex, no resource": { Rule: rbac.PolicyRule{Verbs: []string{"get"}, APIGroups: []string{""}, Resources: []string{}}, Simple: false, - Resource: schema.GroupResource{}, + Resource: simpleResource{}, }, "complex, multi resource": { Rule: rbac.PolicyRule{Verbs: []string{"get"}, APIGroups: []string{""}, Resources: []string{"builds", "images"}}, Simple: false, - Resource: schema.GroupResource{}, + Resource: simpleResource{}, }, "complex, resource names": { - Rule: rbac.PolicyRule{Verbs: []string{"get"}, APIGroups: []string{""}, Resources: []string{"builds"}, ResourceNames: []string{"foo"}}, + Rule: rbac.PolicyRule{Verbs: []string{"get"}, APIGroups: []string{""}, Resources: []string{"builds"}, ResourceNames: []string{"foo", "bar"}}, Simple: false, - Resource: schema.GroupResource{}, + Resource: simpleResource{}, }, "complex, non-resource urls": { Rule: rbac.PolicyRule{Verbs: []string{"get"}, APIGroups: []string{""}, Resources: []string{"builds"}, NonResourceURLs: []string{"/"}}, Simple: false, - Resource: schema.GroupResource{}, + Resource: simpleResource{}, }, }