From af38bdb0fac955ba23cb500409aa96a15e932814 Mon Sep 17 00:00:00 2001 From: Chmouel Boudjnah Date: Thu, 9 Oct 2025 11:51:32 +0200 Subject: [PATCH] feat(gitlab): Cache results acl project membership Implemented caching in gitlab for results returned by `checkMembership`. This was done to reduce repeated calls to the GitLab API when checking the membership status of the same user multiple times during processing of an event. Co-authored-by: Claude Jira: https://issues.redhat.com/browse/SRVKP-9056 Signed-off-by: Chmouel Boudjnah --- pkg/provider/gitlab/acl.go | 20 +++++- pkg/provider/gitlab/acl_test.go | 106 +++++++++++++++++++++++++++++++ pkg/provider/gitlab/gitlab.go | 3 + pkg/provider/gitlab/test/test.go | 12 ++++ 4 files changed, 140 insertions(+), 1 deletion(-) diff --git a/pkg/provider/gitlab/acl.go b/pkg/provider/gitlab/acl.go index da70ce5bbb..84b20fc3d9 100644 --- a/pkg/provider/gitlab/acl.go +++ b/pkg/provider/gitlab/acl.go @@ -27,12 +27,30 @@ func (v *Provider) IsAllowedOwnersFile(_ context.Context, event *info.Event) (bo } func (v *Provider) checkMembership(ctx context.Context, event *info.Event, userid int) bool { + // Initialize cache lazily + if v.memberCache == nil { + v.memberCache = map[int]bool{} + } + + if allowed, ok := v.memberCache[userid]; ok { + return allowed + } + member, _, err := v.Client().ProjectMembers.GetInheritedProjectMember(v.targetProjectID, userid) - if err == nil && member.ID != 0 && member.ID == userid { + if err != nil { + // If the API call fails, fall back without caching the result so a + // transient failure can be retried on the next invocation. + isAllowed, _ := v.IsAllowedOwnersFile(ctx, event) + return isAllowed + } + + if member.ID != 0 && member.ID == userid { + v.memberCache[userid] = true return true } isAllowed, _ := v.IsAllowedOwnersFile(ctx, event) + v.memberCache[userid] = isAllowed return isAllowed } diff --git a/pkg/provider/gitlab/acl_test.go b/pkg/provider/gitlab/acl_test.go index 77aa98fe01..46a8b344a2 100644 --- a/pkg/provider/gitlab/acl_test.go +++ b/pkg/provider/gitlab/acl_test.go @@ -1,6 +1,8 @@ package gitlab import ( + "fmt" + "net/http" "testing" "github.com/openshift-pipelines/pipelines-as-code/pkg/params/info" @@ -130,3 +132,107 @@ func TestIsAllowed(t *testing.T) { }) } } + +func TestMembershipCaching(t *testing.T) { + ctx, _ := rtesting.SetupFakeContext(t) + + v := &Provider{ + targetProjectID: 3030, + userID: 4242, + } + + client, mux, tearDown := thelp.Setup(t) + defer tearDown() + v.gitlabClient = client + + // Count how many times the membership API is hit. + var calls int + thelp.MuxAllowUserIDCounting(mux, v.targetProjectID, v.userID, &calls) + + ev := &info.Event{Sender: "someone", PullRequestNumber: 1} + + // First call should hit the API once and cache the result. + allowed, err := v.IsAllowed(ctx, ev) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !allowed { + t.Fatalf("expected allowed on first membership check") + } + if calls < 1 { + t.Fatalf("expected at least 1 membership API call, got %d", calls) + } + + // Second call should use the cache and not hit the API again. + allowed, err = v.IsAllowed(ctx, ev) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !allowed { + t.Fatalf("expected allowed on cached membership check") + } + if calls != 1 { + t.Fatalf("expected cached result with no extra API call, got %d calls", calls) + } +} + +func TestMembershipAPIFailureDoesNotCacheFalse(t *testing.T) { + ctx, _ := rtesting.SetupFakeContext(t) + + v := &Provider{ + targetProjectID: 3030, + userID: 4242, + } + + client, mux, tearDown := thelp.Setup(t) + defer tearDown() + v.gitlabClient = client + + ev := &info.Event{Sender: "someone"} + + var ( + calls int + success bool + ) + path := fmt.Sprintf("/projects/%d/members/all/%d", v.targetProjectID, v.userID) + mux.HandleFunc(path, func(rw http.ResponseWriter, _ *http.Request) { + calls++ + if !success { + rw.WriteHeader(http.StatusInternalServerError) + _, _ = rw.Write([]byte(`{}`)) + return + } + _, err := fmt.Fprintf(rw, `{"id": %d}`, v.userID) + if err != nil { + t.Fatalf("failed to write response: %v", err) + } + }) + + thelp.MuxDiscussionsNoteEmpty(mux, v.targetProjectID, ev.PullRequestNumber) + + allowed, err := v.IsAllowed(ctx, ev) + if err != nil { + t.Fatalf("unexpected error on failure path: %v", err) + } + if allowed { + t.Fatalf("expected not allowed when membership API fails and no fallback grants access") + } + if calls < 1 { + t.Fatalf("expected at least 1 membership API call, got %d", calls) + } + initialCallCount := calls + + // Make the next API call succeed; the provider should retry because the previous failure wasn't cached. + success = true + + allowed, err = v.IsAllowed(ctx, ev) + if err != nil { + t.Fatalf("unexpected error on retry path: %v", err) + } + if !allowed { + t.Fatalf("expected allowed when membership API succeeds on retry") + } + if calls <= initialCallCount { + t.Fatalf("expected membership API to be called again after retry, got %d total calls (initial %d)", calls, initialCallCount) + } +} diff --git a/pkg/provider/gitlab/gitlab.go b/pkg/provider/gitlab/gitlab.go index 6d6ea8c921..e075dbc20b 100644 --- a/pkg/provider/gitlab/gitlab.go +++ b/pkg/provider/gitlab/gitlab.go @@ -62,6 +62,9 @@ type Provider struct { eventEmitter *events.EventEmitter repo *v1alpha1.Repository triggerEvent string + // memberCache caches membership/permission checks by user ID within the + // current provider instance lifecycle to avoid repeated API calls. + memberCache map[int]bool } func (v *Provider) Client() *gitlab.Client { diff --git a/pkg/provider/gitlab/test/test.go b/pkg/provider/gitlab/test/test.go index 4f76e26d8e..b0dce02072 100644 --- a/pkg/provider/gitlab/test/test.go +++ b/pkg/provider/gitlab/test/test.go @@ -66,6 +66,18 @@ func MuxDisallowUserID(mux *http.ServeMux, projectID, userID int) { }) } +// MuxAllowUserIDCounting registers a handler that returns an allowed member and increments +// the provided counter each time it is called. Useful to assert caching behavior. +func MuxAllowUserIDCounting(mux *http.ServeMux, projectID, userID int, counter *int) { + path := fmt.Sprintf("/projects/%d/members/all/%d", projectID, userID) + mux.HandleFunc(path, func(rw http.ResponseWriter, _ *http.Request) { + if counter != nil { + *counter++ + } + fmt.Fprintf(rw, `{"id": %d}`, userID) + }) +} + func MuxListTektonDir(_ *testing.T, mux *http.ServeMux, pid int, ref, prs string, wantTreeAPIErr, wantFilesAPIErr bool) { mux.HandleFunc(fmt.Sprintf("/projects/%d/repository/tree", pid), func(rw http.ResponseWriter, r *http.Request) { if wantTreeAPIErr {