provider/google: upgrade our image resolution logic #12223
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,96 +2,186 @@ 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 | ||
|
There was a problem hiding this comment. According to https://cloud.google.com/compute/docs/reference/latest/images, it has to conform with https://www.ietf.org/rfc/rfc1035.txt. I'm not sure what people tend to actually do in practice. There was a problem hiding this comment. Yeah, I saw that. I just didn't know which specific part of RFC 1035 it meant. Does it mean it must be a valid domain? A valid subdomain? There was a problem hiding this comment. Just took a closer look and it's the same validation as images, so this looks like the right regex to use. |
||
| 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'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 | ||
| 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 | ||
|
There was a problem hiding this comment. This and the following three checks all follow the same format. It might be a bit cleaner to loop over a slice of structs containing the regex, the expected length, and the result URL (although if we can't do a similar thing elsewhere it might look out of place- use your judgment) There was a problem hiding this comment. I thought of doing that, but wasn't super convinced of the benefit--the indirection didn't make it feel any clearer, and because the format of the code would be essentially the same (define a case, find the matches, check the lengths, handle errors, build a string) it seemed to me like it would be adding more code, not removing code. I'm happy to reconsider on this one, though, if you have any suggestions or pointers. There was a problem hiding this comment. I took a look to see if I could quickly do my own suggestion and it looks like you're right that it would add rather than remove code. |
||
| res := resolveImageProjectImage.FindStringSubmatch(name) | ||
| if len(res)-1 != 2 { // subtract one, index zero is the entire matched expression | ||
|
There was a problem hiding this comment. Are there any cases where the length wouldn't match? It feels like this is just confirming that the go regex library works as expected. There was a problem hiding this comment. If you do decide to keep it, since you do this check a whole bunch of times, you could add a function that does it instead. There was a problem hiding this comment. I'm mostly just sanity-checking to make sure that if the regex string changes and the logic handling it isn't updated, or some other type of human error, it's an error, not a crash. I'm open to removing the code, if you think that's overdoing it. :) In terms of a helper function, you mean just something like replacing if len(res)-1 != 2 {
return "", fmt.Errorf("Expected %d project image regex matches, got %d for %s", 2, len(res)-1, name)
}with if err := checkRegexMatches(res, 2); err != nil {
return "", err
}? I could see a case for that, and how it would clean up some code. There was a problem hiding this comment. I don't think it's necessary, but I'm not opposed to having it there either. I do like your helper function though- I think one of the reasons this part of the code feels messy to me is because all of the repeated |
||
| 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) | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +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_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", | ||
| "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) | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From https://cloud.google.com/resource-manager/reference/rest/v1/projects:
However, is it worth it to be strict here? It seems like what we really care about is just that it isn't a slash, and we can let the API handle whether things are actually well-formed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So my thought on this is that I'd like to try to keep my future options open and not trap myself into a forced backwards incompatibility. In this case, that means accepting the smallest set of strings I can while still accepting every valid string.
On the other hand, however, the typical Terraform approach is to avoid client-side validation, as it is a headache keeping it in sync with the server-side validation.
So I could see both sides on this one. Do you have a preference either way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really, though I don't 100% agree with your logic. If we let the server handle it, then we also don't have to worry about backwards incompatibility (that's Google's problem). Sure, we could try to predict it by being extra strict now just in case, but we can't really tell in what way the Google API would become stricter. The win by doing client-side validation is more so that problems show up faster to the user, which I think is a valid win. If you do keep it this way though, let's get that regex changed to the actual requirements, since the ones for projects are stricter than for images.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where this comes into play is that we have several custom shorthands for this. The one I'm thinking of specifically is
{project}/{image}and{project}/{family}; if, in the future, we wanted to do something like{family}/{image}for whatever reason, we'd find ourselves in a tough spot, as we wouldn't be able to tell between{project}/{image}and{family}/{image}. But the more I'm thinking about this, the more I'm agreeing with you; we try to avoid doing too much client-side validation because it can be brittle, and in this case, guarding against the vagaries of the future doesn't feel like it's yielding enough concrete benefits.My proposal: Let's accept anything that matches
/[a-zA-Z0-9_-]*/for projects, images, and families. Sound like a good way forward?