Make LDAP client certificates optional/configurable and add new options for Bridge interface

[jangrewe]

We don't use a client cert for connecting to LDAP, and the CA certificate path didn't match on Ubuntu 16.04, so this makes it optional and configurable.

[conorsch]

Thanks for submitting this, @jangrewe. Thoughts, @nemesisdesign? Since the proposed changes politely respect current role default behavior, and are opt-in, I'm inclined to merge, but holler if you have feedback on these changes.

[nemesifier]

The line that changes the bridge_ports is not backward compatible. Existing playbooks would break after upgrading.

I suggest something like the following:

bridge_ports {% if 'ports' in openvpn_bridge %}{{ openvpn_bridge.ports }}{% else %}{{ openvpn_dev }}{% endif %}

[jangrewe]

Agreed, @nemesisdesign. I have commited a fix for this situation.

[nemesifier]

Thank you @jangrewe, I'm not 100% sure the syntax is correct, I'll leave an inline comment on the code.

[nemesifier]

Are you sure the - at the end is ok? Shouldn't it be {% endif %}, or is it a jinja2 feature I don't know?

[jangrewe]

The - removes the whitespace/newline, depending on where you put it.

[nemesifier]

👍

[nemesifier]

(this is just a style related comment)
even if it would be long, I would prefer having everything on oneline, the resulting configuration would look more readable, although I would have to test it, which I haven't yet. Have you tested it and if yes does the resulting configuration file look ok and works?

[jangrewe]

I know that the formatting is ugly this way, but otherwise you have the next line either indented one tabstop too much, or on the same line. If there was any way to force a newline character, i'd put it as a oneliner, too! =)

[nemesifier]

👍

[jangrewe]

The - removes the whitespace/newline, depending on where you put it. I know that the formatting is ugly this way, but otherwise you have the next line either indented one tabstop too much, or on the same line. If there was any way to force a newline character, i'd put it as a oneliner, too! =)

[nemesifier]

Thanks for the explaination, ok for me 👍

[conorsch]

Given this structure, the dict openvpn_bridge must contain a script list element. If that's truly a requirement, fine as-is, otherwise a |default([]) would come in handy here.

[nemesifier]

@conorsch you are right, I was not thinking about it. Yes, keeping backward compatibility is better, I would prefer it, what do you think @jangrewe?

[conorsch]

@nemesisdesign @jangrewe Can you both +1 here if you're satisfied for merge? I am not using bridged connections, but these changes look solid. If they work well for both of your uses cases, happy to include.

[nemesifier]

👍

[nemesifier]

Was this forgotten?

[jasiek]

Hey, any chance this could be merged in?

[nkakouros]

I don't have the time right now to go through this PR and resolve the conflicts, but if someone does, I would be glad to merge.