feat: make discovery field optional in plugins

[gdsoumya]

Signed-off-by: Soumya Ghosh Dastidar gdsoumya@gmail.com

Fixes #12027

This PR makes discovery field in cmpv2 plugin spec optional and removes discovery check for plugins when plugin name is explicitly specified in the app.

Note on DCO:

If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.

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).
• 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.
• Optional. My organization is added to USERS.md.
• 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).

[codecov]

Codecov Report

Base: 47.43% // Head: 47.42% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (e7926ed) compared to base (06b98c5).
Patch coverage: 53.12% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #12073      +/-   ##
==========================================
- Coverage   47.43%   47.42%   -0.01%     
==========================================
  Files         246      246              
  Lines       41826    41823       -3     
==========================================
- Hits        19840    19836       -4     
- Misses      19990    19992       +2     
+ Partials     1996     1995       -1     

Impacted Files	Coverage Δ
cmpserver/plugin/config.go	34.37% <ø> (+2.94%)	⬆️
util/app/discovery/discovery.go	36.09% <0.00%> (-1.12%)	⬇️
cmpserver/plugin/plugin.go	78.51% <100.00%> (-0.34%)	⬇️
util/settings/settings.go	49.20% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[crenshaw-dev]

I think this PR goes just a git too far. I think we should make the discovery field optional. But when it's present, I think it should have to match in addition to the name field matching.

This will prevent 2.6 from introducing a new way for users to bypass discovery rules. This is important because Argo CD operators might be using discovery rules to enforce important patterns.

[gdsoumya]

I think this PR goes just a git too far. I think we should make the discovery field optional. But when it's present, I think it should have to match in addition to the name field matching.

This will prevent 2.6 from introducing a new way for users to bypass discovery rules. This is important because Argo CD operators might be using discovery rules to enforce important patterns.

There's no way to know if the discovery field is set or not so we just call the matchRepo rpc and try to check if the repo matches but if discovery is not specified it will automatically return false or no match for the request. I think that's probably fine because we do not break any existing implementations/patterns as previously discovery was compulsory so it's always there and users would probably not be putting plugin names in the app spec that won't work as the ui would have blocked it in the old version. WDYT?

[gdsoumya]

We can maybe update the matchRepo rpc to return one more bool to specify if the discovery was specified or not and always make the matchRepo call and on the response, if discovery is false we continue with the specified plugin.

[crenshaw-dev]

we do not break any existing implementations/patterns as previously discovery was compulsory so it's always there

I'm worried specifically about the cases where discovery is there, and the Argo CD operator expects it to continue to be enforced.

if discovery is not specified it will automatically return false or no match for the request

I think instead of defaulting to false, we can default to true. If no discovery rules are set, matchRepository returns true.

[gdsoumya]

we do not break any existing implementations/patterns as previously discovery was compulsory so it's always there

I'm worried specifically about the cases where discovery is there, and the Argo CD operator expects it to continue to be enforced.

if discovery is not specified it will automatically return false or no match for the request

I think instead of defaulting to false, we can default to true. If no discovery rules are set, matchRepository returns true.

I see so the default behavior can be if discovery is not specified it will match all repos. That sounds good. Will make the change and update the docs to reflect the same.

[gdsoumya]

I see so the default behavior can be if discovery is not specified it will match all repos. That sounds good. Will make the change and update the docs to reflect the same.

On second thought @crenshaw-dev do you think this would cause issues with auto-discovery? The reason I set it to not match by default was that I didn't want it to randomly match any repo that uses auto-discovery. I think that's more of a breaking case because older config using auto-discovery might see modified behavior after we enable this.

[crenshaw-dev]

The reason I set it to not match by default was that I didn't want it to randomly match any repo that uses auto-discovery. I think that's more of a breaking case because older config using auto-discovery might see modified behavior after we enable this.

We would default to matching if and only if discovery is not set. As you mentioned earlier, every user of sidecar CMPs currently has discovery set, so it shouldn't be a breaking change. If discovery is set, we'll respect the discovery rules.

[gdsoumya]

We would default to matching if and only if discovery is not set. As you mentioned earlier, every user of sidecar CMPs currently has discovery set, so it shouldn't be a breaking change. If discovery is set, we'll respect the discovery rules.

Yes but would that not break existing apps? for eg. if a user now creates a plugin with no discovery rules and because of how the plugin is named it get's more priority in the auto-discovery list then older apps that depended on auto-discovery would start matching with this plugin and because the plugin will always match with the repo the generated resources for those apps could be altered ?

This is how we get the list of plugins in auto-discovery mode :

fileList, err := os.ReadDir(pluginSockFilePath)
if err != nil {
      return nil, nil, fmt.Errorf("Failed to list all plugins in dir, error=%w", err)
}

And the ordering of this is influenced by the name of the plugin, so a new plugin with higher precedence here would alter older apps using auto discovery.

[crenshaw-dev]

I understand the issue now. Good catch.

I think to satisfy all use cases, we need to modify the rpc so we can differentiate between "doesn't match because discovery" and "doesn't match because there is no discovery".

[crenshaw-dev]

Thanks, @gdsoumya!

[crenshaw-dev]

Cherry-picked onto release-2.6 for 2.6.0-rc8 or 2.6.0 GA, whichever comes first. :-)