From 90254b945178a8bf6d048f317c250e3cffd3b64e Mon Sep 17 00:00:00 2001 From: Paddy Date: Thu, 23 Feb 2017 21:55:30 -0800 Subject: [PATCH] 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 e4a50905e..912821b59 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 f500c9a4c..e0f56518a 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) +}