🐛 crdgen: compare metav1 pkg by ID (#686)

* 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.

* Reproduce the issue

This patch adds tests to reproduce the issue of the markers
not being discovered when multiple paths are used.

The issue is not markers missing from a type, but markers missing
from a *referenced* type. For example, the new Job type imports
the unserved.CronJobSpec as the field Job.Spec.CronJob. The
markers are not generated for Job.Spec.CronJob as it is referenced
from another package.

Co-authored-by: akutz <[email protected]>
Co-authored-by: akutz <[email protected]>

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/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() {

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

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"`
+}

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

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

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