From 920d37fcdaca74d23e2ae69525a8f31995840e8e Mon Sep 17 00:00:00 2001 From: Hidde Beydals Date: Wed, 22 Apr 2020 01:25:33 +0200 Subject: [PATCH] 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 | 26 +++++++++++++- api/v1alpha1/helmchart_types.go | 9 +++++ api/v1alpha1/helmrepository_types.go | 9 +++++ api/v1alpha1/zz_generated.deepcopy.go | 6 ++++ .../source.fluxcd.io_gitrepositories.yaml | 3 ++ controllers/gitrepository_controller.go | 12 ++++--- controllers/helmchart_controller.go | 36 ++++++++++++------- controllers/helmrepository_controller.go | 16 +++++---- 8 files changed, 93 insertions(+), 24 deletions(-) diff --git a/api/v1alpha1/gitrepository_types.go b/api/v1alpha1/gitrepository_types.go index c2560564..323667af 100644 --- a/api/v1alpha1/gitrepository_types.go +++ b/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 { diff --git a/api/v1alpha1/helmchart_types.go b/api/v1alpha1/helmchart_types.go index b61f82c4..0f3bef06 100644 --- a/api/v1alpha1/helmchart_types.go +++ b/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 { diff --git a/api/v1alpha1/helmrepository_types.go b/api/v1alpha1/helmrepository_types.go index aab539eb..17c9985c 100644 --- a/api/v1alpha1/helmrepository_types.go +++ b/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 { diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index cabc171b..f97d00a3 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/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) diff --git a/config/crd/bases/source.fluxcd.io_gitrepositories.yaml b/config/crd/bases/source.fluxcd.io_gitrepositories.yaml index 7fdd6fec..3b1ac754 100644 --- a/config/crd/bases/source.fluxcd.io_gitrepositories.yaml +++ b/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):// diff --git a/controllers/gitrepository_controller.go b/controllers/gitrepository_controller.go index b76fb1b1..90188635 100644 --- a/controllers/gitrepository_controller.go +++ b/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) - defer cancel() + ctx := context.Background() 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) diff --git a/controllers/helmchart_controller.go b/controllers/helmchart_controller.go index b726ff36..ca38f3b6 100644 --- a/controllers/helmchart_controller.go +++ b/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) - defer cancel() + ctx := context.Background() 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()) { - 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) } diff --git a/controllers/helmrepository_controller.go b/controllers/helmrepository_controller.go index 15f24ec2..4cb69b6a 100644 --- a/controllers/helmrepository_controller.go +++ b/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) - defer cancel() + ctx := context.Background() 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)