Add OmitUnchangedField config for selective inclusion of changed CRD fields in update requests. (#406)

This patch adds a new generator configuration
`update_operation.omit_unchanged_fields` that instructs the code
generator to generate cvode that only include fields in update reuqests
if their values have actually changed. This is accomplished by using
`delta.DifferentAt` before setting any field in `newUpdateRequest`
function.

This change will improve the reliability of the generated code by
preventing the controller from trying to update unecessary fields.

Signed-off-by: Amine Hilaly <[email protected]>

Author: a-hilaly

pkg/config/resource.go

@@ -319,9 +319,14 @@ type UpdateOperationConfig struct {
 	// CustomMethodName is a string for the method name to replace the
 	// sdkUpdate() method implementation for this resource
 	CustomMethodName string `json:"custom_method_name"`
+	// OmitUnchangedFields instructs the code generator on how to generate
+	// `newUpdateRequestPayload` function. If the boolean is true, the code generator
+	// will leverage `delta.DifferentAt` to check whether a field have changed or not
+	// before including it in the update request.
+	OmitUnchangedFields bool `json:"omit_unchanged_fields"`
 }
 
-// AdditionalConfig can be used to specify additional printer columns to be included
+// AdditionalColumnConfig can be used to specify additional printer columns to be included
 // in a Resource's output from kubectl.
 type AdditionalColumnConfig struct {
 	// Name is the thing to display in the column's output.

pkg/generate/code/set_sdk.go

@@ -294,6 +294,19 @@ func SetSDK(
 		//     }
 		//     res.VpnMemberships = f0
 		// }
+
+		omitUnchangedFieldsOnUpdate := op == r.Ops.Update && r.OmitUnchangedFieldsOnUpdate()
+		if omitUnchangedFieldsOnUpdate {
+			fieldJSONPath := fmt.Sprintf("%s.%s", cfg.PrefixConfig.SpecField[1:], f.Names.Camel)
+			out += fmt.Sprintf(
+				"%sif delta.DifferentAt(%q) {\n", indent, fieldJSONPath,
+			)
+
+			// increase indentation level
+			indentLevel++
+			indent = "\t" + indent
+		}
+
 		out += fmt.Sprintf(
 			"%sif %s != nil {\n", indent, sourceAdaptedVarName,
 		)
@@ -353,6 +366,16 @@ func SetSDK(
 		out += fmt.Sprintf(
 			"%s}\n", indent,
 		)
+
+		if omitUnchangedFieldsOnUpdate {
+			// decrease indentation level
+			indentLevel--
+			indent = indent[1:]
+
+			out += fmt.Sprintf(
+				"%s}\n", indent,
+			)
+		}
 	}
 	return out
 }

pkg/generate/code/set_sdk_test.go

@@ -1367,6 +1367,128 @@ func TestSetSDK_Elasticache_User_Create_Override_Values(t *testing.T) {
 	)
 }
 
+func TestSetSDK_MQ_Broker_newUpdateRequest_OmitUnchangedValues(t *testing.T) {
+	assert := assert.New(t)
+	require := require.New(t)
+
+	g := testutil.NewModelForService(t, "mq")
+
+	crd := testutil.GetCRDByName(t, g, "Broker")
+	require.NotNil(crd)
+
+	expected := `
+	if delta.DifferentAt("Spec.AuthenticationStrategy") {
+		if r.ko.Spec.AuthenticationStrategy != nil {
+			res.SetAuthenticationStrategy(*r.ko.Spec.AuthenticationStrategy)
+		}
+	}
+	if delta.DifferentAt("Spec.AutoMinorVersionUpgrade") {
+		if r.ko.Spec.AutoMinorVersionUpgrade != nil {
+			res.SetAutoMinorVersionUpgrade(*r.ko.Spec.AutoMinorVersionUpgrade)
+		}
+	}
+	if delta.DifferentAt("Spec.BrokerID") {
+		if r.ko.Status.BrokerID != nil {
+			res.SetBrokerId(*r.ko.Status.BrokerID)
+		}
+	}
+	if delta.DifferentAt("Spec.Configuration") {
+		if r.ko.Spec.Configuration != nil {
+			f3 := &svcsdk.ConfigurationId{}
+			if r.ko.Spec.Configuration.ID != nil {
+				f3.SetId(*r.ko.Spec.Configuration.ID)
+			}
+			if r.ko.Spec.Configuration.Revision != nil {
+				f3.SetRevision(*r.ko.Spec.Configuration.Revision)
+			}
+			res.SetConfiguration(f3)
+		}
+	}
+	if delta.DifferentAt("Spec.EngineVersion") {
+		if r.ko.Spec.EngineVersion != nil {
+			res.SetEngineVersion(*r.ko.Spec.EngineVersion)
+		}
+	}
+	if delta.DifferentAt("Spec.HostInstanceType") {
+		if r.ko.Spec.HostInstanceType != nil {
+			res.SetHostInstanceType(*r.ko.Spec.HostInstanceType)
+		}
+	}
+	if delta.DifferentAt("Spec.LDAPServerMetadata") {
+		if r.ko.Spec.LDAPServerMetadata != nil {
+			f6 := &svcsdk.LdapServerMetadataInput{}
+			if r.ko.Spec.LDAPServerMetadata.Hosts != nil {
+				f6f0 := []*string{}
+				for _, f6f0iter := range r.ko.Spec.LDAPServerMetadata.Hosts {
+					var f6f0elem string
+					f6f0elem = *f6f0iter
+					f6f0 = append(f6f0, &f6f0elem)
+				}
+				f6.SetHosts(f6f0)
+			}
+			if r.ko.Spec.LDAPServerMetadata.RoleBase != nil {
+				f6.SetRoleBase(*r.ko.Spec.LDAPServerMetadata.RoleBase)
+			}
+			if r.ko.Spec.LDAPServerMetadata.RoleName != nil {
+				f6.SetRoleName(*r.ko.Spec.LDAPServerMetadata.RoleName)
+			}
+			if r.ko.Spec.LDAPServerMetadata.RoleSearchMatching != nil {
+				f6.SetRoleSearchMatching(*r.ko.Spec.LDAPServerMetadata.RoleSearchMatching)
+			}
+			if r.ko.Spec.LDAPServerMetadata.RoleSearchSubtree != nil {
+				f6.SetRoleSearchSubtree(*r.ko.Spec.LDAPServerMetadata.RoleSearchSubtree)
+			}
+			if r.ko.Spec.LDAPServerMetadata.ServiceAccountPassword != nil {
+				f6.SetServiceAccountPassword(*r.ko.Spec.LDAPServerMetadata.ServiceAccountPassword)
+			}
+			if r.ko.Spec.LDAPServerMetadata.ServiceAccountUsername != nil {
+				f6.SetServiceAccountUsername(*r.ko.Spec.LDAPServerMetadata.ServiceAccountUsername)
+			}
+			if r.ko.Spec.LDAPServerMetadata.UserBase != nil {
+				f6.SetUserBase(*r.ko.Spec.LDAPServerMetadata.UserBase)
+			}
+			if r.ko.Spec.LDAPServerMetadata.UserRoleName != nil {
+				f6.SetUserRoleName(*r.ko.Spec.LDAPServerMetadata.UserRoleName)
+			}
+			if r.ko.Spec.LDAPServerMetadata.UserSearchMatching != nil {
+				f6.SetUserSearchMatching(*r.ko.Spec.LDAPServerMetadata.UserSearchMatching)
+			}
+			if r.ko.Spec.LDAPServerMetadata.UserSearchSubtree != nil {
+				f6.SetUserSearchSubtree(*r.ko.Spec.LDAPServerMetadata.UserSearchSubtree)
+			}
+			res.SetLdapServerMetadata(f6)
+		}
+	}
+	if delta.DifferentAt("Spec.Logs") {
+		if r.ko.Spec.Logs != nil {
+			f7 := &svcsdk.Logs{}
+			if r.ko.Spec.Logs.Audit != nil {
+				f7.SetAudit(*r.ko.Spec.Logs.Audit)
+			}
+			if r.ko.Spec.Logs.General != nil {
+				f7.SetGeneral(*r.ko.Spec.Logs.General)
+			}
+			res.SetLogs(f7)
+		}
+	}
+	if delta.DifferentAt("Spec.SecurityGroups") {
+		if r.ko.Spec.SecurityGroups != nil {
+			f8 := []*string{}
+			for _, f8iter := range r.ko.Spec.SecurityGroups {
+				var f8elem string
+				f8elem = *f8iter
+				f8 = append(f8, &f8elem)
+			}
+			res.SetSecurityGroups(f8)
+		}
+	}
+`
+	assert.Equal(
+		expected,
+		code.SetSDK(crd.Config(), crd, model.OpTypeUpdate, "r.ko", "res", 1),
+	)
+}
+
 func TestSetSDK_RDS_DBInstance_Create(t *testing.T) {
 	assert := assert.New(t)
 	require := require.New(t)

pkg/model/crd.go

@@ -362,6 +362,21 @@ func (r *CRD) HasImmutableFieldChanges() bool {
 	return false
 }
 
+// OmitUnchangedFieldsOnUpdate returns whether the controller needs to omit
+// unchanged fields from an update request or not.
+func (r *CRD) OmitUnchangedFieldsOnUpdate() bool {
+	if r.Config() == nil {
+		return false
+	}
+	rConfig, found := r.Config().Resources[r.Names.Original]
+	if found {
+		if rConfig.UpdateOperation != nil {
+			return rConfig.UpdateOperation.OmitUnchangedFields
+		}
+	}
+	return false
+}
+
 // IsARNPrimaryKey returns true if the CRD uses its ARN as its primary key in
 // ReadOne calls.
 func (r *CRD) IsARNPrimaryKey() bool {

pkg/testdata/models/apis/mq/0000-00-00/generator.yaml

@@ -12,3 +12,5 @@ resources:
     fields:
       Users.Password:
         is_secret: true
+    update_operation:
+      omit_unchanged_fields: true

templates/pkg/resource/sdk_update.go.tpl

@@ -25,7 +25,7 @@ func (rm *resourceManager) sdkUpdate(
 		return updated, err
 	}
 {{- end }}
-	input, err := rm.newUpdateRequestPayload(ctx, desired)
+	input, err := rm.newUpdateRequestPayload(ctx, desired, delta)
 	if err != nil {
 		return nil, err
 	}
@@ -68,6 +68,7 @@ func (rm *resourceManager) sdkUpdate(
 func (rm *resourceManager) newUpdateRequestPayload(
 	ctx context.Context,
 	r *resource,
+	delta *ackcompare.Delta,
 ) (*svcsdk.{{ .CRD.Ops.Update.InputRef.Shape.ShapeName }}, error) {
 	res := &svcsdk.{{ .CRD.Ops.Update.InputRef.Shape.ShapeName }}{}
 {{ GoCodeSetUpdateInput .CRD "r.ko" "res" 1 }}