switch image api to use SAR client, not registry

openshift · Sep 6, 2017 · 747b5f1 · 747b5f1
1 parent bde4f7a
commit 747b5f1
Show file tree

Hide file tree

Showing 8 changed files with 62 additions and 63 deletions.
diff --git a/pkg/cmd/server/origin/storage.go b/pkg/cmd/server/origin/storage.go
@@ -113,14 +113,18 @@ func (c OpenshiftAPIConfig) GetRestStorage() (map[schema.GroupVersion]map[string
 		c.KubeClientInternal,
 	)
 
+	authorizationClient, err := authorizationclient.NewForConfig(c.GenericConfig.LoopbackClientConfig)
+	if err != nil {
+		return nil, err
+	}
 	imageStorage, err := imageetcd.NewREST(c.GenericConfig.RESTOptionsGetter)
 	if err != nil {
 		return nil, fmt.Errorf("error building REST storage: %v", err)
 	}
 	imageRegistry := image.NewRegistry(imageStorage)
 	imageSignatureStorage := imagesignature.NewREST(c.DeprecatedOpenshiftClient.Images())
 	imageStreamSecretsStorage := imagesecret.NewREST(c.KubeClientInternal.Core())
-	imageStreamStorage, imageStreamStatusStorage, internalImageStreamStorage, err := imagestreametcd.NewREST(c.GenericConfig.RESTOptionsGetter, c.RegistryHostnameRetriever, subjectAccessReviewRegistry, c.LimitVerifier)
+	imageStreamStorage, imageStreamStatusStorage, internalImageStreamStorage, err := imagestreametcd.NewREST(c.GenericConfig.RESTOptionsGetter, c.RegistryHostnameRetriever, authorizationClient.SubjectAccessReviews(), c.LimitVerifier)
 	if err != nil {
 		return nil, fmt.Errorf("error building REST storage: %v", err)
 	}
@@ -151,10 +155,6 @@ func (c OpenshiftAPIConfig) GetRestStorage() (map[schema.GroupVersion]map[string
 		c.DeprecatedOpenshiftClient.SubjectAccessReviews())
 	imageStreamImageStorage := imagestreamimage.NewREST(imageRegistry, imageStreamRegistry)
 
-	authorizationClient, err := authorizationclient.NewForConfig(c.GenericConfig.LoopbackClientConfig)
-	if err != nil {
-		return nil, err
-	}
 	routeStorage, routeStatusStorage, err := routeetcd.NewREST(c.GenericConfig.RESTOptionsGetter, c.RouteAllocator, authorizationClient.SubjectAccessReviews())
 	if err != nil {
 		return nil, fmt.Errorf("error building REST storage: %v", err)

diff --git a/pkg/image/registry/imagestream/etcd/etcd.go b/pkg/image/registry/imagestream/etcd/etcd.go
@@ -8,8 +8,8 @@ import (
 	"k8s.io/apiserver/pkg/registry/generic/registry"
 	"k8s.io/apiserver/pkg/registry/rest"
 	kapi "k8s.io/kubernetes/pkg/api"
+	authorizationclient "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/typed/authorization/internalversion"
 
-	"github.com/openshift/origin/pkg/authorization/registry/subjectaccessreview"
 	imageadmission "github.com/openshift/origin/pkg/image/admission"
 	imageapi "github.com/openshift/origin/pkg/image/apis/image"
 	"github.com/openshift/origin/pkg/image/registry/imagestream"
@@ -19,13 +19,12 @@ import (
 // REST implements a RESTStorage for image streams against etcd.
 type REST struct {
 	*registry.Store
-	subjectAccessReviewRegistry subjectaccessreview.Registry
 }
 
 var _ rest.StandardStorage = &REST{}
 
 // NewREST returns a new REST.
-func NewREST(optsGetter restoptions.Getter, registryHostname imageapi.RegistryHostnameRetriever, subjectAccessReviewRegistry subjectaccessreview.Registry, limitVerifier imageadmission.LimitVerifier) (*REST, *StatusREST, *InternalREST, error) {
+func NewREST(optsGetter restoptions.Getter, registryHostname imageapi.RegistryHostnameRetriever, subjectAccessReviewRegistry authorizationclient.SubjectAccessReviewInterface, limitVerifier imageadmission.LimitVerifier) (*REST, *StatusREST, *InternalREST, error) {
 	store := registry.Store{
 		Copier:                   kapi.Scheme,
 		NewFunc:                  func() runtime.Object { return &imageapi.ImageStream{} },
@@ -36,7 +35,6 @@ func NewREST(optsGetter restoptions.Getter, registryHostname imageapi.RegistryHo
 
 	rest := &REST{
 		Store: &store,
-		subjectAccessReviewRegistry: subjectAccessReviewRegistry,
 	}
 	// strategy must be able to load image streams across namespaces during tag verification
 	strategy := imagestream.NewStrategy(registryHostname, subjectAccessReviewRegistry, limitVerifier, rest)

diff --git a/pkg/image/registry/imagestream/etcd/etcd_test.go b/pkg/image/registry/imagestream/etcd/etcd_test.go
@@ -9,11 +9,10 @@ import (
 	apirequest "k8s.io/apiserver/pkg/endpoints/request"
 	etcdtesting "k8s.io/apiserver/pkg/storage/etcd/testing"
 	kapihelper "k8s.io/kubernetes/pkg/api/helper"
+	authorizationapi "k8s.io/kubernetes/pkg/apis/authorization"
 	"k8s.io/kubernetes/pkg/registry/registrytest"
 
 	"github.com/openshift/origin/pkg/api/latest"
-	authorizationapi "github.com/openshift/origin/pkg/authorization/apis/authorization"
-	"github.com/openshift/origin/pkg/authorization/registry/subjectaccessreview"
 	"github.com/openshift/origin/pkg/image/admission/testutil"
 	imageapi "github.com/openshift/origin/pkg/image/apis/image"
 	"github.com/openshift/origin/pkg/util/restoptions"
@@ -38,12 +37,14 @@ type fakeSubjectAccessReviewRegistry struct {
 	requestNamespace string
 }
 
-var _ subjectaccessreview.Registry = &fakeSubjectAccessReviewRegistry{}
-
-func (f *fakeSubjectAccessReviewRegistry) CreateSubjectAccessReview(ctx apirequest.Context, subjectAccessReview *authorizationapi.SubjectAccessReview) (*authorizationapi.SubjectAccessReviewResponse, error) {
+func (f *fakeSubjectAccessReviewRegistry) Create(subjectAccessReview *authorizationapi.SubjectAccessReview) (*authorizationapi.SubjectAccessReview, error) {
 	f.request = subjectAccessReview
-	f.requestNamespace = apirequest.NamespaceValue(ctx)
-	return &authorizationapi.SubjectAccessReviewResponse{Allowed: f.allow}, f.err
+	f.requestNamespace = subjectAccessReview.Spec.ResourceAttributes.Namespace
+	return &authorizationapi.SubjectAccessReview{
+		Status: authorizationapi.SubjectAccessReviewStatus{
+			Allowed: f.allow,
+		},
+	}, f.err
 }
 
 func newStorage(t *testing.T) (*REST, *StatusREST, *InternalREST, *etcdtesting.EtcdTestServer) {

diff --git a/pkg/image/registry/imagestream/strategy.go b/pkg/image/registry/imagestream/strategy.go
@@ -18,9 +18,10 @@ import (
 	"k8s.io/apiserver/pkg/storage/names"
 	kapi "k8s.io/kubernetes/pkg/api"
 	kapihelper "k8s.io/kubernetes/pkg/api/helper"
+	authorizationapi "k8s.io/kubernetes/pkg/apis/authorization"
+	authorizationclient "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/typed/authorization/internalversion"
 
-	authorizationapi "github.com/openshift/origin/pkg/authorization/apis/authorization"
-	"github.com/openshift/origin/pkg/authorization/registry/subjectaccessreview"
+	authorizationutil "github.com/openshift/origin/pkg/authorization/util"
 	imageadmission "github.com/openshift/origin/pkg/image/admission"
 	imageapi "github.com/openshift/origin/pkg/image/apis/image"
 	"github.com/openshift/origin/pkg/image/apis/image/validation"
@@ -42,7 +43,7 @@ type Strategy struct {
 
 // NewStrategy is the default logic that applies when creating and updating
 // ImageStream objects via the REST API.
-func NewStrategy(registryHostname imageapi.RegistryHostnameRetriever, subjectAccessReviewClient subjectaccessreview.Registry, limitVerifier imageadmission.LimitVerifier, imageStreamGetter ResourceGetter) Strategy {
+func NewStrategy(registryHostname imageapi.RegistryHostnameRetriever, subjectAccessReviewClient authorizationclient.SubjectAccessReviewInterface, limitVerifier imageadmission.LimitVerifier, imageStreamGetter ResourceGetter) Strategy {
 	return Strategy{
 		ObjectTyper:               kapi.Scheme,
 		NameGenerator:             names.SimpleNameGenerator,
@@ -431,7 +432,7 @@ func updateObservedGenerationForStatusUpdate(stream, oldStream *imageapi.ImageSt
 }
 
 type TagVerifier struct {
-	subjectAccessReviewClient subjectaccessreview.Registry
+	subjectAccessReviewClient authorizationclient.SubjectAccessReviewInterface
 }
 
 func (v *TagVerifier) Verify(old, stream *imageapi.ImageStream, user user.Info) field.ErrorList {
@@ -462,21 +463,24 @@ func (v *TagVerifier) Verify(old, stream *imageapi.ImageStream, user user.Info)
 		}
 
 		// Make sure this user can pull the specified image before allowing them to tag it into another imagestream
-		subjectAccessReview := authorizationapi.AddUserToSAR(user, &authorizationapi.SubjectAccessReview{
-			Action: authorizationapi.Action{
-				Verb:         "get",
-				Group:        imageapi.LegacyGroupName,
-				Resource:     "imagestreams/layers",
-				ResourceName: streamName,
+		subjectAccessReview := authorizationutil.AddUserToSAR(user, &authorizationapi.SubjectAccessReview{
+			Spec: authorizationapi.SubjectAccessReviewSpec{
+				ResourceAttributes: &authorizationapi.ResourceAttributes{
+					Namespace:   tagRef.From.Namespace,
+					Verb:        "get",
+					Group:       imageapi.GroupName,
+					Resource:    "imagestreams",
+					Subresource: "layers",
+					Name:        streamName,
+				},
 			},
 		})
-		ctx := apirequest.WithNamespace(apirequest.WithUser(apirequest.NewContext(), user), tagRef.From.Namespace)
 		glog.V(4).Infof("Performing SubjectAccessReview for user=%s, groups=%v to %s/%s", user.GetName(), user.GetGroups(), tagRef.From.Namespace, streamName)
-		resp, err := v.subjectAccessReviewClient.CreateSubjectAccessReview(ctx, subjectAccessReview)
-		if err != nil || resp == nil || (resp != nil && !resp.Allowed) {
+		resp, err := v.subjectAccessReviewClient.Create(subjectAccessReview)
+		if err != nil || resp == nil || (resp != nil && !resp.Status.Allowed) {
 			message := fmt.Sprintf("%s/%s", tagRef.From.Namespace, streamName)
 			if resp != nil {
-				message = message + fmt.Sprintf(": %q %q", resp.Reason, resp.EvaluationError)
+				message = message + fmt.Sprintf(": %q %q", resp.Status.Reason, resp.Status.EvaluationError)
 			}
 			if err != nil {
 				message = message + fmt.Sprintf("- %v", err)

diff --git a/pkg/image/registry/imagestream/strategy_test.go b/pkg/image/registry/imagestream/strategy_test.go
@@ -13,15 +13,14 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/diff"
-	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	"k8s.io/apiserver/pkg/authentication/user"
 	apirequest "k8s.io/apiserver/pkg/endpoints/request"
 	kapi "k8s.io/kubernetes/pkg/api"
+	authorizationapi "k8s.io/kubernetes/pkg/apis/authorization"
 	kquota "k8s.io/kubernetes/pkg/quota"
 
-	authorizationapi "github.com/openshift/origin/pkg/authorization/apis/authorization"
-	"github.com/openshift/origin/pkg/authorization/registry/subjectaccessreview"
+	oauthorizationapi "github.com/openshift/origin/pkg/authorization/apis/authorization"
 	"github.com/openshift/origin/pkg/image/admission"
 	"github.com/openshift/origin/pkg/image/admission/testutil"
 	imageapi "github.com/openshift/origin/pkg/image/apis/image"
@@ -46,7 +45,7 @@ func (u *fakeUser) GetGroups() []string {
 
 func (u *fakeUser) GetExtra() map[string][]string {
 	return map[string][]string{
-		authorizationapi.ScopesKey: {"a", "b"},
+		oauthorizationapi.ScopesKey: {"a", "b"},
 	}
 }
 
@@ -65,12 +64,14 @@ type fakeSubjectAccessReviewRegistry struct {
 	requestNamespace string
 }
 
-var _ subjectaccessreview.Registry = &fakeSubjectAccessReviewRegistry{}
-
-func (f *fakeSubjectAccessReviewRegistry) CreateSubjectAccessReview(ctx apirequest.Context, subjectAccessReview *authorizationapi.SubjectAccessReview) (*authorizationapi.SubjectAccessReviewResponse, error) {
+func (f *fakeSubjectAccessReviewRegistry) Create(subjectAccessReview *authorizationapi.SubjectAccessReview) (*authorizationapi.SubjectAccessReview, error) {
 	f.request = subjectAccessReview
-	f.requestNamespace = apirequest.NamespaceValue(ctx)
-	return &authorizationapi.SubjectAccessReviewResponse{Allowed: f.allow}, f.err
+	f.requestNamespace = subjectAccessReview.Spec.ResourceAttributes.Namespace
+	return &authorizationapi.SubjectAccessReview{
+		Status: authorizationapi.SubjectAccessReviewStatus{
+			Allowed: f.allow,
+		},
+	}, f.err
 }
 
 func TestPublicDockerImageRepository(t *testing.T) {
@@ -355,15 +356,19 @@ func TestTagVerifier(t *testing.T) {
 				t.Errorf("%s: sar namespace: expected %v, got %v", name, e, a)
 			}
 			expectedSar := &authorizationapi.SubjectAccessReview{
-				Action: authorizationapi.Action{
-					//				Group:        "image.openshift.io",
-					Verb:         "get",
-					Resource:     "imagestreams/layers",
-					ResourceName: "otherstream",
-				},
-				User:   "user",
-				Groups: sets.NewString("group1"),
-				Scopes: []string{"a", "b"},
+				Spec: authorizationapi.SubjectAccessReviewSpec{
+					ResourceAttributes: &authorizationapi.ResourceAttributes{
+						Namespace:   "otherns",
+						Verb:        "get",
+						Group:       "image.openshift.io",
+						Resource:    "imagestreams",
+						Subresource: "layers",
+						Name:        "otherstream",
+					},
+					User:   "user",
+					Groups: []string{"group1"},
+					Extra:  map[string]authorizationapi.ExtraValue{oauthorizationapi.ScopesKey: {"a", "b"}},
+				},
 			}
 			if e, a := expectedSar, sar.request; !reflect.DeepEqual(e, a) {
 				t.Errorf("%s: unexpected SAR request: %s", name, diff.ObjectDiff(e, a))

diff --git a/pkg/image/registry/imagestreamimage/rest_test.go b/pkg/image/registry/imagestreamimage/rest_test.go
@@ -13,10 +13,9 @@ import (
 	etcdtesting "k8s.io/apiserver/pkg/storage/etcd/testing"
 	kapi "k8s.io/kubernetes/pkg/api"
 	"k8s.io/kubernetes/pkg/api/v1"
+	authorizationapi "k8s.io/kubernetes/pkg/apis/authorization"
 	"k8s.io/kubernetes/pkg/registry/registrytest"
 
-	authorizationapi "github.com/openshift/origin/pkg/authorization/apis/authorization"
-	"github.com/openshift/origin/pkg/authorization/registry/subjectaccessreview"
 	"github.com/openshift/origin/pkg/image/admission/testutil"
 	imageapi "github.com/openshift/origin/pkg/image/apis/image"
 	"github.com/openshift/origin/pkg/image/registry/image"
@@ -33,9 +32,7 @@ var testDefaultRegistry = func() (string, bool) { return "defaultregistry:5000",
 type fakeSubjectAccessReviewRegistry struct {
 }
 
-var _ subjectaccessreview.Registry = &fakeSubjectAccessReviewRegistry{}
-
-func (f *fakeSubjectAccessReviewRegistry) CreateSubjectAccessReview(ctx apirequest.Context, subjectAccessReview *authorizationapi.SubjectAccessReview) (*authorizationapi.SubjectAccessReviewResponse, error) {
+func (f *fakeSubjectAccessReviewRegistry) Create(subjectAccessReview *authorizationapi.SubjectAccessReview) (*authorizationapi.SubjectAccessReview, error) {
 	return nil, nil
 }
 

diff --git a/pkg/image/registry/imagestreammapping/rest_test.go b/pkg/image/registry/imagestreammapping/rest_test.go
@@ -21,10 +21,9 @@ import (
 	etcdtesting "k8s.io/apiserver/pkg/storage/etcd/testing"
 	kapi "k8s.io/kubernetes/pkg/api"
 	"k8s.io/kubernetes/pkg/api/v1"
+	authorizationapi "k8s.io/kubernetes/pkg/apis/authorization"
 	"k8s.io/kubernetes/pkg/registry/registrytest"
 
-	authorizationapi "github.com/openshift/origin/pkg/authorization/apis/authorization"
-	"github.com/openshift/origin/pkg/authorization/registry/subjectaccessreview"
 	"github.com/openshift/origin/pkg/image/admission/testutil"
 	imageapi "github.com/openshift/origin/pkg/image/apis/image"
 	"github.com/openshift/origin/pkg/image/registry/image"
@@ -43,9 +42,7 @@ var testDefaultRegistry = func() (string, bool) { return testDefaultRegistryURL,
 type fakeSubjectAccessReviewRegistry struct {
 }
 
-var _ subjectaccessreview.Registry = &fakeSubjectAccessReviewRegistry{}
-
-func (f *fakeSubjectAccessReviewRegistry) CreateSubjectAccessReview(ctx apirequest.Context, subjectAccessReview *authorizationapi.SubjectAccessReview) (*authorizationapi.SubjectAccessReviewResponse, error) {
+func (f *fakeSubjectAccessReviewRegistry) Create(subjectAccessReview *authorizationapi.SubjectAccessReview) (*authorizationapi.SubjectAccessReview, error) {
 	return nil, nil
 }
 

diff --git a/pkg/image/registry/imagestreamtag/rest_test.go b/pkg/image/registry/imagestreamtag/rest_test.go
@@ -17,10 +17,9 @@ import (
 	etcdtesting "k8s.io/apiserver/pkg/storage/etcd/testing"
 	kapi "k8s.io/kubernetes/pkg/api"
 	"k8s.io/kubernetes/pkg/api/v1"
+	authorizationapi "k8s.io/kubernetes/pkg/apis/authorization"
 	"k8s.io/kubernetes/pkg/registry/registrytest"
 
-	authorizationapi "github.com/openshift/origin/pkg/authorization/apis/authorization"
-	"github.com/openshift/origin/pkg/authorization/registry/subjectaccessreview"
 	"github.com/openshift/origin/pkg/image/admission/testutil"
 	imageapi "github.com/openshift/origin/pkg/image/apis/image"
 	"github.com/openshift/origin/pkg/image/registry/image"
@@ -37,9 +36,7 @@ var testDefaultRegistry = func() (string, bool) { return "defaultregistry:5000",
 type fakeSubjectAccessReviewRegistry struct {
 }
 
-var _ subjectaccessreview.Registry = &fakeSubjectAccessReviewRegistry{}
-
-func (f *fakeSubjectAccessReviewRegistry) CreateSubjectAccessReview(ctx apirequest.Context, subjectAccessReview *authorizationapi.SubjectAccessReview) (*authorizationapi.SubjectAccessReviewResponse, error) {
+func (f *fakeSubjectAccessReviewRegistry) Create(subjectAccessReview *authorizationapi.SubjectAccessReview) (*authorizationapi.SubjectAccessReview, error) {
 	return nil, nil
 }