mikaku / Monitorix

Monitorix is a free, open source, lightweight system monitoring tool.
https://www.monitorix.org
GNU General Public License v2.0
1.12k stars 167 forks source link

Suggest HTTP built-in server disabled default #465

Closed pfugwtg closed 9 months ago

pfugwtg commented 9 months ago

Hi, Can I give you a little advice: set HTTP built-in server disabled default, or change the default port.

In production environment, we usually need to disable the built-in server or add authenticators. And the default port 8080 is so commen that it's easy to be conflicted with other program.

Present situation: In my Ubuntu 22.04, after input sudo apt install monitorix and click enter, it installs monitorix and would start automatically. That's not I wanted. What if I have a program running which occupies port 8080 ?

mikaku commented 9 months ago

I understand your concern, but it's expected that people configure Monitorix before executing it for the first time. Once you have configured the network port for the HTTP built-in server, you don't need to worry about it anymore.

pfugwtg commented 9 months ago

I understand your concern, but it's expected that people configure Monitorix before executing it for the first time. Once you have configured the network port for the HTTP built-in server, you don't need to worry about it anymore.

But after installed, it runs automatically...☹

mikaku commented 9 months ago

But after installed, it runs automatically...☹

No, when you install Monitorix or any other tool your package manager should not start it, just install it. Just curious, what OS are you using?

pfugwtg commented 9 months ago

But after installed, it runs automatically...☹

No, when you install Monitorix or any other tool your package manager should not start it, just install it. Just curious, what OS are you using?

Ubuntu 22.04 LTS

mikaku commented 9 months ago

How did you install Monitorix?

IzzySoft commented 9 months ago

