fix: cache missing app path and commit verification errors (argoproj#…

…4947)

* fix: cache missing app path and commit verification errors

Signed-off-by: Alexander Matyushentsev <[email protected]>

reposerver/repository/repository.go

@@ -164,6 +164,23 @@ type operationSettings struct {
 	allowConcurrent bool
 }
 
+// operationContext contains request values which are generated by runRepoOperation (on demand) by a call to the
+// provided operationContextSrc function.
+type operationContext struct {
+
+	// application path or helm chart path
+	appPath string
+
+	// output of 'git verify-(tag/commit)', if signature verifiction is enabled (otherwise "")
+	verificationResult string
+}
+
+// The 'operation' function parameter of 'runRepoOperation' may call this function to retrieve
+// the appPath or GPG verificationResult.
+// Failure to generate either of these values will return an error which may be cached by
+// the calling function (for example, 'runManifestGen')
+type operationContextSrc = func() (*operationContext, error)
+
 // runRepoOperation downloads either git folder or helm chart and executes specified operation
 // - Returns a value from the cache if present (by calling getCached(...)); if no value is present, the
 // provide operation(...) is called. The specific return type of this function is determined by the
@@ -175,13 +192,12 @@ func (s *Service) runRepoOperation(
 	source *v1alpha1.ApplicationSource,
 	verifyCommit bool,
 	getCached func(cacheKey string, firstInvocation bool) (bool, interface{}, error),
-	operation func(appPath, repoRoot, commitSHA, cacheKey, verifyResult string) (interface{}, error),
+	operation func(repoRoot, commitSHA, cacheKey string, ctxSrc operationContextSrc) (interface{}, error),
 	settings operationSettings) (interface{}, error) {
 
 	var gitClient git.Client
 	var helmClient helm.Client
 	var err error
-	var signature string
 	revision = textutils.FirstNonEmpty(revision, source.TargetRevision)
 	if source.IsHelm() {
 		helmClient, revision, err = s.newHelmClientResolveRevision(repo, revision, source.Chart)
@@ -229,7 +245,9 @@ func (s *Service) runRepoOperation(
 			return nil, err
 		}
 		defer io.Close(closer)
-		return operation(chartPath, chartPath, revision, revision, "")
+		return operation(chartPath, revision, revision, func() (*operationContext, error) {
+			return &operationContext{chartPath, ""}, nil
+		})
 	} else {
 		closer, err := s.repoLock.Lock(gitClient.Root(), revision, settings.allowConcurrent, func() error {
 			return checkoutRevision(gitClient, revision)
@@ -253,28 +271,31 @@ func (s *Service) runRepoOperation(
 				return obj, err
 			}
 		}
-		if verifyCommit {
-			signature, err = gitClient.VerifyCommitSignature(revision)
+		// Here commitSHA refers to the SHA of the actual commit, whereas revision refers to the branch/tag name etc
+		// We use the commitSHA to generate manifests and store them in cache, and revision to retrieve them from cache
+		return operation(gitClient.Root(), commitSHA, revision, func() (*operationContext, error) {
+			var signature string
+			if verifyCommit {
+				signature, err = gitClient.VerifyCommitSignature(revision)
+				if err != nil {
+					return nil, err
+				}
+			}
+			appPath, err := argopath.Path(gitClient.Root(), source.Path)
 			if err != nil {
 				return nil, err
 			}
-		}
-		appPath, err := argopath.Path(gitClient.Root(), source.Path)
-		if err != nil {
-			return nil, err
-		}
-		// Here commitSHA refers to the SHA of the actual commit, whereas revision refers to the branch/tag name etc
-		// We use the commitSHA to generate manifests and store them in cache, and revision to retrieve them from cache
-		return operation(appPath, gitClient.Root(), commitSHA, revision, signature)
+			return &operationContext{appPath, signature}, nil
+		})
 	}
 }
 
 func (s *Service) GenerateManifest(ctx context.Context, q *apiclient.ManifestRequest) (*apiclient.ManifestResponse, error) {
 	resultUncast, err := s.runRepoOperation(ctx, q.Revision, q.Repo, q.ApplicationSource, q.VerifySignature,
 		func(cacheKey string, firstInvocation bool) (bool, interface{}, error) {
 			return s.getManifestCacheEntry(cacheKey, q, firstInvocation)
-		}, func(appPath, repoRoot, commitSHA, cacheKey, verifyResult string) (interface{}, error) {
-			return s.runManifestGen(appPath, repoRoot, commitSHA, cacheKey, verifyResult, q)
+		}, func(repoRoot, commitSHA, cacheKey string, ctxSrc operationContextSrc) (interface{}, error) {
+			return s.runManifestGen(repoRoot, commitSHA, cacheKey, ctxSrc, q)
 		}, operationSettings{sem: s.parallelismLimitSemaphore, noCache: q.NoCache, allowConcurrent: q.ApplicationSource.AllowsConcurrentProcessing()})
 	result, ok := resultUncast.(*apiclient.ManifestResponse)
 	if result != nil && !ok {
@@ -289,8 +310,12 @@ func (s *Service) GenerateManifest(ctx context.Context, q *apiclient.ManifestReq
 // - or, the cache does contain a value for this key, but it is an expired manifest generation entry
 // - or, NoCache is true
 // Returns a ManifestResponse, or an error, but not both
-func (s *Service) runManifestGen(appPath, repoRoot, commitSHA, cacheKey, verifyResult string, q *apiclient.ManifestRequest) (interface{}, error) {
-	manifestGenResult, err := GenerateManifests(appPath, repoRoot, commitSHA, q, false)
+func (s *Service) runManifestGen(repoRoot, commitSHA, cacheKey string, ctxSrc operationContextSrc, q *apiclient.ManifestRequest) (interface{}, error) {
+	var manifestGenResult *apiclient.ManifestResponse
+	ctx, err := ctxSrc()
+	if err == nil {
+		manifestGenResult, err = GenerateManifests(ctx.appPath, repoRoot, commitSHA, q, false)
+	}
 	if err != nil {
 
 		// If manifest generation error caching is enabled
@@ -332,7 +357,7 @@ func (s *Service) runManifestGen(appPath, repoRoot, commitSHA, cacheKey, verifyR
 		MostRecentError:                 "",
 	}
 	manifestGenResult.Revision = commitSHA
-	manifestGenResult.VerifyResult = verifyResult
+	manifestGenResult.VerifyResult = ctx.verificationResult
 	err = s.cache.SetManifests(cacheKey, q.ApplicationSource, q.Namespace, q.AppLabelKey, q.AppLabelValue, &manifestGenCacheEntry)
 	if err != nil {
 		log.Warnf("manifest cache set error %s/%s: %v", q.ApplicationSource.String(), cacheKey, err)
@@ -1014,7 +1039,12 @@ func (s *Service) GetAppDetails(ctx context.Context, q *apiclient.RepoServerAppD
 
 	}
 
-	resultUncast, err := s.runRepoOperation(ctx, q.Source.TargetRevision, q.Repo, q.Source, false, getCached, func(appPath, repoRoot, commitSHA, revision, verifyResult string) (interface{}, error) {
+	resultUncast, err := s.runRepoOperation(ctx, q.Source.TargetRevision, q.Repo, q.Source, false, getCached, func(repoRoot, commitSHA, revision string, ctxSrc operationContextSrc) (interface{}, error) {
+		ctx, err := ctxSrc()
+		if err != nil {
+			return nil, err
+		}
+		appPath := ctx.appPath
 
 		res := &apiclient.RepoAppDetailsResponse{}
 		appSourceType, err := GetAppSourceType(q.Source, appPath)