Skip to content

[ResponseOps][Rules] Add OAS schema for handled 4xx errors on rule apis#192616

Merged
Zacqary merged 22 commits intoelastic:mainfrom
Zacqary:188514-rules-4xx-oas
Sep 19, 2024
Merged

[ResponseOps][Rules] Add OAS schema for handled 4xx errors on rule apis#192616
Zacqary merged 22 commits intoelastic:mainfrom
Zacqary:188514-rules-4xx-oas

Conversation

@Zacqary
Copy link
Copy Markdown
Contributor

@Zacqary Zacqary commented Sep 11, 2024

Summary

Closes #188514

Adds OAS schemas for the 403 Forbidden errors that public rule apis can return if a license is invalid, 400 Bad Request for unregistered rule types, and 404 Not Found for missing saved objects.

Checklist

Testing

  1. Start ES
  2. Add server.oas.enabled: true to kibana.dev.yml
  3. Start Kibana yarn start --no-base-path
  4. curl -s -uelastic:changeme http://localhost:5601/api/oas\?pathStartsWith\=/api/alerting/rule/ | jq (If you have jq installed, otherwise pipe to pbcopy and paste the result into a JSON prettifier)
  5. Search the output for the word Forbidden to ensure this schema has been added to create, update, enable, disable, mute, unmute, and update_rule_api_key

@Zacqary Zacqary added release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Platform ResponseOps team (formerly the Cases and Alerting teams) t// Feature:Alerting/RulesFramework Issues related to the Alerting Rules Framework v8.16.0 labels Sep 11, 2024
@Zacqary Zacqary requested a review from a team as a code owner September 11, 2024 17:37
@elasticmachine
Copy link
Copy Markdown
Contributor

Pinging @elastic/response-ops (Team:ResponseOps)

@obltmachine
Copy link
Copy Markdown

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • run docs-build : Re-trigger the docs validation. (use unformatted text in the comment!)

…t --include-path /api/status --include-path /api/alerting/rule/ --include-path /api/alerting/rules --include-path /api/actions --update'
@cnasikas cnasikas self-requested a review September 12, 2024 09:22

import { schema } from '@kbn/config-schema';

export const forbiddenErrorSchema = schema.object({
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not saying to do it but wdyt about having this schema in x-pack/plugins/alerting/common/routes/rule/response/schemas/v1.ts instead?

Or maybe we can move the request and response folders(in x-pack/plugins/alerting/common/routes/rule/) here. We prob should consolidate these, they feel similar.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What do you think of x-pack/plugins/alerting/common/routes/rule/errors/schema/v1.ts?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ah, just noticed I wrote it wrong.

I meant to say

Not saying to do it but wdyt about having this schema in x-pack/plugins/alerting/common/routes/rule/ instead?

yeah an errors folder sounds good!

@adcoelho
Copy link
Copy Markdown
Contributor

No other cases of 4xx or 5xx responses appear to be handled, so this is all I've added to OAS.

What about 400s? I see a bunch of instances of Boom.badRequest. This for example.

Copy link
Copy Markdown
Contributor

@adcoelho adcoelho left a comment

Choose a reason for hiding this comment

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

Some routes appear to be missing:

  • x-pack/plugins/alerting/server/routes/rule/apis/get/get_rule_route.ts
  • x-pack/plugins/alerting/server/routes/rule/apis/find/find_rules_route.ts
  • x-pack/plugins/alerting/server/routes/rule/apis/delete/delete_rule_route.ts

Also, I think we should also document 404s and 500s.

  • 500s because errors in the code are always possible 😅
  • 404s in case of trying to access missing SOs

@Zacqary
Copy link
Copy Markdown
Contributor Author

Zacqary commented Sep 13, 2024

500s because errors in the code are always possible 😅

I can add this, but 500 Internal Server Error means 500 Internal Server Error. That's just HTTP standard. Do we really want to repeat that documentation?

@Zacqary
Copy link
Copy Markdown
Contributor Author

Zacqary commented Sep 13, 2024

x-pack/plugins/alerting/server/routes/rule/apis/find/find_rules_route.ts

This is internal

Zacqary and others added 4 commits September 13, 2024 14:36
@Zacqary Zacqary changed the title [ResponseOps][Rules] Add OAS schema for handled 403 errors on rule apis [ResponseOps][Rules] Add OAS schema for handled 4xx errors on rule apis Sep 13, 2024
@Zacqary
Copy link
Copy Markdown
Contributor Author

Zacqary commented Sep 17, 2024

@cnasikas from the issue:

We discussed it offline with the Core team and the ResponseOps team and we decided to not include the 5xx status codes in the OAS. The reasons are:

  • Unless it means something specific in the context of the API it is not worth creating a specific entry for. It is pretty generic and could be something the core defines for all routes.

  • APIs should not return any 5xx codes. They should not be part of the API itself.

  • The main reason for documenting the 5xx errors in the OAS was for parts of the code that do if (whatever) { throw new Error("my error") } where the route catches it and defaults it to 500. Still, it is better to provide better errors than to document the 500.

@Zacqary Zacqary requested a review from adcoelho September 17, 2024 14:48
@Zacqary
Copy link
Copy Markdown
Contributor Author

Zacqary commented Sep 17, 2024

@elasticmachine merge upstream

@lcawl
Copy link
Copy Markdown
Member

lcawl commented Sep 17, 2024

The API docs output LGTM, thanks!

@Zacqary Zacqary enabled auto-merge (squash) September 17, 2024 19:00
@cnasikas
Copy link
Copy Markdown
Member

@elasticmachine merge upstream

@elastic-vault-github-plugin-prod elastic-vault-github-plugin-prod bot requested a review from a team as a code owner September 19, 2024 12:43
@Zacqary Zacqary added the backport:skip This PR does not require backporting label Sep 19, 2024
@Zacqary Zacqary enabled auto-merge (squash) September 19, 2024 15:08
@Zacqary Zacqary disabled auto-merge September 19, 2024 15:09
@Zacqary Zacqary added backport:version Backport to applied version labels and removed backport:skip This PR does not require backporting labels Sep 19, 2024
@Zacqary Zacqary enabled auto-merge (squash) September 19, 2024 15:09
@Zacqary Zacqary requested a review from a team as a code owner September 19, 2024 15:11
@Zacqary Zacqary requested a review from nikitaindik September 19, 2024 15:11
@Zacqary Zacqary requested a review from a team as a code owner September 19, 2024 15:13
@kibana-ci
Copy link
Copy Markdown

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@Zacqary Zacqary merged commit 18afcae into elastic:main Sep 19, 2024
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Sep 19, 2024
…is (elastic#192616)

## Summary

Closes elastic#188514

Adds OAS schemas for the `403 Forbidden` errors that public rule apis
can return if a license is invalid, `400 Bad Request` for unregistered
rule types, and `404 Not Found` for missing saved objects.

### Checklist

- [x] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [x]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials

### Testing

1. Start ES
2. Add `server.oas.enabled: true` to `kibana.dev.yml`
3. Start Kibana `yarn start --no-base-path`
4. `curl -s -uelastic:changeme
http://localhost:5601/api/oas\?pathStartsWith\=/api/alerting/rule/ | jq`
(If you have `jq` installed, otherwise pipe to `pbcopy` and paste the
result into a JSON prettifier)
5. Search the output for the word `Forbidden` to ensure this schema has
been added to `create`, `update`, `enable`, `disable`, `mute`, `unmute`,
and `update_rule_api_key`

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
(cherry picked from commit 18afcae)
@kibanamachine
Copy link
Copy Markdown
Contributor

💚 All backports created successfully

Status Branch Result
8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Sep 19, 2024
…ule apis (#192616) (#193454)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[ResponseOps][Rules] Add OAS schema for handled 4xx errors on rule
apis (#192616)](#192616)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Zacqary Adam
Xeper","email":"Zacqary@users.noreply.github.com"},"sourceCommit":{"committedDate":"2024-09-19T16:52:17Z","message":"[ResponseOps][Rules]
Add OAS schema for handled 4xx errors on rule apis (#192616)\n\n##
Summary\r\n\r\nCloses #188514 \r\n\r\nAdds OAS schemas for the `403
Forbidden` errors that public rule apis\r\ncan return if a license is
invalid, `400 Bad Request` for unregistered\r\nrule types, and `404 Not
Found` for missing saved objects.\r\n\r\n### Checklist\r\n\r\n- [x] Any
text added follows [EUI's
writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing),
uses\r\nsentence case text and includes
[i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n-
[x]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas
added for features that require explanation or tutorials\r\n\r\n###
Testing\r\n\r\n1. Start ES\r\n2. Add `server.oas.enabled: true` to
`kibana.dev.yml`\r\n3. Start Kibana `yarn start --no-base-path`\r\n4.
`curl -s
-uelastic:changeme\r\nhttp://localhost:5601/api/oas\\?pathStartsWith\\=/api/alerting/rule/
| jq`\r\n(If you have `jq` installed, otherwise pipe to `pbcopy` and
paste the\r\nresult into a JSON prettifier)\r\n5. Search the output for
the word `Forbidden` to ensure this schema has\r\nbeen added to
`create`, `update`, `enable`, `disable`, `mute`, `unmute`,\r\nand
`update_rule_api_key`\r\n\r\n---------\r\n\r\nCo-authored-by:
kibanamachine
<42973632+kibanamachine@users.noreply.github.com>\r\nCo-authored-by:
Elastic Machine
<elasticmachine@users.noreply.github.com>","sha":"18afcae609c9dd142ef158f6f19dd392bc9d6327","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Team:ResponseOps","v9.0.0","Feature:Alerting/RulesFramework","v8.16.0","backport:version"],"title":"[ResponseOps][Rules]
Add OAS schema for handled 4xx errors on rule
apis","number":192616,"url":"https://github.com/elastic/kibana/pull/192616","mergeCommit":{"message":"[ResponseOps][Rules]
Add OAS schema for handled 4xx errors on rule apis (#192616)\n\n##
Summary\r\n\r\nCloses #188514 \r\n\r\nAdds OAS schemas for the `403
Forbidden` errors that public rule apis\r\ncan return if a license is
invalid, `400 Bad Request` for unregistered\r\nrule types, and `404 Not
Found` for missing saved objects.\r\n\r\n### Checklist\r\n\r\n- [x] Any
text added follows [EUI's
writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing),
uses\r\nsentence case text and includes
[i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n-
[x]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas
added for features that require explanation or tutorials\r\n\r\n###
Testing\r\n\r\n1. Start ES\r\n2. Add `server.oas.enabled: true` to
`kibana.dev.yml`\r\n3. Start Kibana `yarn start --no-base-path`\r\n4.
`curl -s
-uelastic:changeme\r\nhttp://localhost:5601/api/oas\\?pathStartsWith\\=/api/alerting/rule/
| jq`\r\n(If you have `jq` installed, otherwise pipe to `pbcopy` and
paste the\r\nresult into a JSON prettifier)\r\n5. Search the output for
the word `Forbidden` to ensure this schema has\r\nbeen added to
`create`, `update`, `enable`, `disable`, `mute`, `unmute`,\r\nand
`update_rule_api_key`\r\n\r\n---------\r\n\r\nCo-authored-by:
kibanamachine
<42973632+kibanamachine@users.noreply.github.com>\r\nCo-authored-by:
Elastic Machine
<elasticmachine@users.noreply.github.com>","sha":"18afcae609c9dd142ef158f6f19dd392bc9d6327"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/192616","number":192616,"mergeCommit":{"message":"[ResponseOps][Rules]
Add OAS schema for handled 4xx errors on rule apis (#192616)\n\n##
Summary\r\n\r\nCloses #188514 \r\n\r\nAdds OAS schemas for the `403
Forbidden` errors that public rule apis\r\ncan return if a license is
invalid, `400 Bad Request` for unregistered\r\nrule types, and `404 Not
Found` for missing saved objects.\r\n\r\n### Checklist\r\n\r\n- [x] Any
text added follows [EUI's
writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing),
uses\r\nsentence case text and includes
[i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n-
[x]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas
added for features that require explanation or tutorials\r\n\r\n###
Testing\r\n\r\n1. Start ES\r\n2. Add `server.oas.enabled: true` to
`kibana.dev.yml`\r\n3. Start Kibana `yarn start --no-base-path`\r\n4.
`curl -s
-uelastic:changeme\r\nhttp://localhost:5601/api/oas\\?pathStartsWith\\=/api/alerting/rule/
| jq`\r\n(If you have `jq` installed, otherwise pipe to `pbcopy` and
paste the\r\nresult into a JSON prettifier)\r\n5. Search the output for
the word `Forbidden` to ensure this schema has\r\nbeen added to
`create`, `update`, `enable`, `disable`, `mute`, `unmute`,\r\nand
`update_rule_api_key`\r\n\r\n---------\r\n\r\nCo-authored-by:
kibanamachine
<42973632+kibanamachine@users.noreply.github.com>\r\nCo-authored-by:
Elastic Machine
<elasticmachine@users.noreply.github.com>","sha":"18afcae609c9dd142ef158f6f19dd392bc9d6327"}},{"branch":"8.x","label":"v8.16.0","branchLabelMappingKey":"^v8.16.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Zacqary Adam Xeper <Zacqary@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:version Backport to applied version labels Feature:Alerting/RulesFramework Issues related to the Alerting Rules Framework release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Platform ResponseOps team (formerly the Cases and Alerting teams) t// v8.16.0 v9.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[ResponseOps][Rules] Add 4xx and 5xx responses to OAS

8 participants