From f4d6566d247700e71aa3c98402185c31fe84bcf0 Mon Sep 17 00:00:00 2001 From: Joyce Ma Date: Thu, 20 Jun 2024 23:32:37 +0000 Subject: [PATCH] Ignore output-only spec field but support it in observed state --- ...buckets.storage.cnrm.cloud.google.com.yaml | 5 -- config/servicemappings/storage.yaml | 2 + .../servicemapping/servicemapping_test.go | 44 ++++++++++++++ .../core/v1alpha1/servicemapping_types.go | 8 +++ pkg/crd/crdgeneration/tf2crdgeneration.go | 24 +++++--- pkg/krmtotf/tftokrm.go | 59 ++++++++++++------- pkg/krmtotf/tftokrm_test.go | 2 +- 7 files changed, 110 insertions(+), 34 deletions(-) diff --git a/config/crds/resources/apiextensions.k8s.io_v1_customresourcedefinition_storagebuckets.storage.cnrm.cloud.google.com.yaml b/config/crds/resources/apiextensions.k8s.io_v1_customresourcedefinition_storagebuckets.storage.cnrm.cloud.google.com.yaml index 91ac26624b0..311e7394a5f 100644 --- a/config/crds/resources/apiextensions.k8s.io_v1_customresourcedefinition_storagebuckets.storage.cnrm.cloud.google.com.yaml +++ b/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. diff --git a/config/servicemappings/storage.yaml b/config/servicemappings/storage.yaml index 22c777a3ecf..28ff8737ee9 100644 --- a/config/servicemappings/storage.yaml +++ b/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 diff --git a/config/tests/servicemapping/servicemapping_test.go b/config/tests/servicemapping/servicemapping_test.go index a8c63c048fd..82cbea0cac0 100644 --- a/config/tests/servicemapping/servicemapping_test.go +++ b/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) + } + } + }) + } + }) + } +} diff --git a/pkg/apis/core/v1alpha1/servicemapping_types.go b/pkg/apis/core/v1alpha1/servicemapping_types.go index fd440bcf784..51bf4a097d3 100644 --- a/pkg/apis/core/v1alpha1/servicemapping_types.go +++ b/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 diff --git a/pkg/crd/crdgeneration/tf2crdgeneration.go b/pkg/crd/crdgeneration/tf2crdgeneration.go index b92558881e1..9c8570b3aa2 100644 --- a/pkg/crd/crdgeneration/tf2crdgeneration.go +++ b/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,14 @@ func removeOverwrittenFields(rc *corekccv1alpha1.ResourceConfig, s *apiextension } } } +func removeIgnoredOutputOnlySpecFields(rc *corekccv1alpha1.ResourceConfig, specJSONSchema *apiextensions.JSONSchemaProps) { + 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 { diff --git a/pkg/krmtotf/tftokrm.go b/pkg/krmtotf/tftokrm.go index 0e7463ff45e..1e63c7f40bd 100644 --- a/pkg/krmtotf/tftokrm.go +++ b/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 } diff --git a/pkg/krmtotf/tftokrm_test.go b/pkg/krmtotf/tftokrm_test.go index 4a23a5737e4..6612b2d74f1 100644 --- a/pkg/krmtotf/tftokrm_test.go +++ b/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) }