chore(cmp): refactor and test plugin discovery #20217

crenshaw-dev · 2024-10-03T17:23:40Z

Plugin discovery logic is complicated and has resulted in difficult to diagnose bugs.

This PR makes a few changes:

Deprecate RepositoryResponse.IsDiscoveryEnabled field. No code actually uses that field anymore since we do a pre-flight CMP request to determine whether it has discovery enabled.
Break cmpSupports into namedCMPSupports and unnamedCMPSupports. Reading cmpSupports was difficult, because the namedPlugin parameter implied more than the name suggested. Specifically, when namedPlugin == false, we could assume that the app spec did not contain a plugin.name. By using two different functions with two different names, we make it very clear that one is for use when the plugin name is specified, and the other is used when the plugin name is not specified.
Make the CMP client constructor mock-able. It requires some code overhead, but discovery is so deeply entangled with CMP server API calls that I think it's worth it.
Test the behavior of both unnamedCMPSupports and namedCMPSupports. I'd love to test all the way up to DetectConfigManagementPlugin, but I think that would require more mock logic than I'm comfortable with.
Use securejoin instead of our internal path traversal detection logic - it's more robust.

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

bunnyshell · 2024-10-03T17:23:44Z

🔴 Preview Environment stopped on Bunnyshell

See: Environment Details | Pipeline Logs

Available commands (reply to this comment):

🔵 /bns:start to start the environment
🚀 /bns:deploy to redeploy the environment
❌ /bns:delete to remove the environment

bunnyshell · 2024-10-03T17:23:45Z

✅ Preview Environment created on Bunnyshell but will not be auto-deployed

See: Environment Details

Available commands (reply to this comment):

🚀 /bns:deploy to deploy the environment

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

codecov · 2024-10-03T18:50:30Z

Codecov Report

Attention: Patch coverage is 79.48718% with 16 lines in your changes missing coverage. Please review.

Please upload report for BASE (master@b2091e3). Learn more about missing BASE report.
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
util/app/discovery/discovery.go	76.47%	11 Missing and 5 partials ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##             master   #20217   +/-   ##
=========================================
  Coverage          ?   55.98%           
=========================================
  Files             ?      322           
  Lines             ?    44569           
  Branches          ?        0           
=========================================
  Hits              ?    24951           
  Misses            ?    17047           
  Partials          ?     2571

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

crenshaw-dev · 2024-10-03T19:11:41Z

@CodiumAI-Agent /review

CodiumAI-Agent · 2024-10-03T19:12:26Z

PR Reviewer Guide 🔍

(Review updated until commit `cf553e6`)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review Refactoring The refactoring of the `DetectConfigManagementPlugin` function to improve modularity and testability is significant. Ensure that the new structure does not introduce any regressions or misunderstandings about how plugins are detected and managed. Logic Change The changes in how discovery is handled (removing `isDiscoveryEnabled` and using `isSupported` only) could affect existing functionality. Verify that these changes align with the intended use cases and do not negatively impact existing workflows.

util/app/discovery/discovery.go

leoluz · 2024-10-04T01:55:10Z

util/app/discovery/discovery.go

-		// check if the given plugin supports the repo
-		conn, cmpClient, connFound = cmpSupports(ctx, pluginSockFilePath, appPath, repoPath, fmt.Sprintf("%v.sock", pluginName), env, tarExcludedGlobs, true)
-		if !connFound {
+		c := newSocketCMPClientConstructorForPluginName(pluginSockFilePath, pluginName)


Not sure if I am understanding it correctly but this PR introduces the new CMPClientConstructor interface however it seems that it is impossible to use a different implementation than the socketCMPClientConstructor. Not sure if this was intentional, but it would be great if we could enable the flexibility to use different CMPClientConstructor implementations. This could help simplifying the CMP as a Service PR.

it seems that it is impossible to use a different implementation

The only other implementation is the mock implementation used for testing.

The other option would have been to have (un)namedCMPSupports accept a getClient function, but the code just looked ickier. I could go either way.

util/app/discovery/discovery.go

pradithya

I wonder if the e2e test include plugin by default?

pradithya · 2024-10-04T02:17:01Z

cmpserver/plugin/plugin.go

@@ -297,11 +297,11 @@ func (s *Service) matchRepositoryGeneric(stream MatchRepositoryStream) error {
 		return fmt.Errorf("match repository error receiving stream: %w", err)
 	}

-	isSupported, isDiscoveryEnabled, err := s.matchRepository(bufferedCtx, workDir, metadata.GetEnv(), metadata.GetAppRelPath())


This is nice! I felt isDiscoveryEnabled and cfg.IsDiscoveryConfigured were redundant upon reading these code the first time.

pradithya · 2024-10-04T02:39:15Z

util/app/discovery/discovery.go

+		// If discovery isn't configured, assume the CMP supports the plugin since it was explicitly named.
+		return cmpClient, closer
+	}
+	usePlugin := cmpSupportsForClient(ctx, cmpClient, appPath, repoPath, env, tarExcludedGlobs)


For named plugin, do you think we should still invoke matchRepositoryCMP? For efficiency, should we just invoke matchRepositoryCMP for unnamed plugin that has discovery functionality configured?

We have to. Even if a plugin is named, if it failed any configured discovery rule, we'll refuse to use it. We did this when introducing plugin.name because some people may have used discovery rules as a security mechanism, and we wouldn't want to allow users to bypass that check.

pradithya · 2024-10-04T02:46:36Z

util/app/discovery/discovery.go

-	matchRepoStream, err := client.MatchRepository(ctx, grpc_retry.Disable())
+// namedCMPSupports will return a client for the named plugin if it supports the given repository. It will also return
+// a closer for the connection.
+func namedCMPSupports(ctx context.Context, c CMPClientConstructor, appPath, repoPath string, env, tarExcludedGlobs []string) (pluginclient.ConfigManagementPluginServiceClient, io.Closer) {


nitpick on the consistency of return types ordering, DetectConfigManagementPlugin return io.Closer, pluginclient.ConfigManagementPluginServiceClient but the newly introduced functions (namedCMPSupports , unnamedCMPSupports, and getClientForFilepath) return pluginclient.ConfigManagementPluginServiceClient, io.Closer .

Updated DetectConfigManagementPlugin. I think client, closer is the more common convention in the code base.

pradithya · 2024-10-04T02:47:51Z

util/app/discovery/discovery.go


-	isSupported, isDiscoveryEnabled, err := matchRepositoryCMP(ctx, appPath, repoPath, cmpClient, env, tarExcludedGlobs)
+// matchRepositoryCMP will send the repoPath to the cmp-server. The cmp-server will


I think this docs is for matchRepositoryCMP function

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

pradithya · 2024-10-04T23:36:48Z

LGTM

CodiumAI-Agent · 2024-10-04T23:37:07Z

Persistent review updated to latest commit cf553e6

andrii-korotkov-verkada · 2024-11-09T04:30:27Z

cmpserver/plugin/plugin.proto

+    // Deprecated: Use the IsDiscoveryConfigured field from the CheckPluginConfigurationResponse instead. Argo CD no
+    //             longer uses an expensive MatchRepository service to determine if discovery is enabled. Instead, we
+    //             make a relatively cheap pre-flight request to the CheckPluginConfiguration service. We'll leave this
+    //             field here to avoid errors on mismatched repo-server and cmp-server versions.
    bool isDiscoveryEnabled = 2;


Do we use a strict parsing mode for protobuf where unrecognized fields would trigger error?

crenshaw-dev added 4 commits October 3, 2024 10:46

chore(cmp): clean up and test discovery logic

5fd79e1

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

refactor

2f197e2

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

better comment

ecfe24f

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

sort imports

932beb7

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

crenshaw-dev added 2 commits October 3, 2024 13:24

remove needless whitespace

7911b2f

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

Merge remote-tracking branch 'origin/master' into test-cmp-discovery

60a5b40

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

crenshaw-dev marked this pull request as ready for review October 3, 2024 19:11

crenshaw-dev requested a review from a team as a code owner October 3, 2024 19:11

leoluz requested changes Oct 4, 2024

View reviewed changes

pradithya reviewed Oct 4, 2024

View reviewed changes

fixes from comments

cf553e6

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

andrii-korotkov-verkada reviewed Nov 9, 2024

View reviewed changes

AJChandler mentioned this pull request Nov 20, 2024

fix: Memory leak in repo-server #20876

Merged

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(cmp): refactor and test plugin discovery #20217

chore(cmp): refactor and test plugin discovery #20217

crenshaw-dev commented Oct 3, 2024 •

edited

Loading

bunnyshell bot commented Oct 3, 2024 •

edited

Loading

bunnyshell bot commented Oct 3, 2024

codecov bot commented Oct 3, 2024

crenshaw-dev commented Oct 3, 2024

CodiumAI-Agent commented Oct 3, 2024 •

edited

Loading

leoluz Oct 4, 2024

crenshaw-dev Oct 4, 2024

pradithya left a comment

pradithya Oct 4, 2024

pradithya Oct 4, 2024

crenshaw-dev Oct 4, 2024

pradithya Oct 4, 2024

pradithya Oct 4, 2024

crenshaw-dev Oct 4, 2024

pradithya Oct 4, 2024

pradithya commented Oct 4, 2024

CodiumAI-Agent commented Oct 4, 2024

andrii-korotkov-verkada Nov 9, 2024


		isSupported, isDiscoveryEnabled, err := matchRepositoryCMP(ctx, appPath, repoPath, cmpClient, env, tarExcludedGlobs)
		// matchRepositoryCMP will send the repoPath to the cmp-server. The cmp-server will

chore(cmp): refactor and test plugin discovery #20217

Are you sure you want to change the base?

chore(cmp): refactor and test plugin discovery #20217

Conversation

crenshaw-dev commented Oct 3, 2024 • edited Loading

bunnyshell bot commented Oct 3, 2024 • edited Loading

🔴 Preview Environment stopped on Bunnyshell

bunnyshell bot commented Oct 3, 2024

✅ Preview Environment created on Bunnyshell but will not be auto-deployed

codecov bot commented Oct 3, 2024

Codecov Report

crenshaw-dev commented Oct 3, 2024

CodiumAI-Agent commented Oct 3, 2024 • edited Loading

PR Reviewer Guide 🔍

(Review updated until commit cf553e6)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pradithya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pradithya commented Oct 4, 2024

CodiumAI-Agent commented Oct 4, 2024

Choose a reason for hiding this comment

crenshaw-dev commented Oct 3, 2024 •

edited

Loading

bunnyshell bot commented Oct 3, 2024 •

edited

Loading

CodiumAI-Agent commented Oct 3, 2024 •

edited

Loading

(Review updated until commit `cf553e6`)