Compositions should allow insertion, deletion, and reordering of resources

[negz]

What problem are you facing?

Each time Crossplane reconciles a Composite Resource (XR) it fetches the Composition the XR references and uses it to render a series of composed resources. The names of these composed resources are usually non-deterministic; Crossplane sets metadata.generateName to request that the API server name the composed resource at create time. The the types and names of composed resources are recorded as an array of references (spec.resourceRefs) on the XR.

Currently Crossplane’s composition logic assumes that the spec.resources array of composed resource templates in a Composition is append-only. This is because after the composed resources are initially created Crossplane matches templates to existing composed resources by matching their indexes; the template at spec.resources[0] of the Composition is assumed to always correspond to spec.resourceRefs[0] in the XR. This means:

• It's not possible to reorder a Composition's spec.resources array.
• It's not possible to delete an element from the spec.resources array.
• It's not possible to add a new template to the spec.resources array, except at the end.

Note that above would all still be true under the CompositionRevision proposal in #1481, which simply snapshots each update to a Composition as a distinct revision to avoid XRs being forced to update whenever a Composition is updated.

How could Crossplane help solve your problem?

I can see a few ways to address this:

1. We could declare that Composition resources and the spec.compositionRef of an XR are immutable. This would simplify our implementation at the expense of reduced flexibility for composition users. It would not be possible to change how an XR should be composed without deleting and recreating it.
2. We could formalise the current behaviour by declaring that the spec.resources array of a Composition is append only. Presumably an XR's spec.compositionRef would either need to be immutable, or required to only ever be updated to a Composition with an appended version of the current Composition's spec.resources.
3. We could ask Composition authors to supply a deterministic identifier for each entry in the Composition's spec.resources, and use that to correlate templates to each of the XR's spec.resourceRefs.

[negz]

My preference is to go with option 3 - supplying a deterministic identifier at the Composition level. For example the GCP Composition from our current documentation would become:

apiVersion: apiextensions.crossplane.io/v1alpha1
kind: Composition
metadata:
  name: compositepostgresqlinstances.gcp.database.example.org
  labels:
    provider: gcp
    guide: quickstart
spec:
  writeConnectionSecretsToNamespace: crossplane-system
  compositeTypeRef:
    apiVersion: database.example.org/v1alpha1
    kind: CompositePostgreSQLInstance
  resources:
    # The 'id' field is new here. It uniquely identifies this entry in the
    # resources array so that we can easily tell when said entry is reordered or
    # deleted.
    - id: instance
      base:
        apiVersion: database.gcp.crossplane.io/v1beta1
        kind: CloudSQLInstance
        spec:
          forProvider:
            databaseVersion: POSTGRES_9_6
            region: us-central1
            settings:
              tier: db-custom-1-3840
              dataDiskType: PD_SSD
              ipConfiguration:
                ipv4Enabled: true
                authorizedNetworks:
                  - value: "0.0.0.0/0"
          writeConnectionSecretToRef:
            namespace: crossplane-system
      patches:
        - fromFieldPath: "metadata.uid"
          toFieldPath: "spec.writeConnectionSecretToRef.name"
          transforms:
            - type: string
              string:
                fmt: "%s-postgresql"
        - fromFieldPath: "spec.parameters.storageGB"
          toFieldPath: "spec.forProvider.settings.dataDiskSizeGb"
      connectionDetails:
        - fromConnectionSecretKey: username
        - fromConnectionSecretKey: password
        - fromConnectionSecretKey: endpoint
        - name: port
          value: "5432"

We'd persist spec.resources[].name to any XR that used the Composition to create its composed resources, e.g.:

apiVersion: database.example.org/v1alpha1
kind: CompositePostgreSQLInstance
metadata:
  name: my-db-mb4wz
spec:
  claimRef:
    apiVersion: database.example.org/v1alpha1
    kind: PostgreSQLInstance
    name: my-db
    namespace: default
  compositionRef:
    name: compositepostgresqlinstances.gcp.database.example.org
  compositionSelector:
    matchLabels:
      provider: gcp
  parameters:
    storageGB: 20
  resourceRefs:
  # This 'id' field corresponds to the same field in the Composition's resources
  # array, allowing us to correlate templates with existing composed resources
  # that should be updated according to that template.
  - id: instance
    apiVersion: database.gcp.crossplane.io/v1beta1
    kind: CloudSQLInstance
    name: my-db-mb4wz-mhklc
    uid: 5f0ae03b-bdf7-4451-9662-f875197a69e6
  writeConnectionSecretToRef:
    name: 2af7e36c-3385-45dd-85a2-ac522f690bae
    namespace: crossplane-system

I believe it would be possible to implement this without a breaking API change to either resource. To do so we would:

1. Make the new id field optional, but require that it be supplied for either all or no spec.resources elements.
2. Fall back to the current behaviour if no id fields are supplied; i.e. treat the array index as the id.

[muvaf]

Not having an identifier made implementation of harder as well since we'd use the index in the array as identifier, which comes with its own complications. In fact, it came up during selectors discussion where we wanted to make it easier for people to select the one that's in the same composition but that's solved in another way. In the end, I wasn't able to find enough justifications for it.

It's not possible to reorder a Composition's spec.resources array.
It's not possible to delete an element from the spec.resources array.
It's not possible to add a new template to the spec.resources array, except at the end.

I feel like from user's perspective, the order of the elements is not really relevant. It's only relevant to controllers making sure everything is provisioned and updated correctly. As the user, I wouldn't really care as long as all of them are created & updated properly.

By (2), do you mean that user would want to delete an entry from Composition.spec.resources and expect the corresponding provisioned composed resources to be deleted from the tree of all composite resources that use that Composition?

[negz]

I feel like from user's perspective, the order of the elements is not really relevant. It's only relevant to controllers making sure everything is provisioned and updated correctly. As the user, I wouldn't really care as long as all of them are created & updated properly.

I find it very hard to identify with that perspective. Assume for example I had a Composition with an SQL server, three SQL server firewall rules, and an EKS cluster. If I wanted to add a fourth SQL server firewall rule I would intuitively want to add it near the other firewall rules, not to be forced to append it to my file. To me configuration is no different than writing code in this way; if I'm adding a new function to my code I want to be able to add it near all the related functions, not forced to always add it at the end of the file.

By (2), do you mean that user would want to delete an entry from Composition.spec.resources and expect the corresponding provisioned composed resources to be deleted from the tree of all composite resources that use that Composition?

Pretty much, though I want to implement #1481 so that the XR author can choose to roll forward to a new Composition revision when they are ready, rather than their XR being instantly updated. In general though I do think it should be possible to remove a composed resource by updating a Composition.

It's also worth noting at the moment that there are two issues with removing a resource from a Composition:

• The composed resources don't actually get deleted.
• Deleting the entry from the resources array breaks our assumption that the resources array will be identical in order to the XR's resourceRefs array.