libgit2: add test coverage for HTTP proxy

Makefile

@@ -98,6 +98,7 @@ build: check-deps $(LIBGIT2) ## Build manager binary
 
 KUBEBUILDER_ASSETS?="$(shell $(ENVTEST) --arch=$(ENVTEST_ARCH) use -i $(ENVTEST_KUBERNETES_VERSION) --bin-dir=$(ENVTEST_ASSETS_DIR) -p path)"
 test: $(LIBGIT2) install-envtest test-api check-deps ## Run tests
+	HTTPS_PROXY="" HTTP_PROXY="" \
 	KUBEBUILDER_ASSETS=$(KUBEBUILDER_ASSETS) \
 	GIT_CONFIG_GLOBAL=/dev/null \
 	go test $(GO_STATIC_FLAGS) \

controllers/gitrepository_controller.go

@@ -455,38 +455,8 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context,
 		return sreconcile.ResultEmpty, e
 	}
 
-	repositoryURL := obj.Spec.URL
-	// managed GIT transport only affects the libgit2 implementation
-	if managed.Enabled() && obj.Spec.GitImplementation == sourcev1.LibGit2Implementation {
-		// At present only HTTP connections have the ability to define remote options.
-		// Although this can be easily extended by ensuring that the fake URL below uses the
-		// target ssh scheme, and the libgit2/managed/ssh.go pulls that information accordingly.
-		//
-		// This is due to the fact the key libgit2 remote callbacks do not take place for HTTP
-		// whilst most still work for SSH.
-		if strings.HasPrefix(repositoryURL, "http") {
-			// Due to the lack of the callback feature, a fake target URL is created to allow
-			// for the smart sub transport be able to pick the options specific for this
-			// GitRepository object.
-			// The URL should use unique information that do not collide in a multi tenant
-			// deployment.
-			repositoryURL = fmt.Sprintf("http://%s/%s/%d", obj.Name, obj.UID, obj.Generation)
-			managed.AddTransportOptions(repositoryURL,
-				managed.TransportOptions{
-					TargetURL: obj.Spec.URL,
-					CABundle:  authOpts.CAFile,
-				})
-
-			// We remove the options from memory, to avoid accumulating unused options over time.
-			defer managed.RemoveTransportOptions(repositoryURL)
-		}
-	}
-
 	// Fetch the included artifact metadata.
 	artifacts, err := r.fetchIncludes(ctx, obj)
-	if err != nil {
-		return sreconcile.ResultEmpty, err
-	}
 
 	// Observe if the artifacts still match the previous included ones
 	if artifacts.Diff(obj.Status.IncludedArtifacts) {
@@ -503,8 +473,15 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context,
 		optimizedClone = true
 	}
 
