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

Auth changes for vNext #987

Closed
wants to merge 20 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/dotcom-acceptance-tests-all.yml
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ jobs:
with:
TF_LOG: INFO
RUN_ALL: true
GITHUB_ORGANIZATION: terraformtesting
GITHUB_TEST_ORGANIZATION: terraformtesting
GITHUB_TEST_USER_TOKEN: ${{ secrets.DOTCOM_TEST_USER_TOKEN }}
GITHUB_TEST_OWNER: github-terraform-test-user

Expand All @@ -80,6 +80,6 @@ jobs:
with:
TF_LOG: DEBUG
RUN_ALLOWED: ${{ steps.acceptance-tests-organization.outputs.failed }}
GITHUB_ORGANIZATION: terraformtesting
GITHUB_TEST_ORGANIZATION: terraformtesting
GITHUB_TEST_USER_TOKEN: ${{ secrets.DOTCOM_TEST_USER_TOKEN }}
GITHUB_TEST_OWNER: github-terraform-test-user
2 changes: 1 addition & 1 deletion .github/workflows/dotcom-acceptance-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,6 @@ jobs:
uses: terraformtesting/acceptance-tests@v2.2.0
with:
TF_LOG: INFO
GITHUB_ORGANIZATION: terraformtesting
GITHUB_TEST_ORGANIZATION: terraformtesting
GITHUB_TEST_USER_TOKEN: ${{ secrets.DOTCOM_TEST_USER_TOKEN }}
GITHUB_TEST_OWNER: github-terraform-test-user
4 changes: 2 additions & 2 deletions .github/workflows/ghes-acceptance-tests-all.yml
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ jobs:
TF_LOG: INFO
RUN_ALL: true
GITHUB_BASE_URL: "https://terraformtesting-ghe.eastus.cloudapp.azure.com/"
GITHUB_ORGANIZATION: terraformtesting
GITHUB_TEST_ORGANIZATION: terraformtesting
GITHUB_TEST_USER_TOKEN: ${{ secrets.GHES_TEST_USER_TOKEN }}

- name: Failed Acceptance Tests (Organization)
Expand All @@ -96,5 +96,5 @@ jobs:
TF_LOG: DEBUG
RUN_ALLOWED: ${{ steps.acceptance-tests-organization.outputs.failed }}
GITHUB_BASE_URL: "https://terraformtesting-ghe.eastus.cloudapp.azure.com/"
GITHUB_ORGANIZATION: terraformtesting
GITHUB_TEST_ORGANIZATION: terraformtesting
GITHUB_TEST_USER_TOKEN: ${{ secrets.GHES_TEST_USER_TOKEN }}
2 changes: 1 addition & 1 deletion .github/workflows/ghes-acceptance-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -63,5 +63,5 @@ jobs:
uses: terraformtesting/acceptance-tests@v2.2.0
with:
GITHUB_BASE_URL: "https://terraformtesting-ghe.eastus.cloudapp.azure.com/"
GITHUB_ORGANIZATION: terraformtesting
GITHUB_TEST_ORGANIZATION: terraformtesting
GITHUB_TEST_USER_TOKEN: ${{ secrets.GHES_TEST_USER_TOKEN }}
12 changes: 8 additions & 4 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ Once you have the repository cloned, there's a couple of additional steps you'll
- Export the necessary configuration for authenticating your provider with GitHub
```sh
export GITHUB_TOKEN=<token of a user with an organization account>
export GITHUB_ORGANIZATION=<name of an organization>
export GITHUB_OWNER=<name of an organization>
```
- Build the project with `make build`
- Try an example test run from the default (`master`) branch, like `TF_LOG=DEBUG TF_ACC=1 go test -v ./... -run ^TestAccGithubRepositories`. All those tests should pass.
Expand Down Expand Up @@ -176,9 +176,9 @@ Commonly required environment variables are listed below:
export TF_LOG=DEBUG

# enable testing of organization scenarios instead of individual or anonymous
export GITHUB_ORGANIZATION=
export GITHUB_TEST_ORGANIZATION=

# enable testing of individual scenarios instead of organizaiton or anonymous
# enable testing of individual scenarios instead of organization or anonymous
export GITHUB_OWNER=

# enable testing of enterprise appliances
Expand All @@ -193,6 +193,10 @@ export GITHUB_TEST_ORGANIZATION=
export GITHUB_TEST_USER_TOKEN=
```

Note that the above `GITHUB_TEST_ORGANIZATION` variable does not have a corresponding `GITHUB_ORGANIZATION` environment variable for use outside of tests. The `GITHUB_ORGANIZATION` variable was deprecated in version 5.0.0 of the GitHub Terraform provider, and the sole way to configure an organization is to use the `owner` provider block configuration or the `GITHUB_OWNER` variable.

Use the `GITHUB_TEST_ORGANIZATION` variable only in acceptance tests, only to specify an organization owner. This is useful in provider scenarios that are valid for both individual and organization owners and need to be tested individually.

See [this project](https://github.com/terraformtesting/acceptance-tests) for more information on how tests are run automatically.

There are also a small amount of unit tests in the provider. Due to the nature of the provider, such tests are currently only recommended for exercising functionality completely internal to the provider. These may be executed by running `make test`.
Expand All @@ -214,7 +218,7 @@ Once the token has been created, it must be exported in your environment as `GIT

### GitHub Organization

If you do not have an organization already that you are comfortable running tests against, you will need to [create one](https://help.github.com/en/articles/creating-a-new-organization-from-scratch). The free "Team for Open Source" org type is fine for these tests. The name of the organization must then be exported in your environment as `GITHUB_ORGANIZATION`.
If you do not have an organization already that you are comfortable running tests against, you will need to [create one](https://help.github.com/en/articles/creating-a-new-organization-from-scratch). The free "Team for Open Source" org type is fine for these tests. The name of the organization must then be exported in your environment as `GITHUB_OWNER`.

Make sure that your organization has a `terraform-module-template` repository ([terraformtesting/terraform-template-module](https://github.com/terraformtesting/terraform-template-module) is an example you can clone) and that its "Template repository" item in Settings is checked.

Expand Down
4 changes: 2 additions & 2 deletions examples/repository_collaborator/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,15 @@ This example will also create a repository in the specified `owner` organization
Alternatively, you may use variables passed via command line:

```console
export GITHUB_ORGANIZATION=
export GITHUB_OWNER=
export GITHUB_TOKEN=
export COLLABORATOR_USERNAME=
export COLLABORATOR_PERMISSION=
```

```console
terraform apply \
-var "organization=${GITHUB_ORGANIZATION}" \
-var "owner=${GITHUB_OWNER}" \
-var "github_token=${GITHUB_TOKEN}" \
-var "username=${COLLABORATOR_USERNAME}" \
-var "permission=${COLLABORATOR_PERMISSION}"
Expand Down
9 changes: 4 additions & 5 deletions github/data_source_github_organization_teams_test.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package github

import (
"fmt"
"testing"

"github.com/hashicorp/terraform-plugin-sdk/helper/resource"
Expand All @@ -11,9 +10,9 @@ func TestAccGithubOrganizationTeamsDataSource(t *testing.T) {

t.Run("queries without error", func(t *testing.T) {

config := fmt.Sprintf(`
config := `
data "github_organization_teams" "all" {}
`)
`

check := resource.ComposeAggregateTestCheckFunc(
resource.TestCheckResourceAttrSet("data.github_organization_teams.all", "teams.0.id"),
Expand Down Expand Up @@ -49,11 +48,11 @@ func TestAccGithubOrganizationTeamsDataSource(t *testing.T) {

t.Run("queries root teams only without error", func(t *testing.T) {

config := fmt.Sprintf(`
config := `
data "github_organization_teams" "root_teams" {
root_teams_only = true
}
`)
`

check := resource.ComposeAggregateTestCheckFunc(
resource.TestCheckResourceAttrSet("data.github_organization_teams.root_teams", "teams.0.id"),
Expand Down
2 changes: 1 addition & 1 deletion github/data_source_github_repository_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ func TestAccGithubRepositoryDataSource(t *testing.T) {
randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum)

config := fmt.Sprintf(`

resource "github_repository" "test" {
name = "tf-acc-%s"
auto_init = true
Expand Down
42 changes: 8 additions & 34 deletions github/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package github
import (
"fmt"
"log"
"os"
"strings"
"time"

Expand All @@ -25,13 +26,6 @@ func Provider() terraform.ResourceProvider {
DefaultFunc: schema.EnvDefaultFunc("GITHUB_OWNER", nil),
Description: descriptions["owner"],
},
"organization": {
Type: schema.TypeString,
Optional: true,
DefaultFunc: schema.EnvDefaultFunc("GITHUB_ORGANIZATION", nil),
Description: descriptions["organization"],
Deprecated: "Use owner (or GITHUB_OWNER) instead of organization (or GITHUB_ORGANIZATION)",
},
"base_url": {
Type: schema.TypeString,
Optional: true,
Expand Down Expand Up @@ -159,11 +153,7 @@ func init() {

"insecure": "Enable `insecure` mode for testing purposes",

"owner": "The GitHub owner name to manage. " +
"Use this field instead of `organization` when managing individual accounts.",

"organization": "The GitHub organization name to manage. " +
"Use this field instead of `owner` when managing organization accounts.",
"owner": "The GitHub organization or user name to manage. ",

"app_auth": "The GitHub App credentials used to connect to GitHub. Conflicts with " +
"`token`. Anonymous mode is enabled if both `token` and `app_auth` are not set.",
Expand All @@ -178,31 +168,15 @@ func init() {
func providerConfigure(p *schema.Provider) schema.ConfigureFunc {
return func(d *schema.ResourceData) (interface{}, error) {
owner := d.Get("owner").(string)
baseURL := d.Get("base_url").(string)
token := d.Get("token").(string)
insecure := d.Get("insecure").(bool)

// BEGIN backwards compatibility
// OwnerOrOrgEnvDefaultFunc used to be the default value for both
// 'owner' and 'organization'. This meant that if 'owner' and
// 'GITHUB_OWNER' were set, 'GITHUB_OWNER' would be used as the default
// value of 'organization' and therefore override 'owner'.
//
// This seems undesirable (an environment variable should not override
// an explicitly set value in a provider block), but is necessary
// for backwards compatibility. We could remove this backwards compatibility
// code in a future major release.
env, _ := OwnerOrOrgEnvDefaultFunc()
if env.(string) != "" {
owner = env.(string)
if owner == "" && os.Getenv("GITHUB_OWNER") != "" {
owner = os.Getenv("GITHUB_OWNER")
log.Printf("[DEBUG] Selecting owner %s from GITHUB_OWNER environment variable", owner)
}
// END backwards compatibility

org := d.Get("organization").(string)
if org != "" {
log.Printf("[INFO] Selecting organization attribute as owner: %s", org)
owner = org
}
baseURL := d.Get("base_url").(string)
token := d.Get("token").(string)
insecure := d.Get("insecure").(bool)

if appAuth, ok := d.Get("app_auth").([]interface{}); ok && len(appAuth) > 0 && appAuth[0] != nil {
appAuthAttr := appAuth[0].(map[string]interface{})
Expand Down
68 changes: 35 additions & 33 deletions github/provider_utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,22 @@ import (
)

var testCollaborator string = os.Getenv("GITHUB_TEST_COLLABORATOR")
var isEnterprise string = os.Getenv("ENTERPRISE_ACCOUNT")
var testOrganization string = testOrganizationFunc()
var testOwner string = os.Getenv("GITHUB_OWNER")

var isEnterprise string = os.Getenv("ENTERPRISE_ACCOUNT")
var testOwner string = testOwnerFunc()
var testToken string = os.Getenv("GITHUB_TOKEN")
var testBaseURLGHES string = os.Getenv("GHES_BASE_URL")

func testAccPreCheck(t *testing.T) {
if v := os.Getenv("GITHUB_TOKEN"); v == "" {
t.Fatal("GITHUB_TOKEN must be set for acceptance tests")
}
if v := os.Getenv("GITHUB_ORGANIZATION"); v == "" && os.Getenv("GITHUB_OWNER") == "" {
t.Fatal("GITHUB_ORGANIZATION or GITHUB_OWNER must be set for acceptance tests")
if v := os.Getenv("GITHUB_OWNER"); v == "" {
t.Fatal("GITHUB_OWNER must be set for acceptance tests")
}
if v := os.Getenv("GITHUB_TEST_ORGANIZATION"); v == "" {
t.Fatal("GITHUB_TEST_ORGANIZATION must be set for acceptance tests")
}
if v := os.Getenv("GITHUB_TEST_USER"); v == "" {
t.Fatal("GITHUB_TEST_USER must be set for acceptance tests")
Expand All @@ -43,23 +47,29 @@ func skipUnlessMode(t *testing.T, providerMode string) {
t.Log("anonymous mode not supported for GHES deployments")
break
}

if os.Getenv("GITHUB_TOKEN") == "" {
log.Printf("[DEBUG] configuring anonymous user without token")
return
} else {
t.Log("GITHUB_TOKEN environment variable should be empty")
t.Log("GITHUB_TOKEN environment variable should be empty in anonymous mode")
}
case individual:
if os.Getenv("GITHUB_TOKEN") != "" && os.Getenv("GITHUB_OWNER") != "" {
log.Printf("[DEBUG] configuring user type: %s", providerMode)
return
} else {
t.Log("GITHUB_TOKEN and GITHUB_OWNER environment variables should be set")
t.Logf("GITHUB_TOKEN and GITHUB_OWNER environment variables should be set for tests in %v mode", providerMode)
}
case organization:
if os.Getenv("GITHUB_TOKEN") != "" && os.Getenv("GITHUB_ORGANIZATION") != "" {
testOwner := os.Getenv("GITHUB_TEST_ORGANIZATION")
if testOwner == "" {
testOwner = os.Getenv("GITHUB_OWNER")
}
if os.Getenv("GITHUB_TOKEN") != "" && testOwner != "" {
log.Printf("[DEBUG] configuring user type: %s", providerMode)
return
} else {
t.Log("GITHUB_TOKEN and GITHUB_ORGANIZATION environment variables should be set")
t.Logf("GITHUB_TOKEN and GITHUB_TEST_ORGANIZATION environment variables should be set for tests in %v mode", providerMode)
}
}

Expand All @@ -71,13 +81,11 @@ func testAccCheckOrganization() error {
baseURL := os.Getenv("GITHUB_BASE_URL")
token := os.Getenv("GITHUB_TOKEN")

owner := os.Getenv("GITHUB_OWNER")
if owner == "" {
organization := os.Getenv("GITHUB_ORGANIZATION")
if organization == "" {
return fmt.Errorf("Neither `GITHUB_OWNER` or `GITHUB_ORGANIZATION` set in environment")
}
owner = organization
owner := os.Getenv("GITHUB_TEST_ORGANIZATION")
if owner == "" && os.Getenv("GITHUB_OWNER") == "" {
return fmt.Errorf("neither `GITHUB_TEST_ORGANIZATION` nor `GITHUB_OWNER` are set in environment")
} else if owner == "" {
owner = os.Getenv("GITHUB_OWNER")
}

config := Config{
Expand All @@ -96,32 +104,26 @@ func testAccCheckOrganization() error {
return nil
}

func OwnerOrOrgEnvDefaultFunc() (interface{}, error) {
if organization := os.Getenv("GITHUB_ORGANIZATION"); organization != "" {
log.Printf("[INFO] Selecting owner %s from GITHUB_ORGANIZATION environment variable", organization)
return organization, nil
}
func testOwnerFunc() string {
owner := os.Getenv("GITHUB_OWNER")
log.Printf("[INFO] Selecting owner %s from GITHUB_OWNER environment variable", owner)
return owner, nil
if owner == "" {
owner = os.Getenv("GITHUB_TEST_OWNER")
}
return owner
}

// testOrganizationFunc returns a test organization. IMPORTANT:
// GITHUB_ORGANIZATION is deprecated. The purpose of this variable is
// to make sure that we can still test organization cases appropriately
// in integration testing.
func testOrganizationFunc() string {
organization := os.Getenv("GITHUB_ORGANIZATION")
organization := os.Getenv("GITHUB_TEST_ORGANIZATION")
if organization == "" {
organization = os.Getenv("GITHUB_TEST_ORGANIZATION")
organization = os.Getenv("GITHUB_OWNER")
}
return organization
}

func testOwnerFunc() string {
owner := os.Getenv("GITHUB_OWNER")
if owner == "" {
owner = os.Getenv("GITHUB_TEST_OWNER")
}
return owner
}

const anonymous = "anonymous"
const individual = "individual"
const organization = "organization"
17 changes: 4 additions & 13 deletions website/docs/index.html.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -98,9 +98,7 @@ The following arguments are supported in the `provider` block:

* `base_url` - (Optional) This is the target GitHub base API endpoint. Providing a value is a requirement when working with GitHub Enterprise. It is optional to provide this value and it can also be sourced from the `GITHUB_BASE_URL` environment variable. The value must end with a slash, for example: `https://terraformtesting-ghe.westus.cloudapp.azure.com/`

* `owner` - (Optional) This is the target GitHub organization or individual user account to manage. For example, `torvalds` and `github` are valid owners. It is optional to provide this value and it can also be sourced from the `GITHUB_OWNER` environment variable. When not provided and a `token` is available, the individual user account owning the `token` will be used. When not provided and no `token` is available, the provider may not function correctly.

* `organization` - (Deprecated) This behaves the same as `owner`, which should be used instead. This value can also be sourced from the `GITHUB_ORGANIZATION` environment variable.
* `owner` - (Optional) This is the target GitHub organization or individual user account to manage. For example, `torvalds` and `github` are valid owners. It is optional to provide this value and it can also be sourced from the `GITHUB_OWNER` environment variable. When not provided and a `token` is available, the individual user account owning the `token` will be used. When not provided and no `token` is available, the provider may not function correctly. If both `owner` and `GITHUB_OWNER` are set, `owner` takes priority.

* `app_auth` - (Optional) Configuration block to use GitHub App installation token. When not provided, the provider can only access resources available anonymously.
* `id` - (Required) This is the ID of the GitHub App. It can sourced from the `GITHUB_APP_ID` environment variable.
Expand All @@ -111,15 +109,8 @@ The following arguments are supported in the `provider` block:

Note: If you have a PEM file on disk, you can pass it in via `pem_file = file("path/to/file.pem")`.

For backwards compatibility, if more than one of `owner`, `organization`,
`GITHUB_OWNER` and `GITHUB_ORGANIZATION` are set, the first in this
list takes priority.
## Authenticating with multiple providers

1. Setting `organization` in the GitHub provider configuration.
2. Setting the `GITHUB_ORGANIZATION` environment variable.
3. Setting the `GITHUB_OWNER` environment variable.
4. Setting `owner` in the GitHub provider configuration.
When authenticating with multiple providers, it is possible to fail to authenticate both of them correctly and experience unexpected behavior. This is especially fraught when using both `integrations/github` and `hashicorp/github`. As [this GitHub comment](https://github.com/integrations/terraform-provider-github/issues/655#issuecomment-804419698) from @tibbes details, [local names](https://www.terraform.io/docs/language/providers/requirements.html#local-names) must be used when using two providers with the same name. Otherwise, as @tibbes mentions, the `provider "github"` block is sent to one of the providers but not the other, which means that the other provider is working in anonymous mode with no token, no owner, and no organization.
kfcampbell marked this conversation as resolved.
Show resolved Hide resolved

~> It is a bug that `GITHUB_OWNER` takes precedence over `owner`, which may
be fixed in a future major release. For compatibility with future releases,
please set only one of `GITHUB_OWNER` and `owner`.
If you are experiencing unexpected behavior with the provider and your HCL config contains both `integrations/github` and `hashicorp/github`, consider using only one of the providers or else local names in your configuration.