Skip to content

Commit 2246fa7

Browse files
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 <[email protected]>
1 parent 841ed7a commit 2246fa7

6 files changed

+65
-59
lines changed

controllers/helmchart_controller.go

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

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

527532
if file != "" {
@@ -534,7 +539,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
534539
clientOpts = append(clientOpts, helmgetter.WithRegistryClient(registryClient))
535540
ociChartRepo, err := repository.NewOCIChartRepository(repo.Spec.URL, repository.WithOCIGetter(r.Getters), repository.WithOCIGetterOptions(clientOpts), repository.WithOCIRegistryClient(registryClient))
536541
if err != nil {
537-
return chartRepoErrorReturn(err, obj)
542+
return chartRepoConfigErrorReturn(err, obj)
538543
}
539544
chartRepo = ociChartRepo
540545

@@ -543,7 +548,12 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
543548
if logOpts != nil {
544549
err = ociChartRepo.Login(logOpts...)
545550
if err != nil {
546-
return chartRepoErrorReturn(err, obj)
551+
e := &serror.Event{
552+
Err: fmt.Errorf("failed to login to OCI registry: %w", err),
553+
Reason: meta.FailedReason,
554+
}
555+
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
556+
return sreconcile.ResultEmpty, e
547557
}
548558
}
549559
default:
@@ -553,7 +563,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
553563
r.IncCacheEvents(event, obj.Name, obj.Namespace)
554564
}))
555565
if err != nil {
556-
return chartRepoErrorReturn(err, obj)
566+
return chartRepoConfigErrorReturn(err, obj)
557567
}
558568
chartRepo = httpChartRepo
559569
defer func() {
@@ -1142,7 +1152,7 @@ func reasonForBuild(build *chart.Build) string {
11421152
return sourcev1.ChartPullSucceededReason
11431153
}
11441154

1145-
func chartRepoErrorReturn(err error, obj *sourcev1.HelmChart) (sreconcile.Result, error) {
1155+
func chartRepoConfigErrorReturn(err error, obj *sourcev1.HelmChart) (sreconcile.Result, error) {
11461156
switch err.(type) {
11471157
case *url.Error:
11481158
e := &serror.Stalling{

controllers/helmchart_controller_test.go

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

793793
// Login to the registry
794-
err := testRegistryserver.RegistryClient.Login(testRegistryserver.DockerRegistryHost,
795-
registry.LoginOptBasicAuth(testUsername, testPassword),
794+
err := testRegistryserver.registryClient.Login(testRegistryserver.dockerRegistryHost,
795+
registry.LoginOptBasicAuth(testRegistryUsername, testRegistryPassword),
796796
registry.LoginOptInsecure(true))
797797
g.Expect(err).NotTo(HaveOccurred())
798798

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

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

810810
storage, err := NewStorage(tmpDir, "example.com", retentionTTL, retentionRecords)
@@ -832,8 +832,8 @@ func TestHelmChartReconciler_buildFromOCIHelmRepository(t *testing.T) {
832832
Name: "auth",
833833
},
834834
Data: map[string][]byte{
835-
"username": []byte(testUsername),
836-
"password": []byte(testPassword),
835+
"username": []byte(testRegistryUsername),
836+
"password": []byte(testRegistryPassword),
837837
},
838838
},
839839
beforeFunc: func(obj *sourcev1.HelmChart, repository *sourcev1.HelmRepository) {
@@ -953,7 +953,7 @@ func TestHelmChartReconciler_buildFromOCIHelmRepository(t *testing.T) {
953953
GenerateName: "helmrepository-",
954954
},
955955
Spec: sourcev1.HelmRepositorySpec{
956-
URL: fmt.Sprintf("oci://%s/testrepo", testRegistryserver.DockerRegistryHost),
956+
URL: fmt.Sprintf("oci://%s/testrepo", testRegistryserver.dockerRegistryHost),
957957
Timeout: &metav1.Duration{Duration: timeout},
958958
Type: sourcev1.HelmRepositoryTypeOCI,
959959
},

controllers/helmrepository_controller_oci.go

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -287,19 +287,15 @@ func (r *HelmRepositoryOCIReconciler) reconcileSource(ctx context.Context, obj *
287287
logOpts = append(logOpts, logOpt)
288288
}
289289

290-
if result, err := r.validateSource(ctx, obj, logOpts...); err != nil || result == sreconcile.ResultEmpty {
291-
return result, err
292-
}
293-
294-
return sreconcile.ResultSuccess, nil
290+
return r.validateSource(ctx, obj, logOpts...)
295291
}
296292

297293
// validateSource the HelmRepository object by checking the url and connecting to the underlying registry
298294
// with he provided credentials.
299295
func (r *HelmRepositoryOCIReconciler) validateSource(ctx context.Context, obj *sourcev1.HelmRepository, logOpts ...registry.LoginOption) (sreconcile.Result, error) {
300296
registryClient, file, err := r.RegistryClientGenerator(logOpts != nil)
301297
if err != nil {
302-
e := &serror.Stalling{
298+
e := &serror.Event{
303299
Err: fmt.Errorf("failed to create registry client:: %w", err),
304300
Reason: meta.FailedReason,
305301
}

controllers/helmrepository_controller_oci_test.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -46,8 +46,8 @@ func TestHelmRepositoryOCIReconciler_Reconcile(t *testing.T) {
4646
Namespace: ns.Name,
4747
},
4848
Data: map[string][]byte{
49-
"username": []byte(testUsername),
50-
"password": []byte(testPassword),
49+
"username": []byte(testRegistryUsername),
50+
"password": []byte(testRegistryPassword),
5151
},
5252
}
5353

@@ -60,7 +60,7 @@ func TestHelmRepositoryOCIReconciler_Reconcile(t *testing.T) {
6060
},
6161
Spec: sourcev1.HelmRepositorySpec{
6262
Interval: metav1.Duration{Duration: interval},
63-
URL: fmt.Sprintf("oci://%s", testRegistryserver.DockerRegistryHost),
63+
URL: fmt.Sprintf("oci://%s", testRegistryserver.dockerRegistryHost),
6464
SecretRef: &meta.LocalObjectReference{
6565
Name: secret.Name,
6666
},

controllers/helmrepository_controller_test.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -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
}

controllers/suite_test.go

Lines changed: 34 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -94,68 +94,66 @@ var (
9494

9595
var (
9696
testRegistryClient *registry.Client
97-
testRegistryserver *RegistryClientTestServer
97+
testRegistryserver *registryClientTestServer
9898
)
9999

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

107107
func init() {
108108
rand.Seed(time.Now().UnixNano())
109109
}
110110

111-
type RegistryClientTestServer struct {
112-
Out io.Writer
113-
DockerRegistryHost string
114-
WorkspaceDir string
115-
RegistryClient *registry.Client
111+
type registryClientTestServer struct {
112+
out io.Writer
113+
dockerRegistryHost string
114+
workspaceDir string
115+
registryClient *registry.Client
116116
}
117117

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

127128
var out bytes.Buffer
128-
server.Out = &out
129+
server.out = &out
129130

130131
// init test client
131-
server.RegistryClient, err = registry.NewClient(
132+
server.registryClient, err = registry.NewClient(
132133
registry.ClientOptDebug(true),
133-
registry.ClientOptWriter(server.Out),
134+
registry.ClientOptWriter(server.out),
134135
)
135136
if err != nil {
136-
panic(fmt.Sprintf("failed to create registry client: %s", err))
137+
return nil, fmt.Errorf("failed to create registry client: %s", err)
137138
}
138139

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

145-
htpasswdPath := filepath.Join(testWorkspaceDir, testHtpasswdFileBasename)
146-
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)
147148
if err != nil {
148-
panic(fmt.Sprintf("failed to create htpasswd file: %s", err))
149+
return nil, fmt.Errorf("failed to create htpasswd file: %s", err)
149150
}
150151

151152
// Registry config
152153
config := &configuration.Configuration{}
153154
port, err := freeport.GetFreePort()
154-
if err != nil {
155-
panic(fmt.Sprintf("failed to get free port: %s", err))
156-
}
157155

158-
server.DockerRegistryHost = fmt.Sprintf("localhost:%d", port)
156+
server.dockerRegistryHost = fmt.Sprintf("localhost:%d", port)
159157
config.HTTP.Addr = fmt.Sprintf("127.0.0.1:%d", port)
160158
config.HTTP.DrainTimeout = time.Duration(10) * time.Second
161159
config.Storage = map[string]configuration.Parameters{"inmemory": map[string]interface{}{}}
@@ -165,15 +163,15 @@ func SetupServer(server *RegistryClientTestServer) string {
165163
"path": htpasswdPath,
166164
},
167165
}
168-
dockerRegistry, err := dockerRegistry.NewRegistry(context.Background(), config)
166+
dockerRegistry, err := dockerRegistry.NewRegistry(ctx, config)
169167
if err != nil {
170-
panic(fmt.Sprintf("failed to create docker registry: %s", err))
168+
return nil, fmt.Errorf("failed to create docker registry: %s", err)
171169
}
172170

173171
// Start Docker registry
174172
go dockerRegistry.ListenAndServe()
175173

176-
return server.WorkspaceDir
174+
return server, nil
177175
}
178176

179177
func TestMain(m *testing.M) {
@@ -198,8 +196,10 @@ func TestMain(m *testing.M) {
198196

199197
testMetricsH = controller.MustMakeMetrics(testEnv)
200198

201-
testRegistryserver = &RegistryClientTestServer{}
202-
registryWorkspaceDir := SetupServer(testRegistryserver)
199+
testRegistryserver, err = setupRegistryServer(ctx)
200+
if err != nil {
201+
panic(fmt.Sprintf("Failed to create a test registry server: %v", err))
202+
}
203203

204204
testRegistryClient, err = registry.NewClient(registry.ClientOptWriter(os.Stdout))
205205
if err != nil {
@@ -280,7 +280,7 @@ func TestMain(m *testing.M) {
280280
panic(fmt.Sprintf("Failed to remove storage server dir: %v", err))
281281
}
282282

283-
if err := os.RemoveAll(registryWorkspaceDir); err != nil {
283+
if err := os.RemoveAll(testRegistryserver.workspaceDir); err != nil {
284284
panic(fmt.Sprintf("Failed to remove registry workspace dir: %v", err))
285285
}
286286

0 commit comments

Comments
 (0)