Implements feedback from code review

Signed-off-by: irbekrm <irbekrm@gmail.com>

pkg/controller/acmeorders/controller.go

@@ -191,8 +191,9 @@ const (
 	ControllerName = "orders"
 )
 
-// controllerWrapper wraps the `controller` structure to make it implement
-// the controllerpkg.queueingController interface
+// controllerWrapper wraps the `controller` structure to make it implement the
+// controllerpkg.queueingController interface. This allows for easier
+// instantiation of this controller in integration tests.
 type controllerWrapper struct {
 	*controller
 }

pkg/controller/acmeorders/sync.go

@@ -42,12 +42,16 @@ import (
 )
 
 const (
-	RequeuePeriod time.Duration = time.Minute
-
 	reasonSolver  = "Solver"
 	reasonCreated = "Created"
 )
 
+var (
+	// RequeuePeriod is the default period after which an Order should be re-queued.
+	// It can be overriden in tests.
+	RequeuePeriod time.Duration = time.Second * 5
+)
+
 func (c *controller) Sync(ctx context.Context, o *cmacme.Order) (err error) {
 	log := logf.FromContext(ctx)
 	dbg := log.V(logf.DebugLevel)
@@ -149,9 +153,19 @@ func (c *controller) Sync(ctx context.Context, o *cmacme.Order) (err error) {
 	}
 
 	acmeOrder, err := getACMEOrder(ctx, cl, o)
+	// Order probably has been deleted, we cannot recover here.
+	if acmeErr, ok := err.(*acmeapi.Error); ok {
+		if acmeErr.StatusCode >= 400 && acmeErr.StatusCode < 500 {
+			log.Error(err, "failed to retrieve the ACME order (4xx error) marking Order as failed")
+			c.setOrderState(&o.Status, string(cmacme.Errored))
+			o.Status.Reason = fmt.Sprintf("Failed to retrieve Order resource: %v", err)
+			return nil
+		}
+	}
 	if err != nil {
 		return err
 	}
+
 	switch {
 	case o.Status.State == cmacme.Ready:
 		log.V(logf.DebugLevel).Info("Finalizing Order as order state is 'Ready'")
@@ -186,14 +200,18 @@ func (c *controller) Sync(ctx context.Context, o *cmacme.Order) (err error) {
 		// This is probably not needed as at this point the Order's status
 		// should already be Pending, but set it anyway to be explicit.
 		c.setOrderState(&o.Status, string(cmacme.Pending))
-		// Re-queue the Order to be processed after RequeuePeriod
 		key, err := cache.MetaNamespaceKeyFunc(o)
 		if err != nil {
-			// if we return an error here, at least the Order will get re-queued.
-			log.V(logf.InfoLevel).Info("Internal error: failed to construct key for pending Order")
-			return err
+			log.Error(err, "failed to construct key for pending Order")
+			// We should never end up here as this error would have been
+			// encountered in informers callback already. This probably cannot
+			// be fixed by re-queueing. If we do start encountering this
+			// scenario, we should consider whether the Order should be marked
+			// as failed here.
+			return nil
 		}
-		c.scheduledWorkQueue.Add(key, time.Second*5)
+		// Re-queue the Order to be processed again after 5 seconds.
+		c.scheduledWorkQueue.Add(key, RequeuePeriod)
 		return nil
 
 	case !anyChallengesFailed(challenges) && allChallengesFinal(challenges):

pkg/controller/acmeorders/sync_test.go

@@ -621,12 +621,14 @@ func runTest(t *testing.T, test testT) {
 	test.builder.Init()
 	defer test.builder.Stop()
 
-	c := &controllerWrapper{}
-	_, _, err := c.Register(test.builder.Context)
+	cw := &controllerWrapper{}
+	_, _, err := cw.Register(test.builder.Context)
 	if err != nil {
 		t.Errorf("Error registering the controller: %v", err)
 	}
-	c.accountRegistry = &accountstest.FakeRegistry{
+
+	// Set some fields on the embedded controller.
+	cw.accountRegistry = &accountstest.FakeRegistry{
 		GetClientFunc: func(_ string) (acmecl.Interface, error) {
 			return test.acmeClient, nil
 		},
@@ -637,10 +639,11 @@ func runTest(t *testing.T, test testT) {
 			gotScheduled = true
 		},
 	}
-	c.scheduledWorkQueue = &fakeScheduler
+	cw.scheduledWorkQueue = &fakeScheduler
+
 	test.builder.Start()
 
-	err = c.Sync(context.Background(), test.order)
+	err = cw.Sync(context.Background(), test.order)
 	if err != nil && !test.expectErr {
 		t.Errorf("Expected function to not error, but got: %v", err)
 	}
@@ -648,7 +651,7 @@ func runTest(t *testing.T, test testT) {
 		t.Errorf("Expected function to get an error, but got: %v", err)
 	}
 	if gotScheduled != test.shouldSchedule {
-		t.Errorf("Expected Order to be re-queued: %v got requeued: %v", test.shouldSchedule, gotScheduled)
+		t.Errorf("Expected Order to be re-queued: %v got re-queued: %v", test.shouldSchedule, gotScheduled)
 	}
 
 	test.builder.CheckAndFinish(err)

test/integration/acme/orders_controller_test.go

@@ -237,10 +237,16 @@ func TestAcmeOrdersController(t *testing.T) {
 		t.Fatal(err)
 	}
 
+	// Override the default requeue period.
+	acmeorders.RequeuePeriod = time.Second * 2
+
 	// Sit here for a little bit checking that the Order status remains pending
 	// and also to verify that this test works.
+	// TODO: instead of waiting for the Order to remain 'pending', we should use
+	// Reason field on Order's status. Change this test once we are setting
+	// Reasons on intermittent Order states.
 	var pendingOrder *cmacme.Order
-	err = wait.Poll(time.Millisecond*200, time.Second*5, func() (bool, error) {
+	err = wait.Poll(time.Millisecond*200, acmeorders.RequeuePeriod, func() (bool, error) {
 		pendingOrder, err = cmCl.AcmeV1().Orders(testName).Get(ctx, testName, metav1.GetOptions{})
 		if err != nil {
 			return false, err
@@ -259,11 +265,11 @@ func TestAcmeOrdersController(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	// Set status of the ACME order to 'ready.
+	// Set status of the ACME order to 'ready'.
 	acmeOrder.Status = acmeapi.StatusReady
 
 	// Wait for the status of the Order to become Valid.
-	err = wait.Poll(time.Millisecond*100, time.Second*20, func() (bool, error) {
+	err = wait.Poll(time.Millisecond*100, acmeorders.RequeuePeriod*3, func() (bool, error) {
 		o, err := cmCl.AcmeV1().Orders(testName).Get(ctx, testName, metav1.GetOptions{})
 		if err != nil {
 			return false, err