integrations / terraform-provider-github

Terraform GitHub provider
https://www.terraform.io/docs/providers/github/
MIT License
896 stars 740 forks source link

anonymous=true should ignore GITHUB_TOKEN env #354

Open nyurik opened 4 years ago

nyurik commented 4 years ago

Terraform Version

Terraform v0.12.20
+ provider.github v2.3.2
+ provider.google v2.20.2
+ provider.random v2.2.1

Affected Resource(s)

Terraform Configuration Files

provider "github" {
  version      = "2.3.2"
  organization = "something"
  anonymous    = true
}

Expected Behavior

If provider "github" contains a token, then yes, it should be an error (because it can be easily fixed by the user), but if it is coming from the env variable GITHUB_TOKEN, then in some cases there is not much user can do -- e.g. if terraform runs as part of the Jenkins job, which has to have access to Github in order to get the code in the first place. Setting anonymous=true should still be possible to prevent the provider from using env variable token.

Actual Behavior

20:34:40 Error: If `anonymous` is true, `token` cannot be set.
20:34:40 
20:34:40   on main.tf line 5, in provider "github":
20:34:40    5: provider "github" {

Steps to Reproduce

Please list the steps required to reproduce the issue, for example:

  1. Set GITHUB_TOKEN to a value
  2. create an anonymous Github provider as described above
  3. terraform plan
kpfleming commented 4 years ago

Agreed. This is an example of implicit behavior, which I hate, because having it means we have to have knobs to turn it off. It's already easy to bring an environment variable into Terraform, users should just have to do that :-)

nyurik commented 4 years ago

I second @kpfleming - a proper fix should be to NOT use any env variables automagically at all, and let everything come via predictable terraform code. If something relies on an env var, let the user explicitly state that -- a few characters will save a lot of unexpected pain.

jeet-parekh commented 4 years ago

I could try to fix this. I see two potential ways of doing it.

  1. Remove the line https://github.com/terraform-providers/terraform-provider-github/blob/master/github/provider.go#L14.

  2. Add an explicit check for anonymous=true and setting token to null in providerConfigure - https://github.com/terraform-providers/terraform-provider-github/blob/master/github/provider.go#L112

Which one would be a proper fix?

nyurik commented 4 years ago

@jeet-parekh I think the best fix would be to go with option 1, and per above discussion, remove DefaultFunc for token, organization, and base_url (instead set Default to nil, nil, "" correspondingly). This would be a breaking change, thus requiring 3.x release.

To help with the migration, you may first start with updating EnvDefaultFunc to add a warning that in the next major release environment variables will no longer be supported, and that users should pass needed environment variables as terraform parameters (variables), i.e. using export TF_VAR_github_token=${GITHUB_TOKEN}; terraform plan, and creating github_token variable inside their terraform file, and using it in the provider configuration. You may also want to delete MultiEnvDefaultFunc as it doesn`t appear to be used (need to dbl check).

If you think 2.x will be around for a while longer (up to maintainers really), I think my first comment proposal is ok, but I am not sure either of your options would fix the issue -- removing the default would break everyone's 2.x, and using an explicit check is not good because it would allow users to make a mistake of having both a token and anonymous=true set in the provider configuration itself.

jeet-parekh commented 4 years ago

The second option would allow this to be a non-breaking change. We could update the documentation to say that if anonymous=true, then the token will be ignored. Probably pass a warning from the provider as well mentioning the same. That user mistake won't actually break anything.

nyurik commented 4 years ago

Sure, that would also work for now. Could someone from the core team comment on removing environment vars automagic in 3.x ? Thanks for working on it!!!

jeet-parekh commented 4 years ago

I started working on this, and I have a confusion now.

How do I check the source of the token? Is there a way of getting the source of the token inside providerConfigure?

I could do a os.LookupEnv("GITHUB_TOKEN"). But that won't tell me if the token came from the environment variable, or was it explicitly set in the TF file. And I guess an error should be expected if the user has explicitly set both anonymous=true and a token in the TF file regardless of the presence of the environment variable.

Another work-around would be to clear the token field if anonymous=true, and update the docs to mention this in bold. If anonymous=false and the token is missing, then we already have a check in place.

https://github.com/terraform-providers/terraform-provider-github/blob/71d0202114e160f99f4c08c12b75a0522bf05b79/github/config.go#L61-L63

jcudit commented 4 years ago

Thanks for the discussion so far everyone 😄. I am in favour of cutting a new major version to accommodate this change as well as adopting the GraphQL API.

add a warning that in the next major release environment variables will no longer be supported, and that users should pass needed environment variables as terraform parameters

I think this is a solid first step that we can take ☝️ in the short term and would be happy to merge something like this into the next 2.x release.

joshua-hancox commented 2 years ago

I have a very similar issue. I run terraform using a GitHub Action that requires an environment variable called GITHUB_TOKEN to be set.

When I try to run a plan in this action using the github app authentication method, I get an error:

provider configuration

provider "github" {
  owner = var.github_cloud_organisation

  app_auth {
    id              = var.github_cloud_app_id
    installation_id = var.github_cloud_app_installation_id
    pem_file        = var.github_cloud_app_private_key
  }
}

terraform output

 ~ terraform plan

│ Error: "app_auth": conflicts with token
│
│   with provider["registry.terraform.io/integrations/github"],
│   on provider.tf line 22, in provider "github":
│   22: provider "github" {

I cannot unset the environment variable because the action running terraform requires it, to interact with github and post back to the PR with the plan.

I also prefer to remove the environment vars automagic. I see that was mentioned at v2 though, and now we are at v4. Is there no plan to implement this?

tf version 1.1.6 provider version 4.20.0

github-actions[bot] commented 1 year ago

👋 Hey Friends, this issue has been automatically marked as stale because it has no recent activity. It will be closed if no further activity occurs. Please add the Status: Pinned label if you feel that this issue needs to remain open/active. Thank you for your contributions and help in keeping things tidy!

kfcampbell commented 1 year ago

I think this is still a very valid issue and should remain open. PRs are appreciated! I'm a little conflicted about removing the environment variables behavior...it sounds reasonable but I'm wary of Chesterton's fence and all that.

I'm curious what other providers do about these types of configuration.

ammarmallik commented 1 year ago

I need to use github_ip_ranges data source. But when GitHub actions run, the github provider runs in non-anonymous mode and breaks the build with the following error:

Error: GET https://api.github.com/user: 403 Resource not accessible by integration []

Firstly, I don't understand why /user endpoint is hit by default. Is it because the token is set as environment variable or something else?

Secondly, even though meta is a public endpoint, I'm unable to use it. I can't unset GITHUB_TOKEN environment variable because it is being used for other stuff in the workflow. I'm using the following versions:

Terraform version: v1.5.0 Github provider version:

github = {
    source = "integrations/github"
    version = "5.26.0"
}

Since anonymous flag has been deprecated, I believe GitHub provider should allow access to public endpoints with/without the authentication. This should be fixed.

vsamofal commented 5 months ago

I do have the same problem, github provider is optional in one of tf modules that I use, and it's hitting /user endpoint when I'm actually not using github provider at all