[v1.3.x] generate: fix multiple Go file type parsing bug

changelog/fragments/bugfix-3744.yaml

@@ -0,0 +1,4 @@
+entries:
+  - description: >
+      Properly consider all Go files when generating a CSV's `spec.customresourcedefinitions.owned`.
+    kind: bugfix

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

@@ -19,8 +19,8 @@ import (
 	"fmt"
 	"go/ast"
 	"strconv"
-	"strings"
 
+	"golang.org/x/tools/go/packages"
 	"sigs.k8s.io/controller-tools/pkg/crd"
 	"sigs.k8s.io/controller-tools/pkg/loader"
 	"sigs.k8s.io/controller-tools/pkg/markers"
@@ -79,20 +79,21 @@ func (im importIdents) findPackagePathForSelExpr(expr *ast.SelectorExpr) (pkgPat
 
 	// Short-circuit if only one import.
 	if len(imports) == 1 {
-		return imports[0].path
-	}
-
-	// If multiple files contain the same local import name, check to see which file contains the selector expression.
-	for _, imp := range imports {
-		if imp.f.Pos() <= expr.Pos() && imp.f.End() >= expr.End() {
-			return imp.path
+		pkgPath = imports[0].path
+	} else {
+		// If multiple files contain the same local import name, check to see which file contains the selector expression.
+		for _, imp := range imports {
+			if imp.f.Pos() <= expr.Pos() && imp.f.End() >= expr.End() {
+				pkgPath = imp.path
+				break
+			}
 		}
 	}
-	return ""
+	return loader.NonVendorPath(pkgPath)
 }
 
 // getMarkedChildrenOfField collects all marked fields from type declarations starting at rootField in depth-first order.
-func (g generator) getMarkedChildrenOfField(rootPkg *loader.Package, rootField markers.FieldInfo) (map[string][]*fieldInfo, error) {
+func (g generator) getMarkedChildrenOfField(rootPkg *loader.Package, rootField markers.FieldInfo) (map[crd.TypeIdent][]*fieldInfo, error) {
 	// Gather all types and imports needed to build the BFS tree.
 	rootPkg.NeedTypesInfo()
 	importIDs, err := newImportIdents(rootPkg)
@@ -102,50 +103,46 @@ func (g generator) getMarkedChildrenOfField(rootPkg *loader.Package, rootField m
 
 	// ast.Inspect will not traverse into fields, so iteratively collect them and to check for markers.
 	nextFields := []*fieldInfo{{FieldInfo: rootField}}
-	markedFields := map[string][]*fieldInfo{}
+	markedFields := map[crd.TypeIdent][]*fieldInfo{}
 	for len(nextFields) > 0 {
 		fields := []*fieldInfo{}
 		for _, field := range nextFields {
-			errs := []error{}
 			ast.Inspect(field.RawField, func(n ast.Node) bool {
 				if n == nil {
 					return true
 				}
 
-				var info *markers.TypeInfo
-				var hasInfo bool
+				var ident crd.TypeIdent
 				switch nt := n.(type) {
-				case *ast.SelectorExpr:
-					// Case of a type definition in an imported package.
-
+				case *ast.SelectorExpr: // Type definition in an imported package.
 					pkgPath := importIDs.findPackagePathForSelExpr(nt)
 					if pkgPath == "" {
 						// Found no reference to pkgPath in any file.
 						return true
 					}
-					if pkg, hasImport := rootPkg.Imports()[loader.NonVendorPath(pkgPath)]; hasImport {
-						// Check if the field's type exists in the known types.
-						info, hasInfo = g.types[crd.TypeIdent{Package: pkg, Name: nt.Sel.Name}]
+					if pkg, hasImport := rootPkg.Imports()[pkgPath]; hasImport {
+						pkg.NeedTypesInfo()
+						ident = crd.TypeIdent{Package: pkg, Name: nt.Sel.Name}
 					}
-				case *ast.Ident:
-					// Case of a local type definition.
-
-					// Only look at type names.
-					if nt.Obj != nil && nt.Obj.Kind == ast.Typ {
-						// Check if the field's type exists in the known types.
-						info, hasInfo = g.types[crd.TypeIdent{Package: rootPkg, Name: nt.Name}]
+				case *ast.Ident: // Local type definition.
+					// Only look at type idents or references to type idents in other files.
+					if nt.Obj == nil || nt.Obj.Kind == ast.Typ {
+						ident = crd.TypeIdent{Package: rootPkg, Name: nt.Name}
 					}
 				}
-				if !hasInfo {
+
+				// Check if the field's type is a known types.
+				info, hasInfo := g.types[ident]
+				if ident == (crd.TypeIdent{}) || !hasInfo {
 					return true
 				}
 
 				// Add all child fields to the list to search next.
 				for _, finfo := range info.Fields {
 					segment, err := getPathSegmentForField(finfo)
 					if err != nil {
-						errs = append(errs, fmt.Errorf("error getting path from type %s field %s: %v", info.Name, finfo.Name, err))
-						return true
+						rootPkg.AddError(fmt.Errorf("error getting path from type %s field %s: %v", info.Name, finfo.Name, err))
+						continue
 					}
 					// Add extra information to the segment if it comes from a certain field type.
 					switch finfo.RawField.Type.(type) {
@@ -166,33 +163,19 @@ func (g generator) getMarkedChildrenOfField(rootPkg *loader.Package, rootField m
 					fields = append(fields, f)
 					// Marked fields get collected for the caller to parse.
 					if len(finfo.Markers) != 0 {
-						markedFields[info.Name] = append(markedFields[info.Name], f)
+						markedFields[ident] = append(markedFields[ident], f)
 					}
 				}
 
 				return true
 			})
-			if err := fmtParseErrors(errs); err != nil {
-				return nil, err
-			}
 		}
 		nextFields = fields
 	}
-	return markedFields, nil
-}
 
-// fmtParseErrors prettifies a list of errors to make them easier to read.
-func fmtParseErrors(errs []error) error {
-	switch len(errs) {
-	case 0:
-		return nil
-	case 1:
-		return errs[0]
+	if loader.PrintErrors([]*loader.Package{rootPkg}, packages.TypeError) {
+		return nil, errors.New("package had type errors")
 	}
-	sb := strings.Builder{}
-	for _, err := range errs {
-		sb.WriteString("\n")
-		sb.WriteString(err.Error())
-	}
-	return errors.New(sb.String())
+
+	return markedFields, nil
 }

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

@@ -133,7 +133,7 @@ func (g generator) getTypedDescriptors(pkg *loader.Package, kindType *markers.Ty
 	return getTypedDescriptors(markedFields, t, descType), nil
 }
 
-func getTypedDescriptors(markedFields map[string][]*fieldInfo, t reflect.Type, descType string) (descriptors []interface{}) {
+func getTypedDescriptors(markedFields map[crd.TypeIdent][]*fieldInfo, t reflect.Type, descType string) (descriptors []interface{}) {
 	descriptorBuckets := make(map[int][]reflect.Value)
 	orders := make([]int, 0)
 	for _, fields := range markedFields {

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

@@ -24,26 +24,28 @@ import (
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 	"github.com/operator-framework/api/pkg/operators/v1alpha1"
+	"sigs.k8s.io/controller-tools/pkg/crd"
+	"sigs.k8s.io/controller-tools/pkg/loader"
 	"sigs.k8s.io/controller-tools/pkg/markers"
 	"sigs.k8s.io/kubebuilder/v2/test/e2e/utils"
 )
 
 var _ = Describe("getTypedDescriptors", func() {
 	var (
-		markedFields map[string][]*fieldInfo
+		markedFields map[crd.TypeIdent][]*fieldInfo
 		out          []interface{}
 	)
 
 	BeforeEach(func() {
-		markedFields = make(map[string][]*fieldInfo)
+		markedFields = make(map[crd.TypeIdent][]*fieldInfo)
 	})
 
 	It("handles an empty set of marked fields", func() {
 		out = getTypedDescriptors(markedFields, reflect.TypeOf(v1alpha1.SpecDescriptor{}), spec)
 		Expect(out).To(HaveLen(0))
 	})
 	It("returns one spec descriptor for one spec marker on a field", func() {
-		markedFields[""] = []*fieldInfo{
+		markedFields[crd.TypeIdent{}] = []*fieldInfo{
 			{
 				FieldInfo: markers.FieldInfo{
 					Markers: markers.MarkerValues{
@@ -64,7 +66,7 @@ var _ = Describe("getTypedDescriptors", func() {
 		}))
 	})
 	It("returns no spec descriptors for one status marker on a field", func() {
-		markedFields[""] = []*fieldInfo{
+		markedFields[crd.TypeIdent{}] = []*fieldInfo{
 			{
 				FieldInfo: markers.FieldInfo{
 					Markers: markers.MarkerValues{
@@ -79,7 +81,7 @@ var _ = Describe("getTypedDescriptors", func() {
 		Expect(out).To(HaveLen(0))
 	})
 	It("returns one status descriptor for one status marker on a field", func() {
-		markedFields[""] = []*fieldInfo{
+		markedFields[crd.TypeIdent{}] = []*fieldInfo{
 			{
 				FieldInfo: markers.FieldInfo{
 					Markers: markers.MarkerValues{
@@ -100,7 +102,7 @@ var _ = Describe("getTypedDescriptors", func() {
 		}))
 	})
 	It("returns one spec descriptor for three spec markers and one status marker on a field", func() {
-		markedFields[""] = []*fieldInfo{
+		markedFields[crd.TypeIdent{}] = []*fieldInfo{
 			{
 				FieldInfo: markers.FieldInfo{
 					Markers: markers.MarkerValues{
@@ -126,7 +128,7 @@ var _ = Describe("getTypedDescriptors", func() {
 		}))
 	})
 	It("returns two spec descriptor for spec markers on two different fields", func() {
-		markedFields[""] = []*fieldInfo{
+		markedFields[crd.TypeIdent{}] = []*fieldInfo{
 			{
 				FieldInfo: markers.FieldInfo{
 					Markers: markers.MarkerValues{
@@ -163,21 +165,22 @@ func intPtr(i int) *int { return &i }
 
 // makeMockMarkedFields returns a randomly generated mock marked field set,
 // and the expected sorted set of descriptors.
-func makeMockMarkedFields() (markedFields map[string][]*fieldInfo, expected []interface{}) {
+func makeMockMarkedFields() (markedFields map[crd.TypeIdent][]*fieldInfo, expected []interface{}) {
 	descBuckets := make(map[int][]v1alpha1.SpecDescriptor, 100)
 	r := rand.New(rand.NewSource(time.Now().UnixNano()))
-	markedFields = make(map[string][]*fieldInfo, 100)
+	markedFields = make(map[crd.TypeIdent][]*fieldInfo, 100)
 	for i := 0; i < 100; i++ {
 		s, err := utils.RandomSuffix()
 		if err != nil {
 			panic(err)
 		}
 		name := strings.Title(s)
 		order := r.Int() % 200 // Very likely to get one conflict.
-		if _, hasName := markedFields[name]; hasName {
+		ident := crd.TypeIdent{Package: &loader.Package{}, Name: name}
+		if _, hasName := markedFields[ident]; hasName {
 			continue
 		}
-		markedFields[name] = []*fieldInfo{
+		markedFields[ident] = []*fieldInfo{
 			{
 				FieldInfo: markers.FieldInfo{
 					Markers: markers.MarkerValues{

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

@@ -68,72 +68,6 @@ type DummyStatus struct {
 	Boss Hog `json:"hog"`
 }
 
-// +k8s:deepcopy-gen=false
-// +k8s:openapi-gen=false
-type Hog struct {
-	// Should be in status but not spec, since Hog isn't in DummySpec
-	// +operator-sdk:csv:customresourcedefinitions:type=spec
-	// +operator-sdk:csv:customresourcedefinitions:type=status,displayName="boss-hog-engine"
-	Engine Engine `json:"engine"`
-	// Not in spec or status, no boolean annotation
-	// +operator-sdk:csv:customresourcedefinitions:displayName="doesnt-matter"
-	Brand string `json:"brand"`
-	// Not in spec or status
-	Helmet string `json:"helmet"`
-	// Fields should be inlined
-	// +operator-sdk:csv:customresourcedefinitions:type=spec
-	// +operator-sdk:csv:customresourcedefinitions:type=status
-	Inlined InlinedComponent `json:",inline"`
-	// Fields should be inlined
-	InlinedComponent `json:",inline"`
-	// Should be ignored
-	// +operator-sdk:csv:customresourcedefinitions:type=spec
-	// +operator-sdk:csv:customresourcedefinitions:type=status
-	Ignored IgnoredComponent `json:"-"`
-	// Should be ignored, but exported children should not be
-	notExported `json:",inline"`
-}
-
-type notExported struct {
-	// +operator-sdk:csv:customresourcedefinitions:type=spec
-	// +operator-sdk:csv:customresourcedefinitions:type=status
-	Public string `json:"foo"`
-	// +operator-sdk:csv:customresourcedefinitions:type=spec
-	// +operator-sdk:csv:customresourcedefinitions:type=status
-	private string `json:"-"`
-}
-
-// +k8s:deepcopy-gen=false
-// +k8s:openapi-gen=false
-type Engine struct {
-	// Should not be included, no annotations.
-	Pistons []string `json:"pistons"`
-}
-
-// +k8s:deepcopy-gen=false
-// +k8s:openapi-gen=false
-type Wheel struct {
-	// Type should be in spec with path equal to wheels[0].type
-	// +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"
-	Type string `json:"type"`
-}
-
-// +k8s:deepcopy-gen=false
-// +k8s:openapi-gen=false
-type InlinedComponent struct {
-	// +operator-sdk:csv:customresourcedefinitions:type=spec
-	// +operator-sdk:csv:customresourcedefinitions:type=status
-	SeatMaterial string `json:"seatMaterial"`
-}
-
-// +k8s:deepcopy-gen=false
-// +k8s:openapi-gen=false
-type IgnoredComponent struct {
-	// +operator-sdk:csv:customresourcedefinitions:type=spec
-	// +operator-sdk:csv:customresourcedefinitions:type=status
-	TrunkSpace string `json:"trunkSpace"`
-}
-
 // +k8s:deepcopy-gen=false
 // +k8s:openapi-gen=false
 type OtherDummyStatus struct {