address review feedback

Signed-off-by: James Munnelly <[email protected]>

Author: munnerz

manager/manager.go

@@ -206,6 +206,8 @@ func NewManager(opts Options) (*Manager, error) {
 	// added the entry to the map instead, and only purged items from the map that are older that N duration (TBD).
 	janitorLogger := opts.Log.WithName("pending_request_janitor")
 	go wait.Until(func() {
+		requestToPrivateKeyLock.Lock()
+		defer requestToPrivateKeyLock.Unlock()
 		reqs, err := lister.List(labels.Everything())
 		if err != nil {
 			janitorLogger.Error(err, "failed listing existing requests")
@@ -217,8 +219,6 @@ func NewManager(opts Options) (*Manager, error) {
 			existsMap[req.UID] = struct{}{}
 		}
 
-		requestToPrivateKeyLock.Lock()
-		defer requestToPrivateKeyLock.Unlock()
 		for uid := range requestToPrivateKeyMap {
 			if _, ok := existsMap[uid]; !ok {
 				// purge the item from the map as it does not exist in the apiserver
@@ -231,9 +231,11 @@ func NewManager(opts Options) (*Manager, error) {
 	informerFactory.WaitForCacheSync(stopCh)
 
 	m := &Manager{
-		client:                  opts.Client,
-		clientForMetadata:       opts.ClientForMetadata,
-		lister:                  lister,
+		client:            opts.Client,
+		clientForMetadata: opts.ClientForMetadata,
+		lister:            lister,
+		// we pass a pointer to the mutex as the janitor routine above also uses this lock,
+		// so we want to avoid making a copy of it
 		requestToPrivateKeyLock: &requestToPrivateKeyLock,
 		requestToPrivateKeyMap:  requestToPrivateKeyMap,
 		metadataReader:          opts.MetadataReader,

test/integration/resume_request_test.go

@@ -20,7 +20,6 @@ import (
 	"context"
 	"crypto"
 	"crypto/x509"
-	"os"
 	"reflect"
 	"testing"
 	"time"
@@ -38,7 +37,14 @@ import (
 	testutil "github.com/cert-manager/csi-lib/test/util"
 )
 
-func testResumesExistingRequest(t *testing.T, issueBeforeCall bool) {
+type WhenToCallIssue bool
+
+const (
+	CallIssueDuringPublish  = false
+	CallIssueBetweenPublish = true
+)
+
+func testResumesExistingRequest(t *testing.T, issueBeforeCall WhenToCallIssue) {
 	store := storage.NewMemoryFS()
 	ns := "certificaterequest-namespace"
 	clock := fakeclock.NewFakeClock(time.Now())
@@ -66,13 +72,9 @@ func testResumesExistingRequest(t *testing.T, issueBeforeCall bool) {
 			return store.WriteMetadata(meta.VolumeID, meta)
 		},
 	})
-	defer stop()
+	t.Cleanup(stop)
 
-	tmpDir, err := os.MkdirTemp("", "*")
-	if err != nil {
-		t.Fatal(err)
-	}
-	defer os.RemoveAll(tmpDir)
+	tmpDir := t.TempDir()
 
 	// create a root, non-expiring context
 	ctx := context.Background()
@@ -91,8 +93,8 @@ func testResumesExistingRequest(t *testing.T, issueBeforeCall bool) {
 
 	// create a context that expires in 2s (enough time for at least a single call of `issue`)
 	twoSecondCtx, cancel := context.WithTimeout(ctx, time.Second*2)
-	defer cancel()
-	_, err = cl.NodePublishVolume(twoSecondCtx, nodePublishVolumeRequest)
+	t.Cleanup(cancel)
+	_, err := cl.NodePublishVolume(twoSecondCtx, nodePublishVolumeRequest)
 	// assert that an error has been returned - we don't mind what kind of error, as due to the async nature of
 	// de-registering metadata from the metadata store upon failures, there is a slim chance that a metadata read error
 	// can be returned instead of a deadline exceeded error.
@@ -155,11 +157,11 @@ func testResumesExistingRequest(t *testing.T, issueBeforeCall bool) {
 }
 
 func TestResumesExistingRequest_IssuedBetweenPublishCalls(t *testing.T) {
-	testResumesExistingRequest(t, true)
+	testResumesExistingRequest(t, CallIssueBetweenPublish)
 }
 
 func TestResumesExistingRequest_IssuedDuringPublishCall(t *testing.T) {
-	testResumesExistingRequest(t, false)
+	testResumesExistingRequest(t, CallIssueDuringPublish)
 }
 
 // ensureOneRequestExists will fail the test if more than a single CertificateRequest exists.