chore: transmit only app manifests to the cmp-server for plugin-based applications

[jsolana]

Closes #17951

Transmit only the necessary information to the cmp-server to improve the performance. More info here

As a quick win, transmit only appPath manifests for plugin-based applications.

Checklist:

• Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
• The title of the PR states what changed and the related issues number (used for the release note).
• The title of the PR conforms to the Toolchain Guide
• I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
• I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
• Does this PR require documentation updates?
• I've updated documentation as required by this PR.
• I have signed off all my commits as required by DCO
• I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
• My build is green (troubleshooting builds).
• My new feature complies with the feature status guidelines.
• I have added a brief description of why this PR is necessary and/or what this PR solves.
• Optional. My organization is added to USERS.md.
• Optional. For bug fixes, I've indicated what older releases this fix should be cherry-picked into (this may or may not happen depending on risk/complexity).

[todaywasawesome]

The performance implications of this change are AWESOME. However controlling this based on having a plugin name set seems strange. I think it needs to be a flag in the configmap to start.

It would be nice to configure per app but that could be a future change.

[crenshaw-dev]

I think a better implementation would be to respect the manifest-generate-paths annotation.

This implementation would break some apps where manifests reference resources outside the app path.

[jsolana]

If we want to provide this info, we should change RPC contract right?

[codecov]

Codecov Report

Attention: Patch coverage is 81.67939% with 24 lines in your changes missing coverage. Please review.

Project coverage is 50.68%. Comparing base (7af5793) to head (d290d48).
Report is 2 commits behind head on master.

Files	Patch %	Lines
cmd/argocd/commands/app.go	0.00%	12 Missing ⚠️
pkg/apis/application/v1alpha1/types.go	0.00%	5 Missing ⚠️
reposerver/repository/repository.go	0.00%	4 Missing ⚠️
reposerver/repository/util.go	96.15%	1 Missing ⚠️
util/argo/argo.go	95.00%	0 Missing and 1 partial ⚠️
util/cmp/stream.go	83.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #18054      +/-   ##
==========================================
+ Coverage   50.65%   50.68%   +0.02%     
==========================================
  Files         315      316       +1     
  Lines       43344    43382      +38     
==========================================
+ Hits        21958    21986      +28     
- Misses      18880    18889       +9     
- Partials     2506     2507       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[crenshaw-dev]

Possibly... I'm not sure if the repo-server's manifest generation request body currently contains the information from that annotation. If not, we'd have to add it so that the repo-server could evaluate which files to send to the CMP.

[jsolana]

This value is not propagated right now . At first glance we have two options

• we can extend ApplicationSource to contains AnnotationManifetGeneratePaths:

types.go

// ApplicationSource contains all required information about the source of an application
type ApplicationSource struct {
	// RepoURL is the URL to the repository (Git or Helm) that contains the application manifests
	RepoURL string `json:"repoURL" protobuf:"bytes,1,opt,name=repoURL"`
	// Path is a directory path within the Git repository, and is only valid for applications sourced from Git.
	Path string `json:"path,omitempty" protobuf:"bytes,2,opt,name=path"`
	// TargetRevision defines the revision of the source to sync the application to.
	// In case of Git, this can be commit, tag, or branch. If omitted, will equal to HEAD.
	// In case of Helm, this is a semver tag for the Chart's version.
	TargetRevision string `json:"targetRevision,omitempty" protobuf:"bytes,4,opt,name=targetRevision"`
	// Helm holds helm specific options
	Helm *ApplicationSourceHelm `json:"helm,omitempty" protobuf:"bytes,7,opt,name=helm"`
	// Kustomize holds kustomize specific options
	Kustomize *ApplicationSourceKustomize `json:"kustomize,omitempty" protobuf:"bytes,8,opt,name=kustomize"`
	// Directory holds path/directory specific options
	Directory *ApplicationSourceDirectory `json:"directory,omitempty" protobuf:"bytes,10,opt,name=directory"`
	// Plugin holds config management plugin specific options
	Plugin *ApplicationSourcePlugin `json:"plugin,omitempty" protobuf:"bytes,11,opt,name=plugin"`
	// Chart is a Helm chart name, and must be specified for applications sourced from a Helm repo.
	Chart string `json:"chart,omitempty" protobuf:"bytes,12,opt,name=chart"`
	// Ref is reference to another source within sources field. This field will not be used if used with a `source` tag.
	Ref string `json:"ref,omitempty" protobuf:"bytes,13,opt,name=ref"`
	// AnnotationManifestGeneratePaths holds argocd.argoproj.io/manifest-generate-paths annotation value of the Application.
	AnnotationManifestGeneratePaths string `json:"annotationManifestGeneratePaths" protobuf:"bytes,14,opt,name=annotationManifestGeneratePaths"`
}

• Or because this annotation only affects manifests generation maybe make sense to modify ManifestRequest:

repository.proto

// ManifestRequest is a query for manifest generation.
message ManifestRequest {
    github.com.argoproj.argo_cd.v2.pkg.apis.application.v1alpha1.Repository repo = 1;
    // revision, potentially un-resolved
    string revision = 2;
    bool noCache = 3;
    string appLabelKey = 4;
    // Name of the application for which the request is triggered
    string appName = 5;
    string namespace = 8;
    github.com.argoproj.argo_cd.v2.pkg.apis.application.v1alpha1.ApplicationSource applicationSource = 10;
    repeated github.com.argoproj.argo_cd.v2.pkg.apis.application.v1alpha1.Repository repos = 11;
    // Deprecated: use sidecar plugins instead.
    repeated github.com.argoproj.argo_cd.v2.pkg.apis.application.v1alpha1.ConfigManagementPlugin plugins = 12;
    github.com.argoproj.argo_cd.v2.pkg.apis.application.v1alpha1.KustomizeOptions kustomizeOptions = 13;
    string kubeVersion = 14;
    repeated string apiVersions = 15;
    // Request to verify the signature when generating the manifests (only for Git repositories)
    bool verifySignature = 16;
    repeated github.com.argoproj.argo_cd.v2.pkg.apis.application.v1alpha1.RepoCreds helmRepoCreds = 17;
    bool noRevisionCache = 18;
    string trackingMethod = 19;
    map<string, bool> enabledSourceTypes = 20;
    github.com.argoproj.argo_cd.v2.pkg.apis.application.v1alpha1.HelmOptions helmOptions = 21;
    bool hasMultipleSources = 22;
    map<string, github.com.argoproj.argo_cd.v2.pkg.apis.application.v1alpha1.RefTarget> refSources = 23;
    // This is used to surface "source not permitted" errors for Helm repositories
    repeated string projectSourceRepos = 24;
    // This is used to surface "source not permitted" errors for Helm repositories
    string projectName = 25;
    // argocd.argoproj.io/manifest-generate-paths annotation value of the Application to allow optimize which resources propagated to cmpserver
    string AnnotationManifestGeneratePaths = 26;
}

IMHO I prefer the second one.

Wdyt @crenshaw-dev ?

[crenshaw-dev]

I think adding it to just the repository.proto makes sense.

[jsolana]

Thanks mate! Im going with it! 💪

[jsolana]

Hi @todaywasawesome , Just to confirm if it is the correct approach!
Let me know :)
Thanks a lot for you help!

[jsolana]

Hi @todaywasawesome ! Let me know if there is something I can do to push for this change
Thanks

[todaywasawesome]

I added a few questions. I want to get an extra eye on this because it touches some important areas.

[crenshaw-dev]

@jsolana can you fix DCO?

[jsolana]

Ey thanks @crenshaw-dev , Im little newbie :(

Is this commit the problematic? link

is there anyway to fix it easily? (or maybe is better to move all the changes in a new PR?)

I was thinking on

git rebase --signoff HEAD^^^^^^^^^^^^ # cause are 12 commits
git push -f origin js/transfer-app-manifests-for-plugin

Thanks a lot for you help!

[todaywasawesome]

@jsolana If you click "details" on the DCO check it will tell you how to fix it.

To add your Signed-off-by line to every commit in this branch:

Ensure you have a local copy of your branch by checking out the pull request locally via command line.
In your local branch, run: git rebase HEAD~12 --signoff
Force push your changes to overwrite the branch: git push --force-with-lease origin js/transfer-app-manifests-for-plugin

[jsolana]

oh 💩 , I ve broken the PR following these instructions :(

I think is better to create a new PR with the changes and close this one ok?

[jsolana]

Hi @todaywasawesome! I guess the problem was related to the branch (too old and with merges in the middle). Running these command cracks the PR (not sure why, maybe cause the merges).

I ve just created #19209 with the changes and with the DCO fixed.
Lets move the conversation there :)
Thanks guys and sorry for the noise