From 760f700c0e18795ccb6d39126ecaefb107cb9edd Mon Sep 17 00:00:00 2001 From: f1v3-dev Date: Fri, 4 Sep 2026 18:09:34 +0900 Subject: [PATCH] REFACTOR: Enhance password validation --- CLAUDE.md | 6 +- cmd/acl/admin/passwd.go | 32 ++++-- cmd/acl/group/add.go | 32 ++++-- cmd/acl/user/add.go | 94 ++++++++++++----- cmd/acl/user/remove.go | 17 +-- internal/cluster/delete.go | 6 +- internal/cluster/deploy.go | 6 +- internal/password.go | 53 ++++++++++ internal/password_test.go | 72 +++++++++++++ internal/prompt.go | 73 +++++++++++++ internal/prompt_test.go | 91 ++++++++++++++++ internal/util.go | 157 ---------------------------- internal/util_test.go | 29 ------ internal/zk/delete.go | 6 +- internal/zk/deploy.go | 6 +- internal/zookeeper.go | 156 ++++++++++++++++++++++++++++ internal/zookeeper_test.go | 208 +++++++++++++++++++++++++++++++++++++ 17 files changed, 800 insertions(+), 244 deletions(-) create mode 100644 internal/password.go create mode 100644 internal/password_test.go create mode 100644 internal/prompt.go create mode 100644 internal/prompt_test.go delete mode 100644 internal/util.go delete mode 100644 internal/util_test.go create mode 100644 internal/zookeeper.go create mode 100644 internal/zookeeper_test.go diff --git a/CLAUDE.md b/CLAUDE.md index 399e4a9..c75d83c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -38,7 +38,9 @@ cmd/ cobra 커맨드 정의 — 얇게 유지. 인자 파싱과 internal/ 모든 실제 로직. cmd/에서 로직을 구현하지 않는다. config.go viper 설정 로드 (Flags, Config 전역) const.go ZooKeeper znode 경로 상수 - util.go ZK 커넥션/znode 헬퍼, 표준입력 읽기 + zookeeper.go ZooKeeper 커넥션/znode 헬퍼 + prompt.go 표준입력 및 확인 프롬프트 + password.go ACL 비밀번호 정책 검증 topology/ YAML 토폴로지 타입 + Validate() + Load*() store/ ~/.arcusctl 하위 배포 메타데이터 영속화 ssh/ ssh/scp 서브프로세스 실행 래퍼 @@ -86,7 +88,7 @@ internal/ 모든 실제 로직. cmd/에서 로직을 구현하지 않 - 사용자에게 보이는 에러 메시지는 **무엇이 / 왜 실패했고 / 어떻게 복구하는지**를 담는다. 원격 호스트가 관련되면 어느 호스트인지 반드시 포함한다. - **cobra 커맨드는 `RunE`로 error를 반환한다.** `cmd.Execute()`가 stderr 출력과 exit code 1을 담당한다. 커맨드 안에서 `panic()`이나 `os.Exit()`을 부르지 않는다. - > 현재 `cmd/` 일부와 `internal/config.go`, `internal/util.go`에 `panic(err)`이 + > 현재 `cmd/` 일부와 `internal/config.go`에 `panic(err)`이 > 남아 있다. 이것은 **정리 대상인 과거 패턴**이며 새 코드가 따라갈 기준이 아니다. > 건드리는 김에 점진적으로 `RunE` + error 반환으로 옮긴다. diff --git a/cmd/acl/admin/passwd.go b/cmd/acl/admin/passwd.go index f30ee03..14c54c8 100644 --- a/cmd/acl/admin/passwd.go +++ b/cmd/acl/admin/passwd.go @@ -22,34 +22,48 @@ For password requirements, see: https://github.com/jam2in/arcusctl/blob/main/doc Example: ` # Change admin password for group 'cache01' arcusctl acl admin passwd cache01`, Args: cobra.ExactArgs(1), - Run: func(cmd *cobra.Command, args []string) { + RunE: func(cmd *cobra.Command, args []string) error { groupName := args[0] - adminName := internal.ReadStdin("admin name", false) - adminPassword := internal.ReadStdin("admin password", true) - newPassword := internal.ReadStdin("new password", true) - if newPassword != internal.ReadStdin("repeat new password", true) { - panic("password does not match") + adminName, err := internal.ReadInput("admin name") + if err != nil { + return fmt.Errorf("read admin name: %w", err) + } + adminPassword, err := internal.ReadPassword("admin password") + if err != nil { + return err + } + newPassword, err := internal.ReadPassword("new password") + if err != nil { + return err + } + repeatedPassword, err := internal.ReadPassword("repeat new password") + if err != nil { + return err + } + if newPassword != repeatedPassword { + return errors.New("password does not match") } conn, err := internal.ConnectZooKeeper(internal.Config.ZooKeeper) if err != nil { - panic(err) + return err } defer conn.Close() if err := conn.AddAuth("digest", []byte(adminName+":"+adminPassword)); err != nil { - panic(err) + return fmt.Errorf("authenticate ZooKeeper admin %q: %w", adminName, err) } acls := append(zk.DigestACL(zk.PermAll, adminName, newPassword), zk.WorldACL(zk.PermRead)...) if err := setAclRecursive(conn, internal.ZPATH_ACL_ROOT+"/"+groupName, acls); err != nil { - panic(err) + return err } fmt.Println("OK") + return nil }, } diff --git a/cmd/acl/group/add.go b/cmd/acl/group/add.go index 2dac9d6..1c855f5 100644 --- a/cmd/acl/group/add.go +++ b/cmd/acl/group/add.go @@ -1,6 +1,7 @@ package group import ( + "errors" "fmt" "github.com/go-zookeeper/zk" @@ -21,32 +22,45 @@ For password requirements, see: https://github.com/jam2in/arcusctl/blob/main/doc Example: ` # Create a new group named 'cache01' arcusctl acl group add cache01`, Args: cobra.ExactArgs(1), - Run: func(cmd *cobra.Command, args []string) { + RunE: func(cmd *cobra.Command, args []string) error { groupName := args[0] - adminName := internal.ReadStdin("admin name", false) - adminPassword := internal.ReadStdin("password", true) - if adminPassword != internal.ReadStdin("repeat password", true) { - panic("password does not match") + adminName, err := internal.ReadInput("admin name") + if err != nil { + return fmt.Errorf("read admin name: %w", err) + } + adminPassword, err := internal.ReadPassword("password") + if err != nil { + return err + } + repeatedPassword, err := internal.ReadPassword("repeat password") + if err != nil { + return err + } + if adminPassword != repeatedPassword { + return errors.New("password does not match") } conn, err := internal.ConnectZooKeeper(internal.Config.ZooKeeper) if err != nil { - panic(err) + return err } defer conn.Close() if err := conn.AddAuth("digest", []byte(adminName+":"+adminPassword)); err != nil { - panic(err) + return fmt.Errorf("authenticate ZooKeeper admin %q: %w", adminName, err) } - internal.EnsureZPath(conn, internal.ZPATH_ACL_ROOT) + if err := internal.EnsureZNode(conn, internal.ZPATH_ACL_ROOT); err != nil { + return err + } if _, err := conn.Create(internal.ZPATH_ACL_ROOT+"/"+groupName, nil, 0, append(zk.DigestACL(zk.PermAll, adminName, adminPassword), zk.WorldACL(zk.PermRead)...)); err != nil { - panic(err) + return fmt.Errorf("create ACL group %q: %w", groupName, err) } fmt.Println("OK") + return nil }, } diff --git a/cmd/acl/user/add.go b/cmd/acl/user/add.go index f67bb08..08f3c7c 100644 --- a/cmd/acl/user/add.go +++ b/cmd/acl/user/add.go @@ -1,6 +1,7 @@ package user import ( + "errors" "fmt" "strings" @@ -25,7 +26,7 @@ For password requirements, see: https://github.com/jam2in/arcusctl/blob/main/doc # Add a user 'john' for operator with logging enabled arcusctl acl user add cache01 john attr,scan,flush,admin logAll`, Args: cobra.RangeArgs(3, 4), - Run: func(cmd *cobra.Command, args []string) { + RunE: func(cmd *cobra.Command, args []string) error { groupName := args[0] userName := args[1] permissions := args[2] @@ -33,27 +34,29 @@ For password requirements, see: https://github.com/jam2in/arcusctl/blob/main/doc if args[3] == "logAll" { permissions += ",logall" } else { - panic("invalid arguments") + return fmt.Errorf("invalid fourth argument %q: expected logAll", args[3]) } } - adminName := internal.ReadStdin("admin name", false) - adminPassword := internal.ReadStdin("admin password", true) - userPassword := internal.ReadStdin("user password", true) - if userPassword != internal.ReadStdin("repeat user password", true) { - panic("password does not match") + adminName, adminPassword, err := readAdminCredentials() + if err != nil { + return err + } + userPassword, err := readConfirmedPassword("user password", "repeat user password") + if err != nil { + return err } secret := scram.GenerateScramSHA256Secret(userPassword, nil, 0) conn, err := internal.ConnectZooKeeper(internal.Config.ZooKeeper) if err != nil { - panic(err) + return err } defer conn.Close() if err := conn.AddAuth("digest", []byte(adminName+":"+adminPassword)); err != nil { - panic(err) + return fmt.Errorf("authenticate ZooKeeper admin %q: %w", adminName, err) } acls := append(zk.DigestACL(zk.PermAll, adminName, adminPassword), @@ -73,10 +76,11 @@ For password requirements, see: https://github.com/jam2in/arcusctl/blob/main/doc Flags: 0, }, ); err != nil { - panic(err) + return fmt.Errorf("create ACL user %q in group %q: %w", userName, groupName, err) } fmt.Println("OK") + return nil }, } @@ -89,35 +93,38 @@ For password requirements, see: https://github.com/jam2in/arcusctl/blob/main/doc Example: ` # Change password for user 'john' in group 'cache01' arcusctl acl user passwd cache01 john`, Args: cobra.ExactArgs(2), - Run: func(cmd *cobra.Command, args []string) { + RunE: func(cmd *cobra.Command, args []string) error { groupName := args[0] userName := args[1] - adminName := internal.ReadStdin("admin name", false) - adminPassword := internal.ReadStdin("admin password", true) - userPassword := internal.ReadStdin("user password", true) - if userPassword != internal.ReadStdin("repeat user password", true) { - panic("password does not match") + adminName, adminPassword, err := readAdminCredentials() + if err != nil { + return err + } + userPassword, err := readConfirmedPassword("user password", "repeat user password") + if err != nil { + return err } secret := scram.GenerateScramSHA256Secret(userPassword, nil, 0) conn, err := internal.ConnectZooKeeper(internal.Config.ZooKeeper) if err != nil { - panic(err) + return err } defer conn.Close() if err := conn.AddAuth("digest", []byte(adminName+":"+adminPassword)); err != nil { - panic(err) + return fmt.Errorf("authenticate ZooKeeper admin %q: %w", adminName, err) } if _, err := conn.Set(internal.ZPATH_ACL_ROOT+"/"+groupName+"/"+userName+"/"+propName, []byte(secret.EncodeToBase64()), -1); err != nil { - panic(err) + return fmt.Errorf("change password for ACL user %q in group %q: %w", userName, groupName, err) } fmt.Println("OK") + return nil }, } @@ -131,28 +138,30 @@ The logAll flag will be preserved from the user's existing configuration.`, Example: ` # Update user permissions to key-value only arcusctl acl user permissions cache01 john kv`, Args: cobra.ExactArgs(3), - Run: func(cmd *cobra.Command, args []string) { + RunE: func(cmd *cobra.Command, args []string) error { groupName := args[0] userName := args[1] permissions := args[2] - adminName := internal.ReadStdin("admin name", false) - adminPassword := internal.ReadStdin("admin password", true) + adminName, adminPassword, err := readAdminCredentials() + if err != nil { + return err + } conn, err := internal.ConnectZooKeeper(internal.Config.ZooKeeper) if err != nil { - panic(err) + return err } defer conn.Close() if err := conn.AddAuth("digest", []byte(adminName+":"+adminPassword)); err != nil { - panic(err) + return fmt.Errorf("authenticate ZooKeeper admin %q: %w", adminName, err) } - // It's pretty stupid + // Preserve the existing logall flag while replacing the other permissions. beforePerm, _, err := conn.Get(internal.ZPATH_ACL_ROOT + "/" + groupName + "/" + userName) if err != nil { - panic(err) + return fmt.Errorf("read permissions for ACL user %q in group %q: %w", userName, groupName, err) } beforePermList := strings.Split(string(beforePerm), ",") if beforePermList[len(beforePermList)-1] == "logall" { @@ -161,9 +170,40 @@ The logAll flag will be preserved from the user's existing configuration.`, if _, err := conn.Set(internal.ZPATH_ACL_ROOT+"/"+groupName+"/"+userName, []byte(permissions), -1); err != nil { - panic(err) + return fmt.Errorf("update permissions for ACL user %q in group %q: %w", userName, groupName, err) } fmt.Println("OK") + return nil }, } + +func readAdminCredentials() (string, string, error) { + name, err := internal.ReadInput("admin name") + if err != nil { + return "", "", fmt.Errorf("read admin name: %w", err) + } + + password, err := internal.ReadPassword("admin password") + if err != nil { + return "", "", err + } + + return name, password, nil +} + +func readConfirmedPassword(prompt string, repeatPrompt string) (string, error) { + password, err := internal.ReadPassword(prompt) + if err != nil { + return "", err + } + repeatedPassword, err := internal.ReadPassword(repeatPrompt) + if err != nil { + return "", err + } + if password != repeatedPassword { + return "", errors.New("password does not match") + } + + return password, nil +} diff --git a/cmd/acl/user/remove.go b/cmd/acl/user/remove.go index 5cb7d50..bb252ca 100644 --- a/cmd/acl/user/remove.go +++ b/cmd/acl/user/remove.go @@ -20,25 +20,27 @@ and will fail if active connections are found.`, Example: ` # Remove user 'john' from group 'cache01' arcusctl acl user remove cache01 john`, Args: cobra.ExactArgs(2), - Run: func(cmd *cobra.Command, args []string) { + RunE: func(cmd *cobra.Command, args []string) error { groupName := args[0] userName := args[1] - adminName := internal.ReadStdin("admin name", false) - adminPassword := internal.ReadStdin("admin password", true) + adminName, adminPassword, err := readAdminCredentials() + if err != nil { + return err + } conn, err := internal.ConnectZooKeeper(internal.Config.ZooKeeper) if err != nil { - panic(err) + return err } defer conn.Close() if err := conn.AddAuth("digest", []byte(adminName+":"+adminPassword)); err != nil { - panic(err) + return fmt.Errorf("authenticate ZooKeeper admin %q: %w", adminName, err) } if err := CheckClientList(conn, groupName, userName); err != nil { - panic(err) + return err } if _, err := conn.Multi( @@ -51,10 +53,11 @@ and will fail if active connections are found.`, Version: -1, }, ); err != nil { - panic(err) + return fmt.Errorf("remove ACL user %q from group %q: %w", userName, groupName, err) } fmt.Println("OK") + return nil }, } diff --git a/internal/cluster/delete.go b/internal/cluster/delete.go index 525c0bf..2a1bf7e 100644 --- a/internal/cluster/delete.go +++ b/internal/cluster/delete.go @@ -22,7 +22,11 @@ func Delete(serviceCode string, purge bool) error { } fmt.Printf("This will remove Arcus cluster %q from all servers.\n", serviceCode) - if !internal.Confirm("Are you sure you want to proceed? (y/N): ") { + confirmed, err := internal.Confirm("Are you sure you want to proceed? (y/N): ") + if err != nil { + return fmt.Errorf("confirm Arcus cluster deletion: %w", err) + } + if !confirmed { fmt.Println("Aborted.") return nil } diff --git a/internal/cluster/deploy.go b/internal/cluster/deploy.go index 48e979b..c559559 100644 --- a/internal/cluster/deploy.go +++ b/internal/cluster/deploy.go @@ -17,7 +17,11 @@ func Deploy(version string, topoPath string) error { } printPlan(topo, version, edition) - if !internal.Confirm("Proceed with deployment? (y/N): ") { + confirmed, err := internal.Confirm("Proceed with deployment? (y/N): ") + if err != nil { + return fmt.Errorf("confirm Arcus cluster deployment: %w", err) + } + if !confirmed { fmt.Println("Aborted.") return nil } diff --git a/internal/password.go b/internal/password.go new file mode 100644 index 0000000..75753a1 --- /dev/null +++ b/internal/password.go @@ -0,0 +1,53 @@ +package internal + +import ( + "fmt" + "strings" + "unicode" + "unicode/utf8" +) + +const ( + minPasswordLen = 12 + minPasswordClasses = 3 +) + +var passwordClasses = []struct { + name string + contains func(rune) bool +}{ + {"uppercase", unicode.IsUpper}, + {"lowercase", unicode.IsLower}, + {"digits", unicode.IsDigit}, + {"symbols", func(r rune) bool { return unicode.IsPunct(r) || unicode.IsSymbol(r) }}, +} + +func validatePassword(password string) error { + if length := utf8.RuneCountInString(password); length < minPasswordLen { + return fmt.Errorf( + "password is %d characters, must be at least %d", + length, + minPasswordLen, + ) + } + + missing := make([]string, 0, len(passwordClasses)) + found := 0 + for _, class := range passwordClasses { + if strings.ContainsFunc(password, class.contains) { + found++ + continue + } + missing = append(missing, class.name) + } + if found < minPasswordClasses { + return fmt.Errorf( + "password has %d character classes, must include at least %d (missing: %s)", + found, + minPasswordClasses, + strings.Join(missing, ", "), + ) + } + + return nil +} diff --git a/internal/password_test.go b/internal/password_test.go new file mode 100644 index 0000000..655ecc8 --- /dev/null +++ b/internal/password_test.go @@ -0,0 +1,72 @@ +package internal + +import ( + "strings" + "testing" + "unicode/utf8" +) + +func TestValidatePassword(t *testing.T) { + testcases := []struct { + name string + src string + wantErr bool + }{ + {name: "all four classes", src: "Abcd1234!@#$", wantErr: false}, + {name: "upper lower digit", src: "Abcdefgh1234", wantErr: false}, + {name: "lower digit symbol", src: "abcd1234!@#$", wantErr: false}, + {name: "upper digit symbol", src: "ABCD1234!@#$", wantErr: false}, + {name: "upper lower symbol", src: "Abcdefgh!@#$", wantErr: false}, + + {name: "empty", src: "", wantErr: true}, + {name: "eleven characters", src: "Abcd1234!@#", wantErr: true}, + {name: "upper lower only", src: "Abcdefghijkl", wantErr: true}, + {name: "lower digit only", src: "abcdefgh1234", wantErr: true}, + {name: "upper digit only", src: "ABCDEFGH1234", wantErr: true}, + {name: "lower symbol only", src: "abcdefgh!@#$", wantErr: true}, + + // 길이는 룬 개수로 센다. 아래 두 값은 15바이트지만 7자다. + {name: "multibyte shorter than min", src: "비밀번호Aa1", wantErr: true}, + {name: "multibyte shorter than min 2", src: "가나다라Xy9", wantErr: true}, + // 12자를 넘기면 멀티바이트 문자가 섞여 있어도 통과한다. + {name: "multibyte long enough", src: "비밀번호Abcd1234", wantErr: false}, + } + + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + err := validatePassword(tc.src) + if (err != nil) != tc.wantErr { + t.Fatalf("validatePassword(%q) = %v, wantErr %v", tc.src, err, tc.wantErr) + } + }) + } +} + +func TestValidatePasswordErrorMessage(t *testing.T) { + t.Run("reports rune count, not byte count", func(t *testing.T) { + const src = "비밀번호Aa1" // 15 bytes, 7 runes + if got, want := utf8.RuneCountInString(src), 7; got != want { + t.Fatalf("test fixture changed: %q has %d runes, want %d", src, got, want) + } + + err := validatePassword(src) + if err == nil { + t.Fatal("validatePassword() = nil, want error") + } + if !strings.Contains(err.Error(), "is 7 characters") { + t.Errorf("error should report the rune count: %v", err) + } + }) + + t.Run("names the missing classes", func(t *testing.T) { + err := validatePassword("abcdefghijkl") + if err == nil { + t.Fatal("validatePassword() = nil, want error") + } + for _, want := range []string{"uppercase", "digits", "symbols"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error should name missing class %q: %v", want, err) + } + } + }) +} diff --git a/internal/prompt.go b/internal/prompt.go new file mode 100644 index 0000000..afb6a2e --- /dev/null +++ b/internal/prompt.go @@ -0,0 +1,73 @@ +package internal + +import ( + "bufio" + "errors" + "fmt" + "io" + "os" + "strings" + + "golang.org/x/term" +) + +var stdinReader = bufio.NewReader(os.Stdin) + +func ReadInput(prompt string) (string, error) { + return readInput(stdinReader, os.Stdout, prompt) +} + +func readInput(reader *bufio.Reader, writer io.Writer, prompt string) (string, error) { + input, err := readLine(reader, writer, prompt+": ") + if err != nil { + return "", fmt.Errorf("read input: %w", err) + } + if input == "" { + return "", errors.New("read input: value must not be empty") + } + return input, nil +} + +func ReadPassword(prompt string) (string, error) { + if _, err := fmt.Fprint(os.Stdout, prompt+": "); err != nil { + return "", fmt.Errorf("write password prompt: %w", err) + } + + raw, err := term.ReadPassword(int(os.Stdin.Fd())) + if err != nil { + return "", fmt.Errorf("read password: %w", err) + } + if _, err := fmt.Fprintln(os.Stdout); err != nil { + return "", fmt.Errorf("write password prompt: %w", err) + } + + password := string(raw) + if err := validatePassword(password); err != nil { + return "", fmt.Errorf("invalid password: %w", err) + } + return password, nil +} + +func Confirm(prompt string) (bool, error) { + return confirm(stdinReader, os.Stdout, prompt) +} + +func confirm(reader *bufio.Reader, writer io.Writer, prompt string) (bool, error) { + input, err := readLine(reader, writer, prompt) + if err != nil { + return false, fmt.Errorf("read confirmation: %w", err) + } + return strings.EqualFold(input, "y") || strings.EqualFold(input, "yes"), nil +} + +func readLine(reader *bufio.Reader, writer io.Writer, prompt string) (string, error) { + if _, err := fmt.Fprint(writer, prompt); err != nil { + return "", fmt.Errorf("write prompt: %w", err) + } + + line, err := reader.ReadString('\n') + if err != nil && (!errors.Is(err, io.EOF) || len(line) == 0) { + return "", err + } + return strings.TrimSpace(line), nil +} diff --git a/internal/prompt_test.go b/internal/prompt_test.go new file mode 100644 index 0000000..127e2c0 --- /dev/null +++ b/internal/prompt_test.go @@ -0,0 +1,91 @@ +package internal + +import ( + "bufio" + "errors" + "strings" + "testing" +) + +func TestConfirm(t *testing.T) { + testcases := []struct { + name string + input string + want bool + wantErr bool + }{ + {name: "short yes", input: "y\n", want: true}, + {name: "case insensitive yes", input: " YES \n", want: true}, + {name: "no", input: "n\n", want: false}, + {name: "anything else", input: "sure\n", want: false}, + {name: "yes ending in EOF", input: "yes", want: true}, + {name: "empty EOF", input: "", wantErr: true}, + } + + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + var output strings.Builder + got, err := confirm(bufio.NewReader(strings.NewReader(tc.input)), &output, "Continue? ") + if (err != nil) != tc.wantErr { + t.Fatalf("confirm() error = %v, wantErr %v", err, tc.wantErr) + } + if got != tc.want { + t.Errorf("confirm() = %v, want %v", got, tc.want) + } + if gotPrompt := output.String(); gotPrompt != "Continue? " { + t.Errorf("prompt = %q, want %q", gotPrompt, "Continue? ") + } + }) + } +} + +func TestReadInput(t *testing.T) { + t.Run("trims surrounding whitespace", func(t *testing.T) { + var output strings.Builder + got, err := readInput( + bufio.NewReader(strings.NewReader(" admin \n")), + &output, + "admin name", + ) + if err != nil { + t.Fatalf("readInput() error = %v", err) + } + if want := "admin"; got != want { + t.Errorf("readInput() = %q, want %q", got, want) + } + if want := "admin name: "; output.String() != want { + t.Errorf("prompt = %q, want %q", output.String(), want) + } + }) + + t.Run("rejects empty input", func(t *testing.T) { + _, err := readInput( + bufio.NewReader(strings.NewReader(" \n")), + &strings.Builder{}, + "admin name", + ) + if err == nil { + t.Fatal("readInput() error = nil, want error") + } + }) +} + +func TestReadLineReturnsPromptWriteError(t *testing.T) { + wantErr := errors.New("write failed") + _, err := readLine( + bufio.NewReader(strings.NewReader("yes\n")), + errorWriter{err: wantErr}, + "Continue? ", + ) + if !errors.Is(err, wantErr) { + t.Fatalf("readLine() error = %v, want wrapped %v", err, wantErr) + } +} + +type errorWriter struct { + err error +} + +func (w errorWriter) Write([]byte) (int, error) { + return 0, w.err +} diff --git a/internal/util.go b/internal/util.go deleted file mode 100644 index 8b42275..0000000 --- a/internal/util.go +++ /dev/null @@ -1,157 +0,0 @@ -package internal - -import ( - "bufio" - "fmt" - "log" - "os" - "strings" - "syscall" - "time" - "unicode" - - "github.com/go-zookeeper/zk" - "golang.org/x/term" -) - -func ConnectZooKeeper(addr string) (*zk.Conn, error) { - conn, _, err := zk.Connect(strings.Split(addr, ","), time.Second, - zk.WithLogInfo(Flags.Verbose)) - return conn, err -} - -// FXIME: EnsureZPath와 통합 가능 여부 검토 필요 -func EnsureZNode(conn *zk.Conn, zpath string) error { - if zpath == "" || zpath == "/" { - return nil - } - - exists, _, err := conn.Exists(zpath) - if err != nil { - return err - } - if exists { - return nil - } - - parent := zpath[:strings.LastIndex(zpath, "/")] - if parent != "" { - if err := EnsureZNode(conn, parent); err != nil { - return err - } - } - - _, err = conn.Create(zpath, []byte{}, 0, zk.WorldACL(zk.PermAll)) - if err == zk.ErrNodeExists { - return nil - } - return err -} - -func EnsureZPath(conn *zk.Conn, zpath string) { - if zpath == "" || zpath == "/" { - return - } - - exists, _, err := conn.Exists(zpath) - if err != nil { - panic(err) - } - if exists { - if Flags.Verbose { - log.Printf("%s exists\n", zpath) - } - return - } - - parent := zpath[:strings.LastIndex(zpath, "/")] - if parent != "" { - EnsureZPath(conn, zpath) - } - - if _, err := conn.Create(zpath, []byte{}, 0, zk.WorldACL(zk.PermAll)); err != nil { - panic(err) - } - - if Flags.Verbose { - log.Printf("%s created\n", zpath) - } -} - -func isValidPassword(p string) bool { - if len(p) < 12 { - return false - } - - var hasUpper, hasLower, hasDigit, hasSpecial int - for _, c := range p { - if unicode.IsUpper(c) { - hasUpper = 1 - } else if unicode.IsLower(c) { - hasLower = 1 - } else if unicode.IsDigit(c) { - hasDigit = 1 - } else if unicode.IsPunct(c) || unicode.IsSymbol(c) { - hasSpecial = 1 - } - } - - return hasUpper+hasLower+hasDigit+hasSpecial >= 3 -} - -func ReadStdin(msg string, isPassword bool) string { - fmt.Print(msg + ": ") - if isPassword { - raw, err := term.ReadPassword(syscall.Stdin) - if err != nil { - panic(err) - } - pw := string(raw) - if !isValidPassword(pw) { - panic("Password must be 12+ characters and include 3+ of: uppercase, lowercase, digits, symbols") - } - fmt.Println() - return pw - } else { - var input string - if _, err := fmt.Scanln(&input); err != nil { - panic(err) - } - return input - } -} - -// FIXME: ReadStdin와 통합 가능 여부 검토 필요 -func Confirm(prompt string) bool { - fmt.Print(prompt) - reader := bufio.NewReader(os.Stdin) - input, err := reader.ReadString('\n') - if err != nil { - return false - } - - input = strings.TrimSpace(strings.ToLower(input)) - return input == "y" || input == "yes" -} - -func DeleteZNode(conn *zk.Conn, zpath string) error { - children, _, err := conn.Children(zpath) - if err == zk.ErrNoNode { - return nil - } - if err != nil { - return err - } - - for _, child := range children { - if err := DeleteZNode(conn, zpath+"/"+child); err != nil { - return err - } - } - - err = conn.Delete(zpath, -1) - if err == zk.ErrNoNode { - return nil - } - return err -} diff --git a/internal/util_test.go b/internal/util_test.go deleted file mode 100644 index 99326c6..0000000 --- a/internal/util_test.go +++ /dev/null @@ -1,29 +0,0 @@ -package internal - -import "testing" - -func TestIsValidPassword(t *testing.T) { - testcases := []struct { - src string - want bool - }{ - {src: "Abcd1234!@#$", want: true}, - {src: "Abcdefgh1234", want: true}, - {src: "abcd1234!@#$", want: true}, - {src: "ABCD1234!@#$", want: true}, - {src: "Abcdefgh!@#$", want: true}, - {src: "", want: false}, - {src: "Abcd1234!@#", want: false}, - {src: "Abcdefghijkl", want: false}, - {src: "abcdefgh1234", want: false}, - {src: "ABCDEFGH1234", want: false}, - {src: "abcdefgh!@#$", want: false}, - } - - for _, tc := range testcases { - got := isValidPassword(tc.src) - if got != tc.want { - t.Errorf("isValidPassword(%s) = %v, want %v", tc.src, got, tc.want) - } - } -} diff --git a/internal/zk/delete.go b/internal/zk/delete.go index b00e406..7653241 100644 --- a/internal/zk/delete.go +++ b/internal/zk/delete.go @@ -28,7 +28,11 @@ func Delete(ensembleName string, purge bool) error { } fmt.Printf("This will remove ZooKeeper ensemble %q from all servers.\n", ensembleName) - if !internal.Confirm("Are you sure you want to proceed? (y/N): ") { + confirmed, err := internal.Confirm("Are you sure you want to proceed? (y/N): ") + if err != nil { + return fmt.Errorf("confirm ZooKeeper deletion: %w", err) + } + if !confirmed { fmt.Println("Aborted.") return nil } diff --git a/internal/zk/deploy.go b/internal/zk/deploy.go index a7043e9..d22c1eb 100644 --- a/internal/zk/deploy.go +++ b/internal/zk/deploy.go @@ -17,7 +17,11 @@ func Deploy(version string, topoPath string) error { } printPlan(topo, version) - if !internal.Confirm("Proceed with deployment? (y/N): ") { + confirmed, err := internal.Confirm("Proceed with deployment? (y/N): ") + if err != nil { + return fmt.Errorf("confirm ZooKeeper deployment: %w", err) + } + if !confirmed { fmt.Println("Aborted.") return nil } diff --git a/internal/zookeeper.go b/internal/zookeeper.go new file mode 100644 index 0000000..7f5797e --- /dev/null +++ b/internal/zookeeper.go @@ -0,0 +1,156 @@ +package internal + +import ( + "errors" + "fmt" + "log" + "path" + "strings" + "time" + + "github.com/go-zookeeper/zk" +) + +const zooKeeperSessionTimeout = time.Second + +type znodeCreator interface { + Exists(path string) (bool, *zk.Stat, error) + Create(path string, data []byte, flags int32, acl []zk.ACL) (string, error) +} + +type znodeDeleter interface { + Children(path string) ([]string, *zk.Stat, error) + Delete(path string, version int32) error +} + +func ConnectZooKeeper(addresses string) (*zk.Conn, error) { + servers, err := parseZooKeeperAddresses(addresses) + if err != nil { + return nil, err + } + + conn, _, err := zk.Connect( + servers, + zooKeeperSessionTimeout, + zk.WithLogInfo(Flags.Verbose), + ) + if err != nil { + return nil, fmt.Errorf("connect to ZooKeeper at %q: %w", addresses, err) + } + + return conn, nil +} + +func parseZooKeeperAddresses(addresses string) ([]string, error) { + parts := strings.Split(addresses, ",") + servers := make([]string, 0, len(parts)) + for _, part := range parts { + server := strings.TrimSpace(part) + if server == "" { + return nil, fmt.Errorf("invalid ZooKeeper addresses %q: address must not be empty", addresses) + } + servers = append(servers, server) + } + + return servers, nil +} + +// EnsureZNode creates zpath and any missing parents. Existing nodes are left +// unchanged, including when another client creates one concurrently. +func EnsureZNode(conn *zk.Conn, zpath string) error { + if conn == nil { + return errors.New("ensure znode: ZooKeeper connection is nil") + } + return ensureZNode(conn, zpath) +} + +func ensureZNode(client znodeCreator, zpath string) error { + if err := validateZNodePath(zpath); err != nil { + return fmt.Errorf("ensure znode: %w", err) + } + if zpath == "/" { + return nil + } + + current := "" + for _, segment := range strings.Split(strings.TrimPrefix(zpath, "/"), "/") { + current += "/" + segment + + exists, _, err := client.Exists(current) + if err != nil { + return fmt.Errorf("check znode %q: %w", current, err) + } + if exists { + logZNodeState(current, "exists") + continue + } + + if _, err := client.Create(current, []byte{}, 0, zk.WorldACL(zk.PermAll)); err != nil { + if errors.Is(err, zk.ErrNodeExists) { + logZNodeState(current, "exists") + continue + } + return fmt.Errorf("create znode %q: %w", current, err) + } + logZNodeState(current, "created") + } + + return nil +} + +// DeleteZNode recursively deletes zpath. A node that is already absent is +// treated as successfully deleted. +func DeleteZNode(conn *zk.Conn, zpath string) error { + if conn == nil { + return errors.New("delete znode: ZooKeeper connection is nil") + } + if err := validateZNodePath(zpath); err != nil { + return fmt.Errorf("delete znode: %w", err) + } + if zpath == "/" { + return errors.New("delete znode: refusing to delete the ZooKeeper root") + } + + return deleteZNode(conn, zpath) +} + +func deleteZNode(client znodeDeleter, zpath string) error { + children, _, err := client.Children(zpath) + if errors.Is(err, zk.ErrNoNode) { + return nil + } + if err != nil { + return fmt.Errorf("list children of znode %q: %w", zpath, err) + } + + for _, child := range children { + if err := deleteZNode(client, path.Join(zpath, child)); err != nil { + return err + } + } + + if err := client.Delete(zpath, -1); err != nil && !errors.Is(err, zk.ErrNoNode) { + return fmt.Errorf("delete znode %q: %w", zpath, err) + } + + return nil +} + +func validateZNodePath(zpath string) error { + if zpath == "" { + return errors.New("path must not be empty") + } + if !strings.HasPrefix(zpath, "/") { + return fmt.Errorf("path %q must be absolute", zpath) + } + if cleaned := path.Clean(zpath); cleaned != zpath { + return fmt.Errorf("path %q is not canonical; use %q", zpath, cleaned) + } + return nil +} + +func logZNodeState(zpath string, state string) { + if Flags.Verbose { + log.Printf("znode %s %s", zpath, state) + } +} diff --git a/internal/zookeeper_test.go b/internal/zookeeper_test.go new file mode 100644 index 0000000..4b9601c --- /dev/null +++ b/internal/zookeeper_test.go @@ -0,0 +1,208 @@ +package internal + +import ( + "errors" + "reflect" + "testing" + + "github.com/go-zookeeper/zk" +) + +func TestParseZooKeeperAddresses(t *testing.T) { + testcases := []struct { + name string + addresses string + want []string + wantErr bool + }{ + {name: "single", addresses: "localhost:2181", want: []string{"localhost:2181"}}, + { + name: "multiple with whitespace", + addresses: "zk1:2181, zk2:2181 ,zk3:2181", + want: []string{"zk1:2181", "zk2:2181", "zk3:2181"}, + }, + {name: "empty", addresses: "", wantErr: true}, + {name: "empty member", addresses: "zk1:2181,,zk2:2181", wantErr: true}, + } + + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + got, err := parseZooKeeperAddresses(tc.addresses) + if (err != nil) != tc.wantErr { + t.Fatalf("parseZooKeeperAddresses(%q) error = %v, wantErr %v", tc.addresses, err, tc.wantErr) + } + if !reflect.DeepEqual(got, tc.want) { + t.Errorf("parseZooKeeperAddresses(%q) = %v, want %v", tc.addresses, got, tc.want) + } + }) + } +} + +func TestEnsureZNodeCreatesMissingParents(t *testing.T) { + client := newFakeZNodeClient() + client.existing["/arcus"] = true + + if err := ensureZNode(client, "/arcus/cache/list"); err != nil { + t.Fatalf("ensureZNode() error = %v", err) + } + + wantCreated := []string{"/arcus/cache", "/arcus/cache/list"} + if !reflect.DeepEqual(client.created, wantCreated) { + t.Errorf("created nodes = %v, want %v", client.created, wantCreated) + } +} + +func TestEnsureZNodeHandlesConcurrentCreate(t *testing.T) { + client := newFakeZNodeClient() + client.createErrors["/arcus"] = zk.ErrNodeExists + + if err := ensureZNode(client, "/arcus/cache"); err != nil { + t.Fatalf("ensureZNode() error = %v", err) + } + + wantCreated := []string{"/arcus", "/arcus/cache"} + if !reflect.DeepEqual(client.created, wantCreated) { + t.Errorf("create attempts = %v, want %v", client.created, wantCreated) + } +} + +func TestEnsureZNodeWrapsOperationError(t *testing.T) { + wantErr := errors.New("connection lost") + client := newFakeZNodeClient() + client.existsErrors["/arcus/cache"] = wantErr + + err := ensureZNode(client, "/arcus/cache") + if !errors.Is(err, wantErr) { + t.Fatalf("ensureZNode() error = %v, want wrapped %v", err, wantErr) + } +} + +func TestDeleteZNodeDeletesChildrenBeforeParent(t *testing.T) { + client := newFakeZNodeClient() + client.children["/arcus"] = []string{"cache", "clients"} + client.children["/arcus/cache"] = []string{"service"} + + if err := deleteZNode(client, "/arcus"); err != nil { + t.Fatalf("deleteZNode() error = %v", err) + } + + wantDeleted := []string{ + "/arcus/cache/service", + "/arcus/cache", + "/arcus/clients", + "/arcus", + } + if !reflect.DeepEqual(client.deleted, wantDeleted) { + t.Errorf("deleted nodes = %v, want %v", client.deleted, wantDeleted) + } +} + +func TestDeleteZNodeTreatsMissingNodeAsSuccess(t *testing.T) { + client := newFakeZNodeClient() + client.childrenErrors["/missing"] = zk.ErrNoNode + + if err := deleteZNode(client, "/missing"); err != nil { + t.Fatalf("deleteZNode() error = %v, want nil", err) + } + if len(client.deleted) != 0 { + t.Errorf("deleted nodes = %v, want none", client.deleted) + } +} + +func TestValidateZNodePath(t *testing.T) { + testcases := []struct { + name string + zpath string + wantErr bool + }{ + {name: "root", zpath: "/"}, + {name: "node", zpath: "/arcus/cache"}, + {name: "empty", zpath: "", wantErr: true}, + {name: "relative", zpath: "arcus/cache", wantErr: true}, + {name: "trailing slash", zpath: "/arcus/", wantErr: true}, + {name: "duplicate slash", zpath: "/arcus//cache", wantErr: true}, + {name: "dot segment", zpath: "/arcus/../cache", wantErr: true}, + } + + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + err := validateZNodePath(tc.zpath) + if (err != nil) != tc.wantErr { + t.Fatalf("validateZNodePath(%q) error = %v, wantErr %v", tc.zpath, err, tc.wantErr) + } + }) + } +} + +func TestZNodePublicFunctionsRejectUnsafeArguments(t *testing.T) { + if err := EnsureZNode(nil, "/arcus"); err == nil { + t.Error("EnsureZNode(nil, ...) error = nil, want error") + } + if err := DeleteZNode(nil, "/arcus"); err == nil { + t.Error("DeleteZNode(nil, ...) error = nil, want error") + } + if err := DeleteZNode(&zk.Conn{}, "/"); err == nil { + t.Error("DeleteZNode(..., root) error = nil, want error") + } +} + +type fakeZNodeClient struct { + existing map[string]bool + children map[string][]string + existsErrors map[string]error + createErrors map[string]error + childrenErrors map[string]error + deleteErrors map[string]error + created []string + deleted []string +} + +func newFakeZNodeClient() *fakeZNodeClient { + return &fakeZNodeClient{ + existing: make(map[string]bool), + children: make(map[string][]string), + existsErrors: make(map[string]error), + createErrors: make(map[string]error), + childrenErrors: make(map[string]error), + deleteErrors: make(map[string]error), + } +} + +func (f *fakeZNodeClient) Exists(zpath string) (bool, *zk.Stat, error) { + if err := f.existsErrors[zpath]; err != nil { + return false, nil, err + } + return f.existing[zpath], nil, nil +} + +func (f *fakeZNodeClient) Create( + zpath string, + _ []byte, + _ int32, + _ []zk.ACL, +) (string, error) { + f.created = append(f.created, zpath) + if err := f.createErrors[zpath]; err != nil { + if errors.Is(err, zk.ErrNodeExists) { + f.existing[zpath] = true + } + return "", err + } + f.existing[zpath] = true + return zpath, nil +} + +func (f *fakeZNodeClient) Children(zpath string) ([]string, *zk.Stat, error) { + if err := f.childrenErrors[zpath]; err != nil { + return nil, nil, err + } + return f.children[zpath], nil, nil +} + +func (f *fakeZNodeClient) Delete(zpath string, _ int32) error { + if err := f.deleteErrors[zpath]; err != nil { + return err + } + f.deleted = append(f.deleted, zpath) + return nil +}