Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions reposerver/repository/repository.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}
Expand Down
37 changes: 24 additions & 13 deletions util/app/discovery/discovery.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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 {
Expand All @@ -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)
Comment on lines -115 to +119
Copy link
Member

@choejwoo choejwoo Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be misunderstanding the intent here, so I just want to make sure I understand correctly.
In discovery mode, iterate over multiple socket files. If the first probe returns an error like EOF, but a later probe returns connfound=false, lastErr=nil (not supported), does that later result overwrite lastErr and effectively drop the earlier error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@choejwoo yes that is correct

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@choejwoo yes that is correct

Thanks for confirming, LGTM.

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")
}
}
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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
}
33 changes: 33 additions & 0 deletions util/app/discovery/discovery_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package discovery

import (
"context"
"testing"

"github.com/argoproj/argo-cd/v3/pkg/apis/application/v1alpha1"
Expand Down Expand Up @@ -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)
}
4 changes: 4 additions & 0 deletions util/cmp/stream.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand Down
Loading