api: add timeout field to GitRepositorySpec

This commit adds a timeout field to the GitRepositorySpec to be used
during the git clone operation when reconciling the resource.
When no interval is defined the default timeout returned by the getter
is 20 seconds.

The timeout can not be added yet to the Helm related sources as it
is currently not possible to inject anything custom into the HTTP
client from the Helm HTTP getter except for the authentication
options built in. A submit has been submitted to make this possible
and is waiting for review.

This commit includes some context changes to the other reconcilers
to tidy them up and make them depend on a single background context.

It also includes some added docblocks that crossed my path.

api/v1alpha1/gitrepository_types.go

@@ -17,6 +17,8 @@ limitations under the License.
 package v1alpha1
 
 import (
+	"time"
+
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
@@ -40,6 +42,11 @@ type GitRepositorySpec struct {
 	// +required
 	Interval metav1.Duration `json:"interval"`
 
+	// The timeout for remote git operations like cloning.
+	// +kubebuilder:validation:Default=20s
+	// +optional
+	Timeout *metav1.Duration `json:"timeout,omitempty"`
+
 	// The git reference to checkout and monitor for changes, defaults to
 	// master branch.
 	// +optional
@@ -105,6 +112,10 @@ const (
 	GitOperationFailedReason string = "GitOperationFailed"
 )
 
+// GitRepositoryReady sets the given artifact and url on the
+// HelmRepository and resets the conditions to SourceCondition of
+// type Ready with status true and the given reason and message.
+// It returns the modified GitRepository.
 func GitRepositoryReady(repository GitRepository, artifact Artifact, url, reason, message string) GitRepository {
 	repository.Status.Conditions = []SourceCondition{
 		{
@@ -128,6 +139,9 @@ func GitRepositoryReady(repository GitRepository, artifact Artifact, url, reason
 	return repository
 }
 
+// GitRepositoryNotReady resets the conditions of the HelmRepository
+// to SourceCondition of type Ready with status false and the given
+// reason and message. It returns the modified GitRepository.
 func GitRepositoryNotReady(repository GitRepository, reason, message string) GitRepository {
 	repository.Status.Conditions = []SourceCondition{
 		{
@@ -141,15 +155,25 @@ func GitRepositoryNotReady(repository GitRepository, reason, message string) Git
 	return repository
 }
 
+// ReadyMessage returns the message of the SourceCondition
+// of type Ready with status true if present, or an empty string.
 func GitRepositoryReadyMessage(repository GitRepository) string {
 	for _, condition := range repository.Status.Conditions {
-		if condition.Type == ReadyCondition {
+		if condition.Type == ReadyCondition && condition.Status == corev1.ConditionTrue {
 			return condition.Message
 		}
 	}
 	return ""
 }
 
+// GetTimeout returns the configured timeout or the default.
+func (in *GitRepository) GetTimeout() time.Duration {
+	if in.Spec.Timeout != nil {
+		return in.Spec.Timeout.Duration
+	}
+	return time.Second * 20
+}
+
 // GetArtifact returns the latest artifact from the source
 // if present in the status sub-resource.
 func (in *GitRepository) GetArtifact() *Artifact {

api/v1alpha1/helmchart_types.go

@@ -66,6 +66,10 @@ const (
 	ChartPullSucceededReason string = "ChartPullSucceeded"
 )
 
+// HelmChartReady sets the given artifact and url on the HelmChart
+// and resets the conditions to SourceCondition of type Ready with
+// status true and the given reason and message. It returns the
+// modified HelmChart.
 func HelmChartReady(chart HelmChart, artifact Artifact, url, reason, message string) HelmChart {
 	chart.Status.Conditions = []SourceCondition{
 		{
@@ -89,6 +93,9 @@ func HelmChartReady(chart HelmChart, artifact Artifact, url, reason, message str
 	return chart
 }
 
+// HelmChartNotReady resets the conditions of the HelmChart to
+// SourceCondition of type Ready with status false and the given
+// reason and message. It returns the modified HelmChart.
 func HelmChartNotReady(chart HelmChart, reason, message string) HelmChart {
 	chart.Status.Conditions = []SourceCondition{
 		{
@@ -102,6 +109,8 @@ func HelmChartNotReady(chart HelmChart, reason, message string) HelmChart {
 	return chart
 }
 
+// HelmChartReadyMessage returns the message of the SourceCondition
+// of type Ready with status true if present, or an empty string.
 func HelmChartReadyMessage(chart HelmChart) string {
 	for _, condition := range chart.Status.Conditions {
 		if condition.Type == ReadyCondition && condition.Status == corev1.ConditionTrue {

api/v1alpha1/helmrepository_types.go

@@ -65,6 +65,10 @@ const (
 	IndexationSucceededReason string = "IndexationSucceed"
 )
 
+// HelmRepositoryReady sets the given artifact and url on the
+// HelmRepository and resets the conditions to SourceCondition of
+// type Ready with status true and the given reason and message.
+// It returns the modified HelmRepository.
 func HelmRepositoryReady(repository HelmRepository, artifact Artifact, url, reason, message string) HelmRepository {
 	repository.Status.Conditions = []SourceCondition{
 		{
@@ -88,6 +92,9 @@ func HelmRepositoryReady(repository HelmRepository, artifact Artifact, url, reas
 	return repository
 }
 
+// HelmRepositoryNotReady resets the conditions of the HelmRepository
+// to SourceCondition of type Ready with status false and the given
+// reason and message. It returns the modified HelmRepository.
 func HelmRepositoryNotReady(repository HelmRepository, reason, message string) HelmRepository {
 	repository.Status.Conditions = []SourceCondition{
 		{
@@ -101,6 +108,8 @@ func HelmRepositoryNotReady(repository HelmRepository, reason, message string) H
 	return repository
 }
 
+// HelmRepositoryReadyMessage returns the message of the SourceCondition
+// of type Ready with status true if present, or an empty string.
 func HelmRepositoryReadyMessage(repository HelmRepository) string {
 	for _, condition := range repository.Status.Conditions {
 		if condition.Type == ReadyCondition && condition.Status == corev1.ConditionTrue {

api/v1alpha1/zz_generated.deepcopy.go

@@ -22,6 +22,7 @@ package v1alpha1
 
 import (
 	"k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	runtime "k8s.io/apimachinery/pkg/runtime"
 )
 
@@ -124,6 +125,11 @@ func (in *GitRepositorySpec) DeepCopyInto(out *GitRepositorySpec) {
 		**out = **in
 	}
 	out.Interval = in.Interval
+	if in.Timeout != nil {
+		in, out := &in.Timeout, &out.Timeout
+		*out = new(metav1.Duration)
+		**out = **in
+	}
 	if in.Reference != nil {
 		in, out := &in.Reference, &out.Reference
 		*out = new(GitRepositoryRef)

config/crd/bases/source.fluxcd.io_gitrepositories.yaml

@@ -82,6 +82,9 @@ spec:
                     TODO: Add other useful fields. apiVersion, kind, uid?'
                   type: string
               type: object
+            timeout:
+              description: The timeout for remote git operations like cloning.
+              type: string
             url:
               description: The repository URL, can be a HTTP or SSH address.
               pattern: ^(http|https|ssh)://

controllers/gitrepository_controller.go

@@ -21,7 +21,6 @@ import (
 	"fmt"
 	"io/ioutil"
 	"os"
-	"time"
 
 	"github.com/blang/semver"
 	"github.com/go-git/go-git/v5"
@@ -51,8 +50,7 @@ type GitRepositoryReconciler struct {
 // +kubebuilder:rbac:groups=source.fluxcd.io,resources=gitrepositories/status,verbs=get;update;patch
 
 func (r *GitRepositoryReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {
-	ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second)
+	ctx := context.Background()
-	defer cancel()
 
 	var repo sourcev1.GitRepository
 	if err := r.Get(ctx, req.NamespacedName, &repo); err != nil {
@@ -164,7 +162,8 @@ func (r *GitRepositoryReconciler) sync(ctx context.Context, repository sourcev1.
 	defer os.RemoveAll(tmpGit)
 
 	// clone to tmp
-	repo, err := git.PlainClone(tmpGit, false, &git.CloneOptions{
+	gitCtx, cancel := context.WithTimeout(ctx, repository.GetTimeout())
+	repo, err := git.PlainCloneContext(gitCtx, tmpGit, false, &git.CloneOptions{
 		URL:               repository.Spec.URL,
 		Auth:              auth,
 		RemoteName:        "origin",
@@ -176,6 +175,7 @@ func (r *GitRepositoryReconciler) sync(ctx context.Context, repository sourcev1.
 		Progress:          nil,
 		Tags:              tagMode,
 	})
+	cancel()
 	if err != nil {
 		err = fmt.Errorf("git clone error: %w", err)
 		return sourcev1.GitRepositoryNotReady(repository, sourcev1.GitOperationFailedReason, err.Error()), err
@@ -340,6 +340,8 @@ func (r *GitRepositoryReconciler) sync(ctx context.Context, repository sourcev1.
 	return sourcev1.GitRepositoryReady(repository, artifact, url, sourcev1.GitOperationSucceedReason, message), nil
 }
 
+// shouldResetStatus returns a boolean indicating if the status of the
+// given repository should be reset and a reset HelmChartStatus.
 func (r *GitRepositoryReconciler) shouldResetStatus(repository sourcev1.GitRepository) (bool, sourcev1.GitRepositoryStatus) {
 	resetStatus := false
 	if repository.Status.Artifact != nil {
@@ -364,6 +366,8 @@ func (r *GitRepositoryReconciler) shouldResetStatus(repository sourcev1.GitRepos
 	}
 }
 
+// gc performs a garbage collection on all but current artifacts of
+// the given repository.
 func (r *GitRepositoryReconciler) gc(repository sourcev1.GitRepository) error {
 	if repository.Status.Artifact != nil {
 		return r.Storage.RemoveAllButCurrent(*repository.Status.Artifact)

controllers/helmchart_controller.go

@@ -21,7 +21,6 @@ import (
 	"fmt"
 	"io/ioutil"
 	"net/url"
-	"time"
 
 	"github.com/go-logr/logr"
 	"helm.sh/helm/v3/pkg/getter"
@@ -51,8 +50,7 @@ type HelmChartReconciler struct {
 // +kubebuilder:rbac:groups=source.fluxcd.io,resources=helmcharts/status,verbs=get;update;patch
 
 func (r *HelmChartReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {
-	ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second)
+	ctx := context.Background()
-	defer cancel()
 
 	var chart sourcev1.HelmChart
 	if err := r.Get(ctx, req.NamespacedName, &chart); err != nil {
@@ -77,7 +75,7 @@ func (r *HelmChartReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {
 	}
 
 	// get referenced chart repository
-	repository, err := r.chartRepository(ctx, chart)
+	repository, err := r.getChartRepositoryWithArtifact(ctx, chart)
 	if err != nil {
 		chart = sourcev1.HelmChartNotReady(*chart.DeepCopy(), sourcev1.ChartPullFailedReason, err.Error())
 		if err := r.Status().Update(ctx, &chart); err != nil {
@@ -93,7 +91,7 @@ func (r *HelmChartReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {
 	}
 
 	// try to pull chart
-	pulledChart, err := r.sync(repository, *chart.DeepCopy())
+	pulledChart, err := r.sync(ctx, repository, *chart.DeepCopy())
 	if err != nil {
 		log.Error(err, "Helm chart sync failed")
 		if err := r.Status().Update(ctx, &pulledChart); err != nil {
@@ -122,7 +120,7 @@ func (r *HelmChartReconciler) SetupWithManager(mgr ctrl.Manager) error {
 		Complete(r)
 }
 
-func (r *HelmChartReconciler) sync(repository sourcev1.HelmRepository, chart sourcev1.HelmChart) (sourcev1.HelmChart, error) {
+func (r *HelmChartReconciler) sync(ctx context.Context, repository sourcev1.HelmRepository, chart sourcev1.HelmChart) (sourcev1.HelmChart, error) {
 	indexBytes, err := ioutil.ReadFile(repository.Status.Artifact.Path)
 	if err != nil {
 		err = fmt.Errorf("failed to read Helm repository index file: %w", err)
@@ -172,7 +170,7 @@ func (r *HelmChartReconciler) sync(repository sourcev1.HelmRepository, chart sou
 		}
 
 		var secret corev1.Secret
-		err := r.Client.Get(context.TODO(), name, &secret)
+		err := r.Client.Get(ctx, name, &secret)
 		if err != nil {
 			err = fmt.Errorf("auth secret error: %w", err)
 			return sourcev1.HelmChartNotReady(chart, sourcev1.AuthenticationFailedReason, err.Error()), err
@@ -189,6 +187,8 @@ func (r *HelmChartReconciler) sync(repository sourcev1.HelmRepository, chart sou
 		clientOpts = opts
 	}
 
+	// TODO(hidde): implement timeout from the HelmRepository
+	//  https://github.com/helm/helm/pull/7950
 	res, err := c.Get(u.String(), clientOpts...)
 	if err != nil {
 		return sourcev1.HelmChartNotReady(chart, sourcev1.ChartPullFailedReason, err.Error()), err
@@ -236,7 +236,10 @@ func (r *HelmChartReconciler) sync(repository sourcev1.HelmRepository, chart sou
 	return sourcev1.HelmChartReady(chart, artifact, chartUrl, sourcev1.ChartPullSucceededReason, message), nil
 }
 
-func (r *HelmChartReconciler) chartRepository(ctx context.Context, chart sourcev1.HelmChart) (sourcev1.HelmRepository, error) {
+// getChartRepositoryWithArtifact attempts to get the ChartRepository
+// for the given chart. It returns an error if the HelmRepository could
+// not be retrieved or if does not have an artifact.
+func (r *HelmChartReconciler) getChartRepositoryWithArtifact(ctx context.Context, chart sourcev1.HelmChart) (sourcev1.HelmRepository, error) {
 	if chart.Spec.HelmRepositoryRef.Name == "" {
 		return sourcev1.HelmRepository{}, fmt.Errorf("no HelmRepository reference given")
 	}
@@ -260,6 +263,8 @@ func (r *HelmChartReconciler) chartRepository(ctx context.Context, chart sourcev
 	return repository, err
 }
 
+// shouldResetStatus returns a boolean indicating if the status of the
+// given chart should be reset and a reset HelmChartStatus.
 func (r *HelmChartReconciler) shouldResetStatus(chart sourcev1.HelmChart) (bool, sourcev1.HelmChartStatus) {
 	resetStatus := false
 	if chart.Status.Artifact != nil {
@@ -285,6 +290,8 @@ func (r *HelmChartReconciler) shouldResetStatus(chart sourcev1.HelmChart) (bool,
 	}
 }
 
+// gc performs a garbage collection on all but current artifacts of
+// the given chart.
 func (r *HelmChartReconciler) gc(chart sourcev1.HelmChart) error {
 	if chart.Status.Artifact != nil {
 		return r.Storage.RemoveAllButCurrent(*chart.Status.Artifact)
@@ -292,11 +299,14 @@ func (r *HelmChartReconciler) gc(chart sourcev1.HelmChart) error {
 	return nil
 }
 
+// setOwnerRef appends the owner reference of the given chart to the
+// repository if it is not present.
 func (r *HelmChartReconciler) setOwnerRef(ctx context.Context, chart *sourcev1.HelmChart, repository sourcev1.HelmRepository) error {
-	if !metav1.IsControlledBy(chart.GetObjectMeta(), repository.GetObjectMeta()) {
+	if metav1.IsControlledBy(chart.GetObjectMeta(), repository.GetObjectMeta()) {
-		chart.SetOwnerReferences(append(chart.GetOwnerReferences(),
+		return nil
-			*metav1.NewControllerRef(repository.GetObjectMeta(), repository.GroupVersionKind())))
-		return r.Update(ctx, chart)
 	}
-	return nil
+	chart.SetOwnerReferences(append(chart.GetOwnerReferences(), *metav1.NewControllerRef(
+		repository.GetObjectMeta(), repository.GroupVersionKind(),
+	)))
+	return r.Update(ctx, chart)
 }

controllers/helmrepository_controller.go

@@ -22,7 +22,6 @@ import (
 	"io/ioutil"
 	"net/url"
 	"path"
-	"time"
 
 	"github.com/go-logr/logr"
 	"helm.sh/helm/v3/pkg/getter"
@@ -53,8 +52,7 @@ type HelmRepositoryReconciler struct {
 // +kubebuilder:rbac:groups=source.fluxcd.io,resources=helmcharts/finalizers,verbs=get;update;patch
 
 func (r *HelmRepositoryReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {
-	ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second)
+	ctx := context.Background()
-	defer cancel()
 
 	var repository sourcev1.HelmRepository
 	if err := r.Get(ctx, req.NamespacedName, &repository); err != nil {
@@ -79,7 +77,7 @@ func (r *HelmRepositoryReconciler) Reconcile(req ctrl.Request) (ctrl.Result, err
 	}
 
 	// try to download index
-	syncedRepo, err := r.sync(*repository.DeepCopy())
+	syncedRepo, err := r.sync(ctx, *repository.DeepCopy())
 	if err != nil {
 		log.Error(err, "Helm repository sync failed")
 		if err := r.Status().Update(ctx, &syncedRepo); err != nil {
@@ -107,7 +105,7 @@ func (r *HelmRepositoryReconciler) SetupWithManager(mgr ctrl.Manager) error {
 		Complete(r)
 }
 
-func (r *HelmRepositoryReconciler) sync(repository sourcev1.HelmRepository) (sourcev1.HelmRepository, error) {
+func (r *HelmRepositoryReconciler) sync(ctx context.Context, repository sourcev1.HelmRepository) (sourcev1.HelmRepository, error) {
 	u, err := url.Parse(repository.Spec.URL)
 	if err != nil {
 		return sourcev1.HelmRepositoryNotReady(repository, sourcev1.URLInvalidReason, err.Error()), err
@@ -129,7 +127,7 @@ func (r *HelmRepositoryReconciler) sync(repository sourcev1.HelmRepository) (sou
 		}
 
 		var secret corev1.Secret
-		err := r.Client.Get(context.TODO(), name, &secret)
+		err := r.Client.Get(ctx, name, &secret)
 		if err != nil {
 			err = fmt.Errorf("auth secret error: %w", err)
 			return sourcev1.HelmRepositoryNotReady(repository, sourcev1.AuthenticationFailedReason, err.Error()), err
@@ -146,6 +144,8 @@ func (r *HelmRepositoryReconciler) sync(repository sourcev1.HelmRepository) (sou
 		clientOpts = opts
 	}
 
+	// TODO(hidde): implement timeout from the HelmRepository
+	//  https://github.com/helm/helm/pull/7950
 	res, err := c.Get(u.String(), clientOpts...)
 	if err != nil {
 		return sourcev1.HelmRepositoryNotReady(repository, sourcev1.IndexationFailedReason, err.Error()), err
@@ -204,6 +204,8 @@ func (r *HelmRepositoryReconciler) sync(repository sourcev1.HelmRepository) (sou
 	return sourcev1.HelmRepositoryReady(repository, artifact, indexURL, sourcev1.IndexationSucceededReason, message), nil
 }
 
+// shouldResetStatus returns a boolean indicating if the status of the
+// given repository should be reset and a reset HelmChartStatus.
 func (r *HelmRepositoryReconciler) shouldResetStatus(repository sourcev1.HelmRepository) (bool, sourcev1.HelmRepositoryStatus) {
 	resetStatus := false
 	if repository.Status.Artifact != nil {
@@ -229,6 +231,8 @@ func (r *HelmRepositoryReconciler) shouldResetStatus(repository sourcev1.HelmRep
 	}
 }
 
+// gc performs a garbage collection on all but current artifacts of
+// the given repository.
 func (r *HelmRepositoryReconciler) gc(repository sourcev1.HelmRepository) error {
 	if repository.Status.Artifact != nil {
 		return r.Storage.RemoveAllButCurrent(*repository.Status.Artifact)