intelmq icon indicating copy to clipboard operation
intelmq copied to clipboard
verified publisher icon certtools

WIP: Merging of API and Manager to IntelMQ project

Open gethvi opened this issue 2 years ago • 3 comments

UPDATED.

Work in progress.

This PR merges API and Manager to the main IntelMQ repository.

Pros:

  • Users only need to install one piece of software.
  • No need to juggle with versions of three components.
  • We only need to make sure that the code works together within each commit.
  • Less packaging.
  • Easier implementation when pieces of code are aware of each other and you can include/import them.
  • Users can disable API and/or Manager easily.
  • One unified configuration file.

Cons:

  • The additional code and requirements take few more kB of disk space.

Updated project structure:

intelmq
├── app
│   ├── api       # <-- updated sources of intelmq-api
│   └── webgui    # <-- updated sources of intelmq-manager
├── bin
├── bots
├── etc
├── lib
└── tests
    └── app
        └── api   # <-- updated tests of intelmq-api
  1. This PR adds intelmq.yaml configuration file with server block and adds new options, example:
server:
  host: 127.0.0.1
  port: 8080
  workers: 3
  debug: false
  enable_webgui: true
  session_store: /var/lib/intelmq/server/sessions.sqlite
  access_log: /var/log/intelmq/access.log
  allowed_path: /var/lib/intelmq/bots/
  intelmq_ctl_cmd:
    - /usr/bin/intelmqctl
  1. This PR changes of Manager (webgui):
  • Mako templates changed to jinja templates.
  • Serves the website using FastAPI (used for the API as well).
  • All links are dynamically generated.
  • Links changed from /management.html to nicer /management etc.
  • Removes rendering of login form from HTML site when session_store is disabled.
  • Removes problem with double slash URL.
  1. This PR integrates intelmq-add-user into one consistent CLI:
intelmq
intelmq server start --debug --port 8080 --host 127.0.0.1
intelmq server adduser --username intelmq --password secret

It is possible to run either:

  • production ready gunicorn server with intelmq server start
  • development uvicorn server with intelmq server start --debug.
  1. Cleans up and updates intelmqsetup from code no longer needed.
  1. Changes positions.conf file path to /var/lib/intelmq/server/positions.json. This file is hardly ever edited manually by the user, no need to put it in etc.

gethvi avatar Nov 22 '23 12:11 gethvi

Thank you for the review. I understand the concerns, it's just easier to push more changes at once instead spending time to make things work in one way just to open another PR to make them work another way.

So at least the first three steps (merging of API, Manager and unify the configuration) seems like logical steps to put together. The rest just snowballed from there.

You have introduced the intelmq [serve/setup] command. I support it, but it's inconsistent with intelmqctl - we should discuss, if we need both (my first idea: deprecate intelmqctl in favour of intelmq - but even if so, please do not migrate and introduce new stuff at once ;))

It might be inconsistent with intelmqctl, but it's more consistent than a bunch of separate script callings such as:

  • intelmq-api-adduser
  • intelmqsetup (which I included just because the merging of projects allowed to remove some of it's now unnecessary code, so really it's just moved somewhere else and it's backwards compatible, you can still call intelmqsetup)
  • hug -m intelmq_api.serve (old I know but still)
  • /usr/bin/gunicorn intelmq_api.main:app --workers 4 --worker-class uvicorn.workers.UvicornWorker --bind unix:intelmq_api.sock.

My current solution was to just allow user easier and consistent interface to all these individual scripts. It doesn't intersect with intelmqctl functionality. The server options should be in my opinion included in the intelmq config (and my plan was to add more later such as workers count). User shouldn't need to know any python, any uvicorn, gunicorn or any other underlaying technology. All the user needs to do in my book is to edit the config file and run a command like intelmq server start which I introduced. Make it simple.

Code responsible for handling intelmq serve should be in my opinion moved to web/ and just imported in the main file.

  • side note: if we touch commands so big, we may consider moving to Click in the future (it should be also available as DEB package as well)

Not sure I totally agree with moving the server.py into web. Web module deals only with Manager code and the user has the option to not even enable it by enable_webgui: false. The server.py only imports the APIRouter object from web.

On the other hand I do agree that Click might be better option.

Do I see correctly, that you removed Gunicorn and systemd units at all? Units are for DEB packages only, but using only uvicorn.run isn't a production-grade solution: https://www.uvicorn.org/deployment/

Thank you, I just simply forgot about the unit files and Gunicorn. I will fix this.

I support unification of configuration file, but I'm against using global in the runtime config. The technical issue here is that this global is then propagated to every bot (so basically, if any bot has a server config field, we just override it silently. I would suggest introducing a separated config.yaml for IntelMQ configuration, and leave runtime.yaml for workflow-only config. This would also be a good move if we eventually introduce supoort for multiple workflows.

I agree. I actually tried to put it on the same level as global, but it broke a lot of things, because other parts of the code thought server is just another bot. :smile: So I went with the easier way (and less changes).

I also agree with separation of intelmq config and workflow config(s).

gethvi avatar Dec 19 '23 16:12 gethvi

Quick reply: ;)

I understand the concerns, it's just easier to push more changes at once instead spending time to make things work in one way just to open another PR to make them work another way.

I fully agree if it requires making things in another way. If the change small, then I think we should separate it (e.g. - API can work without manager, so we can first clean up the API, and then add manager on top of it, without changing any code, just adding new).

My trouble is, that I need to put reviewing IntelMQ between other things, and it's hard to do so with a big PR ;)

Not sure I totally agree with moving the server.py into web. Web module deals only with Manager code [...]

Then I probably missed that web is for Manager only - I'd like to just move the server from the __main__.py, my concern here is that we have a history of big scripts doing many things, so I'd like to logically separate subcommands.

It might be inconsistent with intelmqctl, but it's more consistent than a bunch of separate script callings such as:

I agree, and my goal was to start discussion about moving all into intelmq xxx, and deprecate intelmqctl as well.

All the user needs to do in my book is to edit the config file and run a command like intelmq server start which I introduced. Make it simple.

I definitely agree, and I like it. I just belive it should be a wrapper around qunicorn (but maybe something has changed, and Uvicorn is producion-ready now?). And uvicorn, the same as hug, should never be visible to user - they are development-only servers.

kamil-certat avatar Dec 19 '23 16:12 kamil-certat

  1. Confusing names

You have introduced the names "app" and "server" into the code, which I think can be confusing. It looks like a very core part to me (especially with the 'intelmq server start' command - it looks more like starting the botnet to me), but in our case the "very core" are bots, and I'd like to avoid that. Also, the "app" is in one place, then "serve" in others, and they don't map to each other - which makes it harder to find the right part of the code if you don't have the inside knowledge. I spent a long time thinking about how to solve that. What do you think of the name "web"? We can use it instead of "app" and "server" everywhere: in directory structures and commands, and it would fit better and say better what this piece of code is about.

I see. The logic was:

  • app is an instance of FastAPI Application and it consists of two separate parts (FastAPI routers really): api and webgui where loading the webgui (old Manager) is optional.
  • server was ment to mean the Uvicorn/Gunicorn instance of whichever one runs the app

But I can see that the code might not fully reflect this logic right now.

  1. Renaming the IntelMQ Manager

I see you have introduced the term "WebUI" into the code instead of IntelMQ Manager. It's functionally correct, IntelMQ Manager is the web user interface, but my experience says that every renaming and difference in names used by users and developers makes the onboarding harder for both, and just leads to more time spent on explanations and reviews (I had a few projects struggling with this...). Not to mention the Google/Microsoft culture of renaming tools every month ;). I don't see any benefit in changing the code to webui, it's exactly the same as the established name of IntelMQ Manager. Could we keep the name "Manager" in the code as well?

The thought behind this change is that now the IntelMQ Manager is not a standalone tool anymore, it doesn't really need it's own name because it is just a part of IntelMQ. The user/administrator does not come in contact with the name Manager anymore. The only place where they come in contact with it is in the config which now provides optional boolean enable_webgui. From a new users perspective I find this way more self explaining than enable_manager.

Anyone slightly experienced with IntelMQ should be able to put two and two together and figure out that webgui probably means the only web interface IntelMQ ever had - the Manager.

The website itself will have just IntelMQ in it's title now.

Technically we are not "renaming" the tool, we are merging it to another bigger tool. From now on it is all just "IntelMQ". And as a part of the bigger tools, the code serves the purpose of providing webgui. :)

gethvi avatar Mar 21 '24 11:03 gethvi