Add config.Resource.OverrideScalarFieldType

[ulucinar]

Description of your changes

This PR adds the config.Resource.OverrideScalarFieldType configuration method that allows the generated type of a scalar CRD field at a given Terraform path to be overridden. An example invocation is:

func Configure(p *config.Provider) {
	p.AddResourceConfigurator("azuread_application", func(r *config.Resource) {
		r.OverrideScalarFieldType("prevent_duplicate_names", types.NewStringOrBoolType())
                 ...

The Terraform field path should omit any wildcards, e.g., x.y even x is a collection type.

Breaking CRD scalar field type changes in upjet-based providers, such as bool->string or int->string are common, when the underlying Terraform provider makes changes in the Terraform resource's schema. For instance, in crossplane-contrib/provider-upjet-aws:v1.19.0, the Terraform provider changed the type of the at_rest_encryption_enabled field from a bool to a nullable bool (which is a string type). The provider did not publish a new CRD API version with the necessary API converters and the Terraform runtime converters (probably because that introduces some extra complexity) and instead documented the breaking changes in its release notes.

It's also a common practice not to introduce new CRD API versions with the major version bumps of the providers when the API versioning policies allow such breaking changes.

However, when the CR field is stored in etcd as a different type than what the CRD Go struct type now declares in a new version of the provider, the UX is seriously broken. The provider simply fails to start its informer cache as it cannot decode the object it reads from etcd, fails with something like:

json: cannot unmarshal bool into Go struct field ReplicationGroupParameters.items.spec.forProvider.atRestEncryptionEnabled

This PR introduces the internal generic StringOrPrimitive type and the public StringOrBool type so that provider authors can better handle such breaking changes with a much better and expected UX. When the provider configures (either through manual means, or via the means of the auto-registration framework) a type override for the affected field, the controller-runtime then becomes able to properly decode the (now) broken object into its new memory representation and no fatal errors will be observed. Clients of the affected CRD API will though still need to be migrated to use the new type, this will only prevent a crash loop of the provider.

The existing API clients will fail as expected with the proper API validation errors when they try to use the old field type, until they are migrated to use the new type for the affected field.

Note: The current implementation only supports overriding the generated types for scalar fields; collections and composites are currently not supported. It's a build time error to specify a Terraform path that points to a collection or composite when calling OverrideScalarFieldType.

In the future, we may consider adding support for other primitive type overrides and/or adding support for type overrides of collections/composites. Supporting overrides for non-scalars is more complex to handle in upjet's code generation pipelines and is not needed as of now.

I have:

• Read and followed Upjet's contribution process.
• Run make reviewable to ensure this PR is ready for review.
  - [ ] Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

Tested against crossplane-contrib/provider-upjet-azure. The runtime behavior of the provider has been tested successfully for an update with a breaking scalar field type change (bool -> string as discussed above). The provider no longer crashes as intended.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 8e216458-b8d4-4685-92a1-10d520f6868e

📥 Commits

Reviewing files that changed from the base of the PR and between e4975c5 and 2084422.

📒 Files selected for processing (1)

• pkg/types/builder_test.go

🚧 Files skipped from review as they are similar to previous changes (1)

• pkg/types/builder_test.go

---

📝 Walkthrough

Walkthrough

The change adds scalar Terraform field type overrides to resource configuration and schema generation. It also adds StringOrBool and generic string-backed primitive JSON conversion with comprehensive tests.

Changes

Scalar Type Support

Layer / File(s)	Summary
Resource scalar override configuration pkg/config/common.go, pkg/config/resource.go, pkg/config/resource_test.go	Resource stores Terraform field type overrides and provides registration and lookup methods. Tests cover configured, mismatched, absent, and nil overrides.
Schema generation override handling pkg/types/builder.go, pkg/types/builder_test.go	buildSchema applies overrides to scalar fields in generated APIs and reports errors for non-scalar fields.
String-backed primitive JSON types pkg/types/internal/string.go, pkg/types/internal/string_test.go, pkg/types/string.go, pkg/types/string_test.go	Adds generic string-backed primitive conversion and the StringOrBool type. Tests cover JSON decoding, encoding, round trips, errors, and Go type construction.

Estimated code review effort: 3 (Moderate) | ~30 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Resource
  participant buildSchema
  participant GeneratedAPI
  participant StringOrBool
  Resource->>buildSchema: provide scalar field type override
  buildSchema->>GeneratedAPI: apply overridden parameter type
  GeneratedAPI->>StringOrBool: encode or decode string-backed boolean
  StringOrBool-->>GeneratedAPI: return canonical string value

Loading

Suggested reviewers: erhancagirici, sergenyalcin

🚥 Pre-merge checks | ✅ 7

✅ Passed checks (7 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title is 43 characters, stays under 72 characters, and clearly names the main configuration method added by the pull request.
Description check	✅ Passed	The description directly explains the scalar field type override feature, its purpose, limitations, example usage, and testing.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Configuration Api Breaking Changes	✅ Passed	The PR adds only a private Resource field and new override APIs in pkg/config; the range has 113 insertions and no deletions or existing signature changes.
Generated Code Manual Edits	✅ Passed	The complete PR diff from origin/main changes nine non-generated paths and contains no files matching the zz_*.go pattern.
Template Breaking Changes	✅ Passed	The full PR range changes only config and types files; no pkg/controller/external*.go template files changed.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@pkg/types/builder.go`:
- Around line 227-229: The OverrideScalarFieldType error in the builder
validation must include an actionable correction: state that the override should
target a scalar child field, with an example such as “settings.size,” while
preserving the reported Terraform type and field path.

In `@pkg/types/internal/string.go`:
- Around line 20-24: Extend the Primitive constraint in
pkg/types/internal/string.go:20-24 with ~float32 and ~float64 so
StringOrPrimitive supports documented floating-point values. Add float32,
float64, and named floating-point cases to the tests in
pkg/types/internal/string_test.go:92-129; both sites require changes.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 4b401ea0-d40a-4676-8f92-9db431cfb7cc

📥 Commits

Reviewing files that changed from the base of the PR and between 324a879 and f90b7b7.

📒 Files selected for processing (9)

• pkg/config/common.go
• pkg/config/resource.go
• pkg/config/resource_test.go
• pkg/types/builder.go
• pkg/types/builder_test.go
• pkg/types/internal/string.go
• pkg/types/internal/string_test.go
• pkg/types/string.go
• pkg/types/string_test.go

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[sergenyalcin]

Thanks @ulucinar for this PR. I've left a few comments for discussion.

[sergenyalcin]

One thing that came to mind: should we validate that path actually exists in the Terraform schema? Right now a typo would silently become a no-op. It may never match anything during code generation. If we intentionally want to keep this like that, it might at least be worth documenting that invalid paths fail. What do you think?

[ulucinar]

One thing that came to mind: should we validate that path actually exists in the Terraform schema?

I definitely agree we should do that. I considered implementing a validation in addition to the scalar type check we already have but the issue is the available canonical paths are not readily available to us. This is also the reason we don't have such checks for other similar configuration options like config.Resource.AddSingletonListConversion and others. I was thinking we could try to address this cross-cutting validation concern in a future PR, because this already surfaced in this PR's discussions, let me check what we can do about it...

[sergenyalcin]

One thing I thought about is exposing FieldTypeOverrideConfiguration as part of the public API. The The doc of the FieldTypeOverride field is says that it is only to be used by the code generator, but the returned type is exported. With this state, I am not sure this is just a internal plumbing point. It seems a general use API. What do you think?

I didn't think the alternative for this but still wanted to discuss do we really want to export them for general use instead of really generation internal?

[ulucinar]

I was initially thinking exposing the config.Resource.overrideGeneratedFieldType (as config.Resource.OverrideGeneratedFieldType) so that we would not need a separate config.Resource.OverrideScalarFieldType configuration method. However, in the future, we may need to support overriding types of non-scalar (complex/collection) fields and if we simply expose overrideGeneratedFieldType as our configuration API, it also directly exposes upjet code generation pipeline's implementation details.

Currently, the only "intended" and exposed configuration API is the config.Resource.OverrideScalarFieldType, which is what a provider needs to specify to be able to override the generated type for a scalar field, the field's path and the type to be used for the override. Supporting overrides for complex types or collection types in more involved because such fields can differ across the forProvider, initProvider and atProvider API trees. For scalars, they always have the same exact type (and that's the reason the OverrideScalarFieldType method accepts a single type parameter, not separate types for the 3 API trees we have). If we had exposed overrideGeneratedFieldType directly, it would not be future proof but the OverrideScalarFieldType is future proof because it's not a generic override API, i.e., it's not expected to handle non-scalar field type overrides. Also having the configured performed via OverrideScalarFieldType (instead of directly setting the configuration data structures) would allow us to do some validation in the future.

The cost of hiding the implementation detail from the provider authors is then we need to expose that configuration via an accessor because the code generation pipeline that consumes this configuration lives in the package pkg/types. Go does not have something like a "friend package" concept and an internal package won't help because overrideGeneratedFieldType is not exported. FieldTypeOverrideConfiguration and FieldTypeOverride should be of no use to the provider authors but we cannot also hide them from the provider authors. That's why I just added a Go doc that tells FieldTypeOverride is meant to be used by the code generator.

[sergenyalcin]

LGTM @ulucinar