diff --git a/docs/spec/v1beta2/gitrepositories.md b/docs/spec/v1beta2/gitrepositories.md index 2d95db474..293fd342a 100644 --- a/docs/spec/v1beta2/gitrepositories.md +++ b/docs/spec/v1beta2/gitrepositories.md @@ -388,16 +388,17 @@ Some Git providers like Azure DevOps _require_ the `libgit2` implementation, as their Git servers provide only support for the [v2 protocol](https://git-scm.com/docs/protocol-v2). -#### Experimental managed transport for `libgit2` Git implementation +#### Managed transport for `libgit2` Git implementation -The `libgit2` Git implementation supports a new experimental transport for +The `libgit2` Git implementation supports a new managed transport for improved reliability, adding timeout enforcement for Git network operations. -Opt-in by setting the environment variable `EXPERIMENTAL_GIT_TRANSPORT` to -`true` in the controller's Deployment. This will result in the low-level -transport being handled by the controller, instead of `libgit2`. -This may lead to an increased number of timeout messages in the logs, however -it will fix the bug in which Git operations make the controllers hang indefinitely. +This feature is enabled by default. It can be disabled by starting the +controller with the argument `--feature-gates=GitManagedTransport=false`. + +By disabling this feature the management of the transport is passed on to +`libgit2`, which may result in blocking Git operations leading the controllers +to hang indefinitely. #### Optimized Git clones diff --git a/internal/features/features.go b/internal/features/features.go index e03224af3..a7b4c1c21 100644 --- a/internal/features/features.go +++ b/internal/features/features.go @@ -30,12 +30,25 @@ const ( // the last revision is still the same at the target repository, // and if that is so, skips the reconciliation. OptimizedGitClones = "OptimizedGitClones" + + // GitManagedTransport implements a managed transport for GitRepository + // objects that use the libgit2 implementation. + // + // When enabled, improves the reliability of libgit2 reconciliations, + // by enforcing timeouts and ensuring libgit2 cannot hijack the process + // and hang it indefinitely. + GitManagedTransport = "GitManagedTransport" ) var features = map[string]bool{ // OptimizedGitClones // opt-out from v0.25 OptimizedGitClones: true, + + // GitManagedTransport + // opt-in from v0.22 (via environment variable) + // opt-out from v0.25 + GitManagedTransport: true, } // DefaultFeatureGates contains a list of all supported feature gates and diff --git a/main.go b/main.go index 7b003f461..8ce391e67 100644 --- a/main.go +++ b/main.go @@ -289,7 +289,7 @@ func main() { startFileServer(storage.BasePath, storageAddr, setupLog) }() - if managed.Enabled() { + if enabled, _ := features.Enabled(features.GitManagedTransport); enabled { managed.InitManagedTransport(ctrl.Log.WithName("managed-transport")) } diff --git a/pkg/git/libgit2/managed/flag.go b/pkg/git/libgit2/managed/flag.go deleted file mode 100644 index 2905c7719..000000000 --- a/pkg/git/libgit2/managed/flag.go +++ /dev/null @@ -1,34 +0,0 @@ -/* -Copyright 2022 The Flux authors - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package managed - -import ( - "os" - "strings" -) - -// Enabled defines whether the use of Managed Transport should be enabled. -// This is only affects git operations that uses libgit2 implementation. -// -// True is returned when the environment variable `EXPERIMENTAL_GIT_TRANSPORT` -// is detected with the value of `true` or `1`. -func Enabled() bool { - if v, ok := os.LookupEnv("EXPERIMENTAL_GIT_TRANSPORT"); ok { - return strings.ToLower(v) == "true" || v == "1" - } - return false -} diff --git a/pkg/git/libgit2/managed/init.go b/pkg/git/libgit2/managed/init.go index d0cac9564..f8969db9d 100644 --- a/pkg/git/libgit2/managed/init.go +++ b/pkg/git/libgit2/managed/init.go @@ -40,8 +40,18 @@ var ( debugLog logr.Logger traceLog logr.Logger + enabled bool ) +// Enabled defines whether the use of Managed Transport is enabled which +// is only true if InitManagedTransport was called successfully at least +// once. +// +// This is only affects git operations that uses libgit2 implementation. +func Enabled() bool { + return enabled +} + // InitManagedTransport initialises HTTP(S) and SSH managed transport // for git2go, and therefore only impact git operations using the // libgit2 implementation. @@ -57,7 +67,7 @@ func InitManagedTransport(log logr.Logger) error { var err error once.Do(func() { - log.Info("Enabling experimental managed transport") + log.Info("Initializing managed transport") debugLog = log.V(logger.DebugLevel) traceLog = log.V(logger.TraceLevel) @@ -66,6 +76,7 @@ func InitManagedTransport(log logr.Logger) error { } err = registerManagedSSH() + enabled = true }) return err diff --git a/pkg/git/libgit2/managed/managed_test.go b/pkg/git/libgit2/managed/managed_test.go index 7d87b9141..5bfd1c1ef 100644 --- a/pkg/git/libgit2/managed/managed_test.go +++ b/pkg/git/libgit2/managed/managed_test.go @@ -201,32 +201,6 @@ func TestOptions(t *testing.T) { } } -func TestFlagStatus(t *testing.T) { - if Enabled() { - t.Errorf("experimental transport should not be enabled by default") - } - - os.Setenv("EXPERIMENTAL_GIT_TRANSPORT", "true") - if !Enabled() { - t.Errorf("experimental transport should be enabled when env EXPERIMENTAL_GIT_TRANSPORT=true") - } - - os.Setenv("EXPERIMENTAL_GIT_TRANSPORT", "1") - if !Enabled() { - t.Errorf("experimental transport should be enabled when env EXPERIMENTAL_GIT_TRANSPORT=1") - } - - os.Setenv("EXPERIMENTAL_GIT_TRANSPORT", "somethingelse") - if Enabled() { - t.Errorf("experimental transport should be enabled only when env EXPERIMENTAL_GIT_TRANSPORT is 1 or true but was enabled for 'somethingelse'") - } - - os.Unsetenv("EXPERIMENTAL_GIT_TRANSPORT") - if Enabled() { - t.Errorf("experimental transport should not be enabled when env EXPERIMENTAL_GIT_TRANSPORT is not present") - } -} - func TestManagedTransport_E2E(t *testing.T) { g := NewWithT(t)