Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: CMPv2 does not allow symlinks to adjacent files in same git repo. Fixes #13342 #13360

Merged
merged 10 commits into from
May 8, 2023
16 changes: 8 additions & 8 deletions reposerver/repository/repository.go
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ func (s *Service) ListApps(ctx context.Context, q *apiclient.ListAppsRequest) (*
}

defer io.Close(closer)
apps, err := discovery.Discover(ctx, gitClient.Root(), q.EnabledSourceTypes, s.initConstants.CMPTarExcludedGlobs)
apps, err := discovery.Discover(ctx, gitClient.Root(), gitClient.Root(), q.EnabledSourceTypes, s.initConstants.CMPTarExcludedGlobs)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -1304,7 +1304,7 @@ func GenerateManifests(ctx context.Context, appPath, repoRoot, revision string,

resourceTracking := argo.NewResourceTracking()

appSourceType, err := GetAppSourceType(ctx, q.ApplicationSource, appPath, q.AppName, q.EnabledSourceTypes, opt.cmpTarExcludedGlobs)
appSourceType, err := GetAppSourceType(ctx, q.ApplicationSource, appPath, repoRoot, q.AppName, q.EnabledSourceTypes, opt.cmpTarExcludedGlobs)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -1481,8 +1481,8 @@ func mergeSourceParameters(source *v1alpha1.ApplicationSource, path, appName str
}

// GetAppSourceType returns explicit application source type or examines a directory and determines its application source type
func GetAppSourceType(ctx context.Context, source *v1alpha1.ApplicationSource, path, appName string, enableGenerateManifests map[string]bool, tarExcludedGlobs []string) (v1alpha1.ApplicationSourceType, error) {
err := mergeSourceParameters(source, path, appName)
func GetAppSourceType(ctx context.Context, source *v1alpha1.ApplicationSource, appPath, repoPath, appName string, enableGenerateManifests map[string]bool, tarExcludedGlobs []string) (v1alpha1.ApplicationSourceType, error) {
err := mergeSourceParameters(source, appPath, appName)
if err != nil {
return "", fmt.Errorf("error while parsing source parameters: %v", err)
}
Expand All @@ -1498,7 +1498,7 @@ func GetAppSourceType(ctx context.Context, source *v1alpha1.ApplicationSource, p
}
return *appSourceType, nil
}
appType, err := discovery.AppType(ctx, path, enableGenerateManifests, tarExcludedGlobs)
appType, err := discovery.AppType(ctx, appPath, repoPath, enableGenerateManifests, tarExcludedGlobs)
if err != nil {
return "", err
}
Expand Down Expand Up @@ -1918,7 +1918,7 @@ func runConfigManagementPluginSidecars(ctx context.Context, appPath, repoPath, p
}

// detect config management plugin server (sidecar)
conn, cmpClient, err := discovery.DetectConfigManagementPlugin(ctx, appPath, pluginName, env, tarExcludedGlobs)
jiachengxu marked this conversation as resolved.
Show resolved Hide resolved
conn, cmpClient, err := discovery.DetectConfigManagementPlugin(ctx, appPath, repoPath, pluginName, env, tarExcludedGlobs)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -1975,7 +1975,7 @@ func (s *Service) GetAppDetails(ctx context.Context, q *apiclient.RepoServerAppD
return err
}

appSourceType, err := GetAppSourceType(ctx, q.Source, opContext.appPath, q.AppName, q.EnabledSourceTypes, s.initConstants.CMPTarExcludedGlobs)
appSourceType, err := GetAppSourceType(ctx, q.Source, opContext.appPath, repoRoot, q.AppName, q.EnabledSourceTypes, s.initConstants.CMPTarExcludedGlobs)
if err != nil {
return err
}
Expand Down Expand Up @@ -2169,7 +2169,7 @@ func populatePluginAppDetails(ctx context.Context, res *apiclient.RepoAppDetails
pluginName = q.Source.Plugin.Name
}
// detect config management plugin server (sidecar)
conn, cmpClient, err := discovery.DetectConfigManagementPlugin(ctx, appPath, pluginName, env, tarExcludedGlobs)
conn, cmpClient, err := discovery.DetectConfigManagementPlugin(ctx, appPath, repoPath, pluginName, env, tarExcludedGlobs)
if err != nil {
return fmt.Errorf("failed to detect CMP for app: %w", err)
}
Expand Down
6 changes: 3 additions & 3 deletions reposerver/repository/repository_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1188,15 +1188,15 @@ func TestGenerateNullList(t *testing.T) {
}

func TestIdentifyAppSourceTypeByAppDirWithKustomizations(t *testing.T) {
sourceType, err := GetAppSourceType(context.Background(), &argoappv1.ApplicationSource{}, "./testdata/kustomization_yaml", "testapp", map[string]bool{}, []string{})
sourceType, err := GetAppSourceType(context.Background(), &argoappv1.ApplicationSource{}, "./testdata/kustomization_yaml", "./testdata", "testapp", map[string]bool{}, []string{})
assert.Nil(t, err)
assert.Equal(t, argoappv1.ApplicationSourceTypeKustomize, sourceType)

sourceType, err = GetAppSourceType(context.Background(), &argoappv1.ApplicationSource{}, "./testdata/kustomization_yml", "testapp", map[string]bool{}, []string{})
sourceType, err = GetAppSourceType(context.Background(), &argoappv1.ApplicationSource{}, "./testdata/kustomization_yml", "./testdata", "testapp", map[string]bool{}, []string{})
assert.Nil(t, err)
assert.Equal(t, argoappv1.ApplicationSourceTypeKustomize, sourceType)

sourceType, err = GetAppSourceType(context.Background(), &argoappv1.ApplicationSource{}, "./testdata/Kustomization", "testapp", map[string]bool{}, []string{})
sourceType, err = GetAppSourceType(context.Background(), &argoappv1.ApplicationSource{}, "./testdata/Kustomization", "./testdata", "testapp", map[string]bool{}, []string{})
assert.Nil(t, err)
assert.Equal(t, argoappv1.ApplicationSourceTypeKustomize, sourceType)
}
Expand Down
53 changes: 52 additions & 1 deletion test/e2e/custom_tool_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ func TestCMPDiscoverWithFileName(t *testing.T) {
time.Sleep(1 * time.Second)
os.Setenv("ARGOCD_BINARY_NAME", "argocd")
}).
Path(pluginName).
Path(pluginName + "/subdir").
When().
CreateApp().
Sync().
Expand Down Expand Up @@ -350,3 +350,54 @@ func TestPreserveFileModeForCMP(t *testing.T) {
assert.Equal(t, "ConfigMap", app.Status.Resources[0].Kind)
})
}

