From 16c20a84b85807ad3b079cb8acd7b397c4122689 Mon Sep 17 00:00:00 2001 From: "gcp-cherry-pick-bot[bot]" <98988430+gcp-cherry-pick-bot[bot]@users.noreply.github.com> Date: Mon, 24 Jun 2024 09:55:56 -0400 Subject: [PATCH] fix(server): could not find source for metadata revision (#18744) (#18763) (#18782) * fix(server): could not find source for metadata revision (#18744) * lint * the linter behaves poorly * fix test * share logic with chart endpoint * more intuitive check * remove debug line --------- Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> Co-authored-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> --- server/application/application.go | 157 +++++------ server/application/application_test.go | 264 ++++++++++++++++++ .../application-deployment-history.tsx | 4 +- .../application-details.tsx | 13 +- ui/src/app/applications/components/utils.tsx | 6 +- .../shared/services/applications-service.ts | 30 +- 6 files changed, 373 insertions(+), 101 deletions(-) diff --git a/server/application/application.go b/server/application/application.go index b919bcdd66ac7..af535ca6295c8 100644 --- a/server/application/application.go +++ b/server/application/application.go @@ -1495,71 +1495,9 @@ func (s *Server) RevisionMetadata(ctx context.Context, q *application.RevisionMe return nil, err } - var versionId int64 = 0 - if q.VersionId != nil { - versionId = int64(*q.VersionId) - } - - var source *v1alpha1.ApplicationSource - - // To support changes between single source and multi source revisions - // we have to calculate if the operation has to be done as multisource or not. - // There are 2 different scenarios, checking current revision and historic revision - // - Current revision (VersionId is nil or 0): - // - The application is multi source and required version too -> multi source - // - The application is single source and the required version too -> single source - // - The application is multi source and the required version is single source -> single source - // - The application is single source and the required version is multi source -> multi source - // - Historic revision: - // - The application is multi source and the previous one too -> multi source - // - The application is single source and the previous one too -> single source - // - The application is multi source and the previous one is single source -> multi source - // - The application is single source and the previous one is multi source -> single source - isRevisionMultiSource := a.Spec.HasMultipleSources() - emptyHistory := len(a.Status.History) == 0 - if !emptyHistory { - for _, h := range a.Status.History { - if h.ID == versionId { - isRevisionMultiSource = len(h.Revisions) > 0 - break - } - } - } - - // If the historical data is empty (because the app hasn't been synced yet) - // we can use the source, if not (the app has been synced at least once) - // we have to use the history because sources can be added/removed - if emptyHistory { - if isRevisionMultiSource { - source = &a.Spec.Sources[*q.SourceIndex] - } else { - s := a.Spec.GetSource() - source = &s - } - } else { - // the source count can change during the time, we cannot just trust in .status.sync - // because if a source has been added/removed, the revisions there won't match - // as this is only used for the UI and not internally, we can use the historical data - // using the specific revisionId - for _, h := range a.Status.History { - if h.ID == versionId { - // The iteration values are assigned to the respective iteration variables as in an assignment statement. - // The iteration variables may be declared by the “range” clause using a form of short variable declaration (:=). - // In this case their types are set to the types of the respective iteration values and their scope is the block of the "for" statement; - // they are re-used in each iteration. If the iteration variables are declared outside the "for" statement, - // after execution their values will be those of the last iteration. - // https://golang.org/ref/spec#For_statements - h := h - if isRevisionMultiSource { - source = &h.Sources[*q.SourceIndex] - } else { - source = &h.Source - } - } - } - } - if source == nil { - return nil, fmt.Errorf("revision not found: %w", err) + source, err := getAppSourceBySourceIndexAndVersionId(a, q.SourceIndex, q.VersionId) + if err != nil { + return nil, fmt.Errorf("error getting app source by source index and version ID: %w", err) } repo, err := s.db.GetRepository(ctx, source.RepoURL, proj.Name) @@ -1585,22 +1523,9 @@ func (s *Server) RevisionChartDetails(ctx context.Context, q *application.Revisi return nil, err } - var source *v1alpha1.ApplicationSource - if a.Spec.HasMultipleSources() { - // the source count can change during the time, we cannot just trust in .status.sync - // because if a source has been added/removed, the revisions there won't match - // as this is only used for the UI and not internally, we can use the historical data - // using the specific revisionId - for _, h := range a.Status.History { - if h.ID == int64(*q.VersionId) { - source = &h.Sources[*q.SourceIndex] - } - } - if source == nil { - return nil, fmt.Errorf("revision not found: %w", err) - } - } else { - source = a.Spec.Source + source, err := getAppSourceBySourceIndexAndVersionId(a, q.SourceIndex, q.VersionId) + if err != nil { + return nil, fmt.Errorf("error getting app source by source index and version ID: %w", err) } if source.Chart == "" { @@ -1622,6 +1547,76 @@ func (s *Server) RevisionChartDetails(ctx context.Context, q *application.Revisi }) } +// getAppSourceBySourceIndexAndVersionId returns the source for a specific source index and version ID. Source index and +// version ID are optional. If the source index is not specified, it defaults to 0. If the version ID is not specified, +// we use the source(s) currently configured for the app. If the version ID is specified, we find the source for that +// version ID. If the version ID is not found, we return an error. If the source index is out of bounds for whichever +// source we choose (configured sources or sources for a specific version), we return an error. +func getAppSourceBySourceIndexAndVersionId(a *appv1.Application, sourceIndexMaybe *int32, versionIdMaybe *int32) (appv1.ApplicationSource, error) { + // Start with all the app's configured sources. + sources := a.Spec.GetSources() + + // If the user specified a version, get the sources for that version. If the version is not found, return an error. + if versionIdMaybe != nil { + versionId := int64(*versionIdMaybe) + var err error + sources, err = getSourcesByVersionId(a, versionId) + if err != nil { + return appv1.ApplicationSource{}, fmt.Errorf("error getting source by version ID: %w", err) + } + } + + // Start by assuming we want the first source. + sourceIndex := 0 + + // If the user specified a source index, use that instead. + if sourceIndexMaybe != nil { + sourceIndex = int(*sourceIndexMaybe) + if sourceIndex >= len(sources) { + if len(sources) == 1 { + return appv1.ApplicationSource{}, fmt.Errorf("source index %d not found because there is only 1 source", sourceIndex) + } + return appv1.ApplicationSource{}, fmt.Errorf("source index %d not found because there are only %d sources", sourceIndex, len(sources)) + } + } + + source := sources[sourceIndex] + + return source, nil +} + +// getRevisionHistoryByVersionId returns the revision history for a specific version ID. +// If the version ID is not found, it returns an empty revision history and false. +func getRevisionHistoryByVersionId(histories v1alpha1.RevisionHistories, versionId int64) (appv1.RevisionHistory, bool) { + for _, h := range histories { + if h.ID == versionId { + return h, true + } + } + return appv1.RevisionHistory{}, false +} + +// getSourcesByVersionId returns the sources for a specific version ID. If there is no history, it returns an error. +// If the version ID is not found, it returns an error. If the version ID is found, and there are multiple sources, +// it returns the sources for that version ID. If the version ID is found, and there is only one source, it returns +// a slice with just the single source. +func getSourcesByVersionId(a *appv1.Application, versionId int64) ([]appv1.ApplicationSource, error) { + if len(a.Status.History) == 0 { + return nil, fmt.Errorf("version ID %d not found because the app has no history", versionId) + } + + h, ok := getRevisionHistoryByVersionId(a.Status.History, versionId) + if !ok { + return nil, fmt.Errorf("revision history not found for version ID %d", versionId) + } + + if len(h.Sources) > 0 { + return h.Sources, nil + } + + return []v1alpha1.ApplicationSource{h.Source}, nil +} + func isMatchingResource(q *application.ResourcesQuery, key kube.ResourceKey) bool { return (q.GetName() == "" || q.GetName() == key.Name) && (q.GetNamespace() == "" || q.GetNamespace() == key.Namespace) && diff --git a/server/application/application_test.go b/server/application/application_test.go index ca81e7a6151a2..96bfeaf51221c 100644 --- a/server/application/application_test.go +++ b/server/application/application_test.go @@ -10,6 +10,8 @@ import ( "testing" "time" + "k8s.io/utils/pointer" + "k8s.io/apimachinery/pkg/labels" "github.com/argoproj/gitops-engine/pkg/health" @@ -3025,3 +3027,265 @@ func TestServer_ResolveSourceRevisions_SingleSource(t *testing.T) { assert.Equal(t, ([]string)(nil), sourceRevisions) assert.Equal(t, ([]string)(nil), displayRevisions) } + +func Test_RevisionMetadata(t *testing.T) { + singleSourceApp := newTestApp() + singleSourceApp.Name = "single-source-app" + singleSourceApp.Spec = appv1.ApplicationSpec{ + Source: &appv1.ApplicationSource{ + RepoURL: "https://github.com/argoproj/argocd-example-apps.git", + Path: "helm-guestbook", + TargetRevision: "HEAD", + }, + } + + multiSourceApp := newTestApp() + multiSourceApp.Name = "multi-source-app" + multiSourceApp.Spec = appv1.ApplicationSpec{ + Sources: []appv1.ApplicationSource{ + { + RepoURL: "https://github.com/argoproj/argocd-example-apps.git", + Path: "helm-guestbook", + TargetRevision: "HEAD", + }, + { + RepoURL: "https://github.com/argoproj/argocd-example-apps.git", + Path: "kustomize-guestbook", + TargetRevision: "HEAD", + }, + }, + } + + singleSourceHistory := []appv1.RevisionHistory{ + { + ID: 1, + Source: singleSourceApp.Spec.GetSource(), + Revision: "a", + }, + } + multiSourceHistory := []appv1.RevisionHistory{ + { + ID: 1, + Sources: multiSourceApp.Spec.GetSources(), + Revisions: []string{"a", "b"}, + }, + } + + testCases := []struct { + name string + multiSource bool + history *struct { + matchesSourceType bool + } + sourceIndex *int32 + versionId *int32 + expectErrorContains *string + }{ + { + name: "single-source app without history, no source index, no version ID", + multiSource: false, + }, + { + name: "single-source app without history, no source index, missing version ID", + multiSource: false, + versionId: pointer.Int32(999), + expectErrorContains: pointer.String("the app has no history"), + }, + { + name: "single source app without history, present source index, no version ID", + multiSource: false, + sourceIndex: pointer.Int32(0), + }, + { + name: "single source app without history, invalid source index, no version ID", + multiSource: false, + sourceIndex: pointer.Int32(999), + expectErrorContains: pointer.String("source index 999 not found"), + }, + { + name: "single source app with matching history, no source index, no version ID", + multiSource: false, + history: &struct{ matchesSourceType bool }{true}, + }, + { + name: "single source app with matching history, no source index, missing version ID", + multiSource: false, + history: &struct{ matchesSourceType bool }{true}, + versionId: pointer.Int32(999), + expectErrorContains: pointer.String("history not found for version ID 999"), + }, + { + name: "single source app with matching history, no source index, present version ID", + multiSource: false, + history: &struct{ matchesSourceType bool }{true}, + versionId: pointer.Int32(1), + }, + { + name: "single source app with multi-source history, no source index, no version ID", + multiSource: false, + history: &struct{ matchesSourceType bool }{false}, + }, + { + name: "single source app with multi-source history, no source index, missing version ID", + multiSource: false, + history: &struct{ matchesSourceType bool }{false}, + versionId: pointer.Int32(999), + expectErrorContains: pointer.String("history not found for version ID 999"), + }, + { + name: "single source app with multi-source history, no source index, present version ID", + multiSource: false, + history: &struct{ matchesSourceType bool }{false}, + versionId: pointer.Int32(1), + }, + { + name: "single-source app with multi-source history, source index 1, no version ID", + multiSource: false, + sourceIndex: pointer.Int32(1), + history: &struct{ matchesSourceType bool }{false}, + // Since the user requested source index 1, but no version ID, we'll get an error when looking at the live + // source, because the live source is single-source. + expectErrorContains: pointer.String("there is only 1 source"), + }, + { + name: "single-source app with multi-source history, invalid source index, no version ID", + multiSource: false, + sourceIndex: pointer.Int32(999), + history: &struct{ matchesSourceType bool }{false}, + expectErrorContains: pointer.String("source index 999 not found"), + }, + { + name: "single-source app with multi-source history, valid source index, present version ID", + multiSource: false, + sourceIndex: pointer.Int32(1), + history: &struct{ matchesSourceType bool }{false}, + versionId: pointer.Int32(1), + }, + { + name: "multi-source app without history, no source index, no version ID", + multiSource: true, + }, + { + name: "multi-source app without history, no source index, missing version ID", + multiSource: true, + versionId: pointer.Int32(999), + expectErrorContains: pointer.String("the app has no history"), + }, + { + name: "multi-source app without history, present source index, no version ID", + multiSource: true, + sourceIndex: pointer.Int32(1), + }, + { + name: "multi-source app without history, invalid source index, no version ID", + multiSource: true, + sourceIndex: pointer.Int32(999), + expectErrorContains: pointer.String("source index 999 not found"), + }, + { + name: "multi-source app with matching history, no source index, no version ID", + multiSource: true, + history: &struct{ matchesSourceType bool }{true}, + }, + { + name: "multi-source app with matching history, no source index, missing version ID", + multiSource: true, + history: &struct{ matchesSourceType bool }{true}, + versionId: pointer.Int32(999), + expectErrorContains: pointer.String("history not found for version ID 999"), + }, + { + name: "multi-source app with matching history, no source index, present version ID", + multiSource: true, + history: &struct{ matchesSourceType bool }{true}, + versionId: pointer.Int32(1), + }, + { + name: "multi-source app with single-source history, no source index, no version ID", + multiSource: true, + history: &struct{ matchesSourceType bool }{false}, + }, + { + name: "multi-source app with single-source history, no source index, missing version ID", + multiSource: true, + history: &struct{ matchesSourceType bool }{false}, + versionId: pointer.Int32(999), + expectErrorContains: pointer.String("history not found for version ID 999"), + }, + { + name: "multi-source app with single-source history, no source index, present version ID", + multiSource: true, + history: &struct{ matchesSourceType bool }{false}, + versionId: pointer.Int32(1), + }, + { + name: "multi-source app with single-source history, source index 1, no version ID", + multiSource: true, + sourceIndex: pointer.Int32(1), + history: &struct{ matchesSourceType bool }{false}, + }, + { + name: "multi-source app with single-source history, invalid source index, no version ID", + multiSource: true, + sourceIndex: pointer.Int32(999), + history: &struct{ matchesSourceType bool }{false}, + expectErrorContains: pointer.String("source index 999 not found"), + }, + { + name: "multi-source app with single-source history, valid source index, present version ID", + multiSource: true, + sourceIndex: pointer.Int32(0), + history: &struct{ matchesSourceType bool }{false}, + versionId: pointer.Int32(1), + }, + { + name: "multi-source app with single-source history, source index 1, present version ID", + multiSource: true, + sourceIndex: pointer.Int32(1), + history: &struct{ matchesSourceType bool }{false}, + versionId: pointer.Int32(1), + expectErrorContains: pointer.String("source index 1 not found"), + }, + } + + for _, tc := range testCases { + tcc := tc + t.Run(tcc.name, func(t *testing.T) { + app := singleSourceApp + if tcc.multiSource { + app = multiSourceApp + } + if tcc.history != nil { + if tcc.history.matchesSourceType { + if tcc.multiSource { + app.Status.History = multiSourceHistory + } else { + app.Status.History = singleSourceHistory + } + } else { + if tcc.multiSource { + app.Status.History = singleSourceHistory + } else { + app.Status.History = multiSourceHistory + } + } + } + + s := newTestAppServer(t, app) + + request := &application.RevisionMetadataQuery{ + Name: pointer.String(app.Name), + Revision: pointer.String("HEAD"), + SourceIndex: tcc.sourceIndex, + VersionId: tcc.versionId, + } + + _, err := s.RevisionMetadata(context.Background(), request) + if tcc.expectErrorContains != nil { + require.ErrorContains(t, err, *tcc.expectErrorContains) + } else { + require.NoError(t, err) + } + }) + } +} diff --git a/ui/src/app/applications/components/application-deployment-history/application-deployment-history.tsx b/ui/src/app/applications/components/application-deployment-history/application-deployment-history.tsx index 2e4514821d0d0..c81f2c41c55a0 100644 --- a/ui/src/app/applications/components/application-deployment-history/application-deployment-history.tsx +++ b/ui/src/app/applications/components/application-deployment-history/application-deployment-history.tsx @@ -153,7 +153,9 @@ export const ApplicationDeploymentHistory = ({ )) ) - ) : null} + ) : ( +

Click to see source details.

+ )} ))} diff --git a/ui/src/app/applications/components/application-details/application-details.tsx b/ui/src/app/applications/components/application-details/application-details.tsx index 53e9a9d77914f..34d5121f87557 100644 --- a/ui/src/app/applications/components/application-details/application-details.tsx +++ b/ui/src/app/applications/components/application-details/application-details.tsx @@ -192,7 +192,14 @@ export class ApplicationDetails extends React.Component { + const getContentForChart = ( + aRevision: string, + aSourceIndex: number | null, + aVersionId: number | null, + indx: number, + aSource: models.ApplicationSource, + sourceHeader?: JSX.Element + ) => { const showChartNonMetadataInfo = (aRevision: string, aRepoUrl: string) => { return ( <> @@ -366,9 +373,9 @@ export class ApplicationDetails extends React.Component{cont}; } else if (application.spec.source) { if (source.chart) { - cont.push(getContentForChart(revision, 0, 0, 0, source)); + cont.push(getContentForChart(revision, null, null, 0, source)); } else { - cont.push(getContentForNonChart(revision, 0, getAppCurrentVersion(application), 0, source)); + cont.push(getContentForNonChart(revision, null, getAppCurrentVersion(application), 0, source)); } return <>{cont}; } else { diff --git a/ui/src/app/applications/components/utils.tsx b/ui/src/app/applications/components/utils.tsx index b20dedfe7a25a..7b6e2437d92d8 100644 --- a/ui/src/app/applications/components/utils.tsx +++ b/ui/src/app/applications/components/utils.tsx @@ -1131,9 +1131,9 @@ export function getAppDefaultOperationSyncRevision(app?: appModels.Application) // getAppCurrentVersion gets the first app revisions from `status.sync.revisions` or, if that list is missing or empty, the `revision` // field. -export function getAppCurrentVersion(app?: appModels.Application) { - if (!app || !app.status || !app.status.history) { - return 0; +export function getAppCurrentVersion(app?: appModels.Application): number | null { + if (!app || !app.status || !app.status.history || app.status.history.length === 0) { + return null; } return app.status.history[app.status.history.length - 1].id; } diff --git a/ui/src/app/shared/services/applications-service.ts b/ui/src/app/shared/services/applications-service.ts index c131e9cf592c7..1f7b5eb684416 100644 --- a/ui/src/app/shared/services/applications-service.ts +++ b/ui/src/app/shared/services/applications-service.ts @@ -53,22 +53,26 @@ export class ApplicationsService { .then(res => res.body as models.ApplicationSyncWindowState); } - public revisionMetadata(name: string, appNamespace: string, revision: string, sourceIndex: number, versionId: number): Promise { - return requests - .get(`/applications/${name}/revisions/${revision || 'HEAD'}/metadata`) - .query({appNamespace}) - .query({sourceIndex}) - .query({versionId}) - .then(res => res.body as models.RevisionMetadata); + public revisionMetadata(name: string, appNamespace: string, revision: string, sourceIndex: number | null, versionId: number | null): Promise { + let r = requests.get(`/applications/${name}/revisions/${revision || 'HEAD'}/metadata`).query({appNamespace}); + if (sourceIndex !== null) { + r = r.query({sourceIndex}); + } + if (versionId !== null) { + r = r.query({versionId}); + } + return r.then(res => res.body as models.RevisionMetadata); } public revisionChartDetails(name: string, appNamespace: string, revision: string, sourceIndex: number, versionId: number): Promise { - return requests - .get(`/applications/${name}/revisions/${revision || 'HEAD'}/chartdetails`) - .query({appNamespace}) - .query({sourceIndex}) - .query({versionId}) - .then(res => res.body as models.ChartDetails); + let r = requests.get(`/applications/${name}/revisions/${revision || 'HEAD'}/chartdetails`).query({appNamespace}); + if (sourceIndex !== null) { + r = r.query({sourceIndex}); + } + if (versionId !== null) { + r = r.query({versionId}); + } + return r.then(res => res.body as models.ChartDetails); } public resourceTree(name: string, appNamespace: string): Promise {