Skip to content

Conversation

@hoxhaeris
Copy link
Contributor

Add Organization Roles Support to Peribolos

This PR adds support for managing GitHub organization custom roles in Peribolos, enabling declarative configuration of role assignments to teams and users.

Changes

1. Configuration Schema (pkg/config/org/org.go)

  • Added Roles field to Config struct for declaring role assignments
  • Implemented ValidateRoles() for pre-flight validation with case-insensitive team/user matching
  • Uses github.NormLogin() for consistent username normalization

2. GitHub API Client (pkg/github/client.go)

Added methods for organization role management:

  • ListOrganizationRoles(), ListTeamsWithRole(), ListUsersWithRole()
  • AssignOrganizationRoleToTeam(), RemoveOrganizationRoleFromTeam()
  • AssignOrganizationRoleToUser(), RemoveOrganizationRoleFromUser()

All mutating operations respect dry-run mode.

3. Main Implementation (cmd/peribolos/main.go)

  • Added --fix-org-roles flag to enable role management (gatekeeper)
  • Implemented configureOrgRoles()
  • Team name -> slug resolution for correct API calls
  • Slug -> team name mapping in dump mode for idempotent configurations
  • Roles configured after teams are fully populated

4. Test Coverage (pkg/config/org/org_test.go, cmd/peribolos/main_test.go)

  • 12 validation test cases (nested teams, case-insensitivity, duplicates)
  • 6 team assignment test cases
  • 8 user assignment test cases

Example Configuration

orgs:
  my-org:
    teams:
      security-team:
        members: [user1, user2]
      compliance-team:
        members: [user3, user4]

    admins:
      - admin-user

    roles:
      security-manager:
        teams: [security-team]
        users: [admin-user]

      billing-manager:
        teams: [compliance-team]
        users: [admin-user]

Usage

# Validate configuration only
peribolos --config-path=org.yaml

# Apply with dry-run
peribolos --config-path=org.yaml --fix-org-roles --confirm=false

# Apply changes
peribolos --config-path=org.yaml --fix-org-roles --confirm=true

# Dump current organization state
peribolos --dump my-org --github-token-path=token

@netlify
Copy link

netlify bot commented Nov 24, 2025

Deploy Preview for k8s-prow ready!

Name Link
🔨 Latest commit 7c94f88
🔍 Latest deploy log https://app.netlify.com/projects/k8s-prow/deploys/692edf9a86f9440007774121
😎 Deploy Preview https://deploy-preview-555--k8s-prow.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@k8s-ci-robot k8s-ci-robot added the area/peribolos Issues or PRs related to prow's peribolos component label Nov 24, 2025
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 24, 2025
@k8s-ci-robot
Copy link
Contributor

Hi @hoxhaeris. Thanks for your PR.

I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 24, 2025
@hoxhaeris
Copy link
Contributor Author

hoxhaeris commented Nov 24, 2025

/hold
Holding while I finish a set of manual tests using a test GitHub org
/cc @petr-muller
/cc @bradmwilliams

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 24, 2025
@k8s-ci-robot
Copy link
Contributor

@hoxhaeris: GitHub didn't allow me to request PR reviews from the following users: bradmwilliams.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/hold
Hold while I finish a set of manual tests using a test GitHub org
/cc @petr-muller
/cc @bradmwilliams

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@petr-muller
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 24, 2025
@hoxhaeris hoxhaeris force-pushed the add_org_roles_feature branch 2 times, most recently from a508906 to 8821599 Compare November 25, 2025 16:33
@hoxhaeris
Copy link
Contributor Author

/test pull-prow-unit-test

@hoxhaeris hoxhaeris force-pushed the add_org_roles_feature branch from 8821599 to 1aabf3d Compare November 27, 2025 12:12
@hoxhaeris
Copy link
Contributor Author

/test pull-prow-unit-test

1 similar comment
@hoxhaeris
Copy link
Contributor Author

/test pull-prow-unit-test

@hoxhaeris hoxhaeris force-pushed the add_org_roles_feature branch from 1aabf3d to eeaf178 Compare December 2, 2025 11:06
@hoxhaeris
Copy link
Contributor Author

Tests summary:

What Was Tested

Basic Operations

  • Assign role to team
  • Assign role to user
  • Assign role to both team and user simultaneously
  • Remove role from team
  • Remove role from user

Cleanup Scenarios

  • Role removed from config -> all assignments cleaned up
  • Empty roles: {} -> orphaned assignments removed
  • Omitted roles key -> assignments cleaned up

Case Sensitivity

  • Role names matched case-insensitively (Security_Manager -> security_manager)
  • Team references matched case-insensitively
  • User references matched case-insensitively

Idempotency

  • Running Peribolos twice produces no mutations on second run

Flag Gating

  • Without --fix-org-roles flag -> no role changes made
  • Dry-run mode (--confirm=false) -> no actual API mutations
  • --fix-org-roles requires --fix-teams -> fails with clear error if missing

Dump Functionality

  • --dump output includes roles: section
  • Dump uses team names (not slugs) for idempotent round-trip

Validation

  • Error for role not defined in GitHub
  • Error for team not in config
  • Error for user not an org member
  • Validation runs before any API mutations (fail-fast)

Multiple/Complex Assignments

  • Team AND different user assigned to same role
  • Partial update: add team + remove user in single run
  • Admin users can be assigned roles (not just members)

Pending Invitations

  • Users with pending org invitations gracefully skipped with log message

Indirect Assignments (Team Membership)

  • Users who have role via team membership are NOT removed when users: []
  • Only direct assignments are managed; indirect ones preserved

Indirect Assignment Handling

GitHub's API returns users who have roles through team membership with "assignment": "indirect". Peribolos will:

  • Filters out indirect assignments when comparing to config
  • Only manages direct user assignments
  • Team members keep their role through the team even with users: []

This prevents accidental removal of team members who inherit roles.


@hoxhaeris
Copy link
Contributor Author

/test pull-prow-verify-lint

@hoxhaeris hoxhaeris force-pushed the add_org_roles_feature branch from eeaf178 to 3776034 Compare December 2, 2025 11:50
@k8s-ci-robot k8s-ci-robot added area/branchprotector Issues or PRs related to prow's branchprotector component area/crier Issues or PRs related to prow's crier component needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. area/deck Issues or PRs related to prow's deck component area/documentation Issues or PRs related to documentation labels Dec 2, 2025
@hoxhaeris hoxhaeris force-pushed the add_org_roles_feature branch from 5c39413 to 7c94f88 Compare December 2, 2025 12:46
@hoxhaeris hoxhaeris closed this Dec 2, 2025
@hoxhaeris hoxhaeris reopened this Dec 2, 2025
@k8s-ci-robot
Copy link
Contributor

@hoxhaeris: Those labels are not set on the issue: area/prow/tide, area/prow/hook, area/prow/plank, area/prow/deck, area/prow/crier, area/prow/horologium, area/prow/sinker, area/prow/spyglass, area/prow/status-reconciler

In response to this:

/remove-area prow/tide
/remove-area prow/hook
/remove-area prow/plank
/remove-area prow/deck
/remove-area prow/crier
/remove-area prow/horologium
/remove-area prow/sinker
/remove-area prow/spyglass
/remove-area prow/status-reconciler

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@k8s-ci-robot
Copy link
Contributor

@hoxhaeris: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-prow-verify-lint 7c94f88 link true /test pull-prow-verify-lint

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@hoxhaeris
Copy link
Contributor Author

/remove-area pubsub
/remove-area crier
/remove-area gangway
/remove-area podutils/initupload
/remove-area plugins
/remove-area tide
/remove-area gerrit
/remove-area prowcm
/remove-area hook
/remove-area status-reconciler
/remove-area pod-utilities
/remove-area deck
/remove-area branchprotector
/remove-area podutils/sidecar
/remove-area podutils/entrypoint
/remove-area podutils/clonerefs
/remove-area podutils/gcsupload
/remove-area sinker
/remove-area moonraker
/remove-area ghproxy
/remove-area jenkins-operator
/remove-area spyglass
/remove-area horologium
/remove-area documentation

@k8s-ci-robot k8s-ci-robot removed area/pubsub Issues or PRs related to prow's pubsub reporter component area/crier Issues or PRs related to prow's crier component area/gangway Issues or PRs related to prow's gangway component area/podutils/initupload Issues or PRs related to prow's initupload component area/plugins Issues or PRs related to prow's plugins for the hook component area/tide Issues or PRs related to prow's tide component area/gerrit Issues or PRs related to prow's gerrit component area/prowcm Issues or PRs related to prow's controller manager component area/hook Issues or PRs related to prow's hook component area/status-reconciler Issues or PRs related to reconciling status when jobs change area/pod-utilities Issues or PRs related to prow's pod-utilities component area/deck Issues or PRs related to prow's deck component area/branchprotector Issues or PRs related to prow's branchprotector component area/podutils/sidecar Issues or PRs related to prow's sidecar component area/podutils/entrypoint Issues or PRs related to prow's entrypoint component area/podutils/clonerefs Issues or PRs related to prow's clonerefs component area/podutils/gcsupload Issues or PRs related to prow's gcsupload component area/sinker Issues or PRs related to prow's sinker component area/moonraker Issues or PRs related to prow's moonraker component area/ghproxy Issues or PRs related to prow's ghproxy component area/jenkins-operator Issues or PRs related to prow's jenkins-operator component area/spyglass Issues or PRs related to prow's spyglass UI area/horologium Issues or PRs related to prow's horologium component area/documentation Issues or PRs related to documentation labels Dec 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/peribolos Issues or PRs related to prow's peribolos component cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants