Skip to content

Commit

Permalink
Avoid HTTP error codes when running validation (#1739)
Browse files Browse the repository at this point in the history
  • Loading branch information
Andres Martinez Gotor authored May 18, 2020
1 parent 38834ee commit bb0cb76
Show file tree
Hide file tree
Showing 8 changed files with 161 additions and 67 deletions.
10 changes: 6 additions & 4 deletions dashboard/src/actions/repos.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -698,14 +698,16 @@ describe("checkChart", () => {

describe("validateRepo", () => {
it("dispatches repoValidating and repoValidated if no error", async () => {
AppRepository.validate = jest.fn(() => "OK");
AppRepository.validate = jest.fn(() => {
return { code: 200, message: "OK" };
});
const expectedActions = [
{
type: getType(repoActions.repoValidating),
},
{
type: getType(repoActions.repoValidated),
payload: "OK",
payload: { code: 200, message: "OK" },
},
];

Expand Down Expand Up @@ -735,7 +737,7 @@ describe("validateRepo", () => {

it("dispatches checkRepo and errorRepos when the validation cannot be parsed", async () => {
AppRepository.validate = jest.fn(() => {
return { statusCode: 409 };
return { code: 409, message: "forbidden" };
});
const expectedActions = [
{
Expand All @@ -744,7 +746,7 @@ describe("validateRepo", () => {
{
type: getType(repoActions.errorRepos),
payload: {
err: new Error('Unable to parse validation response, got: {"statusCode":409}'),
err: new Error('{"code":409,"message":"forbidden"}'),
op: "validate",
},
},
Expand Down
10 changes: 2 additions & 8 deletions dashboard/src/actions/repos.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -304,17 +304,11 @@ export const validateRepo = (
try {
dispatch(repoValidating());
const data = await AppRepository.validate(repoURL, authHeader, customCA);
if (data === "OK") {
if (data.code === 200) {
dispatch(repoValidated(data));
return true;
} else {
// Unexpected error
dispatch(
errorRepos(
new Error(`Unable to parse validation response, got: ${JSON.stringify(data)}`),
"validate",
),
);
dispatch(errorRepos(new Error(JSON.stringify(data)), "validate"));
return false;
}
} catch (e) {
Expand Down
21 changes: 14 additions & 7 deletions dashboard/src/components/Config/AppRepoList/AppRepoForm.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -358,13 +358,7 @@ export class AppRepoForm extends React.Component<IAppRepoFormProps, IAppRepoForm
installation.
</div>
)}
{this.props.validationError && (
<UnexpectedErrorAlert
title="Validation Failed. Got:"
text={this.props.validationError.message}
raw={true}
/>
)}
{this.props.validationError && this.parseValidationError(this.props.validationError)}
<div>
<button
className="button button-primary"
Expand Down Expand Up @@ -500,4 +494,17 @@ export class AppRepoForm extends React.Component<IAppRepoFormProps, IAppRepoForm
});
this.setState({ selectedImagePullSecrets });
};

private parseValidationError = (error: Error) => {
let message = error.message;
try {
const parsedMessage = JSON.parse(message);
if (parsedMessage.code && parsedMessage.message) {
message = `Code: ${parsedMessage.code}. Message: ${parsedMessage.message}`;
}
} catch (e) {
// Not a json message
}
return <UnexpectedErrorAlert title="Validation Failed. Got:" text={message} raw={true} />;
};
}
12 changes: 3 additions & 9 deletions pkg/http-handler/http-handler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ package httphandler

import (
"encoding/json"
"io/ioutil"
"net/http"
"os"
"strings"
Expand Down Expand Up @@ -116,17 +115,12 @@ func ValidateAppRepository(handler kube.AuthHandler) func(w http.ResponseWriter,
returnK8sError(err, w)
return
}
body, err := ioutil.ReadAll(res.Body)
responseBody, err := json.Marshal(res)
if err != nil {
returnK8sError(err, w)
JSONError(w, err.Error(), http.StatusInternalServerError)
return
}
w.WriteHeader(res.StatusCode)
if res.StatusCode == 200 {
w.Write([]byte("OK"))
} else {
w.Write(body)
}
w.Write(responseBody)
}
}

Expand Down
36 changes: 27 additions & 9 deletions pkg/http-handler/http-handler_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"bytes"
"encoding/json"
"fmt"
"io/ioutil"
"net/http/httptest"
"strings"
"testing"
Expand Down Expand Up @@ -250,23 +251,35 @@ func TestGetNamespaces(t *testing.T) {

func TestValidateAppRepository(t *testing.T) {
testCases := []struct {
name string
err error
expectedCode int
name string
err error
validationResponse kube.ValidationResponse
expectedCode int
expectedBody string
}{
{
name: "it should return OK if no error is detected",
expectedCode: 200,
name: "it should return OK if no error is detected",
validationResponse: kube.ValidationResponse{Code: 200, Message: "OK"},
expectedCode: 200,
expectedBody: `{"code":200,"message":"OK"}`,
},
{
name: "it should return the error code if given",
err: fmt.Errorf("Boom"),
expectedCode: 500,
name: "it should return the error code if given",
err: fmt.Errorf("Boom"),
validationResponse: kube.ValidationResponse{},
expectedCode: 500,
expectedBody: "\"Boom\"\n",
},
{
name: "it should return an error in the validation response",
validationResponse: kube.ValidationResponse{Code: 401, Message: "Forbidden"},
expectedCode: 200,
expectedBody: `{"code":401,"message":"Forbidden"}`,
},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
validateAppRepoFunc := ValidateAppRepository(&kube.FakeHandler{Err: tc.err})
validateAppRepoFunc := ValidateAppRepository(&kube.FakeHandler{ValRes: &tc.validationResponse, Err: tc.err})
req := httptest.NewRequest("POST", "https://foo.bar/backend/v1/namespaces/kubeapps/apprepositories/validate", strings.NewReader("data"))

response := httptest.NewRecorder()
Expand All @@ -275,6 +288,11 @@ func TestValidateAppRepository(t *testing.T) {
if got, want := response.Code, tc.expectedCode; got != want {
t.Errorf("got: %d, want: %d\nBody: %s", got, want, response.Body)
}

responseBody, _ := ioutil.ReadAll(response.Body)
if got, want := string(responseBody), tc.expectedBody; got != want {
t.Errorf("got: %s, want: %s\n", got, want)
}
})
}
}
Expand Down
8 changes: 3 additions & 5 deletions pkg/kube/fake.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,6 @@ package kube
import (
"fmt"
"io"
"io/ioutil"
"net/http"
"strings"

v1alpha1 "github.com/kubeapps/kubeapps/cmd/apprepository-controller/pkg/apis/apprepository/v1alpha1"
corev1 "k8s.io/api/core/v1"
Expand All @@ -34,6 +31,7 @@ type FakeHandler struct {
UpdatedRepo *v1alpha1.AppRepository
Namespaces []corev1.Namespace
Secrets []*corev1.Secret
ValRes *ValidationResponse
Err error
}

Expand Down Expand Up @@ -89,8 +87,8 @@ func (c *FakeHandler) GetSecret(name, namespace string) (*corev1.Secret, error)
}

// ValidateAppRepository fake
func (c *FakeHandler) ValidateAppRepository(appRepoBody io.ReadCloser, requestNamespace string) (*http.Response, error) {
return &http.Response{Body: ioutil.NopCloser(strings.NewReader("valid: yaml")), StatusCode: 200}, c.Err
func (c *FakeHandler) ValidateAppRepository(appRepoBody io.ReadCloser, requestNamespace string) (*ValidationResponse, error) {
return c.ValRes, c.Err
}

// GetOperatorLogo fake
Expand Down
68 changes: 49 additions & 19 deletions pkg/kube/kube_handler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"encoding/json"
"fmt"
"io"
"io/ioutil"
"net/http"
"strings"

Expand Down Expand Up @@ -93,6 +94,12 @@ type userHandler struct {
clientset combinedClientsetInterface
}

// ValidationResponse represents the response after validating a repo
type ValidationResponse struct {
Code int `json:"code"`
Message string `json:"message"`
}

// This interface is explicitly private so that it cannot be used in function
// args, so that call-sites cannot accidentally pass a service handler in place
// of a user handler.
Expand All @@ -106,7 +113,7 @@ type handler interface {
GetNamespaces() ([]corev1.Namespace, error)
GetSecret(name, namespace string) (*corev1.Secret, error)
GetAppRepository(repoName, repoNamespace string) (*v1alpha1.AppRepository, error)
ValidateAppRepository(appRepoBody io.ReadCloser, requestNamespace string) (*http.Response, error)
ValidateAppRepository(appRepoBody io.ReadCloser, requestNamespace string) (*ValidationResponse, error)
GetOperatorLogo(namespace, name string) ([]byte, error)
}

Expand Down Expand Up @@ -227,24 +234,17 @@ func (a *kubeHandler) clientsetForRequest(token string) (combinedClientsetInterf
return clientset, err
}

func parseRepoAndSecret(appRepoBody io.ReadCloser) (*v1alpha1.AppRepository, *corev1.Secret, error) {
func parseRepoRequest(appRepoBody io.ReadCloser) (*appRepositoryRequest, error) {
var appRepoRequest appRepositoryRequest
err := json.NewDecoder(appRepoBody).Decode(&appRepoRequest)
if err != nil {
log.Infof("unable to decode: %v", err)
return nil, nil, err
return nil, err
}

appRepo := appRepositoryForRequest(appRepoRequest)
repoSecret := secretForRequest(appRepoRequest, appRepo)
return appRepo, repoSecret, nil
return &appRepoRequest, nil
}

func (a *userHandler) applyAppRepositorySecret(repoSecret *corev1.Secret, requestNamespace string, appRepo *v1alpha1.AppRepository) error {
// TODO(#1655) Fixes the immediate issue, but the proper fix would no
// longer set the complete owner reference during secretForRequest and
// rather do so explicitly here.
repoSecret.ObjectMeta.OwnerReferences[0].UID = appRepo.ObjectMeta.UID
_, err := a.clientset.CoreV1().Secrets(requestNamespace).Create(repoSecret)
if err != nil && k8sErrors.IsAlreadyExists(err) {
_, err = a.clientset.CoreV1().Secrets(requestNamespace).Update(repoSecret)
Expand Down Expand Up @@ -275,7 +275,12 @@ func (a *userHandler) CreateAppRepository(appRepoBody io.ReadCloser, requestName
return nil, fmt.Errorf("kubeappsNamespace must be configured to enable app repository handler")
}

appRepo, repoSecret, err := parseRepoAndSecret(appRepoBody)
appRepoRequest, err := parseRepoRequest(appRepoBody)
if err != nil {
return nil, err
}

appRepo := appRepositoryForRequest(appRepoRequest)
if err != nil {
return nil, err
}
Expand All @@ -290,6 +295,7 @@ func (a *userHandler) CreateAppRepository(appRepoBody io.ReadCloser, requestName
return nil, err
}

repoSecret := secretForRequest(appRepoRequest, appRepo)
if repoSecret != nil {
a.applyAppRepositorySecret(repoSecret, requestNamespace, appRepo)
if err != nil {
Expand All @@ -306,7 +312,12 @@ func (a *userHandler) UpdateAppRepository(appRepoBody io.ReadCloser, requestName
return nil, fmt.Errorf("kubeappsNamespace must be configured to enable app repository handler")
}

appRepo, repoSecret, err := parseRepoAndSecret(appRepoBody)
appRepoRequest, err := parseRepoRequest(appRepoBody)
if err != nil {
return nil, err
}

appRepo := appRepositoryForRequest(appRepoRequest)
if err != nil {
return nil, err
}
Expand All @@ -327,6 +338,7 @@ func (a *userHandler) UpdateAppRepository(appRepoBody io.ReadCloser, requestName
return nil, err
}

repoSecret := secretForRequest(appRepoRequest, appRepo)
if repoSecret != nil {
a.applyAppRepositorySecret(repoSecret, requestNamespace, appRepo)
if err != nil {
Expand Down Expand Up @@ -358,7 +370,12 @@ func (a *userHandler) DeleteAppRepository(repoName, repoNamespace string) error
}

func getValidationCliAndReq(appRepoBody io.ReadCloser, requestNamespace, kubeappsNamespace string) (HTTPClient, *http.Request, error) {
appRepo, repoSecret, err := parseRepoAndSecret(appRepoBody)
appRepoRequest, err := parseRepoRequest(appRepoBody)
if err != nil {
return nil, nil, err
}

appRepo := appRepositoryForRequest(appRepoRequest)
if err != nil {
return nil, nil, err
}
Expand All @@ -368,6 +385,7 @@ func getValidationCliAndReq(appRepoBody io.ReadCloser, requestNamespace, kubeapp
return nil, nil, ErrGlobalRepositoryWithSecrets
}

repoSecret := secretForRequest(appRepoRequest, appRepo)
cli, err := InitNetClient(appRepo, repoSecret, repoSecret, nil)
if err != nil {
return nil, nil, fmt.Errorf("Unable to create HTTP client: %w", err)
Expand All @@ -380,14 +398,26 @@ func getValidationCliAndReq(appRepoBody io.ReadCloser, requestNamespace, kubeapp
return cli, req, nil
}

func (a *userHandler) ValidateAppRepository(appRepoBody io.ReadCloser, requestNamespace string) (*http.Response, error) {
func doValidationRequest(cli HTTPClient, req *http.Request) (*ValidationResponse, error) {
res, err := cli.Do(req)
if err != nil {
// If the request fail, it's not an internal error
return &ValidationResponse{Code: 400, Message: err.Error()}, nil
}
body, err := ioutil.ReadAll(res.Body)
if err != nil {
return nil, fmt.Errorf("Unable to parse validation response. Got: %v", err)
}
return &ValidationResponse{Code: res.StatusCode, Message: string(body)}, nil
}

func (a *userHandler) ValidateAppRepository(appRepoBody io.ReadCloser, requestNamespace string) (*ValidationResponse, error) {
// Split body parsing to a different function for ease testing
cli, req, err := getValidationCliAndReq(appRepoBody, requestNamespace, a.kubeappsNamespace)
if err != nil {
return nil, err
}

return cli.Do(req)
return doValidationRequest(cli, req)
}

// GetAppRepository returns an AppRepository resource from a namespace.
Expand All @@ -397,7 +427,7 @@ func (a *userHandler) GetAppRepository(repoName, repoNamespace string) (*v1alpha
}

// appRepositoryForRequest takes care of parsing the request data into an AppRepository.
func appRepositoryForRequest(appRepoRequest appRepositoryRequest) *v1alpha1.AppRepository {
func appRepositoryForRequest(appRepoRequest *appRepositoryRequest) *v1alpha1.AppRepository {
appRepo := appRepoRequest.AppRepository

var auth v1alpha1.AppRepositoryAuth
Expand Down Expand Up @@ -441,7 +471,7 @@ func appRepositoryForRequest(appRepoRequest appRepositoryRequest) *v1alpha1.AppR
}

// secretForRequest takes care of parsing the request data into a secret for an AppRepository.
func secretForRequest(appRepoRequest appRepositoryRequest, appRepo *v1alpha1.AppRepository) *corev1.Secret {
func secretForRequest(appRepoRequest *appRepositoryRequest, appRepo *v1alpha1.AppRepository) *corev1.Secret {
appRepoDetails := appRepoRequest.AppRepository
secrets := map[string]string{}
if appRepoDetails.AuthHeader != "" {
Expand Down
Loading

0 comments on commit bb0cb76

Please sign in to comment.