Skip to content

Commit

Permalink
internal/generate/clusterserviceversion/bases/definitions: consider both
Browse files Browse the repository at this point in the history 
untyped and Typ-typed *ast.Object's that underly an *ast.Ident,
such that types in other files are found properly

Signed-off-by: Eric Stroczynski <ericstroczynski@gmail.com>
  • Loading branch information
@estroz
estroz committed Feb 11, 2021
1 parent d1c14c4 commit 10ba634
Show file tree
Hide file tree
Showing 6 changed files with 132 additions and 127 deletions.
4 changes: 4 additions & 0 deletions changelog/fragments/bugfix-3744.yaml
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
entries:
- description: >
Properly consider all Go files when generating a CSV's `spec.customresourcedefinitions.owned`.
kind: bugfix
81 changes: 32 additions & 49 deletions internal/generate/clusterserviceversion/bases/definitions/ast.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Expand All @@ -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) {
Expand All @@ -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
}
2 changes: 1 addition & 1 deletion internal/generate/clusterserviceversion/bases/definitions/crd.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
25 changes: 14 additions & 11 deletions internal/generate/clusterserviceversion/bases/definitions/crd_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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{
Expand All @@ -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{
Expand All @@ -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{
Expand All @@ -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{
Expand Down Expand Up @@ -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{
Expand Down
66 changes: 0 additions & 66 deletions internal/generate/testdata/go/api/v1alpha2/dummy_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down

0 comments on commit 10ba634

Please sign in to comment.