search

certtools / intelmq-api

FastAPI-based API for the IntelMQ project
https://docs.intelmq.org/latest/user/api/
1 stars 7 forks source link

Rewriting to the FastAPI #38

Closed kamil-certat closed 1 year ago

kamil-certat commented 1 year ago

The API was rewritten to use the currently alive FastAPI framework. The file structure was changed to better align with the new framework. The readme was updated and contains development tips. Code style checks were added to the CI.

Known issue: the packaging was left broken for now. It requires some more care to support the FastAPI, and I wanted to separate those two tasks.

The API structure should have been left intact.

aaronkaplan commented 1 year ago

So question about that: did you do a before and after test? Like as in : input --> old API --> output1 input --> new API --> output2

diff output1 output2

?

kamil-certat commented 1 year ago

It was tested like this, however manually without recording and exactly diffing answers. What I did: 1) export schemas from hug and basing on this create the FastAPI (unfortunately, hug schema was in non-standard format); 2) code initial tests to ensure returning answers from CLI and for other, not-wrapping features; 3) manually go through IntelMQ manager and compare behavior with hug and FastAPI; test and fix API (here I discovered some issues with paths, content type etc.); 4) make a VM with clean installation with hug, ensure it works, then replace hug with new API; 5) make a VM with clean installation with just new API.

So far I remember, I also did a second round of testing with IntelMQ Manager - but I'm not sure. Please don't just belive in my words, but let's test it ;)

aaronkaplan commented 1 year ago

(...) So far I remember, I also did a second round of testing with IntelMQ Manager - but I'm not sure. Please don't just belive in my words, but let's test it ;)

okay, got it. So, then we need to test it in detail (which is good anyway). Thanks a lot for the explanations.

kamil-certat commented 1 year ago

I wanted anyway that someone else test it as well - I might have missed something eventually obvious or important, either in rewriting or packaging.

aaronkaplan commented 1 year ago

Makes sense.

sebix commented 1 year ago

So question about that: did you do a before and after test? Like as in : input --> old API --> output1 input --> new API --> output2

diff output1 output2

?

The intelmq-vagrant tests do cover various API calls. @kamil-certat did you run the vagrant/ansible tests with the new API?

kamil-certat commented 1 year ago

@sebix Thanks for mentioning them, I wasn't aware they exist. I have just run them (it took longer than I expected, partially because meanwhile the unstable repository changed intelmq and apt started to complain ;)).

Results: 1) 00_registerauth -> with FastAPI failed due to redirection to the URL without ending /, fixed by allowing following redirects in Ansible; 2) 01_checkauth -> with FastAPI filed because for no auth header the 403 was returned instead of 401 (and another message). This is a rather unexpected change, so I'll fix it shortly.

All functional checks passed.

BTW, looks like debconf in Ansible lacks one or two configurations required to install API (need to confirm, but it seems like installing sqlite3 using debconf was added without changes in Ansible). I'll follow with PRs shortly.

aaronkaplan commented 1 year ago

Thanks @kamil-certat , let me know when you are ready and I'll review as well. And then we merge it in for 3.1

kamil-certat commented 1 year ago

Closing in favor of review #39. Originally, I thought about merging rewrite and packaging separately, but it looks like now it would be better to do it alltogether.