Functions can't modify XR `metadata` or `spec` about crossplane HOT 13 OPEN

Storage to patch data between composed resources (without using the alpha environment)

I assume this is the major scenario especially since this is how the examples in the documentation are written.

I wonder if it's not actually a problem though within function-patch-and-transform and it's only a problem when the data is needed outside of the function? As long as the function executes the patches properly on every reconciliation, the internal copy of observed.composite.resource.metadata.annotations would be updated appropriately, making the data available to any FromCompositeFieldPath patches that reference it. The annotations would not be stored in the composite, but since they are reconstructed on every reconciliation it should still "work". The one remaining downside is that it makes debugging harder because it's not possible to examine the function-patch-and-transform stored state, the way it is in traditional P&T. So if the user is relying on the composite annotations being updated, it's a reversion from traditional P&T, but it might not affect the results of the P&T sequences.

from crossplane.

I just tried latest patch-and-transform v4.0.0 FromEnvironmentFieldPath/ToEnvironmentFieldPath and now it is working!

from crossplane.

@negz I think we still need to fix the underlying issue with being able to patch into metadata.annotations in a Composition Function. We do that all the time in our compositions, so moving them to functions is impossible without a complete rewrite unless we restore that functionality.

I can understand not wanting to allow modifications to spec, and possibly to other metadata attributes, but we really need metadata.annotations to be available.

from crossplane.

I think this is related to this thread in Slack: https://crossplane.slack.com/archives/CEG3T90A1/p1708724436065099

It seems like missing functionality to me - if we want Function P&T to be equivalent to P&T then we need to support altering the composite.metadata from functions. The next question becomes do we support altering all of metadata like we do today in traditional P&T or do we restrict it to annotations, which I think would cover most cases. @negz @phisco ?

from crossplane.

from crossplane.

I believe this is a symptom of #4581, which overall I think is a positive change. It will affect all functions, not only P&T.

One thing we could do to make this clearer would be to return an error if a function tries to mutate an XR's metadata or spec. I believe today we just drop the changes silently.

from crossplane.

I don't see anything in #4581 that would allow functions to modify composite metadata.annotations - it only references claim/composite annotation handling.

Storing data in the composite metadata.annotations is a widespread pattern in traditional patch and transform.

If functions are not allowed to mutate the composite metadata.anntotations then many existing P&T compositions cannot be migrated to functions without considerable rework of both the composite definition and the composition patches.

I think it's important to preserve existing P&T functionality to ease the transition to functions, so it makes sense to allow functions to mutate the composite metadata.annotations.

from crossplane.

Storing data in the composite metadata.annotations is a widespread pattern in traditional patch and transform.

@bobh66 Understood. Do you think we can enumerate what folks use this pattern for? I'm guessing:

• Storage to patch data between composed resources (without using the alpha environment)
• ???

I know that #4581 fixed a lot of subtle bugs in the flow of data from claim -> XR -> composed and back. Things like fields you deleted coming back, etc. I believe it also makes composition easier to reason about in general. Metadata and spec flow forward from the claim, status flows backward from the composed resources. No merging of data flowing in both directions to worry about.

If there's things that become impossible to do without a clear alternative, and it's possible to patch metadata back from composed resources to the XR without reintroducing subtle bugs I'm open to supporting it again.

That said, I wonder if there are alternatives with function-patch-and-transform. For example

• Using ToEnvironment to patch to the environment to pass data around, which in a function is stored in the pipeline's context.
• Using status fields, if the data is supposed to be user-facing.

from crossplane.

Hey @negz , yes we are using it as a storage in order to patch composed resources (passing one resource status field as an input to another resource)

I just tried use environments (together with function-path-and-transform) and it is not working either

in SecurityGroup resource I have below patch:

        - fromFieldPath: status.atProvider.securityGroupID
          toFieldPath: status_securityGroup_Id
          type: ToEnvironmentFieldPath

than on RDSInstance I would like to use that SG:

        - fromFieldPath: status_securityGroup_Id
          toFieldPath: spec.forProvider.vpcSecurityGroupIds[0]
          type: FromEnvironmentFieldPath

Than I am getting below error:

│   Warning  ComposeResources         2m54s (x3 over 2m57s)  defined/compositeresourcedefinition.apiextensions.crossp │
│ lane.io  cannot compose resources: cannot apply composed resource "rds": failed to create typed patch object (/mysq │
│ l-1-8zzjw-6zw4m; rds.aws.upbound.io/v1beta2, Kind=Instance): .spec.forProvider.vpcSecurityGroupIds: element 0: asso │
│ ciative list without keys has an element that's an explicit null

setting policy.toFieldPath: Optional didn't help

I could try to store it instead in CompositeResource.status property but it requires more efford and I would need to add this property to XRD which doesn't seem to me a good option as I don't want to expose it to the Claim as this is strictly internal configuration users shouldn't be aware of

I would be happy to use EnvironmentConfig if above case would work and if visibility/debugging goanna be improved (e.g. with old approach I could see if annotation was added, today with EnvironmentConfig it is stored in-memory so it is hard to investigate if things get's wrong)

Please let me know what is your recommendation we should follow

from crossplane.

cannot apply composed resource "rds": failed to create typed patch object (/mysq │
│ l-1-8zzjw-6zw4m; rds.aws.upbound.io/v1beta2, Kind=Instance): .spec.forProvider.vpcSecurityGroupIds: element 0: asso │
│ ciative list without keys has an element that's an explicit null

This error looks like it's coming from the Kubernetes API server. I haven't seen it before, but my guess is that at some point during the composition process:

1. status_securityGroup_Id isn't yet populated in the environment.
2. The FromEnvironmentFieldPath is returning null

Resulting in us trying to server-side apply an invalid resource, like:

spec:
  forProvider:
    vpcSecurityGroupIds:
    - null

This seems like it could be a bug somewhere - in FromEnvironmentFieldPath? An optional patch from a field path that doesn't exist should be skipped, not return a null value.

Just reading through the code in function-patch-and-transform I can't see how that would happen though:

• https://github.com/crossplane-contrib/function-patch-and-transform/blob/v0.3.0/patches.go#L67
• https://github.com/crossplane-contrib/function-patch-and-transform/blob/v0.3.0/fn.go#L233

I'd expect ApplyComposedPatch to return an error that satisfies fieldpath.IsNotFound without mutating the composed resource.

from crossplane.

From what I remember when I was running tests against Composition patch-and-transform when annotation was not present it was returning an empty string "", maybe that's the difference in implementation?

from crossplane.

@bobh66 Would that change be safe?

I remember when working on #5313 I went through all the bugs #4896 aimed to solve, and noticed that more of them were addressed by #4581 (i.e. strictly propagating metadata and spec claim -> XR -> MRs) than by server-side apply. Unfortunately I seem to have deleted the notes I took, so this concern is a bit hand wavy.

Maybe with the --enable-claim-ssa flag enabled SSA would be enough?

• Annotations would be SSA-ed claim -> XR
• Annotations would be SSA-ed composed resource -> XR
• Annotations would not back-propagate XR -> claim.

So the XR's annotations would be the set of annotations propagated forward from the claim, and backward from composed resources?

I don't love that this makes the mental model less "clean". i.e. We go from "metadata and spec flow forward, status flows backward", to "(most of) metadata and spec flow forward, status flows backward, but annotations and labels flow in both directions, but not past the XR".

CC @turkenh. I'd like to hear your thoughts on this as the one who proposed #4581.

from crossplane.

@negz I agree with your concerns and my only reason for wanting to do this is backward-compatibility. I think in an ideal world we would never have used metadata.annotations as a shared context/scratchpad, maybe using something like spec.crossplane.context instead, but here we are. I would be fine with the restriction if Function P&T had some way of handling the existing metadata.annotations patches and saving that context somewhere so it is available for the next reconciliation. Or maybe it can reconstruct the context from the patches - if we execute all of the ToCompositeFieldPath patches before we execute the FromCompositeFieldPath patches maybe that works? And we may not need to save the metadata.annotations context in that scenario? It would make debugging harder because the context is not saved anywhere.

from crossplane.