Merge pull request #31289 from deads2k/remove-cast-utilities

Automatic merge from submit-queue

remove cast utilities from rbac

Casting functions like these are a source of pain in OpenShift.  We should eliminate them to avoid drift problems like we've had downstream.

@kubernetes/sig-auth 

@ericchiang ptal

pkg/apis/rbac/validation/cast.go

@@ -1,107 +0,0 @@
-/*
-Copyright 2016 The Kubernetes Authors.
-
-Licensed under the Apache License, Version 2.0 (the "License");
-you may not use this file except in compliance with the License.
-You may obtain a copy of the License at
-
-    http://www.apache.org/licenses/LICENSE-2.0
-
-Unless required by applicable law or agreed to in writing, software
-distributed under the License is distributed on an "AS IS" BASIS,
-WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-See the License for the specific language governing permissions and
-limitations under the License.
-*/
-
-package validation
-
-import "k8s.io/kubernetes/pkg/apis/rbac"
-
-// Casting utilities to and from "Cluster" level equivalents.
-
-func toClusterRole(in *rbac.Role) *rbac.ClusterRole {
-	if in == nil {
-		return nil
-	}
-
-	ret := &rbac.ClusterRole{}
-	ret.ObjectMeta = in.ObjectMeta
-	ret.Rules = in.Rules
-
-	return ret
-}
-
-func toClusterRoleList(in *rbac.RoleList) *rbac.ClusterRoleList {
-	ret := &rbac.ClusterRoleList{}
-	for _, curr := range in.Items {
-		ret.Items = append(ret.Items, *toClusterRole(&curr))
-	}
-
-	return ret
-}
-
-func toClusterRoleBinding(in *rbac.RoleBinding) *rbac.ClusterRoleBinding {
-	if in == nil {
-		return nil
-	}
-
-	ret := &rbac.ClusterRoleBinding{}
-	ret.ObjectMeta = in.ObjectMeta
-	ret.Subjects = in.Subjects
-	ret.RoleRef = in.RoleRef
-
-	return ret
-}
-
-func toClusterRoleBindingList(in *rbac.RoleBindingList) *rbac.ClusterRoleBindingList {
-	ret := &rbac.ClusterRoleBindingList{}
-	for _, curr := range in.Items {
-		ret.Items = append(ret.Items, *toClusterRoleBinding(&curr))
-	}
-
-	return ret
-}
-
-func toRole(in *rbac.ClusterRole) *rbac.Role {
-	if in == nil {
-		return nil
-	}
-
-	ret := &rbac.Role{}
-	ret.ObjectMeta = in.ObjectMeta
-	ret.Rules = in.Rules
-
-	return ret
-}
-
-func toRoleList(in *rbac.ClusterRoleList) *rbac.RoleList {
-	ret := &rbac.RoleList{}
-	for _, curr := range in.Items {
-		ret.Items = append(ret.Items, *toRole(&curr))
-	}
-
-	return ret
-}
-
-func toRoleBinding(in *rbac.ClusterRoleBinding) *rbac.RoleBinding {
-	if in == nil {
-		return nil
-	}
-
-	ret := &rbac.RoleBinding{}
-	ret.ObjectMeta = in.ObjectMeta
-	ret.Subjects = in.Subjects
-	ret.RoleRef = in.RoleRef
-
-	return ret
-}
-
-func toRoleBindingList(in *rbac.ClusterRoleBindingList) *rbac.RoleBindingList {
-	ret := &rbac.RoleBindingList{}
-	for _, curr := range in.Items {
-		ret.Items = append(ret.Items, *toRoleBinding(&curr))
-	}
-
-	return ret
-}

pkg/apis/rbac/validation/validation.go

@@ -30,28 +30,12 @@ func minimalNameRequirements(name string, prefix bool) []string {
 	return path.IsValidPathSegmentName(name)
 }
 
-func ValidateRole(policy *rbac.Role) field.ErrorList {
+func ValidateRole(role *rbac.Role) field.ErrorList {
-	return validateRole(policy, true)
-}
-
-func ValidateRoleUpdate(policy *rbac.Role, oldRole *rbac.Role) field.ErrorList {
-	return validateRoleUpdate(policy, oldRole, true)
-}
-
-func ValidateClusterRole(policy *rbac.ClusterRole) field.ErrorList {
-	return validateRole(toRole(policy), false)
-}
-
-func ValidateClusterRoleUpdate(policy *rbac.ClusterRole, oldRole *rbac.ClusterRole) field.ErrorList {
-	return validateRoleUpdate(toRole(policy), toRole(oldRole), false)
-}
-
-func validateRole(role *rbac.Role, isNamespaced bool) field.ErrorList {
 	allErrs := field.ErrorList{}
-	allErrs = append(allErrs, validation.ValidateObjectMeta(&role.ObjectMeta, isNamespaced, minimalNameRequirements, field.NewPath("metadata"))...)
+	allErrs = append(allErrs, validation.ValidateObjectMeta(&role.ObjectMeta, true, minimalNameRequirements, field.NewPath("metadata"))...)
 
 	for i, rule := range role.Rules {
-		if err := validatePolicyRule(rule, isNamespaced, field.NewPath("rules").Index(i)); err != nil {
+		if err := validatePolicyRule(rule, true, field.NewPath("rules").Index(i)); err != nil {
 			allErrs = append(allErrs, err...)
 		}
 	}
@@ -61,6 +45,35 @@ func validateRole(role *rbac.Role, isNamespaced bool) field.ErrorList {
 	return nil
 }
 
+func ValidateRoleUpdate(role *rbac.Role, oldRole *rbac.Role) field.ErrorList {
+	allErrs := ValidateRole(role)
+	allErrs = append(allErrs, validation.ValidateObjectMetaUpdate(&role.ObjectMeta, &oldRole.ObjectMeta, field.NewPath("metadata"))...)
+
+	return allErrs
+}
+
+func ValidateClusterRole(role *rbac.ClusterRole) field.ErrorList {
+	allErrs := field.ErrorList{}
+	allErrs = append(allErrs, validation.ValidateObjectMeta(&role.ObjectMeta, false, minimalNameRequirements, field.NewPath("metadata"))...)
+
+	for i, rule := range role.Rules {
+		if err := validatePolicyRule(rule, false, field.NewPath("rules").Index(i)); err != nil {
+			allErrs = append(allErrs, err...)
+		}
+	}
+	if len(allErrs) != 0 {
+		return allErrs
+	}
+	return nil
+}
+
+func ValidateClusterRoleUpdate(role *rbac.ClusterRole, oldRole *rbac.ClusterRole) field.ErrorList {
+	allErrs := ValidateClusterRole(role)
+	allErrs = append(allErrs, validation.ValidateObjectMetaUpdate(&role.ObjectMeta, &oldRole.ObjectMeta, field.NewPath("metadata"))...)
+
+	return allErrs
+}
+
 func validatePolicyRule(rule rbac.PolicyRule, isNamespaced bool, fldPath *field.Path) field.ErrorList {
 	allErrs := field.ErrorList{}
 	if len(rule.Verbs) == 0 {
@@ -86,32 +99,9 @@ func validatePolicyRule(rule rbac.PolicyRule, isNamespaced bool, fldPath *field.
 	return allErrs
 }
 
-func validateRoleUpdate(role *rbac.Role, oldRole *rbac.Role, isNamespaced bool) field.ErrorList {
+func ValidateRoleBinding(roleBinding *rbac.RoleBinding) field.ErrorList {
-	allErrs := validateRole(role, isNamespaced)
-	allErrs = append(allErrs, validation.ValidateObjectMetaUpdate(&role.ObjectMeta, &oldRole.ObjectMeta, field.NewPath("metadata"))...)
-
-	return allErrs
-}
-
-func ValidateRoleBinding(policy *rbac.RoleBinding) field.ErrorList {
-	return validateRoleBinding(policy, true)
-}
-
-func ValidateRoleBindingUpdate(policy *rbac.RoleBinding, oldRoleBinding *rbac.RoleBinding) field.ErrorList {
-	return validateRoleBindingUpdate(policy, oldRoleBinding, true)
-}
-
-func ValidateClusterRoleBinding(policy *rbac.ClusterRoleBinding) field.ErrorList {
-	return validateRoleBinding(toRoleBinding(policy), false)
-}
-
-func ValidateClusterRoleBindingUpdate(policy *rbac.ClusterRoleBinding, oldRoleBinding *rbac.ClusterRoleBinding) field.ErrorList {
-	return validateRoleBindingUpdate(toRoleBinding(policy), toRoleBinding(oldRoleBinding), false)
-}
-
-func validateRoleBinding(roleBinding *rbac.RoleBinding, isNamespaced bool) field.ErrorList {
 	allErrs := field.ErrorList{}
-	allErrs = append(allErrs, validation.ValidateObjectMeta(&roleBinding.ObjectMeta, isNamespaced, minimalNameRequirements, field.NewPath("metadata"))...)
+	allErrs = append(allErrs, validation.ValidateObjectMeta(&roleBinding.ObjectMeta, true, minimalNameRequirements, field.NewPath("metadata"))...)
 
 	// roleRef namespace is empty when referring to global policy.
 	if len(roleBinding.RoleRef.Namespace) > 0 {
@@ -130,7 +120,56 @@ func validateRoleBinding(roleBinding *rbac.RoleBinding, isNamespaced bool) field
 
 	subjectsPath := field.NewPath("subjects")
 	for i, subject := range roleBinding.Subjects {
-		allErrs = append(allErrs, validateRoleBindingSubject(subject, isNamespaced, subjectsPath.Index(i))...)
+		allErrs = append(allErrs, validateRoleBindingSubject(subject, true, subjectsPath.Index(i))...)
+	}
+
+	return allErrs
+}
+
+func ValidateRoleBindingUpdate(roleBinding *rbac.RoleBinding, oldRoleBinding *rbac.RoleBinding) field.ErrorList {
+	allErrs := ValidateRoleBinding(roleBinding)
+	allErrs = append(allErrs, validation.ValidateObjectMetaUpdate(&roleBinding.ObjectMeta, &oldRoleBinding.ObjectMeta, field.NewPath("metadata"))...)
+
+	if oldRoleBinding.RoleRef != roleBinding.RoleRef {
+		allErrs = append(allErrs, field.Invalid(field.NewPath("roleRef"), roleBinding.RoleRef, "cannot change roleRef"))
+	}
+
+	return allErrs
+}
+
+func ValidateClusterRoleBinding(roleBinding *rbac.ClusterRoleBinding) field.ErrorList {
+	allErrs := field.ErrorList{}
+	allErrs = append(allErrs, validation.ValidateObjectMeta(&roleBinding.ObjectMeta, false, minimalNameRequirements, field.NewPath("metadata"))...)
+
+	// roleRef namespace is empty when referring to global policy.
+	if len(roleBinding.RoleRef.Namespace) > 0 {
+		for _, msg := range validation.ValidateNamespaceName(roleBinding.RoleRef.Namespace, false) {
+			allErrs = append(allErrs, field.Invalid(field.NewPath("roleRef", "namespace"), roleBinding.RoleRef.Namespace, msg))
+		}
+	}
+
+	if len(roleBinding.RoleRef.Name) == 0 {
+		allErrs = append(allErrs, field.Required(field.NewPath("roleRef", "name"), ""))
+	} else {
+		for _, msg := range minimalNameRequirements(roleBinding.RoleRef.Name, false) {
+			allErrs = append(allErrs, field.Invalid(field.NewPath("roleRef", "name"), roleBinding.RoleRef.Name, msg))
+		}
+	}
+
+	subjectsPath := field.NewPath("subjects")
+	for i, subject := range roleBinding.Subjects {
+		allErrs = append(allErrs, validateRoleBindingSubject(subject, false, subjectsPath.Index(i))...)
+	}
+
+	return allErrs
+}
+
+func ValidateClusterRoleBindingUpdate(roleBinding *rbac.ClusterRoleBinding, oldRoleBinding *rbac.ClusterRoleBinding) field.ErrorList {
+	allErrs := ValidateClusterRoleBinding(roleBinding)
+	allErrs = append(allErrs, validation.ValidateObjectMetaUpdate(&roleBinding.ObjectMeta, &oldRoleBinding.ObjectMeta, field.NewPath("metadata"))...)
+
+	if oldRoleBinding.RoleRef != roleBinding.RoleRef {
+		allErrs = append(allErrs, field.Invalid(field.NewPath("roleRef"), roleBinding.RoleRef, "cannot change roleRef"))
 	}
 
 	return allErrs
@@ -172,14 +211,3 @@ func validateRoleBindingSubject(subject rbac.Subject, isNamespaced bool, fldPath
 
 	return allErrs
 }
-
-func validateRoleBindingUpdate(roleBinding *rbac.RoleBinding, oldRoleBinding *rbac.RoleBinding, isNamespaced bool) field.ErrorList {
-	allErrs := validateRoleBinding(roleBinding, isNamespaced)
-	allErrs = append(allErrs, validation.ValidateObjectMetaUpdate(&roleBinding.ObjectMeta, &oldRoleBinding.ObjectMeta, field.NewPath("metadata"))...)
-
-	if oldRoleBinding.RoleRef != roleBinding.RoleRef {
-		allErrs = append(allErrs, field.Invalid(field.NewPath("roleRef"), roleBinding.RoleRef, "cannot change roleRef"))
-	}
-
-	return allErrs
-}

pkg/apis/rbac/validation/validation_test.go

@@ -25,7 +25,7 @@ import (
 )
 
 func TestValidateRoleBinding(t *testing.T) {
-	errs := validateRoleBinding(
+	errs := ValidateRoleBinding(
 		&rbac.RoleBinding{
 			ObjectMeta: api.ObjectMeta{Namespace: api.NamespaceDefault, Name: "master"},
 			RoleRef:    api.ObjectReference{Namespace: "master", Name: "valid"},
@@ -35,7 +35,6 @@ func TestValidateRoleBinding(t *testing.T) {
 				{Name: "valid@groupname", Kind: rbac.GroupKind},
 			},
 		},
-		true,
 	)
 	if len(errs) != 0 {
 		t.Errorf("expected success: %v", errs)
@@ -107,7 +106,7 @@ func TestValidateRoleBinding(t *testing.T) {
 		},
 	}
 	for k, v := range errorCases {
-		errs := validateRoleBinding(&v.A, true)
+		errs := ValidateRoleBinding(&v.A)
 		if len(errs) == 0 {
 			t.Errorf("expected failure %s for %v", k, v.A)
 			continue
@@ -129,13 +128,12 @@ func TestValidateRoleBindingUpdate(t *testing.T) {
 		RoleRef:    api.ObjectReference{Namespace: "master", Name: "valid"},
 	}
 
-	errs := validateRoleBindingUpdate(
+	errs := ValidateRoleBindingUpdate(
 		&rbac.RoleBinding{
 			ObjectMeta: api.ObjectMeta{Namespace: api.NamespaceDefault, Name: "master", ResourceVersion: "1"},
 			RoleRef:    api.ObjectReference{Namespace: "master", Name: "valid"},
 		},
 		old,
-		true,
 	)
 	if len(errs) != 0 {
 		t.Errorf("expected success: %v", errs)
@@ -156,7 +154,7 @@ func TestValidateRoleBindingUpdate(t *testing.T) {
 		},
 	}
 	for k, v := range errorCases {
-		errs := validateRoleBindingUpdate(&v.A, old, true)
+		errs := ValidateRoleBindingUpdate(&v.A, old)
 		if len(errs) == 0 {
 			t.Errorf("expected failure %s for %v", k, v.A)
 			continue
@@ -197,16 +195,44 @@ func TestNonResourceURLCovers(t *testing.T) {
 	}
 }
 
-type validateRoleTest struct {
+type ValidateRoleTest struct {
 	role    rbac.Role
-	isNamespaced bool
 	wantErr bool
 	errType field.ErrorType
 	field   string
 }
 
-func (v validateRoleTest) test(t *testing.T) {
+func (v ValidateRoleTest) test(t *testing.T) {
-	errs := validateRole(&v.role, v.isNamespaced)
+	errs := ValidateRole(&v.role)
+	if len(errs) == 0 {
+		if v.wantErr {
+			t.Fatal("expected validation error")
+		}
+		return
+	}
+	if !v.wantErr {
+		t.Errorf("didn't expect error, got %v", errs)
+		return
+	}
+	for i := range errs {
+		if errs[i].Type != v.errType {
+			t.Errorf("expected errors to have type %s: %v", v.errType, errs[i])
+		}
+		if errs[i].Field != v.field {
+			t.Errorf("expected errors to have field %s: %v", v.field, errs[i])
+		}
+	}
+}
+
+type ValidateClusterRoleTest struct {
+	role    rbac.ClusterRole
+	wantErr bool
+	errType field.ErrorType
+	field   string
+}
+
+func (v ValidateClusterRoleTest) test(t *testing.T) {
+	errs := ValidateClusterRole(&v.role)
 	if len(errs) == 0 {
 		if v.wantErr {
 			t.Fatal("expected validation error")
@@ -228,11 +254,10 @@ func (v validateRoleTest) test(t *testing.T) {
 }
 
 func TestValidateRoleZeroLengthNamespace(t *testing.T) {
-	validateRoleTest{
+	ValidateRoleTest{
 		role: rbac.Role{
 			ObjectMeta: api.ObjectMeta{Name: "default"},
 		},
-		isNamespaced: true,
 		wantErr: true,
 		errType: field.ErrorTypeRequired,
 		field:   "metadata.namespace",
@@ -240,11 +265,10 @@ func TestValidateRoleZeroLengthNamespace(t *testing.T) {
 }
 
 func TestValidateRoleZeroLengthName(t *testing.T) {
-	validateRoleTest{
+	ValidateRoleTest{
 		role: rbac.Role{
 			ObjectMeta: api.ObjectMeta{Namespace: "default"},
 		},
-		isNamespaced: true,
 		wantErr: true,
 		errType: field.ErrorTypeRequired,
 		field:   "metadata.name",
@@ -252,33 +276,31 @@ func TestValidateRoleZeroLengthName(t *testing.T) {
 }
 
 func TestValidateRoleValidRole(t *testing.T) {
-	validateRoleTest{
+	ValidateRoleTest{
 		role: rbac.Role{
 			ObjectMeta: api.ObjectMeta{
 				Namespace: "default",
 				Name:      "default",
 			},
 		},
-		isNamespaced: true,
 		wantErr: false,
 	}.test(t)
 }
 
 func TestValidateRoleValidRoleNoNamespace(t *testing.T) {
-	validateRoleTest{
+	ValidateClusterRoleTest{
-		role: rbac.Role{
+		role: rbac.ClusterRole{
 			ObjectMeta: api.ObjectMeta{
 				Name: "default",
 			},
 		},
-		isNamespaced: false,
 		wantErr: false,
 	}.test(t)
 }
 
 func TestValidateRoleNonResourceURL(t *testing.T) {
-	validateRoleTest{
+	ValidateClusterRoleTest{
-		role: rbac.Role{
+		role: rbac.ClusterRole{
 			ObjectMeta: api.ObjectMeta{
 				Name: "default",
 			},
@@ -289,13 +311,12 @@ func TestValidateRoleNonResourceURL(t *testing.T) {
 				},
 			},
 		},
-		isNamespaced: false,
 		wantErr: false,
 	}.test(t)
 }
 
 func TestValidateRoleNamespacedNonResourceURL(t *testing.T) {
-	validateRoleTest{
+	ValidateRoleTest{
 		role: rbac.Role{
 			ObjectMeta: api.ObjectMeta{
 				Namespace: "default",
@@ -309,7 +330,6 @@ func TestValidateRoleNamespacedNonResourceURL(t *testing.T) {
 				},
 			},
 		},
-		isNamespaced: true,
 		wantErr: true,
 		errType: field.ErrorTypeInvalid,
 		field:   "rules[0].nonResourceURLs",
@@ -317,8 +337,8 @@ func TestValidateRoleNamespacedNonResourceURL(t *testing.T) {
 }
 
 func TestValidateRoleNonResourceURLNoVerbs(t *testing.T) {
-	validateRoleTest{
+	ValidateClusterRoleTest{
-		role: rbac.Role{
+		role: rbac.ClusterRole{
 			ObjectMeta: api.ObjectMeta{
 				Name: "default",
 			},
@@ -329,7 +349,6 @@ func TestValidateRoleNonResourceURLNoVerbs(t *testing.T) {
 				},
 			},
 		},
-		isNamespaced: false,
 		wantErr: true,
 		errType: field.ErrorTypeRequired,
 		field:   "rules[0].verbs",
@@ -337,10 +356,11 @@ func TestValidateRoleNonResourceURLNoVerbs(t *testing.T) {
 }
 
 func TestValidateRoleMixedNonResourceAndResource(t *testing.T) {
-	validateRoleTest{
+	ValidateRoleTest{
 		role: rbac.Role{
 			ObjectMeta: api.ObjectMeta{
 				Name:      "default",
+				Namespace: "default",
 			},
 			Rules: []rbac.PolicyRule{
 				{
@@ -358,10 +378,11 @@ func TestValidateRoleMixedNonResourceAndResource(t *testing.T) {
 }
 
 func TestValidateRoleValidResource(t *testing.T) {
-	validateRoleTest{
+	ValidateRoleTest{
 		role: rbac.Role{
 			ObjectMeta: api.ObjectMeta{
 				Name:      "default",
+				Namespace: "default",
 			},
 			Rules: []rbac.PolicyRule{
 				{
@@ -376,10 +397,11 @@ func TestValidateRoleValidResource(t *testing.T) {
 }
 
 func TestValidateRoleNoAPIGroup(t *testing.T) {
-	validateRoleTest{
+	ValidateRoleTest{
 		role: rbac.Role{
 			ObjectMeta: api.ObjectMeta{
 				Name:      "default",
+				Namespace: "default",
 			},
 			Rules: []rbac.PolicyRule{
 				{
@@ -395,10 +417,11 @@ func TestValidateRoleNoAPIGroup(t *testing.T) {
 }
 
 func TestValidateRoleNoResources(t *testing.T) {
-	validateRoleTest{
+	ValidateRoleTest{
 		role: rbac.Role{
 			ObjectMeta: api.ObjectMeta{
 				Name:      "default",
+				Namespace: "default",
 			},
 			Rules: []rbac.PolicyRule{
 				{