only expand resources when strictly needed

Author: deads2k

pkg/authorization/api/group_test.go

@@ -7,9 +7,61 @@ import (
 )
 
 func TestEscalating(t *testing.T) {
-	escalatingResources := ExpandResources(sets.NewString(GroupsToResources[EscalatingResourcesGroupName]...))
-	nonEscalatingResources := ExpandResources(sets.NewString(GroupsToResources[NonEscalatingResourcesGroupName]...))
+	escalatingResources := NormalizeResources(sets.NewString(GroupsToResources[EscalatingResourcesGroupName]...))
+	nonEscalatingResources := NormalizeResources(sets.NewString(GroupsToResources[NonEscalatingResourcesGroupName]...))
 	if len(nonEscalatingResources) <= len(escalatingResources) {
 		t.Errorf("groups look bad: escalating=%v nonescalating=%v", escalatingResources.List(), nonEscalatingResources.List())
 	}
 }
+
+func TestNormalizeResources(t *testing.T) {
+	tests := []struct {
+		name     string
+		resource string
+		expected string
+	}{
+		{"capA", "capA", "capa"},
+		{"capH", "capH", "caph"},
+		{"capZ", "capZ", "capz"},
+		{"group", BuildGroupName, "builds"},
+	}
+
+	for _, test := range tests {
+		normalizedNames := NormalizeResources(sets.NewString(test.resource))
+
+		if !normalizedNames.Has(test.expected) {
+			t.Errorf("%s: expected %s, got %v", test.name, test.expected, normalizedNames)
+		}
+
+	}
+}
+
+func TestNeedsNormalization(t *testing.T) {
+	tests := []struct {
+		name     string
+		resource string
+		expected bool
+	}{
+		{"cap", "G", true},
+		{"lowera", "lowera", false},
+		{"lowerh", "lowerh", false},
+		{"lowerz", "lowerz", false},
+		{"0", "0", false},
+		{"5", "5", false},
+		{"9", "9", false},
+		{"/", "/", false},
+		{"-", "-", false},
+		{".", ".", false},
+		{":", ":", true},
+		{"~", "~", true},
+	}
+
+	for _, test := range tests {
+		needsNormalizing := needsNormalizing(test.resource)
+
+		if needsNormalizing != test.expected {
+			t.Errorf("%s: expected %v, got %v", test.name, test.expected, needsNormalizing)
+		}
+
+	}
+}

pkg/authorization/api/helpers.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"sort"
 	"strings"
+	"unicode"
 
 	kapi "k8s.io/kubernetes/pkg/api"
 	"k8s.io/kubernetes/pkg/api/validation"
@@ -13,7 +14,30 @@ import (
 	// uservalidation "github.com/openshift/origin/pkg/user/api/validation"
 )
 
