From ce2c412979c22cab797d7d54d86ea794f46b8414 Mon Sep 17 00:00:00 2001 From: watany <76135106+watany-dev@users.noreply.github.com> Date: Fri, 11 Apr 2025 05:53:20 +0000 Subject: [PATCH 1/8] revert command --- README.md | 2 +- cmd/hawkling/commands/delete.go | 60 ++++++++++++++++++++++++- cmd/hawkling/commands/list.go | 37 ++++++++++++++-- cmd/hawkling/commands/prune.go | 78 ++++++++++++++++++++++++++++++++- cmd/hawkling/main.go | 3 ++ pkg/errors/errors.go | 10 +++++ 6 files changed, 182 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index 1329211..4bbe278 100644 --- a/README.md +++ b/README.md @@ -14,7 +14,7 @@ Hawkling is a command-line tool for managing AWS IAM roles, with a focus on iden ## Installation ```bash -curl -fsSL https://raw.githubusercontent.com/watany-dev/hawkling/main/install.sh | sh +curl -fsSL https://raw.githubusercontent.com/watany-dev/hawkling/main/script/install.sh |sh hawkling -h ``` diff --git a/cmd/hawkling/commands/delete.go b/cmd/hawkling/commands/delete.go index cecd61f..148dfbb 100644 --- a/cmd/hawkling/commands/delete.go +++ b/cmd/hawkling/commands/delete.go @@ -1,8 +1,14 @@ package commands import ( + "bufio" "context" "fmt" + "os" + "strings" + + "hawkling/pkg/aws" + "hawkling/pkg/errors" ) // DeleteOptions contains options for the delete command @@ -31,7 +37,57 @@ func NewDeleteCommand(profile, region, roleName string, options DeleteOptions) * // Execute runs the delete command func (c *DeleteCommand) Execute(ctx context.Context) error { - // Implementation would go here - fmt.Printf("Deleting IAM role: %s\n", c.roleName) + client, err := aws.NewAWSClient(ctx, c.profile, c.region) + if err != nil { + return errors.Wrap(err, "failed to create AWS client") + } + + // Get the role to ensure it exists + roles, err := client.ListRoles(ctx) + if err != nil { + return errors.Wrap(err, "failed to list roles") + } + + var targetRole *aws.Role + for i, role := range roles { + if role.Name == c.roleName { + targetRole = &roles[i] + break + } + } + + if targetRole == nil { + return errors.Errorf("role '%s' not found", c.roleName) + } + + // If dry run, just show what would be deleted + if c.options.DryRun { + fmt.Printf("DRY RUN: Would delete IAM role: %s\n", c.roleName) + return nil + } + + // Confirm deletion if force flag is not set + if !c.options.Force { + fmt.Printf("Are you sure you want to delete role '%s'? This cannot be undone. [y/N]: ", c.roleName) + + reader := bufio.NewReader(os.Stdin) + response, err := reader.ReadString('\n') + if err != nil { + return errors.Wrap(err, "failed to read confirmation") + } + + response = strings.TrimSpace(strings.ToLower(response)) + if response != "y" && response != "yes" { + fmt.Println("Deletion cancelled") + return nil + } + } + + // Delete the role + if err := client.DeleteRole(ctx, c.roleName); err != nil { + return errors.Wrap(err, "failed to delete role") + } + + fmt.Printf("Successfully deleted IAM role: %s\n", c.roleName) return nil } diff --git a/cmd/hawkling/commands/list.go b/cmd/hawkling/commands/list.go index 310b8bc..d4cc666 100644 --- a/cmd/hawkling/commands/list.go +++ b/cmd/hawkling/commands/list.go @@ -2,7 +2,11 @@ package commands import ( "context" - "fmt" + "strings" + + "hawkling/pkg/aws" + "hawkling/pkg/errors" + "hawkling/pkg/formatter" ) // ListOptions contains options for the list command @@ -32,7 +36,34 @@ func NewListCommand(profile, region string, options ListOptions) *ListCommand { // Execute runs the list command func (c *ListCommand) Execute(ctx context.Context) error { - // Implementation would go here - fmt.Println("Listing IAM roles") + client, err := aws.NewAWSClient(ctx, c.profile, c.region) + if err != nil { + return errors.Wrap(err, "failed to create AWS client") + } + + roles, err := client.ListRoles(ctx) + if err != nil { + return errors.Wrap(err, "failed to list roles") + } + + // Filter roles if needed + if c.options.OnlyUsed || c.options.OnlyUnused { + var filteredRoles []aws.Role + for _, role := range roles { + isUnused := role.IsUnused(c.options.Days) + + if (c.options.OnlyUsed && !isUnused) || (c.options.OnlyUnused && isUnused) { + filteredRoles = append(filteredRoles, role) + } + } + roles = filteredRoles + } + + // Format output + format := formatter.Format(strings.ToLower(c.options.Output)) + if err := formatter.FormatRoles(roles, format, c.options.ShowAll); err != nil { + return errors.Wrap(err, "failed to format output") + } + return nil } diff --git a/cmd/hawkling/commands/prune.go b/cmd/hawkling/commands/prune.go index d5e7eb0..fe4c69c 100644 --- a/cmd/hawkling/commands/prune.go +++ b/cmd/hawkling/commands/prune.go @@ -1,8 +1,14 @@ package commands import ( + "bufio" "context" "fmt" + "os" + "strings" + + "hawkling/pkg/aws" + "hawkling/pkg/errors" ) // PruneOptions contains options for the prune command @@ -30,7 +36,75 @@ func NewPruneCommand(profile, region string, options PruneOptions) *PruneCommand // Execute runs the prune command func (c *PruneCommand) Execute(ctx context.Context) error { - // Implementation would go here - fmt.Println("Pruning unused IAM roles") + client, err := aws.NewAWSClient(ctx, c.profile, c.region) + if err != nil { + return errors.Wrap(err, "failed to create AWS client") + } + + // Get all roles + roles, err := client.ListRoles(ctx) + if err != nil { + return errors.Wrap(err, "failed to list roles") + } + + // Find unused roles based on the specified days + var unusedRoles []aws.Role + for _, role := range roles { + if role.IsUnused(c.options.Days) { + unusedRoles = append(unusedRoles, role) + } + } + + if len(unusedRoles) == 0 { + fmt.Println("No unused IAM roles found") + return nil + } + + // Show unused roles + fmt.Printf("Found %d unused IAM roles (not used in the last %d days):\n", len(unusedRoles), c.options.Days) + for i, role := range unusedRoles { + fmt.Printf("%d. %s\n", i+1, role.Name) + } + + // If dry run, stop here + if c.options.DryRun { + fmt.Println("\nDRY RUN: No roles were deleted") + return nil + } + + // Confirm deletion if force flag is not set + if !c.options.Force { + fmt.Printf("\nAre you sure you want to delete %d unused roles? This cannot be undone. [y/N]: ", len(unusedRoles)) + + reader := bufio.NewReader(os.Stdin) + response, err := reader.ReadString('\n') + if err != nil { + return errors.Wrap(err, "failed to read confirmation") + } + + response = strings.TrimSpace(strings.ToLower(response)) + if response != "y" && response != "yes" { + fmt.Println("Deletion cancelled") + return nil + } + } + + // Delete the unused roles + var failedRoles []string + for _, role := range unusedRoles { + if err := client.DeleteRole(ctx, role.Name); err != nil { + failedRoles = append(failedRoles, role.Name) + fmt.Printf("Failed to delete role %s: %v\n", role.Name, err) + } else { + fmt.Printf("Deleted role: %s\n", role.Name) + } + } + + if len(failedRoles) > 0 { + fmt.Printf("\nFailed to delete %d roles: %s\n", len(failedRoles), strings.Join(failedRoles, ", ")) + return errors.Errorf("failed to delete %d roles", len(failedRoles)) + } + + fmt.Printf("\nSuccessfully deleted %d unused IAM roles\n", len(unusedRoles)) return nil } diff --git a/cmd/hawkling/main.go b/cmd/hawkling/main.go index e012d38..3002d5a 100644 --- a/cmd/hawkling/main.go +++ b/cmd/hawkling/main.go @@ -100,5 +100,8 @@ Complete documentation is available at https://github.com/watany-dev/hawkling`, } commands.AddPruneFlags(pruneCmd, &pruneDays, &dryRun, &force) + // Add commands to root command + rootCmd.AddCommand(listCmd,deleteCmd, pruneCmd) + return rootCmd } diff --git a/pkg/errors/errors.go b/pkg/errors/errors.go index 55ff574..7fa708e 100644 --- a/pkg/errors/errors.go +++ b/pkg/errors/errors.go @@ -2,6 +2,16 @@ package errors import "fmt" +// Wrap wraps an error with a message +func Wrap(err error, message string) error { + return fmt.Errorf("%s: %w", message, err) +} + +// Errorf formats an error message +func Errorf(format string, args ...interface{}) error { + return fmt.Errorf(format, args...) +} + // WrapRoleError wraps an error with a role operation context func WrapRoleError(op string, roleName string, err error) error { return fmt.Errorf("failed to %s role %s: %w", op, roleName, err) From c83394a437764c109b10987a82a0c40c11fea953 Mon Sep 17 00:00:00 2001 From: watany <76135106+watany-dev@users.noreply.github.com> Date: Fri, 11 Apr 2025 13:10:50 +0000 Subject: [PATCH 2/8] fmt --- cmd/hawkling/commands/list.go | 23 ++++-- cmd/hawkling/main.go | 2 +- pkg/aws/awsclient.go | 20 ++++- review.md | 74 +++++++++++++++-- test/list_command_test.go | 148 ++++++++++++++++++++++++++++++++++ test/list_filter_test.go | 124 ++++++++++++++++++++++++++++ test/test_unused_roles.go | 66 +++++++++++++++ test/unused_roles_test.go | 130 +++++++++++++++++++++++++++++ 8 files changed, 573 insertions(+), 14 deletions(-) create mode 100644 test/list_command_test.go create mode 100644 test/list_filter_test.go create mode 100644 test/test_unused_roles.go create mode 100644 test/unused_roles_test.go diff --git a/cmd/hawkling/commands/list.go b/cmd/hawkling/commands/list.go index d4cc666..90371fc 100644 --- a/cmd/hawkling/commands/list.go +++ b/cmd/hawkling/commands/list.go @@ -47,14 +47,27 @@ func (c *ListCommand) Execute(ctx context.Context) error { } // Filter roles if needed - if c.options.OnlyUsed || c.options.OnlyUnused { - var filteredRoles []aws.Role + var filteredRoles []aws.Role + + // If both filters are enabled, return empty list (logical conflict) + if c.options.OnlyUsed && c.options.OnlyUnused { + roles = filteredRoles + } else { + filteredRoles = make([]aws.Role, 0, len(roles)) + + // Apply filters for _, role := range roles { - isUnused := role.IsUnused(c.options.Days) + isUnusedForDays := role.IsUnused(c.options.Days) - if (c.options.OnlyUsed && !isUnused) || (c.options.OnlyUnused && isUnused) { - filteredRoles = append(filteredRoles, role) + if c.options.OnlyUsed && (role.LastUsed == nil || isUnusedForDays) { + continue } + + if c.options.OnlyUnused && !isUnusedForDays { + continue + } + + filteredRoles = append(filteredRoles, role) } roles = filteredRoles } diff --git a/cmd/hawkling/main.go b/cmd/hawkling/main.go index 3002d5a..5c9ba6d 100644 --- a/cmd/hawkling/main.go +++ b/cmd/hawkling/main.go @@ -101,7 +101,7 @@ Complete documentation is available at https://github.com/watany-dev/hawkling`, commands.AddPruneFlags(pruneCmd, &pruneDays, &dryRun, &force) // Add commands to root command - rootCmd.AddCommand(listCmd,deleteCmd, pruneCmd) + rootCmd.AddCommand(listCmd, deleteCmd, pruneCmd) return rootCmd } diff --git a/pkg/aws/awsclient.go b/pkg/aws/awsclient.go index 82787fa..57d944d 100644 --- a/pkg/aws/awsclient.go +++ b/pkg/aws/awsclient.go @@ -13,13 +13,31 @@ import ( "github.com/schollz/progressbar/v3" ) +// For testing +var testClient IAMClient + +// SetTestClient sets a test client for unit testing +func SetTestClient(client IAMClient) { + testClient = client +} + +// ClearTestClient clears the test client after tests +func ClearTestClient() { + testClient = nil +} + // AWSclient implements the IAMClient interface type AWSClient struct { iamClient *iam.Client } // NewAWSClient creates a new AWS client with the specified profile and region -func NewAWSClient(ctx context.Context, profile, region string) (*AWSClient, error) { +func NewAWSClient(ctx context.Context, profile, region string) (IAMClient, error) { + // If we're in test mode, return the test client + if testClient != nil { + return testClient, nil + } + var cfg aws.Config var err error diff --git a/review.md b/review.md index 95e1a0e..8e58e1f 100644 --- a/review.md +++ b/review.md @@ -6,6 +6,7 @@ 2. AWS APIリクエストの非効率な使用 3. スライスの初期化方法が非効率 4. フォーマッタにおけるメモリ使用の最適化不足 +5. フィルタリングフラグ(--used, --unused, --days)が正しく機能していない ## 改善提案 @@ -31,7 +32,43 @@ for _, role := range roles { } ``` -### 2. AWS APIリクエストの最適化 (awsclient.go) +### 2. フィルタリングフラグの修正 (list.go) + +現在の実装では、フィルタリングロジックに問題があります: + +```go +// 問題点: フィルタリングロジックが逆になっている +if (c.options.OnlyUsed && !isUnused) || (c.options.OnlyUnused && isUnused) { + filteredRoles = append(filteredRoles, role) +} +``` + +この実装では: +1. `OnlyUsed`フラグが立っていると、`!isUnused`(つまり使用中)のロールをフィルタリングしていますが、`isUnused`が誤った判定をしています。 +2. `OnlyUnused`フラグが立っていると、`isUnused`(未使用)のロールをフィルタリングしていますが、日数の考慮が不十分です。 + +修正案: +```go +// 修正案: フィルタリングロジックの明確化 +for _, role := range roles { + // days > 0 の条件を適切に扱う + isUnusedForDays := role.IsUnused(c.options.Days) + + // OnlyUsed: LastUsed != nil(使用されている)のロールだけを表示 + if c.options.OnlyUsed && (role.LastUsed == nil) { + continue + } + + // OnlyUnused: 指定日数間使用されていないロールだけを表示 + if c.options.OnlyUnused && !isUnusedForDays { + continue + } + + filteredRoles = append(filteredRoles, role) +} +``` + +### 3. AWS APIリクエストの最適化 (awsclient.go) `ListRoles`メソッドでは現在、ロールの取得とロールの最終使用日時の取得が分離されています。AWS APIが提供する情報を最大限活用することで、APIコールを減らせる可能性があります。 @@ -48,7 +85,25 @@ for i := range roles { // 改善後: ワーカープールパターンの採用 ``` -### 3. スライス初期化の最適化 +### 4. IsUnusedロジックの修正 (iam.go) + +現在の`IsUnused`メソッドのロジックも確認が必要です: + +```go +// IsUnused checks if a role is unused for the specified number of days +func (r *Role) IsUnused(days int) bool { + if r.LastUsed == nil { + return true // LastUsed が nil なら未使用 + } + + threshold := time.Now().AddDate(0, 0, -days) + return r.LastUsed.Before(threshold) // days日以前に使われていれば未使用と判断 +} +``` + +このロジックでは、`LastUsed`が`nil`(一度も使われていない)の場合は常に「未使用」として扱われます。また閾値との比較は、最後に使われた日が閾値より前(古い)場合に「未使用」と判断しています。これは論理的に正しいです。 + +### 5. スライス初期化の最適化 多くの場所でスライスが事前容量指定なしで初期化されています。容量を事前に指定することで、動的な拡張によるメモリ割り当てを減らせます。 @@ -60,15 +115,20 @@ var unusedRoles []aws.Role unusedRoles := make([]aws.Role, 0, len(roles)) ``` -### 4. formatter.goのメモリ使用最適化 +### 6. formatter.goのメモリ使用最適化 `FormatRolesAsJSON`関数では、すべてのロール情報がJSONとしてメモリに格納されてから出力されます。大量のロールがある場合、ストリーミング方式の出力に変更することで、メモリ使用量を削減できます。 ## 実装優先度 -1. **高**: スライス初期化の最適化(実装が容易かつ効果が明確) -2. **中**: フィルタリングロジックの統合(コード可読性を維持しつつ改善) -3. **中**: ゴルーチン管理の最適化(現状でもセマフォがあり基本的には問題ない) -4. **低**: JSONフォーマッタのストリーミング出力(大量データ時のみ効果あり) +1. **最高**: フィルタリングフラグの修正(機能面の問題解決) + - ✅ 実装完了: フィルタリングロジックを修正し、--used, --unused, --daysフラグが正しく機能するように修正 + - ✅ テスト追加: フィルタリングロジックのテストを追加 +2. **高**: スライス初期化の最適化(実装が容易かつ効果が明確) + - ✅ 部分的に実装: filteredRoles スライスの初期化を最適化 +3. **中**: フィルタリングロジックの統合(コード可読性を維持しつつ改善) + - ✅ 実装完了: フィルタリングロジックを1回のループで済むように最適化 +4. **中**: ゴルーチン管理の最適化(現状でもセマフォがあり基本的には問題ない) +5. **低**: JSONフォーマッタのストリーミング出力(大量データ時のみ効果あり) これらの改善により、メモリ使用量の削減とCPU使用効率の向上が期待できます。特に大量のロールを処理する場合に効果が顕著になるでしょう。 \ No newline at end of file diff --git a/test/list_command_test.go b/test/list_command_test.go new file mode 100644 index 0000000..b7e917f --- /dev/null +++ b/test/list_command_test.go @@ -0,0 +1,148 @@ +package test + +import ( + "bytes" + "context" + "io" + "os" + "strings" + "testing" + + "hawkling/cmd/hawkling/commands" + "hawkling/pkg/aws" + // "hawkling/pkg/formatter" +) + +// simulatedError is already defined in mock_iamclient.go + +// mockFormatter satisfies the formatter interface for testing +// type mockFormatter struct { +// output string +// } + +// func (m *mockFormatter) FormatRoles(roles []aws.Role, format formatter.Format, showAllInfo bool) error { +// var output strings.Builder +// for _, role := range roles { +// output.WriteString(role.Name + "\n") +// } +// m.output = output.String() +// return nil +// } + +func TestListCommandFilterFlags(t *testing.T) { + // Set up mock AWS client + mockClient := NewMockIAMClient() + aws.SetTestClient(mockClient) + // Clean up test client after test + defer aws.ClearTestClient() + + tests := []struct { + name string + options commands.ListOptions + expectedRoles int // Number of roles expected after filtering + shouldContain []string // Role names that should be in the output + shouldNotContain []string // Role names that should not be in the output + }{ + { + name: "No filters - should return all roles", + options: commands.ListOptions{ + Days: 90, + Output: "table", + ShowAll: false, + OnlyUsed: false, + OnlyUnused: false, + }, + expectedRoles: 3, // All 3 roles from the mock + shouldContain: []string{"ActiveRole", "InactiveRole", "NeverUsedRole"}, + shouldNotContain: []string{}, + }, + { + name: "Used filter - should return only used roles", + options: commands.ListOptions{ + Days: 90, + Output: "table", + ShowAll: false, + OnlyUsed: true, + OnlyUnused: false, + }, + expectedRoles: 1, // Only ActiveRole was used in the last 90 days + shouldContain: []string{"ActiveRole"}, + shouldNotContain: []string{"InactiveRole", "NeverUsedRole"}, + }, + { + name: "Unused filter - should return only unused roles", + options: commands.ListOptions{ + Days: 90, + Output: "table", + ShowAll: false, + OnlyUsed: false, + OnlyUnused: true, + }, + expectedRoles: 2, // InactiveRole and NeverUsedRole are unused + shouldContain: []string{"InactiveRole", "NeverUsedRole"}, + shouldNotContain: []string{"ActiveRole"}, + }, + { + name: "Both filters - should return no roles (conflicting filters)", + options: commands.ListOptions{ + Days: 90, + Output: "table", + ShowAll: false, + OnlyUsed: true, + OnlyUnused: true, + }, + expectedRoles: 0, // No roles match both used and unused + shouldContain: []string{}, + shouldNotContain: []string{"ActiveRole", "InactiveRole", "NeverUsedRole"}, + }, + { + name: "Days filter - 3 days threshold should return different results", + options: commands.ListOptions{ + Days: 3, + Output: "table", + ShowAll: false, + OnlyUsed: false, + OnlyUnused: true, + }, + expectedRoles: 3, // All should be unused with 3 day threshold (ActiveRole was used 5 days ago) + shouldContain: []string{"ActiveRole", "InactiveRole", "NeverUsedRole"}, // ActiveRole should be considered unused with 3 day threshold + shouldNotContain: []string{}, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + // Capture stdout + originalStdout := os.Stdout + r, w, _ := os.Pipe() + os.Stdout = w + + // Run the command + cmd := commands.NewListCommand("test-profile", "us-west-2", test.options) + err := cmd.Execute(context.Background()) + if err != nil { + t.Fatalf("Command execution failed: %v", err) + } + + // Restore stdout and get output + w.Close() + os.Stdout = originalStdout + var buf bytes.Buffer + _, _ = io.Copy(&buf, r) + output := buf.String() + + // Check expectations + for _, expectedStr := range test.shouldContain { + if !strings.Contains(output, expectedStr) { + t.Errorf("Expected output to contain %q, but it didn't.\nOutput: %s", expectedStr, output) + } + } + + for _, unexpectedStr := range test.shouldNotContain { + if strings.Contains(output, unexpectedStr) { + t.Errorf("Expected output to NOT contain %q, but it did.\nOutput: %s", unexpectedStr, output) + } + } + }) + } +} diff --git a/test/list_filter_test.go b/test/list_filter_test.go new file mode 100644 index 0000000..2b8c310 --- /dev/null +++ b/test/list_filter_test.go @@ -0,0 +1,124 @@ +package test + +import ( + "testing" + "time" + + "hawkling/pkg/aws" +) + +// timePtr is defined in mock_iamclient.go + +func TestFilterRoles(t *testing.T) { + now := time.Now() + + // Create test roles with different last used times + roles := []aws.Role{ + { + Name: "ActiveRole", + Arn: "arn:aws:iam::123456789012:role/ActiveRole", + CreateDate: now.AddDate(-1, 0, 0), + LastUsed: timePtr(now.AddDate(0, 0, -5)), // Used 5 days ago + Description: "Recently used role", + }, + { + Name: "InactiveRole", + Arn: "arn:aws:iam::123456789012:role/InactiveRole", + CreateDate: now.AddDate(-2, 0, 0), + LastUsed: timePtr(now.AddDate(0, 0, -100)), // Used 100 days ago + Description: "Role unused for a long time", + }, + { + Name: "NeverUsedRole", + Arn: "arn:aws:iam::123456789012:role/NeverUsedRole", + CreateDate: now.AddDate(0, -6, 0), + LastUsed: nil, // Never used + Description: "Role that was never used", + }, + } + + tests := []struct { + name string + days int + onlyUsed bool + onlyUnused bool + expectedRoles []string // Names of roles that should be in the result + }{ + { + name: "No filters - should return all roles", + days: 90, + onlyUsed: false, + onlyUnused: false, + expectedRoles: []string{"ActiveRole", "InactiveRole", "NeverUsedRole"}, + }, + { + name: "Used filter with 90 days - should return only used roles", + days: 90, + onlyUsed: true, + onlyUnused: false, + expectedRoles: []string{"ActiveRole"}, + }, + { + name: "Unused filter with 90 days - should return only unused roles", + days: 90, + onlyUsed: false, + onlyUnused: true, + expectedRoles: []string{"InactiveRole", "NeverUsedRole"}, + }, + { + name: "Both filters - should return no roles (conflicting filters)", + days: 90, + onlyUsed: true, + onlyUnused: true, + expectedRoles: []string{}, + }, + { + name: "Days filter - 3 days threshold should return different results", + days: 3, + onlyUsed: false, + onlyUnused: true, + expectedRoles: []string{"ActiveRole", "InactiveRole", "NeverUsedRole"}, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + // Apply filter + var filteredRoles []aws.Role + + // If both filters are enabled, return empty list (logical conflict) + if test.onlyUsed && test.onlyUnused { + // Leave filteredRoles empty + } else if test.onlyUsed || test.onlyUnused { + for _, role := range roles { + isUnused := role.IsUnused(test.days) + + if (test.onlyUsed && !isUnused) || (test.onlyUnused && isUnused) { + filteredRoles = append(filteredRoles, role) + } + } + } else { + filteredRoles = roles + } + + // Check if filtered roles match expected + if len(filteredRoles) != len(test.expectedRoles) { + t.Errorf("Expected %d roles, got %d", len(test.expectedRoles), len(filteredRoles)) + } + + // Check if each expected role is in the filtered roles + for _, expectedName := range test.expectedRoles { + found := false + for _, role := range filteredRoles { + if role.Name == expectedName { + found = true + break + } + } + if !found { + t.Errorf("Expected role %q not found in filtered roles", expectedName) + } + } + }) + } +} diff --git a/test/test_unused_roles.go b/test/test_unused_roles.go new file mode 100644 index 0000000..09bc644 --- /dev/null +++ b/test/test_unused_roles.go @@ -0,0 +1,66 @@ +package test + +import ( + "testing" + "time" + + "hawkling/pkg/aws" +) + +// TestIsUnused tests the IsUnused method of Role +func TestIsUnused(t *testing.T) { + now := time.Now() + + tests := []struct { + name string + role aws.Role + days int + expected bool + }{ + { + name: "Role used recently - should not be considered unused", + role: aws.Role{ + Name: "RecentlyUsedRole", + LastUsed: timePtr(now.AddDate(0, 0, -5)), // Used 5 days ago + }, + days: 90, + expected: false, // Not unused for 90 days + }, + { + name: "Role not used for a long time - should be considered unused", + role: aws.Role{ + Name: "OldRole", + LastUsed: timePtr(now.AddDate(0, 0, -100)), // Used 100 days ago + }, + days: 90, + expected: true, // Unused for 90 days + }, + { + name: "Role never used - should be considered unused", + role: aws.Role{ + Name: "NeverUsedRole", + LastUsed: nil, // Never used + }, + days: 90, + expected: true, // Never used, so unused for any days + }, + { + name: "Edge case - Role used exactly at threshold", + role: aws.Role{ + Name: "ThresholdRole", + LastUsed: timePtr(now.AddDate(0, 0, -90)), // Used 90 days ago + }, + days: 90, + expected: true, // At threshold, should be considered unused + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + result := test.role.IsUnused(test.days) + if result != test.expected { + t.Errorf("IsUnused(%d) = %v; want %v", test.days, result, test.expected) + } + }) + } +} diff --git a/test/unused_roles_test.go b/test/unused_roles_test.go new file mode 100644 index 0000000..b7cf865 --- /dev/null +++ b/test/unused_roles_test.go @@ -0,0 +1,130 @@ +package test + +import ( + "testing" + + "hawkling/cmd/hawkling/commands" + "hawkling/pkg/aws" +) + +func TestFilteringLogic(t *testing.T) { + // Create test roles - reusing the MockIAMClient setup + mockClient := NewMockIAMClient() + roles := mockClient.Roles + + tests := []struct { + name string + options commands.ListOptions + expectedCount int + expectedRoles []string + }{ + { + name: "No filtering", + options: commands.ListOptions{ + Days: 90, + OnlyUsed: false, + OnlyUnused: false, + }, + expectedCount: 3, + expectedRoles: []string{"ActiveRole", "InactiveRole", "NeverUsedRole"}, + }, + { + name: "Only used roles - 90 days", + options: commands.ListOptions{ + Days: 90, + OnlyUsed: true, + OnlyUnused: false, + }, + expectedCount: 1, + expectedRoles: []string{"ActiveRole"}, + }, + { + name: "Only unused roles - 90 days", + options: commands.ListOptions{ + Days: 90, + OnlyUsed: false, + OnlyUnused: true, + }, + expectedCount: 2, + expectedRoles: []string{"InactiveRole", "NeverUsedRole"}, + }, + { + name: "Only used roles - 3 days", + options: commands.ListOptions{ + Days: 3, + OnlyUsed: true, + OnlyUnused: false, + }, + expectedCount: 0, // None were used within 3 days + expectedRoles: []string{}, + }, + { + name: "Only unused roles - 3 days", + options: commands.ListOptions{ + Days: 3, + OnlyUsed: false, + OnlyUnused: true, + }, + expectedCount: 3, // All were unused for 3 days + expectedRoles: []string{"ActiveRole", "InactiveRole", "NeverUsedRole"}, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + var filteredRoles []aws.Role + + // Apply the improved filtering logic + for _, role := range roles { + isUnusedForDays := role.IsUnused(test.options.Days) + + // OnlyUsed: LastUsed != nil(使用されている)のロールだけを表示し、かつ指定日数内に使用されている + if test.options.OnlyUsed && (role.LastUsed == nil || isUnusedForDays) { + continue + } + + // OnlyUnused: 指定日数間使用されていないロールだけを表示 + if test.options.OnlyUnused && !isUnusedForDays { + continue + } + + filteredRoles = append(filteredRoles, role) + } + + // Check count + if len(filteredRoles) != test.expectedCount { + t.Errorf("Expected %d roles, got %d", test.expectedCount, len(filteredRoles)) + } + + // Check if expected roles are in the result + for _, expectedName := range test.expectedRoles { + found := false + for _, role := range filteredRoles { + if role.Name == expectedName { + found = true + break + } + } + if !found { + t.Errorf("Expected role %q not found in filtered roles", expectedName) + } + } + + // Check if there are no unexpected roles + if len(filteredRoles) > 0 { + for _, role := range filteredRoles { + found := false + for _, expectedName := range test.expectedRoles { + if role.Name == expectedName { + found = true + break + } + } + if !found { + t.Errorf("Unexpected role %q found in filtered roles", role.Name) + } + } + } + }) + } +} From de32352b2e2ba45e7625b3e6b265092f559d2dd9 Mon Sep 17 00:00:00 2001 From: watany <76135106+watany-dev@users.noreply.github.com> Date: Fri, 11 Apr 2025 14:55:43 +0000 Subject: [PATCH 3/8] fix days --- cmd/hawkling/commands/list.go | 18 ++++++-- test/list_command_test.go | 60 +++++++++++++++++++-------- test/list_filter_test.go | 77 +++++++++++++++++++++++++++-------- 3 files changed, 118 insertions(+), 37 deletions(-) diff --git a/cmd/hawkling/commands/list.go b/cmd/hawkling/commands/list.go index 90371fc..90d9f1c 100644 --- a/cmd/hawkling/commands/list.go +++ b/cmd/hawkling/commands/list.go @@ -3,6 +3,7 @@ package commands import ( "context" "strings" + "time" "hawkling/pkg/aws" "hawkling/pkg/errors" @@ -57,16 +58,25 @@ func (c *ListCommand) Execute(ctx context.Context) error { // Apply filters for _, role := range roles { - isUnusedForDays := role.IsUnused(c.options.Days) - - if c.options.OnlyUsed && (role.LastUsed == nil || isUnusedForDays) { + // OnlyUsed: 一度も使用されていないロール (LastUsed == nil) を除外 + if c.options.OnlyUsed && role.LastUsed == nil { continue } - if c.options.OnlyUnused && !isUnusedForDays { + // OnlyUnused: 一度でも使用されたロール (LastUsed != nil) を除外 + if c.options.OnlyUnused && role.LastUsed != nil { continue } + // Days フィルター: 指定された日数以内に使用されていない場合は除外 + if c.options.Days > 0 && role.LastUsed != nil { + threshold := time.Now().AddDate(0, 0, -c.options.Days) + isUnusedForDays := role.LastUsed.Before(threshold) + if !isUnusedForDays { + continue + } + } + filteredRoles = append(filteredRoles, role) } roles = filteredRoles diff --git a/test/list_command_test.go b/test/list_command_test.go index b7e917f..4b17be8 100644 --- a/test/list_command_test.go +++ b/test/list_command_test.go @@ -46,7 +46,7 @@ func TestListCommandFilterFlags(t *testing.T) { { name: "No filters - should return all roles", options: commands.ListOptions{ - Days: 90, + Days: 0, Output: "table", ShowAll: false, OnlyUsed: false, @@ -57,35 +57,35 @@ func TestListCommandFilterFlags(t *testing.T) { shouldNotContain: []string{}, }, { - name: "Used filter - should return only used roles", + name: "Used filter - should return only roles that were used at least once", options: commands.ListOptions{ - Days: 90, + Days: 0, Output: "table", ShowAll: false, OnlyUsed: true, OnlyUnused: false, }, - expectedRoles: 1, // Only ActiveRole was used in the last 90 days - shouldContain: []string{"ActiveRole"}, - shouldNotContain: []string{"InactiveRole", "NeverUsedRole"}, + expectedRoles: 2, // ActiveRole and InactiveRole have been used + shouldContain: []string{"ActiveRole", "InactiveRole"}, + shouldNotContain: []string{"NeverUsedRole"}, }, { - name: "Unused filter - should return only unused roles", + name: "Unused filter - should return only roles that were never used", options: commands.ListOptions{ - Days: 90, + Days: 0, Output: "table", ShowAll: false, OnlyUsed: false, OnlyUnused: true, }, - expectedRoles: 2, // InactiveRole and NeverUsedRole are unused - shouldContain: []string{"InactiveRole", "NeverUsedRole"}, - shouldNotContain: []string{"ActiveRole"}, + expectedRoles: 1, // Only NeverUsedRole was never used + shouldContain: []string{"NeverUsedRole"}, + shouldNotContain: []string{"ActiveRole", "InactiveRole"}, }, { name: "Both filters - should return no roles (conflicting filters)", options: commands.ListOptions{ - Days: 90, + Days: 0, Output: "table", ShowAll: false, OnlyUsed: true, @@ -96,17 +96,43 @@ func TestListCommandFilterFlags(t *testing.T) { shouldNotContain: []string{"ActiveRole", "InactiveRole", "NeverUsedRole"}, }, { - name: "Days filter - 3 days threshold should return different results", + name: "Days filter with 90 days - should return roles not used in 90 days", + options: commands.ListOptions{ + Days: 90, + Output: "table", + ShowAll: false, + OnlyUsed: false, + OnlyUnused: false, + }, + expectedRoles: 2, // InactiveRole was used 100 days ago, NeverUsedRole was never used + shouldContain: []string{"InactiveRole", "NeverUsedRole"}, + shouldNotContain: []string{"ActiveRole"}, + }, + { + name: "Days filter with Used filter - should return used roles not used in 90 days", + options: commands.ListOptions{ + Days: 90, + Output: "table", + ShowAll: false, + OnlyUsed: true, + OnlyUnused: false, + }, + expectedRoles: 1, // Only InactiveRole was used but not in last 90 days + shouldContain: []string{"InactiveRole"}, + shouldNotContain: []string{"ActiveRole", "NeverUsedRole"}, + }, + { + name: "Days filter with Unused filter - should return never used roles", options: commands.ListOptions{ - Days: 3, + Days: 90, Output: "table", ShowAll: false, OnlyUsed: false, OnlyUnused: true, }, - expectedRoles: 3, // All should be unused with 3 day threshold (ActiveRole was used 5 days ago) - shouldContain: []string{"ActiveRole", "InactiveRole", "NeverUsedRole"}, // ActiveRole should be considered unused with 3 day threshold - shouldNotContain: []string{}, + expectedRoles: 1, // Only NeverUsedRole was never used + shouldContain: []string{"NeverUsedRole"}, + shouldNotContain: []string{"ActiveRole", "InactiveRole"}, }, } diff --git a/test/list_filter_test.go b/test/list_filter_test.go index 2b8c310..6e4ff4f 100644 --- a/test/list_filter_test.go +++ b/test/list_filter_test.go @@ -46,39 +46,60 @@ func TestFilterRoles(t *testing.T) { }{ { name: "No filters - should return all roles", - days: 90, + days: 0, onlyUsed: false, onlyUnused: false, expectedRoles: []string{"ActiveRole", "InactiveRole", "NeverUsedRole"}, }, { - name: "Used filter with 90 days - should return only used roles", - days: 90, + name: "Used filter - should return only roles that were used at least once", + days: 0, onlyUsed: true, onlyUnused: false, - expectedRoles: []string{"ActiveRole"}, + expectedRoles: []string{"ActiveRole", "InactiveRole"}, }, { - name: "Unused filter with 90 days - should return only unused roles", - days: 90, + name: "Unused filter - should return only roles that were never used", + days: 0, onlyUsed: false, onlyUnused: true, - expectedRoles: []string{"InactiveRole", "NeverUsedRole"}, + expectedRoles: []string{"NeverUsedRole"}, }, { name: "Both filters - should return no roles (conflicting filters)", - days: 90, + days: 0, onlyUsed: true, onlyUnused: true, expectedRoles: []string{}, }, { - name: "Days filter - 3 days threshold should return different results", + name: "Days filter with 90 days - should return roles not used in 90 days", + days: 90, + onlyUsed: false, + onlyUnused: false, + expectedRoles: []string{"InactiveRole", "NeverUsedRole"}, + }, + { + name: "Days filter with 3 days - should return roles not used in 3 days", days: 3, onlyUsed: false, - onlyUnused: true, + onlyUnused: false, expectedRoles: []string{"ActiveRole", "InactiveRole", "NeverUsedRole"}, }, + { + name: "Days filter with Used filter - should return used roles not used in 90 days", + days: 90, + onlyUsed: true, + onlyUnused: false, + expectedRoles: []string{"InactiveRole"}, + }, + { + name: "Days filter with Unused filter - should return never used roles", + days: 90, + onlyUsed: false, + onlyUnused: true, + expectedRoles: []string{"NeverUsedRole"}, + }, } for _, test := range tests { @@ -89,21 +110,37 @@ func TestFilterRoles(t *testing.T) { // If both filters are enabled, return empty list (logical conflict) if test.onlyUsed && test.onlyUnused { // Leave filteredRoles empty - } else if test.onlyUsed || test.onlyUnused { + } else { + filteredRoles = make([]aws.Role, 0, len(roles)) + for _, role := range roles { - isUnused := role.IsUnused(test.days) + // OnlyUsed: 一度も使用されていないロール (LastUsed == nil) を除外 + if test.onlyUsed && role.LastUsed == nil { + continue + } + + // OnlyUnused: 一度でも使用されたロール (LastUsed != nil) を除外 + if test.onlyUnused && role.LastUsed != nil { + continue + } - if (test.onlyUsed && !isUnused) || (test.onlyUnused && isUnused) { - filteredRoles = append(filteredRoles, role) + // Days フィルター: 指定された日数以内に使用されていない場合は除外 + if test.days > 0 && role.LastUsed != nil { + threshold := time.Now().AddDate(0, 0, -test.days) + if !role.LastUsed.Before(threshold) { + continue + } } + + filteredRoles = append(filteredRoles, role) } - } else { - filteredRoles = roles } // Check if filtered roles match expected if len(filteredRoles) != len(test.expectedRoles) { t.Errorf("Expected %d roles, got %d", len(test.expectedRoles), len(filteredRoles)) + t.Logf("Expected: %v", test.expectedRoles) + t.Logf("Got: %v", getRoleNames(filteredRoles)) } // Check if each expected role is in the filtered roles @@ -122,3 +159,11 @@ func TestFilterRoles(t *testing.T) { }) } } + +func getRoleNames(roles []aws.Role) []string { + names := make([]string, len(roles)) + for i, role := range roles { + names[i] = role.Name + } + return names +} From d36eb46f325e98321f06c3fcc8802bcf48b53491 Mon Sep 17 00:00:00 2001 From: watany <76135106+watany-dev@users.noreply.github.com> Date: Fri, 11 Apr 2025 15:13:06 +0000 Subject: [PATCH 4/8] filter --- cmd/hawkling/commands/common.go | 2 +- cmd/hawkling/commands/list.go | 9 ++++++--- test/list_filter_test.go | 16 ++++++++++++---- 3 files changed, 19 insertions(+), 8 deletions(-) diff --git a/cmd/hawkling/commands/common.go b/cmd/hawkling/commands/common.go index cc9eca2..de982c2 100644 --- a/cmd/hawkling/commands/common.go +++ b/cmd/hawkling/commands/common.go @@ -12,7 +12,7 @@ func AddCommonFlags(cmd *cobra.Command, profile *string, region *string) { // AddFilterFlags adds filtering flags to a command func AddFilterFlags(cmd *cobra.Command, days *int, onlyUsed *bool, onlyUnused *bool) { - cmd.Flags().IntVarP(days, "days", "d", 90, "Number of days to consider for usage") + cmd.Flags().IntVarP(days, "days", "d", 0, "Number of days to consider for usage") cmd.Flags().BoolVar(onlyUsed, "used", false, "Show only used roles") cmd.Flags().BoolVar(onlyUnused, "unused", false, "Show only unused roles") } diff --git a/cmd/hawkling/commands/list.go b/cmd/hawkling/commands/list.go index 90d9f1c..15c6482 100644 --- a/cmd/hawkling/commands/list.go +++ b/cmd/hawkling/commands/list.go @@ -54,6 +54,11 @@ func (c *ListCommand) Execute(ctx context.Context) error { if c.options.OnlyUsed && c.options.OnlyUnused { roles = filteredRoles } else { + var threshold time.Time + if c.options.Days > 0 { + threshold = time.Now().AddDate(0, 0, -c.options.Days) + } + filteredRoles = make([]aws.Role, 0, len(roles)) // Apply filters @@ -70,9 +75,7 @@ func (c *ListCommand) Execute(ctx context.Context) error { // Days フィルター: 指定された日数以内に使用されていない場合は除外 if c.options.Days > 0 && role.LastUsed != nil { - threshold := time.Now().AddDate(0, 0, -c.options.Days) - isUnusedForDays := role.LastUsed.Before(threshold) - if !isUnusedForDays { + if !role.LastUsed.Before(threshold) { continue } } diff --git a/test/list_filter_test.go b/test/list_filter_test.go index 6e4ff4f..bb19f63 100644 --- a/test/list_filter_test.go +++ b/test/list_filter_test.go @@ -7,7 +7,10 @@ import ( "hawkling/pkg/aws" ) -// timePtr is defined in mock_iamclient.go +// Local timePtr for tests +func localTimePtr(t time.Time) *time.Time { + return &t +} func TestFilterRoles(t *testing.T) { now := time.Now() @@ -18,14 +21,14 @@ func TestFilterRoles(t *testing.T) { Name: "ActiveRole", Arn: "arn:aws:iam::123456789012:role/ActiveRole", CreateDate: now.AddDate(-1, 0, 0), - LastUsed: timePtr(now.AddDate(0, 0, -5)), // Used 5 days ago + LastUsed: localTimePtr(now.AddDate(0, 0, -5)), // Used 5 days ago Description: "Recently used role", }, { Name: "InactiveRole", Arn: "arn:aws:iam::123456789012:role/InactiveRole", CreateDate: now.AddDate(-2, 0, 0), - LastUsed: timePtr(now.AddDate(0, 0, -100)), // Used 100 days ago + LastUsed: localTimePtr(now.AddDate(0, 0, -100)), // Used 100 days ago Description: "Role unused for a long time", }, { @@ -111,6 +114,12 @@ func TestFilterRoles(t *testing.T) { if test.onlyUsed && test.onlyUnused { // Leave filteredRoles empty } else { + // 閾値計算をループの外に移動 + var threshold time.Time + if test.days > 0 { + threshold = time.Now().AddDate(0, 0, -test.days) + } + filteredRoles = make([]aws.Role, 0, len(roles)) for _, role := range roles { @@ -126,7 +135,6 @@ func TestFilterRoles(t *testing.T) { // Days フィルター: 指定された日数以内に使用されていない場合は除外 if test.days > 0 && role.LastUsed != nil { - threshold := time.Now().AddDate(0, 0, -test.days) if !role.LastUsed.Before(threshold) { continue } From a3364a6f795824913c5e4ed06154d1f8653aeeba Mon Sep 17 00:00:00 2001 From: watany <76135106+watany-dev@users.noreply.github.com> Date: Fri, 11 Apr 2025 15:44:51 +0000 Subject: [PATCH 5/8] english --- Makefile | 2 +- cmd/hawkling/commands/list.go | 8 +-- review.md | 106 +++++++++++++++++----------------- test/list_filter_test.go | 8 +-- test/unused_roles_test.go | 4 +- 5 files changed, 64 insertions(+), 64 deletions(-) diff --git a/Makefile b/Makefile index 5425d73..6a7c7dd 100644 --- a/Makefile +++ b/Makefile @@ -25,7 +25,7 @@ build: build-release build-dev: go build -o build/hawkling-dev ./cmd/hawkling -# リリースビルド(サイズ最適化) +# Release build (size optimization) build-release: CGO_ENABLED=0 go build -ldflags="-s -w" -o build/hawkling ./cmd/hawkling diff --git a/cmd/hawkling/commands/list.go b/cmd/hawkling/commands/list.go index 15c6482..08006d2 100644 --- a/cmd/hawkling/commands/list.go +++ b/cmd/hawkling/commands/list.go @@ -63,17 +63,17 @@ func (c *ListCommand) Execute(ctx context.Context) error { // Apply filters for _, role := range roles { - // OnlyUsed: 一度も使用されていないロール (LastUsed == nil) を除外 + // OnlyUsed: exclude roles that have never been used (LastUsed == nil) if c.options.OnlyUsed && role.LastUsed == nil { continue } - // OnlyUnused: 一度でも使用されたロール (LastUsed != nil) を除外 + // OnlyUnused: exclude roles that have been used at least once (LastUsed != nil) if c.options.OnlyUnused && role.LastUsed != nil { continue } - // Days フィルター: 指定された日数以内に使用されていない場合は除外 + // Days filter: exclude roles that have been used within the specified days if c.options.Days > 0 && role.LastUsed != nil { if !role.LastUsed.Before(threshold) { continue @@ -92,4 +92,4 @@ func (c *ListCommand) Execute(ctx context.Context) error { } return nil -} +} \ No newline at end of file diff --git a/review.md b/review.md index 8e58e1f..cd271b1 100644 --- a/review.md +++ b/review.md @@ -1,26 +1,26 @@ -# パフォーマンス改善レビュー +# Performance Improvement Review -## 現状の問題点 +## Current Issues -1. `main.go` における不必要なフィルタリングの重複 -2. AWS APIリクエストの非効率な使用 -3. スライスの初期化方法が非効率 -4. フォーマッタにおけるメモリ使用の最適化不足 -5. フィルタリングフラグ(--used, --unused, --days)が正しく機能していない +1. Unnecessary duplicate filtering in `main.go` +2. Inefficient use of AWS API requests +3. Inefficient slice initialization methods +4. Insufficient memory usage optimization in formatter +5. Filtering flags (--used, --unused, --days) not functioning correctly -## 改善提案 +## Improvement Suggestions -### 1. フィルタリングロジックの最適化 (main.go) +### 1. Optimize Filtering Logic (main.go) -現在、`listRoles`関数では複数のフィルタ条件(days, onlyUsed, onlyUnused)がそれぞれ独立して適用されており、複数回のスライス走査が発生しています。これを1回の走査で済むように最適化できます。 +Currently, in the `listRoles` function, multiple filter conditions (days, onlyUsed, onlyUnused) are applied independently, resulting in multiple slice traversals. This can be optimized to complete with a single traversal. ```go -// 改善前: 複数回のフィルタリングループ -if days > 0 { /* フィルタリング処理 */ } -if onlyUsed { /* フィルタリング処理 */ } -if onlyUnused { /* フィルタリング処理 */ } +// Before improvement: Multiple filtering loops +if days > 0 { /* filtering process */ } +if onlyUsed { /* filtering process */ } +if onlyUnused { /* filtering process */ } -// 改善後: 1回のループで全条件を評価 +// After improvement: Evaluate all conditions in one loop var filteredRoles []aws.Role for _, role := range roles { if (days > 0 && !role.IsUnused(days)) || @@ -32,34 +32,34 @@ for _, role := range roles { } ``` -### 2. フィルタリングフラグの修正 (list.go) +### 2. Fix Filtering Flags (list.go) -現在の実装では、フィルタリングロジックに問題があります: +There are issues with the filtering logic in the current implementation: ```go -// 問題点: フィルタリングロジックが逆になっている +// Issue: Filtering logic is reversed if (c.options.OnlyUsed && !isUnused) || (c.options.OnlyUnused && isUnused) { filteredRoles = append(filteredRoles, role) } ``` -この実装では: -1. `OnlyUsed`フラグが立っていると、`!isUnused`(つまり使用中)のロールをフィルタリングしていますが、`isUnused`が誤った判定をしています。 -2. `OnlyUnused`フラグが立っていると、`isUnused`(未使用)のロールをフィルタリングしていますが、日数の考慮が不十分です。 +In this implementation: +1. When the `OnlyUsed` flag is set, it filters roles that are `!isUnused` (i.e., in use), but `isUnused` is incorrectly determined. +2. When the `OnlyUnused` flag is set, it filters roles that are `isUnused` (unused), but the consideration of days is insufficient. -修正案: +Fix proposal: ```go -// 修正案: フィルタリングロジックの明確化 +// Fix proposal: Clarify filtering logic for _, role := range roles { - // days > 0 の条件を適切に扱う + // Properly handle the days > 0 condition isUnusedForDays := role.IsUnused(c.options.Days) - // OnlyUsed: LastUsed != nil(使用されている)のロールだけを表示 + // OnlyUsed: Show only roles that have been used (LastUsed != nil) if c.options.OnlyUsed && (role.LastUsed == nil) { continue } - // OnlyUnused: 指定日数間使用されていないロールだけを表示 + // OnlyUnused: Show only roles that have not been used within the specified days if c.options.OnlyUnused && !isUnusedForDays { continue } @@ -68,67 +68,67 @@ for _, role := range roles { } ``` -### 3. AWS APIリクエストの最適化 (awsclient.go) +### 3. Optimize AWS API Requests (awsclient.go) -`ListRoles`メソッドでは現在、ロールの取得とロールの最終使用日時の取得が分離されています。AWS APIが提供する情報を最大限活用することで、APIコールを減らせる可能性があります。 +In the `ListRoles` method, currently the retrieval of roles and the retrieval of the last used date for roles are separated. There may be potential to reduce API calls by maximizing the use of information provided by the AWS API. -また、ゴルーチンの起動方法を改善することでオーバーヘッドを減らせます。 +Also, improving how goroutines are launched can reduce overhead. ```go -// 改善前: ゴルーチン起動のオーバーヘッド +// Before improvement: Overhead of launching goroutines for i := range roles { go func(i int) { // ... }(i) } -// 改善後: ワーカープールパターンの採用 +// After improvement: Adopt worker pool pattern ``` -### 4. IsUnusedロジックの修正 (iam.go) +### 4. Fix IsUnused Logic (iam.go) -現在の`IsUnused`メソッドのロジックも確認が必要です: +The logic in the current `IsUnused` method also needs to be checked: ```go // IsUnused checks if a role is unused for the specified number of days func (r *Role) IsUnused(days int) bool { if r.LastUsed == nil { - return true // LastUsed が nil なら未使用 + return true // If LastUsed is nil, the role is unused } threshold := time.Now().AddDate(0, 0, -days) - return r.LastUsed.Before(threshold) // days日以前に使われていれば未使用と判断 + return r.LastUsed.Before(threshold) // If it was last used before 'days' days ago, consider it unused } ``` -このロジックでは、`LastUsed`が`nil`(一度も使われていない)の場合は常に「未使用」として扱われます。また閾値との比較は、最後に使われた日が閾値より前(古い)場合に「未使用」と判断しています。これは論理的に正しいです。 +In this logic, if `LastUsed` is `nil` (never used), it is always treated as "unused". Also, the comparison with the threshold determines that a role is "unused" if the last used date is before (older than) the threshold. This is logically correct. -### 5. スライス初期化の最適化 +### 5. Optimize Slice Initialization -多くの場所でスライスが事前容量指定なしで初期化されています。容量を事前に指定することで、動的な拡張によるメモリ割り当てを減らせます。 +In many places, slices are initialized without specifying a pre-capacity. By specifying capacity in advance, memory allocations due to dynamic expansion can be reduced. ```go -// 改善前 +// Before improvement var unusedRoles []aws.Role -// 改善後 +// After improvement unusedRoles := make([]aws.Role, 0, len(roles)) ``` -### 6. formatter.goのメモリ使用最適化 +### 6. Optimize Memory Usage in formatter.go -`FormatRolesAsJSON`関数では、すべてのロール情報がJSONとしてメモリに格納されてから出力されます。大量のロールがある場合、ストリーミング方式の出力に変更することで、メモリ使用量を削減できます。 +In the `FormatRolesAsJSON` function, all role information is stored in memory as JSON before being output. For cases with many roles, changing to a streaming output method can reduce memory usage. -## 実装優先度 +## Implementation Priorities -1. **最高**: フィルタリングフラグの修正(機能面の問題解決) - - ✅ 実装完了: フィルタリングロジックを修正し、--used, --unused, --daysフラグが正しく機能するように修正 - - ✅ テスト追加: フィルタリングロジックのテストを追加 -2. **高**: スライス初期化の最適化(実装が容易かつ効果が明確) - - ✅ 部分的に実装: filteredRoles スライスの初期化を最適化 -3. **中**: フィルタリングロジックの統合(コード可読性を維持しつつ改善) - - ✅ 実装完了: フィルタリングロジックを1回のループで済むように最適化 -4. **中**: ゴルーチン管理の最適化(現状でもセマフォがあり基本的には問題ない) -5. **低**: JSONフォーマッタのストリーミング出力(大量データ時のみ効果あり) +1. **Highest**: Fix filtering flags (resolve functional issues) + - ✅ Implementation complete: Modified filtering logic so that --used, --unused, --days flags function correctly + - ✅ Tests added: Added tests for filtering logic +2. **High**: Optimize slice initialization (easy to implement and clear effect) + - ✅ Partially implemented: Optimized initialization of filteredRoles slice +3. **Medium**: Integrate filtering logic (improve while maintaining code readability) + - ✅ Implementation complete: Optimized filtering logic to complete in a single loop +4. **Medium**: Optimize goroutine management (currently has semaphores, so basically no problem) +5. **Low**: Stream output in JSON formatter (only effective with large amounts of data) -これらの改善により、メモリ使用量の削減とCPU使用効率の向上が期待できます。特に大量のロールを処理する場合に効果が顕著になるでしょう。 \ No newline at end of file +These improvements are expected to reduce memory usage and improve CPU usage efficiency. The effects will be particularly noticeable when processing large numbers of roles. \ No newline at end of file diff --git a/test/list_filter_test.go b/test/list_filter_test.go index bb19f63..9584819 100644 --- a/test/list_filter_test.go +++ b/test/list_filter_test.go @@ -114,7 +114,7 @@ func TestFilterRoles(t *testing.T) { if test.onlyUsed && test.onlyUnused { // Leave filteredRoles empty } else { - // 閾値計算をループの外に移動 + // Move threshold calculation outside the loop var threshold time.Time if test.days > 0 { threshold = time.Now().AddDate(0, 0, -test.days) @@ -123,17 +123,17 @@ func TestFilterRoles(t *testing.T) { filteredRoles = make([]aws.Role, 0, len(roles)) for _, role := range roles { - // OnlyUsed: 一度も使用されていないロール (LastUsed == nil) を除外 + // OnlyUsed: Exclude roles that have never been used (LastUsed == nil) if test.onlyUsed && role.LastUsed == nil { continue } - // OnlyUnused: 一度でも使用されたロール (LastUsed != nil) を除外 + // OnlyUnused: Exclude roles that have been used at least once (LastUsed != nil) if test.onlyUnused && role.LastUsed != nil { continue } - // Days フィルター: 指定された日数以内に使用されていない場合は除外 + // Days filter: Exclude roles that have not been used within the specified days if test.days > 0 && role.LastUsed != nil { if !role.LastUsed.Before(threshold) { continue diff --git a/test/unused_roles_test.go b/test/unused_roles_test.go index b7cf865..26356ff 100644 --- a/test/unused_roles_test.go +++ b/test/unused_roles_test.go @@ -78,12 +78,12 @@ func TestFilteringLogic(t *testing.T) { for _, role := range roles { isUnusedForDays := role.IsUnused(test.options.Days) - // OnlyUsed: LastUsed != nil(使用されている)のロールだけを表示し、かつ指定日数内に使用されている + // OnlyUsed: Show only roles that have been used (LastUsed != nil) and were used within the specified days if test.options.OnlyUsed && (role.LastUsed == nil || isUnusedForDays) { continue } - // OnlyUnused: 指定日数間使用されていないロールだけを表示 + // OnlyUnused: Show only roles that have not been used within the specified days if test.options.OnlyUnused && !isUnusedForDays { continue } From 5abf1d009aa1a1af9154cb46e58611e3e50f03da Mon Sep 17 00:00:00 2001 From: watany <76135106+watany-dev@users.noreply.github.com> Date: Fri, 11 Apr 2025 16:59:18 +0000 Subject: [PATCH 6/8] fix --- cmd/hawkling/commands/common.go | 21 ++++++++++ cmd/hawkling/commands/delete.go | 12 ++---- cmd/hawkling/commands/list.go | 45 ++++----------------- cmd/hawkling/commands/prune.go | 21 ++++------ cmd/hawkling/main.go | 11 +++-- pkg/aws/filter.go | 46 +++++++++++++++++++++ test/list_filter_test.go | 42 ++++--------------- test/unused_roles_test.go | 72 +++++++++++++++++++-------------- 8 files changed, 142 insertions(+), 128 deletions(-) create mode 100644 pkg/aws/filter.go diff --git a/cmd/hawkling/commands/common.go b/cmd/hawkling/commands/common.go index de982c2..4e59ea9 100644 --- a/cmd/hawkling/commands/common.go +++ b/cmd/hawkling/commands/common.go @@ -1,9 +1,30 @@ package commands import ( + "bufio" + "fmt" + "os" + "strings" + "github.com/spf13/cobra" + + "hawkling/pkg/errors" ) +// ConfirmAction prompts the user for confirmation and returns their response +func ConfirmAction(prompt string) (bool, error) { + fmt.Print(prompt) + + reader := bufio.NewReader(os.Stdin) + response, err := reader.ReadString('\n') + if err != nil { + return false, errors.Wrap(err, "failed to read confirmation") + } + + response = strings.TrimSpace(strings.ToLower(response)) + return response == "y" || response == "yes", nil +} + // AddCommonFlags adds common flags to a command func AddCommonFlags(cmd *cobra.Command, profile *string, region *string) { cmd.PersistentFlags().StringVarP(profile, "profile", "p", "", "AWS profile to use") diff --git a/cmd/hawkling/commands/delete.go b/cmd/hawkling/commands/delete.go index 148dfbb..c4209ac 100644 --- a/cmd/hawkling/commands/delete.go +++ b/cmd/hawkling/commands/delete.go @@ -1,11 +1,8 @@ package commands import ( - "bufio" "context" "fmt" - "os" - "strings" "hawkling/pkg/aws" "hawkling/pkg/errors" @@ -68,16 +65,13 @@ func (c *DeleteCommand) Execute(ctx context.Context) error { // Confirm deletion if force flag is not set if !c.options.Force { - fmt.Printf("Are you sure you want to delete role '%s'? This cannot be undone. [y/N]: ", c.roleName) - - reader := bufio.NewReader(os.Stdin) - response, err := reader.ReadString('\n') + prompt := fmt.Sprintf("Are you sure you want to delete role '%s'? This cannot be undone. [y/N]: ", c.roleName) + confirmed, err := ConfirmAction(prompt) if err != nil { return errors.Wrap(err, "failed to read confirmation") } - response = strings.TrimSpace(strings.ToLower(response)) - if response != "y" && response != "yes" { + if !confirmed { fmt.Println("Deletion cancelled") return nil } diff --git a/cmd/hawkling/commands/list.go b/cmd/hawkling/commands/list.go index 08006d2..8961610 100644 --- a/cmd/hawkling/commands/list.go +++ b/cmd/hawkling/commands/list.go @@ -3,7 +3,6 @@ package commands import ( "context" "strings" - "time" "hawkling/pkg/aws" "hawkling/pkg/errors" @@ -48,43 +47,15 @@ func (c *ListCommand) Execute(ctx context.Context) error { } // Filter roles if needed - var filteredRoles []aws.Role - - // If both filters are enabled, return empty list (logical conflict) - if c.options.OnlyUsed && c.options.OnlyUnused { - roles = filteredRoles - } else { - var threshold time.Time - if c.options.Days > 0 { - threshold = time.Now().AddDate(0, 0, -c.options.Days) - } - - filteredRoles = make([]aws.Role, 0, len(roles)) - - // Apply filters - for _, role := range roles { - // OnlyUsed: exclude roles that have never been used (LastUsed == nil) - if c.options.OnlyUsed && role.LastUsed == nil { - continue - } - - // OnlyUnused: exclude roles that have been used at least once (LastUsed != nil) - if c.options.OnlyUnused && role.LastUsed != nil { - continue - } - - // Days filter: exclude roles that have been used within the specified days - if c.options.Days > 0 && role.LastUsed != nil { - if !role.LastUsed.Before(threshold) { - continue - } - } - - filteredRoles = append(filteredRoles, role) - } - roles = filteredRoles + filterOptions := aws.FilterOptions{ + Days: c.options.Days, + OnlyUsed: c.options.OnlyUsed, + OnlyUnused: c.options.OnlyUnused, } + // Use unified filter implementation + roles = aws.FilterRoles(roles, filterOptions) + // Format output format := formatter.Format(strings.ToLower(c.options.Output)) if err := formatter.FormatRoles(roles, format, c.options.ShowAll); err != nil { @@ -92,4 +63,4 @@ func (c *ListCommand) Execute(ctx context.Context) error { } return nil -} \ No newline at end of file +} diff --git a/cmd/hawkling/commands/prune.go b/cmd/hawkling/commands/prune.go index fe4c69c..213237a 100644 --- a/cmd/hawkling/commands/prune.go +++ b/cmd/hawkling/commands/prune.go @@ -1,10 +1,8 @@ package commands import ( - "bufio" "context" "fmt" - "os" "strings" "hawkling/pkg/aws" @@ -48,13 +46,13 @@ func (c *PruneCommand) Execute(ctx context.Context) error { } // Find unused roles based on the specified days - var unusedRoles []aws.Role - for _, role := range roles { - if role.IsUnused(c.options.Days) { - unusedRoles = append(unusedRoles, role) - } + filterOptions := aws.FilterOptions{ + Days: c.options.Days, + OnlyUnused: true, } + unusedRoles := aws.FilterRoles(roles, filterOptions) + if len(unusedRoles) == 0 { fmt.Println("No unused IAM roles found") return nil @@ -74,16 +72,13 @@ func (c *PruneCommand) Execute(ctx context.Context) error { // Confirm deletion if force flag is not set if !c.options.Force { - fmt.Printf("\nAre you sure you want to delete %d unused roles? This cannot be undone. [y/N]: ", len(unusedRoles)) - - reader := bufio.NewReader(os.Stdin) - response, err := reader.ReadString('\n') + prompt := fmt.Sprintf("\nAre you sure you want to delete %d unused roles? This cannot be undone. [y/N]: ", len(unusedRoles)) + confirmed, err := ConfirmAction(prompt) if err != nil { return errors.Wrap(err, "failed to read confirmation") } - response = strings.TrimSpace(strings.ToLower(response)) - if response != "y" && response != "yes" { + if !confirmed { fmt.Println("Deletion cancelled") return nil } diff --git a/cmd/hawkling/main.go b/cmd/hawkling/main.go index 5c9ba6d..b3453d9 100644 --- a/cmd/hawkling/main.go +++ b/cmd/hawkling/main.go @@ -84,14 +84,16 @@ Complete documentation is available at https://github.com/watany-dev/hawkling`, // Prune command var pruneDays int + var pruneOnlyUnused bool pruneCmd := &cobra.Command{ Use: "prune", - Short: "Delete all unused IAM roles", + Short: "Delete IAM roles based on specified criteria", RunE: func(cmd *cobra.Command, args []string) error { pruneOptions := commands.PruneOptions{ - Days: pruneDays, - DryRun: dryRun, - Force: force, + Days: pruneDays, + DryRun: dryRun, + Force: force, + OnlyUnused: pruneOnlyUnused, } pruneCmd := commands.NewPruneCommand(profile, region, pruneOptions) @@ -99,6 +101,7 @@ Complete documentation is available at https://github.com/watany-dev/hawkling`, }, } commands.AddPruneFlags(pruneCmd, &pruneDays, &dryRun, &force) + pruneCmd.Flags().BoolVar(&pruneOnlyUnused, "unused", false, "Delete only unused roles") // Add commands to root command rootCmd.AddCommand(listCmd, deleteCmd, pruneCmd) diff --git a/pkg/aws/filter.go b/pkg/aws/filter.go new file mode 100644 index 0000000..4c8f12f --- /dev/null +++ b/pkg/aws/filter.go @@ -0,0 +1,46 @@ +package aws + +// FilterOptions contains various filtering criteria +type FilterOptions struct { + Days int + OnlyUsed bool + OnlyUnused bool +} + +// FilterRoles filters roles based on specified options +// This unified implementation follows these rules: +// - Days=0: No days-based filtering +// - OnlyUsed=true: Show only roles that have been used at least once +// - OnlyUnused=true: Show only roles that have never been used +// - Days>0: Show roles not used in the specified days +// - Days>0 + OnlyUsed: Show roles that have been used at least once but not in the specified days +// - Days>0 + OnlyUnused: Show roles that have never been used (days parameter doesn't affect these) +func FilterRoles(roles []Role, options FilterOptions) []Role { + // If both filters are enabled, return empty list (logical conflict) + if options.OnlyUsed && options.OnlyUnused { + return []Role{} + } + + filteredRoles := make([]Role, 0, len(roles)) + + for _, role := range roles { + // OnlyUnused: Include only roles that have never been used (LastUsed == nil) + if options.OnlyUnused && role.LastUsed != nil { + continue + } + + // OnlyUsed: Include only roles that have been used at least once (LastUsed != nil) + if options.OnlyUsed && role.LastUsed == nil { + continue + } + + // Days filter: For roles not matching OnlyUnused, apply the days filter + if options.Days > 0 && !options.OnlyUnused && !role.IsUnused(options.Days) { + continue + } + + filteredRoles = append(filteredRoles, role) + } + + return filteredRoles +} diff --git a/test/list_filter_test.go b/test/list_filter_test.go index 9584819..a21ca0f 100644 --- a/test/list_filter_test.go +++ b/test/list_filter_test.go @@ -107,43 +107,15 @@ func TestFilterRoles(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - // Apply filter - var filteredRoles []aws.Role - - // If both filters are enabled, return empty list (logical conflict) - if test.onlyUsed && test.onlyUnused { - // Leave filteredRoles empty - } else { - // Move threshold calculation outside the loop - var threshold time.Time - if test.days > 0 { - threshold = time.Now().AddDate(0, 0, -test.days) - } - - filteredRoles = make([]aws.Role, 0, len(roles)) - - for _, role := range roles { - // OnlyUsed: Exclude roles that have never been used (LastUsed == nil) - if test.onlyUsed && role.LastUsed == nil { - continue - } - - // OnlyUnused: Exclude roles that have been used at least once (LastUsed != nil) - if test.onlyUnused && role.LastUsed != nil { - continue - } - - // Days filter: Exclude roles that have not been used within the specified days - if test.days > 0 && role.LastUsed != nil { - if !role.LastUsed.Before(threshold) { - continue - } - } - - filteredRoles = append(filteredRoles, role) - } + // Apply filter using unified filtering logic + filterOptions := aws.FilterOptions{ + Days: test.days, + OnlyUsed: test.onlyUsed, + OnlyUnused: test.onlyUnused, } + filteredRoles := aws.FilterRoles(roles, filterOptions) + // Check if filtered roles match expected if len(filteredRoles) != len(test.expectedRoles) { t.Errorf("Expected %d roles, got %d", len(test.expectedRoles), len(filteredRoles)) diff --git a/test/unused_roles_test.go b/test/unused_roles_test.go index 26356ff..8584aa1 100644 --- a/test/unused_roles_test.go +++ b/test/unused_roles_test.go @@ -21,7 +21,7 @@ func TestFilteringLogic(t *testing.T) { { name: "No filtering", options: commands.ListOptions{ - Days: 90, + Days: 0, // Changed from 90 to 0 - no days filter OnlyUsed: false, OnlyUnused: false, }, @@ -29,71 +29,83 @@ func TestFilteringLogic(t *testing.T) { expectedRoles: []string{"ActiveRole", "InactiveRole", "NeverUsedRole"}, }, { - name: "Only used roles - 90 days", + name: "Only used roles - no days", options: commands.ListOptions{ - Days: 90, + Days: 0, OnlyUsed: true, OnlyUnused: false, }, + expectedCount: 2, + expectedRoles: []string{"ActiveRole", "InactiveRole"}, + }, + { + name: "Only unused roles - no days", + options: commands.ListOptions{ + Days: 0, + OnlyUsed: false, + OnlyUnused: true, + }, expectedCount: 1, - expectedRoles: []string{"ActiveRole"}, + expectedRoles: []string{"NeverUsedRole"}, }, { - name: "Only unused roles - 90 days", + name: "Days filter with 90 days", options: commands.ListOptions{ Days: 90, OnlyUsed: false, - OnlyUnused: true, + OnlyUnused: false, }, expectedCount: 2, expectedRoles: []string{"InactiveRole", "NeverUsedRole"}, }, { - name: "Only used roles - 3 days", + name: "Days filter with 3 days", options: commands.ListOptions{ Days: 3, + OnlyUsed: false, + OnlyUnused: false, + }, + expectedCount: 3, + expectedRoles: []string{"ActiveRole", "InactiveRole", "NeverUsedRole"}, + }, + { + name: "Days filter with Used filter - 90 days", + options: commands.ListOptions{ + Days: 90, OnlyUsed: true, OnlyUnused: false, }, - expectedCount: 0, // None were used within 3 days - expectedRoles: []string{}, + expectedCount: 1, + expectedRoles: []string{"InactiveRole"}, }, { - name: "Only unused roles - 3 days", + name: "Days filter with Unused filter", options: commands.ListOptions{ - Days: 3, + Days: 90, OnlyUsed: false, OnlyUnused: true, }, - expectedCount: 3, // All were unused for 3 days - expectedRoles: []string{"ActiveRole", "InactiveRole", "NeverUsedRole"}, + expectedCount: 1, + expectedRoles: []string{"NeverUsedRole"}, }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - var filteredRoles []aws.Role - - // Apply the improved filtering logic - for _, role := range roles { - isUnusedForDays := role.IsUnused(test.options.Days) - - // OnlyUsed: Show only roles that have been used (LastUsed != nil) and were used within the specified days - if test.options.OnlyUsed && (role.LastUsed == nil || isUnusedForDays) { - continue - } - - // OnlyUnused: Show only roles that have not been used within the specified days - if test.options.OnlyUnused && !isUnusedForDays { - continue - } - - filteredRoles = append(filteredRoles, role) + // Apply filter using common filtering logic + filterOptions := aws.FilterOptions{ + Days: test.options.Days, + OnlyUsed: test.options.OnlyUsed, + OnlyUnused: test.options.OnlyUnused, } + filteredRoles := aws.FilterRoles(roles, filterOptions) + // Check count if len(filteredRoles) != test.expectedCount { t.Errorf("Expected %d roles, got %d", test.expectedCount, len(filteredRoles)) + t.Logf("Expected: %v", test.expectedRoles) + t.Logf("Got: %v", getRoleNames(filteredRoles)) } // Check if expected roles are in the result From ec8e234ff01eb5943e493dff7a4bb1da5007fe3e Mon Sep 17 00:00:00 2001 From: watany <76135106+watany-dev@users.noreply.github.com> Date: Fri, 11 Apr 2025 17:06:26 +0000 Subject: [PATCH 7/8] used/unused --- cmd/hawkling/commands/prune.go | 49 +++++++++++++++++++++++----------- cmd/hawkling/main.go | 3 +++ 2 files changed, 37 insertions(+), 15 deletions(-) diff --git a/cmd/hawkling/commands/prune.go b/cmd/hawkling/commands/prune.go index 213237a..3e03ae7 100644 --- a/cmd/hawkling/commands/prune.go +++ b/cmd/hawkling/commands/prune.go @@ -11,9 +11,11 @@ import ( // PruneOptions contains options for the prune command type PruneOptions struct { - Days int - DryRun bool - Force bool + Days int + DryRun bool + Force bool + OnlyUnused bool + OnlyUsed bool } // PruneCommand represents the prune command @@ -45,22 +47,39 @@ func (c *PruneCommand) Execute(ctx context.Context) error { return errors.Wrap(err, "failed to list roles") } - // Find unused roles based on the specified days + // Find roles based on the specified options filterOptions := aws.FilterOptions{ Days: c.options.Days, - OnlyUnused: true, + OnlyUnused: c.options.OnlyUnused, + OnlyUsed: c.options.OnlyUsed, } - unusedRoles := aws.FilterRoles(roles, filterOptions) + filteredRoles := aws.FilterRoles(roles, filterOptions) - if len(unusedRoles) == 0 { - fmt.Println("No unused IAM roles found") + if len(filteredRoles) == 0 { + fmt.Println("No IAM roles found matching criteria") return nil } - // Show unused roles - fmt.Printf("Found %d unused IAM roles (not used in the last %d days):\n", len(unusedRoles), c.options.Days) - for i, role := range unusedRoles { + // Show filtered roles + message := "Found %d IAM roles" + if c.options.OnlyUnused { + message = "Found %d unused IAM roles (not used in the last %d days)" + } else if c.options.OnlyUsed { + message = "Found %d used IAM roles" + if c.options.Days > 0 { + message += " (not used in the last %d days)" + } + } else if c.options.Days > 0 { + message = "Found %d IAM roles (not used in the last %d days)" + } + + if strings.Contains(message, "%d days") { + fmt.Printf(message+":\n", len(filteredRoles), c.options.Days) + } else { + fmt.Printf(message+":\n", len(filteredRoles)) + } + for i, role := range filteredRoles { fmt.Printf("%d. %s\n", i+1, role.Name) } @@ -72,7 +91,7 @@ func (c *PruneCommand) Execute(ctx context.Context) error { // Confirm deletion if force flag is not set if !c.options.Force { - prompt := fmt.Sprintf("\nAre you sure you want to delete %d unused roles? This cannot be undone. [y/N]: ", len(unusedRoles)) + prompt := fmt.Sprintf("\nAre you sure you want to delete %d roles? This cannot be undone. [y/N]: ", len(filteredRoles)) confirmed, err := ConfirmAction(prompt) if err != nil { return errors.Wrap(err, "failed to read confirmation") @@ -84,9 +103,9 @@ func (c *PruneCommand) Execute(ctx context.Context) error { } } - // Delete the unused roles + // Delete the filtered roles var failedRoles []string - for _, role := range unusedRoles { + for _, role := range filteredRoles { if err := client.DeleteRole(ctx, role.Name); err != nil { failedRoles = append(failedRoles, role.Name) fmt.Printf("Failed to delete role %s: %v\n", role.Name, err) @@ -100,6 +119,6 @@ func (c *PruneCommand) Execute(ctx context.Context) error { return errors.Errorf("failed to delete %d roles", len(failedRoles)) } - fmt.Printf("\nSuccessfully deleted %d unused IAM roles\n", len(unusedRoles)) + fmt.Printf("\nSuccessfully deleted %d IAM roles\n", len(filteredRoles)) return nil } diff --git a/cmd/hawkling/main.go b/cmd/hawkling/main.go index b3453d9..6d96991 100644 --- a/cmd/hawkling/main.go +++ b/cmd/hawkling/main.go @@ -85,6 +85,7 @@ Complete documentation is available at https://github.com/watany-dev/hawkling`, // Prune command var pruneDays int var pruneOnlyUnused bool + var pruneOnlyUsed bool pruneCmd := &cobra.Command{ Use: "prune", Short: "Delete IAM roles based on specified criteria", @@ -94,6 +95,7 @@ Complete documentation is available at https://github.com/watany-dev/hawkling`, DryRun: dryRun, Force: force, OnlyUnused: pruneOnlyUnused, + OnlyUsed: pruneOnlyUsed, } pruneCmd := commands.NewPruneCommand(profile, region, pruneOptions) @@ -102,6 +104,7 @@ Complete documentation is available at https://github.com/watany-dev/hawkling`, } commands.AddPruneFlags(pruneCmd, &pruneDays, &dryRun, &force) pruneCmd.Flags().BoolVar(&pruneOnlyUnused, "unused", false, "Delete only unused roles") + pruneCmd.Flags().BoolVar(&pruneOnlyUsed, "used", false, "Delete only used roles") // Add commands to root command rootCmd.AddCommand(listCmd, deleteCmd, pruneCmd) From e71387c90d413cf95d1860a4d4935207b68a3a03 Mon Sep 17 00:00:00 2001 From: watany <76135106+watany-dev@users.noreply.github.com> Date: Mon, 14 Apr 2025 02:43:38 +0000 Subject: [PATCH 8/8] filter options --- cmd/hawkling/commands/common.go | 7 +++ cmd/hawkling/commands/list.go | 14 +++--- cmd/hawkling/commands/prune.go | 24 +++++----- cmd/hawkling/main.go | 24 ++++++---- test/list_command_test.go | 84 +++++++++++++++++++-------------- test/unused_roles_test.go | 62 ++++++++++++++---------- 6 files changed, 125 insertions(+), 90 deletions(-) diff --git a/cmd/hawkling/commands/common.go b/cmd/hawkling/commands/common.go index 4e59ea9..96613d6 100644 --- a/cmd/hawkling/commands/common.go +++ b/cmd/hawkling/commands/common.go @@ -11,6 +11,13 @@ import ( "hawkling/pkg/errors" ) +// FilterOptions contains common filtering options used across commands +type FilterOptions struct { + Days int + OnlyUsed bool + OnlyUnused bool +} + // ConfirmAction prompts the user for confirmation and returns their response func ConfirmAction(prompt string) (bool, error) { fmt.Print(prompt) diff --git a/cmd/hawkling/commands/list.go b/cmd/hawkling/commands/list.go index 8961610..0053721 100644 --- a/cmd/hawkling/commands/list.go +++ b/cmd/hawkling/commands/list.go @@ -11,11 +11,9 @@ import ( // ListOptions contains options for the list command type ListOptions struct { - Days int - Output string - ShowAll bool - OnlyUsed bool - OnlyUnused bool + FilterOptions + Output string + ShowAll bool } // ListCommand represents the list command @@ -48,9 +46,9 @@ func (c *ListCommand) Execute(ctx context.Context) error { // Filter roles if needed filterOptions := aws.FilterOptions{ - Days: c.options.Days, - OnlyUsed: c.options.OnlyUsed, - OnlyUnused: c.options.OnlyUnused, + Days: c.options.FilterOptions.Days, + OnlyUsed: c.options.FilterOptions.OnlyUsed, + OnlyUnused: c.options.FilterOptions.OnlyUnused, } // Use unified filter implementation diff --git a/cmd/hawkling/commands/prune.go b/cmd/hawkling/commands/prune.go index 3e03ae7..df192e5 100644 --- a/cmd/hawkling/commands/prune.go +++ b/cmd/hawkling/commands/prune.go @@ -11,11 +11,9 @@ import ( // PruneOptions contains options for the prune command type PruneOptions struct { - Days int - DryRun bool - Force bool - OnlyUnused bool - OnlyUsed bool + FilterOptions + DryRun bool + Force bool } // PruneCommand represents the prune command @@ -49,9 +47,9 @@ func (c *PruneCommand) Execute(ctx context.Context) error { // Find roles based on the specified options filterOptions := aws.FilterOptions{ - Days: c.options.Days, - OnlyUnused: c.options.OnlyUnused, - OnlyUsed: c.options.OnlyUsed, + Days: c.options.FilterOptions.Days, + OnlyUnused: c.options.FilterOptions.OnlyUnused, + OnlyUsed: c.options.FilterOptions.OnlyUsed, } filteredRoles := aws.FilterRoles(roles, filterOptions) @@ -63,19 +61,19 @@ func (c *PruneCommand) Execute(ctx context.Context) error { // Show filtered roles message := "Found %d IAM roles" - if c.options.OnlyUnused { + if c.options.FilterOptions.OnlyUnused { message = "Found %d unused IAM roles (not used in the last %d days)" - } else if c.options.OnlyUsed { + } else if c.options.FilterOptions.OnlyUsed { message = "Found %d used IAM roles" - if c.options.Days > 0 { + if c.options.FilterOptions.Days > 0 { message += " (not used in the last %d days)" } - } else if c.options.Days > 0 { + } else if c.options.FilterOptions.Days > 0 { message = "Found %d IAM roles (not used in the last %d days)" } if strings.Contains(message, "%d days") { - fmt.Printf(message+":\n", len(filteredRoles), c.options.Days) + fmt.Printf(message+":\n", len(filteredRoles), c.options.FilterOptions.Days) } else { fmt.Printf(message+":\n", len(filteredRoles)) } diff --git a/cmd/hawkling/main.go b/cmd/hawkling/main.go index 6d96991..0876dc1 100644 --- a/cmd/hawkling/main.go +++ b/cmd/hawkling/main.go @@ -50,11 +50,13 @@ Complete documentation is available at https://github.com/watany-dev/hawkling`, Short: "List IAM roles, optionally filtering for unused roles", RunE: func(cmd *cobra.Command, args []string) error { listOptions := commands.ListOptions{ - Days: listDays, - Output: output, - ShowAll: showAllInfo, - OnlyUsed: onlyUsed, - OnlyUnused: onlyUnused, + FilterOptions: commands.FilterOptions{ + Days: listDays, + OnlyUsed: onlyUsed, + OnlyUnused: onlyUnused, + }, + Output: output, + ShowAll: showAllInfo, } listCmd := commands.NewListCommand(profile, region, listOptions) @@ -91,11 +93,13 @@ Complete documentation is available at https://github.com/watany-dev/hawkling`, Short: "Delete IAM roles based on specified criteria", RunE: func(cmd *cobra.Command, args []string) error { pruneOptions := commands.PruneOptions{ - Days: pruneDays, - DryRun: dryRun, - Force: force, - OnlyUnused: pruneOnlyUnused, - OnlyUsed: pruneOnlyUsed, + FilterOptions: commands.FilterOptions{ + Days: pruneDays, + OnlyUnused: pruneOnlyUnused, + OnlyUsed: pruneOnlyUsed, + }, + DryRun: dryRun, + Force: force, } pruneCmd := commands.NewPruneCommand(profile, region, pruneOptions) diff --git a/test/list_command_test.go b/test/list_command_test.go index 4b17be8..fe660fb 100644 --- a/test/list_command_test.go +++ b/test/list_command_test.go @@ -46,11 +46,13 @@ func TestListCommandFilterFlags(t *testing.T) { { name: "No filters - should return all roles", options: commands.ListOptions{ - Days: 0, - Output: "table", - ShowAll: false, - OnlyUsed: false, - OnlyUnused: false, + FilterOptions: commands.FilterOptions{ + Days: 0, + OnlyUsed: false, + OnlyUnused: false, + }, + Output: "table", + ShowAll: false, }, expectedRoles: 3, // All 3 roles from the mock shouldContain: []string{"ActiveRole", "InactiveRole", "NeverUsedRole"}, @@ -59,11 +61,13 @@ func TestListCommandFilterFlags(t *testing.T) { { name: "Used filter - should return only roles that were used at least once", options: commands.ListOptions{ - Days: 0, - Output: "table", - ShowAll: false, - OnlyUsed: true, - OnlyUnused: false, + FilterOptions: commands.FilterOptions{ + Days: 0, + OnlyUsed: true, + OnlyUnused: false, + }, + Output: "table", + ShowAll: false, }, expectedRoles: 2, // ActiveRole and InactiveRole have been used shouldContain: []string{"ActiveRole", "InactiveRole"}, @@ -72,11 +76,13 @@ func TestListCommandFilterFlags(t *testing.T) { { name: "Unused filter - should return only roles that were never used", options: commands.ListOptions{ - Days: 0, - Output: "table", - ShowAll: false, - OnlyUsed: false, - OnlyUnused: true, + FilterOptions: commands.FilterOptions{ + Days: 0, + OnlyUsed: false, + OnlyUnused: true, + }, + Output: "table", + ShowAll: false, }, expectedRoles: 1, // Only NeverUsedRole was never used shouldContain: []string{"NeverUsedRole"}, @@ -85,11 +91,13 @@ func TestListCommandFilterFlags(t *testing.T) { { name: "Both filters - should return no roles (conflicting filters)", options: commands.ListOptions{ - Days: 0, - Output: "table", - ShowAll: false, - OnlyUsed: true, - OnlyUnused: true, + FilterOptions: commands.FilterOptions{ + Days: 0, + OnlyUsed: true, + OnlyUnused: true, + }, + Output: "table", + ShowAll: false, }, expectedRoles: 0, // No roles match both used and unused shouldContain: []string{}, @@ -98,11 +106,13 @@ func TestListCommandFilterFlags(t *testing.T) { { name: "Days filter with 90 days - should return roles not used in 90 days", options: commands.ListOptions{ - Days: 90, - Output: "table", - ShowAll: false, - OnlyUsed: false, - OnlyUnused: false, + FilterOptions: commands.FilterOptions{ + Days: 90, + OnlyUsed: false, + OnlyUnused: false, + }, + Output: "table", + ShowAll: false, }, expectedRoles: 2, // InactiveRole was used 100 days ago, NeverUsedRole was never used shouldContain: []string{"InactiveRole", "NeverUsedRole"}, @@ -111,11 +121,13 @@ func TestListCommandFilterFlags(t *testing.T) { { name: "Days filter with Used filter - should return used roles not used in 90 days", options: commands.ListOptions{ - Days: 90, - Output: "table", - ShowAll: false, - OnlyUsed: true, - OnlyUnused: false, + FilterOptions: commands.FilterOptions{ + Days: 90, + OnlyUsed: true, + OnlyUnused: false, + }, + Output: "table", + ShowAll: false, }, expectedRoles: 1, // Only InactiveRole was used but not in last 90 days shouldContain: []string{"InactiveRole"}, @@ -124,11 +136,13 @@ func TestListCommandFilterFlags(t *testing.T) { { name: "Days filter with Unused filter - should return never used roles", options: commands.ListOptions{ - Days: 90, - Output: "table", - ShowAll: false, - OnlyUsed: false, - OnlyUnused: true, + FilterOptions: commands.FilterOptions{ + Days: 90, + OnlyUsed: false, + OnlyUnused: true, + }, + Output: "table", + ShowAll: false, }, expectedRoles: 1, // Only NeverUsedRole was never used shouldContain: []string{"NeverUsedRole"}, diff --git a/test/unused_roles_test.go b/test/unused_roles_test.go index 8584aa1..2ca3b84 100644 --- a/test/unused_roles_test.go +++ b/test/unused_roles_test.go @@ -21,9 +21,11 @@ func TestFilteringLogic(t *testing.T) { { name: "No filtering", options: commands.ListOptions{ - Days: 0, // Changed from 90 to 0 - no days filter - OnlyUsed: false, - OnlyUnused: false, + FilterOptions: commands.FilterOptions{ + Days: 0, // Changed from 90 to 0 - no days filter + OnlyUsed: false, + OnlyUnused: false, + }, }, expectedCount: 3, expectedRoles: []string{"ActiveRole", "InactiveRole", "NeverUsedRole"}, @@ -31,9 +33,11 @@ func TestFilteringLogic(t *testing.T) { { name: "Only used roles - no days", options: commands.ListOptions{ - Days: 0, - OnlyUsed: true, - OnlyUnused: false, + FilterOptions: commands.FilterOptions{ + Days: 0, + OnlyUsed: true, + OnlyUnused: false, + }, }, expectedCount: 2, expectedRoles: []string{"ActiveRole", "InactiveRole"}, @@ -41,9 +45,11 @@ func TestFilteringLogic(t *testing.T) { { name: "Only unused roles - no days", options: commands.ListOptions{ - Days: 0, - OnlyUsed: false, - OnlyUnused: true, + FilterOptions: commands.FilterOptions{ + Days: 0, + OnlyUsed: false, + OnlyUnused: true, + }, }, expectedCount: 1, expectedRoles: []string{"NeverUsedRole"}, @@ -51,9 +57,11 @@ func TestFilteringLogic(t *testing.T) { { name: "Days filter with 90 days", options: commands.ListOptions{ - Days: 90, - OnlyUsed: false, - OnlyUnused: false, + FilterOptions: commands.FilterOptions{ + Days: 90, + OnlyUsed: false, + OnlyUnused: false, + }, }, expectedCount: 2, expectedRoles: []string{"InactiveRole", "NeverUsedRole"}, @@ -61,9 +69,11 @@ func TestFilteringLogic(t *testing.T) { { name: "Days filter with 3 days", options: commands.ListOptions{ - Days: 3, - OnlyUsed: false, - OnlyUnused: false, + FilterOptions: commands.FilterOptions{ + Days: 3, + OnlyUsed: false, + OnlyUnused: false, + }, }, expectedCount: 3, expectedRoles: []string{"ActiveRole", "InactiveRole", "NeverUsedRole"}, @@ -71,9 +81,11 @@ func TestFilteringLogic(t *testing.T) { { name: "Days filter with Used filter - 90 days", options: commands.ListOptions{ - Days: 90, - OnlyUsed: true, - OnlyUnused: false, + FilterOptions: commands.FilterOptions{ + Days: 90, + OnlyUsed: true, + OnlyUnused: false, + }, }, expectedCount: 1, expectedRoles: []string{"InactiveRole"}, @@ -81,9 +93,11 @@ func TestFilteringLogic(t *testing.T) { { name: "Days filter with Unused filter", options: commands.ListOptions{ - Days: 90, - OnlyUsed: false, - OnlyUnused: true, + FilterOptions: commands.FilterOptions{ + Days: 90, + OnlyUsed: false, + OnlyUnused: true, + }, }, expectedCount: 1, expectedRoles: []string{"NeverUsedRole"}, @@ -94,9 +108,9 @@ func TestFilteringLogic(t *testing.T) { t.Run(test.name, func(t *testing.T) { // Apply filter using common filtering logic filterOptions := aws.FilterOptions{ - Days: test.options.Days, - OnlyUsed: test.options.OnlyUsed, - OnlyUnused: test.options.OnlyUnused, + Days: test.options.FilterOptions.Days, + OnlyUsed: test.options.FilterOptions.OnlyUsed, + OnlyUnused: test.options.FilterOptions.OnlyUnused, } filteredRoles := aws.FilterRoles(roles, filterOptions)