Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix generated RouterOS script, clean up template #100

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

j3n57h0m45
Copy link
Contributor

After prologned testing of the generated MikroTik RouterOS script I became clear that the approach is not feasable and error prone for MikroTik devices such as the "hAP ac"

  1. add ip to address-list
  2. if it fails try to modify the respective entry in the address-list

Currently there is no efficient way to modify an ip address entry within an existing address-list (see also MikroTik Forum '/ip firewall address-list print where' is slow].

This only leaves us with this modified approach:

  1. leave existing address-list untouched
  2. add new ips to temporary address-list
  3. swap out existing address-list with the updated address-list

@LaurenceJJones what do you think?

@LaurenceJJones
Copy link
Contributor

After prologned testing of the generated MikroTik RouterOS script I became clear that the approach is not feasable and error prone for MikroTik devices such as the "hAP ac"

1. add ip to address-list

2. if it fails try to modify the respective entry in the address-list

Currently there is no efficient way to modify an ip address entry within an existing address-list (see also MikroTik Forum '/ip firewall address-list print where' is slow].

This only leaves us with this modified approach:

1. leave existing address-list untouched

2. add new ips to temporary address-list

3. swap out existing address-list with the updated address-list

@LaurenceJJones what do you think?

🤷🏻 I'm kind of relying on your knowledge on this part, if you say this is better then lets do it. I only released an RC so far so it hasnt propagated out to people yet, so lets make sure it optimal for all.

@j3n57h0m45 j3n57h0m45 marked this pull request as draft July 4, 2024 09:38
@j3n57h0m45
Copy link
Contributor Author

So lets keep it at RC for the time being. It might come down to the point where we just have to admit that eg. MIPSBE Architechture in these devices is just not meant for any periodic real time processing of 25k entry lists :/

@j3n57h0m45 j3n57h0m45 force-pushed the formatters/mikrotik branch 2 times, most recently from b1f10db to eed2dad Compare July 5, 2024 11:34
@j3n57h0m45
Copy link
Contributor Author

j3n57h0m45 commented Jul 5, 2024

Given the scope, sometimes the simplest solution is the most effective. When using on-device scripting, performance can be limited. Therefore, the original brute force method: deleting the address list and creating a new address list remains the most practical approach. By properly using the timeout parameter, we can avoid unnecessary logging events on the device, which helps to reduce the overall script execution time.

Testing Results with >22,000 entries

  • hAP ac -> ~2 minutes
  • RB5009 -> ~25 seconds

@j3n57h0m45 j3n57h0m45 marked this pull request as ready for review July 5, 2024 11:53
@j3n57h0m45
Copy link
Contributor Author

Is there anything I can help with to get this merged?

@LaurenceJJones
Copy link
Contributor

Is there anything I can help with to get this merged?

as long as you have tested and happy if there are blank lines then yeah we can merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants