crossplane / upjet

A code generation framework and runtime for Crossplane providers
Apache License 2.0
297 stars 86 forks source link

Observe policy is less useful than TF data resources #283

Open jond3k opened 11 months ago

jond3k commented 11 months ago

What happened?

I'd like to get the equivalent behaviour to data_google_tags_tag_key

The crossplane way to simulate this behaviour is to set the resource as observe-only and use external-name.

However, this isn't semantically the same. Instead it's converted into terraform import resource ${external_name} by upjet.

The problem with this is that imports often aren't as good at resolving resources as data resources. In this case, the data resource will resolve a namespacedName like ${org_id}/${short_name} but import will only resolve a name like tagKeys/${id} but this is much less portable as the number changes all the time.

Perhaps it would be better to make external-name optional for observe-only and use the data resources instead of import if they're available?

How can we reproduce it?

Using a resource which has multiple forms of ID and no support for these IDs in terrafrom import, like google_tags_tag_key we need an equivalent to:

data "google_tags_tag_key" "environment_tag_key"{
  parent = "organizations/567890"
  short_name = "myTagName"
}

This fails as namespacedName isn't supported for import by Terraform:

apiVersion: tags.gcp.upbound.io/v1beta1
kind: TagKey
metadata:
  name: with-namespaced-name
  annotations:
    crossplane.io/external-name: 567890/myTagName
spec:
  managementPolicies: [Observe]
  forProvider: {}

This succeeds because name is supported:

apiVersion: tags.gcp.upbound.io/v1beta1
kind: TagKey
metadata:
  name: with-name
  annotations:
    crossplane.io/external-name: tagKeys/123456
spec:
  managementPolicies: [Observe]
  forProvider: {}

Alternatively, it would be nice if this would work but it currently fails as observe-only only seems to work with an external-name:

apiVersion: tags.gcp.upbound.io/v1beta1
kind: TagKey
metadata:
  name: with-no-name
spec:
  managementPolicies: [Observe]
  forProvider:
    parent: organizations/567890
    short_name: myTagName
turkenh commented 11 months ago

@jond3k thanks a lot for the feedback and well-written issue report 🙌

I agree that the Observe Only resources are not as useful as Terraform data sources primarily because they do not support/enable querying and filtering, which is being tracked by a dedicated issue: https://github.com/crossplane/crossplane/issues/4141

You can read more details on how we landed on the current implementation from the description of the above issue or from the design document , but primary reason could be quoted as below:

We will not support querying and filtering at managed resources level since it violates a fundamental principle with the managed resources, that is, having a one-to-one relationship between a managed resource and the external resource that it represents. When it comes to querying and filtering, it is possible that:

  • There are more than one matching resources
  • There are no matching resources
  • The matching resource may change in time, e.g., most-recent AMI

Hence, we will leave implementing this functionality in an upper layer, which in turn will own managed resources with managementPolicy: ObserveOnly. In this model, it is totally fine if matching resources change in time, including having more than one or no matches where we would expect corresponding managed resources to come and go at runtime.

So, at the Managed Resource level, we need to uniquely identify the external resource, which is why we need a properly set crossplane.io/external-name annotation. Your specific example is a little special since you still want to provide enough details to identify the external resource uniquely; however, this is not usually the case for common usage of Terraform data sources.

jond3k commented 11 months ago

Thanks @turkenh,

Is there a way to make an exception for this resource?

I've opened an issue with the TF provider to add the missing import syntax. This should make external-name work, right? https://github.com/hashicorp/terraform-provider-google/issues/16075

turkenh commented 11 months ago

Is there a way to make an exception for this resource?

I cannot think of an easy way to do this.

I've opened an issue with the TF provider to add the missing import syntax. This should make external-name work, right? hashicorp/terraform-provider-google#16075

Yes, it should.