aws-controllers-k8s / community

AWS Controllers for Kubernetes (ACK) is a project enabling you to manage AWS services from Kubernetes
https://aws-controllers-k8s.github.io/community/
Apache License 2.0
2.36k stars 248 forks source link

ability to deal with server side defaults/initializers #812

Open surajkota opened 3 years ago

surajkota commented 3 years ago

Is your feature request related to a problem?

Ability to specify fields initialized by service post create. Following are the patterns observed:

  1. Generated fields. Example:
    1. roleARN field in ScalableTarget resource is generated by service. See https://github.com/aws-controllers-k8s/applicationautoscaling-controller/pull/12#discussion_r640097037 (service - Applicationautoscaling)
    2. staticHyperparameter field in Hyperparameter tuning resource. Slice has some extra fields returned from the service which should be ignored for comparison. See customSetDefaults in HPJob (service - SageMaker)
  2. Default value for fields
    1. Boolean fields returned as False.
      • Scalable target struct in ScalableTarget resource - SuspendedState.<*> (all fields) returned as False by default. See setDefaults in autoscaling (service - Applicationautoscaling)
    2. String fields return default values. e.g. Model resource - PrimaryContainer.Mode retuned as SingleMode by default (service - SageMaker)

Describe the solution you'd like 1.a, 2.a, 2.b - if DesiredResource.Spec.field = nil set these values from latestResource 1.b - a hook to write custom algorithm for comparison and remove service generated fields, ignore setting values from latestResource to desired

Note: In 2.a,

Describe alternatives you've considered 1.b - a hook to write custom logic to insert only server generated <key, value> pairs into the slice only if diff is False 1.b - a hook to write custom algorithm for comparison and insert service generated fields, set values from latestResource into desired only for comparison

echen-98 commented 3 years ago

1.b - a hook to write custom algorithm for comparison and remove service generated fields, ignore setting values from latestResource to desired

@surajkota do you mean a hook that would give service teams the ability to control whether or not the diff for a certain field should be added to the delta? i.e. essentially overwriting any one of these blocks: https://github.com/aws-controllers-k8s/elasticache-controller/blob/main/pkg/resource/user/delta.go#L35-L41

I definitely agree if so, but please let me know if I misinterpreted the suggestion.

1.a, 2.a, 2.b - if DesiredResource.Spec.field = nil set these values from latestResource

Also, could you elaborate on this suggestion? Do you mean directly populating desired.Spec with the server-returned default values?

echen-98 commented 3 years ago

In general, the generated code for adding a diff to the delta makes the assumption that a nil difference either direction is sufficient grounds for invoking an Update.

However, this is a case in which desired.Spec.field = nil and latest.Spec.field = non-nil, but we don't want to invoke an Update (because by not specifying the field, the user has signaled that they are okay with the default configuration for that field). Even if we did, we would be populating field with the equivalent of nil in the Update payload, which doesn't really make sense.

So some possible solutions would be:

  1. Changing ackcompare.HasNilDifference to only check for the nil difference in one direction, rather than both, on every Spec field. In other words, only add the diff if desired.Spec.field = non-nil and latest.Spec.field = nil but not the other way around.
  2. Adding support in the generator config to enable the above behavior, but on a per-field basis

@vijtrip2 do you have any thoughts on this, and has work already been done on this that I've missed or am not aware of?

surajkota commented 3 years ago

essentially overwriting any one of these blocks:

yes

Do you mean directly populating desired.Spec with the server-returned default values?

yes

RedbackThomson commented 2 years ago

Finished core implementation. Upcoming PR for back-off delay work.

surajkota commented 2 years ago

The current implementation does not handle all types of fields. Would like to keep this issue opened unless there is another one

ack-bot commented 2 years ago

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 commented 2 years ago

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

vijtrip2 commented 2 years ago

/lifecycle frozen