diff --git a/reposerver/repository/repository.go b/reposerver/repository/repository.go index b4cd9187c7036..bade700b8da76 100644 --- a/reposerver/repository/repository.go +++ b/reposerver/repository/repository.go @@ -2162,7 +2162,9 @@ func generateManifestsCMP(ctx context.Context, appPath, rootPath string, env []s cmp.WithTarDoneChan(tarDoneCh), } - err = cmp.SendRepoStream(generateManifestStream.Context(), appPath, rootPath, generateManifestStream, env, tarExcludedGlobs, opts...) + // Use the request context (ctx) rather than stream.Context() to avoid prematurely sending into a stream whose + // context may have been canceled by the gRPC internals or server-side handling. + err = cmp.SendRepoStream(ctx, appPath, rootPath, generateManifestStream, env, tarExcludedGlobs, opts...) if err != nil { return nil, fmt.Errorf("error sending file to cmp-server: %w", err) } @@ -2384,7 +2386,7 @@ func populatePluginAppDetails(ctx context.Context, res *apiclient.RepoAppDetails return fmt.Errorf("error getting parametersAnnouncementStream: %w", err) } - err = cmp.SendRepoStream(parametersAnnouncementStream.Context(), appPath, repoPath, parametersAnnouncementStream, env, tarExcludedGlobs) + err = cmp.SendRepoStream(ctx, appPath, repoPath, parametersAnnouncementStream, env, tarExcludedGlobs) if err != nil { return fmt.Errorf("error sending file to cmp-server: %w", err) } diff --git a/util/app/discovery/discovery.go b/util/app/discovery/discovery.go index 87952c6873dae..4240bb6b4a7de 100644 --- a/util/app/discovery/discovery.go +++ b/util/app/discovery/discovery.go @@ -92,6 +92,7 @@ func DetectConfigManagementPlugin(ctx context.Context, appPath, repoPath, plugin var conn utilio.Closer var cmpClient pluginclient.ConfigManagementPluginServiceClient var connFound bool + var lastErr error pluginSockFilePath := common.GetPluginSockFilePath() log.WithFields(log.Fields{ @@ -101,8 +102,11 @@ func DetectConfigManagementPlugin(ctx context.Context, appPath, repoPath, plugin if pluginName != "" { // check if the given plugin supports the repo - conn, cmpClient, connFound = cmpSupports(ctx, pluginSockFilePath, appPath, repoPath, fmt.Sprintf("%v.sock", pluginName), env, tarExcludedGlobs, true) + conn, cmpClient, connFound, lastErr = cmpSupports(ctx, pluginSockFilePath, appPath, repoPath, fmt.Sprintf("%v.sock", pluginName), env, tarExcludedGlobs, true) if !connFound { + if lastErr != nil { + return nil, nil, fmt.Errorf("could not find cmp-server plugin with name %q supporting the given repository: %w", pluginName, lastErr) + } return nil, nil, fmt.Errorf("could not find cmp-server plugin with name %q supporting the given repository", pluginName) } } else { @@ -112,13 +116,16 @@ func DetectConfigManagementPlugin(ctx context.Context, appPath, repoPath, plugin } for _, file := range fileList { if file.Type() == os.ModeSocket { - conn, cmpClient, connFound = cmpSupports(ctx, pluginSockFilePath, appPath, repoPath, file.Name(), env, tarExcludedGlobs, false) + conn, cmpClient, connFound, lastErr = cmpSupports(ctx, pluginSockFilePath, appPath, repoPath, file.Name(), env, tarExcludedGlobs, false) if connFound { break } } } if !connFound { + if lastErr != nil { + return nil, nil, fmt.Errorf("could not find plugin supporting the given repository: %w", lastErr) + } return nil, nil, errors.New("could not find plugin supporting the given repository") } } @@ -145,16 +152,20 @@ func matchRepositoryCMP(ctx context.Context, appPath, repoPath string, client pl return resp.GetIsSupported(), resp.GetIsDiscoveryEnabled(), nil } -func cmpSupports(ctx context.Context, pluginSockFilePath, appPath, repoPath, fileName string, env []string, tarExcludedGlobs []string, namedPlugin bool) (utilio.Closer, pluginclient.ConfigManagementPluginServiceClient, bool) { +// cmpSupports checks if the given plugin socket supports the repo. If namedPlugin is true +// a plugin that is not discovery-configured will be considered a match. +// Returns a utilio.Closer, the client, a bool indicating a match, and an error describing +// any lower-level failure that occurred while probing the plugin. +func cmpSupports(ctx context.Context, pluginSockFilePath, appPath, repoPath, fileName string, env []string, tarExcludedGlobs []string, namedPlugin bool) (utilio.Closer, pluginclient.ConfigManagementPluginServiceClient, bool, error) { absPluginSockFilePath, err := filepath.Abs(pluginSockFilePath) if err != nil { log.Errorf("error getting absolute path for plugin socket dir %v, %v", pluginSockFilePath, err) - return nil, nil, false + return nil, nil, false, fmt.Errorf("error getting absolute path for plugin socket dir %q: %w", pluginSockFilePath, err) } address := filepath.Join(absPluginSockFilePath, fileName) if !files.Inbound(address, absPluginSockFilePath) { log.Errorf("invalid socket file path, %v is outside plugin socket dir %v", fileName, pluginSockFilePath) - return nil, nil, false + return nil, nil, false, fmt.Errorf("invalid socket file path, %q is outside plugin socket dir %q", fileName, pluginSockFilePath) } cmpclientset := pluginclient.NewConfigManagementPluginClientSet(address) @@ -165,23 +176,23 @@ func cmpSupports(ctx context.Context, pluginSockFilePath, appPath, repoPath, fil common.SecurityField: common.SecurityMedium, common.SecurityCWEField: common.SecurityCWEMissingReleaseOfFileDescriptor, }).Errorf("error dialing to cmp-server for plugin %s, %v", fileName, err) - return nil, nil, false + return nil, nil, false, fmt.Errorf("error dialing to cmp-server for plugin %s: %w", fileName, err) } cfg, err := cmpClient.CheckPluginConfiguration(ctx, &empty.Empty{}) if err != nil { log.Errorf("error checking plugin configuration %s, %v", fileName, err) utilio.Close(conn) - return nil, nil, false + return nil, nil, false, fmt.Errorf("error checking plugin configuration %s: %w", fileName, err) } if !cfg.IsDiscoveryConfigured { // If discovery isn't configured but the plugin is named, then the plugin supports the repo. if namedPlugin { - return conn, cmpClient, true + return conn, cmpClient, true, nil } utilio.Close(conn) - return nil, nil, false + return nil, nil, false, nil } isSupported, isDiscoveryEnabled, err := matchRepositoryCMP(ctx, appPath, repoPath, cmpClient, env, tarExcludedGlobs) @@ -191,20 +202,20 @@ func cmpSupports(ctx context.Context, pluginSockFilePath, appPath, repoPath, fil common.SecurityCWEField: common.SecurityCWEMissingReleaseOfFileDescriptor, }).Errorf("repository %s is not the match because %v", repoPath, err) utilio.Close(conn) - return nil, nil, false + return nil, nil, false, fmt.Errorf("error checking repository support for plugin %s: %w", fileName, err) } if !isSupported { // if discovery is not set and the plugin name is specified, let app use the plugin if !isDiscoveryEnabled && namedPlugin { - return conn, cmpClient, true + return conn, cmpClient, true, nil } log.WithFields(log.Fields{ common.SecurityField: common.SecurityLow, common.SecurityCWEField: common.SecurityCWEMissingReleaseOfFileDescriptor, }).Debugf("Response from socket file %s does not support %v", fileName, repoPath) utilio.Close(conn) - return nil, nil, false + return nil, nil, false, nil } - return conn, cmpClient, true + return conn, cmpClient, true, nil } diff --git a/util/app/discovery/discovery_test.go b/util/app/discovery/discovery_test.go index 82d0b4a737bbb..31fff615bf0b9 100644 --- a/util/app/discovery/discovery_test.go +++ b/util/app/discovery/discovery_test.go @@ -1,6 +1,7 @@ package discovery import ( + "context" "testing" "github.com/argoproj/argo-cd/v3/pkg/apis/application/v1alpha1" @@ -49,3 +50,35 @@ func TestAppType_Disabled(t *testing.T) { require.NoError(t, err) assert.Equal(t, "Directory", appType) } + +func Test_cmpSupports_invalidSocketPath_outsideDir(t *testing.T) { + // Use a temp dir as the base plugin socket dir and provide a fileName that + // resolves outside it to trigger the Inbound check. + pluginSockFilePath := t.TempDir() + // fileName with a path traversal that causes the address to be outside the plugin socket dir + fileName := "../outside.sock" + + conn, client, found, err := cmpSupports(context.Background(), pluginSockFilePath, "appPath", "repoPath", fileName, nil, nil, true) + require.False(t, found) + require.Error(t, err) + assert.Contains(t, err.Error(), "outside plugin socket dir") + assert.Nil(t, conn) + assert.Nil(t, client) +} + +func Test_cmpSupports_dialFailure_returnsError(t *testing.T) { + // Use a temp dir as the base plugin socket dir and provide a socket filename + // that does not have a listening server; dialing should fail, and the error + // returned should reflect a dialing problem. + pluginSockFilePath := t.TempDir() + fileName := "nonexistent.sock" + + conn, client, found, err := cmpSupports(context.Background(), pluginSockFilePath, "appPath", "repoPath", fileName, nil, nil, true) + require.False(t, found) + require.Error(t, err) + // We expect the error to at least indicate dialing failure; exact wording may vary, + // check for the dialing error prefix used in cmpSupports. + assert.Contains(t, err.Error(), "error dialing to cmp-server") + assert.Nil(t, conn) + assert.Nil(t, client) +} diff --git a/util/cmp/stream.go b/util/cmp/stream.go index c6c2195987219..ca7b987fbfbb3 100644 --- a/util/cmp/stream.go +++ b/util/cmp/stream.go @@ -96,6 +96,10 @@ func SendRepoStream(ctx context.Context, appPath, rootPath string, sender Stream defer tgzstream.CloseAndDelete(tgz) err = sender.Send(mr) if err != nil { + // include ctx.Err() in the message to make cancellations/deadlines visible + if ctx != nil && ctx.Err() != nil { + return fmt.Errorf("error sending generate manifest metadata to cmp-server: %w (stream ctx err: %w)", err, ctx.Err()) + } return fmt.Errorf("error sending generate manifest metadata to cmp-server: %w", err) }