Remove immutable fields from initProvider

[mbbush]

What problem are you facing?

In provider-upjet-aws, there are many resources which use the config.IdentifierFromProvider external name config, which doesn't set any identifier fields. However, many of these resources do have fields which serve as an identifier, in that they are part of the terraform id. It would provide a better user experience to update the external name configuration to construct the terraform id from the parameters, so that users can specify the desired spec and not have to worry about whether crossplane is going to create or update it, as opposed to the current behavior where often (but not always) users have to set the external-name annotation to the right format for that resource Kind, which is generally not documented anywhere.

When opening PRs that make these sorts of improvements to an external name configuration, the result is technically a breaking schema change, because it removes fields from spec.initProvider, and there's (understandably!) resistance to releasing such changes in a normal minor release.

While the terraform schema doesn't provide any specific indication that certain fields are part of the terraform id, it does indicate immutable fields with ForceNew: true (at least in the TF SDKv2 form of the schema), and identifier fields are almost always immutable.

How could Upjet help solve your problem?

If we removed immutable fields from initProvider (based on ForceNew: true), then we would substantially reduce the size of our schemas, and allow external name config improvements to be non-breaking, at the cost of removing a feature it makes no sense to use.

In provider-aws, the result of applying this change locally was a decrease of about 60,000 lines of generated code in the CRDs, and another 40,000 lines in the apis folder.

How can we identify an immutable field?

For schemas defined using the TF SDKv2, there's a ForceNew parameter.

For schemas deserialized to the TF SDKv2 format from terraform-json, as far as I can tell there is no indication of which fields are immutable.

For Terraform Plugin Framework schemas, there is a RequiresReplace plan modifier. See https://github.com/hashicorp/terraform-provider-aws/blob/898c9b5a1d8958366b293dad02daa44e24e360ef/internal/service/cognitoidp/user_pool_client.go#L237-L241 for an example.

Breaking change

This would be a breaking change. I can think of two ways to release it.

1. Because it would be very strange for someone to be setting an immutable field in spec.initProvider instead of spec.forProvider, we just make the change, bump the major version of the provider (and probably also upjet), and mention it in the release notes.
2. Write some kind of generic conversion webhook (or code to generate one?) that migrates fields present in v1beta1.initProvider and missing from v1beta2.initProvider by setting the corresponding value in v1beta2.forProvider I'd really prefer our conversion webhooks to be a bit more mature before we add one to nearly every resource, with issues like Code generation error: Import cycle not allowed #368 and Stability improvements to conversion webhooks #369 addressed.

[mbbush]

Consensus from discussion at the Upjet SIG meeting was to consider this in tandem with larger changes to the initProvider api, based on findings from @mergenci when implementing the "surgery" code that handles editing the terraform diffs. One option to consider is changing to a differential api, more like terraform's ignore_changes.

[negz]

Because it would be very strange for someone to be setting an immutable field in spec.initProvider instead of spec.forProvider

Can you help me understand why this would be very strange?

To me, it's almost more intuitive to use initProvider for immutable fields. It communicates that the field will be set once, then won't change on subsequent updates.

[mbbush]

Cem and Alper were confused about this too. I explained at the SIG meeting.

Basically my mental model as a user of crossplane is that spec.forProvider is the primary way to specify the desired state. If I can do everything I need with that, there's no need to use spec.initProvider at all. What spec.initProvider is used for is basically the same idea as terraform's ignore_changes block, and there would be no value in specifying that changes to a field that cannot change should be ignored.

It sounds like this is not quite the same mental model of this feature that some of the crossplane developers have, which is a valid point of view that I just wasn't aware of. It seems like a "typical" crossplane user could potentially be thinking about the feature either way.

My recollection/restating of what @mergenci said was that the actual implementation of this feature in upjet works by always merging spec.forProvider with spec.initProvider, producing a diff, and then (for updates) surgically altering that diff if it determines that the change is a result of the difference between initProvider and forProvider. This is more or less the same way that the ignore_changes block is implemented in the TF cli. However, it is sometimes difficult and sometimes actually impossible to know how to do this correctly, and there were several edge cases he discovered. This reopens the question of whether we'd be better off with a differential api, such as one of the alternatives from the original proposal, perhaps with some tweaks.

@mergenci @ulucinar Does my summary of our conversation look right to you?

[negz]

It sounds like this is not quite the same mental model of this feature that some of the crossplane developers have, which is a valid point of view that I just wasn't aware of. It seems like a "typical" crossplane user could potentially be thinking about the feature either way.

I wonder if this boils down to whether or not you're starting from Terraform's ignore_changes concept/API and deriving how Crossplane's API should work from that.

FWIW, I was the one who pushed for initProvider rather than a mask on forProvider like ignore_changes. If we're going to discuss unwinding this decision I'd like to be involved.