provider/google: upgrade our image resolution logic #12223

paddycarver · 2017-02-24T06:30:16Z

Prior to now, GCP images could be specified as:

The URL of the image, which would be passed straight through to the API
{project}/{name}, which would be resolved as an image in the named project, and if found, passed as a URL to the API. If not found, it would be interpreted as an image family in the named project, and if found, resolved to the latest image in that family, which would be passed through to the API as a URL.
{name}, which would be resolved as an image in the authenticated project, and if found, passed as a URL to the API. If not found, it would be interpreted as an image family in the authenticated project, and if found, resolved to the latest image in that family, which would be passed through to the API as a URL. If still not found, it would be checked against a static map of GCP-provided projects, and if it had a match, would be interpreted as an image in that project. If that image was found, it would be passed through to the API as a URL. Finally, if it still wasn't found, it would be interpreted as a family in the GCP-provided project, and if it was found, if would be resolved to the latest image in that family, which would be passed through to the API as a URL.

This code was used uniformly across all our disk input modes (as a sub-resource of instances, as standalone disks, as instance templates), but it had some shortcomings:

Not all API inputs were valid inputs for Terraform, which feels unintuitive, especially as there's no real reason they couldn't be supported. Valid API inputs:
- projects/{project}/global/images/family/{family}
- projects/{project}/global/images/{image}
- global/images/{image}
- global/images/family/{family}
Families would get resolved to images, meaning things like instance templates would get frozen to a specific image, even though the user asked for them to point to the most recent image for the family at disk spinup time.
The freezing of images happened at disk spinup time, meaning Terraform had no idea whether a newer image was available, and couldn't prompt the user to apply the change.
In the case of an image and a family conflicting in names, there was no way to tell Terraform which to use. While this shouldn't happen, it'd be nice to have the ability to workaround it.

This PR updates the resolveImage helper in two ways:

First, it hews closer to the API input. URLs are still passed as URLs, but everything else gets converted to the API equivalent that most closely matches the user intent. For example, {project}/family specifies the project, but not an image, so it gets converted to projects/{project}/global/images/family/{family}. family/{family} does not specify a project or an image, so it gets converted to global/images/family/{family}, rather than interpolating the authenticating project in.
The following inputs are now accepted:
- family/{family}
- projects/{project}/global/images/{image}
- {family}
- {image}
- {URL to image}
- global/images/{image}
- global/images/family/{family}
- {project}/{image}
- {project}/{family}

I also added an acceptance test that checks that each of those inputs gets converted to the API values we expect, including for images and families provided by GCP.

This change was prompted by #10984 and is half the solution to it. The other half is to provide a data source which will return the latest image in a family, which will allow users to pin disks to the latest image at terraform apply-time, and use terraform apply to roll out updates to the disk image.

Add tests that show what we want image input strings to resolve to, so we can test that behaviour.

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.

Update the explanation of the logic being followed in resolveImage.

danawillow · 2017-02-28T01:47:51Z

builtin/providers/google/image.go

-func resolveImage(c *Config, name string) (string, error) {
+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


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.

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?

Just took a closer look and it's the same validation as images, so this looks like the right regex to use.

danawillow · 2017-02-28T01:54:05Z

builtin/providers/google/image.go

-// image projects, and lastly at image families in hosted image projects.
-func resolveImage(c *Config, name string) (string, error) {
+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


From https://cloud.google.com/resource-manager/reference/rest/v1/projects:

It must be 6 to 30 lowercase letters, digits, or hyphens. It must start with a letter. Trailing hyphens are prohibited.

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.

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?

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.

If we let the server handle it, then we also don't have to worry about backwards incompatibility (that's Google's problem).

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?

danawillow · 2017-02-28T01:58:52Z

builtin/providers/google/image.go

+		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


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.

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.

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.

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 -1s and their associated // subtract one, index zero is the entire matched expression comments, so this would help add more whitespace into that section.

danawillow · 2017-02-28T02:03:55Z

builtin/providers/google/image.go

+	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


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)

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.

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.

* Make our regexes more permissive (though still separated out for readability, despite being identical) * Add a helper that will improve readability while sanity testing our regex results.

paddycarver · 2017-03-13T23:43:15Z

Latest push should address all feedback. Unit test still runs:

=== RUN   TestAccComputeImage_resolveImage
--- PASS: TestAccComputeImage_resolveImage (93.62s)
PASS

danawillow · 2017-03-14T03:31:58Z

builtin/providers/google/image.go

@@ -47,6 +47,13 @@ func resolveImageFamilyExists(c *Config, project, name string) (bool, error) {
 	}
 }

