feat: add support for GitLab groups

[peikk0]

what

Add support for GitLab groups in the commmand allowlist and policy owners.

Unlike with GitHub, the GitLab API doesn't allow retrieving all group membership for a given user (unless you have admin access), so here instead we check in each configured group if the user is an active member of it.

why

• To be able to group permissions with GitLab instead of only users
• Feature parity with GitHub

tests

• make test
• Configured ATLANTIS_GITLAB_GROUP_ALLOWLIST and policy owners with one of my groups and then with a group I'm not a member of instead, then tried an atlantis plan and atlantis approve_policies in a merge request, and I was respectively allowed and denied in each case as expected.

references

Closes #2549

[peikk0]

@jamengual hi, any chance this could make it into the next release? It would be really helpful for us at GitLab, and for all GitLab users. 🙏🏻

[jamengual]

yes, we just need to review it

[lukemassa]

One concern I have with this implementation is introducing more differences between the VCSs instead of unifying them. I wonder if you could instead piggyback on the existing gh-team-allowlist (then if that works out we can work on renaming). I'm also not sure about not respecting configuredTeams in the non-gitlab VCSs; if it's going to be part of the interface it should be taken into account, at least by those VCSs that have a non-trivial implementation (i.e. github).

Another related thought, I wonder if configuredTeams is not really part of the "interface" of the VCSs, but rather is an "implementation detail" of gitlab, because of this limitation about lookups. Thus I wonder if it would be easier to pass the logic in somehow via NewGitlabClient then be able to access it via g in (g *GitlabClient) GetTeamNamesForUser()`.

There's definitely a larger issue here about feature parity and options between the VCSs, so I don't want to bog us down too much on this particular PR, but just to say the fact that this is tricky is part of a larger problem and not the fault of this particular functionality.

The last thing I'll say is that I'm a gitlab user, and the way we deal with this kind of issue is via Merge Request Approval Rules (https://docs.gitlab.com/ee/user/project/merge_requests/approvals/rules.html), which atlantis respects if the approved apply requirement is set.

[peikk0]

Thus I wonder if it would be easier to pass the logic in somehow via NewGitlabClient then be able to access it via g in (g *GitlabClient) GetTeamNamesForUser()`.

That wouldn't work nicely for the policy checks, as that would require instantiating a new client with a different set of groups for each project.

the way we deal with this kind of issue is via Merge Request Approval Rules

That works fine as an alternative for the command allowlist, but not for the policy check approvals.

[lukemassa]

That wouldn't work nicely for the policy checks, as that would require instantiating a new client with a different set of groups for each project.

I'm not sure why; NewGitlabClient() is only ever called once, in server/server.go, don't we have enough information at that point to know what all the possible groups that might be needed, then we can store that as part of the client itself, and GetTeamNamesForUser can use that to seed its list?

Just to clarify, is the reason we aren't simply listing all groups then calling "is member of" on each of them an efficiency thing? I understand only Admins can list full group membership for a user, but atlantis user could list every group that is visible to it, and we're already silently ignoring groups atlantis can't see, so it should be functionally the same, right? I'm not necessarily proposing that implementation, just making sure I understand the issue.

Fundamentally it feels weird to me to pass into the function GetTeamNamesForUser() a list of possible teams to choose from. That said, if we do decide to go down that route, I do think that the github client should respect the configuredTeams parameter that it's passed.

Re the reuse of gh-team-allowlist, my current thought is, this PR would stay as gitlab-team-allowlist, then later if we decide the API is correct we create a new flag group-allowlist and deprecate the other two. Thoughts @jamengual ?

[jamengual]

I agree with you @lukemassa we can deprecate later.

[peikk0]

I'm not sure why; NewGitlabClient() is only ever called once, in server/server.go, don't we have enough information at that point to know what all the possible groups that might be needed, then we can store that as part of the client itself, and GetTeamNamesForUser can use that to seed its list?

I guess we can do that, yes. 🤔

atlantis user could list every group that is visible to it, and we're already silently ignoring groups atlantis can't see, so it should be functionally the same, right?

That might work on small private instances but certainly not on GitLab.com, you would hit the rate limit very quickly and even if you didn't it would take literal days to check every public group. 😄

Re the reuse of gh-team-allowlist, my current thought is, this PR would stay as gitlab-team-allowlist, then later if we decide the API is correct we create a new flag group-allowlist and deprecate the other two.

Unless I missed something I think Atlantis can be configured to use both GitHub and GitLab at the same time, so you would still need 2 separate allowlists. In theory you could want to allowlist team foo in GitHub but not group foo in GitLab.

[lukemassa]

Unless I missed something I think Atlantis can be configured to use both GitHub and GitLab at the same time, so you would still need 2 separate allowlists. In theory you could want to allowlist team foo in GitHub but not group foo in GitLab.

Ah that's a good point. If we were to have some theoretic single flag that handled both of these, it would have to handle that somehow. I think here we should follow the rule of thumb of "don't try to make a generalized abstraction until you do something three times" :) If/when we do this for another VCS, at that point it will hopefully be obvious the cleanest way to do this.

That might work on small private instances but certainly not on GitLab.com, you would hit the rate limit very quickly and even if you didn't it would take literal days to check every public group. 😄

Oh absolutely, my question was mostly about correctness. Basically that the list we're looking for is the intersection of "all groups user X is in" and "all groups atlantis can see". Your code does a further intersection, which is "all groups we might have policy around". Again, just making sure I understand the logic.

[jamengual]

@peikk0 @lukemassa, do we agree on an approach to process another review/code changes?

[github-actions]

This issue is stale because it has been open for 1 month with no activity. Remove stale label or comment or this will be closed in 1 month.

[dorian-tsorfy]

@lukemassa Any updates?
This PR will really help us and many others working with GitLab

[peikk0]

@dorian-tsorfy I'll try to have the patch updated this week. 🤞🏻

[peikk0]

I have rebased/updated the diff but kept the same logic.

While doing so, I remembered why I initially didn't opt for building the list of all groups when initialising the client: in large organisations with many teams the list of groups could grow quite large, each operation would query all of them every single time instead of just a handful configured for the command or policy, which would slow down operations, even though it might not be that significant. Slightly more code complexity vs operation efficiency, I'm not sure which way is best here.

[dorian-tsorfy]

Any updates?

[lukemassa]

While doing so, I remembered why I initially didn't opt for building the list of all groups when initialising the client: in large organisations with many teams the list of groups could grow quite large, each operation would query all of them every single time instead of just a handful configured for the command or policy, which would slow down operations, even though it might not be that significant. Slightly more code complexity vs operation efficiency, I'm not sure which way is best here.

You don't have to loop over all the groups, you can just loop over the configured ones, as you're doing now. I wrote up a PR fe1e359 (not to be merged just as demonstration, feel free to cherry-pick the commit if it helps. Not totally sure what happened with the generated mocks there might want to avoid those...).

Performance wise this should be the same as yours, except that we loop over all teams for all commands, which I would imagine in practice is not a big issue.

Again my main concern is changing the signature of the client interface for GetTeamNamesForUser() to add a parameter that other VCSs have to know to ignore. If there's going to be more complexity, it should be localized to the gitlab client, which has this implementation quirk.

[dorian-tsorfy]

@peikk0 @lukemassa
Is there something that blocks this PR from being merged?

[jamengual]

@peikk0 will have to address @lukemassa comment and try to see if the approach work to get to an agreement.

[peikk0]

@lukemassa OK that sounds good, it will also need to grab all groups from all configured policy sets on init. I'll update this PR next month as I'll be mostly AFK until then.

[github-actions]

This issue is stale because it has been open for 1 month with no activity. Remove stale label or comment or this will be closed in 1 month.

[andreilapkin]

Any thoughts when it will be merged?

[peikk0]

@andreilapkin I need to implement the suggestions above first. Sorry for the delays, been AFK most of last month.

[kolt2050]

this is very important to me too.

[torquemada163]

We are really looking forward to this opportunity, it is important for our team.

[github-actions]

This issue is stale because it has been open for 1 month with no activity. Remove stale label or comment or this will be closed in 1 month.

[peikk0]

Not stale, still on my todo list.

[peikk0]

Rebased and updated with the above suggestions, seems to be working great after a few tests. Happy holidays!

[jamengual]

@lukemassa could you review this one last time? Thanks.