Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG]: team membership flaps constantly after release 6.4.0 apparently due to #2420 #2470

Open
1 task done
JCY-Alchemy opened this issue Nov 23, 2024 · 13 comments · May be fixed by #2481
Open
1 task done

[BUG]: team membership flaps constantly after release 6.4.0 apparently due to #2420 #2470

JCY-Alchemy opened this issue Nov 23, 2024 · 13 comments · May be fixed by #2481
Labels
Status: Triage This is being looked at and prioritized Type: Bug Something isn't working as documented

Comments

@JCY-Alchemy
Copy link

JCY-Alchemy commented Nov 23, 2024

Expected Behavior

  • teams that are referenced by name in github_repository_collaborators should be recognized in plans and applys, or changed implicitly to ID if necessary
  • teams that are repo collaborators should not flap with every plan

Actual Behavior

  • plan detects a need to update team(s) as collaborator(s) because it's comparing team name to ID number (formerly didn't do that)
  • terraform apply with the plan succeeds
  • subsequent plan detects a need to update team(s) as collaborator(s) due to name/ID number issue

Terraform Version

Terraform v1.8.3
on darwin_arm64

  • provider registry.terraform.io/hashicorp/aws v5.66.0
  • provider registry.terraform.io/integrations/github v6.4.0
  • The github provider that introduces the issue is 6.4.0 (acquired due to constraint version = "~> 6.2" in the provider)
  • Pinning version 6.3.1 (last release before 6.4.0) resolves the issue

See #2420 for the change that seems to drive this.

Affected Resource(s)

  • github_repository_collaborators

Terraform Configuration Files

### problem caused by this (auto upgrade to 6.4.0 via the version constraint)
terraform {
  required_version = "~> 1.7"
  required_providers {
    github = {
      source  = "integrations/github"
      version = "~> 6.2"
    }
  }
}

### problem not experienced (pinned to immediate previous release 6.3.1)
terraform {
  required_version = "~> 1.7"
  required_providers {
    github = {
      source  = "integrations/github"
      version = "6.3.1"
    }
  }
}

### we define collaborators like this:
collaborators = {
  users = {
    user_name_1 = "admin"
  }
  teams = {
    team_name_1 = "pull"
    team_name_2 = "pull"
  }
}

### we pass collaborators to a repo-configuring module like this:
variables.tf:
...
variable "collaborators" {
  type        = map(map(string))
  default     = { users = {}, teams = {} }
}

### we unpack user/team collaborators into github_repository_collaborators like this in the module

module.tf:
...
resource "github_repository_collaborators" "my_repo_collab" {
  repository = local.repo_name

  dynamic "user" {
    for_each = local.collaborators_users
    content {
      permission = user.value
      username   = user.key
    }
  }
  dynamic "team" {
    for_each = local.collaborators_teams

    content {
      permission = team.value
      team_id    = team.key
    }
  }
}

Steps to Reproduce

notes

  • If I replace the team name with the team id number in source code, the problem goes away, but our source has the names of teams, not their ID numbers
  • We do not define users and teams in terraform, so those objects are not available. We only have the names.

configure

  • use github provider version 6.4.0

plan

terraform plan

  # module.repo_REPO_NAME.github_repository_collaborators.my_repo_collab will be updated in-place
~ resource "github_repository_collaborators" "my_repo_collab" {
        id             = "REPO_NAME"
        # (2 unchanged attributes hidden)

      - team {
          - permission = "admin" -> null
          - team_id    = "4444444" -> null
        }
      - team {
          - permission = "pull" -> null
          - team_id    = "8888888" -> null
        }
      - team {
          - permission = "push" -> null
          - team_id    = "5555555" -> null
        }
      - team {
          - permission = "push" -> null
          - team_id    = "7777777" -> null
        }
      + team {
          + permission = "admin"
          + team_id    = "name-of-team-1"
        }
      + team {
          + permission = "pull"
          + team_id    = "name-of-team-2"
        }
      + team {
          + permission = "push"
          + team_id    = "name-of-team-3"
        }
      + team {
          + permission = "push"
          + team_id    = "name-of-team-4"
        }
    }

Plan: 0 to add, 1 to change, 0 to destroy.

### apply

terraform apply
module.repo_REPO_NAME.github_repository_collaborators.my_repo_collab: Modifying... [id=REPO_NAME]
module.repo_REPO_NAME.github_repository_collaborators.my_repo_collab: Modifications complete after 3s [id=REPO_NAME]

Apply complete! Resources: 0 added, 1 changed, 0 destroyed.

### plan again
terraform plan

(same plan is generated as above)

Debug Output

No response

Panic Output

No response

Code of Conduct

  • I agree to follow this project's Code of Conduct
@JCY-Alchemy JCY-Alchemy added Status: Triage This is being looked at and prioritized Type: Bug Something isn't working as documented labels Nov 23, 2024
@JCY-Alchemy JCY-Alchemy changed the title [BUG]: team membership flaps constantly after release 3.40 apparently due to #2420 [BUG]: team membership flaps constantly after release 6.4.0 apparently due to #2420 Nov 23, 2024
@al-lac
Copy link

al-lac commented Nov 26, 2024

I have the exact same issue.

@robinbowes
Copy link

Another "me too" comment.

@tiagoasousa
Copy link

+1

@stevehipwell
Copy link
Contributor

PR #2420 fixed the consistency model as the provider currently uses team ID internally for all team resources. You can stop the churn by using a github_team data source to lookup your team ID and you will be using fewer API requests than the old resource used to make prior to #2420.

@robinbowes
Copy link

Take a look at the hoops I've had to jump through to convert team slugs into ids for a bunch of repos defined in human-curated json files, and tell me again how great it is that the provider uses fewer API requests:

locals {
  repo_files    = fileset(path.module, "repository_data/*.json")
  raw_repo_data = [for f in local.repo_files : jsondecode(file(f))]

  repo_data = [
    for repo in local.raw_repo_data : {
      for k, v in repo : k =>
      k == "collaborators" ?
      {
        for k2, v2 in v : k2 =>
        k2 == "teams" ? [
          for block in v2 : {
            for k4, v4 in block : k4 =>
            k4 == "slug" ? local.team_ids[v4] : v4
          }
        ]
        : v2
      }
      : v
    }
  ]

  # Get the set of github team slugs used by all repositories
  team_slugs = toset(
    flatten( # W: local.teams is declared but not used
      [
        for name, repo in local.raw_repo_data : [
          for team in lookup(repo.collaborators, "teams", []) :
          team.slug
        ]
      ]
    )
  )

  team_ids = {
    for team in data.github_team.this : team.slug => team.id
  }
}

data "github_team" "this" {
  for_each = local.team_slugs
  slug     = each.value
}

@JCY-Alchemy
Copy link
Author

JCY-Alchemy commented Nov 28, 2024

Concur with the above.

I see what we /could/ do, but if you look at the example in the other comment, we're effectively asked to implement all the logic that should be in a provider, on top of the provider in the resource definition. At the topmost abstraction, the resource definition layer (our terraform code) lookups of foundational references should be invisible... and the contested provider code clearly shows this behavior (but flipped from invisibly looking up one thing to invisibly looking up some other thing instead)

@stevehipwell
Copy link
Contributor

