Fixes team membership bug

[samrees]

Fixes #323
Closes #326

The current code parses username and team_id from the returned URL with code
that expects the URL to be constructed in a certain fashion. Today that changed,
in the form of "teams/id" to "team/id", making team_id go missing.

Since the upstream API is only meant for obtaining membership status/type, with
the URL being an opaque reference back to itself, we shouldn't depend on that URL
being anything in particular. So this sets our data, if the API responds, to the
username and team_id we already had to make the call.

[samrees]

Since the github API change this morning it probably should be noted that this resource is currently broken for everyone on all? versions of this provider.

[soerenmartius]

Since the github API change this morning it probably should be noted that this resource is currently broken for everyone on all? versions of this provider.

that's true indeed. This should be released asap.

[steakfest]

So this sets our data, if the API responds, to the username and team_id we already had to make the call.

On the surface this seems like the right thing to do. I was wondering why the provider is gathering data from the API call that should already know. But I am not really familiar with the overall architecture of the provider, so I proposed an alternative PR, in case it's something we can get deployed faster. https://github.com/terraform-providers/terraform-provider-github/pull/326

However, I'm totally on board with this PR if the maintainers are comfortable releasing asap.

Thanks @samrees !

[samrees]

If you need this quick:

FROM golang:1.11-alpine as build-github

RUN apk add git make bash build-base

RUN git clone https://github.com/samrees/terraform-provider-github.git && \
cd terraform-provider-github && \
git checkout team_membership_bugfix && \
make build

FROM hashicorp/terraform:0.12.19

RUN mkdir -p /root/.terraform.d/plugins

COPY --from=build-github /go/bin/terraform-provider-github /root/.terraform.d/plugins

Same, I'm not familiar with how this provider works specifically. Since that URL parsing function was from the initial commit I had an idea today to go through all the code and look for more instances of this.

Theres a chance I'm missing something, that Github just made a mistake and broke their API, but I don't think so. I think it makes sense that Github thought they could change this value without consequence, because all the _url values are supposed to be opaque references.

[Jonnymcc]

So I tried this fix Note: I actually tried the fix in #326 and mistakenly commented on this PR. The fix in #326 worked for me, but in my particular case (where the state shows no team_id for these resources) it still shows

1 error occurred:
	* github_team_membership.Managers-Jonnymcc (destroy): 1 error occurred:
	* github_team_membership.Managers-Jonnymcc: Unexpected ID format (""), expected numerical ID. strconv.ParseInt: parsing "": invalid syntax

It only works after having imported the resource again.

[steakfest]

@Jonnymcc based on what I saw in the tfstate file yesterday while debugging this. You may want to roll back to a previous state file. One that doesn't have empty strings for the team_id value on these resources.

I used version S3 so it was easy to find the state file before the provider started breaking it.

That can save you from the toil of having to import a lot of membership resources if you have them.

[samrees]

@steakfest is right on rolling back to a previous version of the statefile is the right choice. On of the off chance you can't rollback (or were dumb like me and thought it was all my fault), heres a ruby script to repair a statefile from data thats already there:

def gen_cmds(r)
  id = r['instances'].first['attributes']['id']
  [
    "terraform state rm github_team_membership.#{r['name']}",
    "terraform import github_team_membership.#{r['name']} #{id}"
  ]
end

data = JSON.parse(File.read('terraform.tfstate'))
cmds = data['resources'].flat_map{|r| gen_cmds(r) if r['type'] == "github_team_membership" }.compact

f1 = File.open('cmds', 'w')
f1.write cmds.join("\n")

# run `bash -eux cmds` to re-import all your git team membership resources

# populates the `team_id` field from the existing data
def fix(r)
  id = r['instances'].first['attributes']['id'].split(':').first
  r['instances'].first['attributes']['team_id'] = id
end

data = JSON.parse(File.read('terraform.tfstate'))
data['resources'].each{|r| fix(r) if r['type'] == "github_team_membership" }

f2 = File.open('terraform-modified.statefile', 'w')
f2.write JSON.pretty_generate(data)

# mv terraform-modified.statefile terraform.tfstate

[dbussink]

Theres a chance I'm missing something, that Github just made a mistake and broke their API, but I don't think so. I think it makes sense that Github thought they could change this value without consequence, because all the _url values are supposed to be opaque references.

👋 from GitHub here. We did upgrade this URL because we are long term deprecating the current endpoint and moving it to always reside under the organization that owns the team. We do this to ensure the future scalability of GitHub where a direct reference by id with no scope to a team poses a challenge for us in this area.

You're also 100% right that we indeed assume that people treat the URLs as opaque references, and that parsing it / interpreting it like this is not a supported usage of these URLs. I think that the fix in this PR makes sense and is the right fix for the urgent issue right now.

As for the longer term move to the new endpoints, that's something that can happen then at a later time and that we would also be happy to help with if needed.

[eerkunt]

@dbussink any plan for a release either this PR or #326 ?

This is literally blocking everyone who manages their GitHub Organisations/Teams via terraform.

[dbussink]

@dbussink any plan for a release either this PR or #326 ?

@eerkunt I'm not myself involved in this project so I have no way to do a release, but I wanted to clarify on what happened here from the GitHub side.

[meabed]

If you need this quick:

FROM golang:1.11-alpine as build-github

RUN apk add git make bash build-base

RUN git clone https://github.com/samrees/terraform-provider-github.git && \
cd terraform-provider-github && \
git checkout team_membership_bugfix && \
make build

FROM hashicorp/terraform:0.12.19

RUN mkdir -p /root/.terraform.d/plugins

COPY --from=build-github /go/bin/terraform-provider-github /root/.terraform.d/plugins

