crdgen: compare metav1 pkg by ID & fsp loader

This patch updates the way the CRD generator compares an imported
metav1 package. Previously the comparison occurred using a Golang
equality operator, !=, against two, in-memory data structures.
However, this fails when multiple root paths are loaded. Their
metav1 packages are identical, just not identical objects in
memory. This patch updates the comparison to compare the package
IDs, not the instance of the object.

This patch also introduces a filesystem-path specific loader for
each unique filesystem path provided as a root. This ensures the
AST is loaded correctly and the kubebuilder markers are parsed as
intended.

pkg/crd/gen.go

@@ -247,7 +247,12 @@ func FindKubeKinds(parser *Parser, metav1Pkg *loader.Package) map[schema.GroupKi
 			}
 			fieldPkgPath := loader.NonVendorPath(namedField.Obj().Pkg().Path())
 			fieldPkg := pkg.Imports()[fieldPkgPath]
-			if fieldPkg != metav1Pkg {
+
+			// Compare the metav1 package by ID and not by the actual instance
+			// of the object. The objects in memory could be different due to
+			// loading from different root paths, even when they both refer to
+			// the same metav1 package.
+			if fieldPkg == nil || fieldPkg.ID != metav1Pkg.ID {
 				continue
 			}
 

pkg/loader/loader.go

@@ -323,7 +323,7 @@ func (l *loader) parseFile(filename string, src []byte) (*ast.File, error) {
 // populated.  Additional information, like ASTs and type-checking information,
 // can be accessed via methods on individual packages.
 func LoadRoots(roots ...string) ([]*Package, error) {
-	return LoadRootsWithConfig(&packages.Config{}, roots...)
+	return LoadRootsWithConfig(nil, roots...)
 }
 
 // LoadRootsWithConfig functions like LoadRoots, except that it allows passing
@@ -367,16 +367,27 @@ func LoadRoots(roots ...string) ([]*Package, error) {
 //     5. Load the filesystem path roots and return the load packages for the
 //        package/module roots AND the filesystem path roots.
 func LoadRootsWithConfig(cfg *packages.Config, roots ...string) ([]*Package, error) {
-	l := &loader{
-		cfg:      cfg,
-		packages: make(map[*packages.Package]*Package),
-	}
-	l.cfg.Mode |= packages.LoadImports | packages.NeedTypesSizes
-	if l.cfg.Fset == nil {
-		l.cfg.Fset = token.NewFileSet()
+	newLoader := func(cfg *packages.Config, cfgDir string) *loader {
+		if cfg == nil {
+			cfg = &packages.Config{}
+			cfg.Dir = cfgDir
+		}
+		l := &loader{
+			cfg:      cfg,
+			packages: make(map[*packages.Package]*Package),
+		}
+		l.cfg.Mode |= packages.LoadImports | packages.NeedTypesSizes
+		if l.cfg.Fset == nil {
+			l.cfg.Fset = token.NewFileSet()
+		}
+		// put our build flags first so that callers can override them
+		l.cfg.BuildFlags = append([]string{"-tags", "ignore_autogenerated"}, l.cfg.BuildFlags...)
+
+		return l
 	}
-	// put our build flags first so that callers can override them
-	l.cfg.BuildFlags = append([]string{"-tags", "ignore_autogenerated"}, l.cfg.BuildFlags...)
+
+	// initialize the default loader
+	l := newLoader(cfg, "")
 
 	// uniquePkgIDs is used to keep track of the discovered packages to be nice
 	// and try and prevent packages from showing up twice when nested module
@@ -389,7 +400,7 @@ func LoadRootsWithConfig(cfg *packages.Config, roots ...string) ([]*Package, err
 	// if validatePkgFn is nil, a package will be returned in the slice,
 	// otherwise the package is only returned if the result of
 	// validatePkgFn(pkg.ID) is truthy
-	loadPackages := func(roots ...string) ([]*Package, error) {
+	loadPackages := func(l *loader, roots ...string) ([]*Package, error) {
 		rawPkgs, err := packages.Load(l.cfg, roots...)
 		if err != nil {
 			return nil, err
@@ -407,12 +418,11 @@ func LoadRootsWithConfig(cfg *packages.Config, roots ...string) ([]*Package, err
 
 	// if no roots were provided then load the current package and return early
 	if len(roots) == 0 {
-		pkgs, err := loadPackages()
+		pkgs, err := loadPackages(l)
 		if err != nil {
 			return nil, err
 		}
-		l.Roots = append(l.Roots, pkgs...)
-		return l.Roots, nil
+		return pkgs, nil
 	}
 
 	// pkgRoots is a slice of roots that are package/modules and fspRoots
@@ -421,6 +431,7 @@ func LoadRootsWithConfig(cfg *packages.Config, roots ...string) ([]*Package, err
 	// please refer to this function's godoc comments for more information on
 	// how these two types of roots are distinguished from one another
 	var (
+		result    []*Package
 		pkgRoots  []string
 		fspRoots  []string
 		fspRootRx = regexp.MustCompile(`^\.{1,2}`)
@@ -438,34 +449,39 @@ func LoadRootsWithConfig(cfg *packages.Config, roots ...string) ([]*Package, err
 	// system path roots due to them needing a custom, calculated value for the
 	// cfg.Dir field
 	if len(pkgRoots) > 0 {
-		pkgs, err := loadPackages(pkgRoots...)
+		pkgs, err := loadPackages(l, pkgRoots...)
 		if err != nil {
 			return nil, err
 		}
-		l.Roots = append(l.Roots, pkgs...)
+		result = append(result, pkgs...)
 	}
 
 	// if there are no filesystem path roots then go ahead and return early
 	if len(fspRoots) == 0 {
-		return l.Roots, nil
+		return result, nil
 	}
 
 	//
 	// at this point we are handling filesystem path roots
 	//
 
+	// store the value of cfg.Dir so we can use it later if it is non-empty.
+	// we need to store it now as the value of cfg.Dir will be updated by
+	// a loop below
+	var cfgDir string
+	if cfg != nil {
+		cfgDir = cfg.Dir
+	}
+
 	// ensure the cfg.Dir field is reset to its original value upon
 	// returning from this function. it should honestly be fine if it is
 	// not given most callers will not send in the cfg parameter directly,
 	// as it's largely for testing, but still, let's be good stewards.
 	defer func(d string) {
-		cfg.Dir = d
-	}(cfg.Dir)
-
-	// store the value of cfg.Dir so we can use it later if it is non-empty.
-	// we need to store it now as the value of cfg.Dir will be updated by
-	// a loop below
-	cfgDir := cfg.Dir
+		if cfg != nil {
+			cfg.Dir = d
+		}
+	}(cfgDir)
 
 	// addNestedGoModulesToRoots is given to filepath.WalkDir and adds the
 	// directory part of p to the list of filesystem path roots IFF p is the
@@ -564,9 +580,16 @@ func LoadRootsWithConfig(cfg *packages.Config, roots ...string) ([]*Package, err
 			b = "."
 		}
 
-		// update the loader configuration's Dir field to the directory part of
-		// the root
-		l.cfg.Dir = d
+		// create the loader for this filesystem path. this is required in order
+		// to properly load the files as AST later in various workflows,
+		// including CRD generation
+		var fspLoader *loader
+		if cfg == nil {
+			fspLoader = newLoader(nil, d)
+		} else {
+			fspLoader = l
+			fspLoader.cfg.Dir = d
+		}
 
 		// update the root to be "./..." or "./."
 		// (with OS-specific filepath separator). please note filepath.Join
@@ -575,14 +598,14 @@ func LoadRootsWithConfig(cfg *packages.Config, roots ...string) ([]*Package, err
 		r = fmt.Sprintf(".%s%s", string(filepath.Separator), b)
 
 		// load the packages from the roots
-		pkgs, err := loadPackages(r)
+		pkgs, err := loadPackages(fspLoader, r)
 		if err != nil {
 			return nil, err
 		}
-		l.Roots = append(l.Roots, pkgs...)
+		result = append(result, pkgs...)
 	}
 
-	return l.Roots, nil
+	return result, nil
 }
 
 // importFunc is an implementation of the single-method