Add full Apache management #105

ekohl · 2020-06-26T13:08:24Z

The goal of this PR is to keep all Apache configuration within the module. There should be no Apache config fragments at all in puppet-foreman_proxy_content. There are parameters to attach fragments to an existing vhost. This makes it easy to switch between the co-located implementation (Foreman is the vhost) or standalone.

Currently this is very much a draft. Tests will fail and the proxy setup is not configured when attaching. It may also need parameters for HTTPS certificates.

manifests/apache.pp

manifests/plugin/file.pp

manifests/plugin/container.pp

ehelms · 2020-06-29T12:54:57Z

Something you may want to consider as part of this re-design is the ability to deploy a content app and its Apache configurations without deploying the API app.

ekohl · 2020-06-29T14:33:42Z

Yes, I agree. But then you also want to be able to deploy a content app without workers. By that point you're really blowing up the size of the PR.

ehelms · 2020-06-29T17:01:04Z

This is already a large change so I don't mind it covering that use case :) If you want to plan to come back and layer that change on top of this design I'm OK with that too.

ekohl · 2020-07-02T09:16:01Z

Something you may want to consider as part of this re-design is the ability to deploy a content app and its Apache configurations without deploying the API app.

What's the goal of deploying one without the API? Shared filesystem with shared database to scale out? I suspect we may also need to support a shared Redis deployment then so I'd prefer to tackle it later. We can write acceptance tests with multiple servers so I'd hope to really set a new level of verification in this module. That does require some research on my side though.

ehelms · 2020-07-02T12:29:03Z

Correct. With the Pulp 3 architecture, it's possible to deploy just a content app connected to the database of a main Pulp or mirror, as well as the storage (either shared filesystem or S3) for a lighter weight scale out. For that architecture, Redis is only needed by the main Pulp where the API app and workers are.

manifests/apache.pp

ekohl · 2020-07-02T14:49:19Z

I've split off #108 as a preparation PR which will reduce the size of this PR. That can already be merged.

I'm starting to feel fairly confident about this but no tests have been done on attaching to another vhost (which is currently the primary deployment). Hence it's still a draft.

ekohl · 2020-07-03T08:56:51Z

Looks like EL8 doesn't generate a certificate by default where EL7 does so which is why they fail.

manifests/apache.pp

ehelms · 2020-07-06T19:30:04Z

manifests/apache.pp

+        ssl         => true,
+        priority    => $vhost_priority,
+        docroot     => $pulpcore::webserver_static_dir,
+        directories => $base_directories + $api_directories,


I think these also need to be passed to the pulpcore::apache::fragment use as well so that authentication is configured through remote user environ.

ekohl · 2020-07-16T16:59:40Z

So a small update. I started with better integration tests:

Introduce a curl_command helper #110 makes doing curl commands easier
A client certificate is generated that can auth
The API is verified as authenticated and non-authenticated to verify that works

Now that this appears to mostly work, I'm going to continue with testing deployment as part of the Foreman vhost.

manifests/apache.pp

ehelms · 2020-07-17T12:09:31Z

manifests/plugin.pp

  }
+
+  if $http_content or $https_content {
+    # TODO: prio below main


I wouldn't know what to do with this TODO. Could you expand on the details?

I think this is where I need to solve the issue you saw in fpc where the content was added after the main reverse proxy so they ended up not being proxied.

spec/acceptance/full_spec.rb

ekohl · 2020-10-10T15:15:26Z

I've started doing testing and this appears to work well now. I'm doing a final EL7 pipeline to verify it end to end. EL8 is untested, but I noticed that on EL8's Apache we need to do more work. https://pulp-certguard.readthedocs.io/en/latest/reverse_proxy_config.html#apache-2-4-10-config-example states we need to encode it.

ehelms · 2020-10-10T16:56:17Z

templates/apache-fragment.epp

@@ -0,0 +1,53 @@
+<%- |


This is painful to read and parse as a developer. Is there empty lines we could add to separate blocks more? Or processing that can happen inside the puppet class rather than in the template? I think, if we are going to go this route of trying to have a generic fragment that handles all cases, rather than having easy to read, specific templates we need to make the template easier to read and accept that the rendered version may not look like you would exactly design an Apache config file indentation and grouping of parameters wise.

I should have mentioned this, but it's a trimmed down version of https://github.com/puppetlabs/puppetlabs-apache/blob/main/templates/vhost/_directories.erb with strict input checking. I could probably avoid it altogether and use template('apache/vhost/_directories.erb') but I wasn't quite sure about scoping.

I also wasn't quite sure about best practices. Room for improvement but I liked the strict typing checks.

I'll probably submit this upstream in puppetlabs-apache, but in the short term I don't think we can rely on that.

Pulpcore has a default for this (which is copied). That means it's a good idea to always set it. Otherwise the illusion is created that it's an optional thing. However, the reverse proxy must be set up to use this. If the header is not cleared, it's a security risk because an attack can spoof the header.

ekohl · 2020-10-12T16:46:47Z

I think the tests should now be green, but Travis is backed up. After that, I think this is pretty much ready to be merged.

ehelms · 2020-10-12T17:37:19Z

templates/apache-fragment.epp

+  } else {
+    $provider = $directory['provider'].capitalize
+  }
+-%>


This is hard to parse in this in this syntax. I think it could benefit from:

<%- $directories.each |$directory| { -%> <%- if $directory['provider'] =~ /^(.*)match$/ { $provider = $1.capitalize + 'Match' } else { $provider = $directory['provider'].capitalize } -%>

ehelms · 2020-10-12T17:37:40Z

templates/apache-fragment.epp

+  }]] $directories,
+  Optional[Array[Hash[String, String]]] $proxy_pass = undef,
+| -%>
+<%-


