Handle server-side defaults

[mbaijal]

The ScalableTarget Resource has two fields which do not work with the existing ACK CompareResource functionality. This PR fixes this issue by adding these two fields to the ignore list for compare and then adding custom logic.

More details discussed in Issue #796

Testing

• I tested that the logs were as expected at each step - before any changes; after removing the compare logic entirely; after adding the custom compare function with no changes and finally the custom compare with the new checks.
• I also checked the logs after manually testing with the suspendedState field added to the spec.

[a-hilaly]

I'm not sure if comparing only the a parameter (a.ko.Spec.RoleARN) will work ? thinking about where someone mistakenly creates a CR without a RoleARN - i guess the changes will not be detected

[surajkota]

RoleARN is optional and user can submit CR without it. autoscaling service creates it for you if the user didn't specify it. In such cases, we don't want the controller to see it as a diff

In future, it should be set in the Spec as a server default

[surajkota]

RoleARN is optional and user can submit CR without it. autoscaling service creates it for you if the user didn't specify it. In such cases, we don't want the controller to see it as a diff

In future, it should be set in the Spec as a server default

[surajkota]

As I think more about this, it seems like a better approach would be to set the server defaults for values that are null and let the comparison happen like normal(i.e. we don't add it to compare.is_ignore). This will handle the case where the user initially sets a field to true and deleted it from spec thinking it will take the default value which in this case is False.

initially

scalableDimension: "string",
serviceNamespace: "string",
suspendedState:
  DynamicScalingInSuspended: true

updates the resource

scalableDimension: "string",
serviceNamespace: "string",

In this scenario, current logic will ignore comparison whereas it should report the diff

This solution only works where we know the server defaults but is a better way to handle this. What do you think?

For example, it doesn't work for Spec.RoleARN because it's a server-generated rather than default. So in future, we need to patch these type of fields to desired.

[mbaijal]

Most of these resources do not support updates, so not really an issue but I agree this approach of setting defaults is a better one, fixed for suspendedState. Left RoleARN as is.

[RedbackThomson]

/ok-to-test

[surajkota]

/approve
/lgtm

[ack-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mbaijal, surajkota

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [mbaijal,surajkota]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[jaypipes]

@mbaijal @surajkota I would recommend this PR be reverted. The code is setting these values properly:

applicationautoscaling-controller/pkg/resource/scalable_target/sdk.go

Lines 85 to 114 in d047c61

	if elem.RoleARN != nil {
	ko.Spec.RoleARN = elem.RoleARN
	} else {
	ko.Spec.RoleARN = nil
	}
	if elem.ScalableDimension != nil {
	ko.Spec.ScalableDimension = elem.ScalableDimension
	} else {
	ko.Spec.ScalableDimension = nil
	}
	if elem.ServiceNamespace != nil {
	ko.Spec.ServiceNamespace = elem.ServiceNamespace
	} else {
	ko.Spec.ServiceNamespace = nil
	}
	if elem.SuspendedState != nil {
	f7 := &svcapitypes.SuspendedState{}
	if elem.SuspendedState.DynamicScalingInSuspended != nil {
	f7.DynamicScalingInSuspended = elem.SuspendedState.DynamicScalingInSuspended
	}
	if elem.SuspendedState.DynamicScalingOutSuspended != nil {
	f7.DynamicScalingOutSuspended = elem.SuspendedState.DynamicScalingOutSuspended
	}
	if elem.SuspendedState.ScheduledScalingSuspended != nil {
	f7.ScheduledScalingSuspended = elem.SuspendedState.ScheduledScalingSuspended
	}
	ko.Spec.SuspendedState = f7
	} else {
	ko.Spec.SuspendedState = nil
	}

The root cause of the issue you are seeing has nothing to do with the Delta/Compare code and everything to do with this line in the code-generator template for manager.go:

https://github.com/aws-controllers-k8s/code-generator/blob/main/templates/pkg/resource/manager.go.tpl#L77

That needs to pass observed to onError instead of r. That is the reason that you are seeing the desired and observed resource not seem to change values when sdkFind() returns.

/cc @a-hilaly