fix(tfjson): Reset Computed flag for SchemaNestingModeSingle blocks with required children - #593
fix(tfjson): Reset Computed flag for SchemaNestingModeSingle blocks with required children#593rwwiv wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughtfJSONBlockTypeToV2Schema now applies SDK v2 semantics: collection modes (Set/List/Map) infer Computed when MinItems==0 && MaxItems==0; Single/Group blocks are represented as object-like with MinItems=0, MaxItems=1, Required/Optional derived from hasRequiredChild instead of inferring Computed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Thanks — quick question: should downstream consumers expect Single/Group to be treated strictly as object-schema (not list) for any subsequent processing? 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/types/conversion/tfjson/tfjson.go (1)
170-179: Add regression test for required-child Single blocks.The behavior introduced around Line 174 should be locked in with a unit test to prevent regressions (Computed must be false when a Single block has required children).
🧪 Suggested test (standard Go testing)
+// SPDX-FileCopyrightText: 2023 The Crossplane Authors <https://crossplane.io> +// +// SPDX-License-Identifier: Apache-2.0 + +package tfjson + +import ( + "testing" + + tfjsonsdk "github.com/hashicorp/terraform-json" + "github.com/zclconf/go-cty/cty" +) + +func TestTFJSONBlockTypeToV2Schema_SingleRequiredChildResetsComputed(t *testing.T) { + nb := &tfjsonsdk.SchemaBlockType{ + NestingMode: tfjsonsdk.SchemaNestingModeSingle, + MinItems: 0, + MaxItems: 0, + Block: &tfjsonsdk.SchemaBlock{ + Attributes: map[string]*tfjsonsdk.SchemaAttribute{ + "name": {Required: true, AttributeType: cty.String}, + }, + }, + } + + got := tfJSONBlockTypeToV2Schema(nb) + + if got.Computed { + t.Fatalf("expected Computed=false for required-child single block") + } + if !got.Required || got.Optional { + t.Fatalf("expected Required=true and Optional=false") + } + if got.MinItems != 1 || got.MaxItems != 1 { + t.Fatalf("expected MinItems/MaxItems=1, got %d/%d", got.MinItems, got.MaxItems) + } +} +``` </details> As per coding guidelines, "All Upjet code must be covered by tests; do not use Ginkgo or third-party testing libraries, use only standard Go testing". </blockquote></details> </blockquote></details>
ea49ff6 to
5bea688
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/types/conversion/tfjson/tfjson.go (1)
176-191:⚠️ Potential issue | 🟠 MajorThe heuristic
Required = hasRequiredChild(nb)is incorrect for Plugin Framework semantics and will over-constrain the CRD.In Terraform Plugin Framework,
SingleNestedBlockandSingleNestedAttribute(Group) have no built-inRequiredflag on the block itself—blocks are optional by default. Required child attributes do not imply the block is required; they only apply when the block is present. To make a block required in Plugin Framework, you must use validators (e.g.,objectvalidator.IsRequired()).The current code marks a block as
Requiredwhenever it contains required children, which incorrectly forces optional blocks to be required in the generated schema. This breaks configurations where the block is legitimately optional.Fix: Determine block Required/Optional status from the tfjson schema itself (if available), not from child attribute requirements. If tfjson does not encode block-level Required, consider whether the absence of a
Requiredindicator in the Plugin Framework schema should default toOptional = true.
This PR temporarily replaces upjet with the version from this PR: crossplane/upjet#593 to generate these resources properly.
This PR temporarily replaces upjet with the version from this PR: crossplane/upjet#593 to generate these resources properly.
Replace upstream upjet with fork that fixes two issues for Plugin Framework resources using SchemaNestingModeSingle blocks: 1. Blocks were incorrectly marked as Computed=true, causing them to be excluded from ForProvider/InitProvider and only appear in Observation. 2. Blocks were using TypeList instead of SchemaTypeObject, causing the generated CRD schema to expect arrays instead of objects. This fix is required for Plugin Framework resources like alertrule and recordingrule where metadata and spec blocks need to be objects, not arrays. Upstream PR: crossplane/upjet#593
72c6d00 to
fb6b3fa
Compare
Replace upstream upjet with fork that fixes two issues for Plugin Framework resources using SchemaNestingModeSingle blocks: 1. Blocks were incorrectly marked as Computed=true, causing them to be excluded from ForProvider/InitProvider and only appear in Observation. 2. Blocks were using TypeList instead of SchemaTypeObject, causing the generated CRD schema to expect arrays instead of objects. This fix is required for Plugin Framework resources like alertrule and recordingrule where metadata and spec blocks need to be objects, not arrays. Upstream PR: crossplane/upjet#593
Replace upstream upjet with fork that fixes two issues for Plugin Framework resources using SchemaNestingModeSingle blocks: 1. Blocks were incorrectly marked as Computed=true, causing them to be excluded from ForProvider/InitProvider and only appear in Observation. 2. Blocks were using TypeList instead of SchemaTypeObject, causing the generated CRD schema to expect arrays instead of objects. This fix is required for Plugin Framework resources like alertrule and recordingrule where metadata and spec blocks need to be objects, not arrays. Upstream PR: crossplane/upjet#593
* fix: use upjet fork with SchemaNestingModeSingle object fix Replace upstream upjet with fork that fixes two issues for Plugin Framework resources using SchemaNestingModeSingle blocks: 1. Blocks were incorrectly marked as Computed=true, causing them to be excluded from ForProvider/InitProvider and only appear in Observation. 2. Blocks were using TypeList instead of SchemaTypeObject, causing the generated CRD schema to expect arrays instead of objects. This fix is required for Plugin Framework resources like alertrule and recordingrule where metadata and spec blocks need to be objects, not arrays. Upstream PR: crossplane/upjet#593 * update generated files * fix: update upjet to use grafana/upjet fork with fix-computed-single-blocks branch --------- Co-authored-by: Moustafa Baiou <moustafa.baiou@grafana.com> Co-authored-by: Duologic <jeroen@simplistic.be>
|
|
1 similar comment
|
|
erhancagirici
left a comment
erhancagirici
left a comment
There was a problem hiding this comment.
@rwwiv thanks for reporting and the PR on this. I've investigated this and below are my findings and notes:
Per this doc comment https://github.com/hashicorp/terraform-plugin-framework/blob/a0219204842978493e5f7742b0c06d5c39951e73/internal/fwschema/block.go#L24 looks like we never get Min/MaxItems data for blocks at the corresponding TF Core schema for plugin-framework resource schemas. This is true for all block nesting types .
As a result of this, today, the inference/heuristic always ends up with Optional: true Computed: true Required: false for all plugin-FW blocks.
In fact, the TF core block fields have no direct notion of "required,optional,computed"ness.
This currently works out SchemaNestingModeSet SchemaNestingModeList SchemaNestingModeMap for fw resources, they always get a spec.forProvider field in the CRD, with the implication that they are always optional. Which is fine
The actual bug resides in the SchemaNestingModeSingle here, please see my comment: https://github.com/crossplane/upjet/pull/593/changes#r3871860815.
on the broader situation: please see https://github.com/crossplane/upjet/pull/593/changes#r3871736101
I'll do a final review after giving it some testing with the existing providers.
There was a problem hiding this comment.
The actual bug is here:
As I mentioned in the main review comment, we initially start with
Required: false, Optional: true, Computed: true for all blocks in plugin-fw resources.
If we determine required: true, we switch Optional: False but computed remains true. i.e. we end up with
Required: true, Optional: false, Computed: true
This is actually an invalid schema configuration. A required field cannot be computed.
However, this ends up in generation pipeline and IsObservation() check treats this as an computed-only field (because computed=true, optional=false)
In summary, adding the following aligns it with the rest of the block types and starts generating those.
| v2sch.Computed = false |
However, please also see comment for the broader situation on this and a proposed change: https://github.com/crossplane/upjet/pull/593/changes#r3871736101
| // TODO(erhan): not sure whether we need this | ||
| // the block itself can be optional, even if some child attribute | ||
| // or block is required | ||
| v2sch.Required = hasRequiredChild(nb) |
There was a problem hiding this comment.
following up my previous todo:
The TF Core schema has no notion of required/optional/computed for a "block" field. Only Min/MaxItems. And they are (intentionally) not available for plugin-fw resources.
So, we have no way of inferring whether a block is "Observation"-only or intended to be configurable. plugin-fw doc comment mentions limited validation capabilities at config time, and says to offload this validation to the provider side.
Additionally, checking the TF SDKv2 code here -> here, sdk resources never end up with NestingSingle block in their core schema, the only exception being the timeout block, which upjet skips anyway.
So, I think it is safe to assume this path is only visited by plugin-fw resource blocks.
Lastly, this converted "v2schema" is only for CRD generation purposes and not utilized at runtime. It acts like an Intermediate representation for seeding CRD generation. e.g. the computed/optional etc.
Considering all of the above, I think hasRequiredChild is not a proper heuristic for requiredness. A required child attribute not necessarily mean that the block itself is required.
I propose to treat all NestingSingle blocks as optional for CRD generation (so that it always generates a nullable forProvider.myFooField ) and let the runtime validate it.
Also, this won't be a breaking change since this path was never generating a required forProvider field anyway, and optionals will just stay the same.
Also, upjet has already config machinery available for explicitly marking the field observe-only or required, so devs can modify according to their needs if they need stricter CRD API validation on this.
TLDR, my suggestion is to have:
case tfjson.SchemaNestingModeSingle, tfjson.SchemaNestingModeGroup:
v2sch.Type = SchemaTypeObject
v2sch.Required = false
v2sch.Optional = true
v2sch.Computed = false| // matching the behavior for nested attributes with SchemaNestingModeSingle | ||
| // and the Terraform Plugin Framework documentation which states that | ||
| // SingleNestedBlock values are represented by an object type. | ||
| v2sch.Type = SchemaTypeObject |
There was a problem hiding this comment.
note: SchemaTypeObject is actually a workaround in upjet to represent plugin-fw schemas in sdkv2 schema structs. This actually leads in an invalid sdkv2 schema struct, and calling member functions like CoreConfigSchema() on the resulting TerraformResource causes a panic.
Though for plugin-framework resources, this object is CRD-generation purposes only and never utilized at runtime, and this path is framework-only ( as mentioned in https://github.com/crossplane/upjet/pull/593/changes#r3871736101 ), so I think this is acceptable.
| } | ||
| if nb.MinItems == 0 && nb.MaxItems == 0 { | ||
| v2sch.Computed = true | ||
| } |
There was a problem hiding this comment.
as mentioned in other comments, let's put this back and fix the NestingSingle branch only.
| // For collection types (Set/List/Map), infer Computed when MinItems and | ||
| // MaxItems are both 0, following SDK v2 semantics. | ||
| if nb.MinItems == 0 && nb.MaxItems == 0 { | ||
| v2sch.Computed = true | ||
| } |
There was a problem hiding this comment.
| // For collection types (Set/List/Map), infer Computed when MinItems and | ||
| // MaxItems are both 0, following SDK v2 semantics. | ||
| if nb.MinItems == 0 && nb.MaxItems == 0 { | ||
| v2sch.Computed = true | ||
| } |
There was a problem hiding this comment.
| // For collection types (Set/List/Map), infer Computed when MinItems and | ||
| // MaxItems are both 0, following SDK v2 semantics. | ||
| if nb.MinItems == 0 && nb.MaxItems == 0 { | ||
| v2sch.Computed = true | ||
| } |
There was a problem hiding this comment.
f132611 to
b8befe5
Compare
When converting tfjson block types to SDK v2 schema, the Computed flag was being inferred as true when MinItems==0 && MaxItems==0. This heuristic works for SDK v2 collection types (List/Set/Map) but is incorrect for Plugin Framework resources using SchemaNestingModeSingle. Plugin Framework resources default MinItems/MaxItems to 0 for single blocks even when they contain user-configurable attributes. This caused blocks like 'metadata' and 'spec' to be incorrectly marked as Computed, excluding them from ForProvider/InitProvider parameters and only including them in Observation. This fix makes SchemaNestingModeSingle blocks always `Optional` and not `Computed`, so that they always generate a configurable spec field. `SchemaNestingModeSingle` is only present in Plugin Framework resource schemas, and FW resources never set `Min/MaxItems`. The heuristic does not make sense here. This allows Plugin Framework resources (e.g., Grafana App Platform resources like AlertruleV0Alpha1) to properly expose their nested blocks in the CRD's forProvider schema. Also adds comprehensive unit tests for tfJSONBlockTypeToV2Schema and hasRequiredChild functions. Signed-off-by: Will Wernert <william.wernert@grafana.com>
b8befe5 to
5bd3d80
Compare
3 participants
Summary
Fixes two issues where
SchemaNestingModeSingleblocks from Plugin Framework resources were incorrectly handled:Computed inference bug: Blocks were incorrectly marked as
Computed=true, causing them to be excluded fromForProvider/InitProviderparameters and only appear inObservation.Type representation bug: Blocks were using
TypeListinstead ofSchemaTypeObject, causing the generated CRD schema to expect arrays instead of objects.Problem
In
tfJSONBlockTypeToV2Schema(), whenMinItems==0 && MaxItems==0, the block was being marked asComputed=true:This heuristic works for SDK v2 collection types (List/Set/Map) where
MinItems=0, MaxItems=0typically indicates a computed-only collection. However, Plugin Framework resources usingSchemaNestingModeSingledefault toMinItems=0, MaxItems=0even for user-configurable blocks.Additionally,
SchemaNestingModeSingleblocks were usingschemav2.TypeList, but according to Terraform Plugin Framework documentation, SingleNestedBlock values should be represented by an object type, not a list.This caused blocks like
metadataandspecto be:Computed=true, which makesIsObservation()returntrue, excluding them from the generated Parameters structSolution
Move the
Computed=trueinference to only apply to collection nesting modes (Set/List/Map), not toSchemaNestingModeSingle. For single blocks, we now rely solely onhasRequiredChild()to determine Required/Optional, leaving Computed=false by default.Use
SchemaTypeObjectinstead ofTypeListforSchemaNestingModeSingleblocks, matching the behavior for nested attributes withSchemaNestingModeSingleintfJSONNestedAttributeTypeToV2Schema().Testing
Added comprehensive unit tests for
tfJSONBlockTypeToV2SchemaandhasRequiredChild:SchemaNestingModeSinglewith required children →Computed=false,Required=true,Type=SchemaTypeObjectSchemaNestingModeSinglewith only optional children →Computed=false,Optional=true,Type=SchemaTypeObjectSchemaNestingModeSinglewith empty/nil blockSchemaNestingModeSinglewith nested blocks containing required childrenSchemaNestingModeList/Set/MapwithMinItems=0, MaxItems=0→Computed=true(preserves existing behavior)SchemaNestingModeListwithMinItems=1→Computed=falseImpact
This fix is particularly important for Terraform Plugin Framework resources. For example, the Grafana provider's App Platform resources (
grafana_apps_rules_alertrule_v0alpha1,grafana_apps_rules_recordingrule_v0alpha1, etc.) havemetadataandspecblocks that were incorrectly excluded fromforProviderin the generated CRDs, and were being generated as arrays instead of objects.Before this fix:
After this fix:
SDK v2-based resources are unaffected because they typically use
MinItems=1, MaxItems=1for required single blocks, so they never hit theMinItems == 0 && MaxItems == 0path.