How do handle cases where GitLab implicitly creates a resource resulting in tf ownership confusion

[timofurrer]

There has been a discussion in #266 about what to do in the following situation:

resource "gitlab_group" "group" {
}

resource "gitlab_group_membership" "membership" {
  user_id = <ID of the user owning the GITLAB_TOKEN>
}

... in which GitLab implicitly adds group membership to the gitlab_goup.group for the user owning the token which was used to create the group. This leads to a group membership already exists error for the gitlab_group_membership.membership resource.

The following questions need to be answered:

• [ ] Is this behavior a bug in the gitlab terraform provider?
• [ ] If yes, how should the gitlab_group_membership resource behave?

[armsnyder]

This relates to #749. Generally, what do we want to do when GitLab creates an upstream resource out-of-band that would map onto another Terraform resource?

[armsnyder]

The main issue that a lot of folks are facing right now has to do with the default branch protection when you create a GitLab project, but this is a problem in a lot of situations like the gitlab_group_membership case, where creating a group also automatically adds the requesting user as a member.

Let's brainstorm some options. I can think of 2:

1. Strictly follow the GitLab API. Allow GitLab to set its defaults like group members and branch protections, and require Terraform users to manually import those resources.
   • Pros: Adheres to GitLab API, so less risk in dealing with breaking changes to the provider that diverge from the GitLab API in the future.
   • Cons: Bad Terraform user experience, requiring multiple applies and imports.
2. Support new attributes on these resources gitlab_project and gitlab_group that let the user opt-into behavior that would allow them to manage these resources. For example gitlab_project could have an attribute disable_branch_protection which defaults to false. When true, we would have logic in the Create function to explicitly remove the branch protection created by GitLab for a project initialized with a branch and README.
   • Pros: Better Terraform user experience, allowing them to apply config in a single pass.
   • Cons: More tech debt, diverges from GitLab API

I'm swaying toward option 2 personally. Thoughts?

[timofurrer]

I don't see any additional options than these two as well and I agree with your pros and cons. I'm also wondering if other providers have the same issue - I'll do some research in the next few days.

I'm also favoring option 2 right now, as long as the GitLab API default behavior can be enabled / disabled (not yet sure what the default should be, I personally never had the issue that I managed the default branch protection explicitly, so 🤷 )

[flybd5]

As I mentioned in another ticket I had created but turned out to be a dup of this, the behavior we are seeing here is a failure of idempotency. If I create a repo with a readme file, that creates a master/default branch with branch protection according to the Gitlab settings. That the master/default branch already exists when I try to set branch protection of what I just created is not an error condition and is irrelevant to branch protection. The desired state is a branch with protection. If the actual state matches the desired state, that should never lead to a failure.

[armsnyder]

@flybd5 Yep, I think we agree on the problem, which is that two resources are competing over ownership of this desired state. What do you think about the two options I laid out, or do you see an alternative?

[flybd5]

It's not that two resources are competing, for me it happens with the single gitlab_project resource I just created.

resource "gitlab_project" "customer-project" {
    name = <give it a name>
    namespace_id = var.gitlab_group_namespace_id
    initialize_with_readme = true
    request_access_enabled = true
    visibility_level = "private"
}

resource "gitlab_branch_protection" "master" {
  project = gitlab_project.customer-project.id
  branch = "master"
  push_access_level = "maintainer"
  merge_access_level = "maintainer"
}

This fails saying branch already exists. So? I am not creating a branch, I just want to make sure it is protected.

Option 1 is a bad idea, and as to option 2, I don't understand why the current state of the Gitlab project I just created is not used, compared against the desired state, and if they match, nothing happens. This is fully supported by the Gitlab API. If the problem is that TF is not making that info available and you can't query the API yourself, then this is a serious shortcoming of TF that they need to address.

[timofurrer]

@flybd5 the thing is that every resource (here gitlab_project and gitlab_branch_protection) have their own underlying terraform state. AFAIK (or is there?!) there is no way to share this state between those resources implicitly.

Now the problem is: which resource is the "single source of truth" for the master branch protection? ... That question can't be answered by terraform right now and leads to the error you are seeing ...

[flybd5]

@timofurrer You know the answer: Gitlab Project -> Branch -> Branch Protection. The source of truth is the project, the rest are attributes of the project. The real problem is the design -- someone has separated those into individual resources and given them independent states artificially separate of each other, and now the flawed design is getting in the way of aligning to the real world. If someone tries to address this with workarounds rather than fixing the design, it will only make it worse.

[timofurrer]

