Add Config to the resource.ReadRequest

[tmatilai]

Module version

v1.4.1

Use-cases

Allow users to (optionally) specify a default value for a common argument in many resources and data sources.

I.e., the resources and data sources have optional and computed foo attribute. If not specified, the value from the provider attribute foo would be used. If even that is not specified, fail the run.

For example:

provider "my" {
  foo = "bar"
}

resource "my_example" "this" {
  # `foo` taken from the provider config
}

resource "my_example" "that" {
  # Explicitly set
  foo = "zap"
}

Attempted Solutions

I have thought and tried different solutions, but all have their issues. I might of course miss something here.

1. Defaults are applied before the resource is configured, so it doesn't have access to the provider configuration.
2. Attribute Plan Modifier doesn't have access to the resource and thus provider configuration.
3. Resource Plan Modifier has all the needed configuration, but if foo is configured in the provider and changes, updating the resource will lead to the Provider produced inconsistent result after apply error on other computed fields. In a single resource it would be fine to set all to unknown in the ModifyPlan(), but no idea how to iterate them in a generic way if the same function is used in many resources.
4. Setting the default in Create() works. But in Read() there is no access to the Config and the State has the old value, so if the value comes from the provider, any changes are not detected. Another downside is that in diffs the value is always (known after apply).
5. Using another attribute for the actual value sounds verbose, and not sure it would help in many of the previous cases.
6. With data sources Read() seems like the only option, but there it works fine.

Proposal

Add Config to the resource.ReadRequest struct to be accessible in the resource's Read() function.

Of course if there are other ways to implement this, nice. 🙂
Some way for setting all the computed, null configured attributes back to unknown at least would be fine.

[bflad]

Hi @tmatilai 👋 Thank you for raising this feature request.

The implementation reason that Config is not exposed in resource.ReadRequest is because this information is not exposed in the protocol between Terraform and providers:

https://github.com/hashicorp/terraform/blob/3420fdd5167c40c04aa79a8170885d5a0148f6ac/docs/plugin-protocol/tfplugin6.4.proto#L308-L321

The comments in the protocol definition mention some of the design reasoning for this:

This message intentionally does not include configuration data as any configuration-based or configuration-conditional changes should occur during the PlanResourceChange RPC. Additionally, the configuration is not guaranteed to be wholly known nor match the given prior state, which could lead to unexpected provider behaviors for practitioners.

Since that is a design problem that would need to be accepted as a design tradeoff (or somehow solved) upstream in Terraform, then implemented in the protocol, there is nothing that can be implemented in terraform-plugin-go or this Go module for the given proposal at the moment.

For the situation you mention though, I believe we may need some additional information before we can provide guidance. Is the optional and computed attribute returned from the API? If not, then this is not a situation that managed resource refresh (resource Read in framework terms) in Terraform was designed for -- it is designed to refresh Terraform's resource state from the "real world" resource state, generally from the remote system.

[tmatilai]

Hi Brian, thanks a lot for the detailed response! Even though I've been using Terraform for a very long time, unfortunately I've never studied the details of the internals. But learning bit by bit now. 🙂

I believe we may need some additional information before we can provide guidance.
Is the optional and computed attribute returned from the API?

Yeah sorry, I should have included more concrete example. I've seen this need already in a couple of places, but now it came with the TFE provider. Most of the resources include organization argument, which can also be configured in the provider. So the value is not coming from the API, but it's a required attribute for many API calls.

Almost all of the resources in the TFE provider still use the SDKv2, and now I came to this when trying to implement a new resource and data source using the plugin framework. And turns out that the existing implementation also suffers from the same issue.

Indeed Read was not my first place trying to implement this. But as said, all the other options had their challenges, too. 😅

Any suggestions highly appreciated. But I understand if the protocol specification doesn't allow perfect solution for this.

[papegaaij]

We also face an issue were we need access to the Config from the ReadRequest. Our API dynamically adds some attributes only when asked for. So, for example, we have this (where foo is one of these dynamic attributes):

resource "my_example" "res" {
  id = 42
  foo = "bar"
}

The provider will create this resource and explicitly ask the API to include foo in the response. If it wouldn't do that, the API will return the resource with the foo attribute set to null, causing a mismatch in the expected stated. When the Read method is called on the provider, it looks in the state to see if foo is set. If it is, it again asks the API to include it in the response.

The problems start when you use import to import the resource with id 42 into the state. The import has no way of knowing which attributes to include, so when Read is called, it only fetches the naked resource. Now, when you create a plan, it will notice a difference because foo is unset. Doing a refresh doesn't help either, because the Read method is still called with only the state it currently has, which does not include foo. For both invocations of Read (after import or at refresh), we need the Config to be able to determine the attributes we need to fetch.

At the moment, the only way out of this situation is to manually edit the state and set the required attributes to some (incorrect) non-null value and do a refresh. I really see no way to implement this correctly in the provider other than requiring the user to explicitly list the required attributes at several locations, which would result in a very poor experience.

