Skip to content

Commit

Permalink
Refactor and fix GitRepository tests
Browse files Browse the repository at this point in the history 
Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com>
  • Loading branch information
@aryan9600
aryan9600 committed May 29, 2022
1 parent 9d6eada commit d740f91
Show file tree
Hide file tree
Showing 9 changed files with 392 additions and 318 deletions.
11 changes: 8 additions & 3 deletions controllers/gitrepository_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@ import (
"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/libgit2/managed"
"github.com/fluxcd/source-controller/pkg/git/strategy"
"github.com/fluxcd/source-controller/pkg/sourceignore"
)
Expand Down Expand Up @@ -142,7 +141,12 @@ func (r *GitRepositoryReconciler) SetupWithManagerAndOptions(mgr ctrl.Manager, o
}

// Check and enable gated features.
if oc, _ := features.Enabled(features.OptimizedGitClones); oc {
mt, _ := features.Enabled(features.GitManagedTransport)
if mt {
r.features[features.GitManagedTransport] = true
}
// OptimizedGitClones is only enabled when GitManagedTransport is enabled.
if oc, _ := features.Enabled(features.OptimizedGitClones); oc && mt {
r.features[features.OptimizedGitClones] = true
}

Expand Down Expand Up @@ -737,7 +741,8 @@ func (r *GitRepositoryReconciler) gitCheckout(ctx context.Context,
}

// managed GIT transport only affects the libgit2 implementation
if managed.Enabled() && obj.Spec.GitImplementation == sourcev1.LibGit2Implementation {
if enabled, ok := r.features[features.GitManagedTransport]; ok && enabled &&
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
Expand Down
6 changes: 6 additions & 0 deletions controllers/suite_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ import (
_ "github.com/distribution/distribution/v3/registry/auth/htpasswd"
_ "github.com/distribution/distribution/v3/registry/storage/driver/inmemory"

feathelper "github.com/fluxcd/pkg/runtime/features"
sourcev1 "github.com/fluxcd/source-controller/api/v1beta2"
"github.com/fluxcd/source-controller/internal/cache"
"github.com/fluxcd/source-controller/internal/features"
Expand Down Expand Up @@ -205,6 +206,11 @@ func TestMain(m *testing.M) {
panic(fmt.Sprintf("Failed to create a test registry server: %v", err))
}

fg := feathelper.FeatureGates{}
fg.SupportedFeatures(map[string]bool{
features.GitManagedTransport: true,
})

managed.InitManagedTransport(logr.Discard())

if err := (&GitRepositoryReconciler{
Expand Down
7 changes: 0 additions & 7 deletions main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -312,13 +312,6 @@ func main() {

if enabled, _ := features.Enabled(features.GitManagedTransport); enabled {
managed.InitManagedTransport(ctrl.Log.WithName("managed-transport"))
} else {
if optimize, _ := feathelper.Enabled(features.OptimizedGitClones); optimize {
features.Disable(features.OptimizedGitClones)
setupLog.Info(
"disabling optimized git clones; git clones can only be optimized when using managed transport",
)
}
}

setupLog.Info("starting manager")
Expand Down
130 changes: 64 additions & 66 deletions pkg/git/libgit2/checkout.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package libgit2

import (
"context"
"errors"
"fmt"
"sort"
"strings"
Expand All @@ -30,6 +31,7 @@ import (
"github.com/fluxcd/pkg/gitutil"
"github.com/fluxcd/pkg/version"

"github.com/fluxcd/source-controller/internal/features"
"github.com/fluxcd/source-controller/pkg/git"
"github.com/fluxcd/source-controller/pkg/git/libgit2/managed"
)
Expand Down Expand Up @@ -73,40 +75,32 @@ func (c *CheckoutBranch) Checkout(ctx context.Context, path, url string, opts *g
// The panics probably happen because we perform multiple fetch ops (introduced as a part of optimizing git clones).
// The branching lets us establish a clear code path to help us be certain of the expected behaviour.
// When we get rid of unmanaged transports, we can get rid of this branching as well.
if managed.Enabled() {
if enabled, _ := features.Enabled(features.GitManagedTransport); enabled {
// We store the target URL and auth options mapped to a unique ID. We overwrite the target URL
// with the TransportOptionsURL, because managed transports don't provide a way for any kind of
// dependency injection. This lets us have a way of doing interop between application level code
// and transport level code.
// Performing all fetch operations with the TransportOptionsURL as the URL, lets the managed
// transport action use it to fetch the registered transport options which contains the
// _actual_ target URL and the correct credentials to use.
if opts == nil {
return nil, fmt.Errorf("can't use managed transport with an empty set of auth options")
}
if opts.TransportOptionsURL == "" {
return nil, fmt.Errorf("can't use managed transport without a valid transport auth id.")
err = registerManagedTransportOptions(ctx, url, opts)
if err != nil {
return nil, err
}
managed.AddTransportOptions(opts.TransportOptionsURL, managed.TransportOptions{
TargetURL: url,
AuthOpts: opts,
ProxyOptions: &git2go.ProxyOptions{Type: git2go.ProxyTypeAuto},
Context: ctx,
})
url = opts.TransportOptionsURL
remoteCallBacks := managed.RemoteCallbacks()
defer managed.RemoveTransportOptions(opts.TransportOptionsURL)

repo, remote, err := initializeRepoWithRemote(ctx, path, url, opts)
repo, remote, err := initializeRepoWithRemote(ctx, path, opts.TransportOptionsURL, opts)
if err != nil {
return nil, err
}

// Open remote connection.
remoteCallBacks := managed.RemoteCallbacks()
err = remote.ConnectFetch(&remoteCallBacks, nil, nil)
if err != nil {
remote.Free()
repo.Free()
return nil, fmt.Errorf("unable to fetch-connect to remote '%s': %w", managed.EffectiveURL(url), gitutil.LibGit2Error(err))
return nil, fmt.Errorf("unable to fetch-connect to remote '%s': %w", url, gitutil.LibGit2Error(err))
}
defer func() {
remote.Disconnect()
Expand All @@ -119,7 +113,7 @@ func (c *CheckoutBranch) Checkout(ctx context.Context, path, url string, opts *g
if c.LastRevision != "" {
heads, err := remote.Ls(c.Branch)
if err != nil {
return nil, fmt.Errorf("unable to remote ls for '%s': %w", managed.EffectiveURL(url), gitutil.LibGit2Error(err))
return nil, fmt.Errorf("unable to remote ls for '%s': %w", url, gitutil.LibGit2Error(err))
}
if len(heads) > 0 {
hash := heads[0].Id.String()
Expand All @@ -143,21 +137,20 @@ func (c *CheckoutBranch) Checkout(ctx context.Context, path, url string, opts *g
},
"")
if err != nil {
return nil, fmt.Errorf("unable to fetch remote '%s': %w",
managed.EffectiveURL(url), gitutil.LibGit2Error(err))
return nil, fmt.Errorf("unable to fetch remote '%s': %w", url, gitutil.LibGit2Error(err))
}

branch, err := repo.References.Lookup(fmt.Sprintf("refs/remotes/origin/%s", c.Branch))
if err != nil {
return nil, fmt.Errorf("unable to lookup branch '%s' for '%s': %w",
c.Branch, managed.EffectiveURL(url), gitutil.LibGit2Error(err))
c.Branch, url, gitutil.LibGit2Error(err))
}
defer branch.Free()

upstreamCommit, err := repo.LookupCommit(branch.Target())
if err != nil {
return nil, fmt.Errorf("unable to lookup commit '%s' for '%s': %w",
c.Branch, managed.EffectiveURL(url), gitutil.LibGit2Error(err))
c.Branch, url, gitutil.LibGit2Error(err))
}
defer upstreamCommit.Free()

Expand All @@ -167,7 +160,7 @@ func (c *CheckoutBranch) Checkout(ctx context.Context, path, url string, opts *g
Strategy: git2go.CheckoutForce,
})
if err != nil {
return nil, fmt.Errorf("unable to hard reset to commit for '%s': %w", managed.EffectiveURL(url), gitutil.LibGit2Error(err))
return nil, fmt.Errorf("unable to hard reset to commit for '%s': %w", url, gitutil.LibGit2Error(err))
}

// Use the current worktree's head as reference for the commit to be returned.
Expand Down Expand Up @@ -202,7 +195,7 @@ func (c *CheckoutBranch) checkoutUnmanaged(ctx context.Context, path, url string
CheckoutBranch: c.Branch,
})
if err != nil {
return nil, fmt.Errorf("unable to clone '%s': %w", managed.EffectiveURL(url), gitutil.LibGit2Error(err))
return nil, fmt.Errorf("unable to clone '%s': %w", url, gitutil.LibGit2Error(err))
}
defer repo.Free()
head, err := repo.Head()
Expand Down Expand Up @@ -230,30 +223,25 @@ func (c *CheckoutTag) Checkout(ctx context.Context, path, url string, opts *git.
// The panics probably happen because we perform multiple fetch ops (introduced as a part of optimizing git clones).
// The branching lets us establish a clear code path to help us be certain of the expected behaviour.
// When we get rid of unmanaged transports, we can get rid of this branching as well.
if managed.Enabled() {
if opts.TransportOptionsURL == "" {
return nil, fmt.Errorf("can't use managed transport without a valid transport auth id.")
if enabled, _ := features.Enabled(features.GitManagedTransport); enabled {
err = registerManagedTransportOptions(ctx, url, opts)
if err != nil {
return nil, err
}
managed.AddTransportOptions(opts.TransportOptionsURL, managed.TransportOptions{
TargetURL: url,
AuthOpts: opts,
ProxyOptions: &git2go.ProxyOptions{Type: git2go.ProxyTypeAuto},
Context: ctx,
})
url = opts.TransportOptionsURL
remoteCallBacks := managed.RemoteCallbacks()
defer managed.RemoveTransportOptions(opts.TransportOptionsURL)

repo, remote, err := initializeRepoWithRemote(ctx, path, url, opts)
repo, remote, err := initializeRepoWithRemote(ctx, path, opts.TransportOptionsURL, opts)
if err != nil {
return nil, err
}

// Open remote connection.
remoteCallBacks := managed.RemoteCallbacks()
err = remote.ConnectFetch(&remoteCallBacks, nil, nil)
if err != nil {
remote.Free()
repo.Free()
return nil, fmt.Errorf("unable to fetch-connect to remote '%s': %w", managed.EffectiveURL(url), gitutil.LibGit2Error(err))
return nil, fmt.Errorf("unable to fetch-connect to remote '%s': %w", url, gitutil.LibGit2Error(err))
}
defer func() {
remote.Disconnect()
Expand All @@ -266,7 +254,7 @@ func (c *CheckoutTag) Checkout(ctx context.Context, path, url string, opts *git.
if c.LastRevision != "" {
heads, err := remote.Ls(c.Tag)
if err != nil {
return nil, fmt.Errorf("unable to remote ls for '%s': %w", managed.EffectiveURL(url), gitutil.LibGit2Error(err))
return nil, fmt.Errorf("unable to remote ls for '%s': %w", url, gitutil.LibGit2Error(err))
}
if len(heads) > 0 {
hash := heads[0].Id.String()
Expand Down Expand Up @@ -300,8 +288,7 @@ func (c *CheckoutTag) Checkout(ctx context.Context, path, url string, opts *git.
"")

if err != nil {
return nil, fmt.Errorf("unable to fetch remote '%s': %w",
managed.EffectiveURL(url), gitutil.LibGit2Error(err))
return nil, fmt.Errorf("unable to fetch remote '%s': %w", url, gitutil.LibGit2Error(err))
}

cc, err := checkoutDetachedDwim(repo, c.Tag)
Expand All @@ -324,7 +311,7 @@ func (c *CheckoutTag) checkoutUnmanaged(ctx context.Context, path, url string, o
},
})
if err != nil {
return nil, fmt.Errorf("unable to clone '%s': %w", managed.EffectiveURL(url), gitutil.LibGit2Error(err))
return nil, fmt.Errorf("unable to clone '%s': %w", url, gitutil.LibGit2Error(err))
}
defer repo.Free()
cc, err := checkoutDetachedDwim(repo, c.Tag)
Expand All @@ -343,30 +330,26 @@ func (c *CheckoutCommit) Checkout(ctx context.Context, path, url string, opts *g
defer recoverPanic(&err)

remoteCallBacks := RemoteCallbacks(ctx, opts)
targetURL := url

if managed.Enabled() {
if opts.TransportOptionsURL == "" {
return nil, fmt.Errorf("can't use managed transport without a valid transport auth id.")
if enabled, _ := features.Enabled(features.GitManagedTransport); enabled {
err = registerManagedTransportOptions(ctx, url, opts)
if err != nil {
return nil, err
}
managed.AddTransportOptions(opts.TransportOptionsURL, managed.TransportOptions{
TargetURL: url,
AuthOpts: opts,
ProxyOptions: &git2go.ProxyOptions{Type: git2go.ProxyTypeAuto},
Context: ctx,
})
url = opts.TransportOptionsURL
remoteCallBacks = managed.RemoteCallbacks()
defer managed.RemoveTransportOptions(opts.TransportOptionsURL)
remoteCallBacks = managed.RemoteCallbacks()
targetURL = opts.TransportOptionsURL
}

repo, err := git2go.Clone(url, path, &git2go.CloneOptions{
repo, err := git2go.Clone(targetURL, path, &git2go.CloneOptions{
FetchOptions: git2go.FetchOptions{
DownloadTags: git2go.DownloadTagsNone,
RemoteCallbacks: remoteCallBacks,
},
})
if err != nil {
return nil, fmt.Errorf("unable to clone '%s': %w", managed.EffectiveURL(url), gitutil.LibGit2Error(err))
return nil, fmt.Errorf("unable to clone '%s': %w", targetURL, gitutil.LibGit2Error(err))
}
defer repo.Free()
oid, err := git2go.NewOid(c.Commit)
Expand All @@ -388,35 +371,31 @@ func (c *CheckoutSemVer) Checkout(ctx context.Context, path, url string, opts *g
defer recoverPanic(&err)

remoteCallBacks := RemoteCallbacks(ctx, opts)
targetURL := url

if managed.Enabled() {
if opts.TransportOptionsURL == "" {
return nil, fmt.Errorf("can't use managed transport without a valid transport auth id.")
if enabled, _ := features.Enabled(features.GitManagedTransport); enabled {
err = registerManagedTransportOptions(ctx, url, opts)
if err != nil {
return nil, err
}
managed.AddTransportOptions(opts.TransportOptionsURL, managed.TransportOptions{
TargetURL: url,
AuthOpts: opts,
ProxyOptions: &git2go.ProxyOptions{Type: git2go.ProxyTypeAuto},
Context: ctx,
})
url = opts.TransportOptionsURL
remoteCallBacks = managed.RemoteCallbacks()
defer managed.RemoveTransportOptions(opts.TransportOptionsURL)
remoteCallBacks = managed.RemoteCallbacks()
targetURL = opts.TransportOptionsURL
}

verConstraint, err := semver.NewConstraint(c.SemVer)
if err != nil {
return nil, fmt.Errorf("semver parse error: %w", err)
}

repo, err := git2go.Clone(url, path, &git2go.CloneOptions{
repo, err := git2go.Clone(targetURL, path, &git2go.CloneOptions{
FetchOptions: git2go.FetchOptions{
DownloadTags: git2go.DownloadTagsAll,
RemoteCallbacks: remoteCallBacks,
},
})
if err != nil {
return nil, fmt.Errorf("unable to clone '%s': %w", managed.EffectiveURL(url), gitutil.LibGit2Error(err))
return nil, fmt.Errorf("unable to clone '%s': %w", targetURL, gitutil.LibGit2Error(err))
}
defer repo.Free()

Expand Down Expand Up @@ -602,6 +581,25 @@ func initializeRepoWithRemote(ctx context.Context, path, url string, opts *git.A
return repo, remote, nil
}

// registerManagedTransportOptions registers the given url and it's transport options.
// Callers must make sure to call `managed.RemoveTransportOptions()` to avoid increase in
// memory consumption.
func registerManagedTransportOptions(ctx context.Context, url string, authOpts *git.AuthOptions) error {
if authOpts == nil {
return errors.New("can't use managed transport with an empty set of auth options")
}
if authOpts.TransportOptionsURL == "" {
return errors.New("can't use managed transport without a valid transport auth id")
}
managed.AddTransportOptions(authOpts.TransportOptionsURL, managed.TransportOptions{
TargetURL: url,
AuthOpts: authOpts,
ProxyOptions: &git2go.ProxyOptions{Type: git2go.ProxyTypeAuto},
Context: ctx,
})
return nil
}

func recoverPanic(err *error) {
if r := recover(); r != nil {
*err = fmt.Errorf("recovered from git2go panic: %v", r)
Expand Down
Loading

0 comments on commit d740f91

Please sign in to comment.