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

[Rules] [Meta] Road to versioned HTTP APIs #157883

Closed
38 of 54 tasks
XavierM opened this issue May 16, 2023 · 3 comments · Fixed by #158786
Closed
38 of 54 tasks

[Rules] [Meta] Road to versioned HTTP APIs #157883

XavierM opened this issue May 16, 2023 · 3 comments · Fixed by #158786
Assignees
Labels
Feature:Alerting/RulesManagement Issues related to the Rules Management UX Meta Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams)

Comments

@XavierM
Copy link
Contributor

XavierM commented May 16, 2023

To not introduce any breaking changes or changes in the behavior of Rules and Connectors APIs when upgrading Kibana, we need to do changes in our endpoints. The needed steps to achieve this goal are described in the following section.

Steps

Disconnect API Interface with SO Attributes

The goal of this step is to isolate the SO attributes from the HTTP APIs. Any change in our SO attributes should not reflect a change in our API interfaces. To do that we need to

  • Create types for domain objects
  • Create TS interfaces/types for SOs and HTTP routes even if they are the same. Types for SOs should be in the server directory
  • Explicitly map the SO attributes to the response and vice-versa even if they are the same. No more spreading

APIs:

Rules:

Rule Settings:

  • GET /internal/alerting/rules/settings/_flapping -> @Zacqary
  • POST /internal/alerting/rules/settings/_flapping -> @Zacqary

Maintenance Windows:

@XavierM XavierM added Meta Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Feature:Alerting/RulesManagement Issues related to the Rules Management UX v8.9.0 labels May 16, 2023
@elasticmachine
Copy link
Contributor

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

@JiaweiWu
Copy link
Contributor

JiaweiWu commented Jun 7, 2023

PoC PR: #158786

JiaweiWu added a commit that referenced this issue Jul 5, 2023
## Summary
Resolves: #157883

# Summary:
This PR acts as a POC for how we would want to version our rule API
types in preparation for the versioning of our HTTP endpoints.

There is now an ES persisted type (called `RuleAttributes`, akin to the
old `RawRule` type). This type should never be used at the business
logic level as we want the ability not to reference saved object
attributes directly in the application. Instead, we should transform
this saved-object to its corresponding domain object type.

HTTP types (`CreateBodyParams`, `RuleResponse`) are now located in
`x-pack/plugins/alerting/common/routes/rule` with a versioning structure
like:


![image](https://github.com/elastic/kibana/assets/74562234/1c8e886d-8983-40f2-9490-aa3864898535)

And follows the guideline here:
https://docs.elastic.dev/kibana-dev-docs/versioning-interfaces

Domain object types (rule for example) are derived from the
`config-schema` schemas using `TypeOf`, this was done to facilitate the
reuse of validation schemas that we might want to run for strict IO
validation, potentially at the `rulesClient` level.

## API:
Only the `createRule` endpoint has been converted for the sake of this
POC, other endpoints might have broken types that will be fixed once we
have a finalized pattern for dealing with versioned types.

At the API route level, I think it would be wise to import versioned
types in our APIs, this way, it forces the developer to deal with broken
types when we have a breaking change to our rule domain object.

The API route level is also responsible for transforming domain objects
to response types, usually, this just entails renaming property names
from camel case to snake case.

in `create_rule_route.ts`

```ts
import type { CreateRuleRequestBodyV1, CreateRuleRequestParamsV1 } from '../../../../common/routes/rule/create';
import type { RuleParamsV1 } from '../../../../common/routes/rule/rule_response';

...

// In the Handler:
const rulesClient = (await context.alerting).getRulesClient();

const createRuleData: CreateRuleRequestBodyV1<RuleParamsV1> = req.body;
const params: CreateRuleRequestParamsV1 = req.params;

const createdRule: Rule<RuleParamsV1> = (await rulesClient.create<RuleParamsV1>({
  data: transformCreateBodyV1<RuleParamsV1>(createRuleData),
  options: { id: params?.id },
})) as Rule<RuleParamsV1>;

const response: CreateRuleResponseV1<RuleParamsV1> = {
  body: transformRuleToRuleResponseV1<RuleParamsV1>(createdRule),
};

return res.ok(response);
```

### RulesClient -> Create
At the rules client level, we now need to transform saved-objects to the
domain objects after we perform CRUD operations on them. We can also
consider validating schemas here too since other solutions are using our
rules client directly instead of through the API.

I don't think we need to version rules client input and output types
since the route level types should deal with breaking changes already.
Therefore, we can freely import the latest of any domain object types
(Rule, for example)

```ts
import { Rule, RuleDomain, RuleParams } from '../types';
```

The flow is the following:
```
1. calling rulesClient.create() with API body and params.
2. perform schema validation on params
3. perform other validation not achievable using config-schema
4. perform rule create -> returns a `savedObject` type
5. convert `savedObject` ES type to the domain object type (`RuleDomain`)
6. convert the domain object type to a public domain object type (`Rule`)
7. We can also validate the created rule using config-schema
```

# Open questions:
- Should we validate input params at both the route and the rules client
level? I think we should since our rules client is shared.

- How do we want to version and validate rule params? Right now they're
typed as generics.

- Should we leave all date fields as `string`, even for domain objects?
Since config-schema is unable to define a `Date` type.

- Should we support partial rule domain object types at all? I can't
really think why we should even for `updates`, since we need to
reconstruct the overall rule to send back to the client.

- Should we have `undefined | null` types in the domain object? I know
we need `null` specifically for the ES types since there is a different
between updating a field as `undefined` or `null`, but this shouldn't
manifest itself in the business logic.

### Checklist
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@XavierM XavierM reopened this Jul 6, 2023
JiaweiWu added a commit that referenced this issue Jul 25, 2023
## Summary

Resolves: #161395
Parent Issue: #157883

Adds versioned types to the rule `bulk_edit` endpoint.

This PR also moves around the folder structure slightly, by adding a sub
folder for the `data`/`route`/`application` methods:

## Before

![image](https://github.com/elastic/kibana/assets/74562234/3ae19871-cc5c-4180-b099-5fed86c3870b)

## After

![image](https://github.com/elastic/kibana/assets/74562234/bbd67bb4-4899-4f65-966e-64964f994a04)

Notice I added a `methods` folder to contain the methods, I did the same
for the `data` and `route` folders as well. I think this improves the
hierarchy of these modules, If folks are ok with it then I will update
the doc with the new folder structure.

### Checklist
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
ThomThomson pushed a commit to ThomThomson/kibana that referenced this issue Aug 1, 2023
## Summary

Resolves: elastic#161395
Parent Issue: elastic#157883

Adds versioned types to the rule `bulk_edit` endpoint.

This PR also moves around the folder structure slightly, by adding a sub
folder for the `data`/`route`/`application` methods:

## Before

![image](https://github.com/elastic/kibana/assets/74562234/3ae19871-cc5c-4180-b099-5fed86c3870b)

## After

![image](https://github.com/elastic/kibana/assets/74562234/bbd67bb4-4899-4f65-966e-64964f994a04)

Notice I added a `methods` folder to contain the methods, I did the same
for the `data` and `route` folders as well. I think this improves the
hierarchy of these modules, If folks are ok with it then I will update
the doc with the new folder structure.

### Checklist
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
JiaweiWu added a commit that referenced this issue Apr 15, 2024
## Summary

Issue: #179476
Parent issue: #157883

This PR versions the update (`PUT '/api/alerting/rule/{id}'`) route.
Still using `config-schema` for now, even though we will eventually
switch to `zod` when core is ready with openapi doc generation support
in the versioned router.

We are now validating update data using the update data schema when
calling `rulesClient->update`. We are also validating (but not throwing)
the updated rule as well.

### Checklist
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
JiaweiWu added a commit that referenced this issue Apr 30, 2024
## Summary

Parent Issue: #157883
Issue: #181263

Versions the GET rule endpoint with added input and output validation.

```
GET /internal/alerting/rule/{id}
GET /api/alerting/rule/{id}
```
### Checklist

Delete any items that are not applicable to this PR.
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
JiaweiWu added a commit that referenced this issue Apr 30, 2024
## Summary
Parent Issue: #157883
Issue: #179669

Versions the `PATCH /internal/alerting/rules/_bulk_enable` endpoint with
added input and output validation.

### Checklist
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
JiaweiWu added a commit that referenced this issue Apr 30, 2024
## Summary

Issue: #180079
Parent Issue: #157883

Versions the clone rule API and adds input/output validation

### Checklist
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
JiaweiWu added a commit that referenced this issue Apr 30, 2024
…180654)

## Summary

Issue: #180551
Parent Issue: #157883

Versions the internal and public find API routes. Adds Input validation
as well.

### Checklist
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
JiaweiWu added a commit that referenced this issue May 13, 2024
## Summary

Parent Issue: #157883
Issue: #181513

Versions the DELETE rule endpoint with added input validation.

`DELETE /api/alerting/rule/{id}`

### Checklist
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
@cnasikas cnasikas changed the title [RAM] [Meta] Road to versioned HTTP APIs [Rules] [Meta] Road to versioned HTTP APIs Jul 2, 2024
@jcger
Copy link
Contributor

jcger commented Oct 17, 2024

continued in #187356

@jcger jcger closed this as completed Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Alerting/RulesManagement Issues related to the Rules Management UX Meta Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams)
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

7 participants