Bug 998650

Thanks for the patch Quentin!

A few things:

1. I won't add a sysvinit script. That's entirely deprecated in Fedora.

2. No need for Group, it's deprecated in Fedora.

3. No need for the Conflicts with old systemd and filesystems, these don't exist any more in Fedora.

4. BuildRequires: systemd, not systemd-units

    => https://fedoraproject.org/wiki/Packaging:Systemd#Filesystem_locations

5. I'm a bit hesitant (but I can be convinced of the contrary) on the file requirement on iptables.service.

    => https://fedoraproject.org/wiki/Packaging:Guidelines#File_Dependencies

Any reason not to simply use Requires: iptables-services ?

6. I don't see the need for the strict versioned requires on the -libs package. After all, rpmbuild already adds the proper library requirements. Or is that actually fixing a problem you had?

7. What's the Requires: systemd-sysv for? It's not required by the guidelines, and it doesn't even exist in Rawhide any more.

8. There's a host of scriptlets for both the base package and the -services subpackage. Shouldn't that be only on the subpackage instead?

9. The scriptlets don't respect the guidelines.

    => https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Systemd

10. After=syslog.target is obsolete.

11. StandardOutput/StandardError defaults to syslog/inherited, so these 2 lines are unneeded.

12. What are the 2 Environment= lines for?

13. Using a sysvinit script as ExecStart= is ugly and overkill. Why not just use directly the ipset commands?

14. The 2 IPSET_STATUS_* options in the sysconfig file are only used by the sysvinit, which I don't want in the package.

15. What is the point of unloading the modules?

16. The IPSET_SAVE_ON_ options could simply be options in the unit file, so we don't need to carry a sysconfig file at all.

Sorry if it seems like I have many criticism, I really do appreciate your submitting a patch. :)

Created attachment 789020 [details]
Updated patch to add services sub-package

(In reply to Mathieu Bridon from comment #1)
> 
> A few things:
> 
> 1. I won't add a sysvinit script. That's entirely deprecated in Fedora.

I had (perhaps mis-)understood from Nicolas Chauvet's comment in bz#994245 that sysvinit scripts were required.

I have now removed all the sysvinit elements.

> 2. No need for Group, it's deprecated in Fedora.

My understanding from:

   =>  https://fedoraproject.org/wiki/Packaging:Guidelines#Group_tag 

is that it's optional, but I've removed it anyway.

> 3. No need for the Conflicts with old systemd and filesystems, these don't
> exist any more in Fedora.

Removed offending lines

> 4. BuildRequires: systemd, not systemd-units

Fixed

> 5. I'm a bit hesitant (but I can be convinced of the contrary) on the file
> requirement on iptables.service.

This was a misguided attempt to allow it to work with iptables prior to the introduction of the iptables-services sub-package.
Now changed to depend on iptables-services.
 
> 6. I don't see the need for the strict versioned requires on the -libs
> package. After all, rpmbuild already adds the proper library requirements.
> Or is that actually fixing a problem you had?

Without the version requirement, the requirements rpmbuild adds are:
libipset.so.3()(64bit)  
libipset.so.3(LIBIPSET_1.0)(64bit)  
libipset.so.3(LIBIPSET_2.0)(64bit)  
libipset.so.3(LIBIPSET_3.0)(64bit)

So, for example, a yum update ipset could update the ipset binary without updating the library package, giving a version mismatch.

A patch I provided upstream updated files that affect both the ipset and ipset-libs packages, so I think they really do need to keep in step.

> 7. What's the Requires: systemd-sysv for? It's not required by the
> guidelines, and it doesn't even exist in Rawhide any more.

Removed

> 8. There's a host of scriptlets for both the base package and the -services
> subpackage. Shouldn't that be only on the subpackage instead?

Oops! I have removed the scriptlets from the base package.

> 9. The scriptlets don't respect the guidelines.
> 
>     => https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Systemd

Ah, I followed the iptables spec file too closely. Now modified in accordance with the guidelines.

> 10. After=syslog.target is obsolete.

Removed

> 11. StandardOutput/StandardError defaults to syslog/inherited, so these 2
> lines are unneeded.

Removed
 
> 12. What are the 2 Environment= lines for?

I don't know and did wonder myself! I was just following the iptables spec file. I have now removed them.

> 13. Using a sysvinit script as ExecStart= is ugly and overkill. Why not just
> use directly the ipset commands?

The init script does more that than a simple load of the saved ipset configuration, since it also removes any entries that were loaded in the ipsets before the service is started, and removes any sets that are not in the configuration being loaded. ipset restore simply adds what is in the file being restored but doesn't remove any pre-existing config.

I have renamed the ipset.init script to ipset.start-stop and placed it in /usr/libexec/ipset in accordance with the guidelines. I have also removed all the sysvinit script elements from it.
 
> 14. The 2 IPSET_STATUS_* options in the sysconfig file are only used by the
> sysvinit, which I don't want in the package.

Removed

> 15. What is the point of unloading the modules?

None that I can see; I was just copying iptables.init. I have now removed it.

> 16. The IPSET_SAVE_ON_ options could simply be options in the unit file, so
> we don't need to carry a sysconfig file at all.

Done

> Sorry if it seems like I have many criticism, I really do appreciate your
> submitting a patch. :)

