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

apache::vhost ProxyPassMatch in Location containers #2222

Merged

Conversation

skylar2-uw
Copy link
Contributor

Apache httpd supports placing ProxyPass and ProxyPassMatch directives either at virtual host scope, or inside a Location/LocationMatch container. When placed inside a container, the path parameter is omitted and inherited from the container's path. For Location containers, this is a simple URI, while LocationMatch allows the path to be regex. While ProxyPass can go into a LocationMatch container, the inherited path is interpreted as a string rather than a regex, which limits its utility.

While the puppetlabs-apache module (via its apache::vhost type) supports both ProxyPass and ProxyPassMatch at virtual host scope, currently it only supports ProxyPass inside a Location/LocationMatch container, which means that it isn't possible to use the regex-aware LocationMatch container type. This pull request enhances the vhost/_directories.erb template to support the same proxy_pass_match dictionary at Location/LocationMatch scope that apache::vhost type supports at virtual host scope.

I couldn't find any documentation for the existing ProxyPass support in apache::vhost's directories array, so I added a short blurb to the apache::vhost Puppet Strings.

I added what I thought would be a decent simple test case, though I confess I'm not a rspec expert so it might bear special review.

@puppet-community-rangefinder
Copy link

apache::vhost is a type

Breaking changes to this file WILL impact these 128 modules (exact match):
Breaking changes to this file MAY impact these 35 modules (near match):

This module is declared in 174 of 579 indexed public Puppetfiles.


These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

@skylar2-uw skylar2-uw marked this pull request as ready for review March 24, 2022 21:37
@skylar2-uw skylar2-uw requested a review from a team as a code owner March 24, 2022 21:37
@skylar2-uw skylar2-uw changed the title Apache vhost proxy pass match directories apache::vhost ProxyPassMatch directories Mar 24, 2022
@skylar2-uw skylar2-uw changed the title apache::vhost ProxyPassMatch directories apache::vhost ProxyPassMatch in Location containers Mar 24, 2022
@david22swan
Copy link
Member

@skylar2-uw Hey, it look's like you've got some syntax error's that where on the main branch, these have been sorted now so if you rebase it should remove them.

skylar2-uw and others added 19 commits May 12, 2022 07:47
It already uses the apache24 package and service names. This aligns the
values.
This takes the approach of listing the disabling steps once rather than
repeating it for every mpm.

I wasn't quite sure on the prefork disabling. Does prefork need to stay
enabled for itk?
It's safe to assume the version is >= 2.2 so the conditional can be
dropped.
The latest passenger include a new option to load ruby bundles earlier,
working around deployment issues.  See:
* phusion/passenger#2410
* phusion/passenger#2409
CentOS Stream verion is just `8`, not `8.foo`.
In 7e233fe a plan was made to change
the naming. In 6b2a20a it was already
from 6.0.0 to 7.0.0. Now that 7.0.0 has been released, the default isn't
going to change. This time I'm suggestion to drop the plan altogether
and stop bothering users with this.
This adds support for the GssapiBasicAuth setting
which allows to fall back to basic auth if NEGOTIATE fails.
@CLAassistant
Copy link

CLAassistant commented May 12, 2022

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
4 out of 5 committers have signed the CLA.

✅ skylar2-uw
✅ smortex
✅ chelnak
✅ kenyon
❌ olifre
You have signed the CLA already but the status is still pending? Let us recheck it.

@skylar2-uw
Copy link
Contributor Author

skylar2-uw commented May 12, 2022

@david22swan Thanks for the heads-up and taking care of the fix, I've got our forked repo updated.

@david22swan
Copy link
Member

@skylar2-uw Hey, thx for getting the rebase done so fast, anyway look's like it has uncovered some failures in the spec test's.
Giving them a closer look they all look to be the same, you are getting errors in the spec/defines/vhost_spec.rb file from the new test that you have added.
Anyway, aside from this everything looks good so if you can get it sorted I'll be happy to go ahead and merge.

@skylar2-uw
Copy link
Contributor Author

@david22swan Sorry for the delay, was trying to get rspec working in our environment (we're a bit old on PE 2019 and CentOS 7). Unfortunately couldn't quite get it to work, but eyeballing the test case I think all it needs is proxy_pass to be changed to proxy_pass_match. I've pushed an update so hopefully that does the trick. Thanks for your patience!

Copy link
Member

@david22swan david22swan left a comment

Choose a reason for hiding this comment

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

@skylar2-uw Yeh that fixed it :)

Anyway, everything looks good to me so I'm gonna go ahead and merge.
Thank's for putting in the work :)

Copy link
Member

@david22swan david22swan left a comment

Choose a reason for hiding this comment

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

@skylar2-uw Yeh that fixed it :)

Anyway, everything looks good to me so I'm gonna go ahead and merge.
Thank's for putting in the work :)

@david22swan david22swan merged commit bff20f9 into puppetlabs:main May 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants