Discover command for side car plugins should have access to application instance variables

[pierrecregut]

Summary

The discover.find.command command does not have access to the plugin variables defined in the Application manifest.

Motivation

we have several plugins used in different applications.

When plugins were configured through a configMap, the first step to use the right plugin in an application was the name of the plugin that was matched. With the right convention, we may use regular plugin env variables to replace this behaviour and this is what we do in the code of the generate command. On the other hand, there is strictly nothing in the git repository structure that tells we should use plugin A (eg argocd-vault plugin) rather than plugin B (eg something that rewrite/enhance some manifests), so we end up having an empty discover command that accept everything and we defer the real selection work to the generate command. The generate command must emulate the standard behaviour of argocd when there is no plugin (note: sidecar discovery is always called even if there is no plugin section). It is also difficult with the current implementation to have several sidecar plugins.

We end up with a single sidecar plugin that contains a lot of the logic that was implemented by argocd-repo-server. The only gain is that we are not tied to a given argocd release but the implementation is rather monolithic and we reimplement the handling of kustomize/helm applications.

Proposal

The implementation does not change the API, but the set of environment variables available to the discover.find.command command is augmented with the variables defined in the plugin configuration section of the application.

How do you think this should be implemented?

In repository.go runConfigurationManagementPluginSidecars, env should be computed before running DetectConfigManagementPlugin and env should be part of the arguments which means a change to the cmpServer/plugin/plugin.proto. Then the code used in generateManifest to compute env should work in matchRepository too.

[crenshaw-dev]

@pierrecregut I agree, that command should get the user-configured env vars. Would you have time to open the PR?

I think that we should prefix all the env vars with ENV_ or something similar, to avoid letting the user override important env vars. This change is planned anyway, and I'd rather avoid allowing folks to introduce new code which depends on the old behavior.

To avoid confusion, I think the other commands should receive the un-prefixed and prefixed env vars so we can properly deprecate the un-prefixed behavior and give folks time to transition.

[pierrecregut]

@crenshaw-dev I have opened a pull request. The code is slightly simpler than expected. I have implemented the ENV_ prefixing keeping a legacy behavior for init/generate. The change in the documentation could probably be worded better but I am not a native english speaker. I check that the env can be used on the server but tests should really focus on the client side. But it does not seem easy to mock a server and the unit test available are on the behavior with local plugins not with a sidecar.

As a side note while developing the fix on the master branch, I noticed that the ui was empty when I used --rootpath in the argocd-server deployment to relocate the path. As it worked well with 2.3 and earlier, it looks like a bug unless I missed an important change in the unreleased version.