Skip to content

[th/zone-lookup] some cleanup of the get_zones() functions etc.#1176

Open
thom311 wants to merge 14 commits intofirewalld:mainfrom
thom311:th/zone-lookup
Open

[th/zone-lookup] some cleanup of the get_zones() functions etc.#1176
thom311 wants to merge 14 commits intofirewalld:mainfrom
thom311:th/zone-lookup

Conversation

@thom311
Copy link
Collaborator

@thom311 thom311 commented Jul 20, 2023

The problem I want to solve, is that the key for policies/zones is p_obj.name, a plain string

Note that for user-created zones we generate 4 policies like zone_foo_ANY and zone_HOST_foo.

The API does not prevent users from creating a policy with name zone_foo_ANY or a zone with name ANY. Bad things happen, if they do.

The eventual solution will be, that the primary key is not a plain string, but a type+name. In particular, there is a difference between user-created policy zone_foo_ANY and a policy zone_foo_ANY for zone foo. And there is a difference between user-created zone ANY and the special, internal ANY zone.

Anyway. Replacing the "name" with an identifier will touch a lot of code. In preparation for that, this branch cleans up some minor things related to zone/policy lookup (check_zone(), get_zone(), get_policies()).

@thom311 thom311 marked this pull request as draft July 20, 2023 11:13
@thom311 thom311 force-pushed the th/zone-lookup branch 9 times, most recently from 0526dad to 3ea8eba Compare July 21, 2023 18:04
@thom311 thom311 marked this pull request as ready for review July 21, 2023 18:06
erig0
erig0 requested changes Jul 28, 2023
View reviewed changes
Copy link
Collaborator

@erig0 erig0 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, I feel like a lot of these changes are unnecessary. I'm not sure they improve code quality or the code efficiency. Some of the changes to use dictionary lookups vs lists are fine. I do not intend to merge this series without major changes.

@thom311 thom311 force-pushed the th/zone-lookup branch 2 times, most recently from 5696b9b to a7f2f34 Compare August 3, 2023 11:28
erig0
erig0 requested changes Sep 6, 2023
View reviewed changes
@thom311 thom311 requested a review from erig0 January 22, 2024 16:07
@thom311
Copy link
Collaborator Author

thom311 commented Jan 26, 2024

Hi @erig0

nftables does:

    def _set_get_family(self, name):
        ipset = self._fw.ipset.get_ipset(name)

where get_ipset() does

    def get_ipset(self, name, applied=False):
        self.check_ipset(name)
        obj = self._ipsets[name]
        if applied:
            self.check_applied_obj(obj)
        return obj

where check_ipset() does

    def check_ipset(self, name):
        if name not in self.get_ipsets()
            raise FirewallError(errors.INVALID_IPSET, name)

where get_ipsets() does:

    def get_ipsets(self):
        return sorted(self._ipsets.keys())

making the entire operation needlessly O(n).

This branch fixes this, among other things. What is the problem with this?

@thom311
improvement(zone): cleanup check_zone() function
1e44207 
Firewall.check_zone() used to call "if _zone not in self.zone.get_zones()", where
get_zones() literally needs to create a list of all zone names,
needlessly sort them, and then we search the string by iterating the
list. All this, when self.zone._zones is already a dictionary with the
zone name as key. Do a dictionary lookup instead.

Note that FirewallZone.get_zone() already was calling Firewall.check_zone()
to resolve the default name (thereby, iterating the list of all zones
too). So even a function like get_zone(), which seemingly does a
dictionary lookup, first iterates the entire list.

Rework this. Firewall.check_zone() now just calls FirewallZone.get_zone()
(using a dictionary lookup).

FirewallZone.get_zone() can resolve the default name directly, without
calling Firewall.check_zone(). It can raise the appropriate exception,
if the zone does not exist.
@thom311
improvement(zone): use FirewallZone.check_zone() function
62bb7a8
@thom311
improvement(ipset): cleanup check_ipset() function
8b38caf
@thom311
improvement(helper): cleanup check_helper() function
e56371b
@thom311
improvement(zone): use has_zone() instead of "in get_zones()" to chec…
f577a1d 
…k for zone

has_zone() just does a dictionary lookup. get_zones() creates a sorted
list of all zones first and then iterates the list. Use has_zone().
@thom311
improvement(zone): cleanup FirewallZone.create_zone_base_by_chain()
6956cc7 
- don't call self.policy_from_chain() twice.

- return early from function and unindent the block.
@thom311
improvement(policy): improve check_policy() to avoid unnecesssary lookup
a578232
@thom311
improvement(zone): use FirewallPolicy.check_policy() function
0909257
@thom311
improvement(policy): simplify get_policies_not_derived_from_zone()
05626c7 
Don't iterate ove the keys of the _policies dictionary, to then lookup
the object.

Also, sorted() creates a new list. We already had a temporary list policies,
which we could have sorted inplace with policies.sort(). But replace this
with a generator expression, which avoids the intermediate list.
@thom311
improvement(policy): simplify FirewallPolicy.apply_policies() to not …
ecaa448 
…iterate policies O(n^2)

This was not necessary.

This change also renders get_policies() without callers. Delete that.
@thom311
improvement(policy): merge implementations of get_policies() getters
ec288a4 
Also, for checking whether a policy is active, avoid initializing the
same sets over and over.
@thom311
improvement(policy): simplify check for active policy
524b791 
No need to iterate the whole list of active policies.
Just check whether the policy in question is active.
@thom311
improvement(icmp): drop redundant check_icmptype() functions
e807fac 
Code does not get simpler, by having aliases that do mostly the same
and call each other. Calling self._fw.icmp.check_icmptype() is
perfectly fine. Use that.
@thom311
improvement(service): drop redundant check_service() functions
a681f7f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@erig0 erig0 Awaiting requested review from erig0

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@thom311 @erig0