fix(protectedbranch): preserve UserID/GroupID and fix reconcile thrash for identity-only access rules

[1872etd7q23wd]

Description of your changes

This fixes two related bugs in ProtectedBranch that together caused the delete/recreate ("thrash") loop described in #267.

1. GenerateProtectRepositoryBranchesOptions dropped UserID/GroupID.
It only ever read the first entry of PushAccessLevels/MergeAccessLevels/UnprotectAccessLevels and mapped its AccessLevel onto the deprecated singular push/merge/unprotect access level fields. UserID- and GroupID-scoped entries, and any entry beyond the first, were silently dropped from the API request GitLab actually received. Fixed by sending the array-based allowed_to_push/allowed_to_merge/allowed_to_unprotect options instead, preserving AccessLevel, UserID, and GroupID for every entry.

2. isAccessLevelsUpToDate required AccessLevel to match even when the spec didn't set one.
For the common case of a userId-only or groupId-only rule (e.g. pushAccessLevels: [{userId: X}], no accessLevel), the match check only ran when specLevel.AccessLevel != nil. Since that's never true for an identity-only rule, isAccessLevelsUpToDate returned false unconditionally — meaning even after fix #1 makes the provider send/observe the userId correctly, IsProtectedBranchUpToDate would still report "not up to date" on every single reconcile. Update() has no real PATCH for protected branches — it always unprotects and re-protects the branch (see controller.go) — so this alone reproduces the exact delete/recreate thrash from the issue. Fixed by only comparing AccessLevel when the spec actually sets it.

I verified fix #2 was necessary, not theoretical, by testing directly against a real GitLab (Premium-tier) project: sending allowed_to_push: [{"user_id": X}] is accepted and persisted correctly, and GitLab reports it back as {"access_level": 40, "user_id": X, ...} — a real, non-zero access_level alongside the user_id. Without fix #2, that response would never satisfy the old comparison for a spec with no accessLevel set, regardless of fix #1.

Fixes #267

I have:

• Read and followed Crossplane's contribution process.
• Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

• Live verification against real GitLab (Premium-tier project, created and torn down for this purpose):
  • Confirmed the array-based allowed_to_push: [{"user_id": X}] request (what fix Bootstrap provider-gitlab #1 now sends) is accepted by GitLab's API and round-trips correctly on read-back, for both a project owner/member and a service-account user.
  • Confirmed GitLab's response includes a real access_level value alongside the user_id (not zero/absent) — which is exactly what makes the old isAccessLevelsUpToDate (pre-fix Implement projects api #2) permanently return "not up to date" for identity-only rules, since it required access_level to match too.
• Added unit tests covering both fixes: a UserID-only push rule (request-building), a mixed AccessLevel+UserID array, a GroupID-only merge rule, the no-access-levels case, and IsProtectedBranchUpToDate scenarios for userId-only match/mismatch and the existing accessLevel-only behavior — all pass.
• Full existing test suite for the two affected packages (pkg/namespaced/clients/projects, pkg/cluster/clients/projects) passes with no regressions.
• go build ./..., go vet, and gofmt -l are clean.
• make reviewable test (generate + lint + test) passes end-to-end across the whole repository: make generate produces no diff (the generated pkg/cluster/clients/projects/zz_protectedbranch.go, mirrored from the namespaced client via hack/generate-cluster-scope.go, stays in sync); make lint (golangci-lint v2.11.2) reports 0 issues; make test passes repo-wide.

[henrysachs]

Please add regression coverage for duplicate/cardinality and nil desired entries, and regenerate the cluster mirror from the namespaced source to keep generated parity.

[henrysachs]

Each desired entry currently scans the full observed slice, so duplicate desired rules can reuse one observed rule; nil desired entries also panic here, unlike branchPermissionOptions. Filter nil desired entries before the cardinality check, then record each matched observed index.

Suggested change

	accessMatch := specLevel.AccessLevel == nil || int64(*specLevel.AccessLevel) == int64(gitlabLevel.AccessLevel)
	filteredSpecLevels := make([]*v1alpha1.BranchAccessDescription, 0, len(specLevels))
	for _, specLevel := range specLevels {
	if specLevel != nil {
	filteredSpecLevels = append(filteredSpecLevels, specLevel)
	}
	}
	if len(filteredSpecLevels) != len(gitlabLevels) {
	return false
	}
	matched := make([]bool, len(gitlabLevels))
	for _, specLevel := range filteredSpecLevels {
	found := false
	for i, gitlabLevel := range gitlabLevels {
	if matched[i] || gitlabLevel == nil {
	continue
	}
	accessMatch := specLevel.AccessLevel == nil || int64(*specLevel.AccessLevel) == int64(gitlabLevel.AccessLevel)
	userMatch := (specLevel.UserID == nil && gitlabLevel.UserID == 0) || (specLevel.UserID != nil && *specLevel.UserID == gitlabLevel.UserID)
	groupMatch := (specLevel.GroupID == nil && gitlabLevel.GroupID == 0) || (specLevel.GroupID != nil && *specLevel.GroupID == gitlabLevel.GroupID)
	if accessMatch && userMatch && groupMatch {
	matched[i] = true
	found = true
	break
	}
	}
	if !found {
	return false
	}
	}
	return true

[1872etd7q23wd]

Applied this and found one more issue while adding the requested tests: the matching is still greedy. Each desired entry takes the first compatible observed slot and keeps it which is fine for exact matches but an unconstrained entry (no AccessLevel/UserID/GroupID set) is compatible with any role-based observed entry. It can grab a slot that a more specific entry needed, making the result depend on entry order:

specA := &v1alpha1.BranchAccessDescription{}                      // no constraints
specB := &v1alpha1.BranchAccessDescription{AccessLevel: &level30}
gitlabLevels := []*gitlab.BranchAccessDescription{{AccessLevel: 30}, {AccessLevel: 40}}

isAccessLevelsUpToDate([]*v1alpha1.BranchAccessDescription{specA, specB}, gitlabLevels) // false
isAccessLevelsUpToDate([]*v1alpha1.BranchAccessDescription{specB, specA}, gitlabLevels) // true

Same state but different answer specA takes the level30 slot even though it could've taken 40 and leaves specB with no match. Since Update() always unprotects/re-protects on "not up to date," this reintroduces the same thrash bug behind a narrower trigger.

Replaced the greedy assignment with augmenting-path bipartite matching (tryAssign), added a test asserting both orderings agree, and re-ran make generate/make reviewable test clean.

@henrysachs take a look and let me know if you agree with this fix

[henrysachs]

also just a tip, your username seems a bit suspicious for supply chain attacks so maybe you want to make it a bit clearer who you are :)

[henrysachs]

@1872etd7q23wd please merge the latest master into your branch so the trivy finding is fixed. Else the pipeline won't let us merge 😅

[henrysachs]

I just learned I can merge anyway. NICE!