func TestCMPWithSymlinkPartialFiles(t *testing.T) {
Given(t, WithTestData("testdata2")).
And(func() {
go startCMPServer("./testdata2/cmp-symlink")
time.Sleep(1 * time.Second)
os.Setenv("ARGOCD_BINARY_NAME", "argocd")
}).
Path("guestbook-partial-symlink-files").
When().
CreateApp().
Sync().
Then().
Expect(OperationPhaseIs(OperationSucceeded)).
Expect(SyncStatusIs(SyncStatusCodeSynced)).
Expect(HealthIs(health.HealthStatusHealthy))
}

func TestCMPWithSymlinkFiles(t *testing.T) {
Given(t, WithTestData("testdata2")).
And(func() {
go startCMPServer("./testdata2/cmp-symlink")
time.Sleep(1 * time.Second)
os.Setenv("ARGOCD_BINARY_NAME", "argocd")
}).
Path("guestbook-symlink-files").
When().
CreateApp().
Sync().
Then().
Expect(OperationPhaseIs(OperationSucceeded)).
Expect(SyncStatusIs(SyncStatusCodeSynced)).
Expect(HealthIs(health.HealthStatusHealthy))
}

func TestCMPWithSymlinkFolder(t *testing.T) {
Given(t, WithTestData("testdata2")).
And(func() {
go startCMPServer("./testdata2/cmp-symlink")
time.Sleep(1 * time.Second)
os.Setenv("ARGOCD_BINARY_NAME", "argocd")
}).
Path("guestbook-symlink-folder").
When().
CreateApp().
Sync().
Then().
Expect(OperationPhaseIs(OperationSucceeded)).
Expect(SyncStatusIs(SyncStatusCodeSynced)).
Expect(HealthIs(health.HealthStatusHealthy))
}
4 changes: 2 additions & 2 deletions test/e2e/fixture/app/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,8 @@ type ContextArgs struct {
AppNamespace string
}

func Given(t *testing.T) *Context {
fixture.EnsureCleanState(t)
func Given(t *testing.T, opts ...fixture.TestOption) *Context {
fixture.EnsureCleanState(t, opts...)
return GivenWithSameState(t)
}

Expand Down
27 changes: 25 additions & 2 deletions test/e2e/fixture/fixture.go
Original file line number Diff line number Diff line change
Expand Up @@ -514,7 +514,30 @@ func SetParamInNotificationsConfigMap(key, value string) {
})
}

func EnsureCleanState(t *testing.T) {
type TestOption func(option *testOption)

type testOption struct {
testdata string
}

func newTestOption(opts ...TestOption) *testOption {
to := &testOption{
testdata: "testdata",
}
for _, opt := range opts {
opt(to)
}
return to
}

func WithTestData(testdata string) TestOption {
return func(option *testOption) {
option.testdata = testdata
}
}

func EnsureCleanState(t *testing.T, opts ...TestOption) {
opt := newTestOption(opts...)
// In large scenarios, we can skip tests that already run
SkipIfAlreadyRun(t)
// Register this test after it has been run & was successfull
Expand Down Expand Up @@ -632,7 +655,7 @@ func EnsureCleanState(t *testing.T) {
}

// set-up tmp repo, must have unique name
FailOnErr(Run("", "cp", "-Rf", "testdata", repoDirectory()))
FailOnErr(Run("", "cp", "-Rf", opt.testdata, repoDirectory()))
FailOnErr(Run(repoDirectory(), "chmod", "777", "."))
FailOnErr(Run(repoDirectory(), "git", "init"))
FailOnErr(Run(repoDirectory(), "git", "add", "."))
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/testdata/cmp-fileName/plugin.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,4 @@ spec:
generate:
command: [sh, -c, 'echo "{\"kind\": \"ConfigMap\", \"apiVersion\": \"v1\", \"metadata\": { \"name\": \"$ARGOCD_APP_NAME\", \"namespace\": \"$ARGOCD_APP_NAMESPACE\", \"annotations\": {\"Foo\": \"$FOO\", \"KubeVersion\": \"$KUBE_VERSION\", \"KubeApiVersion\": \"$KUBE_API_VERSIONS\",\"Bar\": \"baz\"}}}"']
discover:
fileName: "subdir/s*.yaml"
fileName: "cmp-fileName/subdir/s*.yaml"
Copy link
Member Author

Choose a reason for hiding this comment

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

Can you check why TestCMPDiscoverWithFileName is failing please ? Looks like it is related to changes and not flakiness.

@alexmt It turned out that this test failed because of the test case is wrong.
As stated on the config management plugin doc, the filename should be applied to the repository root not the application source:

fileName is a glob pattern (https://pkg.go.dev/path/filepath#Glob) that is applied to the repository's root directory (not the Application source directory). If there is a match, this plugin may be used for the repository.

and in the test case, the testdata is the repo, so we need to have cmp-fileName as part of the fileName.
It was not caught because we only passed appPath but not repoPath so the appPath was considered as repoPath and cmp-fileName was not needed.

Copy link
Member

Choose a reason for hiding this comment

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

This change is causing issues, so I'm going to try to revert this change without reverting your whole bugfix. #13940

13 changes: 13 additions & 0 deletions test/e2e/testdata2/cmp-symlink/plugin.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
apiVersion: argoproj.io/v1alpha1
kind: ConfigManagementPlugin
metadata:
name: cmp-symlink
spec:
version: v1.0
init:
command: [kustomize, version]
generate:
command: [sh, -c, 'kustomize edit set image test=quay.io/argoprojlabs/argocd-e2e-container:0.2 && kustomize build --load-restrictor LoadRestrictionsNone']
discover:
find:
glob: "**/kustomization.yaml"
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
apiVersion: v1
kind: Service
metadata:
name: guestbook-ui
spec:
ports:
- port: 80
targetPort: 80
selector:
app: guestbook-ui
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization

resources:
- ./guestbook-ui-deployment.yaml
- ./guestbook-ui-svc.yaml
1 change: 1 addition & 0 deletions test/e2e/testdata2/guestbook-symlink-folder
23 changes: 23 additions & 0 deletions test/e2e/testdata2/guestbook/guestbook-ui-deployment.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
apiVersion: apps/v1
kind: Deployment
metadata:
name: guestbook-ui
labels:
test: "true"
spec:
replicas: 0
revisionHistoryLimit: 3
selector:
matchLabels:
app: guestbook-ui
template:
metadata:
labels:
app: guestbook-ui
spec:
containers:
- image: test
imagePullPolicy: IfNotPresent
name: guestbook-ui
ports:
- containerPort: 80
10 changes: 10 additions & 0 deletions test/e2e/testdata2/guestbook/guestbook-ui-svc.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
apiVersion: v1
kind: Service
metadata:
name: guestbook-ui
spec:
ports:
- port: 80
targetPort: 80
selector:
app: guestbook-ui
6 changes: 6 additions & 0 deletions test/e2e/testdata2/guestbook/kustomization.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization

resources:
- ./guestbook-ui-deployment.yaml
- ./guestbook-ui-svc.yaml
26 changes: 13 additions & 13 deletions util/app/discovery/discovery.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,11 @@ func IsManifestGenerationEnabled(sourceType v1alpha1.ApplicationSourceType, enab
return enabled
}

func Discover(ctx context.Context, repoPath string, enableGenerateManifests map[string]bool, tarExcludedGlobs []string) (map[string]string, error) {
func Discover(ctx context.Context, appPath, repoPath string, enableGenerateManifests map[string]bool, tarExcludedGlobs []string) (map[string]string, error) {
apps := make(map[string]string)

// Check if it is CMP
conn, _, err := DetectConfigManagementPlugin(ctx, repoPath, "", []string{}, tarExcludedGlobs)
conn, _, err := DetectConfigManagementPlugin(ctx, appPath, repoPath, "", []string{}, tarExcludedGlobs)
if err == nil {
// Found CMP
io.Close(conn)
Expand All @@ -44,14 +44,14 @@ func Discover(ctx context.Context, repoPath string, enableGenerateManifests map[
return apps, nil
}

err = filepath.Walk(repoPath, func(path string, info os.FileInfo, err error) error {
err = filepath.Walk(appPath, func(path string, info os.FileInfo, err error) error {
if err != nil {
return err
}
if info.IsDir() {
return nil
}
dir, err := filepath.Rel(repoPath, filepath.Dir(path))
dir, err := filepath.Rel(appPath, filepath.Dir(path))
if err != nil {
return err
}
Expand All @@ -67,8 +67,8 @@ func Discover(ctx context.Context, repoPath string, enableGenerateManifests map[
return apps, err
}

func AppType(ctx context.Context, path string, enableGenerateManifests map[string]bool, tarExcludedGlobs []string) (string, error) {
apps, err := Discover(ctx, path, enableGenerateManifests, tarExcludedGlobs)
func AppType(ctx context.Context, appPath, repoPath string, enableGenerateManifests map[string]bool, tarExcludedGlobs []string) (string, error) {
apps, err := Discover(ctx, appPath, repoPath, enableGenerateManifests, tarExcludedGlobs)
if err != nil {
return "", err
}
Expand All @@ -85,7 +85,7 @@ func AppType(ctx context.Context, path string, enableGenerateManifests map[strin
// check cmpSupports()
// if supported return conn for the cmp-server

func DetectConfigManagementPlugin(ctx context.Context, repoPath, pluginName string, env []string, tarExcludedGlobs []string) (io.Closer, pluginclient.ConfigManagementPluginServiceClient, error) {
func DetectConfigManagementPlugin(ctx context.Context, appPath, repoPath, pluginName string, env []string, tarExcludedGlobs []string) (io.Closer, pluginclient.ConfigManagementPluginServiceClient, error) {
var conn io.Closer
var cmpClient pluginclient.ConfigManagementPluginServiceClient
var connFound bool
Expand All @@ -98,7 +98,7 @@ func DetectConfigManagementPlugin(ctx context.Context, repoPath, pluginName stri

if pluginName != "" {
// check if the given plugin supports the repo
conn, cmpClient, connFound = cmpSupports(ctx, pluginSockFilePath, repoPath, fmt.Sprintf("%v.sock", pluginName), env, tarExcludedGlobs, true)
conn, cmpClient, connFound = cmpSupports(ctx, pluginSockFilePath, appPath, repoPath, fmt.Sprintf("%v.sock", pluginName), env, tarExcludedGlobs, true)
if !connFound {
return nil, nil, fmt.Errorf("couldn't find cmp-server plugin with name %q supporting the given repository", pluginName)
}
Expand All @@ -109,7 +109,7 @@ func DetectConfigManagementPlugin(ctx context.Context, repoPath, pluginName stri
}
for _, file := range fileList {
if file.Type() == os.ModeSocket {
conn, cmpClient, connFound = cmpSupports(ctx, pluginSockFilePath, repoPath, file.Name(), env, tarExcludedGlobs, false)
conn, cmpClient, connFound = cmpSupports(ctx, pluginSockFilePath, appPath, repoPath, file.Name(), env, tarExcludedGlobs, false)
if connFound {
break
}
Expand All @@ -125,13 +125,13 @@ func DetectConfigManagementPlugin(ctx context.Context, repoPath, pluginName stri
// matchRepositoryCMP will send the repoPath to the cmp-server. The cmp-server will
// inspect the files and return true if the repo is supported for manifest generation.
// Will return false otherwise.
func matchRepositoryCMP(ctx context.Context, repoPath string, client pluginclient.ConfigManagementPluginServiceClient, env []string, tarExcludedGlobs []string) (bool, bool, error) {
func matchRepositoryCMP(ctx context.Context, appPath, repoPath string, client pluginclient.ConfigManagementPluginServiceClient, env []string, tarExcludedGlobs []string) (bool, bool, error) {
matchRepoStream, err := client.MatchRepository(ctx, grpc_retry.Disable())
if err != nil {
return false, false, fmt.Errorf("error getting stream client: %s", err)
}

err = cmp.SendRepoStream(ctx, repoPath, repoPath, matchRepoStream, env, tarExcludedGlobs)
err = cmp.SendRepoStream(ctx, appPath, repoPath, matchRepoStream, env, tarExcludedGlobs)
if err != nil {
return false, false, fmt.Errorf("error sending stream: %s", err)
}
Expand All @@ -142,7 +142,7 @@ func matchRepositoryCMP(ctx context.Context, repoPath string, client pluginclien
return resp.GetIsSupported(), resp.GetIsDiscoveryEnabled(), nil
}

func cmpSupports(ctx context.Context, pluginSockFilePath, repoPath, fileName string, env []string, tarExcludedGlobs []string, namedPlugin bool) (io.Closer, pluginclient.ConfigManagementPluginServiceClient, bool) {
func cmpSupports(ctx context.Context, pluginSockFilePath, appPath, repoPath, fileName string, env []string, tarExcludedGlobs []string, namedPlugin bool) (io.Closer, pluginclient.ConfigManagementPluginServiceClient, bool) {
absPluginSockFilePath, err := filepath.Abs(pluginSockFilePath)
if err != nil {
log.Errorf("error getting absolute path for plugin socket dir %v, %v", pluginSockFilePath, err)
Expand All @@ -165,7 +165,7 @@ func cmpSupports(ctx context.Context, pluginSockFilePath, repoPath, fileName str
return nil, nil, false
}

isSupported, isDiscoveryEnabled, err := matchRepositoryCMP(ctx, repoPath, cmpClient, env, tarExcludedGlobs)
isSupported, isDiscoveryEnabled, err := matchRepositoryCMP(ctx, appPath, repoPath, cmpClient, env, tarExcludedGlobs)
if err != nil {
log.WithFields(log.Fields{
common.SecurityField: common.SecurityMedium,
Expand Down
Loading