fix replica names for CloudSQLInstance

[fahedouch]

Signed-off-by: fahed dorgaa [email protected]

Description of your changes

LateInitializeStringSlice does not correctly populate Slice replica names. It must be intialized by in.ReplicaNames itself
Fixes #392

I have:

• [ x] Read and followed Crossplane's contribution process.
• [ x] Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

I added unit test

[Feggah]

Thanks for your contribution, @fahedouch .

I don't really agree on this solution for this problem. The LateInitialize function responsibility is "initialize optional fields that are empty in K8S API but are filled in the provider API", this means that this function is not supposed to change field values often, just on the first reconcile loop for optional fields that the user didn't specified any value.

The design problem for this is that the "source of truth" for the spec.ReplicaNames is going to be the provider API and not Kubernetes, which is not what we really want.

The problem here is that the field spec.ReplicaNames is empty when we have no replicas. Then, when we create the first one, this field is LateInitialized, because it was empty. When we create the second one, it doesn't change the value (and it shouldn't) because it already has a value in there.

The expected behavior for this field looks like a Observation field, where it just mirrors the value from the API, which is a value not defined by the user. A super common example of an Observation field would be a resource ID.

But when I tried to look to the CloudSQL API documentation, I saw that this field is part of body requests, which made me confused.

We can create replicas pointing to a master using spec.masterInstanceName, but we don't need to specify in the master desired state (the YAML that represents the master instance) which replicas it has? Maybe we should move this field to status? I would love some input from other maintainer, what do you think, @turkenh ?

Should we always define spec.masterInstanceName for replica instances spec and also change the spec.ReplicaNames array from the master spec to make it "consistent"? Or should we move spec.ReplicaNames to the status struct?

[fahedouch]

hi @Feggah ,

thank you for the review.

initialize optional fields that are empty in K8S API but are filled in the provider API

this sentence is missing a part ( from https://github.com/crossplane/crossplane-runtime/blob/master/pkg/reconciler/managed/reconciler.go#L312-L315) :

Crossplane provider may update a
// managed resource's spec fields after it is created or updated, as long as
// the updates are limited to setting previously unset fields, and adding
// keys to maps

I guess we consider adding values ​​to slices also here ( this should be added to the comment above )

[Feggah]

I guess we consider adding values ​​to slices also here ( this should be added to the comment above )

That's interesting. I saw that the documentation that you linked is saying that we can add keys to maps on late initialization, but it isn't saying to add items to an array. I don't know why this isn't there, so I would like some input from other older maintainer to understand how can we solve this problem, @fahedouch . Maybe @hasheddan or @negz could help me here?

[fahedouch]

@Feggah @hasheddan @negz
looking at this https://github.com/kubernetes/community/blob/db7f270f/contributors/devel/sig-architecture/api-conventions.md#late-initialization , I think we should add patchStrategy:"merge" to ReplicaNames field to be able to add data to ReplicaNames slice with lateinitialize :

	// ReplicaNames: The replicas of the instance.
	// +optional
	// +patchStrategy=merge
	ReplicaNames []string `json:"replicaNames,omitempty" patchStrategy:"merge"`  

WDYT ?

[negz]

WDYT ?

If I recall correctly, this isn't supported for CRDs, but only built-in Kubernetes types. Is that no longer the case?

[negz]

I had a little more time to read this thread today. Without having familiarity with the CloudSQLInstance field this PR addresses, I can say that @Feggah is right that it's not safe to late initialize array elements.

When a Crossplane provider reconciles a managed resource the process is:

1. Observe the actual state of the external resource (if any).
2. Late initialize the actual state into the desired state.
3. Determine if the actual state is different from the desired state.
4. If necessary, update the actual state to match the desired state.

During step 2 we're careful to only copy actual to desired state when the desired state was empty - i.e. when the creator of the managed resource had no opinions about that particular setting. The clusterIP of a Kubernetes Service is the quintessential example of late init - if you set it, that's the desired state. If you don't, Kubernetes picks one for you and that becomes the desired state.

When an array has (e.g.) one element in the desired state, we have to assume the creator of the array is explicitly stating that they want one element - not two, three, etc and that Crossplane should keep trying to make sure there's only one element regardless of what happens to the actual state (e.g. via someone editing it outside of Crossplane).

I think we should add patchStrategy:"merge" to ReplicaNames field to be able to add data to ReplicaNames slice with lateinitialize

This is related to initializing objects that are array elements. In some rare cases we do this already. We never late init entire elements into the array, but we might late init a field of an object that is itself an array element if we have a reliable way of matching array elements. For example:

spec:
  widgets:
  - name: first-widget
     size: big  # We might late init this field if it's optional, was not specified, and we can merge array elements by name.
  # We would never late init an entire element into this array.
  - name: second-widget

[fahedouch]

@negz
that seems very logical to me. Thank you you are clarification.
I close the ticket