🐛 Provide error when CRD has multiple versions marked for storage

pkg/crd/crd_spec_test.go

@@ -17,9 +17,13 @@ limitations under the License.
 package crd_test
 
 import (
+	"fmt"
+
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
+	apiext "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	apiextlegacy "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 
 	"sigs.k8s.io/controller-tools/pkg/crd"
 )
@@ -288,5 +292,109 @@ var _ = Describe("CRD Generation", func() {
 				Expect(spec).To(Equal(orig))
 			})
 		})
+
+		Describe("check storage version", func() {
+
+			var (
+				groupKind schema.GroupKind
+			)
+
+			BeforeEach(func() {
+				groupKind = schema.GroupKind{
+					Group: "TestKind",
+					Kind:  "test.example.com",
+				}
+			})
+
+			It("should return an error when multiple storage versions are present", func() {
+				obj := &apiext.CustomResourceDefinition{
+					Spec: apiext.CustomResourceDefinitionSpec{
+						Versions: []apiext.CustomResourceDefinitionVersion{
+							{
+								Name:    "v1",
+								Storage: true,
+								Served:  true,
+							},
+							{
+								Name:    "v2",
+								Storage: true,
+							},
+						},
+					},
+				}
+				listOferrors := crd.CheckVersions(*obj, groupKind)
+				Expect(listOferrors).Should(ContainElement(fmt.Errorf("CRD for %s has more than one storage version", groupKind)))
+			})
+			It("should not return any errors when only one version is present", func() {
+				obj := &apiext.CustomResourceDefinition{
+					Spec: apiext.CustomResourceDefinitionSpec{
+						Versions: []apiext.CustomResourceDefinitionVersion{
+							{
+								Name:   "v1",
+								Served: true,
+							},
+						},
+					},
+				}
+				listOferrors := crd.CheckVersions(*obj, groupKind)
+				Expect(len(listOferrors)).To(Equal(0))
+			})
+			It("should return an error when no storage version is present", func() {
+				obj := &apiext.CustomResourceDefinition{
+					Spec: apiext.CustomResourceDefinitionSpec{
+						Versions: []apiext.CustomResourceDefinitionVersion{
+							{
+								Name:    "v1",
+								Storage: false,
+								Served:  true,
+							},
+							{
+								Name:    "v2",
+								Storage: false,
+							},
+						},
+					},
+				}
+				listOferrors := crd.CheckVersions(*obj, groupKind)
+				Expect(listOferrors).Should(ContainElement(fmt.Errorf("CRD for %s has no storage version", groupKind)))
+			})
+			It("should return an error when no served versions are present", func() {
+				obj := &apiext.CustomResourceDefinition{
+					Spec: apiext.CustomResourceDefinitionSpec{
+						Versions: []apiext.CustomResourceDefinitionVersion{
+							{
+								Name:    "v1",
+								Storage: true,
+							},
+							{
+								Name:    "v2",
+								Storage: false,
+							},
+						},
+					},
+				}
+				listOferrors := crd.CheckVersions(*obj, groupKind)
+				Expect(listOferrors).Should(ContainElement(fmt.Errorf("CRD for %s with version(s) %v does not serve any version", groupKind, obj.Spec.Versions)))
+			})
+			It("should not return any errors when one served storage version is present", func() {
+				obj := &apiext.CustomResourceDefinition{
+					Spec: apiext.CustomResourceDefinitionSpec{
+						Versions: []apiext.CustomResourceDefinitionVersion{
+							{
+								Name:    "v1",
+								Storage: false,
+							},
+							{
+								Name:    "v2",
+								Storage: true,
+								Served:  true,
+							},
+						},
+					},
+				}
+				listOferrors := crd.CheckVersions(*obj, groupKind)
+				Expect(len(listOferrors)).To(Equal(0))
+			})
+		})
 	})
 })

pkg/crd/spec.go

@@ -132,25 +132,42 @@ func (p *Parser) NeedCRDFor(groupKind schema.GroupKind, maxDescLen *int) {
 	// Otherwise, crd.Spec.Version may point to different CRD versions across different runs.
 	sort.Slice(crd.Spec.Versions, func(i, j int) bool { return crd.Spec.Versions[i].Name < crd.Spec.Versions[j].Name })
 
+	// just add the errors to the first relevant package for this CRD,
+	// since there's no specific error location
+	versionErrors := CheckVersions(crd, groupKind)
+	for _, err := range versionErrors {
+		packages[0].AddError(err)
+	}
+
+	crd.Status = apiext.CustomResourceDefinitionStatus{}
+
+	p.CustomResourceDefinitions[groupKind] = crd
+}
+
+func CheckVersions(crd apiext.CustomResourceDefinition, groupKind schema.GroupKind) (errList []error) {
 	// make sure we have *a* storage version
 	// (default it if we only have one, otherwise, bail)
 	if len(crd.Spec.Versions) == 1 {
 		crd.Spec.Versions[0].Storage = true
 	}
 
-	hasStorage := false
+	storageCounter := 0
 	for _, ver := range crd.Spec.Versions {
 		if ver.Storage {
-			hasStorage = true
-			break
+			storageCounter++
 		}
 	}
-	if !hasStorage {
-		// just add the error to the first relevant package for this CRD,
-		// since there's no specific error location
-		packages[0].AddError(fmt.Errorf("CRD for %s has no storage version", groupKind))
+
+	// there should be only one storage version
+	if storageCounter == 0 {
+		errList = append(errList, fmt.Errorf("CRD for %s has no storage version", groupKind))
+	}
+
+	if storageCounter > 1 {
+		errList = append(errList, fmt.Errorf("CRD for %s has more than one storage version", groupKind))
 	}
 
+	// check if the version is being served via REST APIs.
 	served := false
 	for _, ver := range crd.Spec.Versions {
 		if ver.Served {
@@ -159,9 +176,7 @@ func (p *Parser) NeedCRDFor(groupKind schema.GroupKind, maxDescLen *int) {
 		}
 	}
 	if !served {
-		// just add the error to the first relevant package for this CRD,
-		// since there's no specific error location
-		packages[0].AddError(fmt.Errorf("CRD for %s with version(s) %v does not serve any version", groupKind, crd.Spec.Versions))
+		errList = append(errList, fmt.Errorf("CRD for %s with version(s) %v does not serve any version", groupKind, crd.Spec.Versions))
 	}
 
 	// NB(directxman12): CRD's status doesn't have omitempty markers, which means things