diff --git a/pkg/application/service/factory/service_factory.go b/pkg/application/service/factory/service_factory.go index 65e46cdc..20474136 100644 --- a/pkg/application/service/factory/service_factory.go +++ b/pkg/application/service/factory/service_factory.go @@ -8,7 +8,6 @@ import ( "github.com/codeready-toolchain/registration-service/pkg/configuration" "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" signupservice "github.com/codeready-toolchain/registration-service/pkg/signup/service" verificationservice "github.com/codeready-toolchain/registration-service/pkg/verification/service" ) @@ -51,10 +50,6 @@ func (s *ServiceFactory) defaultServiceContextProducer() servicecontext.ServiceC } } -func (s *ServiceFactory) MemberClusterService() service.MemberClusterService { - return clusterservice.NewMemberClusterService(s.getContext().Client(), s.getContext().Services().SignupService()) -} - func (s *ServiceFactory) SignupService() service.SignupService { return s.signupServiceFunc(s.signupServiceOptions...) } diff --git a/pkg/application/service/services.go b/pkg/application/service/services.go index 5322143a..4aae2d35 100644 --- a/pkg/application/service/services.go +++ b/pkg/application/service/services.go @@ -2,7 +2,6 @@ package service import ( toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" - "github.com/codeready-toolchain/registration-service/pkg/proxy/access" "github.com/codeready-toolchain/registration-service/pkg/signup" "github.com/gin-gonic/gin" ) @@ -20,12 +19,7 @@ type VerificationService interface { VerifyActivationCode(ctx *gin.Context, userID, username, code string) error } -type MemberClusterService interface { - GetClusterAccess(userID, username, workspace, proxyPluginName string, publicViewerEnabled bool) (*access.ClusterAccess, error) -} - type Services interface { SignupService() SignupService VerificationService() VerificationService - MemberClusterService() MemberClusterService } diff --git a/pkg/proxy/service/cluster_service.go b/pkg/proxy/members.go similarity index 80% rename from pkg/proxy/service/cluster_service.go rename to pkg/proxy/members.go index 5f57adbc..f575b80d 100644 --- a/pkg/proxy/service/cluster_service.go +++ b/pkg/proxy/members.go @@ -1,4 +1,4 @@ -package service +package proxy import ( "context" @@ -20,29 +20,24 @@ import ( "k8s.io/apimachinery/pkg/types" ) -type Option func(f *ServiceImpl) - -// ServiceImpl represents the implementation of the member cluster service. -type ServiceImpl struct { // nolint:revive +// MemberClusters is a type that helps with retrieving access to a specific member cluster +type MemberClusters struct { // nolint:revive namespaced.Client SignupService service.SignupService GetMembersFunc cluster.GetMemberClustersFunc } -// NewMemberClusterService creates a service object for performing toolchain cluster related activities. -func NewMemberClusterService(client namespaced.Client, signupService service.SignupService, options ...Option) service.MemberClusterService { - si := &ServiceImpl{ +// NewMemberClusters creates an instance of the MemberClusters type +func NewMemberClusters(client namespaced.Client, signupService service.SignupService, getMembersFunc cluster.GetMemberClustersFunc) *MemberClusters { + si := &MemberClusters{ Client: client, SignupService: signupService, - GetMembersFunc: cluster.GetMemberClusters, - } - for _, o := range options { - o(si) + GetMembersFunc: getMembersFunc, } return si } -func (s *ServiceImpl) GetClusterAccess(userID, username, workspace, proxyPluginName string, publicViewerEnabled bool) (*access.ClusterAccess, error) { +func (s *MemberClusters) GetClusterAccess(userID, username, workspace, proxyPluginName string, publicViewerEnabled bool) (*access.ClusterAccess, error) { // if workspace is not provided then return the default space access if workspace == "" { return s.getClusterAccessForDefaultWorkspace(userID, username, proxyPluginName) @@ -52,7 +47,7 @@ func (s *ServiceImpl) GetClusterAccess(userID, username, workspace, proxyPluginN } // getSpaceAccess retrieves space access for an user -func (s *ServiceImpl) getSpaceAccess(userID, username, workspace, proxyPluginName string, publicViewerEnabled bool) (*access.ClusterAccess, error) { +func (s *MemberClusters) getSpaceAccess(userID, username, workspace, proxyPluginName string, publicViewerEnabled bool) (*access.ClusterAccess, error) { // retrieve the user's complaint name complaintUserName, err := s.getUserSignupComplaintName(userID, username, publicViewerEnabled) if err != nil { @@ -70,7 +65,7 @@ func (s *ServiceImpl) getSpaceAccess(userID, username, workspace, proxyPluginNam return s.accessForSpace(space, complaintUserName, proxyPluginName) } -func (s *ServiceImpl) getUserSignupComplaintName(userID, username string, publicViewerEnabled bool) (string, error) { +func (s *MemberClusters) getUserSignupComplaintName(userID, username string, publicViewerEnabled bool) (string, error) { // if PublicViewer is enabled and the requested user is the PublicViewer, than no lookup is required if publicViewerEnabled && username == toolchainv1alpha1.KubesawAuthenticatedUsername { return username, nil @@ -86,7 +81,7 @@ func (s *ServiceImpl) getUserSignupComplaintName(userID, username string, public } // getClusterAccessForDefaultWorkspace retrieves the cluster for the user's default workspace -func (s *ServiceImpl) getClusterAccessForDefaultWorkspace(userID, username, proxyPluginName string) (*access.ClusterAccess, error) { +func (s *MemberClusters) getClusterAccessForDefaultWorkspace(userID, username, proxyPluginName string) (*access.ClusterAccess, error) { // retrieve the UserSignup from cache userSignup, err := s.getSignupFromInformerForProvisionedUser(userID, username) if err != nil { @@ -97,7 +92,7 @@ func (s *ServiceImpl) getClusterAccessForDefaultWorkspace(userID, username, prox return s.accessForCluster(userSignup.APIEndpoint, userSignup.ClusterName, userSignup.CompliantUsername, proxyPluginName) } -func (s *ServiceImpl) getSignupFromInformerForProvisionedUser(userID, username string) (*signup.Signup, error) { +func (s *MemberClusters) 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.GetSignup(nil, userID, username, false) @@ -115,7 +110,7 @@ func (s *ServiceImpl) getSignupFromInformerForProvisionedUser(userID, username s return userSignup, nil } -func (s *ServiceImpl) accessForSpace(space *toolchainv1alpha1.Space, username, proxyPluginName string) (*access.ClusterAccess, error) { +func (s *MemberClusters) accessForSpace(space *toolchainv1alpha1.Space, username, proxyPluginName string) (*access.ClusterAccess, error) { // Get the target member members := s.GetMembersFunc() if len(members) == 0 { @@ -138,7 +133,7 @@ func (s *ServiceImpl) accessForSpace(space *toolchainv1alpha1.Space, username, p return nil, errs.New(errMsg) } -func (s *ServiceImpl) accessForCluster(apiEndpoint, clusterName, username, proxyPluginName string) (*access.ClusterAccess, error) { +func (s *MemberClusters) accessForCluster(apiEndpoint, clusterName, username, proxyPluginName string) (*access.ClusterAccess, error) { // Get the target member members := s.GetMembersFunc() if len(members) == 0 { @@ -161,7 +156,7 @@ func (s *ServiceImpl) accessForCluster(apiEndpoint, clusterName, username, proxy return nil, errs.New("no member cluster found for the user") } -func (s *ServiceImpl) getMemberURL(proxyPluginName string, member *cluster.CachedToolchainCluster) (*url.URL, error) { +func (s *MemberClusters) getMemberURL(proxyPluginName string, member *cluster.CachedToolchainCluster) (*url.URL, error) { if member == nil { return nil, errs.New("nil member provided") } diff --git a/pkg/proxy/service/cluster_service_test.go b/pkg/proxy/members_test.go similarity index 76% rename from pkg/proxy/service/cluster_service_test.go rename to pkg/proxy/members_test.go index 7a04ac77..505daf18 100644 --- a/pkg/proxy/service/cluster_service_test.go +++ b/pkg/proxy/members_test.go @@ -1,4 +1,4 @@ -package service_test +package proxy_test import ( "context" @@ -8,8 +8,8 @@ import ( "testing" "github.com/codeready-toolchain/registration-service/pkg/namespaced" + "github.com/codeready-toolchain/registration-service/pkg/proxy" "github.com/codeready-toolchain/registration-service/pkg/proxy/access" - "github.com/codeready-toolchain/registration-service/pkg/proxy/service" "github.com/codeready-toolchain/registration-service/pkg/signup" "github.com/codeready-toolchain/registration-service/test" "github.com/codeready-toolchain/registration-service/test/fake" @@ -29,15 +29,15 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" ) -type TestClusterServiceSuite struct { +type TestMemberClustersSuite struct { test.UnitTestSuite } -func TestRunClusterServiceSuite(t *testing.T) { - suite.Run(t, &TestClusterServiceSuite{test.UnitTestSuite{}}) +func TestRunMemberClustersSuite(t *testing.T) { + suite.Run(t, &TestMemberClustersSuite{test.UnitTestSuite{}}) } -func (s *TestClusterServiceSuite) TestGetClusterAccess() { +func (s *TestMemberClustersSuite) TestGetClusterAccess() { // given sc := fake.NewSignupService(fake.Signup("123-noise", &signup.Signup{ CompliantUsername: "noise1", @@ -88,7 +88,7 @@ func (s *TestClusterServiceSuite) TestGetClusterAccess() { fake.NewSpace("unknown-cluster", "unknown-cluster", "unknown-cluster"), pp) nsClient := namespaced.NewClient(fakeClient, commontest.HostOperatorNs) - svc := service.NewMemberClusterService(nsClient, sc) + members := proxy.NewMemberClusters(nsClient, sc, commoncluster.GetMemberClusters) tt := map[string]struct { publicViewerEnabled bool @@ -117,7 +117,7 @@ func (s *TestClusterServiceSuite) TestGetClusterAccess() { } // when - _, err := svc.GetClusterAccess("789-ready", "", tc.workspace, "", publicViewerEnabled) + _, err := members.GetClusterAccess("789-ready", "", tc.workspace, "", publicViewerEnabled) // then require.EqualError(s.T(), err, "oopsi woopsi") @@ -127,7 +127,7 @@ func (s *TestClusterServiceSuite) TestGetClusterAccess() { s.Run("userid is not found", func() { // when - _, err := svc.GetClusterAccess("unknown_id", "", tc.workspace, "", publicViewerEnabled) + _, err := members.GetClusterAccess("unknown_id", "", tc.workspace, "", publicViewerEnabled) // then require.EqualError(s.T(), err, "user is not provisioned (yet)") @@ -135,7 +135,7 @@ func (s *TestClusterServiceSuite) TestGetClusterAccess() { s.Run("username is not found", func() { // when - _, err := svc.GetClusterAccess("", "unknown_username", tc.workspace, "", publicViewerEnabled) + _, err := members.GetClusterAccess("", "unknown_username", tc.workspace, "", publicViewerEnabled) // then require.EqualError(s.T(), err, "user is not provisioned (yet)") @@ -143,7 +143,7 @@ func (s *TestClusterServiceSuite) TestGetClusterAccess() { s.Run("user is not provisioned yet", func() { // when - _, err := svc.GetClusterAccess("456-not-ready", "", tc.workspace, "", publicViewerEnabled) + _, err := members.GetClusterAccess("456-not-ready", "", tc.workspace, "", publicViewerEnabled) // then require.EqualError(s.T(), err, "user is not provisioned (yet)") @@ -154,7 +154,7 @@ func (s *TestClusterServiceSuite) TestGetClusterAccess() { }) s.Run("unable to get space", func() { - s.Run("informer service returns error", func() { + s.Run("get Space 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" { @@ -166,10 +166,10 @@ func (s *TestClusterServiceSuite) TestGetClusterAccess() { fakeClient.MockGet = nil }() nsClient := namespaced.NewClient(fakeClient, commontest.HostOperatorNs) - svc := service.NewMemberClusterService(nsClient, sc) + members := proxy.NewMemberClusters(nsClient, sc, commoncluster.GetMemberClusters) // when - _, err := svc.GetClusterAccess("789-ready", "", "smith2", "", publicViewerEnabled) + _, err := members.GetClusterAccess("789-ready", "", "smith2", "", publicViewerEnabled) // then // original error is only logged so that it doesn't reveal information about a space that may not belong to the requestor @@ -178,7 +178,7 @@ func (s *TestClusterServiceSuite) TestGetClusterAccess() { s.Run("space not found", func() { // when - _, err := svc.GetClusterAccess("789-ready", "", "unknown", "", publicViewerEnabled) // unknown workspace requested + _, err := members.GetClusterAccess("789-ready", "", "unknown", "", publicViewerEnabled) // unknown workspace requested // then require.EqualError(s.T(), err, "the requested space is not available") @@ -187,16 +187,12 @@ func (s *TestClusterServiceSuite) TestGetClusterAccess() { s.Run("no member cluster found", func() { s.Run("no member clusters", func() { - svc := service.NewMemberClusterService(nsClient, sc, - func(si *service.ServiceImpl) { - si.GetMembersFunc = func(_ ...commoncluster.Condition) []*commoncluster.CachedToolchainCluster { - return []*commoncluster.CachedToolchainCluster{} - } - }, - ) + members := proxy.NewMemberClusters(nsClient, sc, func(_ ...commoncluster.Condition) []*commoncluster.CachedToolchainCluster { + return []*commoncluster.CachedToolchainCluster{} + }) s.Run("default workspace case", func() { // when - _, err := svc.GetClusterAccess("789-ready", "", "", "", publicViewerEnabled) + _, err := members.GetClusterAccess("789-ready", "", "", "", publicViewerEnabled) // then require.EqualError(s.T(), err, "no member clusters found") @@ -204,7 +200,7 @@ func (s *TestClusterServiceSuite) TestGetClusterAccess() { s.Run("workspace context case", func() { // when - _, err := svc.GetClusterAccess("789-ready", "", "smith2", "", publicViewerEnabled) + _, err := members.GetClusterAccess("789-ready", "", "smith2", "", publicViewerEnabled) // then require.EqualError(s.T(), err, "no member clusters found") @@ -212,17 +208,12 @@ func (s *TestClusterServiceSuite) TestGetClusterAccess() { }) s.Run("no member cluster with the given URL", func() { - svc := service.NewMemberClusterService(nsClient, sc, - func(si *service.ServiceImpl) { - si.GetMembersFunc = func(_ ...commoncluster.Condition) []*commoncluster.CachedToolchainCluster { - return s.memberClusters() - } - }, - ) - + members := proxy.NewMemberClusters(nsClient, sc, func(_ ...commoncluster.Condition) []*commoncluster.CachedToolchainCluster { + return s.memberClusters() + }) s.Run("default workspace case", func() { // when - _, err := svc.GetClusterAccess("012-ready-unknown-cluster", "", "", "", publicViewerEnabled) + _, err := members.GetClusterAccess("012-ready-unknown-cluster", "", "", "", publicViewerEnabled) // then require.EqualError(s.T(), err, "no member cluster found for the user") @@ -230,7 +221,7 @@ func (s *TestClusterServiceSuite) TestGetClusterAccess() { s.Run("workspace context case", func() { // when - _, err := svc.GetClusterAccess("012-ready-unknown-cluster", "", "unknown-cluster", "", publicViewerEnabled) + _, err := members.GetClusterAccess("012-ready-unknown-cluster", "", "unknown-cluster", "", publicViewerEnabled) // then require.EqualError(s.T(), err, "no member cluster found for space 'unknown-cluster'") @@ -270,14 +261,9 @@ func (s *TestClusterServiceSuite) TestGetClusterAccess() { }, } - svc := service.NewMemberClusterService(nsClient, sc, - func(si *service.ServiceImpl) { - si.GetMembersFunc = func(_ ...commoncluster.Condition) []*commoncluster.CachedToolchainCluster { - return memberArray - } - }, - ) - + members := proxy.NewMemberClusters(nsClient, sc, func(_ ...commoncluster.Condition) []*commoncluster.CachedToolchainCluster { + return memberArray + }) s.Run("verify cluster access with route", func() { memberClient.MockGet = func(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { route, ok := obj.(*routev1.Route) @@ -296,7 +282,7 @@ func (s *TestClusterServiceSuite) TestGetClusterAccess() { expectedToken := "abc123" // should match member 2 bearer token // when - ca, err := svc.GetClusterAccess("789-ready", "", "", "tekton-results", publicViewerEnabled) + ca, err := members.GetClusterAccess("789-ready", "", "", "tekton-results", publicViewerEnabled) // then require.NoError(s.T(), err) @@ -309,7 +295,7 @@ func (s *TestClusterServiceSuite) TestGetClusterAccess() { s.Run("cluster access correct when username provided", func() { // when - ca, err := svc.GetClusterAccess("", "smith@", "", "tekton-results", publicViewerEnabled) + ca, err := members.GetClusterAccess("", "smith@", "", "tekton-results", publicViewerEnabled) // then require.NoError(s.T(), err) @@ -322,7 +308,7 @@ func (s *TestClusterServiceSuite) TestGetClusterAccess() { s.Run("cluster access correct when using workspace context", func() { // when - ca, err := svc.GetClusterAccess("789-ready", "", "smith2", "tekton-results", publicViewerEnabled) // workspace-context specified + ca, err := members.GetClusterAccess("789-ready", "", "smith2", "tekton-results", publicViewerEnabled) // workspace-context specified // then require.NoError(s.T(), err) @@ -350,7 +336,7 @@ func (s *TestClusterServiceSuite) TestGetClusterAccess() { return memberClient.Client.Get(ctx, key, obj, opts...) } memberArray[0].Client = mC - ca, err := svc.GetClusterAccess("789-ready", "", "teamspace", "tekton-results", publicViewerEnabled) // workspace-context specified + ca, err := members.GetClusterAccess("789-ready", "", "teamspace", "tekton-results", publicViewerEnabled) // workspace-context specified // then require.NoError(s.T(), err) @@ -368,7 +354,7 @@ func (s *TestClusterServiceSuite) TestGetClusterAccess() { expectedToken := "abc123" // should match member 2 bearer token // when - ca, err := svc.GetClusterAccess("789-ready", "", "", "", publicViewerEnabled) + ca, err := members.GetClusterAccess("789-ready", "", "", "", publicViewerEnabled) // then require.NoError(s.T(), err) @@ -381,7 +367,7 @@ func (s *TestClusterServiceSuite) TestGetClusterAccess() { s.Run("cluster access correct when username provided", func() { // when - ca, err := svc.GetClusterAccess("", "smith@", "", "", publicViewerEnabled) + ca, err := members.GetClusterAccess("", "smith@", "", "", publicViewerEnabled) // then require.NoError(s.T(), err) @@ -394,7 +380,7 @@ func (s *TestClusterServiceSuite) TestGetClusterAccess() { s.Run("cluster access correct when using workspace context", func() { // when - ca, err := svc.GetClusterAccess("789-ready", "", "smith2", "", publicViewerEnabled) // workspace-context specified + ca, err := members.GetClusterAccess("789-ready", "", "smith2", "", publicViewerEnabled) // workspace-context specified // then require.NoError(s.T(), err) @@ -406,7 +392,7 @@ func (s *TestClusterServiceSuite) TestGetClusterAccess() { s.Run("another workspace on another cluster", func() { // when - ca, err := svc.GetClusterAccess("789-ready", "", "teamspace", "", publicViewerEnabled) // workspace-context specified + ca, err := members.GetClusterAccess("789-ready", "", "teamspace", "", publicViewerEnabled) // workspace-context specified // then require.NoError(s.T(), err) @@ -426,7 +412,7 @@ func (s *TestClusterServiceSuite) TestGetClusterAccess() { s.Run("user is public-viewer", func() { s.Run("has no default workspace", func() { // when - ca, err := svc.GetClusterAccess("", toolchainv1alpha1.KubesawAuthenticatedUsername, "", "", true) + ca, err := members.GetClusterAccess("", toolchainv1alpha1.KubesawAuthenticatedUsername, "", "", true) // then require.EqualError(s.T(), err, "user is not provisioned (yet)") @@ -434,17 +420,12 @@ func (s *TestClusterServiceSuite) TestGetClusterAccess() { }) s.Run("get workspace by name", func() { - svc := service.NewMemberClusterService(nsClient, sc, - func(si *service.ServiceImpl) { - si.GetMembersFunc = func(_ ...commoncluster.Condition) []*commoncluster.CachedToolchainCluster { - return s.memberClusters() - } - }, - ) - + members := proxy.NewMemberClusters(nsClient, sc, func(_ ...commoncluster.Condition) []*commoncluster.CachedToolchainCluster { + return s.memberClusters() + }) s.Run("public-viewer is disabled", func() { // when - ca, err := svc.GetClusterAccess("", toolchainv1alpha1.KubesawAuthenticatedUsername, "smith2", "", false) + ca, err := members.GetClusterAccess("", toolchainv1alpha1.KubesawAuthenticatedUsername, "smith2", "", false) // then require.EqualError(s.T(), err, "user is not provisioned (yet)") @@ -458,7 +439,7 @@ func (s *TestClusterServiceSuite) TestGetClusterAccess() { expectedClusterAccess := access.NewClusterAccess(*expectedURL, "token", toolchainv1alpha1.KubesawAuthenticatedUsername) // when - clusterAccess, err := svc.GetClusterAccess("", toolchainv1alpha1.KubesawAuthenticatedUsername, "smith2", "", true) + clusterAccess, err := members.GetClusterAccess("", toolchainv1alpha1.KubesawAuthenticatedUsername, "smith2", "", true) // then require.NoError(s.T(), err) @@ -467,7 +448,7 @@ func (s *TestClusterServiceSuite) TestGetClusterAccess() { s.Run("not-available space", func() { // when - clusterAccess, err := svc.GetClusterAccess("", toolchainv1alpha1.KubesawAuthenticatedUsername, "456-not-ready", "", true) + clusterAccess, err := members.GetClusterAccess("", toolchainv1alpha1.KubesawAuthenticatedUsername, "456-not-ready", "", true) // then require.EqualError(s.T(), err, "the requested space is not available") @@ -476,7 +457,7 @@ func (s *TestClusterServiceSuite) TestGetClusterAccess() { s.Run("ready space with unknown cluster", func() { // when - clusterAccess, err := svc.GetClusterAccess("", toolchainv1alpha1.KubesawAuthenticatedUsername, "012-ready-unknown-cluster", "", true) + clusterAccess, err := members.GetClusterAccess("", toolchainv1alpha1.KubesawAuthenticatedUsername, "012-ready-unknown-cluster", "", true) // then require.EqualError(s.T(), err, "the requested space is not available") @@ -486,19 +467,19 @@ func (s *TestClusterServiceSuite) TestGetClusterAccess() { }) } -func (s *TestClusterServiceSuite) assertClusterAccess(expected, actual *access.ClusterAccess) { +func (s *TestMemberClustersSuite) assertClusterAccess(expected, actual *access.ClusterAccess) { require.NotNil(s.T(), expected) require.NotNil(s.T(), actual) assert.Equal(s.T(), expected.APIURL(), actual.APIURL()) assert.Equal(s.T(), expected.ImpersonatorToken(), actual.ImpersonatorToken()) } -func (s *TestClusterServiceSuite) memberClusters() []*commoncluster.CachedToolchainCluster { - cls := make([]*commoncluster.CachedToolchainCluster, 0, 3) +func (s *TestMemberClustersSuite) memberClusters() []*commoncluster.CachedToolchainCluster { + toolchainClusters := make([]*commoncluster.CachedToolchainCluster, 0, 3) for i := 0; i < 3; i++ { clusterName := fmt.Sprintf("member-%d", i) - cls = append(cls, &commoncluster.CachedToolchainCluster{ + toolchainClusters = append(toolchainClusters, &commoncluster.CachedToolchainCluster{ Config: &commoncluster.Config{ Name: clusterName, APIEndpoint: fmt.Sprintf("https://api.endpoint.%s.com:6443", clusterName), @@ -510,5 +491,5 @@ func (s *TestClusterServiceSuite) memberClusters() []*commoncluster.CachedToolch Client: nil, }) } - return cls + return toolchainClusters } diff --git a/pkg/proxy/proxy.go b/pkg/proxy/proxy.go index d2602ad7..36195b76 100644 --- a/pkg/proxy/proxy.go +++ b/pkg/proxy/proxy.go @@ -18,6 +18,7 @@ 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/auth" "github.com/codeready-toolchain/registration-service/pkg/configuration" "github.com/codeready-toolchain/registration-service/pkg/context" @@ -64,7 +65,7 @@ func authorizationEndpointTarget() string { type Proxy struct { namespaced.Client - app application.Application + signupService service.SignupService tokenParser *auth.TokenParser spaceLister *handlers.SpaceLister metrics *metrics.ProxyMetrics @@ -81,7 +82,7 @@ func NewProxy(nsClient namespaced.Client, app application.Application, proxyMetr spaceLister := handlers.NewSpaceLister(nsClient, app, proxyMetrics) return &Proxy{ Client: nsClient, - app: app, + signupService: app.SignupService(), tokenParser: tokenParser, spaceLister: spaceLister, metrics: proxyMetrics, @@ -296,7 +297,8 @@ func (p *Proxy) processRequest(ctx echo.Context) (string, *access.ClusterAccess, // processHomeWorkspaceRequest process an HTTP Request targeting the user's home workspace. func (p *Proxy) processHomeWorkspaceRequest(ctx echo.Context, userID, username, proxyPluginName string) (*access.ClusterAccess, error) { // retrieves the ClusterAccess for the user and their home workspace - cluster, err := p.app.MemberClusterService().GetClusterAccess(userID, username, "", proxyPluginName, false) + members := NewMemberClusters(p.Client, p.signupService, p.getMembersFunc) + cluster, err := members.GetClusterAccess(userID, username, "", proxyPluginName, false) if err != nil { return nil, crterrors.NewInternalError(errs.New("unable to get target cluster"), err.Error()) } @@ -374,7 +376,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().GetSignup(nil, userID, username, false) + userSignup, err := p.signupService.GetSignup(nil, userID, username, false) if err != nil { return err } @@ -410,7 +412,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().GetSignup(nil, userID, username, false) + userSignup, err := p.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") @@ -418,8 +420,9 @@ func (p *Proxy) getClusterAccessAsUserOrPublicViewer(ctx echo.Context, userID, u // proceed as PublicViewer if the feature is enabled and userSignup is nil publicViewerEnabled := context.IsPublicViewerEnabled(ctx) + members := NewMemberClusters(p.Client, p.signupService, p.getMembersFunc) if publicViewerEnabled && !userHasDirectAccess(userSignup, workspace) { - return p.app.MemberClusterService().GetClusterAccess( + return members.GetClusterAccess( toolchainv1alpha1.KubesawAuthenticatedUsername, toolchainv1alpha1.KubesawAuthenticatedUsername, workspace.Name, @@ -428,7 +431,7 @@ func (p *Proxy) getClusterAccessAsUserOrPublicViewer(ctx echo.Context, userID, u } // otherwise retrieve the ClusterAccess for the cluster hosting the workspace and the given user. - return p.app.MemberClusterService().GetClusterAccess(userID, username, workspace.Name, proxyPluginName, publicViewerEnabled) + return members.GetClusterAccess(userID, username, workspace.Name, proxyPluginName, publicViewerEnabled) } // userHasDirectAccess checks if an UserSignup has access to a workspace. diff --git a/pkg/proxy/proxy_community_test.go b/pkg/proxy/proxy_community_test.go index 793bccfd..92b9eee7 100644 --- a/pkg/proxy/proxy_community_test.go +++ b/pkg/proxy/proxy_community_test.go @@ -42,8 +42,7 @@ func (s *TestProxySuite) TestProxyCommunityEnabled() { testconfig.RegistrationService().Environment(string(environment)), testconfig.PublicViewerConfig(true), ) - fakeApp := &fake.ProxyFakeApp{} - p, server := s.spinUpProxy(fakeApp, port) + proxy, server := s.spinUpProxy(port) defer func() { _ = server.Close() }() @@ -57,12 +56,12 @@ func (s *TestProxySuite) TestProxyCommunityEnabled() { }) // run community tests - s.checkProxyCommunityOK(fakeApp, p, port) + s.checkProxyCommunityOK(proxy, port) }) } } -func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Proxy, port string) { +func (s *TestProxySuite) checkProxyCommunityOK(proxy *Proxy, port string) { podsRequestURL := func(workspace string) string { return fmt.Sprintf("http://localhost:%s/workspaces/%s/api/pods", port, workspace) } @@ -140,19 +139,16 @@ func (s *TestProxySuite) checkProxyCommunityOK(fakeApp *fake.ProxyFakeApp, p *Pr fake.NewBase1NSTemplateTier(), ) - // configure informer - p.Client.Client = cli - - // configure Application - fakeApp.Err = nil - fakeApp.MemberClusterServiceMock = s.newMemberClusterServiceWithMembers(p.Client, signupService, testServer.URL) - fakeApp.SignupServiceMock = signupService + // configure proxy to the latest mocks + proxy.Client.Client = cli + proxy.getMembersFunc = s.newMemberClustersFunc(testServer.URL) + proxy.signupService = signupService // configure proxy - p.spaceLister = &handlers.SpaceLister{ - Client: p.Client, - GetSignupFunc: fakeApp.SignupServiceMock.GetSignup, - ProxyMetrics: p.metrics, + proxy.spaceLister = &handlers.SpaceLister{ + Client: proxy.Client, + GetSignupFunc: proxy.signupService.GetSignup, + ProxyMetrics: proxy.metrics, } // run test cases diff --git a/pkg/proxy/proxy_test.go b/pkg/proxy/proxy_test.go index 44460e11..3976914a 100644 --- a/pkg/proxy/proxy_test.go +++ b/pkg/proxy/proxy_test.go @@ -5,7 +5,6 @@ import ( "context" "crypto/tls" "encoding/base64" - "errors" "fmt" "io" "log" @@ -17,17 +16,15 @@ import ( "testing" "time" - appservice "github.com/codeready-toolchain/registration-service/pkg/application/service" "github.com/codeready-toolchain/registration-service/pkg/auth" "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/proxy/service" proxytest "github.com/codeready-toolchain/registration-service/pkg/proxy/test" "github.com/codeready-toolchain/registration-service/pkg/signup" "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/gin-gonic/gin" "github.com/google/uuid" "github.com/prometheus/client_golang/prometheus" @@ -92,7 +89,7 @@ func (s *TestProxySuite) TestProxy() { s.SetConfig(testconfig.RegistrationService(). Environment(string(environment))) - fakeClient := commontest.NewFakeClient(s.T(), &bannedUser) + fakeClient, app := util.PrepareInClusterApp(s.T(), &bannedUser) fakeClient.MockList = func(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error { listOptions := &client.ListOptions{} for _, opt := range opts { @@ -105,12 +102,11 @@ func (s *TestProxySuite) TestProxy() { } nsClient := namespaced.NewClient(fakeClient, commontest.HostOperatorNs) - fakeApp := &fake.ProxyFakeApp{} proxyMetrics := metrics.NewProxyMetrics(prometheus.NewRegistry()) - p, err := NewProxy(nsClient, fakeApp, proxyMetrics, proxytest.NewGetMembersFunc(commontest.NewFakeClient(s.T()))) + proxy, err := NewProxy(nsClient, app, proxyMetrics, proxytest.NewGetMembersFunc(commontest.NewFakeClient(s.T()))) require.NoError(s.T(), err) - server := p.StartProxy(DefaultPort) + server := proxy.StartProxy(DefaultPort) require.NotNil(s.T(), server) defer func() { _ = server.Close() @@ -123,24 +119,25 @@ func (s *TestProxySuite) TestProxy() { s.checkProxyIsHealthy(DefaultPort) }) - s.checkPlainHTTPErrors(fakeApp) + s.checkPlainHTTPErrors(proxy) s.checkWebsocketsError() s.checkWebLogin() - s.checkProxyOK(fakeApp, p) + s.checkProxyOK(proxy) }) } } -func (s *TestProxySuite) spinUpProxy(fakeApp *fake.ProxyFakeApp, port string) (*Proxy, *http.Server) { +func (s *TestProxySuite) spinUpProxy(port string) (*Proxy, *http.Server) { proxyMetrics := metrics.NewProxyMetrics(prometheus.NewRegistry()) - p, err := NewProxy(namespaced.NewClient(commontest.NewFakeClient(s.T()), commontest.HostOperatorNs), - fakeApp, proxyMetrics, proxytest.NewGetMembersFunc(commontest.NewFakeClient(s.T()))) + fakeClient, app := util.PrepareInClusterApp(s.T()) + proxy, err := NewProxy(namespaced.NewClient(fakeClient, commontest.HostOperatorNs), + app, proxyMetrics, proxytest.NewGetMembersFunc(commontest.NewFakeClient(s.T()))) require.NoError(s.T(), err) - server := p.StartProxy(port) + server := proxy.StartProxy(port) require.NotNil(s.T(), server) - return p, server + return proxy, server } func (s *TestProxySuite) waitForProxyToBeAlive(port string) { @@ -187,7 +184,7 @@ func (s *TestProxySuite) checkProxyIsHealthy(port string) { s.assertResponseBody(resp, `{"alive": true}`) } -func (s *TestProxySuite) checkPlainHTTPErrors(fakeApp *fake.ProxyFakeApp) { +func (s *TestProxySuite) checkPlainHTTPErrors(proxy *Proxy) { s.Run("plain http error", func() { s.Run("unauthorized if no token present", func() { req, err := http.NewRequest("GET", "http://localhost:8081/api/mycoolworkspace/pods", nil) @@ -272,12 +269,16 @@ func (s *TestProxySuite) checkPlainHTTPErrors(fakeApp *fake.ProxyFakeApp) { s.assertResponseBody(resp, "unable to get workspace context: workspace request path has too few segments '/workspaces/myworkspace'; expected path format: /workspaces//api/...") }) - s.Run("internal error if get accesses returns an error", func() { + s.Run("empty set of member clusters", func() { // given - fakeApp.Err = errors.New("some-error") - defer func() { fakeApp.Err = nil }() + origGetMembersFunc := proxy.getMembersFunc + proxy.getMembersFunc = func(_ ...commoncluster.Condition) []*commoncluster.CachedToolchainCluster { + return nil + } + defer func() { + proxy.getMembersFunc = origGetMembersFunc + }() req := s.request() - fakeApp.Accesses = map[string]*access.ClusterAccess{} // when resp, err := http.DefaultClient.Do(req) @@ -287,13 +288,11 @@ func (s *TestProxySuite) checkPlainHTTPErrors(fakeApp *fake.ProxyFakeApp) { require.NotNil(s.T(), resp) defer resp.Body.Close() assert.Equal(s.T(), http.StatusInternalServerError, resp.StatusCode) - s.assertResponseBody(resp, "unable to get target cluster: some-error") + s.assertResponseBody(resp, "unable to get target cluster: user is not provisioned (yet)") }) s.Run("internal error if accessing incorrect url", func() { // given - fakeApp.Err = errors.New("some-error") - defer func() { fakeApp.Err = nil }() req := s.request() req.URL.Path = "http://localhost:8081/metrics" require.NotNil(s.T(), req) @@ -493,7 +492,7 @@ func (s *TestProxySuite) checkWebLogin() { }) } -func (s *TestProxySuite) checkProxyOK(fakeApp *fake.ProxyFakeApp, p *Proxy) { +func (s *TestProxySuite) checkProxyOK(proxy *Proxy) { s.Run("successfully proxy", func() { userID := uuid.New() @@ -792,8 +791,6 @@ func (s *TestProxySuite) checkProxyOK(fakeApp *fake.ProxyFakeApp, p *Proxy) { } } - fakeApp.Err = nil - if !tc.Standalone { testServer.Config.Handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") @@ -818,7 +815,7 @@ func (s *TestProxySuite) checkProxyOK(fakeApp *fake.ProxyFakeApp, p *Proxy) { } } }) - fakeApp.SignupServiceMock = fake.NewSignupService( + proxy.signupService = fake.NewSignupService( fake.Signup("someUserID", &signup.Signup{ Name: "smith1", APIEndpoint: "https://api.endpoint.member-1.com:6443", @@ -848,7 +845,7 @@ func (s *TestProxySuite) checkProxyOK(fakeApp *fake.ProxyFakeApp, p *Proxy) { }, Spec: toolchainv1alpha1.ProxyPluginSpec{ OpenShiftRouteTargetEndpoint: &toolchainv1alpha1.OpenShiftRouteTarget{ - Namespace: commontest.HostOperatorNs, + Namespace: commontest.MemberOperatorNs, Name: "proxy-plugin", }, }, @@ -860,16 +857,16 @@ func (s *TestProxySuite) checkProxyOK(fakeApp *fake.ProxyFakeApp, p *Proxy) { fake.NewSpaceBinding("mycoolworkspace-smith2", "smith2", "mycoolworkspace", "admin"), proxyPlugin, fake.NewBase1NSTemplateTier()) - p.Client.Client = fakeClient - fakeApp.MemberClusterServiceMock = s.newMemberClusterServiceWithMembers(p.Client, fakeApp.SignupServiceMock, testServer.URL) - p.spaceLister = &handlers.SpaceLister{ - Client: p.Client, - GetSignupFunc: fakeApp.SignupServiceMock.GetSignup, - ProxyMetrics: p.metrics, + proxy.Client.Client = fakeClient + proxy.getMembersFunc = s.newMemberClustersFunc(testServer.URL) + proxy.spaceLister = &handlers.SpaceLister{ + Client: proxy.Client, + GetSignupFunc: proxy.signupService.GetSignup, + ProxyMetrics: proxy.metrics, } if tc.OverrideGetSignupFunc != nil { - p.spaceLister.GetSignupFunc = tc.OverrideGetSignupFunc + proxy.spaceLister.GetSignupFunc = tc.OverrideGetSignupFunc } } @@ -906,7 +903,7 @@ type headerToAdd struct { key, value string } -func (s *TestProxySuite) newMemberClusterServiceWithMembers(nsClient namespaced.Client, signupService appservice.SignupService, serverURL string) appservice.MemberClusterService { +func (s *TestProxySuite) newMemberClustersFunc(serverURL string) commoncluster.GetMemberClustersFunc { serverHost := serverURL switch { case strings.HasPrefix(serverURL, "http://"): @@ -917,7 +914,7 @@ func (s *TestProxySuite) newMemberClusterServiceWithMembers(nsClient namespaced. route := &routev1.Route{ ObjectMeta: metav1.ObjectMeta{ - Namespace: nsClient.Namespace, + Namespace: commontest.MemberOperatorNs, Name: "proxy-plugin", }, Spec: routev1.RouteSpec{ @@ -931,34 +928,28 @@ func (s *TestProxySuite) newMemberClusterServiceWithMembers(nsClient namespaced. }, }, } - return service.NewMemberClusterService( - nsClient, - signupService, - func(si *service.ServiceImpl) { - si.GetMembersFunc = func(_ ...commoncluster.Condition) []*commoncluster.CachedToolchainCluster { - return []*commoncluster.CachedToolchainCluster{ - { - Config: &commoncluster.Config{ - Name: "member-1", - APIEndpoint: "https://api.endpoint.member-1.com:6443", - RestConfig: &rest.Config{}, - }, - }, - { - Config: &commoncluster.Config{ - Name: "member-2", - APIEndpoint: serverURL, - OperatorNamespace: "member-operator", - RestConfig: &rest.Config{ - BearerToken: "clusterSAToken", - }, - }, - Client: commontest.NewFakeClient(s.T(), route), + return func(_ ...commoncluster.Condition) []*commoncluster.CachedToolchainCluster { + return []*commoncluster.CachedToolchainCluster{ + { + Config: &commoncluster.Config{ + Name: "member-1", + APIEndpoint: "https://api.endpoint.member-1.com:6443", + RestConfig: &rest.Config{}, + }, + }, + { + Config: &commoncluster.Config{ + Name: "member-2", + APIEndpoint: serverURL, + OperatorNamespace: "member-operator", + RestConfig: &rest.Config{ + BearerToken: "clusterSAToken", }, - } - } - }, - ) + }, + Client: commontest.NewFakeClient(s.T(), route), + }, + } + } } var noCORSHeaders = map[string][]string{ diff --git a/pkg/server/in_cluster_application.go b/pkg/server/in_cluster_application.go index 9658050d..773bc8d6 100644 --- a/pkg/server/in_cluster_application.go +++ b/pkg/server/in_cluster_application.go @@ -30,7 +30,3 @@ func (r InClusterApplication) SignupService() service.SignupService { func (r InClusterApplication) VerificationService() service.VerificationService { return r.serviceFactory.VerificationService() } - -func (r InClusterApplication) MemberClusterService() service.MemberClusterService { - return r.serviceFactory.MemberClusterService() -} diff --git a/test/fake/proxy.go b/test/fake/proxy.go index f25b658b..2cfb95a5 100644 --- a/test/fake/proxy.go +++ b/test/fake/proxy.go @@ -2,47 +2,12 @@ package fake import ( toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" - "github.com/codeready-toolchain/registration-service/pkg/application/service" - "github.com/codeready-toolchain/registration-service/pkg/proxy/access" "github.com/codeready-toolchain/registration-service/pkg/signup" "github.com/gin-gonic/gin" ) // This whole service abstraction is such a huge pain. We have to get rid of it!!! -type ProxyFakeApp struct { - Accesses map[string]*access.ClusterAccess - Err error - SignupServiceMock service.SignupService - MemberClusterServiceMock service.MemberClusterService -} - -func (a *ProxyFakeApp) SignupService() service.SignupService { - if a.SignupServiceMock != nil { - return a.SignupServiceMock - } - return NewSignupService() -} - -func (a *ProxyFakeApp) VerificationService() service.VerificationService { - panic("VerificationService shouldn't be called") -} - -func (a *ProxyFakeApp) MemberClusterService() service.MemberClusterService { - if a.MemberClusterServiceMock != nil { - return a.MemberClusterServiceMock - } - return &fakeClusterService{a} -} - -type fakeClusterService struct { - fakeApp *ProxyFakeApp -} - -func (f *fakeClusterService) GetClusterAccess(userID, _, _, _ string, _ bool) (*access.ClusterAccess, error) { - return f.fakeApp.Accesses[userID], f.fakeApp.Err -} - type SignupDef func() (string, *signup.Signup) func Signup(identifier string, userSignup *signup.Signup) SignupDef {