Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

avoid unnecessary admission plugin initializers #20491

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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