Could this be:

| -%> <%-

ehelms · 2020-10-12T18:05:22Z

I had green install pipeline with this change. I just have questions regarding the EPP template formatting

ekohl · 2020-10-13T12:05:59Z

How about this? I kept it as a separate commit to make the diff easier to read, but will squash if you agree.

ehelms · 2020-10-13T12:33:49Z

Yes, that helps a lot, thank you! I can squash on merge once the tests run.

ehelms · 2020-10-13T17:14:13Z

Trying to kick Travis

The goal of this is that the module can either manage the vhost itself or attach fragments to another vhost to embed the application. This allows composition.

theforeman-bot added Needs testing Not yet reviewed labels Jun 26, 2020

ekohl force-pushed the apache-management branch 2 times, most recently from a40f72f to 18b1e6d Compare June 26, 2020 14:40

ekohl mentioned this pull request Jun 26, 2020

Rely on pulpcore to set up Apache fragments theforeman/puppet-foreman_proxy_content#268

Merged

ekohl added the Backwards incompatible label Jun 26, 2020

ekohl force-pushed the apache-management branch from 18b1e6d to 70928fa Compare June 26, 2020 20:57

ehelms reviewed Jun 29, 2020

View reviewed changes

manifests/apache.pp Outdated Show resolved Hide resolved

ehelms reviewed Jun 29, 2020

View reviewed changes

manifests/plugin/file.pp Outdated Show resolved Hide resolved

ehelms reviewed Jun 29, 2020

View reviewed changes

manifests/plugin/container.pp Show resolved Hide resolved

ehelms reviewed Jul 2, 2020

View reviewed changes

manifests/apache.pp Outdated Show resolved Hide resolved

ekohl force-pushed the apache-management branch from 70928fa to a849298 Compare July 2, 2020 14:44

ekohl force-pushed the apache-management branch from a849298 to 7417e55 Compare July 2, 2020 15:09

ehelms reviewed Jul 6, 2020

View reviewed changes

manifests/apache.pp Show resolved Hide resolved

ehelms reviewed Jul 6, 2020

View reviewed changes

ekohl force-pushed the apache-management branch from 7417e55 to 01b2979 Compare July 7, 2020 09:54

ekohl force-pushed the apache-management branch 2 times, most recently from 715e43c to acdc904 Compare July 16, 2020 16:52

ehelms reviewed Jul 17, 2020

View reviewed changes

manifests/apache.pp Show resolved Hide resolved

ehelms reviewed Jul 17, 2020

View reviewed changes

spec/acceptance/full_spec.rb Outdated Show resolved Hide resolved

ekohl force-pushed the apache-management branch from acdc904 to d8d409d Compare July 17, 2020 16:40

ekohl force-pushed the apache-management branch 2 times, most recently from 3acef9e to 0d401e5 Compare October 10, 2020 14:51

ekohl marked this pull request as ready for review October 10, 2020 15:12

ehelms reviewed Oct 10, 2020

View reviewed changes

ekohl added 2 commits October 12, 2020 17:27

Get full resource test coverage

c17efb4

ekohl force-pushed the apache-management branch from ff3d7b3 to 032e713 Compare October 12, 2020 15:29

ehelms reviewed Oct 12, 2020

View reviewed changes

ehelms added Waiting on contributor and removed Needs testing Not yet reviewed labels Oct 13, 2020

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Oct 13, 2020

ehelms closed this Oct 13, 2020

ehelms reopened this Oct 13, 2020

Add HTTP / HTTPS vhost management

259d8e1

The goal of this is that the module can either manage the vhost itself or attach fragments to another vhost to embed the application. This allows composition.

ekohl force-pushed the apache-management branch from 42d8124 to 259d8e1 Compare October 13, 2020 19:11

ehelms approved these changes Oct 13, 2020

View reviewed changes

theforeman-bot removed the Needs re-review label Oct 13, 2020

ehelms merged commit a60df53 into theforeman:master Oct 13, 2020

ekohl deleted the apache-management branch October 18, 2020 13:31

evgeni mentioned this pull request Sep 3, 2024

Correct container plugin apache header rewrite #356

Merged

Add full Apache management #105

Add full Apache management #105

Uh oh!

Conversation

ekohl commented Jun 26, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ehelms commented Jun 29, 2020

Uh oh!

ekohl commented Jun 29, 2020

Uh oh!

ehelms commented Jun 29, 2020

Uh oh!

ekohl commented Jul 2, 2020

Uh oh!

ehelms commented Jul 2, 2020

Uh oh!

Uh oh!

ekohl commented Jul 2, 2020

Uh oh!

ekohl commented Jul 3, 2020

Uh oh!

Uh oh!

ehelms Jul 6, 2020

Choose a reason for hiding this comment

Uh oh!

ekohl commented Jul 16, 2020

Uh oh!

Uh oh!

ehelms Jul 17, 2020

Choose a reason for hiding this comment

Uh oh!

ekohl Jul 17, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ekohl commented Oct 10, 2020

Uh oh!

ehelms Oct 10, 2020

Choose a reason for hiding this comment

Uh oh!

ekohl Oct 10, 2020

Choose a reason for hiding this comment

Uh oh!

ekohl commented Oct 12, 2020

Uh oh!

ehelms Oct 12, 2020

Choose a reason for hiding this comment

Uh oh!

ehelms Oct 12, 2020

Choose a reason for hiding this comment

Uh oh!

ehelms commented Oct 12, 2020

Uh oh!

ekohl commented Oct 13, 2020

Uh oh!

ehelms commented Oct 13, 2020

Uh oh!

ehelms commented Oct 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants