Skip to content

Commit a611713

Browse files
pkg/crd: reject pointer-receiver-only TextMarshaler types as map keys
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).
1 parent 77554a0 commit a611713

5 files changed

Lines changed: 149 additions & 113 deletions

File tree

pkg/crd/schema.go

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -383,19 +383,21 @@ func arrayToSchema(ctx *schemaContext, array *ast.ArrayType) *apiextensionsv1.JS
383383
}
384384
}
385385

386-
// mapToSchema creates a schema for items of the given map. Key types must eventually resolve
387-
// to string (other types aren't allowed by JSON, and thus the kubernetes API standards).
386+
// mapToSchema creates a schema for items of the given map. Key types must eventually
387+
// resolve to string or implement encoding.TextMarshaler via a value receiver. Pointer-receiver
388+
// implementations are not accepted: map keys are not addressable, so encoding/json cannot call
389+
// a pointer-receiver MarshalText on them and will return an error at runtime.
388390
func mapToSchema(ctx *schemaContext, mapType *ast.MapType) *apiextensionsv1.JSONSchemaProps {
389391
keyType := ctx.pkg.TypesInfo.TypeOf(mapType.Key)
390-
// check that we've got a type that actually corresponds to a string, or that
391-
// implements encoding.TextMarshaler (in which case it serializes to a string,
392-
// just like text-marshaler-implementing field types do).
392+
// Accept string-kinded types, or types whose value receiver implements
393+
// encoding.TextMarshaler (pointer-receiver-only types are rejected because map
394+
// keys are not addressable and encoding/json cannot call pointer methods on them).
393395
keyInfo := keyType
394396
for keyInfo != nil {
395397
switch typedKey := keyInfo.(type) {
396398
case *types.Basic:
397399
if typedKey.Info()&types.IsString == 0 {
398-
if implements(keyType, textMarshaler) {
400+
if types.Implements(keyType, textMarshaler) {
399401
keyInfo = nil // stop iterating
400402
break
401403
}
@@ -406,7 +408,7 @@ func mapToSchema(ctx *schemaContext, mapType *ast.MapType) *apiextensionsv1.JSON
406408
case *types.Named:
407409
keyInfo = typedKey.Underlying()
408410
default:
409-
if implements(keyType, textMarshaler) {
411+
if types.Implements(keyType, textMarshaler) {
410412
keyInfo = nil // stop iterating
411413
break
412414
}

pkg/crd/schema_test.go

Lines changed: 0 additions & 101 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@ import (
2626
pkgstest "golang.org/x/tools/go/packages/packagestest"
2727
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
2828
crdmarkers "sigs.k8s.io/controller-tools/pkg/crd/markers"
29-
"sigs.k8s.io/controller-tools/pkg/loader"
3029
testloader "sigs.k8s.io/controller-tools/pkg/loader/testutils"
3130
"sigs.k8s.io/controller-tools/pkg/markers"
3231
)
@@ -73,106 +72,6 @@ func transform(t *testing.T, expr string) *apiextensionsv1.JSONSchemaProps {
7372
return result
7473
}
7574

76-
// transformPackage loads pkgContents as a fake package and returns the schema
77-
// produced for the type definition named typeName, together with the package so
78-
// callers can inspect (or assert) any errors raised during schema generation.
79-
// Unlike transform, it does not fail the test when schema generation errors,
80-
// which lets negative cases assert that an error was produced.
81-
func transformPackage(t *testing.T, pkgContents, typeName string) (*apiextensionsv1.JSONSchemaProps, *loader.Package) {
82-
moduleName := "sigs.k8s.io/controller-tools/pkg/crd"
83-
modules := []pkgstest.Module{
84-
{
85-
Name: moduleName,
86-
Files: map[string]any{
87-
"test.go": pkgContents,
88-
},
89-
},
90-
}
91-
92-
pkgs, exported, err := testloader.LoadFakeRoots(pkgstest.Modules, modules, moduleName)
93-
if exported != nil {
94-
t.Cleanup(exported.Cleanup)
95-
}
96-
97-
if err != nil {
98-
t.Fatalf("unable to load fake package: %s", err)
99-
}
100-
101-
if len(pkgs) != 1 {
102-
t.Fatal("expected to parse only one package")
103-
}
104-
105-
pkg := pkgs[0]
106-
pkg.NeedTypesInfo()
107-
108-
schemaContext := newSchemaContext(pkg, nil, true, false).ForInfo(&markers.TypeInfo{})
109-
110-
var definedType ast.Expr
111-
for _, decl := range pkg.Syntax[0].Decls {
112-
genDecl, ok := decl.(*ast.GenDecl)
113-
if !ok {
114-
continue
115-
}
116-
for _, spec := range genDecl.Specs {
117-
typeSpec, ok := spec.(*ast.TypeSpec)
118-
if ok && typeSpec.Name.Name == typeName {
119-
definedType = typeSpec.Type
120-
}
121-
}
122-
}
123-
if definedType == nil {
124-
t.Fatalf("could not find type %q in fake package", typeName)
125-
}
126-
127-
return typeToSchema(schemaContext, definedType), pkg
128-
}
129-
130-
const textMarshalerKeyPackage = `
131-
package crd
132-
133-
// textKey implements encoding.TextMarshaler, so it serializes to a string
134-
// and is a valid (JSON string) map key.
135-
type textKey struct{}
136-
137-
func (textKey) MarshalText() ([]byte, error) { return nil, nil }
138-
139-
// nonStringKey is a struct that does not implement encoding.TextMarshaler.
140-
type nonStringKey struct{}
141-
142-
type TextMarshalerKeyMap map[textKey]string
143-
type NonStringKeyMap map[nonStringKey]string
144-
`
145-
146-
// Test_Schema_MapOfTextMarshalerKey verifies that a map keyed by a type that
147-
// implements encoding.TextMarshaler is accepted (mirroring how such types are
148-
// accepted as struct fields) and produces an ordinary string-keyed object
149-
// schema with no key schema emitted.
150-
func Test_Schema_MapOfTextMarshalerKey(t *testing.T) {
151-
g := gomega.NewWithT(t)
152-
153-
output, pkg := transformPackage(t, textMarshalerKeyPackage, "TextMarshalerKeyMap")
154-
failIfErrors(t, pkg.Errors)
155-
g.Expect(output).To(gomega.Equal(&apiextensionsv1.JSONSchemaProps{
156-
Type: "object",
157-
AdditionalProperties: &apiextensionsv1.JSONSchemaPropsOrBool{
158-
Allows: true,
159-
Schema: &apiextensionsv1.JSONSchemaProps{
160-
Type: "string",
161-
},
162-
},
163-
}))
164-
}
165-
166-
// Test_Schema_MapOfNonStringKey verifies that a struct key that does not
167-
// implement encoding.TextMarshaler is still rejected.
168-
func Test_Schema_MapOfNonStringKey(t *testing.T) {
169-
g := gomega.NewWithT(t)
170-
171-
_, pkg := transformPackage(t, textMarshalerKeyPackage, "NonStringKeyMap")
172-
g.Expect(pkg.Errors).ToNot(gomega.BeEmpty())
173-
g.Expect(pkg.Errors[0].Msg).To(gomega.ContainSubstring("map keys must be strings"))
174-
}
175-
17675
func failIfErrors(t *testing.T, errs []packages.Error) {
17776
if len(errs) > 0 {
17877
msgs := make([]string, 0, len(errs))
Lines changed: 126 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,126 @@
1+
/*
2+
Copyright 2026 The Kubernetes Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package crd
18+
19+
import (
20+
"go/ast"
21+
22+
. "github.com/onsi/ginkgo"
23+
. "github.com/onsi/gomega"
24+
pkgstest "golang.org/x/tools/go/packages/packagestest"
25+
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
26+
"sigs.k8s.io/controller-tools/pkg/loader"
27+
testloader "sigs.k8s.io/controller-tools/pkg/loader/testutils"
28+
"sigs.k8s.io/controller-tools/pkg/markers"
29+
)
30+
31+
var _ = Describe("mapToSchema", func() {
32+
Context("TextMarshaler map key validation", func() {
33+
// cleanup holds the teardown function returned by the fake package loader.
34+
// AfterEach calls it so temp directories are removed after every It block.
35+
var cleanup func()
36+
37+
AfterEach(func() {
38+
if cleanup != nil {
39+
cleanup()
40+
cleanup = nil
41+
}
42+
})
43+
44+
// schemaForType loads src as a fake package and returns the schema for the
45+
// named type, together with the package so callers can inspect errors.
46+
// It sets cleanup so AfterEach can release temp files after the It block ends.
47+
schemaForType := func(src, typeName string) (*apiextensionsv1.JSONSchemaProps, *loader.Package) {
48+
const mod = "sigs.k8s.io/controller-tools/pkg/crd"
49+
pkgs, exported, err := testloader.LoadFakeRoots(pkgstest.Modules, []pkgstest.Module{{
50+
Name: mod,
51+
Files: map[string]any{"test.go": src},
52+
}}, mod)
53+
if exported != nil {
54+
cleanup = exported.Cleanup
55+
}
56+
Expect(err).NotTo(HaveOccurred())
57+
Expect(pkgs).To(HaveLen(1))
58+
59+
pkg := pkgs[0]
60+
pkg.NeedTypesInfo()
61+
62+
schemaCtx := newSchemaContext(pkg, nil, true, false).ForInfo(&markers.TypeInfo{})
63+
64+
var definedType ast.Expr
65+
for _, decl := range pkg.Syntax[0].Decls {
66+
if gd, ok := decl.(*ast.GenDecl); ok {
67+
for _, spec := range gd.Specs {
68+
if ts, ok := spec.(*ast.TypeSpec); ok && ts.Name.Name == typeName {
69+
definedType = ts.Type
70+
}
71+
}
72+
}
73+
}
74+
Expect(definedType).NotTo(BeNil(), "type %q not found in fake package", typeName)
75+
return typeToSchema(schemaCtx, definedType), pkg
76+
}
77+
78+
// src defines three map types covering the three cases below.
79+
const src = `
80+
package crd
81+
82+
// valueKey implements encoding.TextMarshaler via a value receiver.
83+
// encoding/json can call MarshalText on map keys of this type because
84+
// the method is reachable without a pointer.
85+
type valueKey struct{}
86+
func (valueKey) MarshalText() ([]byte, error) { return nil, nil }
87+
88+
// ptrKey implements encoding.TextMarshaler via a pointer receiver only.
89+
// Map keys are not addressable, so encoding/json cannot call *ptrKey.MarshalText
90+
// and will return an error at runtime — this type must be rejected.
91+
type ptrKey struct{}
92+
func (*ptrKey) MarshalText() ([]byte, error) { return nil, nil }
93+
94+
// noMarshalKey is a plain struct with no TextMarshaler implementation.
95+
type noMarshalKey struct{}
96+
97+
type ValueKeyMap map[valueKey]string
98+
type PtrKeyMap map[ptrKey]string
99+
type NoMarshalKeyMap map[noMarshalKey]string
100+
`
101+
102+
It("accepts a value-receiver TextMarshaler type as a map key", func() {
103+
output, pkg := schemaForType(src, "ValueKeyMap")
104+
Expect(pkg.Errors).To(BeEmpty())
105+
Expect(output).To(Equal(&apiextensionsv1.JSONSchemaProps{
106+
Type: "object",
107+
AdditionalProperties: &apiextensionsv1.JSONSchemaPropsOrBool{
108+
Allows: true,
109+
Schema: &apiextensionsv1.JSONSchemaProps{Type: "string"},
110+
},
111+
}))
112+
})
113+
114+
It("rejects a pointer-receiver-only TextMarshaler type as a map key", func() {
115+
_, pkg := schemaForType(src, "PtrKeyMap")
116+
Expect(pkg.Errors).NotTo(BeEmpty())
117+
Expect(pkg.Errors[0].Msg).To(ContainSubstring("map keys must be strings"))
118+
})
119+
120+
It("rejects a struct that does not implement TextMarshaler", func() {
121+
_, pkg := schemaForType(src, "NoMarshalKeyMap")
122+
Expect(pkg.Errors).NotTo(BeEmpty())
123+
Expect(pkg.Errors[0].Msg).To(ContainSubstring("map keys must be strings"))
124+
})
125+
})
126+
})

pkg/crd/testdata/cronjob_types.go

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -297,9 +297,9 @@ type CronJobSpec struct {
297297
// Maps of arrays of things-that-aren’t-strings are permitted
298298
MapOfArraysOfFloats map[string][]bool `json:"mapOfArraysOfFloats,omitempty"`
299299

300-
// Maps keyed by a type that implements encoding.TextMarshaler are permitted,
301-
// since such keys serialize to strings (just like TextMarshaler fields do).
302-
MapOfTextMarshalerKeys map[URL3]string `json:"mapOfTextMarshalerKeys,omitempty"`
300+
// Maps keyed by a type that implements encoding.TextMarshaler via a value receiver
301+
// are permitted, since such keys serialize to strings when marshaled to JSON.
302+
MapOfTextMarshalerKeys map[TextMarshalerKey]string `json:"mapOfTextMarshalerKeys,omitempty"`
303303

304304
// +kubebuilder:validation:Minimum=-0.5
305305
// +kubebuilder:validation:Maximum=1.5
@@ -711,6 +711,15 @@ func (u *URL4) MarshalText() (text []byte, err error) {
711711
return (*url.URL)(u).MarshalBinary()
712712
}
713713

714+
// TextMarshalerKey is a string-based type that implements [encoding.TextMarshaler]
715+
// via a value receiver, making it valid as a CRD map key.
716+
type TextMarshalerKey string
717+
718+
var _ encoding.TextMarshaler = TextMarshalerKey("")
719+
720+
// MarshalText implements [encoding.TextMarshaler].
721+
func (t TextMarshalerKey) MarshalText() ([]byte, error) { return []byte(t), nil }
722+
714723
// +kubebuilder:validation:Type=integer
715724
// +kubebuilder:validation:Format=int64
716725
// Time2 is a newtype around [metav1.Time].

pkg/crd/testdata/testdata.kubebuilder.io_cronjobs.yaml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9472,8 +9472,8 @@ spec:
94729472
additionalProperties:
94739473
type: string
94749474
description: |-
9475-
Maps keyed by a type that implements encoding.TextMarshaler are permitted,
9476-
since such keys serialize to strings (just like TextMarshaler fields do).
9475+
Maps keyed by a type that implements encoding.TextMarshaler via a value receiver
9476+
are permitted, since such keys serialize to strings when marshaled to JSON.
94779477
type: object
94789478
minMaxProperties:
94799479
description: This tests that min/max properties work

0 commit comments

Comments
 (0)