I'm very happy for any comments, especially if it helps improve the end result.

Do please let me know if there are any further issues with the patch.

Thanks for getting through that big list Quentin. :)

(In reply to Quentin Armitage from comment #2)
> > 6. I don't see the need for the strict versioned requires on the -libs
> > package. After all, rpmbuild already adds the proper library requirements.
> > Or is that actually fixing a problem you had?
> 
> Without the version requirement, the requirements rpmbuild adds are:
> libipset.so.3()(64bit)  
> libipset.so.3(LIBIPSET_1.0)(64bit)  
> libipset.so.3(LIBIPSET_2.0)(64bit)  
> libipset.so.3(LIBIPSET_3.0)(64bit)
> 
> So, for example, a yum update ipset could update the ipset binary without
> updating the library package, giving a version mismatch.

Ah, right.

I didn't add it in the first place because:

  1. rpmbuild adds the requirement, so "yum install ipset" just works
  2. they are built from the same source package, so "yum update" just works.

I hadn't thought about someone running "yum update ipset" explicitly. In this case you're right, the ipset-libs wouldn't be updated, so that needs to be fixed. Thanks for catching that!

However, you forgot the %{?_isa} to make the requirement arch-specific ;)

> > 13. Using a sysvinit script as ExecStart= is ugly and overkill. Why not just
> > use directly the ipset commands?
> 
> The init script does more that than a simple load of the saved ipset
> configuration, since it also removes any entries that were loaded in the
> ipsets before the service is started, and removes any sets that are not in
> the configuration being loaded. ipset restore simply adds what is in the
> file being restored but doesn't remove any pre-existing config.

Is that really needed?

I mean, someone using the service would not have any sets loaded before starting the service?

I'm not sure here, because as I already mentioned, I don't use ipset as a service. (I have a firewall service which takes care of loading the ipsets)

So I'd personally prefer to keep it as simple as possible, but... :-/

Maybe a better solution is to integrate more ipset with the rest of the firewall? (firewalld and/or iptables.service)

> I have renamed the ipset.init script to ipset.start-stop and placed it in
> /usr/libexec/ipset in accordance with the guidelines. I have also removed
> all the sysvinit script elements from it.

A few more things:

1. What's the purpose of VAR_SUBSYS_IPSET ? It seems like something we'd have done in the old days of sysvinit, but systemd already knows whether the service is started or not. (especially since you used RemainAfterExit=yes)

From a quick look, I think you can drop that entirely?

2. /etc/sysconfig is a Fedora/RHEL specific thing. Eventually, I'd be happier if what we come up with in this bug could be sent upstream. ;)

How about /etc/ipset/ ?

3. This is purely cosmetic, but I'd prefer having the BuildRequires: systemd in the -services subpackage.

4. I'll be annoyingly pedantic: there's only one service in the 'services' package. :P

5. Finally, there's something that bothers me a bit. Say you start ipset.service to load your sets, then iptables.service to load your rules, some of which reference the sets.

Now if you try stopping the ipset.service, it's going to fail, because sets can't be destroyed while they are in use. Or rather, the sets will be flushed, but not destroyed, and the service will be considered stopped.

That's a bit confusing, isn't the point of stopping the service to completely destroy all the sets?

If the ipset package gets removed, the scriptlet will stop the service, so the firewall will have some rules pointing to sets, but without any way to destroy them or do anything with them. They'll be flushed though, so maybe it's not such a problem?

Created attachment 790977 [details]
Further updated patch for ipset-service

(In reply to Mathieu Bridon from comment #3)
> 
> However, you forgot the %{?_isa} to make the requirement arch-specific ;)

%{_isa} added

> > > 13. Using a sysvinit script as ExecStart= is ugly and overkill. Why not just
> > > use directly the ipset commands?
> > 
> > The init script does more that than a simple load of the saved ipset
> > configuration, since it also removes any entries that were loaded in the
> > ipsets before the service is started, and removes any sets that are not in
> > the configuration being loaded. ipset restore simply adds what is in the
> > file being restored but doesn't remove any pre-existing config.
> 
> Is that really needed?
> 
> I mean, someone using the service would not have any sets loaded before
> starting the service?

