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

@@ -2,42 +2,59 @@ 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) {
-
+	// built-in projects to look for images/families containing the string
-	if strings.HasPrefix(name, "https://www.googleapis.com/compute/v1/") {
+	// on the left in
-		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",
@@ -48,50 +65,115 @@ func resolveImage(c *Config, name string) (string, error) {
 		"ubuntu":   "ubuntu-os-cloud",
 		"windows":  "windows-cloud",
 	}
-			var project string
+	var builtInProject string
 	for k, v := range imageMap {
 		if strings.Contains(name, k) {
-					project = v
+			builtInProject = v
 			break
 		}
 	}
-			if project == "" {
+	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 != "" {
-			// There was a match, but the image still may not exist, so check it:
+			if ok, err := resolveImageFamilyExists(c, builtInProject, res[1]); err != nil {
-			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 ok {
-		} else if len(splitName) == 2 {
+				return fmt.Sprintf("projects/%s/global/images/family/%s", builtInProject, res[1]), nil
-
-			// 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
 		}
-
+	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 {
-		} else {
+			return fmt.Sprintf("projects/%s/global/images/%s", res[1], res[2]), nil
-			return "", fmt.Errorf("Invalid image name, require URL, project/name, or just name: %s", name)
+		}
+		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)
 }

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"),
 				),
 			},
 		},
 	})
+}
+
+func testAccCheckComputeImageResolution(n string) resource.TestCheckFunc {
+	return func(s *terraform.State) error {
+		config := testAccProvider.Meta().(*Config)
+		project := config.Project
+
+		rs, ok := s.RootModule().Resources[n]
+		if !ok {
+			return fmt.Errorf("Resource not found: %s", n)
+		}
+
+		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-jessie",
+			"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-jessie":                                                                                     "projects/debian-cloud/global/images/family/debian-8-jessie",
+			"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",
 
-		// TODO(paddy): we need private images/families here to actually test this
+			"global/images/" + name:          "global/images/" + name,
-		"global/images/my-private-image":         "global/images/my-private-image",
+			"global/images/family/" + family: "global/images/family/" + family,
-		"global/images/family/my-private-family": "global/images/family/my-private-family",
+			name:                   "global/images/" + name,
-		"my-private-image":                       "global/images/my-private-image",
+			family:                 "global/images/family/" + family,
-		"my-private-family":                      "global/images/family/my-private-family",
+			"family/" + family:     "global/images/family/" + family,
-		"my-project/my-private-image":            "projects/my-project/global/images/my-private-image",
+			project + "/" + name:   "projects/" + project + "/global/images/" + name,
-		"my-project/my-private-family":           "projects/my-project/global/images/family/my-private-family",
+			project + "/" + family: "projects/" + project + "/global/images/family/" + family,
-		"insert-URL-here":                        "insert-URL-here",
+			link: link,
-	}
-	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)
+				return fmt.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)
+				return fmt.Errorf("Expected input '%s' to resolve to '%s', it resolved to '%s' instead.\n", input, expectation, result)
-			continue
 			}
 		}
+		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)
 }