+func sanityTestRegexMatches(expected int, got []string, regexType, name string) error {
+	if len(got)-1 != expected { // subtract one, index zero is the entire matched expression
+		return fmt.Errorf("Expected %d %s regex matches, got %d for %s", 2, regexType, len(got)-1, name)


Shouldn't this 2 be expected?

Should be addressed now.

danawillow

Looks good (aside from the small comment), merge whenever you're ready.

We never updated the error to use the expectation, not hardcode it to 2.

Document the acceptable inputs everywhere that uses the function.

paddycarver · 2017-03-14T05:16:54Z

Updated docs to document the new acceptable inputs and updated the copy/paste error you noticed. Still look good? :)

danawillow · 2017-03-14T06:21:56Z

website/source/docs/providers/google/r/compute_disk.html.markdown

-* `image` - (Optional) The image from which to initialize this disk. Either the
-    full URL, a contraction of the form "project/name", or just a name (in which
-    case the current project is used).
+* `image` - (Optional) The image from which to initialize this disk. This can be


Can each of the image choices be code font?

danawillow · 2017-03-14T06:22:01Z

website/source/docs/providers/google/r/compute_instance.html.markdown

-    [image family](https://cloud.google.com/compute/docs/images#image_families),
-    or simple the name of an image or image family (in which case the current
-    project is used).
+* `image` - The image from which to initialize this disk. This can be


danawillow · 2017-03-14T06:22:09Z

website/source/docs/providers/google/r/compute_instance_template.html.markdown

-* `source_image` - (Required if source not set) The name of the image to base
-    this disk off of. Accepts same arguments as a [google_compute_instance image](https://www.terraform.io/docs/providers/google/r/compute_instance.html#image).
+* `source_image` - (Required if source not set) The image from which to
+    initialize this disk. This can be one of: the image self_link,


danawillow · 2017-03-14T06:23:00Z

Still looks good! Feel free to merge when ready (after the code font changes if you agree with that comment)

Update our docs to code format all the image template strings that a disk can use.

paddycarver · 2017-03-14T06:40:36Z

Updated! Once Travis goes green, I'll merge.

ghost · 2020-04-12T02:30:06Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

paddycarver and others added 5 commits January 23, 2017 16:45

Start adding tests for image resolution.

59e6324

Add tests that show what we want image input strings to resolve to, so we can test that behaviour.

Merge branch 'master' into paddy_10984_better_image_resolution

95e01ad

Merge branch 'master' into paddy_10984_better_image_resolution

73565af

Update the docs for resolveImage.

6868ba7

Update the explanation of the logic being followed in resolveImage.

paddycarver added enhancement provider/google-cloud labels Feb 24, 2017

paddycarver mentioned this pull request Feb 24, 2017

Google Image Family doesn't detect when "latest" image changes. #10984

Closed

paddycarver requested a review from danawillow February 24, 2017 06:35

danawillow suggested changes Feb 28, 2017

View reviewed changes

Update with @danawillow's feedback.

4c41729

* Make our regexes more permissive (though still separated out for readability, despite being identical) * Add a helper that will improve readability while sanity testing our regex results.

danawillow reviewed Mar 14, 2017

View reviewed changes

danawillow approved these changes Mar 14, 2017

View reviewed changes

paddycarver added 2 commits March 13, 2017 22:04

Update typo.

72bfc43

We never updated the error to use the expectation, not hardcode it to 2.

Update the documentation.

ec93fb5

Document the acceptable inputs everywhere that uses the function.

danawillow reviewed Mar 14, 2017

View reviewed changes

Make all image string templates code formatted.

32c1bb4

Update our docs to code format all the image template strings that a disk can use.

paddycarver merged commit 014983d into master Mar 14, 2017

grubernaut deleted the paddy_10984_better_image_resolution branch May 11, 2017 00:34

ghost locked and limited conversation to collaborators Apr 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

provider/google: upgrade our image resolution logic #12223

provider/google: upgrade our image resolution logic #12223

paddycarver commented Feb 24, 2017

danawillow Feb 28, 2017

paddycarver Mar 2, 2017

danawillow Mar 13, 2017

danawillow Feb 28, 2017

paddycarver Mar 2, 2017

danawillow Mar 13, 2017

paddycarver Mar 13, 2017

danawillow Feb 28, 2017

danawillow Feb 28, 2017

paddycarver Mar 2, 2017

danawillow Mar 13, 2017

danawillow Feb 28, 2017

paddycarver Mar 2, 2017

danawillow Mar 13, 2017

paddycarver commented Mar 13, 2017

danawillow Mar 14, 2017

paddycarver Mar 14, 2017

danawillow left a comment

paddycarver commented Mar 14, 2017

danawillow Mar 14, 2017

danawillow Mar 14, 2017

danawillow Mar 14, 2017

danawillow commented Mar 14, 2017

paddycarver commented Mar 14, 2017

ghost commented Apr 12, 2020

provider/google: upgrade our image resolution logic #12223

provider/google: upgrade our image resolution logic #12223

Conversation

paddycarver commented Feb 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paddycarver commented Mar 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danawillow left a comment

Choose a reason for hiding this comment

paddycarver commented Mar 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danawillow commented Mar 14, 2017

paddycarver commented Mar 14, 2017

ghost commented Apr 12, 2020