Immutable fields should reject changes

[muvaf]

What problem are you facing?

Terraform has ForceNew attribute on fields that represents the immutability but we don't account it at the moment. Changing those fields causes Terraform errors that don't really seem to be related since TF tries to re-create the resource for changes in those fields.

How could Terrajet help solve your problem?

We can invest in https://github.com/crossplane/crossplane-tools/issues/40 and then add that marker to the fields, similar to how we generate reference resolvers. Though it's also worth investigating whether Common Expression Language (CEL) stuff merged in upstream would help here. Though that may require kubebuilder changes since it requires changes on the CRD manifests.

[luebken]

Re CEL and Validation Rules: I found this upcoming blogpost informative.

[muvaf]

Another one specifically about immutability. We might be able to get away with a kubebuilder marker but we need to be sure about the semantics. Specifically, nil values should not be considered immutable till a value is assigned. https://github.com/kubernetes/website/pull/36032

[ulucinar]

Thanks @muvaf. To have cross references with some existing issues and for further discussion:

• https://github.com/crossplane/terrajet/issues/264
• https://github.com/crossplane/terrajet/issues/158#issuecomment-1205292185

[muvaf]

Adding the following is now supported in kubebuilder. Should be straight-forward to add this to our code gen pipeline.

// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="Value is immutable"

https://kubernetes.io/blog/2022/09/29/enforce-immutability-using-cel/

[Hronom]

Hello, we need what described here https://github.com/crossplane/terrajet/issues/158#issuecomment-1205292185 basically lifecycle.prevent_destroy

[jrote1]

Hi, is there any update on this?

[jeanduplessis]

@jrote1 there are no plans to invest in this right now. The Upbound team is focused on improving the performance at the moment and won't be able to look at this in the near term. If someone is interested in picking this up, we would happily support them.

For posterity, here's a high-level overview of the situation:

Terraform marks certain configuration arguments in its schema as ForceNew. In that case, if the argument's value in the Terraform configuration differs from what's available in the Terraform state, the plan generator generates a plan in which the existing external resource is deleted and a new one with the desired argument value is provisioned, and hence forces a new resource to satisfy the new desired state.

Crossplane providers, by convention (referring to the Crossplane Resource Model), never delete external resources unless the corresponding MR is deleted under an appropriate management policy. So in the Terraform configurations that we generate while reconciling the managed resources, we unconditionally include the prevent-destroy lifecycle meta-argument to prevent Terraform from destroying the existing external resource and provisioning a new one.

However, the API server does not know anything about those parameters with the underlying ForceNew indication in the corresponding resource and thus accepts modifications to those spec.forProvider parameters. This results in the Terraform layer complaining with a cryptic `a change to this field has been requested but I cannot do it because of the prevent-destroy lifecycle meta-argument) message.

A possible solution is to add some validation rules to the (generated) OpenAPI v3 schema of the CRDs that will ask the API server to reject changes to such fields if they have already been set.

We can also consider improving the error reported in status conditions (and elsewhere) by explaining that the field should be treated as an immutable one (by parsing the error message from the Terraform layer).