Skip to content

Commit 683b79e

Browse files
joelanfordjoelanford
committed
controller code/test refactoring/additions
main bullet points are: - decrease nesting in main reconcile code by setting conditions and returning early when handling unexpected errors. - move reconciler unit tests to a separate file from the suite. if we add other controllers in the future, we'll likely want separate test files for each controller. - export the reconciler's resolver field to make it easy to swap out for tests (also add and use a test entity source) - remove the manager setup in the unit tests. this ensures we can test individual runs of the Reconcile function, which is good for two reasons: 1. no need to use `Eventually`, no polling, etc. so it speeds up the tests 2. it ensures that we fully understand the transitions that occur from a beginning state to and end state. - added more unit tests to increase code coverage. - moved conditions and reasons checks to invariants that run after every test of reconciling an existing Operator object. - make certain bundle entity properties optional. not all bundles will provide/require GVKs or require other packages. lookups for those entities should return nil slices rather than an error.
1 parent 5541671 commit 683b79e

File tree

7 files changed

+296
-182
lines changed

7 files changed

+296
-182
lines changed

‎api/v1alpha1/operator_types.go‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,9 @@ limitations under the License.
1717
package v1alpha1
1818

1919
import (
20-
operatorutil "github.com/operator-framework/operator-controller/internal/util"
2120
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
21+
22+
operatorutil "github.com/operator-framework/operator-controller/internal/util"
2223
)
2324

2425
// OperatorSpec defines the desired state of Operator
@@ -32,9 +33,9 @@ const (
3233
// TODO(user): add more Types, here and into init()
3334
TypeReady = "Ready"
3435

35-
ReasonNotImplemented = "NotImplemented"
36-
ReasonResolutionFailed = "ResolutionFailed"
3736
ReasonResolutionSucceeded = "ResolutionSucceeded"
37+
ReasonResolutionFailed = "ResolutionFailed"
38+
ReasonBundleLookupFailed = "BundleLookupFailed"
3839
)
3940

4041
func init() {
@@ -44,7 +45,9 @@ func init() {
4445
)
4546
// TODO(user): add Reasons from above
4647
operatorutil.ConditionReasons = append(operatorutil.ConditionReasons,
47-
ReasonNotImplemented, ReasonResolutionSucceeded, ReasonResolutionFailed,
48+
ReasonResolutionSucceeded,
49+
ReasonResolutionFailed,
50+
ReasonBundleLookupFailed,
4851
)
4952
}
5053

‎controllers/operator_controller.go‎

Lines changed: 43 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -36,9 +36,8 @@ import (
3636
// OperatorReconciler reconciles a Operator object
3737
type OperatorReconciler struct {
3838
client.Client
39-
Scheme *runtime.Scheme
40-
41-
resolver *resolution.OperatorResolver
39+
Scheme *runtime.Scheme
40+
Resolver *resolution.OperatorResolver
4241
}
4342

4443
//+kubebuilder:rbac:groups=operators.operatorframework.io,resources=operators,verbs=get;list;watch;create;update;patch;delete
@@ -100,45 +99,60 @@ func checkForUnexpectedFieldChange(a, b operatorsv1alpha1.Operator) bool {
10099

101100
// Helper function to do the actual reconcile
102101
func (r *OperatorReconciler) reconcile(ctx context.Context, op *operatorsv1alpha1.Operator) (ctrl.Result, error) {
103-
// define condition parameters
104-
var status = metav1.ConditionTrue
105-
var reason = operatorsv1alpha1.ReasonResolutionSucceeded
106-
var message = "resolution was successful"
107102

108103
// run resolution
109-
solution, err := r.resolver.Resolve(ctx)
104+
solution, err := r.Resolver.Resolve(ctx)
110105
if err != nil {
111-
status = metav1.ConditionTrue
112-
reason = operatorsv1alpha1.ReasonResolutionFailed
113-
message = err.Error()
114-
} else {
115-
// extract package bundle path from resolved variable
116-
for _, variable := range solution.SelectedVariables() {
117-
switch v := variable.(type) {
118-
case *bundles_and_dependencies.BundleVariable:
119-
packageName, err := v.BundleEntity().PackageName()
106+
apimeta.SetStatusCondition(&op.Status.Conditions, metav1.Condition{
107+
Type: operatorsv1alpha1.TypeReady,
108+
Status: metav1.ConditionFalse,
109+
Reason: operatorsv1alpha1.ReasonResolutionFailed,
110+
Message: err.Error(),
111+
ObservedGeneration: op.GetGeneration(),
112+
})
113+
return ctrl.Result{}, nil
114+
}
115+
116+
// extract package bundle path from resolved variable
117+
for _, variable := range solution.SelectedVariables() {
118+
switch v := variable.(type) {
119+
case *bundles_and_dependencies.BundleVariable:
120+
packageName, err := v.BundleEntity().PackageName()
121+
if err != nil {
122+
apimeta.SetStatusCondition(&op.Status.Conditions, metav1.Condition{
123+
Type: operatorsv1alpha1.TypeReady,
124+
Status: metav1.ConditionFalse,
125+
Reason: operatorsv1alpha1.ReasonBundleLookupFailed,
126+
Message: err.Error(),
127+
ObservedGeneration: op.GetGeneration(),
128+
})
129+
return ctrl.Result{}, err
130+
}
131+
if packageName == op.Spec.PackageName {
132+
bundlePath, err := v.BundleEntity().BundlePath()
120133
if err != nil {
134+
apimeta.SetStatusCondition(&op.Status.Conditions, metav1.Condition{
135+
Type: operatorsv1alpha1.TypeReady,
136+
Status: metav1.ConditionFalse,
137+
Reason: operatorsv1alpha1.ReasonBundleLookupFailed,
138+
Message: err.Error(),
139+
ObservedGeneration: op.GetGeneration(),
140+
})
121141
return ctrl.Result{}, err
122142
}
123-
if packageName == op.Spec.PackageName {
124-
bundlePath, err := v.BundleEntity().BundlePath()
125-
if err != nil {
126-
return ctrl.Result{}, err
127-
}
128-
// TODO(perdasilva): use bundlePath to stamp out bundle deployment
129-
_ = bundlePath
130-
break
131-
}
143+
// TODO(perdasilva): use bundlePath to stamp out bundle deployment
144+
_ = bundlePath
145+
break
132146
}
133147
}
134148
}
135149

136150
// update operator status
137151
apimeta.SetStatusCondition(&op.Status.Conditions, metav1.Condition{
138152
Type: operatorsv1alpha1.TypeReady,
139-
Status: status,
140-
Reason: reason,
141-
Message: message,
153+
Status: metav1.ConditionTrue,
154+
Reason: operatorsv1alpha1.ReasonResolutionSucceeded,
155+
Message: "resolution was successful",
142156
ObservedGeneration: op.GetGeneration(),
143157
})
144158

@@ -147,8 +161,6 @@ func (r *OperatorReconciler) reconcile(ctx context.Context, op *operatorsv1alpha
147161

148162
// SetupWithManager sets up the controller with the Manager.
149163
func (r *OperatorReconciler) SetupWithManager(mgr ctrl.Manager) error {
150-
r.resolver = resolution.NewOperatorResolver(mgr.GetClient(), resolution.HardcodedEntitySource)
151-
152164
err := ctrl.NewControllerManagedBy(mgr).
153165
For(&operatorsv1alpha1.Operator{}).
154166
Complete(r)

‎controllers/operator_controller_test.go‎

Lines changed: 214 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,214 @@
1+
package controllers_test
2+
3+
import (
4+
"context"
5+
"fmt"
6+
7+
. "github.com/onsi/ginkgo/v2"
8+
. "github.com/onsi/gomega"
9+
"github.com/operator-framework/deppy/pkg/deppy"
10+
"github.com/operator-framework/deppy/pkg/deppy/input"
11+
apimeta "k8s.io/apimachinery/pkg/api/meta"
12+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
13+
"k8s.io/apimachinery/pkg/types"
14+
"k8s.io/apimachinery/pkg/util/rand"
15+
ctrl "sigs.k8s.io/controller-runtime"
16+
"sigs.k8s.io/controller-runtime/pkg/client"
17+
18+
operatorsv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1"
19+
"github.com/operator-framework/operator-controller/controllers"
20+
"github.com/operator-framework/operator-controller/internal/resolution"
21+
operatorutil "github.com/operator-framework/operator-controller/internal/util"
22+
)
23+
24+
var _ = Describe("Reconcile Test", func() {
25+
var (
26+
ctx context.Context
27+
reconciler *controllers.OperatorReconciler
28+
)
29+
BeforeEach(func() {
30+
ctx = context.Background()
31+
reconciler = &controllers.OperatorReconciler{
32+
Client: cl,
33+
Scheme: sch,
34+
Resolver: resolution.NewOperatorResolver(cl, testEntitySource),
35+
}
36+
})
37+
When("the operator does not exist", func() {
38+
It("returns no error", func() {
39+
res, err := reconciler.Reconcile(context.Background(), ctrl.Request{NamespacedName: types.NamespacedName{Name: "non-existent"}})
40+
Expect(res).To(Equal(ctrl.Result{}))
41+
Expect(err).NotTo(HaveOccurred())
42+
})
43+
})
44+
When("the operator exists", func() {
45+
var (
46+
operator *operatorsv1alpha1.Operator
47+
opKey types.NamespacedName
48+
)
49+
BeforeEach(func() {
50+
opKey = types.NamespacedName{Name: fmt.Sprintf("operator-test-%s", rand.String(8))}
51+
})
52+
When("the operator specifies a non-existent package", func() {
53+
var pkgName string
54+
BeforeEach(func() {
55+
By("initializing cluster state")
56+
pkgName = fmt.Sprintf("non-existent-%s", rand.String(6))
57+
operator = &operatorsv1alpha1.Operator{
58+
ObjectMeta: metav1.ObjectMeta{Name: opKey.Name},
59+
Spec: operatorsv1alpha1.OperatorSpec{PackageName: pkgName},
60+
}
61+
err := cl.Create(ctx, operator)
62+
Expect(err).NotTo(HaveOccurred())
63+
64+
By("running reconcile")
65+
res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: opKey})
66+
Expect(res).To(Equal(ctrl.Result{}))
67+
Expect(err).To(BeNil())
68+
69+
By("fetching updated operator after reconcile")
70+
Expect(cl.Get(ctx, opKey, operator)).NotTo(HaveOccurred())
71+
})
72+
It("sets resolution failure status", func() {
73+
cond := apimeta.FindStatusCondition(operator.Status.Conditions, operatorsv1alpha1.TypeReady)
74+
Expect(cond).NotTo(BeNil())
75+
Expect(cond.Status).To(Equal(metav1.ConditionFalse))
76+
Expect(cond.Reason).To(Equal(operatorsv1alpha1.ReasonResolutionFailed))
77+
Expect(cond.Message).To(Equal(fmt.Sprintf("package '%s' not found", pkgName)))
78+
})
79+
})
80+
When("the operator specifies a valid available package", func() {
81+
const pkgName = "prometheus"
82+
BeforeEach(func() {
83+
By("initializing cluster state")
84+
operator = &operatorsv1alpha1.Operator{
85+
ObjectMeta: metav1.ObjectMeta{Name: opKey.Name},
86+
Spec: operatorsv1alpha1.OperatorSpec{PackageName: pkgName},
87+
}
88+
err := cl.Create(ctx, operator)
89+
Expect(err).NotTo(HaveOccurred())
90+
91+
By("running reconcile")
92+
res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: opKey})
93+
Expect(res).To(Equal(ctrl.Result{}))
94+
Expect(err).To(BeNil())
95+
96+
By("fetching updated operator after reconcile")
97+
Expect(cl.Get(ctx, opKey, operator)).NotTo(HaveOccurred())
98+
})
99+
It("sets resolution success status", func() {
100+
cond := apimeta.FindStatusCondition(operator.Status.Conditions, operatorsv1alpha1.TypeReady)
101+
Expect(cond).NotTo(BeNil())
102+
Expect(cond.Status).To(Equal(metav1.ConditionTrue))
103+
Expect(cond.Reason).To(Equal(operatorsv1alpha1.ReasonResolutionSucceeded))
104+
Expect(cond.Message).To(Equal("resolution was successful"))
105+
})
106+
})
107+
When("the selected bundle's image ref cannot be parsed", func() {
108+
const pkgName = "badimage"
109+
BeforeEach(func() {
110+
By("initializing cluster state")
111+
operator = &operatorsv1alpha1.Operator{
112+
ObjectMeta: metav1.ObjectMeta{Name: opKey.Name},
113+
Spec: operatorsv1alpha1.OperatorSpec{PackageName: pkgName},
114+
}
115+
err := cl.Create(ctx, operator)
116+
Expect(err).NotTo(HaveOccurred())
117+
})
118+
It("sets resolution failure status and returns an error", func() {
119+
By("running reconcile")
120+
res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: opKey})
121+
Expect(res).To(Equal(ctrl.Result{}))
122+
Expect(err).To(MatchError(ContainSubstring(`error determining bundle path for entity`)))
123+
124+
By("fetching updated operator after reconcile")
125+
Expect(cl.Get(ctx, opKey, operator)).NotTo(HaveOccurred())
126+
127+
cond := apimeta.FindStatusCondition(operator.Status.Conditions, operatorsv1alpha1.TypeReady)
128+
Expect(cond).NotTo(BeNil())
129+
Expect(cond.Status).To(Equal(metav1.ConditionFalse))
130+
Expect(cond.Reason).To(Equal(operatorsv1alpha1.ReasonBundleLookupFailed))
131+
Expect(cond.Message).To(ContainSubstring(`error determining bundle path for entity`))
132+
})
133+
})
134+
When("the operator specifies a duplicate package", func() {
135+
const pkgName = "prometheus"
136+
BeforeEach(func() {
137+
By("initializing cluster state")
138+
err := cl.Create(ctx, &operatorsv1alpha1.Operator{
139+
ObjectMeta: metav1.ObjectMeta{Name: fmt.Sprintf("orig-%s", opKey.Name)},
140+
Spec: operatorsv1alpha1.OperatorSpec{PackageName: pkgName},
141+
})
142+
Expect(err).NotTo(HaveOccurred())
143+
144+
operator = &operatorsv1alpha1.Operator{
145+
ObjectMeta: metav1.ObjectMeta{Name: opKey.Name},
146+
Spec: operatorsv1alpha1.OperatorSpec{PackageName: pkgName},
147+
}
148+
err = cl.Create(ctx, operator)
149+
Expect(err).NotTo(HaveOccurred())
150+
151+
By("running reconcile")
152+
res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: opKey})
153+
Expect(res).To(Equal(ctrl.Result{}))
154+
Expect(err).To(BeNil())
155+
156+
By("fetching updated operator after reconcile")
157+
Expect(cl.Get(ctx, opKey, operator)).NotTo(HaveOccurred())
158+
})
159+
It("sets resolution failure status", func() {
160+
cond := apimeta.FindStatusCondition(operator.Status.Conditions, operatorsv1alpha1.TypeReady)
161+
Expect(cond).NotTo(BeNil())
162+
Expect(cond.Status).To(Equal(metav1.ConditionFalse))
163+
Expect(cond.Reason).To(Equal(operatorsv1alpha1.ReasonResolutionFailed))
164+
Expect(cond.Message).To(Equal(`duplicate identifier "required package prometheus" in input`))
165+
})
166+
})
167+
AfterEach(func() {
168+
verifyInvariants(ctx, operator)
169+
170+
err := cl.Delete(ctx, operator)
171+
Expect(err).To(Not(HaveOccurred()))
172+
})
173+
})
174+
})
175+
176+
func verifyInvariants(ctx context.Context, op *operatorsv1alpha1.Operator) {
177+
key := client.ObjectKeyFromObject(op)
178+
err := cl.Get(ctx, key, op)
179+
Expect(err).To(BeNil())
180+
181+
verifyConditionsInvariants(op)
182+
}
183+
184+
func verifyConditionsInvariants(op *operatorsv1alpha1.Operator) {
185+
// Expect that the operator's set of conditions contains all defined
186+
// condition types for the Operator API. Every reconcile should always
187+
// ensure every condition type's status/reason/message reflects the state
188+
// read during _this_ reconcile call.
189+
Expect(op.Status.Conditions).To(HaveLen(len(operatorutil.ConditionTypes)))
190+
for _, t := range operatorutil.ConditionTypes {
191+
cond := apimeta.FindStatusCondition(op.Status.Conditions, t)
192+
Expect(cond).To(Not(BeNil()))
193+
Expect(cond.Status).NotTo(BeEmpty())
194+
Expect(cond.Reason).To(BeElementOf(operatorutil.ConditionReasons))
195+
Expect(cond.ObservedGeneration).To(Equal(op.GetGeneration()))
196+
}
197+
}
198+
199+
var testEntitySource = input.NewCacheQuerier(map[deppy.Identifier]input.Entity{
200+
"operatorhub/prometheus/0.37.0": *input.NewEntity("operatorhub/prometheus/0.37.0", map[string]string{
201+
"olm.bundle.path": `"quay.io/operatorhubio/prometheus@sha256:3e281e587de3d03011440685fc4fb782672beab044c1ebadc42788ce05a21c35"`,
202+
"olm.channel": `{"channelName":"beta","priority":0}`,
203+
"olm.package": `{"packageName":"prometheus","version":"0.37.0"}`,
204+
}),
205+
"operatorhub/prometheus/0.47.0": *input.NewEntity("operatorhub/prometheus/0.47.0", map[string]string{
206+
"olm.bundle.path": `"quay.io/operatorhubio/prometheus@sha256:5b04c49d8d3eff6a338b56ec90bdf491d501fe301c9cdfb740e5bff6769a21ed"`,
207+
"olm.channel": `{"channelName":"beta","priority":0,"replaces":"prometheusoperator.0.37.0"}`,
208+
"olm.package": `{"packageName":"prometheus","version":"0.47.0"}`,
209+
}),
210+
"operatorhub/badimage/0.1.0": *input.NewEntity("operatorhub/badimage/0.1.0", map[string]string{
211+
"olm.bundle.path": `{"name": "quay.io/operatorhubio/badimage:v0.1.0"}`,
212+
"olm.package": `{"packageName":"badimage","version":"0.1.0"}`,
213+
}),
214+
})

0 commit comments

Comments
 (0)