Skip to content

Commit

Permalink
Add OmitUnchangedField config for selective inclusion of changed CRD
Browse files Browse the repository at this point in the history
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 <hilalyamine@gmail.com>
  • Loading branch information
a-hilaly committed Feb 15, 2023
1 parent e0eac49 commit ea71010
Show file tree
Hide file tree
Showing 6 changed files with 176 additions and 5 deletions.
7 changes: 6 additions & 1 deletion pkg/config/resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
28 changes: 24 additions & 4 deletions pkg/generate/code/set_sdk.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
)
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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
// }
Expand Down
122 changes: 122 additions & 0 deletions pkg/generate/code/set_sdk_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
15 changes: 15 additions & 0 deletions pkg/model/crd.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 2 additions & 0 deletions pkg/testdata/models/apis/mq/0000-00-00/generator.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,5 @@ resources:
fields:
Users.Password:
is_secret: true
update_operation:
omit_unchanged_fields: true
7 changes: 7 additions & 0 deletions templates/pkg/resource/sdk_update.go.tpl
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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 }}
Expand Down

0 comments on commit ea71010

Please sign in to comment.