bugfix: proxy rejects calls when UserSignup is not in a ready state a…

…ttempt 2 (#345) * avoid checking for usersignup complete status in proxy Co-authored-by: Alexey Kazakov <[email protected]>
codeready-toolchain · Sep 11, 2023 · d5ffd83 · d5ffd83
1 parent 7640905
commit d5ffd83
Show file tree

Hide file tree

Showing 8 changed files with 161 additions and 108 deletions.
diff --git a/pkg/application/service/services.go b/pkg/application/service/services.go
@@ -20,7 +20,7 @@ type InformerService interface {
 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) (*signup.Signup, error)
+	GetSignupFromInformer(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_test.go b/pkg/controller/signup_test.go
@@ -902,7 +902,7 @@ 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) (*signup.Signup, error)
+	MockGetSignupFromInformer       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)
@@ -913,8 +913,8 @@ func (m *FakeSignupService) GetSignup(ctx *gin.Context, userID, username string)
 	return m.MockGetSignup(ctx, userID, username)
 }
 
-func (m *FakeSignupService) GetSignupFromInformer(ctx *gin.Context, userID, username string) (*signup.Signup, error) {
-	return m.MockGetSignupFromInformer(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) Signup(ctx *gin.Context) (*crtapi.UserSignup, error) {

diff --git a/pkg/proxy/handlers/spacelister.go b/pkg/proxy/handlers/spacelister.go
@@ -3,6 +3,8 @@ package handlers
 import (
 	"encoding/json"
 	"fmt"
+	"net/http"
+
 	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"
@@ -11,7 +13,6 @@ import (
 	"github.com/codeready-toolchain/registration-service/pkg/signup"
 	commonproxy "github.com/codeready-toolchain/toolchain-common/pkg/proxy"
 	"github.com/gin-gonic/gin"
-	"net/http"
 
 	"github.com/labstack/echo/v4"
 	errs "github.com/pkg/errors"
@@ -23,7 +24,7 @@ import (
 )
 
 type SpaceLister struct {
-	GetSignupFunc          func(ctx *gin.Context, userID, username string) (*signup.Signup, error)
+	GetSignupFunc          func(ctx *gin.Context, userID, username string, checkUserSignupCompleted bool) (*signup.Signup, error)
 	GetInformerServiceFunc func() service.InformerService
 }
 
@@ -60,7 +61,7 @@ func (s *SpaceLister) ListUserWorkspaces(ctx echo.Context) ([]toolchainv1alpha1.
 	userID, _ := ctx.Get(context.SubKey).(string)
 	username, _ := ctx.Get(context.UsernameKey).(string)
 
-	signup, err := s.GetSignupFunc(nil, userID, username)
+	signup, err := s.GetSignupFunc(nil, userID, username, true)
 	if err != nil {
 		ctx.Logger().Error(errs.Wrap(err, "error retrieving signup"))
 		return nil, err

diff --git a/pkg/proxy/handlers/spacelister_test.go b/pkg/proxy/handlers/spacelister_test.go
@@ -3,12 +3,13 @@ package handlers_test
 import (
 	"context"
 	"fmt"
-	"github.com/gin-gonic/gin"
 	"net/http"
 	"net/http/httptest"
 	"strings"
 	"testing"
 
+	"github.com/gin-gonic/gin"
+
 	"github.com/codeready-toolchain/registration-service/pkg/application/service"
 	"github.com/codeready-toolchain/registration-service/pkg/configuration"
 	rcontext "github.com/codeready-toolchain/registration-service/pkg/context"
@@ -67,7 +68,7 @@ func TestHandleSpaceListRequest(t *testing.T) {
 			expectedErr          string
 			expectedErrCode      int
 			expectedWorkspace    string
-			overrideSignupFunc   func(ctx *gin.Context, userID, username string) (*signup.Signup, error)
+			overrideSignupFunc   func(ctx *gin.Context, userID, username string, checkUserSignupComplete bool) (*signup.Signup, error)
 			overrideInformerFunc func() service.InformerService
 		}{
 			"dancelover lists spaces": {
@@ -152,7 +153,7 @@ func TestHandleSpaceListRequest(t *testing.T) {
 				expectedWs:      []toolchainv1alpha1.Workspace{},
 				expectedErr:     "signup error",
 				expectedErrCode: 500,
-				overrideSignupFunc: func(ctx *gin.Context, userID, username string) (*signup.Signup, error) {
+				overrideSignupFunc: func(ctx *gin.Context, userID, username string, checkUserSignupComplete bool) (*signup.Signup, error) {
 					return nil, fmt.Errorf("signup error")
 				},
 			},

diff --git a/pkg/proxy/service/cluster_service.go b/pkg/proxy/service/cluster_service.go
@@ -41,7 +41,7 @@ func NewMemberClusterService(context servicecontext.ServiceContext, options ...O
 }
 
 func (s *ServiceImpl) GetClusterAccess(userID, username, workspace, proxyPluginName string) (*access.ClusterAccess, error) {
-	signup, err := s.Services().SignupService().GetSignupFromInformer(nil, userID, username)
+	signup, err := s.Services().SignupService().GetSignupFromInformer(nil, userID, username, false) // 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.
 	if err != nil {
 		return nil, err
 	}

diff --git a/pkg/signup/service/signup_service.go b/pkg/signup/service/signup_service.go
@@ -337,16 +337,18 @@ func (s *ServiceImpl) reactivateUserSignup(ctx *gin.Context, existing *toolchain
 // 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.defaultProvider, userID, username)
+	return s.DoGetSignup(ctx, s.defaultProvider, 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.
-func (s *ServiceImpl) GetSignupFromInformer(ctx *gin.Context, userID, username string) (*signup.Signup, error) {
-	return s.DoGetSignup(ctx, s.Services().InformerService(), userID, username)
+// 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) {
+	return s.DoGetSignup(ctx, s.Services().InformerService(), userID, username, checkUserSignupCompleted)
 }
 
-func (s *ServiceImpl) DoGetSignup(ctx *gin.Context, provider ResourceProvider, userID, username string) (*signup.Signup, error) {
+func (s *ServiceImpl) DoGetSignup(ctx *gin.Context, provider ResourceProvider, userID, username string, checkUserSignupCompleted bool) (*signup.Signup, error) {
 	var userSignup *toolchainv1alpha1.UserSignup
 	var err error
 
@@ -431,7 +433,10 @@ func (s *ServiceImpl) DoGetSignup(ctx *gin.Context, provider ResourceProvider, u
 		return signupResponse, nil
 	}
 
-	if completeCondition.Status != apiv1.ConditionTrue {
+	// in proxy, we don't care if the UserSignup is completed, since sometimes it might be transitioning from complete to provisioning
+	// which cases issues with some proxy calls, that's why we introduced the checkUserSignupCompleted parameter.
+	// See Jira: https://issues.redhat.com/browse/SANDBOX-375
+	if completeCondition.Status != apiv1.ConditionTrue && checkUserSignupCompleted {
 		// UserSignup is not complete
 		log.Info(nil, fmt.Sprintf("usersignup: %s is not complete", userSignup.GetName()))
 		signupResponse.Status = signup.Status{