Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Helm reconcilers conditions and test improvements #728

Merged
merged 1 commit into from
May 27, 2022
Merged

Helm reconcilers conditions and test improvements #728

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 16 additions & 6 deletions controllers/helmchart_controller.go
View file
Open in desktop
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,
darkowlzz marked this conversation as resolved.
Show resolved Hide resolved
}
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
Open in desktop
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
Open in desktop
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),
souleb marked this conversation as resolved.
Show resolved Hide resolved
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),
souleb marked this conversation as resolved.
Show resolved Hide resolved
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
Open in desktop
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
Open in desktop
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 {
souleb marked this conversation as resolved.
Show resolved Hide resolved
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