Skip to content

Commit 920d37f

Browse files
hiddecohiddeco
committed
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.
1 parent 2f1390d commit 920d37f

File tree

8 files changed

+93
-24
lines changed

8 files changed

+93
-24
lines changed

‎api/v1alpha1/gitrepository_types.go‎

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@ limitations under the License.
1717
package v1alpha1
1818

1919
import (
20+
"time"
21+
2022
corev1 "k8s.io/api/core/v1"
2123
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2224
)
@@ -40,6 +42,11 @@ type GitRepositorySpec struct {
4042
// +required
4143
Interval metav1.Duration `json:"interval"`
4244

45+
// The timeout for remote git operations like cloning.
46+
// +kubebuilder:validation:Default=20s
47+
// +optional
48+
Timeout *metav1.Duration `json:"timeout,omitempty"`
49+
4350
// The git reference to checkout and monitor for changes, defaults to
4451
// master branch.
4552
// +optional
@@ -105,6 +112,10 @@ const (
105112
GitOperationFailedReason string = "GitOperationFailed"
106113
)
107114

115+
// GitRepositoryReady sets the given artifact and url on the
116+
// HelmRepository and resets the conditions to SourceCondition of
117+
// type Ready with status true and the given reason and message.
118+
// It returns the modified GitRepository.
108119
func GitRepositoryReady(repository GitRepository, artifact Artifact, url, reason, message string) GitRepository {
109120
repository.Status.Conditions = []SourceCondition{
110121
{
@@ -128,6 +139,9 @@ func GitRepositoryReady(repository GitRepository, artifact Artifact, url, reason
128139
return repository
129140
}
130141

142+
// GitRepositoryNotReady resets the conditions of the HelmRepository
143+
// to SourceCondition of type Ready with status false and the given
144+
// reason and message. It returns the modified GitRepository.
131145
func GitRepositoryNotReady(repository GitRepository, reason, message string) GitRepository {
132146
repository.Status.Conditions = []SourceCondition{
133147
{
@@ -141,15 +155,25 @@ func GitRepositoryNotReady(repository GitRepository, reason, message string) Git
141155
return repository
142156
}
143157

158+
// ReadyMessage returns the message of the SourceCondition
159+
// of type Ready with status true if present, or an empty string.
144160
func GitRepositoryReadyMessage(repository GitRepository) string {
145161
for _, condition := range repository.Status.Conditions {
146-
if condition.Type == ReadyCondition {
162+
if condition.Type == ReadyCondition && condition.Status == corev1.ConditionTrue {
147163
return condition.Message
148164
}
149165
}
150166
return ""
151167
}
152168

169+
// GetTimeout returns the configured timeout or the default.
170+
func (in *GitRepository) GetTimeout() time.Duration {
171+
if in.Spec.Timeout != nil {
172+
return in.Spec.Timeout.Duration
173+
}
174+
return time.Second * 20
175+
}
176+
153177
// GetArtifact returns the latest artifact from the source
154178
// if present in the status sub-resource.
155179
func (in *GitRepository) GetArtifact() *Artifact {

‎api/v1alpha1/helmchart_types.go‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,10 @@ const (
6666
ChartPullSucceededReason string = "ChartPullSucceeded"
6767
)
6868

69+
// HelmChartReady sets the given artifact and url on the HelmChart
70+
// and resets the conditions to SourceCondition of type Ready with
71+
// status true and the given reason and message. It returns the
72+
// modified HelmChart.
6973
func HelmChartReady(chart HelmChart, artifact Artifact, url, reason, message string) HelmChart {
7074
chart.Status.Conditions = []SourceCondition{
7175
{
@@ -89,6 +93,9 @@ func HelmChartReady(chart HelmChart, artifact Artifact, url, reason, message str
8993
return chart
9094
}
9195

96+
// HelmChartNotReady resets the conditions of the HelmChart to
97+
// SourceCondition of type Ready with status false and the given
98+
// reason and message. It returns the modified HelmChart.
9299
func HelmChartNotReady(chart HelmChart, reason, message string) HelmChart {
93100
chart.Status.Conditions = []SourceCondition{
94101
{
@@ -102,6 +109,8 @@ func HelmChartNotReady(chart HelmChart, reason, message string) HelmChart {
102109
return chart
103110
}
104111

112+
// HelmChartReadyMessage returns the message of the SourceCondition
113+
// of type Ready with status true if present, or an empty string.
105114
func HelmChartReadyMessage(chart HelmChart) string {
106115
for _, condition := range chart.Status.Conditions {
107116
if condition.Type == ReadyCondition && condition.Status == corev1.ConditionTrue {

‎api/v1alpha1/helmrepository_types.go‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,10 @@ const (
6565
IndexationSucceededReason string = "IndexationSucceed"
6666
)
6767

68+
// HelmRepositoryReady sets the given artifact and url on the
69+
// HelmRepository and resets the conditions to SourceCondition of
70+
// type Ready with status true and the given reason and message.
71+
// It returns the modified HelmRepository.
6872
func HelmRepositoryReady(repository HelmRepository, artifact Artifact, url, reason, message string) HelmRepository {
6973
repository.Status.Conditions = []SourceCondition{
7074
{
@@ -88,6 +92,9 @@ func HelmRepositoryReady(repository HelmRepository, artifact Artifact, url, reas
8892
return repository
8993
}
9094

95+
// HelmRepositoryNotReady resets the conditions of the HelmRepository
96+
// to SourceCondition of type Ready with status false and the given
97+
// reason and message. It returns the modified HelmRepository.
9198
func HelmRepositoryNotReady(repository HelmRepository, reason, message string) HelmRepository {
9299
repository.Status.Conditions = []SourceCondition{
93100
{
@@ -101,6 +108,8 @@ func HelmRepositoryNotReady(repository HelmRepository, reason, message string) H
101108
return repository
102109
}
103110

111+
// HelmRepositoryReadyMessage returns the message of the SourceCondition
112+
// of type Ready with status true if present, or an empty string.
104113
func HelmRepositoryReadyMessage(repository HelmRepository) string {
105114
for _, condition := range repository.Status.Conditions {
106115
if condition.Type == ReadyCondition && condition.Status == corev1.ConditionTrue {

‎api/v1alpha1/zz_generated.deepcopy.go‎

Lines changed: 6 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,9 @@ spec:
8282
TODO: Add other useful fields. apiVersion, kind, uid?'
8383
type: string
8484
type: object
85+
timeout:
86+
description: The timeout for remote git operations like cloning.
87+
type: string
8588
url:
8689
description: The repository URL, can be a HTTP or SSH address.
8790
pattern: ^(http|https|ssh)://

‎controllers/gitrepository_controller.go‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@ import (
2121
"fmt"
2222
"io/ioutil"
2323
"os"
24-
"time"
2524

2625
"github.com/blang/semver"
2726
"github.com/go-git/go-git/v5"
@@ -51,8 +50,7 @@ type GitRepositoryReconciler struct {
5150
// +kubebuilder:rbac:groups=source.fluxcd.io,resources=gitrepositories/status,verbs=get;update;patch
5251

5352
func (r *GitRepositoryReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {
54-
ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second)
55-
defer cancel()
53+
ctx := context.Background()
5654

5755
var repo sourcev1.GitRepository
5856
if err := r.Get(ctx, req.NamespacedName, &repo); err != nil {
@@ -164,7 +162,8 @@ func (r *GitRepositoryReconciler) sync(ctx context.Context, repository sourcev1.
164162
defer os.RemoveAll(tmpGit)
165163

166164
// clone to tmp
167-
repo, err := git.PlainClone(tmpGit, false, &git.CloneOptions{
165+
gitCtx, cancel := context.WithTimeout(ctx, repository.GetTimeout())
166+
repo, err := git.PlainCloneContext(gitCtx, tmpGit, false, &git.CloneOptions{
168167
URL: repository.Spec.URL,
169168
Auth: auth,
170169
RemoteName: "origin",
@@ -176,6 +175,7 @@ func (r *GitRepositoryReconciler) sync(ctx context.Context, repository sourcev1.
176175
Progress: nil,
177176
Tags: tagMode,
178177
})
178+
cancel()
179179
if err != nil {
180180
err = fmt.Errorf("git clone error: %w", err)
181181
return sourcev1.GitRepositoryNotReady(repository, sourcev1.GitOperationFailedReason, err.Error()), err
@@ -340,6 +340,8 @@ func (r *GitRepositoryReconciler) sync(ctx context.Context, repository sourcev1.
340340
return sourcev1.GitRepositoryReady(repository, artifact, url, sourcev1.GitOperationSucceedReason, message), nil
341341
}
342342

343+
// shouldResetStatus returns a boolean indicating if the status of the
344+
// given repository should be reset and a reset HelmChartStatus.
343345
func (r *GitRepositoryReconciler) shouldResetStatus(repository sourcev1.GitRepository) (bool, sourcev1.GitRepositoryStatus) {
344346
resetStatus := false
345347
if repository.Status.Artifact != nil {
@@ -364,6 +366,8 @@ func (r *GitRepositoryReconciler) shouldResetStatus(repository sourcev1.GitRepos
364366
}
365367
}
366368

369+
// gc performs a garbage collection on all but current artifacts of
370+
// the given repository.
367371
func (r *GitRepositoryReconciler) gc(repository sourcev1.GitRepository) error {
368372
if repository.Status.Artifact != nil {
369373
return r.Storage.RemoveAllButCurrent(*repository.Status.Artifact)

‎controllers/helmchart_controller.go‎

Lines changed: 23 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@ import (
2121
"fmt"
2222
"io/ioutil"
2323
"net/url"
24-
"time"
2524

2625
"github.com/go-logr/logr"
2726
"helm.sh/helm/v3/pkg/getter"
@@ -51,8 +50,7 @@ type HelmChartReconciler struct {
5150
// +kubebuilder:rbac:groups=source.fluxcd.io,resources=helmcharts/status,verbs=get;update;patch
5251

5352
func (r *HelmChartReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {
54-
ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second)
55-
defer cancel()
53+
ctx := context.Background()
5654

5755
var chart sourcev1.HelmChart
5856
if err := r.Get(ctx, req.NamespacedName, &chart); err != nil {
@@ -77,7 +75,7 @@ func (r *HelmChartReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {
7775
}
7876

7977
// get referenced chart repository
80-
repository, err := r.chartRepository(ctx, chart)
78+
repository, err := r.getChartRepositoryWithArtifact(ctx, chart)
8179
if err != nil {
8280
chart = sourcev1.HelmChartNotReady(*chart.DeepCopy(), sourcev1.ChartPullFailedReason, err.Error())
8381
if err := r.Status().Update(ctx, &chart); err != nil {
@@ -93,7 +91,7 @@ func (r *HelmChartReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {
9391
}
9492

9593
// try to pull chart
96-
pulledChart, err := r.sync(repository, *chart.DeepCopy())
94+
pulledChart, err := r.sync(ctx, repository, *chart.DeepCopy())
9795
if err != nil {
9896
log.Error(err, "Helm chart sync failed")
9997
if err := r.Status().Update(ctx, &pulledChart); err != nil {
@@ -122,7 +120,7 @@ func (r *HelmChartReconciler) SetupWithManager(mgr ctrl.Manager) error {
122120
Complete(r)
123121
}
124122

125-
func (r *HelmChartReconciler) sync(repository sourcev1.HelmRepository, chart sourcev1.HelmChart) (sourcev1.HelmChart, error) {
123+
func (r *HelmChartReconciler) sync(ctx context.Context, repository sourcev1.HelmRepository, chart sourcev1.HelmChart) (sourcev1.HelmChart, error) {
126124
indexBytes, err := ioutil.ReadFile(repository.Status.Artifact.Path)
127125
if err != nil {
128126
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
172170
}
173171

174172
var secret corev1.Secret
175-
err := r.Client.Get(context.TODO(), name, &secret)
173+
err := r.Client.Get(ctx, name, &secret)
176174
if err != nil {
177175
err = fmt.Errorf("auth secret error: %w", err)
178176
return sourcev1.HelmChartNotReady(chart, sourcev1.AuthenticationFailedReason, err.Error()), err
@@ -189,6 +187,8 @@ func (r *HelmChartReconciler) sync(repository sourcev1.HelmRepository, chart sou
189187
clientOpts = opts
190188
}
191189

190+
// TODO(hidde): implement timeout from the HelmRepository
191+
// https://github.com/helm/helm/pull/7950
192192
res, err := c.Get(u.String(), clientOpts...)
193193
if err != nil {
194194
return sourcev1.HelmChartNotReady(chart, sourcev1.ChartPullFailedReason, err.Error()), err
@@ -236,7 +236,10 @@ func (r *HelmChartReconciler) sync(repository sourcev1.HelmRepository, chart sou
236236
return sourcev1.HelmChartReady(chart, artifact, chartUrl, sourcev1.ChartPullSucceededReason, message), nil
237237
}
238238

239-
func (r *HelmChartReconciler) chartRepository(ctx context.Context, chart sourcev1.HelmChart) (sourcev1.HelmRepository, error) {
239+
// getChartRepositoryWithArtifact attempts to get the ChartRepository
240+
// for the given chart. It returns an error if the HelmRepository could
241+
// not be retrieved or if does not have an artifact.
242+
func (r *HelmChartReconciler) getChartRepositoryWithArtifact(ctx context.Context, chart sourcev1.HelmChart) (sourcev1.HelmRepository, error) {
240243
if chart.Spec.HelmRepositoryRef.Name == "" {
241244
return sourcev1.HelmRepository{}, fmt.Errorf("no HelmRepository reference given")
242245
}
@@ -260,6 +263,8 @@ func (r *HelmChartReconciler) chartRepository(ctx context.Context, chart sourcev
260263
return repository, err
261264
}
262265

266+
// shouldResetStatus returns a boolean indicating if the status of the
267+
// given chart should be reset and a reset HelmChartStatus.
263268
func (r *HelmChartReconciler) shouldResetStatus(chart sourcev1.HelmChart) (bool, sourcev1.HelmChartStatus) {
264269
resetStatus := false
265270
if chart.Status.Artifact != nil {
@@ -285,18 +290,23 @@ func (r *HelmChartReconciler) shouldResetStatus(chart sourcev1.HelmChart) (bool,
285290
}
286291
}
287292

293+
// gc performs a garbage collection on all but current artifacts of
294+
// the given chart.
288295
func (r *HelmChartReconciler) gc(chart sourcev1.HelmChart) error {
289296
if chart.Status.Artifact != nil {
290297
return r.Storage.RemoveAllButCurrent(*chart.Status.Artifact)
291298
}
292299
return nil
293300
}
294301

302+
// setOwnerRef appends the owner reference of the given chart to the
303+
// repository if it is not present.
295304
func (r *HelmChartReconciler) setOwnerRef(ctx context.Context, chart *sourcev1.HelmChart, repository sourcev1.HelmRepository) error {
296-
if !metav1.IsControlledBy(chart.GetObjectMeta(), repository.GetObjectMeta()) {
297-
chart.SetOwnerReferences(append(chart.GetOwnerReferences(),
298-
*metav1.NewControllerRef(repository.GetObjectMeta(), repository.GroupVersionKind())))
299-
return r.Update(ctx, chart)
305+
if metav1.IsControlledBy(chart.GetObjectMeta(), repository.GetObjectMeta()) {
306+
return nil
300307
}
301-
return nil
308+
chart.SetOwnerReferences(append(chart.GetOwnerReferences(), *metav1.NewControllerRef(
309+
repository.GetObjectMeta(), repository.GroupVersionKind(),
310+
)))
311+
return r.Update(ctx, chart)
302312
}

‎controllers/helmrepository_controller.go‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@ import (
2222
"io/ioutil"
2323
"net/url"
2424
"path"
25-
"time"
2625

2726
"github.com/go-logr/logr"
2827
"helm.sh/helm/v3/pkg/getter"
@@ -53,8 +52,7 @@ type HelmRepositoryReconciler struct {
5352
// +kubebuilder:rbac:groups=source.fluxcd.io,resources=helmcharts/finalizers,verbs=get;update;patch
5453

5554
func (r *HelmRepositoryReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {
56-
ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second)
57-
defer cancel()
55+
ctx := context.Background()
5856

5957
var repository sourcev1.HelmRepository
6058
if err := r.Get(ctx, req.NamespacedName, &repository); err != nil {
@@ -79,7 +77,7 @@ func (r *HelmRepositoryReconciler) Reconcile(req ctrl.Request) (ctrl.Result, err
7977
}
8078

8179
// try to download index
82-
syncedRepo, err := r.sync(*repository.DeepCopy())
80+
syncedRepo, err := r.sync(ctx, *repository.DeepCopy())
8381
if err != nil {
8482
log.Error(err, "Helm repository sync failed")
8583
if err := r.Status().Update(ctx, &syncedRepo); err != nil {
@@ -107,7 +105,7 @@ func (r *HelmRepositoryReconciler) SetupWithManager(mgr ctrl.Manager) error {
107105
Complete(r)
108106
}
109107

110-
func (r *HelmRepositoryReconciler) sync(repository sourcev1.HelmRepository) (sourcev1.HelmRepository, error) {
108+
func (r *HelmRepositoryReconciler) sync(ctx context.Context, repository sourcev1.HelmRepository) (sourcev1.HelmRepository, error) {
111109
u, err := url.Parse(repository.Spec.URL)
112110
if err != nil {
113111
return sourcev1.HelmRepositoryNotReady(repository, sourcev1.URLInvalidReason, err.Error()), err
@@ -129,7 +127,7 @@ func (r *HelmRepositoryReconciler) sync(repository sourcev1.HelmRepository) (sou
129127
}
130128

131129
var secret corev1.Secret
132-
err := r.Client.Get(context.TODO(), name, &secret)
130+
err := r.Client.Get(ctx, name, &secret)
133131
if err != nil {
134132
err = fmt.Errorf("auth secret error: %w", err)
135133
return sourcev1.HelmRepositoryNotReady(repository, sourcev1.AuthenticationFailedReason, err.Error()), err
@@ -146,6 +144,8 @@ func (r *HelmRepositoryReconciler) sync(repository sourcev1.HelmRepository) (sou
146144
clientOpts = opts
147145
}
148146

147+
// TODO(hidde): implement timeout from the HelmRepository
148+
// https://github.com/helm/helm/pull/7950
149149
res, err := c.Get(u.String(), clientOpts...)
150150
if err != nil {
151151
return sourcev1.HelmRepositoryNotReady(repository, sourcev1.IndexationFailedReason, err.Error()), err
@@ -204,6 +204,8 @@ func (r *HelmRepositoryReconciler) sync(repository sourcev1.HelmRepository) (sou
204204
return sourcev1.HelmRepositoryReady(repository, artifact, indexURL, sourcev1.IndexationSucceededReason, message), nil
205205
}
206206

207+
// shouldResetStatus returns a boolean indicating if the status of the
208+
// given repository should be reset and a reset HelmChartStatus.
207209
func (r *HelmRepositoryReconciler) shouldResetStatus(repository sourcev1.HelmRepository) (bool, sourcev1.HelmRepositoryStatus) {
208210
resetStatus := false
209211
if repository.Status.Artifact != nil {
@@ -229,6 +231,8 @@ func (r *HelmRepositoryReconciler) shouldResetStatus(repository sourcev1.HelmRep
229231
}
230232
}
231233

234+
// gc performs a garbage collection on all but current artifacts of
235+
// the given repository.
232236
func (r *HelmRepositoryReconciler) gc(repository sourcev1.HelmRepository) error {
233237
if repository.Status.Artifact != nil {
234238
return r.Storage.RemoveAllButCurrent(*repository.Status.Artifact)

0 commit comments

Comments
 (0)