@mikaku the Debian packages start the services right after install IIRC. I know the one from my repo does, it has /etc/init.d/monitorix start in %post. Not sure about yours, though (and couldn't check as the *.deb isn't linked from your download page. Just downloaded the one for newer versions from packages.debian.org, it does start the service in postinst as well:

if [ "$1" = "configure" ] || [ "$1" = "abort-upgrade" ] || [ "$1" = "abort-deconfigure" ] || [ "$1" = "abort-remove" ] ; then
    if [ -d /run/systemd/system ]; then
        systemctl --system daemon-reload >/dev/null || true
        if [ -n "$2" ]; then
            _dh_action=restart
        else
            _dh_action=start
        fi
        deb-systemd-invoke $_dh_action 'monitorix.service' >/dev/null || true
    fi
fi
mikaku commented 9 months ago

@IzzySoft, the current docs/monitorix.spec file does not start Monitorix after installing it. Also, all the .spec files in EPEL7, EPEL8 and EPEL9 repositories and also the official Fedora repositories, aren't starting Monitorix after installing it either.

I don't know why on Debian and derivatives distributions the package manager does this extra work.

The normal steps are:

yum install monitorix
systemctl enable monitorix
systemctl start monitorix

With these individual (and not atomic) steps, the user can change the configuration files before starting Monitorix.

IzzySoft commented 9 months ago

I don't know why on Debian and derivatives distributions the package manager does this extra work.

In case of the official Debian package, I should have added one more line at the head then:

# Automatically added by dh_installinit/13.11.3
if [ "$1" = "configure" ] || [ "$1" = "abort-upgrade" ] || [ "$1" = "abort-deconfigure" ] || [ "$1" = "abort-remove" ] ; then

does that explain? I agree it doesn't make much sense if the default config cannot be used, but that's how it works for most services coming on Debian (not all though, so there must be a way to disable that). In my case, I can easily take it out if you want to (but I'd keep the "stop" in %preun just in case). That would still leave the question on updates, as then a simple apt upgrade would no longer include a restart. Might even cause a stop without start then, haven't looked or even checked yet.

mikaku commented 9 months ago

An apt upgrade should include a restart only if the application being updated is already running. This is how yum (and RPM) works, which makes more sense to me.

The problem here is that if you include a modification on this, there will be a different behavior between your .deb and the ones coming from the official Debian repositories. Which could create some confusion.

@pfugwtg, I'm sorry but I don't see an easy solution since this behavior is treated as a feature by the Debian package management.

You will have to stop Monitorix right after the installation. This is the extra step you'll have to do before configure it.

IzzySoft commented 9 months ago

@mikaku we can reach out to who makes the official Debian packages. I know there are some services you have to manually activate, so there must be a way with the official Debian packaging tools for that. I have no idea how to do that with my "converter" (I mean, I could set up manual steps in the corresponding pre/post tasks to test if the service is running – but I don't know how that survives on an upgrade. As my package is intended for the older systems and those not running systemd, I could probably check for symlinks to the init script in the different runlevels, or utilize the corresponding tools – but to be honest, I have no time these days to dig into that :cry: I don't even know for how many people my "legacy package" is still useful at all, maybe one day we should even drop it?

mikaku commented 9 months ago

we can reach out to who makes the official Debian packages. I know there are some services you have to manually activate, so there must be a way with the official Debian packaging tools for that.

@lyknode, Perhaps you can help us here. Basically to know if it's possible to disable the autostart after Monitorix installation.

I don't even know for how many people my "legacy package" is still useful at all, maybe one day we should even drop it?

Due the versioning nature of Debian, people using stable versions still have a very old Monitorix version, and they are forced to go to unstable or untested, (I don't know how they are called), to have a more decent Monitorix version. So, I think that your repository is good for those users on stable releases.

pfugwtg commented 9 months ago

An apt upgrade should include a restart only if the application being updated is already running. This is how yum (and RPM) works, which makes more sense to me.

The problem here is that if you include a modification on this, there will be a different behavior between your .deb and the ones coming from the official Debian repositories. Which could create some confusion.

@pfugwtg, I'm sorry but I don't see an easy solution since this behavior is treated as a feature by the Debian package management.

You will have to stop Monitorix right after the installation. This is the extra step you'll have to do before configure it.

That's OK. Appreciate your efforts.

lyknode commented 9 months ago

Hi,

I'm the maintainer of the Monitorix Debian package from the official archive.

Starting services provided by packages is a distribution-wide policy, as stated in the Debian policy:

The default behaviour is to enable autostarting your package’s daemon.

Basically, when a Debian user install a service, they expect the service to be started right away.

While technically, it is possible to override this behavior in the packaging, Monitorix doesn't really qualify as an exception since it provides a working default. Additionally, having the 8080 port already taken does not fail the installation of monitorix, which would have been a serious bug.

If you do not want this behavior on systems you manage, there is a way to disable autostarting daemon when installing packages.

policy-rc.d will help preventing services from being started on install:

cat << EOF | sudo tee /usr/sbin/policy-rc.d
#!/bin/bash
exit 101 
EOF

sudo chmod +x /usr/sbin/policy-rc.d

While systemd-preset will prevent services to be enabled at boot on install:

sudo mkdir -p /etc/systemd/system-preset
echo 'disable *' | sudo tee /etc/systemd/system-preset/00-disable.preset

Obviously, this does not apply to previously installed services or manually started or enabled services.

I hope this helps. Best,

mikaku commented 9 months ago

@lyknode, your comments are always welcome. I think this clears it up the issue. Thanks!

IzzySoft commented 9 months ago

So we keep it as-is for both Debian packages @mikaku?

mikaku commented 9 months ago

So we keep it as-is for both Debian packages @mikaku?

@IzzySoft yes, better keep aligned with official Debian packages. Thanks!

IzzySoft commented 9 months ago

Will do, thank you!

Saentist commented 9 months ago

@mikaku @IzzySoft Why not include in DEB simple Postinst script witch to ask for server port and option to use credentials? https://www.debian.org/doc/debian-policy/ch-maintainerscripts.html https://wiki.debian.org/MaintainerScripts

Preinst, Postinst, Prerm, and Postrm Script's are to solve similar problems.

Guess RPM and other distribution packages have same options.

Just lets don't forget old bugs and learn from them https://github.com/mikaku/Monitorix/issues/414

mikaku commented 9 months ago

@Saentist,

I believe that introducing a prompt during Monitorix installation asking for specific configuration options, is something that can be annoying for most users. The scripts Preinst, Postinst, etc. shouldn't interfere the installation process, but execute in background.

Also, this wouldn't be aligned with the official Debian package.

Saentist commented 9 months ago

@mikaku Same is situation if service not start at all.

Script can do some simple steps If fresh install > make monitorix.conf and ask user for port and also check if not used by other service, then generate empty config with link to documentation. /monitorix.conf need to be monitorix.conf.example/ if there is monitorix.conf > don't ask anything /in case of update/

There is lot of variations /I will prefer to have something as sensors-detect in case monitorix-detect config generator./

I don't see any more simple logic. Simple prompt is not so different if user say CANCEL and then need do configuration manual

mikaku commented 9 months ago

Simple prompt is not so different if user say CANCEL and then need do configuration manual

Having to type anything (even for cancel) is annoying if you are running an unattended installation.

Saentist commented 9 months ago

Cancel can be default selected option.

Then simple timer for 5 sec and if no action takes, to be as now, automatically without user interaction.

Then again same situation as first post in this issue.