e2e: add options to SyncWithGitSource (#1398)

- Replace MultiRepo.SourceFormat with GitSyncSource.SourceFormat. - Use ntopts.Unstructured on individual SyncWithGitSource now, instead of on the whole parent NT.New. This should make it more obvious that the setting only applies to RootSyncs, and allow more granular configurations. - Move default SourceFormats for e2e tests into ntopts/multi_repo.go, instead of spread across multiple files. - Add GitSourceOptions for SyncWithGitSource to further configure individual RSync Git source configs. - Add RSync Kind validation to setupTestCase, to enforce RootSync and RepoSync. - Change Repository to use SyncKind, to simplify usage, instead of its own enum type.
GoogleContainerTools · Aug 22, 2024 · f6b97be · f6b97be
1 parent ac63dc8
commit f6b97be
Show file tree

Hide file tree

Showing 35 changed files with 239 additions and 180 deletions.
diff --git a/e2e/nomostest/gitproviders/repository.go b/e2e/nomostest/gitproviders/repository.go
@@ -61,16 +61,6 @@ const (
 	DefaultSyncDir = "acme"
 )
 
-// RepoType represents the type of the source repository.
-type RepoType string
-
-const (
-	// RootRepo indicates the resources in the repository are cluster-scoped.
-	RootRepo RepoType = "root"
-	// NamespaceRepo indicates the resources in the repository are namespace-scoped.
-	NamespaceRepo RepoType = "namespace"
-)
-
 // Repository is a local git repository with a connection to a repository
 // on the git-server for the test.
 //
@@ -89,8 +79,8 @@ type Repository struct {
 	// PrivateKeyPath is the local path to the private key on disk to use to
 	// authenticate with the git server.
 	PrivateKeyPath string
-	// Type refers to the type of the repository, i.e. if it is a root repo or a namespace repo.
-	Type RepoType
+	// SyncKind refers to the kind of the RSync using the repository: RootSync or RepoSync.
+	SyncKind string
 	// SafetyNSPath is the path to the safety namespace yaml file.
 	SafetyNSPath string
 	// SafetyNSName is the name of the safety namespace.
@@ -119,7 +109,7 @@ type Repository struct {
 // Locally, it writes the repository to `tmpdir`/repos/`name`.
 // Name is the <NAMESPACE>/<NAME> of the RootSync|RepoSync.
 func NewRepository(
-	repoType RepoType,
+	syncKind string,
 	syncNN types.NamespacedName,
 	sourceFormat configsync.SourceFormat,
 	scheme *runtime.Scheme,
@@ -135,7 +125,7 @@ func NewRepository(
 		Name:                  namespacedName,
 		Root:                  filepath.Join(tmpDir, "repos", namespacedName),
 		Format:                sourceFormat,
-		Type:                  repoType,
+		SyncKind:              syncKind,
 		SafetyNSName:          safetyName,
 		SafetyNSPath:          fmt.Sprintf("acme/namespaces/%s/ns.yaml", safetyName),
 		SafetyClusterRoleName: safetyName,
@@ -197,7 +187,8 @@ func (g *Repository) InitialCommit(sourceFormat configsync.SourceFormat) error {
 	if err := g.AddEmptyDir(DefaultSyncDir); err != nil {
 		return err
 	}
-	if g.Type == RootRepo {
+	switch g.SyncKind {
+	case configsync.RootSyncKind:
 		// Add safety Namespace and ClusterRole to avoid errors from the safety
 		// check (KNV2006) when deleting all the other remaining objects.
 		if err := g.AddSafetyNamespace(); err != nil {
@@ -206,6 +197,10 @@ func (g *Repository) InitialCommit(sourceFormat configsync.SourceFormat) error {
 		if err := g.AddSafetyClusterRole(); err != nil {
 			return err
 		}
+	case configsync.RepoSyncKind:
+		// Nothing extra
+	default:
+		return fmt.Errorf("invalid sync kind: %s", g.SyncKind)
 	}
 	switch sourceFormat {
 	case configsync.SourceFormatHierarchy:

diff --git a/e2e/nomostest/new.go b/e2e/nomostest/new.go
@@ -67,12 +67,15 @@ func newOptStruct(testName, tmpDir string, ntOptions ...ntopts.Opt) *ntopts.New
 			SyncSources: syncsource.Set{
 				// All tests default to having a RootSync named root-sync using
 				// a Git repository, unless otherwise specified.
-				DefaultRootSyncID: &syncsource.GitSyncSource{},
+				DefaultRootSyncID: &syncsource.GitSyncSource{
+					// RootSync root-sync defaults to using hierarchy.
+					// Use ntopts.SyncWithGitSource with ntopts.Unstructured to change.
+					SourceFormat: configsync.SourceFormatHierarchy,
+				},
 			},
 			// Default to 1m to keep tests fast.
 			// To override, use WithReconcileTimeout.
 			ReconcileTimeout: ptr.To(1 * time.Minute),
-			SourceFormat:     configsync.SourceFormatHierarchy,
 		},
 	}
 	for _, opt := range ntOptions {
@@ -425,30 +428,17 @@ func setupTestCase(nt *NT, opts *ntopts.New) {
 	// init all repositories. This ensures that:
 	// - local repo is initialized and empty
 	// - remote repo exists (except for LocalProvider, which is done in portForwardGitServer)
-	for id, source := range opts.SyncSources.RootSyncs() {
-		switch source.(type) {
+	for id, source := range opts.SyncSources {
+		switch tSource := source.(type) {
 		case *syncsource.GitSyncSource:
 			nt.SyncSources[id] = &syncsource.GitSyncSource{
-				Repository: initRepository(nt, gitproviders.RootRepo, id.ObjectKey, opts.SourceFormat),
+				Repository: initRepository(nt, id.Kind, id.ObjectKey, tSource.SourceFormat),
 			}
 		// case *syncsource.HelmSyncSource:
 		// case *syncsource.OCISyncSource:
 		// TODO: setup OCI & Helm RootSyncs
 		default:
-			nt.T.Fatalf("Invalid %s source %T: %s", id.Kind, source, id.Name)
-		}
-	}
-	for id, source := range opts.SyncSources.RepoSyncs() {
-		switch source.(type) {
-		case *syncsource.GitSyncSource:
-			nt.SyncSources[id] = &syncsource.GitSyncSource{
-				Repository: initRepository(nt, gitproviders.NamespaceRepo, id.ObjectKey, configsync.SourceFormatUnstructured),
-			}
-		// case *syncsource.HelmSyncSource:
-		// case *syncsource.OCISyncSource:
-		// TODO: setup OCI & Helm RootSyncs
-		default:
-			nt.T.Fatalf("Invalid %s source %T: %s", id.Kind, source, id.Name)
+			nt.T.Fatalf("Invalid SyncSource source for %v: %T", id, source)
 		}
 	}
 	// set up port forward if using in-cluster git server
@@ -463,21 +453,10 @@ func setupTestCase(nt *NT, opts *ntopts.New) {
 	// - local repo initialized
 	// - remote repo exists
 	// - port forward started (only applicable to in-cluster git server)
-	for id, source := range opts.SyncSources.RootSyncs() {
-		switch source.(type) {
-		case *syncsource.GitSyncSource:
-			initialCommit(nt, gitproviders.RootRepo, id.ObjectKey, opts.SourceFormat)
-		// case *syncsource.HelmSyncSource:
-		// case *syncsource.OCISyncSource:
-		// TODO: setup OCI & Helm RootSyncs
-		default:
-			nt.T.Fatalf("Invalid %s source %T: %s", id.Kind, source, id.Name)
-		}
-	}
-	for id, source := range opts.SyncSources.RepoSyncs() {
-		switch source.(type) {
+	for id, source := range opts.SyncSources {
+		switch tSource := source.(type) {
 		case *syncsource.GitSyncSource:
-			initialCommit(nt, gitproviders.NamespaceRepo, id.ObjectKey, configsync.SourceFormatUnstructured)
+			initialCommit(nt, id.Kind, id.ObjectKey, tSource.SourceFormat)
 		// case *syncsource.HelmSyncSource:
 		// case *syncsource.OCISyncSource:
 		// TODO: setup OCI & Helm RootSyncs

diff --git a/e2e/nomostest/ntopts/multi_repo.go b/e2e/nomostest/ntopts/multi_repo.go
@@ -27,8 +27,6 @@ import (
 // If NonRootRepos is non-empty, the test is assumed to be running in
 // multi-repo mode.
 type MultiRepo struct {
-	configsync.SourceFormat
-
 	// SyncSources is the set of RootSyncs and RepoSync and their source config
 	// for this test.
 	SyncSources syncsource.Set
@@ -59,12 +57,35 @@ type MultiRepo struct {
 	RepoSyncPermissions []rbacv1.PolicyRule
 }
 
+// GitSourceOption mutates a GitSyncSource
+type GitSourceOption func(*syncsource.GitSyncSource)
+
+// Unstructured mutates a GitSyncSource to use the unstructured SourceFormat.
+func Unstructured(source *syncsource.GitSyncSource) {
+	source.SourceFormat = configsync.SourceFormatUnstructured
+}
+
 // SyncWithGitSource tells the test case that a RootSync or RepoSync should be
 // applied that points to an empty Git Repository.
 // TODO: Add another option that allows specifying an existing Repository
-func SyncWithGitSource(id core.ID) func(opt *New) {
+func SyncWithGitSource(id core.ID, gitOpts ...GitSourceOption) func(opt *New) {
 	return func(opt *New) {
-		opt.SyncSources[id] = &syncsource.GitSyncSource{}
+		source := &syncsource.GitSyncSource{}
+		// Set the default source options
+		switch id.Kind {
+		case configsync.RootSyncKind:
+			// RootSyncs use Hierarchy by default
+			source.SourceFormat = configsync.SourceFormatHierarchy
+		case configsync.RepoSyncKind:
+			// RepoSync only ever use unstructured
+			source.SourceFormat = configsync.SourceFormatUnstructured
+		}
+		// Modify the default source with user specified options
+		for _, gitOpt := range gitOpts {
+			gitOpt(source)
+		}
+		// Register the RSync with the specified source
+		opt.SyncSources[id] = source
 	}
 }
 
@@ -140,8 +161,3 @@ func RepoSyncPermissions(policy ...rbacv1.PolicyRule) Opt {
 		opt.RepoSyncPermissions = append(opt.RepoSyncPermissions, policy...)
 	}
 }
-
-// Unstructured will set the option for unstructured repo.
-func Unstructured(opts *New) {
-	opts.SourceFormat = configsync.SourceFormatUnstructured
-}
diff --git a/e2e/nomostest/reset.go b/e2e/nomostest/reset.go
@@ -418,17 +418,17 @@ func stringSliceContains(list []string, value string) bool {
 }
 
 // ResetRepository creates or re-initializes a remote repository.
-func ResetRepository(nt *NT, repoType gitproviders.RepoType, nn types.NamespacedName, sourceFormat configsync.SourceFormat) *gitproviders.Repository {
-	repo := initRepository(nt, repoType, nn, sourceFormat)
-	initialCommit(nt, repoType, nn, sourceFormat)
+func ResetRepository(nt *NT, syncKind string, nn types.NamespacedName, sourceFormat configsync.SourceFormat) *gitproviders.Repository {
+	repo := initRepository(nt, syncKind, nn, sourceFormat)
+	initialCommit(nt, syncKind, nn, sourceFormat)
 	return repo
 }
 
 // initRepository inits a local repository and ensures its remote exists
-func initRepository(nt *NT, repoType gitproviders.RepoType, nn types.NamespacedName, sourceFormat configsync.SourceFormat) *gitproviders.Repository {
+func initRepository(nt *NT, syncKind string, nn types.NamespacedName, sourceFormat configsync.SourceFormat) *gitproviders.Repository {
 	repo, found := nt.RemoteRepositories[nn]
 	if !found {
-		repo = gitproviders.NewRepository(repoType, nn, sourceFormat, nt.Scheme,
+		repo = gitproviders.NewRepository(syncKind, nn, sourceFormat, nt.Scheme,
 			nt.Logger, nt.GitProvider, nt.TmpDir, nt.gitPrivateKeyPath, nt.DefaultWaitTimeout)
 		if err := repo.Create(); err != nil {
 			nt.T.Fatal(err)
@@ -443,7 +443,7 @@ func initRepository(nt *NT, repoType gitproviders.RepoType, nn types.NamespacedN
 }
 
 // initialCommit creates an initial commit and pushes it to the remote
-func initialCommit(nt *NT, repoType gitproviders.RepoType, nn types.NamespacedName, sourceFormat configsync.SourceFormat) {
+func initialCommit(nt *NT, syncKind string, nn types.NamespacedName, sourceFormat configsync.SourceFormat) {
 	repo, found := nt.RemoteRepositories[nn]
 	if !found {
 		nt.T.Fatal("repo %s not found", nn.String())
@@ -453,12 +453,15 @@ func initialCommit(nt *NT, repoType gitproviders.RepoType, nn types.NamespacedNa
 	}
 	// Reset expected objects.
 	// These are used to offset metrics expectations.
-	if repoType == gitproviders.RootRepo {
+	switch syncKind {
+	case configsync.RootSyncKind:
 		nt.MetricsExpectations.ResetRootSync(nn.Name)
 		nt.MetricsExpectations.AddObjectApply(configsync.RootSyncKind, nn, repo.MustGet(nt.T, repo.SafetyNSPath))
 		nt.MetricsExpectations.AddObjectApply(configsync.RootSyncKind, nn, repo.MustGet(nt.T, repo.SafetyClusterRolePath))
-	} else {
+	case configsync.RepoSyncKind:
 		nt.MetricsExpectations.ResetRepoSync(nn)
+	default:
+		nt.T.Fatalf("Invalid sync kind: %s", syncKind)
 	}
 }
 

diff --git a/e2e/nomostest/syncsource/syncsource.go b/e2e/nomostest/syncsource/syncsource.go
@@ -38,6 +38,7 @@ type GitSyncSource struct {
 	// the current/latest commit.
 	Repository *gitproviders.Repository
 	// TODO: Add SyncPath and Branch/Revision to uniquely identify part of a repo
+	SourceFormat configsync.SourceFormat
 }
 
 // Type returns the SourceType of this source.

diff --git a/e2e/testcases/acme_test.go b/e2e/testcases/acme_test.go
@@ -183,7 +183,8 @@ func TestAcmeCorpRepo(t *testing.T) {
 
 // TestObjectInCMSNamespace will test that user can sync object to CMS namespace
 func TestObjectInCMSNamespace(t *testing.T) {
-	nt := nomostest.New(t, nomostesting.Reconciliation1, ntopts.Unstructured)
+	nt := nomostest.New(t, nomostesting.Reconciliation1,
+		ntopts.SyncWithGitSource(nomostest.DefaultRootSyncID, ntopts.Unstructured))
 
 	rootSyncGitRepo := nt.SyncSourceGitRepository(nomostest.DefaultRootSyncID)
 

diff --git a/e2e/testcases/apiservice_test.go b/e2e/testcases/apiservice_test.go
@@ -31,7 +31,8 @@ const (
 )
 
 func TestCreateAPIServiceAndEndpointInTheSameCommit(t *testing.T) {
-	nt := nomostest.New(t, nomostesting.Reconciliation1, ntopts.Unstructured,
+	nt := nomostest.New(t, nomostesting.Reconciliation1,
+		ntopts.SyncWithGitSource(nomostest.DefaultRootSyncID, ntopts.Unstructured),
 		// Increase the timeout from 1m to 5m to avoid reconcile timeout for the
 		// custom-metrics-stackdriver-adapter Deployment on Autopilot cluster.
 		ntopts.WithReconcileTimeout(5*time.Minute))
@@ -76,7 +77,7 @@ func TestCreateAPIServiceAndEndpointInTheSameCommit(t *testing.T) {
 
 func TestReconcilerResilientToFlakyAPIService(t *testing.T) {
 	nt := nomostest.New(t, nomostesting.Reconciliation1,
-		ntopts.Unstructured,
+		ntopts.SyncWithGitSource(nomostest.DefaultRootSyncID, ntopts.Unstructured),
 		// Increase the timeout from 1m to 5m to avoid reconcile timeout for the
 		// custom-metrics-stackdriver-adapter Deployment on Autopilot cluster.
 		ntopts.WithReconcileTimeout(5*time.Minute))

diff --git a/e2e/testcases/cli_test.go b/e2e/testcases/cli_test.go
@@ -547,17 +547,20 @@ func testSyncFromNomosHydrateOutput(nt *nomostest.NT, config string) {
 }
 
 func TestSyncFromNomosHydrateOutputYAMLDir(t *testing.T) {
-	nt := nomostest.New(t, nomostesting.NomosCLI, ntopts.Unstructured)
+	nt := nomostest.New(t, nomostesting.NomosCLI,
+		ntopts.SyncWithGitSource(nomostest.DefaultRootSyncID, ntopts.Unstructured))
 	testSyncFromNomosHydrateOutput(nt, "../../examples/repo-with-cluster-selectors-compiled/cluster-dev/.")
 }
 
 func TestSyncFromNomosHydrateOutputJSONDir(t *testing.T) {
-	nt := nomostest.New(t, nomostesting.NomosCLI, ntopts.Unstructured)
+	nt := nomostest.New(t, nomostesting.NomosCLI,
+		ntopts.SyncWithGitSource(nomostest.DefaultRootSyncID, ntopts.Unstructured))
 	testSyncFromNomosHydrateOutput(nt, "../../examples/repo-with-cluster-selectors-compiled-json/cluster-dev/.")
 }
 
 func testSyncFromNomosHydrateOutputFlat(t *testing.T, sourceFormat configsync.SourceFormat, outputFormat string) {
-	nt := nomostest.New(t, nomostesting.NomosCLI, ntopts.Unstructured)
+	nt := nomostest.New(t, nomostesting.NomosCLI,
+		ntopts.SyncWithGitSource(nomostest.DefaultRootSyncID, ntopts.Unstructured))
 
 	configPath := fmt.Sprintf("../../examples/%s-repo-with-cluster-selectors", sourceFormat)
 	compiledConfigFile := fmt.Sprintf("%s/compiled.%s", nt.TmpDir, outputFormat)
@@ -1192,8 +1195,8 @@ func TestNomosStatusNameFilter(t *testing.T) {
 	nt := nomostest.New(
 		t,
 		nomostesting.NomosCLI,
-		ntopts.Unstructured,
-		ntopts.SyncWithGitSource(rootSync1ID),
+		ntopts.SyncWithGitSource(nomostest.DefaultRootSyncID, ntopts.Unstructured),
+		ntopts.SyncWithGitSource(rootSync1ID, ntopts.Unstructured),
 		ntopts.RepoSyncPermissions(policy.RepoSyncAdmin()),
 		ntopts.SyncWithGitSource(repoSync1ID),
 		ntopts.SyncWithGitSource(repoSync2ID),

diff --git a/e2e/testcases/composition_test.go b/e2e/testcases/composition_test.go
@@ -80,10 +80,9 @@ func TestComposition(t *testing.T) {
 	lvl0ID := nomostest.DefaultRootSyncID
 	nt := nomostest.New(t,
 		nomostesting.MultiRepos,
-		ntopts.Unstructured,
 		ntopts.WithDelegatedControl,
-		ntopts.RepoSyncPermissions(policy.RepoSyncAdmin(), policy.CoreAdmin()), // NS reconciler manages RepoSyncs and ConfigMaps
-		ntopts.SyncWithGitSource(lvl0ID))
+		ntopts.SyncWithGitSource(lvl0ID, ntopts.Unstructured),
+		ntopts.RepoSyncPermissions(policy.RepoSyncAdmin(), policy.CoreAdmin())) // NS reconciler manages RepoSyncs and ConfigMaps
 
 	lvl0NN := lvl0ID.ObjectKey
 	lvl1NN := nomostest.RootSyncNN("level-1")

diff --git a/e2e/testcases/declared_fields_test.go b/e2e/testcases/declared_fields_test.go
@@ -26,7 +26,8 @@ import (
 )
 
 func TestDeclaredFieldsPod(t *testing.T) {
-	nt := nomostest.New(t, nomostesting.Reconciliation1, ntopts.Unstructured)
+	nt := nomostest.New(t, nomostesting.Reconciliation1,
+		ntopts.SyncWithGitSource(nomostest.DefaultRootSyncID, ntopts.Unstructured))
 	rootSyncGitRepo := nt.SyncSourceGitRepository(nomostest.DefaultRootSyncID)
 
 	namespace := k8sobjects.NamespaceObject("bookstore")

diff --git a/e2e/testcases/gatekeeper_test.go b/e2e/testcases/gatekeeper_test.go
@@ -50,7 +50,8 @@ func emptyConstraintTemplate() unstructured.Unstructured {
 }
 
 func TestConstraintTemplateAndConstraintInSameCommit(t *testing.T) {
-	nt := nomostest.New(t, nomostesting.Reconciliation1, ntopts.Unstructured)
+	nt := nomostest.New(t, nomostesting.Reconciliation1,
+		ntopts.SyncWithGitSource(nomostest.DefaultRootSyncID, ntopts.Unstructured))
 	rootSyncGitRepo := nt.SyncSourceGitRepository(nomostest.DefaultRootSyncID)
 
 	crdName := "k8sallowedrepos.constraints.gatekeeper.sh"

diff --git a/e2e/testcases/gcenode_test.go b/e2e/testcases/gcenode_test.go
@@ -52,9 +52,10 @@ const (
 // https://cloud.google.com/anthos-config-management/docs/how-to/installing-config-sync#git-creds-secret
 func TestGCENodeCSR(t *testing.T) {
 	repoSyncID := core.RepoSyncID(configsync.RepoSyncName, testNs)
-	nt := nomostest.New(t, nomostesting.SyncSource, ntopts.Unstructured,
+	nt := nomostest.New(t, nomostesting.SyncSource,
 		ntopts.RequireGKE(t), ntopts.GCENodeTest,
 		ntopts.RequireCloudSourceRepository(t),
+		ntopts.SyncWithGitSource(nomostest.DefaultRootSyncID, ntopts.Unstructured),
 		ntopts.SyncWithGitSource(repoSyncID),
 		ntopts.RepoSyncPermissions(policy.AllAdmin()), // NS reconciler manages a bunch of resources.
 		ntopts.WithDelegatedControl)
@@ -113,9 +114,10 @@ func TestGCENodeCSR(t *testing.T) {
 //   - `roles/containerregistry.ServiceAgent` for access image in Container Registry.
 func TestGCENodeOCI(t *testing.T) {
 	repoSyncID := core.RepoSyncID(configsync.RepoSyncName, testNs)
-	nt := nomostest.New(t, nomostesting.SyncSource, ntopts.Unstructured,
+	nt := nomostest.New(t, nomostesting.SyncSource,
 		ntopts.RequireGKE(t), ntopts.GCENodeTest,
 		ntopts.RequireOCIArtifactRegistry(t),
+		ntopts.SyncWithGitSource(nomostest.DefaultRootSyncID, ntopts.Unstructured),
 		ntopts.SyncWithGitSource(repoSyncID),
 		ntopts.RepoSyncPermissions(policy.AllAdmin()), // NS reconciler manages a bunch of resources.
 		ntopts.WithDelegatedControl)
@@ -189,9 +191,10 @@ func TestGCENodeOCI(t *testing.T) {
 //   - `roles/artifactregistry.reader` for access image in Artifact Registry.
 func TestGCENodeHelm(t *testing.T) {
 	repoSyncID := core.RepoSyncID(configsync.RepoSyncName, testNs)
-	nt := nomostest.New(t, nomostesting.SyncSource, ntopts.Unstructured,
+	nt := nomostest.New(t, nomostesting.SyncSource,
 		ntopts.RequireGKE(t), ntopts.GCENodeTest,
 		ntopts.RequireHelmArtifactRegistry(t),
+		ntopts.SyncWithGitSource(nomostest.DefaultRootSyncID, ntopts.Unstructured),
 		ntopts.SyncWithGitSource(repoSyncID),
 		ntopts.RepoSyncPermissions(policy.AllAdmin()), // NS reconciler manages a bunch of resources.
 		ntopts.WithDelegatedControl)