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.

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

[MD] Refactor data source server side error handling #2661

Merged
merged 7 commits into from
Oct 27, 2022

Conversation

zhongnansu
Copy link
Member

@zhongnansu zhongnansu commented Oct 24, 2022

Signed-off-by: Su szhongna@amazon.com

Description

Refactor data source server side error handling

  • Rename DataSourceConfigError to DataSourceError
  • Add helper createDataSourceError() to create data source error from:
    • Opensearch client error
    • legacy client error
    • saved object client error
    • other runtime errors
  • Replace usage of new DataSourceError(e) with createDataSourceError(e)
  • can't directly import createDataSourceError() to data plugin, have to import from data source plugin setup interface, otherwise, some other UTs in other modules fail
  • refactor to move data source error response logic from router.ts to resolve_index.ts. I check all our codebase, only the route handler defined resolve_index is missing error handling. All other internal/xxx/xxx API has err handling. E.g internal/search/, /internal/_msearch, /internal/index-pattern-manage/preview-script
    catch (err) {
            return res.customError({
              statusCode: err.statusCode,
              body: {
                message: err.message,
                attributes: {
                  error: err.body.error,
                },
              },
            });
    

Error statusCode casting logic

  • If it's response error, we maintain the error statusCode, except 401
  • for any 401 error, we cast to 400, because of [MD] multiple datasource doesn't throw 401 error as expected when security plugin enabled  #2591
  • if it's client non-response error(without statuscode), cast to 400, while keeping the original error message, such as timeout error, connection error, no living connection error, etc. Because by design, datasource could be created without validation of connectivity. Besides, ds could be updated, we won't be aware of the data source side status, also couldn't keep track of ds domain status~~

Issues Resolved

#2591

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
    • yarn test:ftr
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

@zhongnansu zhongnansu added the multiple datasource multiple datasource project label Oct 24, 2022
@zhongnansu zhongnansu requested a review from a team as a code owner October 24, 2022 22:08
Copy link
Member

@ashwin-pc ashwin-pc left a comment

Choose a reason for hiding this comment

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

We cast all search error against data source to 400 bad request, while maintaining the original error message from OS.

This worries me a lil because it is the root of a similar issue we face in other places in Dashboards where we incorrectly assign the wrong error code for a response. 4xx errors are auth errors.

  1. Is there really no way to assign the right error response code for an error?
  2. If not, why did you choose to use a 4xx error instead of a generic 5xx error?

@seraphjiang seraphjiang added backport 2.x enhancement New feature or request labels Oct 25, 2022
Signed-off-by: Su <szhongna@amazon.com>
Signed-off-by: Su <szhongna@amazon.com>
Signed-off-by: Su <szhongna@amazon.com>
@zhongnansu zhongnansu changed the title [MD] handle data source error for internal search api [MD] Refactor data source server side error handling Oct 25, 2022
@zhongnansu
Copy link
Member Author

zhongnansu commented Oct 25, 2022

We cast all search error against data source to 400 bad request, while maintaining the original error message from OS.

This worries me a lil because it is the root of a similar issue we face in other places in Dashboards where we incorrectly assign the wrong error code for a response. 4xx errors are auth errors.

  1. Is there really no way to assign the right error response code for an error?
  2. If not, why did you choose to use a 4xx error instead of a generic 5xx error?

@ashwin-pc , thanks for the comments, really helpful. I re-worked on this PR. basically doing a refactor of existing error handling logic in data source feature. Now it will keep original status code. But for 401, we choose to cast to 400, due to the limitation of security dashboards plugin. For 5xx error, we cast to 400. Because it doesn't make sense to use 5xx, e.g user's data source domain is down, while trying to connect to it, we get 5xx. But there's nothing wrong with local OSD. Security review also mentioned it's not good to return 5xx for such cases

Signed-off-by: Su <szhongna@amazon.com>
@ashwin-pc
Copy link
Member

@zhongnansu Thanks for that explanation! That logic makes sense to me.

Signed-off-by: Su <szhongna@amazon.com>
zengyan-amazon
zengyan-amazon previously approved these changes Oct 26, 2022
const isDataSourceConfigError = (error: any) => {
return error.constructor.name === 'DataSourceConfigError' && error.statusCode === 400;
const isDataSourceError = (error: any) => {
return error.constructor.name === 'DataSourceError' && error.statusCode === 400;
Copy link
Member

Choose a reason for hiding this comment

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

can we use instanceofinstead of error.constructor.name ?

Copy link
Member Author

Choose a reason for hiding this comment

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

importing DataSourceError from plugin module to here(core module) doesn't sound like a good practice. But anyway, I did some refactor to move data source error response logic from router.ts to resolve_index.ts(/internal/index-pattern-manage/resolve-index).

I checked the codebase, only the route handler defined resolve_index is missing error handling. All other internal/xxx/xxx API has err handling. E.g internal/search/, /internal/_msearch, /internal/index-pattern-manage/preview-script

```
catch (err) {
        return res.customError({
          statusCode: err.statusCode,
          body: {
            message: err.message,
            attributes: {
              error: err.body.error,
            },
          },
        });
```   

@zengyan-amazon could you review again

Signed-off-by: Su <szhongna@amazon.com>
Signed-off-by: Su <szhongna@amazon.com>
@zhongnansu zhongnansu added the v2.4.0 'Issues and PRs related to version v2.4.0' label Oct 26, 2022
@zhongnansu zhongnansu merged commit 9a0bb30 into opensearch-project:main Oct 27, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request Oct 27, 2022
- Rename DataSourceConfigError to DataSourceError
- Add helper createDataSourceError() to create data source error from, and replace usage of new DataSourceError(e) with createDataSourceError(e)
- import createDataSourceError()  from data source plugin setup interface,
refactor to move data source error response logic from router.ts to resolve_index.ts.

Signed-off-by: Su <szhongna@amazon.com>
(cherry picked from commit 9a0bb30)
zhongnansu added a commit that referenced this pull request Oct 27, 2022
- Rename DataSourceConfigError to DataSourceError
- Add helper createDataSourceError() to create data source error from, and replace usage of new DataSourceError(e) with createDataSourceError(e)
- import createDataSourceError()  from data source plugin setup interface,
refactor to move data source error response logic from router.ts to resolve_index.ts.

Signed-off-by: Su <szhongna@amazon.com>
(cherry picked from commit 9a0bb30)

Co-authored-by: Zhongnan Su <szhongna@amazon.com>
ajygupta pushed a commit to ajygupta/OpenSearch-Dashboards that referenced this pull request Nov 25, 2022
…ect#2661)

- Rename DataSourceConfigError to DataSourceError
- Add helper createDataSourceError() to create data source error from, and replace usage of new DataSourceError(e) with createDataSourceError(e)
- import createDataSourceError()  from data source plugin setup interface,
refactor to move data source error response logic from router.ts to resolve_index.ts.

Signed-off-by: Su <szhongna@amazon.com>
Signed-off-by: Ajay Gupta <ajyg@amazon.com>
sipopo pushed a commit to sipopo/OpenSearch-Dashboards that referenced this pull request Dec 16, 2022
…ect#2661)

- Rename DataSourceConfigError to DataSourceError
- Add helper createDataSourceError() to create data source error from, and replace usage of new DataSourceError(e) with createDataSourceError(e)
- import createDataSourceError()  from data source plugin setup interface,
refactor to move data source error response logic from router.ts to resolve_index.ts.

Signed-off-by: Su <szhongna@amazon.com>
Signed-off-by: Sergey V. Osipov <sipopo@yandex.ru>
Arpit-Bandejiya pushed a commit to Arpit-Bandejiya/OpenSearch-Dashboards that referenced this pull request Jan 13, 2023
…ect#2661)

- Rename DataSourceConfigError to DataSourceError
- Add helper createDataSourceError() to create data source error from, and replace usage of new DataSourceError(e) with createDataSourceError(e)
- import createDataSourceError()  from data source plugin setup interface,
refactor to move data source error response logic from router.ts to resolve_index.ts.

Signed-off-by: Su <szhongna@amazon.com>
Signed-off-by: Arpit Bandejiya <abandeji@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x enhancement New feature or request multiple datasource multiple datasource project v2.4.0 'Issues and PRs related to version v2.4.0'
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants