Yedidyah Bar David has posted comments on this change. Change subject: packaging: setup: update firewall for all services ......................................................................
Patch Set 22: (13 comments) .................................................... File packaging/setup/plugins/ovirt-engine-setup/network/firewall_manager.py Line 42 Line 43 Line 44 Line 45 Line 46 Done. After some thinking I decided to move it to firewall_manager_firewalld. This: 1. Leaves this file clean from manager-specific code 2. Makes firewall_manager_firewalld special. Which indeed it is, if we decide to base on it all the others. Line 70: self.environment[osetupcons.CoreEnv.DEVELOPER_MODE] Line 71: or ( Line 72: self.environment[ Line 73: osetupcons.ConfigEnv.AUTOMATIC_FIREWALLING_REQUESTED Line 74: ] is False No, it can also be None. If it's None, we want _enabled to be True, but if it's False, we don't. I'll add a comment. Line 75: ) Line 76: ) Line 77: Line 78: @plugin.event( Line 96: if ( Line 97: manager.name in valid_managers or Line 98: not manager.selectable() Line 99: ): Line 100: filtered_managers.append(manager) Done Line 101: self.environment[ Line 102: osetupcons.ConfigEnv.FIREWALL_MANAGERS Line 103: ] = filtered_managers Line 104: Line 120: self.logger.debug( Line 121: '_customization_is_requested - manager %s' % ( Line 122: manager Line 123: ) Line 124: ) Removed, but as I commented elsewhere, we don't have it. I'll try to submit a patch to otopi that will fix that. Line 125: if manager.selectable() and manager.detect(): Line 126: self._detected_managers.append(manager) Line 127: Line 128: if ( Line 122: manager Line 123: ) Line 124: ) Line 125: if manager.selectable() and manager.detect(): Line 126: self._detected_managers.append(manager) Done Line 127: Line 128: if ( Line 129: self.environment[ Line 130: osetupcons.ConfigEnv.AUTOMATIC_FIREWALLING_REQUESTED Line 139: 'overwrite current settings.\n' Line 140: ), Line 141: ) Line 142: self._enabled = self.environment[ Line 143: osetupcons.ConfigEnv.AUTOMATIC_FIREWALLING_REQUESTED I renamed it to make it clearer, hope I succeeded. To know, internally, if we will eventually update the firewall, just use self._enabled, but note that its value changes until the final decision in process_templates. I can also add something to the environment if you want, but currently noone uses it. Line 144: ] = dialog.queryBoolean( Line 145: dialog=self.dialog, Line 146: name='OVESETUP_UPDATE_FIREWALL', Line 147: note=_( Line 168: def _customization(self): Line 169: active_managers = [] Line 170: for manager in self._detected_managers: Line 171: if manager.active(): Line 172: active_managers.append(manager) Done Line 173: Line 174: self._available_managers = ( Line 175: active_managers if active_managers Line 176: else self._detected_managers Line 230: def _process_templates(self): Line 231: for manager in self.environment[ Line 232: osetupcons.ConfigEnv.FIREWALL_MANAGERS Line 233: ]: Line 234: manager.prepare() Ok for now. The reason I am a bit hesitant is that currently, all of the actual logic of configuring the manager is done in otopi, but I am not sure this is going to stay that way - if e.g. we add one day firewall_manager_firehol, it might do (conceptually) both what firewall_manager_firewalld and otopi:src/plugins/otopi/network/firewalld.py do for firewalld. Line 235: Line 236: if self._enabled: Line 237: if self.environment[ Line 238: osetupcons.ConfigEnv.FIREWALL_MANAGER Line 232: osetupcons.ConfigEnv.FIREWALL_MANAGERS Line 233: ]: Line 234: manager.prepare() Line 235: Line 236: if self._enabled: Right. No, I can't prepare at misc - it needs to be done for preparing data for otopi to configure stuff at validation. At one point I did have it separated to two functions, but decided I prefer it this way. The only reason to separate is to prevent an 'if', not good enough for me. Line 237: if self.environment[ Line 238: osetupcons.ConfigEnv.FIREWALL_MANAGER Line 239: ] not in [m.name for m in self._available_managers]: Line 240: raise RuntimeError( Line 250: m for m in self._available_managers Line 251: if m.name == self.environment[ Line 252: osetupcons.ConfigEnv.FIREWALL_MANAGER Line 253: ] Line 254: ).enable() Thanks. Line 255: Line 256: @plugin.event( Line 257: stage=plugin.Stages.STAGE_CLOSEUP, Line 258: before=( .................................................... File packaging/setup/plugins/ovirt-engine-setup/network/firewall_manager_firewalld.py Line 78: ), Line 79: footer_format=( Line 80: '</service>\n' Line 81: ), Line 82: ) Done Line 83: Line 84: self.environment[ Line 85: otopicons.NetEnv.FIREWALLD_SERVICE_PREFIX + Line 86: service.name .................................................... File packaging/setup/plugins/ovirt-engine-setup/network/firewall_services.py Line 118: except IndexError: Line 119: self.index = 0 Line 120: raise StopIteration Line 121: self.index += 1 Line 122: return result I thought it's a little bit nicer, no special reason. If you mean that this entire class is useless and that self.environment[FIREWALL_SERVICES] should simply be a list and that's it, you are also right - but it means that the class above (FirewallService) will have to be public and plugins will need to directly instantiate it somehow. By adding this class and add_service I partially hide this. Not sure if it's a good idea or not. Anyway, I am now dropping this per your request, so this discussion is currently academic. Still, insights are very welcome. Line 123: Line 124: @util.export Line 125: class Plugin(plugin.PluginBase): Line 126: """ Line 152: ) Line 153: def _customization(self): Line 154: # Populates FIREWALL_SERVICES from FIREWALLD_SERVICES. Line 155: # TODO: Make the various plugins append to FIREWALL_SERVICES Line 156: # directly and remove this function. Done Line 157: for firewalld_service in self.environment[ Line 158: osetupcons.NetEnv.FIREWALLD_SERVICES Line 159: ]: Line 160: content = osetuputil.processTemplate( -- To view, visit http://gerrit.ovirt.org/20737 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If3c1a634b2e8539ebd604205b5487290c8d8a1a9 Gerrit-PatchSet: 22 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yedidyah Bar David <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[email protected]> Gerrit-Reviewer: Ofer Schreiber <[email protected]> Gerrit-Reviewer: Sandro Bonazzola <[email protected]> Gerrit-Reviewer: Yedidyah Bar David <[email protected]> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
