Skip to content

Commit 2f839d0

Browse files
perdasilvaperdasilva
authored
Add .spec.version (#142)
* change log stacktrace level to panic Signed-off-by: perdasilva <[email protected]> * add .spec.version field to Operator CRD Signed-off-by: perdasilva <[email protected]> * refactor variable sources and resolver for version range constraint Signed-off-by: perdasilva <[email protected]> * update controller unit tests for .spec.version Signed-off-by: perdasilva <[email protected]> * add controller admission test to validate OpenAPI property constraint annotations Signed-off-by: perdasilva <[email protected]> * update go.mod - zap no longer indirect Signed-off-by: perdasilva <[email protected]> * update resource definition and tests based on review Signed-off-by: perdasilva <[email protected]> * add more robust semver validation and tests Signed-off-by: perdasilva <[email protected]> * fix admission unit test Signed-off-by: perdasilva <[email protected]> * fix format, imports, and lint Signed-off-by: perdasilva <[email protected]> * fix typo and example rendering in .spec.version godoc Signed-off-by: perdasilva <[email protected]> * address reviewer comments Signed-off-by: perdasilva <[email protected]> --------- Signed-off-by: perdasilva <[email protected]>
1 parent 79f406d commit 2f839d0

15 files changed

+470
-35
lines changed

‎api/v1alpha1/operator_types.go

+12
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,16 @@ type OperatorSpec struct {
2727
//+kubebuilder:validation:MaxLength:=48
2828
//+kubebuilder:validation:Pattern:=^[a-z0-9]+(-[a-z0-9]+)*$
2929
PackageName string `json:"packageName"`
30+
31+
//+kubebuilder:validation:MaxLength:=64
32+
//+kubebuilder:validation:Pattern=^(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-]+)*))?$
33+
//+kubebuilder:Optional
34+
// Version is an optional semver constraint on the package version. If not specified, the latest version available of the package will be installed.
35+
// If specified, the specific version of the package will be installed so long as it is available in any of the content sources available.
36+
// Examples: 1.2.3, 1.0.0-alpha, 1.0.0-rc.1
37+
//
38+
// For more information on semver, please see https://semver.org/
39+
Version string `json:"version,omitempty"`
3040
}
3141

3242
const (
@@ -38,6 +48,7 @@ const (
3848
ReasonBundleLookupFailed = "BundleLookupFailed"
3949
ReasonInstallationFailed = "InstallationFailed"
4050
ReasonInstallationStatusUnknown = "InstallationStatusUnknown"
51+
ReasonInvalidSpec = "InvalidSpec"
4152
)
4253

4354
func init() {
@@ -52,6 +63,7 @@ func init() {
5263
ReasonBundleLookupFailed,
5364
ReasonInstallationFailed,
5465
ReasonInstallationStatusUnknown,
66+
ReasonInvalidSpec,
5567
)
5668
}
5769

‎config/crd/bases/operators.operatorframework.io_operators.yaml

+10
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,16 @@ spec:
3939
maxLength: 48
4040
pattern: ^[a-z0-9]+(-[a-z0-9]+)*$
4141
type: string
42+
version:
43+
description: "Version is an optional semver constraint on the package
44+
version. If not specified, the latest version available of the package
45+
will be installed. If specified, the specific version of the package
46+
will be installed so long as it is available in any of the content
47+
sources available. Examples: 1.2.3, 1.0.0-alpha, 1.0.0-rc.1 \n For
48+
more information on semver, please see https://semver.org/"
49+
maxLength: 64
50+
pattern: ^(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-]+)*))?$
51+
type: string
4252
required:
4353
- packageName
4454
type: object

‎controllers/admission_test.go

+75
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
package controllers_test
2+
3+
import (
4+
"context"
5+
6+
. "github.com/onsi/ginkgo/v2"
7+
. "github.com/onsi/gomega"
8+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
9+
10+
operatorsv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1"
11+
)
12+
13+
func operator(spec operatorsv1alpha1.OperatorSpec) *operatorsv1alpha1.Operator {
14+
return &operatorsv1alpha1.Operator{
15+
ObjectMeta: metav1.ObjectMeta{
16+
Name: "test-operator",
17+
},
18+
Spec: spec,
19+
}
20+
}
21+
22+
var _ = Describe("Operator Spec Validations", func() {
23+
var (
24+
ctx context.Context
25+
cancel context.CancelFunc
26+
)
27+
BeforeEach(func() {
28+
ctx, cancel = context.WithCancel(context.Background())
29+
})
30+
AfterEach(func() {
31+
cancel()
32+
})
33+
It("should fail if the spec is empty", func() {
34+
err := cl.Create(ctx, operator(operatorsv1alpha1.OperatorSpec{}))
35+
Expect(err).To(HaveOccurred())
36+
Expect(err.Error()).To(ContainSubstring("spec.packageName in body should match '^[a-z0-9]+(-[a-z0-9]+)*$'"))
37+
})
38+
It("should fail if package name length is greater than 48 characters", func() {
39+
err := cl.Create(ctx, operator(operatorsv1alpha1.OperatorSpec{
40+
PackageName: "this-is-a-really-long-package-name-that-is-greater-than-48-characters",
41+
}))
42+
Expect(err).To(HaveOccurred())
43+
Expect(err.Error()).To(ContainSubstring("Too long: may not be longer than 48"))
44+
})
45+
It("should fail if version is valid semver but length is greater than 64 characters", func() {
46+
err := cl.Create(ctx, operator(operatorsv1alpha1.OperatorSpec{
47+
PackageName: "package",
48+
Version: "1234567890.1234567890.12345678901234567890123456789012345678901234",
49+
}))
50+
Expect(err).To(HaveOccurred())
51+
Expect(err.Error()).To(ContainSubstring("Too long: may not be longer than 64"))
52+
})
53+
It("should fail if an invalid semver is given", func() {
54+
invalidSemvers := []string{
55+
"1.2.3.4",
56+
"1.02.3",
57+
"1.2.03",
58+
"1.2.3-beta!",
59+
"1.2.3.alpha",
60+
"1..2.3",
61+
"1.2.3-pre+bad_metadata",
62+
"1.2.-3",
63+
".1.2.3",
64+
}
65+
for _, invalidSemver := range invalidSemvers {
66+
err := cl.Create(ctx, operator(operatorsv1alpha1.OperatorSpec{
67+
PackageName: "package",
68+
Version: invalidSemver,
69+
}))
70+
71+
Expect(err).To(HaveOccurred(), "expected error for invalid semver %q", invalidSemver)
72+
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-]+)*))?$'"))
73+
}
74+
})
75+
})

‎controllers/operator_controller.go

+14
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,8 @@ import (
3434
"sigs.k8s.io/controller-runtime/pkg/client"
3535
"sigs.k8s.io/controller-runtime/pkg/log"
3636

37+
"github.com/operator-framework/operator-controller/controllers/validators"
38+
3739
operatorsv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1"
3840
"github.com/operator-framework/operator-controller/internal/resolution"
3941
"github.com/operator-framework/operator-controller/internal/resolution/variable_sources/bundles_and_dependencies"
@@ -108,6 +110,18 @@ func checkForUnexpectedFieldChange(a, b operatorsv1alpha1.Operator) bool {
108110

109111
// Helper function to do the actual reconcile
110112
func (r *OperatorReconciler) reconcile(ctx context.Context, op *operatorsv1alpha1.Operator) (ctrl.Result, error) {
113+
// validate spec
114+
if err := validators.ValidateOperatorSpec(op); err != nil {
115+
apimeta.SetStatusCondition(&op.Status.Conditions, metav1.Condition{
116+
Type: operatorsv1alpha1.TypeReady,
117+
Status: metav1.ConditionFalse,
118+
Reason: operatorsv1alpha1.ReasonInvalidSpec,
119+
Message: err.Error(),
120+
ObservedGeneration: op.GetGeneration(),
121+
})
122+
return ctrl.Result{}, nil
123+
}
124+
111125
// run resolution
112126
solution, err := r.Resolver.Resolve(ctx)
113127
if err != nil {

‎controllers/operator_controller_test.go

+81
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import (
1616
"k8s.io/utils/pointer"
1717
ctrl "sigs.k8s.io/controller-runtime"
1818
"sigs.k8s.io/controller-runtime/pkg/client"
19+
"sigs.k8s.io/controller-runtime/pkg/client/fake"
1920

2021
operatorsv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1"
2122
"github.com/operator-framework/operator-controller/controllers"
@@ -80,6 +81,38 @@ var _ = Describe("Operator Controller Test", func() {
8081
Expect(cond.Message).To(Equal(fmt.Sprintf("package '%s' not found", pkgName)))
8182
})
8283
})
84+
When("the operator specifies a version that does not exist", func() {
85+
var pkgName string
86+
BeforeEach(func() {
87+
By("initializing cluster state")
88+
pkgName = fmt.Sprintf("exists-%s", rand.String(6))
89+
operator = &operatorsv1alpha1.Operator{
90+
ObjectMeta: metav1.ObjectMeta{Name: opKey.Name},
91+
Spec: operatorsv1alpha1.OperatorSpec{
92+
PackageName: pkgName,
93+
Version: "0.50.0", // this version of the package does not exist
94+
},
95+
}
96+
err := cl.Create(ctx, operator)
97+
Expect(err).NotTo(HaveOccurred())
98+
})
99+
It("sets resolution failure status", func() {
100+
By("running reconcile")
101+
res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: opKey})
102+
Expect(res).To(Equal(ctrl.Result{}))
103+
Expect(err).To(MatchError(fmt.Sprintf("package '%s' at version '0.50.0' not found", pkgName)))
104+
105+
By("fetching updated operator after reconcile")
106+
Expect(cl.Get(ctx, opKey, operator)).NotTo(HaveOccurred())
107+
108+
By("checking the expected conditions")
109+
cond := apimeta.FindStatusCondition(operator.Status.Conditions, operatorsv1alpha1.TypeReady)
110+
Expect(cond).NotTo(BeNil())
111+
Expect(cond.Status).To(Equal(metav1.ConditionFalse))
112+
Expect(cond.Reason).To(Equal(operatorsv1alpha1.ReasonResolutionFailed))
113+
Expect(cond.Message).To(Equal(fmt.Sprintf("package '%s' at version '0.50.0' not found", pkgName)))
114+
})
115+
})
83116
When("the operator specifies a valid available package", func() {
84117
const pkgName = "prometheus"
85118
BeforeEach(func() {
@@ -568,6 +601,54 @@ var _ = Describe("Operator Controller Test", func() {
568601
Expect(err).To(Not(HaveOccurred()))
569602
})
570603
})
604+
When("an invalid semver is provided that bypasses the regex validation", func() {
605+
var (
606+
operator *operatorsv1alpha1.Operator
607+
opKey types.NamespacedName
608+
pkgName string
609+
fakeClient client.Client
610+
)
611+
BeforeEach(func() {
612+
opKey = types.NamespacedName{Name: fmt.Sprintf("operator-validation-test-%s", rand.String(8))}
613+
614+
By("injecting creating a client with the bad operator CR")
615+
pkgName = fmt.Sprintf("exists-%s", rand.String(6))
616+
operator = &operatorsv1alpha1.Operator{
617+
ObjectMeta: metav1.ObjectMeta{Name: opKey.Name},
618+
Spec: operatorsv1alpha1.OperatorSpec{
619+
PackageName: pkgName,
620+
Version: "1.2.3-123abc_def", // bad semver that matches the regex on the CR validation
621+
},
622+
}
623+
624+
// this bypasses client/server-side CR validation and allows us to test the reconciler's validation
625+
fakeClient = fake.NewClientBuilder().WithScheme(sch).WithObjects(operator).Build()
626+
627+
By("changing the reconciler client to the fake client")
628+
reconciler.Client = fakeClient
629+
})
630+
AfterEach(func() {
631+
By("changing the reconciler client back to the real client")
632+
reconciler.Client = cl
633+
})
634+
635+
It("should add an invalid spec condition and *not* re-enqueue for reconciliation", func() {
636+
By("running reconcile")
637+
res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: opKey})
638+
Expect(res).To(Equal(ctrl.Result{}))
639+
Expect(err).ToNot(HaveOccurred())
640+
641+
By("fetching updated operator after reconcile")
642+
Expect(fakeClient.Get(ctx, opKey, operator)).NotTo(HaveOccurred())
643+
644+
By("checking the expected conditions")
645+
cond := apimeta.FindStatusCondition(operator.Status.Conditions, operatorsv1alpha1.TypeReady)
646+
Expect(cond).NotTo(BeNil())
647+
Expect(cond.Status).To(Equal(metav1.ConditionFalse))
648+
Expect(cond.Reason).To(Equal(operatorsv1alpha1.ReasonInvalidSpec))
649+
Expect(cond.Message).To(Equal("invalid .spec.version: Invalid character(s) found in prerelease \"123abc_def\""))
650+
})
651+
})
571652
})
572653

