From 57d5075aff0ce0eba981875201adef06ad61f748 Mon Sep 17 00:00:00 2001 From: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> Date: Mon, 5 Dec 2022 17:05:45 -0500 Subject: [PATCH 1/4] allow spec update Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> --- pkg/apis/application/v1alpha1/types.go | 13 ++ reposerver/repository/repository.go | 4 +- util/argo/argo.go | 266 +++++++++++++------------ 3 files changed, 158 insertions(+), 125 deletions(-) diff --git a/pkg/apis/application/v1alpha1/types.go b/pkg/apis/application/v1alpha1/types.go index 7e9cae6245803..0770c36298e5b 100644 --- a/pkg/apis/application/v1alpha1/types.go +++ b/pkg/apis/application/v1alpha1/types.go @@ -192,6 +192,19 @@ func (a *ApplicationSpec) GetSource() ApplicationSource { return ApplicationSource{} } +func (a *ApplicationSpec) GetSources() ApplicationSources { + if a.HasMultipleSources() { + return a.Sources + } + if a.Source != nil { + if a.Source == nil { + return ApplicationSources{} + } + return ApplicationSources{*a.Source} + } + return ApplicationSources{} +} + func (a *ApplicationSpec) HasMultipleSources() bool { return a.Sources != nil && len(a.Sources) > 0 } diff --git a/reposerver/repository/repository.go b/reposerver/repository/repository.go index 0e6bfbcccafd6..89bd6037f4b10 100644 --- a/reposerver/repository/repository.go +++ b/reposerver/repository/repository.go @@ -942,8 +942,8 @@ func helmTemplate(appPath string, repoRoot string, env *v1alpha1.Env, q *apiclie pathStrings := strings.Split(val, "/") refVar := strings.Split(val, "/")[0] refSourceRepo := q.RefSources[refVar].Repo.Repo - repoPath, err := gitRepoPaths.GetPath(refSourceRepo) - if err != nil { + repoPath := gitRepoPaths.GetPathIfExists(refSourceRepo) + if repoPath == "" { log.Warnf("Failed to find path for ref %s", refVar) continue } diff --git a/util/argo/argo.go b/util/argo/argo.go index add4dc9159f33..85444734d7962 100644 --- a/util/argo/argo.go +++ b/util/argo/argo.go @@ -186,7 +186,6 @@ func ValidateRepo( settingsMgr *settings.SettingsManager, ) ([]argoappv1.ApplicationCondition, error) { spec := &app.Spec - var kustomizeOptions *argoappv1.KustomizeOptions conditions := make([]argoappv1.ApplicationCondition, 0) @@ -241,63 +240,21 @@ func ValidateRepo( return nil, fmt.Errorf("error getting enabled source types: %w", err) } - // If source is Kustomize add build options - kustomizeSettings, err := settingsMgr.GetKustomizeSettings() - if err != nil { - return nil, fmt.Errorf("error getting kustomize settings: %w", err) - } - - sourceCondition := make([]argoappv1.ApplicationCondition, 0) - if spec.HasMultipleSources() { - for _, source := range spec.Sources { - kustomizeOptions, err = kustomizeSettings.GetOptions(source) - if err != nil { - return nil, err - } - // stores conditions for each source - var condition []argoappv1.ApplicationCondition - condition, err = validateRepo( - ctx, - app, - db, - &source, - repoClient, - kustomizeOptions, - plugins, - permittedHelmRepos, - helmOptions, - cluster, - apiGroups, - proj, - permittedHelmCredentials, - enabledSourceTypes, - settingsMgr) - // append condition of each source to collection of conditions - sourceCondition = append(sourceCondition, condition...) - } - } else { - source := spec.GetSource() - kustomizeOptions, err = kustomizeSettings.GetOptions(source) - if err != nil { - return nil, err - } - sourceCondition, err = validateRepo( - ctx, - app, - db, - &source, - repoClient, - kustomizeOptions, - plugins, - permittedHelmRepos, - helmOptions, - cluster, - apiGroups, - proj, - permittedHelmCredentials, - enabledSourceTypes, - settingsMgr) - } + sourceCondition, err := validateRepo( + ctx, + app, + db, + app.Spec.GetSources(), + repoClient, + plugins, + permittedHelmRepos, + helmOptions, + cluster, + apiGroups, + proj, + permittedHelmCredentials, + enabledSourceTypes, + settingsMgr) if err != nil { return nil, err } @@ -309,9 +266,8 @@ func ValidateRepo( func validateRepo(ctx context.Context, app *argoappv1.Application, db db.ArgoDB, - source *argoappv1.ApplicationSource, + sources []argoappv1.ApplicationSource, repoClient apiclient.RepoServerServiceClient, - kustomizeOptions *argoappv1.KustomizeOptions, plugins []*argoappv1.ConfigManagementPlugin, permittedHelmRepos []*argoappv1.Repository, helmOptions *argoappv1.HelmOptions, @@ -325,45 +281,46 @@ func validateRepo(ctx context.Context, conditions := make([]argoappv1.ApplicationCondition, 0) errMessage := "" - repo, err := db.GetRepository(ctx, source.RepoURL) - if err != nil { - return nil, err - } - if err := TestRepoWithKnownType(ctx, repoClient, repo, source.IsHelm(), source.IsHelmOci()); err != nil { - errMessage = fmt.Sprintf("repositories not accessible: %v", repo) - } - repoAccessible := false + for _, source := range sources { + repo, err := db.GetRepository(ctx, source.RepoURL) + if err != nil { + return nil, err + } + if err := TestRepoWithKnownType(ctx, repoClient, repo, source.IsHelm(), source.IsHelmOci()); err != nil { + errMessage = fmt.Sprintf("repositories not accessible: %v", repo) + } + repoAccessible := false - if errMessage != "" { - conditions = append(conditions, argoappv1.ApplicationCondition{ - Type: argoappv1.ApplicationConditionInvalidSpecError, - Message: fmt.Sprintf("repository not accessible: %v", errMessage), - }) - } else { - repoAccessible = true - } + if errMessage != "" { + conditions = append(conditions, argoappv1.ApplicationCondition{ + Type: argoappv1.ApplicationConditionInvalidSpecError, + Message: fmt.Sprintf("repository not accessible: %v", errMessage), + }) + } else { + repoAccessible = true + } - // Verify only one source type is defined - _, err = source.ExplicitType() - if err != nil { - return nil, fmt.Errorf("error verifying source type: %w", err) - } + // Verify only one source type is defined + _, err = source.ExplicitType() + if err != nil { + return nil, fmt.Errorf("error verifying source type: %w", err) + } - // is the repo inaccessible - abort now - if !repoAccessible { - return conditions, nil + // is the repo inaccessible - abort now + if !repoAccessible { + return conditions, nil + } } conditions = append(conditions, verifyGenerateManifests( ctx, - repo, + db, permittedHelmRepos, helmOptions, app.Name, app.Spec.Destination, - source, + sources, repoClient, - kustomizeOptions, plugins, cluster.ServerVersion, APIResourcesToStrings(apiGroups, true), @@ -598,7 +555,23 @@ func GetAppProject(app *argoappv1.Application, projLister applicationsv1.AppProj } // verifyGenerateManifests verifies a repo path can generate manifests -func verifyGenerateManifests(ctx context.Context, repoRes *argoappv1.Repository, helmRepos argoappv1.Repositories, helmOptions *argoappv1.HelmOptions, name string, dest argoappv1.ApplicationDestination, source *argoappv1.ApplicationSource, repoClient apiclient.RepoServerServiceClient, kustomizeOptions *argoappv1.KustomizeOptions, plugins []*argoappv1.ConfigManagementPlugin, kubeVersion string, apiVersions []string, repositoryCredentials []*argoappv1.RepoCreds, enableGenerateManifests map[string]bool, settingsMgr *settings.SettingsManager, hasMultipleSources bool) []argoappv1.ApplicationCondition { +func verifyGenerateManifests( + ctx context.Context, + db db.ArgoDB, + helmRepos argoappv1.Repositories, + helmOptions *argoappv1.HelmOptions, + name string, + dest argoappv1.ApplicationDestination, + sources []argoappv1.ApplicationSource, + repoClient apiclient.RepoServerServiceClient, + plugins []*argoappv1.ConfigManagementPlugin, + kubeVersion string, + apiVersions []string, + repositoryCredentials []*argoappv1.RepoCreds, + enableGenerateManifests map[string]bool, + settingsMgr *settings.SettingsManager, + hasMultipleSources bool, +) []argoappv1.ApplicationCondition { var conditions []argoappv1.ApplicationCondition if dest.Server == "" { conditions = append(conditions, argoappv1.ApplicationCondition{ @@ -606,41 +579,70 @@ func verifyGenerateManifests(ctx context.Context, repoRes *argoappv1.Repository, Message: errDestinationMissing, }) } - req := apiclient.ManifestRequest{ - Repo: &argoappv1.Repository{ - Repo: source.RepoURL, - Type: repoRes.Type, - Name: repoRes.Name, - Proxy: repoRes.Proxy, - }, - Repos: helmRepos, - Revision: source.TargetRevision, - AppName: name, - Namespace: dest.Namespace, - ApplicationSource: source, - Plugins: plugins, - KustomizeOptions: kustomizeOptions, - KubeVersion: kubeVersion, - ApiVersions: apiVersions, - HelmOptions: helmOptions, - HelmRepoCreds: repositoryCredentials, - TrackingMethod: string(GetTrackingMethod(settingsMgr)), - EnabledSourceTypes: enableGenerateManifests, - NoRevisionCache: true, - HasMultipleSources: hasMultipleSources, - } - req.Repo.CopyCredentialsFromRepo(repoRes) - req.Repo.CopySettingsFrom(repoRes) - - // Only check whether we can access the application's path, - // and not whether it actually contains any manifests. - _, err := repoClient.GenerateManifest(ctx, &req) - if err != nil { - errMessage := fmt.Sprintf("Unable to generate manifests in %s: %s", source.Path, err) - conditions = append(conditions, argoappv1.ApplicationCondition{ - Type: argoappv1.ApplicationConditionInvalidSpecError, - Message: errMessage, - }) + + for _, source := range sources { + repoRes, err := db.GetRepository(ctx, source.RepoURL) + if err != nil { + conditions = append(conditions, argoappv1.ApplicationCondition{ + Type: argoappv1.ApplicationConditionInvalidSpecError, + Message: fmt.Sprintf("Unable to get repository: %v", err), + }) + continue + } + // If source is Kustomize add build options + kustomizeSettings, err := settingsMgr.GetKustomizeSettings() + if err != nil { + conditions = append(conditions, argoappv1.ApplicationCondition{ + Type: argoappv1.ApplicationConditionInvalidSpecError, + Message: fmt.Sprintf("Error getting Kustomize settings: %v", err), + }) + continue + } + kustomizeOptions, err := kustomizeSettings.GetOptions(source) + if err != nil { + conditions = append(conditions, argoappv1.ApplicationCondition{ + Type: argoappv1.ApplicationConditionInvalidSpecError, + Message: fmt.Sprintf("Error getting Kustomize options: %v", err), + }) + continue + } + req := apiclient.ManifestRequest{ + Repo: &argoappv1.Repository{ + Repo: source.RepoURL, + Type: repoRes.Type, + Name: repoRes.Name, + Proxy: repoRes.Proxy, + }, + Repos: helmRepos, + Revision: source.TargetRevision, + AppName: name, + Namespace: dest.Namespace, + ApplicationSource: &source, + Plugins: plugins, + KustomizeOptions: kustomizeOptions, + KubeVersion: kubeVersion, + ApiVersions: apiVersions, + HelmOptions: helmOptions, + HelmRepoCreds: repositoryCredentials, + TrackingMethod: string(GetTrackingMethod(settingsMgr)), + EnabledSourceTypes: enableGenerateManifests, + NoRevisionCache: true, + HasMultipleSources: hasMultipleSources, + } + req.Repo.CopyCredentialsFromRepo(repoRes) + req.Repo.CopySettingsFrom(repoRes) + req.RefSources = GetRefSources(sources, *req.Repo, hasMultipleSources) + + // Only check whether we can access the application's path, + // and not whether it actually contains any manifests. + _, err = repoClient.GenerateManifest(ctx, &req) + if err != nil { + errMessage := fmt.Sprintf("Unable to generate manifests in %s: %s", source.Path, err) + conditions = append(conditions, argoappv1.ApplicationCondition{ + Type: argoappv1.ApplicationConditionInvalidSpecError, + Message: errMessage, + }) + } } return conditions @@ -725,6 +727,24 @@ func NormalizeSource(source *argoappv1.ApplicationSource) *argoappv1.Application return source } +func GetRefSources(sources []argoappv1.ApplicationSource, repo argoappv1.Repository, hasMultipleSources bool) map[string]*argoappv1.RefTargeRevisionMapping { + refSources := make(map[string]*argoappv1.RefTargeRevisionMapping) + if hasMultipleSources { + // Get Repositories for all sources before generating Manifests + for _, source := range sources { + if source.Ref != "" { + refKey := "$" + source.Ref + refSources[refKey] = &argoappv1.RefTargeRevisionMapping{ + Repo: repo, + TargetRevision: source.TargetRevision, + Chart: source.Chart, + } + } + } + } + return refSources +} + func GetPermittedReposCredentials(proj *argoappv1.AppProject, repoCreds []*argoappv1.RepoCreds) ([]*argoappv1.RepoCreds, error) { var permittedRepoCreds []*argoappv1.RepoCreds for _, v := range repoCreds { From 4c09eaf5c50b3a52523b97c563aec3242a067d89 Mon Sep 17 00:00:00 2001 From: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> Date: Tue, 6 Dec 2022 10:11:18 -0500 Subject: [PATCH 2/4] normalize git repo (#7) Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> --- reposerver/repository/repository.go | 4 ++-- util/io/paths.go | 10 ++++++++++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/reposerver/repository/repository.go b/reposerver/repository/repository.go index 9817b5f7e5e33..5e9d10875a689 100644 --- a/reposerver/repository/repository.go +++ b/reposerver/repository/repository.go @@ -923,8 +923,8 @@ func helmTemplate(appPath string, repoRoot string, env *v1alpha1.Env, q *apiclie pathStrings := strings.Split(val, "/") refVar := strings.Split(val, "/")[0] refSourceRepo := q.RefSources[refVar].Repo.Repo - repoPath, err := gitRepoPaths.GetPath(refSourceRepo) - if err != nil { + repoPath := gitRepoPaths.GetPathIfExists(git.NormalizeGitURL(refSourceRepo)) + if repoPath == "" { log.Warnf("Failed to find path for ref %s", refVar) continue } diff --git a/util/io/paths.go b/util/io/paths.go index b9c20283bcdcd..a32d049778784 100644 --- a/util/io/paths.go +++ b/util/io/paths.go @@ -42,3 +42,13 @@ func (p *TempPaths) GetPath(key string) (string, error) { p.paths[key] = repoPath return repoPath, nil } + +// GetPathIfExists gets a path for the given key if it exists. Otherwise, returns an empty string. +func (p *TempPaths) GetPathIfExists(key string) string { + p.lock.Lock() + defer p.lock.Unlock() + if val, ok := p.paths[key]; ok { + return val + } + return "" +} From e7f03ab9b49d5459735436047d355c3c674b828c Mon Sep 17 00:00:00 2001 From: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> Date: Tue, 6 Dec 2022 10:21:03 -0500 Subject: [PATCH 3/4] Do not leak lock releases (#8) * do not leak lock releases Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * prevent deadlock Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * efficiency Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> --- reposerver/repository/repository.go | 61 +++++++++++++++++++---------- 1 file changed, 40 insertions(+), 21 deletions(-) diff --git a/reposerver/repository/repository.go b/reposerver/repository/repository.go index 5e9d10875a689..89b99ede2ad00 100644 --- a/reposerver/repository/repository.go +++ b/reposerver/repository/repository.go @@ -573,6 +573,12 @@ func (s *Service) runManifestGen(ctx context.Context, repoRoot, commitSHA, cache return responsePromise } +type repoRef struct { + revision string + commitSHA string + key string +} + func (s *Service) runManifestGenAsync(ctx context.Context, repoRoot, commitSHA, cacheKey string, opContextSrc operationContextSrc, q *apiclient.ManifestRequest, ch *generateManifestCh) { defer func() { close(ch.errCh) @@ -583,50 +589,67 @@ func (s *Service) runManifestGenAsync(ctx context.Context, repoRoot, commitSHA, // key. Overrides will break the cache anyway, because changes to overrides will change the revision. appSourceCopy := q.ApplicationSource.DeepCopy() - repoLocks := make(map[string]goio.Closer) - var manifestGenResult *apiclient.ManifestResponse opContext, err := opContextSrc() if err == nil { if q.HasMultipleSources { if q.ApplicationSource.Helm != nil { - // Checkout every the referenced Source to the target revision before generating Manifests + repoRefs := make(map[string]repoRef) + + // Checkout every one of the referenced sources to the target revision before generating Manifests for _, valueFile := range q.ApplicationSource.Helm.ValueFiles { if strings.HasPrefix(valueFile, "$") { refVar := strings.Split(valueFile, "/")[0] refSourceMapping, ok := q.RefSources[refVar] if !ok { + if len(q.RefSources) == 0 { + ch.errCh <- fmt.Errorf("source referenced %q, but no source has a 'ref' field defined", refVar) + } refKeys := make([]string, 0) for refKey := range q.RefSources { refKeys = append(refKeys, refKey) } - if len(refKeys) == 0 { - ch.errCh <- fmt.Errorf("source referenced %q, but no source has a 'ref' field defined", refVar) - } ch.errCh <- fmt.Errorf("source referenced %q, which is not one of the available sources (%s)", refVar, strings.Join(refKeys, ", ")) return } if refSourceMapping.Chart != "" { - log.Error("sorry, we do not support referencing a Helm chart yet") + log.Error("source has a 'chart' field defined, but Helm charts are not yet not supported for 'ref' sources") ch.errCh <- err return } - gitClient, targetRevision, err := s.newClientResolveRevision(&refSourceMapping.Repo, refSourceMapping.TargetRevision) - if err != nil { - ch.errCh <- fmt.Errorf("failed to get git client for repo %s", q.Repo.Repo) - return - } - if _, ok := repoLocks[refSourceMapping.Repo.Repo]; !ok { - closer, err := s.repoLock.Lock(gitClient.Root(), targetRevision, true, func() (goio.Closer, error) { - return s.checkoutRevision(gitClient, targetRevision, s.initConstants.SubmoduleEnabled) + normalizedRepoURL := git.NormalizeGitURL(refSourceMapping.Repo.Repo) + closer, ok := repoRefs[normalizedRepoURL] + if ok { + if closer.revision != refSourceMapping.TargetRevision { + ch.errCh <- fmt.Errorf("cannot reference multiple revisions for the same repository (%s references %q while %s references %q)", refVar, refSourceMapping.TargetRevision, closer.key, closer.revision) + return + } + } else { + gitClient, referencedCommitSHA, err := s.newClientResolveRevision(&refSourceMapping.Repo, refSourceMapping.TargetRevision) + if err != nil { + ch.errCh <- fmt.Errorf("failed to get git client for repo %s", q.Repo.Repo) + return + } + if git.NormalizeGitURL(q.ApplicationSource.RepoURL) == normalizedRepoURL && commitSHA != referencedCommitSHA { + ch.errCh <- fmt.Errorf("cannot reference a different revision of the same repository (%s references %q which resolves to %q while the application references %q which resolves to %q)", refVar, refSourceMapping.TargetRevision, referencedCommitSHA, q.Revision, commitSHA) + return + } + closer, err := s.repoLock.Lock(gitClient.Root(), referencedCommitSHA, true, func() (goio.Closer, error) { + return s.checkoutRevision(gitClient, referencedCommitSHA, s.initConstants.SubmoduleEnabled) }) if err != nil { - log.Errorf("failed to acquire lock for referenced source %s", refSourceMapping.Repo.Repo) + log.Errorf("failed to acquire lock for referenced source %s", normalizedRepoURL) ch.errCh <- err return } - repoLocks[refSourceMapping.Repo.Repo] = closer + repoRefs[normalizedRepoURL] = repoRef{revision: refSourceMapping.TargetRevision, commitSHA: referencedCommitSHA, key: refVar} + defer func(closer goio.Closer) { + err := closer.Close() + if err != nil { + log.Errorf("Failed to release repo lock: %v", err) + } + }(closer) } } } @@ -634,10 +657,6 @@ func (s *Service) runManifestGenAsync(ctx context.Context, repoRoot, commitSHA, } manifestGenResult, err = GenerateManifests(ctx, opContext.appPath, repoRoot, commitSHA, q, false, s.gitCredsStore, s.initConstants.MaxCombinedDirectoryManifestsSize, s.gitRepoPaths, WithCMPTarDoneChannel(ch.tarDoneCh), WithCMPTarExcludedGlobs(s.initConstants.CMPTarExcludedGlobs)) - - for _, closer := range repoLocks { - defer io.Close(closer) - } } if err != nil { // If manifest generation error caching is enabled From 6fb63a6f3ec8c52c164f09c871469909b9199fad Mon Sep 17 00:00:00 2001 From: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> Date: Tue, 6 Dec 2022 10:45:41 -0500 Subject: [PATCH 4/4] move settings fetch outside loop Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> --- util/argo/argo.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/util/argo/argo.go b/util/argo/argo.go index 85444734d7962..fd574b67f1349 100644 --- a/util/argo/argo.go +++ b/util/argo/argo.go @@ -579,6 +579,15 @@ func verifyGenerateManifests( Message: errDestinationMissing, }) } + // If source is Kustomize add build options + kustomizeSettings, err := settingsMgr.GetKustomizeSettings() + if err != nil { + conditions = append(conditions, argoappv1.ApplicationCondition{ + Type: argoappv1.ApplicationConditionInvalidSpecError, + Message: fmt.Sprintf("Error getting Kustomize settings: %v", err), + }) + return conditions // Can't perform the next check without settings. + } for _, source := range sources { repoRes, err := db.GetRepository(ctx, source.RepoURL) @@ -589,15 +598,6 @@ func verifyGenerateManifests( }) continue } - // If source is Kustomize add build options - kustomizeSettings, err := settingsMgr.GetKustomizeSettings() - if err != nil { - conditions = append(conditions, argoappv1.ApplicationCondition{ - Type: argoappv1.ApplicationConditionInvalidSpecError, - Message: fmt.Sprintf("Error getting Kustomize settings: %v", err), - }) - continue - } kustomizeOptions, err := kustomizeSettings.GetOptions(source) if err != nil { conditions = append(conditions, argoappv1.ApplicationCondition{