diff --git a/pkg/controller/acmeorders/controller.go b/pkg/controller/acmeorders/controller.go index a299e1540..58df696c8 100644 --- a/pkg/controller/acmeorders/controller.go +++ b/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 } diff --git a/pkg/controller/acmeorders/sync.go b/pkg/controller/acmeorders/sync.go index 16c06b9d8..46f47c2a3 100644 --- a/pkg/controller/acmeorders/sync.go +++ b/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): diff --git a/pkg/controller/acmeorders/sync_test.go b/pkg/controller/acmeorders/sync_test.go index 49663acf5..51294b5e5 100644 --- a/pkg/controller/acmeorders/sync_test.go +++ b/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) diff --git a/test/integration/acme/orders_controller_test.go b/test/integration/acme/orders_controller_test.go index 6e706c2b5..84fc467af 100644 --- a/test/integration/acme/orders_controller_test.go +++ b/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