573654
func verifyInvariants(ctx context.Context, op *operatorsv1alpha1.Operator) {

‎controllers/validators/validators.go

+43
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
package validators
2+
3+
import (
4+
"fmt"
5+
6+
"github.com/blang/semver/v4"
7+
8+
operatorsv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1"
9+
)
10+
11+
type operatorCRValidatorFunc func(operator *operatorsv1alpha1.Operator) error
12+
13+
// validateSemver validates that the operator's version is a valid SemVer.
14+
// this validation should already be happening at the CRD level. But, it depends
15+
// on a regex that could possibly fail to validate a valid SemVer. This is added as an
16+
// extra measure to ensure a valid spec before the CR is processed for resolution
17+
func validateSemver(operator *operatorsv1alpha1.Operator) error {
18+
if operator.Spec.Version == "" {
19+
return nil
20+
}
21+
if _, err := semver.Parse(operator.Spec.Version); err != nil {
22+
return fmt.Errorf("invalid .spec.version: %w", err)
23+
}
24+
return nil
25+
}
26+
27+
// ValidateOperatorSpec validates the operator spec, e.g. ensuring that .spec.version, if provided, is a valid SemVer
28+
func ValidateOperatorSpec(operator *operatorsv1alpha1.Operator) error {
29+
validators := []operatorCRValidatorFunc{
30+
validateSemver,
31+
}
32+
33+
// TODO: currently we only have a single validator, but more will likely be added in the future
34+
// we need to make a decision on whether we want to run all validators or stop at the first error. If the the former,
35+
// we should consider how to present this to the user in a way that is easy to understand and fix.
36+
// this issue is tracked here: https://github.com/operator-framework/operator-controller/issues/167
37+
for _, validator := range validators {
38+
if err := validator(operator); err != nil {
39+
return err
40+
}
41+
}
42+
return nil
43+
}

‎controllers/validators/validators_suite_test.go

+13
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
package validators
2+
3+
import (
4+
"testing"
5+
6+
. "github.com/onsi/ginkgo/v2"
7+
. "github.com/onsi/gomega"
8+
)
9+
10+
func TestValidators(t *testing.T) {
11+
RegisterFailHandler(Fail)
12+
RunSpecs(t, "Validators Suite")
13+
}

‎controllers/validators/validators_test.go

+63
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
package validators_test
2+
3+
import (
4+
. "github.com/onsi/ginkgo/v2"
5+
. "github.com/onsi/gomega"
6+
7+
"github.com/operator-framework/operator-controller/api/v1alpha1"
8+
"github.com/operator-framework/operator-controller/controllers/validators"
9+
)
10+
11+
var _ = Describe("Validators", func() {
12+
Describe("ValidateOperatorSpec", func() {
13+
It("should not return an error for valid SemVer", func() {
14+
operator := &v1alpha1.Operator{
15+
Spec: v1alpha1.OperatorSpec{
16+
Version: "1.2.3",
17+
},
18+
}
19+
err := validators.ValidateOperatorSpec(operator)
20+
Expect(err).NotTo(HaveOccurred())
21+
})
22+
23+
It("should return an error for invalid SemVer", func() {
24+
operator := &v1alpha1.Operator{
25+
Spec: v1alpha1.OperatorSpec{
26+
Version: "invalid-semver",
27+
},
28+
}
29+
err := validators.ValidateOperatorSpec(operator)
30+
Expect(err).To(HaveOccurred())
31+
})
32+
33+
It("should not return an error for empty SemVer", func() {
34+
operator := &v1alpha1.Operator{
35+
Spec: v1alpha1.OperatorSpec{
36+
Version: "",
37+
},
38+
}
39+
err := validators.ValidateOperatorSpec(operator)
40+
Expect(err).NotTo(HaveOccurred())
41+
})
42+
43+
It("should not return an error for valid SemVer with pre-release and metadata", func() {
44+
operator := &v1alpha1.Operator{
45+
Spec: v1alpha1.OperatorSpec{
46+
Version: "1.2.3-alpha.1+metadata",
47+
},
48+
}
49+
err := validators.ValidateOperatorSpec(operator)
50+
Expect(err).NotTo(HaveOccurred())
51+
})
52+
53+
It("should not return an error for valid SemVer with pre-release", func() {
54+
operator := &v1alpha1.Operator{
55+
Spec: v1alpha1.OperatorSpec{
56+
Version: "1.2.3-alpha-beta",
57+
},
58+
}
59+
err := validators.ValidateOperatorSpec(operator)
60+
Expect(err).NotTo(HaveOccurred())
61+
})
62+
})
63+
})

‎go.mod

+1-1
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ require (
1010
github.com/operator-framework/deppy v0.0.0-20230125110717-dc02e928470f
1111
github.com/operator-framework/operator-registry v1.26.2
1212
github.com/operator-framework/rukpak v0.11.0
13+
go.uber.org/zap v1.21.0
1314
golang.org/x/net v0.0.0-20220722155237-a158d28d115b
1415
google.golang.org/grpc v1.47.0
1516
k8s.io/api v0.25.0
@@ -70,7 +71,6 @@ require (
7071
github.com/spf13/pflag v1.0.5 // indirect
7172
go.uber.org/atomic v1.7.0 // indirect
7273
go.uber.org/multierr v1.6.0 // indirect
73-
go.uber.org/zap v1.21.0 // indirect
7474
golang.org/x/crypto v0.0.0-20220408190544-5352b0902921 // indirect
7575
golang.org/x/oauth2 v0.0.0-20211104180415-d3ed0bb246c8 // indirect
7676
golang.org/x/sys v0.0.0-20220722155257-8c9f86f7a55f // indirect

0 commit comments

Comments
 (0)