Rename rewrite_to_https => ssl_redirect (backwards-incompatible change) #957

wyardley · 2016-11-01T22:33:58Z

This PR has some backwards-incompatible changes:

It renames rewrite_to_https to ssl_redirect. I know this will be a pain for some, but the parameter is hard to find since 'ssl' and 'redirect' are not in the current parameter name. Also, we aren't using the 'rewrite' directive, so the current name is a bit misleading (see HTTP->HTTPS #818, for some discussion of this). I've also grouped it along with the other SSL / TLS related parameters.
It forces ssl_port and listen_port to be integers (there has already been a deprecation warning in place for a long time if they're set to strings)

Additionally, it:

Adds an option to override the redirect port with the new ssl_redirect_port parameter (open to suggestion on the exact naming of the parameters).
Suppresses the location block on the non-SSL vhost
Suppresses other default items that aren't needed (~~logs~~ and index files) when ssl_only is set

This also gets rid of the if ($ssl_protocol = "") { (which apparently is un-needed here (per comments by @law), and is also bad style), updated the wording slightly, sets the default to 'false' rather than undef, and adds a test case for when $ssl_port is alternate.

Does anyone know of any reason this if statement is needed here? I think in normal contexts, if SSL or TLS is enabled, the other template will be used anyway.

This is also reworking #820 (try to suppress some additional locations in the non-SSL vhost when just redirecting, accomplished by setting ssl_only), and updating the conditional logic in this section.

wyardley · 2016-11-02T05:06:28Z

I think this also handles #789 as mentioned above, though I coded it slightly differently.

I've added additional tests to cover various scenarios that weren't really covered before, though please review them carefully and make sure I'm doing them in a sane way.

It also now fails instead of giving a deprecation warning when listen_port and ssl_port are set as strings, rather than integers; there has been a warning on the books for quite a while, and making sure they're ints avoids having to do the $x + 0 trick for the comparison.

It should be possible to accomplish this transition in a softer / nicer way if we decide we want to... probably a lot of people are using this parameter.

I will run the acceptance tests and try to do some basic functional testing, but would love some additional testing if anyone's willing.

wyardley · 2016-11-03T04:22:48Z

@law noticed some redundant code in the template, and also mentioned that the logs can / should stay in by default; this all simplifies the code somewhat, and I re-pushed with those edits.

I don't totally love the re-use of "ssl_only" internally in config, as well as passing it on to location, but I think it should work (that is, I think we want to suppress the index directive when ssl_only is set in general).

hunner

Just minor things. 👍 for breaking changes that make things better.

hunner · 2016-11-03T17:28:24Z

manifests/resource/vhost.pp

@@ -214,6 +216,8 @@
  $ssl_client_cert              = undef,
  $ssl_verify_client            = 'on',
  $ssl_dhparam                  = undef,
+  $ssl_force_redirect           = false,


or ssl_redirect to match ssl_redirect_port

renamed as suggested.

hunner · 2016-11-03T17:31:26Z

manifests/resource/vhost.pp

+  # This deals with a situation where the listen directive for SSL doesn't match
+  # the port we want to force the SSL redirect to.
+  if ($ssl_redirect_port) {
+    $ssl_redirect_port_real = $ssl_redirect_port


Current practice is to use $_ssl_redirect_port instead of $ssl_redirect_port_real

invidian · 2016-11-03T19:06:40Z

manifests/resource/vhost.pp

@@ -214,6 +216,8 @@
  $ssl_client_cert              = undef,
  $ssl_verify_client            = 'on',
  $ssl_dhparam                  = undef,
+  $ssl_redirect           = false,


That would be nice to fix that indentation.

Thanks! I missed that and one other when I renamed the parameter. Fixed now.

zachfi · 2016-11-04T22:22:53Z

Seems like maybe a documentation update somewhere is in order.

wyardley · 2016-11-04T22:35:30Z

@xaque208 the parameter change itself is documented the way other params of the vhost resource are:
https://github.com/wyardley/puppet-nginx/blob/cc26a390a7aa5e1a1c3d83f5a881ca10be84f7db/manifests/resource/vhost.pp#L65-L68

As far as docs in general, yes, the current situation is pretty untenable, with examples needing to be updated, and the main classes not documented at all. And, an example using this param in README or examples would also be great, though one didn't exist before this change either.

There was an old PR that had some docs updates but never got merged; #941 is my attempt to get started on moving towards a 1.0 and adding a lot of the missing docs, hopefully with puppet-strings. Hopefully, I'll get a better idea of the exact format and conventions soon. Help with docs would definitely be a great way for people to help contribute!

zachfi · 2016-11-04T22:44:18Z

I'm happy to write docs. #872 is one very similar to an item I'm struggling to get working using this module. A fastcgi application proxied to php-fpm. I have the raw nginx config that works, but have yet to be able to replicate that in the module. For docs @wyardley, there are lots of things in flight right now, so I'd only be wanting to write docs and give examples for items that were relatively stable. Also, not sure where the majority of the conversation about upcoming work is happening, but some idea about where folks want to take this module would help flesh out examples, etc. Happy to chat in IRC too.

…ncompatible change), take out needless "if" statement before return. Also forces integer for $ssl_port and $listen_port, which previously only generated a warning.

wyardley · 2016-11-05T05:25:20Z

@xaque208 Yeah, IRC (#voxpupuli) or the Puppet Community Slack in #voxpupuli is where most of the discussion happens; there are also community module triage online meetings (on Blue Jeans) every week on Thursdays.

I think with these community supported modules, it can be kind of hard as there isn't necessarily one consistent vision of how things should be. But we are trying to move towards a 1.0, which means mostly improving the docs, and trying to get the structure and interface of the module a little more in line with current best practices.

Even though the module itself may get some more structural changes, I don't know of any plans to make more big changes to the interface to the module beyond the one we just merged.

Even though the module has been around for a while, and has been somewhat widely used, because it's still pre 1.0, and because it's community supported, we're able to be a bit looser in terms of making changes like this without leaving in deprecation warnings etc. This can be frustrating as a user at times, but it does help make some of these improvements happen at a quicker pace and leaving less cruft.

zachfi · 2016-11-07T17:31:52Z

This all sounds good. Users can stick to older versions of the module if they require stability. Looking forward to 1.0!

wyardley · 2016-11-07T18:26:28Z

I think the requested changes have been made; can someone give me a final review / merge on this, assuming it looks good?

Rename rewrite_to_https => ssl_redirect (backwards-incompatible change)

wyardley mentioned this pull request Nov 2, 2016

Don't generate http locations if rewrite_to_https #820

Closed

hunner reviewed Nov 3, 2016

View reviewed changes

invidian approved these changes Nov 3, 2016

View reviewed changes

invidian mentioned this pull request Nov 3, 2016

Add rewrite_to_https_port setting support #789

Closed

wyardley changed the title ~~Rename rewrite_to_https => ssl_force_redirect (backwards-incompatible change)~~ Rename rewrite_to_https => ssl_redirect (backwards-incompatible change) Nov 4, 2016

Rename rewrite_to_https => ssl_force_redirect (breaking / backwards-i…

857bc07

…ncompatible change), take out needless "if" statement before return. Also forces integer for $ssl_port and $listen_port, which previously only generated a warning.

bastelfreak merged commit bcb17f2 into voxpupuli:master Nov 7, 2016

wyardley added the backwards-incompatible label Nov 21, 2016

cegeka-jenkins pushed a commit to cegeka/puppet-nginx that referenced this pull request Sep 13, 2019

Merge pull request voxpupuli#957 from wyardley/rework_ssl_redirects

3cc9287

Rename rewrite_to_https => ssl_redirect (backwards-incompatible change)

Rubueno pushed a commit to Rubueno/puppet-nginx that referenced this pull request Oct 19, 2020

Merge pull request voxpupuli#957 from wyardley/rework_ssl_redirects

9bf365f

Rename rewrite_to_https => ssl_redirect (backwards-incompatible change)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename rewrite_to_https => ssl_redirect (backwards-incompatible change) #957

Rename rewrite_to_https => ssl_redirect (backwards-incompatible change) #957

wyardley commented Nov 1, 2016 •

edited

Loading

wyardley commented Nov 2, 2016 •

edited

Loading

wyardley commented Nov 3, 2016 •

edited

Loading

hunner left a comment

hunner Nov 3, 2016

wyardley Nov 3, 2016

hunner Nov 3, 2016

wyardley Nov 3, 2016

invidian Nov 3, 2016 •

edited

Loading

wyardley Nov 4, 2016

zachfi commented Nov 4, 2016

wyardley commented Nov 4, 2016

zachfi commented Nov 4, 2016

wyardley commented Nov 5, 2016

zachfi commented Nov 7, 2016

wyardley commented Nov 7, 2016

Rename rewrite_to_https => ssl_redirect (backwards-incompatible change) #957

Rename rewrite_to_https => ssl_redirect (backwards-incompatible change) #957

Conversation

wyardley commented Nov 1, 2016 • edited Loading

wyardley commented Nov 2, 2016 • edited Loading

wyardley commented Nov 3, 2016 • edited Loading

hunner left a comment

Choose a reason for hiding this comment

hunner Nov 3, 2016

Choose a reason for hiding this comment

wyardley Nov 3, 2016

Choose a reason for hiding this comment

hunner Nov 3, 2016

Choose a reason for hiding this comment

wyardley Nov 3, 2016

Choose a reason for hiding this comment

invidian Nov 3, 2016 • edited Loading

Choose a reason for hiding this comment

wyardley Nov 4, 2016

Choose a reason for hiding this comment

zachfi commented Nov 4, 2016

wyardley commented Nov 4, 2016

zachfi commented Nov 4, 2016

wyardley commented Nov 5, 2016

zachfi commented Nov 7, 2016

wyardley commented Nov 7, 2016

wyardley commented Nov 1, 2016 •

edited

Loading

wyardley commented Nov 2, 2016 •

edited

Loading

wyardley commented Nov 3, 2016 •

edited

Loading

invidian Nov 3, 2016 •

edited

Loading