fix: retain count and order of revisions for multi source apps (#14108)…

… (#14113) * fix: retain order of revisions for multi source apps (#14108) Signed-off-by: Lukas Wöhrl <[email protected]> * fix: retain revision for multi source app with ref-repos Signed-off-by: Lukas Wöhrl <[email protected]> * calculate commitSHA before quitting manifest generation Signed-off-by: Michael Crenshaw <[email protected]> --------- Signed-off-by: Lukas Wöhrl <[email protected]> Signed-off-by: Michael Crenshaw <[email protected]> Co-authored-by: Michael Crenshaw <[email protected]>
argoproj · Jun 20, 2023 · 59e6f2c · 59e6f2c
1 parent 41dc06d
commit 59e6f2c
Show file tree

Hide file tree

Showing 4 changed files with 95 additions and 64 deletions.
diff --git a/controller/appcontroller_test.go b/controller/appcontroller_test.go
@@ -49,6 +49,7 @@ type namespacedResource struct {
 type fakeData struct {
 	apps                   []runtime.Object
 	manifestResponse       *apiclient.ManifestResponse
+	manifestResponses      []*apiclient.ManifestResponse
 	managedLiveObjs        map[kube.ResourceKey]*unstructured.Unstructured
 	namespacedResources    map[kube.ResourceKey]namespacedResource
 	configMapData          map[string]string
@@ -65,7 +66,15 @@ func newFakeController(data *fakeData) *ApplicationController {
 
 	// Mock out call to GenerateManifest
 	mockRepoClient := mockrepoclient.RepoServerServiceClient{}
-	mockRepoClient.On("GenerateManifest", mock.Anything, mock.Anything).Return(data.manifestResponse, nil)
+
+	if len(data.manifestResponses) > 0 {
+		for _, response := range data.manifestResponses {
+			mockRepoClient.On("GenerateManifest", mock.Anything, mock.Anything).Return(response, nil).Once()
+		}
+	} else {
+		mockRepoClient.On("GenerateManifest", mock.Anything, mock.Anything).Return(data.manifestResponse, nil)
+	}
+
 	mockRepoClientset := mockrepoclient.Clientset{RepoServerServiceClient: &mockRepoClient}
 
 	secret := corev1.Secret{
@@ -222,9 +231,14 @@ spec:
   project: default
   sources:
   - path: some/path
+    helm:
+      valueFiles:
+      - $values_test/values.yaml
     repoURL: https://github.com/argoproj/argocd-example-apps.git
   - path: some/other/path
     repoURL: https://github.com/argoproj/argocd-example-apps-fake.git
+  - ref: values_test
+    repoURL: https://github.com/argoproj/argocd-example-apps-fake-ref.git
   syncPolicy:
     automated: {}
 status:
@@ -236,6 +250,7 @@ status:
         revisions:
         - HEAD
         - HEAD
+        - HEAD
     phase: Succeeded
     startedAt: 2018-09-21T23:50:25Z
     syncResult:
@@ -250,11 +265,14 @@ status:
       revisions:
       - aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
       - bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
+      - cccccccccccccccccccccccccccccccccccccccc
       sources:
       - path: some/path
         repoURL: https://github.com/argoproj/argocd-example-apps.git
       - path: some/other/path
         repoURL: https://github.com/argoproj/argocd-example-apps-fake.git
+      - path: some/other/path
+        repoURL: https://github.com/argoproj/argocd-example-apps-fake-ref.git
 `
 
 var fakeAppWithDestName = `

diff --git a/controller/state.go b/controller/state.go
@@ -106,7 +106,7 @@ type appStateManager struct {
 	persistResourceHealth bool
 }
 
-func (m *appStateManager) getRepoObjs(app *v1alpha1.Application, sources []v1alpha1.ApplicationSource, appLabelKey string, revisions []string, noCache, noRevisionCache, verifySignature bool, proj *v1alpha1.AppProject) ([]*unstructured.Unstructured, map[*v1alpha1.ApplicationSource]*apiclient.ManifestResponse, error) {
+func (m *appStateManager) getRepoObjs(app *v1alpha1.Application, sources []v1alpha1.ApplicationSource, appLabelKey string, revisions []string, noCache, noRevisionCache, verifySignature bool, proj *v1alpha1.AppProject) ([]*unstructured.Unstructured, []*apiclient.ManifestResponse, error) {
 
 	ts := stats.NewTimingStats()
 	helmRepos, err := m.db.ListHelmRepositories(context.Background())
@@ -163,7 +163,7 @@ func (m *appStateManager) getRepoObjs(app *v1alpha1.Application, sources []v1alp
 	}
 	defer io.Close(conn)
 
-	manifestInfoMap := make(map[*v1alpha1.ApplicationSource]*apiclient.ManifestResponse)
+	manifestInfos := make([]*apiclient.ManifestResponse, 0)
 	targetObjs := make([]*unstructured.Unstructured, 0)
 
 	// Store the map of all sources having ref field into a map for applications with sources field
@@ -173,13 +173,6 @@ func (m *appStateManager) getRepoObjs(app *v1alpha1.Application, sources []v1alp
 	}
 
 	for i, source := range sources {
-		// 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
-		source := source
 		if len(revisions) < len(sources) || revisions[i] == "" {
 			revisions[i] = source.TargetRevision
 		}
@@ -223,20 +216,14 @@ func (m *appStateManager) getRepoObjs(app *v1alpha1.Application, sources []v1alp
 			return nil, nil, err
 		}
 
-		// GenerateManifest can return empty ManifestResponse without error if app has multiple sources
-		// and if any of the source does not have path and chart field not specified.
-		// In that scenario, we continue to the next source
-		if app.Spec.HasMultipleSources() && len(manifestInfo.Manifests) == 0 {
-			continue
-		}
-
 		targetObj, err := unmarshalManifests(manifestInfo.Manifests)
 
 		if err != nil {
 			return nil, nil, err
 		}
 		targetObjs = append(targetObjs, targetObj...)
-		manifestInfoMap[&source] = manifestInfo
+
+		manifestInfos = append(manifestInfos, manifestInfo)
 	}
 
 	ts.AddCheckpoint("unmarshal_ms")
@@ -246,7 +233,7 @@ func (m *appStateManager) getRepoObjs(app *v1alpha1.Application, sources []v1alp
 	}
 	logCtx = logCtx.WithField("time_ms", time.Since(ts.StartTime).Milliseconds())
 	logCtx.Info("getRepoObjs stats")
-	return targetObjs, manifestInfoMap, nil
+	return targetObjs, manifestInfos, nil
 }
 
 func unmarshalManifests(manifests []string) ([]*unstructured.Unstructured, error) {
@@ -407,7 +394,7 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *v1
 	var targetObjs []*unstructured.Unstructured
 	now := metav1.Now()
 
-	var manifestInfoMap map[*v1alpha1.ApplicationSource]*apiclient.ManifestResponse
+	var manifestInfos []*apiclient.ManifestResponse
 
 	if len(localManifests) == 0 {
 		// If the length of revisions is not same as the length of sources,
@@ -419,7 +406,7 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *v1
 			}
 		}
 
-		targetObjs, manifestInfoMap, err = m.getRepoObjs(app, sources, appLabelKey, revisions, noCache, noRevisionCache, verifySignature, project)
+		targetObjs, manifestInfos, err = m.getRepoObjs(app, sources, appLabelKey, revisions, noCache, noRevisionCache, verifySignature, project)
 		if err != nil {
 			targetObjs = make([]*unstructured.Unstructured, 0)
 			conditions = append(conditions, v1alpha1.ApplicationCondition{Type: v1alpha1.ApplicationConditionComparisonError, Message: err.Error(), LastTransitionTime: &now})
@@ -442,9 +429,7 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *v1
 			}
 		}
 		// empty out manifestInfoMap
-		for as := range manifestInfoMap {
-			delete(manifestInfoMap, as)
-		}
+		manifestInfos = make([]*apiclient.ManifestResponse, 0)
 	}
 	ts.AddCheckpoint("git_ms")
 
@@ -524,12 +509,12 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *v1
 	}
 	manifestRevisions := make([]string, 0)
 
-	for _, manifestInfo := range manifestInfoMap {
+	for _, manifestInfo := range manifestInfos {
 		manifestRevisions = append(manifestRevisions, manifestInfo.Revision)
 	}
 
 	// restore comparison using cached diff result if previous comparison was performed for the same revision
-	revisionChanged := len(manifestInfoMap) != len(sources) || !reflect.DeepEqual(app.Status.Sync.Revisions, manifestRevisions)
+	revisionChanged := len(manifestInfos) != len(sources) || !reflect.DeepEqual(app.Status.Sync.Revisions, manifestRevisions)
 	specChanged := !reflect.DeepEqual(app.Status.Sync.ComparedTo, v1alpha1.ComparedTo{Source: app.Spec.GetSource(), Destination: app.Spec.Destination, Sources: sources})
 
 	_, refreshRequested := app.IsRefreshRequested()
@@ -698,7 +683,7 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *v1
 	// Git has already performed the signature verification via its GPG interface, and the result is available
 	// in the manifest info received from the repository server. We now need to form our opinion about the result
 	// and stop processing if we do not agree about the outcome.
-	for _, manifestInfo := range manifestInfoMap {
+	for _, manifestInfo := range manifestInfos {
 		if gpg.IsGPGEnabled() && verifySignature && manifestInfo != nil {
 			conditions = append(conditions, verifyGnuPGSignature(manifestInfo.Revision, project, manifestInfo)...)
 		}
@@ -715,11 +700,11 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *v1
 	}
 
 	if hasMultipleSources {
-		for _, manifestInfo := range manifestInfoMap {
+		for _, manifestInfo := range manifestInfos {
 			compRes.appSourceTypes = append(compRes.appSourceTypes, v1alpha1.ApplicationSourceType(manifestInfo.SourceType))
 		}
 	} else {
-		for _, manifestInfo := range manifestInfoMap {
+		for _, manifestInfo := range manifestInfos {
 			compRes.appSourceType = v1alpha1.ApplicationSourceType(manifestInfo.SourceType)
 			break
 		}

diff --git a/controller/state_test.go b/controller/state_test.go
@@ -318,8 +318,8 @@ func TestCompareAppStateExtraHook(t *testing.T) {
 	assert.Equal(t, 0, len(app.Status.Conditions))
 }
 
-// TestAppRevisions tests that revisions are properly propagated
-func TestAppRevisions(t *testing.T) {
+// TestAppRevisions tests that revisions are properly propagated for a single source app
+func TestAppRevisionsSingleSource(t *testing.T) {
 	obj1 := NewPod()
 	obj1.SetNamespace(test.FakeDestNamespace)
 	data := fakeData{
@@ -333,28 +333,57 @@ func TestAppRevisions(t *testing.T) {
 	}
 	ctrl := newFakeController(&data)
 
-	// single source
-	{
-		app := newFakeApp()
-		revisions := make([]string, 0)
-		revisions = append(revisions, "")
-		compRes := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, app.Spec.GetSources(), false, false, nil, app.Spec.HasMultipleSources())
-		assert.NotNil(t, compRes)
-		assert.NotNil(t, compRes.syncStatus)
-		assert.NotEmpty(t, compRes.syncStatus.Revision)
-		assert.Len(t, compRes.syncStatus.Revisions, 0)
-	}
-	// multisource
-	{
-		app := newFakeMultiSourceApp()
-		revisions := make([]string, 0)
-		revisions = append(revisions, "")
-		compRes := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, app.Spec.GetSources(), false, false, nil, app.Spec.HasMultipleSources())
-		assert.NotNil(t, compRes)
-		assert.NotNil(t, compRes.syncStatus)
-		assert.Empty(t, compRes.syncStatus.Revision)
-		assert.Len(t, compRes.syncStatus.Revisions, 2)
+	app := newFakeApp()
+	revisions := make([]string, 0)
+	revisions = append(revisions, "")
+	compRes := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, app.Spec.GetSources(), false, false, nil, app.Spec.HasMultipleSources())
+	assert.NotNil(t, compRes)
+	assert.NotNil(t, compRes.syncStatus)
+	assert.NotEmpty(t, compRes.syncStatus.Revision)
+	assert.Len(t, compRes.syncStatus.Revisions, 0)
+
+}
+
+// TestAppRevisions tests that revisions are properly propagated for a multi source app
+func TestAppRevisionsMultiSource(t *testing.T) {
+	obj1 := NewPod()
+	obj1.SetNamespace(test.FakeDestNamespace)
+	data := fakeData{
+		manifestResponses: []*apiclient.ManifestResponse{
+			{
+				Manifests: []string{toJSON(t, obj1)},
+				Namespace: test.FakeDestNamespace,
+				Server:    test.FakeClusterURL,
+				Revision:  "abc123",
+			},
+			{
+				Manifests: []string{toJSON(t, obj1)},
+				Namespace: test.FakeDestNamespace,
+				Server:    test.FakeClusterURL,
+				Revision:  "def456",
+			},
+			{
+				Manifests: []string{},
+				Namespace: test.FakeDestNamespace,
+				Server:    test.FakeClusterURL,
+				Revision:  "ghi789",
+			},
+		},
+		managedLiveObjs: make(map[kube.ResourceKey]*unstructured.Unstructured),
 	}
+	ctrl := newFakeController(&data)
+
+	app := newFakeMultiSourceApp()
+	revisions := make([]string, 0)
+	revisions = append(revisions, "")
+	compRes := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, app.Spec.GetSources(), false, false, nil, app.Spec.HasMultipleSources())
+	assert.NotNil(t, compRes)
+	assert.NotNil(t, compRes.syncStatus)
+	assert.Empty(t, compRes.syncStatus.Revision)
+	assert.Len(t, compRes.syncStatus.Revisions, 3)
+	assert.Equal(t, "abc123", compRes.syncStatus.Revisions[0])
+	assert.Equal(t, "def456", compRes.syncStatus.Revisions[1])
+	assert.Equal(t, "ghi789", compRes.syncStatus.Revisions[2])
 }
 
 func toJSON(t *testing.T, obj *unstructured.Unstructured) string {

diff --git a/reposerver/repository/repository.go b/reposerver/repository/repository.go
@@ -337,14 +337,6 @@ func (s *Service) runRepoOperation(
 		defer settings.sem.Release(1)
 	}
 
-	// do not generate manifests if Path and Chart fields are not set for a source in Multiple Sources
-	if hasMultipleSources && source.Path == "" && source.Chart == "" {
-		log.WithFields(map[string]interface{}{
-			"source": source,
-		}).Debugf("not generating manifests as path and chart fields are empty")
-		return nil
-	}
-
 	if source.IsHelm() {
 		if settings.noCache {
 			err = helmClient.CleanChartCache(source.Chart, revision)
@@ -510,6 +502,17 @@ func (s *Service) GenerateManifest(ctx context.Context, q *apiclient.ManifestReq
 	var promise *ManifestResponsePromise
 
 	operation := func(repoRoot, commitSHA, cacheKey string, ctxSrc operationContextSrc) error {
+		// do not generate manifests if Path and Chart fields are not set for a source in Multiple Sources
+		if q.HasMultipleSources && q.ApplicationSource.Path == "" && q.ApplicationSource.Chart == "" {
+			log.WithFields(map[string]interface{}{
+				"source": q.ApplicationSource,
+			}).Debugf("not generating manifests as path and chart fields are empty")
+			res = &apiclient.ManifestResponse{
+				Revision: commitSHA,
+			}
+			return nil
+		}
+
 		promise = s.runManifestGen(ctx, repoRoot, commitSHA, cacheKey, ctxSrc, q)
 		// The fist channel to send the message will resume this operation.
 		// The main purpose for using channels here is to be able to unlock
@@ -542,10 +545,6 @@ func (s *Service) GenerateManifest(ctx context.Context, q *apiclient.ManifestReq
 			return nil, err
 		}
 	}
-
-	if q.HasMultipleSources && err == nil && res == nil {
-		res = &apiclient.ManifestResponse{}
-	}
 	return res, err
 }