use github token if present when fetching org id

[raymonstah]

When running our acceptance tests in other repos, we often hit the rate limit for GitHub's API. See https://github.com/hashicorp/terraform-provider-vault/actions/runs/3767881277/jobs/6405863202#step:5:349 as an example. This change takes in the Github token (if present) when creating the GH client so that we can make authenticated requests, which bumps the rate limit to 5k requests per hour, instead of the 60/hour that we're currently exceeding.

[fairclothjm]

We should add token to the framework.FieldSchema above in pathConfig(). You can see an example of that here. If we do that then we can safely do the type assertion without this "if ok" pattern. Additionally, we will want to add token to the API docs.

[fairclothjm]

Thanks for taking this one on! This adds an interesting situation. Currently we require the token on Login. But if the user has already configured this auth method with a token in the Config step then they might expect to not have to provide a token on Login. I think we should still require it on Login, but I am curious what others think.

[raymonstah]

@fairclothjm and I discussed this some more, we decided to keep the login path as-is, requiring the user to specify the token, for cases where the user logging in may not necessarily be the same user who is setting up the GitHub config.

[fairclothjm]

@raymonstah I am thinking it might be better to read an environment variable to get the token for writing the config. Adding the token as a new field on the config might cause confusion since this is just to fix an edge case that we are only aware of in the TFVP tests. The real solution for users is to set the org ID in the config. What do you think?

[raymonstah]

@fairclothjm I think that makes much more sense. Thanks for the suggestion.

[fairclothjm]

LGTM!

[vinay-gopalan]

Looks good! Had a quick q: is there a need to call it the VAULT_AUTH_GITHUB_TOKEN if it's never actually being used for authentication? Was curious if this was just because of a precedent or if we can also name this GITHUB_TOKEN to match up w/ what we already have in the TFVP. We can rename the TFVP environment variable as well, just wanted to bring up the question 😄 also tagging @fairclothjm for visibility

[raymonstah]

@vinay-gopalan I prefixed it with VAULT_AUTH because I saw that's the way it was done here

vault/builtin/credential/github/cli.go

Line 27 in 861454e

token = os.Getenv("VAULT_AUTH_GITHUB_TOKEN")

but open to changing it

[vinay-gopalan]

@vinay-gopalan I prefixed it with VAULT_AUTH because I saw that's the way it was done here

vault/builtin/credential/github/cli.go

Line 27 in 861454e

token = os.Getenv("VAULT_AUTH_GITHUB_TOKEN")

but open to changing it

Ah okay, I think that looks good; thanks for confirming @raymonstah!

[fairclothjm]

@raymonstah @vinay-gopalan I missed that VAULT_AUTH_GITHUB_TOKEN is already in use by the login path (only via CLI). I don't see that we have that documented anywhere except in the changelog.

Also, we are using GITHUB_TOKEN in these tests.

I am wondering if we should use something different? Maybe VAULT_AUTH_CONFIG_GITHUB_TOKEN? I don't feel too strongly about this though. I do see value in prefixing with VAULT_* however.

[vinay-gopalan]

@raymonstah @vinay-gopalan I missed that VAULT_AUTH_GITHUB_TOKEN is already in use by the login path (only via CLI). I don't see that we have that documented anywhere except in the changelog.

Also, we are using GITHUB_TOKEN in these tests.

I am wondering if we should use something different? Maybe VAULT_AUTH_CONFIG_GITHUB_TOKEN? I don't feel too strongly about this though. I do see value in prefixing with VAULT_* however.

JM that's a great callout and thanks for going the extra step to confirm the environment variables aren't already being used! I agree with you in that I don't have a strong preference, but I also see the value in adding the prefix since it's a precedent— VAULT_AUTH_CONFIG_GITHUB_TOKEN seems to provide enough info to keep the variable separate only for this use-case so I'm good with that 👍🏼

[fairclothjm]

@raymonstah FYI, we are going to backport this to 1.13, .12, and 1.11 so the TFVP tests are happy. Thanks again for this!