Merge pull request #469 from fluxcd/single-branch

Add feature gate `GitAllBranchReferences`

Author: Paulo Gomes

controllers/imageupdateautomation_controller.go

@@ -261,15 +261,24 @@ func (r *ImageUpdateAutomationReconciler) Reconcile(ctx context.Context, req ctr
 	}
 
 	clientOpts := []gogit.ClientOption{gogit.WithDiskStorage()}
-	forcePush, _ := features.Enabled(features.GitForcePushBranch)
+	forcePush := r.features[features.GitForcePushBranch]
 	if forcePush && pushBranch != ref.Branch {
 		clientOpts = append(clientOpts, gogit.WithForcePush())
 	}
 	if authOpts.Transport == git.HTTP {
 		clientOpts = append(clientOpts, gogit.WithInsecureCredentialsOverHTTP())
 	}
+
+	// If the push branch is different from the checkout ref, we need to
+	// have all the references downloaded at clone time, to ensure that
+	// SwitchBranch will have access to the target branch state. fluxcd/flux2#3384
+	//
+	// To always overwrite the push branch, the feature gate
+	// GitAllBranchReferences can be set to false, which will cause
+	// the SwitchBranch operation to ignore the remote branch state.
+	allReferences := r.features[features.GitAllBranchReferences]
 	if pushBranch != ref.Branch {
-		clientOpts = append(clientOpts, gogit.WithSingleBranch(false))
+		clientOpts = append(clientOpts, gogit.WithSingleBranch(!allReferences))
 	}
 
 	gitClient, err := gogit.NewClient(tmp, authOpts, clientOpts...)

controllers/suite_test.go

@@ -33,7 +33,6 @@ import (
 	sourcev1 "github.com/fluxcd/source-controller/api/v1beta2"
 
 	imagev1 "github.com/fluxcd/image-automation-controller/api/v1beta1"
-	"github.com/fluxcd/image-automation-controller/internal/features"
 	// +kubebuilder:scaffold:imports
 )
 
@@ -60,13 +59,8 @@ func TestMain(m *testing.M) {
 	code := runTestsWithFeatures(m, nil)
 	if code != 0 {
 		fmt.Println("failed with default feature values")
-		os.Exit(code)
 	}
 
-	code = runTestsWithFeatures(m, map[string]bool{
-		features.GitShallowClone: true,
-	})
-
 	os.Exit(code)
 }
 

controllers/update_test.go

@@ -62,6 +62,7 @@ import (
 	sourcev1 "github.com/fluxcd/source-controller/api/v1beta2"
 
 	imagev1 "github.com/fluxcd/image-automation-controller/api/v1beta1"
+	"github.com/fluxcd/image-automation-controller/internal/features"
 	"github.com/fluxcd/image-automation-controller/pkg/test"
 	"github.com/fluxcd/image-automation-controller/pkg/update"
 )
@@ -426,7 +427,7 @@ func TestImageAutomationReconciler_signedCommit(t *testing.T) {
 func TestImageAutomationReconciler_e2e(t *testing.T) {
 	protos := []string{"http", "ssh"}
 
-	testFunc := func(t *testing.T, proto, clientName string) {
+	testFunc := func(t *testing.T, proto string, feats map[string]bool) {
 		g := NewWithT(t)
 
 		const latestImage = "helloworld:1.0.1"
@@ -441,8 +442,18 @@ func TestImageAutomationReconciler_e2e(t *testing.T) {
 			Strategy: imagev1.UpdateStrategySetters,
 		}
 
+		controllerName := "image-automation-controller"
+		// Create ImagePolicy and ImageUpdateAutomation resource for each of the
+		// test cases and cleanup at the end.
+		builder := fakeclient.NewClientBuilder().WithScheme(testEnv.Scheme())
+		r := &ImageUpdateAutomationReconciler{
+			Client:        builder.Build(),
+			EventRecorder: testEnv.GetEventRecorderFor(controllerName),
+			features:      feats,
+		}
+
 		// Create a test namespace.
-		nsCleanup, err := createNamespace(testEnv, namespace)
+		nsCleanup, err := createNamespace(r.Client, namespace)
 		g.Expect(err).ToNot(HaveOccurred(), "failed to create test namespace")
 		defer func() {
 			g.Expect(nsCleanup()).To(Succeed())
@@ -478,12 +489,12 @@ func TestImageAutomationReconciler_e2e(t *testing.T) {
 		if proto == "ssh" {
 			// SSH requires an identity (private key) and known_hosts file
 			// in a secret.
-			err = createSSHIdentitySecret(testEnv, gitSecretName, namespace, repoURL)
+			err = createSSHIdentitySecret(r.Client, gitSecretName, namespace, repoURL)
 			g.Expect(err).ToNot(HaveOccurred())
-			err = createGitRepository(testEnv, gitRepoName, namespace, repoURL, gitSecretName, clientName)
+			err = createGitRepository(r.Client, gitRepoName, namespace, repoURL, gitSecretName)
 			g.Expect(err).ToNot(HaveOccurred())
 		} else {
-			err = createGitRepository(testEnv, gitRepoName, namespace, repoURL, "", clientName)
+			err = createGitRepository(r.Client, gitRepoName, namespace, repoURL, "")
 			g.Expect(err).ToNot(HaveOccurred())
 		}
 
@@ -493,9 +504,6 @@ func TestImageAutomationReconciler_e2e(t *testing.T) {
 			Namespace: namespace,
 		}
 
-		// Create ImagePolicy and ImageUpdateAutomation resource for each of the
-		// test cases and cleanup at the end.
-
 		t.Run("PushSpec", func(t *testing.T) {
 			g := NewWithT(t)
 
@@ -507,16 +515,21 @@ func TestImageAutomationReconciler_e2e(t *testing.T) {
 
 			// NB not testing the image reflector controller; this
 			// will make a "fully formed" ImagePolicy object.
-			err = createImagePolicyWithLatestImage(testEnv, imagePolicyName, namespace, "not-expected-to-exist", "1.x", latestImage)
+			err = createImagePolicyWithLatestImage(r.Client, imagePolicyName, namespace, "not-expected-to-exist", "1.x", latestImage)
 			g.Expect(err).ToNot(HaveOccurred(), "failed to create ImagePolicy resource")
 
 			defer func() {
-				g.Expect(deleteImagePolicy(testEnv, imagePolicyName, namespace)).ToNot(HaveOccurred())
+				g.Expect(deleteImagePolicy(r.Client, imagePolicyName, namespace)).ToNot(HaveOccurred())
 			}()
 
 			imageUpdateAutomationName := "update-" + randStringRunes(5)
 			pushBranch := "pr-" + randStringRunes(5)
 
+			automationKey := types.NamespacedName{
+				Name:      imageUpdateAutomationName,
+				Namespace: namespace,
+			}
+
 			t.Run("update with PushSpec", func(t *testing.T) {
 				g := NewWithT(t)
 
@@ -531,9 +544,12 @@ func TestImageAutomationReconciler_e2e(t *testing.T) {
 
 				// Now create the automation object, and let it (one
 				// hopes!) make a commit itself.
-				err = createImageUpdateAutomation(testEnv, imageUpdateAutomationName, namespace, gitRepoName, namespace, branch, pushBranch, commitMessage, "", updateStrategy)
+				err = createImageUpdateAutomation(r.Client, imageUpdateAutomationName, namespace, gitRepoName, namespace, branch, pushBranch, commitMessage, "", updateStrategy)
 				g.Expect(err).ToNot(HaveOccurred())
 
+				_, err = r.Reconcile(context.TODO(), ctrl.Request{NamespacedName: automationKey})
+				g.Expect(err).To(BeNil())
+
 				initialHead, err := headFromBranch(localRepo, branch)
 				g.Expect(err).ToNot(HaveOccurred())
 
@@ -555,6 +571,10 @@ func TestImageAutomationReconciler_e2e(t *testing.T) {
 			})
 
 			t.Run("push branch gets updated", func(t *testing.T) {
+				if !feats[features.GitAllBranchReferences] {
+					t.Skip("GitAllBranchReferences feature not enabled")
+				}
+
 				g := NewWithT(t)
 
 				initialHead, err := headFromBranch(localRepo, branch)
@@ -569,9 +589,12 @@ func TestImageAutomationReconciler_e2e(t *testing.T) {
 
 				// Update the policy and expect another commit in the push
 				// branch.
-				err = updateImagePolicyWithLatestImage(testEnv, imagePolicyName, namespace, "helloworld:v1.3.0")
+				err = updateImagePolicyWithLatestImage(r.Client, imagePolicyName, namespace, "helloworld:v1.3.0")
 				g.Expect(err).ToNot(HaveOccurred())
 
+				_, err = r.Reconcile(context.TODO(), ctrl.Request{NamespacedName: automationKey})
+				g.Expect(err).To(BeNil())
+
 				waitForNewHead(g, localRepo, pushBranch, preChangeCommitId)
 
 				head, err = getRemoteHead(localRepo, pushBranch)
@@ -586,6 +609,10 @@ func TestImageAutomationReconciler_e2e(t *testing.T) {
 			})
 
 			t.Run("still pushes to the push branch after it's merged", func(t *testing.T) {
+				if !feats[features.GitAllBranchReferences] {
+					t.Skip("GitAllBranchReferences feature not enabled")
+				}
+
 				g := NewWithT(t)
 
 				initialHead, err := headFromBranch(localRepo, branch)
@@ -617,9 +644,12 @@ func TestImageAutomationReconciler_e2e(t *testing.T) {
 
 				// Update the policy and expect another commit in the push
 				// branch.
-				err = updateImagePolicyWithLatestImage(testEnv, imagePolicyName, namespace, "helloworld:v1.3.1")
+				err = updateImagePolicyWithLatestImage(r.Client, imagePolicyName, namespace, "helloworld:v1.3.1")
 				g.Expect(err).ToNot(HaveOccurred())
 
+				_, err = r.Reconcile(context.TODO(), ctrl.Request{NamespacedName: automationKey})
+				g.Expect(err).To(BeNil())
+
 				waitForNewHead(g, localRepo, pushBranch, preChangeCommitId)
 
 				head, err = getRemoteHead(localRepo, pushBranch)
@@ -634,7 +664,7 @@ func TestImageAutomationReconciler_e2e(t *testing.T) {
 			})
 
 			// Cleanup the image update automation used above.
-			g.Expect(deleteImageUpdateAutomation(testEnv, imageUpdateAutomationName, namespace)).To(Succeed())
+			g.Expect(deleteImageUpdateAutomation(r.Client, imageUpdateAutomationName, namespace)).To(Succeed())
 		})
 
 		t.Run("with update strategy setters", func(t *testing.T) {
@@ -652,11 +682,11 @@ func TestImageAutomationReconciler_e2e(t *testing.T) {
 			g.Expect(err).ToNot(HaveOccurred(), "failed to clone git repo")
 
 			g.Expect(checkoutBranch(localRepo, branch)).ToNot(HaveOccurred())
-			err = createImagePolicyWithLatestImage(testEnv, imagePolicyName, namespace, "not-expected-to-exist", "1.x", latestImage)
+			err = createImagePolicyWithLatestImage(r.Client, imagePolicyName, namespace, "not-expected-to-exist", "1.x", latestImage)
 			g.Expect(err).ToNot(HaveOccurred(), "failed to create ImagePolicy resource")
 
 			defer func() {
-				g.Expect(deleteImagePolicy(testEnv, imagePolicyName, namespace)).ToNot(HaveOccurred())
+				g.Expect(deleteImagePolicy(r.Client, imagePolicyName, namespace)).ToNot(HaveOccurred())
 			}()
 
 			preChangeCommitId := commitIdFromBranch(localRepo, branch)
@@ -679,12 +709,15 @@ func TestImageAutomationReconciler_e2e(t *testing.T) {
 				Namespace: namespace,
 				Name:      "update-" + randStringRunes(5),
 			}
-			err = createImageUpdateAutomation(testEnv, updateKey.Name, namespace, gitRepoName, namespace, branch, "", commitMessage, "", updateStrategy)
+			err = createImageUpdateAutomation(r.Client, updateKey.Name, namespace, gitRepoName, namespace, branch, "", commitMessage, "", updateStrategy)
 			g.Expect(err).ToNot(HaveOccurred())
 			defer func() {
-				g.Expect(deleteImageUpdateAutomation(testEnv, updateKey.Name, namespace)).To(Succeed())
+				g.Expect(deleteImageUpdateAutomation(r.Client, updateKey.Name, namespace)).To(Succeed())
 			}()
 
+			_, err = r.Reconcile(context.TODO(), ctrl.Request{NamespacedName: updateKey})
+			g.Expect(err).To(BeNil())
+
 			// Wait for a new commit to be made by the controller.
 			waitForNewHead(g, localRepo, branch, preChangeCommitId)
 
@@ -696,7 +729,7 @@ func TestImageAutomationReconciler_e2e(t *testing.T) {
 			g.Expect(commit.Message).To(Equal(commitMessage))
 
 			var newObj imagev1.ImageUpdateAutomation
-			g.Expect(testEnv.Get(context.Background(), updateKey, &newObj)).To(Succeed())
+			g.Expect(r.Client.Get(context.Background(), updateKey, &newObj)).To(Succeed())
 			g.Expect(newObj.Status.LastPushCommit).To(Equal(commit.Hash.String()))
 			g.Expect(newObj.Status.LastPushTime).ToNot(BeNil())
 
@@ -708,6 +741,12 @@ func TestImageAutomationReconciler_e2e(t *testing.T) {
 		t.Run("no reconciliation when object is suspended", func(t *testing.T) {
 			g := NewWithT(t)
 
+			nsCleanup, err := createNamespace(testEnv, namespace)
+			g.Expect(err).ToNot(HaveOccurred(), "failed to create test namespace")
+			defer func() {
+				g.Expect(nsCleanup()).To(Succeed())
+			}()
+
 			err = createImagePolicyWithLatestImage(testEnv, imagePolicyName, namespace, "not-expected-to-exist", "1.x", latestImage)
 			g.Expect(err).ToNot(HaveOccurred(), "failed to create ImagePolicy resource")
 
@@ -726,6 +765,9 @@ func TestImageAutomationReconciler_e2e(t *testing.T) {
 				g.Expect(deleteImageUpdateAutomation(testEnv, updateKey.Name, namespace)).To(Succeed())
 			}()
 
+			_, err = r.Reconcile(context.TODO(), ctrl.Request{NamespacedName: updateKey})
+			g.Expect(err).To(BeNil())
+
 			// Wait for the object to be available in the cache before
 			// attempting update.
 			g.Eventually(func() bool {
@@ -775,10 +817,16 @@ func TestImageAutomationReconciler_e2e(t *testing.T) {
 		})
 	}
 
-	for _, proto := range protos {
-		t.Run(fmt.Sprintf("%s/%s", gogit.ClientName, proto), func(t *testing.T) {
-			testFunc(t, proto, gogit.ClientName)
-		})
+	for _, enabled := range []bool{true, false} {
+		feats := features.FeatureGates()
+		for k := range feats {
+			feats[k] = enabled
+		}
+		for _, proto := range protos {
+			t.Run(fmt.Sprintf("%s/features=%t", proto, enabled), func(t *testing.T) {
+				testFunc(t, proto, feats)
+			})
+		}
 	}
 }
 
@@ -1361,7 +1409,7 @@ func testWithCustomRepoAndImagePolicy(
 	g.Expect(err).ToNot(HaveOccurred(), "failed to create new remote origin")
 
 	// Create GitRepository resource for the above repo.
-	err = createGitRepository(kClient, args.gitRepoName, args.gitRepoNamespace, repoURL, "", gitImpl)
+	err = createGitRepository(kClient, args.gitRepoName, args.gitRepoNamespace, repoURL, "")
 	g.Expect(err).ToNot(HaveOccurred(), "failed to create GitRepository resource")
 
 	// Create ImagePolicy with populated latest image in the status.
@@ -1412,19 +1460,19 @@ func createNamespace(kClient client.Client, name string) (cleanup, error) {
 	return cleanup, nil
 }
 
-func createGitRepository(kClient client.Client, name, namespace, repoURL, secretRef, gitImpl string) error {
+func createGitRepository(kClient client.Client, name, namespace, repoURL, secretRef string) error {
 	gitRepo := &sourcev1.GitRepository{
 		Spec: sourcev1.GitRepositorySpec{
 			URL:      repoURL,
 			Interval: metav1.Duration{Duration: time.Minute},
+			Timeout:  &metav1.Duration{Duration: time.Minute},
 		},
 	}
 	gitRepo.Name = name
 	gitRepo.Namespace = namespace
 	if secretRef != "" {
 		gitRepo.Spec.SecretRef = &meta.LocalObjectReference{Name: secretRef}
 	}
-	gitRepo.Spec.GitImplementation = gitImpl
 	return kClient.Create(context.Background(), gitRepo)
 }
 
@@ -1580,6 +1628,12 @@ func createSSHIdentitySecret(kClient client.Client, name, namespace, repoURL str
 			"identity":     string(pair.PrivateKey),
 			"identity.pub": string(pair.PublicKey),
 		},
+		// Without KAS, StringData and Data must be kept in sync manually.
+		Data: map[string][]byte{
+			"known_hosts":  knownhosts,
+			"identity":     pair.PrivateKey,
+			"identity.pub": pair.PublicKey,
+		},
 	}
 	sec.Name = name
 	sec.Namespace = namespace

internal/features/features.go

@@ -28,6 +28,9 @@ const (
 	// GitShallowClone enables the use of shallow clones when pulling source from
 	// Git repositories.
 	GitShallowClone = "GitShallowClone"
+	// GitAllBranchReferences enables the download of all branch head references
+	// when push branches are configured. When enabled fixes fluxcd/flux2#3384.
+	GitAllBranchReferences = "GitAllBranchReferences"
 )
 
 var features = map[string]bool{
@@ -38,6 +41,10 @@ var features = map[string]bool{
 	// GitShallowClone
 	// opt-out from v0.28
 	GitShallowClone: true,
+
+	// GitAllBranchReferences
+	// opt-out from v0.28
+	GitAllBranchReferences: true,
 }
 
 // DefaultFeatureGates contains a list of all supported feature gates and