Add .spec.version #142
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| package controllers_test | ||
|
|
||
| import ( | ||
| "context" | ||
|
|
||
| . "github.com/onsi/ginkgo/v2" | ||
| . "github.com/onsi/gomega" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
|
|
||
| operatorsv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1" | ||
| ) | ||
|
|
||
| func operator(spec operatorsv1alpha1.OperatorSpec) *operatorsv1alpha1.Operator { | ||
| return &operatorsv1alpha1.Operator{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "test-operator", | ||
| }, | ||
| Spec: spec, | ||
| } | ||
| } | ||
|
|
||
| var _ = Describe("Operator Spec Validations", func() { | ||
| var ( | ||
| ctx context.Context | ||
| cancel context.CancelFunc | ||
| ) | ||
| BeforeEach(func() { | ||
| ctx, cancel = context.WithCancel(context.Background()) | ||
| }) | ||
| AfterEach(func() { | ||
| cancel() | ||
| }) | ||
| It("should fail if the spec is empty", func() { | ||
| err := cl.Create(ctx, operator(operatorsv1alpha1.OperatorSpec{})) | ||
| Expect(err).To(HaveOccurred()) | ||
| Expect(err.Error()).To(ContainSubstring("spec.packageName in body should match '^[a-z0-9]+(-[a-z0-9]+)*$'")) | ||
| }) | ||
| It("should fail if package name length is greater than 48 characters", func() { | ||
| err := cl.Create(ctx, operator(operatorsv1alpha1.OperatorSpec{ | ||
| PackageName: "this-is-a-really-long-package-name-that-is-greater-than-48-characters", | ||
| })) | ||
| Expect(err).To(HaveOccurred()) | ||
| Expect(err.Error()).To(ContainSubstring("Too long: may not be longer than 48")) | ||
| }) | ||
| It("should fail if version is valid semver but length is greater than 64 characters", func() { | ||
| err := cl.Create(ctx, operator(operatorsv1alpha1.OperatorSpec{ | ||
| PackageName: "package", | ||
| Version: "1234567890.1234567890.12345678901234567890123456789012345678901234", | ||
| })) | ||
| Expect(err).To(HaveOccurred()) | ||
| Expect(err.Error()).To(ContainSubstring("Too long: may not be longer than 64")) | ||
| }) | ||
| It("should fail if an invalid semver is given", func() { | ||
| invalidSemvers := []string{ | ||
| "1.2.3.4", | ||
| "1.02.3", | ||
| "1.2.03", | ||
| "1.2.3-beta!", | ||
| "1.2.3.alpha", | ||
| "1..2.3", | ||
| "1.2.3-pre+bad_metadata", | ||
| "1.2.-3", | ||
| ".1.2.3", | ||
| } | ||
| for _, invalidSemver := range invalidSemvers { | ||
| err := cl.Create(ctx, operator(operatorsv1alpha1.OperatorSpec{ | ||
| PackageName: "package", | ||
| Version: invalidSemver, | ||
| })) | ||
|
|
||
| Expect(err).To(HaveOccurred(), "expected error for invalid semver %q", invalidSemver) | ||
| Expect(err.Error()).To(ContainSubstring("spec.version in body should match '^(0|[1-9]\\d*)\\.(0|[1-9]\\d*)\\.(0|[1-9]\\d*)(-(0|[1-9]\\d*|[0-9]*[a-zA-Z-][0-9a-zA-Z-]*)(\\.(0|[1-9]\\d*|[0-9]*[a-zA-Z-][0-9a-zA-Z-]*))*)?(\\+([0-9a-zA-Z-]+(\\.[0-9a-zA-Z-]+)*))?$'")) | ||
| } | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| package validators | ||
|
|
||
| import ( | ||
| "fmt" | ||
|
|
||
| "github.com/blang/semver/v4" | ||
|
|
||
| operatorsv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1" | ||
| ) | ||
|
|
||
| type operatorCRValidatorFunc func(operator *operatorsv1alpha1.Operator) error | ||
|
|
||
| // validateSemver validates that the operator's version is a valid SemVer. | ||
| // this validation should already be happening at the CRD level. But, it depends | ||
| // on a regex that could possibly fail to validate a valid SemVer. This is added as an | ||
| // extra measure to ensure a valid spec before the CR is processed for resolution | ||
| func validateSemver(operator *operatorsv1alpha1.Operator) error { | ||
|
There was a problem hiding this comment. If the CRD regex validation fails, will the value even be set? So, is this necessary? Are you concerned about false positives? There was a problem hiding this comment. For false positives. It's in case a bad semver somehow slips through the regex. It's so complicated and hard to parse I couldn't guarantee that nothing would slip through. So, I thought it might be a good idea to be defensive, even if 99.999% of the time this code won't probably be executed. There was a problem hiding this comment.
If I understood the question correctly (and please forgive me if I'm going over stuff you already know), my understanding of the way it works is: there's both client-side and server-side validation of the field value against the regex before it reaches the controller. The intent of putting that the validation is to automatically reject anything that isn't a semver so it doesn't even make it to the controller. But, after looking at the regex, I couldn't get any confidence that it would work 100% of the time. I tried to validate with different negative cases, but I couldn't convince myself that it will definitely catch everything. It could be possible that an invalid spec slips through, in which case it would be set. So, I thought it best to handle that case in code (even if it's unlikely that it will ever be executed). Hopefully, for most of the fields that come along in the API we'll be able to get away with relatively simple regexs and length limits that should be sufficient enough for us use of this controller validation sparingly. Since I'm rubber ducking this with you now, it occured to me that should a false positive slip through, we'd get a resolution failure on trying to parse the semver. So, maybe I'm being overzealous? Though a part of me thinks that ensuring that the input is clean before reaching the resolver might be worthwhile. I'm totally open for suggestions here. If we feel we're adding more complexity for relatively little gain, I can remove this 2nd layer of validation. wdyt? There was a problem hiding this comment. It indeed feels like belt and braces, but I think I'm in favour of keeping this validation as it prevents us hittng rest of the code in case of discrepancies between the regex and the library (e.g. bugs in library, for example). There was a problem hiding this comment. Assuming you got the regex from a reliable source, this scenario should never be hit, but better safe than sorry. Even regex's and libraries can have bugs. |
||
| if operator.Spec.Version == "" { | ||
| return nil | ||
| } | ||
| if _, err := semver.Parse(operator.Spec.Version); err != nil { | ||
| return fmt.Errorf("invalid .spec.version: %w", err) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| // ValidateOperatorSpec validates the operator spec, e.g. ensuring that .spec.version, if provided, is a valid SemVer | ||
| func ValidateOperatorSpec(operator *operatorsv1alpha1.Operator) error { | ||
| validators := []operatorCRValidatorFunc{ | ||
| validateSemver, | ||
| } | ||
| for _, validator := range validators { | ||
| if err := validator(operator); err != nil { | ||
| return err | ||
|
There was a problem hiding this comment. For now it is ok as there is only one field which requires validation. Just curious how you see UX for it once we have more validators - Will we bereturning one error at a time or use some sort of aggregated error (so we surface all issues at once)? There was a problem hiding this comment. That's a great call out. Personally, I think we should try as much as possible to add all of the validation errors as conditions. It would suck to keep going back and forth trying to get it right. What I'm not sure of right now is if it's "acceptable" to add multiple conditions of the same type (I don't see why not, but I don't know what the best practice is here). According to my good friend ChatGPT: It seems to suggest we should try to group everything into a single condition. But that seems ugly at first sight. Another option from the top of my head might be to create a condition for each validation error. That also seems ugly. Personally, I'm in favor of just having multiple conditions of the same type each describing its own reason for failure. I suggest we leave it as it is for now. I will add a comment and a ticket so we can follow up on this after a more thorough discussion upstream. wdyt? There was a problem hiding this comment. I have definitevely seen conditions like IMO - if condition is meant to be consumed by humans - we might just want to concatenate errors. We will later be able to split them into separate conditions. I personally not a fan of scrolling through 10s of conditions: I would rather see all validation errors in one place. However if we want to programmatically consume these conditions or want to give our users this ability - then it will definitevely be easier to consume if we have conditions for each issue. But in this specific case I do not see a lot of value in this feature (but I'm happy to be proven wrong).
Agreed There was a problem hiding this comment. The condition type needs to be unique in the list of conditions. Type and reason are part of the API so once we GA, those can't be removed or changed. My recommendation is generally to keep the number of types and reasons as small as possible based on what clients specifically require. In this case, I'd start with whatever type we're already using, maybe even use the existing failure reason, and then put the validation errors concatenated in the message. But +1 to capturing this in a separate issue. |
||
| } | ||
| } | ||
| return nil | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| package validators | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| . "github.com/onsi/ginkgo/v2" | ||
| . "github.com/onsi/gomega" | ||
| ) | ||
|
|
||
| func TestValidators(t *testing.T) { | ||
| RegisterFailHandler(Fail) | ||
| RunSpecs(t, "Validators Suite") | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| package validators_test | ||
|
|
||
| import ( | ||
| . "github.com/onsi/ginkgo/v2" | ||
| . "github.com/onsi/gomega" | ||
|
|
||
| "github.com/operator-framework/operator-controller/api/v1alpha1" | ||
| "github.com/operator-framework/operator-controller/controllers/validators" | ||
| ) | ||
|
|
||
| var _ = Describe("Validators", func() { | ||
|
There was a problem hiding this comment. Maybe I'm missing something - why not to use simple unit tests here? Looks like a classic use case for a table driven test. There was a problem hiding this comment. I think we decided to go for ginkgo for the unit tests. However, I'm starting to think it might have been a mistake. They aren't as easy to execute as go tests, and carry the cruft of the *_suite.go files. From my perspective at the moment, I'm not a huge fan of table driven tests. I think they do have a nice additive property to them and can make it easier to change many tests at once. But from previous experience (which is just anecdotal at best) they carry a higher complexity in trying to understand whats going later on in the project. Especially if you have massive test case structures with huge inputs. On the other hand, singular tests are easier to wrap your head around later on. However, they can carry a higher maintenance burden during refactoring/maintenance. So, I find it to be a tradeoff between readability and and maintainability (which are also interrelated XD). Now with copilot and chatGPT it's easier to write singular tests hahahah. On a serious side, I'm easy either way. I'd say we should do the following:
wdyt? There was a problem hiding this comment. Ginkgo supports table driven test fwiw. Maybe not quite as straightforward as a hand-written one, but maybe a reasonable compromise? |
||
| Describe("ValidateOperatorSpec", func() { | ||
| It("should not return an error for valid SemVer", func() { | ||
| operator := &v1alpha1.Operator{ | ||
| Spec: v1alpha1.OperatorSpec{ | ||
| Version: "1.2.3", | ||
| }, | ||
| } | ||
| err := validators.ValidateOperatorSpec(operator) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| }) | ||
|
|
||
| It("should return an error for invalid SemVer", func() { | ||
| operator := &v1alpha1.Operator{ | ||
| Spec: v1alpha1.OperatorSpec{ | ||
| Version: "invalid-semver", | ||
| }, | ||
| } | ||
| err := validators.ValidateOperatorSpec(operator) | ||
| Expect(err).To(HaveOccurred()) | ||
| }) | ||
|
|
||
| It("should not return an error for empty SemVer", func() { | ||
| operator := &v1alpha1.Operator{ | ||
| Spec: v1alpha1.OperatorSpec{ | ||
| Version: "", | ||
| }, | ||
| } | ||
| err := validators.ValidateOperatorSpec(operator) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| }) | ||
|
|
||
| It("should not return an error for valid SemVer with pre-release and metadata", func() { | ||
| operator := &v1alpha1.Operator{ | ||
| Spec: v1alpha1.OperatorSpec{ | ||
| Version: "1.2.3-alpha.1+metadata", | ||
| }, | ||
| } | ||
| err := validators.ValidateOperatorSpec(operator) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| }) | ||
|
|
||
| It("should not return an error for valid SemVer with pre-release", func() { | ||
| operator := &v1alpha1.Operator{ | ||
| Spec: v1alpha1.OperatorSpec{ | ||
| Version: "1.2.3-alpha-beta", | ||
| }, | ||
| } | ||
| err := validators.ValidateOperatorSpec(operator) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| }) | ||
| }) | ||
| }) | ||
Uh oh!
There was an error while loading. Please reload this page.