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 <[email protected]>
Jira: https://issues.redhat.com/browse/SRVKP-9056
Signed-off-by: Chmouel Boudjnah <[email protected]>

Author: chmouel

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
 }
 

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"
@@ -155,3 +157,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 TestMembershipAPIFailureDoesNotCacheApiError(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)
+	}
+}

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 {

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 {