Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Enterprise Search] Create HttpLogic Kea store, add http interceptors, and manage error connecting at top app-level #75790

Merged
merged 7 commits into from
Aug 24, 2020
Merged

[Enterprise Search] Create HttpLogic Kea store, add http interceptors, and manage error connecting at top app-level #75790

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c8dd2e8
[Setup] Change error connecting status code to 502
cee-chen Aug 22, 2020
93d3e01
Set up new HttpProvider/Logic Kea store & listeners
cee-chen Aug 22, 2020
e25ccc2
Update AS & WS to show error connecting component at app level
cee-chen Aug 22, 2020
5f2bcc2
[WS] Remove errorConnecting logic & http arg from Overview
cee-chen Aug 22, 2020
0874cb3
[AS] Add top-level ErrorConnecting component & remove error logic fro…
cee-chen Aug 22, 2020
ed83457
[AS] Clean up/move EngineOverview child components into subfolder
cee-chen Aug 22, 2020
d6263cb
PR feedback: Update test assertions 404 copy
constancecchen Aug 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions x-pack/plugins/enterprise_search/server/routes/app_search/engines.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,8 @@ describe('engine routes', () => {
it('should return 404 with a message', async () => {
cee-chen marked this conversation as resolved.
Show resolved Hide resolved
await mockRouter.callRoute(mockRequest);

expect(mockRouter.response.notFound).toHaveBeenCalledWith({
expect(mockRouter.response.customError).toHaveBeenCalledWith({
cee-chen marked this conversation as resolved.
Show resolved Hide resolved
statusCode: 502,
body: 'cannot-connect',
});
expect(mockLogger.error).toHaveBeenCalledWith('Cannot connect to App Search: Failed');
Expand All @@ -90,7 +91,8 @@ describe('engine routes', () => {
it('should return 404 with a message', async () => {
cee-chen marked this conversation as resolved.
Show resolved Hide resolved
await mockRouter.callRoute(mockRequest);

expect(mockRouter.response.notFound).toHaveBeenCalledWith({
expect(mockRouter.response.customError).toHaveBeenCalledWith({
statusCode: 502,
body: 'cannot-connect',
});
expect(mockLogger.error).toHaveBeenCalledWith(
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugins/enterprise_search/server/routes/app_search/engines.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ export function registerEnginesRoute({ router, config, log }: IRouteDependencies
log.error(`Cannot connect to App Search: ${e.toString()}`);
if (e instanceof Error) log.debug(e.stack as string);

return response.notFound({ body: 'cannot-connect' });
return response.customError({ statusCode: 502, body: 'cannot-connect' });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we distinguish authentication errors separately?

Copy link
Member Author

@cee-chen cee-chen Aug 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhh hm, great call. I can definitely try in a separate PR - I think I'd probably build off what Jason is doing in #75487 and check for the /login redirect there (since eventually all API endpoints should be using that helper).

We have to be careful about what statusCode we use for authentication though. If we try 401 or 403 I believe Kibana automatically responds to that by logging the Kibana user out (which is not what we want lmao). I'm tempted to keep the status code the same (502) and simply modify the body to specify a "Cannot authenticate this user" error vs a generic "Cannot connect to Enterprise Search" fallback.

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely, separate PR. Was just thinking out loud.

If we try 401 or 403 I believe Kibana automatically responds to that by logging the Kibana user out (which is not what we want lmao).

Oy, definitely not. Does Kibana install some invasive, global hook to catch all 401/403's? I am not close enough to the code, but returning a 401/403 wrapped inside a 502 feels broken. Anyway, we can discuss separately. I don't intend to hold up progress on this PR on a tangential issue. Incremental progress FTW.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't dug through their code super deeply but yeah I believe every http request/response gets run through their global http lib (both on the client-side and server-side). I definitely see why and we get plenty of advantages of doing so, it's just in this one scenario that we don't particularly want that side-effect. I remember spending a real confused half hour trying to figure out why I was getting logged out during my initial MVP work though haha.

Copy link
Member Author

@cee-chen cee-chen Aug 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also there's some nuance to the status codes here - while it's true that the user is unauthorized in Enterprise Search, they're not necessarily unauthorized in Kibana (which is generating the status code), and as such it's not necessarily a client error / a 4xx doesn't necessarily apply.

I do think the 502 status code is the most generic while still applying to our use case:

This error response means that the server, while working as a gateway to get a response needed to handle the request, got an invalid response

In this case, a /login redirect (unauthenticated in Enterprise Search) qualifies as an "invalid response" just IMO.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with you there Constance 👍

}
}
);
Expand Down
6 changes: 4 additions & 2 deletions x-pack/plugins/enterprise_search/server/routes/workplace_search/overview.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,8 @@ describe('engine routes', () => {
it('should return 404 with a message', async () => {
cee-chen marked this conversation as resolved.
Show resolved Hide resolved
await mockRouter.callRoute(mockRequest);

expect(mockRouter.response.notFound).toHaveBeenCalledWith({
expect(mockRouter.response.customError).toHaveBeenCalledWith({
statusCode: 502,
body: 'cannot-connect',
});
expect(mockLogger.error).toHaveBeenCalledWith('Cannot connect to Workplace Search: Failed');
Expand All @@ -84,7 +85,8 @@ describe('engine routes', () => {
it('should return 404 with a message', async () => {
cee-chen marked this conversation as resolved.
Show resolved Hide resolved
await mockRouter.callRoute(mockRequest);

expect(mockRouter.response.notFound).toHaveBeenCalledWith({
expect(mockRouter.response.customError).toHaveBeenCalledWith({
statusCode: 502,
body: 'cannot-connect',
});
expect(mockLogger.error).toHaveBeenCalledWith(
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugins/enterprise_search/server/routes/workplace_search/overview.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ export function registerWSOverviewRoute({ router, config, log }: IRouteDependenc
log.error(`Cannot connect to Workplace Search: ${e.toString()}`);
if (e instanceof Error) log.debug(e.stack as string);

return response.notFound({ body: 'cannot-connect' });
return response.customError({ statusCode: 502, body: 'cannot-connect' });
}
}
);
Expand Down