Skip to content

Commit 370bac6

Browse files
author
Per Goncalves da Silva
committed
Refactor revision controller and add unit tests"
Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com>
1 parent 2e35363 commit 370bac6

File tree

4 files changed

+261
-24
lines changed

4 files changed

+261
-24
lines changed

‎cmd/operator-controller/main.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -534,9 +534,9 @@ func run() error {
534534
}
535535

536536
if err = (&controllers.ClusterExtensionRevisionReconciler{
537-
Client: cl,
538-
AccessManager: accessManager,
539-
RevisionEngineGetter: controllers.OLMRevisionEngineGetter{
537+
Client: cl,
538+
RevisionManager: &controllers.OLMRevisionEngineGetter{
539+
AccessManager: accessManager,
540540
Scheme: mgr.GetScheme(),
541541
RestMapper: mgr.GetRESTMapper(),
542542
DiscoveryClient: discoveryClient,

‎internal/operator-controller/controllers/clusterextensionrevision_controller.go

Lines changed: 28 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ import (
66
"context"
77
"encoding/json"
88
"fmt"
9-
types2 "pkg.package-operator.run/boxcutter/machinery/types"
109
"strings"
1110
"time"
1211

@@ -22,6 +21,7 @@ import (
2221
"k8s.io/utils/ptr"
2322
"pkg.package-operator.run/boxcutter"
2423
"pkg.package-operator.run/boxcutter/machinery"
24+
machinerytypes "pkg.package-operator.run/boxcutter/machinery/types"
2525
"pkg.package-operator.run/boxcutter/managedcache"
2626
"pkg.package-operator.run/boxcutter/ownerhandling"
2727
"pkg.package-operator.run/boxcutter/validation"
@@ -38,18 +38,16 @@ import (
3838
)
3939

4040
const (
41-
ClusterExtensionRevisionOwnerLabel = "olm.operatorframework.io/owner"
42-
41+
ClusterExtensionRevisionOwnerLabel = "olm.operatorframework.io/owner"
4342
boxcutterSystemPrefixFieldOwner = "olm.operatorframework.io"
4443
clusterExtensionRevisionTeardownFinalizer = "olm.operatorframework.io/teardown"
4544
)
4645

4746
// ClusterExtensionRevisionReconciler actions individual snapshots of ClusterExtensions,
4847
// as part of the boxcutter integration.
4948
type ClusterExtensionRevisionReconciler struct {
50-
Client client.Client
51-
AccessManager AccessManager
52-
RevisionEngineGetter RevisionEngineGetter
49+
Client client.Client
50+
RevisionManager RevisionManager
5351
}
5452

5553
type AccessManager interface {
@@ -59,21 +57,28 @@ type AccessManager interface {
5957
}
6058

6159
type RevisionEngine interface {
62-
Teardown(ctx context.Context, rev types2.Revision, opts ...types2.RevisionTeardownOption) (machinery.RevisionTeardownResult, error)
63-
Reconcile(ctx context.Context, rev types2.Revision, opts ...types2.RevisionReconcileOption) (machinery.RevisionResult, error)
60+
Teardown(ctx context.Context, rev machinerytypes.Revision, opts ...machinerytypes.RevisionTeardownOption) (machinery.RevisionTeardownResult, error)
61+
Reconcile(ctx context.Context, rev machinerytypes.Revision, opts ...machinerytypes.RevisionReconcileOption) (machinery.RevisionResult, error)
6462
}
6563

66-
type RevisionEngineGetter interface {
67-
GetRevisionEngineWithAccessor(accessor managedcache.Accessor) RevisionEngine
64+
type RevisionManager interface {
65+
GetScopedRevisionEngine(ctx context.Context, owner *ocv1.ClusterExtension, user client.Object, usedFor []client.Object) (RevisionEngine, error)
66+
HandleDeletion(ctx context.Context, owner *ocv1.ClusterExtension, user client.Object) error
67+
Source(handler.EventHandler, ...predicate.Predicate) source.Source
6868
}
6969

7070
type OLMRevisionEngineGetter struct {
7171
DiscoveryClient *discovery.DiscoveryClient
7272
Scheme *runtime.Scheme
7373
RestMapper meta.RESTMapper
74+
AccessManager AccessManager
7475
}
7576

76-
func (r *OLMRevisionEngineGetter) GetRevisionEngineWithAccessor(accessor managedcache.Accessor) RevisionEngine {
77+
func (r *OLMRevisionEngineGetter) GetScopedRevisionEngine(ctx context.Context, owner *ocv1.ClusterExtension, user client.Object, usedFor []client.Object) (RevisionEngine, error) {
78+
accessor, err := r.AccessManager.GetWithUser(ctx, owner, user, usedFor)
79+
if err != nil {
80+
return nil, fmt.Errorf("get cache: %w", err)
81+
}
7782
return machinery.NewRevisionEngine(
7883
machinery.NewPhaseEngine(
7984
machinery.NewObjectEngine(
@@ -85,7 +90,15 @@ func (r *OLMRevisionEngineGetter) GetRevisionEngineWithAccessor(accessor managed
8590
validation.NewClusterPhaseValidator(r.RestMapper, accessor),
8691
),
8792
validation.NewRevisionValidator(), accessor,
88-
)
93+
), nil
94+
}
95+
96+
func (r *OLMRevisionEngineGetter) HandleDeletion(ctx context.Context, owner *ocv1.ClusterExtension, user client.Object) error {
97+
return r.AccessManager.FreeWithUser(ctx, owner, user)
98+
}
99+
100+
func (r *OLMRevisionEngineGetter) Source(eventHandler handler.EventHandler, p ...predicate.Predicate) source.Source {
101+
return r.AccessManager.Source(eventHandler, p...)
89102
}
90103

91104
//+kubebuilder:rbac:groups=olm.operatorframework.io,resources=clusterextensionrevisions,verbs=get;list;watch;update;patch;create;delete
@@ -148,12 +161,7 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, ce *
148161
}
149162
}
150163

151-
accessor, err := c.AccessManager.GetWithUser(ctx, ce, rev, objects)
152-
if err != nil {
153-
return ctrl.Result{}, fmt.Errorf("get cache: %w", err)
154-
}
155-
156-
re, err := c.RevisionEngineGetter.GetRevisionEngineWithAccessor(accessor)
164+
re, err := c.RevisionManager.GetScopedRevisionEngine(ctx, ce, rev, objects)
157165
if err != nil {
158166
return ctrl.Result{}, err
159167
}
@@ -173,7 +181,7 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, ce *
173181
if !tres.IsComplete() {
174182
return ctrl.Result{}, nil
175183
}
176-
if err := c.AccessManager.FreeWithUser(ctx, ce, rev); err != nil {
184+
if err := c.RevisionManager.HandleDeletion(ctx, ce, rev); err != nil {
177185
return ctrl.Result{}, fmt.Errorf("get cache: %w", err)
178186
}
179187
return ctrl.Result{}, c.removeFinalizer(ctx, rev, clusterExtensionRevisionTeardownFinalizer)
@@ -293,7 +301,7 @@ func (c *ClusterExtensionRevisionReconciler) SetupWithManager(mgr ctrl.Manager)
293301
builder.WithPredicates(predicate.ResourceVersionChangedPredicate{}),
294302
).
295303
WatchesRawSource(
296-
c.AccessManager.Source(
304+
c.RevisionManager.Source(
297305
handler.EnqueueRequestForOwner(mgr.GetScheme(), mgr.GetRESTMapper(), &ocv1.ClusterExtensionRevision{}),
298306
predicate.ResourceVersionChangedPredicate{},
299307
),

‎internal/operator-controller/controllers/clusterextensionrevision_controller_test.go

Lines changed: 229 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1 +1,230 @@
11
package controllers_test
2+
3+
import (
4+
"context"
5+
"testing"
6+
7+
"github.com/stretchr/testify/require"
8+
"k8s.io/apimachinery/pkg/api/meta"
9+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
10+
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
11+
"k8s.io/apimachinery/pkg/types"
12+
"k8s.io/utils/ptr"
13+
"pkg.package-operator.run/boxcutter/machinery"
14+
machinerytypes "pkg.package-operator.run/boxcutter/machinery/types"
15+
"pkg.package-operator.run/boxcutter/validation"
16+
ctrl "sigs.k8s.io/controller-runtime"
17+
"sigs.k8s.io/controller-runtime/pkg/client"
18+
"sigs.k8s.io/controller-runtime/pkg/handler"
19+
"sigs.k8s.io/controller-runtime/pkg/predicate"
20+
"sigs.k8s.io/controller-runtime/pkg/source"
21+
22+
ocv1 "github.com/operator-framework/operator-controller/api/v1"
23+
"github.com/operator-framework/operator-controller/internal/operator-controller/controllers"
24+
)
25+
26+
func Test_Reconcile_InitialRevision(t *testing.T) {
27+
cl := newClient(t)
28+
29+
// existing objects
30+
ext := &ocv1.ClusterExtension{
31+
ObjectMeta: metav1.ObjectMeta{
32+
Name: "ext",
33+
UID: types.UID("ext-uid"),
34+
},
35+
Spec: ocv1.ClusterExtensionSpec{
36+
Namespace: "some-namespace",
37+
ServiceAccount: ocv1.ServiceAccountReference{
38+
Name: "service-account",
39+
},
40+
Source: ocv1.SourceConfig{
41+
SourceType: ocv1.SourceTypeCatalog,
42+
Catalog: &ocv1.CatalogFilter{
43+
PackageName: "some-package",
44+
},
45+
},
46+
},
47+
}
48+
49+
rev1 := &ocv1.ClusterExtensionRevision{
50+
ObjectMeta: metav1.ObjectMeta{
51+
Name: "ext-1",
52+
UID: types.UID("ext-1-uid"),
53+
OwnerReferences: []metav1.OwnerReference{
54+
{
55+
APIVersion: ocv1.GroupVersion.String(),
56+
Kind: "ClusterExtension",
57+
Name: ext.Name,
58+
UID: ext.UID,
59+
Controller: ptr.To(true),
60+
},
61+
},
62+
},
63+
Spec: ocv1.ClusterExtensionRevisionSpec{
64+
Phases: []ocv1.ClusterExtensionRevisionPhase{
65+
{
66+
Name: "everything",
67+
Objects: []ocv1.ClusterExtensionRevisionObject{
68+
{
69+
Object: unstructured.Unstructured{
70+
Object: map[string]interface{}{
71+
"apiVersion": "v1",
72+
"kind": "ConfigMap",
73+
"data": map[string]interface{}{
74+
"foo": "bar",
75+
},
76+
},
77+
},
78+
},
79+
},
80+
},
81+
},
82+
},
83+
}
84+
85+
// create reconciler
86+
r := &controllers.ClusterExtensionRevisionReconciler{
87+
Client: cl,
88+
RevisionManager: mockRevisionManager{
89+
getScopedRevisionEngine: func(ctx context.Context, owner *ocv1.ClusterExtension, user client.Object, usedFor []client.Object) (controllers.RevisionEngine, error) {
90+
return &mockRevisionEngine{
91+
reconcile: func(ctx context.Context, rev machinerytypes.Revision, opts ...machinerytypes.RevisionReconcileOption) (machinery.RevisionResult, error) {
92+
return &mockRevisionResult{
93+
validationError: nil,
94+
isComplete: false,
95+
inTransition: false,
96+
}, nil
97+
},
98+
}, nil
99+
},
100+
},
101+
}
102+
103+
// create pre-reconcile cluster state
104+
require.NoError(t, cl.Create(t.Context(), ext))
105+
require.NoError(t, cl.Create(t.Context(), rev1))
106+
107+
result, err := r.Reconcile(t.Context(), ctrl.Request{
108+
NamespacedName: types.NamespacedName{
109+
Name: rev1.GetName(),
110+
Namespace: rev1.GetNamespace(),
111+
},
112+
})
113+
require.Equal(t, ctrl.Result{}, result)
114+
require.NoError(t, err)
115+
116+
rev := &ocv1.ClusterExtensionRevision{}
117+
err = cl.Get(t.Context(), client.ObjectKey{
118+
Name: rev1.GetName(),
119+
Namespace: rev1.GetNamespace(),
120+
}, rev)
121+
require.NoError(t, err)
122+
cond := meta.FindStatusCondition(rev.Status.Conditions, "Available")
123+
require.NotNil(t, cond)
124+
require.Equal(t, metav1.ConditionFalse, cond.Status)
125+
require.Equal(t, "Incomplete", cond.Reason)
126+
require.Equal(t, "Revision has not been rolled out completely.", cond.Message)
127+
require.Equal(t, int64(1), cond.ObservedGeneration)
128+
}
129+
130+
type mockRevisionManager struct {
131+
getScopedRevisionEngine func(ctx context.Context, owner *ocv1.ClusterExtension, user client.Object, usedFor []client.Object) (controllers.RevisionEngine, error)
132+
handleDeletion func(ctx context.Context, owner *ocv1.ClusterExtension, user client.Object) error
133+
source func(eventHandler handler.EventHandler, p ...predicate.Predicate) source.Source
134+
}
135+
136+
func (m mockRevisionManager) HandleDeletion(ctx context.Context, owner *ocv1.ClusterExtension, user client.Object) error {
137+
return m.handleDeletion(ctx, owner, user)
138+
}
139+
140+
func (m mockRevisionManager) Source(eventHandler handler.EventHandler, p ...predicate.Predicate) source.Source {
141+
return m.source(eventHandler, p...)
142+
}
143+
144+
func (m mockRevisionManager) GetScopedRevisionEngine(ctx context.Context, owner *ocv1.ClusterExtension, user client.Object, usedFor []client.Object) (controllers.RevisionEngine, error) {
145+
return m.getScopedRevisionEngine(ctx, owner, user, usedFor)
146+
}
147+
148+
type mockRevisionEngine struct {
149+
teardown func(ctx context.Context, rev machinerytypes.Revision, opts ...machinerytypes.RevisionTeardownOption) (machinery.RevisionTeardownResult, error)
150+
reconcile func(ctx context.Context, rev machinerytypes.Revision, opts ...machinerytypes.RevisionReconcileOption) (machinery.RevisionResult, error)
151+
}
152+
153+
func (m mockRevisionEngine) Teardown(ctx context.Context, rev machinerytypes.Revision, opts ...machinerytypes.RevisionTeardownOption) (machinery.RevisionTeardownResult, error) {
154+
return m.teardown(ctx, rev)
155+
}
156+
157+
func (m mockRevisionEngine) Reconcile(ctx context.Context, rev machinerytypes.Revision, opts ...machinerytypes.RevisionReconcileOption) (machinery.RevisionResult, error) {
158+
return m.reconcile(ctx, rev)
159+
}
160+
161+
type mockRevisionResult struct {
162+
validationError *validation.RevisionValidationError
163+
phases []machinery.PhaseResult
164+
inTransition bool
165+
isComplete bool
166+
hasProgressed bool
167+
string string
168+
}
169+
170+
func (m mockRevisionResult) GetValidationError() *validation.RevisionValidationError {
171+
return m.validationError
172+
}
173+
174+
func (m mockRevisionResult) GetPhases() []machinery.PhaseResult {
175+
return m.phases
176+
}
177+
178+
func (m mockRevisionResult) InTransistion() bool {
179+
return m.inTransition
180+
}
181+
182+
func (m mockRevisionResult) IsComplete() bool {
183+
return m.isComplete
184+
}
185+
186+
func (m mockRevisionResult) HasProgressed() bool {
187+
return m.hasProgressed
188+
}
189+
190+
func (m mockRevisionResult) String() string {
191+
return m.string
192+
}
193+
194+
//type mockPhaseResult struct {
195+
// name string
196+
// validationError *validation.PhaseValidationError
197+
// objects []machinery.ObjectResult
198+
// inTransition bool
199+
// isComplete bool
200+
// hasProgressed bool
201+
// string string
202+
//}
203+
//
204+
//func (m mockPhaseResult) GetName() string {
205+
// return m.name
206+
//}
207+
//
208+
//func (m mockPhaseResult) GetValidationError() *validation.PhaseValidationError {
209+
// return m.validationError
210+
//}
211+
//
212+
//func (m mockPhaseResult) GetObjects() []machinery.ObjectResult {
213+
// return m.objects
214+
//}
215+
//
216+
//func (m mockPhaseResult) InTransistion() bool {
217+
// return m.inTransition
218+
//}
219+
//
220+
//func (m mockPhaseResult) IsComplete() bool {
221+
// return m.isComplete
222+
//}
223+
//
224+
//func (m mockPhaseResult) HasProgressed() bool {
225+
// return m.hasProgressed
226+
//}
227+
//
228+
//func (m mockPhaseResult) String() string {
229+
// return m.string
230+
//}

‎internal/operator-controller/controllers/suite_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -138,7 +138,7 @@ var (
138138
func TestMain(m *testing.M) {
139139
testEnv := &envtest.Environment{
140140
CRDDirectoryPaths: []string{
141-
filepath.Join("..", "..", "..", "config", "base", "operator-controller", "crd", "standard"),
141+
filepath.Join("..", "..", "..", "config", "base", "operator-controller", "crd", "experimental"),
142142
},
143143
ErrorIfCRDPathMissing: true,
144144
}

0 commit comments

Comments
 (0)