-	c, err := r.gitCheckout(ctx, obj, repositoryURL, authOpts, dir, optimizedClone)
+	c, err := r.gitCheckout(ctx, obj, authOpts, dir, optimizedClone)
 	if err != nil {
+		// If the error is a Stalling error, make sure to record and return that, because returning a Generic
+		// error, would force us to lose context.
+		var stalling *serror.Stalling
+		if errors.As(err, &stalling) {
+			conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, stalling.Reason, stalling.Err.Error())
+			return sreconcile.ResultEmpty, stalling
+		}
 		e := serror.NewGeneric(
 			fmt.Errorf("failed to checkout and determine revision: %w", err),
 			sourcev1.GitOperationFailedReason,
@@ -542,8 +519,15 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context,
 
 		// If we can't skip the reconciliation, checkout again without any
 		// optimization.
-		c, err := r.gitCheckout(ctx, obj, repositoryURL, authOpts, dir, false)
+		c, err := r.gitCheckout(ctx, obj, authOpts, dir, false)
 		if err != nil {
+			// If the error is a Stalling error, make sure to record and return that, because returning a Generic
+			// error, would force us to lose context.
+			var stalling *serror.Stalling
+			if errors.As(err, &stalling) {
+				conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, stalling.Reason, stalling.Err.Error())
+				return sreconcile.ResultEmpty, stalling
+			}
 			e := serror.NewGeneric(
 				fmt.Errorf("failed to checkout and determine revision: %w", err),
 				sourcev1.GitOperationFailedReason,
@@ -739,7 +723,7 @@ func (r *GitRepositoryReconciler) reconcileInclude(ctx context.Context,
 // gitCheckout builds checkout options with the given configurations and
 // performs a git checkout.
 func (r *GitRepositoryReconciler) gitCheckout(ctx context.Context,
-	obj *sourcev1.GitRepository, repoURL string, authOpts *git.AuthOptions, dir string, optimized bool) (*git.Commit, error) {
+	obj *sourcev1.GitRepository, authOpts *git.AuthOptions, dir string, optimized bool) (*git.Commit, error) {
 	// Configure checkout strategy.
 	checkoutOpts := git.CheckoutOptions{RecurseSubmodules: obj.Spec.RecurseSubmodules}
 	if ref := obj.Spec.Reference; ref != nil {
@@ -765,15 +749,33 @@ func (r *GitRepositoryReconciler) gitCheckout(ctx context.Context,
 			Err:    fmt.Errorf("failed to configure checkout strategy for Git implementation '%s': %w", obj.Spec.GitImplementation, err),
 			Reason: sourcev1.GitOperationFailedReason,
 		}
-		conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
 		// Do not return err as recovery without changes is impossible.
 		return nil, e
 	}
 
+	// managed GIT transport only affects the libgit2 implementation
+	if managed.Enabled() && obj.Spec.GitImplementation == sourcev1.LibGit2Implementation {
+		// We set the TransportOptionsURL of this set of authentication options here by constructing
+		// a unique ID that won't clash in a multi tenant environment. This unique ID 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 has invalid transport type: '%s'", obj.Spec.URL),
+				Reason: sourcev1.GitOperationFailedReason,
+			}
+			return nil, e
+		}
+	}
+
 	// Checkout HEAD of reference in object
 	gitCtx, cancel := context.WithTimeout(ctx, obj.Spec.Timeout.Duration)
 	defer cancel()
-	return checkoutStrategy.Checkout(gitCtx, dir, repoURL, authOpts)
+	return checkoutStrategy.Checkout(gitCtx, dir, obj.Spec.URL, authOpts)
 }
 
 // fetchIncludes fetches artifact metadata of all the included repos.

controllers/gitrepository_controller_test.go

@@ -919,6 +919,8 @@ func TestGitRepositoryReconciler_reconcileArtifact(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			g := NewWithT(t)
 
+			resetChmod(tt.dir, 0o755, 0o644)
+
 			r := &GitRepositoryReconciler{
 				EventRecorder: record.NewFakeRecorder(32),
 				Storage:       testStorage,
@@ -2142,3 +2144,24 @@ func TestGitRepositoryReconciler_calculateContentConfigChecksum(t *testing.T) {
 	artifactCsumModChecksum := r.calculateContentConfigChecksum(obj, artifacts)
 	g.Expect(artifactModChecksum).ToNot(Equal(artifactCsumModChecksum))
 }
+
+func resetChmod(path string, dirMode os.FileMode, fileMode os.FileMode) error {
+	err := filepath.Walk(path,
+		func(path string, info os.FileInfo, err error) error {
+			if err != nil {
+				return err
+			}
+
+			if info.IsDir() && info.Mode() != dirMode {
+				os.Chmod(path, dirMode)
+			} else if !info.IsDir() && info.Mode() != fileMode {
+				os.Chmod(path, fileMode)
+			}
+			return nil
+		})
+	if err != nil {
+		return fmt.Errorf("cannot reset file permissions: %v", err)
+	}
+
+	return nil
+}

controllers/helmrepository_controller_test.go

@@ -1166,9 +1166,11 @@ func TestHelmRepositoryReconciler_ReconcileTypeUpdatePredicateFilter(t *testing.
 		Name: secret.Name,
 	}
 
+	oldGen := obj.GetGeneration()
 	g.Expect(testEnv.Update(ctx, obj)).To(Succeed())
+	newGen := oldGen + 1
 
-	// Wait for HelmRepository to be Ready
+	// Wait for HelmRepository to be Ready with new generation.
 	g.Eventually(func() bool {
 		if err := testEnv.Get(ctx, key, obj); err != nil {
 			return false
@@ -1178,8 +1180,8 @@ func TestHelmRepositoryReconciler_ReconcileTypeUpdatePredicateFilter(t *testing.
 		}
 		readyCondition := conditions.Get(obj, meta.ReadyCondition)
 		return readyCondition.Status == metav1.ConditionTrue &&
-			obj.Generation == readyCondition.ObservedGeneration &&
-			obj.Generation == obj.Status.ObservedGeneration
+			newGen == readyCondition.ObservedGeneration &&
+			newGen == obj.Status.ObservedGeneration
 	}, timeout).Should(BeTrue())
 
 	// Check if the object status is valid.

go.mod

@@ -28,7 +28,7 @@ require (
 	github.com/fluxcd/pkg/helmtestserver v0.5.0
 	github.com/fluxcd/pkg/lockedfile v0.1.0
 	github.com/fluxcd/pkg/runtime v0.16.0
-	github.com/fluxcd/pkg/ssh v0.3.4
+	github.com/fluxcd/pkg/ssh v0.4.0
 	github.com/fluxcd/pkg/testserver v0.2.0
 	github.com/fluxcd/pkg/untar v0.1.0
 	github.com/fluxcd/pkg/version v0.1.0

go.sum

@@ -350,8 +350,8 @@ github.com/fluxcd/pkg/lockedfile v0.1.0 h1:YsYFAkd6wawMCcD74ikadAKXA4s2sukdxrn7w
 github.com/fluxcd/pkg/lockedfile v0.1.0/go.mod h1:EJLan8t9MiOcgTs8+puDjbE6I/KAfHbdvIy9VUgIjm8=
 github.com/fluxcd/pkg/runtime v0.16.0 h1:ynzvkOedFFZHlsa47EE7XtxZe8qs8edhtmjVZBEWi1Y=
 github.com/fluxcd/pkg/runtime v0.16.0/go.mod h1:Iklg+r/Jnqc9cNf2NK+iaosvw49CxX07Pyn0r3zSg/o=
-github.com/fluxcd/pkg/ssh v0.3.4 h1:Ko+MUNiiQG3evyoMO19iRk7d4X0VJ6w6+GEeVQ1jLC0=
-github.com/fluxcd/pkg/ssh v0.3.4/go.mod h1:KGgOUOy1uI6RC6+qxIBLvP1AeOOs/nLB25Ca6TZMIXE=
+github.com/fluxcd/pkg/ssh v0.4.0 h1:2HY88irZ5BCSMlzZExR6cnhRkjxCDsK/lTHHQqCJDJQ=
+github.com/fluxcd/pkg/ssh v0.4.0/go.mod h1:KGgOUOy1uI6RC6+qxIBLvP1AeOOs/nLB25Ca6TZMIXE=
 github.com/fluxcd/pkg/testserver v0.2.0 h1:Mj0TapmKaywI6Fi5wvt1LAZpakUHmtzWQpJNKQ0Krt4=
 github.com/fluxcd/pkg/testserver v0.2.0/go.mod h1:bgjjydkXsZTeFzjz9Cr4heGANr41uTB1Aj1Q5qzuYVk=
 github.com/fluxcd/pkg/untar v0.1.0 h1:k97V/xV5hFrAkIkVPuv5AVhyxh1ZzzAKba/lbDfGo6o=

internal/reconcile/reconcile.go

@@ -128,11 +128,11 @@ func ComputeReconcileResult(obj conditions.Setter, res Result, recErr error, rb
 	switch t := recErr.(type) {
 	case *serror.Stalling:
 		if res == ResultEmpty {
+			conditions.MarkStalled(obj, t.Reason, t.Error())
 			// The current generation has been reconciled successfully and it
 			// has resulted in a stalled state. Return no error to stop further
 			// requeuing.
-			pOpts = append(pOpts, patch.WithStatusObservedGeneration{})
-			conditions.MarkStalled(obj, t.Reason, t.Error())
+			pOpts = addPatchOptionWithStatusObservedGeneration(obj, pOpts)
 			return pOpts, result, nil
 		}
 		// NOTE: Non-empty result with stalling error indicates that the
@@ -150,7 +150,7 @@ func ComputeReconcileResult(obj conditions.Setter, res Result, recErr error, rb
 		if t.Ignore {
 			// The current generation has been reconciled successfully with
 			// no-op result. Update status observed generation.
-			pOpts = append(pOpts, patch.WithStatusObservedGeneration{})
+			pOpts = addPatchOptionWithStatusObservedGeneration(obj, pOpts)
 			conditions.Delete(obj, meta.ReconcilingCondition)
 			return pOpts, result, nil
 		}
@@ -159,7 +159,7 @@ func ComputeReconcileResult(obj conditions.Setter, res Result, recErr error, rb
 		// state. If a requeue is requested, the current generation has not been
 		// reconciled successfully.
 		if res != ResultRequeue {
-			pOpts = append(pOpts, patch.WithStatusObservedGeneration{})
+			pOpts = addPatchOptionWithStatusObservedGeneration(obj, pOpts)
 		}
 		conditions.Delete(obj, meta.StalledCondition)
 	default:
@@ -207,3 +207,17 @@ func FailureRecovery(oldObj, newObj conditions.Getter, failConditions []string)
 	}
 	return failuresBefore > 0
 }
+
+// addPatchOptionWithStatusObservedGeneration adds patch option
+// WithStatusObservedGeneration to the provided patch option slice only if there
+// is any condition present on the object, and returns it. This is necessary to
+// prevent setting status observed generation without any effectual observation.
+// An object must have some condition in the status if it has been observed.
+// TODO: Move this to fluxcd/pkg/runtime/patch package after it has proven its
+// need.
+func addPatchOptionWithStatusObservedGeneration(obj conditions.Setter, opts []patch.Option) []patch.Option {
+	if len(obj.GetConditions()) > 0 {
+		opts = append(opts, patch.WithStatusObservedGeneration{})
+	}
+	return opts
+}

internal/reconcile/reconcile_test.go

@@ -71,11 +71,17 @@ func TestComputeReconcileResult(t *testing.T) {
 		afterFunc        func(t *WithT, obj conditions.Setter, patchOpts *patch.HelperOptions)
 	}{
 		{
-			name:       "successful result",
-			result:     ResultSuccess,
+			name:   "successful result",
+			result: ResultSuccess,
+			beforeFunc: func(obj conditions.Setter) {
+				conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "foo")
+			},
 			recErr:     nil,
 			wantResult: ctrl.Result{RequeueAfter: testSuccessInterval},
 			wantErr:    false,
+			assertConditions: []metav1.Condition{
+				*conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "foo"),
+			},
 			afterFunc: func(t *WithT, obj conditions.Setter, patchOpts *patch.HelperOptions) {
 				t.Expect(patchOpts.IncludeStatusObservedGeneration).To(BeTrue())
 			},
@@ -85,10 +91,14 @@ func TestComputeReconcileResult(t *testing.T) {
 			result: ResultSuccess,
 			beforeFunc: func(obj conditions.Setter) {
 				conditions.MarkReconciling(obj, "NewRevision", "new revision")
+				conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "foo")
 			},
 			recErr:     nil,
 			wantResult: ctrl.Result{RequeueAfter: testSuccessInterval},
 			wantErr:    false,
+			assertConditions: []metav1.Condition{
+				*conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "foo"),
+			},
 			afterFunc: func(t *WithT, obj conditions.Setter, patchOpts *patch.HelperOptions) {
 				t.Expect(patchOpts.IncludeStatusObservedGeneration).To(BeTrue())
 				t.Expect(conditions.IsUnknown(obj, meta.ReconcilingCondition)).To(BeTrue())
@@ -367,3 +377,58 @@ func TestFailureRecovery(t *testing.T) {
 		})
 	}
 }
+
+func TestAddOptionWithStatusObservedGeneration(t *testing.T) {
+	tests := []struct {
+		name       string
+		beforeFunc func(obj conditions.Setter)
+		patchOpts  []patch.Option
+		want       bool
+	}{
+		{
+			name: "no conditions",
+			want: false,
+		},
+		{
+			name: "some condition",
+			beforeFunc: func(obj conditions.Setter) {
+				conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "foo")
+			},
+			want: true,
+		},
+		{
+			name: "existing option with conditions",
+			beforeFunc: func(obj conditions.Setter) {
+				conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "foo")
+			},
+			patchOpts: []patch.Option{patch.WithForceOverwriteConditions{}, patch.WithStatusObservedGeneration{}},
+			want:      true,
+		},
+		{
+			name:      "existing option, no conditions, can't remove",
+			patchOpts: []patch.Option{patch.WithForceOverwriteConditions{}, patch.WithStatusObservedGeneration{}},
+			want:      true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			g := NewWithT(t)
+
+			obj := &sourcev1.GitRepository{}
+
+			if tt.beforeFunc != nil {
+				tt.beforeFunc(obj)
+			}
+
+			tt.patchOpts = addPatchOptionWithStatusObservedGeneration(obj, tt.patchOpts)
+
+			// Apply the options and evaluate the result.
+			options := &patch.HelperOptions{}
+			for _, opt := range tt.patchOpts {
+				opt.ApplyToHelper(options)
+			}
+			g.Expect(options.IncludeStatusObservedGeneration).To(Equal(tt.want))
+		})
+	}
+}

main.go

@@ -312,6 +312,14 @@ func main() {
 
 	if enabled, _ := features.Enabled(features.GitManagedTransport); enabled {
 		managed.InitManagedTransport(ctrl.Log.WithName("managed-transport"))
+	} else {
+		if optimize, _ := feathelper.Enabled(features.OptimizedGitClones); optimize {
+			setupLog.Error(
+				fmt.Errorf("OptimizedGitClones=true but GitManagedTransport=false"),
+				"git clones can only be optimized when using managed transort",
+			)
+			os.Exit(1)
+		}
 	}
 
 	setupLog.Info("starting manager")