fix(3.1): change the appset namespace to server namespace when generating appset (argoproj#24478)

Signed-off-by: nitishfy <[email protected]>
Co-authored-by: Alexandre Gaudreault <[email protected]>

Author: nitishfy

applicationset/generators/git.go

@@ -29,10 +29,10 @@ type GitGenerator struct {
 }
 
 // NewGitGenerator creates a new instance of Git Generator
-func NewGitGenerator(repos services.Repos, namespace string) Generator {
+func NewGitGenerator(repos services.Repos, controllerNamespace string) Generator {
 	g := &GitGenerator{
 		repos:     repos,
-		namespace: namespace,
+		namespace: controllerNamespace,
 	}
 
 	return g
@@ -78,11 +78,11 @@ func (g *GitGenerator) GenerateParams(appSetGenerator *argoprojiov1alpha1.Applic
 	if !strings.Contains(appSet.Spec.Template.Spec.Project, "{{") {
 		project := appSet.Spec.Template.Spec.Project
 		appProject := &argoprojiov1alpha1.AppProject{}
-		namespace := g.namespace
-		if namespace == "" {
-			namespace = appSet.Namespace
+		controllerNamespace := g.namespace
+		if controllerNamespace == "" {
+			controllerNamespace = appSet.Namespace
 		}
-		if err := client.Get(context.TODO(), types.NamespacedName{Name: project, Namespace: namespace}, appProject); err != nil {
+		if err := client.Get(context.TODO(), types.NamespacedName{Name: project, Namespace: controllerNamespace}, appProject); err != nil {
 			return nil, fmt.Errorf("error getting project %s: %w", project, err)
 		}
 		// we need to verify the signature on the Git revision if GPG is enabled

applicationset/generators/utils.go

@@ -10,15 +10,15 @@ import (
 	"github.com/argoproj/argo-cd/v3/applicationset/services"
 )
 
-func GetGenerators(ctx context.Context, c client.Client, k8sClient kubernetes.Interface, namespace string, argoCDService services.Repos, dynamicClient dynamic.Interface, scmConfig SCMConfig) map[string]Generator {
+func GetGenerators(ctx context.Context, c client.Client, k8sClient kubernetes.Interface, controllerNamespace string, argoCDService services.Repos, dynamicClient dynamic.Interface, scmConfig SCMConfig) map[string]Generator {
 	terminalGenerators := map[string]Generator{
 		"List":                    NewListGenerator(),
-		"Clusters":                NewClusterGenerator(ctx, c, k8sClient, namespace),
-		"Git":                     NewGitGenerator(argoCDService, namespace),
+		"Clusters":                NewClusterGenerator(ctx, c, k8sClient, controllerNamespace),
+		"Git":                     NewGitGenerator(argoCDService, controllerNamespace),
 		"SCMProvider":             NewSCMProviderGenerator(c, scmConfig),
-		"ClusterDecisionResource": NewDuckTypeGenerator(ctx, dynamicClient, k8sClient, namespace),
+		"ClusterDecisionResource": NewDuckTypeGenerator(ctx, dynamicClient, k8sClient, controllerNamespace),
 		"PullRequest":             NewPullRequestGenerator(c, scmConfig),
-		"Plugin":                  NewPluginGenerator(c, namespace),
+		"Plugin":                  NewPluginGenerator(c, controllerNamespace),
 	}
 
 	nestedGenerators := map[string]Generator{

server/applicationset/applicationset.go

@@ -200,7 +200,7 @@ func (s *Server) Create(ctx context.Context, q *applicationset.ApplicationSetCre
 	}
 
 	if q.GetDryRun() {
-		apps, err := s.generateApplicationSetApps(ctx, log.WithField("applicationset", appset.Name), *appset, namespace)
+		apps, err := s.generateApplicationSetApps(ctx, log.WithField("applicationset", appset.Name), *appset)
 		if err != nil {
 			return nil, fmt.Errorf("unable to generate Applications of ApplicationSet: %w", err)
 		}
@@ -260,12 +260,12 @@ func (s *Server) Create(ctx context.Context, q *applicationset.ApplicationSetCre
 	return updated, nil
 }
 
-func (s *Server) generateApplicationSetApps(ctx context.Context, logEntry *log.Entry, appset v1alpha1.ApplicationSet, namespace string) ([]v1alpha1.Application, error) {
+func (s *Server) generateApplicationSetApps(ctx context.Context, logEntry *log.Entry, appset v1alpha1.ApplicationSet) ([]v1alpha1.Application, error) {
 	argoCDDB := s.db
 
 	scmConfig := generators.NewSCMConfig(s.ScmRootCAPath, s.AllowedScmProviders, s.EnableScmProviders, s.EnableGitHubAPIMetrics, github_app.NewAuthCredentials(argoCDDB.(db.RepoCredsDB)), true)
 	argoCDService := services.NewArgoCDService(s.db, s.GitSubmoduleEnabled, s.repoClientSet, s.EnableNewGitFileGlobbing)
-	appSetGenerators := generators.GetGenerators(ctx, s.client, s.k8sClient, namespace, argoCDService, s.dynamicClient, scmConfig)
+	appSetGenerators := generators.GetGenerators(ctx, s.client, s.k8sClient, s.ns, argoCDService, s.dynamicClient, scmConfig)
 
 	apps, _, err := appsettemplate.GenerateApplications(logEntry, appset, appSetGenerators, &appsetutils.Render{}, s.client)
 	if err != nil {
@@ -363,11 +363,15 @@ func (s *Server) Generate(ctx context.Context, q *applicationset.ApplicationSetG
 	if appset == nil {
 		return nil, errors.New("error creating ApplicationSets: ApplicationSets is nil in request")
 	}
-	namespace := s.appsetNamespaceOrDefault(appset.Namespace)
 
+	// The RBAC check needs to be performed against the appset namespace
+	// However, when trying to generate params, the server namespace needs
+	// to be passed.
+	namespace := s.appsetNamespaceOrDefault(appset.Namespace)
 	if !s.isNamespaceEnabled(namespace) {
 		return nil, security.NamespaceNotPermittedError(namespace)
 	}
+
 	projectName, err := s.validateAppSet(appset)
 	if err != nil {
 		return nil, fmt.Errorf("error validating ApplicationSets: %w", err)
@@ -380,7 +384,16 @@ func (s *Server) Generate(ctx context.Context, q *applicationset.ApplicationSetG
 	logger := log.New()
 	logger.SetOutput(logs)
 
-	apps, err := s.generateApplicationSetApps(ctx, logger.WithField("applicationset", appset.Name), *appset, namespace)
+	// The server namespace will be used in the function
+	// since this is the exact namespace that is being used
+	// to generate parameters (especially for git generator).
+	//
+	// In case of Git generator, if the namespace is set to
+	// appset namespace, we'll look for a project in the appset
+	// namespace that would lead to error when generating params
+	// for an appset in any namespace feature.
+	// See https://github.com/argoproj/argo-cd/issues/22942
+	apps, err := s.generateApplicationSetApps(ctx, logger.WithField("applicationset", appset.Name), *appset)
 	if err != nil {
 		return nil, fmt.Errorf("unable to generate Applications of ApplicationSet: %w\n%s", err, logs.String())
 	}

server/applicationset/applicationset_test.go

@@ -4,6 +4,9 @@ import (
 	"sort"
 	"testing"
 
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	cr_fake "sigs.k8s.io/controller-runtime/pkg/client/fake"
+
 	"github.com/argoproj/gitops-engine/pkg/health"
 	"github.com/argoproj/pkg/v2/sync"
 	"github.com/stretchr/testify/assert"
@@ -50,7 +53,7 @@ func fakeCluster() *appsv1.Cluster {
 }
 
 // return an ApplicationServiceServer which returns fake data
-func newTestAppSetServer(t *testing.T, objects ...runtime.Object) *Server {
+func newTestAppSetServer(t *testing.T, objects ...client.Object) *Server {
 	t.Helper()
 	f := func(enf *rbac.Enforcer) {
 		_ = enf.SetBuiltinPolicy(assets.BuiltinPolicyCSV)
@@ -61,7 +64,7 @@ func newTestAppSetServer(t *testing.T, objects ...runtime.Object) *Server {
 }
 
 // return an ApplicationServiceServer which returns fake data
-func newTestNamespacedAppSetServer(t *testing.T, objects ...runtime.Object) *Server {
+func newTestNamespacedAppSetServer(t *testing.T, objects ...client.Object) *Server {
 	t.Helper()
 	f := func(enf *rbac.Enforcer) {
 		_ = enf.SetBuiltinPolicy(assets.BuiltinPolicyCSV)
@@ -71,7 +74,7 @@ func newTestNamespacedAppSetServer(t *testing.T, objects ...runtime.Object) *Ser
 	return newTestAppSetServerWithEnforcerConfigure(t, f, scopedNamespaces, objects...)
 }
 
-func newTestAppSetServerWithEnforcerConfigure(t *testing.T, f func(*rbac.Enforcer), namespace string, objects ...runtime.Object) *Server {
+func newTestAppSetServerWithEnforcerConfigure(t *testing.T, f func(*rbac.Enforcer), namespace string, objects ...client.Object) *Server {
 	t.Helper()
 	kubeclientset := fake.NewClientset(&corev1.ConfigMap{
 		ObjectMeta: metav1.ObjectMeta{
@@ -115,7 +118,11 @@ func newTestAppSetServerWithEnforcerConfigure(t *testing.T, f func(*rbac.Enforce
 
 	objects = append(objects, defaultProj, myProj)
 
-	fakeAppsClientset := apps.NewSimpleClientset(objects...)
+	runtimeObjects := make([]runtime.Object, len(objects))
+	for i := range objects {
+		runtimeObjects[i] = objects[i]
+	}
+	fakeAppsClientset := apps.NewSimpleClientset(runtimeObjects...)
 	factory := appinformer.NewSharedInformerFactoryWithOptions(fakeAppsClientset, 0, appinformer.WithNamespace(namespace), appinformer.WithTweakListOptions(func(_ *metav1.ListOptions) {}))
 	fakeProjLister := factory.Argoproj().V1alpha1().AppProjects().Lister().AppProjects(testNamespace)
 
@@ -138,6 +145,13 @@ func newTestAppSetServerWithEnforcerConfigure(t *testing.T, f func(*rbac.Enforce
 		panic("Timed out waiting for caches to sync")
 	}
 
+	scheme := runtime.NewScheme()
+	err = appsv1.AddToScheme(scheme)
+	require.NoError(t, err)
+	err = corev1.AddToScheme(scheme)
+	require.NoError(t, err)
+	crClient := cr_fake.NewClientBuilder().WithScheme(scheme).WithObjects(objects...).Build()
+
 	projInformer := factory.Argoproj().V1alpha1().AppProjects().Informer()
 	go projInformer.Run(ctx.Done())
 	if !k8scache.WaitForCacheSync(ctx.Done(), projInformer.HasSynced) {
@@ -148,7 +162,7 @@ func newTestAppSetServerWithEnforcerConfigure(t *testing.T, f func(*rbac.Enforce
 		db,
 		kubeclientset,
 		nil,
-		nil,
+		crClient,
 		enforcer,
 		nil,
 		fakeAppsClientset,
@@ -640,3 +654,54 @@ func TestResourceTree(t *testing.T) {
 		assert.EqualError(t, err, "namespace 'NOT-ALLOWED' is not permitted")
 	})
 }
+
+func TestAppSet_Generate_Cluster(t *testing.T) {
+	appSet1 := newTestAppSet(func(appset *appsv1.ApplicationSet) {
+		appset.Name = "AppSet1"
+		appset.Spec.Template.Name = "{{name}}"
+		appset.Spec.Generators = []appsv1.ApplicationSetGenerator{
+			{
+				Clusters: &appsv1.ClusterGenerator{},
+			},
+		}
+	})
+
+	t.Run("Generate in default namespace", func(t *testing.T) {
+		appSetServer := newTestAppSetServer(t, appSet1)
+		appsetQuery := applicationset.ApplicationSetGenerateRequest{
+			ApplicationSet: appSet1,
+		}
+
+		res, err := appSetServer.Generate(t.Context(), &appsetQuery)
+		require.NoError(t, err)
+		require.Len(t, res.Applications, 2)
+		assert.Equal(t, "fake-cluster", res.Applications[0].Name)
+		assert.Equal(t, "in-cluster", res.Applications[1].Name)
+	})
+
+	t.Run("Generate in different namespace", func(t *testing.T) {
+		appSetServer := newTestAppSetServer(t, appSet1)
+
+		appSet1Ns := appSet1.DeepCopy()
+		appSet1Ns.Namespace = "external-namespace"
+		appsetQuery := applicationset.ApplicationSetGenerateRequest{ApplicationSet: appSet1Ns}
+
+		res, err := appSetServer.Generate(t.Context(), &appsetQuery)
+		require.NoError(t, err)
+		require.Len(t, res.Applications, 2)
+		assert.Equal(t, "fake-cluster", res.Applications[0].Name)
+		assert.Equal(t, "in-cluster", res.Applications[1].Name)
+	})
+
+	t.Run("Generate in not allowed namespace", func(t *testing.T) {
+		appSetServer := newTestAppSetServer(t, appSet1)
+
+		appSet1Ns := appSet1.DeepCopy()
+		appSet1Ns.Namespace = "NOT-ALLOWED"
+
+		appsetQuery := applicationset.ApplicationSetGenerateRequest{ApplicationSet: appSet1Ns}
+
+		_, err := appSetServer.Generate(t.Context(), &appsetQuery)
+		assert.EqualError(t, err, "namespace 'NOT-ALLOWED' is not permitted")
+	})
+}