From cc293f0bd128943e097d9ae76ea69469b7a1c462 Mon Sep 17 00:00:00 2001 From: Kent Rancourt Date: Fri, 22 Nov 2024 14:08:41 -0500 Subject: [PATCH] fix(controller)!: do not attempt to infer desired revision when not specified (#2980) Signed-off-by: Kent Rancourt --- docs/docs/35-references/10-promotion-steps.md | 6 +- internal/directives/argocd_revisions.go | 100 ------------------ internal/directives/argocd_revisions_test.go | 88 ++++++--------- internal/directives/argocd_updater.go | 29 +---- internal/directives/argocd_updater_test.go | 69 ++---------- 5 files changed, 46 insertions(+), 246 deletions(-) diff --git a/docs/docs/35-references/10-promotion-steps.md b/docs/docs/35-references/10-promotion-steps.md index 4d183e326..80445af2c 100644 --- a/docs/docs/35-references/10-promotion-steps.md +++ b/docs/docs/35-references/10-promotion-steps.md @@ -1024,9 +1024,9 @@ spec: | `apps[].sources` | `[]object` | N | Describes Argo CD `ApplicationSource`s to update and how to update them. | | `apps[].sources[].repoURL` | `string` | Y | The value of the target `ApplicationSource`'s own `repoURL` field. This must match exactly. | | `apps[].sources[].chart` | `string` | N | Applicable only when the target `ApplicationSource` references a Helm chart repository, the value of the target `ApplicationSource`'s own `chart` field. This must match exactly. | -| `apps[].sources[].desiredRevision` | `string` | N | Specifies the desired revision for the source. This field is mutually exclusive with `desiredCommitFromStep`. If both are left undefined, the desired revision will be determined by Freight (if possible). Note that the source's `targetRevision` will not be updated to this revision unless `updateTargetRevision=true` is also set. The utility of this field, on its own, is to specify the revision that the `ApplicationSource` should be observably synced to during a health check. | -| `apps[].sources[].desiredCommitFromStep` | `string` | N | Applicable only when `repoURL` references a Git repository, this field references the `commit` output from a previous step and uses it as the desired revision for the source. This field is mutually exclusive with `desiredRevisionFromStep`. If both are left undefined, the desired revision will be determined by Freight (if possible). Note that the source's `targetRevision` will not be updated to this commit unless `updateTargetRevision=true` is also set. The utility of this field, on its own, is to specify the revision that the `ApplicationSource` should be observably synced to during a health check.

__Deprecated: Use `desiredRevision` with an expression instead. Will be removed in v1.2.0.__ | -| `apps[].sources[].updateTargetRevision` | `boolean` | Y | Indicates whether the target `ApplicationSource` should be updated such that its `targetRevision` field points directly at the desired revision. | +| `apps[].sources[].desiredRevision` | `string` | N | Specifies the desired revision for the source. i.e. The revision to which the source must be observably synced when performing a health check. This field is mutually exclusive with `desiredCommitFromStep`. Prior to v1.1.0, if both were left undefined, the desired revision was determined by Freight (if possible). Beginning with v1.1.0, if both are left undefined, Kargo will not require the source to be observably synced to any particular source to be considered healthy. Note that the source's `targetRevision` will not be updated to this revision unless `updateTargetRevision=true` is also set. | +| `apps[].sources[].desiredCommitFromStep` | `string` | N | Applicable only when `repoURL` references a Git repository, this field references the `commit` output from a previous step and uses it as the desired revision for the source. i.e. The revision to which the source must be observably synced when performing a health check. This field is mutually exclusive with `desiredRevisionFromStep`. Prior to v1.1.0, if both were left undefined, the desired revision was determined by Freight (if possible). Beginning with v1.1.0, if both are left undefined, Kargo will not require the source to be observably synced to any particular source to be considered healthy. Note that the source's `targetRevision` will not be updated to this commit unless `updateTargetRevision=true` is also set.

__Deprecated: Use `desiredRevision` with an expression instead. Will be removed in v1.2.0.__ | +| `apps[].sources[].updateTargetRevision` | `boolean` | Y | Indicates whether the target `ApplicationSource` should be updated such that its `targetRevision` field points directly at the desired revision. A `true` value in this field has no effect if `desiredRevision` and `desiredCommitFromStep` are both undefined. | | `apps[].sources[].kustomize` | `object` | N | Describes updates to an Argo CD `ApplicationSource`'s Kustomize-specific properties. | | `apps[].sources[].kustomize.images` | `[]object` | Y | Describes how to update an Argo CD `ApplicationSource`'s Kustomize-specific properties to reference specific versions of container images. | | `apps[].sources[].kustomize.images[].repoURL` | `string` | Y | URL of the image being updated. | diff --git a/internal/directives/argocd_revisions.go b/internal/directives/argocd_revisions.go index 13c2b26a0..c845d0d99 100644 --- a/internal/directives/argocd_revisions.go +++ b/internal/directives/argocd_revisions.go @@ -1,14 +1,9 @@ package directives import ( - "context" - "errors" "fmt" - "strings" - kargoapi "github.com/akuity/kargo/api/v1alpha1" argocd "github.com/akuity/kargo/internal/controller/argocd/api/v1alpha1" - "github.com/akuity/kargo/internal/controller/freight" ) // getDesiredRevisions returns the desired revisions for all sources of the given @@ -18,9 +13,7 @@ import ( // Sources slice. For any source whose desired revision cannot be determined, // the slice will contain an empty string at the corresponding index. func (a *argocdUpdater) getDesiredRevisions( - ctx context.Context, stepCtx *PromotionStepContext, - stepCfg *ArgoCDUpdateConfig, update *ArgoCDAppUpdate, app *argocd.Application, ) ([]string, error) { @@ -51,104 +44,11 @@ func (a *argocdUpdater) getDesiredRevisions( return nil, err } } - if revisions[i] != "" { - continue - } } - var desiredOrigin *kargoapi.FreightOrigin - // If there is a source update that targets this source, it might be - // specific about which origin the desired revision should come from. - if sourceUpdate != nil { - desiredOrigin = getDesiredOrigin(stepCfg, sourceUpdate) - } else { - desiredOrigin = getDesiredOrigin(stepCfg, update) - } - desiredRevision, err := a.getDesiredRevisionForSource( - ctx, - stepCtx, - &src, - desiredOrigin, - ) - if err != nil { - return nil, err - } - revisions[i] = desiredRevision } return revisions, nil } -func (a *argocdUpdater) getDesiredRevisionForSource( - ctx context.Context, - stepCtx *PromotionStepContext, - src *argocd.ApplicationSource, - desiredOrigin *kargoapi.FreightOrigin, -) (string, error) { - switch { - case src.Chart != "": - // This source points to a Helm chart. - repoURL := src.RepoURL - chartName := src.Chart - if !strings.Contains(repoURL, "://") { - // In Argo CD ApplicationSource, if a repo URL specifies no protocol and a - // chart name is set (already confirmed at this point), we can assume that - // the repo URL is an OCI registry URL. Kargo Warehouses and Freight, - // however, do use oci:// at the beginning of such URLs. - // - // Additionally, where OCI is concerned, an ApplicationSource's repoURL is - // really a registry URL, and the chart name is a repository within that - // registry. Warehouses and Freight, however, handle things more correctly - // where a repoURL points directly to a repository and chart name is - // irrelevant / blank. We need to account for this when we search our - // Freight for the chart. - repoURL = fmt.Sprintf( - "oci://%s/%s", - strings.TrimSuffix(repoURL, "/"), - chartName, - ) - chartName = "" - } - chart, err := freight.FindChart( - ctx, - stepCtx.KargoClient, - stepCtx.Project, - stepCtx.FreightRequests, - desiredOrigin, - stepCtx.Freight.References(), - repoURL, - chartName, - ) - if err != nil { - if errors.As(err, &freight.NotFoundError{}) { - return "", nil - } - return "", - fmt.Errorf("error finding chart from repo %q: %w", repoURL, err) - } - return chart.Version, nil - case src.RepoURL != "": - // This source points to a Git repository. - commit, err := freight.FindCommit( - ctx, - stepCtx.KargoClient, - stepCtx.Project, - stepCtx.FreightRequests, - desiredOrigin, - stepCtx.Freight.References(), - src.RepoURL, - ) - if err != nil { - if errors.As(err, &freight.NotFoundError{}) { - return "", nil - } - return "", - fmt.Errorf("error finding commit from repo %q: %w", src.RepoURL, err) - } - return commit.ID, nil - } - // If we end up here, no desired revision was found. - return "", nil -} - // findSourceUpdate finds and returns the ArgoCDSourceUpdate that targets the // given source. If no such update exists, it returns nil. func (a *argocdUpdater) findSourceUpdate( diff --git a/internal/directives/argocd_revisions_test.go b/internal/directives/argocd_revisions_test.go index 7a1d09b8c..a74a8eace 100644 --- a/internal/directives/argocd_revisions_test.go +++ b/internal/directives/argocd_revisions_test.go @@ -1,7 +1,6 @@ package directives import ( - "context" "testing" "github.com/stretchr/testify/require" @@ -41,46 +40,43 @@ func Test_argoCDUpdater_getDesiredRevisions(t *testing.T) { // in this case. }, { - // This has an update and a matching artifact in the Freight. We - // should know what revision we want. + // This has a matching artifact in the Freight, but no update + // that specifies the desired revision. + // + // Before v1.1, we would have inferred the desired revision from + // the Freight. + // + // Beginning with v1.1, we make no attempt to infer the desired + // revision when it is not explicitly specified. + // + // This case is here purely as validation of the updated behavior. RepoURL: "https://example.com", Chart: "fake-chart", }, { - // This has no matching update, but does have a matching artifact - // in the Freight. We should know what revision we want. + // This has an update that directly specifies the desired + // revision. RepoURL: "https://example.com", Chart: "another-fake-chart", }, { - // This has no matching update, but does have a matching artifact - // in the Freight. We should know what revision we want. + // This has a matching artifact in the Freight, but no update + // that specifies the desired revision. // - // OCI is a special case. - RepoURL: "example.com", - Chart: "fake-chart", - }, - { - // This has no matching artifact in the Freight. We should not - // know what revision we want. - RepoURL: "https://example.com", - Chart: "yet-another-fake-chart", - }, - { - // This has an update and a matching artifact in the Freight. We - // should know what revision we want. + // Before v1.1, we would have inferred the desired revision from + // the Freight. + // + // Beginning with v1.1, we make no attempt to infer the desired + // revision when it is not explicitly specified. + // + // This case is here purely as validation of the updated behavior. RepoURL: "https://github.com/universe/42", }, { - // This has no matching update, but does have a matching artifact - // in the Freight. We should know what revision we want. + // This has an update that directly specifies the desired + // revision. RepoURL: "https://github.com/another-universe/42", }, - { - // This has no matching artifact in the Freight. We should not - // know what revision we want. - RepoURL: "https://github.com/yet-another-universe/42", - }, }, }, }, @@ -91,16 +87,7 @@ func Test_argoCDUpdater_getDesiredRevisions(t *testing.T) { { RepoURL: "https://example.com", Name: "fake-chart", - Version: "v2.0.0", - }, - { - RepoURL: "https://example.com", - Name: "another-fake-chart", - Version: "v1.0.0", - }, - { - RepoURL: "oci://example.com/fake-chart", - Version: "v3.0.0", + Version: "fake-version", }, }, Commits: []kargoapi.GitCommit{ @@ -108,14 +95,10 @@ func Test_argoCDUpdater_getDesiredRevisions(t *testing.T) { RepoURL: "https://github.com/universe/42", ID: "fake-commit", }, - { - RepoURL: "https://github.com/another-universe/42", - ID: "another-fake-commit", - }, }, }, }, - want: []string{"", "v2.0.0", "v1.0.0", "v3.0.0", "", "fake-commit", "another-fake-commit", ""}, + want: []string{"", "", "another-fake-version", "", "another-fake-commit"}, }, } @@ -129,28 +112,25 @@ func Test_argoCDUpdater_getDesiredRevisions(t *testing.T) { for _, freight := range testCase.freight { stepCtx.Freight.UpdateOrPush(freight) } - stepCfg := &ArgoCDUpdateConfig{ - Apps: []ArgoCDAppUpdate{{ + revisions, err := runner.getDesiredRevisions( + stepCtx, + &ArgoCDAppUpdate{ FromOrigin: &AppFromOrigin{ Kind: Kind(testOrigin.Kind), Name: testOrigin.Name, }, Sources: []ArgoCDAppSourceUpdate{ { - RepoURL: "https://example.com", - Chart: "fake-chart", + RepoURL: "https://example.com", + Chart: "another-fake-chart", + DesiredRevision: "another-fake-version", }, { - RepoURL: "https://github.com/universe/42", + RepoURL: "https://github.com/another-universe/42", + DesiredRevision: "another-fake-commit", }, }, - }}, - } - revisions, err := runner.getDesiredRevisions( - context.Background(), - stepCtx, - stepCfg, - &stepCfg.Apps[0], + }, testCase.app, ) require.NoError(t, err) diff --git a/internal/directives/argocd_updater.go b/internal/directives/argocd_updater.go index e8c721847..c3b0082d1 100644 --- a/internal/directives/argocd_updater.go +++ b/internal/directives/argocd_updater.go @@ -65,9 +65,7 @@ type argocdUpdater struct { ) (argocd.ApplicationSources, error) mustPerformUpdateFn func( - context.Context, *PromotionStepContext, - *ArgoCDUpdateConfig, *ArgoCDAppUpdate, *argocd.Application, argocd.ApplicationSources, @@ -189,13 +187,7 @@ func (a *argocdUpdater) runPromotionStep( ) } - desiredRevisions, err := a.getDesiredRevisions( - ctx, - stepCtx, - &stepCfg, - update, - app, - ) + desiredRevisions, err := a.getDesiredRevisions(stepCtx, update, app) if err != nil { return PromotionStepResult{Status: kargoapi.PromotionPhaseErrored}, fmt.Errorf( "error determining desired revisions for Argo CD Application %q in "+ @@ -226,14 +218,7 @@ func (a *argocdUpdater) runPromotionStep( } // Check if the update needs to be performed and retrieve its phase. - phase, mustUpdate, err := a.mustPerformUpdateFn( - ctx, - stepCtx, - &stepCfg, - update, - app, - desiredSources, - ) + phase, mustUpdate, err := a.mustPerformUpdateFn(stepCtx, update, app, desiredSources) // If we have a phase, append it to the results. if phase != "" { @@ -378,9 +363,7 @@ updateLoop: } func (a *argocdUpdater) mustPerformUpdate( - ctx context.Context, stepCtx *PromotionStepContext, - stepCfg *ArgoCDUpdateConfig, update *ArgoCDAppUpdate, app *argocd.Application, desiredSources argocd.ApplicationSources, @@ -458,13 +441,7 @@ func (a *argocdUpdater) mustPerformUpdate( } // Check if the desired revisions were applied. - desiredRevisions, err := a.getDesiredRevisions( - ctx, - stepCtx, - stepCfg, - update, - app, - ) + desiredRevisions, err := a.getDesiredRevisions(stepCtx, update, app) if err != nil { return "", true, fmt.Errorf("error determining desired revisions: %w", err) } diff --git a/internal/directives/argocd_updater_test.go b/internal/directives/argocd_updater_test.go index 24c04b7e0..b06b5fb09 100644 --- a/internal/directives/argocd_updater_test.go +++ b/internal/directives/argocd_updater_test.go @@ -516,9 +516,7 @@ func Test_argoCDUpdater_runPromotionStep(t *testing.T) { return []argocd.ApplicationSource{{}}, nil }, mustPerformUpdateFn: func( - context.Context, *PromotionStepContext, - *ArgoCDUpdateConfig, *ArgoCDAppUpdate, *argocd.Application, argocd.ApplicationSources, @@ -558,9 +556,7 @@ func Test_argoCDUpdater_runPromotionStep(t *testing.T) { return []argocd.ApplicationSource{{}}, nil }, mustPerformUpdateFn: func( - context.Context, *PromotionStepContext, - *ArgoCDUpdateConfig, *ArgoCDAppUpdate, *argocd.Application, argocd.ApplicationSources, @@ -608,9 +604,7 @@ func Test_argoCDUpdater_runPromotionStep(t *testing.T) { return []argocd.ApplicationSource{{}}, nil }, mustPerformUpdateFn: func( - context.Context, *PromotionStepContext, - *ArgoCDUpdateConfig, *ArgoCDAppUpdate, *argocd.Application, argocd.ApplicationSources, @@ -650,9 +644,7 @@ func Test_argoCDUpdater_runPromotionStep(t *testing.T) { return []argocd.ApplicationSource{{}}, nil }, mustPerformUpdateFn: func( - context.Context, *PromotionStepContext, - *ArgoCDUpdateConfig, *ArgoCDAppUpdate, *argocd.Application, argocd.ApplicationSources, @@ -692,9 +684,7 @@ func Test_argoCDUpdater_runPromotionStep(t *testing.T) { return []argocd.ApplicationSource{{}}, nil }, mustPerformUpdateFn: func( - context.Context, *PromotionStepContext, - *ArgoCDUpdateConfig, *ArgoCDAppUpdate, *argocd.Application, argocd.ApplicationSources, @@ -743,18 +733,14 @@ func Test_argoCDUpdater_runPromotionStep(t *testing.T) { return []argocd.ApplicationSource{{}}, nil }, mustPerformUpdateFn: func() func( - context.Context, *PromotionStepContext, - *ArgoCDUpdateConfig, *ArgoCDAppUpdate, *argocd.Application, argocd.ApplicationSources, ) (argocd.OperationPhase, bool, error) { var count uint return func( - context.Context, *PromotionStepContext, - *ArgoCDUpdateConfig, *ArgoCDAppUpdate, *argocd.Application, argocd.ApplicationSources, @@ -810,9 +796,7 @@ func Test_argoCDUpdater_runPromotionStep(t *testing.T) { return []argocd.ApplicationSource{{}}, nil }, mustPerformUpdateFn: func( - context.Context, *PromotionStepContext, - *ArgoCDUpdateConfig, *ArgoCDAppUpdate, *argocd.Application, argocd.ApplicationSources, @@ -852,9 +836,7 @@ func Test_argoCDUpdater_runPromotionStep(t *testing.T) { return []argocd.ApplicationSource{{}}, nil }, mustPerformUpdateFn: func( - context.Context, *PromotionStepContext, - *ArgoCDUpdateConfig, *ArgoCDAppUpdate, *argocd.Application, argocd.ApplicationSources, @@ -1008,14 +990,9 @@ func Test_argoCDUpdater_buildDesiredSources(t *testing.T) { func Test_argoCDUpdater_mustPerformUpdate(t *testing.T) { testFreightCollectionID := "fake-freight-collection" - testOrigin := kargoapi.FreightOrigin{ - Kind: kargoapi.FreightOriginKindWarehouse, - Name: "fake-warehouse", - } testCases := []struct { name string modifyApplication func(*argocd.Application) - newFreight []kargoapi.FreightReference desiredSources argocd.ApplicationSources assertions func(t *testing.T, phase argocd.OperationPhase, mustUpdate bool, err error) }{ @@ -1173,13 +1150,6 @@ func Test_argoCDUpdater_mustPerformUpdate(t *testing.T) { }, } }, - newFreight: []kargoapi.FreightReference{{ - Commits: []kargoapi.GitCommit{ - { - RepoURL: "https://github.com/universe/42", - }, - }, - }}, assertions: func(t *testing.T, phase argocd.OperationPhase, mustUpdate bool, err error) { require.ErrorContains(t, err, "operation completed without a sync result") require.Empty(t, phase) @@ -1208,15 +1178,6 @@ func Test_argoCDUpdater_mustPerformUpdate(t *testing.T) { }, } }, - newFreight: []kargoapi.FreightReference{{ - Origin: testOrigin, - Commits: []kargoapi.GitCommit{ - { - RepoURL: "https://github.com/universe/42", - ID: "fake-revision", - }, - }, - }}, assertions: func(t *testing.T, phase argocd.OperationPhase, mustUpdate bool, err error) { require.ErrorContains(t, err, "sync result revisions") require.ErrorContains(t, err, "do not match desired revisions") @@ -1286,15 +1247,6 @@ func Test_argoCDUpdater_mustPerformUpdate(t *testing.T) { }, } }, - newFreight: []kargoapi.FreightReference{{ - Origin: testOrigin, - Commits: []kargoapi.GitCommit{ - { - RepoURL: "https://github.com/universe/42", - ID: "fake-revision", - }, - }, - }}, assertions: func(t *testing.T, phase argocd.OperationPhase, mustUpdate bool, err error) { require.NoError(t, err) require.Equal(t, argocd.OperationSucceeded, phase) @@ -1317,31 +1269,22 @@ func Test_argoCDUpdater_mustPerformUpdate(t *testing.T) { testCase.modifyApplication(app) } - freight := kargoapi.FreightCollection{} - for _, ref := range testCase.newFreight { - freight.UpdateOrPush(ref) - } - // Tamper with the freight collection ID for testing purposes - freight.ID = testFreightCollectionID - stepCfg := &ArgoCDUpdateConfig{ Apps: []ArgoCDAppUpdate{{ Sources: []ArgoCDAppSourceUpdate{{ - FromOrigin: &AppFromOrigin{ - Kind: Kind(testOrigin.Kind), - Name: testOrigin.Name, - }, - RepoURL: "https://github.com/universe/42", + RepoURL: "https://github.com/universe/42", + DesiredRevision: "fake-revision", }}, }}, } phase, mustUpdate, err := runner.mustPerformUpdate( - context.Background(), &PromotionStepContext{ - Freight: freight, + Freight: kargoapi.FreightCollection{ + // Hard-coded for testing purposes + ID: testFreightCollectionID, + }, }, - stepCfg, &stepCfg.Apps[0], app, testCase.desiredSources,