From ea7101091ef10e5d375cf785310de919d32cd46c Mon Sep 17 00:00:00 2001 From: Amine Hilaly Date: Wed, 8 Feb 2023 15:06:07 +0100 Subject: [PATCH] Add `OmitUnchangedField` config for selective inclusion of changed CRD fields in update requests. 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 --- pkg/config/resource.go | 7 +- pkg/generate/code/set_sdk.go | 28 +++- pkg/generate/code/set_sdk_test.go | 122 ++++++++++++++++++ pkg/model/crd.go | 15 +++ .../models/apis/mq/0000-00-00/generator.yaml | 2 + templates/pkg/resource/sdk_update.go.tpl | 7 + 6 files changed, 176 insertions(+), 5 deletions(-) diff --git a/pkg/config/resource.go b/pkg/config/resource.go index ff1546c0..72fa2803 100644 --- a/pkg/config/resource.go +++ b/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. diff --git a/pkg/generate/code/set_sdk.go b/pkg/generate/code/set_sdk.go index 56bca302..53f47f32 100644 --- a/pkg/generate/code/set_sdk.go +++ b/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 } @@ -547,17 +570,14 @@ func SetSDKGetAttributes( // attrMap := map[string]*string{} // // if r.ko.Spec.DeliveryPolicy != nil { -// attrMap["DeliveryPolicy"] = r.ko.Spec.DeliveryPolicy +// attrMap["DeliveryPolicy"] = r.ko.Spec.DeliveryPolicy // } -// // if r.ko.Spec.DisplayName != nil { // attrMap["DisplayName"} = r.ko.Spec.DisplayName // } -// // if r.ko.Spec.KMSMasterKeyID != nil { // attrMap["KmsMasterKeyId"] = r.ko.Spec.KMSMasterKeyID // } -// // if r.ko.Spec.Policy != nil { // attrMap["Policy"] = r.ko.Spec.Policy // } diff --git a/pkg/generate/code/set_sdk_test.go b/pkg/generate/code/set_sdk_test.go index 792a1da4..3821cb3f 100644 --- a/pkg/generate/code/set_sdk_test.go +++ b/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) diff --git a/pkg/model/crd.go b/pkg/model/crd.go index cd0eb4d2..7be710c1 100644 --- a/pkg/model/crd.go +++ b/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 { diff --git a/pkg/testdata/models/apis/mq/0000-00-00/generator.yaml b/pkg/testdata/models/apis/mq/0000-00-00/generator.yaml index 75e61ca4..5e247c1f 100644 --- a/pkg/testdata/models/apis/mq/0000-00-00/generator.yaml +++ b/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 \ No newline at end of file diff --git a/templates/pkg/resource/sdk_update.go.tpl b/templates/pkg/resource/sdk_update.go.tpl index d53229f1..6f91759d 100644 --- a/templates/pkg/resource/sdk_update.go.tpl +++ b/templates/pkg/resource/sdk_update.go.tpl @@ -25,7 +25,11 @@ func (rm *resourceManager) sdkUpdate( return updated, err } {{- end }} + {{- if .CRD.OmitUnchangedFieldsOnUpdate }} + input, err := rm.newUpdateRequestPayload(ctx, desired, delta) + {{- else }} input, err := rm.newUpdateRequestPayload(ctx, desired) + {{- end }} if err != nil { return nil, err } @@ -68,6 +72,9 @@ func (rm *resourceManager) sdkUpdate( func (rm *resourceManager) newUpdateRequestPayload( ctx context.Context, r *resource, +{{- if .CRD.OmitUnchangedFieldsOnUpdate }} + delta *ackcompare.Delta, +{{- end }} ) (*svcsdk.{{ .CRD.Ops.Update.InputRef.Shape.ShapeName }}, error) { res := &svcsdk.{{ .CRD.Ops.Update.InputRef.Shape.ShapeName }}{} {{ GoCodeSetUpdateInput .CRD "r.ko" "res" 1 }}