@robinbowes @JCY-Alchemy I've just re-checked my notes and this is a defect as I'd intended to not churn on slugs. There are no automated acceptance tests here (I'm working on it) and I don't work at GitHub so don't have easy access to test infrastructure. I've got a PR open to work on this resource so I'll figure out why this isn't working and add a fix.

FYI the reason the number of API call matter is that prior to the refactor this resource was unusable at anything approaching scale.

@stevehipwell stevehipwell linked a pull request Nov 28, 2024 that will close this issue
4 tasks
@devopsrick
Copy link

Anyone able to work on the PR referenced here? This bug is causing serious churn (and causing API rate limiting) for us.

@stevehipwell
Copy link
Contributor

@devopsrick there are a number of PRs outstanding tha haven't even had a review.

The following code might be more efficient than defining each team as a data lookup, and it should have a minimal impact on your actual code. The number of GitHub API calls will be comparable or fewer than by passing the slug directly into the resource.

data "github_organization_teams" "all" {
  summary_only = true
}

locals {
  teams = {
    "my-team"       = "pull"
    "my-admin-team" = "admin"
  }

  team_id_lookup = { for t in data.github_organization_teams.all.teams : t.slug => t.id }
}

resource "github_repository_collaborators" "example" {
  repository = var.repo_name

  dynamic "team" {
    for_each = local.teams

    content {
      permission = team.value
      team_id    = local.team_id_lookup[team.key]
    }
  }
}

But if you do want to define each team the same pattern also works.

data "github_team" "lookup" {
  for_each = local.teams

  slug         = each.key
  summary_only = true
}

locals {
  teams = {
    "my-team"       = "pull"
    "my-admin-team" = "admin"
  }

  team_id_lookup = { for t in data.github_organization_teams.lookup : t.slug => t.id }
}

resource "github_repository_collaborators" "example" {
  repository = var.repo_name

  dynamic "team" {
    for_each = local.teams

    content {
      permission = team.value
      team_id    = local.team_id_lookup[team.key]
    }
  }
}

@devopsrick
Copy link

@stevehipwell Thank you for the suggestion but unfortunately that pattern will not work for us as we use modules to create repos and manage permissions and we pass the team slug to the module. If we add data sources to look up the id of each team within the modules this will exponentially increase the API calls made as each module will refresh its own data sources resulting in hundreds of duplicate calls.

For context we manage 760 repositories and 193 teams accessing various. The volume of access across repositories makes it a very delicate process to update more than a few repository collaborator lists at any time. The churn of replacing every repository permission list every time is causing pretty bad issues for us.

The other option is to rework our code entirely to pass team ids instead of slugs but that also require retraining every member who works with our codebase (quite a few) and that isn't really an option.

@stevehipwell
Copy link
Contributor

@devopsrick the code behind the contributors makes a significant number of API calls if you're using slugs but I guess this only happens when there are changes.

After my current in flight PRs are merged, so there are acceptance tests as part of a PR, I plan on refactoring the codebase to use team slugs directly. I can do this for collaborators in the current pending PR, but that doesn't help without a review.

Sorry, but I don't have an easy solution for you.

@stevehipwell
Copy link
Contributor

@devopsrick you might be able to use the second pattern from above with a lifecycle { ignore_changes = all } set on the data source (I'm not sure if this is valid or even works) as the ID to slug mapping should never change.

@robinbowes
Copy link

@stevehipwell Thank you for the suggestion but unfortunately that pattern will not work for us as we use modules to create repos and manage permissions and we pass the team slug to the module. If we add data sources to look up the id of each team within the modules this will exponentially increase the API calls made as each module will refresh its own data sources resulting in hundreds of duplicate calls.

For context we manage 760 repositories and 193 teams accessing various. The volume of access across repositories makes it a very delicate process to update more than a few repository collaborator lists at any time. The churn of replacing every repository permission list every time is causing pretty bad issues for us.

The other option is to rework our code entirely to pass team ids instead of slugs but that also require retraining every member who works with our codebase (quite a few) and that isn't really an option.

Steve,

What I do as a workaround is:

  • generate a list of all the slugs used in my data files
  • create a data source to lookup the team_id for each slug
  • generate a map of team_ids, keyed on slug
  • munge the input data, replacing the team slug with the team id

Something like this:

locals {
  # Read in the raw repo data
  repo_files    = fileset(path.module, "repository_data/*.yaml")
  raw_repo_data = [for f in local.repo_files : yamldecode(file(f))]

  # Get the set of github team slugs used by all repositories
  team_slugs = toset(
    flatten(
      [
        for name, repo in local.raw_repo_data : [
          for team in lookup(repo.collaborators, "teams", []) :
          team.slug
        ]
      ]
    )
  )
}

# Look up team ids
data "github_team" "this" {
  for_each = local.team_slugs
  slug     = each.value
}

locals {
  # create a map of team ids, keyed on slug
  team_ids = {
    for team in data.github_team.this : team.slug => team.id
  }

  # Process the raw repo data, replacing team slug with team id
  repo_data = [
    for repo in local.raw_repo_data : {
      for k, v in repo : k =>
      k == "collaborators" ?
      {
        for k2, v2 in v : k2 =>
        k2 == "teams" ? [
          for block in v2 : {
            for k4, v4 in block : k4 =>
            k4 == "slug" ? local.team_ids[v4] : v4
          }
        ]
        : v2
      }
      : v
    }
  ]
}

@nickfloyd nickfloyd moved this from 🆕 Triage to 🔥 Backlog in 🧰 Octokit Active Dec 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Triage This is being looked at and prioritized Type: Bug Something isn't working as documented
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants