Skip to content

Commit 9d05e57

Browse files
weyfonkweyfonk
authored
Remove HTML response body from HelmOps errors (#4129)
When a chart version cannot be retrieved from a Helm repository, including the response body in the error message brings little context and can lead to excessively long error status messages. Therefore, Fleet now skips the response body in such cases, relying solely on the status code.
1 parent b3766b3 commit 9d05e57

4 files changed

Lines changed: 184 additions & 66 deletions

File tree

‎integrationtests/helmops/controller/controller_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1043,7 +1043,7 @@ var _ = Describe("HelmOps controller", func() {
10431043
fleet.HelmOpAcceptedCondition,
10441044
v1.ConditionFalse,
10451045
"Error",
1046-
"error code: 401, response body: Unauthorized",
1046+
"error code: 401",
10471047
)
10481048

10491049
}).Should(Succeed())
@@ -1109,7 +1109,7 @@ var _ = Describe("HelmOps controller", func() {
11091109
fleet.HelmOpAcceptedCondition,
11101110
v1.ConditionFalse,
11111111
"Error",
1112-
"error code: 401, response body: Unauthorized",
1112+
"error code: 401",
11131113
)
11141114

11151115
}).Should(Succeed())

‎internal/bundlereader/charturl.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -159,7 +159,7 @@ func getHelmChartVersion(location fleet.HelmOptions, auth Auth) (*repo.ChartVers
159159
}
160160

161161
if resp.StatusCode != 200 {
162-
return nil, fmt.Errorf("failed to read helm repo from %s, error code: %v, response body: %s", location.Repo+"index.yaml", resp.StatusCode, bytes)
162+
return nil, fmt.Errorf("failed to read helm repo from %s, error code: %v", location.Repo+"index.yaml", resp.StatusCode)
163163
}
164164

165165
repo := &repo.IndexFile{}

‎internal/bundlereader/helm_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -322,7 +322,7 @@ func TestGetManifestFromHelmChart(t *testing.T) {
322322
expectedNilManifest: true,
323323
expectedResources: []fleet.BundleResource{},
324324
expectedErrNotNil: true,
325-
expectedError: "failed to read helm repo from ##URL##/index.yaml, error code: 401, response body: Unauthorized\n",
325+
expectedError: "failed to read helm repo from ##URL##/index.yaml, error code: 401",
326326
},
327327
{
328328
name: "tls error",

‎internal/cmd/controller/helmops/reconciler/helmop_controller_test.go‎

Lines changed: 180 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@ import (
99
"fmt"
1010
"net/http"
1111
"net/http/httptest"
12+
"reflect"
13+
"regexp"
1214
"strings"
1315
"testing"
1416
"time"
@@ -79,6 +81,13 @@ func createHelmServer() *httptest.Server {
7981
}))
8082
}
8183

84+
func createHelmServerWithErrorHTML() *httptest.Server {
85+
return httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
86+
w.WriteHeader(http.StatusNotFound)
87+
fmt.Fprint(w, `<!DOCTYPE html><html><head><meta charset=\"utf-8\"><body><p>Some irrelevant content here</p></body></html>`)
88+
}))
89+
}
90+
8291
func getCondition(helmop *fleet.HelmOp, condType string) (genericcondition.GenericCondition, bool) {
8392
for _, cond := range helmop.Status.Conditions {
8493
if cond.Type == condType {
@@ -358,75 +367,170 @@ func TestReconcile_Validate(t *testing.T) {
358367
}
359368

360369
func TestReconcile_ErrorCreatingBundleIsShownInStatus(t *testing.T) {
361-
mockCtrl := gomock.NewController(t)
362-
defer mockCtrl.Finish()
363-
scheme := runtime.NewScheme()
364-
utilruntime.Must(batchv1.AddToScheme(scheme))
365-
helmop := fleet.HelmOp{
366-
ObjectMeta: metav1.ObjectMeta{
367-
Name: "helmop",
368-
Namespace: "default",
369-
},
370-
}
371-
namespacedName := types.NamespacedName{Name: helmop.Name, Namespace: helmop.Namespace}
372-
client := mocks.NewMockK8sClient(mockCtrl)
373-
client.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(1).DoAndReturn(
374-
func(ctx context.Context, req types.NamespacedName, fh *fleet.HelmOp, opts ...interface{}) error {
375-
fh.Name = helmop.Name
376-
fh.Namespace = helmop.Namespace
377-
fh.Spec.Helm = &fleet.HelmOptions{
378-
Chart: "chart.tgz",
379-
}
380-
fh.Status = fleet.HelmOpStatus{}
370+
t.Run("propagating error seen when getting the bundle", func(t *testing.T) {
371+
mockCtrl := gomock.NewController(t)
372+
defer mockCtrl.Finish()
373+
scheme := runtime.NewScheme()
374+
utilruntime.Must(batchv1.AddToScheme(scheme))
375+
helmop := fleet.HelmOp{
376+
ObjectMeta: metav1.ObjectMeta{
377+
Name: "helmop",
378+
Namespace: "default",
379+
},
380+
}
381+
namespacedName := types.NamespacedName{Name: helmop.Name, Namespace: helmop.Namespace}
382+
client := mocks.NewMockK8sClient(mockCtrl)
383+
client.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(1).DoAndReturn(
384+
func(ctx context.Context, req types.NamespacedName, fh *fleet.HelmOp, opts ...interface{}) error {
385+
fh.Name = helmop.Name
386+
fh.Namespace = helmop.Namespace
387+
fh.Spec.Helm = &fleet.HelmOptions{
388+
Chart: "chart.tgz",
389+
}
390+
fh.Status = fleet.HelmOpStatus{}
391+
392+
controllerutil.AddFinalizer(fh, finalize.HelmOpFinalizer)
393+
return nil
394+
},
395+
)
381396

382-
controllerutil.AddFinalizer(fh, finalize.HelmOpFinalizer)
383-
return nil
384-
},
385-
)
397+
client.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(1).DoAndReturn(
398+
func(ctx context.Context, req types.NamespacedName, bundle *fleet.Bundle, opts ...interface{}) error {
399+
return fmt.Errorf("this is a test error")
400+
},
401+
)
386402

387-
client.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(1).DoAndReturn(
388-
func(ctx context.Context, req types.NamespacedName, bundle *fleet.Bundle, opts ...interface{}) error {
389-
return fmt.Errorf("this is a test error")
390-
},
391-
)
403+
client.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(1).DoAndReturn(
404+
func(ctx context.Context, req types.NamespacedName, bundle *fleet.HelmOp, opts ...interface{}) error {
405+
return nil
406+
},
407+
)
392408

393-
client.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(1).DoAndReturn(
394-
func(ctx context.Context, req types.NamespacedName, bundle *fleet.HelmOp, opts ...interface{}) error {
395-
return nil
396-
},
397-
)
409+
statusClient := mocks.NewMockSubResourceWriter(mockCtrl)
410+
client.EXPECT().Status().Return(statusClient).Times(1)
411+
statusClient.EXPECT().Patch(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Do(
412+
func(ctx context.Context, helmop *fleet.HelmOp, patch crclient.Patch, opts ...interface{}) {
413+
c, found := getCondition(helmop, fleet.HelmOpAcceptedCondition)
414+
if !found {
415+
t.Errorf("expecting to find the %s condition and could not find it.", fleet.HelmOpAcceptedCondition)
416+
}
417+
if c.Message != "this is a test error" {
418+
t.Errorf("expecting message [this is a test error] in condition, got [%s]", c.Message)
419+
}
420+
},
421+
).Times(1)
398422

399-
statusClient := mocks.NewMockSubResourceWriter(mockCtrl)
400-
client.EXPECT().Status().Return(statusClient).Times(1)
401-
statusClient.EXPECT().Patch(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Do(
402-
func(ctx context.Context, helmop *fleet.HelmOp, patch crclient.Patch, opts ...interface{}) {
403-
c, found := getCondition(helmop, fleet.HelmOpAcceptedCondition)
404-
if !found {
405-
t.Errorf("expecting to find the %s condition and could not find it.", fleet.HelmOpAcceptedCondition)
406-
}
407-
if c.Message != "this is a test error" {
408-
t.Errorf("expecting message [this is a test error] in condition, got [%s]", c.Message)
409-
}
410-
},
411-
).Times(1)
423+
r := HelmOpReconciler{
424+
Client: client,
425+
Scheme: scheme,
426+
}
412427

413-
r := HelmOpReconciler{
414-
Client: client,
415-
Scheme: scheme,
416-
}
428+
ctx := context.TODO()
417429

418-
ctx := context.TODO()
430+
res, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: namespacedName})
431+
if err == nil {
432+
t.Errorf("expecting error, got nil")
433+
}
434+
if err.Error() != "this is a test error" {
435+
t.Errorf("expecting error: [this is a test error], got %v", err.Error())
436+
}
437+
if res.RequeueAfter != 0 {
438+
t.Errorf("expecting no requeue when there's an error, but got RequeueAfter: %v", res.RequeueAfter)
439+
}
440+
})
441+
442+
t.Run("propagating error from Helm registry without HTML response body", func(t *testing.T) {
443+
svr := createHelmServerWithErrorHTML()
444+
defer svr.Close()
445+
446+
mockCtrl := gomock.NewController(t)
447+
defer mockCtrl.Finish()
448+
scheme := runtime.NewScheme()
449+
utilruntime.Must(batchv1.AddToScheme(scheme))
450+
helmop := fleet.HelmOp{
451+
ObjectMeta: metav1.ObjectMeta{
452+
Name: "helmop",
453+
Namespace: "default",
454+
},
455+
Spec: fleet.HelmOpSpec{
456+
BundleSpec: fleet.BundleSpec{
457+
BundleDeploymentOptions: fleet.BundleDeploymentOptions{
458+
Helm: &fleet.HelmOptions{
459+
Repo: svr.URL,
460+
Chart: "alpine",
461+
Version: "0.1.0", // static version
462+
},
463+
},
464+
},
465+
InsecureSkipTLSverify: true,
466+
},
467+
}
419468

420-
res, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: namespacedName})
421-
if err == nil {
422-
t.Errorf("expecting error, got nil")
423-
}
424-
if err.Error() != "this is a test error" {
425-
t.Errorf("expecting error: [this is a test error], got %v", err.Error())
426-
}
427-
if res.RequeueAfter != 0 {
428-
t.Errorf("expecting no requeue when there's an error, but got RequeueAfter: %v", res.RequeueAfter)
429-
}
469+
namespacedName := types.NamespacedName{Name: helmop.Name, Namespace: helmop.Namespace}
470+
client := mocks.NewMockK8sClient(mockCtrl)
471+
client.EXPECT().Get(gomock.Any(), gomock.Any(), OfType(&fleet.HelmOp{}), gomock.Any()).Times(1).DoAndReturn(
472+
func(ctx context.Context, req types.NamespacedName, fh *fleet.HelmOp, opts ...interface{}) error {
473+
fh.Name = helmop.Name
474+
fh.Namespace = helmop.Namespace
475+
fh.Spec = helmop.Spec
476+
fh.Status = fleet.HelmOpStatus{}
477+
478+
controllerutil.AddFinalizer(fh, finalize.HelmOpFinalizer)
479+
return nil
480+
},
481+
)
482+
483+
client.EXPECT().Get(gomock.Any(), gomock.Any(), OfType(&fleet.Bundle{}), gomock.Any()).AnyTimes().DoAndReturn(
484+
func(ctx context.Context, req types.NamespacedName, bundle *fleet.Bundle, opts ...interface{}) error {
485+
bundle.Spec.HelmOpOptions = &fleet.BundleHelmOptions{}
486+
return nil
487+
},
488+
)
489+
490+
client.EXPECT().Get(gomock.Any(), gomock.Any(), OfType(&fleet.HelmOp{}), gomock.Any()).AnyTimes().DoAndReturn(
491+
func(ctx context.Context, req types.NamespacedName, helmOp *fleet.HelmOp, opts ...interface{}) error {
492+
return nil
493+
},
494+
)
495+
496+
statusClient := mocks.NewMockSubResourceWriter(mockCtrl)
497+
client.EXPECT().Status().Return(statusClient).Times(1)
498+
statusClient.EXPECT().Patch(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Do(
499+
func(ctx context.Context, helmop *fleet.HelmOp, patch crclient.Patch, opts ...interface{}) {
500+
c, found := getCondition(helmop, fleet.HelmOpAcceptedCondition)
501+
if !found {
502+
t.Errorf("expecting to find the %s condition and could not find it.", fleet.HelmOpAcceptedCondition)
503+
}
504+
if !strings.Contains(c.Message, "404") {
505+
t.Errorf("expecting message to contain [404] in condition, got [%s]", c.Message)
506+
}
507+
},
508+
).Times(1)
509+
510+
r := HelmOpReconciler{
511+
Client: client,
512+
Scheme: scheme,
513+
}
514+
515+
ctx := context.TODO()
516+
517+
res, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: namespacedName})
518+
if err == nil {
519+
t.Errorf("expecting error, got nil")
520+
}
521+
522+
errRegex := "could not get a chart version.*error code: 404"
523+
match, rErr := regexp.Match(errRegex, []byte(err.Error()))
524+
if rErr != nil {
525+
t.Errorf("something went wrong when compiling the regex: %v", rErr)
526+
}
527+
if !match {
528+
t.Errorf("expecting error matching %q, got %v", errRegex, err)
529+
}
530+
if res.RequeueAfter != 0 {
531+
t.Errorf("expecting no requeue when there's an error, but got RequeueAfter: %v", res.RequeueAfter)
532+
}
533+
})
430534
}
431535

432536
// Validates that the HelmOps reconciler will not create a bundle if another bundle exists with the same name, for
@@ -1264,3 +1368,17 @@ func (s *scheduledJobMatcher) Matches(x interface{}) bool {
12641368
func (s *scheduledJobMatcher) String() string {
12651369
return fmt.Sprintf("matches replace %t and job key %s", s.replaceExisting, s.key)
12661370
}
1371+
1372+
type typeMatcher struct{ t interface{} }
1373+
1374+
func OfType(t interface{}) gomock.Matcher {
1375+
return &typeMatcher{t}
1376+
}
1377+
1378+
func (tm *typeMatcher) Matches(x interface{}) bool {
1379+
return reflect.TypeOf(x) == reflect.TypeOf(tm.t)
1380+
}
1381+
1382+
func (tm *typeMatcher) String() string {
1383+
return "is of type " + reflect.TypeOf(tm.t).String()
1384+
}

0 commit comments

Comments
 (0)