Skip to content

Commit

Permalink
Refactor: fix error handling and improve stability (#163)
Browse files Browse the repository at this point in the history 
* Fix ReadPassword() does not respect argument

* Do not ignore error when context has been cancelled

* Use longer timeout to reveal concurrency design failure

* Refactor: use context.TODO in test
  • Loading branch information
@int128
int128 authored Oct 4, 2019
1 parent 7a0ca20 commit b5922f9
Show file tree
Hide file tree
Showing 4 changed files with 42 additions and 26 deletions.
16 changes: 8 additions & 8 deletions e2e_test/standalone_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ var (
// 4. Verify the kubeconfig.
//
func TestCmd_Run_Standalone(t *testing.T) {
timeout := 1 * time.Second
timeout := 5 * time.Second

type testParameter struct {
startServer func(t *testing.T, h http.Handler) (string, localserver.Shutdowner)
Expand All @@ -57,7 +57,7 @@ func TestCmd_Run_Standalone(t *testing.T) {
runTest := func(t *testing.T, p testParameter) {
t.Run("Defaults", func(t *testing.T) {
t.Parallel()
ctx, cancel := context.WithTimeout(context.Background(), timeout)
ctx, cancel := context.WithTimeout(context.TODO(), timeout)
defer cancel()
ctrl := gomock.NewController(t)
defer ctrl.Finish()
Expand Down Expand Up @@ -85,7 +85,7 @@ func TestCmd_Run_Standalone(t *testing.T) {

t.Run("ResourceOwnerPasswordCredentials", func(t *testing.T) {
t.Parallel()
ctx, cancel := context.WithTimeout(context.Background(), timeout)
ctx, cancel := context.WithTimeout(context.TODO(), timeout)
defer cancel()
ctrl := gomock.NewController(t)
defer ctrl.Finish()
Expand Down Expand Up @@ -116,7 +116,7 @@ func TestCmd_Run_Standalone(t *testing.T) {

t.Run("HasValidToken", func(t *testing.T) {
t.Parallel()
ctx, cancel := context.WithTimeout(context.Background(), timeout)
ctx, cancel := context.WithTimeout(context.TODO(), timeout)
defer cancel()
ctrl := gomock.NewController(t)
defer ctrl.Finish()
Expand Down Expand Up @@ -145,7 +145,7 @@ func TestCmd_Run_Standalone(t *testing.T) {

t.Run("HasValidRefreshToken", func(t *testing.T) {
t.Parallel()
ctx, cancel := context.WithTimeout(context.Background(), timeout)
ctx, cancel := context.WithTimeout(context.TODO(), timeout)
defer cancel()
ctrl := gomock.NewController(t)
defer ctrl.Finish()
Expand Down Expand Up @@ -178,7 +178,7 @@ func TestCmd_Run_Standalone(t *testing.T) {

t.Run("HasExpiredRefreshToken", func(t *testing.T) {
t.Parallel()
ctx, cancel := context.WithTimeout(context.Background(), timeout)
ctx, cancel := context.WithTimeout(context.TODO(), timeout)
defer cancel()
ctrl := gomock.NewController(t)
defer ctrl.Finish()
Expand Down Expand Up @@ -218,7 +218,7 @@ func TestCmd_Run_Standalone(t *testing.T) {

t.Run("env:KUBECONFIG", func(t *testing.T) {
t.Parallel()
ctx, cancel := context.WithTimeout(context.Background(), timeout)
ctx, cancel := context.WithTimeout(context.TODO(), timeout)
defer cancel()
ctrl := gomock.NewController(t)
defer ctrl.Finish()
Expand All @@ -245,7 +245,7 @@ func TestCmd_Run_Standalone(t *testing.T) {

t.Run("ExtraScopes", func(t *testing.T) {
t.Parallel()
ctx, cancel := context.WithTimeout(context.Background(), timeout)
ctx, cancel := context.WithTimeout(context.TODO(), timeout)
defer cancel()
ctrl := gomock.NewController(t)
defer ctrl.Finish()
Expand Down
2 changes: 1 addition & 1 deletion pkg/adaptors/env/env.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ type Env struct{}

// ReadPassword reads a password from the stdin without echo back.
func (*Env) ReadPassword(prompt string) (string, error) {
if _, err := fmt.Fprint(os.Stderr, "Password: "); err != nil {
if _, err := fmt.Fprint(os.Stderr, prompt); err != nil {
return "", xerrors.Errorf("could not write the prompt: %w", err)
}
b, err := terminal.ReadPassword(int(syscall.Stdin))
Expand Down
6 changes: 3 additions & 3 deletions pkg/usecases/auth/auth.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -133,8 +133,9 @@ func (u *Authentication) Do(ctx context.Context, in Input) (*Output, error) {
func (u *Authentication) doAuthCodeFlow(ctx context.Context, in Input, client oidc.Interface) (*Output, error) {
u.Logger.V(1).Infof("performing the authentication code flow")
readyChan := make(chan string, 1)
defer close(readyChan)
var out Output
var eg errgroup.Group
eg, ctx := errgroup.WithContext(ctx)
eg.Go(func() error {
select {
case url, ok := <-readyChan:
Expand All @@ -153,11 +154,10 @@ func (u *Authentication) doAuthCodeFlow(ctx context.Context, in Input, client oi
}
return nil
case <-ctx.Done():
return nil
return xerrors.Errorf("context cancelled while waiting for the local server: %w", ctx.Err())
}
})
eg.Go(func() error {
defer close(readyChan)
tokenSet, err := client.AuthenticateByCode(ctx, in.ListenPort, readyChan)
if err != nil {
return xerrors.Errorf("error while the authorization code flow: %w", err)
Expand Down
44 changes: 30 additions & 14 deletions pkg/usecases/auth/auth_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,13 @@ func TestAuthentication_Do(t *testing.T) {
dummyTokenClaims := map[string]string{"sub": "YOUR_SUBJECT"}
pastTime := time.Now().Add(-time.Hour) //TODO: inject time service
futureTime := time.Now().Add(time.Hour) //TODO: inject time service
timeout := 5 * time.Second

t.Run("AuthorizationCodeFlow", func(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()
ctx := context.TODO()
ctx, cancel := context.WithTimeout(context.TODO(), timeout)
defer cancel()
in := Input{
ListenPort: []int{10000},
SkipOpenBrowser: true,
Expand All @@ -36,7 +38,10 @@ func TestAuthentication_Do(t *testing.T) {
}
mockOIDCClient := mock_oidc.NewMockInterface(ctrl)
mockOIDCClient.EXPECT().
AuthenticateByCode(ctx, []int{10000}, gomock.Any()).
AuthenticateByCode(gomock.Any(), []int{10000}, gomock.Any()).
Do(func(_ context.Context, _ []int, readyChan chan<- string) {
readyChan <- "LOCAL_SERVER_URL"
}).
Return(&oidc.TokenSet{
IDToken: "YOUR_ID_TOKEN",
RefreshToken: "YOUR_REFRESH_TOKEN",
Expand Down Expand Up @@ -75,7 +80,8 @@ func TestAuthentication_Do(t *testing.T) {
t.Run("AuthorizationCodeFlow/OpenBrowser", func(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()
ctx := context.TODO()
ctx, cancel := context.WithTimeout(context.TODO(), timeout)
defer cancel()
in := Input{
ListenPort: []int{10000},
OIDCConfig: kubeconfig.OIDCConfig{
Expand All @@ -85,7 +91,7 @@ func TestAuthentication_Do(t *testing.T) {
}
mockOIDCClient := mock_oidc.NewMockInterface(ctrl)
mockOIDCClient.EXPECT().
AuthenticateByCode(ctx, []int{10000}, gomock.Any()).
AuthenticateByCode(gomock.Any(), []int{10000}, gomock.Any()).
Do(func(_ context.Context, _ []int, readyChan chan<- string) {
readyChan <- "LOCAL_SERVER_URL"
}).
Expand Down Expand Up @@ -127,7 +133,8 @@ func TestAuthentication_Do(t *testing.T) {
t.Run("ResourceOwnerPasswordCredentialsFlow/UsePassword", func(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()
ctx := context.TODO()
ctx, cancel := context.WithTimeout(context.TODO(), timeout)
defer cancel()
in := Input{
Username: "USER",
Password: "PASS",
Expand All @@ -140,7 +147,7 @@ func TestAuthentication_Do(t *testing.T) {
}
mockOIDCClient := mock_oidc.NewMockInterface(ctrl)
mockOIDCClient.EXPECT().
AuthenticateByPassword(ctx, "USER", "PASS").
AuthenticateByPassword(gomock.Any(), "USER", "PASS").
Return(&oidc.TokenSet{
IDToken: "YOUR_ID_TOKEN",
RefreshToken: "YOUR_REFRESH_TOKEN",
Expand Down Expand Up @@ -179,7 +186,8 @@ func TestAuthentication_Do(t *testing.T) {
t.Run("ResourceOwnerPasswordCredentialsFlow/AskPassword", func(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()
ctx := context.TODO()
ctx, cancel := context.WithTimeout(context.TODO(), timeout)
defer cancel()
in := Input{
Username: "USER",
OIDCConfig: kubeconfig.OIDCConfig{
Expand All @@ -189,7 +197,7 @@ func TestAuthentication_Do(t *testing.T) {
}
mockOIDCClient := mock_oidc.NewMockInterface(ctrl)
mockOIDCClient.EXPECT().
AuthenticateByPassword(ctx, "USER", "PASS").
AuthenticateByPassword(gomock.Any(), "USER", "PASS").
Return(&oidc.TokenSet{
IDToken: "YOUR_ID_TOKEN",
RefreshToken: "YOUR_REFRESH_TOKEN",
Expand Down Expand Up @@ -229,7 +237,8 @@ func TestAuthentication_Do(t *testing.T) {
t.Run("ResourceOwnerPasswordCredentialsFlow/AskPasswordError", func(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()
ctx := context.TODO()
ctx, cancel := context.WithTimeout(context.TODO(), timeout)
defer cancel()
in := Input{
Username: "USER",
OIDCConfig: kubeconfig.OIDCConfig{
Expand Down Expand Up @@ -262,7 +271,8 @@ func TestAuthentication_Do(t *testing.T) {
t.Run("HasValidIDToken", func(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()
ctx := context.TODO()
ctx, cancel := context.WithTimeout(context.TODO(), timeout)
defer cancel()
in := Input{
OIDCConfig: kubeconfig.OIDCConfig{
ClientID: "YOUR_CLIENT_ID",
Expand Down Expand Up @@ -302,7 +312,8 @@ func TestAuthentication_Do(t *testing.T) {
t.Run("HasValidRefreshToken", func(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()
ctx := context.TODO()
ctx, cancel := context.WithTimeout(context.TODO(), timeout)
defer cancel()
in := Input{
OIDCConfig: kubeconfig.OIDCConfig{
ClientID: "YOUR_CLIENT_ID",
Expand Down Expand Up @@ -359,9 +370,11 @@ func TestAuthentication_Do(t *testing.T) {
t.Run("HasExpiredRefreshToken", func(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()
ctx := context.TODO()
ctx, cancel := context.WithTimeout(context.TODO(), timeout)
defer cancel()
in := Input{
ListenPort: []int{10000},
ListenPort: []int{10000},
SkipOpenBrowser: true,
OIDCConfig: kubeconfig.OIDCConfig{
ClientID: "YOUR_CLIENT_ID",
ClientSecret: "YOUR_CLIENT_SECRET",
Expand All @@ -382,7 +395,10 @@ func TestAuthentication_Do(t *testing.T) {
Refresh(ctx, "EXPIRED_REFRESH_TOKEN").
Return(nil, xerrors.New("token has expired"))
mockOIDCClient.EXPECT().
AuthenticateByCode(ctx, []int{10000}, gomock.Any()).
AuthenticateByCode(gomock.Any(), []int{10000}, gomock.Any()).
Do(func(_ context.Context, _ []int, readyChan chan<- string) {
readyChan <- "LOCAL_SERVER_URL"
}).
Return(&oidc.TokenSet{
IDToken: "NEW_ID_TOKEN",
RefreshToken: "NEW_REFRESH_TOKEN",
Expand Down

0 comments on commit b5922f9

Please sign in to comment.