diff --git a/pkg/apis/authzed/v1alpha1/conditions.go b/pkg/apis/authzed/v1alpha1/conditions.go index 6784c5a7..11b543cd 100644 --- a/pkg/apis/authzed/v1alpha1/conditions.go +++ b/pkg/apis/authzed/v1alpha1/conditions.go @@ -25,6 +25,7 @@ const ( ConditionTypeRolloutError = "RolloutError" ConditionReasonMissingSecret = "MissingSecret" + ConditionReasonApplyFailed = "ApplyFailed" ) func NewValidatingConfigCondition(secretHash string) metav1.Condition { @@ -106,3 +107,13 @@ func NewPodErrorCondition(message string) metav1.Condition { Message: message, } } + +func NewApplyFailedCondition(err error) metav1.Condition { + return metav1.Condition{ + Type: ConditionTypeRolloutError, + Status: metav1.ConditionTrue, + Reason: ConditionReasonApplyFailed, + LastTransitionTime: metav1.NewTime(time.Now()), + Message: fmt.Sprintf("Error applying deployment: %s", err), + } +} diff --git a/pkg/controller/ensure_deployment.go b/pkg/controller/ensure_deployment.go index 8b67c646..73330cb5 100644 --- a/pkg/controller/ensure_deployment.go +++ b/pkg/controller/ensure_deployment.go @@ -88,9 +88,26 @@ func (m *DeploymentHandler) Handle(ctx context.Context) { ), ) if err != nil { + // surface the failure on the status so it's visible without + // reading operator logs, e.g. when a user-provided patch results + // in a deployment the API server rejects + currentStatus.SetStatusCondition(v1alpha1.NewApplyFailedCondition(err)) + if patchErr := m.patchStatus(ctx, currentStatus); patchErr != nil { + QueueOps.RequeueAPIErr(ctx, patchErr) + return + } QueueOps.RequeueAPIErr(ctx, err) return } + + // clear any error from a previous failed apply + if cond := currentStatus.FindStatusCondition(v1alpha1.ConditionTypeRolloutError); cond != nil && cond.Reason == v1alpha1.ConditionReasonApplyFailed { + currentStatus.RemoveStatusCondition(v1alpha1.ConditionTypeRolloutError) + if err := m.patchStatus(ctx, currentStatus); err != nil { + QueueOps.RequeueAPIErr(ctx, err) + return + } + } ctx = CtxCurrentSpiceDeployment.WithValue(ctx, deployment) } diff --git a/pkg/controller/ensure_deployment_test.go b/pkg/controller/ensure_deployment_test.go index 33bc5b3a..240eac9f 100644 --- a/pkg/controller/ensure_deployment_test.go +++ b/pkg/controller/ensure_deployment_test.go @@ -24,6 +24,7 @@ import ( func TestEnsureDeploymentHandler(t *testing.T) { now := metav1.Now() var nextKey handler.Key = "next" + applyErr := fmt.Errorf("apply error") tests := []struct { name string @@ -34,14 +35,16 @@ func TestEnsureDeploymentHandler(t *testing.T) { pods []*corev1.Pod currentStatus *v1alpha1.SpiceDBCluster replicas int32 + applyErr error - expectNext handler.Key - expectStatus *v1alpha1.SpiceDBCluster - expectRequeueErr error - expectRequeueAfter bool - expectApply bool - expectDelete bool - expectPatchStatus bool + expectNext handler.Key + expectStatus *v1alpha1.SpiceDBCluster + expectRequeueErr error + expectRequeueAPIErr error + expectRequeueAfter bool + expectApply bool + expectDelete bool + expectPatchStatus bool }{ { name: "creates if no deployments", @@ -50,6 +53,40 @@ func TestEnsureDeploymentHandler(t *testing.T) { expectApply: true, expectRequeueAfter: true, }, + { + name: "reports apply failure on status", + migrationHash: "testtesttesttest", + secretHash: "secret", + applyErr: applyErr, + expectApply: true, + expectPatchStatus: true, + expectStatus: &v1alpha1.SpiceDBCluster{Status: v1alpha1.ClusterStatus{Conditions: []metav1.Condition{{ + Type: v1alpha1.ConditionTypeRolloutError, + Status: metav1.ConditionTrue, + LastTransitionTime: now, + Reason: v1alpha1.ConditionReasonApplyFailed, + Message: "Error applying deployment: apply error", + }}}}, + expectRequeueAPIErr: applyErr, + }, + { + name: "clears apply failure once apply succeeds", + currentStatus: &v1alpha1.SpiceDBCluster{Status: v1alpha1.ClusterStatus{Conditions: []metav1.Condition{{ + Type: v1alpha1.ConditionTypeRolloutError, + Status: metav1.ConditionTrue, + LastTransitionTime: now, + Reason: v1alpha1.ConditionReasonApplyFailed, + Message: "Error applying deployment: apply error", + }}}}, + migrationHash: "testtesttesttest", + secretHash: "secret", + expectApply: true, + expectPatchStatus: true, + expectStatus: &v1alpha1.SpiceDBCluster{Status: v1alpha1.ClusterStatus{ + Conditions: []metav1.Condition{}, + }}, + expectRequeueAfter: true, + }, { // Regression test for https://github.com/authzed/spicedb-operator/issues/415 // When all credential refs are configured with `skip: true`, ConfigChangedHandler @@ -407,7 +444,7 @@ func TestEnsureDeploymentHandler(t *testing.T) { h := &DeploymentHandler{ applyDeployment: func(_ context.Context, _ *applyappsv1.DeploymentApplyConfiguration) (*appsv1.Deployment, error) { applyCalled = true - return nil, nil + return nil, tt.applyErr }, deleteDeployment: func(_ context.Context, _ types.NamespacedName) error { deleteCalled = true @@ -439,6 +476,10 @@ func TestEnsureDeploymentHandler(t *testing.T) { require.Equal(t, 1, ctrls.RequeueErrCallCount()) require.Equal(t, tt.expectRequeueErr, ctrls.RequeueErrArgsForCall(0)) } + if tt.expectRequeueAPIErr != nil { + require.Equal(t, 1, ctrls.RequeueAPIErrCallCount()) + require.Equal(t, tt.expectRequeueAPIErr, ctrls.RequeueAPIErrArgsForCall(0)) + } require.Equal(t, tt.expectRequeueAfter, ctrls.RequeueAfterCallCount() == 1) }) }