Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

1.14.5 Native patching behavior change - XR desired state dropped if a resource won't get all required schema values #5292

Closed
y4ri opened this issue Jan 26, 2024 · 9 comments · Fixed by #5365

Comments

@y4ri
Copy link

y4ri commented Jan 26, 2024

What happened?

When one of the resources in composition has a patch to a field that in its schema is required, but the patch lacks fromFieldPath: Required policy, XR desired state from other resources patches will be dropped which can lead to deadlock and stopping composition progress.

This behavior changed, in 1.13.x the resource that lacks required field fails but the desired state of other patches to the XR would be saved and the resource would be created on next reconciliation.

How can we reproduce it?

Apply manifests below, xrd->compositions->claim

On 1.14.5 it will create second XDummy and in summary 2 DeploymentRuntimeConfig only after adding fromFieldPath: Required patch for required value .

Example demo xrd, compositions and claim file:

apiVersion: apiextensions.crossplane.io/v1
kind: CompositeResourceDefinition
metadata: 
  name: xdummys.example.org
spec:
  defaultCompositionRef:
    name: test-comp-two
  group: example.org
  names:
    kind: XDummy
    plural: xdummys
  claimNames:
    kind: Dummy
    plural: dummys
  versions:
  - name: v1alpha1
    served: true
    referenceable: true
    schema:
      openAPIV3Schema:
        type: object
        properties:
          spec:
            type: object
            properties:
              exampleField:
                type: string  
            required:
            - exampleField
apiVersion: apiextensions.crossplane.io/v1
kind: Composition
metadata:
  name: test-comp-one
spec:
  compositeTypeRef:
    apiVersion: example.org/v1alpha1
    kind: XDummy
  resources:
    - name: druntime-one
      base:
        apiVersion: pkg.crossplane.io/v1beta1
        kind: DeploymentRuntimeConfig
        metadata:
          annotations:
            crossplane.io/external-name: druntime-one
        spec: {}
      patches:
      # desired state of this patch will be saved as it is patching MR
      - type: FromCompositeFieldPath
        fromFieldPath: spec.exampleField
        toFieldPath: metadata.annotations.stringFromClaim
      # this one will fail to be saved
      - type: ToCompositeFieldPath
        fromFieldPath: metadata.name
        toFieldPath: metadata.annotations.exampleVal

    - name: second-xdummy
      base:
        apiVersion: example.org/v1alpha1
        kind: XDummy
        metadata:
          annotations:
            crossplane.io/external-name: dummy-two
        spec: {}
      patches:
      - type: FromCompositeFieldPath
        fromFieldPath: metadata.annotations.exampleVal
        toFieldPath: spec.exampleField
        # try without this policy first, 1.13.2 succeeds, 1.14.5 deadlocks
        # policy:
        #   fromFieldPath: Required
apiVersion: apiextensions.crossplane.io/v1
kind: Composition
metadata:
  name: test-comp-two
spec:
  compositeTypeRef:
    apiVersion: example.org/v1alpha1
    kind: XDummy
  resources:
    - name: druntime-two
      base:
        apiVersion: pkg.crossplane.io/v1beta1
        kind: DeploymentRuntimeConfig
        metadata:
          annotations:
            crossplane.io/external-name: druntime-two
        spec: {}
      patches:
      - type: ToCompositeFieldPath
        fromFieldPath: metadata.name
        toFieldPath: metadata.annotations.exampleVal
apiVersion: example.org/v1alpha1
kind: Dummy
metadata:
  name: test
spec:
  exampleField: "test string"
  compositionRef:
    name: test-comp-one

What environment did it happen in?

Crossplane version: 1.14.5

Could replicate on eks 1.24 as well as 1.27 kind cluster.

@y4ri y4ri added the bug Something isn't working label Jan 26, 2024
@negz
Copy link
Member

negz commented Jan 26, 2024

@y4ri Do you see any errors or warnings when this is happening? If so, could you let us know what they are.

@y4ri
Copy link
Author

y4ri commented Jan 27, 2024

1.14.5 - On XR level just a bunch of

Warning  ComposeResources         13s                defined/compositeresourcedefinition.apiextensions.crossplane.io  cannot compose resources: cannot apply composed resource: cannot create object: XDummy.example.org "test-qksw8-dlpcx" is invalid: spec.exampleField: Required value

ending with

Warning  ComposeResources         9s (x14 over 13s)  defined/compositeresourcedefinition.apiextensions.crossplane.io  (combined from similar events): cannot compose resources: cannot apply composed resource: cannot create object: XDummy.example.org "test-qksw8-m96gr" is invalid: spec.exampleField: Required value

On 1.13.2 I only get 1 warrning:

Warning  ComposeResources         21s                defined/compositeresourcedefinition.apiextensions.crossplane.io  composed resource "second-xdummy": cannot use dry-run create to name composed resource: XDummy.example.org "test-jf7q7-5nswr" is invalid: spec.exampleField: Required value

@xoanmi
Copy link

xoanmi commented Feb 5, 2024

We have the same problem. We have not been able to work around it. Did you do it @y4ri ?

@y4ri
Copy link
Author

y4ri commented Feb 6, 2024

@xoanmi just like in "test-comp-one" composition above, you need to add

policy:
  fromFieldPath: Required

to any patches of XRs/MRs in composition that patch in the value required by schema.

With an example of VPC Managed Resource from aws provider you would need that policy for a patch responsible for value of spec.forProvider.cidrBlock

@xoanmi
Copy link

xoanmi commented Feb 6, 2024

@xoanmi just like in "test-comp-one" composition above, you need to add

policy:
  fromFieldPath: Required

to any patches of XRs/MRs in composition that patch in the value required by schema.

With an example of VPC Managed Resource from aws provider you would need that policy for a patch responsible for value of spec.forProvider.cidrBlock

Thanks! I misread your previous message. We have applied the workaround and it works! 🚀

@phisco
Copy link
Contributor

phisco commented Feb 9, 2024

I think this is because we used to dry run here to get the generated name and the code below still assumes the resources to be valid, which in case of a missing required field is not the case anymore and therefore we exit erroring here.
This applies to all 1.14 releases and to v1.15.0-rc.1 too.

@phisco
Copy link
Contributor

phisco commented Feb 12, 2024

I think this is a duplicate of #5114

@jbw976 jbw976 added this to the v1.15 milestone Feb 12, 2024
@negz
Copy link
Member

negz commented Feb 12, 2024

I think this is a duplicate of #5114

@phisco How sure are you? At a glance it sounds like the described behaviour is different.

@phisco
Copy link
Contributor

phisco commented Feb 12, 2024

@negz, pretty sure, the example here is extremely complicated, but the core issue is the same, patches to the xr not being executed because of a failure applying a composed resource.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment