Adds new SSL && protocol specific directives to mailhost setup #769

dol · 2016-02-22T15:43:03Z

The SSL setup customisation of a mailhost is very limited. To allow
more directives to be set, the ssl configuration was extracted and
arranged like the setup for a vhost.
The setup was also expanded to customize protocol specific authentication
method and capabilities.

The SSL setup customisation of a mailhost is very limited. To allow more directives to be set, the ssl configuration was extracted and arranged like the setup for a vhost. The setup was also expanded to customize protocol specific authentication method and capabilities.

…ific options in mailhost setup

…epend, in mailhost

bastelfreak · 2016-10-10T13:50:37Z

Hi @dol, can you please rebase?

wyardley · 2016-10-12T16:57:38Z

btw, I also made a change in #909 that updates the default ciphers for the module, and has the default ciphers in the mailhost manifest inherited from nginx::config, which I think it is how it should be. Can you integrate that change once it's merged?

dol · 2016-10-14T19:36:44Z

@wyardley @bastelfreak Will perform an adjustment and rebase in the upcoming week.

wyardley · 2016-10-17T21:02:57Z

Thanks @dol
Note also that we prefer to have the commits squashed down to one (though if you want to leave a commit for the other author, that's fine).

wyardley · 2016-10-17T21:06:11Z

#909 is now merged. See also #930, which was also merged, and takes $ssl_protocols's default from nginx::config.

wyardley · 2016-10-20T19:57:53Z

@dol: just a reminder to rebase / squash if you've got the time.

dol · 2016-10-21T23:34:17Z

@wyardley I started with the rebase. Due to the divergency of the repo since February a rebase isn't that easy. Most of the work is done. But the test cases need some rearrangement. I'll keep you posted.
I also have to take care that the RP #909 and #930 aren't lost due to rearrangement/split up of the template. See the code comments in the stated PR's.

wyardley · 2016-10-22T04:28:40Z

Thanks for looking at it, yeah, there have been a lot of changes, so sorry that the conflicts will be a bit of a pain to resolve!

It looks like I didn't notice that mailhost.erb had the SSL thing in #930 for the case where STARTTLS is enabled; that's a good catch. Seems almost like (even if they're under separate listeners), the SSL parameters should just be generated once (maybe an additional template that gets included twice?) to keep things in sync and to make it more DRY, but it's fine to just sync them up for now.

wyardley · 2016-10-22T04:29:39Z

ps - Feel free to ping me or @bastelfreak on #voxpupuli in Puppet Slack or on IRC if you want to chat about it more.

dol · 2016-10-24T16:20:23Z

I merged and updated the PR. Next steps are:

Rebase into one commit
Check the code and review it with my coworker

@wyardley The unification of the common parts are part of the initial PR. See https://github.com/voxpupuli/puppet-nginx/pull/769/files#diff-a9378fe4f575a8c57d256e37d135f6ccR38

wyardley · 2016-10-24T17:35:29Z

@dol Thanks for the hard work! Will take a look over the code as well and see if anything sticks out.

Note that the tests are passing except the rubocop; there are some warnings there that you'll need to resolve before we can merge.
https://github.com/voxpupuli/puppet-nginx/blob/master/.github/CONTRIBUTING.md has a bit more info on that, but basically, bundle exec rake rubocop shouldn't report any failures.

Looking at the commits, it looks like maybe an unrelated commit made it in? If it ends up being too difficult to squash, you can also make a new branch and create a new PR off of that one.

wyardley · 2016-10-24T17:38:33Z

spec/defines/resource_mailhost_spec.rb

        },
        {
          title: 'should not enable IPv6',
          attr: 'ipv6_enable',
          value: false,
-          notmatch: %r{  listen \[::\]:80 default ipv6only=on;}
+          notmatch: %r{  listen                \[::\]:80 default ipv6only=on;}
        },


Is it possible to keep the spacing as it was before? In general, the module seems to use just a space or two after parameters, so I think it will be a little inconsistent if the spacing changes here. If we change the way we handle spacing, we'll probably need to do it on a module-wide basis.

Oh, nevermind, I see that it's already indented in the existing ones.

wyardley · 2016-10-24T17:42:41Z

spec/defines/resource_mailhost_spec.rb

            end
          end
        end
      end
    end

-    describe 'mailhost template content (SSL enabled)' do
+    describe "mailhost template content for imap" do


I think Rubocop will also flag this, but use single quotes if there are no interpolated variables.

wyardley · 2016-10-24T17:49:44Z

@wyardley The unification of the common parts are part of the initial PR. See https://github.com/voxpupuli/puppet-nginx/pull/769/files#diff-a9378fe4f575a8c57d256e37d135f6ccR38

This is also great, thank you for making that change. Overall, I think if we can get this with the rubocop tests passing and commits squashed, I think we can merge this after one other person doing a quick review.

However, it does look like there have been some merges vs. rebases in the branch, so may be easier to just do a diff, and apply it to a new branch. Let us know if you need help getting it squashed. I know it's possible to do a squash merge in, but generally, Voxpupuli requires that the commits be squashed prior to merging.

bbriggs · 2016-10-24T18:03:02Z

Looking much better. Can you please also address the complaints from Rubocop? IMO, it's easier to clean those up as you go.

wyardley · 2016-10-31T22:58:50Z

@dol: Do you think you'll have time to rework this a bit more... re-rebase, sqash commits, etc?

Fixing the rubocop warnings isn't too hard.

If not, let me know and I can give it a shot.

I like breaking the common template out, but I'm thinking we should maybe use concat rather than embedding the template in another template where possible? @bastelfreak: thoughts?

I have a sort-of in-progress change going here:
https://github.com/wyardley/puppet-nginx/tree/cyon-fix/ssl_settings_for_mail
Specifically, these are some changes I applied on top, though a couple tests are still failing.
https://github.com/wyardley/puppet-nginx/commit/4bb90905ec3bdd04bfef95810dea45807938886b

dol · 2016-11-07T00:03:53Z

@wyardley I found time to resolve the issues and performed some minor glitches that where introduced into the master during the last rebase.
About the single commit: I prepared an additional branch that contains all the changes in one commit. Squashing/rebasing was not easy possible. I'll push force the changes if you like. Normally the Github comments are lost when push forcing a branch.

dol · 2016-11-07T10:26:29Z

@wyardley 👍

wyardley · 2016-11-07T17:11:32Z

@dol Thanks so much for persisting with this and making all those changes. I think this will really improve things in that section of the module!

…puli#769) * Adds new SSL && protocol specific directives to mailhost setup The SSL setup customisation of a mailhost is very limited. To allow more directives to be set, the ssl configuration was extracted and arranged like the setup for a vhost. The setup was also expanded to customize protocol specific authentication method and capabilities. * Extended test cases to include newly introduced ssl and protocol specific options in mailhost setup * support raw_prepend, raw_append, mailhost_cfg_append, mailhost_cfg_prepend, in mailhost * Fixes rubocop issues * Render template without a new line * Fixes typo in mailhost.pp * Fix inlining after merge and dropping newlines when rendering a template

Dominic Luechinger and others added 3 commits February 22, 2016 16:34

Extended test cases to include newly introduced ssl and protocol spec…

0d623b8

…ific options in mailhost setup

support raw_prepend, raw_append, mailhost_cfg_append, mailhost_cfg_pr…

41a0425

…epend, in mailhost

3flex added the needs-rebase label Aug 30, 2016

This was referenced Oct 21, 2016

add auth_http_header #898

Merged

Use default ssl_protocols for ssl mailhosts #930

Merged

SSL cipher changes (issue 859) #909

Merged

dol mentioned this pull request Oct 24, 2016

listen_options and ipv6_listen_options doesn't have the same default value #874

Open

Merge branch 'master' into fix/ssl_settings_for_mail

965efba

wyardley removed the needs-rebase label Oct 24, 2016

wyardley reviewed Oct 24, 2016

View reviewed changes

wyardley added the needs-rebase label Nov 5, 2016

Dominic Luechinger added 3 commits November 6, 2016 23:56

Fixes rubocop issues

cc33c75

Render template without a new line

eef3d12

Fixes typo in mailhost.pp

492cb37

Dominic Luechinger added 2 commits November 7, 2016 00:14

Merge branch 'master' into fix/ssl_settings_for_mail

9683425

Fix inlining after merge and dropping newlines when rendering a template

f3e39f1

jyaworski merged commit 7a88a11 into voxpupuli:master Nov 7, 2016

dol deleted the fix/ssl_settings_for_mail branch November 7, 2016 10:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds new SSL && protocol specific directives to mailhost setup #769

Adds new SSL && protocol specific directives to mailhost setup #769

dol commented Feb 22, 2016

bastelfreak commented Oct 10, 2016

wyardley commented Oct 12, 2016

dol commented Oct 14, 2016

wyardley commented Oct 17, 2016

wyardley commented Oct 17, 2016

wyardley commented Oct 20, 2016

dol commented Oct 21, 2016

wyardley commented Oct 22, 2016

wyardley commented Oct 22, 2016

dol commented Oct 24, 2016

wyardley commented Oct 24, 2016

wyardley Oct 24, 2016

wyardley Oct 24, 2016

wyardley Oct 24, 2016

wyardley commented Oct 24, 2016

bbriggs commented Oct 24, 2016

wyardley commented Oct 31, 2016

dol commented Nov 7, 2016

dol commented Nov 7, 2016

wyardley commented Nov 7, 2016

Adds new SSL && protocol specific directives to mailhost setup #769

Adds new SSL && protocol specific directives to mailhost setup #769

Conversation

dol commented Feb 22, 2016

bastelfreak commented Oct 10, 2016

wyardley commented Oct 12, 2016

dol commented Oct 14, 2016

wyardley commented Oct 17, 2016

wyardley commented Oct 17, 2016

wyardley commented Oct 20, 2016

dol commented Oct 21, 2016

wyardley commented Oct 22, 2016

wyardley commented Oct 22, 2016

dol commented Oct 24, 2016

wyardley commented Oct 24, 2016

wyardley Oct 24, 2016

Choose a reason for hiding this comment

wyardley Oct 24, 2016

Choose a reason for hiding this comment

wyardley Oct 24, 2016

Choose a reason for hiding this comment

wyardley commented Oct 24, 2016

bbriggs commented Oct 24, 2016

wyardley commented Oct 31, 2016

dol commented Nov 7, 2016

dol commented Nov 7, 2016

wyardley commented Nov 7, 2016