fix: handle annotated git tags correctly in repo server cache (argoproj#21771)

Signed-off-by: Atif Ali <[email protected]>

Author: aali309

test/e2e/fixture/app/actions.go

@@ -78,6 +78,18 @@ func (a *Actions) AddTag(name string) *Actions {
 	return a
 }
 
+func (a *Actions) AddAnnotatedTag(name string, message string) *Actions {
+	a.context.t.Helper()
+	fixture.AddAnnotatedTag(a.context.t, name, message)
+	return a
+}
+
+func (a *Actions) AddTagWithForce(name string) *Actions {
+	a.context.t.Helper()
+	fixture.AddTagWithForce(a.context.t, name)
+	return a
+}
+
 func (a *Actions) RemoveSubmodule() *Actions {
 	a.context.t.Helper()
 	fixture.RemoveSubmodule()

test/e2e/fixture/fixture.go

@@ -861,6 +861,25 @@ func AddTag(name string) {
 	}
 }
 
+func AddTagWithForce(t *testing.T, name string) {
+	t.Helper()
+	prevGnuPGHome := os.Getenv("GNUPGHOME")
+	t.Setenv("GNUPGHOME", TmpDir+"/gpg")
+	defer t.Setenv("GNUPGHOME", prevGnuPGHome)
+	errors.NewHandler(t).FailOnErr(Run(repoDirectory(), "git", "tag", "-f", name))
+	if IsRemote() {
+		errors.NewHandler(t).FailOnErr(Run(repoDirectory(), "git", "push", "--tags", "-f", "origin", "master"))
+	}
+}
+
+func AddAnnotatedTag(t *testing.T, name string, message string) {
+	t.Helper()
+	errors.NewHandler(t).FailOnErr(Run(repoDirectory(), "git", "tag", "-f", "-a", name, "-m", message))
+	if IsRemote() {
+		errors.NewHandler(t).FailOnErr(Run(repoDirectory(), "git", "push", "--tags", "-f", "origin", "master"))
+	}
+}
+
 // create the resource by creating using "kubectl apply", with bonus templating
 func Declarative(filename string, values interface{}) (string, error) {
 	bytes, err := os.ReadFile(path.Join("testdata", filename))

test/e2e/git_test.go

@@ -1,15 +1,22 @@
 package e2e
 
 import (
+	"context"
 	"strings"
 	"testing"
+	"time"
 
 	v1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/apimachinery/pkg/util/wait"
 
-	"github.com/argoproj/argo-cd/v2/test/e2e/fixture"
+	"github.com/stretchr/testify/require"
 
 	. "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
+	"github.com/argoproj/argo-cd/v2/test/e2e/fixture"
 	. "github.com/argoproj/argo-cd/v2/test/e2e/fixture/app"
+	"github.com/argoproj/argo-cd/v2/util/errors"
 )
 
 func TestGitSemverResolutionNotUsingConstraint(t *testing.T) {
@@ -51,3 +58,145 @@ func TestGitSemverResolutionUsingConstraint(t *testing.T) {
 		Expect(SyncStatusIs(SyncStatusCodeSynced)).
 		Expect(Pod(func(p v1.Pod) bool { return strings.HasPrefix(p.Name, "new-app") }))
 }
+
+func TestAnnotatedTagInStatusSyncRevision(t *testing.T) {
+	Given(t).
+		Path(guestbookPath).
+		When().
+		// Create annotated tag name 'annotated-tag'
+		AddAnnotatedTag("annotated-tag", "my-generic-tag-message").
+		// Create Application targeting annotated-tag, with automatedSync: true
+		CreateFromFile(func(app *Application) {
+			app.Spec.Source.TargetRevision = "annotated-tag"
+			app.Spec.SyncPolicy = &SyncPolicy{Automated: &SyncPolicyAutomated{Prune: true, SelfHeal: false}}
+		}).
+		Then().
+		Expect(SyncStatusIs(SyncStatusCodeSynced)).
+		And(func(app *Application) {
+			annotatedTagIDOutput, err := fixture.Run(fixture.TmpDir+"/testdata.git", "git", "show-ref", "annotated-tag")
+			require.NoError(t, err)
+			require.NotEmpty(t, annotatedTagIDOutput)
+			// example command output:
+			// "569798c430515ffe170bdb23e3aafaf8ae24b9ff refs/tags/annotated-tag"
+			annotatedTagIDFields := strings.Fields(string(annotatedTagIDOutput))
+			require.Len(t, annotatedTagIDFields, 2)
+
+			targetCommitID, err := fixture.Run(fixture.TmpDir+"/testdata.git", "git", "rev-parse", "--verify", "annotated-tag^{commit}")
+			// example command output:
+			// "bcd35965e494273355265b9f0bf85075b6bc5163"
+			require.NoError(t, err)
+			require.NotEmpty(t, targetCommitID)
+
+			require.NotEmpty(t, app.Status.Sync.Revision, "revision in sync status should be set by sync operation")
+
+			require.NotEqual(t, app.Status.Sync.Revision, annotatedTagIDFields[0], "revision should not match the annotated tag id")
+			require.Equal(t, app.Status.Sync.Revision, strings.TrimSpace(string(targetCommitID)), "revision SHOULD match the target commit SHA")
+		})
+}
+
+// Test updates to K8s resources should not trigger a self-heal when self-heal is false.
+func TestAutomatedSelfHealingAgainstAnnotatedTag(t *testing.T) {
+	Given(t).
+		Path(guestbookPath).
+		When().
+		AddAnnotatedTag("annotated-tag", "my-generic-tag-message").
+		// App should be auto-synced once created
+		CreateFromFile(func(app *Application) {
+			app.Spec.Source.TargetRevision = "annotated-tag"
+			app.Spec.SyncPolicy = &SyncPolicy{Automated: &SyncPolicyAutomated{Prune: true, SelfHeal: false}}
+		}).
+		Then().
+		Expect(SyncStatusIs(SyncStatusCodeSynced)).
+		ExpectConsistently(SyncStatusIs(SyncStatusCodeSynced), WaitDuration, time.Second*10).
+		When().
+		// Update the annotated tag to a new git commit, that has a new revisionHistoryLimit.
+		PatchFile("guestbook-ui-deployment.yaml", `[{"op": "replace", "path": "/spec/revisionHistoryLimit", "value": 10}]`).
+		AddAnnotatedTag("annotated-tag", "my-generic-tag-message").
+		Refresh(RefreshTypeHard).
+		// The Application should update to the new annotated tag value within 10 seconds.
+		And(func() {
+			// Deployment revisionHistoryLimit should switch to 10
+			timeoutErr := wait.PollUntilContextTimeout(t.Context(), 1*time.Second, 10*time.Second, true, func(context.Context) (done bool, err error) {
+				deployment, err := fixture.KubeClientset.AppsV1().Deployments(fixture.DeploymentNamespace()).Get(t.Context(), "guestbook-ui", metav1.GetOptions{})
+				if err != nil {
+					return false, nil
+				}
+
+				revisionHistoryLimit := deployment.Spec.RevisionHistoryLimit
+				return revisionHistoryLimit != nil && *revisionHistoryLimit == 10, nil
+			})
+			require.NoError(t, timeoutErr)
+		}).
+		// Update the Deployment to a different revisionHistoryLimit
+		And(func() {
+			errors.NewHandler(t).FailOnErr(fixture.KubeClientset.AppsV1().Deployments(fixture.DeploymentNamespace()).Patch(t.Context(),
+				"guestbook-ui", types.MergePatchType, []byte(`{"spec": {"revisionHistoryLimit": 9}}`), metav1.PatchOptions{}))
+		}).
+		// The revisionHistoryLimit should NOT be self-healed, because selfHealing: false. It should remain at 9.
+		And(func() {
+			// Wait up to 10 seconds to ensure that deployment revisionHistoryLimit does NOT should switch to 10, it should remain at 9.
+			waitErr := wait.PollUntilContextTimeout(t.Context(), 1*time.Second, 10*time.Second, true, func(context.Context) (done bool, err error) {
+				deployment, err := fixture.KubeClientset.AppsV1().Deployments(fixture.DeploymentNamespace()).Get(t.Context(), "guestbook-ui", metav1.GetOptions{})
+				if err != nil {
+					return false, nil
+				}
+
+				revisionHistoryLimit := deployment.Spec.RevisionHistoryLimit
+				return revisionHistoryLimit != nil && *revisionHistoryLimit != 9, nil
+			})
+			require.Error(t, waitErr, "A timeout error should occur, indicating that revisionHistoryLimit never changed from 9")
+		})
+}
+
+func TestAutomatedSelfHealingAgainstLightweightTag(t *testing.T) {
+	Given(t).
+		Path(guestbookPath).
+		When().
+		AddTag("annotated-tag").
+		// App should be auto-synced once created
+		CreateFromFile(func(app *Application) {
+			app.Spec.Source.TargetRevision = "annotated-tag"
+			app.Spec.SyncPolicy = &SyncPolicy{Automated: &SyncPolicyAutomated{Prune: true, SelfHeal: false}}
+		}).
+		Then().
+		Expect(SyncStatusIs(SyncStatusCodeSynced)).
+		ExpectConsistently(SyncStatusIs(SyncStatusCodeSynced), WaitDuration, time.Second*10).
+		When().
+		// Update the annotated tag to a new git commit, that has a new revisionHistoryLimit.
+		PatchFile("guestbook-ui-deployment.yaml", `[{"op": "replace", "path": "/spec/revisionHistoryLimit", "value": 10}]`).
+		AddTagWithForce("annotated-tag").
+		Refresh(RefreshTypeHard).
+		// The Application should update to the new annotated tag value within 10 seconds.
+		And(func() {
+			// Deployment revisionHistoryLimit should switch to 10
+			timeoutErr := wait.PollUntilContextTimeout(t.Context(), 1*time.Second, 10*time.Second, true, func(context.Context) (done bool, err error) {
+				deployment, err := fixture.KubeClientset.AppsV1().Deployments(fixture.DeploymentNamespace()).Get(t.Context(), "guestbook-ui", metav1.GetOptions{})
+				if err != nil {
+					return false, nil
+				}
+
+				revisionHistoryLimit := deployment.Spec.RevisionHistoryLimit
+				return revisionHistoryLimit != nil && *revisionHistoryLimit == 10, nil
+			})
+			require.NoError(t, timeoutErr)
+		}).
+		// Update the Deployment to a different revisionHistoryLimit
+		And(func() {
+			errors.NewHandler(t).FailOnErr(fixture.KubeClientset.AppsV1().Deployments(fixture.DeploymentNamespace()).Patch(t.Context(),
+				"guestbook-ui", types.MergePatchType, []byte(`{"spec": {"revisionHistoryLimit": 9}}`), metav1.PatchOptions{}))
+		}).
+		// The revisionHistoryLimit should NOT be self-healed, because selfHealing: false
+		And(func() {
+			// Wait up to 10 seconds to ensure that deployment revisionHistoryLimit does NOT should switch to 10, it should remain at 9.
+			waitErr := wait.PollUntilContextTimeout(t.Context(), 1*time.Second, 10*time.Second, true, func(context.Context) (done bool, err error) {
+				deployment, err := fixture.KubeClientset.AppsV1().Deployments(fixture.DeploymentNamespace()).Get(t.Context(), "guestbook-ui", metav1.GetOptions{})
+				if err != nil {
+					return false, nil
+				}
+
+				revisionHistoryLimit := deployment.Spec.RevisionHistoryLimit
+				return revisionHistoryLimit != nil && *revisionHistoryLimit != 9, nil
+			})
+			require.Error(t, waitErr, "A timeout error should occur, indicating that revisionHistoryLimit never changed from 9")
+		})
+}

util/git/client_test.go

@@ -8,6 +8,7 @@ import (
 	"path/filepath"
 	"testing"
 
+	"github.com/go-git/go-git/v5/plumbing"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -118,6 +119,19 @@ func Test_IsAnnotatedTag(t *testing.T) {
 	assert.False(t, atag)
 }
 
+func Test_resolveTagReference(t *testing.T) {
+	// Setup
+	commitHash := plumbing.NewHash("0123456789abcdef0123456789abcdef01234567")
+	tagRef := plumbing.NewReferenceFromStrings("refs/tags/v1.0.0", "sometaghash")
+
+	// Test single function
+	resolvedRef := plumbing.NewHashReference(tagRef.Name(), commitHash)
+
+	// Verify
+	assert.Equal(t, commitHash, resolvedRef.Hash())
+	assert.Equal(t, tagRef.Name(), resolvedRef.Name())
+}
+
 func Test_ChangedFiles(t *testing.T) {
 	tempDir := t.TempDir()
 

util/git/git_test.go

@@ -501,3 +501,27 @@ func TestLsFiles(t *testing.T) {
 	require.NoError(t, err)
 	assert.Equal(t, nilResult, lsResult)
 }
+
+func TestAnnotatedTagHandling(t *testing.T) {
+	dir := t.TempDir()
+
+	client, err := NewClientExt("https://github.com/argoproj/argo-cd.git", dir, NopCreds{}, false, false, "", "")
+	require.NoError(t, err)
+
+	err = client.Init()
+	require.NoError(t, err)
+
+	// Test annotated tag resolution
+	commitSHA, err := client.LsRemote("v1.0.0") // Known annotated tag
+	require.NoError(t, err)
+
+	// Verify we get commit SHA, not tag SHA
+	assert.True(t, IsCommitSHA(commitSHA))
+
+	// Test tag reference handling
+	refs, err := client.LsRefs()
+	require.NoError(t, err)
+
+	// Verify tag exists in the list and points to a valid commit SHA
+	assert.Contains(t, refs.Tags, "v1.0.0", "Tag v1.0.0 should exist in refs")
+}

util/git/workaround.go

@@ -85,6 +85,12 @@ func listRemote(r *git.Remote, o *git.ListOptions, insecure bool, creds Creds, p
 
 	var resultRefs []*plumbing.Reference
 	_ = refs.ForEach(func(ref *plumbing.Reference) error {
+		if ref.Name().IsTag() {
+			if peeled, ok := ar.Peeled[ref.Name().String()]; ok {
+				resultRefs = append(resultRefs, plumbing.NewHashReference(ref.Name(), peeled))
+				return nil
+			}
+		}
 		resultRefs = append(resultRefs, ref)
 		return nil
 	})