From 66e3ec633c5cfe7b715434c6ac3bc12171faa3be Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Wed, 9 Oct 2024 22:31:29 +0200 Subject: [PATCH 1/5] Use the streamlined ToolchainCluster. (#474) --- go.mod | 4 ++-- go.sum | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/go.mod b/go.mod index 4cd6d610..ca500884 100644 --- a/go.mod +++ b/go.mod @@ -4,8 +4,8 @@ go 1.20 require ( github.com/aws/aws-sdk-go v1.44.100 - github.com/codeready-toolchain/api v0.0.0-20240927104325-b5bfcb3cb1b0 - github.com/codeready-toolchain/toolchain-common v0.0.0-20240905135929-d55d86fdd41e + github.com/codeready-toolchain/api v0.0.0-20241009095520-331aa861d43b + github.com/codeready-toolchain/toolchain-common v0.0.0-20241003135627-55e81430602b github.com/go-logr/logr v1.4.1 github.com/gofrs/uuid v4.2.0+incompatible github.com/pkg/errors v0.9.1 diff --git a/go.sum b/go.sum index 7f85215c..eec2c0cc 100644 --- a/go.sum +++ b/go.sum @@ -139,10 +139,10 @@ github.com/cncf/xds/go v0.0.0-20210312221358-fbca930ec8ed/go.mod h1:eXthEFrGJvWH github.com/cockroachdb/datadriven v0.0.0-20200714090401-bf6692d28da5/go.mod h1:h6jFvWxBdQXxjopDMZyH2UVceIRfR84bdzbkoKrsWNo= github.com/cockroachdb/errors v1.2.4/go.mod h1:rQD95gz6FARkaKkQXUksEje/d9a6wBJoCr5oaCLELYA= github.com/cockroachdb/logtags v0.0.0-20190617123548-eb05cc24525f/go.mod h1:i/u985jwjWRlyHXQbwatDASoW0RMlZ/3i9yJHE2xLkI= -github.com/codeready-toolchain/api v0.0.0-20240927104325-b5bfcb3cb1b0 h1:7cXHlRpoi1Owo8fYawl80PUsVWz+9AtMge6OJ4DjvWU= -github.com/codeready-toolchain/api v0.0.0-20240927104325-b5bfcb3cb1b0/go.mod h1:ie9p4LenCCS0LsnbWp6/xwpFDdCWYE0KWzUO6Sk1g0E= -github.com/codeready-toolchain/toolchain-common v0.0.0-20240905135929-d55d86fdd41e h1:xTqyuImyon/P2QfV5NIJDsVkEWqb9b6Ax9INsmzpI1Q= -github.com/codeready-toolchain/toolchain-common v0.0.0-20240905135929-d55d86fdd41e/go.mod h1:aIbki5CFsykeqZn2/ZwvUb3Krx2f2Tbq58R6MGnk6H8= +github.com/codeready-toolchain/api v0.0.0-20241009095520-331aa861d43b h1:E0x1LM2BkiS7qm6mAJdaiaWg66bjkOLinL4Dca2tvAA= +github.com/codeready-toolchain/api v0.0.0-20241009095520-331aa861d43b/go.mod h1:ie9p4LenCCS0LsnbWp6/xwpFDdCWYE0KWzUO6Sk1g0E= +github.com/codeready-toolchain/toolchain-common v0.0.0-20241003135627-55e81430602b h1:kPp88xZsAGAo/kF1LV6L1PtM3g7YSY7DNdaLSta4+80= +github.com/codeready-toolchain/toolchain-common v0.0.0-20241003135627-55e81430602b/go.mod h1:kENp9EMqJaoZNvM3BLTk/i+CEteHKrJRAAm0H7L8Z+A= github.com/coreos/bbolt v1.3.2/go.mod h1:iRUV2dpdMOn7Bo10OQBFzIJO9kkE559Wcmn+qkEiiKk= github.com/coreos/etcd v3.3.10+incompatible/go.mod h1:uF7uidLiAD3TWHmW31ZFd/JWoc32PjwdhPthX9715RE= github.com/coreos/etcd v3.3.13+incompatible/go.mod h1:uF7uidLiAD3TWHmW31ZFd/JWoc32PjwdhPthX9715RE= From c2644e1157de83ef628a5238abefc01679014606 Mon Sep 17 00:00:00 2001 From: Matous Jobanek Date: Fri, 11 Oct 2024 13:10:36 +0200 Subject: [PATCH 2/5] drop InformerService (#479) --- cmd/main.go | 8 +- .../service/factory/service_factory.go | 5 - pkg/application/service/services.go | 14 -- pkg/controller/usernames.go | 12 +- pkg/controller/usernames_test.go | 12 +- pkg/informers/service/informer_service.go | 76 -------- .../service/informer_service_test.go | 180 ------------------ pkg/middleware/jwt_middleware_test.go | 5 +- pkg/middleware/promhttp_middleware_test.go | 5 +- pkg/proxy/handlers/spacelister.go | 16 +- pkg/proxy/handlers/spacelister_get.go | 45 +++-- pkg/proxy/handlers/spacelister_get_test.go | 27 +-- pkg/proxy/handlers/spacelister_list.go | 12 +- pkg/proxy/handlers/spacelister_list_test.go | 15 +- pkg/proxy/proxy.go | 20 +- pkg/proxy/proxy_community_test.go | 16 +- pkg/proxy/proxy_test.go | 25 +-- pkg/proxy/service/cluster_service_test.go | 6 +- pkg/server/in_cluster_application.go | 9 +- pkg/server/routes.go | 5 +- pkg/server/server_test.go | 5 +- test/fake/mockable_application.go | 12 -- test/fake/proxy.go | 8 - test/util/application.go | 3 +- 24 files changed, 126 insertions(+), 415 deletions(-) delete mode 100644 pkg/informers/service/informer_service.go delete mode 100644 pkg/informers/service/informer_service_test.go diff --git a/cmd/main.go b/cmd/main.go index 202273b5..06d02fdb 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -12,6 +12,7 @@ import ( "github.com/codeready-toolchain/registration-service/pkg/auth" "github.com/codeready-toolchain/registration-service/pkg/configuration" "github.com/codeready-toolchain/registration-service/pkg/log" + "github.com/codeready-toolchain/registration-service/pkg/namespaced" "github.com/codeready-toolchain/registration-service/pkg/proxy" "github.com/codeready-toolchain/registration-service/pkg/proxy/metrics" "github.com/codeready-toolchain/registration-service/pkg/server" @@ -83,8 +84,9 @@ func main() { panic(fmt.Sprintf("cannot set captcha credentials: %s", err.Error())) } } + nsClient := namespaced.NewClient(cl, configuration.Namespace()) - app := server.NewInClusterApplication(cl, configuration.Namespace()) + app := server.NewInClusterApplication(nsClient) // Initialize toolchain cluster cache service // let's cache the member clusters before we start the services, // this will speed up the first request @@ -106,7 +108,7 @@ func main() { proxyMetrics := metrics.NewProxyMetrics(proxyRegistry) proxyMetricsSrv := proxy.StartMetricsServer(proxyRegistry, proxy.ProxyMetricsPort) // Proxy API server - p, err := proxy.NewProxy(app, proxyMetrics, cluster.GetMemberClusters) + p, err := proxy.NewProxy(nsClient, app, proxyMetrics, cluster.GetMemberClusters) if err != nil { panic(errs.Wrap(err, "failed to create proxy")) } @@ -118,7 +120,7 @@ func main() { regsvcRegistry := prometheus.NewRegistry() regsvcMetricsSrv, _ := server.StartMetricsServer(regsvcRegistry, server.RegSvcMetricsPort) regsvcSrv := server.New(app) - err = regsvcSrv.SetupRoutes(proxy.DefaultPort, regsvcRegistry) + err = regsvcSrv.SetupRoutes(proxy.DefaultPort, regsvcRegistry, nsClient) if err != nil { panic(err.Error()) } diff --git a/pkg/application/service/factory/service_factory.go b/pkg/application/service/factory/service_factory.go index 67f947ba..65e46cdc 100644 --- a/pkg/application/service/factory/service_factory.go +++ b/pkg/application/service/factory/service_factory.go @@ -6,7 +6,6 @@ import ( "github.com/codeready-toolchain/registration-service/pkg/application/service" servicecontext "github.com/codeready-toolchain/registration-service/pkg/application/service/context" "github.com/codeready-toolchain/registration-service/pkg/configuration" - informerservice "github.com/codeready-toolchain/registration-service/pkg/informers/service" "github.com/codeready-toolchain/registration-service/pkg/log" "github.com/codeready-toolchain/registration-service/pkg/namespaced" clusterservice "github.com/codeready-toolchain/registration-service/pkg/proxy/service" @@ -52,10 +51,6 @@ func (s *ServiceFactory) defaultServiceContextProducer() servicecontext.ServiceC } } -func (s *ServiceFactory) InformerService() service.InformerService { - return informerservice.NewInformerService(s.getContext().Client(), configuration.Namespace()) -} - func (s *ServiceFactory) MemberClusterService() service.MemberClusterService { return clusterservice.NewMemberClusterService(s.getContext().Client(), s.getContext().Services().SignupService()) } diff --git a/pkg/application/service/services.go b/pkg/application/service/services.go index 4df4130f..2b8c2214 100644 --- a/pkg/application/service/services.go +++ b/pkg/application/service/services.go @@ -5,17 +5,8 @@ import ( "github.com/codeready-toolchain/registration-service/pkg/proxy/access" "github.com/codeready-toolchain/registration-service/pkg/signup" "github.com/gin-gonic/gin" - "k8s.io/apimachinery/pkg/labels" ) -type InformerService interface { - GetMasterUserRecord(name string) (*toolchainv1alpha1.MasterUserRecord, error) - GetSpace(name string) (*toolchainv1alpha1.Space, error) - ListSpaceBindings(reqs ...labels.Requirement) ([]toolchainv1alpha1.SpaceBinding, error) - GetNSTemplateTier(name string) (*toolchainv1alpha1.NSTemplateTier, error) - ListBannedUsersByEmail(email string) ([]toolchainv1alpha1.BannedUser, error) -} - type SignupService interface { Signup(ctx *gin.Context) (*toolchainv1alpha1.UserSignup, error) GetSignup(ctx *gin.Context, userID, username string) (*signup.Signup, error) @@ -25,10 +16,6 @@ type SignupService interface { PhoneNumberAlreadyInUse(userID, username, phoneNumberOrHash string) error } -type SocialEventService interface { - GetEvent(code string) (*toolchainv1alpha1.SocialEvent, error) -} - type VerificationService interface { InitVerification(ctx *gin.Context, userID, username, e164PhoneNumber, countryCode string) error VerifyPhoneCode(ctx *gin.Context, userID, username, code string) error @@ -40,7 +27,6 @@ type MemberClusterService interface { } type Services interface { - InformerService() InformerService SignupService() SignupService VerificationService() VerificationService MemberClusterService() MemberClusterService diff --git a/pkg/controller/usernames.go b/pkg/controller/usernames.go index 573f00d6..62a7dbe2 100644 --- a/pkg/controller/usernames.go +++ b/pkg/controller/usernames.go @@ -3,9 +3,10 @@ package controller import ( "net/http" - "github.com/codeready-toolchain/registration-service/pkg/application" + toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" crterrors "github.com/codeready-toolchain/registration-service/pkg/errors" "github.com/codeready-toolchain/registration-service/pkg/log" + "github.com/codeready-toolchain/registration-service/pkg/namespaced" "github.com/codeready-toolchain/registration-service/pkg/username" "github.com/gin-gonic/gin" "k8s.io/apimachinery/pkg/api/errors" @@ -13,13 +14,13 @@ import ( // Usernames implements the usernames endpoint, which is invoked for checking if a given username/email exists. type Usernames struct { - app application.Application + namespaced.Client } // NewUsernames returns a new Usernames instance. -func NewUsernames(app application.Application) *Usernames { +func NewUsernames(nsClient namespaced.Client) *Usernames { return &Usernames{ - app: app, + Client: nsClient, } } @@ -34,7 +35,8 @@ func (s *Usernames) GetHandler(ctx *gin.Context) { // TODO check if the queryString is an email // in that case we have to fetch the UserSignup resources with the provided email and the MasterUserRecords associated with those. - murResource, err := s.app.InformerService().GetMasterUserRecord(queryString) + murResource := &toolchainv1alpha1.MasterUserRecord{} + err := s.Get(ctx.Request.Context(), s.NamespacedName(queryString), murResource) // handle not found error if errors.IsNotFound(err) { log.Infof(ctx, "MasterUserRecord resource for: %s not found", queryString) diff --git a/pkg/controller/usernames_test.go b/pkg/controller/usernames_test.go index 3fe507ff..2b5467ca 100644 --- a/pkg/controller/usernames_test.go +++ b/pkg/controller/usernames_test.go @@ -9,7 +9,7 @@ import ( "testing" "github.com/codeready-toolchain/registration-service/pkg/controller" - "github.com/codeready-toolchain/registration-service/pkg/informers/service" + "github.com/codeready-toolchain/registration-service/pkg/namespaced" "github.com/codeready-toolchain/registration-service/pkg/username" "github.com/codeready-toolchain/registration-service/test" "github.com/codeready-toolchain/registration-service/test/fake" @@ -40,11 +40,10 @@ func (s *TestUsernamesSuite) TestUsernamesGetHandler() { s.Run("success", func() { - fakeInformer := service.NewInformerService(fakeClient, commontest.HostOperatorNs) - s.Application.MockInformerService(fakeInformer) + nsClient := namespaced.NewClient(fakeClient, commontest.HostOperatorNs) // Create Usernames controller instance. - ctrl := controller.NewUsernames(s.Application) + ctrl := controller.NewUsernames(nsClient) handler := gin.HandlerFunc(ctrl.GetHandler) s.Run("usernames found", func() { @@ -104,11 +103,10 @@ func (s *TestUsernamesSuite) TestUsernamesGetHandler() { fakeClient.MockGet = func(_ context.Context, _ client.ObjectKey, _ client.Object, _ ...client.GetOption) error { return fmt.Errorf("mock error") } - fakeInformer := service.NewInformerService(fakeClient, commontest.HostOperatorNs) - s.Application.MockInformerService(fakeInformer) + nsClient := namespaced.NewClient(fakeClient, commontest.HostOperatorNs) // Create Usernames controller instance. - ctrl := controller.NewUsernames(s.Application) + ctrl := controller.NewUsernames(nsClient) handler := gin.HandlerFunc(ctrl.GetHandler) s.Run("unable to get mur", func() { // We create a ResponseRecorder (which satisfies http.ResponseWriter) to record the response. diff --git a/pkg/informers/service/informer_service.go b/pkg/informers/service/informer_service.go deleted file mode 100644 index 269d1952..00000000 --- a/pkg/informers/service/informer_service.go +++ /dev/null @@ -1,76 +0,0 @@ -package service - -import ( - "context" - - toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" - "github.com/codeready-toolchain/registration-service/pkg/application/service" - "github.com/codeready-toolchain/toolchain-common/pkg/hash" - - "k8s.io/apimachinery/pkg/labels" - "k8s.io/apimachinery/pkg/types" - "sigs.k8s.io/controller-runtime/pkg/client" -) - -type Option func(f *ServiceImpl) - -// ServiceImpl represents the implementation of the informer service. -type ServiceImpl struct { // nolint:revive - client client.Client - namespace string -} - -// NewInformerService creates a service object for getting resources -func NewInformerService(client client.Client, namespace string) service.InformerService { - si := &ServiceImpl{ - client: client, - namespace: namespace, - } - return si -} - -func (s *ServiceImpl) GetMasterUserRecord(name string) (*toolchainv1alpha1.MasterUserRecord, error) { - mur := &toolchainv1alpha1.MasterUserRecord{} - namespacedName := types.NamespacedName{Name: name, Namespace: s.namespace} - err := s.client.Get(context.TODO(), namespacedName, mur) - return mur, err -} - -func (s *ServiceImpl) GetSpace(name string) (*toolchainv1alpha1.Space, error) { - space := &toolchainv1alpha1.Space{} - namespacedName := types.NamespacedName{Name: name, Namespace: s.namespace} - err := s.client.Get(context.TODO(), namespacedName, space) - return space, err -} - -func (s *ServiceImpl) ListSpaceBindings(reqs ...labels.Requirement) ([]toolchainv1alpha1.SpaceBinding, error) { - selector := labels.NewSelector() - for i := range reqs { - selector = selector.Add(reqs[i]) - } - - bindings := &toolchainv1alpha1.SpaceBindingList{} - if err := s.client.List(context.TODO(), bindings, client.InNamespace(s.namespace), client.MatchingLabelsSelector{Selector: selector}); err != nil { - return nil, err - } - return bindings.Items, nil -} - -func (s *ServiceImpl) GetNSTemplateTier(name string) (*toolchainv1alpha1.NSTemplateTier, error) { - tier := &toolchainv1alpha1.NSTemplateTier{} - namespacedName := types.NamespacedName{Name: name, Namespace: s.namespace} - err := s.client.Get(context.TODO(), namespacedName, tier) - return tier, err -} - -func (s *ServiceImpl) ListBannedUsersByEmail(email string) ([]toolchainv1alpha1.BannedUser, error) { - hashedEmail := hash.EncodeString(email) - - bannedUsers := &toolchainv1alpha1.BannedUserList{} - if err := s.client.List(context.TODO(), bannedUsers, client.InNamespace(s.namespace), - client.MatchingLabels{toolchainv1alpha1.BannedUserEmailHashLabelKey: hashedEmail}); err != nil { - return nil, err - } - - return bannedUsers.Items, nil -} diff --git a/pkg/informers/service/informer_service_test.go b/pkg/informers/service/informer_service_test.go deleted file mode 100644 index 832bf78e..00000000 --- a/pkg/informers/service/informer_service_test.go +++ /dev/null @@ -1,180 +0,0 @@ -package service_test - -import ( - "testing" - - toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" - "github.com/codeready-toolchain/registration-service/pkg/informers/service" - "github.com/codeready-toolchain/toolchain-common/pkg/hash" - "github.com/codeready-toolchain/toolchain-common/pkg/test" - "github.com/codeready-toolchain/toolchain-common/pkg/test/masteruserrecord" - "github.com/codeready-toolchain/toolchain-common/pkg/test/space" - "github.com/codeready-toolchain/toolchain-common/pkg/test/usersignup" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" -) - -func TestInformerService(t *testing.T) { - // given - murJohn := masteruserrecord.NewMasterUserRecord(t, "johnMur") - murNoise := masteruserrecord.NewMasterUserRecord(t, "noise") - spaceJohn := space.NewSpace(test.HostOperatorNs, "johnSpace") - spaceNoise := space.NewSpace(test.HostOperatorNs, "noiseSpace") - pluginTekton := &toolchainv1alpha1.ProxyPlugin{ObjectMeta: metav1.ObjectMeta{ - Name: "tekton-results", - Namespace: test.HostOperatorNs, - }} - pluginNoise := &toolchainv1alpha1.ProxyPlugin{ObjectMeta: metav1.ObjectMeta{ - Name: "noise", - Namespace: test.HostOperatorNs, - }} - status := &toolchainv1alpha1.ToolchainStatus{ - ObjectMeta: metav1.ObjectMeta{ - Name: "toolchain-status", - Namespace: test.HostOperatorNs, - }, - Status: toolchainv1alpha1.ToolchainStatusStatus{ - HostOperator: &toolchainv1alpha1.HostOperatorStatus{ - Version: "v1alpha1", - }, - }, - } - signupJohn := usersignup.NewUserSignup(usersignup.WithName("johnUserSignup"), - usersignup.WithTargetCluster("member2")) - signupJohn.Spec.IdentityClaims = toolchainv1alpha1.IdentityClaimsEmbedded{ - PropagatedClaims: toolchainv1alpha1.PropagatedClaims{ - Sub: "foo", - OriginalSub: "sub-key", - }, - PreferredUsername: "foo@redhat.com", - GivenName: "Foo", - FamilyName: "Bar", - Company: "Red Hat", - } - signupNoise := usersignup.NewUserSignup(usersignup.WithName("noise")) - bannedAlice := &toolchainv1alpha1.BannedUser{ - ObjectMeta: metav1.ObjectMeta{ - Name: "alice", - Namespace: test.HostOperatorNs, - Labels: map[string]string{ - toolchainv1alpha1.BannedUserEmailHashLabelKey: hash.EncodeString("alice@redhat.com"), - }, - }, - Spec: toolchainv1alpha1.BannedUserSpec{ - Email: "alice@redhat.com", - }, - } - bannedBob := &toolchainv1alpha1.BannedUser{ - ObjectMeta: metav1.ObjectMeta{ - Name: "bob", - Namespace: test.HostOperatorNs, - Labels: map[string]string{ - toolchainv1alpha1.BannedUserEmailHashLabelKey: hash.EncodeString("bob@redhat.com"), - }, - }, - Spec: toolchainv1alpha1.BannedUserSpec{ - Email: "bob@redhat.com", - }, - } - bannedBobDup := &toolchainv1alpha1.BannedUser{ - ObjectMeta: metav1.ObjectMeta{ - Name: "bob-dup", - Namespace: test.HostOperatorNs, - Labels: map[string]string{ - toolchainv1alpha1.BannedUserEmailHashLabelKey: hash.EncodeString("bob@redhat.com"), - }, - }, - Spec: toolchainv1alpha1.BannedUserSpec{ - Email: "bob@redhat.com", - }, - } - - client := test.NewFakeClient(t, murJohn, murNoise, spaceJohn, spaceNoise, pluginTekton, - pluginNoise, status, signupJohn, signupNoise, bannedAlice, bannedBob, bannedBobDup) - svc := service.NewInformerService(client, test.HostOperatorNs) - - t.Run("masteruserrecords", func(t *testing.T) { - t.Run("not found", func(t *testing.T) { - // when - val, err := svc.GetMasterUserRecord("unknown") - - //then - assert.Empty(t, val) - assert.EqualError(t, err, "masteruserrecords.toolchain.dev.openshift.com \"unknown\" not found") - }) - - t.Run("found", func(t *testing.T) { - // when - val, err := svc.GetMasterUserRecord("johnMur") - - // then - require.NotNil(t, val) - require.NoError(t, err) - assert.Equal(t, murJohn.Spec, val.Spec) - }) - }) - - t.Run("spaces", func(t *testing.T) { - t.Run("not found", func(t *testing.T) { - // when - val, err := svc.GetSpace("unknown") - - // then - assert.Empty(t, val) - assert.EqualError(t, err, "spaces.toolchain.dev.openshift.com \"unknown\" not found") - }) - - t.Run("found", func(t *testing.T) { - // when - val, err := svc.GetSpace("johnSpace") - - // then - require.NotNil(t, val) - require.NoError(t, err) - assert.Equal(t, spaceJohn.Spec, val.Spec) - }) - }) - - t.Run("bannedusers", func(t *testing.T) { - t.Run("not found", func(t *testing.T) { - // when - rbb, err := svc.ListBannedUsersByEmail("unknown@unknown.com") - - // then - require.NoError(t, err) - require.Empty(t, rbb) - }) - - t.Run("invalid email", func(t *testing.T) { - // when - rbb, err := svc.ListBannedUsersByEmail("not-an-email") - - // then - require.NoError(t, err) - require.Empty(t, rbb) - }) - - t.Run("found one", func(t *testing.T) { - // when - rbb, err := svc.ListBannedUsersByEmail(bannedAlice.Spec.Email) - - // then - require.NotNil(t, rbb) - require.NoError(t, err) - require.Len(t, rbb, 1, "expected 1 result for email %s", bannedAlice.Spec.Email) - require.Equal(t, *bannedAlice, rbb[0]) - }) - - t.Run("found multiple", func(t *testing.T) { - // when - rbb, err := svc.ListBannedUsersByEmail(bannedBob.Spec.Email) - - // then - require.NotNil(t, rbb) - require.NoError(t, err) - require.Len(t, rbb, 2, "expected 2 results for email %s", bannedBob.Spec.Email) - require.ElementsMatch(t, []toolchainv1alpha1.BannedUser{*bannedBob, *bannedBobDup}, rbb) - }) - }) -} diff --git a/pkg/middleware/jwt_middleware_test.go b/pkg/middleware/jwt_middleware_test.go index 98d6262c..780d3aa3 100644 --- a/pkg/middleware/jwt_middleware_test.go +++ b/pkg/middleware/jwt_middleware_test.go @@ -10,11 +10,13 @@ import ( "github.com/codeready-toolchain/registration-service/pkg/configuration" "github.com/codeready-toolchain/registration-service/pkg/middleware" + "github.com/codeready-toolchain/registration-service/pkg/namespaced" "github.com/codeready-toolchain/registration-service/pkg/proxy" "github.com/codeready-toolchain/registration-service/pkg/server" "github.com/codeready-toolchain/registration-service/test" "github.com/codeready-toolchain/registration-service/test/fake" "github.com/codeready-toolchain/toolchain-common/pkg/status" + commontest "github.com/codeready-toolchain/toolchain-common/pkg/test" authsupport "github.com/codeready-toolchain/toolchain-common/pkg/test/auth" testconfig "github.com/codeready-toolchain/toolchain-common/pkg/test/config" @@ -86,7 +88,8 @@ func (s *TestAuthMiddlewareSuite) TestAuthMiddlewareService() { assert.Equal(s.T(), keysEndpointURL, cfg.Auth().AuthClientPublicKeysURL(), "key url not set correctly") // Setting up the routes. - err = srv.SetupRoutes(proxy.DefaultPort, prometheus.NewRegistry()) + nsClient := namespaced.NewClient(commontest.NewFakeClient(s.T()), commontest.HostOperatorNs) + err = srv.SetupRoutes(proxy.DefaultPort, prometheus.NewRegistry(), nsClient) require.NoError(s.T(), err) // Check that there are routes registered. diff --git a/pkg/middleware/promhttp_middleware_test.go b/pkg/middleware/promhttp_middleware_test.go index fd716caa..45c3c4e8 100644 --- a/pkg/middleware/promhttp_middleware_test.go +++ b/pkg/middleware/promhttp_middleware_test.go @@ -7,10 +7,12 @@ import ( "testing" "github.com/codeready-toolchain/registration-service/pkg/configuration" + "github.com/codeready-toolchain/registration-service/pkg/namespaced" "github.com/codeready-toolchain/registration-service/pkg/proxy" "github.com/codeready-toolchain/registration-service/pkg/server" "github.com/codeready-toolchain/registration-service/test" "github.com/codeready-toolchain/registration-service/test/fake" + commontest "github.com/codeready-toolchain/toolchain-common/pkg/test" authsupport "github.com/codeready-toolchain/toolchain-common/pkg/test/auth" testconfig "github.com/codeready-toolchain/toolchain-common/pkg/test/config" @@ -42,7 +44,8 @@ func (s *PromHTTPMiddlewareSuite) TestPromHTTPMiddleware() { cfg := configuration.GetRegistrationServiceConfig() assert.Equal(s.T(), keysEndpointURL, cfg.Auth().AuthClientPublicKeysURL(), "key url not set correctly") reg := prometheus.NewRegistry() - err := srv.SetupRoutes(proxy.DefaultPort, reg) + nsClient := namespaced.NewClient(commontest.NewFakeClient(s.T()), commontest.HostOperatorNs) + err := srv.SetupRoutes(proxy.DefaultPort, reg, nsClient) require.NoError(s.T(), err) // making a call on an HTTP endpoint diff --git a/pkg/proxy/handlers/spacelister.go b/pkg/proxy/handlers/spacelister.go index b2b182d2..51779c37 100644 --- a/pkg/proxy/handlers/spacelister.go +++ b/pkg/proxy/handlers/spacelister.go @@ -5,8 +5,8 @@ import ( toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" "github.com/codeready-toolchain/registration-service/pkg/application" - "github.com/codeready-toolchain/registration-service/pkg/application/service" "github.com/codeready-toolchain/registration-service/pkg/context" + "github.com/codeready-toolchain/registration-service/pkg/namespaced" "github.com/codeready-toolchain/registration-service/pkg/proxy/metrics" "github.com/codeready-toolchain/registration-service/pkg/signup" commonproxy "github.com/codeready-toolchain/toolchain-common/pkg/proxy" @@ -27,16 +27,16 @@ const ( ) type SpaceLister struct { - GetSignupFunc func(ctx *gin.Context, userID, username string, checkUserSignupCompleted bool) (*signup.Signup, error) - GetInformerServiceFunc func() service.InformerService - ProxyMetrics *metrics.ProxyMetrics + namespaced.Client + GetSignupFunc func(ctx *gin.Context, userID, username string, checkUserSignupCompleted bool) (*signup.Signup, error) + ProxyMetrics *metrics.ProxyMetrics } -func NewSpaceLister(app application.Application, proxyMetrics *metrics.ProxyMetrics) *SpaceLister { +func NewSpaceLister(client namespaced.Client, app application.Application, proxyMetrics *metrics.ProxyMetrics) *SpaceLister { return &SpaceLister{ - GetSignupFunc: app.SignupService().GetSignupFromInformer, - GetInformerServiceFunc: app.InformerService, - ProxyMetrics: proxyMetrics, + Client: client, + GetSignupFunc: app.SignupService().GetSignupFromInformer, + ProxyMetrics: proxyMetrics, } } diff --git a/pkg/proxy/handlers/spacelister_get.go b/pkg/proxy/handlers/spacelister_get.go index 576ac5c9..8412c99a 100644 --- a/pkg/proxy/handlers/spacelister_get.go +++ b/pkg/proxy/handlers/spacelister_get.go @@ -1,6 +1,7 @@ package handlers import ( + gocontext "context" "encoding/json" "fmt" "net/http" @@ -10,6 +11,7 @@ import ( toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" "github.com/codeready-toolchain/registration-service/pkg/context" regsercontext "github.com/codeready-toolchain/registration-service/pkg/context" + "github.com/codeready-toolchain/registration-service/pkg/namespaced" "github.com/codeready-toolchain/registration-service/pkg/proxy/metrics" "github.com/codeready-toolchain/registration-service/pkg/signup" "github.com/codeready-toolchain/toolchain-common/pkg/cluster" @@ -18,7 +20,6 @@ import ( "github.com/labstack/echo/v4" errs "github.com/pkg/errors" apierrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime/schema" runtimeclient "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -104,12 +105,7 @@ func getUserOrPublicViewerSpaceBinding(ctx echo.Context, spaceLister *SpaceListe // If more than one space bindings are found, then it returns an error. func getUserSpaceBinding(spaceLister *SpaceLister, space *toolchainv1alpha1.Space, compliantUsername string) (*toolchainv1alpha1.SpaceBinding, error) { // recursively get all the spacebindings for the current workspace - listSpaceBindingsFunc := func(spaceName string) ([]toolchainv1alpha1.SpaceBinding, error) { - spaceSelector, _ := labels.SelectorFromSet(labels.Set{toolchainv1alpha1.SpaceBindingSpaceLabelKey: spaceName}).Requirements() - murSelector, _ := labels.SelectorFromSet(labels.Set{toolchainv1alpha1.SpaceBindingMasterUserRecordLabelKey: compliantUsername}).Requirements() - return spaceLister.GetInformerServiceFunc().ListSpaceBindings(spaceSelector[0], murSelector[0]) - } - spaceBindingLister := spacebinding.NewLister(listSpaceBindingsFunc, spaceLister.GetInformerServiceFunc().GetSpace) + spaceBindingLister := NewLister(spaceLister.Client, compliantUsername) userSpaceBindings, err := spaceBindingLister.ListForSpace(space, []toolchainv1alpha1.SpaceBinding{}) if err != nil { return nil, err @@ -127,6 +123,26 @@ func getUserSpaceBinding(spaceLister *SpaceLister, space *toolchainv1alpha1.Spac return &userSpaceBindings[0], nil } +func NewLister(nsClient namespaced.Client, withMurName string) *spacebinding.Lister { + listSpaceBindingsFunc := func(spaceName string) ([]toolchainv1alpha1.SpaceBinding, error) { + labelSelector := runtimeclient.MatchingLabels{ + toolchainv1alpha1.SpaceBindingSpaceLabelKey: spaceName, + } + if withMurName != "" { + labelSelector[toolchainv1alpha1.SpaceBindingMasterUserRecordLabelKey] = withMurName + } + bindings := &toolchainv1alpha1.SpaceBindingList{} + if err := nsClient.List(gocontext.TODO(), bindings, runtimeclient.InNamespace(nsClient.Namespace), labelSelector); err != nil { + return nil, err + } + return bindings.Items, nil + } + return spacebinding.NewLister(listSpaceBindingsFunc, func(spaceName string) (*toolchainv1alpha1.Space, error) { + space := &toolchainv1alpha1.Space{} + return space, nsClient.Get(gocontext.TODO(), nsClient.NamespacedName(spaceName), space) + }) +} + // GetUserWorkspaceWithBindings returns a workspace object with the required fields+bindings (the list with all the users access details). func GetUserWorkspaceWithBindings(ctx echo.Context, spaceLister *SpaceLister, workspaceName string, GetMembersFunc cluster.GetMemberClustersFunc) (*toolchainv1alpha1.Workspace, error) { userSignup, space, err := getUserSignupAndSpace(ctx, spaceLister, workspaceName) @@ -139,11 +155,7 @@ func GetUserWorkspaceWithBindings(ctx echo.Context, spaceLister *SpaceLister, wo } // recursively get all the spacebindings for the current workspace - listSpaceBindingsFunc := func(spaceName string) ([]toolchainv1alpha1.SpaceBinding, error) { - ss, _ := labels.SelectorFromSet(labels.Set{toolchainv1alpha1.SpaceBindingSpaceLabelKey: spaceName}).Requirements() - return spaceLister.GetInformerServiceFunc().ListSpaceBindings(ss[0]) - } - spaceBindingLister := spacebinding.NewLister(listSpaceBindingsFunc, spaceLister.GetInformerServiceFunc().GetSpace) + spaceBindingLister := NewLister(spaceLister.Client, "") allSpaceBindings, err := spaceBindingLister.ListForSpace(space, []toolchainv1alpha1.SpaceBinding{}) if err != nil { ctx.Logger().Error(err, "failed to list space bindings") @@ -181,8 +193,8 @@ func GetUserWorkspaceWithBindings(ctx echo.Context, spaceLister *SpaceLister, wo } // add available roles, this field is populated only for the GET workspace request - nsTemplateTier, err := spaceLister.GetInformerServiceFunc().GetNSTemplateTier(space.Spec.TierName) - if err != nil { + nsTemplateTier := &toolchainv1alpha1.NSTemplateTier{} + if err := spaceLister.Get(ctx.Request().Context(), spaceLister.NamespacedName(space.Spec.TierName), nsTemplateTier); err != nil { ctx.Logger().Error(errs.Wrap(err, "unable to get nstemplatetier")) return nil, err } @@ -206,9 +218,8 @@ func getUserSignupAndSpace(ctx echo.Context, spaceLister *SpaceLister, workspace Name: toolchainv1alpha1.KubesawAuthenticatedUsername, } } - - space, err := spaceLister.GetInformerServiceFunc().GetSpace(workspaceName) - if err != nil { + space := &toolchainv1alpha1.Space{} + if err := spaceLister.Get(gocontext.TODO(), spaceLister.NamespacedName(workspaceName), space); err != nil { ctx.Logger().Error(errs.Wrap(err, "unable to get space")) return userSignup, nil, nil } diff --git a/pkg/proxy/handlers/spacelister_get_test.go b/pkg/proxy/handlers/spacelister_get_test.go index 762cd3e1..2643f111 100644 --- a/pkg/proxy/handlers/spacelister_get_test.go +++ b/pkg/proxy/handlers/spacelister_get_test.go @@ -10,9 +10,8 @@ import ( "time" toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" - "github.com/codeready-toolchain/registration-service/pkg/application/service" rcontext "github.com/codeready-toolchain/registration-service/pkg/context" - infservice "github.com/codeready-toolchain/registration-service/pkg/informers/service" + "github.com/codeready-toolchain/registration-service/pkg/namespaced" "github.com/codeready-toolchain/registration-service/pkg/proxy/handlers" "github.com/codeready-toolchain/registration-service/pkg/proxy/metrics" proxytest "github.com/codeready-toolchain/registration-service/pkg/proxy/test" @@ -557,11 +556,9 @@ func testSpaceListerGet(t *testing.T, publicViewerEnabled bool) { proxyMetrics := metrics.NewProxyMetrics(prometheus.NewRegistry()) s := &handlers.SpaceLister{ + Client: namespaced.NewClient(fakeClient, test.HostOperatorNs), GetSignupFunc: signupProvider, - GetInformerServiceFunc: func() service.InformerService { - return infservice.NewInformerService(fakeClient, test.HostOperatorNs) - }, - ProxyMetrics: proxyMetrics, + ProxyMetrics: proxyMetrics, } e := echo.New() @@ -734,11 +731,9 @@ func TestGetUserWorkspace(t *testing.T) { proxyMetrics := metrics.NewProxyMetrics(prometheus.NewRegistry()) s := &handlers.SpaceLister{ + Client: namespaced.NewClient(fakeClient, test.HostOperatorNs), GetSignupFunc: signupProvider, - GetInformerServiceFunc: func() service.InformerService { - return infservice.NewInformerService(fakeClient, test.HostOperatorNs) - }, - ProxyMetrics: proxyMetrics, + ProxyMetrics: proxyMetrics, } e := echo.New() @@ -843,11 +838,9 @@ func TestSpaceListerGetPublicViewerEnabled(t *testing.T) { proxyMetrics := metrics.NewProxyMetrics(prometheus.NewRegistry()) s := &handlers.SpaceLister{ + Client: namespaced.NewClient(fakeClient, test.HostOperatorNs), GetSignupFunc: signupProvider, - GetInformerServiceFunc: func() service.InformerService { - return infservice.NewInformerService(fakeClient, test.HostOperatorNs) - }, - ProxyMetrics: proxyMetrics, + ProxyMetrics: proxyMetrics, } e := echo.New() @@ -980,11 +973,9 @@ func TestGetUserWorkspaceWithBindingsWithPublicViewerEnabled(t *testing.T) { proxyMetrics := metrics.NewProxyMetrics(prometheus.NewRegistry()) s := &handlers.SpaceLister{ + Client: namespaced.NewClient(fakeClient, test.HostOperatorNs), GetSignupFunc: signupProvider, - GetInformerServiceFunc: func() service.InformerService { - return infservice.NewInformerService(fakeClient, test.HostOperatorNs) - }, - ProxyMetrics: proxyMetrics, + ProxyMetrics: proxyMetrics, } e := echo.New() diff --git a/pkg/proxy/handlers/spacelister_list.go b/pkg/proxy/handlers/spacelister_list.go index afa51985..dfbd8498 100644 --- a/pkg/proxy/handlers/spacelister_list.go +++ b/pkg/proxy/handlers/spacelister_list.go @@ -1,6 +1,7 @@ package handlers import ( + gocontext "context" "encoding/json" "fmt" "net/http" @@ -16,6 +17,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/selection" + runtimeclient "sigs.k8s.io/controller-runtime/pkg/client" ) func HandleSpaceListRequest(spaceLister *SpaceLister) echo.HandlerFunc { @@ -90,7 +92,12 @@ func listSpaceBindingsForUsers(spaceLister *SpaceLister, murNames []string) ([]t if err != nil { return nil, err } - return spaceLister.GetInformerServiceFunc().ListSpaceBindings(*murSelector) + selector := runtimeclient.MatchingLabelsSelector{Selector: labels.NewSelector().Add(*murSelector)} + bindings := &toolchainv1alpha1.SpaceBindingList{} + if err := spaceLister.List(gocontext.TODO(), bindings, runtimeclient.InNamespace(spaceLister.Namespace), selector); err != nil { + return nil, err + } + return bindings.Items, err } func workspacesFromSpaceBindings(ctx echo.Context, spaceLister *SpaceLister, signupName string, spaceBindings []toolchainv1alpha1.SpaceBinding) []toolchainv1alpha1.Workspace { @@ -116,5 +123,6 @@ func getSpace(spaceLister *SpaceLister, spaceBinding *toolchainv1alpha1.SpaceBin // log error and continue so that the api behaves in a best effort manner return nil, fmt.Errorf("spacebinding has no '%s' label", toolchainv1alpha1.SpaceBindingSpaceLabelKey) } - return spaceLister.GetInformerServiceFunc().GetSpace(spaceName) + space := &toolchainv1alpha1.Space{} + return space, spaceLister.Get(gocontext.TODO(), spaceLister.NamespacedName(spaceName), space) } diff --git a/pkg/proxy/handlers/spacelister_list_test.go b/pkg/proxy/handlers/spacelister_list_test.go index 7ca5eff4..3aa9c73d 100644 --- a/pkg/proxy/handlers/spacelister_list_test.go +++ b/pkg/proxy/handlers/spacelister_list_test.go @@ -9,7 +9,7 @@ import ( "testing" "time" - infservice "github.com/codeready-toolchain/registration-service/pkg/informers/service" + "github.com/codeready-toolchain/registration-service/pkg/namespaced" "github.com/gin-gonic/gin" "github.com/labstack/echo/v4" "github.com/prometheus/client_golang/prometheus" @@ -19,7 +19,6 @@ import ( runtimeclient "sigs.k8s.io/controller-runtime/pkg/client" toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" - "github.com/codeready-toolchain/registration-service/pkg/application/service" rcontext "github.com/codeready-toolchain/registration-service/pkg/context" "github.com/codeready-toolchain/registration-service/pkg/proxy/handlers" "github.com/codeready-toolchain/registration-service/pkg/proxy/metrics" @@ -77,11 +76,9 @@ func TestListUserWorkspaces(t *testing.T) { proxyMetrics := metrics.NewProxyMetrics(prometheus.NewRegistry()) s := &handlers.SpaceLister{ + Client: namespaced.NewClient(fakeClient, test.HostOperatorNs), GetSignupFunc: signupProvider, - GetInformerServiceFunc: func() service.InformerService { - return infservice.NewInformerService(fakeClient, test.HostOperatorNs) - }, - ProxyMetrics: proxyMetrics, + ProxyMetrics: proxyMetrics, } e := echo.New() @@ -207,11 +204,9 @@ func TestHandleSpaceListRequest(t *testing.T) { proxyMetrics := metrics.NewProxyMetrics(prometheus.NewRegistry()) s := &handlers.SpaceLister{ + Client: namespaced.NewClient(fakeClient, test.HostOperatorNs), GetSignupFunc: signupProvider, - GetInformerServiceFunc: func() service.InformerService { - return infservice.NewInformerService(fakeClient, test.HostOperatorNs) - }, - ProxyMetrics: proxyMetrics, + ProxyMetrics: proxyMetrics, } e := echo.New() diff --git a/pkg/proxy/proxy.go b/pkg/proxy/proxy.go index e9425456..b76ed983 100644 --- a/pkg/proxy/proxy.go +++ b/pkg/proxy/proxy.go @@ -23,16 +23,19 @@ import ( "github.com/codeready-toolchain/registration-service/pkg/context" crterrors "github.com/codeready-toolchain/registration-service/pkg/errors" "github.com/codeready-toolchain/registration-service/pkg/log" + "github.com/codeready-toolchain/registration-service/pkg/namespaced" "github.com/codeready-toolchain/registration-service/pkg/proxy/access" "github.com/codeready-toolchain/registration-service/pkg/proxy/handlers" "github.com/codeready-toolchain/registration-service/pkg/proxy/metrics" "github.com/codeready-toolchain/registration-service/pkg/signup" commoncluster "github.com/codeready-toolchain/toolchain-common/pkg/cluster" + "github.com/codeready-toolchain/toolchain-common/pkg/hash" "github.com/labstack/echo/v4" "github.com/labstack/echo/v4/middleware" glog "github.com/labstack/gommon/log" errs "github.com/pkg/errors" "k8s.io/apimachinery/pkg/util/httpstream" + "sigs.k8s.io/controller-runtime/pkg/client" "k8s.io/apiserver/pkg/util/wsstream" ) @@ -60,6 +63,7 @@ func authorizationEndpointTarget() string { } type Proxy struct { + namespaced.Client app application.Application tokenParser *auth.TokenParser spaceLister *handlers.SpaceLister @@ -67,15 +71,16 @@ type Proxy struct { getMembersFunc commoncluster.GetMemberClustersFunc } -func NewProxy(app application.Application, proxyMetrics *metrics.ProxyMetrics, getMembersFunc commoncluster.GetMemberClustersFunc) (*Proxy, error) { +func NewProxy(nsClient namespaced.Client, app application.Application, proxyMetrics *metrics.ProxyMetrics, getMembersFunc commoncluster.GetMemberClustersFunc) (*Proxy, error) { tokenParser, err := auth.DefaultTokenParser() if err != nil { return nil, err } // init handlers - spaceLister := handlers.NewSpaceLister(app, proxyMetrics) + spaceLister := handlers.NewSpaceLister(nsClient, app, proxyMetrics) return &Proxy{ + Client: nsClient, app: app, tokenParser: tokenParser, spaceLister: spaceLister, @@ -352,7 +357,8 @@ func (p *Proxy) checkUserIsProvisionedAndSpaceExists(ctx echo.Context, userID, u // checkSpaceExists checks whether the Space exists. func (p *Proxy) checkSpaceExists(workspaceName string) error { - if _, err := p.app.InformerService().GetSpace(workspaceName); err != nil { + space := &toolchainv1alpha1.Space{} + if err := p.Get(gocontext.TODO(), p.NamespacedName(workspaceName), space); err != nil { // log the actual error but do not return it so that it doesn't reveal information about a space that may not belong to the requestor log.Errorf(nil, err, "requested space '%s' does not exist", workspaceName) return fmt.Errorf("access to workspace '%s' is forbidden", workspaceName) @@ -590,14 +596,16 @@ func (p *Proxy) ensureUserIsNotBanned() echo.MiddlewareFunc { } // retrieve banned users - uu, err := p.app.InformerService().ListBannedUsersByEmail(email) - if err != nil { + hashedEmail := hash.EncodeString(email) + bannedUsers := &toolchainv1alpha1.BannedUserList{} + if err := p.List(ctx.Request().Context(), bannedUsers, client.InNamespace(p.Namespace), + client.MatchingLabels{toolchainv1alpha1.BannedUserEmailHashLabelKey: hashedEmail}); err != nil { ctx.Logger().Errorf("error retrieving the list of banned users with email address %s: %v", email, err) return crterrors.NewInternalError(errs.New("user access could not be verified"), "could not define user access") } // if a matching Banned user is found, then user is banned - if len(uu) > 0 { + if len(bannedUsers.Items) > 0 { return crterrors.NewForbiddenError("user access is forbidden", "user access is forbidden") } diff --git a/pkg/proxy/proxy_community_test.go b/pkg/proxy/proxy_community_test.go index 3e59b324..67134e98 100644 --- a/pkg/proxy/proxy_community_test.go +++ b/pkg/proxy/proxy_community_test.go @@ -6,10 +6,7 @@ import ( "net/http/httptest" "time" - appservice "github.com/codeready-toolchain/registration-service/pkg/application/service" "github.com/codeready-toolchain/registration-service/pkg/auth" - infservice "github.com/codeready-toolchain/registration-service/pkg/informers/service" - "github.com/codeready-toolchain/registration-service/pkg/namespaced" "github.com/codeready-toolchain/registration-service/pkg/proxy/handlers" "github.com/codeready-toolchain/registration-service/pkg/signup" "github.com/codeready-toolchain/registration-service/test/fake" @@ -144,23 +141,20 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr ) // configure informer - inf := infservice.NewInformerService(cli, commontest.HostOperatorNs) - nsClient := namespaced.NewClient(cli, commontest.HostOperatorNs) + p.Client.Client = cli // configure Application fakeApp.Err = nil - fakeApp.InformerServiceMock = inf - fakeApp.MemberClusterServiceMock = s.newMemberClusterServiceWithMembers(nsClient, signupService, testServer.URL) + fakeApp.MemberClusterServiceMock = s.newMemberClusterServiceWithMembers(p.Client, signupService, testServer.URL) fakeApp.SignupServiceMock = signupService s.Application.MockSignupService(signupService) - s.Application.MockInformerService(inf) // configure proxy p.spaceLister = &handlers.SpaceLister{ - GetSignupFunc: fakeApp.SignupServiceMock.GetSignupFromInformer, - GetInformerServiceFunc: func() appservice.InformerService { return inf }, - ProxyMetrics: p.metrics, + Client: p.Client, + GetSignupFunc: fakeApp.SignupServiceMock.GetSignupFromInformer, + ProxyMetrics: p.metrics, } // run test cases diff --git a/pkg/proxy/proxy_test.go b/pkg/proxy/proxy_test.go index f803789b..5e8fd5e2 100644 --- a/pkg/proxy/proxy_test.go +++ b/pkg/proxy/proxy_test.go @@ -19,7 +19,6 @@ import ( appservice "github.com/codeready-toolchain/registration-service/pkg/application/service" "github.com/codeready-toolchain/registration-service/pkg/auth" - infservice "github.com/codeready-toolchain/registration-service/pkg/informers/service" "github.com/codeready-toolchain/registration-service/pkg/namespaced" "github.com/codeready-toolchain/registration-service/pkg/proxy/access" "github.com/codeready-toolchain/registration-service/pkg/proxy/handlers" @@ -104,13 +103,11 @@ func (s *TestProxySuite) TestProxy() { } return fakeClient.Client.List(ctx, list, opts...) } - inf := infservice.NewInformerService(fakeClient, commontest.HostOperatorNs) + nsClient := namespaced.NewClient(fakeClient, commontest.HostOperatorNs) - fakeApp := &fake.ProxyFakeApp{ - InformerServiceMock: inf, - } + fakeApp := &fake.ProxyFakeApp{} proxyMetrics := metrics.NewProxyMetrics(prometheus.NewRegistry()) - p, err := NewProxy(fakeApp, proxyMetrics, proxytest.NewGetMembersFunc(commontest.NewFakeClient(s.T()))) + p, err := NewProxy(nsClient, fakeApp, proxyMetrics, proxytest.NewGetMembersFunc(commontest.NewFakeClient(s.T()))) require.NoError(s.T(), err) server := p.StartProxy(DefaultPort) @@ -136,7 +133,7 @@ func (s *TestProxySuite) TestProxy() { func (s *TestProxySuite) spinUpProxy(fakeApp *fake.ProxyFakeApp, port string) (*Proxy, *http.Server) { proxyMetrics := metrics.NewProxyMetrics(prometheus.NewRegistry()) - p, err := NewProxy( + p, err := NewProxy(namespaced.NewClient(commontest.NewFakeClient(s.T()), commontest.HostOperatorNs), fakeApp, proxyMetrics, proxytest.NewGetMembersFunc(commontest.NewFakeClient(s.T()))) require.NoError(s.T(), err) @@ -850,19 +847,13 @@ func (s *TestProxySuite) checkProxyOK(fakeApp *fake.ProxyFakeApp, p *Proxy) { fake.NewSpaceBinding("mycoolworkspace-smith2", "smith2", "mycoolworkspace", "admin"), proxyPlugin, fake.NewBase1NSTemplateTier()) - inf := infservice.NewInformerService(fakeClient, commontest.HostOperatorNs) - nsClient := namespaced.NewClient(fakeClient, commontest.HostOperatorNs) - - s.Application.MockInformerService(inf) - fakeApp.MemberClusterServiceMock = s.newMemberClusterServiceWithMembers(nsClient, fakeApp.SignupServiceMock, testServer.URL) - fakeApp.InformerServiceMock = inf + p.Client.Client = fakeClient + fakeApp.MemberClusterServiceMock = s.newMemberClusterServiceWithMembers(p.Client, fakeApp.SignupServiceMock, testServer.URL) p.spaceLister = &handlers.SpaceLister{ + Client: p.Client, GetSignupFunc: fakeApp.SignupServiceMock.GetSignupFromInformer, - GetInformerServiceFunc: func() appservice.InformerService { - return inf - }, - ProxyMetrics: p.metrics, + ProxyMetrics: p.metrics, } if tc.OverrideGetSignupFunc != nil { p.spaceLister.GetSignupFunc = tc.OverrideGetSignupFunc diff --git a/pkg/proxy/service/cluster_service_test.go b/pkg/proxy/service/cluster_service_test.go index 00fad999..4e05e5d0 100644 --- a/pkg/proxy/service/cluster_service_test.go +++ b/pkg/proxy/service/cluster_service_test.go @@ -7,7 +7,6 @@ import ( "net/url" "testing" - infservice "github.com/codeready-toolchain/registration-service/pkg/informers/service" "github.com/codeready-toolchain/registration-service/pkg/namespaced" "github.com/codeready-toolchain/registration-service/pkg/proxy/access" "github.com/codeready-toolchain/registration-service/pkg/proxy/service" @@ -157,6 +156,7 @@ func (s *TestClusterServiceSuite) TestGetClusterAccess() { s.Run("unable to get space", func() { s.Run("informer service returns error", func() { + fakeClient := commontest.NewFakeClient(s.T()) fakeClient.MockGet = func(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { if _, ok := obj.(*toolchainv1alpha1.Space); ok && key.Name == "smith2" { return fmt.Errorf("oopsi woopsi") @@ -166,8 +166,8 @@ func (s *TestClusterServiceSuite) TestGetClusterAccess() { defer func() { fakeClient.MockGet = nil }() - inf := infservice.NewInformerService(fakeClient, commontest.HostOperatorNs) - s.Application.MockInformerService(inf) + nsClient := namespaced.NewClient(fakeClient, commontest.HostOperatorNs) + svc := service.NewMemberClusterService(nsClient, sc) // when _, err := svc.GetClusterAccess("789-ready", "", "smith2", "", publicViewerEnabled) diff --git a/pkg/server/in_cluster_application.go b/pkg/server/in_cluster_application.go index 60ba20c8..9658050d 100644 --- a/pkg/server/in_cluster_application.go +++ b/pkg/server/in_cluster_application.go @@ -5,18 +5,17 @@ import ( "github.com/codeready-toolchain/registration-service/pkg/application/service" "github.com/codeready-toolchain/registration-service/pkg/application/service/factory" "github.com/codeready-toolchain/registration-service/pkg/namespaced" - "sigs.k8s.io/controller-runtime/pkg/client" ) // NewInClusterApplication creates a new in-cluster application with the specified configuration and options. This // application type is intended to run inside a Kubernetes cluster, where it makes use of the rest.InClusterConfig() // function to determine which Kubernetes configuration to use to create the REST client that interacts with the // Kubernetes service endpoints. -func NewInClusterApplication(client client.Client, namespace string, options ...factory.Option) application.Application { +func NewInClusterApplication(client namespaced.Client, options ...factory.Option) application.Application { return &InClusterApplication{ serviceFactory: factory.NewServiceFactory(append(options, factory.WithServiceContextOptions( - factory.NamespacedClientOption(namespaced.NewClient(client, namespace))))...), + factory.NamespacedClientOption(client)))...), } } @@ -24,10 +23,6 @@ type InClusterApplication struct { serviceFactory *factory.ServiceFactory } -func (r InClusterApplication) InformerService() service.InformerService { - return r.serviceFactory.InformerService() -} - func (r InClusterApplication) SignupService() service.SignupService { return r.serviceFactory.SignupService() } diff --git a/pkg/server/routes.go b/pkg/server/routes.go index b8000926..1882e48f 100644 --- a/pkg/server/routes.go +++ b/pkg/server/routes.go @@ -6,6 +6,7 @@ import ( "github.com/codeready-toolchain/registration-service/pkg/configuration" "github.com/codeready-toolchain/registration-service/pkg/controller" "github.com/codeready-toolchain/registration-service/pkg/middleware" + "github.com/codeready-toolchain/registration-service/pkg/namespaced" "github.com/gin-contrib/static" errs "github.com/pkg/errors" @@ -14,7 +15,7 @@ import ( // SetupRoutes registers handlers for various URL paths. // proxyPort is the API Proxy Server port to be used to setup a route for the health checker for the proxy. -func (srv *RegistrationServer) SetupRoutes(proxyPort string, reg *prometheus.Registry) error { +func (srv *RegistrationServer) SetupRoutes(proxyPort string, reg *prometheus.Registry, nsClient namespaced.Client) error { var err error _, err = auth.InitializeDefaultTokenParser() if err != nil { @@ -53,7 +54,7 @@ func (srv *RegistrationServer) SetupRoutes(proxyPort string, reg *prometheus.Reg authConfigCtrl := controller.NewAuthConfig() analyticsCtrl := controller.NewAnalytics() signupCtrl := controller.NewSignup(srv.application) - usernamesCtrl := controller.NewUsernames(srv.application) + usernamesCtrl := controller.NewUsernames(nsClient) // unsecured routes unsecuredV1 := srv.router.Group("/api/v1") diff --git a/pkg/server/server_test.go b/pkg/server/server_test.go index dc12f33a..fc37e81c 100644 --- a/pkg/server/server_test.go +++ b/pkg/server/server_test.go @@ -6,9 +6,11 @@ import ( "testing" "time" + "github.com/codeready-toolchain/registration-service/pkg/namespaced" "github.com/codeready-toolchain/registration-service/pkg/server" "github.com/codeready-toolchain/registration-service/test" "github.com/codeready-toolchain/registration-service/test/fake" + commontest "github.com/codeready-toolchain/toolchain-common/pkg/test" "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/assert" @@ -38,7 +40,8 @@ func (s *TestServerSuite) TestServer() { fake.MockKeycloakCertsCall(s.T()) // Setting up the routes. - err := srv.SetupRoutes("8091", prometheus.NewRegistry()) // uses a different proxy port than the default one to avoid collision with other concurrent tests + nsClient := namespaced.NewClient(commontest.NewFakeClient(s.T()), commontest.HostOperatorNs) + err := srv.SetupRoutes("8091", prometheus.NewRegistry(), nsClient) // uses a different proxy port than the default one to avoid collision with other concurrent tests require.NoError(s.T(), err) gock.OffAll() diff --git a/test/fake/mockable_application.go b/test/fake/mockable_application.go index 8fed8c2d..c3416b94 100644 --- a/test/fake/mockable_application.go +++ b/test/fake/mockable_application.go @@ -13,7 +13,6 @@ func NewMockableApplication(options ...factory.Option) *MockableApplication { type MockableApplication struct { serviceFactory *factory.ServiceFactory - mockInformerService service.InformerService mockSignupService service.SignupService mockVerificationService service.VerificationService } @@ -39,14 +38,3 @@ func (m *MockableApplication) VerificationService() service.VerificationService func (m *MockableApplication) MemberClusterService() service.MemberClusterService { return m.serviceFactory.MemberClusterService() } - -func (m *MockableApplication) InformerService() service.InformerService { - if m.mockInformerService != nil { - return m.mockInformerService - } - return m.serviceFactory.InformerService() -} - -func (m *MockableApplication) MockInformerService(svc service.InformerService) { - m.mockInformerService = svc -} diff --git a/test/fake/proxy.go b/test/fake/proxy.go index 92133d28..952a29d4 100644 --- a/test/fake/proxy.go +++ b/test/fake/proxy.go @@ -15,14 +15,6 @@ type ProxyFakeApp struct { Err error SignupServiceMock service.SignupService MemberClusterServiceMock service.MemberClusterService - InformerServiceMock service.InformerService -} - -func (a *ProxyFakeApp) InformerService() service.InformerService { - if a.InformerServiceMock != nil { - return a.InformerServiceMock - } - panic("InformerService shouldn't be called") } func (a *ProxyFakeApp) SignupService() service.SignupService { diff --git a/test/util/application.go b/test/util/application.go index 54cfed82..0943eb77 100644 --- a/test/util/application.go +++ b/test/util/application.go @@ -5,6 +5,7 @@ import ( "github.com/codeready-toolchain/registration-service/pkg/application" "github.com/codeready-toolchain/registration-service/pkg/application/service/factory" + "github.com/codeready-toolchain/registration-service/pkg/namespaced" "github.com/codeready-toolchain/registration-service/pkg/server" commontest "github.com/codeready-toolchain/toolchain-common/pkg/test" "k8s.io/apimachinery/pkg/runtime" @@ -17,6 +18,6 @@ func PrepareInClusterApp(t *testing.T, objects ...runtime.Object) (*commontest.F func PrepareInClusterAppWithOption(t *testing.T, option factory.Option, objects ...runtime.Object) (*commontest.FakeClient, application.Application) { fakeClient := commontest.NewFakeClient(t, objects...) - app := server.NewInClusterApplication(fakeClient, commontest.HostOperatorNs, option) + app := server.NewInClusterApplication(namespaced.NewClient(fakeClient, commontest.HostOperatorNs), option) return fakeClient, app } From 3aaf51c461c3fe61048e09dec04bbeb1b47c40f0 Mon Sep 17 00:00:00 2001 From: Matous Jobanek Date: Mon, 14 Oct 2024 13:22:08 +0200 Subject: [PATCH 3/5] drop unnecessary SignupService methods (#480) --- pkg/application/service/services.go | 4 +- pkg/controller/signup.go | 2 +- pkg/controller/signup_test.go | 25 +- pkg/proxy/handlers/spacelister.go | 2 +- pkg/proxy/handlers/spacelister_get_test.go | 8 +- pkg/proxy/handlers/spacelister_list_test.go | 4 +- pkg/proxy/proxy.go | 4 +- pkg/proxy/proxy_community_test.go | 2 +- pkg/proxy/proxy_test.go | 2 +- pkg/proxy/service/cluster_service.go | 2 +- pkg/signup/service/signup_service.go | 25 +- pkg/signup/service/signup_service_test.go | 310 ++---------------- .../service/verification_service.go | 10 +- test/fake/proxy.go | 6 +- test/fake/{informer.go => utils.go} | 0 15 files changed, 50 insertions(+), 356 deletions(-) rename test/fake/{informer.go => utils.go} (100%) diff --git a/pkg/application/service/services.go b/pkg/application/service/services.go index 2b8c2214..5322143a 100644 --- a/pkg/application/service/services.go +++ b/pkg/application/service/services.go @@ -9,10 +9,8 @@ import ( type SignupService interface { Signup(ctx *gin.Context) (*toolchainv1alpha1.UserSignup, error) - GetSignup(ctx *gin.Context, userID, username string) (*signup.Signup, error) - GetSignupFromInformer(ctx *gin.Context, userID, username string, checkUserSignupCompleted bool) (*signup.Signup, error) + GetSignup(ctx *gin.Context, userID, username string, checkUserSignupCompleted bool) (*signup.Signup, error) GetUserSignupFromIdentifier(userID, username string) (*toolchainv1alpha1.UserSignup, error) - UpdateUserSignup(userSignup *toolchainv1alpha1.UserSignup) (*toolchainv1alpha1.UserSignup, error) PhoneNumberAlreadyInUse(userID, username, phoneNumberOrHash string) error } diff --git a/pkg/controller/signup.go b/pkg/controller/signup.go index f9bacd80..66f4914b 100644 --- a/pkg/controller/signup.go +++ b/pkg/controller/signup.go @@ -111,7 +111,7 @@ func (s *Signup) GetHandler(ctx *gin.Context) { // Get the UserSignup resource from the service by the userID userID := ctx.GetString(context.SubKey) username := ctx.GetString(context.UsernameKey) - signupResource, err := s.app.SignupService().GetSignup(ctx, userID, username) + signupResource, err := s.app.SignupService().GetSignup(ctx, userID, username, true) if err != nil { log.Error(ctx, err, "error getting UserSignup resource") crterrors.AbortWithError(ctx, http.StatusInternalServerError, err, "error getting UserSignup resource") diff --git a/pkg/controller/signup_test.go b/pkg/controller/signup_test.go index de5de249..6731037a 100644 --- a/pkg/controller/signup_test.go +++ b/pkg/controller/signup_test.go @@ -207,7 +207,7 @@ func (s *TestSignupSuite) TestSignupGetHandler() { Reason: "Provisioning", }, } - svc.MockGetSignup = func(_ *gin.Context, id, _ string) (*signup.Signup, error) { + svc.MockGetSignup = func(_ *gin.Context, id, _ string, _ bool) (*signup.Signup, error) { if id == userID { return expected, nil } @@ -234,7 +234,7 @@ func (s *TestSignupSuite) TestSignupGetHandler() { ctx.Request = req ctx.Set(context.SubKey, userID) - svc.MockGetSignup = func(_ *gin.Context, _, _ string) (*signup.Signup, error) { + svc.MockGetSignup = func(_ *gin.Context, _, _ string, _ bool) (*signup.Signup, error) { return nil, nil } @@ -251,7 +251,7 @@ func (s *TestSignupSuite) TestSignupGetHandler() { ctx.Request = req ctx.Set(context.SubKey, userID) - svc.MockGetSignup = func(_ *gin.Context, _, _ string) (*signup.Signup, error) { + svc.MockGetSignup = func(_ *gin.Context, _, _ string, _ bool) (*signup.Signup, error) { return nil, errors.New("oopsie woopsie") } @@ -439,9 +439,6 @@ func (s *TestSignupSuite) TestInitVerificationHandler() { states.SetVerificationRequired(&us, true) return &us, nil }, - MockUpdateUserSignup: func(userSignup *crtapi.UserSignup) (userSignup2 *crtapi.UserSignup, e error) { - return userSignup, nil - }, MockPhoneNumberAlreadyInUse: func(_, _, _ string) error { return nil }, @@ -863,20 +860,14 @@ func initActivationCodeVerification(t *testing.T, handler gin.HandlerFunc, usern } type FakeSignupService struct { - MockGetSignup func(ctx *gin.Context, userID, username string) (*signup.Signup, error) - MockGetSignupFromInformer func(ctx *gin.Context, userID, username string, checkUserSignupComplete bool) (*signup.Signup, error) + MockGetSignup func(ctx *gin.Context, userID, username string, checkUserSignupComplete bool) (*signup.Signup, error) MockSignup func(ctx *gin.Context) (*crtapi.UserSignup, error) MockGetUserSignupFromIdentifier func(userID, username string) (*crtapi.UserSignup, error) - MockUpdateUserSignup func(userSignup *crtapi.UserSignup) (*crtapi.UserSignup, error) MockPhoneNumberAlreadyInUse func(userID, username, value string) error } -func (m *FakeSignupService) GetSignup(ctx *gin.Context, userID, username string) (*signup.Signup, error) { - return m.MockGetSignup(ctx, userID, username) -} - -func (m *FakeSignupService) GetSignupFromInformer(ctx *gin.Context, userID, username string, checkUserSignupComplete bool) (*signup.Signup, error) { - return m.MockGetSignupFromInformer(ctx, userID, username, checkUserSignupComplete) +func (m *FakeSignupService) GetSignup(ctx *gin.Context, userID, username string, checkUserSignupComplete bool) (*signup.Signup, error) { + return m.MockGetSignup(ctx, userID, username, checkUserSignupComplete) } func (m *FakeSignupService) Signup(ctx *gin.Context) (*crtapi.UserSignup, error) { @@ -887,10 +878,6 @@ func (m *FakeSignupService) GetUserSignupFromIdentifier(userID, username string) return m.MockGetUserSignupFromIdentifier(userID, username) } -func (m *FakeSignupService) UpdateUserSignup(userSignup *crtapi.UserSignup) (*crtapi.UserSignup, error) { - return m.MockUpdateUserSignup(userSignup) -} - func (m *FakeSignupService) PhoneNumberAlreadyInUse(userID, username, e164phoneNumber string) error { return m.MockPhoneNumberAlreadyInUse(userID, username, e164phoneNumber) } diff --git a/pkg/proxy/handlers/spacelister.go b/pkg/proxy/handlers/spacelister.go index 51779c37..fb1c8b15 100644 --- a/pkg/proxy/handlers/spacelister.go +++ b/pkg/proxy/handlers/spacelister.go @@ -35,7 +35,7 @@ type SpaceLister struct { func NewSpaceLister(client namespaced.Client, app application.Application, proxyMetrics *metrics.ProxyMetrics) *SpaceLister { return &SpaceLister{ Client: client, - GetSignupFunc: app.SignupService().GetSignupFromInformer, + GetSignupFunc: app.SignupService().GetSignup, ProxyMetrics: proxyMetrics, } } diff --git a/pkg/proxy/handlers/spacelister_get_test.go b/pkg/proxy/handlers/spacelister_get_test.go index 2643f111..56bece8c 100644 --- a/pkg/proxy/handlers/spacelister_get_test.go +++ b/pkg/proxy/handlers/spacelister_get_test.go @@ -548,7 +548,7 @@ func testSpaceListerGet(t *testing.T, publicViewerEnabled bool) { tc.mockFakeClient(fakeClient) } - signupProvider := fakeSignupService.GetSignupFromInformer + signupProvider := fakeSignupService.GetSignup if tc.overrideSignupFunc != nil { signupProvider = tc.overrideSignupFunc } @@ -724,7 +724,7 @@ func TestGetUserWorkspace(t *testing.T) { tc.mockFakeClient(fakeClient) } - signupProvider := fakeSignupService.GetSignupFromInformer + signupProvider := fakeSignupService.GetSignup if tc.overrideSignupFunc != nil { signupProvider = tc.overrideSignupFunc } @@ -834,7 +834,7 @@ func TestSpaceListerGetPublicViewerEnabled(t *testing.T) { t.Run(k, func(t *testing.T) { // given - signupProvider := fakeSignupService.GetSignupFromInformer + signupProvider := fakeSignupService.GetSignup proxyMetrics := metrics.NewProxyMetrics(prometheus.NewRegistry()) s := &handlers.SpaceLister{ @@ -969,7 +969,7 @@ func TestGetUserWorkspaceWithBindingsWithPublicViewerEnabled(t *testing.T) { t.Run(k, func(t *testing.T) { // given - signupProvider := fakeSignupService.GetSignupFromInformer + signupProvider := fakeSignupService.GetSignup proxyMetrics := metrics.NewProxyMetrics(prometheus.NewRegistry()) s := &handlers.SpaceLister{ diff --git a/pkg/proxy/handlers/spacelister_list_test.go b/pkg/proxy/handlers/spacelister_list_test.go index 3aa9c73d..4b9d3933 100644 --- a/pkg/proxy/handlers/spacelister_list_test.go +++ b/pkg/proxy/handlers/spacelister_list_test.go @@ -71,7 +71,7 @@ func TestListUserWorkspaces(t *testing.T) { t.Run(k, func(t *testing.T) { // given - signupProvider := fakeSignupService.GetSignupFromInformer + signupProvider := fakeSignupService.GetSignup proxyMetrics := metrics.NewProxyMetrics(prometheus.NewRegistry()) @@ -196,7 +196,7 @@ func TestHandleSpaceListRequest(t *testing.T) { tc.mockFakeClient(fakeClient) } - signupProvider := fakeSignupService.GetSignupFromInformer + signupProvider := fakeSignupService.GetSignup if tc.overrideSignupFunc != nil { signupProvider = tc.overrideSignupFunc } diff --git a/pkg/proxy/proxy.go b/pkg/proxy/proxy.go index b76ed983..dd9930a5 100644 --- a/pkg/proxy/proxy.go +++ b/pkg/proxy/proxy.go @@ -378,7 +378,7 @@ func (p *Proxy) checkUserIsProvisioned(ctx echo.Context, userID, username string // // UserSignup complete status is not checked, since it might cause the proxy blocking the request // and returning an error when quick transitions from ready to provisioning are happening. - userSignup, err := p.app.SignupService().GetSignupFromInformer(nil, userID, username, false) + userSignup, err := p.app.SignupService().GetSignup(nil, userID, username, false) if err != nil { return err } @@ -414,7 +414,7 @@ func (p *Proxy) getClusterAccess(ctx echo.Context, userID, username, proxyPlugin // this function returns an error. func (p *Proxy) getClusterAccessAsUserOrPublicViewer(ctx echo.Context, userID, username, proxyPluginName string, workspace *toolchainv1alpha1.Workspace) (*access.ClusterAccess, error) { // retrieve the requesting user's UserSignup - userSignup, err := p.app.SignupService().GetSignupFromInformer(nil, userID, username, false) + userSignup, err := p.app.SignupService().GetSignup(nil, userID, username, false) if err != nil { log.Error(nil, err, fmt.Sprintf("error retrieving user signup for userID '%s' and username '%s'", userID, username)) return nil, crterrors.NewInternalError(errs.New("unable to get user info"), "error retrieving user") diff --git a/pkg/proxy/proxy_community_test.go b/pkg/proxy/proxy_community_test.go index 67134e98..c2d5513f 100644 --- a/pkg/proxy/proxy_community_test.go +++ b/pkg/proxy/proxy_community_test.go @@ -153,7 +153,7 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr // configure proxy p.spaceLister = &handlers.SpaceLister{ Client: p.Client, - GetSignupFunc: fakeApp.SignupServiceMock.GetSignupFromInformer, + GetSignupFunc: fakeApp.SignupServiceMock.GetSignup, ProxyMetrics: p.metrics, } diff --git a/pkg/proxy/proxy_test.go b/pkg/proxy/proxy_test.go index 5e8fd5e2..d638a274 100644 --- a/pkg/proxy/proxy_test.go +++ b/pkg/proxy/proxy_test.go @@ -852,7 +852,7 @@ func (s *TestProxySuite) checkProxyOK(fakeApp *fake.ProxyFakeApp, p *Proxy) { p.spaceLister = &handlers.SpaceLister{ Client: p.Client, - GetSignupFunc: fakeApp.SignupServiceMock.GetSignupFromInformer, + GetSignupFunc: fakeApp.SignupServiceMock.GetSignup, ProxyMetrics: p.metrics, } if tc.OverrideGetSignupFunc != nil { diff --git a/pkg/proxy/service/cluster_service.go b/pkg/proxy/service/cluster_service.go index 4785458a..5f57adbc 100644 --- a/pkg/proxy/service/cluster_service.go +++ b/pkg/proxy/service/cluster_service.go @@ -100,7 +100,7 @@ func (s *ServiceImpl) getClusterAccessForDefaultWorkspace(userID, username, prox func (s *ServiceImpl) getSignupFromInformerForProvisionedUser(userID, username string) (*signup.Signup, error) { // don't check for usersignup complete status, since it might cause the proxy blocking the request // and returning an error when quick transitions from ready to provisioning are happening. - userSignup, err := s.SignupService.GetSignupFromInformer(nil, userID, username, false) + userSignup, err := s.SignupService.GetSignup(nil, userID, username, false) if err != nil { return nil, err } diff --git a/pkg/signup/service/signup_service.go b/pkg/signup/service/signup_service.go index e4a03564..61c5de1f 100644 --- a/pkg/signup/service/signup_service.go +++ b/pkg/signup/service/signup_service.go @@ -350,16 +350,10 @@ func (s *ServiceImpl) reactivateUserSignup(ctx *gin.Context, existing *toolchain // GetSignup returns Signup resource which represents the corresponding K8s UserSignup // and MasterUserRecord resources in the host cluster. -// Returns nil, nil if the UserSignup resource is not found or if it's deactivated. -func (s *ServiceImpl) GetSignup(ctx *gin.Context, userID, username string) (*signup.Signup, error) { - return s.DoGetSignup(ctx, s.Client, userID, username, true) -} - -// GetSignupFromInformer uses the same logic of the 'GetSignup' function, except it uses informers to get resources. -// This function and the ResourceProvider abstraction can replace the original GetSignup function once it is determined to be stable. // The checkUserSignupCompleted was introduced in order to avoid checking the readiness of the complete condition on the UserSignup in certain situations, // such as proxy calls for example. -func (s *ServiceImpl) GetSignupFromInformer(ctx *gin.Context, userID, username string, checkUserSignupCompleted bool) (*signup.Signup, error) { +// Returns nil, nil if the UserSignup resource is not found or if it's deactivated. +func (s *ServiceImpl) GetSignup(ctx *gin.Context, userID, username string, checkUserSignupCompleted bool) (*signup.Signup, error) { return s.DoGetSignup(ctx, s.Client, userID, username, checkUserSignupCompleted) } @@ -388,10 +382,8 @@ func (s *ServiceImpl) DoGetSignup(ctx *gin.Context, cl namespaced.Client, userID // If there is no need to update the UserSignup then break out of the loop here (by returning nil) // otherwise update the UserSignup if updated { - var updateErr error - userSignup, updateErr = s.UpdateUserSignup(userSignup) - if updateErr != nil { - return updateErr + if err := s.Update(gocontext.TODO(), userSignup); err != nil { + return err } } @@ -582,15 +574,6 @@ func (s *ServiceImpl) DoGetUserSignupFromIdentifier(cl namespaced.Client, userID return userSignup, nil } -// UpdateUserSignup is used to update the provided UserSignup resource, and returning the updated resource -func (s *ServiceImpl) UpdateUserSignup(userSignup *toolchainv1alpha1.UserSignup) (*toolchainv1alpha1.UserSignup, error) { - if err := s.Update(gocontext.TODO(), userSignup); err != nil { - return nil, err - } - - return userSignup, nil -} - var ( md5Matcher = regexp.MustCompile("(?i)[a-f0-9]{32}$") ) diff --git a/pkg/signup/service/signup_service_test.go b/pkg/signup/service/signup_service_test.go index 97d30f14..ab79cc1b 100644 --- a/pkg/signup/service/signup_service_test.go +++ b/pkg/signup/service/signup_service_test.go @@ -244,28 +244,10 @@ func (s *TestSignupServiceSuite) TestGetSignupFailsWithNotFoundThenOtherError() c, _ := gin.CreateTestContext(httptest.NewRecorder()) // when - _, err := application.SignupService().GetSignup(c, "000", "abc") + _, err := application.SignupService().GetSignup(c, "000", "abc", true) // then require.EqualError(s.T(), err, "something quite unfortunate happened: something bad") - - s.T().Run("informer", func(t *testing.T) { - // given - fakeClient := commontest.NewFakeClient(t) - fakeClient.MockGet = func(ctx gocontext.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { - if _, ok := obj.(*toolchainv1alpha1.UserSignup); ok && key.Name != "000" { - return errors2.NewInternalError(errors.New("something quite unfortunate happened"), "something bad") - } - return fakeClient.Client.Get(ctx, key, obj, opts...) - } - svc := service.NewSignupService(namespaced.NewClient(fakeClient, commontest.HostOperatorNs)) - - // when - _, err = svc.GetSignupFromInformer(c, "000", "abc", true) - - // then - require.EqualError(t, err, "something quite unfortunate happened: something bad") - }) } func (s *TestSignupServiceSuite) TestSignupNoSpaces() { @@ -705,29 +687,10 @@ func (s *TestSignupServiceSuite) TestGetUserSignupFails() { } // when - _, err := application.SignupService().GetSignup(c, "", username) + _, err := application.SignupService().GetSignup(c, "", username, true) // then require.EqualError(s.T(), err, "an error occurred") - - s.T().Run("informer", func(t *testing.T) { - // given - fakeClient := commontest.NewFakeClient(t) - fakeClient.MockGet = func(ctx gocontext.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { - if key.Name != username { - return errors.New("an error occurred") - } - return fakeClient.Client.Get(ctx, key, obj, opts...) - } - - svc := service.NewSignupService(namespaced.NewClient(fakeClient, commontest.HostOperatorNs)) - - // when - _, err = svc.GetSignupFromInformer(c, "johnsmith", "abc", true) - - // then - require.EqualError(t, err, "an error occurred") - }) } func (s *TestSignupServiceSuite) TestGetSignupNotFound() { @@ -738,24 +701,11 @@ func (s *TestSignupServiceSuite) TestGetSignupNotFound() { _, application := testutil.PrepareInClusterApp(s.T()) // when - signup, err := application.SignupService().GetSignup(c, userID.String(), "") + signup, err := application.SignupService().GetSignup(c, userID.String(), "", true) // then require.Nil(s.T(), signup) require.NoError(s.T(), err) - - s.T().Run("informer", func(t *testing.T) { - // given - fakeClient := commontest.NewFakeClient(t) - svc := service.NewSignupService(namespaced.NewClient(fakeClient, commontest.HostOperatorNs)) - - // when - signup, err := svc.GetSignupFromInformer(c, userID.String(), "", true) - - // then - require.Nil(t, signup) - require.NoError(t, err) - }) } func (s *TestSignupServiceSuite) TestGetSignupStatusNotComplete() { @@ -800,7 +750,7 @@ func (s *TestSignupServiceSuite) TestGetSignupStatusNotComplete() { _, application := testutil.PrepareInClusterApp(s.T(), userSignupNotComplete) // when - response, err := application.SignupService().GetSignup(c, userID.String(), "") + response, err := application.SignupService().GetSignup(c, userID.String(), "", true) // then require.NoError(s.T(), err) @@ -823,35 +773,7 @@ func (s *TestSignupServiceSuite) TestGetSignupStatusNotComplete() { assert.Empty(s.T(), response.StartDate) assert.Empty(s.T(), response.EndDate) - s.T().Run("informer - with check for usersignup complete condition", func(t *testing.T) { - // given - fakeClient := commontest.NewFakeClient(t, userSignupNotComplete) - svc := service.NewSignupService(namespaced.NewClient(fakeClient, commontest.HostOperatorNs)) - - // when - response, err := svc.GetSignupFromInformer(c, userID.String(), "", true) - - // then - require.NoError(t, err) - require.NotNil(t, response) - - require.Equal(t, userID.String(), response.Name) - require.Equal(t, "bill", response.Username) - require.Equal(t, "bill", response.CompliantUsername) - require.False(t, response.Status.Ready) - require.Equal(t, "test_reason", response.Status.Reason) - require.Equal(t, "test_message", response.Status.Message) - require.True(t, response.Status.VerificationRequired) - require.Empty(t, response.ConsoleURL) - require.Empty(t, response.CheDashboardURL) - require.Empty(t, response.APIEndpoint) - require.Empty(t, response.ClusterName) - require.Empty(t, response.ProxyURL) - assert.Equal(t, "", response.DefaultUserNamespace) - assert.Equal(t, "", response.RHODSMemberURL) - }) - - s.T().Run("informer - with no check for UserSignup complete condition", func(t *testing.T) { + s.T().Run("with no check for UserSignup complete condition", func(t *testing.T) { // given states.SetVerificationRequired(userSignupNotComplete, false) mur := s.newProvisionedMUR("bill") @@ -864,7 +786,7 @@ func (s *TestSignupServiceSuite) TestGetSignupStatusNotComplete() { // when // we set checkUserSignupCompleted to false - response, err := svc.GetSignupFromInformer(c, userID.String(), userSignupNotComplete.Spec.IdentityClaims.PreferredUsername, false) + response, err := svc.GetSignup(c, userID.String(), userSignupNotComplete.Spec.IdentityClaims.PreferredUsername, false) // then require.NoError(t, err) @@ -941,7 +863,7 @@ func (s *TestSignupServiceSuite) TestGetSignupNoStatusNotCompleteCondition() { _, application := testutil.PrepareInClusterApp(s.T(), userSignup) // when - response, err := application.SignupService().GetSignup(c, userID.String(), "bill") + response, err := application.SignupService().GetSignup(c, userID.String(), "bill", true) // then require.NoError(s.T(), err) @@ -961,34 +883,6 @@ func (s *TestSignupServiceSuite) TestGetSignupNoStatusNotCompleteCondition() { require.Empty(s.T(), response.ProxyURL) assert.Equal(s.T(), "", response.DefaultUserNamespace) assert.Equal(s.T(), "", response.RHODSMemberURL) - - s.T().Run("informer", func(t *testing.T) { - // given - fakeClient := commontest.NewFakeClient(t, userSignup) - svc := service.NewSignupService(namespaced.NewClient(fakeClient, commontest.HostOperatorNs)) - - // when - response, err := svc.GetSignupFromInformer(c, userID.String(), "", true) - - // then - require.NoError(t, err) - require.NotNil(t, response) - - require.Equal(t, userID.String(), response.Name) - require.Equal(t, "bill", response.Username) - require.Empty(t, response.CompliantUsername) - require.False(t, response.Status.Ready) - require.Equal(t, "PendingApproval", response.Status.Reason) - require.True(t, response.Status.VerificationRequired) - require.Empty(t, response.Status.Message) - require.Empty(t, response.ConsoleURL) - require.Empty(t, response.CheDashboardURL) - require.Empty(t, response.APIEndpoint) - require.Empty(t, response.ClusterName) - require.Empty(t, response.ProxyURL) - assert.Equal(t, "", response.DefaultUserNamespace) - assert.Equal(t, "", response.RHODSMemberURL) - }) } } @@ -999,28 +893,16 @@ func (s *TestSignupServiceSuite) TestGetSignupDeactivated() { us := s.newUserSignupComplete() us.Status.Conditions = deactivated() - fakeClient, application := testutil.PrepareInClusterApp(s.T(), us) + _, application := testutil.PrepareInClusterApp(s.T(), us) c, _ := gin.CreateTestContext(httptest.NewRecorder()) // when - signup, err := application.SignupService().GetSignup(c, us.Name, "") + signup, err := application.SignupService().GetSignup(c, us.Name, "", true) // then require.Nil(s.T(), signup) require.NoError(s.T(), err) - - s.T().Run("informer", func(t *testing.T) { - // given - svc := service.NewSignupService(namespaced.NewClient(fakeClient, commontest.HostOperatorNs)) - - // when - signup, err := svc.GetSignupFromInformer(c, us.Name, "", true) - - // then - require.Nil(t, signup) - require.NoError(t, err) - }) } func (s *TestSignupServiceSuite) TestGetSignupStatusOK() { @@ -1041,7 +923,7 @@ func (s *TestSignupServiceSuite) TestGetSignupStatusOK() { c, _ := gin.CreateTestContext(httptest.NewRecorder()) // when - response, err := application.SignupService().GetSignup(c, us.Name, "") + response, err := application.SignupService().GetSignup(c, us.Name, "", true) // then require.NoError(t, err) @@ -1064,33 +946,6 @@ func (s *TestSignupServiceSuite) TestGetSignupStatusOK() { assert.Equal(t, "https://proxy-url.com", response.ProxyURL) assert.Equal(t, "ted-dev", response.DefaultUserNamespace) assert.Equal(t, fmt.Sprintf("https://rhods-dashboard-redhat-ods-applications%smember-123.com", appsSubDomain), response.RHODSMemberURL) - - s.T().Run("informer", func(t *testing.T) { - // given - fakeClient := commontest.NewFakeClient(t, us, mur, toolchainStatus, space, spacebinding) - svc := service.NewSignupService(namespaced.NewClient(fakeClient, commontest.HostOperatorNs)) - - // when - response, err := svc.GetSignupFromInformer(c, us.Name, "", true) - - // then - require.NoError(t, err) - require.NotNil(t, response) - - require.Equal(t, "jsmith", response.Username) - require.Equal(t, "ted", response.CompliantUsername) - assert.True(t, response.Status.Ready) - assert.Equal(t, "mur_ready_reason", response.Status.Reason) - assert.Equal(t, "mur_ready_message", response.Status.Message) - assert.False(t, response.Status.VerificationRequired) - assert.Equal(t, fmt.Sprintf("https://console%smember-123.com", appsSubDomain), response.ConsoleURL) - assert.Equal(t, "http://che-toolchain-che.member-123.com", response.CheDashboardURL) - assert.Equal(t, "http://api.devcluster.openshift.com", response.APIEndpoint) - assert.Equal(t, "member-123", response.ClusterName) - assert.Equal(t, "https://proxy-url.com", response.ProxyURL) - assert.Equal(t, "ted-dev", response.DefaultUserNamespace) - assert.Equal(t, fmt.Sprintf("https://rhods-dashboard-redhat-ods-applications%smember-123.com", appsSubDomain), response.RHODSMemberURL) - }) }) } } @@ -1120,7 +975,7 @@ func (s *TestSignupServiceSuite) TestGetSignupByUsernameOK() { c, _ := gin.CreateTestContext(httptest.NewRecorder()) // when - response, err := svc.GetSignup(c, "foo", us.Spec.IdentityClaims.PreferredUsername) + response, err := svc.GetSignup(c, "foo", us.Spec.IdentityClaims.PreferredUsername, true) // then require.NoError(s.T(), err) @@ -1148,34 +1003,6 @@ func (s *TestSignupServiceSuite) TestGetSignupByUsernameOK() { assert.Equal(s.T(), "https://proxy-url.com", response.ProxyURL) assert.Equal(s.T(), "ted-dev", response.DefaultUserNamespace) assert.Equal(s.T(), "https://rhods-dashboard-redhat-ods-applications.apps.member-123.com", response.RHODSMemberURL) - - s.T().Run("informer", func(t *testing.T) { - // given - fakeClient := commontest.NewFakeClient(t, us, mur, toolchainStatus, space, spacebinding) - svc := service.NewSignupService(namespaced.NewClient(fakeClient, commontest.HostOperatorNs)) - - // when - response, err := svc.GetSignupFromInformer(c, "foo", us.Spec.IdentityClaims.PreferredUsername, true) - - // then - require.NoError(t, err) - require.NotNil(t, response) - - require.Equal(t, us.Name, response.Name) - require.Equal(t, "jsmith", response.Username) - require.Equal(t, "ted", response.CompliantUsername) - assert.True(t, response.Status.Ready) - assert.Equal(t, "mur_ready_reason", response.Status.Reason) - assert.Equal(t, "mur_ready_message", response.Status.Message) - assert.False(t, response.Status.VerificationRequired) - assert.Equal(t, "https://console.apps.member-123.com", response.ConsoleURL) - assert.Equal(t, "http://che-toolchain-che.member-123.com", response.CheDashboardURL) - assert.Equal(t, "http://api.devcluster.openshift.com", response.APIEndpoint) - assert.Equal(t, "member-123", response.ClusterName) - assert.Equal(t, "https://proxy-url.com", response.ProxyURL) - assert.Equal(t, "ted-dev", response.DefaultUserNamespace) - assert.Equal(t, "https://rhods-dashboard-redhat-ods-applications.apps.member-123.com", response.RHODSMemberURL) - }) } func (s *TestSignupServiceSuite) newToolchainStatus(appsSubDomain string) *toolchainv1alpha1.ToolchainStatus { @@ -1229,22 +1056,10 @@ func (s *TestSignupServiceSuite) TestGetSignupStatusFailGetToolchainStatus() { _, application := testutil.PrepareInClusterApp(s.T(), us, mur, space) // when - _, err := application.SignupService().GetSignup(c, us.Name, "") + _, err := application.SignupService().GetSignup(c, us.Name, "", true) // then require.EqualError(s.T(), err, fmt.Sprintf("error when retrieving ToolchainStatus to set Che Dashboard for completed UserSignup %s: toolchainstatuses.toolchain.dev.openshift.com \"toolchain-status\" not found", us.Name)) - - s.T().Run("informer", func(t *testing.T) { - // given - fakeClient := commontest.NewFakeClient(t, us, mur, space) - svc := service.NewSignupService(namespaced.NewClient(fakeClient, commontest.HostOperatorNs)) - - // when - _, err = svc.GetSignupFromInformer(c, us.Name, "", true) - - // then - require.EqualError(t, err, fmt.Sprintf("error when retrieving ToolchainStatus to set Che Dashboard for completed UserSignup %s: toolchainstatuses.toolchain.dev.openshift.com \"toolchain-status\" not found", us.Name)) - }) } func (s *TestSignupServiceSuite) TestGetSignupMURGetFails() { @@ -1265,28 +1080,10 @@ func (s *TestSignupServiceSuite) TestGetSignupMURGetFails() { } // when - _, err := application.SignupService().GetSignup(c, us.Name, "") + _, err := application.SignupService().GetSignup(c, us.Name, "", true) // then require.EqualError(s.T(), err, fmt.Sprintf("error when retrieving MasterUserRecord for completed UserSignup %s: an error occurred", us.Name)) - - s.T().Run("informer", func(t *testing.T) { - // given - fakeClient := commontest.NewFakeClient(t, us) - fakeClient.MockGet = func(ctx gocontext.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { - if _, ok := obj.(*toolchainv1alpha1.MasterUserRecord); ok && key.Name == us.Status.CompliantUsername { - return returnedErr - } - return fakeClient.Client.Get(ctx, key, obj, opts...) - } - svc := service.NewSignupService(namespaced.NewClient(fakeClient, commontest.HostOperatorNs)) - - // when - _, err = svc.GetSignupFromInformer(c, us.Name, "", true) - - // then - require.EqualError(t, err, fmt.Sprintf("error when retrieving MasterUserRecord for completed UserSignup %s: an error occurred", us.Name)) - }) } func (s *TestSignupServiceSuite) TestGetSignupReadyConditionStatus() { @@ -1362,23 +1159,10 @@ func (s *TestSignupServiceSuite) TestGetSignupReadyConditionStatus() { tc.condition, }, } - fakeClient, application := testutil.PrepareInClusterApp(s.T(), us, mur, space, toolchainStatus) + _, application := testutil.PrepareInClusterApp(s.T(), us, mur, space, toolchainStatus) // when - response, err := application.SignupService().GetSignup(c, us.Name, "") - - // then - require.NoError(t, err) - require.Equal(t, tc.expectedConditionReady, response.Status.Ready) - require.Equal(t, tc.condition.Reason, response.Status.Reason) - require.Equal(t, tc.condition.Message, response.Status.Message) - - // informer case - // given - svc := service.NewSignupService(namespaced.NewClient(fakeClient, commontest.HostOperatorNs)) - - // when - _, err = svc.GetSignupFromInformer(c, us.Name, "", true) + response, err := application.SignupService().GetSignup(c, us.Name, "", true) // then require.NoError(t, err) @@ -1394,7 +1178,7 @@ func (s *TestSignupServiceSuite) TestGetSignupBannedUserEmail() { s.ServiceConfiguration(true, "", 5) us := s.newBannedUserSignup() - fakeClient, application := testutil.PrepareInClusterApp(s.T(), us) + _, application := testutil.PrepareInClusterApp(s.T(), us) rr := httptest.NewRecorder() ctx, _ := gin.CreateTestContext(rr) @@ -1403,27 +1187,13 @@ func (s *TestSignupServiceSuite) TestGetSignupBannedUserEmail() { ctx.Set(context.EmailKey, "jsmith@gmail.com") // when - response, err := application.SignupService().GetSignup(ctx, us.Name, "") + response, err := application.SignupService().GetSignup(ctx, us.Name, "", true) // then // return not found signup require.NoError(s.T(), err) require.NotNil(s.T(), response) require.Equal(s.T(), toolchainv1alpha1.UserSignupPendingApprovalReason, response.Status.Reason) - - s.T().Run("informer", func(t *testing.T) { - // given - svc := service.NewSignupService(namespaced.NewClient(fakeClient, commontest.HostOperatorNs)) - - // when - response, err := svc.GetSignupFromInformer(ctx, us.Name, "", true) - - // then - require.NoError(t, err) - require.NotNil(t, response) - require.Equal(t, toolchainv1alpha1.UserSignupPendingApprovalReason, response.Status.Reason) - - }) } func (s *TestSignupServiceSuite) TestGetDefaultUserNamespace() { @@ -1558,44 +1328,6 @@ func (s *TestSignupServiceSuite) TestGetUserSignup() { }) } -func (s *TestSignupServiceSuite) TestUpdateUserSignup() { - s.ServiceConfiguration(true, "", 5) - - us := s.newUserSignupComplete() - - s.Run("updateusersignup ok", func() { - _, application := testutil.PrepareInClusterApp(s.T(), us) - - val, err := application.SignupService().GetUserSignupFromIdentifier(us.Name, "") - require.NoError(s.T(), err) - - val.Spec.IdentityClaims.FamilyName = "Johnson" - - updated, err := application.SignupService().UpdateUserSignup(val) - require.NoError(s.T(), err) - - require.Equal(s.T(), val.Spec.IdentityClaims.FamilyName, updated.Spec.IdentityClaims.FamilyName) - }) - - s.Run("updateusersignup returns error", func() { - fakeClient, application := testutil.PrepareInClusterApp(s.T(), us) - - fakeClient.MockUpdate = func(ctx gocontext.Context, obj client.Object, opts ...client.UpdateOption) error { - if _, ok := obj.(*toolchainv1alpha1.UserSignup); ok { - return errors.New("update failed") - } - return fakeClient.Client.Update(ctx, obj, opts...) - } - - val, err := application.SignupService().GetUserSignupFromIdentifier(us.Name, "") - require.NoError(s.T(), err) - - updated, err := application.SignupService().UpdateUserSignup(val) - require.EqualError(s.T(), err, "update failed") - require.Nil(s.T(), updated) - }) -} - func (s *TestSignupServiceSuite) TestIsPhoneVerificationRequired() { commontest.SetEnvVarAndRestore(s.T(), commonconfig.WatchNamespaceEnvVar, commontest.HostOperatorNs) @@ -1745,7 +1477,7 @@ func (s *TestSignupServiceSuite) TestGetSignupUpdatesUserSignupIdentityClaims() c.Set(context.UsernameKey, "cocochanel") fakeClient, application := testutil.PrepareInClusterApp(s.T(), userSignup, mur) - _, err := application.SignupService().GetSignup(c, userSignup.Name, userSignup.Spec.IdentityClaims.PreferredUsername) + _, err := application.SignupService().GetSignup(c, userSignup.Name, userSignup.Spec.IdentityClaims.PreferredUsername, true) require.NoError(s.T(), err) modified := &toolchainv1alpha1.UserSignup{} @@ -1768,7 +1500,7 @@ func (s *TestSignupServiceSuite) TestGetSignupUpdatesUserSignupIdentityClaims() c, _ := gin.CreateTestContext(httptest.NewRecorder()) c.Set(context.GivenNameKey, "Jonathan") - _, err := application.SignupService().GetSignup(c, userSignup.Name, userSignup.Spec.IdentityClaims.PreferredUsername) + _, err := application.SignupService().GetSignup(c, userSignup.Name, userSignup.Spec.IdentityClaims.PreferredUsername, true) require.NoError(s.T(), err) modified := &toolchainv1alpha1.UserSignup{} @@ -1794,7 +1526,7 @@ func (s *TestSignupServiceSuite) TestGetSignupUpdatesUserSignupIdentityClaims() c.Set(context.FamilyNameKey, "Smythe") c.Set(context.CompanyKey, "Red Hat") - _, err := application.SignupService().GetSignup(c, userSignup.Name, userSignup.Spec.IdentityClaims.PreferredUsername) + _, err := application.SignupService().GetSignup(c, userSignup.Name, userSignup.Spec.IdentityClaims.PreferredUsername, true) require.NoError(s.T(), err) modified := &toolchainv1alpha1.UserSignup{} @@ -1822,7 +1554,7 @@ func (s *TestSignupServiceSuite) TestGetSignupUpdatesUserSignupIdentityClaims() c.Set(context.OriginalSubKey, "jsmythe-original-sub") c.Set(context.EmailKey, "jsmythe@redhat.com") - _, err := application.SignupService().GetSignup(c, userSignup.Name, userSignup.Spec.IdentityClaims.PreferredUsername) + _, err := application.SignupService().GetSignup(c, userSignup.Name, userSignup.Spec.IdentityClaims.PreferredUsername, true) require.NoError(s.T(), err) modified := &toolchainv1alpha1.UserSignup{} diff --git a/pkg/verification/service/verification_service.go b/pkg/verification/service/verification_service.go index c8ce7831..e4e990c5 100644 --- a/pkg/verification/service/verification_service.go +++ b/pkg/verification/service/verification_service.go @@ -1,6 +1,7 @@ package service import ( + gocontext "context" "crypto/rand" "errors" "fmt" @@ -185,8 +186,7 @@ func (s *ServiceImpl) InitVerification(ctx *gin.Context, userID, username, e164P for k, v := range annotationValues { signup.Annotations[k] = v } - _, err = s.Services().SignupService().UpdateUserSignup(signup) - if err != nil { + if err := s.Update(gocontext.TODO(), signup); err != nil { return err } @@ -343,8 +343,7 @@ func (s *ServiceImpl) VerifyPhoneCode(ctx *gin.Context, userID, username, code s delete(signup.Annotations, annotationName) } - _, err = s.Services().SignupService().UpdateUserSignup(signup) - if err != nil { + if err := s.Update(gocontext.TODO(), signup); err != nil { log.Error(ctx, err, fmt.Sprintf("error updating usersignup: %s", signup.Name)) return err } @@ -426,8 +425,7 @@ func (s *ServiceImpl) VerifyActivationCode(ctx *gin.Context, userID, username, c if targetCluster != "" { signup.Spec.TargetCluster = targetCluster } - _, err = s.Services().SignupService().UpdateUserSignup(signup) - if err != nil { + if err := s.Update(gocontext.TODO(), signup); err != nil { return err } diff --git a/test/fake/proxy.go b/test/fake/proxy.go index 952a29d4..f25b658b 100644 --- a/test/fake/proxy.go +++ b/test/fake/proxy.go @@ -94,11 +94,7 @@ func (m *SignupService) DefaultMockGetSignup() func(userID, username string) (*s } } -func (m *SignupService) GetSignup(_ *gin.Context, userID, username string) (*signup.Signup, error) { - return m.MockGetSignup(userID, username) -} - -func (m *SignupService) GetSignupFromInformer(_ *gin.Context, userID, username string, _ bool) (*signup.Signup, error) { +func (m *SignupService) GetSignup(_ *gin.Context, userID, username string, _ bool) (*signup.Signup, error) { return m.MockGetSignup(userID, username) } diff --git a/test/fake/informer.go b/test/fake/utils.go similarity index 100% rename from test/fake/informer.go rename to test/fake/utils.go From 795ca54787e84d064d0a1d7cff842b068a99a2a4 Mon Sep 17 00:00:00 2001 From: Alexey Kazakov Date: Tue, 29 Oct 2024 16:28:33 -0700 Subject: [PATCH 4/5] Allow proxy to forward requests to namespaces outside of workspace (#471) * Allow proxy to forward requests to namespaces outside of workspace * linter * Update pkg/proxy/proxy.go Co-authored-by: Kanika Rana <46766610+ranakan19@users.noreply.github.com> * Update pkg/proxy/proxy_community_test.go Co-authored-by: Kanika Rana <46766610+ranakan19@users.noreply.github.com> * Update pkg/proxy/proxy_community_test.go --------- Co-authored-by: Francisc Munteanu Co-authored-by: Kanika Rana <46766610+ranakan19@users.noreply.github.com> --- pkg/proxy/proxy.go | 40 +--------- pkg/proxy/proxy_community_test.go | 126 ++++++++++++++++++------------ pkg/proxy/proxy_test.go | 71 ++++++----------- 3 files changed, 107 insertions(+), 130 deletions(-) diff --git a/pkg/proxy/proxy.go b/pkg/proxy/proxy.go index dd9930a5..d2602ad7 100644 --- a/pkg/proxy/proxy.go +++ b/pkg/proxy/proxy.go @@ -308,9 +308,7 @@ func (p *Proxy) processHomeWorkspaceRequest(ctx echo.Context, userID, username, } // check whether the user has access to the home workspace - // and whether the requestedNamespace -if any- exists in the workspace. - requestedNamespace := namespaceFromCtx(ctx) - if err := validateWorkspaceRequest("", requestedNamespace, workspaces...); err != nil { + if err := validateWorkspaceRequest("", workspaces...); err != nil { return nil, crterrors.NewForbiddenError("invalid workspace request", err.Error()) } @@ -333,9 +331,7 @@ func (p *Proxy) processWorkspaceRequest(ctx echo.Context, userID, username, work } // check whether the user has access to the workspace - // and whether the requestedNamespace -if any- exists in the workspace. - requestedNamespace := namespaceFromCtx(ctx) - if err := validateWorkspaceRequest(workspaceName, requestedNamespace, *workspace); err != nil { + if err := validateWorkspaceRequest(workspaceName, *workspace); err != nil { return nil, crterrors.NewForbiddenError("invalid workspace request", err.Error()) } @@ -828,9 +824,8 @@ func replaceTokenInWebsocketRequest(req *http.Request, newToken string) { } // validateWorkspaceRequest checks whether the requested workspace is in the list of workspaces the user has visibility on (retrieved via the spaceLister). -// If `requestedWorkspace` is zero, this function looks for the home workspace (the one with `status.Type` set to `home`). -// If `requestedNamespace` is NOT zero, this function checks if the namespace exists in the workspace. -func validateWorkspaceRequest(requestedWorkspace, requestedNamespace string, workspaces ...toolchainv1alpha1.Workspace) error { +// If `requestedWorkspace` is empty, then the home workspace (the one with `status.Type` set to `home`) is assumed. +func validateWorkspaceRequest(requestedWorkspace string, workspaces ...toolchainv1alpha1.Workspace) error { // check workspace access isHomeWSRequested := requestedWorkspace == "" @@ -845,32 +840,5 @@ func validateWorkspaceRequest(requestedWorkspace, requestedNamespace string, wor return fmt.Errorf("access to workspace '%s' is forbidden", requestedWorkspace) } - // check namespace access - if requestedNamespace != "" { - allowedNamespace := false - namespaces := workspaces[allowedWorkspace].Status.Namespaces - for _, ns := range namespaces { - if ns.Name == requestedNamespace { - allowedNamespace = true - break - } - } - if !allowedNamespace { - return fmt.Errorf("access to namespace '%s' in workspace '%s' is forbidden", requestedNamespace, workspaces[allowedWorkspace].Name) - } - } return nil } - -func namespaceFromCtx(ctx echo.Context) string { - path := ctx.Request().URL.Path - if strings.Index(path, "/namespaces/") > 0 { - segments := strings.Split(path, "/") - for i, segment := range segments { - if segment == "namespaces" && i+1 < len(segments) { - return segments[i+1] - } - } - } - return "" -} diff --git a/pkg/proxy/proxy_community_test.go b/pkg/proxy/proxy_community_test.go index c2d5513f..aeaf32fa 100644 --- a/pkg/proxy/proxy_community_test.go +++ b/pkg/proxy/proxy_community_test.go @@ -283,73 +283,103 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr }, // Given user alice exists // And alice owns a private workspace - // When alice requests the list of pods in a non existing namespace in alice's workspace - // Then the proxy does NOT forward the request - // And the proxy rejects the call with 403 Forbidden - "plain http request as owner to not existing namespace in private workspace": { - ProxyRequestMethod: "GET", - ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(alice)}}, - ExpectedProxyResponseStatus: http.StatusForbidden, - RequestPath: podsInNamespaceRequestURL("alice-private", "not-existing"), - ExpectedResponse: "invalid workspace request: access to namespace 'not-existing' in workspace 'alice-private' is forbidden", + // When alice requests the list of pods in a namespace which does not belong to the alice's workspace + // Then the proxy does forward the request anyway. + // It's not up to the proxy to check permissions on the specific namespace. + // The target API server will reject the request if the user does not have permissions to access the namespace. + // Here the request is successful because the underlying mock target cluster API always server returns OK + "plain http request as permitted user to namespace outside of private workspace": { + ProxyRequestMethod: "GET", + ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(alice)}}, + ExpectedAPIServerRequestHeaders: map[string][]string{ + "Authorization": {"Bearer clusterSAToken"}, + "Impersonate-User": {"alice"}, + "X-SSO-User": {"username-" + alice.String()}, + }, + ExpectedProxyResponseStatus: http.StatusOK, + RequestPath: podsInNamespaceRequestURL("alice-private", "outside-of-workspace-namespace"), + ExpectedResponse: httpTestServerResponse, }, // Given smith owns a workspace named smith-community // And smith-community is publicly visible (shared with PublicViewer) - // When smith requests the list of pods in a non existing namespace in workspace smith-community - // Then the request is forwarded from the proxy - // And the request impersonates smith - // And the request's X-SSO-User Header is set to smith's ID - // And the request is successful - "plain http request as owner to not existing namespace in community workspace": { - ProxyRequestMethod: "GET", - ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(smith)}}, - ExpectedProxyResponseStatus: http.StatusForbidden, - RequestPath: podsInNamespaceRequestURL("smith-community", "not-existing"), - ExpectedResponse: "invalid workspace request: access to namespace 'not-existing' in workspace 'smith-community' is forbidden", + // When smith requests the list of pods in a namespace which does not belong to the workspace smith-community + // Then the proxy does forward the request anyway. + // It's not up to the proxy to check permissions on the specific namespace. + // The target API server will reject the request if the user does not have permissions to access the namespace. + // Here the request is successful because the underlying mock target cluster API server returns OK + "plain http request as owner to namespace outside of community workspace": { + ProxyRequestMethod: "GET", + ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(smith)}}, + ExpectedAPIServerRequestHeaders: map[string][]string{ + "Authorization": {"Bearer clusterSAToken"}, + "Impersonate-User": {"smith"}, + "X-SSO-User": {"username-" + smith.String()}, + }, + ExpectedProxyResponseStatus: http.StatusOK, + RequestPath: podsInNamespaceRequestURL("smith-community", "outside-of-workspace-namespace"), + ExpectedResponse: httpTestServerResponse, }, // Given smith owns a workspace named smith-community // And smith-community is publicly visible (shared with PublicViewer) // And user alice exists - // When alice requests the list of pods in a non existing namespace in smith's workspace - // Then the proxy does NOT forward the request - // And the proxy rejects the call with 403 Forbidden - "plain http request as community user to not existing namespace in community workspace": { - ProxyRequestMethod: "GET", - ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(alice)}}, - ExpectedProxyResponseStatus: http.StatusForbidden, - RequestPath: podsInNamespaceRequestURL("smith-community", "not-existing"), - ExpectedResponse: "invalid workspace request: access to namespace 'not-existing' in workspace 'smith-community' is forbidden", + // When alice requests the list of pods in a namespace which does not belong to the smith's workspace + // It's not up to the proxy to check permissions on the specific namespace. + // The target API server will reject the request if the user does not have permissions to access the namespace. + // Here the request is successful because the underlying mock target cluster API server returns OK + "plain http request as community user to namespace outside of community workspace": { + ProxyRequestMethod: "GET", + ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(alice)}}, + ExpectedAPIServerRequestHeaders: map[string][]string{ + "Authorization": {"Bearer clusterSAToken"}, + "Impersonate-User": {toolchainv1alpha1.KubesawAuthenticatedUsername}, + "X-SSO-User": {"username-" + alice.String()}, + }, + ExpectedProxyResponseStatus: http.StatusOK, + RequestPath: podsInNamespaceRequestURL("smith-community", "outside-of-workspace-namespace"), + ExpectedResponse: httpTestServerResponse, }, // Given smith owns a workspace named smith-community // And smith-community is publicly visible (shared with PublicViewer) - // When bob requests the list of pods in a non existing namespace in smith's workspace - // Then the proxy does NOT forward the request - // And the proxy rejects the call with 403 Forbidden - "plain http request as unsigned user to not existing namespace in community workspace": { - ProxyRequestMethod: "GET", - ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(bob)}}, - ExpectedProxyResponseStatus: http.StatusForbidden, - RequestPath: podsInNamespaceRequestURL("smith-community", "not-existing"), - ExpectedResponse: "invalid workspace request: access to namespace 'not-existing' in workspace 'smith-community' is forbidden", + // When bob requests the list of pods in a namespace which does not belong to the smith's workspace + // It's not up to the proxy to check permissions on the specific namespace. + // The target API server will reject the request if the user does not have permissions to access the namespace. + // Here the request is successful because the underlying mock target cluster API server returns OK + "plain http request as unsigned user to namespace outside of community workspace": { + ProxyRequestMethod: "GET", + ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(bob)}}, + ExpectedAPIServerRequestHeaders: map[string][]string{ + "Authorization": {"Bearer clusterSAToken"}, + "Impersonate-User": {toolchainv1alpha1.KubesawAuthenticatedUsername}, + "X-SSO-User": {"username-" + bob.String()}, + }, + ExpectedProxyResponseStatus: http.StatusOK, + RequestPath: podsInNamespaceRequestURL("smith-community", "outside-of-workspace-namespace"), + ExpectedResponse: httpTestServerResponse, }, // Given smith owns a workspace named smith-community // And smith-community is publicly visible (shared with PublicViewer) // And not ready user john exists - // When john requests the list of pods in a non existing namespace in smith's workspace - // Then the proxy does NOT forward the request - // And the proxy rejects the call with 403 Forbidden - "plain http request as notReadyUser to not existing namespace in community workspace": { - ProxyRequestMethod: "GET", - ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(john)}}, - ExpectedProxyResponseStatus: http.StatusForbidden, - RequestPath: podsInNamespaceRequestURL("smith-community", "not-existing"), - ExpectedResponse: "invalid workspace request: access to namespace 'not-existing' in workspace 'smith-community' is forbidden", + // When john requests the list of pods in a namespace which does not belong to the smith's workspace + // It's not up to the proxy to check permissions on the specific namespace. + // The target API server will reject the request if the user does not have permissions to access the namespace. + // Here the request is successful because the underlying mock target cluster API server returns OK + "plain http request as notReadyUser to namespace outside community workspace": { + ProxyRequestMethod: "GET", + ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(john)}}, + ExpectedAPIServerRequestHeaders: map[string][]string{ + "Authorization": {"Bearer clusterSAToken"}, + "Impersonate-User": {toolchainv1alpha1.KubesawAuthenticatedUsername}, + "X-SSO-User": {"username-" + john.String()}, + }, + ExpectedProxyResponseStatus: http.StatusOK, + RequestPath: podsInNamespaceRequestURL("smith-community", "outside-of-workspace-namespace"), + ExpectedResponse: httpTestServerResponse, }, // Given banned user eve exists // And user smith exists // And smith owns a workspace named smith-community // And smith-community is publicly visible (shared with PublicViewer) - // When eve requests the list of pods in a non existing namespace smith's workspace + // When eve requests the list of pods in a non-existing namespace smith's workspace // Then the proxy does NOT forward the request // And the proxy rejects the call with 403 Forbidden "plain http actual request as banned user to not existing namespace community workspace": { diff --git a/pkg/proxy/proxy_test.go b/pkg/proxy/proxy_test.go index d638a274..8d87df7d 100644 --- a/pkg/proxy/proxy_test.go +++ b/pkg/proxy/proxy_test.go @@ -702,29 +702,43 @@ func (s *TestProxySuite) checkProxyOK(fakeApp *fake.ProxyFakeApp, p *Proxy) { ExpectedResponse: ptr("unable to get target cluster: access to workspace 'not-existing-workspace' is forbidden"), ExpectedProxyResponseStatus: http.StatusInternalServerError, }, - "unauthorized if namespace does not exist in implicit workspace": { + "request to namespace which does not belong to implicit workspace is still proxied OK": { + // It's not up to the proxy to check permissions on the specific namespace. + // The target API server will reject the request if the user does not have permissions to access the namespace. ProxyRequestPaths: map[string]string{ - "not existing namespace": "http://localhost:8081/api/namespaces/not-existing-namespace/pods", + "not existing namespace": "http://localhost:8081/api/namespaces/namespace-outside-of-workspace/pods", }, ProxyRequestMethod: "GET", ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(userID)}}, ExpectedAPIServerRequestHeaders: map[string][]string{ - "Authorization": {"Bearer clusterSAToken"}, + "Authorization": {"Bearer clusterSAToken"}, + "Impersonate-User": {"smith2"}, }, - ExpectedResponse: ptr("invalid workspace request: access to namespace 'not-existing-namespace' in workspace 'mycoolworkspace' is forbidden"), - ExpectedProxyResponseStatus: http.StatusForbidden, + ExpectedProxyResponseHeaders: map[string][]string{ + "Access-Control-Allow-Origin": {"*"}, + "Access-Control-Allow-Credentials": {"true"}, + "Access-Control-Expose-Headers": {"Content-Length, Content-Encoding, Authorization"}, + "Vary": {"Origin"}, + }, + ExpectedProxyResponseStatus: http.StatusOK, }, - "unauthorized if namespace does not exist in explicit workspace": { + "request to namespace which does not belong to explicit workspace is still proxied OK": { ProxyRequestPaths: map[string]string{ - "not existing namespace": "http://localhost:8081/workspaces/mycoolworkspace/api/namespaces/not-existing-namespace/pods", + "not existing namespace": "http://localhost:8081/workspaces/mycoolworkspace/api/namespaces/namespace-outside-of-workspace/pods", }, ProxyRequestMethod: "GET", ProxyRequestHeaders: map[string][]string{"Authorization": {"Bearer " + s.token(userID)}}, ExpectedAPIServerRequestHeaders: map[string][]string{ - "Authorization": {"Bearer clusterSAToken"}, + "Authorization": {"Bearer clusterSAToken"}, + "Impersonate-User": {"smith2"}, }, - ExpectedResponse: ptr("invalid workspace request: access to namespace 'not-existing-namespace' in workspace 'mycoolworkspace' is forbidden"), - ExpectedProxyResponseStatus: http.StatusForbidden, + ExpectedProxyResponseHeaders: map[string][]string{ + "Access-Control-Allow-Origin": {"*"}, + "Access-Control-Allow-Credentials": {"true"}, + "Access-Control-Expose-Headers": {"Content-Length, Content-Encoding, Authorization"}, + "Vary": {"Origin"}, + }, + ExpectedProxyResponseStatus: http.StatusOK, }, } @@ -1109,13 +1123,11 @@ func (s *TestProxySuite) TestGetWorkspaceContext() { func (s *TestProxySuite) TestValidateWorkspaceRequest() { tests := map[string]struct { requestedWorkspace string - requestedNamespace string workspaces []toolchainv1alpha1.Workspace expectedErr string }{ "valid workspace request": { requestedWorkspace: "myworkspace", - requestedNamespace: "ns-dev", workspaces: []toolchainv1alpha1.Workspace{{ ObjectMeta: metav1.ObjectMeta{ Name: "myworkspace", @@ -1141,7 +1153,6 @@ func (s *TestProxySuite) TestValidateWorkspaceRequest() { }, "valid home workspace request": { requestedWorkspace: "", // home workspace is default when no workspace is specified - requestedNamespace: "test-1234", workspaces: []toolchainv1alpha1.Workspace{{ ObjectMeta: metav1.ObjectMeta{ Name: "homews", @@ -1157,7 +1168,6 @@ func (s *TestProxySuite) TestValidateWorkspaceRequest() { }, "workspace not allowed": { requestedWorkspace: "notexist", - requestedNamespace: "myns", workspaces: []toolchainv1alpha1.Workspace{{ ObjectMeta: metav1.ObjectMeta{ Name: "myworkspace", @@ -1170,42 +1180,11 @@ func (s *TestProxySuite) TestValidateWorkspaceRequest() { }}, expectedErr: "access to workspace 'notexist' is forbidden", }, - "namespace not allowed": { - requestedWorkspace: "myworkspace", - requestedNamespace: "notexist", - workspaces: []toolchainv1alpha1.Workspace{{ - ObjectMeta: metav1.ObjectMeta{ - Name: "myworkspace", - }, - Status: toolchainv1alpha1.WorkspaceStatus{ - Namespaces: []toolchainv1alpha1.SpaceNamespace{ - {Name: "ns-dev"}, - }, - }, - }}, - expectedErr: "access to namespace 'notexist' in workspace 'myworkspace' is forbidden", - }, - "namespace not allowed for home workspace": { - requestedWorkspace: "", // home workspace is default when no workspace is specified - requestedNamespace: "myns", - workspaces: []toolchainv1alpha1.Workspace{{ - ObjectMeta: metav1.ObjectMeta{ - Name: "homews", - }, - Status: toolchainv1alpha1.WorkspaceStatus{ - Type: "home", // home workspace - Namespaces: []toolchainv1alpha1.SpaceNamespace{ - {Name: "test-1234"}, // namespace does not match the requested one - }, - }, - }}, - expectedErr: "access to namespace 'myns' in workspace 'homews' is forbidden", - }, } for k, tc := range tests { s.T().Run(k, func(t *testing.T) { - err := validateWorkspaceRequest(tc.requestedWorkspace, tc.requestedNamespace, tc.workspaces...) + err := validateWorkspaceRequest(tc.requestedWorkspace, tc.workspaces...) if tc.expectedErr == "" { require.NoError(t, err) } else { From 738daee4928d00719bf4f57d5d851a4bf56bea34 Mon Sep 17 00:00:00 2001 From: Matous Jobanek Date: Mon, 4 Nov 2024 13:25:45 +0100 Subject: [PATCH 5/5] drop MockableApplication (#481) --- pkg/middleware/jwt_middleware_test.go | 4 +-- pkg/middleware/promhttp_middleware_test.go | 4 +-- pkg/proxy/proxy_community_test.go | 2 -- pkg/proxy/proxy_test.go | 1 - pkg/proxy/service/cluster_service_test.go | 1 - pkg/server/server_test.go | 3 +- test/fake/mockable_application.go | 40 ---------------------- test/testsuite.go | 10 +----- test/util/application.go | 5 +++ 9 files changed, 12 insertions(+), 58 deletions(-) delete mode 100644 test/fake/mockable_application.go diff --git a/pkg/middleware/jwt_middleware_test.go b/pkg/middleware/jwt_middleware_test.go index 780d3aa3..2a98180f 100644 --- a/pkg/middleware/jwt_middleware_test.go +++ b/pkg/middleware/jwt_middleware_test.go @@ -14,7 +14,7 @@ import ( "github.com/codeready-toolchain/registration-service/pkg/proxy" "github.com/codeready-toolchain/registration-service/pkg/server" "github.com/codeready-toolchain/registration-service/test" - "github.com/codeready-toolchain/registration-service/test/fake" + "github.com/codeready-toolchain/registration-service/test/util" "github.com/codeready-toolchain/toolchain-common/pkg/status" commontest "github.com/codeready-toolchain/toolchain-common/pkg/test" authsupport "github.com/codeready-toolchain/toolchain-common/pkg/test/auth" @@ -77,7 +77,7 @@ func (s *TestAuthMiddlewareSuite) TestAuthMiddlewareService() { keysEndpointURL := tokengenerator.NewKeyServer().URL // create server - srv := server.New(fake.NewMockableApplication()) + srv := server.New(util.PrepareInClusterApplication(s.T())) // set the key service url in the config s.SetConfig(testconfig.RegistrationService(). diff --git a/pkg/middleware/promhttp_middleware_test.go b/pkg/middleware/promhttp_middleware_test.go index 45c3c4e8..a1769b45 100644 --- a/pkg/middleware/promhttp_middleware_test.go +++ b/pkg/middleware/promhttp_middleware_test.go @@ -11,7 +11,7 @@ import ( "github.com/codeready-toolchain/registration-service/pkg/proxy" "github.com/codeready-toolchain/registration-service/pkg/server" "github.com/codeready-toolchain/registration-service/test" - "github.com/codeready-toolchain/registration-service/test/fake" + "github.com/codeready-toolchain/registration-service/test/util" commontest "github.com/codeready-toolchain/toolchain-common/pkg/test" authsupport "github.com/codeready-toolchain/toolchain-common/pkg/test/auth" testconfig "github.com/codeready-toolchain/toolchain-common/pkg/test/config" @@ -35,7 +35,7 @@ func TestPromHTTPMiddlewareSuite(t *testing.T) { func (s *PromHTTPMiddlewareSuite) TestPromHTTPMiddleware() { // given tokengenerator := authsupport.NewTokenManager() - srv := server.New(fake.NewMockableApplication()) + srv := server.New(util.PrepareInClusterApplication(s.T())) keysEndpointURL := tokengenerator.NewKeyServer().URL s.SetConfig(testconfig.RegistrationService(). Environment(configuration.UnitTestsEnvironment). diff --git a/pkg/proxy/proxy_community_test.go b/pkg/proxy/proxy_community_test.go index aeaf32fa..793bccfd 100644 --- a/pkg/proxy/proxy_community_test.go +++ b/pkg/proxy/proxy_community_test.go @@ -148,8 +148,6 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr fakeApp.MemberClusterServiceMock = s.newMemberClusterServiceWithMembers(p.Client, signupService, testServer.URL) fakeApp.SignupServiceMock = signupService - s.Application.MockSignupService(signupService) - // configure proxy p.spaceLister = &handlers.SpaceLister{ Client: p.Client, diff --git a/pkg/proxy/proxy_test.go b/pkg/proxy/proxy_test.go index 8d87df7d..44460e11 100644 --- a/pkg/proxy/proxy_test.go +++ b/pkg/proxy/proxy_test.go @@ -840,7 +840,6 @@ func (s *TestProxySuite) checkProxyOK(fakeApp *fake.ProxyFakeApp, p *Proxy) { }, }), ) - s.Application.MockSignupService(fakeApp.SignupServiceMock) proxyPlugin := &toolchainv1alpha1.ProxyPlugin{ ObjectMeta: metav1.ObjectMeta{ diff --git a/pkg/proxy/service/cluster_service_test.go b/pkg/proxy/service/cluster_service_test.go index 4e05e5d0..7a04ac77 100644 --- a/pkg/proxy/service/cluster_service_test.go +++ b/pkg/proxy/service/cluster_service_test.go @@ -68,7 +68,6 @@ func (s *TestClusterServiceSuite) TestGetClusterAccess() { Ready: true, }, })) - s.Application.MockSignupService(sc) pp := &toolchainv1alpha1.ProxyPlugin{ ObjectMeta: metav1.ObjectMeta{ diff --git a/pkg/server/server_test.go b/pkg/server/server_test.go index fc37e81c..4f678c73 100644 --- a/pkg/server/server_test.go +++ b/pkg/server/server_test.go @@ -10,6 +10,7 @@ import ( "github.com/codeready-toolchain/registration-service/pkg/server" "github.com/codeready-toolchain/registration-service/test" "github.com/codeready-toolchain/registration-service/test/fake" + "github.com/codeready-toolchain/registration-service/test/util" commontest "github.com/codeready-toolchain/toolchain-common/pkg/test" "github.com/prometheus/client_golang/prometheus" @@ -36,7 +37,7 @@ const ( func (s *TestServerSuite) TestServer() { // We're using the example config for the configuration here as the // specific config params do not matter for testing the routes setup. - srv := server.New(fake.NewMockableApplication()) + srv := server.New(util.PrepareInClusterApplication(s.T())) fake.MockKeycloakCertsCall(s.T()) // Setting up the routes. diff --git a/test/fake/mockable_application.go b/test/fake/mockable_application.go deleted file mode 100644 index c3416b94..00000000 --- a/test/fake/mockable_application.go +++ /dev/null @@ -1,40 +0,0 @@ -package fake - -import ( - "github.com/codeready-toolchain/registration-service/pkg/application/service" - "github.com/codeready-toolchain/registration-service/pkg/application/service/factory" -) - -func NewMockableApplication(options ...factory.Option) *MockableApplication { - return &MockableApplication{ - serviceFactory: factory.NewServiceFactory(options...), - } -} - -type MockableApplication struct { - serviceFactory *factory.ServiceFactory - mockSignupService service.SignupService - mockVerificationService service.VerificationService -} - -func (m *MockableApplication) SignupService() service.SignupService { - if m.mockSignupService != nil { - return m.mockSignupService - } - return m.serviceFactory.SignupService() -} - -func (m *MockableApplication) MockSignupService(svc service.SignupService) { - m.mockSignupService = svc -} - -func (m *MockableApplication) VerificationService() service.VerificationService { - if m.mockVerificationService != nil { - return m.mockVerificationService - } - return m.serviceFactory.VerificationService() -} - -func (m *MockableApplication) MemberClusterService() service.MemberClusterService { - return m.serviceFactory.MemberClusterService() -} diff --git a/test/testsuite.go b/test/testsuite.go index 30d85691..a1bbee07 100644 --- a/test/testsuite.go +++ b/test/testsuite.go @@ -4,10 +4,8 @@ import ( "context" toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" - "github.com/codeready-toolchain/registration-service/pkg/application/service/factory" "github.com/codeready-toolchain/registration-service/pkg/configuration" "github.com/codeready-toolchain/registration-service/pkg/log" - "github.com/codeready-toolchain/registration-service/test/fake" commonconfig "github.com/codeready-toolchain/toolchain-common/pkg/configuration" "github.com/codeready-toolchain/toolchain-common/pkg/test" testconfig "github.com/codeready-toolchain/toolchain-common/pkg/test/config" @@ -23,9 +21,7 @@ import ( // UnitTestSuite is the base test suite for unit tests. type UnitTestSuite struct { suite.Suite - Application *fake.MockableApplication - ConfigClient *test.FakeClient - factoryOptions []factory.Option + ConfigClient *test.FakeClient } // SetupSuite sets the suite up and sets testmode. @@ -36,19 +32,16 @@ func (s *UnitTestSuite) SetupSuite() { } func (s *UnitTestSuite) SetupTest() { - s.factoryOptions = nil s.SetupDefaultApplication() } func (s *UnitTestSuite) SetupDefaultApplication() { // initialize the toolchainconfig cache s.DefaultConfig() - s.Application = fake.NewMockableApplication(s.factoryOptions...) } func (s *UnitTestSuite) OverrideApplicationDefault(opts ...testconfig.ToolchainConfigOption) { s.SetConfig(opts...) - s.Application = fake.NewMockableApplication(s.factoryOptions...) } func (s *UnitTestSuite) SetConfig(opts ...testconfig.ToolchainConfigOption) configuration.RegistrationServiceConfig { @@ -105,5 +98,4 @@ func (s *UnitTestSuite) DefaultConfig() configuration.RegistrationServiceConfig func (s *UnitTestSuite) TearDownSuite() { // summon the GC! commonconfig.ResetCache() - s.Application = nil } diff --git a/test/util/application.go b/test/util/application.go index 0943eb77..cd90c2f7 100644 --- a/test/util/application.go +++ b/test/util/application.go @@ -11,6 +11,11 @@ import ( "k8s.io/apimachinery/pkg/runtime" ) +func PrepareInClusterApplication(t *testing.T, objects ...runtime.Object) application.Application { + _, app := PrepareInClusterApp(t, objects...) + return app +} + func PrepareInClusterApp(t *testing.T, objects ...runtime.Object) (*commontest.FakeClient, application.Application) { return PrepareInClusterAppWithOption(t, func(_ *factory.ServiceFactory) { }, objects...)