From 9d242c54f061485d25f4a65a668853784ebc9cb1 Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Tue, 20 Dec 2022 16:56:39 +0000 Subject: [PATCH] Add feature gate GitAllBranchReferences The new feature gate enables users to toggle the download of all branch head references when push branches are configured. Tests were refactored to ensure that they are feature gate sensitive. Signed-off-by: Paulo Gomes --- .../imageupdateautomation_controller.go | 13 ++- controllers/suite_test.go | 6 - controllers/update_test.go | 106 +++++++++++++----- internal/features/features.go | 7 ++ 4 files changed, 98 insertions(+), 34 deletions(-) diff --git a/controllers/imageupdateautomation_controller.go b/controllers/imageupdateautomation_controller.go index 0a31b97..efa300c 100644 --- a/controllers/imageupdateautomation_controller.go +++ b/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...) diff --git a/controllers/suite_test.go b/controllers/suite_test.go index a0be0a6..675250c 100644 --- a/controllers/suite_test.go +++ b/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) } diff --git a/controllers/update_test.go b/controllers/update_test.go index a2953fc..45af770 100644 --- a/controllers/update_test.go +++ b/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,11 +1460,12 @@ 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 @@ -1424,7 +1473,6 @@ func createGitRepository(kClient client.Client, name, namespace, repoURL, secret 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 diff --git a/internal/features/features.go b/internal/features/features.go index 115a17b..a320052 100644 --- a/internal/features/features.go +++ b/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