Merge pull request #2075 from maqiuyujoyce/202406-2051-test

Workaround to hide output-only spec field but support it in observed state

config/crds/resources/apiextensions.k8s.io_v1_customresourcedefinition_storagebuckets.storage.cnrm.cloud.google.com.yaml

@@ -293,11 +293,6 @@ spec:
                   permanently deleted. If it is not provided, by default Google Cloud
                   Storage sets this to default soft delete policy.
                 properties:
-                  effectiveTime:
-                    description: Server-determined value that indicates the time from
-                      which the policy, or one with a greater retention, was effective.
-                      This value is in RFC 3339 format.
-                    type: string
                   retentionDurationSeconds:
                     description: The duration in seconds that soft-deleted objects
                       in the bucket will be retained and cannot be permanently deleted.

config/servicemappings/storage.yaml

@@ -36,6 +36,8 @@ spec:
         labels: labels
       resourceID:
         targetField: name
+      ignoredOutputOnlySpecFields:
+        - soft_delete_policy.effective_time
       observedFields:
         - soft_delete_policy.effective_time
         - soft_delete_policy.retention_duration_seconds

config/tests/servicemapping/servicemapping_test.go

@@ -236,6 +236,11 @@ func TestTerraformFieldsAreInResourceSchema(t *testing.T) {
 				for _, f := range rc.IgnoredFields {
 					fields = append(fields, f)
 				}
+				if rc.IgnoredOutputOnlySpecFields != nil {
+					for _, o := range *rc.IgnoredOutputOnlySpecFields {
+						fields = append(fields, o)
+					}
+				}
 				for _, c := range rc.Containers {
 					fields = append(fields, c.TFField)
 				}
@@ -1431,3 +1436,42 @@ func assertReferencedResourcesNotAlpha(t *testing.T, rc *v1alpha1.ResourceConfig
 		}
 	}
 }
+
+func TestIgnoredOutputOnlySpecFields(t *testing.T) {
+	t.Parallel()
+	serviceMappings := testservicemappingloader.New(t).GetServiceMappings()
+	provider := tfprovider.NewOrLogFatal(tfprovider.UnitTestConfig())
+	for _, sm := range serviceMappings {
+		sm := sm
+		t.Run(sm.Name, func(t *testing.T) {
+			t.Parallel()
+			for _, rc := range sm.Spec.Resources {
+				tfResource := provider.ResourcesMap[rc.Name]
+				rc := rc
+				t.Run(rc.Kind, func(t *testing.T) {
+					t.Parallel()
+					if rc.IgnoredOutputOnlySpecFields == nil {
+						return
+					}
+					if len(*rc.IgnoredOutputOnlySpecFields) == 0 {
+						t.Errorf("kind %v has an empty IgnoredOutputOnlySpecFields slice", rc.Kind)
+						return
+					}
+					for _, f := range *rc.IgnoredOutputOnlySpecFields {
+						if f == "" {
+							t.Errorf("kind %v has an empty value in IgnoredOutputOnlySpecFields slice", rc.Kind)
+							return
+						}
+						fieldSchema, err := tfresource.GetTFSchemaForField(tfResource, f)
+						if err != nil {
+							t.Errorf("error getting TF schema for output-only spec field %v in kind %v", f, rc.Kind)
+						}
+						if tfresource.IsConfigurableField(fieldSchema) {
+							t.Errorf("output-only spec field %v in kind %v is configurable", f, rc.Kind)
+						}
+					}
+				})
+			}
+		})
+	}
+}

pkg/apis/core/v1alpha1/servicemapping_types.go

@@ -131,6 +131,14 @@ type ResourceConfig struct {
 	// Terraform resource.
 	IgnoredFields []string `json:"ignoredFields,omitempty"`
 
+	// IgnoredOutputOnlySpecFields is a list of fields that should not be added
+	// to spec because they are output-only.
+	// We have a legacy bug that adds all the fields under a writable top-level
+	// field into spec during CRD generation even if the subfield itself is
+	// output-only. We should stop the bleeding by manually adding any new
+	// output-only subfields under a writable top-level field into this list.
+	IgnoredOutputOnlySpecFields *[]string `json:"ignoredOutputOnlySpecFields,omitempty"`
+
 	// Deprecated: use HierarchicalReferences instead. Only resources that
 	// already specify Containers should continue to specify Containers so that
 	// these resources can continue to support resource-level container

pkg/crd/crdgeneration/tf2crdgeneration.go

@@ -63,13 +63,6 @@ func GenerateTF2CRD(sm *corekccv1alpha1.ServiceMapping, resourceConfig *corekccv
 	addResourceIDFieldIfSupported(resourceConfig, specJSONSchema)
 	handleHierarchicalReferences(resourceConfig, specJSONSchema)
 
-	if len(specJSONSchema.Properties) > 0 {
-		openAPIV3Schema.Properties["spec"] = *specJSONSchema
-		if len(specJSONSchema.Required) > 0 {
-			openAPIV3Schema.Required = slice.IncludeString(openAPIV3Schema.Required, "spec")
-		}
-	}
-
 	var err error
 	if k8s.OutputOnlyFieldsAreUnderObservedState(kubeschema.GroupVersionKind{
 		Kind:    resourceConfig.Kind,
@@ -84,6 +77,15 @@ func GenerateTF2CRD(sm *corekccv1alpha1.ServiceMapping, resourceConfig *corekccv
 		}
 	}
 	addObservedFieldsToObservedState(resourceConfig, specJSONSchema, statusOrObservedStateJSONSchema)
+	removeIgnoredOutputOnlySpecFields(resourceConfig, specJSONSchema)
+
+	if len(specJSONSchema.Properties) > 0 {
+		openAPIV3Schema.Properties["spec"] = *specJSONSchema
+		if len(specJSONSchema.Required) > 0 {
+			openAPIV3Schema.Required = slice.IncludeString(openAPIV3Schema.Required, "spec")
+		}
+	}
+
 	for k, v := range statusOrObservedStateJSONSchema.Properties {
 		openAPIV3Schema.Properties["status"].Properties[k] = v
 	}
@@ -261,6 +263,17 @@ func removeOverwrittenFields(rc *corekccv1alpha1.ResourceConfig, s *apiextension
 		}
 	}
 }
+func removeIgnoredOutputOnlySpecFields(rc *corekccv1alpha1.ResourceConfig, specJSONSchema *apiextensions.JSONSchemaProps) {
+	if rc.IgnoredOutputOnlySpecFields == nil {
+		return
+	}
+	for _, f := range *rc.IgnoredOutputOnlySpecFields {
+		removedInSpec := removeFieldIfExist(f, specJSONSchema)
+		if !removedInSpec {
+			panic(fmt.Errorf("cannot find the output-only spec field %s in spec JSON schema for resource %s", f, rc.Name))
+		}
+	}
+}
 
 func removeIgnoredFields(rc *corekccv1alpha1.ResourceConfig, specJSONSchema, statusJSONSchema *apiextensions.JSONSchemaProps) {
 	for _, f := range rc.IgnoredFields {

pkg/krmtotf/tftokrm.go

@@ -64,9 +64,10 @@ func ResolveSpecAndStatus(resource *Resource, state *terraform.InstanceState) (
 func GetSpecAndStatusFromState(resource *Resource, state *terraform.InstanceState) (
 	spec map[string]interface{}, status map[string]interface{}) {
 	unmodifiedState := InstanceStateToMap(resource.TFResource, state)
-	krmState := ConvertTFObjToKCCObj(unmodifiedState, resource.Spec, resource.TFResource.Schema,
+	krmState, krmStateWithIgnoredOutputOnlySpecFields := ConvertTFObjToKCCObj(unmodifiedState, resource.Spec, resource.TFResource.Schema,
 		&resource.ResourceConfig, "", resource.ManagedFields)
 	krmState = withCustomExpanders(krmState, resource, resource.Kind)
+	krmStateWithIgnoredOutputOnlySpecFields = withCustomExpanders(krmStateWithIgnoredOutputOnlySpecFields, resource, resource.Kind)
 	spec = make(map[string]interface{})
 	status = make(map[string]interface{})
 	for field, fieldSchema := range resource.TFResource.Schema {
@@ -107,7 +108,7 @@ func GetSpecAndStatusFromState(resource *Resource, state *terraform.InstanceStat
 		status["observedGeneration"] = deepcopy.DeepCopy(observedGeneration)
 	}
 	if resource.ResourceConfig.ObservedFields != nil {
-		observedFields := resolveObservedFields(resource, krmState)
+		observedFields := resolveObservedFields(resource, krmStateWithIgnoredOutputOnlySpecFields)
 		if len(observedFields) > 0 {
 			// Merge the observed fields into the observed state.
 			observedState, ok := status[k8s.ObservedStateFieldName]
@@ -458,8 +459,9 @@ func getValueFromState(state map[string]interface{}, key string) (string, bool)
 }
 
 // ConvertTFObjToKCCObj takes the state (which should be a Terraform resource),
-// and returns a map that is formatted to KCC's custom resource schema for the
-// appropriate Kind.
+// and returns two maps: the first one is formatted to KCC's custom resource
+// schema for the appropriate Kind, the second one contains additional
+// output-only fields that are used in observed state only.
 //
 // prevSpec is used for multiple purposes:
 //   - ensures the returned result has a similar order for objects in lists, reducing
@@ -470,26 +472,35 @@ func getValueFromState(state map[string]interface{}, key string) (string, bool)
 //     state and the prevSpec.
 func ConvertTFObjToKCCObj(state map[string]interface{}, prevSpec map[string]interface{},
 	schemas map[string]*tfschema.Schema, rc *corekccv1alpha1.ResourceConfig, prefix string,
-	managedFields *fieldpath.Set) map[string]interface{} {
-	raw := convertTFMapToKCCMap(state, prevSpec, schemas, rc, prefix, managedFields)
+	managedFields *fieldpath.Set) (krmState, krmStateWithIgnoredOutputOnlySpecFields map[string]interface{}) {
+	rawKRMState := convertTFMapToKCCMap(state, prevSpec, schemas, rc, prefix, managedFields, true)
+	rawKRMStateWithIgnoredOutputOnlySpecFields := deepcopy.DeepCopy(rawKRMState)
+	if rc.IgnoredOutputOnlySpecFields != nil {
+		rawKRMStateWithIgnoredOutputOnlySpecFields =
+			convertTFMapToKCCMap(state, prevSpec, schemas, rc, prefix, managedFields, false)
+	}
 	// Round-trip via JSON in order to ensure consistency with unstructured.Unstructured's Object type.
-	var ret map[string]interface{}
-	if err := util.Marshal(raw, &ret); err != nil {
+	var retKRMState map[string]interface{}
+	if err := util.Marshal(rawKRMState, &retKRMState); err != nil {
 		panic(fmt.Errorf("error normalizing KRM-ified object: %w", err))
 	}
-	return ret
+	var retKRMStateWithIgnoredOutputOnlySpecFields map[string]interface{}
+	if err := util.Marshal(rawKRMStateWithIgnoredOutputOnlySpecFields, &retKRMStateWithIgnoredOutputOnlySpecFields); err != nil {
+		panic(fmt.Errorf("error normalizing KRM-ified object: %w", err))
+	}
+	return retKRMState, retKRMStateWithIgnoredOutputOnlySpecFields
 }
 
 func convertTFMapToKCCMap(state map[string]interface{}, prevSpec map[string]interface{},
 	schemas map[string]*tfschema.Schema, rc *corekccv1alpha1.ResourceConfig, prefix string,
-	managedFields *fieldpath.Set) map[string]interface{} {
+	managedFields *fieldpath.Set, ignoreOutputOnlySpecFields bool) map[string]interface{} {
 	ret := make(map[string]interface{})
 	for field, schema := range schemas {
 		qualifiedName := field
 		if prefix != "" {
 			qualifiedName = prefix + "." + field
 		}
-		if isOverriddenField(qualifiedName, rc) {
+		if isOverriddenField(qualifiedName, rc, ignoreOutputOnlySpecFields) {
 			continue
 		}
 		if ok, refConfig := IsReferenceField(qualifiedName, rc); ok {
@@ -584,7 +595,7 @@ func convertTFMapToKCCMap(state map[string]interface{}, prevSpec map[string]inte
 						nestedManagedFields = fieldpath.NewSet()
 					}
 				}
-				if val := convertTFMapToKCCMap(tfObjMap, prevObjMap, tfObjSchema, rc, qualifiedName, nestedManagedFields); val != nil {
+				if val := convertTFMapToKCCMap(tfObjMap, prevObjMap, tfObjSchema, rc, qualifiedName, nestedManagedFields, ignoreOutputOnlySpecFields); val != nil {
 					ret[key] = val
 				}
 				continue
@@ -595,7 +606,7 @@ func convertTFMapToKCCMap(state map[string]interface{}, prevSpec map[string]inte
 				// the status can be treated the same as lists, as the new state is the definitive
 				// source of truth and there is no reference resolution.
 				if schema.Required || schema.Optional {
-					retObj := convertTFSetToKCCSet(stateVal, prevSpecVal, schema, rc, qualifiedName)
+					retObj := convertTFSetToKCCSet(stateVal, prevSpecVal, schema, rc, qualifiedName, ignoreOutputOnlySpecFields)
 					if retObj != nil {
 						ret[key] = retObj
 					}
@@ -618,7 +629,7 @@ func convertTFMapToKCCMap(state map[string]interface{}, prevSpec map[string]inte
 					if idx < len(prevList) {
 						prevObjMap, _ = prevList[idx].(map[string]interface{})
 					}
-					if val := convertTFMapToKCCMap(tfObjMap, prevObjMap, tfObjSchema, rc, qualifiedName, nil); val != nil {
+					if val := convertTFMapToKCCMap(tfObjMap, prevObjMap, tfObjSchema, rc, qualifiedName, nil, ignoreOutputOnlySpecFields); val != nil {
 						retObjList = append(retObjList, val)
 					}
 				}
@@ -718,7 +729,7 @@ func convertTFReferenceToKCCReference(tfField, specKey string, state map[string]
 }
 
 // convertTFSetToKCCSet converts a set object in Terraform to a KCC set object
-func convertTFSetToKCCSet(stateVal, prevSpecVal interface{}, schema *tfschema.Schema, rc *corekccv1alpha1.ResourceConfig, prefix string) interface{} {
+func convertTFSetToKCCSet(stateVal, prevSpecVal interface{}, schema *tfschema.Schema, rc *corekccv1alpha1.ResourceConfig, prefix string, ignoreOutputOnlySpecFields bool) interface{} {
 	if containsReferenceField(prefix, rc) {
 		// TODO(kcc-eng): Support the case where the hashing function depends on resolved values from
 		//  resource references. For the time being, fall back to the declared state.
@@ -767,12 +778,12 @@ func convertTFSetToKCCSet(stateVal, prevSpecVal interface{}, schema *tfschema.Sc
 			stateElem = map[string]interface{}{}
 		}
 		retObjList = append(retObjList,
-			convertTFElemToKCCElem(schema.Elem, stateElem, prevElem, rc, prefix))
+			convertTFElemToKCCElem(schema.Elem, stateElem, prevElem, rc, prefix, ignoreOutputOnlySpecFields))
 	}
 	// append any new elements in the list to the end
 	for _, newElem := range stateHashMap {
 		retObjList = append(retObjList,
-			convertTFElemToKCCElem(schema.Elem, newElem, nil, rc, prefix))
+			convertTFElemToKCCElem(schema.Elem, newElem, nil, rc, prefix, ignoreOutputOnlySpecFields))
 	}
 	if len(retObjList) == 0 {
 		return nil
@@ -867,7 +878,7 @@ func getDefaultValueForTFType(tfType tfschema.ValueType) interface{} {
 	}
 }
 
-func convertTFElemToKCCElem(elemSchema, tfObj, prevSpecObj interface{}, rc *corekccv1alpha1.ResourceConfig, prefix string) interface{} {
+func convertTFElemToKCCElem(elemSchema, tfObj, prevSpecObj interface{}, rc *corekccv1alpha1.ResourceConfig, prefix string, ignoreOutputOnlySpecFields bool) interface{} {
 	switch elemSchema.(type) {
 	case *tfschema.Schema:
 		if prevSpecObj != nil {
@@ -878,13 +889,13 @@ func convertTFElemToKCCElem(elemSchema, tfObj, prevSpecObj interface{}, rc *core
 		tfObjSchema := elemSchema.(*tfschema.Resource).Schema
 		tfObjMap, _ := tfObj.(map[string]interface{})
 		prevObjMap, _ := prevSpecObj.(map[string]interface{})
-		return convertTFMapToKCCMap(tfObjMap, prevObjMap, tfObjSchema, rc, prefix, nil)
+		return convertTFMapToKCCMap(tfObjMap, prevObjMap, tfObjSchema, rc, prefix, nil, ignoreOutputOnlySpecFields)
 	default:
 		return prevSpecObj
 	}
 }
 
-func isOverriddenField(field string, rc *corekccv1alpha1.ResourceConfig) bool {
+func isOverriddenField(field string, rc *corekccv1alpha1.ResourceConfig, ignoreOutputOnlySpecFields bool) bool {
 	if field == rc.MetadataMapping.Name || field == rc.MetadataMapping.Labels {
 		return true
 	}
@@ -907,7 +918,13 @@ func isOverriddenField(field string, rc *corekccv1alpha1.ResourceConfig) bool {
 				return true
 			}
 		}
-
+	}
+	if ignoreOutputOnlySpecFields && rc.IgnoredOutputOnlySpecFields != nil {
+		for _, f := range *rc.IgnoredOutputOnlySpecFields {
+			if field == f {
+				return true
+			}
+		}
 	}
 	return false
 }

pkg/krmtotf/tftokrm_test.go

@@ -1017,7 +1017,7 @@ func TestConvertTFObjToKCCObj(t *testing.T) {
 				r.TFResource.Schema = tc.schemaOverride
 			}
 			r.SetNamespace(test.Namespace)
-			actual := ConvertTFObjToKCCObj(tc.state, tc.prevSpec, r.TFResource.Schema, &r.ResourceConfig, "", tc.managedFields)
+			actual, _ := ConvertTFObjToKCCObj(tc.state, tc.prevSpec, r.TFResource.Schema, &r.ResourceConfig, "", tc.managedFields)
 			if !reflect.DeepEqual(tc.expected, actual) {
 				t.Fatalf("expected: %v, actual: %v", tc.expected, actual)
 			}

pkg/test/resourcefixture/testdata/basic/storage/v1beta1/storagebucket/storagebucketbasic/_generated_object_storagebucketbasic.golden.yaml

@@ -8,7 +8,7 @@ metadata:
   finalizers:
   - cnrm.cloud.google.com/finalizer
   - cnrm.cloud.google.com/deletion-defender
-  generation: 5
+  generation: 4
   labels:
     cnrm-test: "true"
     label-one: value-one
@@ -31,7 +31,7 @@ status:
     reason: UpToDate
     status: "True"
     type: Ready
-  observedGeneration: 5
+  observedGeneration: 4
   observedState:
     softDeletePolicy:
       retentionDurationSeconds: 0

pkg/test/resourcefixture/testdata/basic/storage/v1beta1/storagebucket/storagebucketsoftdelete/_generated_object_storagebucketsoftdelete.golden.yaml

@@ -26,7 +26,6 @@ spec:
   publicAccessPrevention: inherited
   resourceID: storagebucket-sample-${uniqueId}
   softDeletePolicy:
-    effectiveTime: "1970-01-01T00:00:00Z"
     retentionDurationSeconds: 604800
   storageClass: STANDARD
   versioning:

pkg/test/resourcefixture/testdata/basic/storage/v1beta1/storagebucket/storagebucketzero/_generated_object_storagebucketzero.golden.yaml

@@ -20,7 +20,6 @@ spec:
   publicAccessPrevention: inherited
   resourceID: storagebucket-sample-${uniqueId}
   softDeletePolicy:
-    effectiveTime: "1970-01-01T00:00:00Z"
     retentionDurationSeconds: 604800
   storageClass: STANDARD
   versioning:

pkg/test/resourcefixture/testdata/directives/forcedestroy/_generated_object_forcedestroy.golden.yaml

@@ -21,7 +21,6 @@ spec:
   publicAccessPrevention: inherited
   resourceID: storagebucket-sample-${uniqueId}
   softDeletePolicy:
-    effectiveTime: "1970-01-01T00:00:00Z"
     retentionDurationSeconds: 604800
   storageClass: STANDARD
   versioning:

pkg/test/resourcefixture/testdata/reconcileintervalannotations/storagebucket/_generated_object_storagebucket#01.golden.yaml

@@ -19,7 +19,6 @@ spec:
   publicAccessPrevention: inherited
   resourceID: storagebucket-sample-${uniqueId}
   softDeletePolicy:
-    effectiveTime: "1970-01-01T00:00:00Z"
     retentionDurationSeconds: 604800
   storageClass: STANDARD
   versioning:

pkg/test/resourcefixture/testdata/reconcileintervalannotations/storagebucket/_generated_object_storagebucket.golden.yaml

@@ -19,7 +19,6 @@ spec:
   publicAccessPrevention: inherited
   resourceID: storagebucket-sample-${uniqueId}
   softDeletePolicy:
-    effectiveTime: "1970-01-01T00:00:00Z"
     retentionDurationSeconds: 604800
   storageClass: STANDARD
   versioning: