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

net/haproxy: version 2.0 #330

Merged
merged 10 commits into from
Oct 29, 2017
Merged

net/haproxy: version 2.0 #330

merged 10 commits into from
Oct 29, 2017

Conversation

fraenki
Copy link
Member

@fraenki fraenki commented Oct 22, 2017

New features

Bugfixes


Tasks related to #208:

  • model refactoring
  • JS magic to hide unused GUI elements
  • rename/rephrase GUI elements
  • restructure GUI elements
  • add multiple introduction pages (tiny tutorials)

Follow-up tasks:

@fraenki fraenki self-assigned this Oct 22, 2017
@fraenki fraenki added the feature Adding new functionality label Oct 22, 2017
@fichtner
Copy link
Member

great, thanks for this!

just a small question: will the models be compatible and if not, can they be migrated?

@fraenki
Copy link
Member Author

fraenki commented Oct 22, 2017

just a small question: will the models be compatible and if not, can they be migrated?

It's an incompatible change, but I've added the migration script M2_0_0.php for this purpose.

@fraenki fraenki changed the title [WIP] net/haproxy: extensive model refactoring [WIP] net/haproxy: extensive refactoring Oct 22, 2017
@fichtner
Copy link
Member

superb 👍

@fraenki fraenki mentioned this pull request Oct 22, 2017
7 tasks
@fraenki fraenki changed the title [WIP] net/haproxy: extensive refactoring net/haproxy: extensive refactoring for plugin version 2.0 Oct 22, 2017
@fraenki
Copy link
Member Author

fraenki commented Oct 22, 2017

Hi all, it's a huge change, so I hope some of you have some time for a review. :)
Please review with #208 in mind.

@fraenki
Copy link
Member Author

fraenki commented Oct 23, 2017

@fichtner Would it be possible for you to validate the wording/phrasing in the new GUI? This time I'd like to make sure that the GUI doesn't "hurt" native speakers ;)

Copy link
Member

@fichtner fichtner left a comment

Choose a reason for hiding this comment

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

as requested :)

<type>select_multiple</type>
<style>tokenize</style>
<help><![CDATA[Select one ore more ACLs to be used as condition for this action.]]></help>
<help><![CDATA[Select one ore more conditions to be used for this action.]]></help>
Copy link
Member

Choose a reason for hiding this comment

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

"or"

<type>dropdown</type>
<help><![CDATA[Choose an logical operator to be used to form a condition.]]></help>
<help><![CDATA[Choose an logical operator.]]></help>
Copy link
Member

Choose a reason for hiding this comment

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

"a logical"

<type>dropdown</type>
<help><![CDATA[Choose an action that should be executed if the condition is true.]]></help>
<help><![CDATA[Choose an HAProxy function that should be executed if the condition evaluates to true.]]></help>
Copy link
Member

Choose a reason for hiding this comment

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

"a HAProxy"

</field>
<field>
<id>action.type</id>
<label>Choose action</label>
<label>Run function</label>
Copy link
Member

Choose a reason for hiding this comment

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

Would go with "Execute function" as well, or "Function to execute"

<id>action.http-request_redirect</id>
<label>HTTP Redirect</label>
<type>text</type>
<help><![CDATA[Use HAProxy's powerful redirect function to return a HTTP redirection. See <a href="http://cbonte.github.io/haproxy-dconv/1.7/configuration.html#redirect">HAProxy's documentation</a> for further details and examples.]]></help>
Copy link
Member

Choose a reason for hiding this comment

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

Links in the help text will only make me through out all translations for it as mistakes are easily made by translators. "powerful" is debatable, from a pure documentation perspective we can live without it. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

The syntax for the redirect function is not self-explaining (to say the least). I plan to add a GUI for it, but until then I think the link is required. :-/

<id>action.http-request_lua</id>
<label>Lua function</label>
<type>text</type>
<help><![CDATA[Run the specified Lua function. You will most likely need to include/load your Lua code first.]]></help>
Copy link
Member

Choose a reason for hiding this comment

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

Now you're using "run" again.

<id>action.http-request_add-header_content</id>
<label>Header Content</label>
<type>text</type>
<help><![CDATA[The value that should be set for the specified HTTP header. Note that it's possible to use pre-defined variables, see <a href="http://cbonte.github.io/haproxy-dconv/1.7/configuration.html#8.2.4">HAProxy's documentation</a> for further details and examples.]]></help>
Copy link
Member

Choose a reason for hiding this comment

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

As a general note, technical documentation tries to avoid contractions as much as possible: "it's" becomes "it is" and so on

<id>action.http-request_replace-header_regex</id>
<label>Regular Expression</label>
<type>text</type>
<help><![CDATA[Matches the specified regular expression in all occurrences of header field.]]></help>
Copy link
Member

Choose a reason for hiding this comment

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

"of the header field"

<type>text</type>
<help><![CDATA[Specify a value to match with the action.]]></help>
<help><![CDATA[Run the specified Lua function. You will most likely need to include/load your Lua code first.]]></help>
Copy link
Member

Choose a reason for hiding this comment

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

"run" vs. "execute"

@@ -154,31 +160,31 @@
<id>frontend.customOptions</id>
<label>Option pass-through</label>
<type>textbox</type>
<help><![CDATA[These lines will be added to the HAProxy frontend configuration.<br/><div class="text-info"><b>NOTE:</b> The syntax will not be checked, use at your own risk!</div>]]></help>
<help><![CDATA[These lines will be added to the HAProxy frontend configuration.<br/><div class="text-info"><b>NOTE:</b> This is strongly discouraged! The syntax will not be checked, use at your own risk!</div>]]></help>
Copy link
Member

Choose a reason for hiding this comment

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

Telling users not to do something in a hidden help text does not help. It may be better to ditch this field or simplify the help messages to the minimum.

Copy link
Member Author

Choose a reason for hiding this comment

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

The option pass-through is sort-of a last resort in case a function is missing. I'll try to move these warnings to the "Introduction" page(s).

@fraenki
Copy link
Member Author

fraenki commented Oct 23, 2017

@fichtner Thank you, I'll fix these issues ASAP! Would you do the same for the new "Quick Start Guide" and "Introduction" pages/tabs that I've added to index.volt? Pleeease :)

Copy link
Member

@fichtner fichtner left a comment

Choose a reason for hiding this comment

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

here you go

<div class="col-md-12">
<h1>Quick Start Guide</h1>
<p>{{ lang._('Welcome to the HAProxy plugin! This plugin is designed to offer all the features and flexibility HAProxy is famous for. If you are using HAProxy for the first time, please take some time to get familiar with it. The following information should help you to get started.')}}</p>
<p>{{ lang._('Note that you should configure everything in the following order:') }}</p>
Copy link
Member

Choose a reason for hiding this comment

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

"everything" should read "HAProxy"

<p>{{ lang._('Welcome to the HAProxy plugin! This plugin is designed to offer all the features and flexibility HAProxy is famous for. If you are using HAProxy for the first time, please take some time to get familiar with it. The following information should help you to get started.')}}</p>
<p>{{ lang._('Note that you should configure everything in the following order:') }}</p>
<ul>
<li>{{ lang._('Add %sReal Servers:%s All physical or virtual servers that you want HAProxy to use should be added here.') | format('<b>', '</b>') }}</li>
Copy link
Member

Choose a reason for hiding this comment

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

"Real" is introduced here but other pages only use "Servers". You could go to the extreme with this an call stuff "Servers", "Pools", "Services" ;)

Also, it's not perfectly clear what this means, maybe better to reword like "that you want HAPproxy to use" to "that HAProxy to serve to" ?

Copy link
Member Author

@fraenki fraenki Oct 23, 2017

Choose a reason for hiding this comment

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

@fichtner OK to replace "that you want HAProxy to use should be added here" with "that HAProxy should use for load balancing or proxying must be added here"?

Copy link
Member

Choose a reason for hiding this comment

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

yes and no, the direction should be clearer e.g. "load balance between or proxy to"

<p>{{ lang._('Note that you should configure everything in the following order:') }}</p>
<ul>
<li>{{ lang._('Add %sReal Servers:%s All physical or virtual servers that you want HAProxy to use should be added here.') | format('<b>', '</b>') }}</li>
<li>{{ lang._('Add %sBackend Pools:%s Group the previously added servers to build a server farm. All servers in a group usually deliver the same content. The Backend Pool cares for health monitoring and load distribution. A Backend Pool must also be configured if you only have a single server.') | format('<b>', '</b>')}}</li>
Copy link
Member

Choose a reason for hiding this comment

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

"cares" -> "takes care of"

"A Backend Pool must also be configured if you only have a single server." -> "A Backend Pool must be configured even if you only have a single server."

<ul>
<li>{{ lang._('Add %sReal Servers:%s All physical or virtual servers that you want HAProxy to use should be added here.') | format('<b>', '</b>') }}</li>
<li>{{ lang._('Add %sBackend Pools:%s Group the previously added servers to build a server farm. All servers in a group usually deliver the same content. The Backend Pool cares for health monitoring and load distribution. A Backend Pool must also be configured if you only have a single server.') | format('<b>', '</b>')}}</li>
<li>{{ lang._('Add %sPublic Services:%s The Public Service listens for client connections, optionally applies rules and forwards client request data to the selected Backend Pool for load balancing (or proxying).') | format('<b>', '</b>') }}</li>
Copy link
Member

Choose a reason for hiding this comment

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

stuff in parenthesis: is it important? if yes, drop parenthesis, if no drop its content.

<li>{{ lang._('Add %sReal Servers:%s All physical or virtual servers that you want HAProxy to use should be added here.') | format('<b>', '</b>') }}</li>
<li>{{ lang._('Add %sBackend Pools:%s Group the previously added servers to build a server farm. All servers in a group usually deliver the same content. The Backend Pool cares for health monitoring and load distribution. A Backend Pool must also be configured if you only have a single server.') | format('<b>', '</b>')}}</li>
<li>{{ lang._('Add %sPublic Services:%s The Public Service listens for client connections, optionally applies rules and forwards client request data to the selected Backend Pool for load balancing (or proxying).') | format('<b>', '</b>') }}</li>
<li>{{ lang._('Finally enable HAProxy using the %sService Settings%s.') | format('<b>', '</b>') }}</li>
Copy link
Member

Choose a reason for hiding this comment

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

"Lastly, enable"

<div id="subtab_haproxy-virtual-services-introduction" class="tab-pane fade">
<div class="col-md-12">
<h1>Virtual Services</h1>
<p>{{ lang._("HAProxy requires two virtual services for it's loadbalancing and proxying features. The following virtual services must be configured for everything that should be served by HAProxy:") }}</p>
Copy link
Member

Choose a reason for hiding this comment

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

"its"

<p>{{ lang._("HAProxy requires two virtual services for it's loadbalancing and proxying features. The following virtual services must be configured for everything that should be served by HAProxy:") }}</p>
<ul>
<li>{{ lang._('%sBackend Pools:%s The HAProxy backend. Group the %spreviously added servers%s to build a server farm. All servers in a group usually deliver the same content. The Backend Pool cares for health monitoring and load distribution. A Backend Pool must also be configured if you only have a single server. The same Backend Pool may be used for multiple Public Services.') | format('<b>', '</b>', '<b>', '</b>') }}</li>
<li>{{ lang._('%sPublic Services:%s The HAProxy frontend. The Public Service listens for client connections, optionally applies rules and forwards client request data to the selected Backend Pool for load balancing (or proxying). Every Public Service needs to be connected to a %spreviously created Backend Pool%s.') | format('<b>', '</b>', '<b>', '</b>') }}</li>
Copy link
Member

Choose a reason for hiding this comment

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

parenthesis :D

<div id="subtab_haproxy-rules-checks-introduction" class="tab-pane fade">
<div class="col-md-12">
<h1>Rules &amp; Checks</h1>
<p>{{ lang._("These are all optional features, HAProxy can be used without them. After getting used to HAProxy you'll find these optional features pretty useful:") }}</p>
Copy link
Member

Choose a reason for hiding this comment

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

"features, " becomes "features; " or dop the second part of the sentence, also contraction -> you will, remove "pretty"

In a cold technical manner, the sentences could simply read: "After getting acquainted with HAProxy the following optional features may prove useful."

<p>{{ lang._("These are all optional features, HAProxy can be used without them. After getting used to HAProxy you'll find these optional features pretty useful:") }}</p>
<ul>
<li>{{ lang._('%sHealth Monitors:%s The HAProxy "health checks". Health Monitors are used by %sBackend Pools%s to determine if a server is still able to respond to client requests. If a server fails a health check it will automatically be removed from a Backend Pool and healthy servers are automatically re-added.') | format('<b>', '</b>', '<b>', '</b>') }}</li>
<li>{{ lang._('%sConditions:%s HAProxy is capable of extracting data from requests, responses and other connection data and match it against predefined patterns. Use these powerful patterns to compose a condition that may be used in (multiple) Rules.') | format('<b>', '</b>') }}</li>
Copy link
Member

Choose a reason for hiding this comment

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

parenthesis!

<ul>
<li>{{ lang._('%sHealth Monitors:%s The HAProxy "health checks". Health Monitors are used by %sBackend Pools%s to determine if a server is still able to respond to client requests. If a server fails a health check it will automatically be removed from a Backend Pool and healthy servers are automatically re-added.') | format('<b>', '</b>', '<b>', '</b>') }}</li>
<li>{{ lang._('%sConditions:%s HAProxy is capable of extracting data from requests, responses and other connection data and match it against predefined patterns. Use these powerful patterns to compose a condition that may be used in (multiple) Rules.') | format('<b>', '</b>') }}</li>
<li>{{ lang._('%sRules:%s Perform a large set of actions if one or more %sConditions%s match. These Rules may be used in %sBackend Pools%s as well as %sPublic Services%s.') | format('<b>', '</b>', '<b>', '</b>', '<b>', '</b>', '<b>', '</b>') }}</li>
Copy link
Member

Choose a reason for hiding this comment

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

The markup is, err, extensive... it may kill the reading flow.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, these were supposed to be links in the first place, but I couldn't figure out how to properly set the anchor links ;)

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

$acl->value = NULL;
break;
default:
// echo "DEBUG: no ACL migration required: " . (string)$acl->name . "\n";
Copy link
Member

Choose a reason for hiding this comment

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

unused: remove it

$hc->smtpDomain = NULL;
break;
default:
// echo "DEBUG: no Healthcheck migration required: " . (string)$hc->name . "\n";
Copy link
Member

Choose a reason for hiding this comment

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

remove it

fraenki added a commit to fraenki/plugins that referenced this pull request Oct 23, 2017
fraenki added a commit to fraenki/plugins that referenced this pull request Oct 23, 2017
fraenki added a commit to fraenki/plugins that referenced this pull request Oct 23, 2017
fraenki added a commit to fraenki/plugins that referenced this pull request Oct 23, 2017
@fraenki
Copy link
Member Author

fraenki commented Oct 26, 2017

I've tested the model migration with two rather complex models and found no issues. A diff of haproxy.conf revealed no difference. Yay :)

@fichtner
Copy link
Member

\o/

@fraenki fraenki removed the request for review from AdSchellevis October 28, 2017 12:56
@fraenki fraenki changed the title net/haproxy: extensive refactoring for plugin version 2.0 net/haproxy: version 2.0 Oct 29, 2017
@fraenki fraenki merged commit 6208670 into opnsense:master Oct 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Adding new functionality
Development

Successfully merging this pull request may close these issues.

3 participants