I think this depends if one is just considering system startup/shutdown, or if the service is being restarted while the system is running. 

> So I'd personally prefer to keep it as simple as possible, but... :-/
> 
> Maybe a better solution is to integrate more ipset with the rest of the
> firewall? (firewalld and/or iptables.service)

I think integrating with firewalld and iptables.services has its advantages and disadvantages, but overall I see it as better being separate.

Primarily, I may not want to unload and reload the ipsets if I reload the iptables configuration. Furthermore, for iptables.services I may only be stopping or restarting only one of iptables/ip6tables and so cannot unload/stop ipsets.

I have about 700 entries in my iptables configuration, but 75000 in ipset, so if I just want to reload iptables, it would be an unnecessary overhead to reload ipsets too.

This is obviously more about restarting the service than starting up at boot time, so I have added back in an explicit reload, but on the basis that someone may want to stop the service, do something and then start it again, I think the existing functionality needs to remain for start. If there is no currently loaded ipset configuration when the service starts/restarts, then it does a simple ipset restore -! </etc/ipset/ipset; it's only if there is some existing configuration loaded that it has to go through the more complex process of merging the config.

If someone has been experimenting with their ipset setup, and then wants to restore the original configuration, a service restart is not an unreasonable concept. If a restart is done because iptables is being restarted (due to it being experimented with), then I guess SAVE_ON_RESTART makes sense for ipsets.

> 
> A few more things:
> 
> 1. What's the purpose of VAR_SUBSYS_IPSET 
> 
> From a quick look, I think you can drop that entirely?

VAR_SYS_IPSET code removed

> 2. /etc/sysconfig is a Fedora/RHEL specific thing. Eventually, I'd be
> happier if what we come up with in this bug could be sent upstream. ;)
> 
> How about /etc/ipset/ ?

Done

> 3. This is purely cosmetic, but I'd prefer having the BuildRequires: systemd
> in the -services subpackage.

Done

> 4. I'll be annoyingly pedantic: there's only one service in the 'services'
> package. :P

Where would the world be without pedants? In a mess!

Changed. 

> 5. Finally, there's something that bothers me a bit. Say you start
> ipset.service to load your sets, then iptables.service to load your rules,
> some of which reference the sets.
> 
> Now if you try stopping the ipset.service, it's going to fail, because sets
> can't be destroyed while they are in use. Or rather, the sets will be
> flushed, but not destroyed, and the service will be considered stopped.

I've added a warning message if this happens. I've also changed it so it removes as many sets as possible.

> That's a bit confusing, isn't the point of stopping the service to
> completely destroy all the sets?
> 
> If the ipset package gets removed, the scriptlet will stop the service, so
> the firewall will have some rules pointing to sets, but without any way to
> destroy them or do anything with them. They'll be flushed though, so maybe
> it's not such a problem?

I agree this isn't ideal, but I thought it was the best we could do. Actually, that is no different from what we have now if someone has iptables using an ipset but then removes the ipset service, and at the moment, without such an ipset.service, then if you use ipsets, iptables.service is guaranteed to fail at startup, which is significantly worse than this slight bit of confusion.

If someone removes the ipset package, then yes indeed empty sets will be left around, but that will be sorted out after the next system reboot. I have added a message that not all sets were flushed/deleted if that happens (I can't see how the ipset flush command can fail though), and I have made it iterate through the sets when deleting them, so that it deletes as many as possible.

One alternative, is if the xt_set module is loaded to attempt to remove it, and it that fails (due to iptables using a set), then refuse to stop the ipset service. I've implemented this and also added it in to the preun scriptlets for ipset itself and ipset.service. I'm happy to remove these bits if you don't like them, but I think it addresses your concerns, and actually makes it safer than the current position, and I can't see that it can do any harm.

On system shutdown, iptables/ip6tables will be stopped first, and so the ipset service will successfully stop, so it all seems to hang together OK.

Sorry for the delay in responding to your comments - holidays in the UK!

As I said before, I don't have the need for such a systemd service (I don't even use iptables.service or firewalld either, we have our own thing to handle that here).

So at this point, I'm pretty much of the opinion to just assume that you're using it, and that it responds to the needs (well, at least yours :P).

I assume you tested it, and it works properly. (particularly about upgrades and removal)

So I've committed your patch, made a few trivial changes on top of it (just some more pedantic stuff in the spec file, nothing that touches your feature), and the package is now building in Rawhide and Fedora 20.

Also, if you have an interest in this package, I'd be happy to make you a co-maintainer. If anything, I'd really like it if you could at the very least check the "watchbugzilla" box in PackageDB, so that you can help fix problems arising with the service. (because not using it myself, I probably wouldn't do a good job at it).

Thanks again for the patch!