🐛 Fix panic when using CRs with embedded pointer structs

[alvaroaleman]

This change bumps structured-merge-diff to pick up a fix for it panicing when encountering objects with embedded pointer structs that are nil: kubernetes-sigs/structured-merge-diff#309

This makes us use a newer version of structured-merge-diff than upstream, but for one release that seems okay, especially given that the only usage is in the fake client.

Fixes #3418
/assign @sbueringer
/cherrypick release-0.23

[sbueringer]

Is this only for main or is the idea to backport it?

(For a backport I wonder if this might break some folks somehow, I think they could bump directly by themselves if they want)

[alvaroaleman]

Is this only for main or is the idea to backport it?

(For a backport I wonder if this might break some folks somehow, I think they could bump directly by themselves if they want)

Backport to 0.23. How do you think this could break anyone?

[sbueringer]

Backport to 0.23. How do you think this could break anyone?

I don't know if it breaks someone but downstream consumers will inherit sigs.k8s.io/structured-merge-diff/v6 v6.3.2-0.20260122202528-d9cc6641c482 instead of sigs.k8s.io/structured-merge-diff/v6 v6.3.0.

That means they would get all the changes that happened between those two commits. Given that this library might also be used by downstream consumers outside of the usage in controller-runtime I wonder if they end up with different behavior

(This seems to be the diff: kubernetes-sigs/structured-merge-diff@v6.3.0...d9cc664)

Just not sure if it's worth the risk in a CR patch release considering we are only fixing the fake client. And consumers of CR can also use a replace to bump this dependency if they want.

[alvaroaleman]

Just not sure if it's worth the risk in a CR patch release considering we are only fixing the fake client. And consumers of CR can also use a replace to bump this dependency if they want.

I guess my assumption here is that the odds that this breaks something else are very low and then downstream consumers can still overwrite it. But if you feel strongly about not wanting to backport, I won't insist

[sbueringer]

No strong opinion, your call. Just wanted to bring it up at least

(Folks can also use replace to pin back to v6.3.0 if they run into issues)

/lgtm
/approve
(definitely fine for main)

[k8s-ci-robot]

LGTM label has been added.

Details

Git tree hash: cb9c17de14d9fc62e35bf227352ec554379148c4

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, sbueringer

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [alvaroaleman,sbueringer]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[alvaroaleman]

I do feel its worth it because the fake client becomes unusable for someone with CRDs that have this in both 0.22 and 0.23

[k8s-infra-cherrypick-robot]

@alvaroaleman: new pull request created: #3436

Details

In response to this:

This change bumps structured-merge-diff to pick up a fix for it panicing when encountering objects with embedded pointer structs that are nil: kubernetes-sigs/structured-merge-diff#309

This makes us use a newer version of structured-merge-diff than upstream, but for one release that seems okay, especially given that the only usage is in the fake client.

Fixes #3418
/assign @sbueringer
/cherrypick release-0.23

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[sbueringer]

Sounds good. Strange that we didn't see any issues with CR v0.22 or v0.23 in Cluster API. We don't have a lot of struct pointer fields, but still some. Or does it only happen with a top-level pointer?

[alvaroaleman]

Sounds good. Strange that we didn't see any issues with CR v0.22 or v0.23 in Cluster API. We don't have a lot of struct pointer fields, but still some. Or does it only happen with a top-level pointer?

Its not just a struct pointers, its an embedded struct pointer and must have the json",inline" marker. Do you have tests where such an object is passed to the fakeclient? If yes and that didn't cause the panic we should have a look.

[sbueringer]

Its not just a struct pointers, its an embedded struct pointer and must have the json",inline" marker. Do you have tests where such an object is passed to the fakeclient? If yes and that didn't cause the panic we should have a look.

Ah thx. That's it. I somehow missed the "embedded" part. We have no cases of embedded structs (apart from TypeMeta and ObjectMeta but both are not pointers or don't use inline)