Change mutex scope to bind address port (#430)

pkg/usecases/credentialplugin/get_token.go

@@ -6,19 +6,20 @@ package credentialplugin
 import (
 	"context"
 	"fmt"
+	"net"
 	"strings"
 
-	"github.com/int128/kubelogin/pkg/credentialplugin"
-	"github.com/int128/kubelogin/pkg/infrastructure/mutex"
-	"github.com/int128/kubelogin/pkg/tokencache/repository"
-
 	"github.com/google/wire"
+	"github.com/int128/kubelogin/pkg/credentialplugin"
 	"github.com/int128/kubelogin/pkg/credentialplugin/writer"
 	"github.com/int128/kubelogin/pkg/infrastructure/logger"
+	"github.com/int128/kubelogin/pkg/infrastructure/mutex"
 	"github.com/int128/kubelogin/pkg/oidc"
 	"github.com/int128/kubelogin/pkg/tlsclientconfig"
 	"github.com/int128/kubelogin/pkg/tokencache"
+	"github.com/int128/kubelogin/pkg/tokencache/repository"
 	"github.com/int128/kubelogin/pkg/usecases/authentication"
+	"github.com/int128/kubelogin/pkg/usecases/authentication/authcode"
 )
 
 //go:generate mockgen -destination mock_credentialplugin/mock_credentialplugin.go github.com/int128/kubelogin/pkg/usecases/credentialplugin Interface
@@ -51,14 +52,22 @@ type GetToken struct {
 func (u *GetToken) Do(ctx context.Context, in Input) error {
 	u.Logger.V(1).Infof("WARNING: log may contain your secrets such as token or password")
 
-	// Prevent multiple concurrent token query using a file mutex. See https://github.com/int128/kubelogin/issues/389
-	lock, err := u.Mutex.Acquire(ctx, "get-token")
-	if err != nil {
-		return err
+	// Prevent multiple concurrent port binding using a file mutex.
+	// See https://github.com/int128/kubelogin/issues/389
+	bindPorts := extractBindAddressPorts(in.GrantOptionSet.AuthCodeBrowserOption)
+	if bindPorts != nil {
+		key := fmt.Sprintf("get-token-%s", strings.Join(bindPorts, "-"))
+		u.Logger.V(1).Infof("acquiring a lock %s", key)
+		lock, err := u.Mutex.Acquire(ctx, key)
+		if err != nil {
+			return fmt.Errorf("could not acquire a lock: %w", err)
+		}
+		defer func() {
+			if err := u.Mutex.Release(lock); err != nil {
+				u.Logger.V(1).Infof("could not release the lock: %s", err)
+			}
+		}()
 	}
-	defer func() {
-		_ = u.Mutex.Release(lock)
-	}()
 
 	u.Logger.V(1).Infof("finding a token from cache directory %s", in.TokenCacheDir)
 	tokenCacheKey := tokencache.Key{
@@ -112,3 +121,21 @@ func (u *GetToken) Do(ctx context.Context, in Input) error {
 	}
 	return nil
 }
+
+func extractBindAddressPorts(o *authcode.BrowserOption) []string {
+	if o == nil {
+		return nil
+	}
+	var ports []string
+	for _, addr := range o.BindAddress {
+		_, port, err := net.SplitHostPort(addr)
+		if err != nil {
+			return nil // invalid address
+		}
+		if port == "0" {
+			return nil // any port
+		}
+		ports = append(ports, port)
+	}
+	return ports
+}

pkg/usecases/credentialplugin/get_token_test.go

@@ -45,7 +45,7 @@ func TestGetToken_Do(t *testing.T) {
 	}
 	grantOptionSet := authentication.GrantOptionSet{
 		AuthCodeBrowserOption: &authcode.BrowserOption{
-			BindAddress: []string{"127.0.0.1:8080"},
+			BindAddress: []string{"127.0.0.1:0"},
 		},
 	}
 
@@ -82,7 +82,60 @@ func TestGetToken_Do(t *testing.T) {
 			Authentication:       mockAuthentication,
 			TokenCacheRepository: mockRepository,
 			Writer:               mockWriter,
-			Mutex:                setupMutexMock(ctrl),
+			Mutex:                mock_mutex.NewMockInterface(ctrl),
+			Logger:               logger.New(t),
+		}
+		if err := u.Do(ctx, in); err != nil {
+			t.Errorf("Do returned error: %+v", err)
+		}
+	})
+
+	t.Run("NeedBindPortMutex", func(t *testing.T) {
+		grantOptionSet := authentication.GrantOptionSet{
+			AuthCodeBrowserOption: &authcode.BrowserOption{
+				BindAddress: []string{"127.0.0.1:8080"},
+			},
+		}
+		tokenCacheKey := tokencache.Key{
+			IssuerURL:    "https://accounts.google.com",
+			ClientID:     "YOUR_CLIENT_ID",
+			ClientSecret: "YOUR_CLIENT_SECRET",
+		}
+
+		ctrl := gomock.NewController(t)
+		defer ctrl.Finish()
+		ctx := context.TODO()
+		in := Input{
+			Provider:       dummyProvider,
+			TokenCacheDir:  "/path/to/token-cache",
+			GrantOptionSet: grantOptionSet,
+		}
+		mockAuthentication := mock_authentication.NewMockInterface(ctrl)
+		mockAuthentication.EXPECT().
+			Do(ctx, authentication.Input{
+				Provider:       dummyProvider,
+				GrantOptionSet: grantOptionSet,
+			}).
+			Return(&authentication.Output{TokenSet: issuedTokenSet}, nil)
+		mockRepository := mock_repository.NewMockInterface(ctrl)
+		mockRepository.EXPECT().
+			FindByKey("/path/to/token-cache", tokenCacheKey).
+			Return(nil, errors.New("file not found"))
+		mockRepository.EXPECT().
+			Save("/path/to/token-cache", tokenCacheKey, issuedTokenSet)
+		mockWriter := mock_writer.NewMockInterface(ctrl)
+		mockWriter.EXPECT().Write(issuedOutput)
+		mockMutex := mock_mutex.NewMockInterface(ctrl)
+		mockMutex.EXPECT().
+			Acquire(ctx, "get-token-8080").
+			Return(&mutex.Lock{Data: "testData"}, nil)
+		mockMutex.EXPECT().
+			Release(&mutex.Lock{Data: "testData"})
+		u := GetToken{
+			Authentication:       mockAuthentication,
+			TokenCacheRepository: mockRepository,
+			Writer:               mockWriter,
+			Mutex:                mockMutex,
 			Logger:               logger.New(t),
 		}
 		if err := u.Do(ctx, in); err != nil {
@@ -128,7 +181,7 @@ func TestGetToken_Do(t *testing.T) {
 			Authentication:       mockAuthentication,
 			TokenCacheRepository: mockRepository,
 			Writer:               mockWriter,
-			Mutex:                setupMutexMock(ctrl),
+			Mutex:                mock_mutex.NewMockInterface(ctrl),
 			Logger:               logger.New(t),
 		}
 		if err := u.Do(ctx, in); err != nil {
@@ -170,7 +223,7 @@ func TestGetToken_Do(t *testing.T) {
 			Authentication:       mockAuthentication,
 			TokenCacheRepository: mockRepository,
 			Writer:               mockWriter,
-			Mutex:                setupMutexMock(ctrl),
+			Mutex:                mock_mutex.NewMockInterface(ctrl),
 			Logger:               logger.New(t),
 		}
 		if err := u.Do(ctx, in); err != nil {
@@ -206,20 +259,11 @@ func TestGetToken_Do(t *testing.T) {
 			Authentication:       mockAuthentication,
 			TokenCacheRepository: mockRepository,
 			Writer:               mock_writer.NewMockInterface(ctrl),
-			Mutex:                setupMutexMock(ctrl),
+			Mutex:                mock_mutex.NewMockInterface(ctrl),
 			Logger:               logger.New(t),
 		}
 		if err := u.Do(ctx, in); err == nil {
 			t.Errorf("err wants non-nil but nil")
 		}
 	})
 }
-
-// Setup a mock that expect the mutex to be lock and unlock
-func setupMutexMock(ctrl *gomock.Controller) *mock_mutex.MockInterface {
-	mockMutex := mock_mutex.NewMockInterface(ctrl)
-	lockValue := &mutex.Lock{Data: "testData"}
-	acquireCall := mockMutex.EXPECT().Acquire(gomock.Not(gomock.Nil()), "get-token").Return(lockValue, nil)
-	mockMutex.EXPECT().Release(lockValue).Return(nil).After(acquireCall)
-	return mockMutex
-}