-func ExpandResources(rawResources sets.String) sets.String {
+// NormalizeResources expands all resource groups and forces all resources to lower case.
+// If the rawResources are already normalized, it returns the original set to avoid the
+// allocation and GC cost, since this is hit multiple times for every REST call.
+// That means you should NEVER MODIFY THE RESULT of this call.
+func NormalizeResources(rawResources sets.String) sets.String {
+	// we only need to expand groups if the exist and we don't create them with groups
+	// by default.  Only accept the cost of expansion if we're doing work.
+	needsNormalization := false
+	for currResource := range rawResources {
+		if strings.HasPrefix(currResource, ResourceGroupPrefix) {
+			needsNormalization = true
+			break
+		}
+
+		if needsNormalizing(currResource) {
+			needsNormalization = true
+			break
+		}
+
+	}
+	if !needsNormalization {
+		return rawResources
+	}
+
 	ret := sets.String{}
 	toVisit := rawResources.List()
 	visited := sets.String{}
@@ -25,7 +49,7 @@ func ExpandResources(rawResources sets.String) sets.String {
 		}
 		visited.Insert(currResource)
 
-		if strings.Index(currResource, ResourceGroupPrefix+":") != 0 {
+		if !strings.HasPrefix(currResource, ResourceGroupPrefix) {
 			ret.Insert(strings.ToLower(currResource))
 			continue
 		}
@@ -38,6 +62,19 @@ func ExpandResources(rawResources sets.String) sets.String {
 	return ret
 }
 
+func needsNormalizing(in string) bool {
+	for _, r := range in {
+		if unicode.IsLower(r) || unicode.IsDigit(r) ||
+			(r == '/') || (r == '-') || (r == '.') {
+			continue
+		}
+
+		return true
+	}
+
+	return false
+}
+
 func (r PolicyRule) String() string {
 	return fmt.Sprintf("PolicyRule{Verbs:%v, APIGroups:%v, Resources:%v, ResourceNames:%v, Restrictions:%v}", r.Verbs.List(), r.APIGroups, r.Resources.List(), r.ResourceNames.List(), r.AttributeRestrictions)
 }

pkg/authorization/api/types.go

@@ -33,43 +33,43 @@ const (
 	APIGroupExtensions = "extensions"
 
 	// ResourceGroupPrefix is the prefix for indicating that a resource entry is actually a group of resources.  The groups are defined in code and indicate resources that are commonly permissioned together
-	ResourceGroupPrefix = "resourcegroup"
-	BuildGroupName      = ResourceGroupPrefix + ":builds"
-	DeploymentGroupName = ResourceGroupPrefix + ":deployments"
-	ImageGroupName      = ResourceGroupPrefix + ":images"
-	OAuthGroupName      = ResourceGroupPrefix + ":oauth"
-	UserGroupName       = ResourceGroupPrefix + ":users"
-	TemplateGroupName   = ResourceGroupPrefix + ":templates"
-	SDNGroupName        = ResourceGroupPrefix + ":sdn"
+	ResourceGroupPrefix = "resourcegroup:"
+	BuildGroupName      = ResourceGroupPrefix + "builds"
+	DeploymentGroupName = ResourceGroupPrefix + "deployments"
+	ImageGroupName      = ResourceGroupPrefix + "images"
+	OAuthGroupName      = ResourceGroupPrefix + "oauth"
+	UserGroupName       = ResourceGroupPrefix + "users"
+	TemplateGroupName   = ResourceGroupPrefix + "templates"
+	SDNGroupName        = ResourceGroupPrefix + "sdn"
 	// PolicyOwnerGroupName includes the physical resources behind the PermissionGrantingGroupName.  Unless these physical objects are created first, users with privileges to PermissionGrantingGroupName will
 	// only be able to bind to global roles
-	PolicyOwnerGroupName = ResourceGroupPrefix + ":policy"
+	PolicyOwnerGroupName = ResourceGroupPrefix + "policy"
 	// PermissionGrantingGroupName includes resources that are necessary to maintain authorization roles and bindings.  By itself, this group is insufficient to create anything except for bindings
 	// to master roles.  If a local Policy already exists, then privileges to this group will allow for modification of local roles.
-	PermissionGrantingGroupName = ResourceGroupPrefix + ":granter"
+	PermissionGrantingGroupName = ResourceGroupPrefix + "granter"
 	// OpenshiftExposedGroupName includes resources that are commonly viewed and modified by end users of the system.  It does not include any sensitive resources that control authentication or authorization
-	OpenshiftExposedGroupName = ResourceGroupPrefix + ":exposedopenshift"
-	OpenshiftAllGroupName     = ResourceGroupPrefix + ":allopenshift"
-	OpenshiftStatusGroupName  = ResourceGroupPrefix + ":allopenshift-status"
+	OpenshiftExposedGroupName = ResourceGroupPrefix + "exposedopenshift"
+	OpenshiftAllGroupName     = ResourceGroupPrefix + "allopenshift"
+	OpenshiftStatusGroupName  = ResourceGroupPrefix + "allopenshift-status"
 
-	QuotaGroupName = ResourceGroupPrefix + ":quota"
+	QuotaGroupName = ResourceGroupPrefix + "quota"
 	// KubeInternalsGroupName includes those resources that should reasonably be viewable to end users, but that most users should probably not modify.  Kubernetes herself will maintain these resources
-	KubeInternalsGroupName = ResourceGroupPrefix + ":privatekube"
+	KubeInternalsGroupName = ResourceGroupPrefix + "privatekube"
 	// KubeExposedGroupName includes resources that are commonly viewed and modified by end users of the system.
-	KubeExposedGroupName = ResourceGroupPrefix + ":exposedkube"
-	KubeAllGroupName     = ResourceGroupPrefix + ":allkube"
-	KubeStatusGroupName  = ResourceGroupPrefix + ":allkube-status"
+	KubeExposedGroupName = ResourceGroupPrefix + "exposedkube"
+	KubeAllGroupName     = ResourceGroupPrefix + "allkube"
+	KubeStatusGroupName  = ResourceGroupPrefix + "allkube-status"
 
 	// NonEscalatingResourcesGroupName contains all resources that can be viewed without exposing the risk of using view rights to locate a secret to escalate privileges.  For example, view
 	// rights on secrets could be used locate a secret that happened to be  serviceaccount token that has more privileges
-	NonEscalatingResourcesGroupName         = ResourceGroupPrefix + ":non-escalating"
-	KubeNonEscalatingViewableGroupName      = ResourceGroupPrefix + ":kube-non-escalating"
-	OpenshiftNonEscalatingViewableGroupName = ResourceGroupPrefix + ":openshift-non-escalating"
+	NonEscalatingResourcesGroupName         = ResourceGroupPrefix + "non-escalating"
+	KubeNonEscalatingViewableGroupName      = ResourceGroupPrefix + "kube-non-escalating"
+	OpenshiftNonEscalatingViewableGroupName = ResourceGroupPrefix + "openshift-non-escalating"
 
 	// EscalatingResourcesGroupName contains all resources that can be used to escalate privileges when simply viewed
-	EscalatingResourcesGroupName         = ResourceGroupPrefix + ":escalating"
-	KubeEscalatingViewableGroupName      = ResourceGroupPrefix + ":kube-escalating"
-	OpenshiftEscalatingViewableGroupName = ResourceGroupPrefix + ":openshift-escalating"
+	EscalatingResourcesGroupName         = ResourceGroupPrefix + "escalating"
+	KubeEscalatingViewableGroupName      = ResourceGroupPrefix + "kube-escalating"
+	OpenshiftEscalatingViewableGroupName = ResourceGroupPrefix + "openshift-escalating"
 )
 
 var (
@@ -107,11 +107,11 @@ var (
 
 func init() {
 	// set the non-escalating groups
-	GroupsToResources[OpenshiftNonEscalatingViewableGroupName] = ExpandResources(sets.NewString(GroupsToResources[OpenshiftAllGroupName]...)).
-		Difference(ExpandResources(sets.NewString(GroupsToResources[OpenshiftEscalatingViewableGroupName]...))).List()
+	GroupsToResources[OpenshiftNonEscalatingViewableGroupName] = NormalizeResources(sets.NewString(GroupsToResources[OpenshiftAllGroupName]...)).
+		Difference(NormalizeResources(sets.NewString(GroupsToResources[OpenshiftEscalatingViewableGroupName]...))).List()
 
-	GroupsToResources[KubeNonEscalatingViewableGroupName] = ExpandResources(sets.NewString(GroupsToResources[KubeAllGroupName]...)).
-		Difference(ExpandResources(sets.NewString(GroupsToResources[KubeEscalatingViewableGroupName]...))).List()
+	GroupsToResources[KubeNonEscalatingViewableGroupName] = NormalizeResources(sets.NewString(GroupsToResources[KubeAllGroupName]...)).
+		Difference(NormalizeResources(sets.NewString(GroupsToResources[KubeEscalatingViewableGroupName]...))).List()
 }
 
 // PolicyRule holds information that describes a policy rule, but does not contain information

pkg/authorization/authorizer/attributes.go

@@ -45,7 +45,7 @@ func (a DefaultAuthorizationAttributes) RuleMatches(rule authorizationapi.Policy
 	if a.verbMatches(rule.Verbs) {
 		if a.apiGroupMatches(rule.APIGroups) {
 
-			allowedResourceTypes := authorizationapi.ExpandResources(rule.Resources)
+			allowedResourceTypes := authorizationapi.NormalizeResources(rule.Resources)
 			if a.resourceMatches(allowedResourceTypes) {
 				if a.nameMatches(rule.ResourceNames) {
 					// this rule matches the request, so we should check the additional restrictions to be sure that it's allowed

pkg/authorization/rulevalidation/policy_comparator.go

@@ -63,7 +63,7 @@ func breakdownRule(rule authorizationapi.PolicyRule) []authorizationapi.PolicyRu
 func breadownRuleForGroup(group string, rule authorizationapi.PolicyRule) []authorizationapi.PolicyRule {
 	subrules := []authorizationapi.PolicyRule{}
 
-	for resource := range authorizationapi.ExpandResources(rule.Resources) {
+	for resource := range authorizationapi.NormalizeResources(rule.Resources) {
 		for verb := range rule.Verbs {
 			if len(rule.ResourceNames) > 0 {
 				for _, resourceName := range rule.ResourceNames.List() {
@@ -83,7 +83,7 @@ func breadownRuleForGroup(group string, rule authorizationapi.PolicyRule) []auth
 // ruleCovers determines whether the ownerRule (which may have multiple verbs, resources, and resourceNames) covers
 // the subrule (which may only contain at most one verb, resource, and resourceName)
 func ruleCovers(ownerRule, subrule authorizationapi.PolicyRule) bool {
-	allResources := authorizationapi.ExpandResources(ownerRule.Resources)
+	allResources := authorizationapi.NormalizeResources(ownerRule.Resources)
 
 	ownerGroups := sets.NewString(ownerRule.APIGroups...)
 	groupMatches := ownerGroups.Has(authorizationapi.APIGroupAll) || ownerGroups.HasAll(subrule.APIGroups...) || (len(ownerRule.APIGroups) == 0 && len(subrule.APIGroups) == 0)

pkg/cmd/server/bootstrappolicy/policy.go

@@ -35,7 +35,7 @@ func GetBootstrapOpenshiftRoles(openshiftNamespace string) []authorizationapi.Ro
 	// our default roles and hard for them to reason about what power they are granting their users
 	for i := range roles {
 		for j := range roles[i].Rules {
-			roles[i].Rules[j].Resources = authorizationapi.ExpandResources(roles[i].Rules[j].Resources)
+			roles[i].Rules[j].Resources = authorizationapi.NormalizeResources(roles[i].Rules[j].Resources)
 		}
 	}
 
@@ -560,7 +560,7 @@ func GetBootstrapClusterRoles() []authorizationapi.ClusterRole {
 	// our default roles and hard for them to reason about what power they are granting their users
 	for i := range roles {
 		for j := range roles[i].Rules {
-			roles[i].Rules[j].Resources = authorizationapi.ExpandResources(roles[i].Rules[j].Resources)
+			roles[i].Rules[j].Resources = authorizationapi.NormalizeResources(roles[i].Rules[j].Resources)
 		}
 	}
 

pkg/cmd/server/origin/reststorage_validation_test.go

@@ -49,7 +49,7 @@ func TestValidationRegistration(t *testing.T) {
 
 // TestAllOpenShiftResourceCoverage checks to make sure that the openshift all group actually contains all openshift resources
 func TestAllOpenShiftResourceCoverage(t *testing.T) {
-	allOpenshift := authorizationapi.ExpandResources(sets.NewString(authorizationapi.GroupsToResources[authorizationapi.OpenshiftAllGroupName]...))
+	allOpenshift := authorizationapi.NormalizeResources(sets.NewString(authorizationapi.GroupsToResources[authorizationapi.OpenshiftAllGroupName]...))
 
 	config := fakeMasterConfig()