Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor Git operations and introduce go-git support for Azure DevOps and AWS CodeCommit #944

Merged
merged 6 commits into from
Oct 31, 2022
Merged

Refactor Git operations and introduce go-git support for Azure DevOps and AWS CodeCommit #944

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b6d6b59
gitrepo: refactor reconciler to use fluxcd/pkg/git
aryan9600 Aug 30, 2022
9ef6316
ci: remove explicit managed transport init and tests
aryan9600 Sep 2, 2022
d25ed0c
build: Bump golang-with-libgit2 to v0.4.0
Oct 13, 2022
22e712b
gitrepo: match against api types when creating git client
aryan9600 Oct 19, 2022
6b04907
Bump go-git to enable multi_ack
Oct 26, 2022
e87997c
Use serror.NewStalling() and small PR fixes
Oct 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ ARG GO_VERSION=1.19
ARG XX_VERSION=1.1.2

ARG LIBGIT2_IMG=ghcr.io/fluxcd/golang-with-libgit2-only
ARG LIBGIT2_TAG=v0.3.0
ARG LIBGIT2_TAG=v0.4.0

FROM ${LIBGIT2_IMG}:${LIBGIT2_TAG} AS libgit2-libs

Expand Down
2 changes: 1 addition & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ TAG ?= latest

# Base image used to build the Go binary
LIBGIT2_IMG ?= ghcr.io/fluxcd/golang-with-libgit2-only
LIBGIT2_TAG ?= v0.3.0
LIBGIT2_TAG ?= v0.4.0

# Allows for defining additional Go test args, e.g. '-tags integration'.
GO_TEST_ARGS ?= -race
Expand Down
91 changes: 46 additions & 45 deletions controllers/gitrepository_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"context"
"errors"
"fmt"
"net/url"
"os"
"path/filepath"
"strings"
Expand All @@ -41,6 +42,9 @@ import (
"sigs.k8s.io/controller-runtime/pkg/ratelimiter"

"github.com/fluxcd/pkg/apis/meta"
"github.com/fluxcd/pkg/git"
"github.com/fluxcd/pkg/git/gogit"
"github.com/fluxcd/pkg/git/libgit2"
"github.com/fluxcd/pkg/runtime/conditions"
helper "github.com/fluxcd/pkg/runtime/controller"
"github.com/fluxcd/pkg/runtime/events"
Expand All @@ -54,8 +58,6 @@ import (
sreconcile "github.com/fluxcd/source-controller/internal/reconcile"
"github.com/fluxcd/source-controller/internal/reconcile/summarize"
"github.com/fluxcd/source-controller/internal/util"
"github.com/fluxcd/source-controller/pkg/git"
"github.com/fluxcd/source-controller/pkg/git/strategy"
)

// gitRepositoryReadyCondition contains the information required to summarize a
Expand Down Expand Up @@ -440,9 +442,7 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context,
conditions.Delete(obj, sourcev1.SourceVerifiedCondition)
}

// Configure authentication strategy to access the source
var authOpts *git.AuthOptions
var err error
var authData map[string][]byte
if obj.Spec.SecretRef != nil {
// Attempt to retrieve secret
name := types.NamespacedName{
Expand All @@ -459,20 +459,27 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context,
// Return error as the world as observed may change
return sreconcile.ResultEmpty, e
}
authData = secret.Data
}

// Configure strategy with secret
authOpts, err = git.AuthOptionsFromSecret(obj.Spec.URL, &secret)
} else {
// Set the minimal auth options for valid transport.
authOpts, err = git.AuthOptionsWithoutSecret(obj.Spec.URL)
u, err := url.Parse(obj.Spec.URL)
if err != nil {
e := serror.NewStalling(
fmt.Errorf("failed to parse url '%s': %w", obj.Spec.URL, err),
sourcev1.URLInvalidReason,
)
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
return sreconcile.ResultEmpty, e
}

// Configure authentication strategy to access the source
authOpts, err := git.NewAuthOptions(*u, authData)
darkowlzz marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
e := serror.NewGeneric(
fmt.Errorf("failed to configure auth strategy for Git implementation '%s': %w", obj.Spec.GitImplementation, err),
fmt.Errorf("failed to configure authentication options: %w", err),
sourcev1.AuthenticationFailedReason,
)
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
// Return error as the contents of the secret may change
return sreconcile.ResultEmpty, e
darkowlzz marked this conversation as resolved.
Show resolved Hide resolved
}

Expand Down Expand Up @@ -725,59 +732,52 @@ func (r *GitRepositoryReconciler) reconcileInclude(ctx context.Context,
func (r *GitRepositoryReconciler) gitCheckout(ctx context.Context,
obj *sourcev1.GitRepository, authOpts *git.AuthOptions, dir string, optimized bool) (*git.Commit, error) {
// Configure checkout strategy.
checkoutOpts := git.CheckoutOptions{RecurseSubmodules: obj.Spec.RecurseSubmodules}
cloneOpts := git.CloneOptions{
RecurseSubmodules: obj.Spec.RecurseSubmodules,
ShallowClone: true,
}
if ref := obj.Spec.Reference; ref != nil {
checkoutOpts.Branch = ref.Branch
checkoutOpts.Commit = ref.Commit
checkoutOpts.Tag = ref.Tag
checkoutOpts.SemVer = ref.SemVer
cloneOpts.Branch = ref.Branch
cloneOpts.Commit = ref.Commit
cloneOpts.Tag = ref.Tag
cloneOpts.SemVer = ref.SemVer
}

// Only if the object has an existing artifact in storage, attempt to
// short-circuit clone operation. reconcileStorage has already verified
// that the artifact exists.
if optimized && conditions.IsTrue(obj, sourcev1.ArtifactInStorageCondition) {
if artifact := obj.GetArtifact(); artifact != nil {
checkoutOpts.LastRevision = artifact.Revision
cloneOpts.LastObservedCommit = artifact.Revision
}
}

gitCtx, cancel := context.WithTimeout(ctx, obj.Spec.Timeout.Duration)
defer cancel()

checkoutStrategy, err := strategy.CheckoutStrategyForImplementation(gitCtx,
git.Implementation(obj.Spec.GitImplementation), checkoutOpts)
var gitReader git.RepositoryReader
var err error

switch obj.Spec.GitImplementation {
case sourcev1.LibGit2Implementation:
gitReader, err = libgit2.NewClient(dir, authOpts)
case sourcev1.GoGitImplementation:
gitReader, err = gogit.NewClient(dir, authOpts)
default:
err = fmt.Errorf("invalid Git implementation: %s", obj.Spec.GitImplementation)
}
if err != nil {
// Do not return err as recovery without changes is impossible.
e := &serror.Stalling{
Err: fmt.Errorf("failed to configure checkout strategy for Git implementation '%s': %w", obj.Spec.GitImplementation, err),
Reason: sourcev1.GitOperationFailedReason,
}
e := serror.NewStalling(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While reviewing #945, I noticed that this stalling error may not be appropriate for the above operation. The old code had stalling error because CheckoutStrategyForImplementation() failure actually meant that retrying will not make any change. But for the above, new git client creation can fail due to various reasons and a subsequent retry may succeed.
It may be better to make sure it's an invalid git implementation error before returning stalling error. For other error while creating the git client, a generic error which results in a retry may be better.

fmt.Errorf("failed to create Git client for implementation '%s': %w", obj.Spec.GitImplementation, err),
sourcev1.GitOperationFailedReason,
)
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
return nil, e
}
defer gitReader.Close()

// this is needed only for libgit2, due to managed transport.
if obj.Spec.GitImplementation == sourcev1.LibGit2Implementation {
// We set the TransportOptionsURL of this set of authentication options here by constructing
// a unique URL that won't clash in a multi tenant environment. This unique URL is used by
// libgit2 managed transports. This enables us to bypass the inbuilt credentials callback in
// libgit2, which is inflexible and unstable.
if strings.HasPrefix(obj.Spec.URL, "http") {
authOpts.TransportOptionsURL = fmt.Sprintf("http://%s/%s/%d", obj.Name, obj.UID, obj.Generation)
} else if strings.HasPrefix(obj.Spec.URL, "ssh") {
authOpts.TransportOptionsURL = fmt.Sprintf("ssh://%s/%s/%d", obj.Name, obj.UID, obj.Generation)
} else {
e := &serror.Stalling{
Err: fmt.Errorf("git repository URL '%s' has invalid transport type, supported types are: http, https, ssh", obj.Spec.URL),
Reason: sourcev1.URLInvalidReason,
}
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
return nil, e
}
}

commit, err := checkoutStrategy.Checkout(gitCtx, dir, obj.Spec.URL, authOpts)
commit, err := gitReader.Clone(gitCtx, obj.Spec.URL, cloneOpts)
if err != nil {
e := serror.NewGeneric(
fmt.Errorf("failed to checkout and determine revision: %w", err),
Expand All @@ -786,6 +786,7 @@ func (r *GitRepositoryReconciler) gitCheckout(ctx context.Context,
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
return nil, e
}

return commit, nil
}

Expand Down
12 changes: 6 additions & 6 deletions controllers/gitrepository_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,13 +56,13 @@ import (
"github.com/fluxcd/pkg/ssh"
"github.com/fluxcd/pkg/testserver"

"github.com/fluxcd/pkg/git"
"github.com/fluxcd/pkg/git/libgit2/transport"
sourcev1 "github.com/fluxcd/source-controller/api/v1beta2"
serror "github.com/fluxcd/source-controller/internal/error"
"github.com/fluxcd/source-controller/internal/features"
sreconcile "github.com/fluxcd/source-controller/internal/reconcile"
"github.com/fluxcd/source-controller/internal/reconcile/summarize"
"github.com/fluxcd/source-controller/pkg/git"
"github.com/fluxcd/source-controller/pkg/git/libgit2/managed"
)

const (
Expand Down Expand Up @@ -502,7 +502,7 @@ func TestGitRepositoryReconciler_reconcileSource_authStrategy(t *testing.T) {
EventRecorder: record.NewFakeRecorder(32),
Storage: testStorage,
features: features.FeatureGates(),
Libgit2TransportInitialized: managed.Enabled,
Libgit2TransportInitialized: transport.Enabled,
}

for _, i := range testGitImplementations {
Expand Down Expand Up @@ -731,7 +731,7 @@ func TestGitRepositoryReconciler_reconcileSource_checkoutStrategy(t *testing.T)
EventRecorder: record.NewFakeRecorder(32),
Storage: testStorage,
features: features.FeatureGates(),
Libgit2TransportInitialized: managed.Enabled,
Libgit2TransportInitialized: transport.Enabled,
}

for _, tt := range tests {
Expand Down Expand Up @@ -1404,7 +1404,7 @@ func TestGitRepositoryReconciler_verifyCommitSignature(t *testing.T) {
},
wantErr: true,
assertConditions: []metav1.Condition{
*conditions.FalseCondition(sourcev1.SourceVerifiedCondition, "InvalidCommitSignature", "signature verification of commit 'shasum' failed: failed to verify commit with any of the given key rings"),
*conditions.FalseCondition(sourcev1.SourceVerifiedCondition, "InvalidCommitSignature", "signature verification of commit 'shasum' failed: unable to verify commit with any of the given key rings"),
},
},
{
Expand Down Expand Up @@ -1599,7 +1599,7 @@ func TestGitRepositoryReconciler_ConditionsUpdate(t *testing.T) {
EventRecorder: record.NewFakeRecorder(32),
Storage: testStorage,
features: features.FeatureGates(),
Libgit2TransportInitialized: managed.Enabled,
Libgit2TransportInitialized: transport.Enabled,
}

key := client.ObjectKeyFromObject(obj)
Expand Down
2 changes: 1 addition & 1 deletion controllers/ocirepository_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ import (
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"

"github.com/fluxcd/pkg/apis/meta"
"github.com/fluxcd/pkg/git"
"github.com/fluxcd/pkg/oci"
"github.com/fluxcd/pkg/runtime/conditions"
conditionscheck "github.com/fluxcd/pkg/runtime/conditions/check"
Expand All @@ -66,7 +67,6 @@ import (
sourcev1 "github.com/fluxcd/source-controller/api/v1beta2"
serror "github.com/fluxcd/source-controller/internal/error"
sreconcile "github.com/fluxcd/source-controller/internal/reconcile"
"github.com/fluxcd/source-controller/pkg/git"
)

func TestOCIRepository_Reconcile(t *testing.T) {
Expand Down
8 changes: 4 additions & 4 deletions controllers/suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import (
ctrl "sigs.k8s.io/controller-runtime"

dcontext "github.com/distribution/distribution/v3/context"
"github.com/fluxcd/pkg/git/libgit2/transport"
"github.com/fluxcd/pkg/runtime/controller"
"github.com/fluxcd/pkg/runtime/testenv"
"github.com/fluxcd/pkg/testserver"
Expand All @@ -47,13 +48,12 @@ import (
dockerRegistry "github.com/distribution/distribution/v3/registry"
_ "github.com/distribution/distribution/v3/registry/auth/htpasswd"
_ "github.com/distribution/distribution/v3/registry/storage/driver/inmemory"
git2go "github.com/libgit2/git2go/v33"
git2go "github.com/libgit2/git2go/v34"

sourcev1 "github.com/fluxcd/source-controller/api/v1beta2"
"github.com/fluxcd/source-controller/internal/cache"
"github.com/fluxcd/source-controller/internal/features"
"github.com/fluxcd/source-controller/internal/helm/registry"
"github.com/fluxcd/source-controller/pkg/git/libgit2/managed"
// +kubebuilder:scaffold:imports
)

Expand Down Expand Up @@ -237,7 +237,7 @@ func TestMain(m *testing.M) {
panic(fmt.Sprintf("Failed to create a test registry server: %v", err))
}

if err = managed.InitManagedTransport(); err != nil {
if err = transport.InitManagedTransport(); err != nil {
panic(fmt.Sprintf("Failed to initialize libgit2 managed transport: %v", err))
}

Expand All @@ -247,7 +247,7 @@ func TestMain(m *testing.M) {
Metrics: testMetricsH,
Storage: testStorage,
features: features.FeatureGates(),
Libgit2TransportInitialized: managed.Enabled,
Libgit2TransportInitialized: transport.Enabled,
}).SetupWithManager(testEnv); err != nil {
panic(fmt.Sprintf("Failed to start GitRepositoryReconciler: %v", err))
}
Expand Down
Loading