hashicorp / terraform-provider-google

Terraform Provider for Google Cloud Platform
https://registry.terraform.io/providers/hashicorp/google/latest/docs
Mozilla Public License 2.0
2.29k stars 1.72k forks source link

Allow `google_tags_tag_binding` resource to accept a namespacedName for the `tag_value` argument #11367

Open nphilbrook opened 2 years ago

nphilbrook commented 2 years ago

Community Note

Description

The google_tags_tag_binding resource only accepts the name for the tag_value argument, which is in the format tagValues/12345 - e.g. there is nothing in this value that is readable or contains the tag value's descriptive shortname.

Via the GCP API you can use either the name OR the namespacedName when creating a tag binding - the namespaced name is in the format <org_id>/<key short name>/<value short name> and is much more descriptive/readable.

In our initial rollout of GCP IAM tags we are struggling with writing readable configuration using tagValues/XXXX format. For example, we have a module for which we want to accept a tag value so that we can tag the created resources using this value. But passing in tagValues/XXXX doesn't denote anything descriptive about what the tag value actually is, and even finding the correct value to use is problematic. We considered storing our own copy of namespacedName -> name mappings (which we may still do if this request is rejected). But this seems pretty silly given that you can do this with gcloud:

@arlo ~ $ gcloud alpha resource-manager tags bindings create --tag-value=345678/foo/bar --parent=//cloudresourcemanager.googleapis.com/folders/12345
done: true
response:
  '@type': type.googleapis.com/google.cloud.resourcemanager.v3.TagBinding
  name: tagBindings/%2F%2Fcloudresourcemanager.googleapis.com%2Ffolders%2F12345/tagValues/67890
  parent: //cloudresourcemanager.googleapis.com/folders/12345
  tagValue: tagValues/67890

Notably there is also a lack of any data resources so we can't look up the name of a tagValue given the namespacedName.

New or Affected Resource(s)

Potential Terraform Configuration

resource "google_tags_tag_binding" "bar_binding" {
  parent    = "//cloudresourcemanager.googleapis.com/folders/12345"
  tag_value = "345678/tag_key_short_name_foo/tag_value_short_name_bar"
}

Where 12345 is the ID of a folder, 345678 is the ID of an organization, tag_key_short_name_foo is the shortName of a tag key, and tag_value_short_name_bar is the shortName of a tag value.

References

I could not find any issues requesting this feature.

GCP docs describing the gcloud call I demonstrated above terraform-provider-google docs describing only the tagValues/XXXX format being accepted

N.b. I hope that myself or one of my teammates at Wayfair can contribute this feature in the near future, but as PRs need to reference an issue I am starting with this issue and appreciate any feedback on the feature request.

b/361104631

nphilbrook commented 2 years ago

While poking around the API I discovered that while you can do this operation via gcloud as I pasted an example above, the REST API directly does not accept the namespacedName format :rage:

@lydia ~/temp $ cat tagbinding.json 
{
  "parent": "//cloudresourcemanager.googleapis.com/folders/12345",
  "tagValue": "67890/foo/bar"
}
@lydia ~/temp $ curl -H "Authorization: Bearer $(gcloud auth print-access-token)" -H "Content-Type: application/json" -d @tagbinding.json "https://cloudresourcemanager.googleapis.com/v3/tagBindings"      
{
  "error": {
    "code": 400,
    "message": "field [binding.tag_value] has issue [Must be a resource name for a tag value]",
    "status": "INVALID_ARGUMENT",
    "details": [
      {
        "@type": "type.googleapis.com/google.rpc.BadRequest",
        "fieldViolations": [
          {
            "field": "binding.tag_value",
            "description": "Must be a resource name for a tag value"
          }
        ]
      }
    ]
  }
}

This is frustrating and it follows now why the TF resource doesn't accept this value either, as it's a thin mapping over the REST layer (as I understand it). gcloud must be doing a lookup in the background.

I may pivot to creating a data resource for tag values which would at least get us where we need to go.

nphilbrook commented 2 years ago

Verified using --log-http that the gcloud command I pasted above is in fact doing 3 HTTP calls to create the binding (looks up all tag keys at org level, then looks up the tag values for the matching key, then does the binding call).

nphilbrook commented 2 years ago

@rileykarson I see that you added a t-shirt size to this issue, thank you!

Meta-question I hope you can answer. Is there an appropriate forum to ask question regarding provider contributions?

Actual question: If I wanted to implement the functionality requested in this ticket, would it be possible to leave the magic modules-generated code in place and append this functionality via code in third_party? I'm just learning how the mmv1 stuff works and it doesn't look like this would be an option, based on my limited understanding.

If the only way to add this feature to the google_tags_tag_binding resource were to copy the existing generated code out to the third_party directory, and then add the functionality to look up the tag values by namespaced name, would that contribution be accepted? It's going the wrong direction (away from fully generated code) but is making the resource more user friendly.

Thanks for your time.

rileykarson commented 2 years ago

Is there an appropriate forum to ask question regarding provider contributions?

Generally just on the issues themselves- we don't monitor additional channels for those kinds of questions.

If the only way to add this feature to the google_tags_tag_binding resource were to copy the existing generated code out to the third_party directory, and then add the functionality to look up the tag values by namespaced name, would that contribution be accepted?

We'd be unlikely to accept it. The change should be possible although probably nonobvious in MM. FWIW our docs come from the API docs (https://cloud.google.com/resource-manager/reference/rest/v3/tagBindings), although gcloud hits the same API as we do- so it's likely the API is documented wrong and they manually improved the ones for gcloud.

nphilbrook commented 2 years ago

As noted above gcloud is just doing a lot of HTTP calls behind the scenes to find the tag value based on the namespacedName, so the HTTP API doesn't support it with one call.

I don't think this would be possible via MM but happy if I'm wrong.

Given this info I'll likely pursue a data source to look up a tag value by namespacedName.

nphilbrook commented 2 years ago

@rileykarson Still getting my wits about me regarding MM - is it a true statement that data sources are all hand-written and not generated? It seems that way

@arlo ~/github-external/terraform-provider-google/google $ grep -R -l "func dataSource" *go|xargs grep -l "AUTO GENERATED"
@arlo ~/github-external/terraform-provider-google/google $ 

Thanks for your help on this issue.

rileykarson commented 2 years ago

As noted https://github.com/hashicorp/terraform-provider-google/issues/11367#issuecomment-1096766211 gcloud is just doing a lot of HTTP calls behind the scenes to find the tag value based on the namespacedName, so the HTTP API doesn't support it with one call.

I missed that. Sorry!

is it a true statement that data sources are all hand-written and not generated? It seems that way

Yeah, they are all handwritten. There's a framework for resource-based ones that creates them based on their resource (by stubbing out some handwritten code), but get-only apis are just handwritten code.

nphilbrook commented 12 months ago

Note that the API now supports this - https://cloud.google.com/resource-manager/reference/rest/v3/tagBindings#TagBinding tagValueNamespacedName is now a field on TagBinding.

ggtisc commented 1 month ago

Today it is possible to achieve this following this terraform registry example.

Just need to maintain the prefix tagValues/ and for the ${google_tags_tag_value.value.name} since it is related to the google_tags_tag_value.short_name you could assign a value for it with namespacedName like this example:

resource "google_project" "project_11367" {
    project_id = "project-11367"
    name       = "project-11367"
    org_id     = "1234567890"
}

resource "google_tags_tag_key" "tag_key_11367" {
    parent = "organizations/1234567890"
    short_name = "tag-key-11367"
    description = "something"
}

resource "google_tags_tag_value" "tag_value_11367" {
    parent = "tagKeys/${google_tags_tag_key.tag_key_11367.name}"
    short_name = "name spaced Name"
    description = "something"
}

resource "google_tags_tag_binding" "tag_binding_11367" {
    parent = "//cloudresourcemanager.googleapis.com/projects/${google_project.project_11367.number}"
    tag_value = "tagValues/${google_tags_tag_value.tag_value_11367.name}"
}
roaks3 commented 4 weeks ago

This has a work-around with the datasources added, but since the API now supports the field directly, considering this request a coverage gap.

cherriford commented 3 weeks ago

The "name" workaround mentioned by @ggtisc above does not address the original request. The {google_tags_tag_value.value.name} attribute still outputs the numerically generated ID for the tag value, as noted in the Attributes Reference section of the Terraform Registry.

The original request is for the namespaced_name value to be supported in the tag bindings, which is still not supported. For example, if the namespaced_name were supported, I would expect the binding to look like the below for a key:value pair of env:prod:

resource "google_tags_tag_binding" "binding" {
    parent = "//cloudresourcemanager.googleapis.com/projects/23439259"
    tag_value = "123456789/env/prod"
}

We are also requesting this feature be supported so our disparate teams can attach tags at scale across thousands of projects. These teams are not privy to the numerically generated ID of the tag values, but they do know the key:value pairs.

nphilbrook commented 3 weeks ago

The "name" workaround mentioned by @ggtisc above does not address the original request.

Thank you @cherriford , very succinctly put and as the OP I agree. Looks like it's reopened and considered a gap now.