[bflad]

Hi @tmatilai and @papegaaij 👋

If the resource implements the resource.ResourceWithConfigure interface, it can store any data and/or API clients set by the provider configuration. That resource Configure method is called before a resource ImportState or Read method is called, meaning you can store and reference any provider-level data. We would typically recommend things that can be set at either the provider-level or resource-level to be marked as optional and computed attributes, and always setting the value in state. The typical way to do this would be to treat that data as a default, then interrogate the resource configuration to override the value if its set there.

In the case where the API selectively returns information, my recommendation would be to set all those attributes to optional and computed, and always store/refresh all values. This will ensure that no matter whether the resource is being imported (no state) or remotely drifted (now incorrect state), Terraform will always be able to plan value differences from the given resource configuration.

If you have further questions about either of these scenarios, I would encourage posting a new topic in the Terraform Plugin Development section of HashiCorp Discuss, where there are more developers asking and answering provider development questions similar to these. 👍

[papegaaij]

Hi @bflad,

Access to the provider-level data is no good for our usecase. The required attributes can differ per resource.

Requesting all attributes will require massive permission sets for the Terraform provider for all resources provided by the API. Making it totally impossible to give fine grained permissions. Also, some of these attributes are very heavy to compute, leave audit trails when accessed and/or trigger webhooks. Accessing all attributes will result in very poor performance, massive audit trail flooding, triggering webhooks all over the place, and its very likely to trigger alerts in SIEMs. This is nowhere near acceptable for our API.

Also, Optional and Computed attributes cause major issues with drift if these attributes are not fixed on the external resource, and many or our attributes are dynamic (containing live computed data, statistics or changing data sets). Including all these attributes will result in plans that will never be stable. By selectively enabling only 1 or 2 attributes, the chances of drift are reduced a lot.

[bflad]

Hi again, @papegaaij. That additional context is helpful. The maintainers of this project do not know details of your particular API so we generally have to make broad assumptions using our experiences with other APIs and provider design in the industry.

I think, unfortunately, to be able to make real recommendations here, we would need to know even more details about your particular situation/API, the conceptual data structures and relationships of resources within your API, and how you are expecting practitioners to use Terraform to declaratively interact with the API. If there is public documentation for what you are working with, that would be beneficial for us to understand your situation and help better.

Offhandedly though (again without full context), this now sounds like you could be in a situation where the resource should be split up further for each "part"/"kind" of resource configuration that results in the differing data from the API. For example, the aws_kinesis_firehose_delivery_stream resource exists as it does today as a single resource because the underlying AWS API returns data for all configuration types, regardless of the one configuration type given. If the API did not do this, then we would probably see multiple resource types implemented, one per configuration type. Again, this is just offhanded advice with limited knowledge so it may be completely invalid for your situation, but I mention it since its a typical design pattern for this sort of API problem.

[papegaaij]

Hi @bflad ,

The provider I'm working on is for Topicus KeyHub, our IAM solution. You can find the (skeleton of) the documentation at https://registry.terraform.io/providers/topicuskeyhub/keyhub/latest/docs . As the provider is still under heavy development, the documentation is just what's produced by the generator. The provider is generated from our OpenAPI specification using a generator we developed ourselves. You can find the specification here https://github.com/topicuskeyhub/sdk-go/blob/main/openapi.json .

A factor that makes developing a terraform provider for our API more difficult is that many of our resources do not allow clients to update or delete, only create and read. This stems from our principle that changes to permissions should always be traceable to a person. Creating a new permission is fine, but updating it is not.

A simple example is creating a group. The provider can create a new group, but it cannot update it. Also, changes to memberships can only be done by an authorized manager of that group. The terraform provider can however create a new group with a predefined set of members. This is one of these optional attributes. When set, the provider will create the group with the configured accounts, and will verify the accounts of that groups by reading them. This does however require permissions. If you do not want the provider to manage the accounts of the group, you do not need to give the provider these permissions. The accounts must be specified as part of the resource, because the creation requires them all in a single bundle.

Given the situation that the provider manages the accounts for a specific group, the provider must fetch the accounts for that group when reading. However, when the user uses import to import the state for that group, the provider has no way of knowing whether it must fetch these accounts or not. At the moment, group has 3 of these types of attributes, but we will very likely be adding more.

Another category of attributes are computed (or gathered) statistics. These can change at every request and are therefore not very useful to use in resources, but can be useful for troubleshooting and output.

At the moment, I only see one viable solution for import: require the user to declare which attributes are required in the config (in addition to actually providing the contents of the writable attributes). We already have an attribute for this, called additional. Also, we will need to require the user provide this very same list during an import. Import can then write this list into the state for additional, in addition to the identifiers. This can then be used by read to fetch all required data. This will require the user to specify the same list at several places. Also, there is no way for the provider to verify the correctness of the list. Giving too few or too many values will very likely result in unfixable state mismatches.