Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

✨ Return error when CRD has multiple versions marked for storage #820

Closed
wants to merge 1 commit into from
Closed

✨ Return error when CRD has multiple versions marked for storage #820

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
129 changes: 129 additions & 0 deletions pkg/crd/crd_spec_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,138 @@ 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"
"k8s.io/apimachinery/pkg/runtime/schema"
"sigs.k8s.io/controller-tools/pkg/crd"
)

var _ = Describe("CRD Generation", func() {
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.ValidateStorageVersionAndServe(*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.ValidateStorageVersionAndServe(*obj, groupKind)
Expect(listOferrors).To(Equal([]error{}))
})
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.ValidateStorageVersionAndServe(*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.ValidateStorageVersionAndServe(*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.ValidateStorageVersionAndServe(*obj, groupKind)
Expect(listOferrors).To(Equal([]error{}))
})
It("should not return any errors when multiple served storage version is present", func() {
obj := &apiext.CustomResourceDefinition{
Spec: apiext.CustomResourceDefinitionSpec{
Versions: []apiext.CustomResourceDefinitionVersion{
{
Name: "v1",
Storage: false,
Served: true,
},
{
Name: "v2",
Storage: true,
Served: true,
},
},
},
}
listOferrors := crd.ValidateStorageVersionAndServe(*obj, groupKind)
Expect(listOferrors).To(Equal([]error{}))
})
})
})
21 changes: 17 additions & 4 deletions pkg/crd/spec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,15 @@ 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 })

for _, err := range ValidateStorageVersionAndServe(crd, groupKind) {
packages[0].AddError(err)
}

p.CustomResourceDefinitions[groupKind] = crd
}

func ValidateStorageVersionAndServe(crd apiext.CustomResourceDefinition, groupKind schema.GroupKind) []error {
errs := []error{}
// make sure we have *a* storage version
// (default it if we only have one, otherwise, bail)
if len(crd.Spec.Versions) == 1 {
Expand All @@ -151,14 +160,18 @@ func (p *Parser) NeedCRDFor(groupKind schema.GroupKind, maxDescLen *int) {
hasStorage := false
for _, ver := range crd.Spec.Versions {
if ver.Storage {
// more than one storage, already set it true on previous iteration
if hasStorage {
errs = append(errs, fmt.Errorf("CRD for %s has more than one storage version", groupKind))
Comment on lines +164 to +165
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we collect all the storage versions in a set and error out with information on where the storage flag is set?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vincepri
Wondering how should the map looks like? map[bool]bool? The value we are checking are just boolean value with the same type
https://github.com/kubernetes-sigs/controller-tools/pull/820/files#diff-a205d1d131ac7079cecfe9264fd82b9c7b56f982a42b4085fc4a963734cb9c2cR101-R106

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sets.New[String] should do it?

Copy link
Author

@lauchokyip lauchokyip Jun 1, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is a misunderstanding here.

when you loop through different version, it would be something like v1, v2, v3, and if we put them all in the set, it would be something like set{"v1,"v2", "v3"}. Enough though the version is not duplicated, there are still possible that v1 has storageversion set and v2 has storageversion set. Storage Version is a boolean value, not a string, making it a map would be map{"v1": true,"v2": true, "v3": false}, I don't think there is a need to use a set or map.

We would only need to check if the boolean is triggered twice, that should be more than enough

Please take a look at the unit test https://github.com/kubernetes-sigs/controller-tools/pull/820/files#diff-a205d1d131ac7079cecfe9264fd82b9c7b56f982a42b4085fc4a963734cb9c2cR100 , the Version is a string and the Storage is a boolean

break
}
hasStorage = true
break
}
}
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))
errs = append(errs, fmt.Errorf("CRD for %s has no storage version", groupKind))
Comment on lines -161 to +174
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why was this changed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we pass the package in here and use AddError as before?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vincepri I was following this PR
https://github.com/kubernetes-sigs/controller-tools/pull/399/files#diff-05d11052b3234e4c8ce76c9362508cbd5c3f275a76ee2513240234ed879cb2b5L151-R163

Should I stick with that approach or the one you suggested? I think both should work

 See the comment below

Copy link
Author

@lauchokyip lauchokyip Jun 1, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vincepri I wouldn't recommend to pass the package into the function because it would complicate the unit test writing ( need to setup a package loader and make it harder to compare the error string. In addition to that, the package loader would throw more error if the user has not have the package installed) . I would keep the function loosely coupled by not including package. Both should achieve the some results

Please let me know what you think

}

served := false
Expand All @@ -171,8 +184,8 @@ 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))
errs = append(errs, fmt.Errorf("CRD for %s with version(s) %v does not serve any version", groupKind, crd.Spec.Versions))
}

p.CustomResourceDefinitions[groupKind] = crd
return errs
}