Skip to content

Commit

Permalink
Replace stalling events in HelmChart and HelmRepository_OCI
Browse files Browse the repository at this point in the history 
The setupRegistryServer has been refactored to take into account fluxcd#690
reviews.

Signed-off-by: Soule BA <soule@weave.works>
  • Loading branch information
@souleb
souleb committed May 25, 2022
1 parent 7aa0814 commit c726906
Show file tree
Hide file tree
Showing 6 changed files with 92 additions and 89 deletions.
22 changes: 16 additions & 6 deletions controllers/helmchart_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -513,7 +513,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
case sourcev1.HelmRepositoryTypeOCI:
if !helmreg.IsOCI(repo.Spec.URL) {
err := fmt.Errorf("invalid OCI registry URL: %s", repo.Spec.URL)
return chartRepoErrorReturn(err, obj)
return chartRepoConfigErrorReturn(err, obj)
}

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

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

Expand All @@ -547,7 +552,12 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
if loginOpts != nil {
err = ociChartRepo.Login(loginOpts...)
if err != nil {
return chartRepoErrorReturn(err, obj)
e := &serror.Event{
Err: fmt.Errorf("failed to login to OCI registry: %w", err),
Reason: sourcev1.AuthenticationFailedReason,
}
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
return sreconcile.ResultEmpty, e
}
}
default:
Expand All @@ -556,7 +566,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
r.IncCacheEvents(event, obj.Name, obj.Namespace)
}))
if err != nil {
return chartRepoErrorReturn(err, obj)
return chartRepoConfigErrorReturn(err, obj)
}
chartRepo = httpChartRepo
defer func() {
Expand Down Expand Up @@ -1145,7 +1155,7 @@ func reasonForBuild(build *chart.Build) string {
return sourcev1.ChartPullSucceededReason
}

func chartRepoErrorReturn(err error, obj *sourcev1.HelmChart) (sreconcile.Result, error) {
func chartRepoConfigErrorReturn(err error, obj *sourcev1.HelmChart) (sreconcile.Result, error) {
switch err.(type) {
case *url.Error:
e := &serror.Stalling{
Expand Down
18 changes: 9 additions & 9 deletions controllers/helmchart_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -792,8 +792,8 @@ func TestHelmChartReconciler_buildFromOCIHelmRepository(t *testing.T) {
)

// Login to the registry
err := testRegistryserver.RegistryClient.Login(testRegistryserver.DockerRegistryHost,
helmreg.LoginOptBasicAuth(testUsername, testPassword),
err := testRegistryServer.registryClient.Login(testRegistryServer.registryHost,
helmreg.LoginOptBasicAuth(testRegistryUsername, testRegistryPassword),
helmreg.LoginOptInsecure(true))
g.Expect(err).NotTo(HaveOccurred())

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

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

storage, err := NewStorage(tmpDir, "example.com", retentionTTL, retentionRecords)
Expand Down Expand Up @@ -835,8 +835,8 @@ func TestHelmChartReconciler_buildFromOCIHelmRepository(t *testing.T) {
Type: corev1.SecretTypeDockerConfigJson,
Data: map[string][]byte{
".dockerconfigjson": []byte(`{"auths":{"` +
testRegistryserver.DockerRegistryHost + `":{"` +
`auth":"` + base64.StdEncoding.EncodeToString([]byte(testUsername+":"+testPassword)) + `"}}}`),
testRegistryServer.registryHost + `":{"` +
`auth":"` + base64.StdEncoding.EncodeToString([]byte(testRegistryUsername+":"+testRegistryPassword)) + `"}}}`),
},
},
beforeFunc: func(obj *sourcev1.HelmChart, repository *sourcev1.HelmRepository) {
Expand All @@ -862,8 +862,8 @@ func TestHelmChartReconciler_buildFromOCIHelmRepository(t *testing.T) {
Name: "auth",
},
Data: map[string][]byte{
"username": []byte(testUsername),
"password": []byte(testPassword),
"username": []byte(testRegistryUsername),
"password": []byte(testRegistryPassword),
},
},
beforeFunc: func(obj *sourcev1.HelmChart, repository *sourcev1.HelmRepository) {
Expand Down Expand Up @@ -983,7 +983,7 @@ func TestHelmChartReconciler_buildFromOCIHelmRepository(t *testing.T) {
GenerateName: "helmrepository-",
},
Spec: sourcev1.HelmRepositorySpec{
URL: fmt.Sprintf("oci://%s/testrepo", testRegistryserver.DockerRegistryHost),
URL: fmt.Sprintf("oci://%s/testrepo", testRegistryServer.registryHost),
Timeout: &metav1.Duration{Duration: timeout},
Type: sourcev1.HelmRepositoryTypeOCI,
},
Expand Down
39 changes: 17 additions & 22 deletions controllers/helmrepository_controller_oci.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import (
"errors"
"fmt"
"os"
"strings"
"time"

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

func (r *HelmRepositoryOCIReconciler) reconcileSource(ctx context.Context, obj *sourcev1.HelmRepository) (sreconcile.Result, error) {
if !helmreg.IsOCI(obj.Spec.URL) {
e := &serror.Stalling{
Err: fmt.Errorf("the url scheme is not supported: %s", obj.Spec.URL),
Reason: sourcev1.URLInvalidReason,
}
conditions.MarkFalse(obj, meta.ReadyCondition, e.Reason, e.Err.Error())
return sreconcile.ResultEmpty, e
}

var loginOpts []helmreg.LoginOption
// Configure any authentication related options
if obj.Spec.SecretRef != nil {
Expand Down Expand Up @@ -292,20 +300,16 @@ func (r *HelmRepositoryOCIReconciler) reconcileSource(ctx context.Context, obj *
}
}

if result, err := r.validateSource(ctx, obj, loginOpts...); err != nil || result == sreconcile.ResultEmpty {
return result, err
}

return sreconcile.ResultSuccess, nil
return r.validateSource(ctx, obj, loginOpts...)
}

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

chartRepo, err := repository.NewOCIChartRepository(obj.Spec.URL, repository.WithOCIRegistryClient(registryClient))
if err != nil {
if strings.Contains(err.Error(), "parse") {
e := &serror.Stalling{
Err: fmt.Errorf("failed to parse URL '%s': %w", obj.Spec.URL, err),
Reason: sourcev1.URLInvalidReason,
}
conditions.MarkFalse(obj, meta.ReadyCondition, e.Reason, e.Err.Error())
return sreconcile.ResultEmpty, e
} else if strings.Contains(err.Error(), "the url scheme is not supported") {
e := &serror.Event{
Err: err,
Reason: sourcev1.URLInvalidReason,
}
conditions.MarkFalse(obj, meta.ReadyCondition, e.Reason, e.Err.Error())
return sreconcile.ResultEmpty, e
e := &serror.Stalling{
Err: fmt.Errorf("failed to parse URL '%s': %w", obj.Spec.URL, err),
Reason: sourcev1.URLInvalidReason,
}
conditions.MarkFalse(obj, meta.ReadyCondition, e.Reason, e.Err.Error())
return sreconcile.ResultEmpty, e
}

// Attempt to login to the registry if credentials are provided.
Expand Down
10 changes: 5 additions & 5 deletions controllers/helmrepository_controller_oci_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,8 @@ func TestHelmRepositoryOCIReconciler_Reconcile(t *testing.T) {
{
name: "valid auth data",
secretData: map[string][]byte{
"username": []byte(testUsername),
"password": []byte(testPassword),
"username": []byte(testRegistryUsername),
"password": []byte(testRegistryPassword),
},
},
{
Expand All @@ -56,8 +56,8 @@ func TestHelmRepositoryOCIReconciler_Reconcile(t *testing.T) {
secretType: corev1.SecretTypeDockerConfigJson,
secretData: map[string][]byte{
".dockerconfigjson": []byte(`{"auths":{"` +
testRegistryserver.DockerRegistryHost + `":{"` +
`auth":"` + base64.StdEncoding.EncodeToString([]byte(testUsername+":"+testPassword)) + `"}}}`),
testRegistryServer.registryHost + `":{"` +
`auth":"` + base64.StdEncoding.EncodeToString([]byte(testRegistryUsername+":"+testRegistryPassword)) + `"}}}`),
},
},
}
Expand Down Expand Up @@ -90,7 +90,7 @@ func TestHelmRepositoryOCIReconciler_Reconcile(t *testing.T) {
},
Spec: sourcev1.HelmRepositorySpec{
Interval: metav1.Duration{Duration: interval},
URL: fmt.Sprintf("oci://%s", testRegistryserver.DockerRegistryHost),
URL: fmt.Sprintf("oci://%s", testRegistryServer.registryHost),
SecretRef: &meta.LocalObjectReference{
Name: secret.Name,
},
Expand Down
18 changes: 10 additions & 8 deletions controllers/helmrepository_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1109,7 +1109,7 @@ func TestHelmRepositoryReconciler_ReconcileTypeUpdatePredicateFilter(t *testing.
URL: testServer.URL(),
},
}
g.Expect(testEnv.Create(ctx, obj)).To(Succeed())
g.Expect(testEnv.CreateAndWait(ctx, obj)).To(Succeed())

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

Expand Down Expand Up @@ -1154,14 +1154,14 @@ func TestHelmRepositoryReconciler_ReconcileTypeUpdatePredicateFilter(t *testing.
Namespace: "default",
},
Data: map[string][]byte{
"username": []byte(testUsername),
"password": []byte(testPassword),
"username": []byte(testRegistryUsername),
"password": []byte(testRegistryPassword),
},
}
g.Expect(testEnv.CreateAndWait(ctx, secret)).To(Succeed())

obj.Spec.Type = sourcev1.HelmRepositoryTypeOCI
obj.Spec.URL = fmt.Sprintf("oci://%s", testRegistryserver.DockerRegistryHost)
obj.Spec.URL = fmt.Sprintf("oci://%s", testRegistryServer.registryHost)
obj.Spec.SecretRef = &meta.LocalObjectReference{
Name: secret.Name,
}
Expand Down Expand Up @@ -1223,7 +1223,7 @@ func TestHelmRepositoryReconciler_ReconcileSpecUpdatePredicateFilter(t *testing.
URL: testServer.URL(),
},
}
g.Expect(testEnv.Create(ctx, obj)).To(Succeed())
g.Expect(testEnv.CreateAndWait(ctx, obj)).To(Succeed())

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

Expand Down Expand Up @@ -1263,20 +1263,22 @@ func TestHelmRepositoryReconciler_ReconcileSpecUpdatePredicateFilter(t *testing.

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

// Wait for HelmRepository to be Ready
g.Eventually(func() bool {
if err := testEnv.Get(ctx, key, obj); err != nil {
return false
}
if !conditions.IsReady(obj) {
if !conditions.IsReady(obj) && obj.Status.Artifact == nil {
return false
}
readyCondition := conditions.Get(obj, meta.ReadyCondition)
return readyCondition.Status == metav1.ConditionTrue &&
obj.Generation == readyCondition.ObservedGeneration &&
obj.Generation == obj.Status.ObservedGeneration
newGen == readyCondition.ObservedGeneration &&
newGen == obj.Status.ObservedGeneration
}, timeout).Should(BeTrue())

// Check if the object status is valid.
Expand Down
Loading

0 comments on commit c726906

Please sign in to comment.