From 787f964ea8cc69717e83ea3cb1e8816a96f4895a Mon Sep 17 00:00:00 2001 From: Sanskar Jaiswal Date: Thu, 10 Aug 2023 18:38:32 +0530 Subject: [PATCH] gitrepo: add .status.observedVerificationMode to track Git verification Add `.status.observedVerificationMode` to record the mode used to verify the Git object(s) and use that to determine whether we need to re-verify and trigger a full reconciliation. Signed-off-by: Sanskar Jaiswal --- api/v1/gitrepository_types.go | 4 ++ api/v1/zz_generated.deepcopy.go | 5 ++ ...rce.toolkit.fluxcd.io_gitrepositories.yaml | 4 ++ docs/api/v1/source.md | 15 +++++ go.mod | 6 +- go.sum | 10 +-- .../controller/gitrepository_controller.go | 59 +++++++++++------- .../gitrepository_controller_test.go | 62 ++++++------------- 8 files changed, 93 insertions(+), 72 deletions(-) diff --git a/api/v1/gitrepository_types.go b/api/v1/gitrepository_types.go index 35aa4b49a..d491fdc5e 100644 --- a/api/v1/gitrepository_types.go +++ b/api/v1/gitrepository_types.go @@ -246,6 +246,10 @@ type GitRepositoryStatus struct { // +optional ObservedInclude []GitRepositoryInclude `json:"observedInclude,omitempty"` + // ObservedVerificationMode is the observed mode indicating which Git + // object(s) have been verified. + ObservedVerificationMode *GitVerificationMode `json:"observedVerificationMode,omitempty"` + meta.ReconcileRequestStatus `json:",inline"` } diff --git a/api/v1/zz_generated.deepcopy.go b/api/v1/zz_generated.deepcopy.go index 23630ff9f..40a27a959 100644 --- a/api/v1/zz_generated.deepcopy.go +++ b/api/v1/zz_generated.deepcopy.go @@ -232,6 +232,11 @@ func (in *GitRepositoryStatus) DeepCopyInto(out *GitRepositoryStatus) { *out = make([]GitRepositoryInclude, len(*in)) copy(*out, *in) } + if in.ObservedVerificationMode != nil { + in, out := &in.ObservedVerificationMode, &out.ObservedVerificationMode + *out = new(GitVerificationMode) + **out = **in + } out.ReconcileRequestStatus = in.ReconcileRequestStatus } diff --git a/config/crd/bases/source.toolkit.fluxcd.io_gitrepositories.yaml b/config/crd/bases/source.toolkit.fluxcd.io_gitrepositories.yaml index c5f6ac58a..cc9db6a8f 100644 --- a/config/crd/bases/source.toolkit.fluxcd.io_gitrepositories.yaml +++ b/config/crd/bases/source.toolkit.fluxcd.io_gitrepositories.yaml @@ -410,6 +410,10 @@ spec: description: ObservedRecurseSubmodules is the observed resource submodules configuration used to produce the current Artifact. type: boolean + observedVerificationMode: + description: ObservedVerificationMode is the observed mode indicating + which Git object(s) have been verified. + type: string type: object type: object served: true diff --git a/docs/api/v1/source.md b/docs/api/v1/source.md index 6f8e61e1a..8baf7a81c 100644 --- a/docs/api/v1/source.md +++ b/docs/api/v1/source.md @@ -796,6 +796,20 @@ produce the current Artifact.

+observedVerificationMode
+ + +GitVerificationMode + + + + +

ObservedVerificationMode is the observed mode indicating which Git +object(s) have been verified.

+ + + + ReconcileRequestStatus
@@ -870,6 +884,7 @@ authors.

(string alias)

(Appears on: +GitRepositoryStatus, GitRepositoryVerification)

GitVerificationMode specifies the verification mode for a Git repository.

diff --git a/go.mod b/go.mod index 02b927f26..d9c04a48e 100644 --- a/go.mod +++ b/go.mod @@ -14,9 +14,9 @@ replace github.com/opencontainers/go-digest => github.com/opencontainers/go-dige // Check again when oras.land/oras-go is updated, which is a dependency of Helm. replace github.com/docker/docker => github.com/docker/docker v23.0.6+incompatible -replace github.com/fluxcd/pkg/git => github.com/fluxcd/pkg/git v0.12.4-0.20230731130338-0830185ed818 +replace github.com/fluxcd/pkg/git => github.com/fluxcd/pkg/git v0.12.5-0.20230810075312-c35892e27d72 -replace github.com/fluxcd/pkg/git/gogit => github.com/fluxcd/pkg/git/gogit v0.12.2-0.20230731130338-0830185ed818 +replace github.com/fluxcd/pkg/git/gogit => github.com/fluxcd/pkg/git/gogit v0.12.2-0.20230810075312-c35892e27d72 require ( cloud.google.com/go/storage v1.31.0 @@ -32,7 +32,7 @@ require ( github.com/fluxcd/pkg/apis/event v0.5.2 github.com/fluxcd/pkg/apis/meta v1.1.2 github.com/fluxcd/pkg/git v0.12.4 - github.com/fluxcd/pkg/git/gogit v0.12.1 + github.com/fluxcd/pkg/git/gogit v0.12.2-0.20230810075312-c35892e27d72 github.com/fluxcd/pkg/gittestserver v0.8.5 github.com/fluxcd/pkg/helmtestserver v0.13.2 github.com/fluxcd/pkg/lockedfile v0.1.0 diff --git a/go.sum b/go.sum index 40fa6a5af..a55b6c194 100644 --- a/go.sum +++ b/go.sum @@ -354,7 +354,7 @@ github.com/docker/libtrust v0.0.0-20150114040149-fa567046d9b1/go.mod h1:cyGadeNE github.com/docopt/docopt-go v0.0.0-20180111231733-ee0de3bc6815/go.mod h1:WwZ+bS3ebgob9U8Nd0kOddGdZWjyMGR8Wziv+TBNwSE= github.com/dustin/go-humanize v1.0.1 h1:GzkhY7T5VNhEkwH0PVJgjz+fX1rhBrR7pRT3mDkpeCY= github.com/dustin/go-humanize v1.0.1/go.mod h1:Mu1zIs6XwVuF/gI1OepvI0qD18qycQx+mFykh5fBlto= -github.com/elazarl/goproxy v0.0.0-20221015165544-a0805db90819 h1:RIB4cRk+lBqKK3Oy0r2gRX4ui7tuhiZq2SuTtTCi0/0= +github.com/elazarl/goproxy v0.0.0-20230731152917-f99041a5c027 h1:1L0aalTpPz7YlMxETKpmQoWMBkeiuorElZIXoNmgiPE= github.com/emicklei/go-restful/v3 v3.10.2 h1:hIovbnmBTLjHXkqEBUz3HGpXZdM7ZrE9fJIZIqlJLqE= github.com/emicklei/go-restful/v3 v3.10.2/go.mod h1:6n3XBCmQQb25CM2LCACGz8ukIrRry+4bhvbpWn3mrbc= github.com/emirpasic/gods v1.18.1 h1:FXtiHYKDGKCW2KzwZKx0iC0PQmdlorYgdFG9jPXJ1Bc= @@ -393,10 +393,10 @@ github.com/fluxcd/pkg/apis/event v0.5.2 h1:WtnCOeWglf7wR3dpyiWxb1JtYkw1G5OXcERb1 github.com/fluxcd/pkg/apis/event v0.5.2/go.mod h1:5l6SSxVTkqrXrYjgEqAajOOHkl4x0TPocAuSdu+3AEs= github.com/fluxcd/pkg/apis/meta v1.1.2 h1:Unjo7hxadtB2dvGpeFqZZUdsjpRA08YYSBb7dF2WIAM= github.com/fluxcd/pkg/apis/meta v1.1.2/go.mod h1:BHQyRHCskGMEDf6kDGbgQ+cyiNpUHbLsCOsaMYM2maI= -github.com/fluxcd/pkg/git v0.12.4-0.20230731130338-0830185ed818 h1:5RiT/G6Lmbg5eRHh4cjTM9t2v/txc5Yx5rs7vDe3dVk= -github.com/fluxcd/pkg/git v0.12.4-0.20230731130338-0830185ed818/go.mod h1:VBPwN/pjvkqWmGUzou53fAQDT2tRJAUJ45SC7TyW5TI= -github.com/fluxcd/pkg/git/gogit v0.12.2-0.20230731130338-0830185ed818 h1:G01A3JDlqrh6JzK/5ydFtdIQE7Nkfaw8+WEshMwMLZQ= -github.com/fluxcd/pkg/git/gogit v0.12.2-0.20230731130338-0830185ed818/go.mod h1:Z4Ysp8VifKTvWpjJMKncJsgb2iBqHuIeK80VGjlU41Y= +github.com/fluxcd/pkg/git v0.12.5-0.20230810075312-c35892e27d72 h1:NGeVmHjeSlb2q0jo8QYKJfuHd0mkoHz8lHC+E1NG6Y8= +github.com/fluxcd/pkg/git v0.12.5-0.20230810075312-c35892e27d72/go.mod h1:rKB1puk7sbC4AYF1oZDBrkvu3cr0aibkd4I5yNbxSQg= +github.com/fluxcd/pkg/git/gogit v0.12.2-0.20230810075312-c35892e27d72 h1:bfk1G3hynmx4rYN/s1A8hvbDK/QS3BGxXimGZpOZSt8= +github.com/fluxcd/pkg/git/gogit v0.12.2-0.20230810075312-c35892e27d72/go.mod h1:cS/hPj9E2cONT2bI9RD97mmtWnIsusSBuZ+HXI64be4= github.com/fluxcd/pkg/gittestserver v0.8.5 h1:EGqDF4240xPRgW1FFrQAs0Du7fZb8OGXC5qKDIqyXD8= github.com/fluxcd/pkg/gittestserver v0.8.5/go.mod h1:SyGEh+OBzFpdlTWWqv3XBkiLB42Iu+mijfIQ4hPlEZQ= github.com/fluxcd/pkg/helmtestserver v0.13.2 h1:Wypmc8kr9UrUwB32v2InK8oRDb9tGaixATAXqaZFurI= diff --git a/internal/controller/gitrepository_controller.go b/internal/controller/gitrepository_controller.go index 16da537a6..e6c9aee4b 100644 --- a/internal/controller/gitrepository_controller.go +++ b/internal/controller/gitrepository_controller.go @@ -552,12 +552,8 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, sp *patch // If it's a partial commit obtained from an existing artifact, check if the // reconciliation can be skipped if other configurations have not changed. if !git.IsConcreteCommit(*commit) { - // If we need to verify a Git object that we previously haven't verified, then - // we need to do a full reconciliation. - verificationRequired := requiresVerification(obj) - // Check if the content config contributing to the artifact has changed. - if !gitContentConfigChanged(obj, includes) && !verificationRequired { + if !gitContentConfigChanged(obj, includes) { ge := serror.NewGeneric( fmt.Errorf("no changes since last reconcilation: observed revision '%s'", commitReference(obj, commit)), sourcev1.GitOperationSucceedReason, @@ -590,7 +586,7 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, sp *patch conditions.Delete(obj, sourcev1.FetchFailedCondition) // Verify commit signature - if result, err := r.verifyCommitSignature(ctx, obj, *commit); err != nil || result == sreconcile.ResultEmpty { + if result, err := r.verifySignature(ctx, obj, *commit); err != nil || result == sreconcile.ResultEmpty { return result, err } @@ -927,14 +923,14 @@ func (r *GitRepositoryReconciler) fetchIncludes(ctx context.Context, obj *source return &artifacts, nil } -// verifyCommitSignature verifies the signature of the given Git commit and/or its parent tag +// verifySignature verifies the signature of the given Git commit and/or its parent tag // depedning on the verification mode specified on the object. // If the signature can not be verified or the verification fails, it records // v1beta2.SourceVerifiedCondition=False and returns. // When successful, it records v1beta2.SourceVerifiedCondition=True. // If no verification mode is specified on the object, the // v1beta2.SourceVerifiedCondition Condition is removed. -func (r *GitRepositoryReconciler) verifyCommitSignature(ctx context.Context, obj *sourcev1.GitRepository, commit git.Commit) (sreconcile.Result, error) { +func (r *GitRepositoryReconciler) verifySignature(ctx context.Context, obj *sourcev1.GitRepository, commit git.Commit) (sreconcile.Result, error) { // Check if there is a commit verification is configured and remove any old // observations if there is none if obj.Spec.Verification == nil || obj.Spec.Verification.Mode == "" { @@ -975,12 +971,20 @@ func (r *GitRepositoryReconciler) verifyCommitSignature(ctx context.Context, obj verifyTag = true } - if verifyTag && obj.Spec.Reference.Tag == "" { - err := errors.New("cannot verify tag object's signature if a tag reference is not specified") - conditions.MarkStalled(obj, "InvalidVerificationMode", err.Error()) + // check if a refname points to a tag, since that can also be used to + // fetch and verify the tag object. + isRefnameTag := func(name string) bool { + return strings.HasPrefix(name, "refs/tags") && !strings.HasSuffix(name, "^{}") + } + if verifyTag && ((obj.Spec.Reference == nil) || (obj.Spec.Reference.Tag == "" && obj.Spec.Reference.SemVer == "" && + !isRefnameTag(obj.Spec.Reference.Name))) { + err := serror.NewStalling( + errors.New("cannot verify tag object's signature if a tag reference is not specified"), + "InvalidVerificationMode", + ) + conditions.MarkFalse(obj, sourcev1.SourceVerifiedCondition, err.Reason, err.Err.Error()) return sreconcile.ResultEmpty, err } - conditions.Delete(obj, meta.StalledCondition) var message strings.Builder var reason string @@ -998,6 +1002,7 @@ func (r *GitRepositoryReconciler) verifyCommitSignature(ctx context.Context, obj } message.WriteString(fmt.Sprintf("verified signature of commit '%s' with key '%s'", commit.Hash.String(), headEntity)) reason = sourcev1.VerifiedCommitReason + obj.Status.ObservedVerificationMode = ptrToVerificationMode(sourcev1.GitVerificationMode(sourcev1.VerifiedCommitReason)) } if verifyTag { @@ -1017,9 +1022,11 @@ func (r *GitRepositoryReconciler) verifyCommitSignature(ctx context.Context, obj if verifyHead { message.WriteString(fmt.Sprintf(" and signature of tag '%s' with key '%s'", tag.String(), tagEntity)) reason = sourcev1.VerifiedTagAndCommitReason + obj.Status.ObservedVerificationMode = ptrToVerificationMode(sourcev1.GitVerificationMode(sourcev1.VerifiedTagAndCommitReason)) } else { message.WriteString(fmt.Sprintf("verified signature of tag '%s' with key '%s'", tag.String(), tagEntity)) reason = sourcev1.VerifiedTagReason + obj.Status.ObservedVerificationMode = ptrToVerificationMode(sourcev1.GitVerificationMode(sourcev1.VerifiedTagReason)) } } } @@ -1100,7 +1107,8 @@ func (r *GitRepositoryReconciler) eventLogf(ctx context.Context, obj runtime.Obj // gitContentConfigChanged evaluates the current spec with the observations of // the artifact in the status to determine if artifact content configuration has -// changed and requires rebuilding the artifact. +// changed and requires rebuilding the artifact. Rebuilding the artifact is also +// required if the object needs to be (re)verified. func gitContentConfigChanged(obj *sourcev1.GitRepository, includes *artifactSet) bool { if !pointer.StringEqual(obj.Spec.Ignore, obj.Status.ObservedIgnore) { return true @@ -1111,6 +1119,9 @@ func gitContentConfigChanged(obj *sourcev1.GitRepository, includes *artifactSet) if len(obj.Spec.Include) != len(obj.Status.ObservedInclude) { return true } + if requiresVerification(obj) { + return true + } // Convert artifactSet to index addressable artifacts and ensure that it and // the included artifacts include all the include from the spec. @@ -1171,25 +1182,29 @@ func commitReference(obj *sourcev1.GitRepository, commit *git.Commit) string { // first checking if the GitRepository has a verification spec. If it does, then // it returns true based on the following three conditions: // -// - If the object does not have a SourceVerifiedCondition in its status. -// - If the current reason for the SourceVerifiedCondition indicates that only the -// the tag has been verified earlier and the HEAD also needs to be verified now. -// - If the current reason for the SourceVerifiedCondition indicates that only the -// the HEAD has been verified earlier and the tag also needs to be verified now. +// - If the object does not have a observed verification mode in its status. +// - If the observed verification mode indicates that only the tag had been +// verified earlier and the HEAD also needs to be verified now. +// - If the observed verification mode indicates that only the HEAD had been +// verified earlier and the tag also needs to be verified now. func requiresVerification(obj *sourcev1.GitRepository) bool { var verificationRequired bool if obj.Spec.Verification != nil { - sourceVerifiedCondition := conditions.Get(obj, sourcev1.SourceVerifiedCondition) + observedVerificationMode := obj.Status.ObservedVerificationMode mode := obj.Spec.Verification.GetMode() - if sourceVerifiedCondition == nil { + if observedVerificationMode == nil { verificationRequired = true - } else if sourceVerifiedCondition.Reason == sourcev1.VerifiedTagReason && + } else if *observedVerificationMode == sourcev1.ModeTag && (mode == sourcev1.ModeHEAD || mode == sourcev1.ModeTagAndHEAD) { verificationRequired = true - } else if sourceVerifiedCondition.Reason == sourcev1.VerifiedCommitReason && + } else if *observedVerificationMode == sourcev1.ModeHEAD && (mode == sourcev1.ModeTag || mode == sourcev1.ModeTagAndHEAD) { verificationRequired = true } } return verificationRequired } + +func ptrToVerificationMode(mode sourcev1.GitVerificationMode) *sourcev1.GitVerificationMode { + return &mode +} diff --git a/internal/controller/gitrepository_controller_test.go b/internal/controller/gitrepository_controller_test.go index 232070b2a..651b6fd73 100644 --- a/internal/controller/gitrepository_controller_test.go +++ b/internal/controller/gitrepository_controller_test.go @@ -1531,7 +1531,7 @@ func TestGitRepositoryReconciler_reconcileDelete(t *testing.T) { g.Expect(obj.Status.Artifact).To(BeNil()) } -func TestGitRepositoryReconciler_verifyCommitSignature(t *testing.T) { +func TestGitRepositoryReconciler_verifySignature(t *testing.T) { tests := []struct { name string secret *corev1.Secret @@ -1539,10 +1539,11 @@ func TestGitRepositoryReconciler_verifyCommitSignature(t *testing.T) { beforeFunc func(obj *sourcev1.GitRepository) want sreconcile.Result wantErr bool + err error assertConditions []metav1.Condition }{ { - name: "Valid commit with mode=head makes SourceVerifiedCondition=True", + name: "Valid commit with mode=HEAD makes SourceVerifiedCondition=True", secret: &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: "existing", @@ -1644,7 +1645,7 @@ func TestGitRepositoryReconciler_verifyCommitSignature(t *testing.T) { }, }, { - name: "Verification of tag with no tag ref marks the object as stalled", + name: "Verification of tag with no tag ref sets no SourceVerifiedCondition and returns a stalling error", secret: &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: "existing", @@ -1666,8 +1667,12 @@ func TestGitRepositoryReconciler_verifyCommitSignature(t *testing.T) { } }, wantErr: true, + err: serror.NewStalling( + errors.New("cannot verify tag object's signature if a tag reference is not specified"), + "InvalidVerificationMode", + ), assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.StalledCondition, "InvalidVerificationMode", "cannot verify tag object's signature if a tag reference is not specified"), + *conditions.FalseCondition(sourcev1.SourceVerifiedCondition, "InvalidVerificationMode", "cannot verify tag object's signature if a tag reference is not specified"), }, }, { @@ -1793,9 +1798,12 @@ func TestGitRepositoryReconciler_verifyCommitSignature(t *testing.T) { tt.beforeFunc(obj) } - got, err := r.verifyCommitSignature(context.TODO(), obj, tt.commit) + got, err := r.verifySignature(context.TODO(), obj, tt.commit) g.Expect(obj.Status.Conditions).To(conditions.MatchConditions(tt.assertConditions)) g.Expect(err != nil).To(Equal(tt.wantErr)) + if tt.err != nil { + g.Expect(err).To(Equal(tt.err)) + } g.Expect(got).To(Equal(tt.want)) }) } @@ -2958,7 +2966,7 @@ func Test_requiresVerification(t *testing.T) { want: false, }, { - name: "GitRepository with verification and no SourceVerifiedCondition in status requires verification", + name: "GitRepository with verification and no observed verification mode in status requires verification", obj: &sourcev1.GitRepository{ Spec: sourcev1.GitRepositorySpec{ Verification: &sourcev1.GitRepositoryVerification{}, @@ -2975,12 +2983,7 @@ func Test_requiresVerification(t *testing.T) { }, }, Status: sourcev1.GitRepositoryStatus{ - Conditions: []metav1.Condition{ - { - Type: sourcev1.SourceVerifiedCondition, - Reason: sourcev1.VerifiedTagReason, - }, - }, + ObservedVerificationMode: ptrToVerificationMode(sourcev1.ModeTag), }, }, want: true, @@ -2994,12 +2997,7 @@ func Test_requiresVerification(t *testing.T) { }, }, Status: sourcev1.GitRepositoryStatus{ - Conditions: []metav1.Condition{ - { - Type: sourcev1.SourceVerifiedCondition, - Reason: sourcev1.VerifiedTagReason, - }, - }, + ObservedVerificationMode: ptrToVerificationMode(sourcev1.ModeTag), }, }, want: true, @@ -3013,12 +3011,7 @@ func Test_requiresVerification(t *testing.T) { }, }, Status: sourcev1.GitRepositoryStatus{ - Conditions: []metav1.Condition{ - { - Type: sourcev1.SourceVerifiedCondition, - Reason: sourcev1.VerifiedCommitReason, - }, - }, + ObservedVerificationMode: ptrToVerificationMode(sourcev1.ModeHEAD), }, }, want: true, @@ -3032,12 +3025,7 @@ func Test_requiresVerification(t *testing.T) { }, }, Status: sourcev1.GitRepositoryStatus{ - Conditions: []metav1.Condition{ - { - Type: sourcev1.SourceVerifiedCondition, - Reason: sourcev1.VerifiedCommitReason, - }, - }, + ObservedVerificationMode: ptrToVerificationMode(sourcev1.ModeHEAD), }, }, want: true, @@ -3051,12 +3039,7 @@ func Test_requiresVerification(t *testing.T) { }, }, Status: sourcev1.GitRepositoryStatus{ - Conditions: []metav1.Condition{ - { - Type: sourcev1.SourceVerifiedCondition, - Reason: sourcev1.VerifiedTagAndCommitReason, - }, - }, + ObservedVerificationMode: ptrToVerificationMode(sourcev1.ModeTagAndHEAD), }, }, want: false, @@ -3070,12 +3053,7 @@ func Test_requiresVerification(t *testing.T) { }, }, Status: sourcev1.GitRepositoryStatus{ - Conditions: []metav1.Condition{ - { - Type: sourcev1.SourceVerifiedCondition, - Reason: sourcev1.VerifiedTagAndCommitReason, - }, - }, + ObservedVerificationMode: ptrToVerificationMode(sourcev1.ModeTagAndHEAD), }, }, want: false,