Consume latest crossplane-runtime's fake.Composite to prevent panics caught by CNCF fuzzing tests #3394

ulucinar · 2022-10-18T07:06:50Z

Description of your changes

Depends on: crossplane/crossplane-runtime#361

CNCF Crossplane fuzzing tests are currently failing (specifically the fuzz_patch_apply test) because if we pass a resource.Composite with a nil connection details last published time to Patch.Apply, subsequent calls to runtime.DefaultUnstructuredConverter.ToUnstructured to convert it to an unstructured.Unstructured panic. This PR proposes a change to the Patch.Apply signature so that we make sure the first arg is actually a resource.Composite and consumes a new implementation from crossplane-runtime's fake.Composite, which annotates the fake.ConnectionDetailsLastPublishedTimer.Time field with an omitempty json tag so that runtime.DefaultUnstructuredConverter.ToUnstructured does not attempt to convert it.

The function signature change has been motivated by observing that the fuzzer passes a resource.Composed as the first argument to Patch.Apply but what Patch.Apply expects is a resource.Composite and we can enforce this at compile-time.

Note: I've also reported an upstream issue here.

I have:

Read and followed Crossplane's contribution process.
Run make reviewable to ensure this PR is ready for review.
Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

Tested against the fuzz_patch_apply fuzzer using the provided minimized testcase.

Also tested with the following XRD, composition and claim:

apiVersion: apiextensions.crossplane.io/v1
kind: CompositeResourceDefinition
metadata:
  name: xmyresources.test.com
spec:
  group: test.com
  names:
    kind: XMyResource
    plural: xmyresources
  claimNames:
    kind: MyResource
    plural: myresources
  versions:
  - name: v1alpha1
    served: true
    referenceable: true
    schema:
      openAPIV3Schema:
        type: object
        properties:
          spec:
            type: object
            properties:
              parameters:
                type: object
                properties:
                  tagValue:
                    type: string
                required:
                - tagValue
            required:
            - parameters

---

apiVersion: apiextensions.crossplane.io/v1
kind: Composition
metadata:
  name: example
  labels:
    purpose: example
spec:
  compositeTypeRef:
    apiVersion: test.com/v1alpha1
    kind: XMyResource
  resources:
    - name: myrole
      base:
        apiVersion: azure.upbound.io/v1beta1
        kind: ResourceGroup
        spec:
          forProvider:
            location: "West Europe"
            tags:
              key: ""
      patches:
        - fromFieldPath: "spec.parameters.tagValue"
          toFieldPath: spec.forProvider.tags["key"]

---

apiVersion: test.com/v1alpha1
kind: MyResource
metadata:
  namespace: upbound-system
  name: my-resource
spec:
  parameters:
    tagValue: demo-test
  compositionRef:
    name: example

muvaf · 2022-10-18T08:29:29Z

apis/apiextensions/v1/composition_patches.go

+	// runtime.DefaultUnstructuredConverter.ToUnstructured
+	// calls to convert the Composite to an Unstructured
+	// panic.
+	if cp.GetConnectionDetailsLastPublishedTime() == nil {


It's no-op for the function signature change but I couldn't catch why we can't have a nil published time. Is runtime.DefaultUnstructuredConverter.ToUnstructured not able to work with nil time values at all or is it about how we call it? I'm pretty sure we have time fields in other types that may be nil in certain cases.

Yes, correct, the runtime.DefaultUnstructuredConverter.ToUnstructured cannot handle the nested nil *Time field and fails with:

panic: value method k8s.io/apimachinery/pkg/apis/meta/v1.Time.ToUnstructured called using nil *Time pointer goroutine 17 [running, locked to thread]: k8s.io/apimachinery/pkg/apis/meta/v1.(*Time).ToUnstructured(0x0) <autogenerated>:1 +0xf1 sigs.k8s.io/structured-merge-diff/v4/value.TypeReflectCacheEntry.ToUnstructured({0x1, 0x0, 0x0, 0x0, 0x1, 0x0, 0x0, {0x0, 0x0, 0x0}}, ...) sigs.k8s.io/structured-merge-diff/v4@v4.2.1/value/reflectcache.go:188 +0x127 k8s.io/apimachinery/pkg/runtime.toUnstructured({0x2b36fc0?, 0x10c000020a78?, 0x263f805?}, {0x299e7e0?, 0x10c0007e4590?, 0x10c000836270?}) k8s.io/apimachinery@v0.24.0/pkg/runtime/converter.go:655 +0x1e59 k8s.io/apimachinery/pkg/runtime.structToUnstructured({0x29e2d40?, 0x10c000020a78?, 0x64097c?}, {0x299e7e0?, 0x10c0007e4520?, 0x2b5e770?}) k8s.io/apimachinery@v0.24.0/pkg/runtime/converter.go:843 +0x1685 k8s.io/apimachinery/pkg/runtime.toUnstructured({0x29e2d40?, 0x10c000020a78?, 0x263f805?}, {0x299e7e0?, 0x10c0007e4520?, 0x98?}) k8s.io/apimachinery@v0.24.0/pkg/runtime/converter.go:692 +0x1d08 k8s.io/apimachinery/pkg/runtime.structToUnstructured({0x2ad9880?, 0x10c000020900?, 0x0?}, {0x29b48e0?, 0x10c000844558?, 0x0?}) k8s.io/apimachinery@v0.24.0/pkg/runtime/converter.go:843 +0x1685 k8s.io/apimachinery/pkg/runtime.toUnstructured({0x2ad9880?, 0x10c000020900?, 0x10c000020900?}, {0x29b48e0?, 0x10c000844558?, 0x0?}) k8s.io/apimachinery@v0.24.0/pkg/runtime/converter.go:692 +0x1d08 k8s.io/apimachinery/pkg/runtime.(*unstructuredConverter).ToUnstructured(0x3f76840, {0x2b35700?, 0x10c000020900}) k8s.io/apimachinery@v0.24.0/pkg/runtime/converter.go:586 +0xc2e github.com/crossplane/crossplane/apis/apiextensions/v1.(*Patch).applyFromFieldPathPatch(0x10c0005014a0, {0x2b432d8?, 0x10c000020900}, {0x2b432b0, 0x10c000597c20}) github.com/crossplane/crossplane/apis/apiextensions/v1/composition_patches.go:224 +0x1b2 github.com/crossplane/crossplane/apis/apiextensions/v1.(*Patch).Apply(0x10c0005014a0, {0x2b432b0, 0x10c000597c20}, {0x2b432d8, 0x10c000020900}, {0x0, 0x0, 0x0?}) github.com/crossplane/crossplane/apis/apiextensions/v1/composition_patches.go:131 +0x688 github.com/crossplane/crossplane/apis/apiextensions/v1.FuzzPatchApply({0x603000001900, 0x16, 0x16}) github.com/crossplane/crossplane/apis/apiextensions/v1/patch_fuzzer.go:84 +0x61e main.LLVMFuzzerTestOneInput(...) ./main.3877846031.go:21

I tested the PR with a Composite resource, and as you mentioned above @muvaf, our composite.Unstructured can bypass this panic with a shortcut taken in unstructuredConverter.ToUnstructured, i.e., because Crossplane's composite.Unstructured implements runtime.Unstructured, the map is directly returned without attempting a conversion, and hence no panic occurs. This explains why we do not observe this panic in runtime.DefaultUnstructuredConverter.ToUnstructured with crossplane-runtime's composite.Unstructured implementation. Thus, it turns out that a nil value here is a valid case. I will adapt the PR accordingly. Thank you :)

As I suspected, adding the omitempty annotation to fake.ConnectionDetailsLastPublishedTimer.Time field did the job. I will also check the behavior in the latest apimachinery runtime and see if there are any existing issues there.

I also did the following simple two tests to confirm the observed behavior in a simpler setting:

The following program:

package main import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" ) type Test struct { Time *metav1.Time } func main() { if _, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&Test{}); err != nil { panic(err) } }

panics as follows:

panic: value method k8s.io/apimachinery/pkg/apis/meta/v1.Time.ToUnstructured called using nil *Time pointer goroutine 1 [running]: k8s.io/apimachinery/pkg/apis/meta/v1.(*Time).ToUnstructured(0x100000001?) <autogenerated>:1 +0x48 sigs.k8s.io/structured-merge-diff/v4/value.TypeReflectCacheEntry.ToUnstructured({0x1, 0x0, 0x0, 0x0, 0x1, 0x0, 0x0, {0x0, 0x0, 0x0}}, ...) /Users/alper/data/workspaces/go/pkg/mod/sigs.k8s.io/structured-merge-diff/v4@v4.2.1/value/reflectcache.go:188 +0x6b0 k8s.io/apimachinery/pkg/runtime.toUnstructured({0x100d10ba0?, 0x140000103e0?, 0x0?}, {0x100c92f60?, 0x14000021f30?, 0x0?}) /Users/alper/data/workspaces/go/pkg/mod/k8s.io/apimachinery@v0.24.0/pkg/runtime/converter.go:655 +0x8a0 k8s.io/apimachinery/pkg/runtime.structToUnstructured({0x100cb3260?, 0x140000103e0?, 0x100d135e0?}, {0x100c9e3c0?, 0x140000103e8?, 0x1007f5548?}) /Users/alper/data/workspaces/go/pkg/mod/k8s.io/apimachinery@v0.24.0/pkg/runtime/converter.go:843 +0x7ac k8s.io/apimachinery/pkg/runtime.toUnstructured({0x100cb3260?, 0x140000103e0?, 0x140000103e0?}, {0x100c9e3c0?, 0x140000103e8?, 0x140000021a0?}) /Users/alper/data/workspaces/go/pkg/mod/k8s.io/apimachinery@v0.24.0/pkg/runtime/converter.go:692 +0x7f4 k8s.io/apimachinery/pkg/runtime.(*unstructuredConverter).ToUnstructured(0x10100b530, {0x100c70a80?, 0x140000103e0}) /Users/alper/data/workspaces/go/pkg/mod/k8s.io/apimachinery@v0.24.0/pkg/runtime/converter.go:586 +0x364 main.main() /Users/alper/data/workspaces/github.com/ulucinar/crossplane/crossplane/cmd/test/main.go:13 +0x44

However, the following exits with a zero code:

package main import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" ) type Test struct { Time *metav1.Time `json:"time,omitempty"` } func main() { if _, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&Test{}); err != nil { panic(err) } }

I have observed the same behavior both with k8s.io/apimachinery@v0.24.0 (the dependency version in Crossplane current HEAD) and also with k8s.io/apimachinery@v0.25.3.

The problematic code that breaks runtime.DefaultUnstructuredConverter.ToUnstructured lives in sigs.k8s.io/structured-merge-diff/v4. I've reported the issue there.

Thanks @ulucinar ! I think the fix in crossplane-runtime is appropriate if it works - json tags should've been there anyway. Seems like you have a possible solution for the upstream issue as well, I'd say opening a PR there would help moving it forward faster.

Agreed. Thank you @muvaf.

- Consume crossplane-runtime where fake.ConnectionDetailsLastPublishedTimer.Time has an `omitempty` json tag so that we keep runtime.DefaultUnstructuredConverter.ToUnstructured happy. Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>

muvaf

Thanks @ulucinar !

ulucinar · 2022-10-20T13:02:06Z

Thank you @muvaf, for the excellent guidance in this PR.
Ran the minimized test case once more against Crossplane master HEAD with success.

ulucinar requested review from a team and muvaf as code owners October 18, 2022 07:06

ulucinar requested review from nullable-eth and MisterMX October 18, 2022 07:06

muvaf reviewed Oct 18, 2022

View reviewed changes

ulucinar mentioned this pull request Oct 18, 2022

Add omitempty json tag to fake.ConnectionDetailsLastPublishedTimer.Time field crossplane/crossplane-runtime#361

Merged

2 tasks

ulucinar force-pushed the fix-cncf-fuzzing branch from 3237278 to c312cce Compare October 18, 2022 11:17

ulucinar requested a review from a team as a code owner October 18, 2022 11:17

ulucinar changed the title ~~Check GetConnectionDetailsLastPublishedTime returns non-nil to prevent panics caught by CNCF fuzzing tests~~ Consume latest crossplane-runtime's fake.Composite to prevent panics caught by CNCF fuzzing tests Oct 18, 2022

ulucinar force-pushed the fix-cncf-fuzzing branch 2 times, most recently from 059bdd1 to d49d028 Compare October 20, 2022 07:58

ulucinar force-pushed the fix-cncf-fuzzing branch from d49d028 to 43b18d6 Compare October 20, 2022 08:01

muvaf approved these changes Oct 20, 2022

View reviewed changes

muvaf merged commit a1d9fc0 into crossplane:master Oct 20, 2022

ulucinar deleted the fix-cncf-fuzzing branch October 20, 2022 12:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consume latest crossplane-runtime's fake.Composite to prevent panics caught by CNCF fuzzing tests #3394

Consume latest crossplane-runtime's fake.Composite to prevent panics caught by CNCF fuzzing tests #3394

ulucinar commented Oct 18, 2022 •

edited

muvaf Oct 18, 2022

ulucinar Oct 18, 2022 •

edited

ulucinar Oct 18, 2022

ulucinar Oct 18, 2022

ulucinar Oct 18, 2022 •

edited

ulucinar Oct 18, 2022

muvaf Oct 19, 2022

ulucinar Oct 19, 2022

muvaf left a comment

ulucinar commented Oct 20, 2022

Consume latest crossplane-runtime's fake.Composite to prevent panics caught by CNCF fuzzing tests #3394

Consume latest crossplane-runtime's fake.Composite to prevent panics caught by CNCF fuzzing tests #3394

Conversation

ulucinar commented Oct 18, 2022 • edited

Description of your changes

How has this code been tested

muvaf Oct 18, 2022

Choose a reason for hiding this comment

ulucinar Oct 18, 2022 • edited

Choose a reason for hiding this comment

ulucinar Oct 18, 2022

Choose a reason for hiding this comment

ulucinar Oct 18, 2022

Choose a reason for hiding this comment

ulucinar Oct 18, 2022 • edited

Choose a reason for hiding this comment

ulucinar Oct 18, 2022

Choose a reason for hiding this comment

muvaf Oct 19, 2022

Choose a reason for hiding this comment

ulucinar Oct 19, 2022

Choose a reason for hiding this comment

muvaf left a comment

Choose a reason for hiding this comment

ulucinar commented Oct 20, 2022

ulucinar commented Oct 18, 2022 •

edited

ulucinar Oct 18, 2022 •

edited

ulucinar Oct 18, 2022 •

edited