Skip to content

Commit

Permalink
libgit2: enforce context timeout
Browse files Browse the repository at this point in the history
Some scenarios could lead a goroutine to be running indefinetely within managed ssh.
Previously between the two git operations, the reconciliation
could take twice the timeout set for the Flux object.

Signed-off-by: Paulo Gomes <paulo.gomes@weave.works>
  • Loading branch information
Paulo Gomes committed May 27, 2022
1 parent 7953d0e commit 978148e
Show file tree
Hide file tree
Showing 5 changed files with 34 additions and 12 deletions.
9 changes: 4 additions & 5 deletions controllers/gitrepository_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -721,7 +721,10 @@ func (r *GitRepositoryReconciler) gitCheckout(ctx context.Context,
}
}

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

checkoutStrategy, err := strategy.CheckoutStrategyForImplementation(gitCtx,
git.Implementation(obj.Spec.GitImplementation), checkoutOpts)
if err != nil {
// Do not return err as recovery without changes is impossible.
Expand Down Expand Up @@ -753,10 +756,6 @@ func (r *GitRepositoryReconciler) gitCheckout(ctx context.Context,
}
}

// Checkout HEAD of reference in object
gitCtx, cancel := context.WithTimeout(ctx, obj.Spec.Timeout.Duration)
defer cancel()

commit, err := checkoutStrategy.Checkout(gitCtx, dir, obj.Spec.URL, authOpts)
if err != nil {
e := serror.NewGeneric(
Expand Down
1 change: 1 addition & 0 deletions pkg/git/libgit2/checkout.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ func (c *CheckoutBranch) Checkout(ctx context.Context, path, url string, opts *g
TargetURL: url,
AuthOpts: opts,
ProxyOptions: &git2go.ProxyOptions{Type: git2go.ProxyTypeAuto},
Context: ctx,
})
url = opts.TransportOptionsURL
remoteCallBacks := managed.RemoteCallbacks()
Expand Down
2 changes: 2 additions & 0 deletions pkg/git/libgit2/managed/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package managed

import (
"context"
"sync"

"github.com/fluxcd/source-controller/pkg/git"
Expand All @@ -29,6 +30,7 @@ type TransportOptions struct {
TargetURL string
AuthOpts *git.AuthOptions
ProxyOptions *git2go.ProxyOptions
Context context.Context
}

var (
Expand Down
32 changes: 26 additions & 6 deletions pkg/git/libgit2/managed/ssh.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ type sshSmartSubtransport struct {
currentStream *sshSmartSubtransportStream
addr string
connected bool
ctx context.Context
}

func (t *sshSmartSubtransport) Action(transportOptionsURL string, action git2go.SmartServiceAction) (git2go.SmartSubtransportStream, error) {
Expand All @@ -103,6 +104,8 @@ func (t *sshSmartSubtransport) Action(transportOptionsURL string, action git2go.
return nil, fmt.Errorf("could not find transport options for object: %s", transportOptionsURL)
}

t.ctx = opts.Context

u, err := url.Parse(opts.TargetURL)
if err != nil {
return nil, err
Expand Down Expand Up @@ -206,16 +209,33 @@ func (t *sshSmartSubtransport) Action(transportOptionsURL string, action git2go.
// xref: https://github.com/golang/crypto/blob/eb4f295cb31f7fb5d52810411604a2638c9b19a2/ssh/session.go#L553-L558
go func() error {
defer w.Close()

var cancel context.CancelFunc
ctx := t.ctx

// When context is nil, creates a new with internal SSH connection timeout.
if ctx == nil {
ctx, cancel = context.WithTimeout(context.Background(), sshConnectionTimeOut)
defer cancel()
}

for {
if !t.connected {
select {
case <-ctx.Done():
t.Close()
return nil
}

_, err := io.Copy(w, reader)
if err != nil {
return err
default:
if !t.connected {
return nil
}

_, err := io.Copy(w, reader)
if err != nil {
return err
}
time.Sleep(5 * time.Millisecond)
}
time.Sleep(5 * time.Millisecond)
}
}()

Expand Down
2 changes: 1 addition & 1 deletion pkg/git/strategy/proxy/strategy_proxy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,7 @@ func TestCheckoutStrategyForImplementation_Proxied(t *testing.T) {

return nil, func() {}
},
shortTimeout: false,
shortTimeout: true,
wantUsedProxy: false,
wantError: true,
},
Expand Down

0 comments on commit 978148e

Please sign in to comment.