fix(sync): create namespace before dry-run (#731)

Author: agaudreault

pkg/sync/sync_context.go

@@ -430,10 +430,25 @@ func (sc *syncContext) Sync() {
 		// to perform the sync. we only wish to do this once per operation, performing additional dry-runs
 		// is harmless, but redundant. The indicator we use to detect if we have already performed
 		// the dry-run for this operation, is if the resource or hook list is empty.
-
 		dryRunTasks := tasks
+
+		// Before doing any validation, we have to create the application namespace if it does not exist.
+		// The validation is expected to fail in multiple scenarios if a namespace does not exist.
+		if nsCreateTask := sc.getNamespaceCreationTask(dryRunTasks); nsCreateTask != nil {
+			nsSyncTasks := syncTasks{nsCreateTask}
+			// No need to perform a dry-run on the namespace creation, because if it fails we stop anyway
+			sc.log.WithValues("task", nsCreateTask).Info("Creating namespace")
+			if sc.runTasks(nsSyncTasks, false) == failed {
+				sc.setOperationFailed(syncTasks{}, nsSyncTasks, "the namespace failed to apply")
+				return
+			}
+
+			// The namespace was created, we can remove this task from the dry-run
+			dryRunTasks = tasks.Filter(func(t *syncTask) bool { return t != nsCreateTask })
+		}
+
 		if sc.applyOutOfSyncOnly {
-			dryRunTasks = sc.filterOutOfSyncTasks(tasks)
+			dryRunTasks = sc.filterOutOfSyncTasks(dryRunTasks)
 		}
 
 		sc.log.WithValues("tasks", dryRunTasks).Info("Tasks (dry-run)")
@@ -608,6 +623,18 @@ func (sc *syncContext) filterOutOfSyncTasks(tasks syncTasks) syncTasks {
 	})
 }
 
+// getNamespaceCreationTask returns a task that will create the current namespace
+// or nil if the syncTasks does not contain one
+func (sc *syncContext) getNamespaceCreationTask(tasks syncTasks) *syncTask {
+	creationTasks := tasks.Filter(func(task *syncTask) bool {
+		return task.liveObj == nil && isNamespaceWithName(task.targetObj, sc.namespace)
+	})
+	if len(creationTasks) > 0 {
+		return creationTasks[0]
+	}
+	return nil
+}
+
 func (sc *syncContext) removeHookFinalizer(task *syncTask) error {
 	if task.liveObj == nil {
 		return nil
@@ -961,12 +988,11 @@ func (sc *syncContext) autoCreateNamespace(tasks syncTasks) syncTasks {
 			case err == nil:
 				nsTask := &syncTask{phase: common.SyncPhasePreSync, targetObj: managedNs, liveObj: liveObj}
 				_, ok := sc.syncRes[nsTask.resultKey()]
-				if ok {
-					tasks = sc.appendNsTask(tasks, nsTask, managedNs, liveObj)
-				} else if liveObj != nil {
+				if !ok && liveObj != nil {
 					sc.log.WithValues("namespace", sc.namespace).Info("Namespace already exists")
-					tasks = sc.appendNsTask(tasks, &syncTask{phase: common.SyncPhasePreSync, targetObj: managedNs, liveObj: liveObj}, managedNs, liveObj)
 				}
+				tasks = sc.appendNsTask(tasks, nsTask, managedNs, liveObj)
+
 			case apierrors.IsNotFound(err):
 				tasks = sc.appendNsTask(tasks, &syncTask{phase: common.SyncPhasePreSync, targetObj: managedNs, liveObj: nil}, managedNs, nil)
 			default:

pkg/sync/sync_context_test.go

@@ -121,6 +121,52 @@ func TestSyncNotPermittedNamespace(t *testing.T) {
 	assert.Contains(t, resources[0].Message, "not permitted in project")
 }
 
+func TestSyncNamespaceCreatedBeforeDryRunWithoutFailure(t *testing.T) {
+	pod := testingutils.NewPod()
+	syncCtx := newTestSyncCtx(nil, WithNamespaceModifier(func(_, _ *unstructured.Unstructured) (bool, error) {
+		return true, nil
+	}))
+	syncCtx.resources = groupResources(ReconciliationResult{
+		Live:   []*unstructured.Unstructured{nil, nil},
+		Target: []*unstructured.Unstructured{pod},
+	})
+	syncCtx.Sync()
+	phase, msg, resources := syncCtx.GetState()
+	assert.Equal(t, synccommon.OperationRunning, phase)
+	assert.Equal(t, "waiting for healthy state of /Namespace/fake-argocd-ns", msg)
+	require.Len(t, resources, 1)
+	assert.Equal(t, "Namespace", resources[0].ResourceKey.Kind)
+	assert.Equal(t, synccommon.ResultCodeSynced, resources[0].Status)
+}
+
+func TestSyncNamespaceCreatedBeforeDryRunWithFailure(t *testing.T) {
+	pod := testingutils.NewPod()
+	syncCtx := newTestSyncCtx(nil, WithNamespaceModifier(func(_, _ *unstructured.Unstructured) (bool, error) {
+		return true, nil
+	}), func(ctx *syncContext) {
+		resourceOps := ctx.resourceOps.(*kubetest.MockResourceOps)
+		resourceOps.Commands = map[string]kubetest.KubectlOutput{}
+		resourceOps.Commands[pod.GetName()] = kubetest.KubectlOutput{
+			Output: "should not be returned",
+			Err:    errors.New("invalid object failing dry-run"),
+		}
+	})
+	syncCtx.resources = groupResources(ReconciliationResult{
+		Live:   []*unstructured.Unstructured{nil, nil},
+		Target: []*unstructured.Unstructured{pod},
+	})
+	syncCtx.Sync()
+	phase, msg, resources := syncCtx.GetState()
+	assert.Equal(t, synccommon.OperationFailed, phase)
+	assert.Equal(t, "one or more objects failed to apply (dry run)", msg)
+	require.Len(t, resources, 2)
+	assert.Equal(t, "Namespace", resources[0].ResourceKey.Kind)
+	assert.Equal(t, synccommon.ResultCodeSynced, resources[0].Status)
+	assert.Equal(t, "Pod", resources[1].ResourceKey.Kind)
+	assert.Equal(t, synccommon.ResultCodeSyncFailed, resources[1].Status)
+	assert.Equal(t, "invalid object failing dry-run", resources[1].Message)
+}
+
 func TestSyncCreateInSortedOrder(t *testing.T) {
 	syncCtx := newTestSyncCtx(nil)
 	syncCtx.resources = groupResources(ReconciliationResult{
@@ -1045,7 +1091,7 @@ func TestNamespaceAutoCreation(t *testing.T) {
 
 	// Namespace auto creation pre-sync task should not be there
 	// since there is namespace resource in syncCtx.resources
-	t.Run("no pre-sync task 1", func(t *testing.T) {
+	t.Run("no pre-sync task if resource is managed", func(t *testing.T) {
 		syncCtx.resources = groupResources(ReconciliationResult{
 			Live:   []*unstructured.Unstructured{nil},
 			Target: []*unstructured.Unstructured{namespace},
@@ -1057,23 +1103,21 @@ func TestNamespaceAutoCreation(t *testing.T) {
 		assert.NotContains(t, tasks, task)
 	})
 
-	// Namespace auto creation pre-sync task should not be there
-	// since there is no existing sync result
-	t.Run("no pre-sync task 2", func(t *testing.T) {
+	// Namespace auto creation pre-sync task should be there when it is not managed
+	t.Run("pre-sync task when resource is not managed", func(t *testing.T) {
 		syncCtx.resources = groupResources(ReconciliationResult{
 			Live:   []*unstructured.Unstructured{nil},
 			Target: []*unstructured.Unstructured{pod},
 		})
 		tasks, successful := syncCtx.getSyncTasks()
 
 		assert.True(t, successful)
-		assert.Len(t, tasks, 1)
-		assert.NotContains(t, tasks, task)
+		assert.Len(t, tasks, 2)
+		assert.Contains(t, tasks, task)
 	})
 
-	// Namespace auto creation pre-sync task should be there
-	// since there is existing sync result which means that task created this namespace
-	t.Run("pre-sync task created", func(t *testing.T) {
+	// Namespace auto creation pre-sync task should be there after sync
+	t.Run("pre-sync task when resource is not managed with existing sync", func(t *testing.T) {
 		syncCtx.resources = groupResources(ReconciliationResult{
 			Live:   []*unstructured.Unstructured{nil},
 			Target: []*unstructured.Unstructured{pod},
@@ -1100,24 +1144,13 @@ func TestNamespaceAutoCreation(t *testing.T) {
 
 	// Namespace auto creation pre-sync task not should be there
 	// since there is no namespace modifier present
-	t.Run("no pre-sync task created", func(t *testing.T) {
+	t.Run("no pre-sync task created if no modifier", func(t *testing.T) {
 		syncCtx.resources = groupResources(ReconciliationResult{
 			Live:   []*unstructured.Unstructured{nil},
 			Target: []*unstructured.Unstructured{pod},
 		})
-		syncCtx.syncNamespace = nil
 
-		res := synccommon.ResourceSyncResult{
-			ResourceKey: kube.GetResourceKey(task.obj()),
-			Version:     task.version(),
-			Status:      task.syncStatus,
-			Message:     task.message,
-			HookType:    task.hookType(),
-			HookPhase:   task.operationState,
-			SyncPhase:   task.phase,
-		}
-		syncCtx.syncRes = map[string]synccommon.ResourceSyncResult{}
-		syncCtx.syncRes[task.resultKey()] = res
+		syncCtx.syncNamespace = nil
 
 		tasks, successful := syncCtx.getSyncTasks()