Skip to content

Commit a6f19b3

Browse files
makkesmakkes
authored
Merge pull request #728 from souleb/revisit-oci-events
Helm reconcilers conditions and test improvements
2 parents 4b3e0f9 + b98093d commit a6f19b3

File tree

6 files changed

+92
-89
lines changed

6 files changed

+92
-89
lines changed

‎controllers/helmchart_controller.go‎

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -513,7 +513,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
513513
case sourcev1.HelmRepositoryTypeOCI:
514514
if !helmreg.IsOCI(repo.Spec.URL) {
515515
err := fmt.Errorf("invalid OCI registry URL: %s", repo.Spec.URL)
516-
return chartRepoErrorReturn(err, obj)
516+
return chartRepoConfigErrorReturn(err, obj)
517517
}
518518

519519
// with this function call, we create a temporary file to store the credentials if needed.
@@ -522,7 +522,12 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
522522
// or rework to enable reusing credentials to avoid the unneccessary handshake operations
523523
registryClient, file, err := r.RegistryClientGenerator(loginOpts != nil)
524524
if err != nil {
525-
return chartRepoErrorReturn(err, obj)
525+
e := &serror.Event{
526+
Err: fmt.Errorf("failed to construct Helm client: %w", err),
527+
Reason: meta.FailedReason,
528+
}
529+
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
530+
return sreconcile.ResultEmpty, e
526531
}
527532

528533
if file != "" {
@@ -538,7 +543,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
538543
clientOpts = append(clientOpts, helmgetter.WithRegistryClient(registryClient))
539544
ociChartRepo, err := repository.NewOCIChartRepository(repo.Spec.URL, repository.WithOCIGetter(r.Getters), repository.WithOCIGetterOptions(clientOpts), repository.WithOCIRegistryClient(registryClient))
540545
if err != nil {
541-
return chartRepoErrorReturn(err, obj)
546+
return chartRepoConfigErrorReturn(err, obj)
542547
}
543548
chartRepo = ociChartRepo
544549

@@ -547,7 +552,12 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
547552
if loginOpts != nil {
548553
err = ociChartRepo.Login(loginOpts...)
549554
if err != nil {
550-
return chartRepoErrorReturn(err, obj)
555+
e := &serror.Event{
556+
Err: fmt.Errorf("failed to login to OCI registry: %w", err),
557+
Reason: sourcev1.AuthenticationFailedReason,
558+
}
559+
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
560+
return sreconcile.ResultEmpty, e
551561
}
552562
}
553563
default:
@@ -556,7 +566,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
556566
r.IncCacheEvents(event, obj.Name, obj.Namespace)
557567
}))
558568
if err != nil {
559-
return chartRepoErrorReturn(err, obj)
569+
return chartRepoConfigErrorReturn(err, obj)
560570
}
561571
chartRepo = httpChartRepo
562572
defer func() {
@@ -1145,7 +1155,7 @@ func reasonForBuild(build *chart.Build) string {
11451155
return sourcev1.ChartPullSucceededReason
11461156
}
11471157

1148-
func chartRepoErrorReturn(err error, obj *sourcev1.HelmChart) (sreconcile.Result, error) {
1158+
func chartRepoConfigErrorReturn(err error, obj *sourcev1.HelmChart) (sreconcile.Result, error) {
11491159
switch err.(type) {
11501160
case *url.Error:
11511161
e := &serror.Stalling{

‎controllers/helmchart_controller_test.go‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -792,8 +792,8 @@ func TestHelmChartReconciler_buildFromOCIHelmRepository(t *testing.T) {
792792
)
793793

794794
// Login to the registry
795-
err := testRegistryserver.RegistryClient.Login(testRegistryserver.DockerRegistryHost,
796-
helmreg.LoginOptBasicAuth(testUsername, testPassword),
795+
err := testRegistryServer.registryClient.Login(testRegistryServer.registryHost,
796+
helmreg.LoginOptBasicAuth(testRegistryUsername, testRegistryPassword),
797797
helmreg.LoginOptInsecure(true))
798798
g.Expect(err).NotTo(HaveOccurred())
799799

@@ -804,8 +804,8 @@ func TestHelmChartReconciler_buildFromOCIHelmRepository(t *testing.T) {
804804
g.Expect(err).NotTo(HaveOccurred())
805805

806806
// Upload the test chart
807-
ref := fmt.Sprintf("%s/testrepo/%s:%s", testRegistryserver.DockerRegistryHost, metadata.Name, metadata.Version)
808-
_, err = testRegistryserver.RegistryClient.Push(chartData, ref)
807+
ref := fmt.Sprintf("%s/testrepo/%s:%s", testRegistryServer.registryHost, metadata.Name, metadata.Version)
808+
_, err = testRegistryServer.registryClient.Push(chartData, ref)
809809
g.Expect(err).NotTo(HaveOccurred())
810810

811811
storage, err := NewStorage(tmpDir, "example.com", retentionTTL, retentionRecords)
@@ -835,8 +835,8 @@ func TestHelmChartReconciler_buildFromOCIHelmRepository(t *testing.T) {
835835
Type: corev1.SecretTypeDockerConfigJson,
836836
Data: map[string][]byte{
837837
".dockerconfigjson": []byte(`{"auths":{"` +
838-
testRegistryserver.DockerRegistryHost + `":{"` +
839-
`auth":"` + base64.StdEncoding.EncodeToString([]byte(testUsername+":"+testPassword)) + `"}}}`),
838+
testRegistryServer.registryHost + `":{"` +
839+
`auth":"` + base64.StdEncoding.EncodeToString([]byte(testRegistryUsername+":"+testRegistryPassword)) + `"}}}`),
840840
},
841841
},
842842
beforeFunc: func(obj *sourcev1.HelmChart, repository *sourcev1.HelmRepository) {
@@ -862,8 +862,8 @@ func TestHelmChartReconciler_buildFromOCIHelmRepository(t *testing.T) {
862862
Name: "auth",
863863
},
864864
Data: map[string][]byte{
865-
"username": []byte(testUsername),
866-
"password": []byte(testPassword),
865+
"username": []byte(testRegistryUsername),
866+
"password": []byte(testRegistryPassword),
867867
},
868868
},
869869
beforeFunc: func(obj *sourcev1.HelmChart, repository *sourcev1.HelmRepository) {
@@ -983,7 +983,7 @@ func TestHelmChartReconciler_buildFromOCIHelmRepository(t *testing.T) {
983983
GenerateName: "helmrepository-",
984984
},
985985
Spec: sourcev1.HelmRepositorySpec{
986-
URL: fmt.Sprintf("oci://%s/testrepo", testRegistryserver.DockerRegistryHost),
986+
URL: fmt.Sprintf("oci://%s/testrepo", testRegistryServer.registryHost),
987987
Timeout: &metav1.Duration{Duration: timeout},
988988
Type: sourcev1.HelmRepositoryTypeOCI,
989989
},

‎controllers/helmrepository_controller_oci.go‎

Lines changed: 17 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@ import (
2121
"errors"
2222
"fmt"
2323
"os"
24-
"strings"
2524
"time"
2625

2726
"github.com/fluxcd/pkg/apis/meta"
@@ -257,6 +256,15 @@ func (r *HelmRepositoryOCIReconciler) reconcile(ctx context.Context, obj *source
257256
}
258257

259258
func (r *HelmRepositoryOCIReconciler) reconcileSource(ctx context.Context, obj *sourcev1.HelmRepository) (sreconcile.Result, error) {
259+
if !helmreg.IsOCI(obj.Spec.URL) {
260+
e := &serror.Stalling{
261+
Err: fmt.Errorf("the url scheme is not supported: %s", obj.Spec.URL),
262+
Reason: sourcev1.URLInvalidReason,
263+
}
264+
conditions.MarkFalse(obj, meta.ReadyCondition, e.Reason, e.Err.Error())
265+
return sreconcile.ResultEmpty, e
266+
}
267+
260268
var loginOpts []helmreg.LoginOption
261269
// Configure any authentication related options
262270
if obj.Spec.SecretRef != nil {
@@ -292,20 +300,16 @@ func (r *HelmRepositoryOCIReconciler) reconcileSource(ctx context.Context, obj *
292300
}
293301
}
294302

295-
if result, err := r.validateSource(ctx, obj, loginOpts...); err != nil || result == sreconcile.ResultEmpty {
296-
return result, err
297-
}
298-
299-
return sreconcile.ResultSuccess, nil
303+
return r.validateSource(ctx, obj, loginOpts...)
300304
}
301305

302306
// validateSource the HelmRepository object by checking the url and connecting to the underlying registry
303307
// with he provided credentials.
304308
func (r *HelmRepositoryOCIReconciler) validateSource(ctx context.Context, obj *sourcev1.HelmRepository, logOpts ...helmreg.LoginOption) (sreconcile.Result, error) {
305309
registryClient, file, err := r.RegistryClientGenerator(logOpts != nil)
306310
if err != nil {
307-
e := &serror.Stalling{
308-
Err: fmt.Errorf("failed to create registry client: %w", err),
311+
e := &serror.Event{
312+
Err: fmt.Errorf("failed to create registry client:: %w", err),
309313
Reason: meta.FailedReason,
310314
}
311315
conditions.MarkFalse(obj, meta.ReadyCondition, e.Reason, e.Err.Error())
@@ -323,21 +327,12 @@ func (r *HelmRepositoryOCIReconciler) validateSource(ctx context.Context, obj *s
323327

324328
chartRepo, err := repository.NewOCIChartRepository(obj.Spec.URL, repository.WithOCIRegistryClient(registryClient))
325329
if err != nil {
326-
if strings.Contains(err.Error(), "parse") {
327-
e := &serror.Stalling{
328-
Err: fmt.Errorf("failed to parse URL '%s': %w", obj.Spec.URL, err),
329-
Reason: sourcev1.URLInvalidReason,
330-
}
331-
conditions.MarkFalse(obj, meta.ReadyCondition, e.Reason, e.Err.Error())
332-
return sreconcile.ResultEmpty, e
333-
} else if strings.Contains(err.Error(), "the url scheme is not supported") {
334-
e := &serror.Event{
335-
Err: err,
336-
Reason: sourcev1.URLInvalidReason,
337-
}
338-
conditions.MarkFalse(obj, meta.ReadyCondition, e.Reason, e.Err.Error())
339-
return sreconcile.ResultEmpty, e
330+
e := &serror.Stalling{
331+
Err: fmt.Errorf("failed to parse URL '%s': %w", obj.Spec.URL, err),
332+
Reason: sourcev1.URLInvalidReason,
340333
}
334+
conditions.MarkFalse(obj, meta.ReadyCondition, e.Reason, e.Err.Error())
335+
return sreconcile.ResultEmpty, e
341336
}
342337

343338
// Attempt to login to the registry if credentials are provided.

‎controllers/helmrepository_controller_oci_test.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -43,8 +43,8 @@ func TestHelmRepositoryOCIReconciler_Reconcile(t *testing.T) {
4343
{
4444
name: "valid auth data",
4545
secretData: map[string][]byte{
46-
"username": []byte(testUsername),
47-
"password": []byte(testPassword),
46+
"username": []byte(testRegistryUsername),
47+
"password": []byte(testRegistryPassword),
4848
},
4949
},
5050
{
@@ -56,8 +56,8 @@ func TestHelmRepositoryOCIReconciler_Reconcile(t *testing.T) {
5656
secretType: corev1.SecretTypeDockerConfigJson,
5757
secretData: map[string][]byte{
5858
".dockerconfigjson": []byte(`{"auths":{"` +
59-
testRegistryserver.DockerRegistryHost + `":{"` +
60-
`auth":"` + base64.StdEncoding.EncodeToString([]byte(testUsername+":"+testPassword)) + `"}}}`),
59+
testRegistryServer.registryHost + `":{"` +
60+
`auth":"` + base64.StdEncoding.EncodeToString([]byte(testRegistryUsername+":"+testRegistryPassword)) + `"}}}`),
6161
},
6262
},
6363
}
@@ -90,7 +90,7 @@ func TestHelmRepositoryOCIReconciler_Reconcile(t *testing.T) {
9090
},
9191
Spec: sourcev1.HelmRepositorySpec{
9292
Interval: metav1.Duration{Duration: interval},
93-
URL: fmt.Sprintf("oci://%s", testRegistryserver.DockerRegistryHost),
93+
URL: fmt.Sprintf("oci://%s", testRegistryServer.registryHost),
9494
SecretRef: &meta.LocalObjectReference{
9595
Name: secret.Name,
9696
},

‎controllers/helmrepository_controller_test.go‎

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1109,7 +1109,7 @@ func TestHelmRepositoryReconciler_ReconcileTypeUpdatePredicateFilter(t *testing.
11091109
URL: testServer.URL(),
11101110
},
11111111
}
1112-
g.Expect(testEnv.Create(ctx, obj)).To(Succeed())
1112+
g.Expect(testEnv.CreateAndWait(ctx, obj)).To(Succeed())
11131113

11141114
key := client.ObjectKey{Name: obj.Name, Namespace: obj.Namespace}
11151115

@@ -1154,14 +1154,14 @@ func TestHelmRepositoryReconciler_ReconcileTypeUpdatePredicateFilter(t *testing.
11541154
Namespace: "default",
11551155
},
11561156
Data: map[string][]byte{
1157-
"username": []byte(testUsername),
1158-
"password": []byte(testPassword),
1157+
"username": []byte(testRegistryUsername),
1158+
"password": []byte(testRegistryPassword),
11591159
},
11601160
}
11611161
g.Expect(testEnv.CreateAndWait(ctx, secret)).To(Succeed())
11621162

11631163
obj.Spec.Type = sourcev1.HelmRepositoryTypeOCI
1164-
obj.Spec.URL = fmt.Sprintf("oci://%s", testRegistryserver.DockerRegistryHost)
1164+
obj.Spec.URL = fmt.Sprintf("oci://%s", testRegistryServer.registryHost)
11651165
obj.Spec.SecretRef = &meta.LocalObjectReference{
11661166
Name: secret.Name,
11671167
}
@@ -1223,7 +1223,7 @@ func TestHelmRepositoryReconciler_ReconcileSpecUpdatePredicateFilter(t *testing.
12231223
URL: testServer.URL(),
12241224
},
12251225
}
1226-
g.Expect(testEnv.Create(ctx, obj)).To(Succeed())
1226+
g.Expect(testEnv.CreateAndWait(ctx, obj)).To(Succeed())
12271227

12281228
key := client.ObjectKey{Name: obj.Name, Namespace: obj.Namespace}
12291229

@@ -1263,20 +1263,22 @@ func TestHelmRepositoryReconciler_ReconcileSpecUpdatePredicateFilter(t *testing.
12631263

12641264
// Change spec Interval to validate spec update
12651265
obj.Spec.Interval = metav1.Duration{Duration: interval + time.Second}
1266+
oldGen := obj.GetGeneration()
12661267
g.Expect(testEnv.Update(ctx, obj)).To(Succeed())
1268+
newGen := oldGen + 1
12671269

12681270
// Wait for HelmRepository to be Ready
12691271
g.Eventually(func() bool {
12701272
if err := testEnv.Get(ctx, key, obj); err != nil {
12711273
return false
12721274
}
1273-
if !conditions.IsReady(obj) {
1275+
if !conditions.IsReady(obj) && obj.Status.Artifact == nil {
12741276
return false
12751277
}
12761278
readyCondition := conditions.Get(obj, meta.ReadyCondition)
12771279
return readyCondition.Status == metav1.ConditionTrue &&
1278-
obj.Generation == readyCondition.ObservedGeneration &&
1279-
obj.Generation == obj.Status.ObservedGeneration
1280+
newGen == readyCondition.ObservedGeneration &&
1281+
newGen == obj.Status.ObservedGeneration
12801282
}, timeout).Should(BeTrue())
12811283

12821284
// Check if the object status is valid.

0 commit comments

Comments
 (0)