-
Notifications
You must be signed in to change notification settings - Fork 45
Feat/oci helm support #500
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
d95e6ca
f7c0c18
d27d19a
cd25e1a
60e3a99
4be635f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -100,6 +100,151 @@ func preprocessSources(app *v1alpha1.Application, pullRequest vcs.PullRequest) ( | |||||||||||||
| return contentSources, refSources | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // isExternalHelmChart returns true when a source refers to a Helm chart that is | ||||||||||||||
| // fetched directly from a Helm registry (OCI or HTTPS), not from a Git repository. | ||||||||||||||
| // Such sources cannot be git-cloned; ArgoCD's repo-server is responsible for | ||||||||||||||
| // pulling the chart using its own credentials and OCI support. | ||||||||||||||
| func isExternalHelmChart(source v1alpha1.ApplicationSource) bool { | ||||||||||||||
| // source.Chart is only set for Helm chart sources (not git-path sources) | ||||||||||||||
| if source.Chart == "" { | ||||||||||||||
| return false | ||||||||||||||
| } | ||||||||||||||
| // OCI registries with explicit oci:// scheme | ||||||||||||||
| if strings.HasPrefix(source.RepoURL, "oci://") { | ||||||||||||||
| return true | ||||||||||||||
| } | ||||||||||||||
| // HTTPS Helm repos: no .git suffix and no source path means it is a plain | ||||||||||||||
| // Helm repository URL (e.g. https://charts.example.io) | ||||||||||||||
| if strings.HasPrefix(source.RepoURL, "https://") && !strings.HasSuffix(source.RepoURL, ".git") && source.Path == "" { | ||||||||||||||
| return true | ||||||||||||||
| } | ||||||||||||||
| // OCI registry shorthand without scheme (e.g. "docker.io/envoyproxy", | ||||||||||||||
| // "ghcr.io/org/charts"). ArgoCD accepts these for OCI Helm sources. | ||||||||||||||
| // If Chart is set and the URL has no scheme and doesn't end in .git, | ||||||||||||||
| // it's an OCI/Helm registry reference, not a git repo. | ||||||||||||||
| if source.Chart != "" && !strings.Contains(source.RepoURL, "://") && !strings.HasSuffix(source.RepoURL, ".git") { | ||||||||||||||
| return true | ||||||||||||||
| } | ||||||||||||||
| return false | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // generateManifestForExternalChart handles manifest generation for external Helm chart | ||||||||||||||
| // sources (OCI registries and HTTPS Helm repos). Instead of streaming local files via | ||||||||||||||
| // GenerateManifestWithFiles, it calls GenerateManifest directly — the repo-server will | ||||||||||||||
| // fetch the chart from the registry using its own credentials and OCI/Helm support. | ||||||||||||||
| // This avoids needing a local copy of the chart and correctly handles chart dependencies. | ||||||||||||||
| func (a *ArgoClient) generateManifestForExternalChart(ctx context.Context, app v1alpha1.Application, source v1alpha1.ApplicationSource, refs []v1alpha1.ApplicationSource) ([]string, error) { | ||||||||||||||
| clusterCloser, clusterClient := a.GetClusterClient() | ||||||||||||||
| defer pkg.WithErrorLogging(clusterCloser.Close, "failed to close connection") | ||||||||||||||
|
|
||||||||||||||
| clusterData, err := clusterClient.Get(ctx, &cluster.ClusterQuery{Name: app.Spec.Destination.Name, Server: app.Spec.Destination.Server}) | ||||||||||||||
| if err != nil { | ||||||||||||||
| getManifestsFailed.WithLabelValues(app.Name).Inc() | ||||||||||||||
| return nil, errors.Wrap(err, "failed to get cluster") | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| settingsCloser, settingsClient := a.GetSettingsClient() | ||||||||||||||
| defer pkg.WithErrorLogging(settingsCloser.Close, "failed to close connection") | ||||||||||||||
|
|
||||||||||||||
| argoSettings, err := settingsClient.Get(ctx, &settings.SettingsQuery{}) | ||||||||||||||
| if err != nil { | ||||||||||||||
| getManifestsFailed.WithLabelValues(app.Name).Inc() | ||||||||||||||
| return nil, errors.Wrap(err, "failed to get settings") | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| settingsMgr := argosettings.NewSettingsManager(ctx, a.k8s, a.cfg.ArgoCDNamespace) | ||||||||||||||
| argoDB := db.NewDB(a.cfg.ArgoCDNamespace, settingsMgr, a.k8s) | ||||||||||||||
|
|
||||||||||||||
| helmRepos, err := argoDB.ListHelmRepositories(ctx) | ||||||||||||||
| if err != nil { | ||||||||||||||
| return nil, fmt.Errorf("error listing helm repositories: %w", err) | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| proj, err := func() (*v1alpha1.AppProject, error) { | ||||||||||||||
| closer, projectClient, err := a.client.NewProjectClient() | ||||||||||||||
| if err != nil { | ||||||||||||||
| return nil, errors.Wrap(err, "failed to get project client") | ||||||||||||||
| } | ||||||||||||||
| defer pkg.WithErrorLogging(closer.Close, "failed to close connection") | ||||||||||||||
| return projectClient.Get(ctx, &project.ProjectQuery{Name: app.Spec.Project}) | ||||||||||||||
| }() | ||||||||||||||
| if err != nil { | ||||||||||||||
| return nil, fmt.Errorf("error getting app project: %w", err) | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| permittedHelmRepos, err := argo.GetPermittedRepos(proj, helmRepos) | ||||||||||||||
| if err != nil { | ||||||||||||||
| return nil, fmt.Errorf("error getting permitted helm repos: %w", err) | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| helmRepositoryCredentials, err := argoDB.GetAllHelmRepositoryCredentials(ctx) | ||||||||||||||
| if err != nil { | ||||||||||||||
| return nil, fmt.Errorf("error getting helm repository credentials: %w", err) | ||||||||||||||
| } | ||||||||||||||
| permittedHelmCredentials, err := argo.GetPermittedReposCredentials(proj, helmRepositoryCredentials) | ||||||||||||||
| if err != nil { | ||||||||||||||
| return nil, fmt.Errorf("error getting permitted repos credentials: %w", err) | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| enabledSourceTypes, err := settingsMgr.GetEnabledSourceTypes() | ||||||||||||||
| if err != nil { | ||||||||||||||
| return nil, fmt.Errorf("error getting settings enabled source types: %w", err) | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| helmOptions, err := settingsMgr.GetHelmSettings() | ||||||||||||||
| if err != nil { | ||||||||||||||
| return nil, fmt.Errorf("error getting helm settings: %w", err) | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| refSources, err := argo.GetRefSources(context.Background(), app.Spec.Sources, app.Spec.Project, argoDB.GetRepository, []string{}) | ||||||||||||||
| if err != nil { | ||||||||||||||
| return nil, fmt.Errorf("failed to get ref sources: %w", err) | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // Resolve the repository object so repo-server has credentials for the chart registry. | ||||||||||||||
| repo, err := argoDB.GetRepository(ctx, source.RepoURL, app.Spec.Project) | ||||||||||||||
| if err != nil { | ||||||||||||||
| log.Warn().Err(err).Str("repoURL", source.RepoURL).Msg("could not resolve repository credentials; proceeding without them") | ||||||||||||||
| repo = &v1alpha1.Repository{Repo: source.RepoURL} | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| q := repoapiclient.ManifestRequest{ | ||||||||||||||
| Repo: repo, | ||||||||||||||
| Revision: source.TargetRevision, | ||||||||||||||
| AppLabelKey: argoSettings.AppLabelKey, | ||||||||||||||
| AppName: app.Name, | ||||||||||||||
| Namespace: app.Spec.Destination.Namespace, | ||||||||||||||
| ApplicationSource: &source, | ||||||||||||||
| Repos: permittedHelmRepos, | ||||||||||||||
| KustomizeOptions: argoSettings.KustomizeOptions, | ||||||||||||||
| KubeVersion: clusterData.Info.ServerVersion, | ||||||||||||||
| ApiVersions: clusterData.Info.APIVersions, | ||||||||||||||
| HelmRepoCreds: permittedHelmCredentials, | ||||||||||||||
| HelmOptions: helmOptions, | ||||||||||||||
| TrackingMethod: argoSettings.TrackingMethod, | ||||||||||||||
| EnabledSourceTypes: enabledSourceTypes, | ||||||||||||||
| ProjectName: proj.Name, | ||||||||||||||
| ProjectSourceRepos: proj.Spec.SourceRepos, | ||||||||||||||
| HasMultipleSources: app.Spec.HasMultipleSources(), | ||||||||||||||
| RefSources: refSources, | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| repoClient, conn, err := a.createRepoServerClient() | ||||||||||||||
| if err != nil { | ||||||||||||||
| return nil, errors.Wrap(err, "error creating repo client") | ||||||||||||||
| } | ||||||||||||||
| defer pkg.WithErrorLogging(conn.Close, "failed to close connection") | ||||||||||||||
|
|
||||||||||||||
| log.Debug().Caller().Str("app", app.Name).Msg("calling GenerateManifest for external Helm chart") | ||||||||||||||
| resp, err := repoClient.GenerateManifest(ctx, &q) | ||||||||||||||
| if err != nil { | ||||||||||||||
| getManifestsFailed.WithLabelValues(app.Name).Inc() | ||||||||||||||
| return nil, fmt.Errorf("failed to generate manifests for external Helm chart %s: %w", app.Name, err) | ||||||||||||||
| } | ||||||||||||||
| getManifestsSuccess.WithLabelValues(app.Name).Inc() | ||||||||||||||
|
Comment on lines
+241
to
+244
|
||||||||||||||
| getManifestsFailed.WithLabelValues(app.Name).Inc() | |
| return nil, fmt.Errorf("failed to generate manifests for external Helm chart %s: %w", app.Name, err) | |
| } | |
| getManifestsSuccess.WithLabelValues(app.Name).Inc() | |
| return nil, fmt.Errorf("failed to generate manifests for external Helm chart %s: %w", app.Name, err) | |
| } |
Copilot
AI
Mar 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR description says external Helm chart sources should be skipped only under certain ArgoCDSendFullRepository settings (warn+return nil when false), but the code now always delegates external charts to repo-server GenerateManifest regardless of that config. Either update the PR description to match current behavior or implement the described conditional handling so runtime behavior is unambiguous.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In generateManifestForExternalChart, ref sources are resolved from app.Spec.Sources using context.Background(), which ignores the
refsslice returned by preprocessSources (including PR base->head targetRevision rewrites) and also ignores cancellation/deadlines. This can cause external Helm chart manifest generation to use the wrong ref revisions during PR previews. Usectxand build the sources list consistently (e.g., prependsourceand append the passed-inrefs) when calling argo.GetRefSources / populating RefSources.