From 59e632435ebf48487eb3e96430e1406d031bbb91 Mon Sep 17 00:00:00 2001 From: Paddy Date: Mon, 23 Jan 2017 16:45:06 -0800 Subject: [PATCH 001/385] Start adding tests for image resolution. Add tests that show what we want image input strings to resolve to, so we can test that behaviour. --- builtin/providers/google/image_test.go | 61 +++++++++++++++++++ .../providers/google/resource_compute_disk.go | 1 + 2 files changed, 62 insertions(+) create mode 100644 builtin/providers/google/image_test.go diff --git a/builtin/providers/google/image_test.go b/builtin/providers/google/image_test.go new file mode 100644 index 0000000000..f500c9a4ca --- /dev/null +++ b/builtin/providers/google/image_test.go @@ -0,0 +1,61 @@ +package google + +import ( + "testing" + + "github.com/hashicorp/terraform/helper/resource" +) + +func TestAccComputeImage_resolveImage(t *testing.T) { + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckComputeImageDestroy, + Steps: []resource.TestStep{ + { + Config: testAccComputeImage_basedondisk, + Check: resource.ComposeTestCheckFunc( + testAccCheckComputeImageExists( + "google_compute_image.foobar", &image), + ), + }, + }, + }) + images := map[string]string{ + "family/debian-8": "projects/debian-cloud/global/images/family/debian-8-jessie", + "projects/debian-cloud/global/images/debian-8-jessie-v20170110": "projects/debian-cloud/global/images/debian-8-jessie-v20170110", + "debian-8-jessie": "projects/debian-cloud/global/images/family/debian-8-jessie", + "debian-8-jessie-v20170110": "projects/debian-cloud/global/images/debian-8-jessie-v20170110", + "https://www.googleapis.com/compute/v1/projects/debian-cloud/global/images/debian-8-jessie-v20170110": "https://www.googleapis.com/compute/v1/projects/debian-cloud/global/images/debian-8-jessie-v20170110", + + // TODO(paddy): we need private images/families here to actually test this + "global/images/my-private-image": "global/images/my-private-image", + "global/images/family/my-private-family": "global/images/family/my-private-family", + "my-private-image": "global/images/my-private-image", + "my-private-family": "global/images/family/my-private-family", + "my-project/my-private-image": "projects/my-project/global/images/my-private-image", + "my-project/my-private-family": "projects/my-project/global/images/family/my-private-family", + "insert-URL-here": "insert-URL-here", + } + config := &Config{ + Credentials: credentials, + Project: project, + Region: region, + } + + err := config.loadAndValidate() + if err != nil { + t.Fatalf("Error loading config: %s\n", err) + } + for input, expectation := range images { + result, err := resolveImage(config, input) + if err != nil { + t.Errorf("Error resolving input %s to image: %+v\n", input, err) + continue + } + if result != expectation { + t.Errorf("Expected input '%s' to resolve to '%s', it resolved to '%s' instead.\n", input, expectation, result) + continue + } + } +} diff --git a/builtin/providers/google/resource_compute_disk.go b/builtin/providers/google/resource_compute_disk.go index c8ef8007aa..94d23d3402 100644 --- a/builtin/providers/google/resource_compute_disk.go +++ b/builtin/providers/google/resource_compute_disk.go @@ -112,6 +112,7 @@ func resourceComputeDiskCreate(d *schema.ResourceData, meta interface{}) error { } disk.SourceImage = imageUrl + log.Printf("[DEBUG] Image name resolved to: %s", imageUrl) } if v, ok := d.GetOk("type"); ok { From 90254b945178a8bf6d048f317c250e3cffd3b64e Mon Sep 17 00:00:00 2001 From: Paddy Date: Thu, 23 Feb 2017 21:55:30 -0800 Subject: [PATCH 002/385] provider/google: update image resolution code. Add tests that ensure that image syntax resolves to API input the way we want it to. Add a lot of different input forms for images, to more closely map to what the API accepts, so anything that's valid input to the API should also be valid input in a config. Stop resolving image families to specific image URLs, allowing things like instance templates to evolve over time as new images are pushed. --- builtin/providers/google/image.go | 244 +++++++++++++++++-------- builtin/providers/google/image_test.go | 112 ++++++++---- 2 files changed, 242 insertions(+), 114 deletions(-) diff --git a/builtin/providers/google/image.go b/builtin/providers/google/image.go index e4a50905ef..912821b59c 100644 --- a/builtin/providers/google/image.go +++ b/builtin/providers/google/image.go @@ -2,96 +2,178 @@ package google import ( "fmt" + "regexp" "strings" + + "google.golang.org/api/googleapi" ) +const ( + resolveImageProjectRegex = "[a-z](?:[-a-z0-9]{0,61}[a-z0-9])?" // TODO(paddy): this isn't based on any documentation; we're just copying the image name restrictions. Need to follow up with @danawillow and/or @evandbrown and see if there's an actual limit to this + resolveImageFamilyRegex = "[a-z](?:[-a-z0-9]{0,61}[a-z0-9])?" // TODO(paddy): this isn't based on any documentation; we're just copying the image name restrictions. Need to follow up with @danawillow and/or @evandbrown and see if there's an actual limit to this + resolveImageImageRegex = "[a-z](?:[-a-z0-9]{0,61}[a-z0-9])?" // 1-63 characters, lowercase letters, numbers, and hyphens only, beginning and ending in a lowercase letter or number +) + +var ( + resolveImageProjectImage = regexp.MustCompile(fmt.Sprintf("^projects/(%s)/global/images/(%s)$", resolveImageProjectRegex, resolveImageImageRegex)) + resolveImageProjectFamily = regexp.MustCompile(fmt.Sprintf("^projects/(%s)/global/images/family/(%s)$", resolveImageProjectRegex, resolveImageFamilyRegex)) + resolveImageGlobalImage = regexp.MustCompile(fmt.Sprintf("^global/images/(%s)$", resolveImageImageRegex)) + resolveImageGlobalFamily = regexp.MustCompile(fmt.Sprintf("^global/images/family/(%s)$", resolveImageFamilyRegex)) + resolveImageFamilyFamily = regexp.MustCompile(fmt.Sprintf("^family/(%s)$", resolveImageFamilyRegex)) + resolveImageProjectImageShorthand = regexp.MustCompile(fmt.Sprintf("^(%s)/(%s)$", resolveImageProjectRegex, resolveImageImageRegex)) + resolveImageProjectFamilyShorthand = regexp.MustCompile(fmt.Sprintf("^(%s)/(%s)$", resolveImageProjectRegex, resolveImageFamilyRegex)) + resolveImageFamily = regexp.MustCompile(fmt.Sprintf("^(%s)$", resolveImageFamilyRegex)) + resolveImageImage = regexp.MustCompile(fmt.Sprintf("^(%s)$", resolveImageImageRegex)) + resolveImageLink = regexp.MustCompile(fmt.Sprintf("^https://www.googleapis.com/compute/v1/projects/(%s)/global/images/(%s)", resolveImageProjectRegex, resolveImageImageRegex)) +) + +func resolveImageImageExists(c *Config, project, name string) (bool, error) { + if _, err := c.clientCompute.Images.Get(project, name).Do(); err == nil { + return true, nil + } else if gerr, ok := err.(*googleapi.Error); ok && gerr.Code == 404 { + return false, nil + } else { + return false, fmt.Errorf("Error checking if image %s exists: %s", name, err) + } +} + +func resolveImageFamilyExists(c *Config, project, name string) (bool, error) { + if _, err := c.clientCompute.Images.GetFromFamily(project, name).Do(); err == nil { + return true, nil + } else if gerr, ok := err.(*googleapi.Error); ok && gerr.Code == 404 { + return false, nil + } else { + return false, fmt.Errorf("Error checking if family %s exists: %s", name, err) + } +} + // If the given name is a URL, return it. // If it is of the form project/name, search the specified project first, then // search image families in the specified project. // If it is of the form name then look in the configured project, then hosted // image projects, and lastly at image families in hosted image projects. func resolveImage(c *Config, name string) (string, error) { - - if strings.HasPrefix(name, "https://www.googleapis.com/compute/v1/") { - return name, nil - - } else { - splitName := strings.Split(name, "/") - if len(splitName) == 1 { - - // Must infer the project name: - - // First, try the configured project for a specific image: - image, err := c.clientCompute.Images.Get(c.Project, name).Do() - if err == nil { - return image.SelfLink, nil - } - - // If it doesn't exist, try to see if it works as an image family: - image, err = c.clientCompute.Images.GetFromFamily(c.Project, name).Do() - if err == nil { - return image.SelfLink, nil - } - - // If we match a lookup for an alternate project, then try that next. - // If not, we return the original error. - - // If the image name contains the left hand side, we use the project from - // the right hand side. - imageMap := map[string]string{ - "centos": "centos-cloud", - "coreos": "coreos-cloud", - "debian": "debian-cloud", - "opensuse": "opensuse-cloud", - "rhel": "rhel-cloud", - "sles": "suse-cloud", - "ubuntu": "ubuntu-os-cloud", - "windows": "windows-cloud", - } - var project string - for k, v := range imageMap { - if strings.Contains(name, k) { - project = v - break - } - } - if project == "" { - return "", err - } - - // There was a match, but the image still may not exist, so check it: - image, err = c.clientCompute.Images.Get(project, name).Do() - if err == nil { - return image.SelfLink, nil - } - - // If it doesn't exist, try to see if it works as an image family: - image, err = c.clientCompute.Images.GetFromFamily(project, name).Do() - if err == nil { - return image.SelfLink, nil - } - - return "", err - - } else if len(splitName) == 2 { - - // Check if image exists in the specified project: - image, err := c.clientCompute.Images.Get(splitName[0], splitName[1]).Do() - if err == nil { - return image.SelfLink, nil - } - - // If it doesn't, check if it exists as an image family: - image, err = c.clientCompute.Images.GetFromFamily(splitName[0], splitName[1]).Do() - if err == nil { - return image.SelfLink, nil - } - - return "", err - - } else { - return "", fmt.Errorf("Invalid image name, require URL, project/name, or just name: %s", name) + // built-in projects to look for images/families containing the string + // on the left in + imageMap := map[string]string{ + "centos": "centos-cloud", + "coreos": "coreos-cloud", + "debian": "debian-cloud", + "opensuse": "opensuse-cloud", + "rhel": "rhel-cloud", + "sles": "suse-cloud", + "ubuntu": "ubuntu-os-cloud", + "windows": "windows-cloud", + } + var builtInProject string + for k, v := range imageMap { + if strings.Contains(name, k) { + builtInProject = v + break } } - + switch { + case resolveImageLink.MatchString(name): // https://www.googleapis.com/compute/v1/projects/xyz/global/images/xyz + return name, nil + case resolveImageProjectImage.MatchString(name): // projects/xyz/global/images/xyz + res := resolveImageProjectImage.FindStringSubmatch(name) + if len(res)-1 != 2 { // subtract one, index zero is the entire matched expression + return "", fmt.Errorf("Expected %d project image regex matches, got %d for %s", 2, len(res)-1, name) + } + return fmt.Sprintf("projects/%s/global/images/%s", res[1], res[2]), nil + case resolveImageProjectFamily.MatchString(name): // projects/xyz/global/images/family/xyz + res := resolveImageProjectFamily.FindStringSubmatch(name) + if len(res)-1 != 2 { // subtract one, index zero is the entire matched expression + return "", fmt.Errorf("Expected %d project family regex matches, got %d for %s", 2, len(res)-1, name) + } + return fmt.Sprintf("projects/%s/global/images/family/%s", res[1], res[2]), nil + case resolveImageGlobalImage.MatchString(name): // global/images/xyz + res := resolveImageGlobalImage.FindStringSubmatch(name) + if len(res)-1 != 1 { // subtract one, index zero is the entire matched expression + return "", fmt.Errorf("Expected %d global image regex matches, got %d for %s", 1, len(res)-1, name) + } + return fmt.Sprintf("global/images/%s", res[1]), nil + case resolveImageGlobalFamily.MatchString(name): // global/images/family/xyz + res := resolveImageGlobalFamily.FindStringSubmatch(name) + if len(res)-1 != 1 { // subtract one, index zero is the entire matched expression + return "", fmt.Errorf("Expected %d global family regex matches, got %d for %s", 1, len(res)-1, name) + } + return fmt.Sprintf("global/images/family/%s", res[1]), nil + case resolveImageFamilyFamily.MatchString(name): // family/xyz + res := resolveImageFamilyFamily.FindStringSubmatch(name) + if len(res)-1 != 1 { // subtract one, index zero is the entire matched expression + return "", fmt.Errorf("Expected %d family family regex matches, got %d for %s", 1, len(res)-1, name) + } + if ok, err := resolveImageFamilyExists(c, c.Project, res[1]); err != nil { + return "", err + } else if ok { + return fmt.Sprintf("global/images/family/%s", res[1]), nil + } + if builtInProject != "" { + if ok, err := resolveImageFamilyExists(c, builtInProject, res[1]); err != nil { + return "", err + } else if ok { + return fmt.Sprintf("projects/%s/global/images/family/%s", builtInProject, res[1]), nil + } + } + case resolveImageProjectImageShorthand.MatchString(name): // xyz/xyz + res := resolveImageProjectImageShorthand.FindStringSubmatch(name) + if len(res)-1 != 2 { // subtract one, index zero is the entire matched expression + return "", fmt.Errorf("Expected %d project image shorthand regex matches, got %d for %s", 2, len(res)-1, name) + } + if ok, err := resolveImageImageExists(c, res[1], res[2]); err != nil { + return "", err + } else if ok { + return fmt.Sprintf("projects/%s/global/images/%s", res[1], res[2]), nil + } + fallthrough // check if it's a family + case resolveImageProjectFamilyShorthand.MatchString(name): // xyz/xyz + res := resolveImageProjectFamilyShorthand.FindStringSubmatch(name) + if len(res)-1 != 2 { // subtract one, index zero is the entire matched expression + return "", fmt.Errorf("Expected %d project family shorthand regex matches, got %d for %s", 2, len(res)-1, name) + } + if ok, err := resolveImageFamilyExists(c, res[1], res[2]); err != nil { + return "", err + } else if ok { + return fmt.Sprintf("projects/%s/global/images/family/%s", res[1], res[2]), nil + } + case resolveImageImage.MatchString(name): // xyz + res := resolveImageImage.FindStringSubmatch(name) + if len(res)-1 != 1 { // subtract one, index zero is the entire matched expression + return "", fmt.Errorf("Expected %d image regex matches, got %d for %s", 1, len(res)-1, name) + } + if ok, err := resolveImageImageExists(c, c.Project, res[1]); err != nil { + return "", err + } else if ok { + return fmt.Sprintf("global/images/%s", res[1]), nil + } + if builtInProject != "" { + // check the images GCP provides + if ok, err := resolveImageImageExists(c, builtInProject, res[1]); err != nil { + return "", err + } else if ok { + return fmt.Sprintf("projects/%s/global/images/%s", builtInProject, res[1]), nil + } + } + fallthrough // check if the name is a family, instead of an image + case resolveImageFamily.MatchString(name): // xyz + res := resolveImageFamily.FindStringSubmatch(name) + if len(res)-1 != 1 { // subtract one, index zero is the entire matched expression + return "", fmt.Errorf("Expected %d family regex matches, got %d for %s", 1, len(res)-1, name) + } + if ok, err := resolveImageFamilyExists(c, c.Project, res[1]); err != nil { + return "", err + } else if ok { + return fmt.Sprintf("global/images/family/%s", res[1]), nil + } + if builtInProject != "" { + // check the families GCP provides + if ok, err := resolveImageFamilyExists(c, builtInProject, res[1]); err != nil { + return "", err + } else if ok { + return fmt.Sprintf("projects/%s/global/images/family/%s", builtInProject, res[1]), nil + } + } + } + return "", fmt.Errorf("Could not find image or family %s", name) } diff --git a/builtin/providers/google/image_test.go b/builtin/providers/google/image_test.go index f500c9a4ca..e0f56518af 100644 --- a/builtin/providers/google/image_test.go +++ b/builtin/providers/google/image_test.go @@ -1,61 +1,107 @@ package google import ( + "fmt" "testing" + compute "google.golang.org/api/compute/v1" + + "github.com/hashicorp/terraform/helper/acctest" "github.com/hashicorp/terraform/helper/resource" + "github.com/hashicorp/terraform/terraform" ) func TestAccComputeImage_resolveImage(t *testing.T) { + var image compute.Image + rand := acctest.RandString(10) + name := fmt.Sprintf("test-image-%s", rand) + fam := fmt.Sprintf("test-image-family-%s", rand) + resource.Test(t, resource.TestCase{ PreCheck: func() { testAccPreCheck(t) }, Providers: testAccProviders, CheckDestroy: testAccCheckComputeImageDestroy, Steps: []resource.TestStep{ { - Config: testAccComputeImage_basedondisk, + Config: testAccComputeImage_resolving(name, fam), Check: resource.ComposeTestCheckFunc( testAccCheckComputeImageExists( "google_compute_image.foobar", &image), + testAccCheckComputeImageResolution("google_compute_image.foobar"), ), }, }, }) - images := map[string]string{ - "family/debian-8": "projects/debian-cloud/global/images/family/debian-8-jessie", - "projects/debian-cloud/global/images/debian-8-jessie-v20170110": "projects/debian-cloud/global/images/debian-8-jessie-v20170110", - "debian-8-jessie": "projects/debian-cloud/global/images/family/debian-8-jessie", - "debian-8-jessie-v20170110": "projects/debian-cloud/global/images/debian-8-jessie-v20170110", - "https://www.googleapis.com/compute/v1/projects/debian-cloud/global/images/debian-8-jessie-v20170110": "https://www.googleapis.com/compute/v1/projects/debian-cloud/global/images/debian-8-jessie-v20170110", +} - // TODO(paddy): we need private images/families here to actually test this - "global/images/my-private-image": "global/images/my-private-image", - "global/images/family/my-private-family": "global/images/family/my-private-family", - "my-private-image": "global/images/my-private-image", - "my-private-family": "global/images/family/my-private-family", - "my-project/my-private-image": "projects/my-project/global/images/my-private-image", - "my-project/my-private-family": "projects/my-project/global/images/family/my-private-family", - "insert-URL-here": "insert-URL-here", - } - config := &Config{ - Credentials: credentials, - Project: project, - Region: region, - } +func testAccCheckComputeImageResolution(n string) resource.TestCheckFunc { + return func(s *terraform.State) error { + config := testAccProvider.Meta().(*Config) + project := config.Project - err := config.loadAndValidate() - if err != nil { - t.Fatalf("Error loading config: %s\n", err) - } - for input, expectation := range images { - result, err := resolveImage(config, input) - if err != nil { - t.Errorf("Error resolving input %s to image: %+v\n", input, err) - continue + rs, ok := s.RootModule().Resources[n] + if !ok { + return fmt.Errorf("Resource not found: %s", n) } - if result != expectation { - t.Errorf("Expected input '%s' to resolve to '%s', it resolved to '%s' instead.\n", input, expectation, result) - continue + + if rs.Primary.ID == "" { + return fmt.Errorf("No ID is set") } + if rs.Primary.Attributes["name"] == "" { + return fmt.Errorf("No image name is set") + } + if rs.Primary.Attributes["family"] == "" { + return fmt.Errorf("No image family is set") + } + if rs.Primary.Attributes["self_link"] == "" { + return fmt.Errorf("No self_link is set") + } + + name := rs.Primary.Attributes["name"] + family := rs.Primary.Attributes["family"] + link := rs.Primary.Attributes["self_link"] + + images := map[string]string{ + "family/debian-8": "projects/debian-cloud/global/images/family/debian-8", + "projects/debian-cloud/global/images/debian-8-jessie-v20170110": "projects/debian-cloud/global/images/debian-8-jessie-v20170110", + "debian-8": "projects/debian-cloud/global/images/family/debian-8", + "debian-8-jessie-v20170110": "projects/debian-cloud/global/images/debian-8-jessie-v20170110", + "https://www.googleapis.com/compute/v1/projects/debian-cloud/global/images/debian-8-jessie-v20170110": "https://www.googleapis.com/compute/v1/projects/debian-cloud/global/images/debian-8-jessie-v20170110", + + "global/images/" + name: "global/images/" + name, + "global/images/family/" + family: "global/images/family/" + family, + name: "global/images/" + name, + family: "global/images/family/" + family, + "family/" + family: "global/images/family/" + family, + project + "/" + name: "projects/" + project + "/global/images/" + name, + project + "/" + family: "projects/" + project + "/global/images/family/" + family, + link: link, + } + + for input, expectation := range images { + result, err := resolveImage(config, input) + if err != nil { + return fmt.Errorf("Error resolving input %s to image: %+v\n", input, err) + } + if result != expectation { + return fmt.Errorf("Expected input '%s' to resolve to '%s', it resolved to '%s' instead.\n", input, expectation, result) + } + } + return nil } } + +func testAccComputeImage_resolving(name, family string) string { + return fmt.Sprintf(` +resource "google_compute_disk" "foobar" { + name = "%s" + zone = "us-central1-a" + image = "debian-8-jessie-v20160803" +} +resource "google_compute_image" "foobar" { + name = "%s" + family = "%s" + source_disk = "${google_compute_disk.foobar.self_link}" +} +`, name, name, family) +} From 6868ba72c7107651aa21c30db865e798a1babcb2 Mon Sep 17 00:00:00 2001 From: Paddy Date: Thu, 23 Feb 2017 22:09:07 -0800 Subject: [PATCH 003/385] Update the docs for resolveImage. Update the explanation of the logic being followed in resolveImage. --- builtin/providers/google/image.go | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/builtin/providers/google/image.go b/builtin/providers/google/image.go index 912821b59c..e772d95e43 100644 --- a/builtin/providers/google/image.go +++ b/builtin/providers/google/image.go @@ -48,10 +48,18 @@ func resolveImageFamilyExists(c *Config, project, name string) (bool, error) { } // If the given name is a URL, return it. -// If it is of the form project/name, search the specified project first, then -// search image families in the specified project. -// If it is of the form name then look in the configured project, then hosted -// image projects, and lastly at image families in hosted image projects. +// If it's in the form projects/{project}/global/images/{image}, return it +// If it's in the form projects/{project}/global/images/family/{family}, return it +// If it's in the form global/images/{image}, return it +// If it's in the form global/images/family/{family}, return it +// If it's in the form family/{family}, check if it's a family in the current project. If it is, return it as global/images/family/{family}. +// If not, check if it could be a GCP-provided family, and if it exists. If it does, return it as projects/{project}/global/images/family/{family}. +// If it's in the form {project}/{family-or-image}, check if it's an image in the named project. If it is, return it as projects/{project}/global/images/{image}. +// If not, check if it's a family in the named project. If it is, return it as projects/{project}/global/images/family/{family}. +// If it's in the form {family-or-image}, check if it's an image in the current project. If it is, return it as global/images/{image}. +// If not, check if it could be a GCP-provided image, and if it exists. If it does, return it as projects/{project}/global/images/{image}. +// If not, check if it's a family in the current project. If it is, return it as global/images/family/{family}. +// If not, check if it could be a GCP-provided family, and if it exists. If it does, return it as projects/{project}/global/images/family/{family} func resolveImage(c *Config, name string) (string, error) { // built-in projects to look for images/families containing the string // on the left in From c87d4101a8d7978cdf4691883942571afb466fa6 Mon Sep 17 00:00:00 2001 From: Paddy Date: Fri, 24 Feb 2017 00:48:03 -0800 Subject: [PATCH 004/385] provider/google: add migration notes for projects Update the docs for `google_project` with @zachgersh's suggestions (#11895) to properly communicate the changes that took place in 0.8.5. While they don't break any current configs or state, the new behaviour should be called out for people who were using the old behaviour and are adding new projects to their configs/state. I also took this opportunity to update google_project_iam_policy with a note to users letting them know that there be dragons. --- .../google/r/google_project.html.markdown | 23 +++++++++++++++---- .../r/google_project_iam_policy.html.markdown | 3 +++ 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/website/source/docs/providers/google/r/google_project.html.markdown b/website/source/docs/providers/google/r/google_project.html.markdown index d84d38cc28..ad668b6725 100755 --- a/website/source/docs/providers/google/r/google_project.html.markdown +++ b/website/source/docs/providers/google/r/google_project.html.markdown @@ -19,6 +19,24 @@ resource must have `roles/resourcemanager.projectCreator`. See the [Access Control for Organizations Using IAM](https://cloud.google.com/resource-manager/docs/access-control-org) doc for more information. +Note that prior to 0.8.5, `google_project` functioned like a data source, +meaning any project referenced by it had to be created and managed outside +Terraform. As of 0.8.5, `google_project` functions like any other Terraform +resource, with Terraform creating and managing the project. To replicate the old +behavior, either: + +* Use the project ID directly in whatever is referencing the project, using the + [google_project_iam_policy](/docs/providers/google/r/google_project_iam_policy.html) + to replace the old `policy_data` property. +* Use the [import](/docs/import/usage.html) functionality + to import your pre-existing project into Terraform, where it can be referenced and + used just like always, keeping in mind that Terraform will attempt to undo any changes + made outside Terraform. + +~> It's important to note that any project resources that were added to your Terraform config +prior to 0.8.5 will continue to function as they always have, and will not be managed by +Terraform. Only newly added projects are affected. + ## Example Usage ```js @@ -58,9 +76,6 @@ The following arguments are supported: * `name` - (Optional) The display name of the project. This is required if you are creating a new project. -* `services` - (Optional) The services/APIs that are enabled for this project. - For a list of available services, run `gcloud beta service-management list` - * `skip_delete` - (Optional) If true, the Terraform resource can be deleted without deleting the Project via the Google API. @@ -81,7 +96,7 @@ exported: ## ID Field -In previous versions of Terraform, `google_project` resources used an `id` field in +In versions of Terraform prior to 0.8.5, `google_project` resources used an `id` field in config files to specify the project ID. Unfortunately, due to limitations in Terraform, this field always looked empty to Terraform. Terraform fell back on using the project the Google Cloud provider is configured with. If you're using the `id` field in your diff --git a/website/source/docs/providers/google/r/google_project_iam_policy.html.markdown b/website/source/docs/providers/google/r/google_project_iam_policy.html.markdown index 94a991f975..dcc9d87b75 100644 --- a/website/source/docs/providers/google/r/google_project_iam_policy.html.markdown +++ b/website/source/docs/providers/google/r/google_project_iam_policy.html.markdown @@ -11,6 +11,9 @@ description: |- Allows creation and management of an IAM policy for an existing Google Cloud Platform project. +~> **Be careful!** You can accidentally lock yourself out of your project + using this resource. Proceed with caution. + ## Example Usage ```js From 2a949093ed7cb3059d631ac8217b03243e37c1aa Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 2 Mar 2017 14:10:41 -0500 Subject: [PATCH 005/385] report all errors from module validation It can be tedious fixing a new module with many errors when Terraform only outputs the first random error it encounters. Accumulate all errors from validation, and format them for the user. --- .../validate-required-var/child/main.tf | 1 + config/module/tree.go | 87 ++++++++++++------- config/module/tree_test.go | 11 ++- 3 files changed, 67 insertions(+), 32 deletions(-) diff --git a/config/module/test-fixtures/validate-required-var/child/main.tf b/config/module/test-fixtures/validate-required-var/child/main.tf index 618ae3c42e..00b6c4e5bd 100644 --- a/config/module/test-fixtures/validate-required-var/child/main.tf +++ b/config/module/test-fixtures/validate-required-var/child/main.tf @@ -1 +1,2 @@ variable "memory" {} +variable "feature" {} diff --git a/config/module/tree.go b/config/module/tree.go index d20f163a49..8ce24a321b 100644 --- a/config/module/tree.go +++ b/config/module/tree.go @@ -259,7 +259,7 @@ func (t *Tree) Validate() error { } // If something goes wrong, here is our error template - newErr := &TreeError{Name: []string{t.Name()}} + newErr := &treeError{Name: []string{t.Name()}} // Terraform core does not handle root module children named "root". // We plan to fix this in the future but this bug was brought up in @@ -271,15 +271,14 @@ func (t *Tree) Validate() error { // Validate our configuration first. if err := t.config.Validate(); err != nil { - newErr.Err = err - return newErr + newErr.Add(err) } // If we're the root, we do extra validation. This validation usually // requires the entire tree (since children don't have parent pointers). if len(t.path) == 0 { if err := t.validateProviderAlias(); err != nil { - return err + newErr.Add(err) } } @@ -293,7 +292,7 @@ func (t *Tree) Validate() error { continue } - verr, ok := err.(*TreeError) + verr, ok := err.(*treeError) if !ok { // Unknown error, just return... return err @@ -301,7 +300,7 @@ func (t *Tree) Validate() error { // Append ourselves to the error and then return verr.Name = append(verr.Name, t.Name()) - return verr + newErr.AddChild(verr) } // Go over all the modules and verify that any parameters are valid @@ -327,10 +326,9 @@ func (t *Tree) Validate() error { // Compare to the keys in our raw config for the module for k, _ := range m.RawConfig.Raw { if _, ok := varMap[k]; !ok { - newErr.Err = fmt.Errorf( + newErr.Add(fmt.Errorf( "module %s: %s is not a valid parameter", - m.Name, k) - return newErr + m.Name, k)) } // Remove the required @@ -339,10 +337,9 @@ func (t *Tree) Validate() error { // If we have any required left over, they aren't set. for k, _ := range requiredMap { - newErr.Err = fmt.Errorf( - "module %s: required variable %s not set", - m.Name, k) - return newErr + newErr.Add(fmt.Errorf( + "module %s: required variable %q not set", + m.Name, k)) } } @@ -369,33 +366,61 @@ func (t *Tree) Validate() error { } } if !found { - newErr.Err = fmt.Errorf( + newErr.Add(fmt.Errorf( "%s: %s is not a valid output for module %s", - source, mv.Field, mv.Name) - return newErr + source, mv.Field, mv.Name)) } } } + return newErr.ErrOrNil() +} + +// treeError is an error use by Tree.Validate to accumulates all +// validation errors. +type treeError struct { + Name []string + Errs []error + Children []*treeError +} + +func (e *treeError) Add(err error) { + e.Errs = append(e.Errs, err) +} + +func (e *treeError) AddChild(err *treeError) { + e.Children = append(e.Children, err) +} + +func (e *treeError) ErrOrNil() error { + if len(e.Errs) > 0 || len(e.Children) > 0 { + return e + } return nil } -// TreeError is an error returned by Tree.Validate if an error occurs -// with validation. -type TreeError struct { - Name []string - Err error -} +func (e *treeError) Error() string { + name := strings.Join(e.Name, ".") + var out bytes.Buffer + fmt.Fprintf(&out, "module %s: ", name) -func (e *TreeError) Error() string { - // Build up the name - var buf bytes.Buffer - for _, n := range e.Name { - buf.WriteString(n) - buf.WriteString(".") + if len(e.Errs) == 1 { + // single like error + out.WriteString(e.Errs[0].Error()) + } else { + // multi-line error + for _, err := range e.Errs { + fmt.Fprintf(&out, "\n %s", err) + } } - buf.Truncate(buf.Len() - 1) - // Format the value - return fmt.Sprintf("module %s: %s", buf.String(), e.Err) + if len(e.Children) > 0 { + // start the next error on a new line + out.WriteString("\n ") + } + for _, child := range e.Children { + out.WriteString(child.Error()) + } + + return out.String() } diff --git a/config/module/tree_test.go b/config/module/tree_test.go index 6ca5f2a72e..475ce7a7cb 100644 --- a/config/module/tree_test.go +++ b/config/module/tree_test.go @@ -410,9 +410,18 @@ func TestTreeValidate_requiredChildVar(t *testing.T) { t.Fatalf("err: %s", err) } - if err := tree.Validate(); err == nil { + err := tree.Validate() + if err == nil { t.Fatal("should error") } + + // ensure both variables are mentioned in the output + errMsg := err.Error() + for _, v := range []string{"feature", "memory"} { + if !strings.Contains(errMsg, v) { + t.Fatalf("no mention of missing variable %q", v) + } + } } const treeLoadStr = ` From 9b1da31ca418165e793fb262020652742a4b1364 Mon Sep 17 00:00:00 2001 From: Paddy Date: Thu, 2 Mar 2017 14:00:45 -0800 Subject: [PATCH 006/385] provider/google: ignore expanded v collapsed policies in diff When comparing the config and state for google_project_iam_policy, always merge the bindings down to a common representation, to avoid a perpetual diff. Fixes #11763. --- .../resource_google_project_iam_policy.go | 2 + ...resource_google_project_iam_policy_test.go | 93 ++++++++++++++++++- 2 files changed, 94 insertions(+), 1 deletion(-) diff --git a/builtin/providers/google/resource_google_project_iam_policy.go b/builtin/providers/google/resource_google_project_iam_policy.go index cf9c87ef8a..4b2ec79b79 100644 --- a/builtin/providers/google/resource_google_project_iam_policy.go +++ b/builtin/providers/google/resource_google_project_iam_policy.go @@ -373,6 +373,8 @@ func jsonPolicyDiffSuppress(k, old, new string, d *schema.ResourceData) bool { log.Printf("[ERROR] Could not unmarshal new policy %s: %v", new, err) return false } + oldPolicy.Bindings = mergeBindings(oldPolicy.Bindings) + newPolicy.Bindings = mergeBindings(newPolicy.Bindings) if newPolicy.Etag != oldPolicy.Etag { return false } diff --git a/builtin/providers/google/resource_google_project_iam_policy_test.go b/builtin/providers/google/resource_google_project_iam_policy_test.go index 59903ca8ae..f0a897e2c2 100644 --- a/builtin/providers/google/resource_google_project_iam_policy_test.go +++ b/builtin/providers/google/resource_google_project_iam_policy_test.go @@ -254,7 +254,24 @@ func TestAccGoogleProjectIamPolicy_basic(t *testing.T) { }) } -func testAccCheckGoogleProjectIamPolicyIsMerged(projectRes, policyRes, pid string) resource.TestCheckFunc { +// Test that a non-collapsed IAM policy doesn't perpetually diff +func TestAccGoogleProjectIamPolicy_expanded(t *testing.T) { + pid := "terraform-" + acctest.RandString(10) + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: testAccGoogleProjectAssociatePolicyExpanded(pid, pname, org), + Check: resource.ComposeTestCheckFunc( + testAccCheckGoogleProjectIamPolicyExists("google_project_iam_policy.acceptance", "data.google_iam_policy.expanded", pid), + ), + }, + }, + }) +} + +func testAccCheckGoogleProjectIamPolicyExists(projectRes, policyRes, pid string) resource.TestCheckFunc { return func(s *terraform.State) error { // Get the project resource project, ok := s.RootModule().Resources[projectRes] @@ -290,11 +307,56 @@ func testAccCheckGoogleProjectIamPolicyIsMerged(projectRes, policyRes, pid strin } // The bindings in both policies should be identical + projectP.Bindings = mergeBindings(projectP.Bindings) + policyP.Bindings = mergeBindings(policyP.Bindings) sort.Sort(sortableBindings(projectP.Bindings)) sort.Sort(sortableBindings(policyP.Bindings)) if !reflect.DeepEqual(derefBindings(projectP.Bindings), derefBindings(policyP.Bindings)) { return fmt.Errorf("Project and data source policies do not match: project policy is %+v, data resource policy is %+v", derefBindings(projectP.Bindings), derefBindings(policyP.Bindings)) } + return nil + } +} + +func testAccCheckGoogleProjectIamPolicyIsMerged(projectRes, policyRes, pid string) resource.TestCheckFunc { + return func(s *terraform.State) error { + // Get the project resource + project, ok := s.RootModule().Resources[projectRes] + if !ok { + return fmt.Errorf("Not found: %s", projectRes) + } + // The project ID should match the config's project ID + if project.Primary.ID != pid { + return fmt.Errorf("Expected project %q to match ID %q in state", pid, project.Primary.ID) + } + + err := testAccCheckGoogleProjectIamPolicyExists(projectRes, policyRes, pid)(s) + if err != nil { + return err + } + + var projectP, policyP cloudresourcemanager.Policy + // The project should have a policy + ps, ok := project.Primary.Attributes["policy_data"] + if !ok { + return fmt.Errorf("Project resource %q did not have a 'policy_data' attribute. Attributes were %#v", project.Primary.Attributes["id"], project.Primary.Attributes) + } + if err := json.Unmarshal([]byte(ps), &projectP); err != nil { + return fmt.Errorf("Could not unmarshal %s:\n: %v", ps, err) + } + + // The data policy resource should have a policy + policy, ok := s.RootModule().Resources[policyRes] + if !ok { + return fmt.Errorf("Not found: %s", policyRes) + } + ps, ok = policy.Primary.Attributes["policy_data"] + if !ok { + return fmt.Errorf("Data policy resource %q did not have a 'policy_data' attribute. Attributes were %#v", policy.Primary.Attributes["id"], project.Primary.Attributes) + } + if err := json.Unmarshal([]byte(ps), &policyP); err != nil { + return err + } // Merge the project policy in Terraform state with the policy the project had before the config was applied expected := make([]*cloudresourcemanager.Binding, 0) @@ -634,3 +696,32 @@ resource "google_project" "acceptance" { billing_account = "%s" }`, pid, name, org, billing) } + +func testAccGoogleProjectAssociatePolicyExpanded(pid, name, org string) string { + return fmt.Sprintf(` +resource "google_project" "acceptance" { + project_id = "%s" + name = "%s" + org_id = "%s" +} +resource "google_project_iam_policy" "acceptance" { + project = "${google_project.acceptance.id}" + policy_data = "${data.google_iam_policy.expanded.policy_data}" + authoritative = false +} +data "google_iam_policy" "expanded" { + binding { + role = "roles/viewer" + members = [ + "user:paddy@carvers.co", + ] + } + + binding { + role = "roles/viewer" + members = [ + "user:paddy@hashicorp.com", + ] + } +}`, pid, name, org) +} From e58a02405ee8e3e77d0a322b0edec4565cf50f3b Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 3 Mar 2017 18:19:56 -0500 Subject: [PATCH 007/385] missing args assignment after parsing flags `env list` was missing the args re-assignment after parsing the flags. This is only a problem if the variables are automatically be populated as arguments from a tfvars file. --- command/env_command_test.go | 10 ++++++++++ command/env_list.go | 1 + 2 files changed, 11 insertions(+) diff --git a/command/env_command_test.go b/command/env_command_test.go index 356c8d66aa..0b28beb014 100644 --- a/command/env_command_test.go +++ b/command/env_command_test.go @@ -64,6 +64,16 @@ func TestEnv_createAndList(t *testing.T) { defer os.RemoveAll(td) defer testChdir(t, td)() + // make sure a vars file doesn't interfere + err := ioutil.WriteFile( + DefaultVarsFilename, + []byte(`foo = "bar"`), + 0644, + ) + if err != nil { + t.Fatal(err) + } + newCmd := &EnvNewCommand{} envs := []string{"test_a", "test_b", "test_c"} diff --git a/command/env_list.go b/command/env_list.go index 219b32bd05..12d768e802 100644 --- a/command/env_list.go +++ b/command/env_list.go @@ -19,6 +19,7 @@ func (c *EnvListCommand) Run(args []string) int { return 1 } + args = cmdFlags.Args() configPath, err := ModulePath(args) if err != nil { c.Ui.Error(err.Error()) From 6cce8d6c1a6750cdfdf6965e524eefcad5f11863 Mon Sep 17 00:00:00 2001 From: Paddy Date: Mon, 6 Mar 2017 21:14:32 -0800 Subject: [PATCH 008/385] provider/google: fix container instance group URLs Google Container Engine's cluster API returned instance group manager URLs when it meant to return instance group URLs. See #4336 for details about the bug. While this is undeniably an upstream problem, this PR: * detects the error, meaning it will work as expected when the API is fixed. * corrects the error by requesting the instance group manager, then retrieving its instance group URL, and using that instead. * adds a test that exercises the error and the solution, to ensure it is functioning properly. --- .../google/resource_container_cluster.go | 27 +++++++- .../google/resource_container_cluster_test.go | 66 ++++++++++++++++++- 2 files changed, 91 insertions(+), 2 deletions(-) diff --git a/builtin/providers/google/resource_container_cluster.go b/builtin/providers/google/resource_container_cluster.go index fd9aa43a96..ccea976b6d 100644 --- a/builtin/providers/google/resource_container_cluster.go +++ b/builtin/providers/google/resource_container_cluster.go @@ -13,6 +13,10 @@ import ( "google.golang.org/api/googleapi" ) +var ( + instanceGroupManagerURL = regexp.MustCompile("^https://www.googleapis.com/compute/v1/projects/([a-z][a-z0-9-]{5}(?:[-a-z0-9]{0,23}[a-z0-9])?)/zones/([a-z0-9-]*)/instanceGroupManagers/([^/]*)") +) + func resourceContainerCluster() *schema.Resource { return &schema.Resource{ Create: resourceContainerClusterCreate, @@ -474,7 +478,28 @@ func resourceContainerClusterRead(d *schema.ResourceData, meta interface{}) erro d.Set("network", d.Get("network").(string)) d.Set("subnetwork", cluster.Subnetwork) d.Set("node_config", flattenClusterNodeConfig(cluster.NodeConfig)) - d.Set("instance_group_urls", cluster.InstanceGroupUrls) + + // container engine's API currently mistakenly returns the instance group manager's + // URL instead of the instance group's URL in its responses. This shim detects that + // error, and corrects it, by fetching the instance group manager URL and retrieving + // the instance group manager, then using that to look up the instance group URL, which + // is then substituted. + // + // This should be removed when the API response is fixed. + instanceGroupURLs := make([]string, 0, len(cluster.InstanceGroupUrls)) + for _, u := range cluster.InstanceGroupUrls { + if !instanceGroupManagerURL.MatchString(u) { + instanceGroupURLs = append(instanceGroupURLs, u) + continue + } + matches := instanceGroupManagerURL.FindStringSubmatch(u) + instanceGroupManager, err := config.clientCompute.InstanceGroupManagers.Get(matches[1], matches[2], matches[3]).Do() + if err != nil { + return fmt.Errorf("Error reading instance group manager returned as an instance group URL: %s", err) + } + instanceGroupURLs = append(instanceGroupURLs, instanceGroupManager.InstanceGroup) + } + d.Set("instance_group_urls", instanceGroupURLs) return nil } diff --git a/builtin/providers/google/resource_container_cluster_test.go b/builtin/providers/google/resource_container_cluster_test.go index 4f4ff82010..1461af9324 100644 --- a/builtin/providers/google/resource_container_cluster_test.go +++ b/builtin/providers/google/resource_container_cluster_test.go @@ -4,10 +4,11 @@ import ( "fmt" "testing" + "strconv" + "github.com/hashicorp/terraform/helper/acctest" "github.com/hashicorp/terraform/helper/resource" "github.com/hashicorp/terraform/terraform" - "strconv" ) func TestAccContainerCluster_basic(t *testing.T) { @@ -116,6 +117,23 @@ func TestAccContainerCluster_network(t *testing.T) { }) } +func TestAccContainerCluster_backend(t *testing.T) { + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckContainerClusterDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: testAccContainerCluster_backendRef, + Check: resource.ComposeTestCheckFunc( + testAccCheckContainerClusterExists( + "google_container_cluster.primary"), + ), + }, + }, + }) +} + func testAccCheckContainerClusterDestroy(s *terraform.State) error { config := testAccProvider.Meta().(*Config) @@ -296,3 +314,49 @@ resource "google_container_cluster" "with_net_ref_by_name" { network = "${google_compute_network.container_network.name}" }`, acctest.RandString(10), acctest.RandString(10), acctest.RandString(10)) + +var testAccContainerCluster_backendRef = fmt.Sprintf(` +resource "google_compute_backend_service" "my-backend-service" { + name = "terraform-test-%s" + port_name = "http" + protocol = "HTTP" + + backend { + group = "${element(google_container_cluster.primary.instance_group_urls, 1)}" + } + + health_checks = ["${google_compute_http_health_check.default.self_link}"] +} + +resource "google_compute_http_health_check" "default" { + name = "terraform-test-%s" + request_path = "/" + check_interval_sec = 1 + timeout_sec = 1 +} + +resource "google_container_cluster" "primary" { + name = "terraform-test-%s" + zone = "us-central1-a" + initial_node_count = 3 + + additional_zones = [ + "us-central1-b", + "us-central1-c", + ] + + master_auth { + username = "mr.yoda" + password = "adoy.rm" + } + + node_config { + oauth_scopes = [ + "https://www.googleapis.com/auth/compute", + "https://www.googleapis.com/auth/devstorage.read_only", + "https://www.googleapis.com/auth/logging.write", + "https://www.googleapis.com/auth/monitoring", + ] + } +} +`, acctest.RandString(10), acctest.RandString(10), acctest.RandString(10)) From 1d9d7be28c32d25bfcc557071b5aac285ec06e8a Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 7 Mar 2017 09:19:02 -0500 Subject: [PATCH 009/385] Add schema.Provider.TestReset method Provider.TestReset resets the internal state of the Provider at the start of a test. This also adds a MetaReset function field to schema.Provider, which is called by TestReset and can be used to reset any other tsated stored in the provider metadata. This is currently used to reset the internal Context returned by StopContext between tests, and should be implemented by a provider if it stores a Context from a previous test. --- helper/schema/provider.go | 30 ++++++++++++++++++++++++++++++ helper/schema/provider_test.go | 26 ++++++++++++++++++++++++++ 2 files changed, 56 insertions(+) diff --git a/helper/schema/provider.go b/helper/schema/provider.go index 5b50d54a1f..d52d2f5f06 100644 --- a/helper/schema/provider.go +++ b/helper/schema/provider.go @@ -50,8 +50,15 @@ type Provider struct { // See the ConfigureFunc documentation for more information. ConfigureFunc ConfigureFunc + // MetaReset is called by TestReset to reset any state stored in the meta + // interface. This is especially important if the StopContext is stored by + // the provider. + MetaReset func() error + meta interface{} + // a mutex is required because TestReset can directly repalce the stopCtx + stopMu sync.Mutex stopCtx context.Context stopCtxCancel context.CancelFunc stopOnce sync.Once @@ -124,20 +131,43 @@ func (p *Provider) Stopped() bool { // StopCh returns a channel that is closed once the provider is stopped. func (p *Provider) StopContext() context.Context { p.stopOnce.Do(p.stopInit) + + p.stopMu.Lock() + defer p.stopMu.Unlock() + return p.stopCtx } func (p *Provider) stopInit() { + p.stopMu.Lock() + defer p.stopMu.Unlock() + p.stopCtx, p.stopCtxCancel = context.WithCancel(context.Background()) } // Stop implementation of terraform.ResourceProvider interface. func (p *Provider) Stop() error { p.stopOnce.Do(p.stopInit) + + p.stopMu.Lock() + defer p.stopMu.Unlock() + p.stopCtxCancel() return nil } +// TestReset resets any state stored in the Provider, and will call TestReset +// on Meta if it implements the TestProvider interface. +// This may be used to reset the schema.Provider at the start of a test, and is +// automatically called by resource.Test. +func (p *Provider) TestReset() error { + p.stopInit() + if p.MetaReset != nil { + return p.MetaReset() + } + return nil +} + // Input implementation of terraform.ResourceProvider interface. func (p *Provider) Input( input terraform.UIInput, diff --git a/helper/schema/provider_test.go b/helper/schema/provider_test.go index ed5918844b..5b06c5e576 100644 --- a/helper/schema/provider_test.go +++ b/helper/schema/provider_test.go @@ -381,3 +381,29 @@ func TestProviderStop_stopFirst(t *testing.T) { t.Fatal("should be stopped") } } + +func TestProviderReset(t *testing.T) { + var p Provider + stopCtx := p.StopContext() + p.MetaReset = func() error { + stopCtx = p.StopContext() + return nil + } + + // cancel the current context + p.Stop() + + if err := p.TestReset(); err != nil { + t.Fatal(err) + } + + // the first context should have been replaced + if err := stopCtx.Err(); err != nil { + t.Fatal(err) + } + + // we should not get a canceled context here either + if err := p.StopContext().Err(); err != nil { + t.Fatal(err) + } +} From 1eb9a2d083eb070a2b49b4ca7261a23e452b8bb9 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 7 Mar 2017 10:03:03 -0500 Subject: [PATCH 010/385] Reset ResourceProviders Call all ResourceProviderFactories and reset the providers before tests. Store the provider and/or the error in a fixed factory function to be returned again later. --- helper/resource/testing.go | 53 +++++++++++++++++++++++++++++++++----- 1 file changed, 46 insertions(+), 7 deletions(-) diff --git a/helper/resource/testing.go b/helper/resource/testing.go index 6d2deb9e53..b166c28fef 100644 --- a/helper/resource/testing.go +++ b/helper/resource/testing.go @@ -22,6 +22,12 @@ import ( const TestEnvVar = "TF_ACC" +// TestProvider is implemented by scheme.Provider to allow resetting the +// provider state between tests. +type TestProvider interface { + TestReset() error +} + // TestCheckFunc is the callback type used with acceptance tests to check // the state of a resource. The state passed in is the latest state known, // or in the case of being after a destroy, it is the last known state when @@ -216,13 +222,9 @@ func Test(t TestT, c TestCase) { c.PreCheck() } - // Build our context options that we can - ctxProviders := c.ProviderFactories - if ctxProviders == nil { - ctxProviders = make(map[string]terraform.ResourceProviderFactory) - for k, p := range c.Providers { - ctxProviders[k] = terraform.ResourceProviderFactoryFixed(p) - } + ctxProviders, err := testProviderFactories(c) + if err != nil { + t.Fatal(err) } opts := terraform.ContextOpts{Providers: ctxProviders} @@ -333,6 +335,43 @@ func Test(t TestT, c TestCase) { } } +// testProviderFactories is a helper to build the ResourceProviderFactory map +// with pre instantiated ResourceProviders, so that we can reset them for the +// test, while only calling the factory function once. +// Any errors are stored so that they can be returned by the factory in +// terraform to match non-test behavior. +func testProviderFactories(c TestCase) (map[string]terraform.ResourceProviderFactory, error) { + ctxProviders := make(map[string]terraform.ResourceProviderFactory) + + // add any fixed providers + for k, p := range c.Providers { + ctxProviders[k] = terraform.ResourceProviderFactoryFixed(p) + } + + // call any factory functions and store the result. + for k, pf := range c.ProviderFactories { + p, err := pf() + ctxProviders[k] = func() (terraform.ResourceProvider, error) { + return p, err + } + } + + // reset the providers if needed + for k, pf := range ctxProviders { + // we can ignore any errors here, if we don't have a provider to reset + // the error will be handled later + p, _ := pf() + if p, ok := p.(TestProvider); ok { + err := p.TestReset() + if err != nil { + return nil, fmt.Errorf("[ERROR] failed to reset provider %q: %s", k, err) + } + } + } + + return ctxProviders, nil +} + // UnitTest is a helper to force the acceptance testing harness to run in the // normal unit test suite. This should only be used for resource that don't // have any external dependencies. From 0279d11c8a2bd5ee667635d6c882e69377bf80d6 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 7 Mar 2017 10:05:07 -0500 Subject: [PATCH 011/385] Add TestReset to terraformMockResourceProvider Have MockResourceProvider implement TestProvider to check that TestReset is called by the test harness. --- terraform/resource_provider_mock.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/terraform/resource_provider_mock.go b/terraform/resource_provider_mock.go index f5315339fb..19a4495978 100644 --- a/terraform/resource_provider_mock.go +++ b/terraform/resource_provider_mock.go @@ -56,6 +56,8 @@ type MockResourceProvider struct { ReadDataDiffFn func(*InstanceInfo, *ResourceConfig) (*InstanceDiff, error) ReadDataDiffReturn *InstanceDiff ReadDataDiffReturnError error + TestResetCalled bool + TestResetError error StopCalled bool StopFn func() error StopReturnError error @@ -144,6 +146,14 @@ func (p *MockResourceProvider) Configure(c *ResourceConfig) error { return p.ConfigureReturnError } +func (p *MockResourceProvider) TestReset() error { + p.Lock() + defer p.Unlock() + + p.TestResetCalled = true + return p.TestResetError +} + func (p *MockResourceProvider) Stop() error { p.Lock() defer p.Unlock() From 4b2e96b2e2663eae5a89f7bcdadb6a011dc7d201 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 7 Mar 2017 10:18:09 -0500 Subject: [PATCH 012/385] test for TestReset and fixed resource factories --- helper/resource/testing_test.go | 49 +++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/helper/resource/testing_test.go b/helper/resource/testing_test.go index d2e05c0c52..56e9cd02eb 100644 --- a/helper/resource/testing_test.go +++ b/helper/resource/testing_test.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "os" + "regexp" "strings" "sync/atomic" "testing" @@ -95,6 +96,9 @@ func TestTest(t *testing.T) { if !checkDestroy { t.Fatal("didn't call check for destroy") } + if !mp.TestResetCalled { + t.Fatal("didn't call TestReset") + } } func TestTest_idRefresh(t *testing.T) { @@ -355,6 +359,51 @@ func TestTest_stepError(t *testing.T) { } } +func TestTest_factoryError(t *testing.T) { + resourceFactoryError := fmt.Errorf("resource factory error") + + factory := func() (terraform.ResourceProvider, error) { + return nil, resourceFactoryError + } + + mt := new(mockT) + Test(mt, TestCase{ + ProviderFactories: map[string]terraform.ResourceProviderFactory{ + "test": factory, + }, + Steps: []TestStep{ + TestStep{ + ExpectError: regexp.MustCompile("resource factory error"), + }, + }, + }) + + if !mt.failed() { + t.Fatal("test should've failed") + } +} + +func TestTest_resetError(t *testing.T) { + mp := testProvider() + mp.TestResetError = fmt.Errorf("provider reset error") + + mt := new(mockT) + Test(mt, TestCase{ + Providers: map[string]terraform.ResourceProvider{ + "test": mp, + }, + Steps: []TestStep{ + TestStep{ + ExpectError: regexp.MustCompile("provider reset error"), + }, + }, + }) + + if !mt.failed() { + t.Fatal("test should've failed") + } +} + func TestComposeAggregateTestCheckFunc(t *testing.T) { check1 := func(s *terraform.State) error { return errors.New("Error 1") From 3c41a7ca1ef5aeb6370c3e339136fcd32ad3a115 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 7 Mar 2017 15:01:29 -0500 Subject: [PATCH 013/385] Add test for Validate crash Crash during Validate walk with nested variable default. --- config/config_test.go | 6 ++++++ config/test-fixtures/validate-var-nested/main.tf | 6 ++++++ 2 files changed, 12 insertions(+) create mode 100644 config/test-fixtures/validate-var-nested/main.tf diff --git a/config/config_test.go b/config/config_test.go index 2ef68dae97..3c7828b6a6 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -201,6 +201,12 @@ func TestConfigValidate_table(t *testing.T) { true, "cannot contain interp", }, + { + "nested types in variable default", + "validate-var-nested", + true, + "ERROR", + }, } for i, tc := range cases { diff --git a/config/test-fixtures/validate-var-nested/main.tf b/config/test-fixtures/validate-var-nested/main.tf new file mode 100644 index 0000000000..a3d64647b1 --- /dev/null +++ b/config/test-fixtures/validate-var-nested/main.tf @@ -0,0 +1,6 @@ +variable "foo" { + default = [["foo", "bar"]] +} +variable "bar" { + default = [{foo = "bar"}] +} From a11163590818587e5f17f9467688f6ebca4d75cf Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 7 Mar 2017 15:33:39 -0500 Subject: [PATCH 014/385] Fix panic in interpolate_walk Verify that we have enough containers in the stack to look for a map in replaceCurrent. --- config/config_test.go | 4 ++-- config/interpolate_walk.go | 6 ++++++ 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/config/config_test.go b/config/config_test.go index 3c7828b6a6..b391295c86 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -204,8 +204,8 @@ func TestConfigValidate_table(t *testing.T) { { "nested types in variable default", "validate-var-nested", - true, - "ERROR", + false, + "", }, } diff --git a/config/interpolate_walk.go b/config/interpolate_walk.go index 81fa812087..ead3d102e1 100644 --- a/config/interpolate_walk.go +++ b/config/interpolate_walk.go @@ -206,6 +206,12 @@ func (w *interpolationWalker) Primitive(v reflect.Value) error { } func (w *interpolationWalker) replaceCurrent(v reflect.Value) { + // if we don't have at least 2 values, we're not going to find a map, but + // we could panic. + if len(w.cs) < 2 { + return + } + c := w.cs[len(w.cs)-2] switch c.Kind() { case reflect.Map: From d93df393e9eeb44de8986bcb74574c11aa13a14e Mon Sep 17 00:00:00 2001 From: Jason Costello Date: Thu, 2 Mar 2017 21:59:41 -0800 Subject: [PATCH 015/385] Add mega nav :zap: --- website/source/assets/javascripts/application.js | 3 +++ website/source/assets/stylesheets/application.scss | 3 +++ website/source/layouts/layout.erb | 1 + 3 files changed, 7 insertions(+) diff --git a/website/source/assets/javascripts/application.js b/website/source/assets/javascripts/application.js index 5e308bc94e..bc9250187c 100644 --- a/website/source/assets/javascripts/application.js +++ b/website/source/assets/javascripts/application.js @@ -23,3 +23,6 @@ //= require app/_Engine.Typewriter //= require app/_Sidebar //= require app/_Init + +// assets/javascripts/application.js +//= require hashicorp/mega-nav diff --git a/website/source/assets/stylesheets/application.scss b/website/source/assets/stylesheets/application.scss index e4c0ae7a89..1a039a39c4 100755 --- a/website/source/assets/stylesheets/application.scss +++ b/website/source/assets/stylesheets/application.scss @@ -3,6 +3,9 @@ @import url("//fonts.googleapis.com/css?family=Open+Sans:300,400,600"); +// Mega Nav +@import 'hashicorp/mega-nav'; + // Core variables and mixins @import '_variables'; diff --git a/website/source/layouts/layout.erb b/website/source/layouts/layout.erb index 2b7a67363a..5203f16b47 100644 --- a/website/source/layouts/layout.erb +++ b/website/source/layouts/layout.erb @@ -1,4 +1,5 @@ <%= partial "layouts/meta" %> +<%= mega_nav :terraform %> <%= partial "layouts/header" %> <%= partial "layouts/sidebar" %> <%= yield %> From 98424f87df70e5c5a9acc4f65e12ab03129dfda9 Mon Sep 17 00:00:00 2001 From: Jason Costello Date: Thu, 2 Mar 2017 22:02:19 -0800 Subject: [PATCH 016/385] Remove 'by hashicorp' --- website/source/layouts/_header.erb | 1 - 1 file changed, 1 deletion(-) diff --git a/website/source/layouts/_header.erb b/website/source/layouts/_header.erb index 8d98f7095d..18ca6505b9 100644 --- a/website/source/layouts/_header.erb +++ b/website/source/layouts/_header.erb @@ -6,7 +6,6 @@
- by<%= partial "layouts/svg/svg-by-hashicorp" %><%= partial "layouts/svg/svg-hashicorp-logo" %>HashiCorp