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

Configuration Sync - Initial commit into the OPNsense-plugins repo #777

Closed
wants to merge 29 commits into from

Conversation

ndejong
Copy link

@ndejong ndejong commented Aug 6, 2018

Submitting plugin named "Configuration Sync" which, as per the README

Configuration Sync is a tool designed to one-way synchronize the system configuration files from the OPNsense host to S3 compatible object data storage in close to real time. While the tool has the effect of being a great configuration backup tool the intent is to provide a tool that stores the OPNsense system configuration in a location that is readily addressable using DevOps automation tools such as Terraform.
The ability to start an OPNsense instance using automation tools means OPNsense becomes a first-class choice for building and managing network infrastructure within cloud-compute providers.

The use case I am seeking to address here is the requirement to always (as best possible) have the latest configuration file stored in S3 so that an OPNsense instance can be destroyed without (much) concern for the system state and then re-create it using an automation tool (such as Terraform) - I acknowledge there is still an open question on how to deal with required plugins.

I'm left wondering if we could be so very bold enough to propose a new top-level menu called "Cloud" under which this plugin might exist along with other components that assist in making OPNsense work in such environments live - as part of another project (see our verbnetworks git repo) we have tools to generate images suitable for AWS and Digital Ocean, these tools rely on syshook scripts to tweak the config.xml files at boot to make the system work in such environments - these tools should perhaps also exist as plugins and would "fit" nicely under such a "Cloud" top-level menu.

Feedback, suggestions all welcome.

@ndejong
Copy link
Author

ndejong commented Aug 6, 2018

Also - this plugin requires commit opnsense/core@68f0559 else the responseMsg dialogue will not update when pressing the "Test Credentials" button with a message stuck at "Running tests..."

@fichtner fichtner self-requested a review August 6, 2018 11:07
@fichtner
Copy link
Member

fichtner commented Aug 6, 2018

Hi @ndejong,

opnsense/core@68f0559 will be in 18.7.1. I'll find a bit of review time this week.

Cheers,
Franco

@ndejong
Copy link
Author

ndejong commented Aug 8, 2018

I can already see some dead code and a ControllerUtils.php file that needs to be tidied up within the UI controller - happy enough to receive basic high-level feedback to start with before we get into the nut-n-bolts if that helps getting started with this one - N

Copy link
Member

@fabianfrz fabianfrz left a comment

Choose a reason for hiding this comment

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

some findings

$data['settings']['ProviderKey'],
$data['settings']['ProviderSecret'],
$data['settings']['StorageBucket'],
$data['settings']['StoragePath']
Copy link
Member

Choose a reason for hiding this comment

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

does this need escaping?

Copy link
Author

Choose a reason for hiding this comment

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

Yes - these represent user-input so they absolutely do need to be escaped - I'd naively wrapped the inputs in double quotes which could indeed be "broken out of" with an input arg itself containing a " double-quote mark - note to self, remember that PHP has a escapeshellarg() function...

if (file_exists('/etc/hostid')) {
$hostid = trim(file_get_contents('/etc/hostid'));
}
return $hostid;
Copy link
Member

Choose a reason for hiding this comment

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

you can also directly return - the variable is not needed.

Copy link
Author

Choose a reason for hiding this comment

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

Done

<label>Storage Bucket</label>
<type>text</type>
<help><![CDATA[Storage provider bucket name to be used.<br/>
<b>NOTE:</b> the storage-bucket must exist prior to use, the Configuration Sync feature <u>will not</u> attempt to create the bucket for you.]]></help>
Copy link
Member

Choose a reason for hiding this comment

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

we try to avoid note: prefixes whenever possible.

Are you sure you want to use this style as it is deprecated: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/u

same also later in this file

Copy link
Author

@ndejong ndejong Aug 13, 2018

Choose a reason for hiding this comment

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

Did not know about <u> thanks - and note: fields have been removed


<ProviderKey type="TextField">
<Required>Y</Required>
<Mask>/^([a-zA-Z0-9]){16,128}$/u</Mask>
Copy link
Member

Choose a reason for hiding this comment

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

the i attribute makes the regex not case sensitive

Copy link
Author

Choose a reason for hiding this comment

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

Oh the irony, story for another day - http://sensorynetworks.com - but yes, sloppy regex dealt with


<ProviderSecret type="TextField">
<Required>Y</Required>
<Mask>/^([a-zA-Z0-9\+\/]){16,128}$/u</Mask>
Copy link
Member

Choose a reason for hiding this comment

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

same here


/**
* $(document).ready
*/
Copy link
Member

Choose a reason for hiding this comment

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

remove the comment

Copy link
Author

Choose a reason for hiding this comment

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

Done

});

$("#testAction").click(function(){
$("#responseMsg").removeClass("hidden").removeClass("alert-danger").addClass('alert-info').html("Running tests...");
Copy link
Member

Choose a reason for hiding this comment

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

gettext (lang._)

Copy link
Author

Choose a reason for hiding this comment

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

Yes and thanks, too easy to forget - Done

callback_ok=function(){
$("#responseMsg").html("Configuration Sync service settings saved.");
ajaxCall(url="/api/configsync/service/reload", sendData={}, callback=function(data, status) {
$("#responseMsg").html("Configuration Sync service settings saved and reloaded.");
Copy link
Member

Choose a reason for hiding this comment

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

lang._

Copy link
Author

Choose a reason for hiding this comment

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

.. and caught a few others - Done

});

});
</script>
Copy link
Member

Choose a reason for hiding this comment

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

the script should have spaces around the operators etc. like a = b

Copy link
Author

Choose a reason for hiding this comment

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

Done

@@ -0,0 +1,29 @@
Copyright (c) 2012-2013 Paul Tax <paultax@gmail.com> All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

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

why not pull it in as a dependency?

Copy link
Author

Choose a reason for hiding this comment

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

Do you mean pull in as a git submodule?

Copy link
Member

Choose a reason for hiding this comment

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

no as a plugin dependency if possible like: https://github.com/opnsense/plugins/blob/master/www/nginx/Makefile#L4

Copy link
Author

Choose a reason for hiding this comment

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

Cool - thanks for explaining

A few things drove me to manually include these Python modules:-

  • these modules are not available via the pkg system (double checked, shout out to @fichtner because I made an embarrassing mistake about the autossh package last week)
  • the OPNsense system does not make available a pip Python package management system (leads to arbitrary code etc.)
  • as best I can tell the supplied functionality is not covered by any other existing Python module available on the system

I'm not expert with FreeBSD or OPNsense packaging, I could easily be wrong, but it seems dependencies added via the PLUGIN_DEPENDS= attribute of a Makefile need to be available via the system packaging system - which in this case does not apply.

Is there a better way that I'm missing?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this are dependent pkg files. Are the libs also not available in rhe ports repository? If they are included, a pkg can be built.

Copy link
Author

Choose a reason for hiding this comment

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

Hmm, okay so a bit of research turns up the following

requestsaws

  • this Python module is not available via the FreeBSD ports tree
  • there is an alternative module which looks to be better py27-aws-requests-auth-0.4.0 that is available via FreeBSD ports
  • I'm happy to migrate to that library if OPNsense team are interested in adopting this library, however if we are adding packages then an altogether better option might be py27-boto-2.48.0

xmltodict

  • this Python module is available via the FreeBSD ports tree as pkg py27-xmltodict-0.11.0
  • the package maintainer is not the same individual as the author however the change logs point to the same martinblech github source.

I don't know the OPNsense vetting process for accepting packages into the release, as I understand it to make these happen they will need to be added to the OPNsense ports.conf

Copy link
Member

Choose a reason for hiding this comment

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

this is probably the maintainer of the port (freebsd package) - in this case usually not the developer of the software. Using Ports has multiple advantages:

  • better updates (libs can be updated independently)
  • keeps the plugins clean

you only have to wait for it until the next opnsense update is out or build it by yourself.

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

@fichtner this is for you

@ndejong
Copy link
Author

ndejong commented Aug 14, 2018

Have addressed the first round, really appreciate the time and attention @fabianfrz

Two things that require some assistance with:-

  • the item "why not pull it in as a dependency?"
  • the anonymous function wrapping the change() callback event.

})
);
} else {
$('#configsync\\.settings\\.StorageProviderLink').html("{{ lang._('Please provide') }} <b>Storage Bucket</b> {{ lang._('and') }} <b>Storage Path</b> {{ lang._('values') }}");
Copy link
Member

Choose a reason for hiding this comment

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

please don't split this string, it breaks translations when a different grammar is in use. Use |format() instead to keep the tags out of the translated string.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks - I was wondering how the grammar was going to get handled

@@ -1,6 +1,6 @@
PLUGIN_NAME= configsync
PLUGIN_VERSION= 0.1
PLUGIN_REVISION= 3
PLUGIN_REVISION= 4
Copy link
Member

Choose a reason for hiding this comment

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

If 3 was not released, you don't need to bump.

Copy link
Author

Choose a reason for hiding this comment

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

Hmm - just implemented today something that auto-magically incremented this number each time through our internal package build system - will stop that behavior and peg this at revision 5 for the time being other wise this number is going backwards which "feels" wrong.

Reference perhaps for others, a list of plugin Makefile build flags can be discovered like so.

opnsense-plugins$ find . -type f -name Makefile -exec grep ^PLUGIN {} \; | cut -d'=' -f1 | sort -u
PLUGIN_COMMENT
PLUGIN_CONFLICTS
PLUGIN_DEPENDS
PLUGIN_DEVEL
PLUGIN_DIRS+
PLUGIN_MAINTAINER
PLUGIN_NAME
PLUGIN_REVISION
PLUGIN_VERSION
PLUGIN_WWW

@fabianfrz
Copy link
Member

@fichtner the PHP / JS is ok, you can handle the rest.

@ndejong
Copy link
Author

ndejong commented Sep 11, 2018

Hi @fichtner - anything I can do to help this along for you?

@fichtner fichtner self-assigned this Nov 13, 2018
@fichtner
Copy link
Member

@ndejong We should work on inclusion for one plugin first... this one first or #818 ? This one has one third fewer lines to review and is older in submission date.

@ndejong
Copy link
Author

ndejong commented Nov 14, 2018

@ndejong We should work on inclusion for one plugin first... this one first or #818 ? This one has one third fewer lines to review and is older in submission date.

Lets work on this #777 first as it is easier - the other has a range of bugs and issues that I've discovered since the last push and perhaps need some guidance on, particularly around logging - I'll follow that up on that ticket.

WRT namespaces: you've mentioned namespace in the other project and I expect the same comments apply here - Earlier in the year when I started this plugin and after reading the samples and dev docs it was still an open question for me that felt a bit weird so I emailed project@opnsense about it - response was that other namespaces where okay but many people just chose the default OPNsense - perhaps a paragraph in the dev docs would help clear this up.

No problem if you'd like me to revise and have this migrated into the OPNsense namespace - let me know either way.

@fichtner
Copy link
Member

This hasn't been moving since 2018 so after internal discussion this can no longer be included in the current form. Also we are still concerned about sizeable first-time contributions as our community review time is not unlimited.

Cheers,
Franco

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

Successfully merging this pull request may close these issues.

None yet

3 participants