diff --git a/api/v1alpha1/stage_types.go b/api/v1alpha1/stage_types.go index bdc4b3a495..78d550fb3f 100644 --- a/api/v1alpha1/stage_types.go +++ b/api/v1alpha1/stage_types.go @@ -750,6 +750,12 @@ type VerificationInfo struct { AnalysisRun *AnalysisRunReference `json:"analysisRun,omitempty" protobuf:"bytes,3,opt,name=analysisRun"` } +// HasAnalysisRun returns a bool indicating whether the VerificationInfo has an +// associated AnalysisRun. +func (v *VerificationInfo) HasAnalysisRun() bool { + return v != nil && v.AnalysisRun != nil +} + type VerificationInfoStack []VerificationInfo // UpdateOrPush updates the VerificationInfo with the same ID as the provided diff --git a/api/v1alpha1/stage_types_test.go b/api/v1alpha1/stage_types_test.go index ae9a759c83..601373514c 100644 --- a/api/v1alpha1/stage_types_test.go +++ b/api/v1alpha1/stage_types_test.go @@ -6,6 +6,37 @@ import ( "github.com/stretchr/testify/require" ) +func TestVerificationInfo_HasAnalysisRun(t *testing.T) { + testCases := []struct { + name string + info *VerificationInfo + expectedResult bool + }{ + { + name: "VerificationInfo is nil", + info: nil, + expectedResult: false, + }, + { + name: "AnalysisRun is nil", + info: &VerificationInfo{}, + expectedResult: false, + }, + { + name: "AnalysisRun is not nil", + info: &VerificationInfo{ + AnalysisRun: &AnalysisRunReference{}, + }, + expectedResult: true, + }, + } + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + require.Equal(t, testCase.expectedResult, testCase.info.HasAnalysisRun()) + }) + } +} + func TestFreightReferenceStackEmpty(t *testing.T) { testCases := []struct { name string diff --git a/internal/controller/stages/stages.go b/internal/controller/stages/stages.go index d2cc4db249..62c22b54f0 100644 --- a/internal/controller/stages/stages.go +++ b/internal/controller/stages/stages.go @@ -87,7 +87,7 @@ type reconciler struct { startVerificationFn func( context.Context, *kargoapi.Stage, - ) *kargoapi.VerificationInfo + ) (*kargoapi.VerificationInfo, error) abortVerificationFn func( context.Context, @@ -97,7 +97,7 @@ type reconciler struct { getVerificationInfoFn func( context.Context, *kargoapi.Stage, - ) *kargoapi.VerificationInfo + ) (*kargoapi.VerificationInfo, error) getAnalysisTemplateFn func( context.Context, @@ -755,14 +755,32 @@ func (r *reconciler) syncNormalStage( // check if verification step is necessary and if yes execute // step irrespective of phase if status.Phase == kargoapi.StagePhaseVerifying || status.Phase == kargoapi.StagePhaseNotApplicable { - if status.CurrentFreight.VerificationInfo == nil { + if !status.CurrentFreight.VerificationInfo.HasAnalysisRun() { if status.Health == nil || status.Health.Status == kargoapi.HealthStateHealthy { log.Debug("starting verification") - status.CurrentFreight.VerificationInfo = r.startVerificationFn(ctx, stage) + var err error + if status.CurrentFreight.VerificationInfo, err = r.startVerificationFn( + ctx, + stage, + ); err != nil && !status.CurrentFreight.VerificationInfo.HasAnalysisRun() { + status.CurrentFreight.VerificationHistory.UpdateOrPush( + *status.CurrentFreight.VerificationInfo, + ) + return status, fmt.Errorf("error starting verification: %w", err) + } } } else { log.Debug("checking verification results") - status.CurrentFreight.VerificationInfo = r.getVerificationInfoFn(ctx, stage) + var err error + if status.CurrentFreight.VerificationInfo, err = r.getVerificationInfoFn( + ctx, + stage, + ); err != nil && status.CurrentFreight.VerificationInfo.HasAnalysisRun() { + status.CurrentFreight.VerificationHistory.UpdateOrPush( + *status.CurrentFreight.VerificationInfo, + ) + return status, fmt.Errorf("error getting verification info: %w", err) + } // Abort the verification if it's still running and the Stage has // been marked to do so. diff --git a/internal/controller/stages/stages_test.go b/internal/controller/stages/stages_test.go index c1da97c27d..f7e3db805c 100644 --- a/internal/controller/stages/stages_test.go +++ b/internal/controller/stages/stages_test.go @@ -366,7 +366,7 @@ func TestSyncNormalStage(t *testing.T) { startVerificationFn: func( context.Context, *kargoapi.Stage, - ) *kargoapi.VerificationInfo { + ) (*kargoapi.VerificationInfo, error) { return &kargoapi.VerificationInfo{ ID: "new-fake-id", Phase: kargoapi.VerificationPhasePending, @@ -374,7 +374,7 @@ func TestSyncNormalStage(t *testing.T) { AnalysisRun: &kargoapi.AnalysisRunReference{ Name: "new-fake-analysis-run", }, - } + }, nil }, }, assertions: func( @@ -479,11 +479,11 @@ func TestSyncNormalStage(t *testing.T) { startVerificationFn: func( context.Context, *kargoapi.Stage, - ) *kargoapi.VerificationInfo { + ) (*kargoapi.VerificationInfo, error) { return &kargoapi.VerificationInfo{ Phase: kargoapi.VerificationPhaseError, Message: "something went wrong", - } + }, nil }, }, assertions: func( @@ -519,6 +519,71 @@ func TestSyncNormalStage(t *testing.T) { }, }, + { + name: "retryable error starting verification", + stage: &kargoapi.Stage{ + Spec: &kargoapi.StageSpec{ + PromotionMechanisms: &kargoapi.PromotionMechanisms{}, + Verification: &kargoapi.Verification{}, + }, + Status: kargoapi.StageStatus{ + Phase: kargoapi.StagePhaseVerifying, + CurrentFreight: &kargoapi.FreightReference{}, + }, + }, + reconciler: &reconciler{ + hasNonTerminalPromotionsFn: noNonTerminalPromotionsFn, + checkHealthFn: func( + context.Context, + kargoapi.FreightReference, + []kargoapi.ArgoCDAppUpdate, + ) *kargoapi.Health { + return nil + }, + startVerificationFn: func( + context.Context, + *kargoapi.Stage, + ) (*kargoapi.VerificationInfo, error) { + return &kargoapi.VerificationInfo{ + Phase: kargoapi.VerificationPhaseError, + Message: "something went wrong", + }, errors.New("retryable error") + }, + }, + assertions: func( + t *testing.T, + initialStatus kargoapi.StageStatus, + newStatus kargoapi.StageStatus, + err error, + ) { + require.ErrorContains(t, err, "retryable error") + require.NotNil(t, newStatus.CurrentFreight) + require.Equal(t, kargoapi.StagePhaseVerifying, newStatus.Phase) + + expectInfo := kargoapi.VerificationInfo{ + Phase: kargoapi.VerificationPhaseError, + Message: "something went wrong", + } + + require.Equal( + t, + &expectInfo, + newStatus.CurrentFreight.VerificationInfo, + ) + require.Equal( + t, + kargoapi.VerificationInfoStack{expectInfo}, + newStatus.CurrentFreight.VerificationHistory, + ) + + // Everything else should be returned unchanged + newStatus.CurrentFreight.VerificationInfo = nil + newStatus.CurrentFreight.VerificationHistory = nil + newStatus.Phase = initialStatus.Phase + require.Equal(t, initialStatus, newStatus) + }, + }, + { name: "error checking verification result", stage: &kargoapi.Stage{ @@ -529,7 +594,13 @@ func TestSyncNormalStage(t *testing.T) { Status: kargoapi.StageStatus{ Phase: kargoapi.StagePhaseVerifying, CurrentFreight: &kargoapi.FreightReference{ - VerificationInfo: &kargoapi.VerificationInfo{}, + VerificationInfo: &kargoapi.VerificationInfo{ + Phase: kargoapi.VerificationPhasePending, + AnalysisRun: &kargoapi.AnalysisRunReference{ + Name: "fake-analysis-run", + Namespace: "fake-namespace", + }, + }, }, }, }, @@ -542,11 +613,11 @@ func TestSyncNormalStage(t *testing.T) { ) *kargoapi.Health { return nil }, - getVerificationInfoFn: func(_ context.Context, _ *kargoapi.Stage) *kargoapi.VerificationInfo { + getVerificationInfoFn: func(_ context.Context, _ *kargoapi.Stage) (*kargoapi.VerificationInfo, error) { return &kargoapi.VerificationInfo{ Phase: kargoapi.VerificationPhaseError, Message: "something went wrong", - } + }, nil }, }, assertions: func( @@ -574,6 +645,77 @@ func TestSyncNormalStage(t *testing.T) { }, }, + { + name: "retryable error checking verification result", + stage: &kargoapi.Stage{ + Spec: &kargoapi.StageSpec{ + PromotionMechanisms: &kargoapi.PromotionMechanisms{}, + Verification: &kargoapi.Verification{}, + }, + Status: kargoapi.StageStatus{ + Phase: kargoapi.StagePhaseVerifying, + CurrentFreight: &kargoapi.FreightReference{ + VerificationInfo: &kargoapi.VerificationInfo{ + Phase: kargoapi.VerificationPhasePending, + AnalysisRun: &kargoapi.AnalysisRunReference{ + Name: "fake-analysis-run", + Namespace: "fake-namespace", + }, + }, + }, + }, + }, + reconciler: &reconciler{ + hasNonTerminalPromotionsFn: noNonTerminalPromotionsFn, + checkHealthFn: func( + context.Context, + kargoapi.FreightReference, + []kargoapi.ArgoCDAppUpdate, + ) *kargoapi.Health { + return nil + }, + getVerificationInfoFn: func(_ context.Context, _ *kargoapi.Stage) (*kargoapi.VerificationInfo, error) { + return &kargoapi.VerificationInfo{ + Phase: kargoapi.VerificationPhaseError, + Message: "something went wrong", + AnalysisRun: &kargoapi.AnalysisRunReference{ + Name: "fake-analysis-run", + Namespace: "fake-namespace", + }, + }, errors.New("retryable error") + }, + }, + assertions: func( + t *testing.T, + initialStatus kargoapi.StageStatus, + newStatus kargoapi.StageStatus, + err error, + ) { + require.ErrorContains(t, err, "retryable error") + require.NotNil(t, newStatus.CurrentFreight) + require.Equal( + t, + &kargoapi.VerificationInfo{ + Phase: kargoapi.VerificationPhaseError, + Message: "something went wrong", + AnalysisRun: &kargoapi.AnalysisRunReference{ + Name: "fake-analysis-run", + Namespace: "fake-namespace", + }, + }, + newStatus.CurrentFreight.VerificationInfo, + ) + require.Len(t, newStatus.CurrentFreight.VerificationHistory, 1) + + // Phase should not be changed to Steady + require.Equal(t, kargoapi.StagePhaseVerifying, newStatus.Phase) + // Everything else should be unchanged + newStatus.Phase = initialStatus.Phase + newStatus.CurrentFreight = initialStatus.CurrentFreight + require.Equal(t, initialStatus, newStatus) + }, + }, + { name: "verification aborted", stage: &kargoapi.Stage{ @@ -611,8 +753,8 @@ func TestSyncNormalStage(t *testing.T) { getVerificationInfoFn: func( _ context.Context, s *kargoapi.Stage, - ) *kargoapi.VerificationInfo { - return s.Status.CurrentFreight.VerificationInfo + ) (*kargoapi.VerificationInfo, error) { + return s.Status.CurrentFreight.VerificationInfo, nil }, abortVerificationFn: func( context.Context, @@ -683,10 +825,10 @@ func TestSyncNormalStage(t *testing.T) { getVerificationInfoFn: func( _ context.Context, s *kargoapi.Stage, - ) *kargoapi.VerificationInfo { + ) (*kargoapi.VerificationInfo, error) { i := s.Status.CurrentFreight.VerificationInfo.DeepCopy() i.Phase = kargoapi.VerificationPhaseError - return i + return i, nil }, abortVerificationFn: func( context.Context, @@ -1174,7 +1316,13 @@ func TestSyncNormalStage(t *testing.T) { Phase: kargoapi.StagePhaseVerifying, CurrentPromotion: &kargoapi.PromotionInfo{}, CurrentFreight: &kargoapi.FreightReference{ - VerificationInfo: &kargoapi.VerificationInfo{}, + VerificationInfo: &kargoapi.VerificationInfo{ + Phase: kargoapi.VerificationPhasePending, + AnalysisRun: &kargoapi.AnalysisRunReference{ + Name: "fake-analysis-run", + Namespace: "fake-namespace", + }, + }, }, }, }, @@ -1192,7 +1340,7 @@ func TestSyncNormalStage(t *testing.T) { getVerificationInfoFn: func( context.Context, *kargoapi.Stage, - ) *kargoapi.VerificationInfo { + ) (*kargoapi.VerificationInfo, error) { return &kargoapi.VerificationInfo{ Phase: kargoapi.VerificationPhaseSuccessful, AnalysisRun: &kargoapi.AnalysisRunReference{ @@ -1200,7 +1348,7 @@ func TestSyncNormalStage(t *testing.T) { Namespace: "fake-namespace", Phase: string(rollouts.AnalysisPhaseSuccessful), }, - } + }, nil }, verifyFreightInStageFn: func(context.Context, string, string, string) error { return nil diff --git a/internal/controller/stages/verification.go b/internal/controller/stages/verification.go index ca4081ba9a..a258783db5 100644 --- a/internal/controller/stages/verification.go +++ b/internal/controller/stages/verification.go @@ -15,20 +15,30 @@ import ( kargoapi "github.com/akuity/kargo/api/v1alpha1" rollouts "github.com/akuity/kargo/internal/controller/rollouts/api/v1alpha1" + "github.com/akuity/kargo/internal/kubeclient" "github.com/akuity/kargo/internal/logging" ) +// startVerification starts a verification for the given Stage. If the Stage +// does not have a reverification annotation, it checks if there is an existing +// AnalysisRun for the Stage and Freight. If there is, it returns the status of +// this AnalysisRun. If there is not, it creates a new AnalysisRun for the Stage +// and Freight. +// +// In case of an error, it returns a VerificationInfo with the error message and +// the phase set to Error. If the error may be due to a transient issue, it is +// returned, so that the caller can retry the operation. func (r *reconciler) startVerification( ctx context.Context, stage *kargoapi.Stage, -) *kargoapi.VerificationInfo { +) (*kargoapi.VerificationInfo, error) { if r.rolloutsClient == nil { return &kargoapi.VerificationInfo{ ID: uuid.NewString(), Phase: kargoapi.VerificationPhaseError, Message: "Rollouts integration is disabled on this controller; " + "cannot start verification", - } + }, nil } logger := logging.LoggerFromContext(ctx) @@ -63,7 +73,7 @@ func (r *reconciler) startVerification( namespace, err, ).Error(), - } + }, err } if len(analysisRuns.Items) > 0 { // Sort the AnalysisRuns by creation timestamp, so that the most recent @@ -82,7 +92,7 @@ func (r *reconciler) startVerification( Namespace: latestAnalysisRun.Namespace, Phase: string(latestAnalysisRun.Status.Phase), }, - } + }, nil } } @@ -108,7 +118,7 @@ func (r *reconciler) startVerification( stage.Namespace, err, ).Error(), - } + }, err } if template == nil { return &kargoapi.VerificationInfo{ @@ -119,7 +129,7 @@ func (r *reconciler) startVerification( templateRef.Name, stage.Namespace, ).Error(), - } + }, nil } templates[i] = template } @@ -142,7 +152,7 @@ func (r *reconciler) startVerification( stage.Namespace, err, ).Error(), - } + }, err } if freight == nil { return &kargoapi.VerificationInfo{ @@ -153,7 +163,7 @@ func (r *reconciler) startVerification( stage.Status.CurrentFreight.Name, stage.Namespace, ).Error(), - } + }, nil } run, err := r.buildAnalysisRunFn(stage, freight, templates) @@ -168,7 +178,7 @@ func (r *reconciler) startVerification( stage.Namespace, err, ).Error(), - } + }, nil } if err := r.createAnalysisRunFn(ctx, run); err != nil { @@ -181,7 +191,7 @@ func (r *reconciler) startVerification( run.Namespace, err, ).Error(), - } + }, kubeclient.IgnoreInvalid(err) // Ignore errors which are due to validation issues } return &kargoapi.VerificationInfo{ @@ -191,20 +201,29 @@ func (r *reconciler) startVerification( Name: run.Name, Namespace: run.Namespace, }, - } + }, nil } +// getVerificationInfo returns the status of the AnalysisRun for the given Stage. +// +// In case of an error, it returns a VerificationInfo with the error message and +// the phase set to Error. If the error may be due to a transient issue, it is +// returned, so that the caller can retry the operation. +// +// If an error is returned, the AnalysisRun reference in the VerificationInfo +// will always be set to the AnalysisRun that was being checked. This is to +// ensure the caller can continue to track the status of the AnalysisRun. func (r *reconciler) getVerificationInfo( ctx context.Context, stage *kargoapi.Stage, -) *kargoapi.VerificationInfo { +) (*kargoapi.VerificationInfo, error) { if r.rolloutsClient == nil { return &kargoapi.VerificationInfo{ ID: stage.Status.CurrentFreight.VerificationInfo.ID, Phase: kargoapi.VerificationPhaseError, Message: "Rollouts integration is disabled on this controller; cannot " + "get verification info", - } + }, nil } namespace := r.getAnalysisRunNamespace(stage) @@ -227,7 +246,8 @@ func (r *reconciler) getVerificationInfo( namespace, err, ).Error(), - } + AnalysisRun: stage.Status.CurrentFreight.VerificationInfo.AnalysisRun.DeepCopy(), + }, err } if analysisRun == nil { return &kargoapi.VerificationInfo{ @@ -238,7 +258,7 @@ func (r *reconciler) getVerificationInfo( analysisRunName, namespace, ).Error(), - } + }, nil } return &kargoapi.VerificationInfo{ ID: stage.Status.CurrentFreight.VerificationInfo.ID, @@ -248,7 +268,7 @@ func (r *reconciler) getVerificationInfo( Namespace: analysisRun.Namespace, Phase: string(analysisRun.Status.Phase), }, - } + }, nil } func (r *reconciler) abortVerification( @@ -284,6 +304,7 @@ func (r *reconciler) abortVerification( ar.Namespace, err, ).Error(), + AnalysisRun: stage.Status.CurrentFreight.VerificationInfo.AnalysisRun.DeepCopy(), } } @@ -293,13 +314,10 @@ func (r *reconciler) abortVerification( // We do not use the further information from the AnalysisRun, as this // will indicate a "Succeeded" phase due to Argo Rollouts behavior. return &kargoapi.VerificationInfo{ - ID: stage.Status.CurrentFreight.VerificationInfo.ID, - Phase: kargoapi.VerificationPhaseAborted, - Message: "Verification aborted by user", - AnalysisRun: &kargoapi.AnalysisRunReference{ - Name: ar.Name, - Namespace: ar.Namespace, - }, + ID: stage.Status.CurrentFreight.VerificationInfo.ID, + Phase: kargoapi.VerificationPhaseAborted, + Message: "Verification aborted by user", + AnalysisRun: stage.Status.CurrentFreight.VerificationInfo.AnalysisRun.DeepCopy(), } } diff --git a/internal/controller/stages/verification_test.go b/internal/controller/stages/verification_test.go index 026ae41e92..eb1884d2cf 100644 --- a/internal/controller/stages/verification_test.go +++ b/internal/controller/stages/verification_test.go @@ -7,7 +7,9 @@ import ( "testing" "github.com/stretchr/testify/require" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" @@ -22,12 +24,13 @@ func TestStartVerification(t *testing.T) { name string stage *kargoapi.Stage reconciler *reconciler - assertions func(*testing.T, *kargoapi.VerificationInfo) + assertions func(*testing.T, *kargoapi.VerificationInfo, error) }{ { name: "rollouts integration not enabled", reconciler: &reconciler{}, - assertions: func(t *testing.T, vi *kargoapi.VerificationInfo) { + assertions: func(t *testing.T, vi *kargoapi.VerificationInfo, err error) { + require.NoError(t, err) require.Contains( t, vi.Message, @@ -54,7 +57,8 @@ func TestStartVerification(t *testing.T) { return errors.New("something went wrong") }, }, - assertions: func(t *testing.T, vi *kargoapi.VerificationInfo) { + assertions: func(t *testing.T, vi *kargoapi.VerificationInfo, err error) { + require.ErrorContains(t, err, "something went wrong") require.Contains(t, vi.Message, "something went wrong") require.Contains(t, vi.Message, "error listing AnalysisRuns for Stage") }, @@ -81,7 +85,8 @@ func TestStartVerification(t *testing.T) { return nil }, }, - assertions: func(t *testing.T, vi *kargoapi.VerificationInfo) { + assertions: func(t *testing.T, vi *kargoapi.VerificationInfo, err error) { + require.NoError(t, err) require.NotNil(t, vi) require.Empty(t, vi.Message) }, @@ -151,7 +156,8 @@ func TestStartVerification(t *testing.T) { return nil }, }, - assertions: func(t *testing.T, vi *kargoapi.VerificationInfo) { + assertions: func(t *testing.T, vi *kargoapi.VerificationInfo, err error) { + require.NoError(t, err) require.NotNilf(t, vi, "expected non-nil VerificationInfo") require.NotEmptyf(t, vi.ID, "expected non-empty VerificationInfo.ID") require.Equal(t, kargoapi.VerificationPhasePending, vi.Phase) @@ -192,7 +198,8 @@ func TestStartVerification(t *testing.T) { return nil, errors.New("something went wrong") }, }, - assertions: func(t *testing.T, vi *kargoapi.VerificationInfo) { + assertions: func(t *testing.T, vi *kargoapi.VerificationInfo, err error) { + require.ErrorContains(t, err, "something went wrong") require.NotNil(t, vi) require.Contains(t, vi.Message, "something went wrong") require.Contains(t, vi.Message, "error getting AnalysisTemplate") @@ -229,7 +236,8 @@ func TestStartVerification(t *testing.T) { return nil, nil }, }, - assertions: func(t *testing.T, vi *kargoapi.VerificationInfo) { + assertions: func(t *testing.T, vi *kargoapi.VerificationInfo, err error) { + require.NoError(t, err) require.NotNil(t, vi) require.Contains(t, vi.Message, "AnalysisTemplate") require.Contains(t, vi.Message, "not found") @@ -273,7 +281,8 @@ func TestStartVerification(t *testing.T) { return nil, fmt.Errorf("something went wrong") }, }, - assertions: func(t *testing.T, vi *kargoapi.VerificationInfo) { + assertions: func(t *testing.T, vi *kargoapi.VerificationInfo, err error) { + require.ErrorContains(t, err, "something went wrong") require.NotNil(t, vi) require.Contains(t, vi.Message, "something went wrong") require.Contains(t, vi.Message, "error getting Freight") @@ -317,7 +326,8 @@ func TestStartVerification(t *testing.T) { return nil, nil }, }, - assertions: func(t *testing.T, vi *kargoapi.VerificationInfo) { + assertions: func(t *testing.T, vi *kargoapi.VerificationInfo, err error) { + require.NoError(t, err) require.NotNil(t, vi) require.Contains(t, vi.Message, "Freight") require.Contains(t, vi.Message, "not found") @@ -367,7 +377,8 @@ func TestStartVerification(t *testing.T) { return nil, errors.New("something went wrong") }, }, - assertions: func(t *testing.T, vi *kargoapi.VerificationInfo) { + assertions: func(t *testing.T, vi *kargoapi.VerificationInfo, err error) { + require.NoError(t, err) require.NotNil(t, vi) require.Contains(t, vi.Message, "something went wrong") require.Contains(t, vi.Message, "error building AnalysisRun for Stage") @@ -425,12 +436,74 @@ func TestStartVerification(t *testing.T) { return errors.New("something went wrong") }, }, - assertions: func(t *testing.T, vi *kargoapi.VerificationInfo) { + assertions: func(t *testing.T, vi *kargoapi.VerificationInfo, err error) { + require.ErrorContains(t, err, "something went wrong") require.NotNil(t, vi) require.Contains(t, vi.Message, "something went wrong") require.Contains(t, vi.Message, "error creating AnalysisRun") }, }, + { + name: "validation error creating AnalysisRun", + stage: &kargoapi.Stage{ + Spec: &kargoapi.StageSpec{ + Verification: &kargoapi.Verification{ + AnalysisTemplates: []kargoapi.AnalysisTemplateReference{{}}, + }, + }, + Status: kargoapi.StageStatus{ + CurrentFreight: &kargoapi.FreightReference{ + Name: "fake-id", + }, + }, + }, + reconciler: &reconciler{ + rolloutsClient: fake.NewClientBuilder().Build(), + listAnalysisRunsFn: func( + context.Context, + client.ObjectList, + ...client.ListOption, + ) error { + return nil + }, + getAnalysisTemplateFn: func( + context.Context, + client.Client, + types.NamespacedName, + ) (*rollouts.AnalysisTemplate, error) { + return &rollouts.AnalysisTemplate{}, nil + }, + getFreightFn: func( + context.Context, + client.Client, + types.NamespacedName, + ) (*kargoapi.Freight, error) { + return &kargoapi.Freight{}, nil + }, + buildAnalysisRunFn: func( + *kargoapi.Stage, + *kargoapi.Freight, + []*rollouts.AnalysisTemplate, + ) (*rollouts.AnalysisRun, error) { + return &rollouts.AnalysisRun{}, nil + }, + createAnalysisRunFn: func( + context.Context, + client.Object, + ...client.CreateOption, + ) error { + return apierrors.NewInvalid(schema.GroupKind{ + Group: kargoapi.GroupVersion.Group, + Kind: "AnalysisRun", + }, "fake-name", nil) + }, + }, + assertions: func(t *testing.T, vi *kargoapi.VerificationInfo, err error) { + require.NoError(t, err) + require.NotNil(t, vi) + require.Contains(t, vi.Message, "error creating AnalysisRun") + }, + }, { name: "success", stage: &kargoapi.Stage{ @@ -488,7 +561,8 @@ func TestStartVerification(t *testing.T) { return nil }, }, - assertions: func(t *testing.T, vi *kargoapi.VerificationInfo) { + assertions: func(t *testing.T, vi *kargoapi.VerificationInfo, err error) { + require.NoError(t, err) require.NotNilf(t, vi, "expected non-nil VerificationInfo") require.NotEmptyf(t, vi.ID, "expected non-empty VerificationInfo.ID") require.Equal(t, kargoapi.VerificationPhasePending, vi.Phase) @@ -501,12 +575,14 @@ func TestStartVerification(t *testing.T) { } for _, testCase := range testCases { t.Run(testCase.name, func(t *testing.T) { + info, err := testCase.reconciler.startVerification( + context.Background(), + testCase.stage, + ) testCase.assertions( t, - testCase.reconciler.startVerification( - context.Background(), - testCase.stage, - ), + info, + err, ) }) } @@ -517,7 +593,7 @@ func TestGetVerificationInfo(t *testing.T) { name string stage *kargoapi.Stage reconciler *reconciler - assertions func(*testing.T, *kargoapi.VerificationInfo) + assertions func(*testing.T, *kargoapi.VerificationInfo, error) }{ { name: "rollouts integration not enabled", @@ -529,7 +605,8 @@ func TestGetVerificationInfo(t *testing.T) { }, }, }, - assertions: func(t *testing.T, vi *kargoapi.VerificationInfo) { + assertions: func(t *testing.T, vi *kargoapi.VerificationInfo, err error) { + require.NoError(t, err) require.NotNil(t, vi) require.Contains( t, @@ -562,7 +639,8 @@ func TestGetVerificationInfo(t *testing.T) { return nil, errors.New("something went wrong") }, }, - assertions: func(t *testing.T, vi *kargoapi.VerificationInfo) { + assertions: func(t *testing.T, vi *kargoapi.VerificationInfo, err error) { + require.ErrorContains(t, err, "something went wrong") require.NotNil(t, vi) require.Contains(t, vi.Message, "something went wrong") require.Contains(t, vi.Message, "error getting AnalysisRun") @@ -592,7 +670,8 @@ func TestGetVerificationInfo(t *testing.T) { return nil, nil }, }, - assertions: func(t *testing.T, vi *kargoapi.VerificationInfo) { + assertions: func(t *testing.T, vi *kargoapi.VerificationInfo, err error) { + require.NoError(t, err) require.NotNil(t, vi) require.Contains(t, vi.Message, "AnalysisRun") require.Contains(t, vi.Message, "not found") @@ -630,7 +709,8 @@ func TestGetVerificationInfo(t *testing.T) { }, nil }, }, - assertions: func(t *testing.T, vi *kargoapi.VerificationInfo) { + assertions: func(t *testing.T, vi *kargoapi.VerificationInfo, err error) { + require.NoError(t, err) require.Equal( t, &kargoapi.VerificationInfo{ @@ -648,12 +728,14 @@ func TestGetVerificationInfo(t *testing.T) { } for _, testCase := range testCases { t.Run(testCase.name, func(t *testing.T) { + info, err := testCase.reconciler.getVerificationInfo( + context.Background(), + testCase.stage, + ) testCase.assertions( t, - testCase.reconciler.getVerificationInfo( - context.Background(), - testCase.stage, - ), + info, + err, ) }) } diff --git a/internal/kubeclient/errors.go b/internal/kubeclient/errors.go new file mode 100644 index 0000000000..d792cbe98e --- /dev/null +++ b/internal/kubeclient/errors.go @@ -0,0 +1,14 @@ +package kubeclient + +import ( + "k8s.io/apimachinery/pkg/api/errors" +) + +// IgnoreInvalid returns nil on Invalid errors. +// All other values that are not Invalid errors or nil are returned unmodified. +func IgnoreInvalid(err error) error { + if errors.IsInvalid(err) { + return nil + } + return err +} diff --git a/internal/kubeclient/errors_test.go b/internal/kubeclient/errors_test.go new file mode 100644 index 0000000000..2e1b3a1416 --- /dev/null +++ b/internal/kubeclient/errors_test.go @@ -0,0 +1,49 @@ +package kubeclient + +import ( + "errors" + "testing" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime/schema" + + kargoapi "github.com/akuity/kargo/api/v1alpha1" +) + +func TestIgnoreInvalid(t *testing.T) { + testCases := []struct { + name string + err error + wantErr bool + }{ + { + name: "nil", + err: nil, + wantErr: false, + }, + { + name: "arbitrary error", + err: errors.New("not invalid"), + wantErr: true, + }, + { + name: "invalid", + err: apierrors.NewInvalid( + schema.GroupKind{ + Group: kargoapi.GroupVersion.Group, + Kind: "Freight", + }, + "freight", + nil, + ), + wantErr: false, + }, + } + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + if err := IgnoreInvalid(testCase.err); (err != nil) != testCase.wantErr { + t.Errorf("IgnoreInvalid() error = %v, wantErr %v", err, testCase.wantErr) + } + }) + } +}