Skip to content

Commit 8187c1b

Browse files
authored
Merge pull request #37 from munnerz/manager-testing
Add more exhaustive unit testing for manager
2 parents eb8bb6d + 2b45933 commit 8187c1b

File tree

7 files changed

+609
-150
lines changed

7 files changed

+609
-150
lines changed

manager/manager.go

Lines changed: 21 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -189,6 +189,9 @@ func NewManager(opts Options) (*Manager, error) {
189189
maxRequestsPerVolume: opts.MaxRequestsPerVolume,
190190
nodeNameHash: nodeNameHash,
191191
backoffConfig: *opts.RenewalBackoffConfig,
192+
requestNameGenerator: func() string {
193+
return string(uuid.NewUUID())
194+
},
192195
}
193196

194197
vols, err := opts.MetadataReader.ListVolumes()
@@ -287,6 +290,10 @@ type Manager struct {
287290

288291
// backoffConfig configures the exponential backoff applied to certificate renewal failures.
289292
backoffConfig wait.Backoff
293+
294+
// requestNameGenerator generates a new random name for a certificaterequest object
295+
// Defaults to uuid.NewUUID() from k8s.io/apimachinery/pkg/util/uuid.
296+
requestNameGenerator func() string
290297
}
291298

292299
// issue will step through the entire issuance flow for a volume.
@@ -334,7 +341,7 @@ func (m *Manager) issue(ctx context.Context, volumeID string) error {
334341

335342
// Poll every 1s for the CertificateRequest to be ready
336343
lastFailureReason := ""
337-
if err := wait.PollUntil(time.Second, func() (done bool, err error) {
344+
if err := wait.PollUntilWithContext(ctx, time.Second, func(ctx context.Context) (done bool, err error) {
338345
updatedReq, err := m.lister.CertificateRequests(req.Namespace).Get(req.Name)
339346
if apierrors.IsNotFound(err) {
340347
// A NotFound error implies something deleted the resource - fail
@@ -358,8 +365,9 @@ func (m *Manager) issue(ctx context.Context, volumeID string) error {
358365
return false, fmt.Errorf("request %q has been denied by the approval plugin: %s", updatedReq.Name, cond.Message)
359366
}
360367

361-
if !apiutil.CertificateRequestIsApproved(updatedReq) {
362-
lastFailureReason = "request has not yet been approved by approval plugin"
368+
isApproved := apiutil.CertificateRequestIsApproved(updatedReq)
369+
if !isApproved {
370+
lastFailureReason = fmt.Sprintf("request %q has not yet been approved by approval plugin", updatedReq.Name)
363371
// we don't stop execution here, as some versions of cert-manager (and some external issuer plugins)
364372
// may not be aware/utilise approval.
365373
// If the certificate is still issued despite never being approved, the CSI driver should continue
@@ -368,11 +376,11 @@ func (m *Manager) issue(ctx context.Context, volumeID string) error {
368376

369377
readyCondition := apiutil.GetCertificateRequestCondition(updatedReq, cmapi.CertificateRequestConditionReady)
370378
if readyCondition == nil {
371-
if apiutil.CertificateRequestIsApproved(updatedReq) {
372-
lastFailureReason = "request has no ready condition"
379+
// only overwrite the approval failure message if the request is actually approved
380+
// otherwise we may hide more useful information from the user by accident.
381+
if isApproved {
382+
lastFailureReason = fmt.Sprintf("request %q has no ready condition", updatedReq.Name)
373383
}
374-
log.V(2).Info("CertificateRequest is still pending")
375-
// Issuance is still pending
376384
return false, nil
377385
}
378386

@@ -382,11 +390,12 @@ func (m *Manager) issue(ctx context.Context, volumeID string) error {
382390
case cmapi.CertificateRequestReasonFailed:
383391
return false, fmt.Errorf("request %q has failed: %s", updatedReq.Name, readyCondition.Message)
384392
case cmapi.CertificateRequestReasonPending:
385-
lastFailureReason = fmt.Sprintf("request pending: %v", readyCondition.Message)
386-
log.V(2).Info("CertificateRequest is still pending")
393+
if isApproved {
394+
lastFailureReason = fmt.Sprintf("request %q is pending: %v", updatedReq.Name, readyCondition.Message)
395+
}
387396
return false, nil
388397
default:
389-
lastFailureReason = fmt.Sprintf("unrecognised Ready condition state (%s): %s", readyCondition.Reason, readyCondition.Message)
398+
lastFailureReason = fmt.Sprintf("request %q has unrecognised Ready condition state (%s): %s", updatedReq.Name, readyCondition.Reason, readyCondition.Message)
390399
log.Info("unrecognised state for Ready condition", "request_namespace", updatedReq.Namespace, "request_name", updatedReq.Name, "condition", *readyCondition)
391400
return false, nil
392401
}
@@ -396,7 +405,7 @@ func (m *Manager) issue(ctx context.Context, volumeID string) error {
396405
// writing out signed certificate
397406
req = updatedReq
398407
return true, nil
399-
}, ctx.Done()); err != nil {
408+
}); err != nil {
400409
if errors.Is(err, wait.ErrWaitTimeout) {
401410
// try and return a more helpful error message than "timed out waiting for the condition"
402411
return fmt.Errorf("waiting for request: %s", lastFailureReason)
@@ -481,7 +490,7 @@ func (m *Manager) labelSelectorForVolume(volumeID string) (labels.Selector, erro
481490
func (m *Manager) submitRequest(ctx context.Context, meta metadata.Metadata, csrBundle *CertificateRequestBundle, csrPEM []byte) (*cmapi.CertificateRequest, error) {
482491
req := &cmapi.CertificateRequest{
483492
ObjectMeta: metav1.ObjectMeta{
484-
Name: string(uuid.NewUUID()),
493+
Name: m.requestNameGenerator(),
485494
Namespace: csrBundle.Namespace,
486495
Annotations: csrBundle.Annotations,
487496
Labels: map[string]string{

manager/manager_test.go

Lines changed: 264 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,19 +2,283 @@ package manager
22

33
import (
44
"context"
5+
"fmt"
56
"testing"
7+
"time"
68

79
cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1"
10+
cmmeta "github.com/cert-manager/cert-manager/pkg/apis/meta/v1"
811
testpkg "github.com/cert-manager/cert-manager/pkg/controller/test"
912
"github.com/go-logr/logr/testr"
1013
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1114
"k8s.io/apimachinery/pkg/runtime"
15+
"k8s.io/apimachinery/pkg/util/wait"
1216
coretesting "k8s.io/client-go/testing"
1317

1418
internalapi "github.com/cert-manager/csi-lib/internal/api"
1519
internalapiutil "github.com/cert-manager/csi-lib/internal/api/util"
20+
"github.com/cert-manager/csi-lib/metadata"
21+
"github.com/cert-manager/csi-lib/storage"
22+
testutil "github.com/cert-manager/csi-lib/test/util"
1623
)
1724

25+
func TestManager_ManageVolumeImmediate_issueOnceAndSucceed(t *testing.T) {
26+
ctx := context.Background()
27+
28+
opts := newDefaultTestOptions(t)
29+
m, err := NewManager(opts)
30+
if err != nil {
31+
t.Fatal(err)
32+
}
33+
34+
// Setup a goroutine to issue one CertificateRequest
35+
stopCh := make(chan struct{})
36+
go testutil.IssueOneRequest(t, opts.Client, defaultTestNamespace, stopCh, selfSignedExampleCertificate, []byte("ca bytes"))
37+
defer close(stopCh)
38+
39+
// Register a new volume with the metadata store
40+
store := opts.MetadataReader.(storage.Interface)
41+
meta := metadata.Metadata{
42+
VolumeID: "vol-id",
43+
TargetPath: "/fake/path",
44+
}
45+
store.RegisterMetadata(meta)
46+
// Ensure we stop managing the volume after the test
47+
defer func() {
48+
store.RemoveVolume(meta.VolumeID)
49+
m.UnmanageVolume(meta.VolumeID)
50+
}()
51+
52+
// Attempt to issue the certificate & put it under management
53+
managed, err := m.ManageVolumeImmediate(ctx, meta.VolumeID)
54+
if !managed {
55+
t.Errorf("expected management to have started, but it did not")
56+
}
57+
if err != nil {
58+
t.Errorf("expected no error from ManageVolumeImmediate but got: %v", err)
59+
}
60+
61+
// Assert the certificate is under management
62+
if !m.IsVolumeReady(meta.VolumeID) {
63+
t.Errorf("expected volume to be marked as Ready but it is not")
64+
}
65+
}
66+
67+
func TestManager_PropagatesRequestConditionMessages(t *testing.T) {
68+
tests := []struct {
69+
approved string
70+
ready string
71+
expectedError string
72+
}{
73+
{
74+
approved: "",
75+
ready: "",
76+
expectedError: "waiting for request: request \"certificaterequest-name\" has not yet been approved by approval plugin",
77+
},
78+
{
79+
approved: "",
80+
ready: "pending",
81+
expectedError: "waiting for request: request \"certificaterequest-name\" has not yet been approved by approval plugin",
82+
},
83+
{
84+
approved: "",
85+
ready: "failed",
86+
expectedError: "waiting for request: request \"certificaterequest-name\" has failed: failed",
87+
},
88+
//"pending approval, ready == true": {}
89+
{
90+
approved: "denied",
91+
ready: "",
92+
expectedError: "waiting for request: request \"certificaterequest-name\" has been denied by the approval plugin: denied",
93+
},
94+
{
95+
approved: "denied",
96+
ready: "pending",
97+
expectedError: "waiting for request: request \"certificaterequest-name\" has been denied by the approval plugin: denied",
98+
},
99+
{
100+
approved: "denied",
101+
ready: "failed",
102+
expectedError: "waiting for request: request \"certificaterequest-name\" has been denied by the approval plugin: denied",
103+
},
104+
//"denied, ready == true": {},
105+
{
106+
approved: "approved",
107+
ready: "",
108+
expectedError: "waiting for request: request \"certificaterequest-name\" has no ready condition",
109+
},
110+
{
111+
approved: "approved",
112+
ready: "pending",
113+
expectedError: "waiting for request: request \"certificaterequest-name\" is pending: pending",
114+
},
115+
{
116+
approved: "approved",
117+
ready: "failed",
118+
expectedError: "waiting for request: request \"certificaterequest-name\" has failed: failed",
119+
},
120+
//"approved, ready == true": {},
121+
}
122+
for _, test := range tests {
123+
t.Run(fmt.Sprintf("approval=%q, readiness=%q", test.approved, test.ready), func(t *testing.T) {
124+
ctx, cancel := context.WithTimeout(context.Background(), time.Second*3)
125+
defer cancel()
126+
127+
opts := newDefaultTestOptions(t)
128+
m, err := NewManager(opts)
129+
if err != nil {
130+
t.Fatal(err)
131+
}
132+
defer m.Stop()
133+
m.requestNameGenerator = func() string { return "certificaterequest-name" }
134+
135+
// build conditions to set based on test configuration
136+
var conditions []cmapi.CertificateRequestCondition
137+
switch test.approved {
138+
case "":
139+
case "approved":
140+
conditions = append(conditions, cmapi.CertificateRequestCondition{Type: cmapi.CertificateRequestConditionApproved, Status: cmmeta.ConditionTrue, Reason: "SetByTest", Message: "approved"})
141+
case "denied":
142+
conditions = append(conditions, cmapi.CertificateRequestCondition{Type: cmapi.CertificateRequestConditionDenied, Status: cmmeta.ConditionTrue, Reason: "SetByTest", Message: "denied"})
143+
}
144+
switch test.ready {
145+
case "":
146+
case "pending":
147+
conditions = append(conditions, cmapi.CertificateRequestCondition{Type: cmapi.CertificateRequestConditionReady, Status: cmmeta.ConditionFalse, Reason: cmapi.CertificateRequestReasonPending, Message: "pending"})
148+
case "failed":
149+
conditions = append(conditions, cmapi.CertificateRequestCondition{Type: cmapi.CertificateRequestConditionReady, Status: cmmeta.ConditionFalse, Reason: cmapi.CertificateRequestReasonFailed, Message: "failed"})
150+
}
151+
// Automatically set the request to be approved & pending once created
152+
go testutil.SetCertificateRequestConditions(ctx, t, opts.Client, defaultTestNamespace, conditions...)
153+
154+
// Register a new volume with the metadata store
155+
store := opts.MetadataReader.(storage.Interface)
156+
meta := metadata.Metadata{
157+
VolumeID: "vol-id",
158+
TargetPath: "/fake/path",
159+
}
160+
store.RegisterMetadata(meta)
161+
// Ensure we stop managing the volume after the test
162+
defer func() {
163+
store.RemoveVolume(meta.VolumeID)
164+
m.UnmanageVolume(meta.VolumeID)
165+
}()
166+
167+
// Attempt to issue the certificate & put it under management
168+
managed, err := m.ManageVolumeImmediate(ctx, meta.VolumeID)
169+
if !managed {
170+
t.Errorf("expected volume to still be managed after failure")
171+
}
172+
if err == nil {
173+
t.Errorf("expected to get an error from ManageVolumeImmediate")
174+
}
175+
if err.Error() != test.expectedError {
176+
t.Errorf("expected '%s' but got: %s", test.expectedError, err.Error())
177+
}
178+
})
179+
}
180+
}
181+
182+
func TestManager_ResumesManagementOfExistingVolumes(t *testing.T) {
183+
ctx, cancel := context.WithTimeout(context.Background(), time.Second*3)
184+
defer cancel()
185+
186+
store := storage.NewMemoryFS()
187+
opts := defaultTestOptions(t, Options{MetadataReader: store})
188+
189+
m, err := NewManager(opts)
190+
if err != nil {
191+
t.Fatal(err)
192+
}
193+
194+
m.requestNameGenerator = func() string { return "certificaterequest-name" }
195+
// Automatically issue the request once created
196+
go testutil.IssueOneRequest(t, opts.Client, defaultTestNamespace, ctx.Done(), selfSignedExampleCertificate, []byte("ca bytes"))
197+
198+
// Register a new volume with the metadata store
199+
meta := metadata.Metadata{
200+
VolumeID: "vol-id",
201+
TargetPath: "/fake/path",
202+
}
203+
_, err = store.RegisterMetadata(meta)
204+
if err != nil {
205+
t.Fatal(err)
206+
}
207+
208+
// Attempt to issue the certificate & put it under management
209+
managed, err := m.ManageVolumeImmediate(ctx, meta.VolumeID)
210+
if !managed {
211+
t.Fatalf("expected volume to be managed")
212+
}
213+
if err != nil {
214+
t.Fatalf("unexpected error: %v", err)
215+
}
216+
217+
// Shutdown the manager
218+
m.Stop()
219+
220+
m, err = NewManager(opts)
221+
if err != nil {
222+
t.Fatal(err)
223+
}
224+
defer m.Stop()
225+
226+
if !m.IsVolumeReady(meta.VolumeID) {
227+
t.Errorf("expected volume to be monitored & ready but it is not")
228+
}
229+
}
230+
231+
func TestManager_ManageVolume_beginsManagingAndProceedsIfNotReady(t *testing.T) {
232+
ctx, cancel := context.WithCancel(context.Background())
233+
defer cancel()
234+
235+
opts := newDefaultTestOptions(t)
236+
m, err := NewManager(opts)
237+
if err != nil {
238+
t.Fatal(err)
239+
}
240+
241+
// Register a new volume with the metadata store
242+
store := opts.MetadataReader.(storage.Interface)
243+
meta := metadata.Metadata{
244+
VolumeID: "vol-id",
245+
TargetPath: "/fake/path",
246+
}
247+
store.RegisterMetadata(meta)
248+
// Ensure we stop managing the volume after the test
249+
defer func() {
250+
store.RemoveVolume(meta.VolumeID)
251+
m.UnmanageVolume(meta.VolumeID)
252+
}()
253+
254+
// Put the certificate under management
255+
managed := m.ManageVolume(meta.VolumeID)
256+
if !managed {
257+
t.Errorf("expected management to have started, but it did not")
258+
}
259+
260+
if err := wait.PollUntilWithContext(ctx, time.Millisecond*500, func(ctx context.Context) (done bool, err error) {
261+
reqs, err := opts.Client.CertmanagerV1().CertificateRequests("").List(ctx, metav1.ListOptions{})
262+
if err != nil {
263+
return false, err
264+
}
265+
if len(reqs.Items) == 1 {
266+
return true, nil
267+
}
268+
return false, nil
269+
}); err != nil {
270+
t.Errorf("failed waiting for CertificateRequest to exist: %v", err)
271+
}
272+
273+
// Assert the certificate is under management - it won't be ready as no issuer has issued the certificate
274+
if m.IsVolumeReady(meta.VolumeID) {
275+
t.Errorf("expected volume to not be Ready but it is")
276+
}
277+
if _, ok := m.managedVolumes[meta.VolumeID]; !ok {
278+
t.Errorf("expected volume to be part of managedVolumes map but it is not")
279+
}
280+
}
281+
18282
func TestManager_cleanupStaleRequests(t *testing.T) {
19283

20284
ctx := context.TODO()

0 commit comments

Comments
 (0)