🐛 pkg/crd: reject pointer-receiver-only TextMarshaler types as map keys

[camilamacedo86]

encoding/json marshals map keys by value, not by pointer. Map keys are not addressable in Go's reflect model, so if a type implements MarshalText only via a pointer receiver, encoding/json returns an error at runtime for any map that uses it as a key.

PR #1419 introduced the TextMarshaler map key check using the implements() helper, which accepts both value and pointer receivers. This caused controller-gen to accept types that would fail at runtime. Fix both occurrences in mapToSchema to use types.Implements directly, which checks value receivers only.

The testdata example had the same problem: URL3 implements MarshalText only via a pointer receiver, so map[URL3]string would have failed at runtime. Replace it with a new TextMarshalerKey type that uses a value receiver and actually works. Add a Ginkgo test suite covering all three cases: accepted (value receiver), rejected (pointer receiver only), and rejected (no TextMarshaler at all).

[camilamacedo86]

Hi @HadrienPatte,

I was reviewing your PR and found a scenario that I think is worth addressing before this reaches users.

The check in mapToSchema currently uses the implements() helper, which accepts both value and pointer receivers. That works well for struct fields because they are addressable in memory, allowing encoding/json to call a pointer-receiver MarshalText implementation when needed.

Map keys are a bit different, though. Go's reflect package does not allow taking the address of a map key, which means encoding/json cannot invoke a pointer-receiver method on them. As a result, if someone defines a type like URL3 (implementing MarshalText only via *URL3) and uses it as a map key, controller-gen will accept it, but json.Marshal will fail at runtime with an unsupported type error.

The test data in the PR currently demonstrates exactly this case. URL3 implements encoding.TextMarshaler only through *URL3, so map[URL3]string would not actually work at runtime.

To make the behavior align with what encoding/json supports, I opened a follow-up PR that:

• Fixes both implements() calls in mapToSchema to use types.Implements (value receivers only)
• Replaces URL3 in the test data with a new TextMarshalerKey type that implements MarshalText with a value receiver
• Adds a Ginkgo test that explicitly verifies pointer-receiver-only types are rejected

Thanks for adding this feature — the use case itself makes a lot of sense. I think it just needed this additional guard to ensure we only accept types that will actually work at runtime.

Let me know what you think or if you'd like to discuss any of the details.

@alvaroaleman please, feel free to check as well and let me know wdyt.

[HadrienPatte]

Thanks for catching that nuance 🙏

In my PR I used url.URL3 in the tests which indeed only implements encoding.TextMarshaler as a pointer receiver, but in the associated issue #1420, I described the real-life use case that prompted me to come up with that PR which was using a netip.Addr as a map key, and that type does implement encoding.TextMarshaler as a value receiver.

[k8s-ci-robot]

@HadrienPatte: changing LGTM is restricted to collaborators

Details

In response to this:

Thanks for catching that nuance 🙏

In my PR I used url.URL3 in the tests which indeed only implements encoding.TextMarshaler as a pointer receiver, but in the associated issue #1420, I described the real-life use case that prompted me to come up with that PR which was using a netip.Addr as a map key, and that type does implement encoding.TextMarshaler as a value receiver.

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.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: camilamacedo86, HadrienPatte
Once this PR has been reviewed and has the lgtm label, please assign joelanford for approval. For more information see the Code Review Process.

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

Details

Needs approval from an approver in each of these files:

• OWNERS

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

[camilamacedo86]

Hi @HadrienPatte

That is great news. IMO the real use case from issue #1420, using netip.Addr as a map key, is totally fine. netip.Addr implements TextMarshaler via a value receiver, so it works correctly with json.Marshal and your fix still accepts it.

The problem is the testdata. URL3 only implements TextMarshaler via *URL3, so map[URL3]string would have failed at runtime. I am proposing to change that in this follow-up. Are you ok with it?

I also moved the tests to Ginkgo since it is the project standard.
I would love to get your help on the review. 🙌

Thank you so much for the nice contribution 🥇

[camilamacedo86]

Hi @sbueringer @alvaroaleman Could we get this one merged to fix the issue ?
The author of the PR @HadrienPatte already help us in the review here and approve this one.

Thx

[camilamacedo86]

@JoelSpeed this one has a LGTM/approval from the author already.
WDYT? Could we move forward here?

[alvaroaleman]

I think its basically irrelevant if we error here or not, people will notice this doesn't work the first time they try to json marshal their api type

[camilamacedo86]

Hi @alvaroaleman and @sbueringer,

Thank you for looking on this one.

I agree that the impact is probably low, since users will eventually get an error when the object is marshaled.

That said, I still don’t see a strong reason to reject this PR.

This change:

• Makes controller-gen validation match the actual behavior of the Go JSON encoder.
• Improves the developer experience by reporting the problem where the API type is defined, instead of later at runtime.
• Provides a clearer and more actionable error message.
• Helps catch the issue before it reaches tests, controllers, or the API server.
• Prevents confusing situations where the code compiles and some tests pass, but serialization fails later when a specific code path is exercised.
• Makes the validation more consistent and trustworthy. If controller-gen says a type is valid, developers should be able to rely on that.
• Is a small, targeted, and low-risk correctness fix.
• Does not break valid APIs. It only rejects types that cannot be serialized successfully anyway.

Moreover:

• this PR also updates the tests to use Ginkgo, keeping them consistent with the testing style used across the project. Using the same testing framework everywhere improves consistency, readability, and long-term maintainability.
• it is approved for original author as well. 🐛 pkg/crd: reject pointer-receiver-only TextMarshaler types as map keys #1441 (review)

I see it is as reasonable follow up of PR #1419 to address what we did not catcher in time and to shape it better. Then, given all of the above, I'm trying to understand what is still preventing this PR from moving forward.

Could you please clarify what the remaining concerns are? Is there something specific in the implementation that should be changed, or is there another reason you don't think this fix should be merged?

I'd really appreciate understanding what is missing for this PR to receive an approval/LGTM.

[camilamacedo86]

Hi @alvaroaleman and @sbueringer and @JoelSpeed

I think this one should get merged before the next release as well.
So that we ensure the changes / follow up accordingly.

Could we get this one merged? WDYT?

[sbueringer]

I don't think this PR should block or defer the next CR minor release

[JoelSpeed]

What is the full error message here?