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

FRR Copy Running to Saved option for the raw config page. Issue #9003 #777

Merged
merged 1 commit into from Mar 9, 2020

Conversation

vktg
Copy link
Contributor

@vktg vktg commented Feb 23, 2020

Redmine Issue: https://redmine.pfsense.org/issues/9003
Ready for review

Would be possible to add these buttons to the raw config page as the Quagga package does, i'm slowly moving over to using FRR over Quagga for the new features being added but having to cat the /var/etc/frr/*.conf file and paste is into the UI for each daemon can be error prone.
As an improvement over the quagga a 'copy all' button would be excellent.

I do not understand why quagga-way (saving configuration in base64 format in config.xml) may be error prone. Both *.conf and config.xml contains same data.
This is the port from Quagga with a 'copy all' button.

@rbgarga rbgarga requested a review from jim-p February 24, 2020 13:27
} else {
$config['installedpackages']['frrglobalraw']['config'][0][$module . "running"] = base64_encode("!!!!! {$module}.conf does not exist or is empty.");
}
write_config( $desc = gettext("FRR: Wrote {$module}.conf startup-config to pfSense config file."), $backup = false, $write_config_only = true );
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this setting variables by name here in the function call? Seems like it could be problematic. If this was copied directly over from quagga I suspect this may also be why on quagga I see lots of config writes which update the running config with a broken description of (system): 1.

This will need tested and maybe fixed both here and in quagga.

Copy link
Contributor

Choose a reason for hiding this comment

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

When I wrote that function for Quagga, the idea of the variable being part of the function call was to reduce/simplify the code, allowing the function to be used to write the config for any module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed & rebased

Copy link
Contributor

@jim-p jim-p left a comment

Choose a reason for hiding this comment

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

The more I think about it, this should be optional and off by default. This is going to cause as many as five extra config writes any time the package resyncs.

The function should not do a write config itself, it should do a single write at the end, and it should only be done if the user enabled the function to do so. For example if you're using the GUI to manage settings, this is completely unnecessary. It only makes sense if someone is using a manually added config and maybe making changes in vtysh occasionally.

And the same logic should also be added to quagga so it doesn't make these excessive writes (and has the save description fixed)

@vktg
Copy link
Contributor Author

vktg commented Mar 5, 2020

Now it uses 'Update RUNNING' button on the frr_global_raw.xml page to put base64-encoded configs to config.xml

@netgate-git-updates netgate-git-updates merged commit f4b0900 into pfsense:devel Mar 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants