diff --git a/pkg/crd/gen.go b/pkg/crd/gen.go index 43293d2a9..92eefb4ae 100644 --- a/pkg/crd/gen.go +++ b/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 } diff --git a/pkg/crd/parser_integration_test.go b/pkg/crd/parser_integration_test.go index b9fcab09f..46ce7c149 100644 --- a/pkg/crd/parser_integration_test.go +++ b/pkg/crd/parser_integration_test.go @@ -17,6 +17,7 @@ limitations under the License. package crd_test import ( + "fmt" "io/ioutil" "os" @@ -52,63 +53,112 @@ func packageErrors(pkg *loader.Package, filterKinds ...packages.ErrorKind) error } var _ = Describe("CRD Generation From Parsing to CustomResourceDefinition", func() { - It("should properly generate and flatten the rewritten CronJob schema", func() { - // TODO(directxman12): test generation across multiple versions (right - // now, we're trusting k/k's conversion code, though, which is probably - // fine for the time being) - By("switching into testdata to appease go modules") - cwd, err := os.Getwd() - Expect(err).NotTo(HaveOccurred()) - Expect(os.Chdir("./testdata")).To(Succeed()) // go modules are directory-sensitive - defer func() { Expect(os.Chdir(cwd)).To(Succeed()) }() - - By("loading the roots") - pkgs, err := loader.LoadRoots("./", "./unserved", "./deprecated") - Expect(err).NotTo(HaveOccurred()) - Expect(pkgs).To(HaveLen(3)) - cronJobPkg := pkgs[0] - - By("setting up the parser") - reg := &markers.Registry{} - Expect(crdmarkers.Register(reg)).To(Succeed()) - parser := &crd.Parser{ - Collector: &markers.Collector{Registry: reg}, - Checker: &loader.TypeChecker{}, - IgnoreUnexportedFields: true, - AllowDangerousTypes: true, // need to allow “dangerous types” in this file for testing - } - crd.AddKnownTypes(parser) + Context("should properly generate and flatten the rewritten schemas", func() { + + var ( + prevCwd string + pkgPaths []string + pkgs []*loader.Package + reg *markers.Registry + parser *crd.Parser + expPkgLen int + ) + + BeforeEach(func() { + var err error + By("switching into testdata to appease go modules") + prevCwd, err = os.Getwd() + Expect(err).NotTo(HaveOccurred()) + Expect(prevCwd).To(BeADirectory()) + Expect(os.Chdir("./testdata")).To(Succeed()) + + By("setting up the parser") + reg = &markers.Registry{} + Expect(crdmarkers.Register(reg)).To(Succeed()) + parser = &crd.Parser{ + Collector: &markers.Collector{Registry: reg}, + Checker: &loader.TypeChecker{}, + IgnoreUnexportedFields: true, + AllowDangerousTypes: true, // need to allow “dangerous types” in this file for testing + } + crd.AddKnownTypes(parser) + }) + + AfterEach(func() { + Expect(os.Chdir(prevCwd)).To(Succeed()) + }) + + JustBeforeEach(func() { + var err error + By("loading the roots") + pkgs, err = loader.LoadRoots(pkgPaths...) + Expect(err).NotTo(HaveOccurred()) + Expect(pkgs).To(HaveLen(expPkgLen)) + + By("requesting that the packages be parsed") + for _, p := range pkgs { + parser.NeedPackage(p) + } + }) - By("requesting that the packages be parsed") - for _, pkg := range pkgs { - parser.NeedPackage(pkg) - } + assertCRD := func(pkg *loader.Package, kind, fileName string) { + By(fmt.Sprintf("requesting that the %s CRD be generated", kind)) + groupKind := schema.GroupKind{Kind: kind, Group: "testdata.kubebuilder.io"} + parser.NeedCRDFor(groupKind, nil) - By("requesting that the CRD be generated") - groupKind := schema.GroupKind{Kind: "CronJob", Group: "testdata.kubebuilder.io"} - parser.NeedCRDFor(groupKind, nil) + By(fmt.Sprintf("fixing top level ObjectMeta on the %s CRD", kind)) + crd.FixTopLevelMetadata(parser.CustomResourceDefinitions[groupKind]) - By("fixing top level ObjectMeta on the CRD") - crd.FixTopLevelMetadata(parser.CustomResourceDefinitions[groupKind]) + By("checking that no errors occurred along the way (expect for type errors)") + ExpectWithOffset(1, packageErrors(pkg, packages.TypeError)).NotTo(HaveOccurred()) - By("checking that no errors occurred along the way (expect for type errors)") - Expect(packageErrors(cronJobPkg, packages.TypeError)).NotTo(HaveOccurred()) + By(fmt.Sprintf("checking that the %s CRD is present", kind)) + ExpectWithOffset(1, parser.CustomResourceDefinitions).To(HaveKey(groupKind)) - By("checking that the CRD is present") - Expect(parser.CustomResourceDefinitions).To(HaveKey(groupKind)) + By(fmt.Sprintf("loading the desired %s YAML", kind)) + expectedFile, err := ioutil.ReadFile(fileName) + ExpectWithOffset(1, err).NotTo(HaveOccurred()) - By("loading the desired YAML") - expectedFile, err := ioutil.ReadFile("testdata.kubebuilder.io_cronjobs.yaml") - Expect(err).NotTo(HaveOccurred()) + By(fmt.Sprintf("parsing the desired %s YAML", kind)) + var crd apiext.CustomResourceDefinition + ExpectWithOffset(1, yaml.Unmarshal(expectedFile, &crd)).To(Succeed()) + // clear the annotations -- we don't care about the attribution annotation + crd.Annotations = nil - By("parsing the desired YAML") - var crd apiext.CustomResourceDefinition - Expect(yaml.Unmarshal(expectedFile, &crd)).To(Succeed()) - // clear the annotations -- we don't care about the attribution annotation - crd.Annotations = nil + By(fmt.Sprintf("comparing the two %s CRDs", kind)) + ExpectWithOffset(1, parser.CustomResourceDefinitions[groupKind]).To(Equal(crd), "type not as expected, check pkg/crd/testdata/README.md for more details.\n\nDiff:\n\n%s", cmp.Diff(parser.CustomResourceDefinitions[groupKind], crd)) + } - By("comparing the two") - Expect(parser.CustomResourceDefinitions[groupKind]).To(Equal(crd), "type not as expected, check pkg/crd/testdata/README.md for more details.\n\nDiff:\n\n%s", cmp.Diff(parser.CustomResourceDefinitions[groupKind], crd)) + Context("CronJob API", func() { + BeforeEach(func() { + pkgPaths = []string{"./", "./unserved", "./deprecated"} + expPkgLen = 3 + }) + It("should successfully generate the CronJob CRD", func() { + assertCRD(pkgs[0], "CronJob", "testdata.kubebuilder.io_cronjobs.yaml") + }) + }) + + Context("Job API", func() { + BeforeEach(func() { + pkgPaths = []string{"./job/..."} + expPkgLen = 1 + }) + It("should successfully generate the Job CRD", func() { + assertCRD(pkgs[0], "Job", "testdata.kubebuilder.io_jobs.yaml") + }) + }) + + Context("CronJob and Job API", func() { + BeforeEach(func() { + pkgPaths = []string{"./", "./unserved", "./deprecated", "./job/..."} + expPkgLen = 4 + }) + It("should successfully generate the CronJob and Job CRDs", func() { + assertCRD(pkgs[0], "CronJob", "testdata.kubebuilder.io_cronjobs.yaml") + assertCRD(pkgs[3], "Job", "testdata.kubebuilder.io_jobs.yaml") + }) + }) }) It("should generate plural words for Kind correctly", func() { diff --git a/pkg/crd/testdata/cronjob_types.go b/pkg/crd/testdata/cronjob_types.go index 66222f6ea..46a61ef13 100644 --- a/pkg/crd/testdata/cronjob_types.go +++ b/pkg/crd/testdata/cronjob_types.go @@ -16,7 +16,7 @@ limitations under the License. // TODO(directxman12): test this across both versions (right now we're just // trusting k/k conversion, which is probably fine though) -//go:generate ../../../.run-controller-gen.sh crd:ignoreUnexportedFields=true,allowDangerousTypes=true paths=./;./deprecated;./unserved output:dir=. +//go:generate ../../../.run-controller-gen.sh crd:ignoreUnexportedFields=true,allowDangerousTypes=true paths=./;./deprecated;./unserved;./job/... output:dir=. // +groupName=testdata.kubebuilder.io // +versionName=v1 diff --git a/pkg/crd/testdata/job/types.go b/pkg/crd/testdata/job/types.go new file mode 100644 index 000000000..955266bed --- /dev/null +++ b/pkg/crd/testdata/job/types.go @@ -0,0 +1,64 @@ +/* + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +// +groupName=testdata.kubebuilder.io +// +versionName=v1beta1 +package job + +import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "testdata.kubebuilder.io/cronjob/unserved" +) + +// +kubebuilder:object:root=true +// +kubebuilder:subresource:status +// +kubebuilder:resource:singular=job + +// JobSpec is the spec for the jobs API. +type JobSpec struct { + // FriendlyName is the friendly name for the job. + // + // +kubebuilder:validation:MinLength=5 + FriendlyName string `json:"friendlyName"` + + // Count is the number of times a job may be executed. + // + // +kubebuilder:validation:Minimum=0 + // +kubebuilder:validation:Maximum=10 + Count int32 `json:"count"` + + // CronJob is the spec for the related CrongJob. + CronnJob unserved.CronJobSpec `json:"crongJob"` +} + +// Job is the Schema for the jobs API +type Job struct { + /* + */ + metav1.TypeMeta `json:",inline"` + metav1.ObjectMeta `json:"metadata,omitempty"` + + Spec JobSpec `json:"spec"` +} + +// +kubebuilder:object:root=true + +// JobList contains a list of Job +type JobList struct { + metav1.TypeMeta `json:",inline"` + metav1.ListMeta `json:"metadata,omitempty"` + Items []Job `json:"items"` +} diff --git a/pkg/crd/testdata/testdata.kubebuilder.io_cronjobs.yaml b/pkg/crd/testdata/testdata.kubebuilder.io_cronjobs.yaml index a4b031f47..8d6c15e70 100644 --- a/pkg/crd/testdata/testdata.kubebuilder.io_cronjobs.yaml +++ b/pkg/crd/testdata/testdata.kubebuilder.io_cronjobs.yaml @@ -7504,6 +7504,24 @@ spec: type: string metadata: type: object + spec: + description: CronJobSpec defines the desired state of CronJob + properties: + int32WithValidations: + format: int32 + maximum: 2 + minimum: -2 + multipleOf: 2 + type: integer + twoOfAKindPart0: + description: This tests that markers that are allowed on both fields + and types are applied to fields + minLength: 4 + type: string + required: + - int32WithValidations + - twoOfAKindPart0 + type: object type: object served: false storage: false diff --git a/pkg/crd/testdata/testdata.kubebuilder.io_jobs.yaml b/pkg/crd/testdata/testdata.kubebuilder.io_jobs.yaml new file mode 100644 index 000000000..e4eed5463 --- /dev/null +++ b/pkg/crd/testdata/testdata.kubebuilder.io_jobs.yaml @@ -0,0 +1,75 @@ +--- +apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition +metadata: + annotations: + controller-gen.kubebuilder.io/version: (devel) + creationTimestamp: null + name: jobs.testdata.kubebuilder.io +spec: + group: testdata.kubebuilder.io + names: + kind: Job + listKind: JobList + plural: jobs + singular: job + scope: Namespaced + versions: + - name: v1beta1 + schema: + openAPIV3Schema: + description: Job is the Schema for the jobs API + properties: + apiVersion: + description: 'APIVersion defines the versioned schema of this representation + of an object. Servers should convert recognized schemas to the latest + internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources' + type: string + kind: + description: 'Kind is a string value representing the REST resource this + object represents. Servers may infer this from the endpoint the client + submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds' + type: string + metadata: + type: object + spec: + description: JobSpec is the spec for the jobs API. + properties: + count: + description: Count is the number of times a job may be executed. + format: int32 + maximum: 10 + minimum: 0 + type: integer + crongJob: + description: CronJob is the spec for the related CrongJob. + properties: + int32WithValidations: + format: int32 + maximum: 2 + minimum: -2 + multipleOf: 2 + type: integer + twoOfAKindPart0: + description: This tests that markers that are allowed on both + fields and types are applied to fields + minLength: 4 + type: string + required: + - int32WithValidations + - twoOfAKindPart0 + type: object + friendlyName: + description: FriendlyName is the friendly name for the job. + minLength: 5 + type: string + required: + - count + - crongJob + - friendlyName + type: object + required: + - spec + type: object + served: true + storage: true diff --git a/pkg/crd/testdata/unserved/types.go b/pkg/crd/testdata/unserved/types.go index c4a7b3124..1c549f3bd 100644 --- a/pkg/crd/testdata/unserved/types.go +++ b/pkg/crd/testdata/unserved/types.go @@ -21,6 +21,18 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) +// CronJobSpec defines the desired state of CronJob +type CronJobSpec struct { + // This tests that markers that are allowed on both fields and types are applied to fields + // +kubebuilder:validation:MinLength=4 + TwoOfAKindPart0 string `json:"twoOfAKindPart0"` + + // +kubebuilder:validation:Minimum=-2 + // +kubebuilder:validation:Maximum=2 + // +kubebuilder:validation:MultipleOf=2 + Int32WithValidations int32 `json:"int32WithValidations"` +} + // +kubebuilder:object:root=true // +kubebuilder:subresource:status // +kubebuilder:resource:singular=mycronjob @@ -32,6 +44,8 @@ type CronJob struct { */ metav1.TypeMeta `json:",inline"` metav1.ObjectMeta `json:"metadata,omitempty"` + + Spec CronJobSpec `json:"spec,omitempty"` } // +kubebuilder:object:root=true diff --git a/pkg/loader/loader.go b/pkg/loader/loader.go index 064efa30f..4d98117b8 100644 --- a/pkg/loader/loader.go +++ b/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 @@ -366,17 +366,40 @@ 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() +// +//nolint:gocyclo +func LoadRootsWithConfig(cfg *packages.Config, roots ...string) (result []*Package, retErr error) { + defer func() { + if retErr != nil { + return + } + for i := range result { + visitImports(result, result[i], nil) + } + }() + + newLoader := func(cfg *packages.Config, cfgDir string) *loader { + if cfg == nil { + cfg = &packages.Config{ + Dir: cfgDir, + } + } + l := &loader{ + cfg: cfg, + packages: 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 +412,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 +430,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 @@ -438,34 +460,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 +591,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 +609,35 @@ 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 +} + +// visitImports walks a dependency graph, replacing imported package +// references with those from the rootPkgs list. This ensures the +// kubebuilder marker generation is handled correctly. For more info, +// please see issue 680. +func visitImports(rootPkgs []*Package, pkg *Package, seen sets.String) { + if seen == nil { + seen = sets.String{} + } + for importedPkgID, importedPkg := range pkg.Imports() { + for i := range rootPkgs { + if importedPkgID == rootPkgs[i].ID { + pkg.imports[importedPkgID] = rootPkgs[i] + } + } + if !seen.Has(importedPkgID) { + visitImports(rootPkgs, importedPkg, seen) + seen.Insert(importedPkgID) + } + } } // importFunc is an implementation of the single-method