I think I can only partially agree to that, because the Branch Protection is a separate (REST) resource - although they are always within a project. However, would your reasoning be the whole true, then most of the resources which exist today would be part of either the project or group resource ... which wouldn't make the design any better.

The root cause of this "design dilemma" is that the GitLab API creates sub resources implicitly (which isn't usually a problem, but here by the nature of how terraform resources work, it is one :) )

[flybd5]

Having a separate REST call to query an attribute doesn't make it independent. Look, I can see you're going to come up with an exaggerated rationalization as an answer to anything I say, so I will no longer waste my time with this thread. I have zero interest in debating simple facts. The design for this is flawed. Fix it.

[Stromweld]

I somewhat agree with flybd5, and would like to point to the aws provider for security groups as a simple example but there are many others in that provider like aws_instance resource as well that use multiple blocks like the ingress and egress blocks in the aws_security_group resource. https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/security_group With gitlab_project we should be able to specify branch attribute, protected_branch, and possibly others with multiple blocks being able to describe multiple branches etc.... similar to the aws_security_group ingress and egress blocks. Having seperate resources for those is also good much like the aws_security_group_rules. Gives you options and ways to avoid circular references etc...

[Stromweld]

I think being able to combine more into the main project and group resource will also save time/enhance performance of the state refresh. As it is I use this provider along with the tfe one for managing our repos and workspaces. We have about 100 repos and 300 workspaces. Because it takes about 5-6 resources from this repo to define all of our repo parameters and Mr_approvals, branch_protection etc… our pipelines take over 4 minutes to simply refresh state during a tf run as it’s parsing each resource and making separate api calls. If it could parse 1 resource and make multiple api calls for all the parts of it that would significantly speed things up.

With that I don’t know how that’d translate into code. I’d hope the AWS provider source could could give some good examples.

[armsnyder]

@flybd5 We are all trying to find a solution for this problem, given the technical constrains of the Terraform declarative model. This is a place for productive discussion around an issue. 🙂 I politely request everyone to read the community standards section of the GitLab Community Code of Conduct.

@Stromweld Thanks for the suggestion, let's call that option 3. I can see that working for the two examples we have brought up so far:

resource "gitlab_project" "this" {
  name                   = "foo"
  initialize_with_readme = true

  protected_branch {
    name = "main"
  }

  protected_branch {
    name = "release/*"
  }
}

where "main" is the default branch, and

resource "gitlab_group" "this" {
  name = "foo"

  member {
    user_id      = 1
    access_level = "developer"
  }

  member {
    user_id      = 2
    access_level = "maintainer"
  }
}

where "1" is the user ID of the user token used to run Terraform.

And if we were going to consider wanting a strict set of group members (see #453) or protected branches, we could support:

resource "gitlab_group" "this" {
  name = "foo"
  members = [
    {
      user_id      = 1
      access_level = "developer"
    },
    {
      user_id      = 2
      access_level = "maintainer"
    },
  ]
}

or would that be too confusing and error prone?

[armsnyder]

I share @timofurrer's concern about having one resource calling multiple APIs. It's a lot harder to manage from a design and testing standpoint. If we only need this kind of workaround for a handful of resources, I think it's acceptable, but I wouldn't want the gitlab_project resource to get too bloated.

[Stromweld]

For option 3 here I wouldn't recommend being explicit with an array, a hash would be more inclusive so if other members are added using the group_membership for example in another repo it should be removed during an update to primary group resource repo.

[timofurrer]

@Stromweld yes, that sounds like a viable solution, too :)

@armsnyder I'd also limit ourselves to a few project subresources to start with, if we'd go that way.

What do you think about planning something for v4 ?

[armsnyder]

Ok I think we've got something that works then.

For gitlab_project and gitlab_group, we can add protected_branch and member blocks, respectively. These blocks are not all-inclusive, so they still allow users to use gitlab_branch_protection and gitlab_group_membership resources in conjunction with the main resources.

If we wanted to, we can also make a new resource like gitlab_group_all_members to solve the #453 case of wanting tighter control of group members. I don't foresee any conflicts here.

Sound good @timofurrer @Stromweld?

@timofurrer This doesn't seem like a breaking change to me, so it could be in the current major version. Thoughts?

[Stromweld]

I like it.

[timofurrer]

Sounds good, and yes you are right @armsnyder , isn't breaking :)

[github-actions[bot]]

Marking this issue as stale due to 120 days of inactivity. If this issue receives no comments in the next 30 days it will be closed. Maintainers can also remove the stale label.

Please comment with more information if you would like this issue to remain open.

[timofurrer]

In case anyone is interested to discuss some mitigation for the protecting the default branch: https://github.com/gitlabhq/terraform-provider-gitlab/pull/1216