Skip to content

Commit

Permalink
fix(controller)!: do not attempt to infer desired revision when not s…
Browse files Browse the repository at this point in the history 
…pecified (#2980)

Signed-off-by: Kent Rancourt <[email protected]>
  • Loading branch information
@krancour
krancour authored Nov 22, 2024
1 parent e22d494 commit cc293f0
Show file tree
Hide file tree
Showing 5 changed files with 46 additions and 246 deletions.
6 changes: 3 additions & 3 deletions docs/docs/35-references/10-promotion-steps.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.<br/><br/>__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.<br/><br/>__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. |
Expand Down
100 changes: 0 additions & 100 deletions internal/directives/argocd_revisions.go
View file
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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) {
Expand Down Expand Up @@ -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(
Expand Down
88 changes: 34 additions & 54 deletions internal/directives/argocd_revisions_test.go
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package directives

import (
"context"
"testing"

"github.com/stretchr/testify/require"
Expand Down Expand Up @@ -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",
},
},
},
},
Expand All @@ -91,31 +87,18 @@ 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{
{
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"},
},
}

Expand All @@ -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)
Expand Down
29 changes: 3 additions & 26 deletions internal/directives/argocd_updater.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,7 @@ type argocdUpdater struct {
) (argocd.ApplicationSources, error)

mustPerformUpdateFn func(
context.Context,
*PromotionStepContext,
*ArgoCDUpdateConfig,
*ArgoCDAppUpdate,
*argocd.Application,
argocd.ApplicationSources,
Expand Down Expand Up @@ -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 "+
Expand Down Expand Up @@ -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 != "" {
Expand Down Expand Up @@ -378,9 +363,7 @@ updateLoop:
}

func (a *argocdUpdater) mustPerformUpdate(
ctx context.Context,
stepCtx *PromotionStepContext,
stepCfg *ArgoCDUpdateConfig,
update *ArgoCDAppUpdate,
app *argocd.Application,
desiredSources argocd.ApplicationSources,
Expand Down Expand Up @@ -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)
}
Expand Down
Loading

0 comments on commit cc293f0

Please sign in to comment.