terraform transport replaceVars always escapes all variables using url.PathEscape

[odedniv]

Moving from hashicorp/terraform-provider-google#3347
Reverts the changes from 0dad6e1
Fixes hashicorp/terraform-provider-google#2895

Description

According to https://cloud.google.com/storage/docs/json_api/#encoding URI path parts need to be encoded.

For example in google_storage_object_access_control if the object name contains a /, the request will fail with:

* google_storage_object_access_control.public_rule: Error creating ObjectAccessControl: googleapi: got HTTP response code 404 with body: Not Found

To my knowledge, replaceVars() is specifically for replacing variables in URLs (due to the name of the parameter linkTmpl), so I'm assuming this would work globally. Maybe this function should be renamed to replaceURLVars()?

The change from 0dad6e1 allowed prepending the variable name with %, but I think the user should not be aware of this API limitation (of needing to escape). Also note that only variables inserted to URLs should be escaped (as opposed to variables used in the body of the API request), so it may be confusing for the user who specified he wants to escape the variable but then the variable is not always escaped across the different API requests.

[modular-magician]

Hello! I am a robot who works on Magic Modules PRs.

I have detected that you are a community contributor, so your PR will be assigned to someone with a commit-bit on this repo for initial review. They will authorize it to run through our CI pipeline, which will generate downstream PRs.

Thanks for your contribution! A human will be with you soon.

[rileykarson]

Hey @odedniv! The reason we used an opt-in mechanism here instead of converting all values is that when forming some URLs, replaceVars needs to preserve / characters in the original input. See AccessLevel for example, where {{name}} has the form accessPolicies/{policy_id}/accessLevels/{short_name}.

Even with the opt-in solution, users aren't required to url-encode their values at import time / know they're encoded; making the change as we've done means that users will specify non-encoded values and replaceVars will encode the user-specified value when building the URL.

We can mark the necessary field in google_storage_object_access_control with a % and it should behave as expected to close out hashicorp/terraform-provider-google#2895.

[lawliet89]

Related to https://github.com/GoogleCloudPlatform/magic-modules/issues/1414

[odedniv]

I get it now @rileykarson, it's not the user who adds the % to his object name, it's in the resource's code. This makes sense.

I see the "bad" use of replaceVars in https://github.com/terraform-providers/terraform-provider-google/blob/master/google/resource_storage_object_access_control.go#L131, but I don't understand where it's located in magic-modules. Can you point me in the right direction so I can create a pull request?

Also I don't understand why they use raw HTTP request rather than use the Storage client like they do in https://github.com/terraform-providers/terraform-provider-google/blob/master/google/resource_storage_object_acl.go#L309. Using the official client to make the request would probably be a better solution here.

[rileykarson]

That's sourced in the api.yaml file for the product, under the base_url for the resource: https://github.com/GoogleCloudPlatform/magic-modules/blob/master/products/storage/api.yaml#L534

We make direct calls to the API because some newer APIs have no generated client libraries, and existing client libraries have some limitations that means that generating code to interop with them is difficult; not only would we need to encode the API information, we'd need to encode client library information as well.

[odedniv]

Moved to #1759, thanks @rileykarson