Skip to content

Commit 11f5c93

Browse files
soulebsouleb
committed
Replace some stalling event by normal event in HelmChart and
HelmRepository_OCI reconcilers to make to retry on failure The setupRegistryServer has been refactored to take into account #690 reviews. Signed-off-by: Soule BA <soule@weave.works>
1 parent 55a594a commit 11f5c93

6 files changed

+71
-71
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: meta.FailedReason,
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.dockerRegistryHost,
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.dockerRegistryHost, 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.dockerRegistryHost + `":{"` +
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.dockerRegistryHost),
987987
Timeout: &metav1.Duration{Duration: timeout},
988988
Type: sourcev1.HelmRepositoryTypeOCI,
989989
},

‎controllers/helmrepository_controller_oci.go

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -292,20 +292,16 @@ func (r *HelmRepositoryOCIReconciler) reconcileSource(ctx context.Context, obj *
292292
}
293293
}
294294

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
295+
return r.validateSource(ctx, obj, loginOpts...)
300296
}
301297

302298
// validateSource the HelmRepository object by checking the url and connecting to the underlying registry
303299
// with he provided credentials.
304300
func (r *HelmRepositoryOCIReconciler) validateSource(ctx context.Context, obj *sourcev1.HelmRepository, logOpts ...helmreg.LoginOption) (sreconcile.Result, error) {
305301
registryClient, file, err := r.RegistryClientGenerator(logOpts != nil)
306302
if err != nil {
307-
e := &serror.Stalling{
308-
Err: fmt.Errorf("failed to create registry client: %w", err),
303+
e := &serror.Event{
304+
Err: fmt.Errorf("failed to create registry client:: %w", err),
309305
Reason: meta.FailedReason,
310306
}
311307
conditions.MarkFalse(obj, meta.ReadyCondition, e.Reason, e.Err.Error())

‎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.dockerRegistryHost + `":{"` +
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.dockerRegistryHost),
9494
SecretRef: &meta.LocalObjectReference{
9595
Name: secret.Name,
9696
},

‎controllers/helmrepository_controller_test.go

Lines changed: 6 additions & 6 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.dockerRegistryHost)
11651165
obj.Spec.SecretRef = &meta.LocalObjectReference{
11661166
Name: secret.Name,
11671167
}
@@ -1221,7 +1221,7 @@ func TestHelmRepositoryReconciler_ReconcileSpecUpdatePredicateFilter(t *testing.
12211221
URL: testServer.URL(),
12221222
},
12231223
}
1224-
g.Expect(testEnv.Create(ctx, obj)).To(Succeed())
1224+
g.Expect(testEnv.CreateAndWait(ctx, obj)).To(Succeed())
12251225

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

@@ -1268,7 +1268,7 @@ func TestHelmRepositoryReconciler_ReconcileSpecUpdatePredicateFilter(t *testing.
12681268
if err := testEnv.Get(ctx, key, obj); err != nil {
12691269
return false
12701270
}
1271-
if !conditions.IsReady(obj) {
1271+
if !conditions.IsReady(obj) && obj.Status.Artifact == nil {
12721272
return false
12731273
}
12741274
readyCondition := conditions.Get(obj, meta.ReadyCondition)

‎controllers/suite_test.go

Lines changed: 32 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -94,69 +94,66 @@ var (
9494
)
9595

9696
var (
97-
testRegistryClient *helmreg.Client
98-
testRegistryserver *RegistryClientTestServer
97+
testRegistryserver *registryClientTestServer
9998
)
10099

101100
var (
102-
testWorkspaceDir = "registry-test"
103-
testHtpasswdFileBasename = "authtest.htpasswd"
104-
testUsername = "myuser"
105-
testPassword = "mypass"
101+
testRegistryWorkspaceDir = "/tmp/registry-test"
102+
testRegistryHtpasswdFileBasename = "authtest.htpasswd"
103+
testRegistryUsername = "myuser"
104+
testRegistryPassword = "mypass"
106105
)
107106

108107
func init() {
109108
rand.Seed(time.Now().UnixNano())
110109
}
111110

112-
type RegistryClientTestServer struct {
113-
Out io.Writer
114-
DockerRegistryHost string
115-
WorkspaceDir string
116-
RegistryClient *helmreg.Client
111+
type registryClientTestServer struct {
112+
out io.Writer
113+
dockerRegistryHost string
114+
workspaceDir string
115+
registryClient *helmreg.Client
117116
}
118117

119-
func SetupServer(server *RegistryClientTestServer) string {
118+
func setupRegistryServer(ctx context.Context) (*registryClientTestServer, error) {
119+
server := &registryClientTestServer{}
120120
// Create a temporary workspace directory for the registry
121-
server.WorkspaceDir = testWorkspaceDir
122-
os.RemoveAll(server.WorkspaceDir)
123-
err := os.Mkdir(server.WorkspaceDir, 0700)
121+
server.workspaceDir = testRegistryWorkspaceDir
122+
os.RemoveAll(server.workspaceDir)
123+
err := os.Mkdir(server.workspaceDir, 0o700)
124124
if err != nil {
125-
panic(fmt.Sprintf("failed to create workspace directory: %s", err))
125+
return nil, fmt.Errorf("failed to create workspace directory: %s", err)
126126
}
127127

128128
var out bytes.Buffer
129-
server.Out = &out
129+
server.out = &out
130130

131131
// init test client
132-
server.RegistryClient, err = helmreg.NewClient(
132+
server.registryClient, err = helmreg.NewClient(
133133
helmreg.ClientOptDebug(true),
134-
helmreg.ClientOptWriter(server.Out),
134+
helmreg.ClientOptWriter(server.out),
135135
)
136136
if err != nil {
137-
panic(fmt.Sprintf("failed to create registry client: %s", err))
137+
return nil, fmt.Errorf("failed to create registry client: %s", err)
138138
}
139139

140140
// create htpasswd file (w BCrypt, which is required)
141-
pwBytes, err := bcrypt.GenerateFromPassword([]byte(testPassword), bcrypt.DefaultCost)
141+
pwBytes, err := bcrypt.GenerateFromPassword([]byte(testRegistryPassword), bcrypt.DefaultCost)
142142
if err != nil {
143-
panic(fmt.Sprintf("failed to generate password: %s", err))
143+
return nil, fmt.Errorf("failed to generate password: %s", err)
144144
}
145145

146-
htpasswdPath := filepath.Join(testWorkspaceDir, testHtpasswdFileBasename)
147-
err = ioutil.WriteFile(htpasswdPath, []byte(fmt.Sprintf("%s:%s\n", testUsername, string(pwBytes))), 0644)
146+
htpasswdPath := filepath.Join(testRegistryWorkspaceDir, testRegistryHtpasswdFileBasename)
147+
err = ioutil.WriteFile(htpasswdPath, []byte(fmt.Sprintf("%s:%s\n", testRegistryUsername, string(pwBytes))), 0644)
148148
if err != nil {
149-
panic(fmt.Sprintf("failed to create htpasswd file: %s", err))
149+
return nil, fmt.Errorf("failed to create htpasswd file: %s", err)
150150
}
151151

152152
// Registry config
153153
config := &configuration.Configuration{}
154154
port, err := freeport.GetFreePort()
155-
if err != nil {
156-
panic(fmt.Sprintf("failed to get free port: %s", err))
157-
}
158155

159-
server.DockerRegistryHost = fmt.Sprintf("localhost:%d", port)
156+
server.dockerRegistryHost = fmt.Sprintf("localhost:%d", port)
160157
config.HTTP.Addr = fmt.Sprintf("127.0.0.1:%d", port)
161158
config.HTTP.DrainTimeout = time.Duration(10) * time.Second
162159
config.Storage = map[string]configuration.Parameters{"inmemory": map[string]interface{}{}}
@@ -166,15 +163,15 @@ func SetupServer(server *RegistryClientTestServer) string {
166163
"path": htpasswdPath,
167164
},
168165
}
169-
dockerRegistry, err := dockerRegistry.NewRegistry(context.Background(), config)
166+
dockerRegistry, err := dockerRegistry.NewRegistry(ctx, config)
170167
if err != nil {
171-
panic(fmt.Sprintf("failed to create docker registry: %s", err))
168+
return nil, fmt.Errorf("failed to create docker registry: %s", err)
172169
}
173170

174171
// Start Docker registry
175172
go dockerRegistry.ListenAndServe()
176173

177-
return server.WorkspaceDir
174+
return server, nil
178175
}
179176

180177
func TestMain(m *testing.M) {
@@ -199,12 +196,9 @@ func TestMain(m *testing.M) {
199196

200197
testMetricsH = controller.MustMakeMetrics(testEnv)
201198

202-
testRegistryserver = &RegistryClientTestServer{}
203-
registryWorkspaceDir := SetupServer(testRegistryserver)
204-
205-
testRegistryClient, err = helmreg.NewClient(helmreg.ClientOptWriter(os.Stdout))
199+
testRegistryserver, err = setupRegistryServer(ctx)
206200
if err != nil {
207-
panic(fmt.Sprintf("Failed to create OCI registry client"))
201+
panic(fmt.Sprintf("Failed to create a test registry server: %v", err))
208202
}
209203

210204
if err := (&GitRepositoryReconciler{
@@ -282,7 +276,7 @@ func TestMain(m *testing.M) {
282276
panic(fmt.Sprintf("Failed to remove storage server dir: %v", err))
283277
}
284278

285-
if err := os.RemoveAll(registryWorkspaceDir); err != nil {
279+
if err := os.RemoveAll(testRegistryserver.workspaceDir); err != nil {
286280
panic(fmt.Sprintf("Failed to remove registry workspace dir: %v", err))
287281
}
288282

0 commit comments

Comments
 (0)