Previously it was only possible to add services to be enabled on the firewall, but removing already enabled services was not possible. This was mainly problematic that there are some services, such as dhcpv6-client, mdns and ssh, that are enabled by default and up till now, there was no way to remove them from the firewall in kickstart.
The first patch adds support for the --remove-service option & some tests to pykickstart.
And the second patch adds service removal logic to Anaconda.
Martin Kolman (1): Add --remove-service option for the firewall command (#957809)
pykickstart/commands/firewall.py | 42 ++++++++++++++++++++++++++++++++++++++++ pykickstart/handlers/control.py | 2 +- tests/commands/firewall.py | 10 ++++++++++ 3 files changed, 53 insertions(+), 1 deletion(-)
The --remove-service option makes it possible to remove services that were enabled on the firewall.
Signed-off-by: Martin Kolman mkolman@redhat.com --- pykickstart/commands/firewall.py | 42 ++++++++++++++++++++++++++++++++++++++++ pykickstart/handlers/control.py | 2 +- tests/commands/firewall.py | 10 ++++++++++ 3 files changed, 53 insertions(+), 1 deletion(-)
diff --git a/pykickstart/commands/firewall.py b/pykickstart/commands/firewall.py index 664bea6..1384d63 100644 --- a/pykickstart/commands/firewall.py +++ b/pykickstart/commands/firewall.py @@ -191,3 +191,45 @@ class F14_Firewall(F10_Firewall): op = F10_Firewall._getParser(self) op.remove_option("--telnet") return op + +class F20_Firewall(F14_Firewall): + removedKeywords = F14_Firewall.removedKeywords + removedAttrs = F14_Firewall.removedAttrs + + def _getParser(self): + def remove_service_cb(option, opt_str, value, parser): + # python2.4 does not support action="append_const" that we were + # using for these options. Instead, we have to fake it by + # appending whatever the option string is to the service list. + # XXX: is this still relevant ? + if not value: + parser.values.ensure_value(option.dest, []).append(opt_str[2:]) + return + + for p in value.split(","): + p = p.strip() + parser.values.ensure_value(option.dest, []).append(p) + + op = F14_Firewall._getParser(self) + op.add_option("--remove-service", dest="remove_services", + action="callback", callback=remove_service_cb, + nargs=1, type="string") + return op + + def __str__(self): + if self.enabled is None: + return "" + + retval = F10_Firewall.__str__(self) + if self.enabled: + retval = retval.strip() + + svcstr = ",".join(self.remove_services) + if len(svcstr) > 0: + svcstr = " --remove-service=" + svcstr + else: + svcstr = "" + + return retval + "%s\n" % svcstr + else: + return retval diff --git a/pykickstart/handlers/control.py b/pykickstart/handlers/control.py index 83dece0..987158c 100644 --- a/pykickstart/handlers/control.py +++ b/pykickstart/handlers/control.py @@ -1040,7 +1040,7 @@ commandMap = { "driverdisk": driverdisk.F14_DriverDisk, "eula": eula.F20_Eula, "fcoe": fcoe.F13_Fcoe, - "firewall": firewall.F14_Firewall, + "firewall": firewall.F20_Firewall, "firstboot": firstboot.FC3_Firstboot, "graphical": displaymode.FC3_DisplayMode, "group": group.F12_Group, diff --git a/tests/commands/firewall.py b/tests/commands/firewall.py index 392684e..bfe7426 100644 --- a/tests/commands/firewall.py +++ b/tests/commands/firewall.py @@ -43,6 +43,16 @@ class FC3_TestCase(CommandTest): self.assert_parse("firewall --enable --trust=eth0,eth1 --ssh --http --smtp --ftp --port=1234:udp" "firewall --enabled --port=1234:udp --trust=eth0,eth1 --service=ssh,http,smtp,ftp\n")
+ # remove service + if "--remove-service" in self.optionList: + self.assert_parse("firewall --remove-service=mdns") # remove only + # service & remove service at once + self.assert_parse("firewall --service=ssh --remove-service=mdns") + # with alternative service notation + self.assert_parse("firewall --ssh --smtp --ftp --remove-service=mdns") + # multiple remove & remove ssh + self.assert_parse("firewall --service=mdns --remove-service=dhcpv6-client --remove-service=ssh") + # disable firewall self.assert_parse("firewall --disabled", "firewall --disabled\n") self.assert_parse("firewall --disable", "firewall --disabled\n")
On Fri, Oct 04, 2013 at 06:07:04PM +0200, Martin Kolman wrote:
The --remove-service option makes it possible to remove services that were enabled on the firewall.
Signed-off-by: Martin Kolman mkolman@redhat.com
pykickstart/commands/firewall.py | 42 ++++++++++++++++++++++++++++++++++++++++ pykickstart/handlers/control.py | 2 +- tests/commands/firewall.py | 10 ++++++++++ 3 files changed, 53 insertions(+), 1 deletion(-)
diff --git a/pykickstart/commands/firewall.py b/pykickstart/commands/firewall.py index 664bea6..1384d63 100644 --- a/pykickstart/commands/firewall.py +++ b/pykickstart/commands/firewall.py @@ -191,3 +191,45 @@ class F14_Firewall(F10_Firewall): op = F10_Firewall._getParser(self) op.remove_option("--telnet") return op
+class F20_Firewall(F14_Firewall):
- removedKeywords = F14_Firewall.removedKeywords
- removedAttrs = F14_Firewall.removedAttrs
You also need an updated __init__ that sets the empty value for self.remove_services (see the F10_Firewall class).
# remove service
if "--remove-service" in self.optionList:
self.assert_parse("firewall --remove-service=mdns") # remove only
# service & remove service at once
self.assert_parse("firewall --service=ssh --remove-service=mdns")
# with alternative service notation
self.assert_parse("firewall --ssh --smtp --ftp --remove-service=mdns")
# multiple remove & remove ssh
self.assert_parse("firewall --service=mdns --remove-service=dhcpv6-client --remove-service=ssh")
These are missing the result string, which is just the original with \n on the end.
On Fri, 2013-10-04 at 13:43 -0700, Brian C. Lane wrote:
On Fri, Oct 04, 2013 at 06:07:04PM +0200, Martin Kolman wrote:
The --remove-service option makes it possible to remove services that were enabled on the firewall.
Signed-off-by: Martin Kolman mkolman@redhat.com
pykickstart/commands/firewall.py | 42 ++++++++++++++++++++++++++++++++++++++++ pykickstart/handlers/control.py | 2 +- tests/commands/firewall.py | 10 ++++++++++ 3 files changed, 53 insertions(+), 1 deletion(-)
diff --git a/pykickstart/commands/firewall.py b/pykickstart/commands/firewall.py index 664bea6..1384d63 100644 --- a/pykickstart/commands/firewall.py +++ b/pykickstart/commands/firewall.py @@ -191,3 +191,45 @@ class F14_Firewall(F10_Firewall): op = F10_Firewall._getParser(self) op.remove_option("--telnet") return op
+class F20_Firewall(F14_Firewall):
- removedKeywords = F14_Firewall.removedKeywords
- removedAttrs = F14_Firewall.removedAttrs
You also need an updated __init__ that sets the empty value for self.remove_services (see the F10_Firewall class).
Oh, right, thanks !
# remove service
if "--remove-service" in self.optionList:
self.assert_parse("firewall --remove-service=mdns") # remove only
# service & remove service at once
self.assert_parse("firewall --service=ssh --remove-service=mdns")
# with alternative service notation
self.assert_parse("firewall --ssh --smtp --ftp --remove-service=mdns")
# multiple remove & remove ssh
self.assert_parse("firewall --service=mdns --remove-service=dhcpv6-client --remove-service=ssh")
These are missing the result string, which is just the original with \n on the end.
OK, fixing both locally.
anaconda-patches mailing list anaconda-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
On Fri, 2013-10-04 at 18:07 +0200, Martin Kolman wrote:
The --remove-service option makes it possible to remove services that were enabled on the firewall.
Signed-off-by: Martin Kolman mkolman@redhat.com
pykickstart/commands/firewall.py | 42 ++++++++++++++++++++++++++++++++++++++++ pykickstart/handlers/control.py | 2 +- tests/commands/firewall.py | 10 ++++++++++ 3 files changed, 53 insertions(+), 1 deletion(-)
diff --git a/pykickstart/commands/firewall.py b/pykickstart/commands/firewall.py index 664bea6..1384d63 100644 --- a/pykickstart/commands/firewall.py +++ b/pykickstart/commands/firewall.py @@ -191,3 +191,45 @@ class F14_Firewall(F10_Firewall): op = F10_Firewall._getParser(self) op.remove_option("--telnet") return op
+class F20_Firewall(F14_Firewall):
- removedKeywords = F14_Firewall.removedKeywords
- removedAttrs = F14_Firewall.removedAttrs
I think you don't need to do this, these two attributes get inherited.
diff --git a/pykickstart/commands/firewall.py b/pykickstart/commands/firewall.py index 664bea6..1384d63 100644 --- a/pykickstart/commands/firewall.py +++ b/pykickstart/commands/firewall.py @@ -191,3 +191,45 @@ class F14_Firewall(F10_Firewall): op = F10_Firewall._getParser(self) op.remove_option("--telnet") return op
+class F20_Firewall(F14_Firewall):
- removedKeywords = F14_Firewall.removedKeywords
- removedAttrs = F14_Firewall.removedAttrs
- def _getParser(self):
def remove_service_cb(option, opt_str, value, parser):
# python2.4 does not support action="append_const" that we were
# using for these options. Instead, we have to fake it by
# appending whatever the option string is to the service list.
# XXX: is this still relevant ?
This breaks "make check".
- Chris
On Mon, 2013-10-07 at 10:37 -0400, Chris Lumens wrote:
diff --git a/pykickstart/commands/firewall.py b/pykickstart/commands/firewall.py index 664bea6..1384d63 100644 --- a/pykickstart/commands/firewall.py +++ b/pykickstart/commands/firewall.py @@ -191,3 +191,45 @@ class F14_Firewall(F10_Firewall): op = F10_Firewall._getParser(self) op.remove_option("--telnet") return op
+class F20_Firewall(F14_Firewall):
- removedKeywords = F14_Firewall.removedKeywords
- removedAttrs = F14_Firewall.removedAttrs
- def _getParser(self):
def remove_service_cb(option, opt_str, value, parser):
# python2.4 does not support action="append_const" that we were
# using for these options. Instead, we have to fake it by
# appending whatever the option string is to the service list.
# XXX: is this still relevant ?
This breaks "make check".
Oh, right - but unfortunately I've already pushed it to master...
So I guess I can make a patch removing the Pylint-hated triple-X and maybe also remove the whole python2.4 & append_const related comment ? It doesn't look like very up-to-date/relevant.
- Chris
anaconda-patches mailing list anaconda-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
So I guess I can make a patch removing the Pylint-hated triple-X and maybe also remove the whole python2.4 & append_const related comment ? It doesn't look like very up-to-date/relevant.
It depends on if we are still using the latest version of pykickstart on RHEL5 hosts internally, which I do not have any data on. However I'd like to avoid breaking them just in case.
- Chris
On Mon, 2013-10-07 at 11:23 -0400, Chris Lumens wrote:
So I guess I can make a patch removing the Pylint-hated triple-X and maybe also remove the whole python2.4 & append_const related comment ? It doesn't look like very up-to-date/relevant.
It depends on if we are still using the latest version of pykickstart on RHEL5 hosts internally, which I do not have any data on. However I'd like to avoid breaking them just in case.
- Chris
OK, so the comment is therefore probably still valid and I'll just make a simple patch removing the XXX line.
anaconda-patches mailing list anaconda-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
As the comment above the XXX line showed up to be still valid (the current master Pykickstart might still be used with Python 2.4), the XXX line is not needed anymore. Also Pylint doesn't seem to like XXX showing up in comments.
Related: rhbz#957809 Signed-off-by: Martin Kolman mkolman@redhat.com --- pykickstart/commands/firewall.py | 1 - 1 file changed, 1 deletion(-)
diff --git a/pykickstart/commands/firewall.py b/pykickstart/commands/firewall.py index 89e2212..2e61f70 100644 --- a/pykickstart/commands/firewall.py +++ b/pykickstart/commands/firewall.py @@ -203,7 +203,6 @@ class F20_Firewall(F14_Firewall): # python2.4 does not support action="append_const" that we were # using for these options. Instead, we have to fake it by # appending whatever the option string is to the service list. - # XXX: is this still relevant ? if not value: parser.values.ensure_value(option.dest, []).append(opt_str[2:]) return
As the comment above the XXX line showed up to be still valid (the current master Pykickstart might still be used with Python 2.4), the XXX line is not needed anymore. Also Pylint doesn't seem to like XXX showing up in comments.
Related: rhbz#957809 Signed-off-by: Martin Kolman mkolman@redhat.com
I pushed this so I can do some builds. Thanks.
- Chris
On Tue, 2013-10-08 at 14:33 -0400, Chris Lumens wrote:
As the comment above the XXX line showed up to be still valid (the current master Pykickstart might still be used with Python 2.4), the XXX line is not needed anymore. Also Pylint doesn't seem to like XXX showing up in comments.
Related: rhbz#957809 Signed-off-by: Martin Kolman mkolman@redhat.com
I pushed this so I can do some builds. Thanks.
OK, thanks ! :)
- Chris
anaconda-patches mailing list anaconda-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
Add support for removing services, that were enabled on the firewall. This also makes it possible to disable services that are enabled on the firewall by default, such as dhcpv6-client, mdns or ssh. Services that are not enabled but are requested for removal are ignored.
Signed-off-by: Martin Kolman mkolman@redhat.com --- pyanaconda/kickstart.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/pyanaconda/kickstart.py b/pyanaconda/kickstart.py index 34929b9..df874f7 100644 --- a/pyanaconda/kickstart.py +++ b/pyanaconda/kickstart.py @@ -501,7 +501,7 @@ class Fcoe(commands.fcoe.F13_Fcoe):
return fc
-class Firewall(commands.firewall.F14_Firewall): +class Firewall(commands.firewall.F20_Firewall): def execute(self, storage, ksdata, instClass): args = [] # enabled is None if neither --enable or --disable is passed @@ -511,7 +511,8 @@ class Firewall(commands.firewall.F14_Firewall): else: args += ["--enabled"]
- if not "ssh" in self.services and not "22:tcp" in self.ports: + if "ssh" not in self.services and "ssh" not in self.remove_services \ + and "22:tcp" not in self.ports: args += ["--service=ssh"]
for dev in self.trusts: @@ -520,6 +521,9 @@ class Firewall(commands.firewall.F14_Firewall): for port in self.ports: args += [ "--port=%s" % (port,) ]
+ for remove_service in self.remove_services: + args += ["--remove-service=%s" % remove_service] + for service in self.services: args += [ "--service=%s" % (service,) ]
On Fri, Oct 04, 2013 at 06:07:05PM +0200, Martin Kolman wrote:
@@ -520,6 +521,9 @@ class Firewall(commands.firewall.F14_Firewall): for port in self.ports: args += [ "--port=%s" % (port,) ]
for remove_service in self.remove_services:
args += ["--remove-service=%s" % remove_service]
I have since changed my mind on the (xxx,) form for single tuples, but this should match the surrounding code just for clarity.
for service in self.services: args += [ "--service=%s" % (service,) ]
On Fri, 2013-10-04 at 13:44 -0700, Brian C. Lane wrote:
On Fri, Oct 04, 2013 at 06:07:05PM +0200, Martin Kolman wrote:
@@ -520,6 +521,9 @@ class Firewall(commands.firewall.F14_Firewall): for port in self.ports: args += [ "--port=%s" % (port,) ]
for remove_service in self.remove_services:
args += ["--remove-service=%s" % remove_service]
I have since changed my mind on the (xxx,) form for single tuples, but this should match the surrounding code just for clarity.
OK, fixing locally.
for service in self.services: args += [ "--service=%s" % (service,) ]
anaconda-patches mailing list anaconda-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
anaconda-patches@lists.fedorahosted.org