From 1a9403240861d97906558ba20f33aebd2a9ab856 Mon Sep 17 00:00:00 2001 From: KangManJoo Date: Thu, 8 Aug 2024 13:32:37 +0900 Subject: [PATCH] chore: improve error logs (#10592) (#19260) * chore: wrapped all errors and add context in account.go Signed-off-by: KangManJoo * chore: rollback error msg Signed-off-by: KangManJoo * chore: To align the test code error messages with the actual error message returned. Signed-off-by: KangManJoo --------- Signed-off-by: KangManJoo --- server/account/account.go | 32 ++++++++++++++++---------------- server/account/account_test.go | 3 ++- 2 files changed, 18 insertions(+), 17 deletions(-) diff --git a/server/account/account.go b/server/account/account.go index 8c499c7da2707..541401a731022 100644 --- a/server/account/account.go +++ b/server/account/account.go @@ -48,7 +48,7 @@ func (s *Server) UpdatePassword(ctx context.Context, q *account.UpdatePasswordRe // assuming user is trying to update someone else if username is different or issuer is not Argo CD if updatedUsername != username || issuer != session.SessionManagerClaimsIssuer { if err := s.enf.EnforceErr(ctx.Value("claims"), rbacpolicy.ResourceAccounts, rbacpolicy.ActionUpdate, q.Name); err != nil { - return nil, err + return nil, fmt.Errorf("permission denied: %w", err) } } @@ -70,7 +70,7 @@ func (s *Server) UpdatePassword(ctx context.Context, q *account.UpdatePasswordRe iat, err := session.Iat(ctx) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to get issue time: %w", err) } if time.Since(iat) > common.ChangePasswordSSOTokenMaxAge { return nil, errors.New("SSO token is too old. Please use 'argocd relogin' to get a new token.") @@ -80,12 +80,12 @@ func (s *Server) UpdatePassword(ctx context.Context, q *account.UpdatePasswordRe // Need to validate password complexity with regular expression passwordPattern, err := s.settingsMgr.GetPasswordPattern() if err != nil { - return nil, err + return nil, fmt.Errorf("failed to get password pattern: %w", err) } validPasswordRegexp, err := regexp.Compile(passwordPattern) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to compile password regex: %w", err) } if !validPasswordRegexp.Match([]byte(q.NewPassword)) { @@ -95,7 +95,7 @@ func (s *Server) UpdatePassword(ctx context.Context, q *account.UpdatePasswordRe hashedPassword, err := password.HashPassword(q.NewPassword) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to hash password: %w", err) } err = s.settingsMgr.UpdateAccount(updatedUsername, func(acc *settings.Account) error { @@ -105,7 +105,7 @@ func (s *Server) UpdatePassword(ctx context.Context, q *account.UpdatePasswordRe return nil }) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to update account password: %w", err) } if updatedUsername == username { @@ -132,7 +132,7 @@ func (s *Server) CanI(ctx context.Context, r *account.CanIRequest) (*account.Can if r.Resource == "logs" { serverRBACLogEnforceEnable, err := s.settingsMgr.GetServerRBACLogEnforceEnable() if err != nil { - return nil, err + return nil, fmt.Errorf("failed to get server RBAC log enforcement setting: %w", err) } if !serverRBACLogEnforceEnable { @@ -174,7 +174,7 @@ func (s *Server) ensureHasAccountPermission(ctx context.Context, action string, return nil } if err := s.enf.EnforceErr(ctx.Value("claims"), rbacpolicy.ResourceAccounts, action, account); err != nil { - return err + return fmt.Errorf("permission denied for account %s with action %s: %w", account, action, err) } return nil } @@ -184,7 +184,7 @@ func (s *Server) ListAccounts(ctx context.Context, r *account.ListAccountRequest resp := account.AccountsList{} accounts, err := s.settingsMgr.GetAccounts() if err != nil { - return nil, err + return nil, fmt.Errorf("failed to get accounts: %w", err) } for name, a := range accounts { if err := s.ensureHasAccountPermission(ctx, rbacpolicy.ActionGet, name); err == nil { @@ -200,11 +200,11 @@ func (s *Server) ListAccounts(ctx context.Context, r *account.ListAccountRequest // GetAccount returns an account func (s *Server) GetAccount(ctx context.Context, r *account.GetAccountRequest) (*account.Account, error) { if err := s.ensureHasAccountPermission(ctx, rbacpolicy.ActionGet, r.Name); err != nil { - return nil, err + return nil, fmt.Errorf("permission denied to get account %s: %w", r.Name, err) } a, err := s.settingsMgr.GetAccount(r.Name) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to get account %s: %w", r.Name, err) } return toApiAccount(r.Name, *a), nil } @@ -212,14 +212,14 @@ func (s *Server) GetAccount(ctx context.Context, r *account.GetAccountRequest) ( // CreateToken creates a token func (s *Server) CreateToken(ctx context.Context, r *account.CreateTokenRequest) (*account.CreateTokenResponse, error) { if err := s.ensureHasAccountPermission(ctx, rbacpolicy.ActionUpdate, r.Name); err != nil { - return nil, err + return nil, fmt.Errorf("permission denied to create token for account %s: %w", r.Name, err) } id := r.Id if id == "" { uniqueId, err := uuid.NewRandom() if err != nil { - return nil, err + return nil, fmt.Errorf("failed to generate unique ID: %w", err) } id = uniqueId.String() } @@ -252,7 +252,7 @@ func (s *Server) CreateToken(ctx context.Context, r *account.CreateTokenRequest) return nil }) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to update account with new token: %w", err) } return &account.CreateTokenResponse{Token: tokenString}, nil } @@ -260,7 +260,7 @@ func (s *Server) CreateToken(ctx context.Context, r *account.CreateTokenRequest) // DeleteToken deletes a token func (s *Server) DeleteToken(ctx context.Context, r *account.DeleteTokenRequest) (*account.EmptyResponse, error) { if err := s.ensureHasAccountPermission(ctx, rbacpolicy.ActionUpdate, r.Name); err != nil { - return nil, err + return nil, fmt.Errorf("permission denied to delete account %s: %w", r.Name, err) } err := s.settingsMgr.UpdateAccount(r.Name, func(account *settings.Account) error { @@ -271,7 +271,7 @@ func (s *Server) DeleteToken(ctx context.Context, r *account.DeleteTokenRequest) return status.Errorf(codes.NotFound, "token with id '%s' does not exist", r.Id) }) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to delete account %s: %w", r.Name, err) } return &account.EmptyResponse{}, nil } diff --git a/server/account/account_test.go b/server/account/account_test.go index ca5571f117048..03290ad3692c4 100644 --- a/server/account/account_test.go +++ b/server/account/account_test.go @@ -292,7 +292,8 @@ func TestCreateToken_UserSpecifiedID(t *testing.T) { _, err = accountServer.CreateToken(ctx, &account.CreateTokenRequest{Name: "account1", Id: "test"}) require.Error(t, err) - assert.Contains(t, "account already has token with id 'test'", err.Error()) + assert.Contains(t, err.Error(), "failed to update account with new token:") + assert.Contains(t, err.Error(), "account already has token with id 'test'") } func TestDeleteToken_SuccessfullyRemoved(t *testing.T) {