Skip to content

Commit e4fc90c

Browse files
[v1.3.x] generate: fix multiple Go file type parsing bug (#4508)
Signed-off-by: Eric Stroczynski <[email protected]> Co-authored-by: Eric Stroczynski <[email protected]>
1 parent 0ec4b70 commit e4fc90c

File tree

6 files changed

+132
-127
lines changed

6 files changed

+132
-127
lines changed
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
entries:
2+
- description: >
3+
Properly consider all Go files when generating a CSV's `spec.customresourcedefinitions.owned`.
4+
kind: bugfix

internal/generate/clusterserviceversion/bases/definitions/ast.go

Lines changed: 32 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,8 @@ import (
1919
"fmt"
2020
"go/ast"
2121
"strconv"
22-
"strings"
2322

23+
"golang.org/x/tools/go/packages"
2424
"sigs.k8s.io/controller-tools/pkg/crd"
2525
"sigs.k8s.io/controller-tools/pkg/loader"
2626
"sigs.k8s.io/controller-tools/pkg/markers"
@@ -79,20 +79,21 @@ func (im importIdents) findPackagePathForSelExpr(expr *ast.SelectorExpr) (pkgPat
7979

8080
// Short-circuit if only one import.
8181
if len(imports) == 1 {
82-
return imports[0].path
83-
}
84-
85-
// If multiple files contain the same local import name, check to see which file contains the selector expression.
86-
for _, imp := range imports {
87-
if imp.f.Pos() <= expr.Pos() && imp.f.End() >= expr.End() {
88-
return imp.path
82+
pkgPath = imports[0].path
83+
} else {
84+
// If multiple files contain the same local import name, check to see which file contains the selector expression.
85+
for _, imp := range imports {
86+
if imp.f.Pos() <= expr.Pos() && imp.f.End() >= expr.End() {
87+
pkgPath = imp.path
88+
break
89+
}
8990
}
9091
}
91-
return ""
92+
return loader.NonVendorPath(pkgPath)
9293
}
9394

9495
// getMarkedChildrenOfField collects all marked fields from type declarations starting at rootField in depth-first order.
95-
func (g generator) getMarkedChildrenOfField(rootPkg *loader.Package, rootField markers.FieldInfo) (map[string][]*fieldInfo, error) {
96+
func (g generator) getMarkedChildrenOfField(rootPkg *loader.Package, rootField markers.FieldInfo) (map[crd.TypeIdent][]*fieldInfo, error) {
9697
// Gather all types and imports needed to build the BFS tree.
9798
rootPkg.NeedTypesInfo()
9899
importIDs, err := newImportIdents(rootPkg)
@@ -102,50 +103,46 @@ func (g generator) getMarkedChildrenOfField(rootPkg *loader.Package, rootField m
102103

103104
// ast.Inspect will not traverse into fields, so iteratively collect them and to check for markers.
104105
nextFields := []*fieldInfo{{FieldInfo: rootField}}
105-
markedFields := map[string][]*fieldInfo{}
106+
markedFields := map[crd.TypeIdent][]*fieldInfo{}
106107
for len(nextFields) > 0 {
107108
fields := []*fieldInfo{}
108109
for _, field := range nextFields {
109-
errs := []error{}
110110
ast.Inspect(field.RawField, func(n ast.Node) bool {
111111
if n == nil {
112112
return true
113113
}
114114

115-
var info *markers.TypeInfo
116-
var hasInfo bool
115+
var ident crd.TypeIdent
117116
switch nt := n.(type) {
118-
case *ast.SelectorExpr:
119-
// Case of a type definition in an imported package.
120-
117+
case *ast.SelectorExpr: // Type definition in an imported package.
121118
pkgPath := importIDs.findPackagePathForSelExpr(nt)
122119
if pkgPath == "" {
123120
// Found no reference to pkgPath in any file.
124121
return true
125122
}
126-
if pkg, hasImport := rootPkg.Imports()[loader.NonVendorPath(pkgPath)]; hasImport {
127-
// Check if the field's type exists in the known types.
128-
info, hasInfo = g.types[crd.TypeIdent{Package: pkg, Name: nt.Sel.Name}]
123+
if pkg, hasImport := rootPkg.Imports()[pkgPath]; hasImport {
124+
pkg.NeedTypesInfo()
125+
ident = crd.TypeIdent{Package: pkg, Name: nt.Sel.Name}
129126
}
130-
case *ast.Ident:
131-
// Case of a local type definition.
132-
133-
// Only look at type names.
134-
if nt.Obj != nil && nt.Obj.Kind == ast.Typ {
135-
// Check if the field's type exists in the known types.
136-
info, hasInfo = g.types[crd.TypeIdent{Package: rootPkg, Name: nt.Name}]
127+
case *ast.Ident: // Local type definition.
128+
// Only look at type idents or references to type idents in other files.
129+
if nt.Obj == nil || nt.Obj.Kind == ast.Typ {
130+
ident = crd.TypeIdent{Package: rootPkg, Name: nt.Name}
137131
}
138132
}
139-
if !hasInfo {
133+
134+
// Check if the field's type is a known types.
135+
info, hasInfo := g.types[ident]
136+
if ident == (crd.TypeIdent{}) || !hasInfo {
140137
return true
141138
}
142139

143140
// Add all child fields to the list to search next.
144141
for _, finfo := range info.Fields {
145142
segment, err := getPathSegmentForField(finfo)
146143
if err != nil {
147-
errs = append(errs, fmt.Errorf("error getting path from type %s field %s: %v", info.Name, finfo.Name, err))
148-
return true
144+
rootPkg.AddError(fmt.Errorf("error getting path from type %s field %s: %v", info.Name, finfo.Name, err))
145+
continue
149146
}
150147
// Add extra information to the segment if it comes from a certain field type.
151148
switch finfo.RawField.Type.(type) {
@@ -166,33 +163,19 @@ func (g generator) getMarkedChildrenOfField(rootPkg *loader.Package, rootField m
166163
fields = append(fields, f)
167164
// Marked fields get collected for the caller to parse.
168165
if len(finfo.Markers) != 0 {
169-
markedFields[info.Name] = append(markedFields[info.Name], f)
166+
markedFields[ident] = append(markedFields[ident], f)
170167
}
171168
}
172169

173170
return true
174171
})
175-
if err := fmtParseErrors(errs); err != nil {
176-
return nil, err
177-
}
178172
}
179173
nextFields = fields
180174
}
181-
return markedFields, nil
182-
}
183175

184-
// fmtParseErrors prettifies a list of errors to make them easier to read.
185-
func fmtParseErrors(errs []error) error {
186-
switch len(errs) {
187-
case 0:
188-
return nil
189-
case 1:
190-
return errs[0]
176+
if loader.PrintErrors([]*loader.Package{rootPkg}, packages.TypeError) {
177+
return nil, errors.New("package had type errors")
191178
}
192-
sb := strings.Builder{}
193-
for _, err := range errs {
194-
sb.WriteString("\n")
195-
sb.WriteString(err.Error())
196-
}
197-
return errors.New(sb.String())
179+
180+
return markedFields, nil
198181
}

internal/generate/clusterserviceversion/bases/definitions/crd.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -133,7 +133,7 @@ func (g generator) getTypedDescriptors(pkg *loader.Package, kindType *markers.Ty
133133
return getTypedDescriptors(markedFields, t, descType), nil
134134
}
135135

136-
func getTypedDescriptors(markedFields map[string][]*fieldInfo, t reflect.Type, descType string) (descriptors []interface{}) {
136+
func getTypedDescriptors(markedFields map[crd.TypeIdent][]*fieldInfo, t reflect.Type, descType string) (descriptors []interface{}) {
137137
descriptorBuckets := make(map[int][]reflect.Value)
138138
orders := make([]int, 0)
139139
for _, fields := range markedFields {

internal/generate/clusterserviceversion/bases/definitions/crd_test.go

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -24,26 +24,28 @@ import (
2424
. "github.com/onsi/ginkgo"
2525
. "github.com/onsi/gomega"
2626
"github.com/operator-framework/api/pkg/operators/v1alpha1"
27+
"sigs.k8s.io/controller-tools/pkg/crd"
28+
"sigs.k8s.io/controller-tools/pkg/loader"
2729
"sigs.k8s.io/controller-tools/pkg/markers"
2830
"sigs.k8s.io/kubebuilder/v2/test/e2e/utils"
2931
)
3032

3133
var _ = Describe("getTypedDescriptors", func() {
3234
var (
33-
markedFields map[string][]*fieldInfo
35+
markedFields map[crd.TypeIdent][]*fieldInfo
3436
out []interface{}
3537
)
3638

3739
BeforeEach(func() {
38-
markedFields = make(map[string][]*fieldInfo)
40+
markedFields = make(map[crd.TypeIdent][]*fieldInfo)
3941
})
4042

4143
It("handles an empty set of marked fields", func() {
4244
out = getTypedDescriptors(markedFields, reflect.TypeOf(v1alpha1.SpecDescriptor{}), spec)
4345
Expect(out).To(HaveLen(0))
4446
})
4547
It("returns one spec descriptor for one spec marker on a field", func() {
46-
markedFields[""] = []*fieldInfo{
48+
markedFields[crd.TypeIdent{}] = []*fieldInfo{
4749
{
4850
FieldInfo: markers.FieldInfo{
4951
Markers: markers.MarkerValues{
@@ -64,7 +66,7 @@ var _ = Describe("getTypedDescriptors", func() {
6466
}))
6567
})
6668
It("returns no spec descriptors for one status marker on a field", func() {
67-
markedFields[""] = []*fieldInfo{
69+
markedFields[crd.TypeIdent{}] = []*fieldInfo{
6870
{
6971
FieldInfo: markers.FieldInfo{
7072
Markers: markers.MarkerValues{
@@ -79,7 +81,7 @@ var _ = Describe("getTypedDescriptors", func() {
7981
Expect(out).To(HaveLen(0))
8082
})
8183
It("returns one status descriptor for one status marker on a field", func() {
82-
markedFields[""] = []*fieldInfo{
84+
markedFields[crd.TypeIdent{}] = []*fieldInfo{
8385
{
8486
FieldInfo: markers.FieldInfo{
8587
Markers: markers.MarkerValues{
@@ -100,7 +102,7 @@ var _ = Describe("getTypedDescriptors", func() {
100102
}))
101103
})
102104
It("returns one spec descriptor for three spec markers and one status marker on a field", func() {
103-
markedFields[""] = []*fieldInfo{
105+
markedFields[crd.TypeIdent{}] = []*fieldInfo{
104106
{
105107
FieldInfo: markers.FieldInfo{
106108
Markers: markers.MarkerValues{
@@ -126,7 +128,7 @@ var _ = Describe("getTypedDescriptors", func() {
126128
}))
127129
})
128130
It("returns two spec descriptor for spec markers on two different fields", func() {
129-
markedFields[""] = []*fieldInfo{
131+
markedFields[crd.TypeIdent{}] = []*fieldInfo{
130132
{
131133
FieldInfo: markers.FieldInfo{
132134
Markers: markers.MarkerValues{
@@ -163,21 +165,22 @@ func intPtr(i int) *int { return &i }
163165

164166
// makeMockMarkedFields returns a randomly generated mock marked field set,
165167
// and the expected sorted set of descriptors.
166-
func makeMockMarkedFields() (markedFields map[string][]*fieldInfo, expected []interface{}) {
168+
func makeMockMarkedFields() (markedFields map[crd.TypeIdent][]*fieldInfo, expected []interface{}) {
167169
descBuckets := make(map[int][]v1alpha1.SpecDescriptor, 100)
168170
r := rand.New(rand.NewSource(time.Now().UnixNano()))
169-
markedFields = make(map[string][]*fieldInfo, 100)
171+
markedFields = make(map[crd.TypeIdent][]*fieldInfo, 100)
170172
for i := 0; i < 100; i++ {
171173
s, err := utils.RandomSuffix()
172174
if err != nil {
173175
panic(err)
174176
}
175177
name := strings.Title(s)
176178
order := r.Int() % 200 // Very likely to get one conflict.
177-
if _, hasName := markedFields[name]; hasName {
179+
ident := crd.TypeIdent{Package: &loader.Package{}, Name: name}
180+
if _, hasName := markedFields[ident]; hasName {
178181
continue
179182
}
180-
markedFields[name] = []*fieldInfo{
183+
markedFields[ident] = []*fieldInfo{
181184
{
182185
FieldInfo: markers.FieldInfo{
183186
Markers: markers.MarkerValues{

internal/generate/testdata/go/api/v1alpha2/dummy_types.go

Lines changed: 0 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -68,72 +68,6 @@ type DummyStatus struct {
6868
Boss Hog `json:"hog"`
6969
}
7070

71-
// +k8s:deepcopy-gen=false
72-
// +k8s:openapi-gen=false
73-
type Hog struct {
74-
// Should be in status but not spec, since Hog isn't in DummySpec
75-
// +operator-sdk:csv:customresourcedefinitions:type=spec
76-
// +operator-sdk:csv:customresourcedefinitions:type=status,displayName="boss-hog-engine"
77-
Engine Engine `json:"engine"`
78-
// Not in spec or status, no boolean annotation
79-
// +operator-sdk:csv:customresourcedefinitions:displayName="doesnt-matter"
80-
Brand string `json:"brand"`
81-
// Not in spec or status
82-
Helmet string `json:"helmet"`
83-
// Fields should be inlined
84-
// +operator-sdk:csv:customresourcedefinitions:type=spec
85-
// +operator-sdk:csv:customresourcedefinitions:type=status
86-
Inlined InlinedComponent `json:",inline"`
87-
// Fields should be inlined
88-
InlinedComponent `json:",inline"`
89-
// Should be ignored
90-
// +operator-sdk:csv:customresourcedefinitions:type=spec
91-
// +operator-sdk:csv:customresourcedefinitions:type=status
92-
Ignored IgnoredComponent `json:"-"`
93-
// Should be ignored, but exported children should not be
94-
notExported `json:",inline"`
95-
}
96-
97-
type notExported struct {
98-
// +operator-sdk:csv:customresourcedefinitions:type=spec
99-
// +operator-sdk:csv:customresourcedefinitions:type=status
100-
Public string `json:"foo"`
101-
// +operator-sdk:csv:customresourcedefinitions:type=spec
102-
// +operator-sdk:csv:customresourcedefinitions:type=status
103-
private string `json:"-"`
104-
}
105-
106-
// +k8s:deepcopy-gen=false
107-
// +k8s:openapi-gen=false
108-
type Engine struct {
109-
// Should not be included, no annotations.
110-
Pistons []string `json:"pistons"`
111-
}
112-
113-
// +k8s:deepcopy-gen=false
114-
// +k8s:openapi-gen=false
115-
type Wheel struct {
116-
// Type should be in spec with path equal to wheels[0].type
117-
// +operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Wheel Type",xDescriptors="urn:alm:descriptor:com.tectonic.ui:arrayFieldGroup:wheels" ; "urn:alm:descriptor:com.tectonic.ui:text"
118-
Type string `json:"type"`
119-
}
120-
121-
// +k8s:deepcopy-gen=false
122-
// +k8s:openapi-gen=false
123-
type InlinedComponent struct {
124-
// +operator-sdk:csv:customresourcedefinitions:type=spec
125-
// +operator-sdk:csv:customresourcedefinitions:type=status
126-
SeatMaterial string `json:"seatMaterial"`
127-
}
128-
129-
// +k8s:deepcopy-gen=false
130-
// +k8s:openapi-gen=false
131-
type IgnoredComponent struct {
132-
// +operator-sdk:csv:customresourcedefinitions:type=spec
133-
// +operator-sdk:csv:customresourcedefinitions:type=status
134-
TrunkSpace string `json:"trunkSpace"`
135-
}
136-
13771
// +k8s:deepcopy-gen=false
13872
// +k8s:openapi-gen=false
13973
type OtherDummyStatus struct {

0 commit comments

Comments
 (0)