search

certtools / intelmq-api

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

API rewritten to FastAPI #39

Closed kamil-certat closed 1 year ago

kamil-certat commented 1 year ago

The FastAPI-based API is now an ASGI application, that isn't natively supported by Apache. Instead, a recommended FastAPI setup with web server Gunicorn is used. To keep backward compatibility, Apache is still used as a Proxy. Apache and Gunicorn communicate using unix socket.

The package installation creates intelmq-api service and socket for systemd, and configure the Apache2 to communicate with them. The INTELMQ_API_CONFIG env can now be set in the service unit file.

Provided configuration is an MVP and should be reviewed in the production environement.

Depends on #38

kamil-certat commented 1 year ago

The changes in compare to the #38 are visible here: https://github.com/kamil-certat/intelmq-api/pull/1

This PR is ready to review, but will be opened after merging the #38

aaronkaplan commented 1 year ago

@kamil-certat I reviewed the PR. Could you please have a look at my comments, then I will be very happy to merge it in and test. My main point - apart from the lots of little comments - is: did you test the old API vs. new API code? Like are there any diffs when given the same input?

input -> old api -> output1 input -> new api -> output2

diff output1 output2

?

kamil-certat commented 1 year ago

@aaronkaplan I don't see any your comments, have you published them? The diff question answered in #38

aaronkaplan commented 1 year ago

@aaronkaplan I don't see any your comments, have you published them? The diff question answered in #38

Hm, yeah... you should see all my comments in the PR on github. Ping me in case you don't

kamil-certat commented 1 year ago

I still don't see anything to address...

aaronkaplan commented 1 year ago

now?

kamil-certat commented 1 year ago

Now I see, will answer shortly :)

sebix commented 1 year ago

@aaronkaplan aaronkaplan requested a review from sebix

Sorry, but I can't manage to review the API rewrite until Friday. That would include tests on various platforms, verifying documentation and examples and updating the package build instructions for multiple operating systems.

kamil-certat commented 1 year ago

As I mentioned in some e-mail, there is trouble that for Debian < 11 packages for FastAPI don't exist. I think it would be a terrible exercise to prepare all of them in two days.

kamil-certat commented 1 year ago

@sebix Unfortunately not, and this is why I wanted to separate rewrite and packaging at the beginning. It was, in fact, the first approach to package it.

It works on Debian 11, on the Debian 10 - as mentioned - we have lack of dependencies. RedHat's platform need to be tested.

aaronkaplan commented 1 year ago

@aaronkaplan aaronkaplan requested a review from sebix

Sorry, but I can't manage to review the API rewrite until Friday. That would include tests on various platforms, verifying documentation and examples and updating the package build instructions for multiple operating systems.

okay, got it. @kamil-certat would it be OK to put that into 3.2 then?

kamil-certat commented 1 year ago

I'm okay with this or just releasing API 3.1 a little later this week. Let me do additional tests and packaging fixes, hopefully today or tomorrow. If I fail with proper packaging until tomorrow, I may need help here if we want to finish this week.

And any additional testing is welcome :)

kamil-certat commented 1 year ago

Anyway, I will not back-port dependencies for Debian 10 (or eventually CentOS, if needed) - will you eventually handle it during release packaging, @sebix?

sebix commented 1 year ago

Anyway, I will not back-port dependencies for Debian 10 (or eventually CentOS, if needed) - will you eventually handle it during release packaging, @sebix?

If there's no explicit contract or funding: No.

kamil-certat commented 1 year ago

Sure, I understand it. Then we have a releasing issue because it works out-of-the-box for Debian 11, but will fail for the earlier.

@aaronkaplan: this is a question to you, how you think it's the best to handle it. I see the following options: 1) we can wait until someone find time to back-port dependencies (or maybe someone has already done it somewhere); 2) let's release API 3.1 with native packages for Debian 11, and leave systems without already built FastAPI on the PyPI packages as newest and 3.0.1 as native package (should work so far); change it once we find time to back-port dependencies. Already done native support doesn't look good: https://pkgs.org/search/?q=fastapi 3) abandon the FastAPI and look for a framework that is already widely supported; flask looks promising and could be served as WSGI, so the same way as hug, by the Apache.

@sebix - is it possible to contribute to your building server?

sebix commented 1 year ago

I don't think backporting fastapi is useful. It has some dependencies which are not fulfilled in Debian 10, all of them need backports as well.

@sebix - is it possible to contribute to your building server?

It's not mine, but a publicly usable instance. Contact me privately for the details (username etc.)

kamil-certat commented 1 year ago

I don't think backporting fastapi is useful. It has some dependencies which are not fulfilled in Debian 10, all of them need backports as well.

Yeah, it would need the full back-porting, the same way as we do with hug know. I'm also not a fan of maintaining all package dependencies on our own. So, what would you suggest?

t's not mine, but a publicly usable instance. Contact me privately for the details (username etc.)

I know, I called it yours because the repo is under your namespace (sebix/intelmq) ;) I'll contact you once I find it useful to change something :)

kamil-certat commented 1 year ago

Okay, so after the mailing thread, my goals for packaging are:

1) native package for Debian 11 (all dependencies exists), 2) for other platforms, the pip package as recommended way.

To achieve 2) I'll update the documentation (PR https://github.com/certtools/intelmq/pull/2292) and try to prepare a script that should help with installing service units. In the future, it could be somehow integrated with intelmqsetup (but not at the moment, to not introduce additional complexity).

aaronkaplan commented 1 year ago

As I mentioned in some e-mail, there is trouble that for Debian < 11 packages for FastAPI don't exist. I think it would be a terrible exercise to prepare all of them in two days.

Yes, okay. Makes sense. So, what I propose is that we add a sentence to the documentation (if not already done) that only versions x,y,z are supported (Debian 11, Ubuntu 22, etc.) and what steps users of older OSes or other distros can do to get it running. OK?

kamil-certat commented 1 year ago

Exactly

kamil-certat commented 1 year ago

Please take a look now. I'll follow up tomorrow with final testing, today I didn't manage to finally test CentOS (some VirtualBox issues).

kamil-certat commented 1 year ago

Could you please elaborate, what do you mean by wrong place? Files you mentioned are in /etc/intelmq because of the setup.py configuration. The /etc/intelmq/api-sudoers.conf was always there. Do you mean, that we shouldn't include them in the python package, or in another dir? The Debian config placed them in the right dirs separately:

https://github.com/kamil-certat/intelmq-api/blob/727c3563cb79e72e770213ed739864e7ac683146/debian/install#L1-L5

kamil-certat commented 1 year ago

re: tests. They were never running ;) https://build.opensuse.org/package/live_build_log/home:sebix:intelmq/intelmq-api/Debian_10/x86_64

We could make them working, but it would require downloading the IntelMQ package as Build-Depends in the build server. Do you know how to configure OBS to get package from another project?

sebix commented 1 year ago

Do you know how to configure OBS to get package from another project?

it will pick the intelmq package from the same repo (i.e. unstable). Why do you need another one? OBS uses the packages in the same project and then the repositories as defined per distributions (with preference on the local packages). I don't see a reason why you need to specify the project for your purpose.

re: tests. They were never running ;) https://build.opensuse.org/package/live_build_log/home:sebix:intelmq/intelmq-api/Debian_10/x86_64

now we have better tests (previously only the session store was tested) and it makes more sense now

sebix commented 1 year ago

The /etc/intelmq/api-sudoers.conf was always there. Do you mean, that we shouldn't include them in the python package, or in another dir? The Debian config placed them in the right dirs separately:

https://github.com/kamil-certat/intelmq-api/blob/727c3563cb79e72e770213ed739864e7ac683146/debian/install#L1-L5

The Debian package does place them correctly but does not remove the files placed incorrectly by setuptools. To cut a long story short, I recommend (as the simplest and explicit solution) removing the wrongly placed files in the debian/rules file, section override_dh_auto_install:

rm debian/intelmq-api/etc/....
kamil-certat commented 1 year ago

it will pick the intelmq package from the same repo (i.e. unstable). Why do you need another one? OBS uses the packages in the same project and then the repositories as defined per distributions (with preference on the local packages). I don't see a reason why you need to specify the project for your purpose.

Okay, I'll try it :)

kamil-certat commented 1 year ago

I fixed what you asked for @sebix

However, building for Ubuntu 22.04 is broken due to the upstram bug: https://bugs.launchpad.net/ubuntu/+source/fastapi/+bug/1970557 Basically, the Ubuntu FastAPI and Starlette packages are out-of-sync in this version, and FastAPI is unable to work.

sebix commented 1 year ago

I fixed what you asked for @sebix

Thanks, let's merge then!

However, building for Ubuntu 22.04 is broken due to the upstram bug: https://bugs.launchpad.net/ubuntu/+source/fastapi/+bug/1970557 Basically, the Ubuntu FastAPI and Starlette packages are out-of-sync in this version, and FastAPI is unable to work.

https://bugs.launchpad.net/ubuntu/+source/fastapi/+bug/1970557/comments/6 suggests that backporting 0.74.1-1 from Ubuntu 22.10 to Ubuntu 22.04 would work without requiring any updates of depending packages. Would this be a viable solution?