Same, I'm not familiar with how this provider works specifically. Since that URL parsing function was from the initial commit I had an idea today to go through all the code and look for more instances of this.

Theres a chance I'm missing something, that Github just made a mistake and broke their API, but I don't think so. I think it makes sense that Github thought they could change this value without consequence, because all the _url values are supposed to be opaque references.

this is not working, compiled on darwin.

[damm]

@meabed I needed to deploy this without Docker so I'll give you my quick instructions.

Requirements: GVM

I used go1.12.15 because I got an issue with crypto requiring go1.12 yet the Dockerfile specifies 1.11 which I could not get to compile it for anything on Ubuntu 19.04

gvm install go1.12.15 --binary
gvm use go1.12.15
git clone https://github.com/samrees/terraform-provider-github.git
git checkout team_membership_bugfix
make build
mkdir -p ~/.terraform.d/plugins/darwin_amd64
cp $GOPATH/bin/terraform-provider-github ~/.terraform.d/plugins/darwin_amd64/terraform-provider-github

You may need to symlink it as terraform-provider-github_v2.2.1_x4 as well if terraform init still tries to download and install the plugin

Anyone else using this instructions modify darwin_amd64 for linux_amd64 or whatever.

[jcudit]

I think this is good to 🚢 despite not being able to get acceptance tests passing.

TestAccGithubTeamMembership_basic and TestAccGithubTeamMembership_caseInsensitive began failing on January 17th prior to this change being published. I'm able to get passing tests if I toggle ImportStateVerify to false, which gives me confidence that this change has no broader effects on functionality. I'd like to spend more time understanding the failure but I do not want to delay getting a potential fix released.

[meabed]

@jcudit Thanks! would this be merged soon?

[jcudit]

@meabed I'd like another set of Hashicorp eyes on this before merging and have pinged for review. I am unable to cut releases as I am still onboarding but would expect things to pick up now that the extended weekend has passed. If merging this to master helps get the changes into affected organizations, I'm comfortable doing so after a couple of days to allow for another reviewer to jump in.

Apologies for hesitating on this one. I'm still new 🙃.

[eerkunt]

Apologies for hesitating on this one. I'm still new 🙃.

I think you became quite famous on your first days with this world-wide effecting issue :)

[rndstr]

make build

if you build this locally and then push it somewhere, make sure to do CGO_ENABLED=0 go install instead (to get a statically linked library at $GOPATH/bin/terraform-provider-github)

[stefansedich]

Any ETA on getting this in @jcudit it is unfortunate that this happened to us the day we had a new-hire start 😆 causing some grief getting them provisioned.

[steakfest]

I'm not sure you anybody else noticed this, so I'm share my experience.

I've found that because of the order of operations by the provider, that generally what I want to happen is still happening. Since the apply fails to delete after any new memberships have been created, this hans't blocked me from being able to add new members to teams, since terraform doesn't roll back.

[stefansedich]

I just noticed this too @steakfest my new user was at least added even with the errors which is enough for now.

[meabed]

I am not sure if this fix is working correctly.
It’s adding users, but next time you apply to destroy them and then it fail on destruction again.
Is this the case for anyone? I have built it like 20 times :)

[radeksimko]

Thank you @samrees for raising the PR with patch promptly.

Your patch does a good job in fixing the bug for any new team memberships. I just pushed a small change to your branch to allow reconciliation, so that folks don't have to re-import resources or otherwise tinker with the state. I will explain bellow why that is necessary.

I pushed this as a separate commit, so your original contribution is still credited to you.

---

Detailed explanation for anyone interested

Basically what happened after the upstream API change is that the provider code parsed out empty string and so team_id was set to empty string. Therefore anyone who attempted to refresh, apply, destroy after the API change (17th January) ended up with empty team_id in state.

Read() function which runs during refresh stage (refresh, and by default apply/destroy) normally allows reconciliation from such inconsistent state, but because there was a short-circuiting (effectively caching) logic which prevented it from making any changes if nothing has changed ("304 Not Modified" was received from the API). The change I made just sets these fields regardless.

All relevant acceptance tests (TestAccGithubTeamMembership_*) are now passing.

---

State manipulation

Reverting state to a previous version is possible, assuming your state is versioned, although you may run into a situation where state is drifted, if you made any genuine changes to config and/or in GitHub manually.

Generally speaking we always discourage from manipulating the state file outside of Terraform. Terraform offers commands (terraform state or terraform import) to allow manipulation on resource or data source level, which is considered safe. When using these commands the worst case scenario is that you will end up in a state reproducible by anyone else.

Changing state manually may get you into a state which is hard to recover from (because the provider code can make assumptions about which fields are set to what and when) and more importantly if you run into a problem/bug we are less likely be able to help as the problem will be unreproducible for us.

---

Recovery

When this patch is applied and released it should fix the problem for majority of users automatically (without having to touch the state) during the next apply or destroy. Those which for some reason employ -refresh=false will need to either run terraform refresh manually to reconcile the state, or run apply/destroy without this flag.

---

Thanks everyone for the patience.

[thanasisk]

Hello everyone and thanks for the great work! When can we expect a new release of the GH provider that will contain this fix?

[radeksimko]

2.3.0 version with this patch was just released.

Feel free to upgrade by setting version = "~> 2.3" in your config, which will get picked up by terraform init and upgrade the provider to the latest (patched) version.

e.g.

provider "github" {
  version = "~> 2.3"
}

---

Recovery (after upgrading)

As mentioned in my previous comment, upgrading the provider alone should resolve the problem for majority of users automatically during the next apply or destroy.

Those which for any reason employ -refresh=false will need to either run terraform refresh to reconcile the state, or run apply/destroy without this flag.