Clarifying ACK Update logic (spec/status, delta, etc.)

[echen-98]

The intent of this issue is to outline some of the challenges faced in determining the latest state of a resource and when/how it should be updated. Many of these items have already been addressed and have working solutions, but others still require input from other service teams and support at an ACK framework level.

Summary/TL;DR: What still needs to be addressed?

1. Should latest.Spec be populated from scratch in ReadOne/sdkFind?
2. How do we make the latest observed state in latest.Spec visible to the user?
3. When should a Spec difference actually be added to the delta?
   1. Nil difference in one direction/default fields: in progress in ability to deal with server side defaults/initializers #812
   2. Nil difference in the other direction (e.g. k8s secrets fields): Issue opened (Support for SecretKeyReference updates #700), no discussion has occurred
   3. No nil difference, server returns different value than requested: workaround possible, but no framework-level solution yet
   4. No nil difference, but diff present because fields are immutable: a solution has been merged in, but there are still remaining questions
   5. Handle "update-only" fields (support update code paths for opensearchservice #816), issue opened
4. How do we only add meaningful Spec differences to the delta?
5. Should an Update payload only include the resource identifier along with diffs present in the delta, as opposed to everything currently in desired.Spec?

Dependent issues can be created as needed to discuss each of these items. The rest of the sections in the description provide some common context:

Definitions (descriptive, not prescriptive)

Spec - the fields corresponding to inputs of the Create API for a resource
Status - a collection of server responses from Create, Read, and Modify calls, including fields both related and unrelated to Spec fields. Can also contain additional state-keeping fields (e.g. conditions)

desired.Spec: the user-provided configuration of the resource
desired.Status: essentially a copy of latest.Status (after patching occurs)

latest.Spec: ideally, the currently observed state of all Spec fields resulting from a ReadOne call. In practice, some fields get updated with the observed state and others are copied over from desired.Spec

latest.Status: the aggregation of information returned by the API response, combined with previously observed state (latest is copied from desired, then modified, then patched back into desired) + other state-keeping fields.

More info on k8s conventions here

Given that updates are triggered upon Spec differences, latest.Spec must reliably represent the latest observed state. How do we populate each Spec field with the latest observed state?

There are a number of possible cases:

1. Field in API response has same name as Spec field: generated code populates this field directly. Example
2. API response contains information to populate the corresponding Spec field, but it may require some processing: the SetOutput methods or the post_set_output hooks allow service teams to manually populate the relevant Spec field. Example
3. Additional API calls are required to obtain the information to populate the Spec field: service teams can also use the SetOutput methods or post_set_output hooks to handle this. Example 1 , Example 2
4. The latest observed state of the field cannot be retrieved in the API response. This is the case with k8s secrets-enabled fields. (e.g. ElastiCache Replication Group has AuthTokenEnabled but you obviously can't retrieve the value AuthToken from the server). The update behavior of such fields may require some ACK framework-level input or support. Example

This discussion raises the first question below:

[echen-98]

1. If latest.Spec is meant to be a reliable measure of the latest observed state, should latest.Spec be populated from scratch?

The current approach is to copy latest from desired, and for each field in the API output shape, set the corresponding field nil, or the non-nil value returned by the server. Example

Therefore, the default generated code correctly handles case 1 above. However, many Spec fields fall under cases 2 and 3. Without any custom code, latest.Spec will not reflect the latest observed state, but instead will just contain a copy of the desired state for these fields.

Populating latest.Spec from scratch means that a latest.Spec field having a nil value sends a clear message that the latest observed state could not be retrieved from the server. The current approach may not inspire confidence in the sense that unless all bases are covered as far as custom code, one cannot be sure whether a latest.Spec field contains the latest observed state or merely a copy of the desired state.

[echen-98]

2. How does the user know they're getting what they asked for? In other words, assuming we can retrieve the latest observed state, should it be displayed to the user?

This isn't being done by default. As mentioned above, some fields get updated in latest.Spec in generated code (case 1), but the desired behavior is to have all latest.Spec fields reflect the latest observed state, since a Spec difference is the precondition for an Update.

However, latest.Spec is not visible to the user because upon patching, only latest.Status gets applied to desired.Status.

Essentially, the latest observed state in latest.Spec is thrown away (after being used to decide whether to invoke an update) in that it isn't displayed to the user.

Service teams have resorted to workarounds to make the server-returned value visible to the user, but in most cases the actual latest observed state for Spec fields is not available to the user.

With the current framework, we would have to duplicate every Spec field in Status if we wanted to make the latest observed state available to the user, as suggested in #814.

This approach was rejected on the basis of being inconsistent with k8s conventions, but there has been no alternative suggested which makes the latest observed state visible to the user. Currently, the user would have to manually process Status fields and separately make AWS API calls to obtain the latest state of their resource even though this work is already being done in ReadOne/sdkFind.

[echen-98]

3. When is a Spec difference meaningful? i.e, When should a Spec difference actually be added to the delta?

A Spec difference might be considered "meaningful" if we should actually invoke an update based on it.

1. Optional fields/default response from API: if desired.Spec.field is nil and latest.Spec.field is non-nil, this can mean that the user did not specify a value, but the server populated the field with a value. In this case the user has signaled they are fine with the default configuration, so even though there is a nil difference, it may not make sense to add this to the delta. This is being worked on in ability to deal with server side defaults/initializers #812.
2. Nil difference in the other direction: this may happen for a Spec field that falls under case 4 above. Suppose a field is a k8s secret – if latest.Spec is populated from scratch, the field may be nil in latest.Spec since the plain text value is not returned from the API. This would make for a situation where desired.Spec.field is non-nil, but latest.Spec.field is nil. By default this would be added to the delta and trigger an update every time ReadOne is called. How can we handle this ReadOne/Update logic for k8s secrets fields to trigger an update only if the actual secret value has changed, or if the secret name/key has changed? Issue opened, but no discussion has occurred Support for SecretKeyReference updates #700
3. Even if both latest.Spec.field and desired.Spec.field are non-nil, There are situations in which we would not want to add a diff to the delta.
   1. e.g. for the ElastiCache User resource, the AccessString returned from the server has a different representation than the specified one, and there is no straightforward way to translate between the two representations. The current workaround is to store a LastRequestedAccessString in Status and only add the diff if the current desired.Spec does not match it – otherwise, there would always be a Spec difference and unnecessary updates would be triggered. See here and here for further details and discussion.
   2. Another similar case: immutable fields, where the field can be specified upon creation of the resource, but not updated. Work has already been done here and here to handle this case. However, there are some remaining questions. For example, setting the advisory condition is located in (and therefore coupled with) sdkUpdate. An update API call is a precondition for setting the advisory condition on an immutable field, so if a user only updates an immutable field in the resource config, an unnecessary update call may occur.
4. This case is not as easily categorized, but certain fields are "update-only" (in a sense, the opposite of immutable fields). These may need to be included or excluded from the delta depending on the case: support update code paths for opensearchservice #816

[echen-98]

4. How do we ensure that only meaningful Spec differences get added to the delta?

Current workaround: use the post-delta hook to remove non-meaningful Spec differences from the delta after the fact.

Potential solution: add a custom hook containing logic for whether or not to add a diff to the delta on a per-field basis. For example, such a hook would replace any one of these blocks. A similar solution is being discussed in #812 for default fields.

[echen-98]

5. What goes in an Update payload?

The default generated code places everything in desired.Spec (that has a corresponding field in the update API input shape) in the update payload. Example

Now that the resource delta gives us the capability to do so, should Update payloads instead be populated from scratch to only include the fields which have a diff present in delta (along with the resource's identifier)?

[vijtrip2]

2. How does the user know they're getting what they asked for? In other words, assuming we can retrieve the latest observed state, should it be displayed to the user?

This can be addressed while working on "handling late-initialization" for ACK.
Proposal PR: #849

[ack-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.
Provide feedback via https://github.com/aws-controllers-k8s/community.
/lifecycle stale

[ack-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
If this issue is safe to close now please do so with /close.
Provide feedback via https://github.com/aws-controllers-k8s/community.
/lifecycle rotten