feat(github): add glob pattern support for token scope repos#2386
Conversation
|
/gemini review |
Summary of ChangesHello @theakshaypant, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the flexibility of GitHub App token scoping by introducing glob pattern support. This allows users to define repository access for GitHub Apps using wildcard patterns, such as "myorg/*", rather than listing each repository individually. The change applies to both global and repository-level configurations, improving manageability and reducing configuration overhead for projects with many repositories. It also includes robust validation for glob patterns to ensure correct usage. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces support for glob patterns in repository scoping for GitHub App tokens, a valuable feature for managing permissions across multiple repositories. The implementation covers both global and repository-level configurations and includes corresponding documentation and test updates.
My review has identified a few areas for improvement. The documentation for repository-level globbing could be more precise to avoid user confusion. There's also a fragile glob detection mechanism in CreateToken that could be made more robust to handle edge cases correctly. Additionally, there are minor improvements to be made in error handling and documentation examples for consistency. Overall, these are great changes that enhance the flexibility of token scoping.
|
|
||
| You can use the `Repository` custom resource to scope the generated GitHub token to a list of repositories. | ||
| The repositories can be public or private, but must reside in the same namespace as the repository with which the `Repository` resource is associated. | ||
| You can specify repositories using exact names or glob patterns (e.g., `myorg/*` to match all repositories under an organization which have the GitHub app installed). |
There was a problem hiding this comment.
The description of how glob patterns work for repository-level configuration is slightly misleading. It currently states that it matches all repositories where the app is installed, which is true for the global configuration. For repository-level configuration, matching is restricted to Repository CRDs within the same namespace. Clarifying this distinction will help users understand the feature correctly.
| You can specify repositories using exact names or glob patterns (e.g., `myorg/*` to match all repositories under an organization which have the GitHub app installed). | |
| You can specify repositories using exact names or glob patterns (e.g., `myorg/*` to match all repositories that are defined as `Repository` CRs in the same namespace). |
|
|
||
| ```console | ||
| failed to scope GitHub token as repo owner1/project1 does not exist in namespace test-repo | ||
| failed to scope GitHub token as repo with pattern owner1/project1 does not exist in namespace test-repo |
There was a problem hiding this comment.
The example error message is inconsistent with the Repository CR example provided earlier. The CR uses the glob pattern owner1/*, but the error message refers to owner1/project1. To avoid confusion, the error message should reflect the pattern that failed to match.
| failed to scope GitHub token as repo with pattern owner1/project1 does not exist in namespace test-repo | |
| failed to scope GitHub token as repo with pattern owner1/* does not exist in namespace test-repo |
| // We can skip error check here as all the glob compilation has been checked | ||
| // before this method is called. | ||
| reposToScope, _ := glob.Compile(repoPattern) |
There was a problem hiding this comment.
Ignoring a potential error from glob.Compile with the blank identifier _ is risky, even if you assume the pattern has been pre-validated. It's better practice to handle the error defensively. This makes the code more robust and helps catch unexpected issues during development or future refactoring.
reposToScope, err := glob.Compile(repoPattern)
if err != nil {
// This should not happen since we have validated the pattern before, but being defensive.
return fmt.Errorf("BUG: previously validated glob pattern %q failed to compile: %w", repoPattern, err)
}There was a problem hiding this comment.
yeah probably a good idea to add the error in the event namespace of the user
There was a problem hiding this comment.
Code Review
This pull request introduces support for glob patterns when scoping GitHub App tokens, which is a great enhancement for flexibility. The changes cover both global and repository-level configurations, along with corresponding documentation and test updates.
My review focuses on improving robustness, efficiency, and documentation clarity. I've identified a potential panic due to an unhandled error, an opportunity to optimize token creation by avoiding redundant API calls, and a point of confusion in the updated documentation. Addressing these points will make the feature more resilient and easier to use.
| func (v *Provider) expandGlobAndAddRepoIDs(ctx context.Context, repoPattern string, cache *[]*github.Repository) error { | ||
| // We can skip error check here as all the glob compilation has been checked | ||
| // before this method is called. | ||
| reposToScope, _ := glob.Compile(repoPattern) |
There was a problem hiding this comment.
Ignoring the error returned by glob.Compile can lead to a panic. If repoPattern is an invalid glob, glob.Compile will return a nil Glob object and an error. The subsequent call to reposToScope.Match() on line 684 would then cause a nil pointer dereference.
While the current call sites in scope.go pre-validate the glob patterns, this function is exported and could be called from other places in the future without this validation. It's safer to handle the error explicitly to prevent panics.
reposToScope, err := glob.Compile(repoPattern)
if err != nil {
return fmt.Errorf("invalid glob pattern %q: %w", repoPattern, err)
}|
|
||
| You can use the `Repository` custom resource to scope the generated GitHub token to a list of repositories. | ||
| The repositories can be public or private, but must reside in the same namespace as the repository with which the `Repository` resource is associated. | ||
| You can specify repositories using exact names or glob patterns (e.g., `myorg/*` to match all repositories under an organization which have the GitHub app installed). |
There was a problem hiding this comment.
The explanation for glob patterns in repository-level configuration is slightly misleading. It states that it matches 'all repositories under an organization which have the GitHub app installed', which is how the global configuration works.
However, for repository-level configuration, glob patterns are matched against Repository CRDs that exist in the same namespace.
To avoid confusion, I suggest clarifying that the matching happens within the namespace's defined repositories.
| You can specify repositories using exact names or glob patterns (e.g., `myorg/*` to match all repositories under an organization which have the GitHub app installed). | |
| You can specify repositories using exact names or glob patterns (e.g., `myorg/*` to match all repositories defined as `Repository` CRDs in the same namespace). |
| ) | ||
| listURLs := map[string]string{} | ||
| listURLs := []string{} | ||
| repoListToScopeToken := []string{} |
There was a problem hiding this comment.
The repoListToScopeToken slice can accumulate duplicate repository names. This can happen if a repository is specified in the global configuration and also matches a glob pattern in the repository-level configuration, or if multiple glob patterns match the same repository.
While uniqueRepositoryID will eventually deduplicate the repository IDs, CreateToken will still perform redundant Repositories.Get API calls for each duplicate entry.
To improve efficiency and avoid unnecessary API calls, consider using a map as a set to collect unique repository names before converting it to a slice at the end.
| continue | ||
| } | ||
| // May or may not be a glob | ||
| if _, err := glob.Compile(configValueS); err != nil { |
There was a problem hiding this comment.
Global config glob is compiled for early exit
| if !globMatchFound { | ||
| msg := fmt.Sprintf("failed to scope GitHub token as repo with pattern %s does not exist in namespace %s", repo.Spec.Settings.GithubAppTokenScopeRepos[i], ns) |
There was a problem hiding this comment.
To keep the non-matching behaviour similar, an error is returned if repo config globs (or exact names) do not match any repo CR in the anmespace.
|
|
||
| for _, r := range repository { | ||
| // Check if this is a glob pattern | ||
| if strings.ContainsAny(r, "*?[") { |
There was a problem hiding this comment.
The repository list still contains globs coming from the global config which are matched first.
| return nil | ||
| } | ||
|
|
||
| func (v *Provider) listAppRepos(ctx context.Context) ([]*github.Repository, error) { |
There was a problem hiding this comment.
This method lists all repos that the PAC application is installed on.
| for _, repo := range *cache { | ||
| repoFullName := repo.GetFullName() | ||
| if reposToScope.Match(repoFullName) { | ||
| v.RepositoryIDs = uniqueRepositoryID(v.RepositoryIDs, repo.GetID()) | ||
| } | ||
| } |
There was a problem hiding this comment.
No error if the global config pattern does not match any repos the app is installed on
| continue | ||
| } | ||
|
|
||
| split := strings.Split(r, "/") |
There was a problem hiding this comment.
All global non-glob and namespace matched repos flow remains unchanged
Add support for glob patterns when specifying repositories for GitHub App token scoping in both global and repository-level configurations. Users can now use patterns like "myorg/*" to match multiple repos instead of listing each one explicitly. For global config (secret-github-app-scope-extra-repos), glob patterns are expanded by listing all repositories where the GitHub App is installed. Results are cached to avoid repeated API calls when multiple patterns are specified. For repository-level config (github_app_token_scope_repos), glob patterns are matched against Repository CRDs in the same namespace, maintaining the existing namespace isolation requirement. Both exact matches and glob patterns can be combined. Invalid glob patterns are validated early and return clear error messages. Jira: https://issues.redhat.com/browse/SRVKP-10030 Signed-off-by: Akshay Pant <akshay.akshaypant@gmail.com>
dbf7b97 to
55820ee
Compare
🔍 PR Lint Feedback
|
There was a problem hiding this comment.
Not very sure about this. Updating local env to run these tests.
|
/test go-testing |
|
I spent some time on it, I don't see any security issues, with what we are doing here. The first globbing should always match.. We are bypassing webhook repo validation in here tho, I don't think we should enforce it to keep the flexibility but we need to document it |
|
that go-testing error is weird, does not happen on that PR #2363 for example |
|
while invesigating this i figured a potential issue that is related in #2395 |
|
/retest |
Passed on retest |
📝 Description of the Change
Add support for glob patterns when specifying repositories for GitHub App token scoping in both global and repository-level configurations. Users can now use patterns like "myorg/*" to match multiple repos instead of listing each one explicitly.
For global config (secret-github-app-scope-extra-repos), glob patterns are expanded by listing all repositories where the GitHub App is installed. Results are cached to avoid repeated API calls when multiple patterns are specified.
For repository-level config (github_app_token_scope_repos), glob patterns are matched against Repository CRDs in the same namespace, maintaining the existing namespace isolation requirement.
Both exact matches and glob patterns can be combined. Invalid glob patterns are validated early and return clear error messages.
👨🏻 Linked Jira
https://issues.redhat.com/browse/SRVKP-10030
🔗 Linked GitHub Issue
N/A
🚀 Type of Change
fix:)feat:)feat!:,fix!:)docs:)chore:)refactor:)enhance:)deps:)🧪 Testing Strategy
PAC global config
Repo CR:
Controller logs (added logging after
🤖 AI Assistance
If you have used AI assistance, please provide the following details:
Which LLM was used?
Extent of AI Assistance:
Important
If the majority of the code in this PR was generated by an AI, please add a
Co-authored-bytrailer to your commit message.For example:
Co-authored-by: Gemini gemini@google.com
Co-authored-by: ChatGPT noreply@chatgpt.com
Co-authored-by: Claude noreply@anthropic.com
Co-authored-by: Cursor noreply@cursor.com
Co-authored-by: Copilot Copilot@users.noreply.github.com
**💡You can use the script
./hack/add-llm-coauthor.shto automatically addthese co-author trailers to your commits.
✅ Submitter Checklist
fix:,feat:) matches the "Type of Change" I selected above.make testandmake lintlocally to check for and fix anyissues. For an efficient workflow, I have considered installing
pre-commit and running
pre-commit installtoautomate these checks.