Skip to content

Commit

Permalink
Merge pull request #20491 from deads2k/server-05-admission-shrink
Browse files Browse the repository at this point in the history
avoid unnecessary admission plugin initializers
  • Loading branch information
openshift-merge-robot authored Aug 1, 2018
2 parents c83ba6c + c3f9477 commit 72e6ce7
Show file tree
Hide file tree
Showing 41 changed files with 196 additions and 269 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,18 @@ import (
"fmt"
"io"

"k8s.io/client-go/rest"
kclientset "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset"

"github.com/golang/glog"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
kerrors "k8s.io/apimachinery/pkg/util/errors"
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
"k8s.io/apiserver/pkg/admission"
"k8s.io/kubernetes/pkg/apis/rbac"
kadmission "k8s.io/kubernetes/pkg/kubeapiserver/admission"

userapi "github.com/openshift/api/user/v1"
authorizationclient "github.com/openshift/client-go/authorization/clientset/versioned"
authorizationtypedclient "github.com/openshift/client-go/authorization/clientset/versioned/typed/authorization/v1"
userclient "github.com/openshift/client-go/user/clientset/versioned"
userinformer "github.com/openshift/client-go/user/informers/externalversions"
Expand Down Expand Up @@ -46,8 +47,7 @@ type restrictUsersAdmission struct {
groupCache GroupCache
}

var _ = oadmission.WantsOpenshiftInternalAuthorizationClient(&restrictUsersAdmission{})
var _ = oadmission.WantsOpenshiftInternalUserClient(&restrictUsersAdmission{})
var _ = oadmission.WantsRESTClientConfig(&restrictUsersAdmission{})
var _ = oadmission.WantsUserInformer(&restrictUsersAdmission{})
var _ = kadmission.WantsInternalKubeClientSet(&restrictUsersAdmission{})

Expand All @@ -63,12 +63,18 @@ func (q *restrictUsersAdmission) SetInternalKubeClientSet(c kclientset.Interface
q.kclient = c
}

func (q *restrictUsersAdmission) SetOpenshiftInternalAuthorizationClient(roleBindingRestrictionsGetter authorizationclient.Interface) {
q.roleBindingRestrictionsGetter = roleBindingRestrictionsGetter.Authorization()
}

func (q *restrictUsersAdmission) SetOpenshiftInternalUserClient(userClient userclient.Interface) {
q.userClient = userClient
func (q *restrictUsersAdmission) SetRESTClientConfig(restClientConfig rest.Config) {
var err error
q.roleBindingRestrictionsGetter, err = authorizationtypedclient.NewForConfig(&restClientConfig)
if err != nil {
utilruntime.HandleError(err)
return
}
q.userClient, err = userclient.NewForConfig(&restClientConfig)
if err != nil {
utilruntime.HandleError(err)
return
}
}

func (q *restrictUsersAdmission) SetUserInformer(userInformers userinformer.SharedInformerFactory) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@ import (
"strings"
"testing"

authorizationapi "github.com/openshift/api/authorization/v1"
userapi "github.com/openshift/api/user/v1"
fakeauthorizationclient "github.com/openshift/client-go/authorization/clientset/versioned/fake"
fakeuserclient "github.com/openshift/client-go/user/clientset/versioned/fake"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
Expand All @@ -13,13 +17,6 @@ import (
kapi "k8s.io/kubernetes/pkg/apis/core"
"k8s.io/kubernetes/pkg/apis/rbac"
"k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/fake"
kadmission "k8s.io/kubernetes/pkg/kubeapiserver/admission"

authorizationapi "github.com/openshift/api/authorization/v1"
userapi "github.com/openshift/api/user/v1"
fakeauthorizationclient "github.com/openshift/client-go/authorization/clientset/versioned/fake"
fakeuserclient "github.com/openshift/client-go/user/clientset/versioned/fake"
oadmission "github.com/openshift/origin/pkg/cmd/server/admission"
)

func TestAdmission(t *testing.T) {
Expand Down Expand Up @@ -365,9 +362,9 @@ func TestAdmission(t *testing.T) {
t.Errorf("unexpected error initializing admission plugin: %v", err)
}

plugin.(kadmission.WantsInternalKubeClientSet).SetInternalKubeClientSet(kclientset)
plugin.(oadmission.WantsOpenshiftInternalAuthorizationClient).SetOpenshiftInternalAuthorizationClient(fakeAuthorizationClient)
plugin.(oadmission.WantsOpenshiftInternalUserClient).SetOpenshiftInternalUserClient(fakeUserClient)
plugin.(*restrictUsersAdmission).kclient = kclientset
plugin.(*restrictUsersAdmission).roleBindingRestrictionsGetter = fakeAuthorizationClient.AuthorizationV1()
plugin.(*restrictUsersAdmission).userClient = fakeUserClient
plugin.(*restrictUsersAdmission).groupCache = fakeGroupCache{}

err = admission.ValidateInitialization(plugin)
Expand Down
16 changes: 8 additions & 8 deletions pkg/authorization/util/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,24 +3,24 @@ package util
import (
"errors"

authorizationv1 "k8s.io/api/authorization/v1"
kerrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apiserver/pkg/authentication/user"
"k8s.io/kubernetes/pkg/apis/authorization"
"k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/typed/authorization/internalversion"
authorizationclient "k8s.io/client-go/kubernetes/typed/authorization/v1"
)

// AddUserToSAR adds the requisite user information to a SubjectAccessReview.
// It returns the modified SubjectAccessReview.
func AddUserToSAR(user user.Info, sar *authorization.SubjectAccessReview) *authorization.SubjectAccessReview {
func AddUserToSAR(user user.Info, sar *authorizationv1.SubjectAccessReview) *authorizationv1.SubjectAccessReview {
sar.Spec.User = user.GetName()
// reminiscent of the bad old days of C. Copies copy the min number of elements of both source and dest
sar.Spec.Groups = make([]string, len(user.GetGroups()))
copy(sar.Spec.Groups, user.GetGroups())
sar.Spec.Extra = map[string]authorization.ExtraValue{}
sar.Spec.Extra = map[string]authorizationv1.ExtraValue{}

for k, v := range user.GetExtra() {
sar.Spec.Extra[k] = authorization.ExtraValue(v)
sar.Spec.Extra[k] = authorizationv1.ExtraValue(v)
}

return sar
Expand All @@ -29,9 +29,9 @@ func AddUserToSAR(user user.Info, sar *authorization.SubjectAccessReview) *autho
// Authorize verifies that a given user is permitted to carry out a given
// action. If this cannot be determined, or if the user is not permitted, an
// error is returned.
func Authorize(sarClient internalversion.SubjectAccessReviewInterface, user user.Info, resourceAttributes *authorization.ResourceAttributes) error {
sar := AddUserToSAR(user, &authorization.SubjectAccessReview{
Spec: authorization.SubjectAccessReviewSpec{
func Authorize(sarClient authorizationclient.SubjectAccessReviewInterface, user user.Info, resourceAttributes *authorizationv1.ResourceAttributes) error {
sar := AddUserToSAR(user, &authorizationv1.SubjectAccessReview{
Spec: authorizationv1.SubjectAccessReviewSpec{
ResourceAttributes: resourceAttributes,
},
})
Expand Down
47 changes: 27 additions & 20 deletions pkg/build/apiserver/admission/strategyrestrictions/admission.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,24 +5,26 @@ import (
"io"
"strings"

"github.com/openshift/api/build"
"github.com/openshift/origin/pkg/api/legacy"
"github.com/openshift/origin/pkg/build/buildscheme"
authorizationv1 "k8s.io/api/authorization/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime/schema"
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
"k8s.io/apiserver/pkg/admission"
"k8s.io/client-go/kubernetes"
authorizationclient "k8s.io/client-go/kubernetes/typed/authorization/v1"
"k8s.io/client-go/rest"
kapihelper "k8s.io/kubernetes/pkg/apis/core/helper"
"k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset"
authorizationclient "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/typed/authorization/internalversion"
kubeadmission "k8s.io/kubernetes/pkg/kubeapiserver/admission"
rbacregistry "k8s.io/kubernetes/pkg/registry/rbac"

"github.com/openshift/api/build"
buildclient "github.com/openshift/client-go/build/clientset/versioned"
"github.com/openshift/origin/pkg/api/legacy"
"github.com/openshift/origin/pkg/authorization/util"
buildapi "github.com/openshift/origin/pkg/build/apis/build"
"github.com/openshift/origin/pkg/build/buildscheme"
oadmission "github.com/openshift/origin/pkg/cmd/server/admission"
"github.com/openshift/origin/pkg/cmd/server/bootstrappolicy"
"k8s.io/kubernetes/pkg/apis/authorization"
"k8s.io/apiserver/pkg/admission/initializer"
)

func Register(plugins *admission.Plugins) {
Expand All @@ -38,8 +40,8 @@ type buildByStrategy struct {
buildClient buildclient.Interface
}

var _ = kubeadmission.WantsInternalKubeClientSet(&buildByStrategy{})
var _ = oadmission.WantsOpenshiftInternalBuildClient(&buildByStrategy{})
var _ = initializer.WantsExternalKubeClientSet(&buildByStrategy{})
var _ = oadmission.WantsRESTClientConfig(&buildByStrategy{})

// NewBuildByStrategy returns an admission control for builds that checks
// on policy based on the build strategy type
Expand Down Expand Up @@ -84,12 +86,17 @@ func (a *buildByStrategy) Admit(attr admission.Attributes) error {
}
}

func (a *buildByStrategy) SetInternalKubeClientSet(c internalclientset.Interface) {
a.sarClient = c.Authorization().SubjectAccessReviews()
func (a *buildByStrategy) SetExternalKubeClientSet(c kubernetes.Interface) {
a.sarClient = c.AuthorizationV1().SubjectAccessReviews()
}

func (a *buildByStrategy) SetOpenshiftInternalBuildClient(c buildclient.Interface) {
a.buildClient = c
func (a *buildByStrategy) SetRESTClientConfig(restClientConfig rest.Config) {
var err error
a.buildClient, err = buildclient.NewForConfig(&restClientConfig)
if err != nil {
utilruntime.HandleError(err)
return
}
}

func (a *buildByStrategy) ValidateInitialization() error {
Expand Down Expand Up @@ -139,9 +146,9 @@ func (a *buildByStrategy) checkBuildAuthorization(build *buildapi.Build, attr ad
subresource = tokens[1]
}

sar := util.AddUserToSAR(attr.GetUserInfo(), &authorization.SubjectAccessReview{
Spec: authorization.SubjectAccessReviewSpec{
ResourceAttributes: &authorization.ResourceAttributes{
sar := util.AddUserToSAR(attr.GetUserInfo(), &authorizationv1.SubjectAccessReview{
Spec: authorizationv1.SubjectAccessReviewSpec{
ResourceAttributes: &authorizationv1.ResourceAttributes{
Namespace: attr.GetNamespace(),
Verb: "create",
Group: resource.Group,
Expand All @@ -167,9 +174,9 @@ func (a *buildByStrategy) checkBuildConfigAuthorization(buildConfig *buildapi.Bu
subresource = tokens[1]
}

sar := util.AddUserToSAR(attr.GetUserInfo(), &authorization.SubjectAccessReview{
Spec: authorization.SubjectAccessReviewSpec{
ResourceAttributes: &authorization.ResourceAttributes{
sar := util.AddUserToSAR(attr.GetUserInfo(), &authorizationv1.SubjectAccessReview{
Spec: authorizationv1.SubjectAccessReviewSpec{
ResourceAttributes: &authorizationv1.ResourceAttributes{
Namespace: attr.GetNamespace(),
Verb: "create",
Group: resource.Group,
Expand Down Expand Up @@ -213,7 +220,7 @@ func (a *buildByStrategy) checkBuildRequestAuthorization(req *buildapi.BuildRequ
}
}

func (a *buildByStrategy) checkAccess(strategy buildapi.BuildStrategy, subjectAccessReview *authorization.SubjectAccessReview, attr admission.Attributes) error {
func (a *buildByStrategy) checkAccess(strategy buildapi.BuildStrategy, subjectAccessReview *authorizationv1.SubjectAccessReview, attr admission.Attributes) error {
resp, err := a.sarClient.Create(subjectAccessReview)
if err != nil {
return admission.NewForbidden(attr, err)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,21 +4,19 @@ import (
"fmt"
"testing"

authorizationv1 "k8s.io/api/authorization/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apiserver/pkg/admission"
"k8s.io/apiserver/pkg/authentication/user"
fakekubeclient "k8s.io/client-go/kubernetes/fake"
clientgotesting "k8s.io/client-go/testing"
"k8s.io/kubernetes/pkg/apis/authorization"
fakekubeclient "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/fake"
kubeadmission "k8s.io/kubernetes/pkg/kubeapiserver/admission"

buildapiv1 "github.com/openshift/api/build/v1"
fakebuildclient "github.com/openshift/client-go/build/clientset/versioned/fake"
buildapi "github.com/openshift/origin/pkg/build/apis/build"
oadmission "github.com/openshift/origin/pkg/cmd/server/admission"

"github.com/openshift/api/build"
_ "github.com/openshift/origin/pkg/build/apis/build/install"
Expand All @@ -33,7 +31,7 @@ func TestBuildAdmission(t *testing.T) {
object runtime.Object
oldObject runtime.Object
responseObject runtime.Object
reviewResponse *authorization.SubjectAccessReview
reviewResponse *authorizationv1.SubjectAccessReview
expectedResource string
expectedSubresource string
expectAccept bool
Expand Down Expand Up @@ -179,7 +177,7 @@ func TestBuildAdmission(t *testing.T) {
},
}

emptyResponse := &authorization.SubjectAccessReview{}
emptyResponse := &authorizationv1.SubjectAccessReview{}
ops := []admission.Operation{admission.Create, admission.Update}
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
Expand All @@ -191,7 +189,7 @@ func TestBuildAdmission(t *testing.T) {

fakeKubeClient := fakekubeclient.NewSimpleClientset()
fakeKubeClient.PrependReactor("create", "subjectaccessreviews", func(action clientgotesting.Action) (handled bool, ret runtime.Object, err error) {
review, ok := action.(clientgotesting.CreateAction).GetObject().(*authorization.SubjectAccessReview)
review, ok := action.(clientgotesting.CreateAction).GetObject().(*authorizationv1.SubjectAccessReview)
if !ok {
return true, emptyResponse, fmt.Errorf("unexpected object received: %#v", review)
}
Expand All @@ -211,8 +209,8 @@ func TestBuildAdmission(t *testing.T) {
})

c := NewBuildByStrategy()
c.(kubeadmission.WantsInternalKubeClientSet).SetInternalKubeClientSet(fakeKubeClient)
c.(oadmission.WantsOpenshiftInternalBuildClient).SetOpenshiftInternalBuildClient(fakeBuildClient)
c.(*buildByStrategy).sarClient = fakeKubeClient.AuthorizationV1().SubjectAccessReviews()
c.(*buildByStrategy).buildClient = fakeBuildClient
attrs := admission.NewAttributesRecord(test.object, test.oldObject, test.kind.WithVersion("version"), "foo", "test-build", test.resource.WithVersion("version"), test.subResource, op, fakeUser())
err := c.(admission.MutationInterface).Admit(attrs)
if err != nil && test.expectAccept {
Expand Down Expand Up @@ -298,9 +296,9 @@ func v1TestBuildConfig(strategy buildapiv1.BuildStrategy) *buildapiv1.BuildConfi
}
}

func reviewResponse(allowed bool, msg string) *authorization.SubjectAccessReview {
return &authorization.SubjectAccessReview{
Status: authorization.SubjectAccessReviewStatus{
func reviewResponse(allowed bool, msg string) *authorizationv1.SubjectAccessReview {
return &authorizationv1.SubjectAccessReview{
Status: authorizationv1.SubjectAccessReviewStatus{
Allowed: allowed,
Reason: msg,
},
Expand Down
2 changes: 1 addition & 1 deletion pkg/cmd/openshift-apiserver/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ func RunOpenShiftAPIServer(masterConfig *configapi.MasterConfig) error {
return err
}

if err := informers.GetInternalOpenshiftUserInformers().User().V1().Groups().Informer().AddIndexers(cache.Indexers{
if err := informers.GetOpenshiftUserInformers().User().V1().Groups().Informer().AddIndexers(cache.Indexers{
usercache.ByUserIndexName: usercache.ByUserIndexKeys,
}); err != nil {
return err
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ func RunTemplateInstanceController(ctx ControllerContext) (bool, error) {
go templatecontroller.NewTemplateInstanceController(
ctx.RestMapper,
dynamicClient,
ctx.ClientBuilder.ClientGoClientOrDie(saName).AuthorizationV1(),
ctx.ClientBuilder.KubeInternalClientOrDie(saName),
ctx.ClientBuilder.OpenshiftInternalBuildClientOrDie(saName),
ctx.ClientBuilder.OpenshiftInternalTemplateClientOrDie(saName),
Expand Down
2 changes: 1 addition & 1 deletion pkg/cmd/openshift-controller-manager/controller_manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ func newControllerContext(
InternalNetworkInformers: originInformers.GetInternalOpenshiftNetworkInformers(),
InternalQuotaInformers: originInformers.GetInternalOpenshiftQuotaInformers(),
InternalSecurityInformers: originInformers.GetInternalOpenshiftSecurityInformers(),
InternalRouteInformers: originInformers.GetInternalOpenshiftRouteInformers(),
InternalRouteInformers: originInformers.GetOpenshiftRouteInformers(),
InternalTemplateInformers: originInformers.GetInternalOpenshiftTemplateInformers(),
GenericResourceInformer: originInformers.ToGenericInformer(),
Stop: stopCh,
Expand Down
Loading

0 comments on commit 72e6ce7

Please sign in to comment.