Skip to content

Commit 8491e57

Browse files
committed
Replace stalling events in HelmChart and HelmRepository_OCI
The setupRegistryServer has been refactored to take into account fluxcd#690 reviews. Signed-off-by: Soule BA <soule@weave.works>
1 parent 55a594a commit 8491e57

6 files changed

+74
-72
lines changed

controllers/helmchart_controller.go

+16-6
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

+9-9
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

+3-7
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

+5-5
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

+6-6
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

+35-39
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,12 @@ const (
6666
retentionRecords = 2
6767
)
6868

69+
const (
70+
testRegistryHtpasswdFileBasename = "authtest.htpasswd"
71+
testRegistryUsername = "myuser"
72+
testRegistryPassword = "mypass"
73+
)
74+
6975
var (
7076
testEnv *testenv.Environment
7177
testStorage *Storage
@@ -94,69 +100,62 @@ var (
94100
)
95101

96102
var (
97-
testRegistryClient *helmreg.Client
98-
testRegistryserver *RegistryClientTestServer
99-
)
100-
101-
var (
102-
testWorkspaceDir = "registry-test"
103-
testHtpasswdFileBasename = "authtest.htpasswd"
104-
testUsername = "myuser"
105-
testPassword = "mypass"
103+
testRegistryServer *registryClientTestServer
106104
)
107105

108106
func init() {
109107
rand.Seed(time.Now().UnixNano())
110108
}
111109

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

119-
func SetupServer(server *RegistryClientTestServer) string {
117+
func setupRegistryServer(ctx context.Context) (*registryClientTestServer, error) {
118+
server := &registryClientTestServer{}
119+
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+
workspaceDir, err := os.MkdirTemp("", "registry-test-")
124122
if err != nil {
125-
panic(fmt.Sprintf("failed to create workspace directory: %s", err))
123+
return nil, fmt.Errorf("failed to create workspace directory: %w", err)
126124
}
125+
server.workspaceDir = workspaceDir
127126

128127
var out bytes.Buffer
129-
server.Out = &out
128+
server.out = &out
130129

131130
// init test client
132-
server.RegistryClient, err = helmreg.NewClient(
131+
server.registryClient, err = helmreg.NewClient(
133132
helmreg.ClientOptDebug(true),
134-
helmreg.ClientOptWriter(server.Out),
133+
helmreg.ClientOptWriter(server.out),
135134
)
136135
if err != nil {
137-
panic(fmt.Sprintf("failed to create registry client: %s", err))
136+
return nil, fmt.Errorf("failed to create registry client: %s", err)
138137
}
139138

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

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

152151
// Registry config
153152
config := &configuration.Configuration{}
154153
port, err := freeport.GetFreePort()
155154
if err != nil {
156-
panic(fmt.Sprintf("failed to get free port: %s", err))
155+
return nil, fmt.Errorf("failed to get free port: %s", err)
157156
}
158157

159-
server.DockerRegistryHost = fmt.Sprintf("localhost:%d", port)
158+
server.dockerRegistryHost = fmt.Sprintf("localhost:%d", port)
160159
config.HTTP.Addr = fmt.Sprintf("127.0.0.1:%d", port)
161160
config.HTTP.DrainTimeout = time.Duration(10) * time.Second
162161
config.Storage = map[string]configuration.Parameters{"inmemory": map[string]interface{}{}}
@@ -166,15 +165,15 @@ func SetupServer(server *RegistryClientTestServer) string {
166165
"path": htpasswdPath,
167166
},
168167
}
169-
dockerRegistry, err := dockerRegistry.NewRegistry(context.Background(), config)
168+
dockerRegistry, err := dockerRegistry.NewRegistry(ctx, config)
170169
if err != nil {
171-
panic(fmt.Sprintf("failed to create docker registry: %s", err))
170+
return nil, fmt.Errorf("failed to create docker registry: %w", err)
172171
}
173172

174173
// Start Docker registry
175174
go dockerRegistry.ListenAndServe()
176175

177-
return server.WorkspaceDir
176+
return server, nil
178177
}
179178

180179
func TestMain(m *testing.M) {
@@ -199,12 +198,9 @@ func TestMain(m *testing.M) {
199198

200199
testMetricsH = controller.MustMakeMetrics(testEnv)
201200

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

210206
if err := (&GitRepositoryReconciler{
@@ -282,7 +278,7 @@ func TestMain(m *testing.M) {
282278
panic(fmt.Sprintf("Failed to remove storage server dir: %v", err))
283279
}
284280

285-
if err := os.RemoveAll(registryWorkspaceDir); err != nil {
281+
if err := os.RemoveAll(testRegistryServer.workspaceDir); err != nil {
286282
panic(fmt.Sprintf("Failed to remove registry workspace dir: %v", err))
287283
}
288284

0 commit comments

Comments
 (0)