Merge pull request #33 from fluxcd/git-timeout

api: add timeout field to GitRepositorySpec

Author: hiddeco

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

@@ -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)://

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

@@ -21,7 +21,6 @@ import (
 	"fmt"
 	"io/ioutil"
 	"os"
-	"time"
 
 	"github.com/blang/semver"
 	"github.com/go-git/go-git/v5"
@@ -52,8 +51,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)
-	defer cancel()
+	ctx := context.Background()
 
 	var repo sourcev1.GitRepository
 	if err := r.Get(ctx, req.NamespacedName, &repo); err != nil {
@@ -174,7 +172,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",
@@ -186,6 +185,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
@@ -350,6 +350,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 {
@@ -374,6 +376,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/gitrepository_controller.go

@@ -21,7 +21,6 @@ import (
 	"fmt"
 	"io/ioutil"
 	"net/url"
-	"time"
 
 	"github.com/go-logr/logr"
 	"helm.sh/helm/v3/pkg/getter"
@@ -52,8 +51,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)
-	defer cancel()
+	ctx := context.Background()
 
 	var chart sourcev1.HelmChart
 	if err := r.Get(ctx, req.NamespacedName, &chart); err != nil {
@@ -78,7 +76,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 {
@@ -94,7 +92,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 {
@@ -132,7 +130,7 @@ func (r *HelmChartReconciler) SetupWithManagerAndOptions(mgr ctrl.Manager, opts
 		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)
@@ -182,7 +180,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
@@ -199,6 +197,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
@@ -246,7 +246,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")
 	}
@@ -270,6 +273,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 {
@@ -295,18 +300,23 @@ 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)
 	}
 	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()) {
-		chart.SetOwnerReferences(append(chart.GetOwnerReferences(),
-			*metav1.NewControllerRef(repository.GetObjectMeta(), repository.GroupVersionKind())))
-		return r.Update(ctx, chart)
+	if metav1.IsControlledBy(chart.GetObjectMeta(), repository.GetObjectMeta()) {
+		return nil
 	}
-	return nil
+	chart.SetOwnerReferences(append(chart.GetOwnerReferences(), *metav1.NewControllerRef(
+		repository.GetObjectMeta(), repository.GroupVersionKind(),
+	)))
+	return r.Update(ctx, chart)
 }

controllers/helmchart_controller.go

@@ -22,7 +22,6 @@ import (
 	"io/ioutil"
 	"net/url"
 	"path"
-	"time"
 
 	"github.com/go-logr/logr"
 	"helm.sh/helm/v3/pkg/getter"
@@ -54,8 +53,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)
-	defer cancel()
+	ctx := context.Background()
 
 	var repository sourcev1.HelmRepository
 	if err := r.Get(ctx, req.NamespacedName, &repository); err != nil {
@@ -80,7 +78,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 {
@@ -117,7 +115,7 @@ func (r *HelmRepositoryReconciler) SetupWithManagerAndOptions(mgr ctrl.Manager,
 		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
@@ -139,7 +137,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
@@ -156,6 +154,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
@@ -214,6 +214,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 {
@@ -239,6 +241,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)