diff --git a/README.md b/README.md index 1414203..a8afc48 100644 --- a/README.md +++ b/README.md @@ -396,6 +396,8 @@ GithubOrganization labels: | `repo-guard.cloudoperators.dev/removeTeam` | "true"/"false" | Allows the controller to remove teams that are out of policy. | Disabled (must be "true" to remove) | | `repo-guard.cloudoperators.dev/addRepositoryTeam` | "true"/"false" | Allows setting default team permissions on repositories. | Disabled (must be "true" to add) | | `repo-guard.cloudoperators.dev/removeRepositoryTeam` | "true"/"false" | Allows removing default team permissions from repositories. | Disabled (must be "true" to remove) | +| `repo-guard.cloudoperators.dev/removeOrganizationMember` | "true"/"dryRun"/"false" | Allows the controller to remove org members who are not in any GitHub team. Use `"dryRun"` to calculate and preview pending removals in `.status` without executing them. All team-member lists must be fetched successfully; if any single fetch fails (non-rate-limit error), no removals are generated (safety rail). If the organization has no teams, no removals are generated either. See `spec.protectedMembers` to exempt specific logins. | Disabled (must be "true" to remove) | +| `repo-guard.cloudoperators.dev/removeRepositoryDirectCollaborator` | "true"/"dryRun"/"false" | Allows the controller to remove all direct repository collaborators, so that repository access is managed exclusively through teams. Use `"dryRun"` to calculate and preview pending removals in `.status` without executing them. Org owners and `spec.protectedMembers` logins are exempt. | Disabled (must be "true" to remove) | | `repo-guard.cloudoperators.dev/dryRun` | "true"/"false" | When "true", no changes are made on GitHub; status shows planned operations. | "false" | | `repo-guard.cloudoperators.dev/cleanOperations` | "complete"/"failed" | When in dryRun, set to "complete" to purge completed operations from status, or "failed" to purge failed ones. The label is removed automatically after cleanup. | Not set | | `repo-guard.cloudoperators.dev/failedTTL` | Go duration (e.g., 1h, 30m) | Automatically clears failed operations and failed status after the duration since last status timestamp. | Not set | @@ -403,6 +405,8 @@ GithubOrganization labels: Note: GithubOrganization also supports the annotation `repo-guard.cloudoperators.dev/skipDefaultRepositoryTeams` to skip applying default team permissions on a comma-separated list of repositories. +Note: `spec.protectedMembers` (a list of GitHub logins) exempts specific accounts from both `removeOrganizationMember` and `removeRepositoryDirectCollaborator`. Use it to protect bot accounts, the GitHub App installation user, and any emergency escape-hatch accounts. + GithubTeam labels: | Key | Allowed values | Description | Default | diff --git a/api/v1/githuborganization_types.go b/api/v1/githuborganization_types.go index cacaf9d..d570702 100644 --- a/api/v1/githuborganization_types.go +++ b/api/v1/githuborganization_types.go @@ -21,6 +21,13 @@ type GithubOrganizationSpec struct { DefaultPublicRepositoryTeams []GithubTeamWithPermission `json:"defaultPublicRepositoryTeams,omitempty"` DefaultPrivateRepositoryTeams []GithubTeamWithPermission `json:"defaultPrivateRepositoryTeams,omitempty"` + // ProtectedMembers is a list of GitHub logins that must never be removed by + // the removeOrganizationMember or removeRepositoryDirectCollaborator features. + // Use this to protect bot accounts (including the GitHub App installation user) + // or human escape-hatch accounts. + // +optional + ProtectedMembers []string `json:"protectedMembers,omitempty"` + InstallationID int64 `json:"installationID,omitempty"` } @@ -112,10 +119,37 @@ type GithubOrganizationStatus struct { } type GithubOrganizationStatusOperations struct { - OrganizationOwnerOperations []GithubUserOperation `json:"organizationOwnerOperations,omitempty"` - GithubTeamOperations []GithubTeamOperation `json:"teamOperations,omitempty"` - RepositoryTeamOperations []GithubRepoTeamOperation `json:"repoOperations,omitempty"` + OrganizationOwnerOperations []GithubUserOperation `json:"organizationOwnerOperations,omitempty"` + OrganizationMemberOperations []GithubUserOperation `json:"organizationMemberOperations,omitempty"` + GithubTeamOperations []GithubTeamOperation `json:"teamOperations,omitempty"` + RepositoryTeamOperations []GithubRepoTeamOperation `json:"repoOperations,omitempty"` + RepositoryCollaboratorOperations []GithubRepoUserOperation `json:"repoCollaboratorOperations,omitempty"` +} + +// GithubRepoUserOperation represents a pending/completed operation on a direct +// repository collaborator (e.g., removing a non-team direct collaborator). +type GithubRepoUserOperation struct { + Operation GithubRepoUserOperationType `json:"operation,omitempty"` + Repo string `json:"repo,omitempty"` + User string `json:"user,omitempty"` + State GithubRepoUserOperationState `json:"state,omitempty"` + Error string `json:"error,omitempty"` + Timestamp metav1.Time `json:"timestamp,omitempty"` } + +type GithubRepoUserOperationType string + +const GithubRepoUserOperationTypeRemove GithubRepoUserOperationType = "remove" + +type GithubRepoUserOperationState string + +const ( + GithubRepoUserOperationStatePending GithubRepoUserOperationState = "pending" + GithubRepoUserOperationStateComplete GithubRepoUserOperationState = "complete" + GithubRepoUserOperationStateFailed GithubRepoUserOperationState = "failed" + GithubRepoUserOperationStateSkipped GithubRepoUserOperationState = "skipped" +) + type GithubOrganizationState string const ( @@ -591,6 +625,12 @@ func (g GithubOrganization) PendingOperationsFound() bool { } } + for _, op := range g.Status.Operations.OrganizationMemberOperations { + if op.State == GithubUserOperationStatePending { + return true + } + } + for _, op := range g.Status.Operations.RepositoryTeamOperations { if op.State == GithubRepoTeamOperationStatePending { return true @@ -602,6 +642,12 @@ func (g GithubOrganization) PendingOperationsFound() bool { } } + for _, op := range g.Status.Operations.RepositoryCollaboratorOperations { + if op.State == GithubRepoUserOperationStatePending { + return true + } + } + return false } @@ -614,6 +660,12 @@ func (g GithubOrganization) FailedOperationsFound() bool { } } + for _, op := range g.Status.Operations.OrganizationMemberOperations { + if op.State == GithubUserOperationStateFailed { + return true + } + } + for _, op := range g.Status.Operations.RepositoryTeamOperations { if op.State == GithubRepoTeamOperationStateFailed { return true @@ -625,6 +677,12 @@ func (g GithubOrganization) FailedOperationsFound() bool { } } + for _, op := range g.Status.Operations.RepositoryCollaboratorOperations { + if op.State == GithubRepoUserOperationStateFailed { + return true + } + } + return false } @@ -643,6 +701,15 @@ func (g GithubOrganization) CleanCompletedOperations() (GithubOrganizationStatus } } + newOrganizationMemberOperations := []GithubUserOperation{} + for _, op := range g.Status.Operations.OrganizationMemberOperations { + if op.State == GithubUserOperationStateComplete { + cleaned = true + } else { + newOrganizationMemberOperations = append(newOrganizationMemberOperations, op) + } + } + newRepositoryTeamOperations := []GithubRepoTeamOperation{} for _, op := range g.Status.Operations.RepositoryTeamOperations { @@ -662,9 +729,20 @@ func (g GithubOrganization) CleanCompletedOperations() (GithubOrganizationStatus } } + newRepositoryCollaboratorOperations := []GithubRepoUserOperation{} + for _, op := range g.Status.Operations.RepositoryCollaboratorOperations { + if op.State == GithubRepoUserOperationStateComplete { + cleaned = true + } else { + newRepositoryCollaboratorOperations = append(newRepositoryCollaboratorOperations, op) + } + } + newStatus.Operations.OrganizationOwnerOperations = newOrganizationOwnerOperations + newStatus.Operations.OrganizationMemberOperations = newOrganizationMemberOperations newStatus.Operations.RepositoryTeamOperations = newRepositoryTeamOperations newStatus.Operations.GithubTeamOperations = newGithubTeamOperations + newStatus.Operations.RepositoryCollaboratorOperations = newRepositoryCollaboratorOperations return *newStatus, cleaned @@ -684,6 +762,15 @@ func (g GithubOrganization) CleanFailedOperations() (GithubOrganizationStatus, b } } + newOrganizationMemberOperations := []GithubUserOperation{} + for _, op := range g.Status.Operations.OrganizationMemberOperations { + if op.State == GithubUserOperationStateFailed { + cleaned = true + } else { + newOrganizationMemberOperations = append(newOrganizationMemberOperations, op) + } + } + newRepositoryTeamOperations := []GithubRepoTeamOperation{} for _, op := range g.Status.Operations.RepositoryTeamOperations { @@ -703,9 +790,20 @@ func (g GithubOrganization) CleanFailedOperations() (GithubOrganizationStatus, b } } + newRepositoryCollaboratorOperations := []GithubRepoUserOperation{} + for _, op := range g.Status.Operations.RepositoryCollaboratorOperations { + if op.State == GithubRepoUserOperationStateFailed { + cleaned = true + } else { + newRepositoryCollaboratorOperations = append(newRepositoryCollaboratorOperations, op) + } + } + newStatus.Operations.OrganizationOwnerOperations = newOrganizationOwnerOperations + newStatus.Operations.OrganizationMemberOperations = newOrganizationMemberOperations newStatus.Operations.RepositoryTeamOperations = newRepositoryTeamOperations newStatus.Operations.GithubTeamOperations = newGithubTeamOperations + newStatus.Operations.RepositoryCollaboratorOperations = newRepositoryCollaboratorOperations return *newStatus, cleaned @@ -776,3 +874,155 @@ func (g GithubOrganization) RepoChangeCalculator(exceptions []GithubTeamReposito } const GITHUB_ORG_ANNOTATION_SKIP_DEFAULT_TEAM_REPOSITORY = "repo-guard.cloudoperators.dev/skipDefaultRepositoryTeams" + +// OrganizationMemberChangeCalculator computes remove operations for org members +// that are not in any GitHub team, not an org owner, and not in the protected list. +// +// Safety rail: if teamObservationsCount == 0, no remove operations are generated +// regardless of the member list. This value is 0 either because the org has no +// teams, or because a team-member fetch failed and the controller zeroed the count +// to suppress removals. Both cases prevent mass-removal when team data is incomplete. +func (g *GithubOrganization) OrganizationMemberChangeCalculator( + orgMembers []string, + orgOwners []string, + teamMembers map[string]struct{}, + protected []string, + teamObservationsCount int, +) (bool, GithubOrganizationStatus) { + newStatus := g.Status.DeepCopy() + + // Safety rail: no team data observed — skip generating remove ops + if teamObservationsCount == 0 { + return false, *newStatus + } + + ownerSet := make(map[string]struct{}, len(orgOwners)) + for _, o := range orgOwners { + ownerSet[strings.ToLower(o)] = struct{}{} + } + + protectedSet := make(map[string]struct{}, len(protected)) + for _, p := range protected { + protectedSet[strings.ToLower(p)] = struct{}{} + } + + // Build set of users with existing pending ops to avoid duplicates + pendingSet := make(map[string]struct{}) + for _, op := range g.Status.Operations.OrganizationMemberOperations { + if op.State == GithubUserOperationStatePending || op.State == GithubUserOperationStateFailed { + pendingSet[strings.ToLower(op.User)] = struct{}{} + } + } + + changed := false + for _, member := range orgMembers { + login := strings.ToLower(member) + // skip org owners + if _, isOwner := ownerSet[login]; isOwner { + continue + } + // skip protected members + if _, isProt := protectedSet[login]; isProt { + continue + } + // skip if already in a team + if _, inTeam := teamMembers[login]; inTeam { + continue + } + // skip if already has a pending or failed op + if _, hasPending := pendingSet[login]; hasPending { + continue + } + newStatus.Operations.OrganizationMemberOperations = append( + newStatus.Operations.OrganizationMemberOperations, + GithubUserOperation{ + Operation: GithubUserOperationTypeRemove, + User: member, + State: GithubUserOperationStatePending, + Timestamp: metav1.Now(), + }, + ) + pendingSet[login] = struct{}{} + changed = true + } + + if changed { + newStatus.OrganizationStatus = GithubOrganizationStatePendingOperations + newStatus.OrganizationStatusError = "" + newStatus.OrganizationStatusTimestamp = metav1.Now() + } + + return changed, *newStatus +} + +// RepositoryDirectCollaboratorChangeCalculator computes remove operations for +// all direct repository collaborators, except org owners and protected members. +// Team membership is not considered — the intent is that repository access is +// managed exclusively through teams. +func (g *GithubOrganization) RepositoryDirectCollaboratorChangeCalculator( + repoCollaborators map[string][]string, + orgOwners []string, + protected []string, +) (bool, GithubOrganizationStatus) { + newStatus := g.Status.DeepCopy() + + ownerSet := make(map[string]struct{}, len(orgOwners)) + for _, o := range orgOwners { + ownerSet[strings.ToLower(o)] = struct{}{} + } + + protectedSet := make(map[string]struct{}, len(protected)) + for _, p := range protected { + protectedSet[strings.ToLower(p)] = struct{}{} + } + + // Build set of (repo, user) pairs with existing pending ops to avoid duplicates + type repoUser struct{ repo, user string } + pendingSet := make(map[repoUser]struct{}) + for _, op := range g.Status.Operations.RepositoryCollaboratorOperations { + if op.State == GithubRepoUserOperationStatePending || op.State == GithubRepoUserOperationStateFailed { + pendingSet[repoUser{repo: op.Repo, user: strings.ToLower(op.User)}] = struct{}{} + } + } + + changed := false + for repo, collaborators := range repoCollaborators { + for _, collab := range collaborators { + login := strings.ToLower(collab) + + // skip org owners + if _, isOwner := ownerSet[login]; isOwner { + continue + } + // skip protected members + if _, isProt := protectedSet[login]; isProt { + continue + } + // skip if already has a pending or failed op + if _, hasPending := pendingSet[repoUser{repo: repo, user: login}]; hasPending { + continue + } + + newStatus.Operations.RepositoryCollaboratorOperations = append( + newStatus.Operations.RepositoryCollaboratorOperations, + GithubRepoUserOperation{ + Operation: GithubRepoUserOperationTypeRemove, + Repo: repo, + User: collab, + State: GithubRepoUserOperationStatePending, + Timestamp: metav1.Now(), + }, + ) + pendingSet[repoUser{repo: repo, user: login}] = struct{}{} + changed = true + } + } + + if changed { + newStatus.OrganizationStatus = GithubOrganizationStatePendingOperations + newStatus.OrganizationStatusError = "" + newStatus.OrganizationStatusTimestamp = metav1.Now() + } + + return changed, *newStatus +} diff --git a/api/v1/githuborganization_types_test.go b/api/v1/githuborganization_types_test.go index 6a00836..a6ca44c 100644 --- a/api/v1/githuborganization_types_test.go +++ b/api/v1/githuborganization_types_test.go @@ -7,6 +7,15 @@ import ( "testing" ) +// set builds a map[string]struct{} from a slice of strings. +func set(members ...string) map[string]struct{} { + m := make(map[string]struct{}, len(members)) + for _, s := range members { + m[s] = struct{}{} + } + return m +} + func TestRepoChangeCalculatorMethod(t *testing.T) { repoWithTeam := GithubRepository{ Name: "repo1", @@ -296,3 +305,317 @@ func TestRepoChangeCalculator(t *testing.T) { }) } } + +func TestOrganizationMemberChangeCalculator(t *testing.T) { + tests := []struct { + name string + orgMembers []string + orgOwners []string + teamMembers map[string]struct{} + protected []string + teamObservationsCount int + existingOps []GithubUserOperation + wantChanged bool + wantOpsLen int + wantRemoveUsers []string + }{ + { + name: "member in a team — no op", + orgMembers: []string{"alice"}, + orgOwners: []string{}, + teamMembers: set("alice"), + protected: nil, + teamObservationsCount: 1, + wantChanged: false, + wantOpsLen: 0, + }, + { + name: "member not in any team — remove op queued", + orgMembers: []string{"bob"}, + orgOwners: []string{}, + teamMembers: set(), + protected: nil, + teamObservationsCount: 1, + wantChanged: true, + wantOpsLen: 1, + wantRemoveUsers: []string{"bob"}, + }, + { + name: "member is org owner — no op", + orgMembers: []string{"carol"}, + orgOwners: []string{"carol"}, + teamMembers: set(), + protected: nil, + teamObservationsCount: 1, + wantChanged: false, + wantOpsLen: 0, + }, + { + name: "member is in protected list — no op", + orgMembers: []string{"bot-user"}, + orgOwners: []string{}, + teamMembers: set(), + protected: []string{"bot-user"}, + teamObservationsCount: 1, + wantChanged: false, + wantOpsLen: 0, + }, + { + name: "teamObservationsCount == 0 — safety rail, no ops generated", + orgMembers: []string{"dave", "eve"}, + orgOwners: []string{}, + teamMembers: set(), + protected: nil, + teamObservationsCount: 0, + wantChanged: false, + wantOpsLen: 0, + }, + { + name: "existing pending op for same user — not duplicated", + orgMembers: []string{"frank"}, + orgOwners: []string{}, + teamMembers: set(), + protected: nil, + existingOps: []GithubUserOperation{ + {User: "frank", Operation: GithubUserOperationTypeRemove, State: GithubUserOperationStatePending}, + }, + teamObservationsCount: 1, + wantChanged: false, + wantOpsLen: 1, + }, + { + name: "existing failed op for same user — no new pending op generated", + orgMembers: []string{"grace"}, + orgOwners: []string{}, + teamMembers: set(), + protected: nil, + existingOps: []GithubUserOperation{ + {User: "grace", Operation: GithubUserOperationTypeRemove, State: GithubUserOperationStateFailed}, + }, + teamObservationsCount: 1, + wantChanged: false, + wantOpsLen: 1, + }, + { + name: "mixed: one team member, one non-member — only non-member gets remove op", + orgMembers: []string{"alice", "bob"}, + orgOwners: []string{}, + teamMembers: set("alice"), + protected: nil, + teamObservationsCount: 2, + wantChanged: true, + wantOpsLen: 1, + wantRemoveUsers: []string{"bob"}, + }, + { + name: "case-insensitive: owner stored as uppercase — still skipped", + orgMembers: []string{"Admin"}, + orgOwners: []string{"admin"}, + teamMembers: set(), + protected: nil, + teamObservationsCount: 1, + wantChanged: false, + wantOpsLen: 0, + }, + { + name: "case-insensitive: protected stored as uppercase — still skipped", + orgMembers: []string{"Bot"}, + orgOwners: []string{}, + teamMembers: set(), + protected: []string{"BOT"}, + teamObservationsCount: 1, + wantChanged: false, + wantOpsLen: 0, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + org := &GithubOrganization{} + org.Status.Operations.OrganizationMemberOperations = tt.existingOps + + changed, newStatus := org.OrganizationMemberChangeCalculator( + tt.orgMembers, + tt.orgOwners, + tt.teamMembers, + tt.protected, + tt.teamObservationsCount, + ) + + if changed != tt.wantChanged { + t.Errorf("changed = %v, want %v", changed, tt.wantChanged) + } + + got := newStatus.Operations.OrganizationMemberOperations + if len(got) != tt.wantOpsLen { + t.Fatalf("len(OrganizationMemberOperations) = %d, want %d\ngot: %+v", len(got), tt.wantOpsLen, got) + } + + for _, wantUser := range tt.wantRemoveUsers { + found := false + for _, op := range got { + if op.User == wantUser && op.Operation == GithubUserOperationTypeRemove && op.State == GithubUserOperationStatePending { + found = true + break + } + } + if !found { + t.Errorf("expected remove op for user %q not found in ops: %+v", wantUser, got) + } + } + + if tt.wantChanged && newStatus.OrganizationStatus != GithubOrganizationStatePendingOperations { + t.Errorf("OrganizationStatus = %q, want %q", newStatus.OrganizationStatus, GithubOrganizationStatePendingOperations) + } + }) + } +} + +func TestRepositoryDirectCollaboratorChangeCalculator(t *testing.T) { + tests := []struct { + name string + repoCollaborators map[string][]string + orgOwners []string + protected []string + existingOps []GithubRepoUserOperation + wantChanged bool + wantOpsLen int + wantRemove []struct{ repo, user string } + }{ + { + name: "collaborator — remove op (team membership irrelevant)", + repoCollaborators: map[string][]string{"repo1": {"alice"}}, + orgOwners: nil, + protected: nil, + wantChanged: true, + wantOpsLen: 1, + wantRemove: []struct{ repo, user string }{{"repo1", "alice"}}, + }, + { + name: "collaborator (different user) — remove op", + repoCollaborators: map[string][]string{"repo1": {"bob"}}, + orgOwners: nil, + protected: nil, + wantChanged: true, + wantOpsLen: 1, + wantRemove: []struct{ repo, user string }{{"repo1", "bob"}}, + }, + { + name: "collaborator is org owner — no op", + repoCollaborators: map[string][]string{"repo1": {"carol"}}, + orgOwners: []string{"carol"}, + protected: nil, + wantChanged: false, + wantOpsLen: 0, + }, + { + name: "collaborator is protected — no op", + repoCollaborators: map[string][]string{"repo1": {"deploy-bot"}}, + orgOwners: nil, + protected: []string{"deploy-bot"}, + wantChanged: false, + wantOpsLen: 0, + }, + { + name: "outside collaborator — repo has no teams — remove op", + repoCollaborators: map[string][]string{"repo1": {"outsider"}}, + orgOwners: nil, + protected: nil, + wantChanged: true, + wantOpsLen: 1, + wantRemove: []struct{ repo, user string }{{"repo1", "outsider"}}, + }, + { + name: "multiple repos — all collaborators get ops", + repoCollaborators: map[string][]string{"repo1": {"alice", "dave"}, "repo2": {"eve"}}, + orgOwners: nil, + protected: nil, + wantChanged: true, + wantOpsLen: 3, + wantRemove: []struct{ repo, user string }{ + {"repo1", "alice"}, + {"repo1", "dave"}, + {"repo2", "eve"}, + }, + }, + { + name: "existing pending op for same repo+user — not duplicated", + repoCollaborators: map[string][]string{"repo1": {"frank"}}, + orgOwners: nil, + protected: nil, + existingOps: []GithubRepoUserOperation{ + {Repo: "repo1", User: "frank", Operation: GithubRepoUserOperationTypeRemove, State: GithubRepoUserOperationStatePending}, + }, + wantChanged: false, + wantOpsLen: 1, + }, + { + name: "existing failed op for same repo+user — no new pending op", + repoCollaborators: map[string][]string{"repo1": {"grace"}}, + orgOwners: nil, + protected: nil, + existingOps: []GithubRepoUserOperation{ + {Repo: "repo1", User: "grace", Operation: GithubRepoUserOperationTypeRemove, State: GithubRepoUserOperationStateFailed}, + }, + wantChanged: false, + wantOpsLen: 1, + }, + { + name: "case-insensitive owner match — no op", + repoCollaborators: map[string][]string{"repo1": {"Admin"}}, + orgOwners: []string{"admin"}, + protected: nil, + wantChanged: false, + wantOpsLen: 0, + }, + { + name: "no collaborators — no ops", + repoCollaborators: map[string][]string{}, + orgOwners: nil, + protected: nil, + wantChanged: false, + wantOpsLen: 0, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + org := &GithubOrganization{} + org.Status.Operations.RepositoryCollaboratorOperations = tt.existingOps + + changed, newStatus := org.RepositoryDirectCollaboratorChangeCalculator( + tt.repoCollaborators, + tt.orgOwners, + tt.protected, + ) + + if changed != tt.wantChanged { + t.Errorf("changed = %v, want %v", changed, tt.wantChanged) + } + + got := newStatus.Operations.RepositoryCollaboratorOperations + if len(got) != tt.wantOpsLen { + t.Fatalf("len(RepositoryCollaboratorOperations) = %d, want %d\ngot: %+v", len(got), tt.wantOpsLen, got) + } + + for _, want := range tt.wantRemove { + found := false + for _, op := range got { + if op.Repo == want.repo && op.User == want.user && + op.Operation == GithubRepoUserOperationTypeRemove && + op.State == GithubRepoUserOperationStatePending { + found = true + break + } + } + if !found { + t.Errorf("expected remove op for repo=%q user=%q not found in ops: %+v", want.repo, want.user, got) + } + } + + if tt.wantChanged && newStatus.OrganizationStatus != GithubOrganizationStatePendingOperations { + t.Errorf("OrganizationStatus = %q, want %q", newStatus.OrganizationStatus, GithubOrganizationStatePendingOperations) + } + }) + } +} diff --git a/api/v1/zz_generated.deepcopy.go b/api/v1/zz_generated.deepcopy.go index 027414b..1310204 100644 --- a/api/v1/zz_generated.deepcopy.go +++ b/api/v1/zz_generated.deepcopy.go @@ -550,6 +550,11 @@ func (in *GithubOrganizationSpec) DeepCopyInto(out *GithubOrganizationSpec) { *out = make([]GithubTeamWithPermission, len(*in)) copy(*out, *in) } + if in.ProtectedMembers != nil { + in, out := &in.ProtectedMembers, &out.ProtectedMembers + *out = make([]string, len(*in)) + copy(*out, *in) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new GithubOrganizationSpec. @@ -618,6 +623,13 @@ func (in *GithubOrganizationStatusOperations) DeepCopyInto(out *GithubOrganizati (*in)[i].DeepCopyInto(&(*out)[i]) } } + if in.OrganizationMemberOperations != nil { + in, out := &in.OrganizationMemberOperations, &out.OrganizationMemberOperations + *out = make([]GithubUserOperation, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } if in.GithubTeamOperations != nil { in, out := &in.GithubTeamOperations, &out.GithubTeamOperations *out = make([]GithubTeamOperation, len(*in)) @@ -632,6 +644,13 @@ func (in *GithubOrganizationStatusOperations) DeepCopyInto(out *GithubOrganizati (*in)[i].DeepCopyInto(&(*out)[i]) } } + if in.RepositoryCollaboratorOperations != nil { + in, out := &in.RepositoryCollaboratorOperations, &out.RepositoryCollaboratorOperations + *out = make([]GithubRepoUserOperation, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new GithubOrganizationStatusOperations. @@ -660,6 +679,22 @@ func (in *GithubRepoTeamOperation) DeepCopy() *GithubRepoTeamOperation { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *GithubRepoUserOperation) DeepCopyInto(out *GithubRepoUserOperation) { + *out = *in + in.Timestamp.DeepCopyInto(&out.Timestamp) +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new GithubRepoUserOperation. +func (in *GithubRepoUserOperation) DeepCopy() *GithubRepoUserOperation { + if in == nil { + return nil + } + out := new(GithubRepoUserOperation) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *GithubRepository) DeepCopyInto(out *GithubRepository) { *out = *in diff --git a/charts/repo-guard/crds/githuborganization-crd.yaml b/charts/repo-guard/crds/githuborganization-crd.yaml index 6a7308d..d79928c 100644 --- a/charts/repo-guard/crds/githuborganization-crd.yaml +++ b/charts/repo-guard/crds/githuborganization-crd.yaml @@ -82,6 +82,15 @@ spec: items: type: string type: array + protectedMembers: + description: |- + ProtectedMembers is a list of GitHub logins that must never be removed by + the removeOrganizationMember or removeRepositoryDirectCollaborator features. + Use this to protect bot accounts (including the GitHub App installation user) + or human escape-hatch accounts. + items: + type: string + type: array type: object status: description: GithubOrganizationStatus defines the observed state of GithubOrganization @@ -90,6 +99,22 @@ spec: type: string operations: properties: + organizationMemberOperations: + items: + properties: + error: + type: string + operation: + type: string + state: + type: string + timestamp: + format: date-time + type: string + user: + type: string + type: object + type: array organizationOwnerOperations: items: properties: @@ -106,6 +131,27 @@ spec: type: string type: object type: array + repoCollaboratorOperations: + items: + description: |- + GithubRepoUserOperation represents a pending/completed operation on a direct + repository collaborator (e.g., removing a non-team direct collaborator). + properties: + error: + type: string + operation: + type: string + repo: + type: string + state: + type: string + timestamp: + format: date-time + type: string + user: + type: string + type: object + type: array repoOperations: items: properties: diff --git a/charts/repo-guard/templates/githuborganization.yaml b/charts/repo-guard/templates/githuborganization.yaml index d7542d1..f97d1d5 100644 --- a/charts/repo-guard/templates/githuborganization.yaml +++ b/charts/repo-guard/templates/githuborganization.yaml @@ -14,6 +14,8 @@ metadata: repo-guard.cloudoperators.dev/addRepositoryTeam: "{{ $org.addRepositoryTeam | default "true" }}" repo-guard.cloudoperators.dev/removeRepositoryTeam: "{{ $org.removeRepositoryTeam | default "true" }}" repo-guard.cloudoperators.dev/dryRun: "{{ $org.dryRun | default "false" }}" + repo-guard.cloudoperators.dev/removeOrganizationMember: "{{ $org.removeOrganizationMember | default "false" }}" + repo-guard.cloudoperators.dev/removeRepositoryDirectCollaborator: "{{ $org.removeRepositoryDirectCollaborator | default "false" }}" {{- $ttl := (get $org "ttl") -}} {{- if not (kindIs "map" $ttl) }}{{- $ttl = dict -}}{{- end }} repo-guard.cloudoperators.dev/failedTTL: "{{ ((get $ttl "failed") | default $.Values.ttl.organization.failed) }}" @@ -37,6 +39,10 @@ spec: {{- if and $org.organizationOwnerTeams (gt (len $org.organizationOwnerTeams) 0) }} organizationOwnerTeams: {{ toYaml $org.organizationOwnerTeams | indent 4 }} + {{- end }} + {{- if and $org.protectedMembers (gt (len $org.protectedMembers) 0) }} + protectedMembers: +{{ toYaml $org.protectedMembers | indent 4 }} {{- end }} installationID: {{ $org.installationID | int64 }} --- diff --git a/charts/repo-guard/values.yaml b/charts/repo-guard/values.yaml index 7e9969e..77db744 100644 --- a/charts/repo-guard/values.yaml +++ b/charts/repo-guard/values.yaml @@ -107,6 +107,15 @@ monitoring: # addRepositoryTeam: # removeRepositoryTeam: # dryRun: +# # Opt-in: remove org members that are not in any GitHub team. +# # Values: "true" (calculate + execute), "dryRun" (calculate only, ops appear in status but not executed), "false"/unset (disabled). +# removeOrganizationMember: +# # Opt-in: remove all direct repo collaborators so repository access is managed exclusively through teams. +# # Values: "true" (calculate + execute), "dryRun" (calculate only, ops appear in status but not executed), "false"/unset (disabled). +# removeRepositoryDirectCollaborator: +# # List of GitHub logins that must never be removed by removeOrganizationMember or +# # removeRepositoryDirectCollaborator. Typically includes bot accounts and the GitHub App user. +# protectedMembers: [] # disableInternalUsernames: # # GithubAccountLink email verification settings applied to all GALs # # associated with this GitHub (organization-level policy). diff --git a/config/crd/bases/repo-guard.cloudoperators.dev_githuborganizations.yaml b/config/crd/bases/repo-guard.cloudoperators.dev_githuborganizations.yaml index d777950..ea75bf2 100644 --- a/config/crd/bases/repo-guard.cloudoperators.dev_githuborganizations.yaml +++ b/config/crd/bases/repo-guard.cloudoperators.dev_githuborganizations.yaml @@ -83,6 +83,15 @@ spec: items: type: string type: array + protectedMembers: + description: |- + ProtectedMembers is a list of GitHub logins that must never be removed by + the removeOrganizationMember or removeRepositoryDirectCollaborator features. + Use this to protect bot accounts (including the GitHub App installation user) + or human escape-hatch accounts. + items: + type: string + type: array type: object status: description: GithubOrganizationStatus defines the observed state of GithubOrganization @@ -91,6 +100,22 @@ spec: type: string operations: properties: + organizationMemberOperations: + items: + properties: + error: + type: string + operation: + type: string + state: + type: string + timestamp: + format: date-time + type: string + user: + type: string + type: object + type: array organizationOwnerOperations: items: properties: @@ -107,6 +132,27 @@ spec: type: string type: object type: array + repoCollaboratorOperations: + items: + description: |- + GithubRepoUserOperation represents a pending/completed operation on a direct + repository collaborator (e.g., removing a non-team direct collaborator). + properties: + error: + type: string + operation: + type: string + repo: + type: string + state: + type: string + timestamp: + format: date-time + type: string + user: + type: string + type: object + type: array repoOperations: items: properties: diff --git a/dashboards/repoguard-status-ops-perses.json b/dashboards/repoguard-status-ops-perses.json index 6412660..310d47f 100644 --- a/dashboards/repoguard-status-ops-perses.json +++ b/dashboards/repoguard-status-ops-perses.json @@ -157,6 +157,84 @@ ] } }, + "orgMemberPending": { + "kind": "Panel", + "spec": { + "display": { + "name": "⏳ Org Member Pending Ops" + }, + "plugin": { + "kind": "StatChart", + "spec": { + "calculation": "last-number", + "thresholds": { + "steps": [ + { + "color": "green", + "value": 0 + }, + { + "color": "orange", + "value": 1 + } + ] + } + } + }, + "queries": [ + { + "kind": "TimeSeriesQuery", + "spec": { + "plugin": { + "kind": "PrometheusTimeSeriesQuery", + "spec": { + "query": "sum(repo_guard_githuborganization_operations{scope=\"orgmembers\", state=\"pending\", github=~\"$github\", organization=~\"$org\"}) or vector(0)" + } + } + } + } + ] + } + }, + "repoCollabPending": { + "kind": "Panel", + "spec": { + "display": { + "name": "⏳ Repo Direct Collab Pending Ops" + }, + "plugin": { + "kind": "StatChart", + "spec": { + "calculation": "last-number", + "thresholds": { + "steps": [ + { + "color": "green", + "value": 0 + }, + { + "color": "orange", + "value": 1 + } + ] + } + } + }, + "queries": [ + { + "kind": "TimeSeriesQuery", + "spec": { + "plugin": { + "kind": "PrometheusTimeSeriesQuery", + "spec": { + "query": "sum(repo_guard_githuborganization_operations{scope=\"repocollaborators\", state=\"pending\", github=~\"$github\", organization=~\"$org\"}) or vector(0)" + } + } + } + } + ] + } + }, "orgsAttention": { "kind": "Panel", "spec": { @@ -382,7 +460,7 @@ "kind": "Panel", "spec": { "display": { - "name": "🏢 Team & Repo Operations" + "name": "🏢 Org Operations (Teams, Repos, Members & Collaborators)" }, "plugin": { "kind": "TimeSeriesChart", @@ -399,7 +477,7 @@ "plugin": { "kind": "PrometheusTimeSeriesQuery", "spec": { - "query": "sum by (scope, operation) (increase(repo_guard_githuborganization_operations{scope=~\"teams|repos\", state=\"complete\", github=~\"$github\", organization=~\"$org\"}[5m]))", + "query": "sum by (scope, operation) (increase(repo_guard_githuborganization_operations{scope=~\"teams|repos|orgmembers|repocollaborators\", state=\"complete\", github=~\"$github\", organization=~\"$org\"}[5m]))", "seriesNameFormat": "{{scope}} {{operation}}" } } @@ -591,6 +669,24 @@ { "x": 0, "y": 4, + "width": 6, + "height": 4, + "content": { + "$ref": "#/spec/panels/orgMemberPending" + } + }, + { + "x": 6, + "y": 4, + "width": 6, + "height": 4, + "content": { + "$ref": "#/spec/panels/repoCollabPending" + } + }, + { + "x": 0, + "y": 8, "width": 12, "height": 8, "content": { @@ -599,7 +695,7 @@ }, { "x": 12, - "y": 4, + "y": 8, "width": 12, "height": 8, "content": { @@ -608,7 +704,7 @@ }, { "x": 0, - "y": 12, + "y": 16, "width": 12, "height": 8, "content": { @@ -617,7 +713,7 @@ }, { "x": 12, - "y": 12, + "y": 16, "width": 12, "height": 8, "content": { diff --git a/dashboards/repoguard-status-ops-plutono.json b/dashboards/repoguard-status-ops-plutono.json index 3857b36..0dd4bbc 100644 --- a/dashboards/repoguard-status-ops-plutono.json +++ b/dashboards/repoguard-status-ops-plutono.json @@ -140,6 +140,58 @@ { "expr": "sum(repo_guard_githubteam_operations{state=\"pending\", organization=~\"$org\", team=~\"$team\"}) or vector(0)", "refId": "A" } ] }, + { + "datasource": "${DS_PROMETHEUS}", + "fieldConfig": { + "defaults": { + "color": { "mode": "thresholds" }, + "mappings": [ + { "options": { "0": { "color": "green", "index": 0, "text": "✅ OK" } }, "type": "value" } + ], + "thresholds": { + "mode": "absolute", + "steps": [ + { "color": "green", "value": null }, + { "color": "orange", "value": 1 } + ] + } + } + }, + "gridPos": { "h": 4, "w": 6, "x": 0, "y": 5 }, + "id": 31, + "options": { "colorMode": "value", "graphMode": "area", "justifyMode": "center", "reduceOptions": { "calcs": ["lastNotNull"] }, "textMode": "value_and_name" }, + "title": "⏳ Org Member Pending Ops", + "type": "stat", + "targets": [ + { "expr": "sum(repo_guard_githuborganization_operations{scope=\"orgmembers\", state=\"pending\", github=~\"$github\", organization=~\"$org\"}) or vector(0)", "refId": "A" } + ] + }, + { + "datasource": "${DS_PROMETHEUS}", + "fieldConfig": { + "defaults": { + "color": { "mode": "thresholds" }, + "mappings": [ + { "options": { "0": { "color": "green", "index": 0, "text": "✅ OK" } }, "type": "value" } + ], + "thresholds": { + "mode": "absolute", + "steps": [ + { "color": "green", "value": null }, + { "color": "orange", "value": 1 } + ] + } + } + }, + "gridPos": { "h": 4, "w": 6, "x": 6, "y": 5 }, + "id": 32, + "options": { "colorMode": "value", "graphMode": "area", "justifyMode": "center", "reduceOptions": { "calcs": ["lastNotNull"] }, "textMode": "value_and_name" }, + "title": "⏳ Repo Direct Collab Pending Ops", + "type": "stat", + "targets": [ + { "expr": "sum(repo_guard_githuborganization_operations{scope=\"repocollaborators\", state=\"pending\", github=~\"$github\", organization=~\"$org\"}) or vector(0)", "refId": "A" } + ] + }, { "collapsed": false, "gridPos": { "h": 1, "w": 24, "x": 0, "y": 5 }, @@ -219,10 +271,10 @@ "gridPos": { "h": 8, "w": 12, "x": 12, "y": 15 }, "id": 35, "options": { "legend": { "displayMode": "table", "placement": "bottom" }, "tooltip": { "mode": "multi" } }, - "title": "🏢 Team & Repo Operations (Line Chart)", + "title": "🏢 Org Operations (Teams, Repos, Members & Collaborators) (Line Chart)", "type": "timeseries", "targets": [ - { "expr": "sum by (scope, operation) (increase(repo_guard_githuborganization_operations{scope=~\"teams|repos\", state=\"complete\", github=~\"$github\", organization=~\"$org\"}[5m]))", "legendFormat": "{{scope}} {{operation}}", "refId": "A" } + { "expr": "sum by (scope, operation) (increase(repo_guard_githuborganization_operations{scope=~\"teams|repos|orgmembers|repocollaborators\", state=\"complete\", github=~\"$github\", organization=~\"$org\"}[5m]))", "legendFormat": "{{scope}} {{operation}}", "refId": "A" } ] }, { diff --git a/internal/controller/githuborganization_controller.go b/internal/controller/githuborganization_controller.go index b254b70..8fe6ce5 100644 --- a/internal/controller/githuborganization_controller.go +++ b/internal/controller/githuborganization_controller.go @@ -154,6 +154,10 @@ func (r *GithubOrganizationReconciler) Reconcile(ctx context.Context, req ctrl.R newStatus.Operations.OrganizationOwnerOperations = updated anyChanged = true } + if updated, changed := applyUserOpsTTL(newStatus.Operations.OrganizationMemberOperations, ttl, userState, now); changed { + newStatus.Operations.OrganizationMemberOperations = updated + anyChanged = true + } if updated, changed := applyRepoOpsTTL(newStatus.Operations.RepositoryTeamOperations, ttl, repoState, now); changed { newStatus.Operations.RepositoryTeamOperations = updated anyChanged = true @@ -162,6 +166,20 @@ func (r *GithubOrganizationReconciler) Reconcile(ctx context.Context, req ctrl.R newStatus.Operations.GithubTeamOperations = updated anyChanged = true } + var repoUserState v1.GithubRepoUserOperationState + switch userState { + case v1.GithubUserOperationStateFailed: + repoUserState = v1.GithubRepoUserOperationStateFailed + case v1.GithubUserOperationStateComplete: + repoUserState = v1.GithubRepoUserOperationStateComplete + default: + // No corresponding GithubRepoUserOperationState; skip cleanup for this bucket. + return + } + if updated, changed := applyRepoUserOpsTTL(newStatus.Operations.RepositoryCollaboratorOperations, ttl, repoUserState, now); changed { + newStatus.Operations.RepositoryCollaboratorOperations = updated + anyChanged = true + } } failedTTL := githubOrganization.Labels[GITHUB_ORG_LABEL_FAILED_TTL] @@ -688,6 +706,201 @@ func (r *GithubOrganizationReconciler) Reconcile(ctx context.Context, req ctrl.R return reconcile.Result{}, nil } } + // PART 4: org-member comparison (#147) — remove org members not in any GitHub team + removeOrgMemberLabelValue := "" + if githubOrganization.Labels != nil { + removeOrgMemberLabelValue = githubOrganization.Labels[GITHUB_ORG_LABEL_REMOVE_ORG_MEMBER] + } + if removeOrgMemberLabelValue == GITHUB_ORG_LABEL_REMOVE_ORG_MEMBER_ENABLED_VALUE || removeOrgMemberLabelValue == GITHUB_ORG_LABEL_REMOVE_ORG_MEMBER_DRYRUN_VALUE { + orgMembers, err := organizationsProvider.Members(ctx) + if err != nil { + if t, ok := parseGitHubRateLimitReset(err.Error()); ok { + now := time.Now().UTC() + githubOrganization.Status.OrganizationStatus = v1.GithubOrganizationStateRateLimited + githubOrganization.Status.OrganizationStatusError = "error in getting org members: " + err.Error() + githubOrganization.Status.OrganizationStatusTimestamp = metav1.Now() + newStatus := githubOrganization.Status + uerr := retry.RetryOnConflict(retry.DefaultRetry, func() error { + latest := &v1.GithubOrganization{} + if err := r.Get(ctx, req.NamespacedName, latest); err != nil { + return err + } + latest.Status = newStatus + return r.Client.Status().Update(ctx, latest) + }) + if uerr != nil { + l.Error(uerr, "error during status update") + return reconcile.Result{}, uerr + } + if t.After(now) { + return reconcile.Result{RequeueAfter: t.Sub(now)}, nil + } + return reconcile.Result{Requeue: true}, nil + } + // non-rate-limit error: return so controller-runtime retries with backoff + l.Error(err, "org-member calculator: error fetching org members, requeueing") + return reconcile.Result{}, err + } else { + // Fetch GitHub-side team members for the org-member safety check. + // Reuse teamsList fetched earlier in the reconcile (already validated non-error). + teamMembersUnion := make(map[string]struct{}) + teamObservationsCount := 0 + var teamMembersRateLimitResult *reconcile.Result + var teamMembersRateLimitErr string + for _, team := range teamsList { + members, merr := teamsProvider.Members(ctx, team) + if merr != nil { + if t, ok := parseGitHubRateLimitReset(merr.Error()); ok { + now := time.Now().UTC() + if t.After(now) { + teamMembersRateLimitResult = &reconcile.Result{RequeueAfter: t.Sub(now)} + } else { + teamMembersRateLimitResult = &reconcile.Result{Requeue: true} + } + teamMembersRateLimitErr = merr.Error() + break + } + // Non-rate-limit error: treat as hard stop to avoid false-positive + // org-member removals from an incomplete team-member union. + l.Error(merr, "org-member calculator: error fetching team members, aborting safety check", "team", team) + teamObservationsCount = 0 + break + } + for _, m := range members { + teamMembersUnion[strings.ToLower(m)] = struct{}{} + } + teamObservationsCount++ + } + if teamMembersRateLimitResult != nil { + githubOrganization.Status.OrganizationStatus = v1.GithubOrganizationStateRateLimited + githubOrganization.Status.OrganizationStatusError = "rate limited fetching team members for org-member safety check: " + teamMembersRateLimitErr + githubOrganization.Status.OrganizationStatusTimestamp = metav1.Now() + rlStatus := githubOrganization.Status + if uerr := retry.RetryOnConflict(retry.DefaultRetry, func() error { + latest := &v1.GithubOrganization{} + if err := r.Get(ctx, req.NamespacedName, latest); err != nil { + return err + } + latest.Status = rlStatus + return r.Client.Status().Update(ctx, latest) + }); uerr != nil { + l.Error(uerr, "error during status update") + return reconcile.Result{}, uerr + } + return *teamMembersRateLimitResult, nil + } + + // Build owner login list from extended owner data + orgOwnerLogins := make([]string, 0, len(ownerList)) + for _, o := range ownerList { + orgOwnerLogins = append(orgOwnerLogins, o.Login) + } + + statusChanged, newStatus := githubOrganization.OrganizationMemberChangeCalculator( + orgMembers, + orgOwnerLogins, + teamMembersUnion, + githubOrganization.Spec.ProtectedMembers, + teamObservationsCount, + ) + if statusChanged { + l.Info("status update for organization due to org-member change calculation") + ns := newStatus + err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + latest := &v1.GithubOrganization{} + if err := r.Get(ctx, req.NamespacedName, latest); err != nil { + return err + } + latest.Status = ns + return r.Client.Status().Update(ctx, latest) + }) + if err != nil { + if errors.IsNotFound(err) { + l.Info("resource not found in kubernetes: reconcile is skipped") + return reconcile.Result{}, nil + } + l.Error(err, "error during status update") + return reconcile.Result{}, err + } + return reconcile.Result{}, nil + } + } + } + + // PART 5: repo direct-collaborator comparison (#146) — remove all direct collaborators (team membership is irrelevant) + removeRepoCollabLabelValue := "" + if githubOrganization.Labels != nil { + removeRepoCollabLabelValue = githubOrganization.Labels[GITHUB_ORG_LABEL_REMOVE_REPOSITORY_DIRECT_COLLABORATOR] + } + if removeRepoCollabLabelValue == GITHUB_ORG_LABEL_REMOVE_REPOSITORY_DIRECT_COLLABORATOR_ENABLED_VALUE || removeRepoCollabLabelValue == GITHUB_ORG_LABEL_REMOVE_REPOSITORY_DIRECT_COLLABORATOR_DRYRUN_VALUE { + // Build owner login list from extended owner data + orgOwnerLogins := make([]string, 0, len(ownerList)) + for _, o := range ownerList { + orgOwnerLogins = append(orgOwnerLogins, o.Login) + } + + repoCollaborators := make(map[string][]string) + allRepos := append(publicRepos, privateRepos...) + for _, repo := range allRepos { + collabs, err := reposProvider.RepositoryCollobarators(ctx, repo.Name) + if err != nil { + if t, ok := parseGitHubRateLimitReset(err.Error()); ok { + now := time.Now().UTC() + githubOrganization.Status.OrganizationStatus = v1.GithubOrganizationStateRateLimited + githubOrganization.Status.OrganizationStatusError = "error in getting repo collaborators: " + err.Error() + githubOrganization.Status.OrganizationStatusTimestamp = metav1.Now() + newStatus := githubOrganization.Status + uerr := retry.RetryOnConflict(retry.DefaultRetry, func() error { + latest := &v1.GithubOrganization{} + if err := r.Get(ctx, req.NamespacedName, latest); err != nil { + return err + } + latest.Status = newStatus + return r.Client.Status().Update(ctx, latest) + }) + if uerr != nil { + l.Error(uerr, "error during status update") + return reconcile.Result{}, uerr + } + if t.After(now) { + return reconcile.Result{RequeueAfter: t.Sub(now)}, nil + } + return reconcile.Result{Requeue: true}, nil + } + l.Error(err, "repo-collab calculator: error fetching collaborators, skipping repo", "repo", repo.Name) + continue + } + repoCollaborators[repo.Name] = collabs + } + + statusChanged, newStatus := githubOrganization.RepositoryDirectCollaboratorChangeCalculator( + repoCollaborators, + orgOwnerLogins, + githubOrganization.Spec.ProtectedMembers, + ) + if statusChanged { + l.Info("status update for organization due to repo-collaborator change calculation") + ns := newStatus + err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + latest := &v1.GithubOrganization{} + if err := r.Get(ctx, req.NamespacedName, latest); err != nil { + return err + } + latest.Status = ns + return r.Client.Status().Update(ctx, latest) + }) + if err != nil { + if errors.IsNotFound(err) { + l.Info("resource not found in kubernetes: reconcile is skipped") + return reconcile.Result{}, nil + } + l.Error(err, "error during status update") + return reconcile.Result{}, err + } + return reconcile.Result{}, nil + } + } + // check for empty status in kubernetes resource (for the first run) // no error until here, if there is already error in the status, remove it if githubOrganization.Status.OrganizationStatus == "" { @@ -1072,6 +1285,180 @@ func (r *GithubOrganizationReconciler) Reconcile(ctx context.Context, req ctrl.R } } + // OrganizationMemberOperations (#147) — remove org members not in any team + orgMemberProtectedSet := make(map[string]struct{}, len(githubOrganization.Spec.ProtectedMembers)) + for _, p := range githubOrganization.Spec.ProtectedMembers { + orgMemberProtectedSet[strings.ToLower(p)] = struct{}{} + } + for i, op := range newStatus.Operations.OrganizationMemberOperations { + if op.State != v1.GithubUserOperationStatePending { + continue + } + orgMemberLabel := "" + if githubOrganization.Labels != nil { + orgMemberLabel = githubOrganization.Labels[GITHUB_ORG_LABEL_REMOVE_ORG_MEMBER] + } + if orgMemberLabel == GITHUB_ORG_LABEL_REMOVE_ORG_MEMBER_DRYRUN_VALUE { + // dry-run: keep op pending so operators can inspect who would be removed + l.Info("removing organization members is in dry-run mode: operation left pending (not executed)", "user", op.User) + continue + } + if orgMemberLabel != GITHUB_ORG_LABEL_REMOVE_ORG_MEMBER_ENABLED_VALUE { + l.Info("removing organization members is not enabled: operation skipped", "user", op.User) + newStatus.Operations.OrganizationMemberOperations[i].State = v1.GithubUserOperationStateSkipped + newStatus.Operations.OrganizationMemberOperations[i].Timestamp = metav1.Now() + statusChanged = true + continue + } + if op.Operation != v1.GithubUserOperationTypeRemove { + l.Info("org-member operation: unexpected operation type, skipping", "user", op.User, "operation", op.Operation) + newStatus.Operations.OrganizationMemberOperations[i].State = v1.GithubUserOperationStateSkipped + newStatus.Operations.OrganizationMemberOperations[i].Timestamp = metav1.Now() + statusChanged = true + continue + } + // Re-check protected members at execution time (spec may have changed since the op was queued). + if _, isProt := orgMemberProtectedSet[strings.ToLower(op.User)]; isProt { + l.Info("org-member operation: user is protected, skipping", "user", op.User) + newStatus.Operations.OrganizationMemberOperations[i].State = v1.GithubUserOperationStateSkipped + newStatus.Operations.OrganizationMemberOperations[i].Timestamp = metav1.Now() + statusChanged = true + continue + } + err := organizationsProvider.RemoveFromOrg(ctx, op.User) + if err != nil { + if t, ok := parseGitHubRateLimitReset(err.Error()); ok { + now := time.Now().UTC() + newStatus.OrganizationStatus = v1.GithubOrganizationStateRateLimited + newStatus.OrganizationStatusError = "rate limited during org member removal: " + err.Error() + newStatus.OrganizationStatusTimestamp = metav1.Now() + ns := *newStatus + uerr := retry.RetryOnConflict(retry.DefaultRetry, func() error { + latest := &v1.GithubOrganization{} + if err := r.Get(ctx, req.NamespacedName, latest); err != nil { + return err + } + latest.Status = ns + return r.Client.Status().Update(ctx, latest) + }) + if uerr != nil { + l.Error(uerr, "error during status update") + return reconcile.Result{}, uerr + } + if t.After(now) { + return reconcile.Result{RequeueAfter: t.Sub(now)}, nil + } + return reconcile.Result{Requeue: true}, nil + } + // "user not a member" (404) — treat as success/complete + if strings.Contains(err.Error(), "404") { + l.Info("org member already not a member (404), treating as complete", "user", op.User) + newStatus.Operations.OrganizationMemberOperations[i].State = v1.GithubUserOperationStateComplete + newStatus.Operations.OrganizationMemberOperations[i].Timestamp = metav1.Now() + statusChanged = true + } else { + l.Error(err, "error during removing org member", "user", op.User) + newStatus.Operations.OrganizationMemberOperations[i].State = v1.GithubUserOperationStateFailed + newStatus.Operations.OrganizationMemberOperations[i].Error = err.Error() + newStatus.Operations.OrganizationMemberOperations[i].Timestamp = metav1.Now() + statusChanged = true + failed = true + } + } else { + l.Info("org member removed", "user", op.User) + newStatus.Operations.OrganizationMemberOperations[i].State = v1.GithubUserOperationStateComplete + newStatus.Operations.OrganizationMemberOperations[i].Timestamp = metav1.Now() + statusChanged = true + } + } + + // RepositoryCollaboratorOperations (#146) — remove all direct collaborators (team membership is irrelevant) + repoCollabProtectedSet := make(map[string]struct{}, len(githubOrganization.Spec.ProtectedMembers)) + for _, p := range githubOrganization.Spec.ProtectedMembers { + repoCollabProtectedSet[strings.ToLower(p)] = struct{}{} + } + for i, op := range newStatus.Operations.RepositoryCollaboratorOperations { + if op.State != v1.GithubRepoUserOperationStatePending { + continue + } + repoCollabLabel := "" + if githubOrganization.Labels != nil { + repoCollabLabel = githubOrganization.Labels[GITHUB_ORG_LABEL_REMOVE_REPOSITORY_DIRECT_COLLABORATOR] + } + if repoCollabLabel == GITHUB_ORG_LABEL_REMOVE_REPOSITORY_DIRECT_COLLABORATOR_DRYRUN_VALUE { + // dry-run: keep op pending so operators can inspect who would be removed + l.Info("removing repository direct collaborators is in dry-run mode: operation left pending (not executed)", "repo", op.Repo, "user", op.User) + continue + } + if repoCollabLabel != GITHUB_ORG_LABEL_REMOVE_REPOSITORY_DIRECT_COLLABORATOR_ENABLED_VALUE { + l.Info("removing repository direct collaborators is not enabled: operation skipped", "repo", op.Repo, "user", op.User) + newStatus.Operations.RepositoryCollaboratorOperations[i].State = v1.GithubRepoUserOperationStateSkipped + newStatus.Operations.RepositoryCollaboratorOperations[i].Timestamp = metav1.Now() + statusChanged = true + continue + } + if op.Operation != v1.GithubRepoUserOperationTypeRemove { + l.Info("repo-collab operation: unexpected operation type, skipping", "repo", op.Repo, "user", op.User, "operation", op.Operation) + newStatus.Operations.RepositoryCollaboratorOperations[i].State = v1.GithubRepoUserOperationStateSkipped + newStatus.Operations.RepositoryCollaboratorOperations[i].Timestamp = metav1.Now() + statusChanged = true + continue + } + // Re-check protected members at execution time (spec may have changed since the op was queued). + if _, isProt := repoCollabProtectedSet[strings.ToLower(op.User)]; isProt { + l.Info("repo-collab operation: user is protected, skipping", "repo", op.Repo, "user", op.User) + newStatus.Operations.RepositoryCollaboratorOperations[i].State = v1.GithubRepoUserOperationStateSkipped + newStatus.Operations.RepositoryCollaboratorOperations[i].Timestamp = metav1.Now() + statusChanged = true + continue + } + _, err := reposProvider.RepositoryCollobaratorRemove(ctx, op.Repo, op.User) + if err != nil { + if t, ok := parseGitHubRateLimitReset(err.Error()); ok { + now := time.Now().UTC() + newStatus.OrganizationStatus = v1.GithubOrganizationStateRateLimited + newStatus.OrganizationStatusError = "rate limited during repo collaborator removal: " + err.Error() + newStatus.OrganizationStatusTimestamp = metav1.Now() + ns := *newStatus + uerr := retry.RetryOnConflict(retry.DefaultRetry, func() error { + latest := &v1.GithubOrganization{} + if err := r.Get(ctx, req.NamespacedName, latest); err != nil { + return err + } + latest.Status = ns + return r.Client.Status().Update(ctx, latest) + }) + if uerr != nil { + l.Error(uerr, "error during status update") + return reconcile.Result{}, uerr + } + if t.After(now) { + return reconcile.Result{RequeueAfter: t.Sub(now)}, nil + } + return reconcile.Result{Requeue: true}, nil + } + // "user not found" (404) — treat as success/complete + if strings.Contains(err.Error(), "user not found in github") { + l.Info("repo collaborator already removed (not found)", "repo", op.Repo, "user", op.User) + newStatus.Operations.RepositoryCollaboratorOperations[i].State = v1.GithubRepoUserOperationStateComplete + newStatus.Operations.RepositoryCollaboratorOperations[i].Timestamp = metav1.Now() + statusChanged = true + } else { + l.Error(err, "error during removing repo collaborator", "repo", op.Repo, "user", op.User) + newStatus.Operations.RepositoryCollaboratorOperations[i].State = v1.GithubRepoUserOperationStateFailed + newStatus.Operations.RepositoryCollaboratorOperations[i].Error = err.Error() + newStatus.Operations.RepositoryCollaboratorOperations[i].Timestamp = metav1.Now() + statusChanged = true + failed = true + } + } else { + l.Info("repo collaborator removed", "repo", op.Repo, "user", op.User) + newStatus.Operations.RepositoryCollaboratorOperations[i].State = v1.GithubRepoUserOperationStateComplete + newStatus.Operations.RepositoryCollaboratorOperations[i].Timestamp = metav1.Now() + statusChanged = true + } + } + // status changed check & reflect on Kubernetes if statusChanged { @@ -1291,6 +1678,16 @@ const GITHUB_ORG_LABEL_CLEAN_OPERATIONS_FAILED = "failed" const GITHUB_ORG_LABEL_FAILED_TTL = "repo-guard.cloudoperators.dev/failedTTL" const GITHUB_ORG_LABEL_COMPLETED_TTL = "repo-guard.cloudoperators.dev/completedTTL" +// Opt-in labels for #147 (remove org members not in any team) +const GITHUB_ORG_LABEL_REMOVE_ORG_MEMBER = "repo-guard.cloudoperators.dev/removeOrganizationMember" +const GITHUB_ORG_LABEL_REMOVE_ORG_MEMBER_ENABLED_VALUE = "true" +const GITHUB_ORG_LABEL_REMOVE_ORG_MEMBER_DRYRUN_VALUE = "dryRun" + +// Opt-in labels for #146 (remove direct repo collaborators not in any team) +const GITHUB_ORG_LABEL_REMOVE_REPOSITORY_DIRECT_COLLABORATOR = "repo-guard.cloudoperators.dev/removeRepositoryDirectCollaborator" +const GITHUB_ORG_LABEL_REMOVE_REPOSITORY_DIRECT_COLLABORATOR_ENABLED_VALUE = "true" +const GITHUB_ORG_LABEL_REMOVE_REPOSITORY_DIRECT_COLLABORATOR_DRYRUN_VALUE = "dryRun" + // ttlExpired parses a duration string (e.g., "24h", "30m") and checks if since+TTL is before now. func ttlExpired(ttlStr string, since time.Time, now time.Time) (bool, error) { d, err := time.ParseDuration(ttlStr) diff --git a/internal/controller/githuborganization_ttl_test.go b/internal/controller/githuborganization_ttl_test.go index 9969a6e..b991136 100644 --- a/internal/controller/githuborganization_ttl_test.go +++ b/internal/controller/githuborganization_ttl_test.go @@ -139,6 +139,17 @@ var _ = Describe("GithubOrganization TTL labels maintenance", Ordered, func() { Expect(ensureResourceCreated(ctx, org)).To(Succeed()) DeferCleanup(func() { _ = deleteIgnoreNotFound(ctx, k8sClient, org) }) + // Wait for the controller to settle to a stable state before seeding ops, + // so the initial reconcile (which writes `complete` for empty status) has + // already fired and won't race with our seeded status. + Eventually(func() repoguardsapv1.GithubOrganizationState { + cur := &repoguardsapv1.GithubOrganization{} + if err := k8sClient.Get(ctx, types.NamespacedName{Namespace: org.Namespace, Name: org.Name}, cur); err != nil { + return "" + } + return cur.Status.OrganizationStatus + }, 3*timeout, interval).Should(BeEquivalentTo(repoguardsapv1.GithubOrganizationStateComplete)) + Expect(updateStatusWithRetry(ctx, k8sClient, &repoguardsapv1.GithubOrganization{ ObjectMeta: metav1.ObjectMeta{Name: org.Name, Namespace: org.Namespace}, }, func(cur *repoguardsapv1.GithubOrganization) { @@ -184,6 +195,17 @@ var _ = Describe("GithubOrganization TTL labels maintenance", Ordered, func() { Expect(ensureResourceCreated(ctx, org)).To(Succeed()) DeferCleanup(func() { _ = deleteIgnoreNotFound(ctx, k8sClient, org) }) + // Wait for the controller to settle to a stable state before seeding ops, + // so the initial reconcile (which writes `complete` for empty status) has + // already fired and won't race with our seeded status. + Eventually(func() repoguardsapv1.GithubOrganizationState { + cur := &repoguardsapv1.GithubOrganization{} + if err := k8sClient.Get(ctx, types.NamespacedName{Namespace: org.Namespace, Name: org.Name}, cur); err != nil { + return "" + } + return cur.Status.OrganizationStatus + }, 3*timeout, interval).Should(BeEquivalentTo(repoguardsapv1.GithubOrganizationStateComplete)) + Expect(updateStatusWithRetry(ctx, k8sClient, &repoguardsapv1.GithubOrganization{ ObjectMeta: metav1.ObjectMeta{Name: org.Name, Namespace: org.Namespace}, }, func(cur *repoguardsapv1.GithubOrganization) { diff --git a/internal/controller/ttl.go b/internal/controller/ttl.go index c69d946..d3ff999 100644 --- a/internal/controller/ttl.go +++ b/internal/controller/ttl.go @@ -91,3 +91,30 @@ func applyTeamOpsTTL( } return out, true } + +// applyRepoUserOpsTTL drops GithubRepoUserOperations whose State equals state and +// whose own Timestamp is older than ttl. See applyUserOpsTTL for semantics. +func applyRepoUserOpsTTL( + ops []v1.GithubRepoUserOperation, + ttl time.Duration, + state v1.GithubRepoUserOperationState, + now time.Time, +) ([]v1.GithubRepoUserOperation, bool) { + var out []v1.GithubRepoUserOperation + for i, op := range ops { + if op.State == state && !op.Timestamp.IsZero() && now.After(op.Timestamp.Add(ttl)) { + if out == nil { + out = make([]v1.GithubRepoUserOperation, i, len(ops)) + copy(out, ops[:i]) + } + continue + } + if out != nil { + out = append(out, op) + } + } + if out == nil { + return ops, false + } + return out, true +} diff --git a/internal/controller/ttl_test.go b/internal/controller/ttl_test.go index 417e3b8..0b880a4 100644 --- a/internal/controller/ttl_test.go +++ b/internal/controller/ttl_test.go @@ -208,6 +208,88 @@ func TestApplyRepoOpsTTL(t *testing.T) { } } +// makeRepoUserOp builds a GithubRepoUserOperation with the given state and timestamp. +func makeRepoUserOp(repo, user string, state v1.GithubRepoUserOperationState, ts time.Time) v1.GithubRepoUserOperation { + return v1.GithubRepoUserOperation{ + Operation: v1.GithubRepoUserOperationTypeRemove, + Repo: repo, + User: user, + State: state, + Timestamp: metav1.NewTime(ts), + } +} + +func TestApplyRepoUserOpsTTL(t *testing.T) { + now := time.Date(2025, 6, 1, 12, 0, 0, 0, time.UTC) + old := now.Add(-48 * time.Hour) + fresh := now.Add(-1 * time.Hour) + + tests := []struct { + name string + ops []v1.GithubRepoUserOperation + ttl time.Duration + state v1.GithubRepoUserOperationState + wantRepos []string + wantChanged bool + }{ + { + name: "non-matching state preserved", + ops: []v1.GithubRepoUserOperation{makeRepoUserOp("r1", "u1", v1.GithubRepoUserOperationStateComplete, old)}, + ttl: 24 * time.Hour, + state: v1.GithubRepoUserOperationStateFailed, + wantRepos: []string{"r1"}, + wantChanged: false, + }, + { + name: "zero timestamp preserved", + ops: []v1.GithubRepoUserOperation{ + {Operation: v1.GithubRepoUserOperationTypeRemove, Repo: "r1", User: "u1", State: v1.GithubRepoUserOperationStateFailed}, + }, + ttl: 24 * time.Hour, + state: v1.GithubRepoUserOperationStateFailed, + wantRepos: []string{"r1"}, + wantChanged: false, + }, + { + name: "mixed aged and fresh: only aged dropped", + ops: []v1.GithubRepoUserOperation{ + makeRepoUserOp("r-old", "u1", v1.GithubRepoUserOperationStateFailed, old), + makeRepoUserOp("r-fresh", "u1", v1.GithubRepoUserOperationStateFailed, fresh), + makeRepoUserOp("r-other-state", "u1", v1.GithubRepoUserOperationStateComplete, old), + }, + ttl: 24 * time.Hour, + state: v1.GithubRepoUserOperationStateFailed, + wantRepos: []string{"r-fresh", "r-other-state"}, + wantChanged: true, + }, + { + name: "empty input slice", + ops: []v1.GithubRepoUserOperation{}, + ttl: 24 * time.Hour, + state: v1.GithubRepoUserOperationStateFailed, + wantRepos: []string{}, + wantChanged: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, changed := applyRepoUserOpsTTL(tt.ops, tt.ttl, tt.state, now) + if changed != tt.wantChanged { + t.Errorf("changed = %v, want %v", changed, tt.wantChanged) + } + if len(got) != len(tt.wantRepos) { + t.Fatalf("got %d ops, want %d (got=%v want=%v)", len(got), len(tt.wantRepos), got, tt.wantRepos) + } + for i, op := range got { + if op.Repo != tt.wantRepos[i] { + t.Errorf("ops[%d].Repo = %q, want %q", i, op.Repo, tt.wantRepos[i]) + } + } + }) + } +} + func TestApplyTeamOpsTTL(t *testing.T) { now := time.Date(2025, 6, 1, 12, 0, 0, 0, time.UTC) old := now.Add(-48 * time.Hour) diff --git a/internal/metrics/metrics.go b/internal/metrics/metrics.go index dfb70c0..2f9b681 100644 --- a/internal/metrics/metrics.go +++ b/internal/metrics/metrics.go @@ -164,7 +164,7 @@ func SetGithubOrganizationMetrics(org *v1.GithubOrganization) { setOrgOps := func(scope, op, state string, v float64) { GithubOrganizationOperations.WithLabelValues(github, organization, scope, op, state).Set(v) } - scopes := []string{"owners", "teams", "repos"} + scopes := []string{"owners", "teams", "repos", "orgmembers", "repocollaborators"} ops := []string{"add", "remove"} states := []string{"pending", "complete", "failed", "skipped", "notfound"} for _, sc := range scopes { @@ -225,6 +225,40 @@ func SetGithubOrganizationMetrics(org *v1.GithubOrganization) { } } } + // orgmembers (#147) + if org.Status.Operations.OrganizationMemberOperations != nil { + counts := map[string]map[string]float64{} + for _, o := range org.Status.Operations.OrganizationMemberOperations { + op := string(o.Operation) + st := string(o.State) + if counts[op] == nil { + counts[op] = map[string]float64{} + } + counts[op][st] += 1 + } + for op, byState := range counts { + for st, v := range byState { + setOrgOps("orgmembers", op, st, v) + } + } + } + // repocollaborators (#146) + if org.Status.Operations.RepositoryCollaboratorOperations != nil { + counts := map[string]map[string]float64{} + for _, o := range org.Status.Operations.RepositoryCollaboratorOperations { + op := string(o.Operation) + st := string(o.State) + if counts[op] == nil { + counts[op] = map[string]float64{} + } + counts[op][st] += 1 + } + for op, byState := range counts { + for st, v := range byState { + setOrgOps("repocollaborators", op, st, v) + } + } + } } // SetGithubTeamMetrics sets gauges for the given GithubTeam's current status and operation counts. diff --git a/internal/metrics/metrics_test.go b/internal/metrics/metrics_test.go index 8faf8c9..f95393d 100644 --- a/internal/metrics/metrics_test.go +++ b/internal/metrics/metrics_test.go @@ -28,6 +28,21 @@ func TestSetGithubOrganizationMetrics(t *testing.T) { State: v1.GithubUserOperationStatePending, }, }, + OrganizationMemberOperations: []v1.GithubUserOperation{ + { + Operation: v1.GithubUserOperationTypeRemove, + User: "user1", + State: v1.GithubUserOperationStatePending, + }, + }, + RepositoryCollaboratorOperations: []v1.GithubRepoUserOperation{ + { + Operation: v1.GithubRepoUserOperationTypeRemove, + Repo: "repo1", + User: "user2", + State: v1.GithubRepoUserOperationStatePending, + }, + }, }, }, } @@ -55,6 +70,14 @@ repo_guard_githuborganization_status{github="github.com",organization="sapcc",st count := testutil.ToFloat64(GithubOrganizationOperations.WithLabelValues("github.com", "sapcc", "teams", "add", "pending")) assert.Equal(t, 1.0, count) + + // Check orgmembers scope (#147) + countOrgMembers := testutil.ToFloat64(GithubOrganizationOperations.WithLabelValues("github.com", "sapcc", "orgmembers", "remove", "pending")) + assert.Equal(t, 1.0, countOrgMembers) + + // Check repocollaborators scope (#146) + countRepoCollab := testutil.ToFloat64(GithubOrganizationOperations.WithLabelValues("github.com", "sapcc", "repocollaborators", "remove", "pending")) + assert.Equal(t, 1.0, countRepoCollab) } func TestSetGithubTeamMetrics(t *testing.T) {